fix: ensure libblockdev-loop package on EL7 for loop mounts

[richm]

Cause: The blivet module on EL7 will try to use a function that is undefined when checking
for mounts if there is a loop mount (/dev/loopX) on the system. The function is provided
by the libblockdev-loop package but that isn't always installed.

Consequence: The role will give an error like:
"The function 'bd_loop_get_backing_file' called, but not implemented"

Fix: Ensure that the libblockdev-loop package is installed if there are loop mounts
on the EL7 system.

Result: The role works on EL7 when there are loop mounts.

Added a test to create a loop mount.

Signed-off-by: Rich Megginson rmeggins@redhat.com

Summary by Sourcery

Ensure EL7 systems with loop mounts have the required libblockdev-loop package installed to prevent blivet errors and add coverage for loop-mount scenarios in tests.

Bug Fixes:

• Install libblockdev-loop on EL7 systems with detected loop mounts to avoid blivet calling an undefined bd_loop_get_backing_file function.

Tests:

• Add an integration test that creates a loop-backed mount, runs the storage role, and then cleans up the loop device and mount.

[sourcery-ai]

Reviewer's Guide

Ensures libblockdev-loop is installed on EL7 hosts with loop mounts to avoid blivet runtime errors, and adds an integration test that creates and cleans up a loop mount to exercise this behavior.

Sequence diagram for libblockdev-loop installation on EL7 with loop mounts

sequenceDiagram
  participant Controller
  participant AnsibleRole as Role_storage
  participant TargetHost
  participant PackageManager

  Controller->>AnsibleRole: Run tasks/main-blivet.yml
  AnsibleRole->>TargetHost: Check ansible_facts.os_family and distribution_major_version
  TargetHost-->>AnsibleRole: RedHat, 7
  AnsibleRole->>TargetHost: Evaluate package_info.packages and ansible_facts.packages for libblockdev-loop
  TargetHost-->>AnsibleRole: libblockdev-loop not present
  AnsibleRole->>TargetHost: setup gather_subset: mounts
  TargetHost-->>AnsibleRole: ansible_facts.mounts including /dev/loopX
  AnsibleRole->>TargetHost: package libblockdev-loop state: present
  TargetHost->>PackageManager: Install libblockdev-loop
  PackageManager-->>TargetHost: libblockdev-loop installed
  TargetHost-->>AnsibleRole: Package install result
  AnsibleRole-->>Controller: Continue role execution without blivet error

Loading

Flow diagram for EL7 libblockdev-loop installation logic

flowchart TD
  A[Start tasks/main-blivet.yml] --> B{os_family == RedHat?}
  B -->|No| Z[Skip libblockdev-loop block]
  B -->|Yes| C{distribution_major_version == 7?}
  C -->|No| Z
  C -->|Yes| D{libblockdev-loop in package_info.packages?}
  D -->|Yes| Z
  D -->|No| E{libblockdev-loop in ansible_facts.packages?}
  E -->|Yes| Z
  E -->|No| F[setup gather_subset: mounts]
  F --> G{ansible_facts.mounts has device matching ^/dev/loop?}
  G -->|No| Z
  G -->|Yes| H[package name: libblockdev-loop state: present
use: rhel_rpm_ostree if __storage_is_ostree else omit]
  H --> Z
  Z[Continue with Make sure required packages are installed]

Loading

File-Level Changes

Change	Details	Files
Conditionally install libblockdev-loop on EL7 systems when loop devices are mounted to work around a blivet/libblockdev bug.	Add an EL7-specific task block that refreshes mount facts and installs libblockdev-loop only when not already present Use ansible_facts and package_info to avoid redundant installation when libblockdev-loop is already installed Limit the installation task to systems with /dev/loop* devices detected in ansible_facts["mounts"]	tasks/main-blivet.yml
Add an integration-style test that creates a loop mount before running the storage role and cleans it up afterward.	Create a small loop-backed filesystem, mount it, and verify loop mounts exist before running the role Run the linux-system-roles.storage role under this loop-mount condition Unmount and remove the loop-backed filesystem and mountpoint as test cleanup	tests/tests_change_disk_mount.yml

---

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

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've left some high level feedback:

• The condition checking if libblockdev-loop is installed mixes package_info.packages and ansible_facts['packages'] with a selectattr('name', ...) that assumes a list of objects; since ansible_facts['packages'] is typically a dict keyed by package name, consider normalizing on a single, reliable source (e.g. just package_info or using 'libblockdev-loop' in ansible_facts['packages']) to avoid type mismatches and make the check more robust.
• In the test playbook, the loop device setup/cleanup currently uses raw shell commands for mount/umount and directory handling; consider using the mount and file modules where possible to make the test idempotent and less dependent on shell semantics.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The condition checking if `libblockdev-loop` is installed mixes `package_info.packages` and `ansible_facts['packages']` with a `selectattr('name', ...)` that assumes a list of objects; since `ansible_facts['packages']` is typically a dict keyed by package name, consider normalizing on a single, reliable source (e.g. just `package_info` or using `'libblockdev-loop' in ansible_facts['packages']`) to avoid type mismatches and make the check more robust.
- In the test playbook, the loop device setup/cleanup currently uses raw `shell` commands for `mount`/`umount` and directory handling; consider using the `mount` and `file` modules where possible to make the test idempotent and less dependent on shell semantics.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[richm]

[citest]

[vojtechtrefny]

Why not add this the loop plugin vars/RedHat_7.yml? It would be installed every time, but we already install other libblockdev plugins and the loop one is the smallest one, so it shouldn't be a big problem to install it on systems without loop devices.

[richm]

Why not add this the loop plugin vars/RedHat_7.yml? It would be installed every time, but we already install other libblockdev plugins and the loop one is the smallest one, so it shouldn't be a big problem to install it on systems without loop devices.

I thought about that - there is some risk in installing a "new" package on el7 which is supposed to be a "stable" platform - that is - the user runs the role on el7 expecting no changes, but they get the unexpected change of a new libblockdev-loop package being installed - perhaps that isn't worth worrying about

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 10.33%. Comparing base (59fd1c6) to head (4c96482).
⚠️ Report is 115 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (59fd1c6) and HEAD (4c96482). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (59fd1c6)	HEAD (4c96482)
sanity	1	0

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #591      +/-   ##
==========================================
- Coverage   16.54%   10.33%   -6.22%     
==========================================
  Files           2        8       +6     
  Lines         284     2023    +1739     
  Branches       79        0      -79     
==========================================
+ Hits           47      209     +162     
- Misses        237     1814    +1577     

Flag	Coverage Δ
sanity	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[richm]

Why not add this the loop plugin vars/RedHat_7.yml? It would be installed every time, but we already install other libblockdev plugins and the loop one is the smallest one, so it shouldn't be a big problem to install it on systems without loop devices.

After further thought - I think this is the way to go - however, this means that a user who upgrades to this version of the storage role and runs the role may see an unexpected change due to the library being installed if if was not already installed - I think this issue is minor enough not to worry about it

[richm]

[citest]