Skip to content

Add CLI command for ECMP calculator#2538

Closed
liorghub wants to merge 4 commits intosonic-net:masterfrom
liorghub:ecmp_calc_2
Closed

Add CLI command for ECMP calculator#2538
liorghub wants to merge 4 commits intosonic-net:masterfrom
liorghub:ecmp_calc_2

Conversation

@liorghub
Copy link
Contributor

@liorghub liorghub commented Dec 5, 2022

What I did

I have added a new CLI command for calling ECMP calculator.

How I did it

Add new CLI command. Command handler calls script /usr/bin/ecmp_calc.py

How to verify it

Call command and verify its output.

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)

root@sonic:/home/admin# show ip ecmp-egress-port --ingress-port Ethernet64 --packet /tmp/packet.json
Egress port: Ethernet0
root@sonic:/home/admin# 

@liat-grozovik
Copy link
Collaborator

@prsunny could you please review or suggest someone who can?

Copy link
Collaborator

@liat-grozovik liat-grozovik left a comment

Choose a reason for hiding this comment

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

missing tests for new code. please add
missing changes in command reference guide. please add

I dont think we should have -i before the interface name. just use the interface name
check for reference 'show interfaces transceiver presence Ethernet0'
same for the -p.

@prsunny
Copy link
Contributor

prsunny commented Dec 12, 2022

This is a vendor specific utility. I thought we can directly use the utility to get the hash result. I don't think we need to expose it via a generic CLI command.

@liat-grozovik
Copy link
Collaborator

In this case, the utility is already available and readme file explain directly how it can be used.
I was under the impression you would like to have the CLI and if the utility available it will be running.
But if this is not required, we can close this PR and leave with the utility itself.

@liorghub liorghub closed this Dec 14, 2022
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.

3 participants