Skip to content
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

sonic-utils: initial support for link-training #2071

Merged
merged 5 commits into from
May 18, 2022

Conversation

ds952811
Copy link
Contributor

@ds952811 ds952811 commented Feb 16, 2022

HLD: sonic-net/SONiC#925

What I did

Add CLI support for link-trainig

How I did it

  1. portconfig: initial support for link-training config
  2. config/main.py: initial support for link-training config
  3. intfutil: initial support for link-training show command
  4. show/interfaces/init.py: initial support for link-training show command

How to verify it

  1. Manual test
  2. Ran the Unit-tests to the corresponding changes

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)

admin@sonic:~$ sudo config interface link-training Ethernet0 on
admin@sonic:~$ sudo config interface link-training Ethernet8 on
admin@sonic:~$ sudo config interface link-training Ethernet16 off
admin@sonic:~$ sudo config interface link-training Ethernet24 on
admin@sonic:~$ sudo config interface link-training Ethernet32 off
admin@sonic:~$ show interfaces link-training status
  Interface      LT Oper    LT Admin    Oper    Admin
-----------  -----------  ----------  ------  -------
  Ethernet0      trained          on      up       up
  Ethernet8      trained          on      up       up
 Ethernet16          off         off    down       up
 Ethernet24  not trained          on    down       up
 Ethernet32          off         off    down       up
 Ethernet40          off           -    down       up
 Ethernet48          off           -    down       up
 Ethernet56          off           -    down       up
 Ethernet64          off           -    down       up
 Ethernet72          off           -    down       up
 Ethernet80          off           -    down       up
 Ethernet88          off           -    down       up
 Ethernet96          off           -    down       up
Ethernet104          off           -    down       up
Ethernet112          off           -    down       up
Ethernet120          off           -    down       up
Ethernet128          off           -    down       up
Ethernet136          off           -    down       up
Ethernet144          off           -    down       up
Ethernet152          off           -    down       up
Ethernet160          off           -    down       up
Ethernet168          off           -    down       up
Ethernet176          off           -    down       up
Ethernet184          off           -    down       up
Ethernet192          off           -    down       up
Ethernet200          off           -    down       up
Ethernet208          off           -    down       up
Ethernet216          off           -    down       up
Ethernet224          off           -    down       up
Ethernet232          off           -    down       up
Ethernet240          off           -    down       up
Ethernet248          off           -    down       up
admin@sonic:~$



@lgtm-com
Copy link

lgtm-com bot commented Feb 16, 2022

This pull request introduces 3 alerts when merging 65e43cd into 9e50124 - view on LGTM.com

new alerts:

  • 3 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Mar 24, 2022

This pull request introduces 2 alerts when merging 188c561 into f71ef64 - view on LGTM.com

new alerts:

  • 2 for Unused import

config/main.py Outdated
@click.argument('mode', metavar='<mode>', required=True, type=click.Choice(["on", "off"]))
@click.option('-v', '--verbose', is_flag=True, help="Enable verbose output")
def link_training(ctx, interface_name, mode, verbose):
"""Set interface auto negotiation mode"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo. Should be link training instead of auto negotiation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@dgsudharsan
Copy link
Collaborator

The changes look good to me. Can you please resolve the conflicts? I will approve once the conflicts are resolved

if self.verbose:
print("Setting link-training %s on port %s" % (mode, port))
if mode not in ['on', 'off']:
mode = 'off'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe print an error and exit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted, and it's done

dgsudharsan
dgsudharsan previously approved these changes Apr 29, 2022
@adyeung
Copy link

adyeung commented May 5, 2022

@zhangyanzhao @yxieca pls help signoff and merge

- What I did
Add CLI support for link-trainig

- How I did it
portconfig: initial support for link-training config
config/main.py: initial support for link-training config
intfutil: initial support for link-training show command
show/interfaces/init.py: initial support for link-training show command

- How to verify it
Manual test
Ran the Unit-tests to the corresponding changes

Signed-off-by: Dante Su <[email protected]>
@@ -639,3 +639,36 @@ def autoneg_status(interfacename, namespace, display, verbose):
cmd += " -n {}".format(namespace)

clicommon.run_command(cmd, display_cmd=verbose)

#
# link-training group (show interfaces link-training ...)
Copy link
Contributor

@qiluo-msft qiluo-msft May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

show interfaces

Is there a reason to choose show interfaces vs show interface? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea, this is a legacy command, and my best guess is that it supports both signle interface and multi-interface at the same time, hence the original author adopted 'show interfaces' instead of 'show interface'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you fix the PR description? I see many wrong subcommand.

sudo config interface link-training Ethernet0 on

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not following you, is this about 'interfaces' v.s. 'interface' in show and config command?
If that's the case, this is a legacy command format since day 1, while the link-training is actually a plug-in to these legacy CLICK based CLI commands, hence it's not expected to change the upper level command syntax.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I see the difference between show and config.

@qiluo-msft qiluo-msft merged commit 88286cb into sonic-net:master May 18, 2022
dprital added a commit to dprital/sonic-buildimage that referenced this pull request May 25, 2022
Update sonic-utilities submodule pointer to include the following:
* [GCU] Handling type1 lists ([sonic-net#2171](sonic-net/sonic-utilities#2171))
* [yang] extend ConfigMgmt constructor to pass YANG options ([sonic-net#2118](sonic-net/sonic-utilities#2118))
* [dump] implement ACL modules ([sonic-net#2153](sonic-net/sonic-utilities#2153))
* show commands for SYSTEM READY ([sonic-net#1851](sonic-net/sonic-utilities#1851))
* [GCU] Handling non-compliant leaf-list with string values ([sonic-net#2174](sonic-net/sonic-utilities#2174))
* Add sonic-delayed.target to Application Extension .timer file generator ([sonic-net#2176](sonic-net/sonic-utilities#2176))
* [portconfig] Allow to configure interface mtu for physical ports ([#l](https://github.com/Azure/sonic-utilities/pull/l))
* Broadcast Unknown-multicast and Unknown-unicast Storm-control  ([sonic-net#928](sonic-net/sonic-utilities#928))
* sonic-utils: initial support for link-training ([sonic-net#2071](sonic-net/sonic-utilities#2071))
* [portchannel] Added ACL/PBH binding checks to the port before getting added to portchannel ([sonic-net#2151](sonic-net/sonic-utilities#2151))
* Modify override testcase to cover PORT admin_status ([sonic-net#2165](sonic-net/sonic-utilities#2165))
* [GCU] Validate peer_group_range ip_range are correct ([sonic-net#2145](sonic-net/sonic-utilities#2145))
* [auto-ts] add memory check ([sonic-net#2116](sonic-net/sonic-utilities#2116))
* support new interface types CR8/SR8/KR8/LR8 which are brougnt by SAI V.1.10.2 ([sonic-net#2167](sonic-net/sonic-utilities#2167))
* [scripts/fast-reboot] Add option to include ssd-upgrader-part boot option with SONiC partition ([sonic-net#2150](sonic-net/sonic-utilities#2150))
* [config reload] Fix invalid rstrip. ([sonic-net#2157](sonic-net/sonic-utilities#2157))
* Accept 0 for queue and dscp ([sonic-net#2162](sonic-net/sonic-utilities#2162))

Signed-off-by: dprital <[email protected]>
@Blueve
Copy link
Contributor

Blueve commented Jul 7, 2022

@ds952811 can you help update CLI doc as well?

@ds952811
Copy link
Contributor Author

ds952811 commented Jul 7, 2022

@ds952811 can you help update CLI doc as well?

Done, it's now available in PR #2257

yxieca pushed a commit that referenced this pull request Aug 11, 2022
HLD: sonic-net/SONiC#925

#### What I did
Add CLI support for link-trainig

#### How I did it
1. portconfig: initial support for link-training config
2. config/main.py: initial support for link-training config
3. intfutil: initial support for link-training show command
4. show/interfaces/__init__.py: initial support for link-training show command

#### How to verify it
1. Manual test
2. Ran the Unit-tests to the corresponding changes

#### 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)

```
admin@sonic:~$ sudo config interface link-training Ethernet0 on
admin@sonic:~$ sudo config interface link-training Ethernet8 on
admin@sonic:~$ sudo config interface link-training Ethernet16 off
admin@sonic:~$ sudo config interface link-training Ethernet24 on
admin@sonic:~$ sudo config interface link-training Ethernet32 off
admin@sonic:~$ show interfaces link-training status
  Interface      LT Oper    LT Admin    Oper    Admin
-----------  -----------  ----------  ------  -------
  Ethernet0      trained          on      up       up
  Ethernet8      trained          on      up       up
 Ethernet16          off         off    down       up
 Ethernet24  not trained          on    down       up
 Ethernet32          off         off    down       up
 Ethernet40          off           -    down       up
 Ethernet48          off           -    down       up
 Ethernet56          off           -    down       up
 Ethernet64          off           -    down       up
 Ethernet72          off           -    down       up
 Ethernet80          off           -    down       up
 Ethernet88          off           -    down       up
 Ethernet96          off           -    down       up
Ethernet104          off           -    down       up
Ethernet112          off           -    down       up
Ethernet120          off           -    down       up
Ethernet128          off           -    down       up
Ethernet136          off           -    down       up
Ethernet144          off           -    down       up
Ethernet152          off           -    down       up
Ethernet160          off           -    down       up
Ethernet168          off           -    down       up
Ethernet176          off           -    down       up
Ethernet184          off           -    down       up
Ethernet192          off           -    down       up
Ethernet200          off           -    down       up
Ethernet208          off           -    down       up
Ethernet216          off           -    down       up
Ethernet224          off           -    down       up
Ethernet232          off           -    down       up
Ethernet240          off           -    down       up
Ethernet248          off           -    down       up
admin@sonic:~$
```
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Aug 12, 2022
Update sonic-utilities submodule pointer to include the following:
* Fix test failure in dump table test in 202205 ([sonic-net#2307](sonic-net/sonic-utilities#2307))
* Convert IPv6 addresses to lowercase in apply-patch ([sonic-net#2299](sonic-net/sonic-utilities#2299))
* [config][muxcable] add support to enable/disable ycable telemetry ([sonic-net#2297](sonic-net/sonic-utilities#2297))
* Fix GCU bug when backend service modifying config ([sonic-net#2295](sonic-net/sonic-utilities#2295))
* [intfutil] Check whether the FEC mode is supported on the platform before configuring it to CONFIG_DB ([sonic-net#2223](sonic-net/sonic-utilities#2223))
* Improve the way to check port type of RJ45 port ([sonic-net#2249](sonic-net/sonic-utilities#2249))
* sonic-utils: initial support for link-training ([sonic-net#2071](sonic-net/sonic-utilities#2071))
* Support to enable fips for the command sonic_installer (sonic-net#2154) ([sonic-net#2303](sonic-net/sonic-utilities#2303))

Signed-off-by: dprital <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants