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

Fixing the delete vlan function #391

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tientienle
Copy link
Contributor

@tientienle tientienle commented Nov 26, 2018

Base on this link https://github.com/Azure/SONiC/wiki/Configuration, there are three tables involve when configuring vlan: VLAN, VLAN_MEMBER and VLAN_INTERFACE.
However, in the delete vlan function in https://github.com/Azure/sonic-utilities/blob/master/config/main.py, just two table are deleted, and the VLAN_INTERFACE has been left.

- What I did
Deleting VLAN_INTERFACE table as well when deleting a vlan.

- How I did it
Find the Vlan in VLAN_INTERFACE table and delete that table:
keys = [ (k, v) for k, v in db.get_table('VLAN_INTERFACE') if k == 'Vlan{}'.format(vid)]
for k in keys:
db.set_entry('VLAN_INTERFACE',k,None)

- How to verify it
Config a vlan with 3 tables: VLAN, VLAN_MEMBER and VLAN_INTERFACE as in https://github.com/Azure/SONiC/wiki/Configuration
Checking information in redis before and after delete vlan as below:

root@leaf2-10-58-0-22:/# redis-cli -n 4 keys *Vlan5*

  1. "VLAN_INTERFACE|Vlan5|192.168.5.1/24"
  2. "VLAN_MEMBER|Vlan5|Ethernet16"
  3. "VLAN|Vlan5"

root@leaf2-10-58-0-22:/# config vlan del 5
root@leaf2-10-58-0-22:/# redis-cli -n 4 keys *Vlan5*
(empty list or set)
root@leaf2-10-58-0-22:/#

Deleting VLAN_INTERFACE table as well when deleting a vlan
Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

Please make pull request title more clear and descriptive.

@tientienle
Copy link
Contributor Author

tientienle commented Nov 27, 2018

Updated. Please let me know if anything needs to be more clarified.

@jleveque
Copy link
Contributor

@tientienle: You did not make changes to the PR title. You updated the description. Please make the title more descriptive.

@tientienle tientienle changed the title Update main.py in del_vlan Update main.py in del_vla Nov 28, 2018
@tientienle tientienle changed the title Update main.py in del_vla Fixing bug when delete a vlan Nov 28, 2018
@tientienle tientienle changed the title Fixing bug when delete a vlan Fixing the delete vlan function Nov 28, 2018
@stcheng
Copy link
Contributor

stcheng commented Dec 2, 2018

@tientienle I believe right now the correct flow is to remove the VLAN interface IP first before removing the VLAN itself. What should be enhanced is to indicate the user that the current VLAN interface still has IP configured on it and shall not be removed directly. I don't think it is proper to directly remove the IPs associated with the VLAN in one shot.

@tientienle
Copy link
Contributor Author

@stcheng Thanks for your comment. I've just checked the remove ip address in interface function. That makes sense now. So this is something that users need to be aware of.

mihirpat1 pushed a commit to mihirpat1/sonic-utilities that referenced this pull request Sep 15, 2023
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.

3 participants