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

Switchport Mode & CLI Modified Fix #3247

Merged
merged 97 commits into from
May 10, 2024

Conversation

sabakram
Copy link
Contributor

@sabakram sabakram commented Mar 28, 2024

What I did

This PR is Fixture for #3108 The PR was reverted due to backward compatibility issues. As per new suggestions, removed db migrator changes from this new change along with vlan.py & switchport.py changes

To Fix issues as per suggestions removed default mode from YANG model and removed minigraph changes:

How I did it

   1. Removed Db migrator changes from code. 
   2. Modified Vlan.py & Switchport.py changes 

How to verify it

New commands have been added in Command-Reference.md All the syntax and examples have been added there and they can be verified by running the specific command

@sabakram
Copy link
Contributor Author

sabakram commented Apr 2, 2024

Hi @wen587, can you please review this PR. This is in reference to PR.

We have modified code. We removed Db-migrator changes along with updated vlan.py & switchport.py to remove default "routed" mode behavior.

config/main.py Outdated Show resolved Hide resolved
tests/vlan_test.py Outdated Show resolved Hide resolved
@ridahanif96
Copy link
Contributor

Hi @theasianpianist , @prsunny @venkatmahalingam can you please help review this PR.

wen587
wen587 previously approved these changes Apr 3, 2024
Copy link
Contributor

@wen587 wen587 left a comment

Choose a reason for hiding this comment

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

lgtm. Please check with other reviewer.

@ridahanif96
Copy link
Contributor

Hi @theasianpianist , @prsunny @venkatmahalingam can you please help review this PR.

Hi All,

Can you help with review this PR. Thanks.

theasianpianist
theasianpianist previously approved these changes Apr 8, 2024
Copy link
Contributor

@theasianpianist theasianpianist left a comment

Choose a reason for hiding this comment

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

Vlan changes lgtm

@ridahanif96
Copy link
Contributor

Hi @prsunny & @venkatmahalingam , can you please help with review. Thanks in advance,

@prsunny
Copy link
Contributor

prsunny commented Apr 10, 2024

Can you please confirm there is no changes to existing 'show' commands? I see there is a new show CLI command which is ok.

@prsunny
Copy link
Contributor

prsunny commented Apr 10, 2024

Please confirm, this issue is fixed in this PR - sonic-net/sonic-buildimage#18392

@sabakram
Copy link
Contributor Author

Can you please confirm there is no changes to existing 'show' commands? I see there is a new show CLI command which is ok.

Hi @prsunny , we didn't make any change in exisiting show commands i.e in "show vlan brief" & "show int status". We proposed new show commands for switchport mode i.e "show int switchport config" & "show int switchport status"

@sabakram
Copy link
Contributor Author

Please confirm, this issue is fixed in this PR - sonic-net/sonic-buildimage#18392

Yes, We have checked this change with latest build. Please see behavior
ShowVersion
Cffgen_2
Cffgen_l2
Portchannel

@sabakram
Copy link
Contributor Author

Hi @wen587 Do you also suggest change for this PR in sonic_mgmt/gcu/test_vlan.py?

prsunny
prsunny previously approved these changes Apr 11, 2024
@ridahanif96
Copy link
Contributor

@venkatmahalingam Hi Venkat, can you pls help review this PR.


def vlan_range_list(ctx, vid_range: str) -> list:

vid1, vid2 = map(int, vid_range.split("-"))
Copy link
Contributor

@qiluo-msft qiluo-msft Apr 11, 2024

Choose a reason for hiding this comment

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

vid_range

will it throw if there is no "-" in vid_range? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @qiluo-msft, this function is only called if "-" is present in the argument passed to the VLAN command. If there is no "-" character in the argument provided, then it will be considered as a comma-separated list instead, not as a range. This behavior can be seen in the multiple_vlan_parser() function defined on line 289 in utilities-common/cli.py

@qiluo-msft
Copy link
Contributor

Waiting on @prsunny approval.

@@ -4639,19 +4645,38 @@ def add(ctx, interface_name, ip_addr, gw):
if interface_name is None:
ctx.fail("'interface_name' is None!")

# Add a validation to check this interface is not a member in vlan before
# changing it to a router port
vlan_member_table = config_db.get_table('VLAN_MEMBER')
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this checked now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have added a check for switchport mode routed, a routed port fulfill this behavior that a interface has a vlan membership or not. So this check becomes redundant after addition of new check.

@qiluo-msft qiluo-msft merged commit a01a0a6 into sonic-net:master May 10, 2024
7 checks passed
arfeigin pushed a commit to arfeigin/sonic-utilities that referenced this pull request Jun 16, 2024
This PR is Fixture for sonic-net#3108 The PR was reverted due to backward compatibility issues. As per new suggestions, removed db migrator changes from this new change along with vlan.py & switchport.py changes

To Fix issues as per suggestions removed default mode from YANG model and removed minigraph changes:

       1. Removed Db migrator changes from code.
       2. Modified Vlan.py & Switchport.py changes

New commands have been added in Command-Reference.md  All the syntax and examples have been added there and they can be verified by running the specific command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants