From 5398c7a7ac68c0de7a9bd22eac219529f0c67b26 Mon Sep 17 00:00:00 2001 From: Damien Thenot Date: Tue, 26 Nov 2024 16:45:24 +0100 Subject: [PATCH] fix(cleanup.py): bad live coalesce check regarding FileSR The `VDI.canLiveCoalesce` method can manipulates sizes of different units because of this change: ``` CP-40871: use VHD allocation size in checking canLiveCoalesce 2f863b9fce6f2978499892d8c019bb3ab7ad72c5 ``` As a result, the `canLiveCoalesce` method can return `True` and cause coalesce attempts resulting in "Timed out" exceptions. * Only drivers deriving from `FileSR` are impacted. * The size of `self._sizeAllocated` is calculated correctly when `vhdutil.getAllocatedSize` is called but there is a problematic case where `getVHDInfo` is used instead. And this function does not convert `info.sizeAllocated` from block size to bytes. * Additionally, the getter `getAllocatedSize` should retrieve the allocated size if it's equal to -1. Otherwise `VDI.canLiveCoalesce` can compare -1 to a positive value and would always return `True` causing a `Timed out` error error... Signed-off-by: Damien Thenot Co-authored-by: Ronan Abhamon --- drivers/cleanup.py | 5 +++++ drivers/vhdutil.py | 10 +++++++--- tests/test_cleanup.py | 2 ++ tests/test_vhdutil.py | 40 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 54 insertions(+), 3 deletions(-) diff --git a/drivers/cleanup.py b/drivers/cleanup.py index fb253e105..76fcb8d1a 100755 --- a/drivers/cleanup.py +++ b/drivers/cleanup.py @@ -1177,6 +1177,11 @@ def delete(self): raise util.SMException("os.unlink(%s) failed" % self.path) VDI.delete(self) + def getAllocatedSize(self): + if self._sizeAllocated == -1: + self._sizeAllocated = vhdutil.getAllocatedSize(self.path) + return self._sizeAllocated + class LVHDVDI(VDI): """Object representing a VDI in an LVHD SR""" diff --git a/drivers/vhdutil.py b/drivers/vhdutil.py index 40807e840..c4be0eef6 100755 --- a/drivers/vhdutil.py +++ b/drivers/vhdutil.py @@ -94,6 +94,11 @@ def ioretry(cmd, text=True): errlist=[errno.EIO, errno.EAGAIN]) +def convertAllocatedSizeToBytes(size): + # Assume we have standard 2MB allocation blocks + return size * 2 * 1024 * 1024 + + def getVHDInfo(path, extractUuidFunction, includeParent=True): """Get the VHD info. The parent info may optionally be omitted: vhd-util tries to verify the parent by opening it, which results in error if the VHD @@ -115,7 +120,7 @@ def getVHDInfo(path, extractUuidFunction, includeParent=True): vhdInfo.parentUuid = extractUuidFunction(fields[nextIndex]) nextIndex += 1 vhdInfo.hidden = int(fields[nextIndex].replace("hidden: ", "")) - vhdInfo.sizeAllocated = int(fields[nextIndex+1]) + vhdInfo.sizeAllocated = convertAllocatedSizeToBytes(int(fields[nextIndex+1])) vhdInfo.path = path return vhdInfo @@ -274,8 +279,7 @@ def setSizePhys(path, size, debug=True): def getAllocatedSize(path): cmd = [VHD_UTIL, "query", OPT_LOG_ERR, '-a', '-n', path] ret = ioretry(cmd) - # Assume we have standard 2MB allocation blocks - return int(ret) * 2 * 1024 * 1024 + return convertAllocatedSizeToBytes(int(ret)) def killData(path): "zero out the disk (kill all data inside the VHD file)" diff --git a/tests/test_cleanup.py b/tests/test_cleanup.py index 99ea6ed5f..a2b14d602 100644 --- a/tests/test_cleanup.py +++ b/tests/test_cleanup.py @@ -1461,6 +1461,8 @@ def add_vdis_for_coalesce(self, sr): vdi = cleanup.FileVDI(sr, vdi_uuid, False) vdi.path = '%s.vhd' % (vdi_uuid) vdi.parent = parent + # Set an initial value to make Mock happy. + vdi._sizeAllocated = 20971520 # 10 blocks of 2MB changed in the child. parent.children.append(vdi) sr.vdis[vdi_uuid] = vdi diff --git a/tests/test_vhdutil.py b/tests/test_vhdutil.py index 331bafd42..6aa91e15b 100644 --- a/tests/test_vhdutil.py +++ b/tests/test_vhdutil.py @@ -362,3 +362,43 @@ def test_function(args, inp): # Act/Assert self.assertEqual(25, vhdutil.coalesce(TEST_VHD_PATH)) + + @testlib.with_context + def test_get_vhd_info_allocated_size(self, context): + """ + Test that vhdutil.getVHDInfo return the allocated size in byte + """ + # Arrange + def test_function(args, inp): + return 0, "51200\n39621239296\nd90f890c-d173-4eaf-ba09-fc2d6e50f6c0.vhd has no parent\nhidden: 0\n18856", "" + + context.add_executable(VHD_UTIL, test_function) + import FileSR + vhdinfo = vhdutil.getVHDInfo(TEST_VHD_PATH, FileSR.FileVDI.extractUuid) + + # Act/Assert + self.assertEqual(18856*2*1024*1024 , vhdinfo.sizeAllocated) + + @testlib.with_context + def test_get_allocated_size(self, context): + """ + Test that vhdutil.getAllocatedSize return the size in byte + """ + # Arrange + call_args = None + def test_function(args, inp): + nonlocal call_args + call_args = args + return 0, b"18856", b"" + + context.add_executable(VHD_UTIL, test_function) + + # Act + result = vhdutil.getAllocatedSize(TEST_VHD_NAME) + + # Assert + self.assertEqual(18856*2*1024*1024, result) + self.assertEqual( + [VHD_UTIL, "query", "--debug", "-a", + "-n", TEST_VHD_NAME], + call_args)