fix: problem with bootable flag not getting passed properly to snapm

library/snapshot.py

@@ -343,10 +343,10 @@ def run_module():
     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:
+    if len(module.params.get("snapshot_lvm_set", {}).get("volumes", [])) > 0:
         cmd_result, snapset_dict = validate_snapset_json(
             cmd,
-            module.params["snapshot_lvm_set"],
+            module.params,
             False,
         )
     else:

module_utils/snapshot_lsr/consts.py

@@ -59,6 +59,8 @@ class SnapshotStatus:
     ERROR_UMOUNT_NOT_MOUNTED = 40
     ERROR_CREATE_FAILED = 41
     ERROR_SNAPM_INTERNAL_ERROR = 42
+    ERROR_BOOTABLE_NOT_SUPPORTED = 43
+    ERROR_BOOTABLE_CONFLICT = 44
 
 
 COMMAND_ENV = {"LC_ALL": "C", "LVM_COMMAND_PROFILE": "lvmdbusd"}

module_utils/snapshot_lsr/lvm.py

@@ -1209,6 +1209,21 @@ def snapshot_create_set(module, snapset_json, check_mode):
 
 def snapshot_set(module, snapset_json, check_mode):
     changed = False
+
+    # If this function has been called with bootable snapshot requested, return error
+    # because snapm is required for bootable snapshots.
+    if any(
+        (
+            snapset_json.get("snapshot_lvm_bootable", False),
+            snapset_json.get("bootable", False),
+        )
+    ):
+        return (
+            SnapshotStatus.ERROR_BOOTABLE_NOT_SUPPORTED,
+            "Bootable snapshots are not supported without snapm",
+            changed,
+        )
+
     rc, message = verify_snapset_source_lvs_exist(module, snapset_json)
     if rc != SnapshotStatus.SNAPSHOT_OK:
         return rc, message, changed

module_utils/snapshot_lsr/snapmgr.py

@@ -188,6 +188,7 @@ def mgr_check_verify_lvs_set(manager, module, snapset_json):
 
 
 def mgr_snapshot_cmd(module, module_args, snapset_json):
+    bootable = None
     snapset_name = snapset_json["name"]
     logger.info("mgr_snapshot_cmd: %s", snapset_name)
     changed = False
@@ -200,10 +201,28 @@ def mgr_snapshot_cmd(module, module_args, snapset_json):
 
     snapset_name = snapset_json["name"]
     volume_list = snapset_json["volumes"]
-    if "bootable" in snapset_json:
-        bootable = snapset_json["bootable"]
-    else:
-        bootable = False
+
+    # Bootable global variable is set
+    if module_args["snapshot_lvm_bootable"]:
+        bootable = module_args["snapshot_lvm_bootable"]
+
+    # Global is not set, check the snapset
+    if bootable is None:
+        if "bootable" in snapset_json:
+            bootable = snapset_json["bootable"]
+        else:
+            bootable = False
+    else:  # Global is set, check for conflict
+        if (
+            "bootable" in snapset_json
+            and snapset_json["bootable"] is not None
+            and bootable != snapset_json["bootable"]
+        ):
+            return {
+                "return_code": SnapshotStatus.ERROR_BOOTABLE_CONFLICT,
+                "errors": "Conflicting values for bootable",
+                "changed": False,
+            }
 
     source_list = mgr_get_source_list_for_create(volume_list)
 

module_utils/snapshot_lsr/validate.py

@@ -332,7 +332,12 @@ def validate_json_umount(snapset_dict):
     return SnapshotStatus.SNAPSHOT_OK, ""
 
 
-def validate_snapset_json(cmd, snapset_dict, verify_only):
+def validate_snapset_json(cmd, module_args, verify_only):
+
+    snapset_dict = module_args["snapshot_lvm_set"]
+
+    if module_args["snapshot_lvm_bootable"]:
+        snapset_dict["snapshot_lvm_bootable"] = module_args["snapshot_lvm_bootable"]
 
     if cmd == SnapshotCommand.SNAPSHOT:
         rc, message = validate_json_request(snapset_dict, True)

tasks/main.yml

@@ -14,22 +14,14 @@
     use: "{{ (__snapshot_is_ostree | d(false)) |
               ternary('ansible.posix.rhel_rpm_ostree', omit) }}"
 
-- name: Get snapm version
-  check_mode: false
-  command: snapm --version
-  changed_when: false
-  register: __snapshot_snapm_version_output
-  # Ignore errors as snapm may not be available
-  ignore_errors: true
+- name: Get package facts
+  package_facts:
+  no_log: "{{ ansible_verbosity < 2 }}"
 
-- name: Set snapm availability fact
+- name: Set snapm availability fact and version
   set_fact:
-    __snapshot_snapm_available: "{{ __snapshot_snapm_version_output is success }}"
-
-- name: Set snapm version
-  set_fact:
-    __snapshot_snapm_version: "{{ __snapshot_snapm_version_output.stdout }}"
-  when: __snapshot_snapm_available
+    __snapshot_snapm_available: "{{ 'snapm' in ansible_facts.packages }}"
+    __snapshot_snapm_version: "{{ ansible_facts.packages.get('snapm', [{}])[0].get('version', '0.0') }}"
 
 # Determine if bootable support is needed
 # If snapshot_lvm_bootable is set to true, or any of the volumes in snapshot_lvm_set

tests/get_snapset_status.yml

@@ -0,0 +1,15 @@
+# get_snapset_status.yml
+- name: Invoke snapm for {{ snapset_name }}
+  ansible.builtin.command: snapm snapset show {{ snapset_name | quote }} -j
+  register: snapm_raw
+  changed_when: false
+
+- name: Parse and set facts
+  ansible.builtin.set_fact:
+    # Extracting the first dictionary from the JSON list
+    _raw_data: "{{ snapm_raw.stdout | from_json | first }}"
+
+- name: Define reusable variables
+  ansible.builtin.set_fact:
+    is_bootable: "{{ _raw_data.Bootable | bool }}"
+    boot_entries_list: "{{ _raw_data.BootEntries.values() | list }}"

tests/library/find_unused_disk.py

@@ -80,7 +80,6 @@
 from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.size import Size
 
-
 SYS_CLASS_BLOCK = "/sys/class/block/"
 IGNORED_DEVICES = [re.compile(r"^/dev/nullb\d+$")]
 

tests/tests_basic_bootable.yml

@@ -0,0 +1,143 @@
+---
+- name: Basic snapshot test
+  hosts: all
+  vars:
+    # only use vgs matching this pattern
+    snapshot_lvm_vg_include: "^test_"
+    test_disk_min_size: "1g"
+    test_disk_count: 10
+    test_storage_pools:
+      - name: test_vg1
+        disks: "{{ range(0, 3) | map('extract', unused_disks) | list }}"
+        volumes:
+          - name: lv1
+            size: "15%"
+          - name: lv2
+            size: "50%"
+      - name: test_vg2
+        disks: "{{ range(3, 6) | map('extract', unused_disks) | list }}"
+        volumes:
+          - name: lv3
+            size: "10%"
+          - name: lv4
+            size: "20%"
+      - name: test_vg3
+        disks: "{{ range(6, 10) | map('extract', unused_disks) | list }}"
+        volumes:
+          - name: lv5
+            size: "30%"
+          - name: lv6
+            size: "25%"
+          - name: lv7
+            size: "10%"
+          - name: lv8
+            size: "10%"
+  tasks:
+    - name: Run tests
+      block:
+        - name: Setup
+          include_tasks: tasks/setup.yml
+
+        - name: Run the snapshot role to create snapshot LVs
+          include_role:
+            name: linux-system-roles.snapshot
+          vars:
+            snapshot_lvm_percent_space_required: 15
+            snapshot_lvm_all_vgs: true
+            snapshot_lvm_snapset_name: snapset1
+            snapshot_lvm_action: snapshot
+            snapshot_lvm_bootable: true
+
+        - name: Assert changes for creation
+          assert:
+            that: snapshot_cmd["changed"]
+
+        - name: Verify the snapshot LVs are created
+          include_role:
+            name: linux-system-roles.snapshot
+          vars:
+            snapshot_lvm_all_vgs: true
+            snapshot_lvm_snapset_name: snapset1
+            snapshot_lvm_verify_only: true
+            snapshot_lvm_action: check
+
+        - name: Run the snapshot role again to check idempotence
+          include_role:
+            name: linux-system-roles.snapshot
+          vars:
+            snapshot_lvm_percent_space_required: 15
+            snapshot_lvm_all_vgs: true
+            snapshot_lvm_snapset_name: snapset1
+            snapshot_lvm_action: snapshot
+
+        - name: Assert no changes for creation
+          assert:
+            that: not snapshot_cmd["changed"]
+
+        - name: Verify again to check idempotence
+          include_role:
+            name: linux-system-roles.snapshot
+          vars:
+            snapshot_lvm_all_vgs: true
+            snapshot_lvm_snapset_name: snapset1
+            snapshot_lvm_verify_only: true
+            snapshot_lvm_action: check
+
+        - name: Assert no changes for verify
+          assert:
+            that: not snapshot_cmd["changed"]
+
+        - name: Run the snapshot role remove the snapshot LVs
+          include_role:
+            name: linux-system-roles.snapshot
+          vars:
+            snapshot_lvm_snapset_name: snapset1
+            snapshot_lvm_action: remove
+
+        - name: Assert changes for removal
+          assert:
+            that: snapshot_cmd["changed"]
+
+        - name: Use the snapshot_lvm_verify option to make sure remove is done
+          include_role:
+            name: linux-system-roles.snapshot
+          vars:
+            snapshot_lvm_snapset_name: snapset1
+            snapshot_lvm_verify_only: true
+            snapshot_lvm_action: remove
+
+        - name: Remove again to check idempotence
+          include_role:
+            name: linux-system-roles.snapshot
+          vars:
+            snapshot_lvm_snapset_name: snapset1
+            snapshot_lvm_action: remove
+
+        - name: Assert no changes for remove
+          assert:
+            that: not snapshot_cmd["changed"]
+
+        - name: Verify remove again to check idempotence
+          include_role:
+            name: linux-system-roles.snapshot
+          vars:
+            snapshot_lvm_snapset_name: snapset1
+            snapshot_lvm_verify_only: true
+            snapshot_lvm_action: remove
+
+        - name: Assert no changes for remove verify
+          assert:
+            that: not snapshot_cmd["changed"]
+
+      rescue:
+        - name: Do not fail if snapm is too old for test
+          assert:
+            that: ansible_failed_result.msg is search(msg1) or ansible_failed_result.msg is search(msg2)
+          vars:
+            msg1: "Package snapm version .* is too old"
+            msg2: "Package snapm version 0.5 or later is required to use bootable snapsets"
+
+      always:
+        - name: Cleanup
+          include_tasks: tasks/cleanup.yml
+          tags: tests::cleanup

tests/tests_set_bootable.yml

@@ -49,7 +49,6 @@
           vg: test_vg3
           lv: lv7
           percent_space_required: 15
-    snapshot_lvm_bootable: true
   tasks:
     - name: Load test variables
       include_vars:
@@ -66,49 +65,28 @@
             name: linux-system-roles.snapshot
           vars:
             snapshot_lvm_action: snapshot
+            snapshot_lvm_bootable: true
             snapshot_lvm_set: "{{ snapshot_test_set }}"
 
         - name: Assert changes for create snapset
           assert:
             that: snapshot_cmd["changed"]
 
-        - name: Run the snapshot role to verify the set of snapshots for the LVs
-          include_role:
-            name: linux-system-roles.snapshot
-          vars:
-            snapshot_lvm_action: check
-            snapshot_lvm_set: "{{ snapshot_test_set }}"
-            snapshot_lvm_verify_only: true
-
-        - name: Create snapset again for idempotence
-          include_role:
-            name: linux-system-roles.snapshot
-          vars:
-            snapshot_lvm_action: snapshot
-            snapshot_lvm_set: "{{ snapshot_test_set }}"
-
-        - name: Assert no changes for create snapset
-          assert:
-            that: not snapshot_cmd["changed"]
-
-        - name: Run the snapshot role remove the set
-          include_role:
-            name: linux-system-roles.snapshot
+        - name: Get snapset details
+          ansible.builtin.include_tasks: get_snapset_status.yml
           vars:
-            snapshot_lvm_action: remove
-            snapshot_lvm_set: "{{ snapshot_test_set }}"
+            snapset_name: "{{ snapshot_test_set.name }}"
 
-        - name: Assert changes for remove snapset
+        - name: Fail if not bootable
           assert:
-            that: snapshot_cmd["changed"]
+            that: is_bootable
 
-        - name: Run the snapshot role to verify the set is removed
+        - name: Remove the snapshot set
           include_role:
             name: linux-system-roles.snapshot
           vars:
             snapshot_lvm_action: remove
             snapshot_lvm_set: "{{ snapshot_test_set }}"
-            snapshot_lvm_verify_only: true
 
         - name: Remove again to check idempotence
           include_role:
@@ -121,12 +99,12 @@
           assert:
             that: not snapshot_cmd["changed"]
       rescue:
-        - name: Check if error is due to snapm version too old
-          fail:
-            msg: Unexpected error occurred {{ ansible_failed_result | to_nice_json }}
-          when: not ansible_failed_result.msg is search(err_msg)
+        - name: Do not fail if snapm is too old for test
+          assert:
+            that: ansible_failed_result.msg is search(msg1) or ansible_failed_result.msg is search(msg2)
           vars:
-            err_msg: Package snapm .*version 0.5 or later is required to use bootable snapsets
+            msg1: "Package snapm version .* is too old"
+            msg2: "Package snapm version 0.5 or later is required to use bootable snapsets"
       always:
         - name: Cleanup
           include_tasks: tasks/cleanup.yml