Add ECMP calculator tool #12482

liorghub · 2022-10-25T03:47:37Z

This PR must be merged after PR #12840

Why I did it

Added ECMP calculator tool.

How I did it

New files were added.

How to verify it

Manual tests performed according to tests chapter in HLD
Automated tests will be added by verification.

Which release branch to backport (provide reason below if selected)

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

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

lgtm-com · 2022-10-25T04:12:29Z

This pull request introduces 1 alert when merging b53de67 into a0661e2 - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2022-10-31T10:08:21Z

This pull request introduces 4 alerts when merging d97f5c0 into 6bed69a - view on LGTM.com

new alerts:

3 for Unused import
1 for Unused local variable

This reverts commit d97f5c0.

This reverts commit 6c4162e.

lgtm-com · 2022-10-31T12:10:09Z

This pull request introduces 1 alert when merging 5b6d48c into 6bed69a - view on LGTM.com

new alerts:

1 for Unused import

liat-grozovik · 2022-11-08T11:43:02Z

@liorghub is this PR ready for review? anything else is missing?

liorghub · 2022-11-08T20:04:30Z

@liat-grozovik No, this PR is not ready for review. Rest of the code is ready and reviewed internally, I will publish it once I will get verification approval.

liorghub · 2022-11-09T09:19:13Z

@liat-grozovik
PR was updated and now contains all the code, however, it is marked as draft.
When verification will approve I will remove the draft.

@nazariig
Can you please approve?

liat-grozovik · 2022-11-14T19:40:21Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2022-11-14T19:40:35Z

Azure Pipelines successfully started running 1 pipeline(s).

liorghub · 2022-11-15T07:33:25Z

@liat-grozovik Compilation is failing since SDK does not contain hash calculator debian. PR Add SDK hash calculator debian to SONiC Bullseye should be merged to fix it.

Build log error:

cp: cannot stat 'platform/mellanox/sdk-src/sx-kernel/Switch-SDK-drivers/bin//bullseye/amd64//sx-hash-calc_1.mlnx.4.5.3186_amd64.deb': No such file or directory

dgsudharsan · 2022-11-23T02:49:56Z

platform/mellanox/docker-syncd-mlnx/ecmp_calculator/ecmp_calc_sdk.py

+        delete_uint32_t_p(entries_cnt_p)
+        delete_sx_ip_prefix_t_p(ip_prefix_p)
+
+def sx_router_operational_ecmp_get(handle, ecmp_id):


Can we rename this API to something like sx_router_ecmp_get_nexthops?

Done, API name changed.

dgsudharsan · 2022-11-23T02:54:12Z

platform/mellanox/docker-syncd-mlnx/ecmp_calculator/ecmp_calc_sdk.py

+        delete_sx_router_id_t_p(vrid_p)
+
+def sx_port_vport_base_get(handle, vport):
+    """ Get SDK logical index for given vport


Logical index and Vlan?

Yes, I updated function documentation.

dgsudharsan · 2022-11-23T02:55:57Z

platform/mellanox/docker-syncd-mlnx/ecmp_calculator/ecmp_calc_sdk.py

+        handle (sx_api_handle_t): SDK handle
+        rif (sx_port_id_t): SDK vport id
+        addr (sx_ip_addr_t): Neighbour IP address
+        type_ipv6 (bool): True for IPv6


type_ipv6 is not used. Please remove.

Unused variable type_ipv6 was removed.

dgsudharsan · 2022-11-23T03:11:16Z

platform/mellanox/docker-syncd-mlnx/Dockerfile.j2

@@ -70,4 +71,10 @@ RUN mkdir -p /etc/supervisor/conf.d/
 RUN sonic-cfggen -a "{\"ENABLE_ASAN\":\"{{ENABLE_ASAN}}\"}" -t /usr/share/sonic/templates/supervisord.conf.j2 > /etc/supervisor/conf.d/supervisord.conf
 RUN rm -f /usr/share/sonic/templates/supervisord.conf.j2

+RUN mkdir -p /usr/bin/ecmp_calculator
+COPY ["ecmp_calculator/ecmp_calc_sdk.py", "/usr/bin/ecmp_calculator"]
+COPY ["ecmp_calculator/ecmp_calc.py", "/usr/bin/ecmp_calculator"]


Apart from ecmp_calc.py other files are not binaries but libraries.Can we package them accordingly?

dgsudharsan · 2022-11-23T03:15:48Z

platform/mellanox/docker-syncd-mlnx/ecmp_calculator/ecmp_calc.py

+    def validate_inner_header(self):
+        inner_header = None
+        try:
+            inner_header = self.packet['packet_info']['inner']


Instead we could do something like inner_header = self.packet['packet_info'].get('inner') instead of doing try except. get will return None if there is no such key.

I have changed the code to use get().

dgsudharsan · 2022-11-23T03:21:51Z

platform/mellanox/docker-syncd-mlnx/ecmp_calculator/ecmp_calc.py

+            if ip in header and is_ip_valid(header[ip], IP_VERSION_IPV6) == False:
+                raise ValueError("Json validation failed: invalid IP {}".format(header[ip]))
+
+    def validate_layer2_header(self, header):


Should ethertype, vlan and other L2 fields be validated as well?

By design, Ethertype, vlan and other L2 fields are being validated for their types only.
Each field is defined in PACKET_SCHEME as a number or as a string and validation is being performed using python3-jsonschema module.

dgsudharsan · 2022-11-23T03:25:22Z

platform/mellanox/docker-syncd-mlnx/ecmp_calculator/ecmp_calc.py

+
+        raise ValueError("VRF validation failed: VRF {} does not exist".format(self.user_vrf))
+
+    def get_ecmp_ids(self):


get_ecmp_id* since we will get only one per route

Function name changed to get_ecmp_id().

dgsudharsan · 2022-11-23T03:45:03Z

platform/mellanox/docker-syncd-mlnx/ecmp_calculator/ecmp_calc_sdk.py

+    log_cb_fn = log_cb_type(log_cb)
+    handle_p = ctypes.pointer(ctypes.c_int64())
+
+    rc = sx_api.sx_api_open(log_cb_fn, handle_p)


During cleanup before the application exits can we invoke sx_api_close?

I added call to sx_api_close() in destructor.

This reverts commit f61999a.

liorghub · 2022-11-28T11:36:03Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2022-11-28T11:36:09Z

Commenter does not have sufficient privileges for PR 12482 in repo sonic-net/sonic-buildimage

liorghub · 2022-11-28T11:37:18Z

/azpw run Azure.sonic-buildimage

mssonicbld · 2022-11-28T11:37:19Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2022-11-28T11:37:32Z

Azure Pipelines successfully started running 1 pipeline(s).

dprital · 2022-11-28T12:00:43Z

platform/mellanox/docker-syncd-mlnx/lib/port_utils.py

+            lane_index = get_lane_index(lane_bmap, ASIC_MAX_LANES[chip_type])
+            assert lane_index != -1, "Failed to calculate port index"
+
+            sonic_index = label_port * ASIC_MAX_LANES[chip_type] + lane_index;


Check if label port start from 1 or 0

Label port is the port number printed on the front panel minus one.
On ACS-MSN3700C with 32 ports, first label port is 0 and last is 31 (total of 32 as expected).

dprital · 2022-11-28T12:30:24Z

platform/mellanox/docker-syncd-mlnx/ecmp_calculator/ecmp_calc.py

+        out = exec_cmd([HASH_CALC_PATH, '-c', HASH_CALC_INPUT_FILE, '-o', HASH_CALC_OUTPUT_FILE, '-d'])
+        self.debug_print ("Hash calculator output:\n{}".format(out))
+
+        with open(HASH_CALC_OUTPUT_FILE, 'r') as openfile:


you need to delete the file after using it. There is no delete for this file

I added call to temporary files cleanup in destructor.

…tion

…compile it (sonic-net#12840) - Why I did it Add SDK hash calculator Debian and update SDK makefile to compile it. - How I did it SDK hash calculator Debian will be used by ECMP calculator (PR sonic-net#12482) - How to verify it Compile sonic-buildimage and verify SDK hash calculator Debian exist in target folder.

…compile it (#12840) - Why I did it Add SDK hash calculator Debian and update SDK makefile to compile it. - How I did it SDK hash calculator Debian will be used by ECMP calculator (PR #12482) - How to verify it Compile sonic-buildimage and verify SDK hash calculator Debian exist in target folder.

- Why I did it Added ECMP calculator tool. - How I did it New files were added. - How to verify it Manual tests performed according to tests chapter in HLD Automated tests will be added by verification.

mssonicbld · 2023-01-08T14:33:13Z

Cherry-pick PR to 202211: #13301

liorghub added 2 commits October 12, 2022 14:37

Add ECMP calculator SDK module

ffcad80

Fix review comments

b53de67

liorghub requested a review from lguohan as a code owner October 25, 2022 03:47

liorghub marked this pull request as draft October 25, 2022 03:48

liat-grozovik added the Platform: Mellanox label Oct 25, 2022

Add SDK hash calculator debian to SONiC

6c4162e

liat-grozovik requested a review from prsunny October 30, 2022 07:57

dprital requested a review from nazariig October 30, 2022 11:18

Add ECMP calculator to SONiC

d97f5c0

liorghub added 2 commits October 31, 2022 11:28

Revert "Add ECMP calculator to SONiC"

29a01d0

This reverts commit d97f5c0.

Revert "Add SDK hash calculator debian to SONiC"

5b6d48c

This reverts commit 6c4162e.

liorghub added 5 commits November 9, 2022 09:10

Add SDK hash calculator debian to SONiC

f61999a

Add ECMP calculator to SONiC

95d2267

Add README file

3b1215c

Fix review comments

8b27f63

Fix Sempreg checker issue in subprocess call

310d546

liorghub changed the title ~~Add ECMP calculator SDK module~~ Add ECMP calculator tool Nov 9, 2022

Add more validations of packet json

cd3ac8c

dgsudharsan suggested changes Nov 23, 2022

View reviewed changes

liorghub mentioned this pull request Nov 27, 2022

[Mellanox] Add SDK hash calculator debian and update SDK makefile to compile it #12840

Merged

7 tasks

Revert "Add SDK hash calculator debian to SONiC"

b32b483

This reverts commit f61999a.

dprital reviewed Nov 28, 2022

View reviewed changes

liorghub added 3 commits November 29, 2022 12:05

Do not parse VRF query results if empty string is returned

44e56c8

Fix more review comments

ab9af4d

Remove unused object from packet scheme

50f03bf

dgsudharsan approved these changes Nov 29, 2022

View reviewed changes

liorghub marked this pull request as ready for review November 29, 2022 15:50

Fix issue in syncd Dockerfile, create needed folder before copy opera…

a2be99e

…tion

dprital added the Request for 202211 Branch label Dec 1, 2022

Update ECMP calculator README to use CLI command

b0e372d

liat-grozovik approved these changes Dec 4, 2022

View reviewed changes

liat-grozovik merged commit e580802 into sonic-net:master Dec 4, 2022

This was referenced Dec 7, 2022

SONiC ECMP calculator PR tracker sonic-net/SONiC#1140

Closed

SONiC ECMP calculator PR tracker liorghub/SONiC#4

Closed

[PR tracker] SONiC ECMP calculator sonic-net/SONiC#1163

Merged

StormLiangMS added the Approved for 202211 Branch label Jan 8, 2023

mssonicbld added the Created PR to 202211 Branch label Jan 8, 2023

mssonicbld mentioned this pull request Jan 8, 2023

[action] [PR:12482] Add ECMP calculator tool #13301

Merged

mssonicbld added a commit that referenced this pull request Jan 8, 2023

Add ECMP calculator tool (#12482) (#13301)

1e522ff

mssonicbld added Included in 202211 Branch and removed Approved for 202211 Branch Created PR to 202211 Branch labels Jan 8, 2023


		raise ValueError("VRF validation failed: VRF {} does not exist".format(self.user_vrf))

		def get_ecmp_ids(self):

Add ECMP calculator tool #12482

Add ECMP calculator tool #12482

Conversation

liorghub commented Oct 25, 2022 • edited Loading

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

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

lgtm-com bot commented Oct 25, 2022

lgtm-com bot commented Oct 31, 2022

lgtm-com bot commented Oct 31, 2022

liat-grozovik commented Nov 8, 2022

liorghub commented Nov 8, 2022

liorghub commented Nov 9, 2022

liat-grozovik commented Nov 14, 2022

azure-pipelines bot commented Nov 14, 2022

liorghub commented Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

liorghub Nov 28, 2022 • 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

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

liorghub commented Nov 28, 2022

azure-pipelines bot commented Nov 28, 2022

liorghub commented Nov 28, 2022

mssonicbld commented Nov 28, 2022

azure-pipelines bot commented Nov 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liorghub Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

mssonicbld commented Jan 8, 2023

liorghub commented Oct 25, 2022 •

edited

Loading

liorghub commented Nov 15, 2022 •

edited

Loading

liorghub Nov 28, 2022 •

edited

Loading

liorghub Nov 29, 2022 •

edited

Loading