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.

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

[FRR]: Port some patches from sonic-quagga repo #3017

Merged
merged 11 commits into from
Jun 23, 2019

Conversation

pavel-shirshov
Copy link
Contributor

@pavel-shirshov pavel-shirshov commented Jun 15, 2019

Please don't merge it

- What I did
Port 3 patches from sonic-quagga.

- How I did it
Manually

- How to verify it
Build it and run on your dut
- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@lguohan
Copy link
Collaborator

lguohan commented Jun 15, 2019

can you uncomment the invalid_nexthop vs test to make the patch is working?

Copy link
Collaborator

@lguohan lguohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uncomment invalid nexthop vs test please.

@pavel-shirshov
Copy link
Contributor Author

@lguohan Sure. Already did that.

@lguohan
Copy link
Collaborator

lguohan commented Jun 17, 2019

@nikos-github
Copy link
Collaborator

nikos-github commented Jun 18, 2019

The nexthop patch is not correct. I have pushed a different version of it a while back into frr latest and I also have diffs to push for the others. The log change needs to be under a cli. Best to hold on to these for a bit until I push into frr the rest of the code. I'm fine after that to push into 7.0 latest and sync to that or commit into sonic-frr but please wait and lets not create patches.

Copy link
Collaborator

@nikos-github nikos-github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment above.

@pavel-shirshov
Copy link
Contributor Author

@nikos-github Thank you for your comment. I've ported your patch from the master to frr-7.
What do you mean by "The log change needs to be under a cli."?
Thanks

@pavel-shirshov
Copy link
Contributor Author

retest this please

@pavel-shirshov
Copy link
Contributor Author

retest vs please

@nikos-github
Copy link
Collaborator

@nikos-github Thank you for your comment. I've ported your patch from the master to frr-7.
What do you mean by "The log change needs to be under a cli."?
Thanks

The change from info to debug in vty log needs to go under a cli cmd option. I have that already.

Also the other patch that Guohan pointed me in quagga regarding the dscp, is also not correct and I had pointed that out to him and I have fixed in my diffs.

@pavel-shirshov
Copy link
Contributor Author

@nikos-github
Where can I find your patches?
Thanks


/* If NEXT_HOP is present, validate it. */
if (attr->flag & ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP)) {
- if (attr->nexthop.s_addr == 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What failure are you seeing with the presence of the previous patch above that's forcing you to make this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@nikos-github nikos-github Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavel-shirshov @lguohan This isn't the right way to address the case of an IPv6 NLRI being sent and a nexthop attribute of 0. The flag needs to be either cleared or the martian check needs to happen earlier for the relevant AFI/SAFIs. The diffs are just punching a hole in the code but the nexthop attribute flag still remains set. So for AFI/SAFI 1/1 with IPv6 nexthop and also the nexthop attribute present, this is still a problem. Same goes for any AFI/SAFI that is sent along with the 0 nexthop attribute - we will be skipping a valid check.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikos-github Currently, when we allow bgpd to have 0.0.0.0 as next-hop ip address, bgpd tries to resolve the next-hop using zebra, zebra responds with the message "unresolved" and this next-hop is not used.
Also, why do you consider that this check was valid? I tried to find in the RFC why NEXT_HOP attribute can't be 0.0.0.0 and I didn't find anything. Probably I miss something. Can you please point me to the place in the RFC which says us we should filter-out next_hops equal to 0.0.0.0?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulnice There is no such thing as nexthop of 0.0.0.0 for a prefix learned via a neighbor. Nexthops for prefixes received by peers, correspond to an interface address. 0.0.0.0 is not a valid interface address. It's all described in the RFC.

@lguohan
Copy link
Collaborator

lguohan commented Jun 22, 2019

retest vs please

1 similar comment
@lguohan
Copy link
Collaborator

lguohan commented Jun 23, 2019

retest vs please

@lguohan
Copy link
Collaborator

lguohan commented Jun 23, 2019

@pavel-shirshov , tests are still failing.

@pavel-shirshov pavel-shirshov merged commit dd0f005 into sonic-net:master Jun 23, 2019
@pavel-shirshov pavel-shirshov deleted the pavelsh/frr_patches branch June 23, 2019 22:26
@imzyxwvu
Copy link

Hello, could someone tell me what vendor bug is 0004-Allow-BGP-attr-NEXT_HOP-to-be-0.0.0.0-due-to-allevia.patch trying to alleviate. Is there any branded switch sending route update with the Next Hop property value 0.0.0.0?

@nikos-github
Copy link
Collaborator

Arista.

mssonicbld added a commit that referenced this pull request Feb 7, 2024
…lly (#18051)

#### Why I did it
src/sonic-swss
```
* b18cbac6 - (HEAD -> master, origin/master, origin/HEAD) [Ci] Fix the test script naming issue (#3021) (81 minutes ago) [xumia]
* 5fd896f6 - [PortOrch] Add FEC codeword errors in port stats (#3029) (87 minutes ago) [vdahiya12]
* 77d56e6e - Fix the Orchagent crash seen during Port channel OC test cases. (#3042) (9 hours ago) [saksarav-nokia]
* 4d470592 - Fix memory leak and object copying bugs in orchagent (#3017) (10 hours ago) [Saikrishna Arcot]
```
#### How I did it
#### How to verify it
#### Description for the changelog
yxieca pushed a commit that referenced this pull request Feb 21, 2024
…lly (#18141)

src/sonic-swss

* d322f660 - (HEAD -> 202311, origin/202311) Fix memory leak and object copying bugs in orchagent (#3017) (4 hours ago) [Saikrishna Arcot]
sonic-otn pushed a commit to Weitang-Zheng/sonic-buildimage that referenced this pull request Mar 11, 2024
…lly (sonic-net#18051)

#### Why I did it
src/sonic-swss
```
* b18cbac6 - (HEAD -> master, origin/master, origin/HEAD) [Ci] Fix the test script naming issue (sonic-net#3021) (81 minutes ago) [xumia]
* 5fd896f6 - [PortOrch] Add FEC codeword errors in port stats (sonic-net#3029) (87 minutes ago) [vdahiya12]
* 77d56e6e - Fix the Orchagent crash seen during Port channel OC test cases. (sonic-net#3042) (9 hours ago) [saksarav-nokia]
* 4d470592 - Fix memory leak and object copying bugs in orchagent (sonic-net#3017) (10 hours ago) [Saikrishna Arcot]
```
#### How I did it
#### How to verify it
#### Description for the changelog
saksarav-nokia pushed a commit to saksarav-nokia/sonic-buildimage that referenced this pull request Mar 12, 2024
…lly (sonic-net#18051)

#### Why I did it
src/sonic-swss
```
* b18cbac6 - (HEAD -> master, origin/master, origin/HEAD) [Ci] Fix the test script naming issue (sonic-net#3021) (81 minutes ago) [xumia]
* 5fd896f6 - [PortOrch] Add FEC codeword errors in port stats (sonic-net#3029) (87 minutes ago) [vdahiya12]
* 77d56e6e - Fix the Orchagent crash seen during Port channel OC test cases. (sonic-net#3042) (9 hours ago) [saksarav-nokia]
* 4d470592 - Fix memory leak and object copying bugs in orchagent (sonic-net#3017) (10 hours ago) [Saikrishna Arcot]
```
#### How I did it
#### How to verify it
#### Description for the changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants