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

[fdbshow] Adding more options for fdbshow and show mac #1982

Merged
merged 10 commits into from
Jan 18, 2022

Conversation

dgsudharsan
Copy link
Collaborator

@dgsudharsan dgsudharsan commented Dec 22, 2021

What I did

Added more options to filter output in show mac and fdbshow command.
Introduced options for filter by address and filter by type.
Added one more option to display only count.
Introduced show command to display fdb aging time in the switch.

How I did it

Modifying fdbshow and show scripts to include the above-mentioned options

How to verify it

Added UT for all the newly introduced options and commands

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

show mac -h
Usage: show mac [OPTIONS]

  Show MAC (FDB) entries

Options:
  -v, --vlan TEXT
  -p, --port TEXT
  --verbose        Enable verbose output
  -h, -?, --help   Show this message and exit.

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

show mac -h
Usage: show mac [OPTIONS] COMMAND [ARGS]...

  Show MAC (FDB) entries

Options:
  -v, --vlan TEXT
  -p, --port TEXT
  -a, --address TEXT
  -t, --type TEXT
  -c, --count
  --verbose           Enable verbose output
  -h, -?, --help      Show this message and exit.

Commands:
  aging-time

show mac
  No.    Vlan  MacAddress         Port         Type
-----  ------  -----------------  -----------  -------
    1      10  98:03:9B:82:BB:5B  Ethernet60   Dynamic
    2      10  EC:0D:9A:CD:91:72  Ethernet64   Dynamic
    3      10  EC:0D:9A:CD:91:73  Ethernet124  Dynamic
Total number of entries 3

show mac --address EC:0D:9A:CD:91:72
  No.    Vlan  MacAddress         Port        Type
-----  ------  -----------------  ----------  -------
    1      10  EC:0D:9A:CD:91:72  Ethernet64  Dynamic

show mac --count
Total number of entries 3

show mac --type Dynamic
  No.    Vlan  MacAddress         Port         Type
-----  ------  -----------------  -----------  -------
    1      10  98:03:9B:82:BB:5B  Ethernet60   Dynamic
    2      10  EC:0D:9A:CD:91:72  Ethernet64   Dynamic
    3      10  EC:0D:9A:CD:91:73  Ethernet124  Dynamic
Total number of entries 3

show mac aging-time
Aging time for switch is 600 seconds

@liat-grozovik
Copy link
Collaborator

/azp run Azure.sonic-utilities

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Total number of entries 2
```
```
admin@sonic:~$ show mac -c
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 3, 2022

Choose a reason for hiding this comment

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

show mac -c

Is it same as show mac | grep Total? Not add much value. #Closed

Copy link
Collaborator Author

@dgsudharsan dgsudharsan Jan 12, 2022

Choose a reason for hiding this comment

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

Hi Qi,
I wrote a script and this would save a good amount of time in scaled configuration
Script is as below

date
fdbshow -c
date
fdbshow | grep Total
date

Output: -c takes 8 seconds whereas with grep the time taken is 14 seconds.

Wed 12 Jan 2022 05:30:23 AM UTC
Total number of entries 30747
Wed 12 Jan 2022 05:30:31 AM UTC
Total number of entries 30747
Wed 12 Jan 2022 05:30:45 AM UTC

In some scenarios it almost halves the time taken. On another note most of the options could be replaced by simple grep. The point of having them is performance as well as ease of use to the user. Most other operating systems do have these options and hence it makes beneficial to add this

Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering why it is so slow to grep. The only difference is collecting and printing a table. Is tabulate the slowest procedure?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Tabulate appears to be the bottleneck especially with bulk configurations. I prefer having the count option as is.

@qiluo-msft qiluo-msft requested a review from ganglyu January 3, 2022 23:14
scripts/fdbshow Outdated Show resolved Hide resolved
doc/Command-Reference.md Show resolved Hide resolved
doc/Command-Reference.md Show resolved Hide resolved
show/main.py Show resolved Hide resolved
Added validation for different options
Added new tests to validate error inputs
@lgtm-com
Copy link

lgtm-com bot commented Jan 13, 2022

This pull request introduces 1 alert when merging 8834555 into dd8c8fe - view on LGTM.com

new alerts:

  • 1 for Unused import

ganglyu
ganglyu previously approved these changes Jan 13, 2022
@dgsudharsan
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dgsudharsan
Copy link
Collaborator Author

/azpw run Azure.sonic-utilities

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-utilities

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

scripts/fdbshow Outdated

if not count:
for fdb in self.bridge_mac_list:
self.FDB_COUNT += 1
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 13, 2022

Choose a reason for hiding this comment

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

self.FDB_COUNT

Mixing class var and instance var is confusing. In this case, just use a function local var? #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed.

@dgsudharsan
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dgsudharsan dgsudharsan requested a review from ganglyu January 13, 2022 22:31
@dgsudharsan
Copy link
Collaborator Author

/azpw run Azure.sonic-utilities

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-utilities

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dgsudharsan
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dgsudharsan
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dgsudharsan
Copy link
Collaborator Author

/azpw run Azure.sonic-utilities

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-utilities

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dgsudharsan
Copy link
Collaborator Author

/azpw run Azure.sonic-utilities

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-utilities

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@qiluo-msft qiluo-msft merged commit d9f3afe into sonic-net:master Jan 18, 2022
@qiluo-msft qiluo-msft changed the title [fdbshow]Adding more options for fdbshow and show mac [fdbshow] Adding more options for fdbshow and show mac Jan 18, 2022
judyjoseph pushed a commit that referenced this pull request Jan 23, 2022
#### What I did
 Added more options to filter output in show mac and fdbshow command.
Introduced options for filter by address and filter by type.
Added one more option to display only count.
Introduced show command to display fdb aging time in the switch.

#### How I did it
Modifying fdbshow and show scripts to include the above-mentioned options

#### How to verify it
Added UT for all the newly introduced options and commands

#### Previous command output (if the output of a command-line utility has changed)
```
show mac -h
Usage: show mac [OPTIONS]

  Show MAC (FDB) entries

Options:
  -v, --vlan TEXT
  -p, --port TEXT
  --verbose        Enable verbose output
  -h, -?, --help   Show this message and exit.
```
#### New command output (if the output of a command-line utility has changed)
```
show mac -h
Usage: show mac [OPTIONS] COMMAND [ARGS]...

  Show MAC (FDB) entries

Options:
  -v, --vlan TEXT
  -p, --port TEXT
  -a, --address TEXT
  -t, --type TEXT
  -c, --count
  --verbose           Enable verbose output
  -h, -?, --help      Show this message and exit.

Commands:
  aging-time

show mac
  No.    Vlan  MacAddress         Port         Type
-----  ------  -----------------  -----------  -------
    1      10  98:03:9B:82:BB:5B  Ethernet60   Dynamic
    2      10  EC:0D:9A:CD:91:72  Ethernet64   Dynamic
    3      10  EC:0D:9A:CD:91:73  Ethernet124  Dynamic
Total number of entries 3

show mac --address EC:0D:9A:CD:91:72
  No.    Vlan  MacAddress         Port        Type
-----  ------  -----------------  ----------  -------
    1      10  EC:0D:9A:CD:91:72  Ethernet64  Dynamic

show mac --count
Total number of entries 3

show mac --type Dynamic
  No.    Vlan  MacAddress         Port         Type
-----  ------  -----------------  -----------  -------
    1      10  98:03:9B:82:BB:5B  Ethernet60   Dynamic
    2      10  EC:0D:9A:CD:91:72  Ethernet64   Dynamic
    3      10  EC:0D:9A:CD:91:73  Ethernet124  Dynamic
Total number of entries 3

show mac aging-time
Aging time for switch is 600 seconds
```
@dgsudharsan dgsudharsan deleted the show_mac branch March 9, 2023 02:06
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.

6 participants