From 489448b33eda042f2b382a95fdc90c7e8e389367 Mon Sep 17 00:00:00 2001 From: "Bernhard M. Wiedemann" Date: Fri, 31 Jul 2026 16:31:37 +0200 Subject: [PATCH 1/8] Fix mount.swap state for UUID= and LABEL= device specifications fstab(5) allows a swap to be identified by a TAG=value specification instead of a device path, and swapon(8) resolves those tags. The state did not: it compared the configured name against the output of mount.swaps, which is read from /proc/swaps and therefore only ever contains device paths. The comparison could never match, so the state called mount.swapon on every run and then reported Swap UUID=... failed to activate even though the swap was active all along. Resolve TAG=value specifications to a device path with blkid before looking them up, reusing the _convert_to() helper that fstab_present already uses for the same purpose. Specifications that no block device matches are left untouched, so a genuine failure is still reported with the name the user configured. --- changelog/16084.fixed.md | 1 + salt/states/mount.py | 51 +++++++++-- tests/pytests/unit/states/test_mount.py | 110 ++++++++++++++++++++++++ 3 files changed, 154 insertions(+), 8 deletions(-) create mode 100644 changelog/16084.fixed.md diff --git a/changelog/16084.fixed.md b/changelog/16084.fixed.md new file mode 100644 index 000000000000..646d94b18f9d --- /dev/null +++ b/changelog/16084.fixed.md @@ -0,0 +1 @@ +Fixed `mount.swap` reporting "failed to activate" on every run when the swap device is given as a `UUID=`, `LABEL=`, `PARTUUID=` or `PARTLABEL=` specification diff --git a/salt/states/mount.py b/salt/states/mount.py index 6b56290ec0f1..2d84aced302f 100644 --- a/salt/states/mount.py +++ b/salt/states/mount.py @@ -844,6 +844,35 @@ def mounted( return ret +# Specifications that fstab(5) accepts in place of a device path. The +# kernel only ever reports device paths, so these have to be resolved before +# an active swap can be looked up. +_SWAP_SPEC_TAGS = ("UUID=", "LABEL=", "PARTUUID=", "PARTLABEL=") + + +def _resolve_swap_device(name): + """ + Return the device path of a swap device specification. + + ``name`` can be the path of a device node or of a swap file, a symlink to + either, or one of the ``TAG=value`` specifications accepted by fstab(5), + e.g. ``UUID=066e0200-2867-4ebe-b9e6-f30026ca2314``. + + Specifications that cannot be resolved are returned unchanged, so that + the caller can still report them the way the user spelled them. + """ + if name.upper().startswith(_SWAP_SPEC_TAGS): + return _convert_to(name, "device") or name + + if __salt__["file.is_link"](name): + real_swap_device = __salt__["file.readlink"](name) + if not real_swap_device.startswith("/"): + real_swap_device = f"/dev/{os.path.basename(real_swap_device)}" + return real_swap_device + + return name + + def swap(name, persist=True, config="/etc/fstab"): """ Activates a swap device @@ -853,18 +882,24 @@ def swap(name, persist=True, config="/etc/fstab"): /root/swapfile: mount.swap - .. note:: - ``swap`` does not currently support LABEL + The name can also be one of the ``TAG=value`` specifications accepted by + fstab(5): + + .. code-block:: yaml + + UUID=066e0200-2867-4ebe-b9e6-f30026ca2314: + mount.swap + + .. versionchanged:: 3008.3 + ``UUID=``, ``LABEL=``, ``PARTUUID=`` and ``PARTLABEL=`` names are + resolved to the underlying device before the active swaps are + checked. Previously such a state was reported as failed on every + run, even though the swap was active. """ ret = {"name": name, "changes": {}, "result": True, "comment": ""} on_ = __salt__["mount.swaps"]() - if __salt__["file.is_link"](name): - real_swap_device = __salt__["file.readlink"](name) - if not real_swap_device.startswith("/"): - real_swap_device = f"/dev/{os.path.basename(real_swap_device)}" - else: - real_swap_device = name + real_swap_device = _resolve_swap_device(name) if real_swap_device in on_: ret["comment"] = f"Swap {name} already active" diff --git a/tests/pytests/unit/states/test_mount.py b/tests/pytests/unit/states/test_mount.py index 148646274041..e95b48a32ac8 100644 --- a/tests/pytests/unit/states/test_mount.py +++ b/tests/pytests/unit/states/test_mount.py @@ -456,6 +456,116 @@ def test_swap(): assert mount.swap(name) == ret +@pytest.mark.parametrize( + "name", + [ + "UUID=066e0200-2867-4ebe-b9e6-f30026ca2314", + "uuid=066e0200-2867-4ebe-b9e6-f30026ca2314", + "LABEL=swap", + "PARTUUID=8ba0a1c0-e1a0-4dc1-b1b6-b2c1de41c6bb", + "PARTLABEL=swap", + ], +) +def test_swap_device_spec_already_active(name): + """ + A swap given as an fstab(5) TAG=value specification is resolved to its + device before the active swaps are looked up, so an already active swap + is not reported as failing to activate. + """ + device = "/dev/sdb1" + + mock_swp = MagicMock(return_value={device: {"type": "partition"}}) + mock_blkid = MagicMock(return_value={device: {"TYPE": "swap"}}) + mock_swapon = MagicMock() + + with patch.dict(mount.__grains__, {"os": "test"}), patch.dict( + mount.__opts__, {"test": False} + ), patch.dict( + mount.__salt__, + { + "mount.swaps": mock_swp, + "mount.swapon": mock_swapon, + "disk.blkid": mock_blkid, + }, + ): + ret = mount.swap(name, persist=False) + + assert ret == { + "name": name, + "changes": {}, + "result": True, + "comment": f"Swap {name} already active", + } + mock_blkid.assert_called_once_with(token=name) + mock_swapon.assert_not_called() + + +def test_swap_device_spec_activated(): + """ + An inactive swap given as a TAG=value specification is activated by + device path and reported as changed. + """ + name = "UUID=066e0200-2867-4ebe-b9e6-f30026ca2314" + device = "/dev/sdb1" + stats = {"type": "partition", "size": "1048572", "used": "0", "priority": "-2"} + + mock_swp = MagicMock(side_effect=[{}, {device: stats}]) + mock_blkid = MagicMock(return_value={device: {"TYPE": "swap"}}) + mock_swapon = MagicMock() + + with patch.dict(mount.__grains__, {"os": "test"}), patch.dict( + mount.__opts__, {"test": False} + ), patch.dict( + mount.__salt__, + { + "mount.swaps": mock_swp, + "mount.swapon": mock_swapon, + "disk.blkid": mock_blkid, + }, + ): + ret = mount.swap(name, persist=False) + + assert ret == { + "name": name, + "changes": stats, + "result": True, + "comment": f"Swap {name} activated", + } + mock_swapon.assert_called_once_with(device) + + +def test_swap_device_spec_unresolvable(): + """ + A TAG=value specification that no block device matches is left alone, so + that the failure is reported with the name the user configured. + """ + name = "UUID=066e0200-2867-4ebe-b9e6-f30026ca2314" + + mock_swp = MagicMock(return_value={}) + mock_blkid = MagicMock(return_value={}) + mock_swapon = MagicMock() + + with patch.dict(mount.__grains__, {"os": "test"}), patch.dict( + mount.__opts__, {"test": False} + ), patch.dict( + mount.__salt__, + { + "mount.swaps": mock_swp, + "mount.swapon": mock_swapon, + "disk.blkid": mock_blkid, + }, + ): + ret = mount.swap(name, persist=False) + + assert ret == { + "name": name, + "changes": {}, + "result": False, + "comment": f"Swap {name} failed to activate", + } + mock_swapon.assert_called_once_with(name) + + def test_unmounted(): """ Test to verify that a device is not mounted From 00313e070f0e4b85e03a5b986be4f7e528da94cc Mon Sep 17 00:00:00 2001 From: "Bernhard M. Wiedemann" Date: Fri, 31 Jul 2026 16:31:46 +0200 Subject: [PATCH 2/8] Compare swap devices by their canonical path in the mount.swap state /proc/swaps, and therefore mount.swaps, reports the path the kernel arrived at when the swap was activated, which need not be the name used to activate it: swapon(8) follows symlinks, so a swap activated as /dev/mapper/system-swap is reported as /dev/dm-0. That is not a corner case for the TAG=value specifications resolved in the previous commit, it is the normal result for LVM and dm-crypt setups. blkid names those devices through /dev/mapper: # blkid -t UUID=42fbd303-5ed5-4c40-b382-d1bef97e5d88 /dev/mapper/cr_home: UUID="42fbd303-..." TYPE="ext4" so the resolved device still would not have matched what the kernel reports. The same applies to a swap configured as a symlink, such as /dev/disk/by-uuid/. Resolve both the configured name and the keys of the active swaps with os.path.realpath() before comparing them. This also replaces the hand-rolled symlink resolution, which only followed a single level and assumed that a relative link target lived in /dev. While here, use the resolved device for the "is set to be added to the fstab and to be activated" test-mode check as well; it looked up the unresolved name and so never fired for a symlinked device. --- changelog/16084.fixed.md | 2 +- salt/states/mount.py | 41 +++++++++----- tests/pytests/unit/states/test_mount.py | 71 ++++++++++++++++++++++++- 3 files changed, 99 insertions(+), 15 deletions(-) diff --git a/changelog/16084.fixed.md b/changelog/16084.fixed.md index 646d94b18f9d..10486a672c43 100644 --- a/changelog/16084.fixed.md +++ b/changelog/16084.fixed.md @@ -1 +1 @@ -Fixed `mount.swap` reporting "failed to activate" on every run when the swap device is given as a `UUID=`, `LABEL=`, `PARTUUID=` or `PARTLABEL=` specification +Fixed `mount.swap` reporting "failed to activate" on every run when the swap device is given as a `UUID=`, `LABEL=`, `PARTUUID=` or `PARTLABEL=` specification, as a symlink such as `/dev/disk/by-uuid/`, or as a device-mapper name such as `/dev/mapper/` diff --git a/salt/states/mount.py b/salt/states/mount.py index 2d84aced302f..88a6a5b5a34a 100644 --- a/salt/states/mount.py +++ b/salt/states/mount.py @@ -852,7 +852,7 @@ def mounted( def _resolve_swap_device(name): """ - Return the device path of a swap device specification. + Return the canonical device path of a swap device specification. ``name`` can be the path of a device node or of a swap file, a symlink to either, or one of the ``TAG=value`` specifications accepted by fstab(5), @@ -862,15 +862,29 @@ def _resolve_swap_device(name): the caller can still report them the way the user spelled them. """ if name.upper().startswith(_SWAP_SPEC_TAGS): - return _convert_to(name, "device") or name + device = _convert_to(name, "device") + if not device: + # No block device carries that tag. Hand the specification back + # unchanged rather than turning it into a bogus path. + return name + name = device - if __salt__["file.is_link"](name): - real_swap_device = __salt__["file.readlink"](name) - if not real_swap_device.startswith("/"): - real_swap_device = f"/dev/{os.path.basename(real_swap_device)}" - return real_swap_device + return os.path.realpath(name) - return name + +def _active_swaps(): + """ + Return the active swaps, keyed by canonical device path. + + The kernel reports the path it ended up with when the swap was activated, + so a device-mapper device shows up as ``/dev/dm-0`` while blkid and the + configuration name it ``/dev/mapper/``. Resolving both sides makes + the two spellings compare equal. + """ + return { + os.path.realpath(device): stats + for device, stats in __salt__["mount.swaps"]().items() + } def swap(name, persist=True, config="/etc/fstab"): @@ -892,12 +906,13 @@ def swap(name, persist=True, config="/etc/fstab"): .. versionchanged:: 3008.3 ``UUID=``, ``LABEL=``, ``PARTUUID=`` and ``PARTLABEL=`` names are - resolved to the underlying device before the active swaps are - checked. Previously such a state was reported as failed on every + resolved to the underlying device, and both the name and the active + swaps are resolved to their canonical device path, before they are + compared. Previously such a state was reported as failed on every run, even though the swap was active. """ ret = {"name": name, "changes": {}, "result": True, "comment": ""} - on_ = __salt__["mount.swaps"]() + on_ = _active_swaps() real_swap_device = _resolve_swap_device(name) @@ -909,7 +924,7 @@ def swap(name, persist=True, config="/etc/fstab"): else: __salt__["mount.swapon"](real_swap_device) - on_ = __salt__["mount.swaps"]() + on_ = _active_swaps() if real_swap_device in on_: ret["comment"] = f"Swap {name} activated" @@ -933,7 +948,7 @@ def swap(name, persist=True, config="/etc/fstab"): fstab_data[item]["device"] for item in fstab_data ]: ret["result"] = None - if name in on_: + if real_swap_device in on_: ret["comment"] = ( "Swap {} is set to be added to the fstab and to be activated".format( name diff --git a/tests/pytests/unit/states/test_mount.py b/tests/pytests/unit/states/test_mount.py index e95b48a32ac8..8d95b6eedb64 100644 --- a/tests/pytests/unit/states/test_mount.py +++ b/tests/pytests/unit/states/test_mount.py @@ -294,7 +294,7 @@ def test_swap(): mock = MagicMock(side_effect=["present", "new", "change", "bad config"]) mock_f = MagicMock(return_value=False) - mock_swp = MagicMock(return_value=[name]) + mock_swp = MagicMock(return_value={name: {"type": "file"}}) mock_fs = MagicMock(return_value={"none": {"device": name, "fstype": "xfs"}}) mock_fs_diff = MagicMock( return_value={"none": {"device": "something_else", "fstype": "xfs"}} @@ -566,6 +566,75 @@ def test_swap_device_spec_unresolvable(): mock_swapon.assert_called_once_with(name) +def test_swap_symlinked_device_already_active(tmp_path): + """ + A swap named through a symlink, e.g. /dev/disk/by-uuid/, is matched + against the device path the kernel reports in /proc/swaps. + """ + device = tmp_path / "sdb1" + device.touch() + link = tmp_path / "by-uuid" / "066e0200-2867-4ebe-b9e6-f30026ca2314" + link.parent.mkdir() + link.symlink_to(os.path.join("..", device.name)) + + name = str(link) + mock_swp = MagicMock(return_value={str(device): {"type": "partition"}}) + mock_swapon = MagicMock() + + with patch.dict(mount.__grains__, {"os": "test"}), patch.dict( + mount.__opts__, {"test": False} + ), patch.dict( + mount.__salt__, {"mount.swaps": mock_swp, "mount.swapon": mock_swapon} + ): + ret = mount.swap(name, persist=False) + + assert ret == { + "name": name, + "changes": {}, + "result": True, + "comment": f"Swap {name} already active", + } + mock_swapon.assert_not_called() + + +def test_swap_device_mapper_already_active(tmp_path): + """ + blkid names a device-mapper device /dev/mapper/ while the kernel + reports it as /dev/dm-N in /proc/swaps. Both have to resolve to the same + device, otherwise an active swap is reported as failing to activate. + """ + dm_node = tmp_path / "dm-0" + dm_node.touch() + mapper = tmp_path / "mapper" / "system-swap" + mapper.parent.mkdir() + mapper.symlink_to(os.path.join("..", dm_node.name)) + + name = "UUID=066e0200-2867-4ebe-b9e6-f30026ca2314" + mock_swp = MagicMock(return_value={str(dm_node): {"type": "partition"}}) + mock_blkid = MagicMock(return_value={str(mapper): {"TYPE": "swap"}}) + mock_swapon = MagicMock() + + with patch.dict(mount.__grains__, {"os": "test"}), patch.dict( + mount.__opts__, {"test": False} + ), patch.dict( + mount.__salt__, + { + "mount.swaps": mock_swp, + "mount.swapon": mock_swapon, + "disk.blkid": mock_blkid, + }, + ): + ret = mount.swap(name, persist=False) + + assert ret == { + "name": name, + "changes": {}, + "result": True, + "comment": f"Swap {name} already active", + } + mock_swapon.assert_not_called() + + def test_unmounted(): """ Test to verify that a device is not mounted From 5698d0024f9dead82f02a6cd77714aedc9ad5a9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Tue, 1 Sep 2026 13:26:47 +0100 Subject: [PATCH 3/8] Add resolve_canonical arg to use canonical name for comparison Adjust code after moving functions to salt.utils.mount --- salt/modules/mount.py | 30 ++++- salt/states/mount.py | 151 +++++++++++------------ salt/utils/mount.py | 61 +++++++++ tests/pytests/unit/modules/test_mount.py | 2 +- tests/pytests/unit/states/test_mount.py | 98 +++++++++++++-- 5 files changed, 251 insertions(+), 91 deletions(-) diff --git a/salt/modules/mount.py b/salt/modules/mount.py index 080454296781..287d7414fa35 100644 --- a/salt/modules/mount.py +++ b/salt/modules/mount.py @@ -359,7 +359,7 @@ def norm_path(path): """ return os.path.normcase(os.path.normpath(path)) - def match(self, line): + def match(self, line, resolve_canonical=False): """ Compare potentially partial criteria against line """ @@ -370,6 +370,11 @@ def match(self, line): cr_opts = sorted(value.split(",")) if ex_opts != cr_opts: return False + elif key == "device" and resolve_canonical: + if salt.utils.mount._resolve_canonical( + entry[key], __salt__ + ) != salt.utils.mount._resolve_canonical(value, __salt__): + return False elif entry[key] != value: return False return True @@ -463,7 +468,7 @@ def norm_path(path): """ return os.path.normcase(os.path.normpath(path)) - def match(self, line): + def match(self, line, **kwargs): """ Compare potentially partial criteria against line """ @@ -709,12 +714,19 @@ def vfstab(config="/etc/vfstab"): return fstab(config) -def rm_fstab(name, device, config="/etc/fstab"): +def rm_fstab(name, device, config="/etc/fstab", resolve_canonical=False): """ .. versionchanged:: 2016.3.2 Remove the mount point from the fstab + resolve_canonical + ``UUID=``, ``LABEL=``, ``PARTUUID=`` and ``PARTLABEL=`` names are + resolved to the underlying device, so the canonical device path is + used for comparison. + + .. versionadded:: 3008.3 + CLI Example: .. code-block:: bash @@ -734,7 +746,7 @@ def rm_fstab(name, device, config="/etc/fstab"): for line in ifile: line = salt.utils.stringutils.to_unicode(line) try: - if criteria.match(line): + if criteria.match(line, resolve_canonical): modified = True else: lines.append(line) @@ -788,6 +800,7 @@ def set_fstab( test=False, match_on="auto", not_change=False, + resolve_canonical=False, **kwargs, ): """ @@ -797,6 +810,13 @@ def set_fstab( If the entry is found via `match_on` and `not_change` is True, the current line will be preserved. + resolve_canonical + ``UUID=``, ``LABEL=``, ``PARTUUID=`` and ``PARTLABEL=`` names are + resolved to the underlying device, so the canonical device path is + used for comparison. + + .. versionadded:: 3008.3 + CLI Example: .. code-block:: bash @@ -880,7 +900,7 @@ def filterFn(key): for line in ifile: line = salt.utils.stringutils.to_unicode(line) try: - if criteria.match(line): + if criteria.match(line, resolve_canonical): # Note: If ret isn't None here, # we've matched multiple lines ret = "present" diff --git a/salt/states/mount.py b/salt/states/mount.py index 88a6a5b5a34a..b3367a03a21c 100644 --- a/salt/states/mount.py +++ b/salt/states/mount.py @@ -39,6 +39,8 @@ import os.path import re +import salt.utils.mount + log = logging.getLogger(__name__) @@ -71,6 +73,7 @@ def mounted( extra_mount_translate_options=None, hidden_opts=None, bind_mount_copy_active_opts=True, + resolve_canonical=False, **kwargs, ): """ @@ -193,6 +196,13 @@ def mounted( copying the options from the bind mount if it was found to be active. .. versionadded:: 3006.0 + + resolve_canonical + ``UUID=``, ``LABEL=``, ``PARTUUID=`` and ``PARTLABEL=`` names are + resolved to the underlying device, so the canonical device path is + used for comparison. + + .. versionadded:: 3008.3 """ ret = {"name": name, "changes": {}, "result": True, "comment": ""} @@ -761,6 +771,7 @@ def mounted( config, test=True, match_on=match_on, + resolve_canonical=resolve_canonical, ) if out != "present": ret["result"] = None @@ -818,6 +829,7 @@ def mounted( pass_num, config, match_on=match_on, + resolve_canonical=resolve_canonical, ) if update_mount_cache: @@ -844,34 +856,6 @@ def mounted( return ret -# Specifications that fstab(5) accepts in place of a device path. The -# kernel only ever reports device paths, so these have to be resolved before -# an active swap can be looked up. -_SWAP_SPEC_TAGS = ("UUID=", "LABEL=", "PARTUUID=", "PARTLABEL=") - - -def _resolve_swap_device(name): - """ - Return the canonical device path of a swap device specification. - - ``name`` can be the path of a device node or of a swap file, a symlink to - either, or one of the ``TAG=value`` specifications accepted by fstab(5), - e.g. ``UUID=066e0200-2867-4ebe-b9e6-f30026ca2314``. - - Specifications that cannot be resolved are returned unchanged, so that - the caller can still report them the way the user spelled them. - """ - if name.upper().startswith(_SWAP_SPEC_TAGS): - device = _convert_to(name, "device") - if not device: - # No block device carries that tag. Hand the specification back - # unchanged rather than turning it into a bogus path. - return name - name = device - - return os.path.realpath(name) - - def _active_swaps(): """ Return the active swaps, keyed by canonical device path. @@ -887,10 +871,17 @@ def _active_swaps(): } -def swap(name, persist=True, config="/etc/fstab"): +def swap(name, persist=True, config="/etc/fstab", resolve_canonical=False): """ Activates a swap device + resolve_canonical + ``UUID=``, ``LABEL=``, ``PARTUUID=`` and ``PARTLABEL=`` names are + resolved to the underlying device, so the canonical device path is + used for comparison. + + .. versionadded:: 3008.3 + .. code-block:: yaml /root/swapfile: @@ -904,17 +895,12 @@ def swap(name, persist=True, config="/etc/fstab"): UUID=066e0200-2867-4ebe-b9e6-f30026ca2314: mount.swap - .. versionchanged:: 3008.3 - ``UUID=``, ``LABEL=``, ``PARTUUID=`` and ``PARTLABEL=`` names are - resolved to the underlying device, and both the name and the active - swaps are resolved to their canonical device path, before they are - compared. Previously such a state was reported as failed on every - run, even though the swap was active. + """ ret = {"name": name, "changes": {}, "result": True, "comment": ""} on_ = _active_swaps() - real_swap_device = _resolve_swap_device(name) + real_swap_device = salt.utils.mount._resolve_canonical(name, __salt__) if real_swap_device in on_: ret["comment"] = f"Swap {name} already active" @@ -972,7 +958,14 @@ def swap(name, persist=True, config="/etc/fstab"): # present, new, change, bad config # Make sure the entry is in the fstab out = __salt__["mount.set_fstab"]( - "none", name, "swap", ["defaults"], 0, 0, config + "none", + name, + "swap", + ["defaults"], + 0, + 0, + config, + resolve_canonical=resolve_canonical, ) if out == "present": return ret @@ -992,7 +985,13 @@ def swap(name, persist=True, config="/etc/fstab"): def unmounted( - name, device=None, config="/etc/fstab", persist=False, user=None, **kwargs + name, + device=None, + config="/etc/fstab", + persist=False, + user=None, + resolve_canonical=False, + **kwargs, ): """ .. versionadded:: 0.17.0 @@ -1017,6 +1016,13 @@ def unmounted( user The user to own the mount; this defaults to the user salt is running as on the minion + + resolve_canonical + ``UUID=``, ``LABEL=``, ``PARTUUID=`` and ``PARTLABEL=`` names are + resolved to the underlying device, so the canonical device path is + used for comparison. + + .. versionadded:: 3008.3 """ ret = {"name": name, "changes": {}, "result": True, "comment": ""} @@ -1097,7 +1103,9 @@ def unmounted( elif "AIX" in __grains__["os"]: out = __salt__["mount.rm_filesystems"](name, device, config) else: - out = __salt__["mount.rm_fstab"](name, device, config) + out = __salt__["mount.rm_fstab"]( + name, device, config, resolve_canonical=resolve_canonical + ) if out is not True: ret["result"] = False ret["comment"] += ". Failed to persist purge" @@ -1138,41 +1146,6 @@ def mod_watch(name, user=None, **kwargs): return ret -def _convert_to(maybe_device, convert_to): - """ - Convert a device name, UUID or LABEL to a device name, UUID or - LABEL. - - Return the fs_spec required for fstab. - - """ - - # Fast path. If we already have the information required, we can - # save one blkid call - if ( - not convert_to - or (convert_to == "device" and maybe_device.startswith("/")) - or maybe_device.startswith(f"{convert_to.upper()}=") - ): - return maybe_device - - # Get the device information - if maybe_device.startswith("/"): - blkid = __salt__["disk.blkid"](maybe_device) - else: - blkid = __salt__["disk.blkid"](token=maybe_device) - - result = None - if len(blkid) == 1: - if convert_to == "device": - result = next(iter(blkid)) - else: - key = convert_to.upper() - result = f"{key}={next(iter(blkid.values()))[key]}" - - return result - - def fstab_present( name, fs_file, @@ -1186,6 +1159,7 @@ def fstab_present( match_on="auto", not_change=False, fs_mount=True, + resolve_canonical=False, ): """Makes sure that a fstab mount point is present. @@ -1243,6 +1217,13 @@ def fstab_present( parameter is set to ``True`` and the line is found, the original content will be preserved. + resolve_canonical + ``UUID=``, ``LABEL=``, ``PARTUUID=`` and ``PARTLABEL=`` names are + resolved to the underlying device, so the canonical device path is + used for comparison. + + .. versionadded:: 3008.3 + """ ret = { "name": name, @@ -1268,7 +1249,7 @@ def fstab_present( if not fs_file == "/": fs_file = fs_file.rstrip("/") - fs_spec = _convert_to(name, mount_by) + fs_spec = salt.utils.mount._convert_to(name, mount_by, __salt__) # Validate that the device is valid after the conversion if not fs_spec: @@ -1311,6 +1292,7 @@ def fstab_present( test=True, match_on=match_on, not_change=not_change, + resolve_canonical=resolve_canonical, ) ret["result"] = None if out == "present": @@ -1363,6 +1345,7 @@ def fstab_present( config=config, match_on=match_on, not_change=not_change, + resolve_canonical=resolve_canonical, ) ret["result"] = True @@ -1393,7 +1376,9 @@ def fstab_present( return ret -def fstab_absent(name, fs_file, mount_by=None, config="/etc/fstab"): +def fstab_absent( + name, fs_file, mount_by=None, config="/etc/fstab", resolve_canonical=False +): """ Makes sure that a fstab mount point is absent. @@ -1415,6 +1400,13 @@ def fstab_absent(name, fs_file, mount_by=None, config="/etc/fstab"): config Place where the fstab file lives + resolve_canonical + ``UUID=``, ``LABEL=``, ``PARTUUID=`` and ``PARTLABEL=`` names are + resolved to the underlying device, so the canonical device path is + used for comparison. + + .. versionadded:: 3008.3 + """ ret = { "name": name, @@ -1433,7 +1425,7 @@ def fstab_absent(name, fs_file, mount_by=None, config="/etc/fstab"): if not fs_file == "/": fs_file = fs_file.rstrip("/") - fs_spec = _convert_to(name, mount_by) + fs_spec = salt.utils.mount._convert_to(name, mount_by, __salt__) if __grains__["os"] in ["MacOS", "Darwin"]: fstab_data = __salt__["mount.automaster"](config) @@ -1463,7 +1455,10 @@ def fstab_absent(name, fs_file, mount_by=None, config="/etc/fstab"): ) else: out = __salt__["mount.rm_fstab"]( - name=fs_file, device=fs_spec, config=config + name=fs_file, + device=fs_spec, + config=config, + resolve_canonical=resolve_canonical, ) if out is not True: diff --git a/salt/utils/mount.py b/salt/utils/mount.py index c09791e668e6..9997b115dd27 100644 --- a/salt/utils/mount.py +++ b/salt/utils/mount.py @@ -12,6 +12,10 @@ log = logging.getLogger(__name__) +# Specifications that fstab(5) accepts in place of a device path. +# The kernel only ever reports device paths +_SWAP_SPEC_TAGS = ("UUID=", "LABEL=", "PARTUUID=", "PARTLABEL=") + def _read_file(path): """ @@ -53,3 +57,60 @@ def write_cache(cache, opts): except OSError: log.error("Failed to cache mounts", exc_info_on_loglevel=logging.DEBUG) return False + + +def _resolve_canonical(name, salt_obj=None): + """ + Return the canonical device path of a device specification. + + ``name`` can be the path of a device node or file, a symlink to + either, or one of the ``TAG=value`` specifications accepted by fstab(5), + e.g. ``UUID=066e0200-2867-4ebe-b9e6-f30026ca2314``. + + Specifications that cannot be resolved are returned unchanged, so that + the caller can still report them the way the user spelled them. + """ + if name.upper().startswith(_SWAP_SPEC_TAGS): + device = _convert_to(name, "device", salt_obj) + if not device: + # No block device carries that tag. Hand the specification back + # unchanged rather than turning it into a bogus path. + return name + name = device + + return os.path.realpath(name) + + +def _convert_to(maybe_device, convert_to, salt_obj=None): + """ + Convert a device name, UUID or LABEL to a device name, UUID or + LABEL. + + Return the fs_spec required for fstab. + + """ + + # Fast path. If we already have the information required, we can + # save one blkid call + if ( + not convert_to + or (convert_to == "device" and maybe_device.startswith("/")) + or maybe_device.startswith(f"{convert_to.upper()}=") + ): + return maybe_device + + # Get the device information + if maybe_device.startswith("/"): + blkid = salt_obj["disk.blkid"](maybe_device) + else: + blkid = salt_obj["disk.blkid"](token=maybe_device) + + result = None + if len(blkid) == 1: + if convert_to == "device": + result = next(iter(blkid)) + else: + key = convert_to.upper() + result = f"{key}={next(iter(blkid.values()))[key]}" + + return result diff --git a/tests/pytests/unit/modules/test_mount.py b/tests/pytests/unit/modules/test_mount.py index 0415a5c8164d..5fc86b7c5880 100644 --- a/tests/pytests/unit/modules/test_mount.py +++ b/tests/pytests/unit/modules/test_mount.py @@ -1,5 +1,5 @@ """ - :codeauthor: Rupesh Tare +:codeauthor: Rupesh Tare """ import logging diff --git a/tests/pytests/unit/states/test_mount.py b/tests/pytests/unit/states/test_mount.py index 8d95b6eedb64..2c1738f8f7f5 100644 --- a/tests/pytests/unit/states/test_mount.py +++ b/tests/pytests/unit/states/test_mount.py @@ -3,6 +3,7 @@ import pytest import salt.states.mount as mount +import salt.utils.mount as mount_utils from tests.support.mock import MagicMock, patch @@ -249,6 +250,7 @@ def test_mounted(): "/etc/fstab", test=True, match_on="auto", + resolve_canonical=False, ) with patch.dict(mount.__grains__, {"os": "AIX"}): @@ -817,21 +819,23 @@ def test__convert_to_fast_none(): """ Test the device name conversor """ - assert mount._convert_to("/dev/sda1", None) == "/dev/sda1" + assert mount_utils._convert_to("/dev/sda1", None, mount.__salt__) == "/dev/sda1" def test__convert_to_fast_device(): """ Test the device name conversor """ - assert mount._convert_to("/dev/sda1", "device") == "/dev/sda1" + assert mount_utils._convert_to("/dev/sda1", "device", mount.__salt__) == "/dev/sda1" def test__convert_to_fast_token(): """ Test the device name conversor """ - assert mount._convert_to("LABEL=home", "label") == "LABEL=home" + assert ( + mount_utils._convert_to("LABEL=home", "label", mount.__salt__) == "LABEL=home" + ) def test__convert_to_device_none(): @@ -842,7 +846,7 @@ def test__convert_to_device_none(): "disk.blkid": MagicMock(return_value={}), } with patch.dict(mount.__salt__, salt_mock): - assert mount._convert_to("/dev/sda1", "uuid") is None + assert mount_utils._convert_to("/dev/sda1", "uuid", mount.__salt__) is None salt_mock["disk.blkid"].assert_called_with("/dev/sda1") @@ -856,7 +860,7 @@ def test__convert_to_device_token(): } with patch.dict(mount.__salt__, salt_mock): uuid = f"UUID={uuid}" - assert mount._convert_to("/dev/sda1", "uuid") == uuid + assert mount_utils._convert_to("/dev/sda1", "uuid", mount.__salt__) == uuid salt_mock["disk.blkid"].assert_called_with("/dev/sda1") @@ -870,7 +874,7 @@ def test__convert_to_token_device(): } with patch.dict(mount.__salt__, salt_mock): uuid = f"UUID={uuid}" - assert mount._convert_to(uuid, "device") == "/dev/sda1" + assert mount_utils._convert_to(uuid, "device", mount.__salt__) == "/dev/sda1" salt_mock["disk.blkid"].assert_called_with(token=uuid) @@ -963,6 +967,7 @@ def test_fstab_present_test_present(): test=True, match_on="auto", not_change=False, + resolve_canonical=False, ) @@ -998,6 +1003,7 @@ def test_fstab_present_test_new(): test=True, match_on="auto", not_change=False, + resolve_canonical=False, ) @@ -1030,6 +1036,7 @@ def test_fstab_present_test_change(): test=True, match_on="auto", not_change=False, + resolve_canonical=False, ) @@ -1062,6 +1069,7 @@ def test_fstab_present_test_error(): test=True, match_on="auto", not_change=False, + resolve_canonical=False, ) @@ -1151,6 +1159,42 @@ def test_fstab_present_present(): config="/etc/fstab", match_on="auto", not_change=False, + resolve_canonical=False, + ) + + +def test_fstab_present_present_resolve_canonical_true(): + """ + Test fstab_present + """ + ret = { + "name": "/dev/sda1", + "result": True, + "changes": {}, + "comment": ["/home entry was already in /etc/fstab."], + } + + grains_mock = {"os": "Linux"} + opts_mock = {"test": False} + salt_mock = {"mount.set_fstab": MagicMock(return_value="present")} + with patch.dict(mount.__grains__, grains_mock), patch.dict( + mount.__opts__, opts_mock + ), patch.dict(mount.__salt__, salt_mock): + assert ( + mount.fstab_present("/dev/sda1", "/home", "ext2", resolve_canonical=True) + == ret + ) + salt_mock["mount.set_fstab"].assert_called_with( + name="/home", + device="/dev/sda1", + fstype="ext2", + opts="defaults", + dump=0, + pass_num=0, + config="/etc/fstab", + match_on="auto", + not_change=False, + resolve_canonical=True, ) @@ -1188,6 +1232,7 @@ def test_fstab_present_new(): config="/etc/fstab", match_on="auto", not_change=False, + resolve_canonical=False, ) @@ -1219,6 +1264,7 @@ def test_fstab_present_new_no_mount(): config="/etc/fstab", match_on="auto", not_change=False, + resolve_canonical=False, ) @@ -1250,6 +1296,7 @@ def test_fstab_present_change(): config="/etc/fstab", match_on="auto", not_change=False, + resolve_canonical=False, ) @@ -1281,6 +1328,7 @@ def test_fstab_present_fail(): config="/etc/fstab", match_on="auto", not_change=False, + resolve_canonical=False, ) @@ -1445,7 +1493,40 @@ def test_fstab_absent_present(): assert mount.fstab_absent("/dev/sda1", "/home") == ret salt_mock["mount.fstab"].assert_called_with("/etc/fstab") salt_mock["mount.rm_fstab"].assert_called_with( - name="/home", device="/dev/sda1", config="/etc/fstab" + name="/home", + device="/dev/sda1", + config="/etc/fstab", + resolve_canonical=False, + ) + + +def test_fstab_absent_present_resolve_canonical_true(): + """ + Test fstab_absent + """ + ret = { + "name": "/dev/sda1", + "result": True, + "changes": {"persist": "removed"}, + "comment": ["/home entry removed from /etc/fstab."], + } + + grains_mock = {"os": "Linux"} + opts_mock = {"test": False} + salt_mock = { + "mount.fstab": MagicMock(return_value={"/home": {}}), + "mount.rm_fstab": MagicMock(return_value=True), + } + with patch.dict(mount.__grains__, grains_mock), patch.dict( + mount.__opts__, opts_mock + ), patch.dict(mount.__salt__, salt_mock): + assert mount.fstab_absent("/dev/sda1", "/home", resolve_canonical=True) == ret + salt_mock["mount.fstab"].assert_called_with("/etc/fstab") + salt_mock["mount.rm_fstab"].assert_called_with( + name="/home", + device="/dev/sda1", + config="/etc/fstab", + resolve_canonical=True, ) @@ -1579,6 +1660,7 @@ def test_bind_mount_copy_active_opts(mount_name): 0, "/etc/fstab", match_on="auto", + resolve_canonical=False, ) # bind_mount_copy_active_opts is on (default) @@ -1609,6 +1691,7 @@ def test_bind_mount_copy_active_opts(mount_name): 0, "/etc/fstab", match_on="auto", + resolve_canonical=False, ) @@ -1708,4 +1791,5 @@ def test_mount_opts_change_lazy_umount(): 0, "/etc/fstab", match_on="auto", + resolve_canonical=False, ) From ccc8c49180140201ebfb8324c00cffb5a4f553a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Tue, 1 Sep 2026 15:23:03 +0100 Subject: [PATCH 4/8] Add extra unit tests around resolve_canonical MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Yeray Gutiérrez Cedrés --- tests/pytests/unit/modules/test_mount.py | 130 +++++++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/tests/pytests/unit/modules/test_mount.py b/tests/pytests/unit/modules/test_mount.py index 5fc86b7c5880..aee6b5a90e71 100644 --- a/tests/pytests/unit/modules/test_mount.py +++ b/tests/pytests/unit/modules/test_mount.py @@ -951,3 +951,133 @@ def test_set_fstab_ceph_special_filesystem(): match_on="auto", ) assert result == "new" + + +def test_set_fstab_matches_existing_entry_resolving_canonical_from_uuid(): + """ + An fstab entry written as a device path must be recognised when the same + device is passed as UUID=, only when resolve_canonical=True is passed to + set_fstab function, so the existing line is updated instead of a duplicate + one being appended. + """ + file_data = "/dev/vdb\t\t/mnt/data\text4\tdefaults\t0 0\n" + blkid_info = { + "/dev/vdb": {"UUID": "9e7c0810-abf8-49d9-b08e-c54974add143"}, + } + + helper = mock_open(read_data=file_data) + with patch.dict( + mount.__salt__, {"disk.blkid": MagicMock(return_value=blkid_info)} + ), patch.object(os.path, "isfile", MagicMock(return_value=True)), patch( + "salt.utils.files.fopen", helper + ): + # Without canonical matching this returns "new" and appends a line + assert ( + mount.set_fstab( + "/mnt/data", + "UUID=9e7c0810-abf8-49d9-b08e-c54974add143", + "ext4", + resolve_canonical=True, + ) + == "change" + ) + + written = b"".join(line for call in helper.writelines_calls() for line in call) + assert written.count(b"/mnt/data") == 1, written + + +def test_set_fstab_not_matches_existing_entry_without_resolving_canonical_from_uuid(): + """ + By default an fstab entry written as a device path should not be recognised when the same + device is passed as UUID=, so another entry will be appended. + """ + file_data = "/dev/vdb\t\t/mnt/data\text4\tdefaults\t0 0\n" + blkid_info = { + "/dev/vdb": {"UUID": "9e7c0810-abf8-49d9-b08e-c54974add143"}, + } + + helper = mock_open(read_data=file_data) + with patch.dict( + mount.__salt__, {"disk.blkid": MagicMock(return_value=blkid_info)} + ), patch.object(os.path, "isfile", MagicMock(return_value=True)), patch( + "salt.utils.files.fopen", helper + ): + # Without canonical matching this returns "new" and appends a line + assert ( + mount.set_fstab( + "/mnt/data", + "UUID=9e7c0810-abf8-49d9-b08e-c54974add143", + "ext4", + ) + == "new" + ) + + written = b"".join(line for call in helper.writelines_calls() for line in call) + assert written.count(b"/mnt/data") == 2, written + + +def test_set_fstab_matches_existing_swap_entry_resolving_canonical_from_uuid(): + """ + Swap entries must be matched by device as well. Switching a swap device + from a path to UUID=, when resolve_canonical=True, must end up with the existing + line rewritten rather than a second line for the same device, which would + leave the system with two swap entries for one disk. + """ + file_data = "/dev/vdc\t\tnone\tswap\tdefaults\t0 0\n" + blkid_info = { + "/dev/vdc": {"UUID": "34614621-0a6b-4df5-8247-37f06b14c966"}, + } + + helper = mock_open(read_data=file_data) + with patch.dict( + mount.__salt__, {"disk.blkid": MagicMock(return_value=blkid_info)} + ), patch.object(os.path, "isfile", MagicMock(return_value=True)), patch( + "salt.utils.files.fopen", helper + ): + assert ( + mount.set_fstab( + "none", + "UUID=34614621-0a6b-4df5-8247-37f06b14c966", + "swap", + ["defaults"], + 0, + 0, + resolve_canonical=True, + ) + == "change" + ) + + written = b"".join(line for call in helper.writelines_calls() for line in call) + assert written.count(b"swap") == 1, written + + +def test_set_fstab_not_matches_existing_swap_entry_without_resolving_canonical_from_uuid(): + """ + By default swap entries should not be matched by device as well. Switching a swap device + from a path to UUID=, so it would leave the system with two swap entries for one disk. + """ + file_data = "/dev/vdc\t\tnone\tswap\tdefaults\t0 0\n" + blkid_info = { + "/dev/vdc": {"UUID": "34614621-0a6b-4df5-8247-37f06b14c966"}, + } + + helper = mock_open(read_data=file_data) + with patch.dict( + mount.__salt__, {"disk.blkid": MagicMock(return_value=blkid_info)} + ), patch.object(os.path, "isfile", MagicMock(return_value=True)), patch( + "salt.utils.files.fopen", helper + ): + assert ( + mount.set_fstab( + "none", + "UUID=34614621-0a6b-4df5-8247-37f06b14c966", + "swap", + ["defaults"], + 0, + 0, + ) + == "new" + ) + + written = b"".join(line for call in helper.writelines_calls() for line in call) + assert written.count(b"swap") == 2, written From 4dde1c8a337821df821d5c013480d8672431b55a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Tue, 1 Sep 2026 16:36:16 +0100 Subject: [PATCH 5/8] Update changelog --- changelog/70201.added.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/70201.added.md diff --git a/changelog/70201.added.md b/changelog/70201.added.md new file mode 100644 index 000000000000..857ec4b07e6b --- /dev/null +++ b/changelog/70201.added.md @@ -0,0 +1 @@ +Add ``resolve_canonical`` argument to different functions of the ``mount`` execution and states modules, to allow the matching mechanism for the existing fstab entries to always compare using the canonical names of the devices, even if there are defined as with ``UUID`` or other labels. This prevents the creation of duplicate entries in fstab that refer to the same device. From 2c32e45a22d9079e908a8c2213f8f6191ab0495f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Tue, 1 Sep 2026 16:45:43 +0100 Subject: [PATCH 6/8] Fix pylint issue --- salt/modules/mount.py | 1 + 1 file changed, 1 insertion(+) diff --git a/salt/modules/mount.py b/salt/modules/mount.py index 287d7414fa35..eff76709d448 100644 --- a/salt/modules/mount.py +++ b/salt/modules/mount.py @@ -746,6 +746,7 @@ def rm_fstab(name, device, config="/etc/fstab", resolve_canonical=False): for line in ifile: line = salt.utils.stringutils.to_unicode(line) try: + # pylint: disable-next=too-many-function-args if criteria.match(line, resolve_canonical): modified = True else: From 7702f6ebc99f22dc1ae20f4d7e67b311472cabb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Wed, 2 Sep 2026 09:24:01 +0100 Subject: [PATCH 7/8] Fix alignment to prevent pre-commit failure --- salt/modules/mount.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/salt/modules/mount.py b/salt/modules/mount.py index eff76709d448..8411c7d823d8 100644 --- a/salt/modules/mount.py +++ b/salt/modules/mount.py @@ -746,7 +746,7 @@ def rm_fstab(name, device, config="/etc/fstab", resolve_canonical=False): for line in ifile: line = salt.utils.stringutils.to_unicode(line) try: - # pylint: disable-next=too-many-function-args + # pylint: disable-next=too-many-function-args if criteria.match(line, resolve_canonical): modified = True else: From 1557dff73fb259ef03639f3652f286ce9fd14666 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Thu, 3 Sep 2026 12:20:09 +0100 Subject: [PATCH 8/8] Fix function name to be consistent with its purpose --- salt/utils/mount.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/salt/utils/mount.py b/salt/utils/mount.py index 9997b115dd27..4676decb369e 100644 --- a/salt/utils/mount.py +++ b/salt/utils/mount.py @@ -14,7 +14,7 @@ # Specifications that fstab(5) accepts in place of a device path. # The kernel only ever reports device paths -_SWAP_SPEC_TAGS = ("UUID=", "LABEL=", "PARTUUID=", "PARTLABEL=") +_FSTAB_SPEC_TAGS = ("UUID=", "LABEL=", "PARTUUID=", "PARTLABEL=") def _read_file(path): @@ -70,7 +70,7 @@ def _resolve_canonical(name, salt_obj=None): Specifications that cannot be resolved are returned unchanged, so that the caller can still report them the way the user spelled them. """ - if name.upper().startswith(_SWAP_SPEC_TAGS): + if name.upper().startswith(_FSTAB_SPEC_TAGS): device = _convert_to(name, "device", salt_obj) if not device: # No block device carries that tag. Hand the specification back