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-utilities updates for MPLS #1537

Merged
merged 2 commits into from
Jul 13, 2021
Merged

sonic-utilities updates for MPLS #1537

merged 2 commits into from
Jul 13, 2021

Conversation

qbdwlr
Copy link
Contributor

@qbdwlr qbdwlr commented Mar 31, 2021

What I did
SONiC CLI support for MPLS:

  1. "config interface mpls (add | remove) <if-name>"
  2. "show interface mpls [<if-name>]"
  3. CRM threshold configuration for mpls_inseg and mpls_nexthop
  4. Updated SONiC CLI command reference manual.

Why I did it
SONiC CLI support for MPLS

How I verified it
Unit-tests in sonic-swss/tests/test_mpls.py and sonic-utilities/tests

Details if related
Refer to HLD: sonic-net/SONiC#706

@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2021

This pull request introduces 2 alerts when merging 48f08d186793a19b9553c1801370ac378e84091b into 6b51bcd - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

config/main.py Outdated Show resolved Hide resolved
config/main.py Show resolved Hide resolved
@abdosi
Copy link
Contributor

abdosi commented Jun 7, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@smaheshm
Copy link
Contributor

Refer to MPLS HLD: Azure/SONiC#706

Update description with the proper format, use merged PR's description as an example.

If you are adding a CLI, update the description section with the new CLIs added, and a snippet of output.

# 'mpls' subcommand ("show interfaces mpls")
@interfaces.command()
@click.argument('interfacename', required=False)
@multi_asic_util.multi_asic_click_options
Copy link
Contributor

Choose a reason for hiding this comment

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

for now remove the 'multi_asic_click_options' and the 'namespace' argument. Support for multi ASIC platform is requried but it would be hard to test and write unit tests without multi ASIC platform. (It is possible to test with multi ASIC VM)

@qbdwlr qbdwlr force-pushed the mplsPR branch 2 times, most recently from e3bad19 to 1f21769 Compare June 14, 2021 15:43
@qbdwlr
Copy link
Contributor Author

qbdwlr commented Jun 15, 2021

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 1537 in repo Azure/sonic-utilities

@smaheshm
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@qbdwlr qbdwlr changed the title sonic-utilities changes for MPLS sonic-utilities updates for MPLS Jun 15, 2021
@smaheshm
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

crm/main.py Outdated
Comment on lines 556 to 564
@mpls.command()
@click.pass_context
def inseg(ctx):
"""Show CRM information for in-segment resource"""
if ctx.obj["crm"].cli_mode == 'thresholds':
ctx.obj["crm"].show_thresholds('{0}_inseg'.format(ctx.obj["crm"].addr_family))
elif ctx.obj["crm"].cli_mode == 'resources':
ctx.obj["crm"].show_resources('{0}_inseg'.format(ctx.obj["crm"].addr_family))

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't understand why this is a repeat of line 520-528

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smaheshm This cut-n-paste remnant has been removed.

@qbdwlr qbdwlr requested a review from smaheshm July 7, 2021 12:24
Copy link
Contributor

@smaheshm smaheshm left a comment

Choose a reason for hiding this comment

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

Looks good.

# 'add' subcommand
#

@mpls.command()
Copy link
Contributor

Choose a reason for hiding this comment

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

why not move mpls into a mpls.py instead of put them in the main.py file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lguohan You asked this same question 3 months ago. I will restate my answer from then: All subgroups, including mpls, of the interface.group are in main.py. The mpls subgroup is very small and a separate file is overkill.

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.

5 participants