Skip to content

Commit

Permalink
Revert "sonic-utilities: Update config reload() to verify formatting …
Browse files Browse the repository at this point in the history 
…of an input file (#2529)" (#2595)

What I did
There are use cases like config reload /dev/stdin, for example L2 Switch mode · sonic-net/SONiC Wiki (github.com). The original PR would read input file twice, so /dev/stdin does not work.

How I did it
Reverts #2529

How to verify it
Run unit test
  • Loading branch information
@ganglyu
ganglyu authored Jan 13, 2023
1 parent 3442815 commit 4bd35e6
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 208 deletions.
39 changes: 6 additions & 33 deletions config/main.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1189,17 +1189,6 @@ def load_backend_acl(cfg_db, device_type):
if os.path.isfile(BACKEND_ACL_FILE):
clicommon.run_command("acl-loader update incremental {}".format(BACKEND_ACL_FILE), display_cmd=True)

def validate_config_file(file):
"""
A validator to check config files for syntax errors
"""
try:
# Load golden config json
read_json_file(file)
except Exception as e:
click.secho("Bad format: json file '{}' broken.\n{}".format(file, str(e)),
fg='magenta')
sys.exit(1)

# This is our main entrypoint - the main 'config' command
@click.group(cls=clicommon.AbbreviationGroup, context_settings=CONTEXT_SETTINGS)
Expand Down Expand Up @@ -1578,8 +1567,10 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form
click.echo("Input {} config file(s) separated by comma for multiple files ".format(num_cfg_file))
return

# Create a dictionary to store each cfg_file, namespace, and a bool representing if a the file exists
cfg_file_dict = {}
#Stop services before config push
if not no_service_restart:
log.log_info("'reload' stopping services...")
_stop_services()

# In Single ASIC platforms we have single DB service. In multi-ASIC platforms we have a global DB
# service running in the host + DB services running in each ASIC namespace created per ASIC.
Expand All @@ -1604,27 +1595,9 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form
else:
file = DEFAULT_CONFIG_YANG_FILE

# Check if the file exists before proceeding
# Instead of exiting, skip the current namespace and check the next one
if not os.path.exists(file):
cfg_file_dict[inst] = [file, namespace, False]
continue
cfg_file_dict[inst] = [file, namespace, True]

# Check the file is properly formatted before proceeding.
validate_config_file(file)

#Validate INIT_CFG_FILE if it exits
if os.path.isfile(INIT_CFG_FILE):
validate_config_file(INIT_CFG_FILE)

#Stop services before config push
if not no_service_restart:
log.log_info("'reload' stopping services...")
_stop_services()

for file, namespace, file_exists in cfg_file_dict.values():
if not file_exists:
# Check the file exists before proceeding.
if not os.path.exists(file):
click.echo("The config file {} doesn't exist".format(file))
continue

Expand Down
62 changes: 0 additions & 62 deletions tests/config_reload_input/config_db_invalid.json
View file

This file was deleted.

115 changes: 2 additions & 113 deletions tests/config_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import traceback
import json
import jsonpatch
import shutil
import sys
import unittest
import ipaddress
Expand Down Expand Up @@ -89,12 +88,6 @@
Reloading Monit configuration ...
"""

RELOAD_CONFIG_DB_OUTPUT_INVALID_MSG = """\
Bad format: json file"""

RELOAD_CONFIG_DB_OUTPUT_INVALID_ERROR = """\
Expecting ',' delimiter: line 12 column 5 (char 321)"""

RELOAD_YANG_CFG_OUTPUT = """\
Stopping SONiC target ...
Running command: /usr/local/bin/sonic-cfggen -Y /tmp/config.json --write-to-db
Expand All @@ -111,15 +104,6 @@
Reloading Monit configuration ...
"""

RELOAD_MASIC_CONFIG_DB_OUTPUT_FILE_NOT_EXIST = """\
Stopping SONiC target ...
Running command: /usr/local/bin/sonic-cfggen -j /tmp/config.json --write-to-db
The config file non_exist.json doesn't exist
Running command: /usr/local/bin/sonic-cfggen -j /tmp/config.json -n asic1 --write-to-db
Restarting SONiC target ...
Reloading Monit configuration ...
"""

reload_config_with_sys_info_command_output="""\
Running command: /usr/local/bin/sonic-cfggen -H -k Seastone-DX010-25-50 --write-to-db"""

Expand Down Expand Up @@ -211,7 +195,6 @@ def mock_run_command_side_effect_gnmi(*args, **kwargs):

class TestConfigReload(object):
dummy_cfg_file = os.path.join(os.sep, "tmp", "config.json")
dummy_cfg_file_contents = os.path.join(mock_db_path, "config_db.json")

@classmethod
def setup_class(cls):
Expand All @@ -223,8 +206,7 @@ def setup_class(cls):

import config.main
importlib.reload(config.main)
shutil.copyfile(cls.dummy_cfg_file_contents, cls.dummy_cfg_file)
open(cls.dummy_cfg_file, 'r').close()
open(cls.dummy_cfg_file, 'w').close()

def test_config_reload(self, get_cmd_module, setup_single_broadcom_asic):
with mock.patch("utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_side_effect)) as mock_run_command:
Expand Down Expand Up @@ -497,17 +479,14 @@ def teardown_class(cls):

class TestReloadConfig(object):
dummy_cfg_file = os.path.join(os.sep, "tmp", "config.json")
dummy_cfg_file_contents = os.path.join(mock_db_path, "config_db.json")
dummy_cfg_file_invalid = os.path.join(mock_db_path, "config_db_invalid.json")

@classmethod
def setup_class(cls):
os.environ['UTILITIES_UNIT_TESTING'] = "1"
print("SETUP")
import config.main
importlib.reload(config.main)
shutil.copyfile(cls.dummy_cfg_file_contents, cls.dummy_cfg_file)
open(cls.dummy_cfg_file, 'r').close()
open(cls.dummy_cfg_file, 'w').close()

def test_reload_config(self, get_cmd_module, setup_single_broadcom_asic):
with mock.patch(
Expand All @@ -528,27 +507,6 @@ def test_reload_config(self, get_cmd_module, setup_single_broadcom_asic):
assert "\n".join([l.rstrip() for l in result.output.split('\n')]) \
== RELOAD_CONFIG_DB_OUTPUT

def test_reload_config_invalid_config_file(self, get_cmd_module, setup_single_broadcom_asic):
with mock.patch(
"utilities_common.cli.run_command",
mock.MagicMock(side_effect=mock_run_command_side_effect)
) as mock_run_command:
(config, show) = get_cmd_module
runner = CliRunner()

result = runner.invoke(
config.config.commands["reload"],
[self.dummy_cfg_file_invalid, '-y', '-f'])

print(result.exit_code)
print(result.output)
traceback.print_tb(result.exc_info[2])
assert result.exit_code == 1

output = "\n".join([l.rstrip() for l in result.output.split('\n')])
assert RELOAD_CONFIG_DB_OUTPUT_INVALID_MSG in output
assert RELOAD_CONFIG_DB_OUTPUT_INVALID_ERROR in output

def test_config_reload_disabled_service(self, get_cmd_module, setup_single_broadcom_asic):
with mock.patch(
"utilities_common.cli.run_command",
Expand Down Expand Up @@ -591,54 +549,6 @@ def test_reload_config_masic(self, get_cmd_module, setup_multi_broadcom_masic):
assert "\n".join([l.rstrip() for l in result.output.split('\n')]) \
== RELOAD_MASIC_CONFIG_DB_OUTPUT

def test_reload_config_masic_invalid(self, get_cmd_module, setup_multi_broadcom_masic):
with mock.patch(
"utilities_common.cli.run_command",
mock.MagicMock(side_effect=mock_run_command_side_effect)
) as mock_run_command:
(config, show) = get_cmd_module
runner = CliRunner()
# 3 config files: 1 for host and 2 for asic
cfg_files = "{},{},{}".format(
self.dummy_cfg_file,
self.dummy_cfg_file_invalid,
self.dummy_cfg_file)
result = runner.invoke(
config.config.commands["reload"],
[cfg_files, '-y', '-f'])

print(result.exit_code)
print(result.output)
traceback.print_tb(result.exc_info[2])
assert result.exit_code == 1

output = "\n".join([l.rstrip() for l in result.output.split('\n')])
assert RELOAD_CONFIG_DB_OUTPUT_INVALID_MSG in output
assert RELOAD_CONFIG_DB_OUTPUT_INVALID_ERROR in output

def test_reload_config_masic_non_exist_file(self, get_cmd_module, setup_multi_broadcom_masic):
with mock.patch(
"utilities_common.cli.run_command",
mock.MagicMock(side_effect=mock_run_command_side_effect)
) as mock_run_command:
(config, show) = get_cmd_module
runner = CliRunner()
# 3 config files: 1 for host and 2 for asic
cfg_files = "{},{},{}".format(
self.dummy_cfg_file,
"non_exist.json",
self.dummy_cfg_file)
result = runner.invoke(
config.config.commands["reload"],
[cfg_files, '-y', '-f'])

print(result.exit_code)
print(result.output)
traceback.print_tb(result.exc_info[2])
assert result.exit_code == 0
assert "\n".join([l.rstrip() for l in result.output.split('\n')]) \
== RELOAD_MASIC_CONFIG_DB_OUTPUT_FILE_NOT_EXIST

def test_reload_yang_config(self, get_cmd_module,
setup_single_broadcom_asic):
with mock.patch(
Expand All @@ -658,27 +568,6 @@ def test_reload_yang_config(self, get_cmd_module,
assert "\n".join([l.rstrip() for l in result.output.split('\n')]) \
== RELOAD_YANG_CFG_OUTPUT

def test_reload_yang_config_invalid(self, get_cmd_module,
setup_single_broadcom_asic):
with mock.patch(
"utilities_common.cli.run_command",
mock.MagicMock(side_effect=mock_run_command_side_effect)
) as mock_run_command:
(config, show) = get_cmd_module
runner = CliRunner()

result = runner.invoke(config.config.commands["reload"],
[self.dummy_cfg_file_invalid, '-y', '-f', '-t', 'config_yang'])

print(result.exit_code)
print(result.output)
traceback.print_tb(result.exc_info[2])
assert result.exit_code == 1

output = "\n".join([l.rstrip() for l in result.output.split('\n')])
assert RELOAD_CONFIG_DB_OUTPUT_INVALID_MSG in output
assert RELOAD_CONFIG_DB_OUTPUT_INVALID_ERROR in output

@classmethod
def teardown_class(cls):
os.environ['UTILITIES_UNIT_TESTING'] = "0"
Expand Down

0 comments on commit 4bd35e6

Please sign in to comment.