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]config reload should generate sysinfo if missing #2778

Merged
merged 4 commits into from
May 1, 2023

Conversation

wen587
Copy link
Contributor

@wen587 wen587 commented Apr 4, 2023

What I did

Missing platform and mac in CONFIG_DB will result in container failure. We should make the config reload generate those info if missing.

How I did it

Add missing sys info if config_db.json doesn't contain it.

How to verify it

Unit test

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 requested a review from qiluo-msft April 4, 2023 09:00
@wen587 wen587 marked this pull request as ready for review April 4, 2023 09:00
@@ -1560,6 +1560,17 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form
click.echo("The config file {} doesn't exist".format(file))
continue

if file_format == 'config_db':
Copy link
Contributor

@qiluo-msft qiluo-msft Apr 8, 2023

Choose a reason for hiding this comment

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

Is this feature already implement by argument load_sysinfo? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The purpose of the PR is to keep the CLI cmd identical for Golden Config reload.
From NDM team's knowledge, they don't have such info from their side.

@wen587 wen587 requested a review from qiluo-msft April 10, 2023 05:18
@qiluo-msft
Copy link
Contributor

qiluo-msft commented Apr 10, 2023

@click.option('-o', '--override_config', default=False, is_flag=True, help='Enable config override. Proceed with default path.')

Is it also a bug inside override_config logic?


In reply to: 1502458655


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

@wen587
Copy link
Contributor Author

wen587 commented Apr 11, 2023

@click.option('-o', '--override_config', default=False, is_flag=True, help='Enable config override. Proceed with default path.')

Is it also a bug inside override_config logic?

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

I think so. I will submit a fix in this PR: #2738

config/main.py Outdated Show resolved Hide resolved
config/main.py Outdated Show resolved Hide resolved
@wen587 wen587 merged commit 88ffb16 into sonic-net:master May 1, 2023
dprital added a commit to dprital/sonic-buildimage that referenced this pull request May 1, 2023
Update sonic-utilities submodule pointer to include the following:
* 88ffb167 [config]config reload should generate sysinfo if missing ([sonic-net#2778](sonic-net/sonic-utilities#2778))
* 7443b9e5 [sonic-package-manager] support extension with multiple YANG modules ([sonic-net#2752](sonic-net/sonic-utilities#2752))
* 522c3a9e [sonic-package-manager] add support for multiple CLI plugin files ([sonic-net#2753](sonic-net/sonic-utilities#2753))
* b38fcfd1 [show][muxcable] fix  RC ([sonic-net#2812](sonic-net/sonic-utilities#2812))
* 7e24463f [chassis]: remote cli commands infra for sonic chassis ([sonic-net#2701](sonic-net/sonic-utilities#2701))
* bee593e4 [DPB]Fixing typo in config breakout output ([sonic-net#2802](sonic-net/sonic-utilities#2802))
* ada603c5 [config]Support multi-asic  Golden Config override ([sonic-net#2738](sonic-net/sonic-utilities#2738))
* 88a7daa8 [show][barefoot] replace shell=True ([sonic-net#2699](sonic-net/sonic-utilities#2699))
* 5e99edb5 [sonic_package_manager] replace shell=True ([sonic-net#2726](sonic-net/sonic-utilities#2726))
* b547bb45 [acl-loader] Only add default deny rule when table is L3 or L3V6 ([sonic-net#2796](sonic-net/sonic-utilities#2796))

Signed-off-by: dprital <[email protected]>
yxieca pushed a commit that referenced this pull request May 4, 2023
What I did
Missing platform and mac in CONFIG_DB will result in container failure. We should make the config reload generate those info if missing.

How I did it
Add missing sys info if config_db.json doesn't contain it.

How to verify it
Unit test
wen587 added a commit to wen587/sonic-utilities that referenced this pull request Jun 5, 2023
wen587 added a commit that referenced this pull request Jun 5, 2023
yxieca pushed a commit that referenced this pull request Jun 15, 2023
pdhruv-marvell pushed a commit to pdhruv-marvell/sonic-utilities that referenced this pull request Aug 23, 2023
wen587 added a commit to wen587/sonic-utilities that referenced this pull request Oct 31, 2023
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.

3 participants