From 37b4acdbe262643535051e653722c357dc6a9a52 Mon Sep 17 00:00:00 2001 From: Guohan Lu Date: Fri, 31 Jul 2020 08:05:15 +0000 Subject: [PATCH 1/8] [show/config]: combine feature and container feature cli merge container feature cli into feature cli config command: ``` admin@vlab-01:~$ sudo config feature Usage: config feature [OPTIONS] COMMAND [ARGS]... Modify configuration of features Options: -?, -h, --help Show this message and exit. Commands: autorestart Configure autorestart status for a feature state Configure status of feature ``` show command: ``` admin@vlab-01:~$ show feature Usage: show feature [OPTIONS] COMMAND [ARGS]... Show feature status Options: -?, -h, --help Show this message and exit. Commands: autorestart Show auto-restart status for a feature status Show feature status ``` output: ``` admin@vlab-01:~$ show feature status Feature Status AutoRestart ---------- -------- ------------- lldp enabled enabled pmon enabled enabled sflow disabled enabled database enabled disabled restapi disabled enabled telemetry enabled enabled snmp enabled enabled bgp enabled enabled radv enabled enabled dhcp_relay enabled enabled nat enabled enabled teamd enabled enabled syncd enabled enabled swss enabled enabled admin@vlab-01:~$ show feature autorestart Feature AutoRestart ---------- ------------- lldp enabled pmon enabled sflow enabled database disabled restapi enabled telemetry enabled snmp enabled bgp enabled radv enabled dhcp_relay enabled nat enabled teamd enabled syncd enabled swss enabled ``` Signed-off-by: Guohan Lu --- config/main.py | 80 ++++++++++++++------------------ show/main.py | 77 ++++++++++++++---------------- tests/mock_tables/config_db.json | 74 ++++++++++++++++++++++++++++- 3 files changed, 144 insertions(+), 87 deletions(-) diff --git a/config/main.py b/config/main.py index aad6b3227a..f2e2159919 100755 --- a/config/main.py +++ b/config/main.py @@ -552,7 +552,7 @@ def _is_neighbor_ipaddress(config_db, ipaddress): def _get_all_neighbor_ipaddresses(config_db, ignore_local_hosts=False): """Returns list of strings containing IP addresses of all BGP neighbors - if the flag ignore_local_hosts is set to True, additional check to see if + if the flag ignore_local_hosts is set to True, additional check to see if if the BGP neighbor AS number is same as local BGP AS number, if so ignore that neigbor. """ addrs = [] @@ -1007,7 +1007,7 @@ def load(filename, yes): # if any of the config files in linux host OR namespace is not present, return if not os.path.isfile(file): click.echo("The config_db file {} doesn't exist".format(file)) - return + return if namespace is None: command = "{} -j {} --write-to-db".format(SONIC_CFGGEN_PATH, file) @@ -1225,7 +1225,7 @@ def load_minigraph(no_service_restart): if os.path.isfile('/etc/sonic/acl.json'): run_command("acl-loader update full /etc/sonic/acl.json", display_cmd=True) - + # Write latest db version string into db db_migrator='/usr/bin/db_migrator.py' if os.path.isfile(db_migrator) and os.access(db_migrator, os.X_OK): @@ -1235,7 +1235,7 @@ def load_minigraph(no_service_restart): else: cfggen_namespace_option = " -n {}".format(namespace) run_command(db_migrator + ' -o set_version' + cfggen_namespace_option) - + # We first run "systemctl reset-failed" to remove the "failed" # status from all services before we attempt to restart them if not no_service_restart: @@ -1891,7 +1891,7 @@ def add_snmp_agent_address(ctx, agentip, port, vrf): #Construct SNMP_AGENT_ADDRESS_CONFIG table key in the format ip|| key = agentip+'|' if port: - key = key+port + key = key+port key = key+'|' if vrf: key = key+vrf @@ -1912,7 +1912,7 @@ def del_snmp_agent_address(ctx, agentip, port, vrf): key = agentip+'|' if port: - key = key+port + key = key+port key = key+'|' if vrf: key = key+vrf @@ -2174,7 +2174,7 @@ def all(verbose): config_db = ConfigDBConnector(use_unix_socket_path=True, namespace=namespace) config_db.connect() bgp_neighbor_ip_list = _get_all_neighbor_ipaddresses(config_db, ignore_local_hosts) - for ipaddress in bgp_neighbor_ip_list: + for ipaddress in bgp_neighbor_ip_list: _change_bgp_session_status_by_addr(config_db, ipaddress, 'up', verbose) # 'neighbor' subcommand @@ -2463,7 +2463,7 @@ def breakout(ctx, interface_name, mode, verbose, force_remove_dependencies, load sys.exit(0) def _get_all_mgmtinterface_keys(): - """Returns list of strings containing mgmt interface keys + """Returns list of strings containing mgmt interface keys """ config_db = ConfigDBConnector() config_db.connect() @@ -3191,9 +3191,9 @@ def priority(ctx, interface_name, priority, status): interface_name = interface_alias_to_name(interface_name) if interface_name is None: ctx.fail("'interface_name' is None!") - + run_command("pfc config priority {0} {1} {2}".format(status, interface_name, priority)) - + # # 'platform' group ('config platform ...') # @@ -3292,10 +3292,10 @@ def naming_mode_alias(): def is_loopback_name_valid(loopback_name): """Loopback name validation """ - + if loopback_name[:CFG_LOOPBACK_PREFIX_LEN] != CFG_LOOPBACK_PREFIX : return False - if (loopback_name[CFG_LOOPBACK_PREFIX_LEN:].isdigit() is False or + if (loopback_name[CFG_LOOPBACK_PREFIX_LEN:].isdigit() is False or int(loopback_name[CFG_LOOPBACK_PREFIX_LEN:]) > CFG_LOOPBACK_ID_MAX_VAL) : return False if len(loopback_name) > CFG_LOOPBACK_NAME_TOTAL_LEN_MAX: @@ -3462,7 +3462,7 @@ def add_ntp_server(ctx, ntp_ip_address): if ntp_ip_address in ntp_servers: click.echo("NTP server {} is already configured".format(ntp_ip_address)) return - else: + else: db.set_entry('NTP_SERVER', ntp_ip_address, {'NULL': 'NULL'}) click.echo("NTP server {} added to configuration".format(ntp_ip_address)) try: @@ -3483,7 +3483,7 @@ def del_ntp_server(ctx, ntp_ip_address): if ntp_ip_address in ntp_servers: db.set_entry('NTP_SERVER', '{}'.format(ntp_ip_address), None) click.echo("NTP server {} removed from configuration".format(ntp_ip_address)) - else: + else: ctx.fail("NTP server {} is not configured.".format(ntp_ip_address)) try: click.echo("Restarting ntp-config service...") @@ -3770,12 +3770,20 @@ def delete(ctx): config_db.set_entry('SFLOW', 'global', sflow_tbl['global']) # -# 'feature' command ('config feature name state') -# -@config.command('feature') +# 'feature' group ('config feature ...') +# +@config.group(cls=AbbreviationGroup, name='feature', invoke_without_command=False) +def feature(): + """Modify configuration of features""" + pass + +# +# 'feature' command ('config feature state ...') +# +@feature.command('state') @click.argument('name', metavar='', required=True) @click.argument('state', metavar='', required=True, type=click.Choice(["enabled", "disabled"])) -def feature_status(name, state): +def feature_state(name, state): """ Configure status of feature""" config_db = ConfigDBConnector() config_db.connect() @@ -3788,40 +3796,24 @@ def feature_status(name, state): config_db.mod_entry('FEATURE', name, {'status': state}) # -# 'container' group ('config container ...') -# -@config.group(cls=AbbreviationGroup, name='container', invoke_without_command=False) -def container(): - """Modify configuration of containers""" - pass - -# -# 'feature' group ('config container feature ...') +# 'autorestart' subcommand ('config feature autorestart ...') # -@container.group(cls=AbbreviationGroup, name='feature', invoke_without_command=False) -def feature(): - """Modify configuration of container features""" - pass - -# -# 'autorestart' subcommand ('config container feature autorestart ...') -# -@feature.command(name='autorestart', short_help="Configure the status of autorestart feature for specific container") -@click.argument('container_name', metavar='', required=True) -@click.argument('autorestart_status', metavar='', required=True, type=click.Choice(["enabled", "disabled"])) -def autorestart(container_name, autorestart_status): +@feature.command(name='autorestart', short_help="Configure the autosrestart status for a feature") +@click.argument('name', metavar='', required=True) +@click.argument('autorestart_status', metavar='', required=True, type=click.Choice(["enabled", "disabled"])) +def feature_autorestart(name, autorestart_status): config_db = ConfigDBConnector() config_db.connect() - container_feature_table = config_db.get_table('CONTAINER_FEATURE') + container_feature_table = config_db.get_table('FEATURE') if not container_feature_table: - click.echo("Unable to retrieve container feature table from Config DB.") + click.echo("Unable to retrieve feature table from Config DB.") return - if not container_feature_table.has_key(container_name): - click.echo("Unable to retrieve features for container '{}'".format(container_name)) + if not container_feature_table.has_key(name): + click.echo("Unable to retrieve feature '{}'".format(name)) return - config_db.mod_entry('CONTAINER_FEATURE', container_name, {'auto_restart': autorestart_status}) + config_db.mod_entry('FEATURE', name, {'auto_restart': autorestart_status}) if __name__ == '__main__': config() diff --git a/show/main.py b/show/main.py index b670596466..815379bc60 100755 --- a/show/main.py +++ b/show/main.py @@ -1179,8 +1179,8 @@ def priority(interface): cmd = 'pfc show priority' if interface is not None and get_interface_mode() == "alias": interface = iface_alias_converter.alias_to_name(interface) - - if interface is not None: + + if interface is not None: cmd += ' {0}'.format(interface) run_command(cmd) @@ -1192,8 +1192,8 @@ def asymmetric(interface): cmd = 'pfc show asymmetric' if interface is not None and get_interface_mode() == "alias": interface = iface_alias_converter.alias_to_name(interface) - - if interface is not None: + + if interface is not None: cmd += ' {0}'.format(interface) run_command(cmd) @@ -1727,7 +1727,7 @@ def protocol(verbose): from .bgp_quagga_v6 import bgp ipv6.add_command(bgp) elif routing_stack == "frr": - from .bgp_frr_v4 import bgp + from .bgp_frr_v4 import bgp ip.add_command(bgp) from .bgp_frr_v6 import bgp ipv6.add_command(bgp) @@ -2173,7 +2173,7 @@ def ntp(ctx, verbose): ntpstat_cmd = "ntpstat" ntpcmd = "ntpq -p -n" if is_mgmt_vrf_enabled(ctx) is True: - #ManagementVRF is enabled. Call ntpq using "ip vrf exec" or cgexec based on linux version + #ManagementVRF is enabled. Call ntpq using "ip vrf exec" or cgexec based on linux version os_info = os.uname() release = os_info[2].split('-') if parse_version(release[0]) > parse_version("4.9.0"): @@ -2971,7 +2971,7 @@ def pool(verbose): # Define GEARBOX commands only if GEARBOX is configured app_db = SonicV2Connector(host='127.0.0.1') -app_db.connect(app_db.APPL_DB) +app_db.connect(app_db.APPL_DB) if app_db.keys(app_db.APPL_DB, '_GEARBOX_TABLE:phy:*'): @cli.group(cls=AliasedGroup) @@ -3051,53 +3051,46 @@ def ztp(status, verbose): run_command(cmd, display_cmd=verbose) # -# show features -# -@cli.command('features') -def features(): - """Show status of optional features""" - config_db = ConfigDBConnector() - config_db.connect() - header = ['Feature', 'Status'] - body = [] - status_data = config_db.get_table('FEATURE') - for key in status_data.keys(): - body.append([key, status_data[key]['status']]) - click.echo(tabulate(body, header)) - -# -# 'container' group (show container ...) +# 'feature' group (show feature ...) # -@cli.group(name='container', invoke_without_command=False) -def container(): - """Show container""" +@cli.group(name='feature', invoke_without_command=False) +def feature(): + """Show feature status""" pass # -# 'feature' group (show container feature ...) +# 'status' subcommand (show feature status) # -@container.group(name='feature', invoke_without_command=False) -def feature(): - """Show container feature""" - pass +@feature.command('status', short_help="Show feature status") +@click.argument('feature_name', required=False) +def autorestart(feature_name): + config_db = ConfigDBConnector() + config_db.connect() + header = ['Feature', 'Status', 'AutoRestart'] + body = [] + feature_table = config_db.get_table('FEATURE') + for key in feature_table.keys(): + body.append([key, feature_table[key]['status']]) + body.append([key, feature_table[key]['auto_restart']]) + click.echo(tabulate(body, header)) # -# 'autorestart' subcommand (show container feature autorestart) +# 'autorestart' subcommand (show feature autorestart) # -@feature.command('autorestart', short_help="Show whether the auto-restart feature for container(s) is enabled or disabled") -@click.argument('container_name', required=False) -def autorestart(container_name): +@feature.command('autorestart', short_help="Show auto-restart status for a feature") +@click.argument('feature_name', required=False) +def autorestart(feature_name): config_db = ConfigDBConnector() config_db.connect() - header = ['Container Name', 'Status'] + header = ['Feature', 'AutoRestart'] body = [] - container_feature_table = config_db.get_table('CONTAINER_FEATURE') - if container_name: - if container_feature_table and container_feature_table.has_key(container_name): - body.append([container_name, container_feature_table[container_name]['auto_restart']]) + feature_table = config_db.get_table('FEATURE') + if feature_name: + if feature_table and feature_table.has_key(feature): + body.append([feature_name, feature_table[feature_name]['auto_restart']]) else: - for name in container_feature_table.keys(): - body.append([name, container_feature_table[name]['auto_restart']]) + for name in feature_table.keys(): + body.append([name, feature_table[name]['auto_restart']]) click.echo(tabulate(body, header)) # diff --git a/tests/mock_tables/config_db.json b/tests/mock_tables/config_db.json index ec278c3450..418a01de87 100644 --- a/tests/mock_tables/config_db.json +++ b/tests/mock_tables/config_db.json @@ -150,5 +150,77 @@ "DEBUG_COUNTER_DROP_REASON|DEBUG_0|IP_HEADER_ERROR": {}, "DEBUG_COUNTER_DROP_REASON|DEBUG_1|ACL_ANY": {}, "DEBUG_COUNTER_DROP_REASON|DEBUG_2|IP_HEADER_ERROR": {}, - "DEBUG_COUNTER_DROP_REASON|DEBUG_2|NO_L3_HEADER": {} + "DEBUG_COUNTER_DROP_REASON|DEBUG_2|NO_L3_HEADER": {}, + "FEATURE": { + "bgp": { + "status": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "database": { + "status": "enabled", + "auto_restart": "disabled", + "high_mem_alert": "disabled" + }, + "dhcp_relay": { + "status": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "lldp": { + "status": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "nat": { + "status": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "pmon": { + "status": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "radv": { + "status": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "restapi": { + "status": "disabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "sflow": { + "status": "disabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "snmp": { + "status": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "swss": { + "status": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "syncd": { + "status": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "teamd": { + "status": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "telemetry": { + "status": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + } + } } From 20967a55557aab7d55595438e75556b3daf0a7f2 Mon Sep 17 00:00:00 2001 From: Guohan Lu Date: Fri, 31 Jul 2020 19:05:40 +0000 Subject: [PATCH 2/8] rename status to state Signed-off-by: Guohan Lu --- config/main.py | 35 ++++++++++++++++++----------------- show/main.py | 10 +++++----- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/config/main.py b/config/main.py index f2e2159919..dd58cb14d8 100755 --- a/config/main.py +++ b/config/main.py @@ -692,12 +692,12 @@ def _get_disabled_services_list(): log_warning("Feature is None") continue - status = feature_table[feature_name]['status'] - if not status: - log_warning("Status of feature '{}' is None".format(feature_name)) + state = feature_table[feature_name]['state'] + if not state: + log_warning("Enable state of feature '{}' is None".format(feature_name)) continue - if status == "disabled": + if state == "disabled": disabled_services_list.append(feature_name) else: log_warning("Unable to retreive FEATURE table") @@ -3780,40 +3780,41 @@ def feature(): # # 'feature' command ('config feature state ...') # -@feature.command('state') +@feature.command('state', short_help="Enable/disable a feature") @click.argument('name', metavar='', required=True) -@click.argument('state', metavar='', required=True, type=click.Choice(["enabled", "disabled"])) +@click.argument('state', metavar='', required=True, type=click.Choice(["enabled", "disabled"])) def feature_state(name, state): - """ Configure status of feature""" + """Enable/disable a feature""" config_db = ConfigDBConnector() config_db.connect() - status_data = config_db.get_entry('FEATURE', name) + state_data = config_db.get_entry('FEATURE', name) - if not status_data: + if not state_data: click.echo(" Feature '{}' doesn't exist".format(name)) return - config_db.mod_entry('FEATURE', name, {'status': state}) + config_db.mod_entry('FEATURE', name, {'state': state}) # # 'autorestart' subcommand ('config feature autorestart ...') # -@feature.command(name='autorestart', short_help="Configure the autosrestart status for a feature") +@feature.command(name='autorestart', short_help="Enable/disable autosrestart of a feature") @click.argument('name', metavar='', required=True) -@click.argument('autorestart_status', metavar='', required=True, type=click.Choice(["enabled", "disabled"])) -def feature_autorestart(name, autorestart_status): +@click.argument('autorestart', metavar='', required=True, type=click.Choice(["enabled", "disabled"])) +def feature_autorestart(name, autorestart): + """Enable/disable autorestart of a feature""" config_db = ConfigDBConnector() config_db.connect() - container_feature_table = config_db.get_table('FEATURE') - if not container_feature_table: + feature_table = config_db.get_table('FEATURE') + if not feature_table: click.echo("Unable to retrieve feature table from Config DB.") return - if not container_feature_table.has_key(name): + if not feature_table.has_key(name): click.echo("Unable to retrieve feature '{}'".format(name)) return - config_db.mod_entry('FEATURE', name, {'auto_restart': autorestart_status}) + config_db.mod_entry('FEATURE', name, {'auto_restart': autorestart}) if __name__ == '__main__': config() diff --git a/show/main.py b/show/main.py index 815379bc60..90332f07b1 100755 --- a/show/main.py +++ b/show/main.py @@ -3059,25 +3059,25 @@ def feature(): pass # -# 'status' subcommand (show feature status) +# 'state' subcommand (show feature state) # -@feature.command('status', short_help="Show feature status") +@feature.command('state', short_help="Show feature state") @click.argument('feature_name', required=False) def autorestart(feature_name): config_db = ConfigDBConnector() config_db.connect() - header = ['Feature', 'Status', 'AutoRestart'] + header = ['Feature', 'State', 'AutoRestart'] body = [] feature_table = config_db.get_table('FEATURE') for key in feature_table.keys(): - body.append([key, feature_table[key]['status']]) + body.append([key, feature_table[key]['State']]) body.append([key, feature_table[key]['auto_restart']]) click.echo(tabulate(body, header)) # # 'autorestart' subcommand (show feature autorestart) # -@feature.command('autorestart', short_help="Show auto-restart status for a feature") +@feature.command('autorestart', short_help="Show auto-restart state for a feature") @click.argument('feature_name', required=False) def autorestart(feature_name): config_db = ConfigDBConnector() From abcc53e3e396084af9db4c874a7266dfc1d4e994 Mon Sep 17 00:00:00 2001 From: Guohan Lu Date: Fri, 31 Jul 2020 19:45:17 +0000 Subject: [PATCH 3/8] fix comments Signed-off-by: Guohan Lu --- config/main.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/main.py b/config/main.py index dd58cb14d8..c12bad620f 100755 --- a/config/main.py +++ b/config/main.py @@ -3778,7 +3778,7 @@ def feature(): pass # -# 'feature' command ('config feature state ...') +# 'state' command ('config feature state ...') # @feature.command('state', short_help="Enable/disable a feature") @click.argument('name', metavar='', required=True) @@ -3796,7 +3796,7 @@ def feature_state(name, state): config_db.mod_entry('FEATURE', name, {'state': state}) # -# 'autorestart' subcommand ('config feature autorestart ...') +# 'autorestart' command ('config feature autorestart ...') # @feature.command(name='autorestart', short_help="Enable/disable autosrestart of a feature") @click.argument('name', metavar='', required=True) From b13931b2bc347d88010edb1eb9b4096776aff456 Mon Sep 17 00:00:00 2001 From: Guohan Lu Date: Sun, 2 Aug 2020 08:04:46 +0000 Subject: [PATCH 4/8] add unit test Signed-off-by: Guohan Lu --- config/main.py | 45 +++---- show/main.py | 34 ++++-- sonic-utilities-tests/feature_test.py | 168 ++++++++++++++++++++++++++ tests/mock_tables/config_db.json | 140 +++++++++++---------- 4 files changed, 283 insertions(+), 104 deletions(-) create mode 100644 sonic-utilities-tests/feature_test.py diff --git a/config/main.py b/config/main.py index c12bad620f..7d86a8032e 100755 --- a/config/main.py +++ b/config/main.py @@ -48,6 +48,10 @@ CFG_LOOPBACK_NAME_TOTAL_LEN_MAX = 11 CFG_LOOPBACK_ID_MAX_VAL = 999 CFG_LOOPBACK_NO="<0-999>" + +asic_type = None +config_db = None + # ========================== Syslog wrappers ========================== def log_debug(msg): @@ -112,17 +116,6 @@ def get_command(self, ctx, cmd_name): ctx.fail('Too many matches: %s' % ', '.join(sorted(matches))) - -# -# Load asic_type for further use -# - -try: - version_info = sonic_device_util.get_sonic_version_info() - asic_type = version_info['asic_type'] -except (KeyError, TypeError): - raise click.Abort() - # # Load breakout config file for Dynamic Port Breakout # @@ -3202,9 +3195,6 @@ def priority(ctx, interface_name, priority, status): def platform(): """Platform-related configuration tasks""" -if asic_type == 'mellanox': - platform.add_command(mlnx.mlnx) - # 'firmware' subgroup ("config platform firmware ...") @platform.group(cls=AbbreviationGroup) def firmware(): @@ -3785,13 +3775,11 @@ def feature(): @click.argument('state', metavar='', required=True, type=click.Choice(["enabled", "disabled"])) def feature_state(name, state): """Enable/disable a feature""" - config_db = ConfigDBConnector() - config_db.connect() state_data = config_db.get_entry('FEATURE', name) if not state_data: - click.echo(" Feature '{}' doesn't exist".format(name)) - return + click.echo("Feature '{}' doesn't exist".format(name)) + sys.exit(1) config_db.mod_entry('FEATURE', name, {'state': state}) @@ -3803,18 +3791,31 @@ def feature_state(name, state): @click.argument('autorestart', metavar='', required=True, type=click.Choice(["enabled", "disabled"])) def feature_autorestart(name, autorestart): """Enable/disable autorestart of a feature""" - config_db = ConfigDBConnector() - config_db.connect() feature_table = config_db.get_table('FEATURE') if not feature_table: click.echo("Unable to retrieve feature table from Config DB.") - return + sys.exit(1) if not feature_table.has_key(name): click.echo("Unable to retrieve feature '{}'".format(name)) - return + sys.exit(1) config_db.mod_entry('FEATURE', name, {'auto_restart': autorestart}) if __name__ == '__main__': + # + # Load asic_type for further use + # + try: + version_info = sonic_device_util.get_sonic_version_info() + asic_type = version_info['asic_type'] + except KeyError, TypeError: + raise click.Abort() + + if asic_type == 'mellanox': + platform.add_command(mlnx.mlnx) + + config_db = ConfigDBConnector() + config_db.connect() + config() diff --git a/show/main.py b/show/main.py index 90332f07b1..1a48a75e9e 100755 --- a/show/main.py +++ b/show/main.py @@ -31,6 +31,8 @@ VLAN_SUB_INTERFACE_SEPARATOR = '.' +config_db = None + try: # noinspection PyPep8Naming import ConfigParser as configparser @@ -3059,19 +3061,24 @@ def feature(): pass # -# 'state' subcommand (show feature state) +# 'state' subcommand (show feature status) # -@feature.command('state', short_help="Show feature state") +@feature.command('status', short_help="Show feature state") @click.argument('feature_name', required=False) def autorestart(feature_name): - config_db = ConfigDBConnector() - config_db.connect() header = ['Feature', 'State', 'AutoRestart'] body = [] feature_table = config_db.get_table('FEATURE') - for key in feature_table.keys(): - body.append([key, feature_table[key]['State']]) - body.append([key, feature_table[key]['auto_restart']]) + if feature_name: + if feature_table and feature_table.has_key(feature_name): + body.append([feature_name, feature_table[feature_name]['state'], \ + feature_table[feature_name]['auto_restart']]) + else: + click.echo("Can not find feature {}".format(feature_name)) + sys.exit(1) + else: + for key in natsorted(feature_table.keys()): + body.append([key, feature_table[key]['state'], feature_table[key]['auto_restart']]) click.echo(tabulate(body, header)) # @@ -3080,16 +3087,17 @@ def autorestart(feature_name): @feature.command('autorestart', short_help="Show auto-restart state for a feature") @click.argument('feature_name', required=False) def autorestart(feature_name): - config_db = ConfigDBConnector() - config_db.connect() header = ['Feature', 'AutoRestart'] body = [] feature_table = config_db.get_table('FEATURE') if feature_name: - if feature_table and feature_table.has_key(feature): + if feature_table and feature_table.has_key(feature_name): body.append([feature_name, feature_table[feature_name]['auto_restart']]) + else: + click.echo("Can not find feature {}".format(feature_name)) + sys.exit(1) else: - for name in feature_table.keys(): + for name in natsorted(feature_table.keys()): body.append([name, feature_table[name]['auto_restart']]) click.echo(tabulate(body, header)) @@ -3413,4 +3421,8 @@ def tunnel(): click.echo(tabulate(table, header)) if __name__ == '__main__': + + config_db = ConfigDBConnector() + config_db.connect() + cli() diff --git a/sonic-utilities-tests/feature_test.py b/sonic-utilities-tests/feature_test.py new file mode 100644 index 0000000000..85296a0481 --- /dev/null +++ b/sonic-utilities-tests/feature_test.py @@ -0,0 +1,168 @@ +import sys +import os +import mock +import pytest +import traceback +from click.testing import CliRunner +from swsssdk import ConfigDBConnector + +test_path = os.path.dirname(os.path.abspath(__file__)) +modules_path = os.path.dirname(test_path) +sys.path.insert(0, modules_path) + +import mock_tables.dbconnector +import show.main as show +import config.main as config + +config.asic_type = mock.MagicMock(return_value = "broadcom") +config._get_device_type = mock.MagicMock(return_value = "ToRRouter") + +config_db = ConfigDBConnector() +config_db.connect() + +show.config_db = config_db +config.config_db = config_db + +show_feature_status_output="""\ +Feature State AutoRestart +---------- -------- ------------- +bgp enabled enabled +database enabled disabled +dhcp_relay enabled enabled +lldp enabled enabled +nat enabled enabled +pmon enabled enabled +radv enabled enabled +restapi disabled enabled +sflow disabled enabled +snmp enabled enabled +swss enabled enabled +syncd enabled enabled +teamd enabled enabled +telemetry enabled enabled +""" + +show_feature_bgp_status_output="""\ +Feature State AutoRestart +--------- ------- ------------- +bgp enabled enabled +""" + +show_feature_bgp_disabled_status_output="""\ +Feature State AutoRestart +--------- -------- ------------- +bgp disabled enabled +""" + +show_feature_autorestart_output="""\ +Feature AutoRestart +---------- ------------- +bgp enabled +database disabled +dhcp_relay enabled +lldp enabled +nat enabled +pmon enabled +radv enabled +restapi enabled +sflow enabled +snmp enabled +swss enabled +syncd enabled +teamd enabled +telemetry enabled +""" + +show_feature_bgp_autorestart_output="""\ +Feature AutoRestart +--------- ------------- +bgp enabled +""" + + +show_feature_bgp_disabled_autorestart_output="""\ +Feature AutoRestart +--------- ------------- +bgp disabled +""" + +class TestFeature(object): + @classmethod + def setup_class(cls): + print("SETUP") + + def test_show_feature_status(self): + runner = CliRunner() + result = runner.invoke(show.cli.commands["feature"].commands["status"], []) + print(result.exit_code) + print(result.output) + assert result.exit_code == 0 + assert result.output == show_feature_status_output + + def test_show_bgp_feature_status(self): + runner = CliRunner() + result = runner.invoke(show.cli.commands["feature"].commands["status"], ["bgp"]) + print(result.exit_code) + print(result.output) + assert result.exit_code == 0 + assert result.output == show_feature_bgp_status_output + + def test_show_unknown_feature_status(self): + runner = CliRunner() + result = runner.invoke(show.cli.commands["feature"].commands["status"], ["foo"]) + print(result.exit_code) + print(result.output) + assert result.exit_code == 1 + + def test_show_feature_autorestart(self): + runner = CliRunner() + result = runner.invoke(show.cli.commands["feature"].commands["autorestart"], []) + print(result.exit_code) + print(result.output) + assert result.exit_code == 0 + assert result.output == show_feature_autorestart_output + + def test_show_bgp_autorestart_status(self): + runner = CliRunner() + result = runner.invoke(show.cli.commands["feature"].commands["autorestart"], ["bgp"]) + print(result.exit_code) + print(result.output) + assert result.exit_code == 0 + assert result.output == show_feature_bgp_autorestart_output + + def test_show_unknown_autorestart_status(self): + runner = CliRunner() + result = runner.invoke(show.cli.commands["feature"].commands["autorestart"], ["foo"]) + print(result.exit_code) + print(result.output) + assert result.exit_code == 1 + + def test_config_bgp_feature_state(self): + runner = CliRunner() + result = runner.invoke(config.config.commands["feature"].commands["state"], ["bgp", "disabled"]) + print(result.exit_code) + print(result.output) + result = runner.invoke(show.cli.commands["feature"].commands["status"], ["bgp"]) + print(result.output) + assert result.exit_code == 0 + assert result.output == show_feature_bgp_disabled_status_output + + def test_config_bgp_autorestart(self): + runner = CliRunner() + result = runner.invoke(config.config.commands["feature"].commands["autorestart"], ["bgp", "disabled"]) + print(result.exit_code) + print(result.output) + result = runner.invoke(show.cli.commands["feature"].commands["autorestart"], ["bgp"]) + print(result.output) + assert result.exit_code == 0 + assert result.output == show_feature_bgp_disabled_autorestart_output + + def test_config_unknown_feature(self): + runner = CliRunner() + result = runner.invoke(config.config.commands["feature"].commands['state'], ["foo", "enabled"]) + print(result.output) + assert result.exit_code == 1 + + @classmethod + def teardown_class(cls): + print("TEARDOWN") diff --git a/tests/mock_tables/config_db.json b/tests/mock_tables/config_db.json index 418a01de87..f60ee3ef12 100644 --- a/tests/mock_tables/config_db.json +++ b/tests/mock_tables/config_db.json @@ -151,76 +151,74 @@ "DEBUG_COUNTER_DROP_REASON|DEBUG_1|ACL_ANY": {}, "DEBUG_COUNTER_DROP_REASON|DEBUG_2|IP_HEADER_ERROR": {}, "DEBUG_COUNTER_DROP_REASON|DEBUG_2|NO_L3_HEADER": {}, - "FEATURE": { - "bgp": { - "status": "enabled", - "auto_restart": "enabled", - "high_mem_alert": "disabled" - }, - "database": { - "status": "enabled", - "auto_restart": "disabled", - "high_mem_alert": "disabled" - }, - "dhcp_relay": { - "status": "enabled", - "auto_restart": "enabled", - "high_mem_alert": "disabled" - }, - "lldp": { - "status": "enabled", - "auto_restart": "enabled", - "high_mem_alert": "disabled" - }, - "nat": { - "status": "enabled", - "auto_restart": "enabled", - "high_mem_alert": "disabled" - }, - "pmon": { - "status": "enabled", - "auto_restart": "enabled", - "high_mem_alert": "disabled" - }, - "radv": { - "status": "enabled", - "auto_restart": "enabled", - "high_mem_alert": "disabled" - }, - "restapi": { - "status": "disabled", - "auto_restart": "enabled", - "high_mem_alert": "disabled" - }, - "sflow": { - "status": "disabled", - "auto_restart": "enabled", - "high_mem_alert": "disabled" - }, - "snmp": { - "status": "enabled", - "auto_restart": "enabled", - "high_mem_alert": "disabled" - }, - "swss": { - "status": "enabled", - "auto_restart": "enabled", - "high_mem_alert": "disabled" - }, - "syncd": { - "status": "enabled", - "auto_restart": "enabled", - "high_mem_alert": "disabled" - }, - "teamd": { - "status": "enabled", - "auto_restart": "enabled", - "high_mem_alert": "disabled" - }, - "telemetry": { - "status": "enabled", - "auto_restart": "enabled", - "high_mem_alert": "disabled" - } + "FEATURE|bgp": { + "state": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "FEATURE|database": { + "state": "enabled", + "auto_restart": "disabled", + "high_mem_alert": "disabled" + }, + "FEATURE|dhcp_relay": { + "state": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "FEATURE|lldp": { + "state": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "FEATURE|nat": { + "state": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "FEATURE|pmon": { + "state": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "FEATURE|radv": { + "state": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "FEATURE|restapi": { + "state": "disabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "FEATURE|sflow": { + "state": "disabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "FEATURE|snmp": { + "state": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "FEATURE|swss": { + "state": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "FEATURE|syncd": { + "state": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "FEATURE|teamd": { + "state": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" + }, + "FEATURE|telemetry": { + "state": "enabled", + "auto_restart": "enabled", + "high_mem_alert": "disabled" } } From 64d3db38b9bae6fea478794b5acfb2ffe828d699 Mon Sep 17 00:00:00 2001 From: Guohan Lu Date: Sun, 2 Aug 2020 09:32:33 +0000 Subject: [PATCH 5/8] address lgtm warning Signed-off-by: Guohan Lu --- sonic-utilities-tests/feature_test.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/sonic-utilities-tests/feature_test.py b/sonic-utilities-tests/feature_test.py index 85296a0481..1480a1167b 100644 --- a/sonic-utilities-tests/feature_test.py +++ b/sonic-utilities-tests/feature_test.py @@ -1,8 +1,6 @@ import sys import os import mock -import pytest -import traceback from click.testing import CliRunner from swsssdk import ConfigDBConnector @@ -10,7 +8,6 @@ modules_path = os.path.dirname(test_path) sys.path.insert(0, modules_path) -import mock_tables.dbconnector import show.main as show import config.main as config From 51a77b4830afbfd7f06e21f1fcdc770e9cc5bc69 Mon Sep 17 00:00:00 2001 From: Guohan Lu Date: Sun, 2 Aug 2020 21:14:44 +0000 Subject: [PATCH 6/8] address comments Signed-off-by: Guohan Lu --- show/main.py | 1 - sonic-utilities-tests/feature_test.py | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/show/main.py b/show/main.py index 1a48a75e9e..cf3286b14f 100755 --- a/show/main.py +++ b/show/main.py @@ -3421,7 +3421,6 @@ def tunnel(): click.echo(tabulate(table, header)) if __name__ == '__main__': - config_db = ConfigDBConnector() config_db.connect() diff --git a/sonic-utilities-tests/feature_test.py b/sonic-utilities-tests/feature_test.py index 1480a1167b..01aa387fe0 100644 --- a/sonic-utilities-tests/feature_test.py +++ b/sonic-utilities-tests/feature_test.py @@ -1,5 +1,6 @@ -import sys import os +import sys + import mock from click.testing import CliRunner from swsssdk import ConfigDBConnector From 89c4cb14295957b3f1bc5b098178776e23782c75 Mon Sep 17 00:00:00 2001 From: Guohan Lu Date: Mon, 3 Aug 2020 05:17:19 +0000 Subject: [PATCH 7/8] move code int def cli and config Signed-off-by: Guohan Lu --- config/main.py | 30 +++++++++++++++--------------- show/main.py | 8 ++++---- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/config/main.py b/config/main.py index 7d86a8032e..cbe416347e 100755 --- a/config/main.py +++ b/config/main.py @@ -883,6 +883,17 @@ def validate_mirror_session_config(config_db, session_name, dst_port, src_port, @click.group(cls=AbbreviationGroup, context_settings=CONTEXT_SETTINGS) def config(): """SONiC command line - 'config' command""" + # + # Load asic_type for further use + # + try: + version_info = sonic_device_util.get_sonic_version_info() + asic_type = version_info['asic_type'] + except KeyError, TypeError: + raise click.Abort() + + if asic_type == 'mellanox': + platform.add_command(mlnx.mlnx) # Load the global config file database_global.json once. SonicDBConfig.load_sonic_global_db_config() @@ -892,6 +903,10 @@ def config(): SonicDBConfig.load_sonic_global_db_config() + global config_db + + config_db = ConfigDBConnector() + config_db.connect() config.add_command(aaa.aaa) config.add_command(aaa.tacacs) @@ -3803,19 +3818,4 @@ def feature_autorestart(name, autorestart): config_db.mod_entry('FEATURE', name, {'auto_restart': autorestart}) if __name__ == '__main__': - # - # Load asic_type for further use - # - try: - version_info = sonic_device_util.get_sonic_version_info() - asic_type = version_info['asic_type'] - except KeyError, TypeError: - raise click.Abort() - - if asic_type == 'mellanox': - platform.add_command(mlnx.mlnx) - - config_db = ConfigDBConnector() - config_db.connect() - config() diff --git a/show/main.py b/show/main.py index cf3286b14f..26c5f280e9 100755 --- a/show/main.py +++ b/show/main.py @@ -559,7 +559,10 @@ def get_bgp_neighbor_ip_to_name(ip, static_neighbors, dynamic_neighbors): @click.group(cls=AliasedGroup, context_settings=CONTEXT_SETTINGS) def cli(): """SONiC command line - 'show' command""" - pass + global config_db + + config_db = ConfigDBConnector() + config_db.connect() # # 'vrf' command ("show vrf") @@ -3421,7 +3424,4 @@ def tunnel(): click.echo(tabulate(table, header)) if __name__ == '__main__': - config_db = ConfigDBConnector() - config_db.connect() - cli() From 65d54bbbdb6edf272f079d9b15795e0faa1eeeac Mon Sep 17 00:00:00 2001 From: Guohan Lu Date: Mon, 3 Aug 2020 07:42:47 +0000 Subject: [PATCH 8/8] change asic_type to global Signed-off-by: Guohan Lu --- config/main.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/main.py b/config/main.py index cbe416347e..128152e6cd 100755 --- a/config/main.py +++ b/config/main.py @@ -886,6 +886,8 @@ def config(): # # Load asic_type for further use # + global asic_type + try: version_info = sonic_device_util.get_sonic_version_info() asic_type = version_info['asic_type']