fix: problem with bootable flag not getting passed properly to snapm #138
Conversation
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
Add check to test that uses snapm to validate the snapset attribute is set to bootable. Signed-off-by: Todd Gill <tgill@redhat.com>
Reviewer's GuideEnsures the global bootable flag is correctly propagated through the snapshot module to snapm, adds validation and error codes for unsupported/conflicting bootable configurations, makes the snapshot entrypoint more robust, and extends the test suite with bootable-specific coverage including snapm-based verification. Sequence diagram for bootable flag propagation to snapmsequenceDiagram
actor AnsibleUser
participant snapshot_py as snapshot
participant validate_py as validate
participant snapmgr_py as snapmgr
AnsibleUser->>snapshot_py: run_module(params)
activate snapshot_py
snapshot_py->>snapshot_py: check snapshot_lvm_set and volumes
snapshot_py->>validate_py: validate_snapset_json(SNAPSHOT, module_params, verify_only=False)
activate validate_py
validate_py->>validate_py: snapset_dict = module_args.snapshot_lvm_set
validate_py->>validate_py: if module_args.snapshot_lvm_bootable
validate_py->>validate_py: snapset_dict.snapshot_lvm_bootable = module_args.snapshot_lvm_bootable
validate_py-->>snapshot_py: rc, message, updated_snapset_dict
deactivate validate_py
snapshot_py->>snapmgr_py: mgr_snapshot_cmd(module, module_params, snapset_json)
activate snapmgr_py
snapmgr_py->>snapmgr_py: bootable = None
snapmgr_py->>snapmgr_py: if module_args.snapshot_lvm_bootable
snapmgr_py->>snapmgr_py: bootable = module_args.snapshot_lvm_bootable
snapmgr_py->>snapmgr_py: if bootable is None and snapset_json.bootable present
snapmgr_py->>snapmgr_py: bootable = snapset_json.bootable
snapmgr_py->>snapmgr_py: else if bootable is None
snapmgr_py->>snapmgr_py: bootable = False
snapmgr_py->>snapmgr_py: if conflict between global and snapset bootable
snapmgr_py-->>snapshot_py: {return_code: ERROR_BOOTABLE_CONFLICT, changed: False}
deactivate snapmgr_py
snapshot_py-->>AnsibleUser: result (includes bootable propagated or conflict error)
Class diagram for updated snapshot bootable handlingclassDiagram
class SnapshotStatus {
<<enumeration>>
int SNAPSHOT_OK
int ERROR_UMOUNT_NOT_MOUNTED
int ERROR_CREATE_FAILED
int ERROR_SNAPM_INTERNAL_ERROR
int ERROR_BOOTABLE_NOT_SUPPORTED
int ERROR_BOOTABLE_CONFLICT
}
class snapshot_py {
+run_module()
}
class validate_py {
+validate_snapset_json(cmd, module_args, verify_only)
+validate_json_request(snapset_dict, verify_sources)
+validate_json_umount(snapset_dict)
}
class lvm_py {
+snapshot_create_set(module, snapset_json, check_mode)
+snapshot_set(module, snapset_json, check_mode)
+verify_snapset_source_lvs_exist(module, snapset_json)
}
class snapmgr_py {
+mgr_check_verify_lvs_set(manager, module, snapset_json)
+mgr_snapshot_cmd(module, module_args, snapset_json)
+mgr_get_source_list_for_create(volume_list)
}
SnapshotStatus <.. lvm_py : uses
SnapshotStatus <.. snapmgr_py : uses
SnapshotStatus <.. validate_py : uses
snapshot_py ..> validate_py : calls
snapshot_py ..> snapmgr_py : calls
snapshot_py ..> lvm_py : calls
validate_py ..> SnapshotStatus : returns
lvm_py ..> SnapshotStatus : returns
snapmgr_py ..> SnapshotStatus : returns
Flow diagram for bootable resolution and validationflowchart TD
A[module.run_module entry] --> B{snapshot_lvm_set exists and has volumes}
B -- No --> Z[Exit or other handling]
B -- Yes --> C[Call validate_snapset_json]
subgraph Validate_snapset_json
C --> D[Set snapset_dict = module_args.snapshot_lvm_set]
D --> E{module_args.snapshot_lvm_bootable set}
E -- Yes --> F[Set snapset_dict.snapshot_lvm_bootable = module_args.snapshot_lvm_bootable]
E -- No --> G[Leave snapset_dict unchanged]
F --> H[Continue validation]
G --> H[Continue validation]
end
H --> I{Using snapm path}
I -- Yes --> J[Call mgr_snapshot_cmd]
I -- No --> K[Call snapshot_set in lvm]
subgraph mgr_snapshot_cmd bootable resolution
J --> L[bootable = None]
L --> M{module_args.snapshot_lvm_bootable set}
M -- Yes --> N[bootable = module_args.snapshot_lvm_bootable]
M -- No --> O[bootable remains None]
N --> P
O --> P{bootable is None}
P -- Yes --> Q{snapset_json.bootable present}
Q -- Yes --> R[bootable = snapset_json.bootable]
Q -- No --> S[bootable = False]
P -- No --> T[bootable already set from global]
R --> U
S --> U
T --> U
U --> V{Global and snapset bootable conflict}
V -- Yes --> W[Return ERROR_BOOTABLE_CONFLICT]
V -- No --> X[Proceed to create bootable snapset]
end
subgraph snapshot_set non_snapm path
K --> Y{snapshot_lvm_bootable is True or bootable is True}
Y -- Yes --> AA[Return ERROR_BOOTABLE_NOT_SUPPORTED]
Y -- No --> AB[Proceed with non bootable snapshot creation]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Collaborator
Author
|
Note: this is a draft PR because we need a fix in snapm to land before CI will pass. snapshotmanager/snapm#570 is required. snapm 0.5.3 is planned for release/tag today (1/5/26). |
richm
reviewed
Jan 5, 2026
| if module.params["snapshot_lvm_vg_include"]: | ||
| vg_include = re.compile(module.params["snapshot_lvm_vg_include"]) | ||
|
|
||
| if len(module.params["snapshot_lvm_set"].get("volumes")) > 0: |
Contributor
There was a problem hiding this comment.
if len(module.params.get("snapshot_lvm_set", {}).get("volumes", [])) > 0:This should work as a one-liner - it will work if params has no snapshot_lvm_set, and if snapshot_lvm_set has no volumes - I believe the problem with the old code was that you could call len(None) which is an error
richm
reviewed
Jan 5, 2026
|
|
||
| # If this function has been called with bootable snapshot requested, return error | ||
| # because snapm is required for bootable snapshots. | ||
| if ( |
Contributor
There was a problem hiding this comment.
if any((snapset_json["snapshot_lvm_bootable"], snapset_json["bootable"])):
this is more pythonic - you shouldn't need to compare a value to True unless snapset_json["snapshot_lvm_bootable"] can have values other than None or boolean
richm
reviewed
Jan 5, 2026
| @@ -0,0 +1,15 @@ | |||
| # get_snapset_status.yml | |||
| - name: Invoke snapm for {{ snapset_name }} | |||
| ansible.builtin.command: "snapm snapset show {{ snapset_name }} -j" | |||
Contributor
There was a problem hiding this comment.
Suggested change
| ansible.builtin.command: "snapm snapset show {{ snapset_name }} -j" | |
| ansible.builtin.command: snapm snapset show {{ snapset_name | quote }} -j |
richm
reviewed
Jan 5, 2026
| - name: Define reusable variables | ||
| ansible.builtin.set_fact: | ||
| is_bootable: "{{ _raw_data.Bootable | bool }}" | ||
| boot_entries_list: "{{ _raw_data.BootEntries.values() | list }}" No newline at end of file |
Contributor
There was a problem hiding this comment.
missing newline at end of file
richm
reviewed
Jan 5, 2026
| snapshot_lvm_all_vgs: true | ||
| snapshot_lvm_snapset_name: snapset1 | ||
| snapshot_lvm_action: snapshot | ||
| snapshot_lvm_bootable: true, |
Contributor
There was a problem hiding this comment.
Suggested change
| snapshot_lvm_bootable: true, | |
| snapshot_lvm_bootable: true |
?
richm
reviewed
Jan 5, 2026
| @@ -0,0 +1,135 @@ | |||
| --- | |||
| - name: Basic snapshot test | |||
Contributor
There was a problem hiding this comment.
What is the difference between this new test and tests_set_bootable.yml?
Contributor
|
You'll need to rebase on top of the latest |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Add check to test that uses snapm to validate the snapset attribute is set to bootable.
Reason:
Global bootable flag was not being passed to snapm
Result:
Snapset will now be created with bootable attribute set and corresponding boot menu options.
Issue Tracker Tickets (Jira or BZ if any):
RHEL-137034
Summary by Sourcery
Ensure the snapshot role correctly propagates and validates the bootable flag, including when using snapm.
New Features:
Bug Fixes:
Enhancements:
Tests: