-
Notifications
You must be signed in to change notification settings - Fork 684
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
[Show] Update the subcommands of Kdump. #1682
[Show] Update the subcommands of Kdump. #1682
Conversation
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Please also update the Command Reference guide to reflect these changes. |
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
show/kdump.py
Outdated
config_db = ConfigDBConnector() | ||
if config_db is not None: | ||
if config_db: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this will be an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then please handle the error instead of keeping silent. Or remove the check and let config.connect()
throw exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated and rely on the config.connect()
to throw exception if any error occurs.
show/kdump.py
Outdated
|
||
Returns: | ||
admin_mode: If Kdump is ready, returns "Ready"; If Kdump is not ready, | ||
returns "Unready"; Otherwise, returns "Unknown". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion! Updated.
utilities_common/cli.py
Outdated
output = proc.communicate()[0] | ||
return output | ||
command_stdout, command_stderr = proc.communicate() | ||
return command_stdout, command_stderr, proc.returncode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the code which uses this function to get the standard output of a specific command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you find a way not impacting other code? They are not relevant to your feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As comments
This pull request introduces 1 alert when merging 7be11c62487ef272cf5c6ad3df1fa939dcf17e1c into 0efd297 - view on LGTM.com new alerts:
|
The Do we need remove this |
7be11c6
to
9b490cc
Compare
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Yong Zhao yozhao@microsoft.com What I did This PR fixed the following issues: Initially we need add prefix sudo when ran the show command of Kdump. This is unnecessary. I re-organized the logic of implementing show command and also update the script sonic-kdump-config. How I did it Since the script sonic-kdump-config can only be executed by root user, I moved some functions from this file to the kdump.py such that we do not need run the show command with sudo. How to verify it I verified this change on the DuT str-msn2700-03.
Update sonic-utilities submodule to latest in 202012 branch: [show priority-group drop counters] Add user info output when user want to check PG counters and polling are disabled sonic-net/sonic-utilities#1678 [route_check] Filter out VNET routes sonic-net/sonic-utilities#1612 [Show] Update the subcommands of Kdump. sonic-net/sonic-utilities#1682 Add mock support for swsscommon classes sonic-net/sonic-utilities#1780 [acl_loader]: add iptype match to the rules for dataplane acl sonic-net/sonic-utilities@205aff8 [202012][fast-reboot] Remove FLEX_COUNTER_TABLE from config_db.json before fast-reboot sonic-net/sonic-utilities#1774
Signed-off-by: Yong Zhao yozhao@microsoft.com
What I did
This PR fixed the following issues:
Initially we need add prefix
sudo
when ran theshow
command of Kdump. This is unnecessary.I re-organized the logic of implementing
show
command and also update the scriptsonic-kdump-config
.How I did it
Since the script
sonic-kdump-config
can only be executed byroot
user, I moved some functions from this file tothe
kdump.py
such that we do not need run theshow
command withsudo
.How to verify it
I verified this change on the DuT
str-msn2700-03
.Previous command output (if the output of a command-line utility has changed)
Before the
Kdump
is enabled:After the
Kdump
is enabled:After device was rebooted and Kernel crash was triggered twice manually:
New command output (if the output of a command-line utility has changed)
Before the
Kdump
is enabled:After
Kdump
is enabled:After device was rebooted and Kernel crash was triggered twice manually: