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

Subinterface vrf bind issue fix #2211

Merged
merged 13 commits into from
Aug 29, 2022
Merged

Conversation

preetham-singh
Copy link
Contributor

What I did

This commit addresses below issues:

  1. Fixes #10802 : Sub-interface created using short parent interface name failed to bind to VRF sonic-buildimage#10802
  2. Fixes #10878 : [Sub-Interface] CLI should throw an error if short name format is called without vlan when adding a sub-interface sonic-buildimage#10878

How I did it

How to verify it

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

@lgtm-com
Copy link

lgtm-com bot commented Jun 11, 2022

This pull request introduces 1 alert and fixes 1 when merging 380339c into a3d1345 - view on LGTM.com

new alerts:

  • 1 for Syntax error

fixed alerts:

  • 1 for Unused local variable

Copy link
Collaborator

@dgsudharsan dgsudharsan left a comment

Choose a reason for hiding this comment

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

Please add UT to cover both the fixes

config/main.py Outdated Show resolved Hide resolved
config/main.py Show resolved Hide resolved
config/main.py Outdated
if interface_type == "VLAN_SUB_INTERFACE":
subintf = config_db.get_entry(interface_type, alias)
if 'vrf_name' in subintf_entry:
subintf_entry.pop('vrf')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain the logic here? Why should vrf be popped?

config/main.py Show resolved Hide resolved
@dgsudharsan
Copy link
Collaborator

Can you please add Unit tests?

@dgsudharsan
Copy link
Collaborator

@preetham-singh @adyeung Can you please share ETA to add the unit tests?

@preetham-singh
Copy link
Contributor Author

preetham-singh commented Jul 12, 2022

@preetham-singh @adyeung Can you please share ETA to add the unit tests?

Sure, I will update tunit tests by 7/15.

format
Update "show subniterface status" to reflect subinterface in user
configured long name and short name format.
1. Short Format Subinterface getting deleted after vrf binding . Issue
10802:sonic-net/sonic-buildimage#10802
2. Do not allow short format subinterface to be created without encap
vlan configuration. Issue 10878:
sonic-net/sonic-buildimage#10878
1. Short Format Subinterface getting deleted after vrf binding . Issue
10802:sonic-net/sonic-buildimage#10802
2. Do not allow short format subinterface to be created without encap
vlan configuration. Issue 10878:
sonic-net/sonic-buildimage#10878
instead update existing subinterface.
Updating unit testcases for subinterfaces.
@dgsudharsan
Copy link
Collaborator

@preetham-singh Can you please check the coverage failure?

@dgsudharsan
Copy link
Collaborator

Hi @adyeung @preetham-singh Can you please solve the build failure?

@zhangyanzhao
Copy link
Collaborator

Adam will follow up. Thanks

@preetham-singh
Copy link
Contributor Author

Working with SONiC team to resolve code coverage issue for VRF BIND CLI. I will update the PR once code coverage scripts are updated for VRF Bind CLI.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 12, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@preetham-singh
Copy link
Contributor Author

This test failure should get resolved once PR #2309 is merged.

@preetham-singh
Copy link
Contributor Author

/azpw run Azure.sonic-utilities

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-utilities

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prsunny prsunny merged commit 899ba12 into sonic-net:master Aug 29, 2022
yxieca pushed a commit that referenced this pull request Sep 1, 2022
* Add support to configure routed subinterface in short name and long name format
* Update "show subniterface status" to reflect subinterface in user configured long name and short name format.
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Sep 7, 2022
Update sonic-utilities submodule pointer to include the following:
* [route_check]: Ignore standalone tunnel routes (sonic-net#2325) ([sonic-net#2346](sonic-net/sonic-utilities#2346))
* [VRF]Adding CLI checks to ensure Vrf is valid in interface bind and static route commands ([sonic-net#2333](sonic-net/sonic-utilities#2333))
* Subinterface vrf bind issue fix ([sonic-net#2211](sonic-net/sonic-utilities#2211))
* [decode-syseeprom] Fix setting use_db based on support_eeprom_db ([sonic-net#2270](sonic-net/sonic-utilities#2270))
* Fix vrf UT failed issue ([sonic-net#2309](sonic-net/sonic-utilities#2309))

Signed-off-by: dprital <drorp@nvidia.com>
yxieca pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Sep 8, 2022
Update sonic-utilities submodule pointer to include the following:
* [route_check]: Ignore standalone tunnel routes (#2325) ([#2346](sonic-net/sonic-utilities#2346))
* [VRF]Adding CLI checks to ensure Vrf is valid in interface bind and static route commands ([#2333](sonic-net/sonic-utilities#2333))
* Subinterface vrf bind issue fix ([#2211](sonic-net/sonic-utilities#2211))
* [decode-syseeprom] Fix setting use_db based on support_eeprom_db ([#2270](sonic-net/sonic-utilities#2270))
* Fix vrf UT failed issue ([#2309](sonic-net/sonic-utilities#2309))

Signed-off-by: dprital <drorp@nvidia.com>

Signed-off-by: dprital <drorp@nvidia.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.

6 participants