Minigraph ECMP parsing support (cleaner format) #4985

kktheballer · 2020-07-17T01:11:25Z

- Why I did it
To support FG_ECMP for certain azd scenarios
- How I did it
Modified minigraph parser to parse ECMP fields in the case they are present in minigraph
- How to verify it
Loaded ensuing config_db file on a DUT to verify the fields are parsed and configure device correctly
- Description for the changelog

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

lguohan · 2020-07-21T21:57:25Z

need to add sonic-cfggen tests, please

kktheballer · 2020-08-03T18:38:27Z

retest mellanox please.

kktheballer · 2020-08-03T18:39:34Z

retest mellanox please

kktheballer · 2020-08-05T18:45:11Z

retest mellanox please

kktheballer · 2020-08-10T23:30:08Z

retest mellanox please

kktheballer · 2020-08-13T00:24:10Z

retest mellanox please

kktheballer · 2020-08-14T19:44:43Z

retest mellanox please

anish-n · 2020-08-22T01:10:59Z

src/sonic-config-engine/minigraph.py

+    LCM = sum(lcm_array)
+    FG_NHG_MEMBER[0] = {ip: {"FG_NHG": ipv4_tag, "Bank": bank} for ip, bank in nhipv4_bank_map.items()}
+    FG_NHG_PREFIX[0] = {nhgaddr[0]: {"FG_NHG": ipv4_tag}}
+    FG_NHG[0] = {ipv4_tag: {"hash_bucket_size": LCM}}


Change "hash_bucket_size" to "bucket_size" per schema

anish-n · 2020-08-22T17:48:19Z

src/sonic-config-engine/minigraph.py

+            lcm_comp = lcm_comp * i / gcd(lcm_comp, i)
+        lcm_array.append(lcm_comp)
+    LCM = sum(lcm_array)
+    FG_NHG_MEMBER[0] = {ip: {"FG_NHG": ipv4_tag, "Bank": bank} for ip, bank in nhipv4_bank_map.items()}


Change "Bank" to "bank"

kktheballer · 2020-09-02T21:37:26Z

retests vsimage please

kktheballer · 2020-09-08T16:23:14Z

retest vsimage please

anish-n · 2020-09-08T17:28:26Z

src/sonic-config-engine/minigraph.py

+                    endport = link.find(str(QName(ns, "EndPort"))).text
+                    startdevice = link.find(str(QName(ns, "StartDevice"))).text
+                    port_device_map[endport] = startdevice
+


Suggest moving this logic to line 211, and utilizing the endport and startdevice values already generated on line 194 and 195.

I believe I had written like this to work around the already in-place lines of code in 190-191

anish-n · 2020-09-08T17:38:59Z

src/sonic-config-engine/minigraph.py

+        dpg_ecmp_content = []
+        ipnhs = child.find(str(QName(ns, "IPNextHops")))
+        if ipnhs is not None:
+            for ipnh in ipnhs.findall(str(QName(ns, "IPNextHop"))):


Lets add a check for IPNextHop type FineGrainedECMPGroupMember

anish-n · 2020-09-08T17:42:29Z

src/sonic-config-engine/minigraph.py

+                    a = ipaddress.IPAddress(unicode(subnet_check_ip))
+                    n = ipaddress.IPNetwork(unicode(subnet_range))
+                    if (n.Contains(a)):
+                        nhgvlan = ip_intfs_map[subnet_range]


I suggest that we rename the variable name, not necessary that it is a vlan

i can use nhg_int if that is better

anish-n · 2020-09-08T17:58:43Z

src/sonic-config-engine/minigraph.py

+                        nhgvlan = ip_intfs_map[subnet_range]
+            dwnstrms = child.find(str(QName(ns, "DownstreamSummarySet")))
+            for dwnstrm in dwnstrms.findall(str(QName(ns, "DownstreamSummary"))):
+                dwnstrmentry = str(ET.tostring(dwnstrm))


Why don't we use the QName based search/parse in the below section?

For that one specific section, very oddly, there were errors trying to do so. I will look into it again

anish-n · 2020-09-10T00:53:47Z

src/sonic-config-engine/minigraph.py

+    port_nhipv4_map = dpg_ecmp_content[0]
+    port_nhipv6_map = dpg_ecmp_content[1]
+    nhgaddr = dpg_ecmp_content[2]
+    nhgvlan = dpg_ecmp_content[3]


change variable name since its not necessary that it is a vlan

anish-n · 2020-09-10T01:22:30Z

src/sonic-config-engine/minigraph.py

+    nhdevices_bank_map = {device: bank for bank, device in enumerate(nhdevices)}
+    nhipv4_bank_map = {ip: nhdevices_bank_map[device] for ip, device in nhipv4_device_map.items()}
+    nhipv6_bank_map = {ip: nhdevices_bank_map[device] for ip, device in nhipv6_device_map.items()}
+    for value in nhdevices_bank_map.values():


IPv4 and IPv6 could have a different enumeration of banks and as a result a different lcm. I would also suggest moving this calculation of lcm to a helper function.

ok will do this

kktheballer · 2020-10-21T17:08:14Z

retest vsimage please

lgtm-com · 2020-11-17T20:32:38Z

This pull request introduces 1 alert and fixes 10 when merging bfb5236488f4f5049f3c61650dc9a2c9b4874e10 into b595a6e - view on LGTM.com

new alerts:

1 for Syntax error

fixed alerts:

5 for Unused import
3 for Unused local variable
1 for Testing equality to None
1 for Except block handles 'BaseException'

kktheballer · 2020-11-17T21:07:47Z

retest mellanox please

anish-n · 2020-11-20T18:07:51Z

src/sonic-config-engine/minigraph.py

+                    dwnstrmentry = str(ET.tostring(dwnstrm))
+                    if ("FineGrainedECMPGroupDestination" in dwnstrmentry):
+                        subnet_ip = dwnstrm.find(str(QName(ns1, "Subnet"))).text
+                        truncsubnet_ip = subnet_ip.split("/")[0]


Why do we need to truncate it here for the IPv4/IPv6 check?

anish-n · 2020-11-20T18:17:38Z

src/sonic-config-engine/minigraph.py

+    port_nhipv4_map = dpg_ecmp_content[0]
+    port_nhipv6_map = dpg_ecmp_content[1]
+    nhgaddr = dpg_ecmp_content[2]
+    nhg_int = dpg_ecmp_content[3]


nit: readability consideration, I would recommend making dpg_ecmp_content a dictionary instead of an array.

addressed for dpg_ecmp_content as well as png_ecmp_content

kktheballer · 2020-11-29T18:39:18Z

retest mellanox please

kktheballer · 2020-12-10T01:39:49Z

retest mellanox please

anish-n · 2020-12-11T03:07:48Z

src/sonic-config-engine/minigraph.py

+                        a = ipaddress.ip_address(UNICODE_TYPE(subnet_check_ip))
+                        n = list(ipaddress.ip_network(UNICODE_TYPE(subnet_range), False).hosts())
+                        if a in n:
+                            nhg_int = ip_intfs_map[subnet_range]


Assumes a single interface for all IPs, though it may be addressed in a future PR: it is better to not to assume single interface associated with all IPs.

src/sonic-config-engine/minigraph.py

src/sonic-config-engine/tests/test_cfggen.py

abdosi

Please see the comments and update it accordingly.

abdosi

LGTM

kktheballer · 2020-12-23T23:58:53Z

retest vsimage please

kktheballer · 2020-12-27T22:37:27Z

retest vsimage please

abdosi

LGTM

anish-n · 2020-12-30T22:01:33Z

src/sonic-config-engine/tests/test_cfggen.py

+        output = self.run_script(argument)
+        self.assertEqual(output.strip(), "['200.200.200.1', '200.200.200.10', '200.200.200.2', '200.200.200.3', '200.200.200.4', '200.200.200.5',"
+                                         " '200.200.200.6', '200.200.200.7', '200.200.200.8', '200.200.200.9', '200:200:200:200::1', '200:200:200:200::10',"
+                                         " '200:200:200:200::2', '200:200:200:200::3', '200:200:200:200::4', '200:200:200:200::5', '200:200:200:200::6',"


In a future PR can we address adding checks for the values being populated in these entries(FG_NHG, link, and bank), having only the keys tested prevents 100% test coverage.

anish-n

looks good to me, I provided a comment on test improvement, I would recommend that for a follow up PR.

kktheballer force-pushed the ecmp_parsing branch from 1288c68 to 29a69cc Compare July 28, 2020 21:50

kktheballer force-pushed the ecmp_parsing branch from 29a69cc to 48a323d Compare August 5, 2020 21:10

kktheballer force-pushed the ecmp_parsing branch 2 times, most recently from 318355b to cb2c833 Compare August 11, 2020 21:04

prsunny requested review from anish-n and abdosi August 18, 2020 17:34

anish-n reviewed Aug 22, 2020

View reviewed changes

kktheballer force-pushed the ecmp_parsing branch from efb0339 to 1dab9c7 Compare August 26, 2020 22:15

anish-n reviewed Sep 10, 2020

View reviewed changes

kktheballer force-pushed the ecmp_parsing branch from 1dab9c7 to 13beebc Compare September 15, 2020 18:06

kktheballer force-pushed the ecmp_parsing branch from 65c39a4 to bfb5236 Compare November 17, 2020 20:17

anish-n reviewed Nov 20, 2020

View reviewed changes

kktheballer force-pushed the ecmp_parsing branch from 806610c to cf74368 Compare November 29, 2020 23:12

anish-n reviewed Dec 11, 2020

View reviewed changes

anish-n previously approved these changes Dec 11, 2020

View reviewed changes

abdosi reviewed Dec 15, 2020

View reviewed changes

src/sonic-config-engine/minigraph.py Show resolved Hide resolved

abdosi reviewed Dec 15, 2020

View reviewed changes

src/sonic-config-engine/minigraph.py Outdated Show resolved Hide resolved

abdosi reviewed Dec 15, 2020

View reviewed changes

src/sonic-config-engine/minigraph.py Outdated Show resolved Hide resolved

abdosi reviewed Dec 15, 2020

View reviewed changes

src/sonic-config-engine/minigraph.py Outdated Show resolved Hide resolved

abdosi reviewed Dec 15, 2020

View reviewed changes

src/sonic-config-engine/minigraph.py Show resolved Hide resolved

abdosi reviewed Dec 15, 2020

View reviewed changes

src/sonic-config-engine/tests/test_cfggen.py Show resolved Hide resolved

abdosi requested changes Dec 15, 2020

View reviewed changes

kktheballer dismissed anish-n’s stale review via f62c63b December 17, 2020 17:32

kktheballer force-pushed the ecmp_parsing branch 3 times, most recently from bf3e563 to da29af4 Compare December 23, 2020 00:49

kktheballer requested a review from abdosi December 23, 2020 17:27

kktheballer force-pushed the ecmp_parsing branch from f7678b3 to d90543b Compare December 23, 2020 19:06

abdosi previously approved these changes Dec 23, 2020

View reviewed changes

kktheballer dismissed abdosi’s stale review via 3615e69 December 23, 2020 20:00

Minigraph ECMP parsing support (cleaner format)

940dd94

kktheballer force-pushed the ecmp_parsing branch from 3615e69 to 940dd94 Compare December 23, 2020 20:09

Adding revised test cases and changing test minigraph.xml content

13787ee

lguohan force-pushed the master branch from 3690c1a to 512eb6b Compare December 25, 2020 18:34

kktheballer requested review from abdosi and anish-n December 28, 2020 18:27

abdosi approved these changes Dec 28, 2020

View reviewed changes

anish-n reviewed Dec 30, 2020

View reviewed changes

anish-n approved these changes Dec 30, 2020

View reviewed changes

abdosi merged commit ba92a08 into sonic-net:master Dec 30, 2020

Minigraph ECMP parsing support (cleaner format) #4985

Minigraph ECMP parsing support (cleaner format) #4985

Conversation

kktheballer commented Jul 17, 2020

lguohan commented Jul 21, 2020

kktheballer commented Aug 3, 2020

kktheballer commented Aug 3, 2020

kktheballer commented Aug 5, 2020

kktheballer commented Aug 10, 2020

kktheballer commented Aug 13, 2020

kktheballer commented Aug 14, 2020

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

kktheballer commented Sep 2, 2020

kktheballer commented Sep 8, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kktheballer commented Oct 21, 2020

lgtm-com bot commented Nov 17, 2020

kktheballer commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kktheballer commented Nov 29, 2020

kktheballer commented Dec 10, 2020

Choose a reason for hiding this comment

abdosi left a comment

Choose a reason for hiding this comment

abdosi left a comment

Choose a reason for hiding this comment

kktheballer commented Dec 23, 2020

kktheballer commented Dec 27, 2020

abdosi left a comment

Choose a reason for hiding this comment

anish-n Dec 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anish-n left a comment

Choose a reason for hiding this comment

anish-n Dec 30, 2020 •

edited

Loading