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

[Mellanox] Add new CLI commands to support TXMON feature #1

Open
wants to merge 1 commit into
base: 201911
Choose a base branch
from

Conversation

deran1980
Copy link
Owner

- What I did
Added new CLI commands to support TXMON (LAB 5 ) feature.
2 configuration commands and 2 show commands.

- How I did it
edited show show/main.py and config config/main.py

- How to verify it
Manual testing:
config commands:

  • sudo config interface tx_thresh <set|clear> <interface_alias> <threshold_value>
  • sudo config txmon <period|disable>

show commands:

  • show interfaces txmon - show current status of ports
  • show txmon - show if period is set and its value

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

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

@click.pass_context
@click.argument('interface_name', metavar='<interface_name>', required=True)
@click.argument('tx_thresh', metavar='<tx_thresh>', required=True, type=int)
def set(ctx, interface_name, tx_thresh):

Choose a reason for hiding this comment

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

please also verify the "tx_thresh" is bigger than 0.

# 'period' command ('config txmon period ...')
#
@txmon.command('period', short_help="Set global period interval (seconds) for Tx Monitor")
@click.argument('interval', metavar='<interval>', required=True, type=int)

Choose a reason for hiding this comment

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

please also verify the "interval" is bigger than 0.


state_db = SonicV2Connector(host='127.0.0.1')
state_db.connect(state_db.STATE_DB, False) # Make one attempt only
TABLE_NAME_SEPARATOR = '|'

Choose a reason for hiding this comment

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

TABLE_NAME_SEPARATOR is not used

@@ -2001,6 +2001,63 @@ def shutdown(ctx, interface_name):
else:
config_db.mod_entry("PORTCHANNEL", interface_name, {"admin_status": "down"})

#

Choose a reason for hiding this comment

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

Usually when we add a new CLI command in SONiC we don't mix other existing commands with new options.
A correct implementation will be:

  • config txmon set Ethernet# 10
  • config txmon clear Ethernet#
  • config txmon period 10
  • config txmon state enable/disable

@@ -668,6 +668,48 @@ def alias(interfacename):
body.append([port_name, port_name])

click.echo(tabulate(body, header))

Copy link

@shlomibitton shlomibitton Jan 11, 2021

Choose a reason for hiding this comment

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

Same comment here as in 'config'
A correct implementation can be:

  • show txmon status
    To show the current interval and then a table of all configured interfaces.

or

  • show txmon interval
  • show txmon interfaces

Copy link

@shlomibitton shlomibitton left a comment

Choose a reason for hiding this comment

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

As commented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants