[202205][show]Fix show route return code on error (#2547)

Porting of changes #2542 to 202205

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

config/main.py

@@ -767,13 +767,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 = []
@@ -808,16 +808,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):
@@ -842,7 +842,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):
@@ -2631,7 +2631,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

@@ -73,7 +73,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'),)

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

@@ -124,13 +124,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]
@@ -140,17 +140,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]
@@ -160,15 +160,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
 
 # Load sonic-cfggen from source since /usr/local/bin/sonic-cfggen does not have .py extension.
 sonic_cfggen = load_module_from_source('sonic_cfggen', '/usr/local/bin/sonic-cfggen')

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

@@ -9,6 +9,7 @@
 import config.main as config
 import show.main as show
 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")
@@ -30,13 +31,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()
@@ -284,4 +292,5 @@ def test_intf_unknown_vrf_bind(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/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()