Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[fpmsyncd] don't manipulate route weight #2320

Merged
merged 1 commit into from
Jun 10, 2022
Merged

[fpmsyncd] don't manipulate route weight #2320

merged 1 commit into from
Jun 10, 2022

Conversation

yxieca
Copy link
Contributor

@yxieca yxieca commented Jun 9, 2022 •
edited
Loading

What I did
Return the next hop weight obtained from kernel as-is.

Sample next hop weight:

admin@svcstr-7050-acs-4:~$ ip route show 193.11.248.128/25
193.11.248.128/25 nhid 1452 proto bgp src 10.1.0.33 metric 20
nexthop via 10.0.1.61 dev PortChannel103 weight 1
nexthop via 10.0.1.63 dev PortChannel104 weight 1
nexthop via 10.0.1.59 dev PortChannel102 weight 1
nexthop via 10.0.1.57 dev PortChannel101 weight 1

Why I did it
Some platforms have issue dealing with arbitrary next hop weight values.

Signed-off-by: Ying Xie [email protected]

@yxieca yxieca requested a review from prsunny as a code owner June 9, 2022 20:13
@lguohan
Copy link
Contributor

lguohan commented Jun 9, 2022

can you add kernel show ip route sample here?

@prsunny prsunny mentioned this pull request Jun 9, 2022
Closed
@lguohan
Copy link
Contributor

lguohan commented Jun 9, 2022 •
edited
Loading

checked more on the frr code. it looks like when zebra send the weight to the kernel it minus -1, FRRouting/frr@df7fb58 .

however, for the weight read from fpm netlink, there is no such minus -1. https://github.com/Azure/sonic-frr/blob/frr/8.2/zebra/zebra_fpm_netlink.c#L486

@caizhenghui-juniper
Copy link
Contributor

I can't revert my approval, but here is the reason that we do the logic of +1 when there is weight assigned.
In FRR code(zebra) the netlink message sent to kernel is different than sending to fpm. The weight sent to kernel is the configured weight -1, where that logic is not in the message to FPM. I believe we need to take kernel value as root of truth, thus in fpmsyncd we compensate that with +1 logic in order to retrieve the desired weight setting from routing stack. The best solution is to patch SONiC version of FRR so that the weight value sent to FPM agrees the one sent to kernel.

@hasan-brcm
Copy link

I see kernel and frr are in sync:

root@sonic:/home/admin# ip route show 10.10.10.10/32
10.10.10.10 nhid 151 proto bgp src 5.5.5.5 metric 20
        nexthop via 30.0.0.2 dev Ethernet4 weight 1
        nexthop via 30.1.0.2 dev Ethernet12 weight 1
root@sonic:/home/admin# vtysh -c"show ip route 10.10.10.10/32"
Routing entry for 10.10.10.10/32
  Known via "bgp", distance 20, metric 0, best
  Last update 02:18:16 ago
  * 30.0.0.2, via Ethernet4, weight 1
  * 30.1.0.2, via Ethernet12, weight 1

root@sonic:/home/admin# sonic-db-cli APPL_DB "hgetall ROUTE_TABLE:10.10.10.10"
nexthop
30.0.0.2,30.1.0.2
ifname
Ethernet4,Ethernet12
weight
2,2
root@sonic:/home/admin#

@lguohan
Copy link
Contributor

lguohan commented Jun 9, 2022

@hasan-brcm, check here, looks like ip route show tool compensate this kernel behavior.

https://github.com/shemminger/iproute2/blob/main/ip/ipnexthop.c#L242

@yxieca yxieca requested a review from lguohan June 9, 2022 21:54
@yxieca yxieca mentioned this pull request Jun 9, 2022
Merged
@hasan-brcm
Copy link

@hasan-brcm, check here, looks like ip route show tool compensate this kernel behavior.

https://github.com/shemminger/iproute2/blob/main/ip/ipnexthop.c#L242

Thanks @lguohan for the reference. Yes, it confirms that user space weight is always +1 of what is in the kernel. The fpmsyncd should increment the weight only if it is receiving route updates from kernel (as what iproute2 does.) Given that fpmsyncd receives updates from frr (instead of kernel) in the default build image, it shouldn't increment the weight.

In order to take care of such scenarios, there should be awareness of where the updates are coming from and handle it accordingly.

@lguohan
Copy link
Contributor

lguohan commented Jun 9, 2022

@hasan-brcm, my worry is that current fpm behavior is an oversight from frr, they may change current behavior to align with kernel.

yxieca added a commit that referenced this pull request Jun 9, 2022
@yxieca
[fpmsyncd] don't manipulate route weight (#2321)
77043fb 
What I did
This is a cherry-pick PR for #2320.

Why I did it
We don't necessarily think it is the right solution, but before we can make Zebra behavior consistent. This change will unblock SONiC tests.

It will be a temporary solution until we converge with FRR/Zebra changes.

Signed-off-by: Ying Xie <[email protected]>
@lguohan lguohan merged commit b12af41 into sonic-net:master Jun 10, 2022
@lguohan
Copy link
Contributor

lguohan commented Jun 10, 2022 •
edited
Loading

opened frr issue on this. let's track that, if frr decides to sync the fpmlink and netlink behavior, we will need to adjust. FRRouting/frr#11384

@yxieca yxieca deleted the weight branch June 10, 2022 01:54
@caizhenghui-juniper
Copy link
Contributor

if weight =1 means ECMP without weight, the logic should be
if (weight > 1) ....
To prevent weight being set in app DB in case SAI implementations from vendors don't support it

1 similar comment
@caizhenghui-juniper
Copy link
Contributor

if weight =1 means ECMP without weight, the logic should be
if (weight > 1) ....
To prevent weight being set in app DB in case SAI implementations from vendors don't support it

@lguohan
Copy link
Contributor

lguohan commented Jun 14, 2022

if weight =1 means ECMP without weight, the logic should be

is this some kind of convention?

@caizhenghui-juniper
Copy link
Contributor

caizhenghui-juniper commented Jun 14, 2022 via email

@lguohan
Copy link
Contributor

lguohan commented Jun 17, 2022

frr decides to keep current behavior as it is.

FRRouting/frr#11384

@caizhenghui-juniper
Copy link
Contributor

caizhenghui-juniper commented Jun 17, 2022 via email

preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022
@yxieca @preetham-singh
[fpmsyncd] don't manipulate route weight (sonic-net#2320)
02b3cf2 
Return the next hop weight obtained from kernel as-is.

Sample next hop weight:

admin@svcstr-7050-acs-4:~$ ip route show 193.11.248.128/25
193.11.248.128/25 nhid 1452 proto bgp src 10.1.0.33 metric 20
nexthop via 10.0.1.61 dev PortChannel103 weight 1
nexthop via 10.0.1.63 dev PortChannel104 weight 1
nexthop via 10.0.1.59 dev PortChannel102 weight 1
nexthop via 10.0.1.57 dev PortChannel101 weight 1

Signed-off-by: Ying Xie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caizhenghui-juniper caizhenghui-juniper caizhenghui-juniper approved these changes

@prsunny prsunny Awaiting requested review from prsunny prsunny is a code owner

@lguohan lguohan Awaiting requested review from lguohan

Assignees
No one assigned
Labels
Included in 202205 Branch Request for 202205 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yxieca @lguohan @caizhenghui-juniper @hasan-brcm