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.

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

[config]Improve config save cli to save to one file for multiasic #3288

Merged
merged 8 commits into from
May 17, 2024

Conversation

wen587
Copy link
Contributor

@wen587 wen587 commented Apr 24, 2024

HLD design : sonic-net/SONiC#1684
ADO: 27595294

What I did

Add support for config save to one file for multi-aisc.

How I did it

Extend support for one file save for multiasic using the below format:

{
  "localhost": {/*host config*/},
  "asic0": {/*asic0 config*/},
  ...
  "asicN": {/*asicN config*/}
}

How to verify it

Unit test and manual test on multiasic platform.
Example running multi:

admin@str2-8800-sup-2:~$ sudo config save -y tmp.json
Integrate each ASIC's config into a single JSON file tmp.json.
admin@str2-8800-sup-2:~$ cat tmp.json |more
{
    "localhost": {
        "ACL_TABLE": {
            "NTP_ACL": {
                "policy_desc": "NTP_ACL",
                "services": [
                    "NTP"
...
    "asic0": {
        "AUTO_TECHSUPPORT": {
            "GLOBAL": {
                "available_mem_threshold": "10.0",

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)

@wen587 wen587 marked this pull request as ready for review April 24, 2024 03:45
@qiluo-msft
Copy link
Contributor

qiluo-msft commented Apr 26, 2024

Do you have a HLD PR to link? Since this is a new feature, people need to read HLD to understand your big picture and goals in each code PR. #Closed

@qiluo-msft
Copy link
Contributor

/EasyCLA

config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
@xincunli-sonic
Copy link
Contributor

            file = "/etc/sonic/config_db{}.json".format(inst)

How about save all configurations to /etc/sonic too? Also, could we change reload from a single file in same PR which has consistent logic for save and reload?


Refers to: config/main.py:1271 in a30ce28. [](commit_id = a30ce28, deletion_comment = False)

@wen587
Copy link
Contributor Author

wen587 commented Apr 28, 2024

Do you have a HLD PR to link? Since this is a new feature, people need to read HLD to understand your big picture and goals in each code PR.

Will submit a HLD PR for this.

@wen587
Copy link
Contributor Author

wen587 commented Apr 28, 2024

            file = "/etc/sonic/config_db{}.json".format(inst)

How about save all configurations to /etc/sonic too? Also, could we change reload from a single file in same PR which has consistent logic for save and reload?

Refers to: config/main.py:1271 in a30ce28. [](commit_id = a30ce28, deletion_comment = False)

Hi @xincunli-sonic , what do you mean save all configuration to /etc/sonic. If we didn't specify the path, it will save default config_db.json, config_db0.json... to /etc/sonic.

config reload in single file is much more complex than config save. I prefer to split PR for this.

xincunli-sonic
xincunli-sonic previously approved these changes Apr 29, 2024
Copy link
Contributor

@xincunli-sonic xincunli-sonic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@wenyiz2021 wenyiz2021 left a comment

Choose a reason for hiding this comment

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

in this case for multi-asic, when single file name is provided to do config save, we are saving to 1 single file:

in this single file, do we have duplicate content from both host and asics?
what is the consequences of overwriting all config in one file? should we prevent this behavior

@dgsudharsan dgsudharsan removed their request for review May 7, 2024 05:13
wenyiz2021
wenyiz2021 previously approved these changes May 8, 2024
@wen587 wen587 dismissed stale reviews from wenyiz2021 and xincunli-sonic via b810d94 May 14, 2024 09:17
config/main.py Outdated Show resolved Hide resolved
Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

LGTM with minor comment on function name

@wen587 wen587 requested a review from qiluo-msft May 16, 2024 06:13
xincunli-sonic
xincunli-sonic previously approved these changes May 16, 2024
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
xincunli-sonic
xincunli-sonic previously approved these changes May 17, 2024
Copy link
Contributor

@xincunli-sonic xincunli-sonic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@judyjoseph judyjoseph left a comment

Choose a reason for hiding this comment

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

LGTM

@qiluo-msft qiluo-msft merged commit 556026c into sonic-net:master May 17, 2024
7 checks passed
arfeigin pushed a commit to arfeigin/sonic-utilities that referenced this pull request Jun 16, 2024
…nic-net#3288)

HLD design : sonic-net/SONiC#1684

#### What I did
Add support for config save to one file for multi-aisc.
#### How I did it
Extend support for one file save for multiasic using the below format:
```
{
  "localhost": {/*host config*/},
  "asic0": {/*asic0 config*/},
  ...
  "asicN": {/*asicN config*/}
}
```
#### How to verify it
Unit test and manual test on multiasic platform.
Example running multi:
```
admin@str2-8800-sup-2:~$ sudo config save -y tmp.json
Integrate each ASIC's config into a single JSON file tmp.json.
admin@str2-8800-sup-2:~$ cat tmp.json |more
{
    "localhost": {
        "ACL_TABLE": {
            "NTP_ACL": {
                "policy_desc": "NTP_ACL",
                "services": [
                    "NTP"
...
    "asic0": {
        "AUTO_TECHSUPPORT": {
            "GLOBAL": {
                "available_mem_threshold": "10.0",
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants