Skip to content

Conversation

@trgill
Copy link
Collaborator

@trgill trgill commented Jan 20, 2026

Enhancement:
Update to use snapm for mount/umount

Reason:
Removing dependency on legacy code

Result:
Same functionality, but dependency switched to snapm

Issue Tracker Tickets (Jira or BZ if any):
STORAGECFG-1023 - use snapm mount tools when available

Summary by Sourcery

Switch snapshot mount and unmount handling to use snapm when available and introduce bootable snapshot support and related validation.

New Features:

  • Add support for bootable snapshot sets controlled via a global flag or per-snapset configuration, including conflict detection.
  • Expose bootability information by querying snapm for snapset status in tests.

Bug Fixes:

  • Guard snapset validation against missing or empty volume lists when invoking the snapshot module.

Enhancements:

  • Route mount and unmount operations through snapm manager helpers with verification logic instead of legacy helper functions.
  • Reject bootable snapshot requests when snapm is not in use, returning explicit error codes for unsupported or conflicting configurations.
  • Extend snapshot validation to propagate the global bootable flag into the snapset definition for downstream logic.
  • Document how to extend only a subset of a snapset by adjusting percent_space_required per volume.

Tests:

  • Add new tests covering bootable snapshot lifecycle, including creation, verification, idempotence, and removal.
  • Add tests ensuring attempts to remove snapshot origin volumes fail with the expected LVM error message.
  • Update existing bootable-related test to validate bootable state via snapm instead of relying solely on role behavior.

trgill added 4 commits January 4, 2026 20:53
Add check to test that uses snapm to validate the snapset attribute is set
to bootable.

Signed-off-by: Todd Gill <tgill@redhat.com>
Update the snapmgr.py module to use manager.mounts features.

Signed-off-by: Todd Gill <tgill@redhat.com>
Attempt to remove an origin of a snapshot and validate that the
attempt fails.

Signed-off-by: Todd Gill <tgill@redhat.com>
…lumes

Signed-off-by: Todd Gill <tgill@redhat.com>
@sourcery-ai
Copy link

sourcery-ai bot commented Jan 20, 2026

Reviewer's Guide

Switches snapshot mount/umount operations to use snapm when available, introduces explicit bootable snapshot handling and validation, and adds tests and status codes to cover bootable behavior and error paths.

Sequence diagram for snapm-based snapshot mount command

sequenceDiagram
    actor User
    participant AnsibleModule as AnsibleModule
    participant Snapmgr as snapmgr_mgmt
    participant Manager as snap_manager_Manager
    participant SnapmMounts as snapm_Mounts

    User->>AnsibleModule: invoke snapshot module (mount)
    AnsibleModule->>Snapmgr: mgr_mount_cmd(module, module_args, snapset_json)
    Snapmgr->>Manager: Manager()
    Snapmgr->>Manager: find_snapshot_sets(Selection(name))
    Manager-->>Snapmgr: snapsets
    Snapmgr->>Snapmgr: mgr_mount_snapshot_set(module, manager, snapsets[0], snapset_json, verify_only)

    alt verify_only is False
        Snapmgr->>Manager: find_snapshot_sets(Selection(name))
        Manager-->>Snapmgr: snapsets
        Snapmgr->>SnapmMounts: mount(snapsets[0])
        SnapmMounts-->>Snapmgr: success or exception
        Snapmgr-->>AnsibleModule: SnapshotStatus and changed=True/False
    else verify_only is True
        loop for each volume in snapset_json.volumes
            Snapmgr->>Snapmgr: determine lv_to_check (origin or mgr_get_snapshot_name)
            Snapmgr->>AnsibleModule: mount_verify(module, mountpoint, vg_name, lv_to_check)
            AnsibleModule-->>Snapmgr: rc, message
            Snapmgr->>Snapmgr: abort on error
        end
        Snapmgr-->>AnsibleModule: SnapshotStatus.SNAPSHOT_OK, changed=False
    end

    AnsibleModule-->>User: return results
Loading

Sequence diagram for snapm-based snapshot unmount command

sequenceDiagram
    actor User
    participant AnsibleModule as AnsibleModule
    participant Snapmgr as snapmgr_mgmt
    participant Manager as snap_manager_Manager
    participant SnapmMounts as snapm_Mounts

    User->>AnsibleModule: invoke snapshot module (umount)
    AnsibleModule->>Snapmgr: mgr_umount_cmd(module, module_args, snapset_json)
    Snapmgr->>Manager: Manager()
    Snapmgr->>Manager: find_snapshot_sets(Selection(name))
    Manager-->>Snapmgr: snapsets
    Snapmgr->>Snapmgr: mgr_umount_snapshot_set(module, manager, snapsets[0], snapset_json, verify_only)

    alt verify_only is False
        Snapmgr->>SnapmMounts: umount(snapset)
        SnapmMounts-->>Snapmgr: success or exception
        Snapmgr-->>AnsibleModule: SnapshotStatus and changed=True/False
    else verify_only is True
        loop for each volume in snapset_json.volumes
            Snapmgr->>Snapmgr: determine lv_to_check (origin or mgr_get_snapshot_name)
            Snapmgr->>AnsibleModule: umount_verify(module, mountpoint, vg_name, lv_to_check)
            AnsibleModule-->>Snapmgr: rc, message
            Snapmgr->>Snapmgr: abort on error
        end
        Snapmgr-->>AnsibleModule: SnapshotStatus.SNAPSHOT_OK, changed=False
    end

    AnsibleModule-->>User: return results
Loading

Sequence diagram for bootable snapshot validation using legacy LVM path

sequenceDiagram
    actor User
    participant AnsibleModule as AnsibleModule
    participant Validate as validate_module
    participant Lvm as lvm_module

    User->>AnsibleModule: invoke snapshot module (create bootable)
    AnsibleModule->>Validate: validate_snapset_json(cmd, module.params, verify_only=False)
    Validate->>Validate: snapset_dict = module_args[snapshot_lvm_set]
    Validate->>Validate: set snapset_dict.snapshot_lvm_bootable from module_args if present
    Validate-->>AnsibleModule: cmd_result, snapset_dict

    AnsibleModule->>Lvm: snapshot_set(module, snapset_dict, check_mode)
    Lvm->>Lvm: check bootable flags
    alt bootable requested and using legacy LVM
        Lvm-->>AnsibleModule: ERROR_BOOTABLE_NOT_SUPPORTED, message, changed=False
    else non bootable
        Lvm->>Lvm: verify_snapset_source_lvs_exist
        Lvm-->>AnsibleModule: status, message, changed
    end

    AnsibleModule-->>User: return results
Loading

Updated class diagram for snapshot management and status types

classDiagram
    class SnapshotStatus {
        <<enumeration>>
        int SNAPSHOT_OK
        int ERROR_MOUNT_FAILED
        int ERROR_UMOUNT_FAILED
        int ERROR_SNAPM_INTERNAL_ERROR
        int ERROR_BOOTABLE_NOT_SUPPORTED
        int ERROR_BOOTABLE_CONFLICT
    }

    class snapmgr_module {
        +mgr_snapshot_cmd(module, module_args, snapset_json)
        +mgr_mount_cmd(module, module_args, snapset_json)
        +mgr_umount_cmd(module, module_args, snapset_json)
        +mgr_mount_snapshot_set(module, manager, snapset, snapset_json, verify_only)
        +mgr_umount_snapshot_set(module, manager, snapset, snapset_json, verify_only)
    }

    class snap_manager_Manager {
        +find_snapshot_sets(selection)
        +mounts: snapm_Mounts
    }

    class snapm_Mounts {
        +mount(snapset)
        +umount(snapset)
    }

    class validate_module {
        +validate_snapset_json(cmd, module_args, verify_only)
    }

    class lvm_module {
        +snapshot_set(module, snapset_json, check_mode)
    }

    class utils_module {
        +to_bool(value)
        +mgr_get_snapshot_name(module, vg_name, lv_name, snapset)
        +mount_verify(module, mountpoint, vg_name, lv_to_check)
        +umount_verify(module, mountpoint, vg_name, lv_to_check)
        +verify_source_lvs_exist(module, snapset_json)
    }

    snapmgr_module --> snap_manager_Manager : uses
    snap_manager_Manager --> snapm_Mounts : composition
    snapmgr_module --> utils_module : uses
    snapmgr_module --> SnapshotStatus : returns
    validate_module --> SnapshotStatus : returns
    lvm_module --> SnapshotStatus : returns
    lvm_module --> utils_module : uses verify_source_lvs_exist
    validate_module --> lvm_module : prepares snapset_json for
Loading

File-Level Changes

Change Details Files
Route mount/umount operations through snapm manager APIs with verification fallbacks instead of legacy helpers.
  • Replace direct mount_snapshot_set/umount_snapshot_set calls with mgr_mount_snapshot_set/mgr_umount_snapshot_set wrappers using snapm.Manager and its mounts interface.
  • Implement mgr_mount_snapshot_set and mgr_umount_snapshot_set to call snapm manager.mounts.mount/umount in non-verify mode, with verification-only paths using mount_verify/umount_verify and mgr_get_snapshot_name.
  • Adjust mgr_mount_cmd and mgr_umount_cmd to work with snapm snapshot set discovery results and updated helper signatures, including corrected error codes for umount failures.
module_utils/snapshot_lsr/snapmgr.py
Introduce explicit handling, validation, and error codes for bootable snapshots, including conflict detection between global and per-snapset settings.
  • In mgr_snapshot_cmd, derive the bootable flag from module_args or snapset_json with conflict detection, returning a dedicated ERROR_BOOTABLE_CONFLICT status on mismatch.
  • Update validate_snapset_json to accept full module parameters, inject snapshot_lvm_bootable into the snapset dict when set, and wire this into snapshot.py.
  • Add validation in lvm.snapshot_set to reject bootable snapshot requests when snapm is not in use, returning ERROR_BOOTABLE_NOT_SUPPORTED with a clear message.
  • Extend SnapshotStatus with ERROR_BOOTABLE_NOT_SUPPORTED and ERROR_BOOTABLE_CONFLICT codes.
module_utils/snapshot_lsr/snapmgr.py
module_utils/snapshot_lsr/validate.py
library/snapshot.py
module_utils/snapshot_lsr/lvm.py
module_utils/snapshot_lsr/consts.py
Adjust and extend tests to cover bootable behavior, snapm-based status checks, and error scenarios.
  • Refactor tests_set_bootable to pass snapshot_lvm_bootable as a module variable, fetch bootable status via a new get_snapset_status.yml helper that calls snapm snapset show, and assert that the snapset is marked bootable.
  • Add tests_basic_bootable.yml to validate end-to-end bootable snapshot lifecycle (create, verify, idempotence, remove, verify remove) when using snapm.
  • Add tests_remove_member_fail.yml to validate behavior when attempting to remove an LVM origin volume that is a snapshot origin, asserting correct failure messaging.
  • Introduce get_snapset_status.yml task file to query snapm for snapset metadata and expose is_bootable and boot entries as Ansible facts.
tests/tests_set_bootable.yml
tests/tests_basic_bootable.yml
tests/tests_remove_member_fail.yml
tests/get_snapset_status.yml
Minor documentation improvements related to snapshot extension behavior.
  • Document in README that percent_space_required can be adjusted to extend a subset of volumes in a snapset by changing the value for only those volumes.
README.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

mountpoint = list_item["mountpoint"]

if list_item.get("all_targets") is not None:
all_targets = to_bool(list_item["all_targets"])

Check warning

Code scanning / CodeQL

Variable defined multiple times Warning

This assignment to 'all_targets' is unnecessary as it is
redefined
before this value is used.
mountpoint = list_item["mountpoint"]

if list_item.get("all_targets") is not None:
all_targets = to_bool(list_item["all_targets"])

Check warning

Code scanning / CodeQL

Variable defined multiple times Warning

This assignment to 'all_targets' is unnecessary as it is
redefined
before this value is used.

return SnapshotStatus.SNAPSHOT_OK, "", changed

return rc, message, changed

Check warning

Code scanning / CodeQL

Unreachable code Warning

This statement is unreachable.
@@ -1,9 +1,13 @@
from __future__ import absolute_import, division, print_function

from module_utils.snapshot_lsr.utils import mount

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'mount' is not used.
__metaclass__ = type

import logging
import os

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'os' is not used.
import logging
import os
from os.path import join as path_join
import stat

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'stat' is not used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant