From 31a6584cc250552cc2f4166ee828de1f88623830 Mon Sep 17 00:00:00 2001 From: Mai Bui Date: Tue, 6 Feb 2024 04:06:36 -0500 Subject: [PATCH] Fix `sudo config load_mgmt_config` fails with error "File /var/run/dhclient.eth0.pid does not exist" (#3149) * Fix load_mgmt_config not exit when dhclient.eth0.pid not exists Signed-off-by: Mai Bui * add UT Signed-off-by: Mai Bui --------- Signed-off-by: Mai Bui --- config/main.py | 20 +++-- tests/config_test.py | 171 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 175 insertions(+), 16 deletions(-) diff --git a/config/main.py b/config/main.py index 5eb2634614..a64a26a97c 100644 --- a/config/main.py +++ b/config/main.py @@ -1683,17 +1683,15 @@ def load_mgmt_config(filename): clicommon.run_command(command, display_cmd=True, ignore_error=True) if len(config_data['MGMT_INTERFACE'].keys()) > 0: filepath = '/var/run/dhclient.eth0.pid' - if not os.path.isfile(filepath): - sys.exit('File {} does not exist'.format(filepath)) - - out0, rc0 = clicommon.run_command(['cat', filepath], display_cmd=True, return_cmd=True) - if rc0 != 0: - sys.exit('Exit: {}. Command: cat {} failed.'.format(rc0, filepath)) - - out1, rc1 = clicommon.run_command(['kill', str(out0).strip('\n')], return_cmd=True) - if rc1 != 0: - sys.exit('Exit: {}. Command: kill {} failed.'.format(rc1, out0)) - clicommon.run_command(['rm', '-f', filepath], display_cmd=True, return_cmd=True) + if os.path.isfile(filepath): + out0, rc0 = clicommon.run_command(['cat', filepath], display_cmd=True, return_cmd=True) + if rc0 != 0: + sys.exit('Exit: {}. Command: cat {} failed.'.format(rc0, filepath)) + + out1, rc1 = clicommon.run_command(['kill', str(out0).strip('\n')], display_cmd=True, return_cmd=True) + if rc1 != 0: + sys.exit('Exit: {}. Command: kill {} failed.'.format(rc1, out0)) + clicommon.run_command(['rm', '-f', filepath], display_cmd=True, return_cmd=True) click.echo("Please note loaded setting will be lost after system reboot. To preserve setting, run `config save`.") @config.command("load_minigraph") diff --git a/tests/config_test.py b/tests/config_test.py index c773ad29cb..cc0ac22e98 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -70,7 +70,8 @@ Running command: ifconfig eth0 10.0.0.100 netmask 255.255.255.0 Running command: ip route add default via 10.0.0.1 dev eth0 table default Running command: ip rule add from 10.0.0.100 table default -Running command: kill `cat /var/run/dhclient.eth0.pid` +Running command: cat /var/run/dhclient.eth0.pid +Running command: kill 101 Running command: rm -f /var/run/dhclient.eth0.pid Please note loaded setting will be lost after system reboot. To preserve setting, run `config save`. """ @@ -82,7 +83,8 @@ Running command: ifconfig eth0 add fc00:1::32/64 Running command: ip -6 route add default via fc00:1::1 dev eth0 table default Running command: ip -6 rule add from fc00:1::32 table default -Running command: kill `cat /var/run/dhclient.eth0.pid` +Running command: cat /var/run/dhclient.eth0.pid +Running command: kill 101 Running command: rm -f /var/run/dhclient.eth0.pid Please note loaded setting will be lost after system reboot. To preserve setting, run `config save`. """ @@ -97,11 +99,41 @@ Running command: ifconfig eth0 add fc00:1::32/64 Running command: ip -6 route add default via fc00:1::1 dev eth0 table default Running command: ip -6 rule add from fc00:1::32 table default -Running command: kill `cat /var/run/dhclient.eth0.pid` +Running command: cat /var/run/dhclient.eth0.pid +Running command: kill 101 Running command: rm -f /var/run/dhclient.eth0.pid Please note loaded setting will be lost after system reboot. To preserve setting, run `config save`. """ +load_mgmt_config_command_ipv4_ipv6_cat_failed_output="""\ +Running command: /usr/local/bin/sonic-cfggen -M device_desc.xml --write-to-db +parse dummy device_desc.xml +change hostname to dummy +Running command: ifconfig eth0 10.0.0.100 netmask 255.255.255.0 +Running command: ip route add default via 10.0.0.1 dev eth0 table default +Running command: ip rule add from 10.0.0.100 table default +Running command: ifconfig eth0 add fc00:1::32/64 +Running command: ip -6 route add default via fc00:1::1 dev eth0 table default +Running command: ip -6 rule add from fc00:1::32 table default +Running command: cat /var/run/dhclient.eth0.pid +Exit: 2. Command: cat /var/run/dhclient.eth0.pid failed. +""" + +load_mgmt_config_command_ipv4_ipv6_kill_failed_output="""\ +Running command: /usr/local/bin/sonic-cfggen -M device_desc.xml --write-to-db +parse dummy device_desc.xml +change hostname to dummy +Running command: ifconfig eth0 10.0.0.100 netmask 255.255.255.0 +Running command: ip route add default via 10.0.0.1 dev eth0 table default +Running command: ip rule add from 10.0.0.100 table default +Running command: ifconfig eth0 add fc00:1::32/64 +Running command: ip -6 route add default via fc00:1::1 dev eth0 table default +Running command: ip -6 rule add from fc00:1::32 table default +Running command: cat /var/run/dhclient.eth0.pid +Running command: kill 104 +Exit: 4. Command: kill 104 failed. +""" + RELOAD_CONFIG_DB_OUTPUT = """\ Stopping SONiC target ... Running command: /usr/local/bin/sonic-cfggen -j /tmp/config.json --write-to-db @@ -143,8 +175,6 @@ def mock_run_command_side_effect(*args, **kwargs): command = ' '.join(command) if kwargs.get('display_cmd'): - if 'cat /var/run/dhclient.eth0.pid' in command: - command = 'kill `cat /var/run/dhclient.eth0.pid`' click.echo(click.style("Running command: ", fg='cyan') + click.style(command, fg='green')) if kwargs.get('return_cmd'): @@ -154,6 +184,54 @@ def mock_run_command_side_effect(*args, **kwargs): return 'sonic.target\nswss', 0 elif command == "systemctl is-enabled snmp.timer": return 'enabled', 0 + elif command == 'cat /var/run/dhclient.eth0.pid': + return '101', 0 + else: + return '', 0 + +def mock_run_command_cat_failed_side_effect(*args, **kwargs): + command = args[0] + if isinstance(command, str): + command = command + elif isinstance(command, list): + command = ' '.join(command) + + if kwargs.get('display_cmd'): + click.echo(click.style("Running command: ", fg='cyan') + click.style(command, fg='green')) + + if kwargs.get('return_cmd'): + if command == "systemctl list-dependencies --plain sonic-delayed.target | sed '1d'": + return 'snmp.timer', 0 + elif command == "systemctl list-dependencies --plain sonic.target": + return 'sonic.target\nswss', 0 + elif command == "systemctl is-enabled snmp.timer": + return 'enabled', 0 + elif command == 'cat /var/run/dhclient.eth0.pid': + return '102', 2 + else: + return '', 0 + +def mock_run_command_kill_failed_side_effect(*args, **kwargs): + command = args[0] + if isinstance(command, str): + command = command + elif isinstance(command, list): + command = ' '.join(command) + + if kwargs.get('display_cmd'): + click.echo(click.style("Running command: ", fg='cyan') + click.style(command, fg='green')) + + if kwargs.get('return_cmd'): + if command == "systemctl list-dependencies --plain sonic-delayed.target | sed '1d'": + return 'snmp.timer', 0 + elif command == "systemctl list-dependencies --plain sonic.target": + return 'sonic.target\nswss', 0 + elif command == "systemctl is-enabled snmp.timer": + return 'enabled', 0 + elif command == 'cat /var/run/dhclient.eth0.pid': + return '104', 0 + elif command == 'kill 104': + return 'Failed to kill 104', 4 else: return '', 0 @@ -1663,6 +1741,89 @@ def change_hostname_side_effect(hostname): assert "\n".join([l.rstrip() for l in result.output.split('\n')]) == expected_output assert mock_run_command.call_count == expected_command_call_count + + def test_config_load_mgmt_config_ipv4_ipv6_cat_failed(self, get_cmd_module, setup_single_broadcom_asic): + device_desc_result = { + 'DEVICE_METADATA': { + 'localhost': { + 'hostname': 'dummy' + } + }, + 'MGMT_INTERFACE': { + ('eth0', '10.0.0.100/24') : { + 'gwaddr': ipaddress.ip_address(u'10.0.0.1') + }, + ('eth0', 'FC00:1::32/64') : { + 'gwaddr': ipaddress.ip_address(u'fc00:1::1') + } + } + } + self.check_output_cat_failed(get_cmd_module, device_desc_result, load_mgmt_config_command_ipv4_ipv6_cat_failed_output, 8) + + def check_output_cat_failed(self, get_cmd_module, parse_device_desc_xml_result, expected_output, expected_command_call_count): + def parse_device_desc_xml_side_effect(filename): + print("parse dummy device_desc.xml") + return parse_device_desc_xml_result + def change_hostname_side_effect(hostname): + print("change hostname to {}".format(hostname)) + with mock.patch("utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_cat_failed_side_effect)) as mock_run_command: + with mock.patch('os.path.isfile', mock.MagicMock(return_value=True)): + with mock.patch('config.main.parse_device_desc_xml', mock.MagicMock(side_effect=parse_device_desc_xml_side_effect)): + with mock.patch('config.main._change_hostname', mock.MagicMock(side_effect=change_hostname_side_effect)): + (config, show) = get_cmd_module + runner = CliRunner() + with runner.isolated_filesystem(): + with open('device_desc.xml', 'w') as f: + f.write('dummy') + result = runner.invoke(config.config.commands["load_mgmt_config"], ["-y", "device_desc.xml"]) + print(result.exit_code) + print(result.output) + traceback.print_tb(result.exc_info[2]) + assert result.exit_code == 1 + assert "\n".join([l.rstrip() for l in result.output.split('\n')]) == expected_output + assert mock_run_command.call_count == expected_command_call_count + + def test_config_load_mgmt_config_ipv4_ipv6_kill_failed(self, get_cmd_module, setup_single_broadcom_asic): + device_desc_result = { + 'DEVICE_METADATA': { + 'localhost': { + 'hostname': 'dummy' + } + }, + 'MGMT_INTERFACE': { + ('eth0', '10.0.0.100/24') : { + 'gwaddr': ipaddress.ip_address(u'10.0.0.1') + }, + ('eth0', 'FC00:1::32/64') : { + 'gwaddr': ipaddress.ip_address(u'fc00:1::1') + } + } + } + self.check_output_kill_failed(get_cmd_module, device_desc_result, load_mgmt_config_command_ipv4_ipv6_kill_failed_output, 9) + + def check_output_kill_failed(self, get_cmd_module, parse_device_desc_xml_result, expected_output, expected_command_call_count): + def parse_device_desc_xml_side_effect(filename): + print("parse dummy device_desc.xml") + return parse_device_desc_xml_result + def change_hostname_side_effect(hostname): + print("change hostname to {}".format(hostname)) + with mock.patch("utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_kill_failed_side_effect)) as mock_run_command: + with mock.patch('os.path.isfile', mock.MagicMock(return_value=True)): + with mock.patch('config.main.parse_device_desc_xml', mock.MagicMock(side_effect=parse_device_desc_xml_side_effect)): + with mock.patch('config.main._change_hostname', mock.MagicMock(side_effect=change_hostname_side_effect)): + (config, show) = get_cmd_module + runner = CliRunner() + with runner.isolated_filesystem(): + with open('device_desc.xml', 'w') as f: + f.write('dummy') + result = runner.invoke(config.config.commands["load_mgmt_config"], ["-y", "device_desc.xml"]) + print(result.exit_code) + print(result.output) + traceback.print_tb(result.exc_info[2]) + assert result.exit_code == 1 + assert "\n".join([l.rstrip() for l in result.output.split('\n')]) == expected_output + assert mock_run_command.call_count == expected_command_call_count + @classmethod def teardown_class(cls): print("TEARDOWN")