[show]Fix show route return code on error (sonic-net#2542)

- What I did
Fix show route return command to return error code on failure cases. The parameter return_cmd=True in run_command will suppress the return code and return success even in error scenarios.

- How I did it
When run command is called with return_cmd = True, modified its return to include return code, which can then be used to assess if there is an error by the caller

- How to verify it
Added UT to verify it

- Previous command output (if the output of a command-line utility has changed)
root@sonic:/home/admin# show ip route 123
% Unknown command: show ip route 123 

root@sonic:/home/admin# echo $?
0

- New command output (if the output of a command-line utility has changed)
root@sonic:/home/admin# show ip route 123
% Unknown command: show ip route 123 

root@sonic:/home/admin# echo $?
1

config/main.py

@@ -858,13 +858,13 @@ def _stop_services():
 
 
 def _get_sonic_services():
-    out = clicommon.run_command("systemctl list-dependencies --plain sonic.target | sed '1d'", return_cmd=True)
+    out, _ = clicommon.run_command("systemctl list-dependencies --plain sonic.target | sed '1d'", return_cmd=True)
     return (unit.strip() for unit in out.splitlines())
 
 
 def _get_delayed_sonic_units(get_timers=False):
-    rc1 = clicommon.run_command("systemctl list-dependencies --plain sonic-delayed.target | sed '1d'", return_cmd=True)
-    rc2 = clicommon.run_command("systemctl is-enabled {}".format(rc1.replace("\n", " ")), return_cmd=True)
+    rc1, _ = clicommon.run_command("systemctl list-dependencies --plain sonic-delayed.target | sed '1d'", return_cmd=True)
+    rc2, _ = clicommon.run_command("systemctl is-enabled {}".format(rc1.replace("\n", " ")), return_cmd=True)
     timer = [line.strip() for line in rc1.splitlines()]
     state = [line.strip() for line in rc2.splitlines()]
     services = []
@@ -899,16 +899,16 @@ def _restart_services():
 
 def _delay_timers_elapsed():
     for timer in _get_delayed_sonic_units(get_timers=True):
-        out = clicommon.run_command("systemctl show {} --property=LastTriggerUSecMonotonic --value".format(timer), return_cmd=True)
+        out, _ = clicommon.run_command("systemctl show {} --property=LastTriggerUSecMonotonic --value".format(timer), return_cmd=True)
         if out.strip() == "0":
             return False
     return True
 
 def _per_namespace_swss_ready(service_name):
-    out = clicommon.run_command("systemctl show {} --property ActiveState --value".format(service_name), return_cmd=True)
+    out, _ = clicommon.run_command("systemctl show {} --property ActiveState --value".format(service_name), return_cmd=True)
     if out.strip() != "active":
         return False
-    out = clicommon.run_command("systemctl show {} --property ActiveEnterTimestampMonotonic --value".format(service_name), return_cmd=True)
+    out, _ = clicommon.run_command("systemctl show {} --property ActiveEnterTimestampMonotonic --value".format(service_name), return_cmd=True)
     swss_up_time = float(out.strip())/1000000
     now =  time.monotonic()
     if (now - swss_up_time > 120):
@@ -933,7 +933,7 @@ def _swss_ready():
     return True
 
 def _is_system_starting():
-    out = clicommon.run_command("sudo systemctl is-system-running", return_cmd=True)
+    out, _ = clicommon.run_command("sudo systemctl is-system-running", return_cmd=True)
     return out.strip() == "starting"
 
 def interface_is_in_vlan(vlan_member_table, interface_name):
@@ -2813,7 +2813,7 @@ def _qos_update_ports(ctx, ports, dry_run, json_data):
         command = "{} {} {} -t {},config-db -t {},config-db -y {} --print-data".format(
             SONIC_CFGGEN_PATH, cmd_ns, from_db, buffer_template_file, qos_template_file, sonic_version_file
         )
-        jsonstr = clicommon.run_command(command, display_cmd=False, return_cmd=True)
+        jsonstr, _ = clicommon.run_command(command, display_cmd=False, return_cmd=True)
 
         jsondict = json.loads(jsonstr)
         port_table = jsondict.get('PORT')

config/vlan.py

@@ -87,7 +87,7 @@ def restart_ndppd():
     ndppd_config_gen_cmd = "sonic-cfggen -d -t /usr/share/sonic/templates/ndppd.conf.j2,/etc/ndppd.conf"
     ndppd_restart_cmd = "supervisorctl restart ndppd"
 
-    output = clicommon.run_command(verify_swss_running_cmd, return_cmd=True)
+    output, _ = clicommon.run_command(verify_swss_running_cmd, return_cmd=True)
 
     if output and output.strip() != "running":
         click.echo(click.style('SWSS container is not running, changes will take effect the next time the SWSS container starts', fg='red'),)

scripts/sonic-bootchart

@@ -50,11 +50,13 @@ def check_bootchart_installed():
 
 def get_enabled_status():
     """ Get systemd-bootchart status """
-    return clicommon.run_command("systemctl is-enabled systemd-bootchart", return_cmd=True)
+    output, _ = clicommon.run_command("systemctl is-enabled systemd-bootchart", return_cmd=True)
+    return output
 
 def get_active_status():
     """ Get systemd-bootchart status """
-    return clicommon.run_command("systemctl is-active systemd-bootchart", return_cmd=True)
+    output, _ = clicommon.run_command("systemctl is-active systemd-bootchart", return_cmd=True)
+    return output
 
 def get_output_files():
     bootchart_output_files = []

show/kdump.py

@@ -49,7 +49,7 @@ def get_kdump_oper_mode():
                   returns "Not Ready";
     """
     oper_mode = "Not Ready"
-    command_stdout = clicommon.run_command("/usr/sbin/kdump-config status", return_cmd=True)
+    command_stdout, _ = clicommon.run_command("/usr/sbin/kdump-config status", return_cmd=True)
 
     for line in command_stdout.splitlines():
         if ": ready to kdump" in line:
@@ -99,7 +99,7 @@ def get_kdump_core_files():
     dump_file_list = []
     cmd_message = None
 
-    command_stdout = clicommon.run_command(find_core_dump_files, return_cmd=True)
+    command_stdout, _ = clicommon.run_command(find_core_dump_files, return_cmd=True)
 
     dump_file_list = command_stdout.splitlines()
     if not dump_file_list:
@@ -123,7 +123,7 @@ def get_kdump_dmesg_files():
     dmesg_file_list = []
     cmd_message = None
 
-    command_stdout = clicommon.run_command(find_dmesg_files, return_cmd=True)
+    command_stdout, _ = clicommon.run_command(find_dmesg_files, return_cmd=True)
 
     dmesg_file_list = command_stdout.splitlines()
     if not dmesg_file_list:

tests/config_test.py

@@ -142,13 +142,13 @@ def mock_run_command_side_effect(*args, **kwargs):
 
     if kwargs.get('return_cmd'):
         if command == "systemctl list-dependencies --plain sonic-delayed.target | sed '1d'":
-            return 'snmp.timer'
+            return 'snmp.timer' , 0
         elif command == "systemctl list-dependencies --plain sonic.target | sed '1d'":
-            return 'swss'
+            return 'swss', 0
         elif command == "systemctl is-enabled snmp.timer":
-            return 'enabled'
+            return 'enabled', 0
         else:
-            return ''
+            return '', 0
 
 def mock_run_command_side_effect_disabled_timer(*args, **kwargs):
     command = args[0]
@@ -158,17 +158,17 @@ def mock_run_command_side_effect_disabled_timer(*args, **kwargs):
 
     if kwargs.get('return_cmd'):
         if command == "systemctl list-dependencies --plain sonic-delayed.target | sed '1d'":
-            return 'snmp.timer'
+            return 'snmp.timer', 0
         elif command == "systemctl list-dependencies --plain sonic.target | sed '1d'":
-            return 'swss'
+            return 'swss', 0
         elif command == "systemctl is-enabled snmp.timer":
-            return 'masked'
+            return 'masked', 0
         elif command == "systemctl show swss.service --property ActiveState --value":
-            return 'active'
+            return 'active', 0
         elif command == "systemctl show swss.service --property ActiveEnterTimestampMonotonic --value":
-            return '0'
+            return '0', 0
         else:
-            return ''
+            return '', 0
 
 def mock_run_command_side_effect_untriggered_timer(*args, **kwargs):
     command = args[0]
@@ -178,15 +178,15 @@ def mock_run_command_side_effect_untriggered_timer(*args, **kwargs):
 
     if kwargs.get('return_cmd'):
         if command == "systemctl list-dependencies --plain sonic-delayed.target | sed '1d'":
-            return 'snmp.timer'
+            return 'snmp.timer', 0
         elif command == "systemctl list-dependencies --plain sonic.target | sed '1d'":
-            return 'swss'
+            return 'swss', 0
         elif command == "systemctl is-enabled snmp.timer":
-            return 'enabled'
+            return 'enabled', 0
         elif command == "systemctl show snmp.timer --property=LastTriggerUSecMonotonic --value":
-            return '0'
+            return '0', 0
         else:
-            return ''
+            return '', 0
 
 def mock_run_command_side_effect_gnmi(*args, **kwargs):
     command = args[0]
@@ -196,13 +196,13 @@ def mock_run_command_side_effect_gnmi(*args, **kwargs):
 
     if kwargs.get('return_cmd'):
         if command == "systemctl list-dependencies --plain sonic-delayed.target | sed '1d'":
-            return 'gnmi.timer'
+            return 'gnmi.timer', 0
         elif command == "systemctl list-dependencies --plain sonic.target | sed '1d'":
-            return 'swss'
+            return 'swss', 0
         elif command == "systemctl is-enabled gnmi.timer":
-            return 'enabled'
+            return 'enabled', 0
         else:
-            return ''
+            return '', 0
 
 
 # Load sonic-cfggen from source since /usr/local/bin/sonic-cfggen does not have .py extension.

tests/conftest.py

@@ -145,9 +145,13 @@ def mock_show_bgp_summary(
         bgp_mocked_json = os.path.join(
             test_path, 'mock_tables', 'ipv6_bgp_summary_chassis.json')
 
+    _old_run_bgp_command = bgp_util.run_bgp_command
     bgp_util.run_bgp_command = mock.MagicMock(
         return_value=mock_show_bgp_summary("", ""))
 
+    yield
+    bgp_util.run_bgp_command = _old_run_bgp_command
+
 
 @pytest.fixture
 def setup_t1_topo():
@@ -213,6 +217,7 @@ def mock_run_bgp_command(vtysh_cmd, bgp_namespace, vtysh_shell_cmd=constants.RVT
         else:
             return ""
 
+    _old_run_bgp_command = bgp_util.run_bgp_command
     if any ([request.param == 'ip_route',\
              request.param == 'ip_specific_route', request.param == 'ip_special_route',\
              request.param == 'ipv6_route', request.param == 'ipv6_specific_route']):
@@ -230,7 +235,6 @@ def mock_run_bgp_command(vtysh_cmd, bgp_namespace, vtysh_shell_cmd=constants.RVT
         bgp_util.run_bgp_command = mock.MagicMock(
             return_value=mock_show_bgp_network_single_asic(request))
     elif request.param == 'ip_route_for_int_ip':
-        _old_run_bgp_command = bgp_util.run_bgp_command
         bgp_util.run_bgp_command = mock_run_bgp_command_for_static
     elif request.param == "show_bgp_summary_no_neigh":
         bgp_util.run_bgp_command = mock.MagicMock(
@@ -241,8 +245,7 @@ def mock_run_bgp_command(vtysh_cmd, bgp_namespace, vtysh_shell_cmd=constants.RVT
 
     yield
 
-    if request.param == 'ip_route_for_int_ip':
-        bgp_util.run_bgp_command = _old_run_bgp_command
+    bgp_util.run_bgp_command = _old_run_bgp_command
 
 
 @pytest.fixture

tests/ip_config_test.py

@@ -12,6 +12,7 @@
 import show.main as show
 import config.validated_config_db_connector as validated_config_db_connector
 from utilities_common.db import Db
+import utilities_common.bgp_util as bgp_util
 
 test_path = os.path.dirname(os.path.abspath(__file__))
 ip_config_input_path = os.path.join(test_path, "ip_config_input")
@@ -33,13 +34,20 @@
 """
 
 class TestConfigIP(object):
+    _old_run_bgp_command = None
     @classmethod
     def setup_class(cls):
         os.environ['UTILITIES_UNIT_TESTING'] = "1"
+        cls._old_run_bgp_command = bgp_util.run_bgp_command
+        bgp_util.run_bgp_command = mock.MagicMock(
+            return_value=cls.mock_run_bgp_command())
         print("SETUP")
 
     ''' Tests for IPv4  '''
 
+    def mock_run_bgp_command():
+        return ""
+
     def test_add_del_interface_valid_ipv4(self):
         db = Db()
         runner = CliRunner()
@@ -330,4 +338,5 @@ def test_del_vrf_invalid_configdb_yang_validation(self):
     @classmethod
     def teardown_class(cls):
         os.environ['UTILITIES_UNIT_TESTING'] = "0"
+        bgp_util.run_bgp_command = cls._old_run_bgp_command
         print("TEARDOWN")

tests/ip_show_routes_test.py

@@ -3,11 +3,15 @@
 
 from . import show_ip_route_common
 from click.testing import CliRunner
+import mock
+import sys
 
 test_path = os.path.dirname(os.path.abspath(__file__))
 modules_path = os.path.dirname(test_path)
 scripts_path = os.path.join(modules_path, "scripts")
 
+sys.path.insert(0, test_path)
+
 
 class TestShowIpRouteCommands(object):
     @classmethod
@@ -18,6 +22,25 @@ def setup_class(cls):
         os.environ["UTILITIES_UNIT_TESTING"] = "0"
         os.environ["UTILITIES_UNIT_TESTING_TOPOLOGY"] = ""
         import mock_tables.dbconnector
+
+    def test_show_ip_route_err(
+            self,
+            setup_ip_route_commands):
+        show = setup_ip_route_commands
+
+        def mock_run_bgp_command(*args, **kwargs):
+            command = args[0]
+            return "% Unknown command: show ip route unknown", 1
+
+        with mock.patch('utilities_common.cli.run_command', mock.MagicMock(side_effect=mock_run_bgp_command)) as mock_run_command:
+            runner = CliRunner()
+            result = runner.invoke(
+                show.cli.commands["ip"].commands["route"], ["unknown"])
+            print("{}".format(result.output))
+            print(result.exit_code)
+            assert result.exit_code == 1
+            assert result.output == "% Unknown command: show ip route unknown" + "\n"
+
     @pytest.mark.parametrize('setup_single_bgp_instance',
                              ['ip_route'], indirect=['setup_single_bgp_instance'])
     def test_show_ip_route(
@@ -117,3 +140,4 @@ def test_show_ipv6_route_err(
         print("{}".format(result.output))
         assert result.exit_code == 0
         assert result.output == show_ip_route_common.show_ipv6_route_err_expected_output + "\n"
+

tests/sonic_bootchart_test.py

@@ -51,9 +51,9 @@ def test_disable(self, mock_run_command):
     def test_config_show(self, mock_run_command):
         def run_command_side_effect(command, **kwargs):
             if "is-enabled" in command:
-                return "enabled"
+                return "enabled", 0
             elif "is-active" in command:
-                return "active"
+                return "active", 0
             else:
                 raise Exception("unknown command")
 

tests/vlan_test.py

@@ -8,6 +8,7 @@
 import show.main as show
 from utilities_common.db import Db
 from importlib import reload
+import utilities_common.bgp_util as bgp_util
 
 show_vlan_brief_output="""\
 +-----------+-----------------+-----------------+----------------+-------------+
@@ -143,16 +144,23 @@
 +-----------+-----------------+-----------------+----------------+-------------+
 """
 class TestVlan(object):
+    _old_run_bgp_command = None
     @classmethod
     def setup_class(cls):
         os.environ['UTILITIES_UNIT_TESTING'] = "1"
         # ensure that we are working with single asic config
+        cls._old_run_bgp_command = bgp_util.run_bgp_command
+        bgp_util.run_bgp_command = mock.MagicMock(
+            return_value=cls.mock_run_bgp_command())
         from .mock_tables import dbconnector
         from .mock_tables import mock_single_asic
         reload(mock_single_asic)
         dbconnector.load_namespace_config()
         print("SETUP")
 
+    def mock_run_bgp_command():
+        return ""
+
     def test_show_vlan(self):
         runner = CliRunner()
         result = runner.invoke(show.cli.commands["vlan"], [])
@@ -579,4 +587,5 @@ def test_config_vlan_add_member_of_portchannel(self):
     @classmethod
     def teardown_class(cls):
         os.environ['UTILITIES_UNIT_TESTING'] = "0"
+        bgp_util.run_bgp_command = cls._old_run_bgp_command
         print("TEARDOWN")

utilities_common/bgp_util.py

@@ -1,6 +1,7 @@
 import ipaddress
 import json
 import re
+import sys
 
 import click
 import utilities_common.cli as clicommon
@@ -185,7 +186,10 @@ def run_bgp_command(vtysh_cmd, bgp_namespace=multi_asic.DEFAULT_NAMESPACE, vtysh
     cmd = 'sudo {} {} -c "{}"'.format(
         vtysh_shell_cmd, bgp_instance_id, vtysh_cmd)
     try:
-        output = clicommon.run_command(cmd, return_cmd=True)
+        output, ret = clicommon.run_command(cmd, return_cmd=True)
+        if ret != 0:
+            click.echo(output.rstrip('\n'))
+            sys.exit(ret)
     except Exception:
         ctx = click.get_current_context()
         ctx.fail("Unable to get summary from bgp {}".format(bgp_instance_id))

utilities_common/cli.py

@@ -539,7 +539,7 @@ def run_command(command, display_cmd=False, ignore_error=False, return_cmd=False
 
     if return_cmd:
         output = proc.communicate()[0]
-        return output
+        return output, proc.returncode
 
     if not interactive_mode:
         (out, err) = proc.communicate()