-
Notifications
You must be signed in to change notification settings - Fork 520
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
[fpmsyncd] updates for MPLS #1794
Conversation
@@ -897,6 +1004,123 @@ bool RouteSync::getIfName(int if_index, char *if_name, size_t name_len) | |||
return true; | |||
} | |||
|
|||
#ifndef LIBNL3_ENCAP_MPLS |
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.
lets remove this and update azure pipeline to fetch libnl3 debian package from sonic-buildimaage
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.
@smaheshm I am not familiar with the azure pipeline infrastructure and will need input with exact changes required to fetch libnl3 debian packages from sonic-buildimage.
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.
@smaheshm I have submitted a new PR sonic-net/sonic-buildimage#8064 that is the only other change required to make this PR work in the Azure pipeline as it is currently.
07b2b94
to
b643761
Compare
.azure-pipelines/build-template.yml
Outdated
@@ -21,6 +21,9 @@ parameters: | |||
- name: sonic_slave | |||
type: string | |||
|
|||
- name: buildimage_artifact_name |
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.
The value for this variable must be passed from "azure-pipelines.yml", I don't see that change in the PR.
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.
@smaheshm Did you look at the pipeline details for this? Doesn't even get to the point where the .azure-pipelines stuff is needed. lgtm.yml is run first and fails... so this issue you have pointed out is irrelevant for now.
a6b6583
to
4a2617b
Compare
lgtm.yml
Outdated
- sudo dpkg -i libnl-route-3-dev_3.5.0-1_amd64.deb | ||
- sudo dpkg -i libnl-nf-3-dev_3.5.0-1_amd64.deb | ||
- popd | ||
- git clone https://github.com/qbdwlr/sonic-swss-common; pushd sonic-swss-common; git checkout azp; ./autogen.sh; fakeroot dpkg-buildpackage -us -uc -b; popd |
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 using a personal repo?
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.
I guess @qbdwlr was trying out if this change will fix the LGTM analysis. Looks like it didn't.
979d608
to
06d3387
Compare
/azp run |
Azure Pipelines failed to run 1 pipeline(s). |
/* Get the index of the master device */ | ||
uint32_t master_index = rtnl_route_get_table(route_obj); | ||
/* if the table_id is not set in the route obj then route is for default vrf. */ | ||
if (master_index) |
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.
How did you find out non-zero value implies a VRF table. "254" is the main table that ip route installs, 255 and 253 are local and default route table. All these are non-VRF tables.
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.
@smahesh You do know that SONiC has modified the fpm netlink message content from frr to something non-standard, right? The rtnl_route_get_table() function will not return the RT_TABLE value in SONiC fpmsyncd, it returns the VRF ifindex value instead because SONiC has patched this.
https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-frr/patch/0003-Use-vrf_id-for-vrf-not-tabled_id.patch
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.
thanks for the info, no I wasn't aware.
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.
@smaheshm Yes, this is the reason I have to disable VRF support in the other PR (#1765) where I provide the fpmsyncd runtime option to listen to kernel netlink instead of frr fpm. kernel netlink messages send the standard RT_TABLE ID in the netlink message, whereas frr fpm substitutes the patched VRF ifindex value.
c7a1cc2
to
c978f74
Compare
ddf91d7
to
66d7cf2
Compare
122ae51
to
a626695
Compare
@prsunny - would you please help review? thanks. |
@qbdwlr - please close this if this PR is not to be used anymore. |
closing in favor of #1871 |
…nic-net#1794) #### What I did Implemented [JSON Patch Ordering using YANG Models Design Doc](https://github.com/Azure/SONiC/blob/master/doc/config-generic-update-rollback/Json_Patch_Ordering_using_YANG_Models_Design.md) #### How to verify it Unit-Tests **NOTE: The code in this PR was [reverted](github.com/Azure/sonic-utilities/commit/0a145e8027380e8d4decb36bdfc647062c722612) before because of some [build issues](sonic-net/sonic-utilities#1761). Build issues have been fixed [here](sonic-net/sonic-buildimage#8632). To check the original PR comments please go [here](sonic-net/sonic-utilities#1599
What I did
SONiC swss fpmsyncd support for MPLS:
Why I did it
SONiC swss fpmsyncd support for MPLS
How I verified it
Unit-tests in sonic-swss/tests/test_mpls.py
System tests in sonic-mgmt
Details if related
Build dependency on sonic-net/sonic-buildimage#8064
Refer to HLD: sonic-net/SONiC#706