[Config] Update config command of kdump by yozhao101 · Pull Request #1700 · sonic-net/sonic-utilities

yozhao101 · 2021-06-29T05:48:02Z

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)

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

lgtm-com · 2021-06-29T05:56:53Z

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

new alerts:

1 for Unused import

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

qiluo-msft · 2021-07-10T08:39:39Z

config/kdump.py

 import click
 import utilities_common.cli as clicommon
-from swsscommon.swsscommon import ConfigDBConnector
+from swsssdk import ConfigDBConnector


swsssdk

swsssdk is deprecated. Please use libswsscommon instead. #Closed

qiluo-msft · 2021-07-10T08:41:50Z

config/kdump.py

+    """Disable kdump feature"""
    config_db = ConfigDBConnector()
-    if config_db is not None:
+    if config_db:


if

If the condition is false, you silently exit. Is it an error? #Closed

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

qiluo-msft · 2021-07-10T08:43:59Z

config/kdump.py

+    if config_db:
        config_db.connect()
+        kdump_table = config_db.get_table("KDUMP")
+        if "config" not in kdump_table:


"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

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.

config/kdump.py

qiluo-msft

As comment

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]>

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

qiluo-msft · 2021-08-06T09:41:33Z

config/kdump.py

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


Could you reuse code? I see it 4 times in this file. #Closed

was configured in Config_DB or not. Signed-off-by: Yong Zhao <[email protected]>

qiluo-msft · 2021-08-19T09:16:30Z

tests/mock_tables/config_db.json

+    "KDUMP|config": {
+	"enabled": "false",
+	"memory": "256MB",
+	"num_dumps": "3"


Please indent like others in this file.

I see you mix TAB with SPACE

Good catch! Updated.

qiluo-msft

One minor issue added

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

qiluo-msft · 2021-08-23T03:10:31Z

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

yozhao101 · 2021-08-23T23:45:30Z

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

Please review this PR against 202012: #1778.

[kdump] Update config command of kdump.

6a235b8

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

yozhao101 changed the title ~~[config] Update config command of kdump~~ [Config] Update config command of kdump Jun 29, 2021

[config] Use sys.exit(...) instead of return statement.

590bc1c

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

yozhao101 marked this pull request as ready for review July 5, 2021 21:20

[config] Delete the unused import os.

78c0b84

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

yozhao101 added the Enhancement label Jul 6, 2021

qiluo-msft reviewed Jul 10, 2021

View reviewed changes

config/kdump.py Show resolved Hide resolved

qiluo-msft reviewed Jul 10, 2021

View reviewed changes

yozhao101 added 2 commits July 20, 2021 21:37

[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]>

[config] Add the unit test cases and update the implementation.

aae9cfb

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

qiluo-msft reviewed Aug 6, 2021

View reviewed changes

[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 approved these changes Aug 19, 2021

View reviewed changes

qiluo-msft reviewed Aug 19, 2021

View reviewed changes

qiluo-msft requested changes Aug 19, 2021

View reviewed changes

[config] Fix the indentation issue.

4eed738

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

qiluo-msft approved these changes Aug 20, 2021

View reviewed changes

yozhao101 added the Request for 202012 Branch label Aug 22, 2021

yozhao101 merged commit cd3ee78 into sonic-net:master Aug 22, 2021

yozhao101 mentioned this pull request Aug 22, 2021

[sonic-utilities] Update the submodule of sonic-utilities. sonic-net/sonic-buildimage#8551

Closed

qiluo-msft removed the Request for 202012 Branch label Aug 24, 2021

Conversation

yozhao101 commented Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What I did

How I did it

How to verify it

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)

Uh oh!

lgtm-com bot commented Jun 29, 2021

Uh oh!

qiluo-msft Jul 10, 2021 • 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!

yozhao101 Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jul 10, 2021 • 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!

yozhao101 Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jul 10, 2021 • 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!

yozhao101 Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Aug 6, 2021 • 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!

yozhao101 Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

yozhao101 Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

qiluo-msft commented Aug 23, 2021

Uh oh!

yozhao101 commented Aug 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yozhao101 commented Jun 29, 2021 •

edited

Loading

qiluo-msft Jul 10, 2021 •

edited

Loading

qiluo-msft Jul 10, 2021 •

edited

Loading

qiluo-msft Jul 10, 2021 •

edited

Loading

qiluo-msft Aug 6, 2021 •

edited

Loading