-
Notifications
You must be signed in to change notification settings - Fork 710
[VRF]: submit vrf CLI #392 #558
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
Conversation
1. support bind to a new vrf even when had binded or ipaddress 2. do nothing when bind to same vrf or unbind at unbinded status
show/main.py
Outdated
@@ -1000,7 +1056,7 @@ def ipv6(): | |||
@ipv6.command() | |||
def interfaces(): | |||
"""Show interfaces IPv6 address""" | |||
header = ['Interface', 'IPv6 address/mask', 'Admin/Oper'] | |||
header = ['Interface', 'Master', 'IPv6 address/mask', 'Admin/Oper'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have 'VRF' header instead of 'Master'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 'Master' may be more precise, while device name contains Vrf prefix either.
|
||
table_name = get_interface_table_name(interface_name) | ||
if table_name == "": | ||
ctx.fail("'interface_name' is not valid. Valid names [Ethernet/PortChannel/Vlan/Loopback]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add any support for mgmt intf bind to mgmt VRF (if not present already) as part of this pull request or that should be handled separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now mgmt intf vrf has its own CLI.
state_db = SonicV2Connector(host='127.0.0.1') | ||
state_db.connect(state_db.STATE_DB, False) | ||
_hash = '{}{}'.format('INTERFACE_TABLE|', interface_name) | ||
while state_db.get(state_db.STATE_DB, _hash, "state") == "ok": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this handling? dont we handle this in VRF-mgrd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In intfmgrd. It is a good question. The codes here is to assure DEL op has been handled, or else the new SET op will overwite DEL op and then we have to handle complex intf-move-vrf in intfsorch.
# | ||
# 'vrf' subgroup ('config interface vrf ...') | ||
# | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hope we delete the dependent IP, VRF configs upon deleting the portchannel and Vlan...etc? or we expect the user the clean-up the dependent configs before deleting the interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleting dependent configs implicitly can help user to simplify operation, actually it makes CLI implementation more complex.
@tylerlinp , can you rebase to latest code. MGMT VRF PR is merged, so request to resolve conflicts. |
retest this please |
2 similar comments
retest this please |
retest this please |
config_db.set_entry("INTERFACE", interface_name, {"NULL": "NULL"}) | ||
elif interface_name == 'eth0': | ||
|
||
if interface_name == 'eth0': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest to don't hardcode here and below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has nothing to do with this PR, it is just merged from master. If to change, a new PR is needed. I think mgmt intf should be handled in another command.
@tylerlinp , can you please resolve conflict? |
retest this please |
sonic-swss-build/264 build failed and sonic-swss-build/260 not including PR swss/943, so this PR test failed. |
retest this please |
2 similar comments
retest this please |
retest this please |
@tylerlinp , can you please check the VS test failures |
retest this please |
- What I did
Submit VRF CLI according to vrf HLD #392
- How I did it
- How to verify it
Pass the test on nephos lab
- 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)
-->