-
Notifications
You must be signed in to change notification settings - Fork 545
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
[orchagent]: VXLAN: Fix oper_status and tunnel encapsulation TTL #3383
base: master
Are you sure you want to change the base?
Conversation
603980f
to
75c4f03
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
@VladimirKuk any idea if those test failures are actually a symptom of the patch itself? Or is it just common for things to fail in the tests from time to time? |
Tests do fail from time to time. |
@prsunny please review |
Thank you for pushing this forward! |
@prsunny ping |
1ad6c1e
to
89b365a
Compare
@VladimirKuk I ended up having to sprinkle your suggestion in 2 places to get it fully working. |
@@ -275,7 +275,7 @@ create_tunnel( | |||
sai_ip_address_t *dst_ip, | |||
sai_object_id_t underlay_rif, | |||
bool p2p, | |||
sai_uint8_t encap_ttl=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removed? IIRC, it is done for a purpose to skip adding this attribute for those scenarios where the underlying implementation doesn't support this setting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is a static function that only exists in that file and is never called without the encap_ttl argument. The public functions both set the default value and are optional. I can of course set to a proper default value there if you'd prefer, its just never used so I didn't see a point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternatively we can just go back to default of 0 everywhere to explicitly mean "choose something for me", then check for 0 in this and set it to 64. Some old revisions did that but @VladimirKuk suggested to change it. I personally don't care either way, both ways definitively fix the issue at hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prsunny how to move forward on this?
@prsunny how would you like to proceed on this? I think this is a critical issue since community SONiC doesn't support ARP/ND suppression so things just don't work at all without this. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@prsunny looks like the internal msconflict is resolved now since pvst was merged to main. Please review and hopefully merge. Thanks! |
a000ef4
to
6a9f067
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
rebased on top of current master to get rid of merged stuff that is now upstream |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
I can't tell from the output if somehow the test failures are related to this PR. I'm not familiar enough with the test case output to differentiate expected failures from real ones. |
This fixes 2 issues across a range of open tickets building upon patches created by others with modifications as requested by @VladimirKuk. The first issue this resolves is the status shown for remote vteps which in the fact that it is wrong makes debugging nearly impossible: ``` +------------+------------+-------------------+--------------+ | SIP | DIP | Creation Source | OperStatus | +============+============+===================+==============+ | 172.16.0.1 | 172.16.0.2 | EVPN | oper_down | +------------+------------+-------------------+--------------+ Total count : 1 ``` The VTEP is really up. Original PR for that is sonic-net#2080. Also fixes sonic-net/sonic-buildimage#10004 or at least the error message which hurts debugging. The next issue is in reachabiity across VXLANs. This fixes IP/MAC learning via ARP. The original PR for that is sonic-net#3216, however it appears it has its origins in sonic-net/sonic-buildimage#10050 which goes into greater detail about the issue itself. Also there is talk about it here kamelnetworks/sonic#9 as well as another similar patch here: kamelnetworks@02ee3e3 Fixes sonic-net#3216 Fixes sonic-net#2080 Fixes sonic-net/sonic-buildimage#10050 Fixes sonic-net/sonic-buildimage#10004 Signed-off-by: Brad House (@bradh352)
66e36ea
to
86d1383
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
…onic-net#3383) This fixes 2 issues across a range of open tickets building upon patches created by others with modifications as requested by @VladimirKuk. The first issue this resolves is the status shown for remote vteps which in the fact that it is wrong makes debugging nearly impossible: ``` +------------+------------+-------------------+--------------+ | SIP | DIP | Creation Source | OperStatus | +============+============+===================+==============+ | 172.16.0.1 | 172.16.0.2 | EVPN | oper_down | +------------+------------+-------------------+--------------+ Total count : 1 ``` The VTEP is really up. Original PR for that is sonic-net#2080. Also fixes sonic-net/sonic-buildimage#10004 or at least the error message which hurts debugging. The next issue is in reachabiity across VXLANs. This fixes IP/MAC learning via ARP. The original PR for that is sonic-net#3216, however it appears it has its origins in sonic-net/sonic-buildimage#10050 which goes into greater detail about the issue itself. Also there is talk about it here kamelnetworks/sonic#9 as well as another similar patch here: kamelnetworks@02ee3e3 Fixes sonic-net#3216 Fixes sonic-net#2080 Fixes sonic-net/sonic-buildimage#10050 Fixes sonic-net/sonic-buildimage#10004 Signed-off-by: Brad House (@bradh352)
…onic-net#3383) This fixes 2 issues across a range of open tickets building upon patches created by others with modifications as requested by @VladimirKuk. The first issue this resolves is the status shown for remote vteps which in the fact that it is wrong makes debugging nearly impossible: ``` +------------+------------+-------------------+--------------+ | SIP | DIP | Creation Source | OperStatus | +============+============+===================+==============+ | 172.16.0.1 | 172.16.0.2 | EVPN | oper_down | +------------+------------+-------------------+--------------+ Total count : 1 ``` The VTEP is really up. Original PR for that is sonic-net#2080. Also fixes sonic-net/sonic-buildimage#10004 or at least the error message which hurts debugging. The next issue is in reachabiity across VXLANs. This fixes IP/MAC learning via ARP. The original PR for that is sonic-net#3216, however it appears it has its origins in sonic-net/sonic-buildimage#10050 which goes into greater detail about the issue itself. Also there is talk about it here kamelnetworks/sonic#9 as well as another similar patch here: kamelnetworks@02ee3e3 Fixes sonic-net#3216 Fixes sonic-net#2080 Fixes sonic-net/sonic-buildimage#10050 Fixes sonic-net/sonic-buildimage#10004 Signed-off-by: Brad House (@bradh352)
…onic-net#3383) This fixes 2 issues across a range of open tickets building upon patches created by others with modifications as requested by @VladimirKuk. The first issue this resolves is the status shown for remote vteps which in the fact that it is wrong makes debugging nearly impossible: ``` +------------+------------+-------------------+--------------+ | SIP | DIP | Creation Source | OperStatus | +============+============+===================+==============+ | 172.16.0.1 | 172.16.0.2 | EVPN | oper_down | +------------+------------+-------------------+--------------+ Total count : 1 ``` The VTEP is really up. Original PR for that is sonic-net#2080. Also fixes sonic-net/sonic-buildimage#10004 or at least the error message which hurts debugging. The next issue is in reachabiity across VXLANs. This fixes IP/MAC learning via ARP. The original PR for that is sonic-net#3216, however it appears it has its origins in sonic-net/sonic-buildimage#10050 which goes into greater detail about the issue itself. Also there is talk about it here kamelnetworks/sonic#9 as well as another similar patch here: kamelnetworks@02ee3e3 Fixes sonic-net#3216 Fixes sonic-net#2080 Fixes sonic-net/sonic-buildimage#10050 Fixes sonic-net/sonic-buildimage#10004 Signed-off-by: Brad House (@bradh352)
…onic-net#3383) This fixes 2 issues across a range of open tickets building upon patches created by others with modifications as requested by @VladimirKuk. The first issue this resolves is the status shown for remote vteps which in the fact that it is wrong makes debugging nearly impossible: ``` +------------+------------+-------------------+--------------+ | SIP | DIP | Creation Source | OperStatus | +============+============+===================+==============+ | 172.16.0.1 | 172.16.0.2 | EVPN | oper_down | +------------+------------+-------------------+--------------+ Total count : 1 ``` The VTEP is really up. Original PR for that is sonic-net#2080. Also fixes sonic-net/sonic-buildimage#10004 or at least the error message which hurts debugging. The next issue is in reachabiity across VXLANs. This fixes IP/MAC learning via ARP. The original PR for that is sonic-net#3216, however it appears it has its origins in sonic-net/sonic-buildimage#10050 which goes into greater detail about the issue itself. Also there is talk about it here kamelnetworks/sonic#9 as well as another similar patch here: kamelnetworks@02ee3e3 Fixes sonic-net#3216 Fixes sonic-net#2080 Fixes sonic-net/sonic-buildimage#10050 Fixes sonic-net/sonic-buildimage#10004 Signed-off-by: Brad House (@bradh352)
…onic-net#3383) This fixes 2 issues across a range of open tickets building upon patches created by others with modifications as requested by @VladimirKuk. The first issue this resolves is the status shown for remote vteps which in the fact that it is wrong makes debugging nearly impossible: ``` +------------+------------+-------------------+--------------+ | SIP | DIP | Creation Source | OperStatus | +============+============+===================+==============+ | 172.16.0.1 | 172.16.0.2 | EVPN | oper_down | +------------+------------+-------------------+--------------+ Total count : 1 ``` The VTEP is really up. Original PR for that is sonic-net#2080. Also fixes sonic-net/sonic-buildimage#10004 or at least the error message which hurts debugging. The next issue is in reachabiity across VXLANs. This fixes IP/MAC learning via ARP. The original PR for that is sonic-net#3216, however it appears it has its origins in sonic-net/sonic-buildimage#10050 which goes into greater detail about the issue itself. Also there is talk about it here kamelnetworks/sonic#9 as well as another similar patch here: kamelnetworks@02ee3e3 Fixes sonic-net#3216 Fixes sonic-net#2080 Fixes sonic-net/sonic-buildimage#10050 Fixes sonic-net/sonic-buildimage#10004 Signed-off-by: Brad House (@bradh352)
…onic-net#3383) This fixes 2 issues across a range of open tickets building upon patches created by others with modifications as requested by @VladimirKuk. The first issue this resolves is the status shown for remote vteps which in the fact that it is wrong makes debugging nearly impossible: ``` +------------+------------+-------------------+--------------+ | SIP | DIP | Creation Source | OperStatus | +============+============+===================+==============+ | 172.16.0.1 | 172.16.0.2 | EVPN | oper_down | +------------+------------+-------------------+--------------+ Total count : 1 ``` The VTEP is really up. Original PR for that is sonic-net#2080. Also fixes sonic-net/sonic-buildimage#10004 or at least the error message which hurts debugging. The next issue is in reachabiity across VXLANs. This fixes IP/MAC learning via ARP. The original PR for that is sonic-net#3216, however it appears it has its origins in sonic-net/sonic-buildimage#10050 which goes into greater detail about the issue itself. Also there is talk about it here kamelnetworks/sonic#9 as well as another similar patch here: kamelnetworks@02ee3e3 Fixes sonic-net#3216 Fixes sonic-net#2080 Fixes sonic-net/sonic-buildimage#10050 Fixes sonic-net/sonic-buildimage#10004 Signed-off-by: Brad House (@bradh352)
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
…onic-net#3383) This fixes 2 issues across a range of open tickets building upon patches created by others with modifications as requested by @VladimirKuk. The first issue this resolves is the status shown for remote vteps which in the fact that it is wrong makes debugging nearly impossible: ``` +------------+------------+-------------------+--------------+ | SIP | DIP | Creation Source | OperStatus | +============+============+===================+==============+ | 172.16.0.1 | 172.16.0.2 | EVPN | oper_down | +------------+------------+-------------------+--------------+ Total count : 1 ``` The VTEP is really up. Original PR for that is sonic-net#2080. Also fixes sonic-net/sonic-buildimage#10004 or at least the error message which hurts debugging. The next issue is in reachabiity across VXLANs. This fixes IP/MAC learning via ARP. The original PR for that is sonic-net#3216, however it appears it has its origins in sonic-net/sonic-buildimage#10050 which goes into greater detail about the issue itself. Also there is talk about it here kamelnetworks/sonic#9 as well as another similar patch here: kamelnetworks@02ee3e3 Fixes sonic-net#3216 Fixes sonic-net#2080 Fixes sonic-net/sonic-buildimage#10050 Fixes sonic-net/sonic-buildimage#10004 Signed-off-by: Brad House (@bradh352)
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@prsunny figured out the test case failure and corrected it, it was due to specifically checking tunnel attributes and we added 2 new ones. |
What I did
This fixes 2 issues across a range of open tickets building upon patches created by others with modifications as requested by @VladimirKuk.
The first issue this resolves is the status shown for remote vteps which in the fact that it is wrong makes debugging nearly impossible:
The remote VTEP is really up.
Original PR for that is #2080.
Also fixes sonic-net/sonic-buildimage#10004 or at least the error message which hurts debugging.
The next issue is in reachabiity across VXLANs. This fixes IP/MAC learning via ARP. The original PR for that is #3216, however it appears it has its origins in
sonic-net/sonic-buildimage#10050 which goes into greater detail about the issue itself. Also there is talk about it here kamelnetworks/sonic#9 as well as another similar patch here: kamelnetworks@02ee3e3
Why I did it
Fixes #3216
Fixes #2080
Fixes sonic-net/sonic-buildimage#10050
Fixes sonic-net/sonic-buildimage#10004
How I verified it
Pulled into my private sonic-swss fork:
https://github.com/bradh352/sonic-swss/commits/bradh352/master
Which is pulled in by my private sonic-buildimage fork:
https://github.com/bradh352/sonic-buildimage/tree/bradh352/master
Which is then automatically built when changes are made. Then the uploaded asset of sonic-broadcom.bin is installed onto Dell S5248F and N3248TE switches and tested.
Details if related
Signed-off-by: Brad House (@bradh352)
This should also be backported to 202405