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

Add IP remove warnings for VRF commands #2351

Merged
merged 4 commits into from
Nov 1, 2022

Conversation

hamnarauf
Copy link
Contributor

@hamnarauf hamnarauf commented Sep 6, 2022

What I did

Added IP address remove warnings when performing various VRF commands i.e.,
- vrf bind
- vrf unbind
- del vrf

How I did it

  • Modify config/main.py to include click.echo warnings.
  • Add test cases for interface/subinterface vrf bind, unbind and del.

How to verify it

Tested the changes locally by building sonic-utilities Python wheel package inside Bullseye slave container

mdanish@3be53d98ab67:/sonic/src/sonic-utilities$ pytest-3 tests/show_vrf_test.py
==================================================================== test session starts =====================================================================
platform linux -- Python 3.9.2, pytest-6.0.2, py-1.10.0, pluggy-0.13.0
rootdir: /sonic/src/sonic-utilities/tests, configfile: pytest.ini
plugins: pyfakefs-5.0.0, cov-2.10.1
collected 3 items                                                                                                                                            

tests/show_vrf_test.py ....                                                                                                                            [100%]

===================================================================== 3 passed in 0.36s ======================================================================
mdanish@3be53d98ab67:/sonic/src/sonic-utilities$ pytest-3 tests/subintf_test.py 
==================================================================== test session starts =====================================================================
platform linux -- Python 3.9.2, pytest-6.0.2, py-1.10.0, pluggy-0.13.0
rootdir: /sonic/src/sonic-utilities/tests, configfile: pytest.ini
plugins: pyfakefs-5.0.0, cov-2.10.1
collected 7 items                                                                                                                                            

tests/subintf_test.py .......                                                                                                                          [100%]

===================================================================== 7 passed in 0.50s ======================================================================
mdanish@3be53d98ab67:/sonic/src/sonic-utilities$ 

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

Example output with interface name Ethernet0 and vrf name Vrf1:

admin@sonic:~$ sudo config interface vrf bind Ethernet0 Vrf1
admin@sonic:~$ sudo config interface vrf unbind Ethernet0
admin@sonic:~$ sudo config vrf del Vrf1

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

Example output with interface name Ethernet0 and vrf name Vrf1:

admin@sonic:~$ sudo config interface vrf bind Ethernet0 Vrf1
Interface Ethernet0 IPv4 disabled and address(es) removed due to binding VRF Vrf1
admin@sonic:~$ sudo config interface vrf unbind Ethernet0
Interface Ethernet0 IPv4 disabled and address(es) removed due to unbinding VRF
admin@sonic:~$ sudo config vrf del Vrf1
VRF Vrf1 deleted and all associated IPv4 addresses removed.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 6, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: hamnarauf / name: Hamna Rauf (3ea09a0)

@hamnarauf hamnarauf force-pushed the ip-remove-warnings branch 2 times, most recently from 6ee4e17 to df4a565 Compare September 6, 2022 10:56
@hamnarauf hamnarauf closed this Oct 13, 2022
@hamnarauf hamnarauf reopened this Oct 13, 2022
@mdanish-kh mdanish-kh force-pushed the ip-remove-warnings branch 3 times, most recently from 5cbd5ea to 0e1b0cf Compare October 22, 2022 19:50
@hamnarauf hamnarauf marked this pull request as draft October 23, 2022 12:57
@hamnarauf hamnarauf closed this Oct 24, 2022
@hamnarauf hamnarauf reopened this Oct 24, 2022
@hamnarauf hamnarauf marked this pull request as ready for review October 24, 2022 05:17
@hamnarauf
Copy link
Contributor Author

@prsunny @qiluo-msft @dgsudharsan @preetham-singh Please review.

prsunny
prsunny previously approved these changes Oct 24, 2022
@prsunny
Copy link
Contributor

prsunny commented Oct 24, 2022

@prsunny @qiluo-msft @dgsudharsan @preetham-singh Please review.

looks ok to me but would like to know the motivation of the PR with such warnings? Giving such a message may fail the return value check for services invoking this function. I'll wait for @dgsudharsan and @preetham-singh to sign-off before merge?

config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
@hamnarauf
Copy link
Contributor Author

looks ok to me but would like to know the motivation of the PR with such warnings? Giving such a message may fail the return value check for services invoking this function. I'll wait for @dgsudharsan and @preetham-singh to sign-off before merge?

The intention is to print a helpful message that informs the user in case IP gets removed by the system from an interface to avoid user pit falling into an inconsistent configuration state. In my opinion, the user needs to be informed promptly to avoid ambiguity. Such helpful messages are present in other SONiC CLI commands as well.

@hamnarauf
Copy link
Contributor Author

@prsunny @preetham-singh Please review again

@hamnarauf hamnarauf requested a review from prsunny October 31, 2022 08:07
@prsunny prsunny merged commit e8b1dcd into sonic-net:master Nov 1, 2022
@mdanish-kh mdanish-kh deleted the ip-remove-warnings branch November 1, 2022 03:53
preetham-singh pushed a commit to preetham-singh/sonic-utilities that referenced this pull request Nov 21, 2022
Added IP address remove warnings when performing various VRF commands i.e.,
- vrf bind
- vrf unbind
- del vrf
mdanish-kh pushed a commit to mdanish-kh/sonic-utilities that referenced this pull request Nov 23, 2022
Added IP address remove warnings when performing various VRF commands i.e.,
- vrf bind
- vrf unbind
- del vrf
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.

5 participants