Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[load_minigraph]: Add support to load default_config.json file #1228

Closed
wants to merge 1 commit into from
Closed

[load_minigraph]: Add support to load default_config.json file #1228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 38 additions & 1 deletion config/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,22 @@ def _get_device_type():

return device_type

def _get_hwsku(config_db):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove this function here. Same function available in sonic_py_common get_hwsku

Copy link
Contributor Author

@mlorrillere mlorrillere Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_hwsku in sonic_py_common does not support namespaces, it will always connect to the host redis instance.

Also, get_hwsku in sonic_py_common will be stuck because it will try to open a new connection to CONFIG_DB.

This can be revisited when sonic_py_common provides support for namespaces and existing connection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hwsku is for the whole device, wouldn't getting the hwsku form the host redis work will it change for each namespace/asic ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be the same hwsku for all namespace/asic. However we can't get it before minigraph.xml is parsed and CONFIG_DB is populated, so calling get_hwsku from sonic_py_common beforehand is not possible since CONFIG_DB will be empty.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if config_db is empty then _get_hwsku won't return the correct hwsku also. I am trying to understand what the difference between _get_hwsku define here and get_hwsku in sonic_py_common

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_get_hwsku is called after minigraph.xml is parsed and CONFIG_DB is populated for the corresponding namespace CONFIG_DB initialized at line 1274, _get_hwsku is called at line 1287).

The difference with get_hwsku from sonic_py_common is that _get_hwsku here reuse the existing connection to CONFIG_DB. If we cann get_hwsku from sonic_py_common instead it will get stuck because we can't open multiple connections the same time.

"""
Get hwsku from CONFIG_DB

TODO: move to sonic-py-common
"""

metadata = config_db.get_table('DEVICE_METADATA')
if metadata and 'localhost' in metadata and 'hwsku' in metadata['localhost']:
return metadata['localhost']['hwsku']

click.echo("Could not get the hwsku from CONFIG_DB, setting hwsku to Unknown")
hwsku = 'Unknown'

return hwsku

def interface_alias_to_name(config_db, interface_alias):
"""Return default interface name if alias name is given as argument
"""
Expand Down Expand Up @@ -1284,11 +1300,13 @@ def load_minigraph(db, no_service_restart):
log.log_info("'load_minigraph' stopping services...")
_stop_services()

platform = device_info.get_platform()

# For Single Asic platform the namespace list has the empty string
# for mulit Asic platform the empty string to generate the config
# for host
namespace_list = [DEFAULT_NAMESPACE]
num_npus = multi_asic.get_num_asics()
num_npus = device_info.get_num_npus()
if num_npus > 1:
namespace_list += multi_asic.get_namespaces_from_linux()

Expand All @@ -1309,6 +1327,25 @@ def load_minigraph(db, no_service_restart):
else:
command = "{} -H -m --write-to-db {}".format(SONIC_CFGGEN_PATH, cfggen_namespace_option)
clicommon.run_command(command, display_cmd=True)

if namespace is DEFAULT_NAMESPACE:
npu_id = ''
else:
npu_id = device_info.get_npu_id_from_name(namespace)

# load default_config.json file from platform directory
default_config_file = os.path.join('/usr/share/sonic/device/', platform, npu_id, 'default_config.json')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you give an example what is present in 'default_config.json'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the following example in the description:

{
    "DEVICE_METADATA": {
        "localhost": {
            "instance_name": "ASIC0",
            "chassis_db_address" : "240.127.1.1",
            "start_chassis_db" : "1",
            "asic_id": "06:00.0"
        }
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per PR sonic-net/sonic-buildimage#5991, for VOQ chassis systems the "asic_id" and "instance_name" are configured via minigraph.xml. Will these values from default_config.json be preferred over what is configured through minigraph?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For VOQ chassis systems, we get "start_chassis_db" and "chassis_db_address" from chassisdb.conf from device specific directory (for example: https://github.com/Azure/sonic-buildimage/blob/master/device/arista/x86_64-arista_7800_sup/chassisdb.conf). These are needed before the database service is started in supervisor card. Having these in config_db is of no use. Also, if these are not used in multi-asic we should not have these in default_config.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per PR Azure/sonic-buildimage#5991, for VOQ chassis systems the "asic_id" and "instance_name" are configured via minigraph.xml. Will these values from default_config.json be preferred over what is configured through minigraph?

Both are still valid. If the platform has configuration in default_config.json it has to be static. If it is present in both files then default_config.json will override the value configured via minigraph.xml, which is fine.

The example above is probably incorrect once VoQ config using minigraph.xml is complete (PR5991), however asic_id is still required in this file as it is platform-dependent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For VOQ chassis systems, we get "start_chassis_db" and "chassis_db_address" from chassisdb.conf from device specific directory (for example: https://github.com/Azure/sonic-buildimage/blob/master/device/arista/x86_64-arista_7800_sup/chassisdb.conf). These are needed before the database service is started in supervisor card. Having these in config_db is of no use. Also, if these are not used in multi-asic we should not have these in default_config.json.

Right. Actually in the above example only asic_id is required as it is tied to each asic physically on the hardware. Other configurations can easily be set using minigraph.xml

if os.path.isfile(default_config_file):
command = "{} -j {} {} --write-to-db".format(SONIC_CFGGEN_PATH, default_config_file, cfggen_namespace_option)
clicommon.run_command(command, display_cmd=True)

hwsku = _get_hwsku(config_db)
# load default_config.json file from hwsku directory
default_config_file = os.path.join('/usr/share/sonic/device/', platform, hwsku, npu_id, 'default_config.json')
if os.path.isfile(default_config_file):
command = "{} -j {} {} --write-to-db".format(SONIC_CFGGEN_PATH, default_config_file, cfggen_namespace_option)
clicommon.run_command(command, display_cmd=True)

client.set(config_db.INIT_INDICATOR, 1)

# get the device type
Expand Down
53 changes: 49 additions & 4 deletions tests/config_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,32 @@

import config.main as config

load_minigraph_command_output="""\
Stopping SONiC target ...
Running command: /usr/local/bin/sonic-cfggen -H -m --write-to-db
def get_load_minigraph_command_output(init_cfg=False, default_config=False):
load_minigraph_command_output="""\
Stopping SONiC target ..."""

if init_cfg:
load_minigraph_command_output += """
Running command: /usr/local/bin/sonic-cfggen -H -m -j /etc/sonic/init_cfg.json --write-to-db"""
else:
load_minigraph_command_output += """
Running command: /usr/local/bin/sonic-cfggen -H -m --write-to-db"""

if default_config:
load_minigraph_command_output += """
Running command: /usr/local/bin/sonic-cfggen -j /usr/share/sonic/device/x86_64-mlnx_msn3800-r0/default_config.json --write-to-db
Running command: /usr/local/bin/sonic-cfggen -j /usr/share/sonic/device/x86_64-mlnx_msn3800-r0/Arista-VM/default_config.json --write-to-db"""

load_minigraph_command_output += """
Running command: pfcwd start_default
Running command: config qos reload --no-dynamic-buffer
Restarting SONiC target ...
Reloading Monit configuration ...
Please note setting loaded from minigraph will be lost after system reboot. To preserve setting, run `config save`.
"""

return load_minigraph_command_output

def mock_run_command_side_effect(*args, **kwargs):
command = args[0]

Expand Down Expand Up @@ -55,9 +71,38 @@ def test_load_minigraph(self, get_cmd_module, setup_single_broadcom_asic):
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')]) == load_minigraph_command_output
assert "\n".join([l.rstrip() for l in result.output.split('\n')]) == get_load_minigraph_command_output()
assert mock_run_command.call_count == 7

def test_load_minigraph_with_init_cfg(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, \
mock.patch("os.path.isfile") as mock_isfile:
mock_isfile.side_effect = lambda filename: filename.endswith( "init_cfg.json" )
(config, show) = get_cmd_module
runner = CliRunner()
result = runner.invoke(config.config.commands["load_minigraph"], ["-y"])
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')]) == get_load_minigraph_command_output( init_cfg=True )
assert mock_run_command.call_count == 7

def test_load_minigraph_with_default_config(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, \
mock.patch("os.path.isfile") as mock_isfile:
mock_isfile.side_effect = lambda filename: filename.endswith( "default_config.json" )
(config, show) = get_cmd_module
runner = CliRunner()
result = runner.invoke(config.config.commands["load_minigraph"], ["-y"])
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')]) == get_load_minigraph_command_output( default_config=True )
assert mock_run_command.call_count == 9


@classmethod
def teardown_class(cls):
os.environ['UTILITIES_UNIT_TESTING'] = "0"
Expand Down
1 change: 1 addition & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ def set_mock_apis():
cwd = os.path.dirname(os.path.realpath(__file__))
config.asic_type = mock.MagicMock(return_value="broadcom")
config._get_device_type = mock.MagicMock(return_value="ToRRouter")
config._get_hwsku = mock.MagicMock(return_value="Arista-VM")

@pytest.fixture
def setup_qos_mock_apis():
Expand Down