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

Validations checks while adding a member to PortChannel and removing a member from a Portchannel #1328

Merged
merged 8 commits into from
Jan 12, 2021

Conversation

AkhileshSamineni
Copy link
Contributor

@AkhileshSamineni AkhileshSamineni commented Dec 22, 2020

Added below validation checks when user the configures a port as member a portchannel:
1. Check if the given portchannel name is valid or not
2. Check if the given portchannel name exists in CONFIG_DB
3. Check if the given port is configured with an IP address or not
4. Check if the given port is configured with with VLAN membership or not
5. Check if the given port is already member of a portchannel
6. Check if the given port speed matches the speed of existing members
7. Check if the given port MTU matches the speed of existing members

Added below validation checks when user the removes member port from a portchannel:
1. Check if the given portchannel name is valid or not
2. Check if the given portchannel name exists in exists in CONFIG_DB or not
3. Check if the given port is member of the given portchannel or not

This PR fixes the issue - #805
With this PR changes :

root@sonic:/home/admin# config portchannel add PortChannel0001
root@sonic:/home/admin# config portchannel add PortChannel0002
root@sonic:/home/admin#
root@sonic:/home/admin# config portchannel member add PortChannel0001 Ethernet28
root@sonic:/home/admin#
root@sonic:/home/admin# show interfaces portchannel
Flags: A - active, I - inactive, Up - up, Dw - Down, N/A - not available, S - selected, D - deselected
No. Team Dev Protocol Ports


0001 PortChannel0001 LACP(A)(Dw) Ethernet28(D)
0002 PortChannel0002 LACP(A)(Dw) N/A
root@sonic:/home/admin#
root@sonic:/home/admin# config portchannel member add PortChannel0002 Ethernet28
Usage: config portchannel member add [OPTIONS] <portchannel_name> <port_name>

Error: Ethernet28 Interface is already member of PortChannel0001
root@sonic:/home/admin#

This PR also fixes the issue - #806
With this PR changes :

root@sonic:/home/admin# config vlan add 100
root@sonic:/home/admin# config vlan member add 100 Ethernet9
root@sonic:/home/admin# config portchannel add PortChannel0001
root@sonic:/home/admin# config portchannel member add PortChannel0001 Ethernet9
Usage: config portchannel member add [OPTIONS] <portchannel_name> <port_name>

Error: Ethernet9 Interface configured as VLAN_MEMBER under vlan : Vlan100
root@sonic:/home/admin#

Signed-off-by: Akhilesh Samineni akhilesh.samineni@broadcom.com

…a member from a Portchannel

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
Copy link
Contributor

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

Could you also add some unit test to cover this scenarios?


# Dont allow a port to be member of port channel if its MTU does not match with portchannel
portchannel_entry = db.get_entry('PORTCHANNEL', portchannel_name)
if portchannel_entry and portchannel_entry.get(PORT_MTU) is not None :
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also reject configuring speed/mtu on an interface if its already a port-channel member? Else the check is only in one flow, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added similar check while configuring the mtu for an interface.

@AkhileshSamineni
Copy link
Contributor Author

Retest this please.

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
@AkhileshSamineni
Copy link
Contributor Author

Could you also add some unit test to cover this scenarios?

@prsunny
Added test cases to cover these scenarios.

@AkhileshSamineni
Copy link
Contributor Author

Retest this please.

@AkhileshSamineni
Copy link
Contributor Author

@prsunny Seems like build is failing below error
13:02:56 ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.

@AkhileshSamineni
Copy link
Contributor Author

Retest this please.

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
@AkhileshSamineni
Copy link
Contributor Author

Retest this please.

@AkhileshSamineni
Copy link
Contributor Author

@lguohan @prsunny Please merge this PR.

@prsunny prsunny merged commit dba8fcb into sonic-net:master Jan 12, 2021
lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jan 20, 2021
- [route_check.py] - update includes checks on subscriptions (sonic-net/sonic-utilities#1344)
- Validations checks while adding a member to PortChannel and removing a member from a Portchannel (sonic-net/sonic-utilities#1328)
- [show] Add subcommand to show midplane status for modular chassis (sonic-net/sonic-utilities#1267)
- [pytest][qos][config] Added pytests for "config qos reload" commands" (sonic-net/sonic-utilities#1346)
- Drop explict 3 seconds pause between two object updates/deletes. (sonic-net/sonic-utilities#1359)
- [show]fix for show muxcable status by replacing "hostname" to "peer_switch" for deriving tor ipv4_address (sonic-net/sonic-utilities#1360)
- [PFCWD] Fix 'start' pfcwd command (sonic-net/sonic-utilities#1345)

Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>
lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jan 21, 2021
- [route_check.py] - update includes checks on subscriptions (sonic-net/sonic-utilities#1344)
- Validations checks while adding a member to PortChannel and removing a member from a Portchannel (sonic-net/sonic-utilities#1328)
- [show] Add subcommand to show midplane status for modular chassis (sonic-net/sonic-utilities#1267)
- [pytest][qos][config] Added pytests for "config qos reload" commands" (sonic-net/sonic-utilities#1346)
- Drop explict 3 seconds pause between two object updates/deletes. (sonic-net/sonic-utilities#1359)
- [show]fix for show muxcable status by replacing "hostname" to "peer_switch" for deriving tor ipv4_address (sonic-net/sonic-utilities#1360)
- [PFCWD] Fix 'start' pfcwd command (sonic-net/sonic-utilities#1345)

Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>

# Dont allow a port to be member of port channel if its MTU does not match with portchannel
portchannel_entry = db.get_entry('PORTCHANNEL', portchannel_name)
if portchannel_entry and portchannel_entry.get(PORT_MTU) is not None :
Copy link

@yintao1995 yintao1995 Feb 1, 2021

Choose a reason for hiding this comment

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

May I ask a question, has it cover this kind of situation below:
if i configure a portchannel using json file, but it doesn't contain mtu. e.g:
{
"PORTCHANNEL":{
"PortChannel1234": {}
}
}
so teammgrd will consider it as default mtu 9100.
then i want to join a port with mtu 2000 into this portchannel with config cmd, the cmd will accept even their mtu are different because portchannel's mtu attrbute would not appear in config_db in this situation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yintao1995 Right, this kind of situation is not covered.

anand-kumar-subramanian pushed a commit to anand-kumar-subramanian/sonic-utilities that referenced this pull request Mar 2, 2021
…a member from a Portchannel (sonic-net#1328)

* Validations checks while adding a member to PortChannel and removing a member from a Portchannel

Added below validation checks when user the configures a port as member a portchannel:
1. Check if the given portchannel name is valid or not
2. Check if the given portchannel name exists in CONFIG_DB
3. Check if the given port is configured with an IP address or not
4. Check if the given port is configured with with VLAN membership or not
5. Check if the given port is already member of a portchannel
6. Check if the given port speed matches the speed of existing members
7. Check if the given port MTU matches the speed of existing members

Added below validation checks when user the removes member port from a portchannel:
1. Check if the given portchannel name is valid or not
2. Check if the given portchannel name exists in exists in CONFIG_DB or not
3. Check if the given port is member of the given portchannel or not

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
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.

4 participants