From 8ec2edda399c3721d6c3315ac3940315cb4c96e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mina=20Gali=C4=87?= Date: Thu, 11 Apr 2024 23:33:24 +0100 Subject: [PATCH] growpart: Fix behaviour for ZFS datasets (#5169) on FreeBSD, ZFS datasets aren't exposed directly as disks devices, so stats will fail. Currently, this means that growpart will be skipped for FreeBSD ZFS volumes, even tho we are perfectly capable of resizing ZFS. - Return fs from devent2dev(), which is now inappropriately named - Add get_zfs_size() helper, and extend get_size() to accept an `fs` parameter. - Add _call_resizer() helper, to avoid an indent on `if fs == zfs` - Add a unittest to show the behaviour Sponsored by: The FreeBSD Foundation --- cloudinit/config/cc_growpart.py | 146 +++++++++++++-------- tests/unittests/config/test_cc_growpart.py | 81 ++++++++++-- 2 files changed, 161 insertions(+), 66 deletions(-) diff --git a/cloudinit/config/cc_growpart.py b/cloudinit/config/cc_growpart.py index 65bf65db9c3..3fb19a92e3d 100644 --- a/cloudinit/config/cc_growpart.py +++ b/cloudinit/config/cc_growpart.py @@ -161,7 +161,7 @@ def available(self, devices: list) -> bool: ... @abstractmethod - def resize(self, diskdev, partnum, partdev): + def resize(self, diskdev, partnum, partdev, fs): ... @@ -178,8 +178,8 @@ def available(self, devices: list): pass return False - def resize(self, diskdev, partnum, partdev): - before = get_size(partdev) + def resize(self, diskdev, partnum, partdev, fs): + before = get_size(partdev, fs) # growpart uses tmp dir to store intermediate states # and may conflict with systemd-tmpfiles-clean @@ -211,7 +211,7 @@ def resize(self, diskdev, partnum, partdev): util.logexc(LOG, "Failed: growpart %s %s", diskdev, partnum) raise ResizeFailedException(e) from e - return (before, get_size(partdev)) + return (before, get_size(partdev, fs)) class ResizeGrowFS(Resizer): @@ -231,15 +231,15 @@ def available(self, devices: list): """growfs only works on the root partition""" return os.path.isfile("/etc/rc.d/growfs") and devices == ["/"] - def resize(self, diskdev, partnum, partdev): - before = get_size(partdev) + def resize(self, diskdev, partnum, partdev, fs): + before = get_size(partdev, fs) try: self._distro.manage_service(action="onestart", service="growfs") except subp.ProcessExecutionError as e: util.logexc(LOG, "Failed: service growfs onestart") raise ResizeFailedException(e) from e - return (before, get_size(partdev)) + return (before, get_size(partdev, fs)) class ResizeGpart(Resizer): @@ -255,7 +255,7 @@ def available(self, devices: list): pass return False - def resize(self, diskdev, partnum, partdev): + def resize(self, diskdev, partnum, partdev, fs): """ GPT disks store metadata at the beginning (primary) and at the end (secondary) of the disk. When launching an image with a @@ -270,36 +270,49 @@ def resize(self, diskdev, partnum, partdev): util.logexc(LOG, "Failed: gpart recover %s", diskdev) raise ResizeFailedException(e) from e - before = get_size(partdev) + before = get_size(partdev, fs) try: subp.subp(["gpart", "resize", "-i", partnum, diskdev]) except subp.ProcessExecutionError as e: util.logexc(LOG, "Failed: gpart resize -i %s %s", partnum, diskdev) raise ResizeFailedException(e) from e - return (before, get_size(partdev)) + return (before, get_size(partdev, fs)) -def get_size(filename) -> Optional[int]: +def get_size(filename, fs) -> Optional[int]: fd = None try: fd = os.open(filename, os.O_RDONLY) return os.lseek(fd, 0, os.SEEK_END) except FileNotFoundError: + if fs == "zfs": + return get_zfs_size(filename) return None finally: if fd: os.close(fd) +def get_zfs_size(dataset) -> Optional[int]: + zpool = dataset.split("/")[0] + try: + size, _ = subp.subp(["zpool", "get", "-Hpovalue", "size", zpool]) + except subp.ProcessExecutionError as e: + LOG.debug("Failed: zpool get size %s: %s", zpool, e) + return None + return int(size.strip()) + + def devent2dev(devent): if devent.startswith("/dev/"): - return devent - else: - result = util.get_mount_info(devent) - if not result: - raise ValueError("Could not determine device of '%s' % dev_ent") - dev = result[0] + return devent, None + + result = util.get_mount_info(devent) + if not result: + raise ValueError("Could not determine device of '%s' % dev_ent") + dev = result[0] + fs = result[1] container = util.is_container() @@ -310,9 +323,9 @@ def devent2dev(devent): if os.path.exists(dev): # if /dev/root exists, but we failed to convert # that to a "real" /dev/ path device, then return it. - return dev + return dev, None raise ValueError("Unable to find device '/dev/root'") - return dev + return dev, fs def is_encrypted(blockdev, partition) -> bool: @@ -414,6 +427,52 @@ def resize_encrypted(blockdev, partition) -> Tuple[str, str]: ) +def _call_resizer(resizer, devent, disk, ptnum, blockdev, fs): + info = [] + try: + old, new = resizer.resize(disk, ptnum, blockdev, fs) + if old == new: + info.append( + ( + devent, + RESIZE.NOCHANGE, + "no change necessary (%s, %s)" % (disk, ptnum), + ) + ) + elif new is None or old is None: + msg = "" + if disk is not None and ptnum is None: + msg = "changed (%s, %s) size, new size is unknown" % ( + disk, + ptnum, + ) + else: + msg = "changed (%s) size, new size is unknown" % blockdev + info.append((devent, RESIZE.CHANGED, msg)) + else: + msg = "" + if disk is not None and ptnum is None: + msg = "changed (%s, %s) from %s to %s" % ( + disk, + ptnum, + old, + new, + ) + else: + msg = "changed (%s) from %s to %s" % (blockdev, old, new) + info.append((devent, RESIZE.CHANGED, msg)) + + except ResizeFailedException as e: + info.append( + ( + devent, + RESIZE.FAILED, + "failed to resize: disk=%s, ptnum=%s: %s" % (disk, ptnum, e), + ) + ) + return info + + def resize_devices(resizer, devices, distro: Distro): # returns a tuple of tuples containing (entry-in-devices, action, message) devices = copy.copy(devices) @@ -421,8 +480,11 @@ def resize_devices(resizer, devices, distro: Distro): while devices: devent = devices.pop(0) + disk = None + ptnum = None + try: - blockdev = devent2dev(devent) + blockdev, fs = devent2dev(devent) except ValueError as e: info.append( ( @@ -433,6 +495,11 @@ def resize_devices(resizer, devices, distro: Distro): ) continue + LOG.debug("growpart found fs=%s", fs) + if fs == "zfs": + info += _call_resizer(resizer, devent, disk, ptnum, blockdev, fs) + return info + try: statret = os.stat(blockdev) except OSError as e: @@ -512,44 +579,7 @@ def resize_devices(resizer, devices, distro: Distro): ) continue - try: - old, new = resizer.resize(disk, ptnum, blockdev) - if old == new: - info.append( - ( - devent, - RESIZE.NOCHANGE, - "no change necessary (%s, %s)" % (disk, ptnum), - ) - ) - elif new is None or old is None: - info.append( - ( - devent, - RESIZE.CHANGED, - "changed (%s, %s) size, new size is unknown" - % (disk, ptnum), - ) - ) - else: - info.append( - ( - devent, - RESIZE.CHANGED, - "changed (%s, %s) from %s to %s" - % (disk, ptnum, old, new), - ) - ) - - except ResizeFailedException as e: - info.append( - ( - devent, - RESIZE.FAILED, - "failed to resize: disk=%s, ptnum=%s: %s" - % (disk, ptnum, e), - ) - ) + info += _call_resizer(resizer, devent, disk, ptnum, blockdev, fs) return info diff --git a/tests/unittests/config/test_cc_growpart.py b/tests/unittests/config/test_cc_growpart.py index 490f4cc1984..1ba051c3483 100644 --- a/tests/unittests/config/test_cc_growpart.py +++ b/tests/unittests/config/test_cc_growpart.py @@ -14,7 +14,7 @@ import pytest -from cloudinit import cloud, subp, temp_utils +from cloudinit import cloud, distros, subp, temp_utils from cloudinit.config import cc_growpart from cloudinit.config.schema import ( SchemaValidationError, @@ -215,7 +215,7 @@ def test_force_lang_check_tempfile(self, *args, **kwargs): diskdev = "/dev/sdb" partnum = 1 partdev = "/dev/sdb" - ret.resize(diskdev, partnum, partdev) + ret.resize(diskdev, partnum, partdev, None) mockobj.assert_has_calls( [ mock.call( @@ -351,8 +351,8 @@ def test_simple_devices(self): resize_calls = [] class myresizer: - def resize(self, diskdev, partnum, partdev): - resize_calls.append((diskdev, partnum, partdev)) + def resize(self, diskdev, partnum, partdev, fs): + resize_calls.append((diskdev, partnum, partdev, fs)) if partdev == "/dev/YYda2": return (1024, 2048) return (1024, 1024) # old size, new size @@ -395,7 +395,70 @@ def find(name, res): os.stat = real_stat +class TestResizeZFS: + def _devent2dev_side_effect(self, value): + if value.startswith("zroot"): + return value, "zfs" + raise RuntimeError(f"unexpected value {value}") + + def _subp_side_effect(self, value, **kwargs): + if value[0] == "growpart": + raise subp.ProcessExecutionError() + elif value[0] == "zpool": + return ("1024\n", "") + raise subp.ProcessExecutionError() + + @pytest.fixture + def common_mocks(self, mocker): + # These are all "happy path" mocks which will get overridden + # when needed + mocker.patch( + "cloudinit.config.cc_growpart.devent2dev", + side_effect=self._devent2dev_side_effect, + ) + mocker.patch("cloudinit.util.is_container", return_value=False) + # Find /etc/rc.d/growfs + mocker.patch("os.path.isfile", return_value=True) + mocker.patch( + "cloudinit.config.cc_growpart.subp.subp", + side_effect=self._subp_side_effect, + ) + cls = distros.fetch("freebsd") + # patch ifconfig -a + mocker.patch( + "cloudinit.distros.networking.subp.subp", return_value=("", None) + ) + self.distro = cls("freebsd", {}, None) + + @pytest.mark.parametrize( + "dev, expected", + [ + ("zroot/ROOT/changed", cc_growpart.RESIZE.CHANGED), + ("zroot/ROOT/nochange", cc_growpart.RESIZE.NOCHANGE), + ], + ) + def test_zroot(self, dev, expected, common_mocks): + resize_calls = [] + + class myresizer: + def resize(self, diskdev, partnum, partdev, fs): + resize_calls.append((diskdev, partnum, partdev, fs)) + if partdev == "zroot/ROOT/changed": + return (1024, 2048) + return (1024, 1024) # old size, new size + + def find(name, res): + for f in res: + if f[0] == name: + return f + return None + + resized = cc_growpart.resize_devices(myresizer(), [dev], self.distro) + assert expected == find(dev, resized)[1] + + class TestGetSize: + # TODO: add tests for get_zfs_size() @pytest.mark.parametrize( "file_exists, expected", ( @@ -408,7 +471,7 @@ def test_get_size_behaves(self, file_exists, expected, tmp_path): tmp_file = tmp_path / "tmp.txt" if file_exists: tmp_file.write_bytes(b"0") - assert expected == cc_growpart.get_size(tmp_file) + assert expected == cc_growpart.get_size(tmp_file, None) class TestEncrypted: @@ -437,11 +500,13 @@ def _device_part_info_side_effect(self, value): def _devent2dev_side_effect(self, value): if value == "/fake_encrypted": - return "/dev/mapper/fake" + return "/dev/mapper/fake", "ext3" elif value == "/": - return "/dev/vdz" + return "/dev/vdz", "ext4" + elif value.startswith("zroot"): + return value, "zfs" elif value.startswith("/dev"): - return value + return value, None raise RuntimeError(f"unexpected value {value}") def _realpath_side_effect(self, value):