Skip to content

[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

Merged
merged 7 commits into from
Nov 26, 2019
Merged

[VRF]: submit vrf CLI #392 #558

merged 7 commits into from
Nov 26, 2019

Conversation

tylerlinp
Copy link
Contributor

- 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)

show ip interfaces
Interface    IPv4 address/mask    Admin/Oper
-----------  -------------------  ------------
Ethernet0    1.1.1.1/24           up/up

show ipv6 interfaces
Interface    IPv6 address/mask                       Admin/Oper
-----------  --------------------------------------  ------------
Ethernet0    fe80::6eec:5aff:fe08:164b%Ethernet0/64  up/up

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

show ip interfaces
Interface    Master    IPv4 address/mask    Admin/Oper
-----------  --------  -------------------  ------------
Ethernet0    Vrf-blue  1.1.1.1/24           up/up

show ipv6 interfaces
Interface    Master    IPv6 address/mask                       Admin/Oper
-----------  --------  --------------------------------------  ------------
Ethernet0    Vrf-blue  fe80::6eec:5aff:fe08:164b%Ethernet0/64  up/up

-->

Tyler Li added 3 commits June 11, 2019 05:21
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']
Copy link
Contributor

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'?

Copy link
Contributor Author

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]")
Copy link
Contributor

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?

Copy link
Contributor Author

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":
Copy link
Contributor

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?

Copy link
Contributor Author

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 ...')
#

Copy link
Contributor

@venkatmahalingam venkatmahalingam Sep 16, 2019

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?

Copy link
Contributor Author

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.

@prsunny
Copy link
Contributor

prsunny commented Oct 8, 2019

@tylerlinp , can you rebase to latest code. MGMT VRF PR is merged, so request to resolve conflicts.

@tylerlinp
Copy link
Contributor Author

retest this please

2 similar comments
@shine4chen
Copy link
Contributor

retest this please

@tylerlinp
Copy link
Contributor Author

retest this please

config_db.set_entry("INTERFACE", interface_name, {"NULL": "NULL"})
elif interface_name == 'eth0':

if interface_name == 'eth0':
Copy link
Collaborator

@keboliu keboliu Oct 25, 2019

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

Copy link
Contributor Author

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.

@prsunny
Copy link
Contributor

prsunny commented Nov 6, 2019

@tylerlinp , can you please resolve conflict?

@tylerlinp
Copy link
Contributor Author

retest this please

@tylerlinp
Copy link
Contributor Author

sonic-swss-build/264 build failed and sonic-swss-build/260 not including PR swss/943, so this PR test failed.

@prsunny
Copy link
Contributor

prsunny commented Nov 12, 2019

retest this please

2 similar comments
@prsunny
Copy link
Contributor

prsunny commented Nov 13, 2019

retest this please

@prsunny
Copy link
Contributor

prsunny commented Nov 13, 2019

retest this please

@prsunny
Copy link
Contributor

prsunny commented Nov 14, 2019

@tylerlinp , can you please check the VS test failures

@tylerlinp
Copy link
Contributor Author

retest this please

@prsunny prsunny merged commit 1b5679c into sonic-net:master Nov 26, 2019
@tylerlinp
Copy link
Contributor Author

@cony7642 Oh, that's a issue after this PR merged. I ignored same command defined in 2 PRs.
@prsunny My suggestion is change mgmt vrf commands

config vrf add mgmt
config vrf del mgmt

to

config mgmt-vrf add mgmt
config mgmt-vrf del mgmt

and that just like show commands.

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