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

Jump to bottom

[Config] Update config command of kdump #1700

Merged
merged 7 commits into from
Aug 22, 2021
Merged

[Config] Update config command of kdump #1700

merged 7 commits into from
Aug 22, 2021

Conversation

yozhao101
Copy link
Contributor

@yozhao101 yozhao101 commented Jun 29, 2021 •
edited
Loading

Signed-off-by: Yong Zhao [email protected]

What I did

This PR aims to update the config command of Kdump (Linux kernel dump) mechanism.

How I did it

Before updating the value of a field in the table of KDUMP, we need decide whether the KDUMP table and corresponding field does exist in the table.

How to verify it

I verifies this on device str-msn2700-03 and unit test passed.

    tests/kdump_test.py::TestKdump::test_config_kdump_disable PASSED                                                                                                 
    tests/kdump_test.py::TestKdump::test_config_kdump_enable PASSED                                                                                                  
    tests/kdump_test.py::TestKdump::test_config_kdump_memory PASSED                                                                                                  
    tests/kdump_test.py::TestKdump::test_config_kdump_num_dumps PASSED

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)

@yozhao101 yozhao101 changed the title [config] Update config command of kdump [Config] Update config command of kdump Jun 29, 2021
@lgtm-com
Copy link

lgtm-com bot commented Jun 29, 2021

This pull request introduces 1 alert when merging 6a235b8 into 186d851 - view on LGTM.com

new alerts:

  • 1 for Unused import

@yozhao101 yozhao101 marked this pull request as ready for review July 5, 2021 21:20
qiluo-msft
qiluo-msft reviewed Jul 10, 2021
View reviewed changes
config/kdump.py Outdated
import click
import utilities_common.cli as clicommon
from swsscommon.swsscommon import ConfigDBConnector
from swsssdk import ConfigDBConnector
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

swsssdk

swsssdk is deprecated. Please use libswsscommon instead. #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.

Updated!

qiluo-msft
qiluo-msft reviewed Jul 10, 2021
View reviewed changes
config/kdump.py Outdated
config_db = ConfigDBConnector()
if config_db is not None:
if config_db:
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

if

If the condition is false, you silently exit. Is it an error? #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.

Good catch! If this condition is false, we need show an error message to user instead of exiting silently.

qiluo-msft
qiluo-msft reviewed Jul 10, 2021
View reviewed changes
config/kdump.py Outdated
config_db.connect()
kdump_table = config_db.get_table("KDUMP")
if "config" not in kdump_table:
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

"config" not in kdump_table

In what situation user will meet this error? This config command will exit with error code, what should user do to make it working? #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.

If the configuration of KDUMP is not present in CONIFG_DB or some keys are missing , then this error will be shown to user.

The user can try to run the command sudo config reload -y to refresh the CONFIG_DB to solve this issue.

qiluo-msft
qiluo-msft reviewed Jul 10, 2021
View reviewed changes
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.

As comment

yozhao101 added 2 commits July 20, 2021 21:37
@yozhao101
[config] This commit includes the following changes:
2c9d2f5 
1.Use the `libswsscommon` instead of `swsssdk`.
2.Show an error message when an instance of `ConfigDBConnector` can not
be created.

Signed-off-by: Yong Zhao <[email protected]>
@yozhao101
[config] Add the unit test cases and update the implementation.
aae9cfb 
Signed-off-by: Yong Zhao <[email protected]>
qiluo-msft
qiluo-msft reviewed Aug 6, 2021
View reviewed changes
config/kdump.py Outdated

if "config" not in kdump_table:
click.echo("Unable to retrieve key 'config' from KDUMP table.")
sys.exit(6)
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Could you reuse code? I see it 4 times in this file. #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.

Updated!

@yozhao101
[kdump] Create a separate function to check whether the kdump table
841fd11 
was configured in Config_DB or not.

Signed-off-by: Yong Zhao <[email protected]>
qiluo-msft
qiluo-msft reviewed Aug 19, 2021
View reviewed changes
tests/mock_tables/config_db.json Outdated
"KDUMP|config": {
"enabled": "false",
"memory": "256MB",
"num_dumps": "3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please indent like others in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see you mix TAB with SPACE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Updated.

qiluo-msft
qiluo-msft requested changes Aug 19, 2021
View reviewed changes
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.

One minor issue added

@yozhao101 yozhao101 merged commit cd3ee78 into sonic-net:master Aug 22, 2021
@yozhao101 yozhao101 mentioned this pull request Aug 22, 2021
Closed
@qiluo-msft
Copy link
Contributor

This PR could not be cleanly cherry-pick to 202012. Please submit another PR.

@yozhao101
Copy link
Contributor Author

This PR could not be cleanly cherry-pick to 202012. Please submit another PR.

Please review this PR against 202012: #1778.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiluo-msft qiluo-msft qiluo-msft approved these changes

Assignees
No one assigned
Labels
Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yozhao101 @qiluo-msft