Confirm portchannel and member ports exist before attempting delete operations#454
Confirm portchannel and member ports exist before attempting delete operations#454madhupalu wants to merge 7 commits intosonic-net:masterfrom
Conversation
…members add/delete
only when it is configured (sonic-net#277)
Signed-off-by: madhu Pal <[email protected]>
|
|
…red (sonic-net#277) Signed-off-by: madhu Pal <[email protected]>
madhupalu
changed the title
|
Issue Link- sonic-net/SONiC#277 |
jleveque
left a comment
jleveque
left a comment
There was a problem hiding this comment.
These functions need to work under both interface naming modes (default and alias). Please refer to add_vlan_member() and del_vlan_member() for examples. You can test by switching modes using config interface_naming_mode [default | alias].
|
thanks for the comments, I'll add interface alias test cases to the code changes. |
…erations, covered vendor interfaces as well. (sonic-net#277) Signed-off-by: madhu Pal <[email protected]>
… into azure277 wq Signed-off-by: madhu Pal <[email protected]>
Signed-off-by: madhu Pal <[email protected]>
|
I've updated the PR with all switching modes [default | alias] cases for your review. |
|
jleveque, Can you review and provide your comments. |
jleveque
changed the title
|
@madhupalu: I updated the title of this PR for clarity. |
|
Retest this please |
…tempting delete operations (sonic-net#277) Signed-off-by: madhu Pal <[email protected]>
|
Joe, Shuotian, |
config/main.py
Outdated
| members = port_channel.get('members', []) | ||
| if port_name in members: | ||
| if get_interface_naming_mode() == "alias": | ||
| port_name = interface_name_to_alias(port_name) |
There was a problem hiding this comment.
will this line duplicate with line 475?
if it is 'alias' mode and the port_name is already in members, it will try to convert the name again?
There was a problem hiding this comment.
Thanks for review comments, I'll fix and run the test cases, will update the PR shortly
| if len(members) == 0: | ||
| del port_channel['members'] | ||
| else: | ||
| port_channel['members'] = members |
There was a problem hiding this comment.
we need the **members** variable to delete the port channel when there are no members in it. AFAIK, we delete port channels when there are no members in it, not sure the SONiC use case. please advise.
Thanks
Madhu
There was a problem hiding this comment.
addressed the review comments and updated the PR. Thanks.
There was a problem hiding this comment.
there're alternative ways of checking if the port-channel has members or not. please do not use this approach. we are trying to make the code DRY by not having duplicated information on two places; this attribute won't be supported in the future release
…h remove_portchannel Signed-off-by: madhu Pal <[email protected]>
|
@stcheng: Can you please re-review? |
| if len(members) == 0: | ||
| del port_channel['members'] | ||
| else: | ||
| port_channel['members'] = members |
There was a problem hiding this comment.
there're alternative ways of checking if the port-channel has members or not. please do not use this approach. we are trying to make the code DRY by not having duplicated information on two places; this attribute won't be supported in the future release
4 participants
commit 70c12ed41138a25ce486581354ba7139bfecd14c (HEAD -> azure277)
Author: madhu Pal [email protected]
Date: Sun Feb 3 00:00:21 2019 +0000
- What I did
Added validations to portchannel & port channel members delete operations only it is configured
- How I did it
Checked Portchannel and PortChannel member configDB to validate the port channel/members exists before delete them.
- How to verify it
1. delete a portchannel which is not configured, CLI should throws an error
2. delete a portchannel member which is not configured, CLI should throws an error
3. Add a portchannel member where portchannel didn't configured, CLI throws an error
I Found this bug while doing unit testing.
Additional cases with vendor interfaces
Test 4: delete port channel without deleting port channel members