[autoneg] add support for remote speed advertisement by ds952811 · Pull Request #2124 · sonic-net/sonic-utilities

ds952811 · 2022-04-01T03:35:45Z

Add support for remote speed advertisement, such that users could easily
identify the connection issues when autoneg is enabled.

HLD: sonic-net/SONiC#924

Signed-off-by: Dante Su dante.su@broadcom.com

What I did

Add support for remote speed advertisement

How I did it

Implementation is done according to the AutoNeg HLD

How to verify it

Built-in unit tests
Manual tests

Previous command output (if the output of a command-line utility has changed)

  Interface    Auto-Neg Mode    Speed    Adv Speeds    Type    Adv Types    Oper    Admin
-----------  ---------------  -------  ------------  ------  -----------  ------  -------
  Ethernet0          enabled      25G       10G,50G     CR4      CR4,CR2    down       up
 Ethernet16              N/A     100M           N/A     N/A          N/A      up       up
 Ethernet24              N/A       1G           N/A     N/A          N/A      up       up
 Ethernet28              N/A    1000M           N/A     N/A          N/A      up       up
 Ethernet32         disabled      40G           all     N/A          all      up       up
 Ethernet36              N/A      10M           N/A     N/A          N/A      up       up

New command output (if the output of a command-line utility has changed)

  Interface    Auto-Neg Mode    Speed    Adv Speeds    Rmt Adv Speeds    Type    Adv Types    Oper    Admin
-----------  ---------------  -------  ------------  ----------------  ------  -----------  ------  -------
  Ethernet0          enabled      25G       10G,50G       10M,100M,1G     CR4      CR4,CR2    down       up
 Ethernet16              N/A     100M           N/A               N/A     N/A          N/A      up       up
 Ethernet24              N/A       1G           N/A               N/A     N/A          N/A      up       up
 Ethernet28              N/A    1000M           N/A               N/A     N/A          N/A      up       up
 Ethernet32         disabled      40G           all               N/A     N/A          all      up       up
 Ethernet36              N/A      10M           N/A               N/A     N/A          N/A      up       up

dgsudharsan

Please update the description with old CLI output and new CLI output.
The CLI output change will impact existing systems in production and automation frameworks that rely on a specific output pattern. Has this been considered?

dgsudharsan · 2022-04-06T02:18:22Z

tests/intfutil_test.py

        assert result.output == show_interface_auto_neg_status_Ethernet0_output

-    def test_show_interfaces_autoneg_status_etp9_in_alias_mode(self):
+    def test_show_interfaces_autoneg_status_eth9_in_alias_mode(self):


Why do you need to change the test case name to eth9 when it is using etp9?

I thought it's a typo, and now it's reverted

dgsudharsan · 2022-04-06T02:20:04Z

scripts/intfutil

        status = ','.join(new_speed_list)
    return status

+def state_db_port_status_get(db, intf_name, type):


Please rename 'type' to 'field'

dgsudharsan · 2022-04-06T02:22:07Z

scripts/intfutil

+        speed_list = status.split(',')
+        new_speed_list = []
+        for s in natsorted(speed_list):
+            new_speed_list.append('{}G'.format(s[:-3]))


This logic will not work for switches that have less than 1G speed (e.g mgmt switches which run sonic) Please consider the use case as well.

Junchao-Mellanox · 2022-04-08T03:17:33Z

scripts/intfutil

+    """
+    full_table_id = PORT_STATE_TABLE_PREFIX + intf_name
+    status = db.get(db.STATE_DB, full_table_id, type)
+    if status in [None, ""]:


"if not status" should do the job

Junchao-Mellanox · 2022-04-08T03:19:35Z

tests/dump_tests/dump_state_test.py

                                  'APPL_DB': {'keys': [{'PORT_TABLE:Ethernet0': {'index': '0', 'lanes': '0', 'alias': 'Ethernet0', 'description': 'ARISTA01T2:Ethernet1', 'speed': '25000', 'oper_status': 'down', 'pfc_asym': 'off', 'mtu': '9100', 'fec': 'rs', 'admin_status': 'up'}}], 'tables_not_found': []},
                                  'ASIC_DB': {'keys': [{'ASIC_STATE:SAI_OBJECT_TYPE_HOSTIF:oid:0xd00000000056d': {'SAI_HOSTIF_ATTR_NAME': 'Ethernet0', 'SAI_HOSTIF_ATTR_OBJ_ID': 'oid:0x10000000004a4', 'SAI_HOSTIF_ATTR_OPER_STATUS': 'true', 'SAI_HOSTIF_ATTR_TYPE': 'SAI_HOSTIF_TYPE_NETDEV', 'SAI_HOSTIF_ATTR_VLAN_TAG': 'SAI_HOSTIF_VLAN_TAG_STRIP'}}, {'ASIC_STATE:SAI_OBJECT_TYPE_PORT:oid:0x10000000004a4': {'NULL': 'NULL', 'SAI_PORT_ATTR_ADMIN_STATE': 'true', 'SAI_PORT_ATTR_MTU': '9122', 'SAI_PORT_ATTR_SPEED': '100000'}}], 'tables_not_found': [], 'vidtorid': {'oid:0xd00000000056d': 'oid:0xd', 'oid:0x10000000004a4': 'oid:0x1690000000001'}},
-                                  'STATE_DB': {'keys': [{'PORT_TABLE|Ethernet0': {'speed': '100000', 'supported_speeds': '10000,25000,40000,100000'}}], 'tables_not_found': []}}}
+                                  'STATE_DB': {'keys': [{'PORT_TABLE|Ethernet0': {'rmt_adv_speeds': '40000', 'speed': '100000', 'supported_speeds': '10000,25000,40000,100000'}}], 'tables_not_found': []}}}


Maybe we should cover case that 'rmt_adv_speeds' has multiple values.

dgsudharsan · 2022-04-21T00:45:08Z

scripts/intfutil

+            status = '{}G'.format(int(speed / 1000))
+        else:
+            status = '{}M'.format(speed)
+    elif field in [PORT_ADV_SPEEDS, PORT_RMT_ADV_SPEEDS] and status not in ["N/A", "all"]:


I don't think we have PORT_ADV_SPEEDS field in STATE_DB. Why are we adding check here?

Yes, you're right, it's removed now

dgsudharsan · 2022-04-21T02:12:54Z

Please resolve the conflicts

dgsudharsan · 2022-04-29T03:53:25Z

Please update the PR description with old command output and new command output as requested.

adyeung · 2022-05-05T21:16:20Z

@zhangyanzhao @yxieca pls help signoff and merge

prgeor · 2022-05-09T17:44:13Z

scripts/intfutil

+    status = db.get(db.STATE_DB, full_table_id, field)
+    if not status:
+        return "N/A"
+    if field == PORT_SPEED and status != "N/A":


there is already port_oper_speed_get() to get the speed, so why is this if check required?

Add support for remote speed advertisement, such that users could easily identify the connection issues when autoneg is enabled. HLD: sonic-net/SONiC#924 - What I did Add support for remote speed advertisement - How I did it Implementation is done according to the AutoNeg HLD Signed-off-by: Dante Su <dante.su@broadcom.com>

Signed-off-by: Dante Su <dante.su@broadcom.com>

ds952811 mentioned this pull request Apr 1, 2022

[autoneg] add support for remote speed advertisement and clarify the expected autoneg behaviors sonic-net/SONiC#924

Merged

dgsudharsan requested review from Junchao-Mellanox and dgsudharsan April 6, 2022 01:11

dgsudharsan requested changes Apr 6, 2022

View reviewed changes

Junchao-Mellanox requested changes Apr 8, 2022

View reviewed changes

dgsudharsan requested changes Apr 21, 2022

View reviewed changes

ds952811 force-pushed the autoneg branch from 19a7589 to 63c6b47 Compare April 22, 2022 02:17

Junchao-Mellanox previously approved these changes Apr 27, 2022

View reviewed changes

dgsudharsan previously approved these changes Apr 29, 2022

View reviewed changes

prgeor reviewed May 9, 2022

View reviewed changes

ds952811 dismissed stale reviews from dgsudharsan and Junchao-Mellanox via 3d2f5c3 May 10, 2022 13:36

ds952811 added 5 commits May 10, 2022 13:40

fix test failures in dump_state_test.py

6e1d7e7

Signed-off-by: Dante Su <dante.su@broadcom.com>

address review comments

1f6d48a

Signed-off-by: Dante Su <dante.su@broadcom.com>

drop PORT_ADV_SPEEDS from state_db_port_status_get()

5c32a21

Signed-off-by: Dante Su <dante.su@broadcom.com>

address review comments

74e2d2c

Signed-off-by: Dante Su <dante.su@broadcom.com>

ds952811 force-pushed the autoneg branch from 3d2f5c3 to 74e2d2c Compare May 10, 2022 13:56

prgeor approved these changes May 10, 2022

View reviewed changes

prgeor merged commit bce4694 into sonic-net:master May 10, 2022

yozhao101 mentioned this pull request May 12, 2022

[master][sonic-utilities] Update sonic-utilities submodule sonic-net/sonic-buildimage#10781

Closed

Conversation

ds952811 commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What I did

How I did it

How to verify it

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

Uh oh!

dgsudharsan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgsudharsan commented Apr 21, 2022

Uh oh!

dgsudharsan commented Apr 29, 2022

Uh oh!

adyeung commented May 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ds952811 commented Apr 1, 2022 •

edited

Loading