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

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

Merged
merged 12 commits into from
Oct 4, 2019
127 changes: 125 additions & 2 deletions config/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
import json
import subprocess
import netaddr
import logging
import logging.handlers
jleveque marked this conversation as resolved.
Show resolved Hide resolved
import re
import syslog

Expand Down Expand Up @@ -789,6 +791,86 @@ def del_vlan_member(ctx, vid, interface_name):
db.set_entry('VLAN_MEMBER', (vlan_name, interface_name), None)



prsunny marked this conversation as resolved.
Show resolved Hide resolved
def vrf_add_management_vrf():
"""Enable management vrf"""

config_db = ConfigDBConnector()
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.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you fix the alignment here? Looks like tab spaces

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done.

return None
config_db.mod_entry('MGMT_VRF_CONFIG',"vrf_global",{"mgmtVrfEnabled": "true"})
cmd="service ntp stop"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

os.system (cmd)
cmd="systemctl restart interfaces-config"
os.system (cmd)
cmd="service ntp start"
os.system (cmd)


prsunny marked this conversation as resolved.
Show resolved Hide resolved
def vrf_delete_management_vrf():
"""Disable management vrf"""

config_db = ConfigDBConnector()
config_db.connect()
entry = config_db.get_entry('MGMT_VRF_CONFIG', "vrf_global")
if not entry or entry['mgmtVrfEnabled'] == 'false' :
click.echo("ManagementVRF is already Disabled.")
return None
config_db.mod_entry('MGMT_VRF_CONFIG',"vrf_global",{"mgmtVrfEnabled": "false"})
cmd="service ntp stop"
os.system (cmd)
cmd="systemctl restart interfaces-config"
os.system (cmd)
cmd="service ntp start"
os.system (cmd)


#
# 'vrf' group ('config vrf ...')
#

@config.group('vrf')
def vrf():
"""VRF-related configuration tasks"""
pass


Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove extra line

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done

@vrf.command('add')
@click.argument('vrfname', metavar='<vrfname>. Type mgmt for management VRF', required=True)
@click.pass_context
def vrf_add (ctx, vrfname):
"""VRF ADD"""
jleveque marked this conversation as resolved.
Show resolved Hide resolved
if vrfname == 'mgmt' or vrfname == 'management':
vrf_add_management_vrf()
else:
click.echo("Creation of data vrf={} is not yet supported".format(vrfname))


@vrf.command('del')
@click.argument('vrfname', metavar='<vrfname>. Type mgmt for management VRF', required=False)
@click.pass_context
def vrf_del (ctx, vrfname):
"""VRF Delete"""
jleveque marked this conversation as resolved.
Show resolved Hide resolved
if vrfname == 'mgmt' or vrfname == 'management':
vrf_delete_management_vrf()
else:
click.echo("Deletion of data vrf={} is not yet supported".format(vrfname))


@config.command('clear_mgmt')
@click.pass_context
def clear_mgmt(ctx):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

MGMT_TABLE_NAMES = [
'MGMT_INTERFACE',
'MGMT_VRF_CONFIG']
config_db = ConfigDBConnector()
config_db.connect()
for mgmt_table in MGMT_TABLE_NAMES:
config_db.delete_table(mgmt_table)

#
# 'bgp' group ('config bgp ...')
#
Expand Down Expand Up @@ -925,6 +1007,14 @@ def speed(ctx, interface_name, interface_speed, verbose):
command += " -vv"
run_command(command, display_cmd=verbose)

def _get_all_mgmtinterface_keys():
"""Returns list of strings containing mgmt interface keys
"""
config_db = ConfigDBConnector()
config_db.connect()
return config_db.get_table('MGMT_INTERFACE').keys()

jleveque marked this conversation as resolved.
Show resolved Hide resolved

#
# 'ip' subgroup ('config interface ip ...')
#
Expand All @@ -942,8 +1032,9 @@ def ip(ctx):
@ip.command()
@click.argument('interface_name', metavar='<interface_name>', required=True)
@click.argument("ip_addr", metavar="<ip_addr>", required=True)
@click.argument('gw', metavar='<default gateway IP address>', required=False)
@click.pass_context
def add(ctx, interface_name, ip_addr):
def add(ctx, interface_name, ip_addr, gw):
"""Add an IP address towards the interface"""
config_db = ctx.obj["config_db"]
if get_interface_naming_mode() == "alias":
Expand All @@ -956,6 +1047,32 @@ def add(ctx, interface_name, ip_addr):
if interface_name.startswith("Ethernet"):
config_db.set_entry("INTERFACE", (interface_name, ip_addr), {"NULL": "NULL"})
config_db.set_entry("INTERFACE", interface_name, {"NULL": "NULL"})
elif interface_name == 'eth0':

# Configuring more than 1 IPv4 or more than 1 IPv6 address fails.
# Allow only one IPv4 and only one IPv6 address to be configured for IPv6.
# If a row already exist, overwrite it (by doing delete and add).
mgmtintf_key_list = _get_all_mgmtinterface_keys()

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

# If user has configured IPv6 address and the already available row is also IPv6, delete it here.
config_db.set_entry("MGMT_INTERFACE", ("eth0", key[1]), None)
elif ':' not in ip_addr and ':' not in key[1]:
# If user has configured IPv4 address and the already available row is also IPv6, delete it here.
config_db.set_entry("MGMT_INTERFACE", ("eth0", key[1]), None)

# Set the new row with new value
if not gw:
config_db.set_entry("MGMT_INTERFACE", (interface_name, ip_addr), {"NULL": "NULL"})
else:
config_db.set_entry("MGMT_INTERFACE", (interface_name, ip_addr), {"gwaddr": gw})
cmd="systemctl restart interfaces-config"
os.system (cmd)
cmd="systemctl restart ntp-config"
os.system (cmd)

elif interface_name.startswith("PortChannel"):
config_db.set_entry("PORTCHANNEL_INTERFACE", (interface_name, ip_addr), {"NULL": "NULL"})
config_db.set_entry("PORTCHANNEL_INTERFACE", interface_name, {"NULL": "NULL"})
Expand Down Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done.

os.system (cmd)
cmd="systemctl restart ntp-config"
os.system (cmd)
elif interface_name.startswith("PortChannel"):
config_db.set_entry("PORTCHANNEL_INTERFACE", (interface_name, ip_addr), None)
if_table = "PORTCHANNEL_INTERFACE"
Expand All @@ -1003,7 +1126,7 @@ def remove(ctx, interface_name, ip_addr):
ctx.fail("'interface_name' is not valid. Valid names [Ethernet/PortChannel/Vlan/Loopback]")
except ValueError:
ctx.fail("'ip_addr' is not valid.")

exists = False
if if_table:
interfaces = config_db.get_table(if_table)
Expand Down
107 changes: 105 additions & 2 deletions show/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,93 @@ def ndp(ip6address, iface, verbose):

run_command(cmd, display_cmd=verbose)

#
# 'mgmt-vrf' group ("show mgmt-vrf ...")
#

@cli.group('mgmt-vrf', invoke_without_command=True)
@click.pass_context
def mgmt_vrf(ctx):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide output of the commands in description for reference as in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Output of the show commands are added in this PR. All these details will be added into the CLI document once if the PR is merged.


"""Show management VRF attributes"""

if ctx.invoked_subcommand is None:
cmd = 'sonic-cfggen -d --var-json "MGMT_VRF_CONFIG"'

p = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is again repeated in show ntp. Can you put this in a function like is_mgmt_vrf_enabled

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done.

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

# if the mgmtVrfEnabled attribute is configured, check the value
# and print Enabled or Disabled accordingly.
if 'mgmtVrfEnabled' in mvrf_dict['vrf_global']:
if (mvrf_dict['vrf_global']['mgmtVrfEnabled'] == "true"):
click.echo("\nManagementVRF : Enabled")
else:
click.echo("\nManagementVRF : Disabled")

click.echo("\nManagement VRF in Linux:")
cmd = "sudo ip link show type vrf"
Copy link
Contributor

Choose a reason for hiding this comment

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

This will also show data VRFs/VNets if present, correct?. Can you fix this to only show mgmt VRF?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it was a mistake. Its corrected now to display only the management VRF related data.

run_command(cmd)

@mgmt_vrf.command('interfaces')
def mgmt_vrf_interfaces ():
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what is the value add of this command?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed this command. Earlier, the idea was to provide separate commands for each of the linux command that were required for debugging. Now the required show is addressed in "show mgmt_vrf" command itself. Hence, removed this command.

"""Show management VRF attributes"""

click.echo("\neth0 Interfaces in Management VRF:")
cmd = "sudo ifconfig eth0"
run_command(cmd)
return None

@mgmt_vrf.command('route')
def mgmt_vrf_route ():
"""Show management VRF routes"""

click.echo("\nRoutes in Management VRF Routing Table:")
cmd = "sudo ip route show table 5000"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note 'sudo' doesn't really work on show command unless you explicitly add the command to /etc/sudoers. Can you remove the unwanted sudo?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done.

run_command(cmd)
return None


Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove extra line

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done.

@mgmt_vrf.command('addresses')
def mgmt_vrf_addresses ():
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

"""Show management VRF addresses"""

click.echo("\nIP Addresses for interfaces in Management VRF:")
cmd = "sudo ip address show mgmt"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here - sudo

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done.

run_command(cmd)
return None


prsunny marked this conversation as resolved.
Show resolved Hide resolved

#
# 'management_interface' group ("show management_interface ...")
#

@cli.group(cls=AliasedGroup, default_if_no_args=False)
def management_interface():
"""Show management interface parameters"""
pass

# 'address' subcommand ("show management_interface address")
@management_interface.command()
def address ():
"""Show IP address configured for management interface"""

config_db = ConfigDBConnector()
config_db.connect()
header = ['IFNAME', 'IP Address', 'PrefixLen',]
body = []

# Fetching data from config_db for MGMT_INTERFACE
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']))
Copy link
Contributor

Choose a reason for hiding this comment

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

Network

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done.



#
# 'interfaces' group ("show interfaces ...")
#
Expand Down Expand Up @@ -1443,8 +1530,24 @@ def bgp(verbose):
@click.option('--verbose', is_flag=True, help="Enable verbose output")
def ntp(verbose):
"""Show NTP information"""
cmd = "ntpq -p -n"
run_command(cmd, display_cmd=verbose)
ntpcmd = "ntpq -p -n"
if ctx.invoked_subcommand is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not yet done, please address the earlier comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

cmd = 'sonic-cfggen -d --var-json "MGMT_VRF_CONFIG"'

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

# if the mgmtVrfEnabled attribute is configured, check the value
# and print Enabled or Disabled accordingly.
if 'mgmtVrfEnabled' in mvrf_dict['vrf_global']:
if (mvrf_dict['vrf_global']['mgmtVrfEnabled'] == "true"):
#ManagementVRF is enabled. Call ntpq using cgexec
ntpcmd = "cgexec -g l3mdev:mgmt ntpq -p -n"
run_command(ntpcmd, display_cmd=verbose)



#
Expand Down