[config] Add support for multi-ASIC devices by judyjoseph · Pull Request #877 · sonic-net/sonic-utilities

judyjoseph · 2020-04-10T18:18:58Z

- What I did
Changes to the following config commands and scripts for Multi-ASIC devices.

config load : supports config load across all namespaces
config save : supports config save across all namespaces
config reload : supports reload of all services across namespaces.

- How I did it

The following are the changes introduced in this PR

The changes done here are based on the namespace support added in sonic-net/sonic-py-swsssdk#63

(a) Added API's to
> is_multi_asic()
> get_all_namespaces()
> validate_namespace(namespace)

(b) config load/save/reload commands : the action is taken on the linux host + if we are in multi-asic platform repeat the action in the namespaces. The filename argument is optional as before. If not given these are the default files used ( below eg: for a multi-ASIC device with 3 ASIC's )
/etc/sonic/config_db.json --> for global linux host database
/etc/sonic/config_db0.json --> for "asic0" namespace
/etc/sonic/config_db1.json --> for "asic1" namespace
/etc/sonic/config_db2.json --> for "asic2" namespace

If the filename argument needs to be provided it needs to be given as a string of config file names separated by comma

(c) Updates to the db_migrator.py script.

- How to verify it
Verified on multi-ASIC platform

- Previous command output (if the output of a command-line utility has changed)

- New command output (if the output of a command-line utility has changed)

…ates for handing namespace.

config/main.py

jleveque · 2020-04-10T21:08:36Z

config/main.py

    """Clear current configuration and import a previous saved config DB dump file."""
    if not yes:
-        click.confirm('Clear current config and reload config from the file %s?' % filename, abort=True)
+        click.confirm('Clear current config and reload config ?', abort=True)


So, for multi-ASIC platforms, there is no way to specify a file to load from because there are multiple files involved, is this correct? If so, maybe we should consider fixing the file for single-ASIC platforms as well, so that the behavior is consistent for both.

Yes there are multiple files that need to be specified, might be difficult for user also to give as input. Are you referring to removing the filename option itself from the command ? if we remove won't it affect backward compatibility with some scripts using this command may be ?

Yes, we would need to check. Most of the time we just load from the default conifg_db.json file.

I am finding couple of testcases which uses filename option to specify the config_db.json file, there could be other use cases as well.
Hence I have updated the implementation so that the user can input specific config_db.json files even for namespace. This will make sure the behavior is consistent for all platforms. Please take a look.

… per namespace also.

lguohan · 2020-04-21T18:07:14Z

scripts/db_migrator.py

                        default = None )
+        parser.add_argument('-n',
+                        dest='namespace',
+                        metavar='namespace details',


namespace details [](start = 33, length = 17)

asic namespace

lguohan · 2020-04-21T18:07:36Z

scripts/db_migrator.py

+                        metavar='namespace details',
+                        type = str,
+                        required = False,
+                        help = 'The namespace whose DB instance we need to connect',


namespace [](start = 36, length = 9)

asic namespace

lguohan · 2020-04-21T18:09:33Z

config/main.py

-@click.argument('filename', default='/etc/sonic/config_db.json', type=click.Path())
-def save(filename):
+                expose_value=False, prompt='Existing files will be overwritten, continue?')
+@click.argument('filename',nargs=-1)


keep a space after comma

I also suggest for multi-npu case, the filename should be a a list files separated by comma as a single string.

Made the change to the argument "filename" from user to accepts as a single string with filenames separated by comma.

lguohan · 2020-04-21T18:09:52Z

config/main.py

 config.add_command(nat.nat)

 @config.command()
+@click.option('-n', '--namespace', help='Namespace name')


Asic namespace

This option of getting namespace from user is removed.

lguohan · 2020-04-21T20:01:30Z

config/main.py

-    run_command(command, display_cmd=True)
+    if namespace:
+        if not is_multi_asic():
+            click.echo("Namespace is not significant in a Single ASIC platform")


use this is more clear. "Asic namespace is not enabled on this platform!"

This option of getting namespace from user is removed.

lguohan · 2020-04-21T20:02:13Z

config/main.py

+            click.echo("Namespace is not significant in a Single ASIC platform")
+            return None
+        if not validate_namespace(namespace):
+            click.echo("Invalid Namespace entered {}".format(namespace))


Invalid Asic namespace {}.

lguohan · 2020-04-21T20:03:37Z

config/main.py

+            inst = namespace[len(NAMESPACE_PREFIX)]
+            cfg_file = "/etc/sonic/config_db{}.json".format(inst)
+        else:
+            cfg_file = filename[0]


I do not understand the logic here. what is filename[0]?

Ok filename is returned as a tuple, as this is of type nargs=-1. But yes this is wrong in this case as here the tuple has only one element and I can just use cfg_file = filename.

Will fix it and update this and other similar places.

lguohan · 2020-04-21T20:16:38Z

config/main.py

    """Export current config DB to a file on disk."""
-    command = "{} -d --print-data > {}".format(SONIC_CFGGEN_PATH, filename)
-    run_command(command, display_cmd=True)
+    if namespace:


is there a use case to save config db for each individual namespace? if not, then we do not need to provide this option. I feel like everytime we want to save config db, we should save all config dbs.

Ok, I will remove the namespace option. So the only option available to the user is "filename" which he could give as input.

lguohan · 2020-04-21T20:18:14Z

config/main.py

+            num_asic = _get_num_asic()
+            for inst in range(num_asic):
+                namespace = "{}{}".format(NAMESPACE_PREFIX, inst)
+                # Get the file from user input, else take the default file /etc/sonic/config_db{NS_id}.json


if user provide input, we should make sure user provide all 7 filenames. we should not sometimes take user input, sometime use the default values.

Ok, now I have removed the namespace option to the commands - the user can either not provide any filenames , if he provides he needs to provide all namespacecount+1 of them

lguohan · 2020-04-21T20:22:04Z

config/main.py

-@click.argument('filename', default='/etc/sonic/config_db.json', type=click.Path(exists=True))
-def load(filename, yes):
+@click.argument('filename',nargs=-1)
+def load(filename, yes, namespace):


check my comments on the save comments. i think they apply to this command as well.

Take care of.

lguohan · 2020-04-21T20:22:58Z

config/main.py

-    else:
-        command = "{} -j {} --write-to-db".format(SONIC_CFGGEN_PATH, filename)
+    """ In Single AISC platforms we have single DB service. In multi-ASIC platforms we have a global DB
+        service running in the host + DB services running in the namespace created per ASIC.


DB services running in each ASIC namespace

lguohan · 2020-04-21T20:24:24Z

config/main.py

-    client.set(config_db.INIT_INDICATOR, 1)
+        # Get the config file, based on user input 
+        if len(filename) > inst+1:
+            cfg_file = filename[inst+1]


check the user input first.

lguohan · 2020-04-21T20:25:51Z

config/main.py

+        # the default config_db<namespaceID>.json format is used.
+
+        if os.path.isfile(INIT_CFG_FILE):
+            command = "{} -j {} -j {} -n \"{}\" --write-to-db".format(SONIC_CFGGEN_PATH, INIT_CFG_FILE, cfg_file, namespace)


why quote is needed for namespace variable, but not others?

This code change was done earlier as per the initial approach for SonicV2Connector/ConfigDBConnector class .. where we had the default namespace as ''. Now that we have changed that to default None, I have updated the code likewise.

lguohan · 2020-04-21T20:27:27Z

config/main.py

+    for inst in range(-1, num_asic-1):
+        # Get the namespace name, for linux host it is '' empty namespace string.
+        if inst is -1:
+            namespace = ''


for global db, you do not need to supply -n option.

This again was done earlier as per the initial approach for SonicV2Connector/ConfigDBConnector class. Now I have added explicit check for namespace is None and added the -n flag.

…Connector class design

judyjoseph · 2020-04-26T06:15:02Z

There are some testfailures here as the code change needs latest sonic-py-swsssdk , which is still not submodule updated in sonic-buildimage. Please ignore them while reviewing.

qiluo-msft · 2020-04-27T22:20:09Z

@lguohan Why LGTM checker is not enabled in this PR? Could you help setup?

lguohan · 2020-04-27T22:30:26Z

it says enabled.

judyjoseph · 2020-04-30T20:20:39Z

retest this please

config/main.py

jleveque · 2020-05-01T21:44:16Z

Looks good to me. Will wait for @lguohan to review again.

rlhui · 2020-05-02T17:27:31Z

retest vs please

rlhui · 2020-05-02T17:27:41Z

retest vsimage please

* [Phase 1] Multi ASIC config command changes, db_mgrator.py script updates for handing namespace. * Fixes and comment updates * Comments addressed + added support for user to input the config files per namespace also. * Updates per comments + based on the updated SonicV2Connector/ConfigDBConnector class design * Review comments update. * Help string updated for config save/reload/load

config/main.py

judyjoseph added 2 commits April 10, 2020 08:07

[Phase 1] Multi ASIC config command changes, db_mgrator.py script upd…

cddefcf

…ates for handing namespace.

Fixes and comment updates

27cdded

judyjoseph requested review from jleveque and yxieca and removed request for jleveque April 10, 2020 20:56

jleveque changed the title ~~[multi-asic-cfg] config commands changes~~ [config] Add support for multi-ASIC devices Apr 10, 2020

jleveque suggested changes Apr 10, 2020

View reviewed changes

judyjoseph requested a review from lguohan April 12, 2020 02:45

Comments addressed + added support for user to input the config files…

eaa2a2d

… per namespace also.

judyjoseph requested a review from jleveque April 19, 2020 23:22

lguohan reviewed Apr 21, 2020

View reviewed changes

judyjoseph removed the request for review from yxieca April 24, 2020 18:48

Updates per comments + based on the updated SonicV2Connector/ConfigDB…

40873b2

…Connector class design

judyjoseph added Request for 201911 Branch Multi-ASIC 🔢 labels Apr 30, 2020

jleveque suggested changes May 1, 2020

View reviewed changes

config/main.py Outdated Show resolved Hide resolved

config/main.py Outdated Show resolved Hide resolved

Review comments update.

2f11139

judyjoseph requested review from jleveque and lguohan May 1, 2020 15:09

jleveque suggested changes May 1, 2020

View reviewed changes

config/main.py Outdated Show resolved Hide resolved

Help string updated for config save/reload/load

4794beb

jleveque suggested changes May 1, 2020

View reviewed changes

config/main.py Outdated Show resolved Hide resolved

config/main.py Outdated Show resolved Hide resolved

config/main.py Outdated Show resolved Hide resolved

Minor update.

3f06c98

jleveque suggested changes May 1, 2020

View reviewed changes

config/main.py Outdated Show resolved Hide resolved

Minor update

125c297

lguohan approved these changes May 2, 2020

View reviewed changes

jleveque approved these changes May 2, 2020

View reviewed changes

judyjoseph merged commit cb68e7d into sonic-net:master May 2, 2020

judyjoseph deleted the config_cmds branch May 2, 2020 20:51

rlhui added the Included in 201911 Branch label May 4, 2020

prsunny reviewed Sep 11, 2020

View reviewed changes

config/main.py Show resolved Hide resolved

judyjoseph mentioned this pull request Sep 14, 2020

Update to make config load/reload backward compatible. #1115

Merged

zhenggen-xu mentioned this pull request Jun 15, 2021

[config] Fix 'config reload -l' command to get filename by default #1611

Merged

stepanblyschak pushed a commit to stepanblyschak/sonic-utilities that referenced this pull request Apr 28, 2022

[config] Add support for multi-ASIC devices (sonic-net#877)

fed5009

wen587 mentioned this pull request Mar 29, 2024

[multi-asic][202305]: 'config reload -l <>' option loads incorrect config sonic-net/sonic-buildimage#18431

Closed

Conversation

judyjoseph commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

judyjoseph Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

judyjoseph Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

judyjoseph Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

judyjoseph commented Apr 26, 2020

Uh oh!

qiluo-msft commented Apr 27, 2020

Uh oh!

lguohan commented Apr 27, 2020

Uh oh!

judyjoseph commented Apr 10, 2020 •

edited

Loading

judyjoseph Apr 30, 2020 •

edited

Loading

judyjoseph Apr 30, 2020 •

edited

Loading

judyjoseph Apr 30, 2020 •

edited

Loading