diff --git a/python/zfs/__init__.py b/python/zfs/__init__.py index 74d7d0b..6af5ee8 100644 --- a/python/zfs/__init__.py +++ b/python/zfs/__init__.py @@ -1,11 +1,30 @@ """init.""" -from python.zfs.dataset import Dataset, Snapshot, get_datasets +# run_command is deliberately not re-exported here. It is available from +# python.zfs.command when something genuinely needs another binary, but the +# wrappers are what callers should reach for by default. +from python.zfs.command import CommandResult, run_zfs, run_zpool +from python.zfs.dataset import ( + Dataset, + Snapshot, + create_dataset, + get_datasets, + get_properties, + list_dataset_names, + set_property, +) from python.zfs.zpool import Zpool __all__ = [ + "CommandResult", "Dataset", "Snapshot", "Zpool", + "create_dataset", "get_datasets", + "get_properties", + "list_dataset_names", + "run_zfs", + "run_zpool", + "set_property", ] diff --git a/python/zfs/command.py b/python/zfs/command.py new file mode 100644 index 0000000..0eeca84 --- /dev/null +++ b/python/zfs/command.py @@ -0,0 +1,90 @@ +"""Running zfs and zpool commands. + +One implementation shared by both, so the zpool side gets the same handling the +zfs side does: arguments passed as a list, streams kept apart, and failures +returned as data rather than guessed at by the caller. +""" + +from __future__ import annotations + +import logging +import subprocess +from dataclasses import dataclass + +logger = logging.getLogger(__name__) + + +@dataclass(frozen=True) +class CommandResult: + """The outcome of a single zfs or zpool invocation.""" + + args: tuple[str, ...] + stdout: str + stderr: str + return_code: int + + @property + def ok(self) -> bool: + """Whether the command reported success.""" + return self.return_code == 0 + + @property + def message(self) -> str: + """The most useful description of what went wrong.""" + return (self.stderr or self.stdout).strip() + + +def run_command(*args: str) -> CommandResult: + """Run a command, passing arguments as a list rather than a shell string. + + Two things this buys over bash_wrapper. Arguments are never split on + whitespace, so a value containing a space arrives intact. And stdout stays + separate from stderr, so a warning on a successful command is never + mistaken for output, which bash_wrapper does whenever stderr is non-empty + regardless of the return code. + + The encoding is pinned rather than using text=True, which would decode with + the locale encoding. These run from systemd units, where LANG is often + unset. + + Args: + *args: The command and its arguments. + + Returns: + CommandResult: The streams and return code, never raising on failure. + """ + completed = subprocess.run(list(args), capture_output=True, encoding="utf-8", check=False) + + if completed.returncode != 0: + logger.debug(f"{' '.join(args)} exited {completed.returncode}: {completed.stderr.strip()}") + + return CommandResult( + args=tuple(args), + stdout=completed.stdout, + stderr=completed.stderr, + return_code=completed.returncode, + ) + + +def run_zfs(*args: str) -> CommandResult: + """Run a zfs command. + + Args: + *args: The arguments to pass to zfs. + + Returns: + CommandResult: The streams and return code. + """ + return run_command("zfs", *args) + + +def run_zpool(*args: str) -> CommandResult: + """Run a zpool command. + + Args: + *args: The arguments to pass to zpool. + + Returns: + CommandResult: The streams and return code. + """ + return run_command("zpool", *args) diff --git a/python/zfs/dataset.py b/python/zfs/dataset.py index a184c78..2fa8958 100644 --- a/python/zfs/dataset.py +++ b/python/zfs/dataset.py @@ -8,6 +8,7 @@ from datetime import UTC, datetime from typing import Any from python.common import bash_wrapper +from python.zfs.command import run_zfs logger = logging.getLogger(__name__) @@ -207,8 +208,91 @@ def get_datasets() -> list[Dataset]: """ logger.info("Getting zfs list") - dataset_names, _ = bash_wrapper("zfs list -Hp -t filesystem -o name") + return [Dataset(dataset_name) for dataset_name in list_dataset_names() if "/" in dataset_name] - cleaned_datasets = dataset_names.strip().split("\n") - return [Dataset(dataset_name) for dataset_name in cleaned_datasets if "/" in dataset_name] +def get_properties(name: str) -> dict[str, tuple[str, str]]: + """Get every property of a dataset along with where its value came from. + + The source is what distinguishes a property that was deliberately set on + this dataset from one that is merely inherited or left at its default. + + Args: + name (str): The name of the dataset. + + Returns: + dict[str, tuple[str, str]]: A mapping of property name to (value, source). + """ + result = run_zfs("get", "-Hp", "-o", "property,value,source", "all", name) + if not result.ok: + error = f"Failed to get properties for {name}: {result.message}" + raise RuntimeError(error) + + properties = {} + for line in result.stdout.strip().splitlines(): + if not line: + continue + prop, value, source = line.split("\t") + properties[prop] = (value, source) + + return properties + + +def create_dataset(name: str, properties: dict[str, str]) -> str | None: + """Create a dataset with the given properties. + + Args: + name (str): The name of the dataset. + properties (dict[str, str]): The properties to create the dataset with. + + Returns: + str | None: An error message on failure, None on success. + """ + options = [argument for key, value in sorted(properties.items()) for argument in ("-o", f"{key}={value}")] + logger.debug(f"creating {name} with {properties}") + + result = run_zfs("create", *options, name) + if not result.ok: + return f"Failed to create {name}: {result.message}" + return None + + +def set_property(name: str, key: str, value: str) -> str | None: + """Set a single property on a dataset. + + Args: + name (str): The name of the dataset. + key (str): The property to set. + value (str): The value to set the property to. + + Returns: + str | None: An error message on failure, None on success. + """ + logger.debug(f"setting {key}={value} on {name}") + + result = run_zfs("set", f"{key}={value}", name) + if not result.ok: + return f"Failed to set {key}={value} on {name}: {result.message}" + return None + + +def list_dataset_names() -> list[str]: + """List every zfs filesystem name, including pool root filesystems. + + Unlike get_datasets this does not build Dataset objects and does not filter + out pool roots, which makes it usable for existence checks. + + Returns: + list[str]: The names of every zfs filesystem. + + Raises: + RuntimeError: If zfs list fails. Never returns a partial or error + derived list, since treating stderr as dataset names would make + the reconciler think every dataset is missing. + """ + result = run_zfs("list", "-Hp", "-t", "filesystem", "-o", "name") + if not result.ok: + error = f"Failed to list ZFS datasets: {result.message}" + raise RuntimeError(error) + + return [name for name in result.stdout.strip().splitlines() if name] diff --git a/python/zfs/zpool.py b/python/zfs/zpool.py index 95a093b..71f11f3 100644 --- a/python/zfs/zpool.py +++ b/python/zfs/zpool.py @@ -1,28 +1,42 @@ -"""test.""" +"""zpool.""" from __future__ import annotations import json from typing import Any -from python.common import bash_wrapper +from python.zfs.command import run_zpool -def _zpool_list(zfs_list: str) -> dict[str, Any]: - """Check the version of zfs.""" - raw_zfs_list_data, _ = bash_wrapper(zfs_list) +def _zpool_list(*args: str) -> dict[str, Any]: + """Run a zpool list and check the output is a format we understand. - zfs_list_data = json.loads(raw_zfs_list_data) + Args: + *args: The arguments to pass to zpool. - vers_major = zfs_list_data["output_version"]["vers_major"] - vers_minor = zfs_list_data["output_version"]["vers_minor"] - command = zfs_list_data["output_version"]["command"] + Returns: + dict[str, Any]: The decoded output. + + Raises: + RuntimeError: If zpool fails, or reports a format this does not parse. + Never decodes a partial or error derived payload. + """ + result = run_zpool(*args) + if not result.ok: + error = f"Failed to run zpool {' '.join(args)}: {result.message}" + raise RuntimeError(error) + + zpool_data = json.loads(result.stdout) + + vers_major = zpool_data["output_version"]["vers_major"] + vers_minor = zpool_data["output_version"]["vers_minor"] + command = zpool_data["output_version"]["command"] if vers_major != 0 or vers_minor != 1 or command != "zpool list": error = f"Datasets are not in the correct format {vers_major=} {vers_minor=} {command=}" raise RuntimeError(error) - return zfs_list_data + return zpool_data class Zpool: @@ -33,7 +47,7 @@ class Zpool: name: str, ) -> None: """__init__.""" - zpool_data = _zpool_list(f"zpool list {name} -pHj -o all") + zpool_data = _zpool_list("list", name, "-pHj", "-o", "all") properties = zpool_data["pools"][name]["properties"] diff --git a/tests/test_zfs.py b/tests/test_zfs.py index 6c6621a..876fa4b 100644 --- a/tests/test_zfs.py +++ b/tests/test_zfs.py @@ -2,15 +2,35 @@ import json from datetime import UTC, datetime +from subprocess import CompletedProcess +from typing import TYPE_CHECKING from unittest.mock import call import pytest -from pytest_mock import MockerFixture -from python.zfs import Dataset, Snapshot, Zpool, get_datasets +from python.zfs import ( + CommandResult, + Dataset, + Snapshot, + Zpool, + create_dataset, + get_datasets, + get_properties, + list_dataset_names, + run_zfs, + run_zpool, + set_property, +) + +# Not re-exported from python.zfs on purpose: the wrappers are the default. +from python.zfs.command import run_command from python.zfs.dataset import _zfs_list from python.zfs.zpool import _zpool_list +if TYPE_CHECKING: + from pytest_mock import MockerFixture + +COMMAND = "python.zfs.command" DATASET = "python.zfs.dataset" ZPOOL = "python.zfs.zpool" SAMPLE_SNAPSHOT_DATA = { @@ -207,12 +227,20 @@ def test_zfs_list_version_check(mocker: MockerFixture) -> None: def test_get_datasets(mocker: MockerFixture) -> None: """Test get_datasets.""" - mock_bash = mocker.patch(f"{DATASET}.bash_wrapper", return_value=("pool/dataset\npool/other\ninvalid", 0)) + mock_run = mocker.patch( + f"{DATASET}.run_zfs", + return_value=CommandResult( + args=(), + stdout="pool/dataset\npool/other\ninvalid", + stderr="", + return_code=0, + ), + ) mock_dataset = mocker.patch(f"{DATASET}.Dataset") get_datasets() - mock_bash.assert_called_once_with("zfs list -Hp -t filesystem -o name") + mock_run.assert_called_once_with("list", "-Hp", "-t", "filesystem", "-o", "name") calls = [call("pool/dataset"), call("pool/other")] @@ -287,11 +315,16 @@ def test_zpool_repr(mocker: MockerFixture) -> None: def test_zpool_list(mocker: MockerFixture) -> None: """Test version validation in _zpool_list.""" mocker.patch( - f"{ZPOOL}.bash_wrapper", - return_value=(json.dumps({"output_version": {"vers_major": 0, "vers_minor": 1, "command": "zpool list"}}), 0), + f"{ZPOOL}.run_zpool", + return_value=CommandResult( + args=(), + stdout=json.dumps({"output_version": {"vers_major": 0, "vers_minor": 1, "command": "zpool list"}}), + stderr="", + return_code=0, + ), ) - result = _zpool_list("zpool list invalid -pHj -o all") + result = _zpool_list("list", "invalid", "-pHj", "-o", "all") assert result == {"output_version": {"command": "zpool list", "vers_major": 0, "vers_minor": 1}} @@ -299,11 +332,352 @@ def test_zpool_list(mocker: MockerFixture) -> None: def test_zpool_list_version_check(mocker: MockerFixture) -> None: """Test version validation in _zpool_list.""" mocker.patch( - f"{ZPOOL}.bash_wrapper", - return_value=(json.dumps({"output_version": {"vers_major": 1, "vers_minor": 0, "command": "zpool list"}}), 0), + f"{ZPOOL}.run_zpool", + return_value=CommandResult( + args=(), + stdout=json.dumps({"output_version": {"vers_major": 1, "vers_minor": 0, "command": "zpool list"}}), + stderr="", + return_code=0, + ), ) with pytest.raises(RuntimeError) as excinfo: - _zpool_list("zpool list invalid -pHj -o all") + _zpool_list("list", "invalid", "-pHj", "-o", "all") assert "Datasets are not in the correct format" in str(excinfo.value) + + +# -- run_zfs, the subprocess boundary ----------------------------------------- + + +def completed(returncode: int = 0, stdout: str = "", stderr: str = "") -> CompletedProcess: + """Build a CompletedProcess the way subprocess.run would return one.""" + return CompletedProcess(args=["zfs"], returncode=returncode, stdout=stdout, stderr=stderr) + + +def test_run_zfs_passes_arguments_as_a_list(mocker: MockerFixture) -> None: + """Arguments must never be joined into a string and re-split.""" + mock_run = mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed(stdout="ok\n")) + + run_zfs("get", "-Hp", "all", "media/temp") + + mock_run.assert_called_once_with( + ["zfs", "get", "-Hp", "all", "media/temp"], + capture_output=True, + encoding="utf-8", + check=False, + ) + + +def test_run_zfs_keeps_values_containing_spaces_intact(mocker: MockerFixture) -> None: + """A property value with a space must reach zfs as one argument. + + bash_wrapper split on whitespace, so this silently became several + arguments and zfs was handed something it could not parse. + """ + mock_run = mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed()) + + run_zfs("set", "mountpoint=/zfs/two words", "media/temp") + + assert mock_run.call_args.args[0] == ["zfs", "set", "mountpoint=/zfs/two words", "media/temp"] + + +def test_run_zfs_reports_success(mocker: MockerFixture) -> None: + mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed(stdout="output\n", stderr="")) + + result = run_zfs("list") + + assert result.ok + assert result.stdout == "output\n" + assert result.return_code == 0 + assert result.args == ("zfs", "list") + + +def test_run_zfs_returns_failures_rather_than_raising(mocker: MockerFixture) -> None: + """Callers decide what a failure means, so run_zfs never raises.""" + mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed(returncode=1, stderr="no such pool\n")) + + result = run_zfs("list") + + assert not result.ok + assert result.stderr == "no such pool\n" + assert result.message == "no such pool" + + +def test_run_zfs_keeps_streams_separate(mocker: MockerFixture) -> None: + """A warning on stderr must not contaminate stdout. + + bash_wrapper returned stderr in place of stdout whenever stderr was + non-empty, even on success, which is how a warning could be parsed as a + list of dataset names. + """ + mocker.patch( + f"{COMMAND}.subprocess.run", + return_value=completed(stdout="pool/one\n", stderr="warning: something\n"), + ) + + result = run_zfs("list") + + assert result.ok + assert result.stdout == "pool/one\n" + assert result.stderr == "warning: something\n" + + +def test_command_result_message_falls_back_to_stdout() -> None: + """Some zfs errors land on stdout, so the message must not be empty.""" + result = CommandResult(args=("list",), stdout=" something went wrong \n", stderr="", return_code=1) + + assert result.message == "something went wrong" + + +# -- list_dataset_names ------------------------------------------------------- + + +def patch_run(mocker: MockerFixture, **kwargs) -> object: + """Patch run_zfs with a single canned result.""" + return mocker.patch(f"{DATASET}.run_zfs", return_value=CommandResult(args=(), **kwargs)) + + +def test_list_dataset_names_builds_the_right_command(mocker: MockerFixture) -> None: + mock_run = patch_run(mocker, stdout="pool\n", stderr="", return_code=0) + + list_dataset_names() + + mock_run.assert_called_once_with("list", "-Hp", "-t", "filesystem", "-o", "name") + + +def test_list_dataset_names_includes_pool_roots(mocker: MockerFixture) -> None: + """Unlike get_datasets, nothing is filtered out, so existence checks work.""" + patch_run(mocker, stdout="media\nmedia/temp\nmedia/secure/docker\n", stderr="", return_code=0) + + assert list_dataset_names() == ["media", "media/temp", "media/secure/docker"] + + +def test_list_dataset_names_drops_blank_lines(mocker: MockerFixture) -> None: + patch_run(mocker, stdout="media\n\nmedia/temp\n\n", stderr="", return_code=0) + + assert list_dataset_names() == ["media", "media/temp"] + + +def test_list_dataset_names_raises_on_failure(mocker: MockerFixture) -> None: + """Never return a partial list: the reconciler would create everything.""" + patch_run(mocker, stdout="", stderr="cannot open 'media': no such pool\n", return_code=1) + + with pytest.raises(RuntimeError) as excinfo: + list_dataset_names() + + assert "Failed to list ZFS datasets" in str(excinfo.value) + assert "no such pool" in str(excinfo.value) + + +def test_list_dataset_names_never_parses_stderr_as_names(mocker: MockerFixture) -> None: + """The regression this guards: error text read as a dataset list.""" + patch_run(mocker, stdout="", stderr="permission denied\n", return_code=1) + + with pytest.raises(RuntimeError): + list_dataset_names() + + +# -- get_properties ----------------------------------------------------------- + + +def test_get_properties_builds_the_right_command(mocker: MockerFixture) -> None: + mock_run = patch_run(mocker, stdout="", stderr="", return_code=0) + + get_properties("media/temp") + + mock_run.assert_called_once_with("get", "-Hp", "-o", "property,value,source", "all", "media/temp") + + +def test_get_properties_parses_value_and_source(mocker: MockerFixture) -> None: + patch_run( + mocker, + stdout="compression\tzstd-9\tlocal\natime\toff\tinherited from media\nexec\ton\tdefault\n", + stderr="", + return_code=0, + ) + + assert get_properties("media/temp") == { + "compression": ("zstd-9", "local"), + "atime": ("off", "inherited from media"), + "exec": ("on", "default"), + } + + +def test_get_properties_skips_blank_lines(mocker: MockerFixture) -> None: + """A stray blank line in the middle must not raise on unpacking.""" + patch_run(mocker, stdout="compression\tzstd\tlocal\n\natime\toff\tlocal\n", stderr="", return_code=0) + + assert get_properties("media/temp") == { + "compression": ("zstd", "local"), + "atime": ("off", "local"), + } + + +def test_get_properties_raises_on_failure(mocker: MockerFixture) -> None: + patch_run(mocker, stdout="", stderr="dataset does not exist\n", return_code=1) + + with pytest.raises(RuntimeError) as excinfo: + get_properties("media/gone") + + assert "Failed to get properties for media/gone" in str(excinfo.value) + + +# -- create_dataset ----------------------------------------------------------- + + +def test_create_dataset_builds_sorted_option_flags(mocker: MockerFixture) -> None: + mock_run = patch_run(mocker, stdout="", stderr="", return_code=0) + + assert create_dataset("media/temp", {"sync": "disabled", "compression": "zstd-9"}) is None + + mock_run.assert_called_once_with( + "create", + "-o", + "compression=zstd-9", + "-o", + "sync=disabled", + "media/temp", + ) + + +def test_create_dataset_with_no_properties(mocker: MockerFixture) -> None: + mock_run = patch_run(mocker, stdout="", stderr="", return_code=0) + + create_dataset("media/temp", {}) + + mock_run.assert_called_once_with("create", "media/temp") + + +def test_create_dataset_keeps_a_value_with_spaces_together(mocker: MockerFixture) -> None: + mock_run = patch_run(mocker, stdout="", stderr="", return_code=0) + + create_dataset("media/temp", {"mountpoint": "/zfs/two words"}) + + assert mock_run.call_args.args == ("create", "-o", "mountpoint=/zfs/two words", "media/temp") + + +def test_create_dataset_returns_the_error_on_failure(mocker: MockerFixture) -> None: + patch_run(mocker, stdout="", stderr="cannot create 'media/temp': out of space\n", return_code=1) + + error = create_dataset("media/temp", {}) + + assert error is not None + assert "Failed to create media/temp" in error + assert "out of space" in error + + +# -- set_property ------------------------------------------------------------- + + +def test_set_property_builds_the_right_command(mocker: MockerFixture) -> None: + mock_run = patch_run(mocker, stdout="", stderr="", return_code=0) + + assert set_property("media/temp", "compression", "zstd-9") is None + + mock_run.assert_called_once_with("set", "compression=zstd-9", "media/temp") + + +def test_set_property_keeps_a_value_with_spaces_together(mocker: MockerFixture) -> None: + mock_run = patch_run(mocker, stdout="", stderr="", return_code=0) + + set_property("media/temp", "mountpoint", "/zfs/two words") + + assert mock_run.call_args.args == ("set", "mountpoint=/zfs/two words", "media/temp") + + +def test_set_property_returns_the_error_on_failure(mocker: MockerFixture) -> None: + patch_run(mocker, stdout="", stderr="permission denied\n", return_code=1) + + error = set_property("media/temp", "compression", "zstd-9") + + assert error is not None + assert "Failed to set compression=zstd-9 on media/temp" in error + assert "permission denied" in error + + +# -- run_zpool, the same wrapper the zfs side uses ---------------------------- + + +def test_run_zpool_prefixes_the_binary(mocker: MockerFixture) -> None: + mock_run = mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed(stdout="{}")) + + run_zpool("list", "media", "-pHj", "-o", "all") + + mock_run.assert_called_once_with( + ["zpool", "list", "media", "-pHj", "-o", "all"], + capture_output=True, + encoding="utf-8", + check=False, + ) + + +def test_run_zpool_reports_failures(mocker: MockerFixture) -> None: + mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed(returncode=1, stderr="no such pool\n")) + + result = run_zpool("list", "gone") + + assert not result.ok + assert result.message == "no such pool" + assert result.args == ("zpool", "list", "gone") + + +def test_run_zpool_keeps_streams_separate(mocker: MockerFixture) -> None: + """The same contamination bug the zfs side had must not exist here.""" + mocker.patch( + f"{COMMAND}.subprocess.run", + return_value=completed(stdout="{}", stderr="warning: pool is degraded\n"), + ) + + result = run_zpool("list") + + assert result.ok + assert result.stdout == "{}" + + +def test_run_zpool_keeps_values_containing_spaces_intact(mocker: MockerFixture) -> None: + mock_run = mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed()) + + run_zpool("set", "comment=two words", "media") + + assert mock_run.call_args.args[0] == ["zpool", "set", "comment=two words", "media"] + + +def test_run_command_runs_what_it_is_given(mocker: MockerFixture) -> None: + """zfs and zpool are the same function with a different first argument.""" + mock_run = mocker.patch(f"{COMMAND}.subprocess.run", return_value=completed()) + + run_command("zpool", "status") + + assert mock_run.call_args.args[0] == ["zpool", "status"] + + +def test_zpool_list_raises_when_the_command_fails(mocker: MockerFixture) -> None: + """A failed zpool must never have its error text decoded as json.""" + mocker.patch( + f"{ZPOOL}.run_zpool", + return_value=CommandResult(args=(), stdout="", stderr="no such pool: media\n", return_code=1), + ) + + with pytest.raises(RuntimeError) as excinfo: + _zpool_list("list", "media", "-pHj", "-o", "all") + + assert "Failed to run zpool" in str(excinfo.value) + assert "no such pool" in str(excinfo.value) + + +def test_zpool_builds_the_right_command(mocker: MockerFixture) -> None: + """Zpool passes argv through rather than a formatted string.""" + mock_run = mocker.patch( + f"{ZPOOL}.run_zpool", + return_value=CommandResult( + args=(), + stdout=json.dumps(SAMPLE_ZPOOL_DATA), + stderr="", + return_code=0, + ), + ) + + Zpool("testpool") + + mock_run.assert_called_once_with("list", "testpool", "-pHj", "-o", "all")