[sonic-utilities] managementVRF cli support(l3mdev) #463

vharish02 · 2019-02-18T16:57:02Z

This commit adds CLI support for management VRF using l3dev. mVRF can
be enabled using config vrf add mgmt and deleted using config vrf del mgmt.
Show commands for management VRF are added which displays the Linux command
output, will update show command display after concluding what would be the
output for the show commands.
Added cli to configure management interface(eth0), config interface ip eth0
add can be used to configure eth0 ip and config ip eth0 remove is used to
remove eth0 ip.

New cli config/show commands:

config vrf add mgmt
config vrf del mgmt
config interface eth0 ip add ip/mask gatewayIP
config interface eth0 ip remove ip/mask

show mgmt-vrf
show mgmt-vrf route
show mgmt-vrf addresses
show mgmt-vrf interfaces

Signed-off-by: Harish Venkatraman [email protected]

- What I did
Added CLI support for management VRF
- How I did it
Added following new CLI commands to enable and disable management VRF using l3mdev and configure eth0 management interface ip address.

config commands:
config vrf add mgmt
config vrf del mgmt
config interface ip add eth0 ip/mask gatewayIP
config interface ip remove eth0 ip/mask

show commands:
show mgmt-vrf 
show mgmt-vrf routes

Modified show ntp command

- How to verify it
Configure management VRF using the above config command and use the show commands to display the output of mgmt-vrf interfaces, addresses, route and ntp.

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

-->
SAMPLE OUTPUTS OF SHOW COMMANDS
show mgmt-vrf

root@sonic:/etc/init.d# show mgmt-vrf 

ManagementVRF : Enabled

Management VRF interfaces in Linux:
348: mgmt: <NOARP,MASTER,UP,LOWER_UP> mtu 65536 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether f2:2a:d9:bc:e8:f0 brd ff:ff:ff:ff:ff:ff
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq master mgmt state UP mode DEFAULT group default qlen 1000
    link/ether 4c:76:25:f4:f9:f3 brd ff:ff:ff:ff:ff:ff
350: lo-m: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue master mgmt state UNKNOWN mode DEFAULT group default qlen 1000
    link/ether b2:4c:c6:f3:e9:92 brd ff:ff:ff:ff:ff:ff
root@sonic:/etc/init.d#

show mgmt-vrf routes

root@sonic:/etc/init.d# show mgmt-vrf routes

Routes in Management VRF Routing Table:
default via 10.16.210.254 dev eth0 metric 201 
broadcast 10.16.210.0 dev eth0 proto kernel scope link src 10.16.210.75 
10.16.210.0/24 dev eth0 proto kernel scope link src 10.16.210.75 
local 10.16.210.75 dev eth0 proto kernel scope host src 10.16.210.75 
broadcast 10.16.210.255 dev eth0 proto kernel scope link src 10.16.210.75 
broadcast 127.0.0.0 dev lo-m proto kernel scope link src 127.0.0.1 
127.0.0.0/8 dev lo-m proto kernel scope link src 127.0.0.1 
local 127.0.0.1 dev lo-m proto kernel scope host src 127.0.0.1 
broadcast 127.255.255.255 dev lo-m proto kernel scope link src 127.0.0.1 
root@sonic:/etc/init.d#

show management_interface address

root@sonic:/etc/init.d# show management_interface address 
Management IP address = 10.16.210.75/24
Management NetWork Default Gateway = 10.16.210.254
root@sonic:/etc/init.d#

This commit adds CLI support for management VRF using l3dev. mVRF can be enabled using config vrf add mgmt and deleted using config vrf del mgmt. Show commands for management VRF are added which displays the linux command output, will update show command display after concluding what would be the output for the show commands. Added cli to configure management interface(eth0), config interface ip eth0 add can be used to configure eth0 ip and config ip eth0 remove is used to remove eth0 ip. New cli config/show commands: config vrf add mgmt config vrf del mgmt config interface eth0 ip add ip/mask gatewayIP config interface eth0 ip remove ip/mask show mgmt-vrf show mgmt-vrf route show mgmt-vrf addresses show mgmt-vrf interfaces Signed-off-by: Harish Venkatraman <[email protected]>

a-barboza · 2019-07-30T00:09:59Z

What relationship exists between managementVRF and VRF feature ?
There is a VRF feature which has its CLI. (https://github.com/Azure/SONiC/blob/master/doc/sonic-vrf-hld.md). Are there any CLIs which are common ?
How can i add the VRF folks to this review ?

prsunny · 2019-07-30T19:04:40Z

What relationship exists between managementVRF and VRF feature ?
There is a VRF feature which has its CLI. (https://github.com/Azure/SONiC/blob/master/doc/sonic-vrf-hld.md). Are there any CLIs which are common ?
How can i add the VRF folks to this review ?

@a-barboza, these are two different features implemented separately. This CLI is being reviewed by the VRF work-group and checked for any commonalities.

a-barboza · 2019-07-30T22:49:14Z

What relationship exists between managementVRF and VRF feature ?
There is a VRF feature which has its CLI. (https://github.com/Azure/SONiC/blob/master/doc/sonic-vrf-hld.md). Are there any CLIs which are common ?
How can i add the VRF folks to this review ?

@a-barboza, these are two different features implemented separately. This CLI is being reviewed by the VRF work-group and checked for any commonalities.

@prsunny Looks like there are quire a few commonalities (Eg: config vrf add, ...). So it is likely the patch will be redone.

kannankvs · 2019-09-11T07:47:01Z

@jleveque : Resolved the conflict. Request you to kindly review and merge it.

kannankvs · 2019-09-19T20:52:35Z

@prsunny and @jleveque : Anything else required on this? Or, can it be merged?

a-barboza · 2019-09-24T23:47:06Z

Is there a location where the table # of 5000 use for mgmt-vrf is mentioned ? I checked the mgmt-vrf HLD document but could not find a reference other than "1" in the example linux commands.

kannankvs · 2019-09-25T18:35:45Z

@a-barboza : We shall update the document once if the PR is merged. Just waiting to see if there could be any changes in the CLI before merging based on any review comments.

kannankvs · 2019-09-25T19:07:00Z

@a-barboza : Some of your questions from PR472 that are relevant for this PR are answered here.

Why restart NTP ? (These changes are for SNMP ?)
<> This is not part of SNMP PR. This is part of this 463 PR. Once if 463 is merged, 472 PR will contain only mvrf SNMP CLI changes.
clear_mgmt command: There are some new commands common to all mgmt-vrf ?
MGMT_INTERFACE, MGMT_VRF_CONFIG tables ?
<> This command is an easy way to clear both the tables.
There is a new "--gw" option? Why is this only for eth0 ? Can Front Panel (In-Band) port be part of the mgmt-vrf ?
<> As of now, eth0 alone will be part of mvrf; not front panel ports. This "gw" is to add default route for mvrf which is already there in config_db as part of MGMT_INTERFACE. We just added CLI for supporting that. This CLI enhancement is only for mvrf and hence other non mvrf options are not being touched.
Is the table Updated natshow script to support DNAT Pool changes #1001 reserved for mgmt-vrf, or is it Table# 1 ?
<> Table 5000 is reserved for mvrf. CLI in this PR is already corrected to use 5000 (not 1001).
Will be replied in PR472.
Is the Curl, Wget, SSH also supported for mgmt-vrf ?
<> Yes, all applications are supported for mgmt-vrf. Those applications do not need any enhancement. Users can do "cgexec -g l3mdev:mgmt COMMAND" for running any application from the device. Similarly, users can establish any application connections to the device from external network as well.
Where is the mgmtvrf_ntpq command ?
<> mgmtvrf_ntpq is removed now in this PR463. Once if this PR463 is merged, same changes will be there in 472.

a-barboza · 2019-09-25T21:44:06Z

@kannankvs: thanks for the replies. For answer 3 above: Will the HLD for management vrf be updated to state the following " As of now, eth0 alone will be part of mvrf; not front panel ports" ?
Currently the HLD has use cases for In-band management.

kannankvs · 2019-09-26T06:35:41Z

@a-barboza : Yes, we will update it.

prsunny · 2019-10-02T00:13:42Z

config/main.py

+	config_db.connect()
+        entry = config_db.get_entry('MGMT_VRF_CONFIG', "vrf_global")
+        if entry and entry['mgmtVrfEnabled'] == 'true' :
+	    click.echo("ManagementVRF is already Enabled.")


Can you fix the alignment here? Looks like tab spaces

prsunny · 2019-10-02T00:17:20Z

config/main.py

+	    click.echo("ManagementVRF is already Enabled.")
+	    return None
+	config_db.mod_entry('MGMT_VRF_CONFIG',"vrf_global",{"mgmtVrfEnabled": "true"})
+        cmd="service ntp stop"


Can you add this to a function and give a comment why this has to be restarted? Also in future if another service is required to be restarted, it can be in one place. Currently I see this repeated

Done. Moved it to a function and provided the required comments.

prsunny · 2019-10-02T00:20:33Z

config/main.py

+
+@config.command('clear_mgmt')
+@click.pass_context
+def clear_mgmt(ctx):


What is the purpose of this? Is the backend code handling this? What if the user configures management VRF and then do a clear?

clear_mgmt was earlier indented for easy deletion of configured values. Now that we have required "del/remove" functions, it is not required. It is removed now.

prsunny · 2019-10-02T00:30:57Z

config/main.py

+
+            for key in mgmtintf_key_list:
+                # For loop runs for max 2 rows, once for IPv4 and once for IPv6.
+                if ':' in ip_addr and ':' in key[1]:


Please use version from IPAddress to check for IPv4 and IPv6.

Done. When mvrf was implemented in Feb2019, "ipaddress" module did not have the required path and hence it was unable to import. Now, the "ipaddress" module is available and it is already being used in main.py. It is now used in this function to check the version.

prsunny · 2019-10-02T00:31:43Z

config/main.py

@@ -991,6 +1108,12 @@ def remove(ctx, interface_name, ip_addr):
        if interface_name.startswith("Ethernet"):
            config_db.set_entry("INTERFACE", (interface_name, ip_addr), None)
            if_table = "INTERFACE"
+        elif interface_name == 'eth0':
+            config_db.set_entry("MGMT_INTERFACE", (interface_name, ip_addr), None)
+            cmd="systemctl restart interfaces-config"


As mentioned above, this must be in a separate function to avoid duplication

prsunny · 2019-10-02T00:58:35Z

show/main.py

+
+
+@mgmt_vrf.command('addresses')
+def mgmt_vrf_addresses ():


This is confusing to show 'addresses' and 'address'. Can you paste both output. I think this can be removed and just use address in L493

Removed the confusing command. Like you said "show management_interface address" is good enough to display the configure address for eth0.

prsunny · 2019-10-02T00:59:21Z

show/main.py

-    cmd = "ntpq -p -n"
-    run_command(cmd, display_cmd=verbose)
+    ntpcmd = "ntpq -p -n"
+    if ctx.invoked_subcommand is None:


As mentioned above, please use a function to check if mgmt vrf is enabled.

Not yet done, please address the earlier comment

While resolving conflict, change got overwritten. Its resolved now. Apologies.

config/main.py

prsunny · 2019-10-02T01:01:58Z

config/main.py

+    """VRF-related configuration tasks"""
+    pass
+
+


Please remove extra line

kannankvs · 2019-10-03T22:31:53Z

@prsunny : Comments are addressed. Request you to review and merge.

config/main.py

prsunny · 2019-10-04T00:34:57Z

config/main.py

+        click.echo("ManagementVRF is already Enabled.")
+        return None
+    config_db.mod_entry('MGMT_VRF_CONFIG',"vrf_global",{"mgmtVrfEnabled": "true"})
+    # To move eth0 to management VRF, interfaces-config service has be to be 


Please move this comments to the below function. This is repeated below as well. Also please follow the multi-line comment format

Actually the comments are slightly different for enable and disable conditions. That is why it was provided at this place. Now, the comments are moved inside the function and reworded to suit for both enable and disable conditions. By multi-line, we assume you meant the usage of """, which is also done now.

prsunny · 2019-10-04T00:40:42Z

config/main.py

+    return config_db.get_table('MGMT_INTERFACE').keys()
+
+
+def restart_interfaces_config_ntp_config():


What is the difference between this and mvrf_restart_interfaces_config_ntp? Can we use the same?

There is a difference. One is restarting the "ntp-config" service and the other is restarting "ntp" service. "ntp-config" service will regenerate the ntp.conf file, which is required when the IP address for eth0 is modified. Same is not required when VRF is enabled or disabled.

config/main.py

prsunny · 2019-10-04T00:44:06Z

config/main.py

+                # No need to capture the exception since the ip_addr is already validated earlier
+                ip_input = ipaddress.ip_interface(ip_addr)
+                current_ip = ip = ipaddress.ip_interface(key[1])
+                if (ip_input.version == 6) and (current_ip.version == 6):


if (ip_input.version == current_ip.version) will cover both cases right?

yes, it will cover. Changed.

prsunny · 2019-10-04T00:46:53Z

show/main.py

+
+        p = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        res = p.communicate()
+        if p.returncode == 0 :


Extra space after 0

prsunny · 2019-10-04T00:48:23Z

show/main.py

+            mvrf_dict = json.loads(p.stdout.read())
+
+            # if the mgmtVrfEnabled attribute is configured, check the value
+            # and print Enabled or Disabled accordingly.


You may want to rephrase the comment here.

prsunny · 2019-10-04T00:49:49Z

show/main.py

+    """Show management VRF attributes"""
+
+    if is_mgmt_vrf_enabled(ctx) is False:
+        click.echo("\nKVSK:ManagementVRF : Disabled")


prsunny · 2019-10-04T00:52:00Z

show/main.py

+    mgmt_ip_data = config_db.get_table('MGMT_INTERFACE')
+    for key in natsorted(mgmt_ip_data.keys()):
+        click.echo("Management IP address = {0}".format(key[1]))
+        click.echo("Management NetWork Default Gateway = {0}".format(mgmt_ip_data[key]['gwaddr']))


prsunny · 2019-10-04T00:59:31Z

show/main.py

-    cmd = "ntpq -p -n"
-    run_command(cmd, display_cmd=verbose)
+    ntpcmd = "ntpq -p -n"
+    if ctx.invoked_subcommand is None:


Not yet done, please address the earlier comment

kannankvs · 2019-10-04T08:17:52Z

@prsunny : Addressed the new comments as well.

config/main.py

wendani · 2020-01-24T21:37:53Z

Shall "MGMT_INTERFACE|eth0" be updated into the vrf "mgmt" in the CONFIG_DB when doing sudo config vrf add mgmt according to the VRF schema convention?

    "MGMT_INTERFACE": {
        "eth0": {
            "vrf_name": "mgmt"
        }
        "eth0|10.2.17.103/23": {
            "gwaddr": "10.2.16.1"
        }
    },

prsunny · 2020-01-24T21:54:40Z

Thats right Wenda

vharish02 force-pushed the managementVRF_cli_support branch from c69c660 to fcdec1b Compare June 26, 2019 12:20

vharish02 force-pushed the managementVRF_cli_support branch from fcdec1b to c7917f1 Compare July 16, 2019 17:27

vharish02 force-pushed the managementVRF_cli_support branch from c7917f1 to afcfc4b Compare July 16, 2019 19:01

This was referenced Aug 30, 2019

NTP support for management VRF sonic-net/sonic-buildimage#3400

Closed

Management vrf ntp support sonic-net/sonic-buildimage#3204

Merged

kannankvs added 2 commits September 11, 2019 13:11

updated to resolve the conflict on show/main.py

f83b462

missing empty line w.r.t conflict added.

14cc90f

table number corrected.

4152cf3

jleveque requested a review from prsunny September 11, 2019 18:49

kannankvs mentioned this pull request Sep 25, 2019

Management vrf snmp cli support #472

Merged

prsunny reviewed Oct 2, 2019

View reviewed changes

kannankvs added 4 commits October 3, 2019 14:18

addressed review comments

ac1a3c1

resolving conflict

c5816e7

resolving conflict2

c8351a5

resolving conflict3

ad030ff

prsunny reviewed Oct 4, 2019

View reviewed changes

addressed the comments2

30fea2e

renamed functions

c4c0124

renamed eth0_ip to mgmt_ip

f65c733

prsunny approved these changes Oct 4, 2019

View reviewed changes

jleveque suggested changes Oct 4, 2019

View reviewed changes

config/main.py Outdated Show resolved Hide resolved

config/main.py Outdated Show resolved Hide resolved

config/main.py Outdated Show resolved Hide resolved

modified few comments for Docstring and remove unwanted import

ea811a5

jleveque approved these changes Oct 4, 2019

View reviewed changes

prsunny merged commit 3247828 into sonic-net:master Oct 4, 2019

prsunny mentioned this pull request Oct 8, 2019

[VRF]: submit vrf CLI #392 #558

Merged

a-barboza mentioned this pull request Nov 13, 2019

[SONIC] Management VRF design doc sonic-net/SONiC#226

Open

		return config_db.get_table('MGMT_INTERFACE').keys()


		def restart_interfaces_config_ntp_config():

[sonic-utilities] managementVRF cli support(l3mdev) #463

[sonic-utilities] managementVRF cli support(l3mdev) #463

Conversation

vharish02 commented Feb 18, 2019 • edited by prsunny Loading

a-barboza commented Jul 30, 2019

prsunny commented Jul 30, 2019

a-barboza commented Jul 30, 2019

kannankvs commented Sep 11, 2019

kannankvs commented Sep 19, 2019

a-barboza commented Sep 24, 2019

kannankvs commented Sep 25, 2019

kannankvs commented Sep 25, 2019

a-barboza commented Sep 25, 2019

kannankvs commented Sep 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kannankvs commented Oct 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kannankvs commented Oct 4, 2019

wendani commented Jan 24, 2020

prsunny commented Jan 24, 2020

vharish02 commented Feb 18, 2019 •

edited by prsunny

Loading