fix: problem with bootable flag not getting passed properly to snapm#138
Merged
richm merged 3 commits intolinux-system-roles:mainfrom Feb 3, 2026
Merged
fix: problem with bootable flag not getting passed properly to snapm#138richm merged 3 commits intolinux-system-roles:mainfrom
richm merged 3 commits intolinux-system-roles:mainfrom
Conversation
Reviewer's GuidePropagates the global bootable flag through snapshot validation and snapm integration, adds explicit error codes for unsupported or conflicting bootable settings, and extends tests to verify bootable behavior via snapm and a new end-to-end scenario. Sequence diagram for bootable snapshot creation via snapmsequenceDiagram
actor AnsibleUser
participant SnapshotModule as snapshot_py
participant Validate as validate_py
participant SnapManager as snapmgr_py
participant Snapm as snapm_binary
AnsibleUser->>SnapshotModule: run snapshot module with snapshot_lvm_bootable, snapshot_lvm_set
SnapshotModule->>SnapshotModule: check snapshot_lvm_set volumes length
SnapshotModule->>Validate: validate_snapset_json(cmd, module_params, verify_only=False)
Validate->>Validate: snapset_dict = module_args.snapshot_lvm_set
Validate->>Validate: if module_args.snapshot_lvm_bootable set
Validate->>Validate: snapset_dict.snapshot_lvm_bootable = module_args.snapshot_lvm_bootable
Validate-->>SnapshotModule: rc, message
SnapshotModule->>SnapManager: mgr_snapshot_cmd(module, module_args, snapset_json)
SnapManager->>SnapManager: bootable = None
SnapManager->>SnapManager: if module_args.snapshot_lvm_bootable set
SnapManager->>SnapManager: bootable = module_args.snapshot_lvm_bootable
SnapManager->>SnapManager: if bootable is None and snapset_json.bootable exists
SnapManager->>SnapManager: else default bootable = False
SnapManager->>SnapManager: if both global and snapset bootable set and differ
SnapManager-->>SnapshotModule: return ERROR_BOOTABLE_CONFLICT
SnapManager->>Snapm: create bootable snapset with bootable attribute
Snapm-->>SnapManager: snapset created, boot menu updated
SnapManager-->>SnapshotModule: result with changed=True and bootable info
SnapshotModule-->>AnsibleUser: task result including bootable status facts
Class diagram for updated snapshot_lsr snapshot handlingclassDiagram
class SnapshotStatus {
<<enumeration>>
SNAPSHOT_OK
ERROR_UMOUNT_INVALID_CMD
ERROR_UMOUNT_NOT_MOUNTED
ERROR_CREATE_FAILED
ERROR_SNAPM_INTERNAL_ERROR
ERROR_BOOTABLE_NOT_SUPPORTED
ERROR_BOOTABLE_CONFLICT
}
class snapshot_py {
+run_module()
+main()
}
class validate_py {
+validate_json_umount(snapset_dict)
+validate_snapset_json(cmd, module_args, verify_only)
}
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)
}
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)
}
snapshot_py --> validate_py : calls
snapshot_py --> snapmgr_py : uses for snapm based snapshots
snapshot_py --> lvm_py : uses for lvm only snapshots
validate_py --> SnapshotStatus : returns
snapmgr_py --> SnapshotStatus : returns
lvm_py --> SnapshotStatus : returns
snapmgr_py --> lvm_py : may coordinate with
lvm_py --> validate_py : uses validation results
Flow diagram for bootable flag resolution in mgr_snapshot_cmdflowchart TD
A[start mgr_snapshot_cmd] --> B[bootable = None]
B --> C{module_args.snapshot_lvm_bootable is truthy}
C -- Yes --> D[bootable = module_args.snapshot_lvm_bootable]
C -- No --> E[bootable remains None]
D --> F{snapset_json.bootable is not None}
F -- Yes --> G{bootable != snapset_json.bootable}
G -- Yes --> H[return ERROR_BOOTABLE_CONFLICT, changed False]
G -- No --> I[proceed to source_list creation]
F -- No --> I
E --> J{bootable is None}
J -- Yes --> K{bootable in snapset_json}
K -- Yes --> L[bootable = snapset_json.bootable]
K -- No --> M[bootable = False]
L --> I
M --> I
I --> N[use bootable when building snapset for snapm]
N --> O[end mgr_snapshot_cmd]
Flow diagram for handling bootable snapshots without snapmflowchart TD
A[start snapshot_set in lvm_py] --> B{bootable requested?}
B --> C[snapset_json.snapshot_lvm_bootable]
B --> D[snapset_json.bootable]
C --> E{any of snapshot_lvm_bootable or bootable is truthy}
D --> E
E -- Yes --> F[return ERROR_BOOTABLE_NOT_SUPPORTED, message 'Bootable snapshots are not supported without snapm', changed False]
E -- No --> G[verify_snapset_source_lvs_exist]
G --> H{rc == SNAPSHOT_OK?}
H -- No --> I[return error from verification]
H -- Yes --> J[proceed with non bootable snapshot_set]
J --> K[end snapshot_set]
F --> K
I --> K
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
richm
reviewed
Jan 5, 2026
richm
reviewed
Jan 5, 2026
richm
reviewed
Jan 5, 2026
richm
reviewed
Jan 5, 2026
richm
reviewed
Jan 5, 2026
Contributor
|
You'll need to rebase on top of the latest |
trgill
force-pushed
the
bootable_variable_fix
branch
from
27969eb to
c5cdf33
Compare
Contributor
|
1 minor python |
Collaborator
Author
Looking at that now... I didn't change that file in this PR. It is the file we copied from the storage role for finding disks for the tests. It was just an extra line - fixed now. |
trgill
force-pushed
the
bootable_variable_fix
branch
from
c5cdf33 to
748a899
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
snapshot_set, accessingsnapset_json["snapshot_lvm_bootable"]andsnapset_json["bootable"]without.get()or default values can raise aKeyErrorfor existing call sites that don’t populate these keys; consider usingsnapset_json.get(...)and treating missing/None as non-bootable. - The new
validate_snapset_jsonsignature now takesmodule_argsbut immediately overwritessnapset_dictfrommodule_args["snapshot_lvm_set"]and assumessnapshot_lvm_bootableis present; it may be clearer and safer to keep the explicitsnapset_dictparameter and use.get("snapshot_lvm_bootable")to avoid KeyErrors and tight coupling to the module’s param structure. - In
mgr_snapshot_cmd, the logicif module_args["snapshot_lvm_bootable"]:only treats truthy values as a set global flag, so an explicitfalsefrom the user won’t override atruevalue insnapset_json; if you intend explicitfalseto take precedence, check foris not Noneor similar rather than truthiness.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `snapshot_set`, accessing `snapset_json["snapshot_lvm_bootable"]` and `snapset_json["bootable"]` without `.get()` or default values can raise a `KeyError` for existing call sites that don’t populate these keys; consider using `snapset_json.get(...)` and treating missing/None as non-bootable.
- The new `validate_snapset_json` signature now takes `module_args` but immediately overwrites `snapset_dict` from `module_args["snapshot_lvm_set"]` and assumes `snapshot_lvm_bootable` is present; it may be clearer and safer to keep the explicit `snapset_dict` parameter and use `.get("snapshot_lvm_bootable")` to avoid KeyErrors and tight coupling to the module’s param structure.
- In `mgr_snapshot_cmd`, the logic `if module_args["snapshot_lvm_bootable"]:` only treats truthy values as a set global flag, so an explicit `false` from the user won’t override a `true` value in `snapset_json`; if you intend explicit `false` to take precedence, check for `is not None` or similar rather than truthiness.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
richm
reviewed
Feb 2, 2026
Add check to test that uses snapm to validate the snapset attribute is set to bootable. Signed-off-by: Todd Gill <tgill@redhat.com>
trgill
force-pushed
the
bootable_variable_fix
branch
from
748a899 to
cf3b8cc
Compare
Signed-off-by: Rich Megginson <rmeggins@redhat.com>
Contributor
Collaborator
Author
Contributor
fix: fix issues with bootable snapshot code
Contributor
|
[citest] |
Contributor
|
@trgill ready to merge? Did you test to see if you can boot from a bootable snapshot? |
Collaborator
Author
yes it is ready to merge. I've tested with a VM that boots from LVM. I'm not aware of any way to automate that test. |
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
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 bootable snapshot flag is correctly propagated to snapm and validate bootable behavior, while preventing unsupported or conflicting bootable configurations.
New Features:
Bug Fixes:
Enhancements:
Tests: