Confirm portchannel and member ports exist before attempting delete operations #454
Conversation
…members add/delete
only when it is configured (sonic-net#277)
Signed-off-by: madhu Pal <madhupa@aviznetworks.com>
|
|
…red (sonic-net#277) Signed-off-by: madhu Pal <madhupa@aviznetworks.com>
madhupalu
changed the title
|
Issue Link- sonic-net/SONiC#277 |
|
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 <madhupa@aviznetworks.com>
… into azure277 wq Signed-off-by: madhu Pal <madhupa@aviznetworks.com>
Signed-off-by: madhu Pal <madhupa@aviznetworks.com>
|
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 <madhupa@aviznetworks.com>
|
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 <madhupa@aviznetworks.com>
|
@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
commit 70c12ed41138a25ce486581354ba7139bfecd14c (HEAD -> azure277)
Author: madhu Pal madhupa@aviznetworks.com
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