libnl3 updates for MPLS #7195

qbdwlr · 2021-03-31T01:56:47Z

What I did
SONiC buildimage support for MPLS:

New accessors in libnl3 for MPLS attributes
bug fixes in libnl3 for MPLS attribute parsing

Why I did it
SONiC libnl3 support for MPLS

How I verified it
Unit-tests in sonic-swss/tests/test_mpls.py and sonic-utilities/tests
System tests in sonic-mgmt

Details if related
Refer to HLD: sonic-net/SONiC#706
PR to upstream to libnl3 project: thom311/libnl#284

lguohan · 2021-03-31T07:58:12Z

src/libnl3/patch/0001-parse-multipath-infinite-loop-fix.patch

@@ -0,0 +1,13 @@
+diff --git a/lib/route/route_obj.c b/lib/route/route_obj.c


are they upstreamed? can you format the patch properly, author, description, bla, bla

These changes are not upstreamed. Please provide an example of how patch files should be formatted.

The patch files have been reformatted as requested. A PR has been created to upstream libnl3 changes, but there have been no PRs merged in that project in 2021, so these patches remain necessary.

lguohan · 2021-06-03T17:56:16Z

src/libnl3/patch/0002-mpls-remove-nl_addr_valid.patch

+From c89d1a129f71d3d2f76e6cbadb11ef41d8941a73 Mon Sep 17 00:00:00 2001
+From: Ann Pokora <apokora@juniper.net>
+Date: Tue, 25 May 2021 18:10:04 -0700
+Subject: [PATCH 2/2] mpls remove nl_addr_valid


why are we remove the validation?

@lguohan nl_addr_valid() arguments are supposed to be a pointer to an ASCII string and an address family value (AF_MPLS, AF_INET, AF_INET6)... it then uses these to validate that the string format contains an address for the specified family.
The removed calls to nl_addr_valid() were instead passing in a pointer to a binary address and the length of the binary address... this is completely wrong. For most cases, the length of the binary address would not match any of the values of supported address families, so nl_addr_valid() would return true. But if the binary address was an MPLS label stack with 3 labels, then the length would match the value of AF_DECnet (12) and it attempts to parse the binary address as if it were an ASCII string... this fails and nl_addr_valid() returns false.

can you put the above explaination into the this patch file?

@lguohan explanation is added to patch file.

qbdwlr · 2021-06-07T17:30:55Z

@lguohan @prsunny Please review/approve this sonic-buildimage PR as the associated sonic-swss PR has build dependencies on the libnl3 changes.

smaheshm · 2021-06-08T17:12:09Z

src/libnl3/patch/0002-mpls-remove-nl_addr_valid.patch

+ {
+ 	struct nl_addr *old = nh->rtnh_newdst;
+
+-	if (!nl_addr_valid(nl_addr_get_binary_addr(addr),


Instead of removing the validation, can we fix the arguments.

replace nl_addr_get_len(addr) with nl_addr_get_family(addr)

@smahesh That still isn't valid. Look at the code for nl_addr_valid() in libnl3. It is looking for an ASCII string and validating the string format. addr does not container a string, The return from nl_addr_get_binary_addr() is not a string.
We would have to first convert addr to string. It's pointless. nl_addr_valid() is not used anywhere else in libnl3, so I do not know why someone decided to use it in these MPLS functions.

nl_addr_valid() is using the route_nh_set which is not mpls specific. this is the IP functions. how can this function pass now?

@lguohan RTA_NEWDST is specific to MPLS. NEWDST contains the MPLS labelstack associated with the NH.

lguohan · 2021-06-10T23:17:41Z

src/sonic-yang-models/yang-models/sonic-interface.yang

+				leaf mpls {
+					type string {
+						pattern "enable|disable";
+					}


need description here, what does enable/disable mean here.

@lguohan description is added.

lguohan · 2021-06-14T15:51:33Z

src/libnl3/patch/0002-mpls-remove-nl_addr_valid.patch

+Date: Tue, 25 May 2021 18:10:04 -0700
+Subject: [PATCH 2/2] mpls remove nl_addr_valid
+
+The removed calls to nl_addr_valid() are passing in a pointer to a binary address and the length of the binary address, which does not match expected arguments for nl_addr_valid(). nl_addr_valid() expects a pointer to an ASCII string and the address family of the string format.  The incorrect arguments cause unexpected failures and the expected arguments are not available in the context.


can you make it more readable, make it multi-line, limit by 80 char per line?

lguohan · 2021-06-14T15:53:40Z

i am only review the libnl part, for the yang model part, it should be reviewed in the sonic yang subgroup meeting. @rlhui,
is this properly communicated to the team?

qbdwlr · 2021-06-14T18:19:11Z

/azp run

azure-pipelines · 2021-06-14T18:19:16Z

Commenter does not have sufficient privileges for PR 7195 in repo Azure/sonic-buildimage

prsunny · 2021-06-14T21:04:36Z

/azp run

azure-pipelines · 2021-06-14T21:04:54Z

Azure Pipelines successfully started running 1 pipeline(s).

rlhui · 2021-06-15T15:34:44Z

i am only review the libnl part, for the yang model part, it should be reviewed in the sonic yang subgroup meeting. @rlhui,
is this properly communicated to the team?

I was not aware this PR contains yang changes until last week. @qbdwlr could email the yang subgroup sonic-yang-subgroup@googlegroups.com and @anshuv-mfst to request for review asap.

qbdwlr · 2021-06-15T19:05:30Z

@rlhui @lguohan Changes in sonic-yang-models have been pulled from this PR and placed in a separate PR specifically for sonic-yang-models: #7881

smaheshm · 2021-06-15T19:22:53Z

Looks good to me.. waiting for builds to pass.

qbdwlr · 2021-06-16T12:31:10Z

@lguohan @smaheshm Please review/approve this PR. The sonic-swss PR (sonic-net/sonic-swss#1686) will not build successfully until this PR is merged to master.

smaheshm

👍

smaheshm · 2021-06-16T22:05:45Z

For posterity can you add the link to libnl3 upstream patch in the description section. Thanks!

lguohan reviewed Mar 31, 2021

View reviewed changes

qbdwlr mentioned this pull request Mar 31, 2021

MPLS for SONiC infrastructure HLD sonic-net/SONiC#706

Merged

qbdwlr force-pushed the mplsPR branch 2 times, most recently from 3f4c34d to a0c399c Compare April 15, 2021 13:47

qbdwlr force-pushed the mplsPR branch from a0c399c to 8c7cfd0 Compare April 21, 2021 00:46

qbdwlr force-pushed the mplsPR branch from 8c7cfd0 to 61b4286 Compare May 11, 2021 00:36

qbdwlr force-pushed the mplsPR branch 2 times, most recently from 8d820c9 to 1de29ea Compare May 19, 2021 15:02

lguohan reviewed Jun 3, 2021

View reviewed changes

qbdwlr marked this pull request as ready for review June 3, 2021 23:34

qbdwlr force-pushed the mplsPR branch 2 times, most recently from 36f7338 to ce5205f Compare June 7, 2021 03:03

qbdwlr requested a review from lguohan June 7, 2021 17:29

qbdwlr closed this Jun 7, 2021

qbdwlr force-pushed the mplsPR branch from ce5205f to 73447ef Compare June 7, 2021 19:33

qbdwlr reopened this Jun 7, 2021

qbdwlr mentioned this pull request Jun 8, 2021

sonic-swss changes for MPLS sonic-net/sonic-swss#1686

Merged

smaheshm reviewed Jun 8, 2021

View reviewed changes

qbdwlr force-pushed the mplsPR branch from c806bbc to 8a72a55 Compare June 10, 2021 23:00

lguohan reviewed Jun 10, 2021

View reviewed changes

qbdwlr force-pushed the mplsPR branch 4 times, most recently from ab18f4d to 90d5aa8 Compare June 14, 2021 15:45

lguohan reviewed Jun 14, 2021

View reviewed changes

qbdwlr force-pushed the mplsPR branch 2 times, most recently from e6cd1ae to 2fc8eb8 Compare June 14, 2021 17:03

qbdwlr closed this Jun 15, 2021

qbdwlr force-pushed the mplsPR branch from 2fc8eb8 to 4d07bbb Compare June 15, 2021 18:39

sonic-buildimage updates for MPLS

38294e2

qbdwlr reopened this Jun 15, 2021

qbdwlr requested review from lguohan and smaheshm June 15, 2021 19:05

qbdwlr changed the title ~~sonic-buildimage changes for MPLS~~ libnl3 changes for MPLS Jun 15, 2021

qbdwlr changed the title ~~libnl3 changes for MPLS~~ libnl3 updates for MPLS Jun 15, 2021

smaheshm approved these changes Jun 16, 2021

View reviewed changes

lguohan approved these changes Jun 16, 2021

View reviewed changes

smaheshm merged commit 3d62923 into sonic-net:master Jun 16, 2021

vaibhavhd mentioned this pull request Jun 21, 2021

[crm] handleSetCommand fails to parse CRM config: Unknown attribute mpls_inseg_high_threshold #7933

Open

alexrallen mentioned this pull request Jun 23, 2021

[DPB] Port Breakout Fails with "sonic_yang(3):All Keys are not parsed in CRM" #7956

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libnl3 updates for MPLS #7195

libnl3 updates for MPLS #7195

qbdwlr commented Mar 31, 2021 •

edited

Loading

lguohan Mar 31, 2021

qbdwlr Mar 31, 2021

qbdwlr May 26, 2021

lguohan Jun 3, 2021

qbdwlr Jun 3, 2021

lguohan Jun 10, 2021

qbdwlr Jun 11, 2021

qbdwlr commented Jun 7, 2021

smaheshm Jun 8, 2021

qbdwlr Jun 8, 2021

lguohan Jun 10, 2021

qbdwlr Jun 11, 2021

lguohan Jun 10, 2021

qbdwlr Jun 11, 2021

lguohan Jun 14, 2021

lguohan commented Jun 14, 2021

qbdwlr commented Jun 14, 2021

azure-pipelines bot commented Jun 14, 2021

prsunny commented Jun 14, 2021

azure-pipelines bot commented Jun 14, 2021

rlhui commented Jun 15, 2021

qbdwlr commented Jun 15, 2021

smaheshm commented Jun 15, 2021

qbdwlr commented Jun 16, 2021

smaheshm left a comment

smaheshm commented Jun 16, 2021

		@@ -0,0 +1,13 @@
		diff --git a/lib/route/route_obj.c b/lib/route/route_obj.c

libnl3 updates for MPLS #7195

libnl3 updates for MPLS #7195

Conversation

qbdwlr commented Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qbdwlr commented Jun 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lguohan commented Jun 14, 2021

qbdwlr commented Jun 14, 2021

azure-pipelines bot commented Jun 14, 2021

prsunny commented Jun 14, 2021

azure-pipelines bot commented Jun 14, 2021

rlhui commented Jun 15, 2021

qbdwlr commented Jun 15, 2021

smaheshm commented Jun 15, 2021

qbdwlr commented Jun 16, 2021

smaheshm left a comment

Choose a reason for hiding this comment

smaheshm commented Jun 16, 2021

qbdwlr commented Mar 31, 2021 •

edited

Loading