Fix sonic-kdump-config --remote parameter handling#4193
Open
Kirankumar-Muchandi-ML wants to merge 9 commits intosonic-net:masterfrom
Open
Fix sonic-kdump-config --remote parameter handling#4193Kirankumar-Muchandi-ML wants to merge 9 commits intosonic-net:masterfrom
Kirankumar-Muchandi-ML wants to merge 9 commits intosonic-net:masterfrom
Conversation
Signed-off-by: Kirankumar-Muchandi-ML <Kirankumar.Muchandi@Xoriant.Com>
Collaborator
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
… message Signed-off-by: Kirankumar-Muchandi-ML <Kirankumar.Muchandi@Xoriant.Com>
Signed-off-by: Kirankumar-Muchandi-ML <Kirankumar.Muchandi@Xoriant.Com>
- Changed test validator from str_to_bool to validate_remote_action
- Updated tests to use 'enable'/'disable' instead of 'true'/'false'
- Removed tests for unsupported values ('0', '1', 'yes', 'no')
- Added tests to verify 'true'/'false' are rejected
- Tests now match the actual script implementation
Signed-off-by: Kirankumar-Muchandi-ML <Kirankumar.Muchandi@Xoriant.Com>
- Updated test to use ConfigDBConnector mock instead of run_command - Test now verifies CONFIG_DB mod_entry calls with 'true'/'false' strings - Test verifies correct print messages for enable/disable - Removed old tests that expected sed commands (old implementation) This fixes the test failures in CI where tests expected the old implementation that used sed commands to modify /etc/default/kdump-tools Signed-off-by: Kirankumar-Muchandi-ML <Kirankumar.Muchandi@Xoriant.Com>
Signed-off-by: Kirankumar-Muchandi-ML <Kirankumar.Muchandi@Xoriant.Com>
- Remove f-string without placeholders (line 38) - Split long lines to comply with 79 character limit (lines 42-45) - Improve code readability with proper line breaks Signed-off-by: Kirankumar-Muchandi-ML <Kirankumar.Muchandi@Xoriant.Com>
f1db530 to
4298007
Compare
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
- Remove duplicate validate_remote_action function in test class - Use sonic_kdump_config.validate_remote_action to test actual implementation - This ensures the function in the main script gets proper test coverage Signed-off-by: Kirankumar-Muchandi-ML <Kirankumar.Muchandi@Xoriant.Com>
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Add -v/--verbose flag to display additional reboot information including power cycle status and reboot type. Changes: - Add verbose parameter to fetch_data_from_db() function - Add verbose parameter to fetch_reboot_cause_history_from_db() function - Add -v/--verbose option to history() command - Display 'Power Cycle' and 'Reboot Type' columns when verbose flag is used - Show 'N/A' for fields not present in database (backward compatible) This enables vendor-agnostic testing to verify cold reboots trigger power cycles. Fixes: sonic-net/sonic-buildimage#25420 Signed-off-by: Kirankumar-Muchandi-ML <Kirankumar.Muchandi@Xoriant.Com>
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue : sonic-net/SONiC#2121
Why I did it
The
sonic-kdump-configcommand's--remoteflag was not saving the configuration to CONFIG_DB, and it only worked as a boolean flag without accepting explicit enable/disable parameters.Current behavior (broken):
Additionally, even when using just
--remoteflag, the setting was not persisted to CONFIG_DB.How I did it
1. Changed
--remoteto acceptenable/disableactionsparser.add_argument('--remote', action='store_true')parser.add_argument('--remote', type=str, choices=['enable', 'disable'], metavar='ACTION')enableordisableactions (nottrue/false)2. Added custom validation with helpful error messages
When invalid value is provided:
3. Fixed CONFIG_DB save functionality
remote: true/falseto CONFIG_DB based onenable/disableactionelif options.remote:toelif options.remote is not None:kdump_defaults['remote']assignment based on action4. Updated unit tests
enable/disableinstead oftrue/falseHow to verify it
Test 1: Enable remote kdump
sudo sonic-kdump-config --remote enable show kdump configExpected:
Remote kdump feature enabled.Kdump ssh connection string: <value>Test 2: Disable remote kdump
Expected:
Remote kdump feature disabled.Kdump ssh connection string and ssh_path not foundTest 3: Verify CONFIG_DB persistence
Expected:
'remote': 'true'sudo sonic-kdump-config --remote disable sudo sonic-kdump-config --dump-db | grep remoteExpected:
'remote': 'false'Test 4: Test invalid value (should fail with clear error)
sudo sonic-kdump-config --remote trueExpected error:
sudo sonic-kdump-config --remote falseExpected error:
Test 5: Run unit tests
cd src/sonic-utilities python -m pytest tests/sonic_kdump_config_test.py -vExpected: All tests pass ✓
Previous Discussion Context
This PR supersedes the previous approach that used
true/falseboolean values. After review feedback, the implementation was changed to useenable/disableactions for consistency with other SONiC CLI commands and better user experience.Files Modified
Submodule: src/sonic-utilities
scripts/sonic-kdump-config- Updated--remoteargument handling and CONFIG_DB save logictests/sonic_kdump_config_test.py- Updated test cases to useenable/disableSubmodule commits:
0da311da- Fix: Save remote kdump setting to CONFIG_DB when using --remote flag1c727f27- Update --remote to accept only 'enable' or 'disable' with clear error message48491c24- Fix sonic-kdump-config --remote parameter handling