probert: make failed commands actually produce CalledProcessError

[ogayot]

In multiple places, probert had except subprocess.CalledProcessError blocks around calls to subprocess.run(). However, such an exception can only be raised when check=True is passed.

This means the except subprocess.CalledProcessError blocks where essentially unreachable, and only covered by mock side effects.

We now pass check=True to all relevant commands.

Note that commands failing with output would previously be parsed as if they succeeded. With this change, parsing will be skipped if the commands fail.

Originally, this bug was pointed out by @copilot in #169.

[Copilot]

Pull request overview

This PR makes subprocess failures actually raise subprocess.CalledProcessError by adding check=True to several subprocess.run() calls, ensuring existing except subprocess.CalledProcessError blocks are reachable and preventing parsing of failed-command output as if it were valid.

Changes:

• Add check=True to multiple subprocess.run() invocations across ZFS/LVM/RAID/multipath/mount/dasd/storage probing helpers.
• Expand/adjust unit tests to assert check=True usage and validate failure-path behavior (including new targeted test modules for RAID and mount).
• Ensure failure paths skip parsing and return empty/neutral results as intended.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
probert/zfs.py	Adds check=True so ZFS command failures raise and are handled.
probert/storage.py	Adds check=True to sfdisk probing to make failures raise/handled.
probert/raid.py	Adds check=True to mdadm calls so failures raise and log paths are exercised.
probert/multipath.py	Adds check=True so multipathd failures raise and return [].
probert/mount.py	Adds check=True so findmnt failures raise and return {}.
probert/lvm.py	Adds check=True so LVM probe and scan failures raise and are handled.
probert/dasd.py	Adds check=True so dasdview failures raise and are handled.
probert/tests/test_zfs.py	Adds/updates tests to assert check=True and validate failure returns.
probert/tests/test_storage.py	Updates imports and adds an async test covering sfdisk failure behavior with check=True.
probert/tests/test_raid.py	New tests asserting mdadm failure logging and check=True invocation.
probert/tests/test_multipath.py	Adds parameterized failure tests for _extract_mpath_data with check=True.
probert/tests/test_mount.py	New tests validating findmnt success/failure paths with check=True.
probert/tests/test_lvm.py	Updates failure-path test to align with check=True and asserts invocation.
probert/tests/test_dasd.py	Adds parameterized failure test and updates assertions for check=True.

Comments suppressed due to low confidence (3)

probert/storage.py:122

• sfdisk may not be installed on all target systems; in that case subprocess.run() raises FileNotFoundError, which currently bubbles up and can abort blockdev_probe(). Since partition-table probing is optional metadata, treat missing sfdisk the same as a failing command and return None.

            result = subprocess.run(cmd, stdout=subprocess.PIPE,
                                    stderr=subprocess.DEVNULL,
                                    check=True)
            output = result.stdout.decode('utf-8')
        except subprocess.CalledProcessError as e:
            log.error('Failed to probe partition table on %s:%s', devname, e)
            return None

probert/lvm.py:57

• If LVM tooling (e.g. pvs/vgs/lvs) is not installed, subprocess.run() raises FileNotFoundError, which is not currently handled and can abort probing. Consider treating FileNotFoundError the same as a failing command and returning an empty list.

    try:
        result = subprocess.run(cmd, stdout=subprocess.PIPE,
                                stderr=subprocess.DEVNULL,
                                check=True)
        output = result.stdout.decode('utf-8')
    except subprocess.CalledProcessError as e:
        log.error('Failed to probe LVM devices on system: %s', e)
        return []

probert/raid.py:40

• mdadm_assemble() takes an ignore_errors parameter, but failures are currently always swallowed. With check=True this means callers cannot opt into strict behavior (and the parameter becomes misleading). Consider honoring ignore_errors by re-raising when it is False.

    except subprocess.CalledProcessError as e:
        log.error('Failed mdadm_assemble command %s: %s', cmd, e)
    except FileNotFoundError as e:
        log.error('Failed mdadm_assemble, mdadm command not found: %s', e)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ogayot]

Right, let's consider this a separate issue.

[dbungert]

This is a legitimate concern - zfsutils-linux is indeed not installed by default in the live layer, instead currently installed as needed at runtime. (maybe it should be?)

Presumably we could make that case for all of these? But some will be unlikely due to being in the live layer or minimal or standard.

[ogayot]

This is a legitimate concern

Interestingly, the two functions that use the zfs command are only called if zdb_asdict (which internally uses zdb - also provided by zfsutils-linux) returns data. And zdb_asdict already guards against missing zdb.

Presumably we could make that case for all of these? But some will be unlikely due to being in the live layer or minimal or standard.

Yes, I think it would make sense across the whole code tree. In the RAID code, we have two except blocks, one for subprocess.CalledProcessError and one for FileNotFoundError - to display a different error message.

I'm all in for unifying the implementation.

[dbungert]

this and the previous assert seems redundant, I guess keep this one?.

[ogayot]

This one is to make sure we pass check=True (so that the except block is actually useful) whereas the previous one is to ensure we can find the command that failed in the logs.

I'd like to preserve both, but I will reorder the assertions and factorize the command list.

[dbungert]

see previous