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

[teamshow]: Fixed teamshow output not displaying the custom portchann… #436

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

madhupalu
Copy link
Contributor

@madhupalu madhupalu commented Jan 18, 2019

…els #276

commit 4e80674 (HEAD -> teamshow, origin/teamshow)
Author: madhu Pal madhupa@aviznetworks.com
Date: Fri Jan 18 04:45:06 2019 +0000

[teamshow]: Fixed teamshow output not displaying the custom portchannels #276

- What I did
Fixed teamshow output with custom port channels.
- How I did it
Added a validation to allow portchannels with prefix 'PortChannel' and suffix 'xxxx'.

- How to verify it
root@sonic:/home/admin# config portchannel add helloPortChannel
Usage: config portchannel add [OPTIONS] <portchannel_name>

Error: helloPortChannel is invalid!, name should be prefix 'PortChannel' and suffix 'xxxx'
root@sonic:/home/admin# config portchannel add PortChannel009
root@sonic:/home/admin# config portchannel add PortCha
Usage: config portchannel add [OPTIONS] <portchannel_name>

Error: PortCha is invalid!, name should be prefix 'PortChannel' and suffix 'xxxx'

@lguohan lguohan requested a review from stcheng January 31, 2019 01:59
@jleveque jleveque added the Bug label Feb 1, 2019
@madhupalu
Copy link
Contributor Author

In addition that, confirmed custom portchannels are operational with broadcom.

@ramachandrareddygaddam
Copy link
Contributor

In the display first 11 characters displayed under 'Team dev' and remaining characters displayed under 'No.'.
Instead of that user input can be restricted to enter portchannel.

@madhupalu
Copy link
Contributor Author

I fixed this code, as SONiC is allowed custom port channels in the hardware and it's operational, so not restricting the user.
For instance, user wanted to program portchannel name as Uplink/serverfacing user defined names, we should allow, I do agree that this is not the use case SONiC trying to solve.
My opinion, user could provision to have port channel name and port channel id, this could be more involved.

@ramachandrareddygaddam
Copy link
Contributor

I agree that you fix is allowing to display the user entered custom port channels, instead of fixed name 'PortChannel' + team_id.

In your review the team_id value is taken from team_id[11:]. What happens when users enters the port channel name enters less than 11 characters.

@madhupalu
Copy link
Contributor Author

(No)it displayed nothing, empty. Hope (No)it doesn't have significance in show commands. Referring team show header below(No)
No. Team Dev Protocol Ports

@ramachandrareddygaddam
Copy link
Contributor

If (No) doesn't have significance in show commands, then request you to remove from display. In that case there is no restriction for Port Channel Name display.

@stcheng
Copy link
Contributor

stcheng commented Feb 12, 2019

Right now more than one place is using "PortChannel" + No as the logic for all the port channel interfaces. This is similar to the "Vlan" + No as the VLAN interfaces. I would not recommend to use random names for port channel interfaces since it would introduce more issues. (e.g. if the port channel interface's name is shorter than 11 characters, your pull request would not work).

Copy link
Contributor

@stcheng stcheng left a comment

Choose a reason for hiding this comment

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

Right now the prefix of port channel interfaces is fixed as "PortChannel".

@madhupalu
Copy link
Contributor Author

Hi All,
I'll validate the portchannels as discussed will raise a PR shortly. Only concern is, we are blocking the user from programming the user defined port channels.

-Madhu

…annel' and Suffix 'xxxx' (sonic-net#276)

Signed-off-by: madhu Pal <madhupa@aviznetworks.com>
@madhupalu
Copy link
Contributor Author

Shuotian,
Added a validation to allow port channels with prefix 'PortChannel' and suffix 'xxxx'. Though I suggested PortChannelNo should be 4 digits (4096), right now not validating NO should restrict to 4 digits, user allowed configure any number.
Please review and let us know your comments.

Thanks,
-Madhu

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

Successfully merging this pull request may close these issues.

4 participants