From e6995a2c1dae33c66bd5f8c7b3c89e082f64ab21 Mon Sep 17 00:00:00 2001 From: Philip Guyton Date: Thu, 20 Jun 2024 20:50:57 +0100 Subject: [PATCH] 5.0.9-0 & 5.0.10-0 lsblk whitespace only values - not enough values to unpack #2853 VM instance report had all whitespace VENDOR value returned by lsblk. Add re.sub() pre-processor to each lsblk line prior to main parsing. Includes - Remarked out (noisy) debug line to easy future investigations. - Additional unit-test reproducer to prove fix and guard against regression. --- src/rockstor/system/osi.py | 4 +- src/rockstor/system/tests/test_osi.py | 84 +++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/src/rockstor/system/osi.py b/src/rockstor/system/osi.py index 39d66d93c..e8c416c64 100644 --- a/src/rockstor/system/osi.py +++ b/src/rockstor/system/osi.py @@ -327,6 +327,8 @@ def scan_disks(min_size: int, test_mode: bool = False) -> list[Disk]: # lsblk example line (incomplete): 'NAME="/dev/sdb" MODEL="TOSHIBA MK1652GS" VENDOR="ATA " LABEL="" UUID=""' # line.strip().split('" ') = ['NAME="/dev/sdb', 'MODEL="TOSHIBA MK1652GS', 'VENDOR="ATA ', 'LABEL="', 'UUID=""'] # noqa E501 # Device information built from each lsblk line in turn. + clean_line = re.sub('"\s+"', '""', line).strip() + # logger.debug(f"Scan_disks() using lsblk clean_line={clean_line}") blk_dev_properties: dict = { key.lower() if key != "TRAN" @@ -334,7 +336,7 @@ def scan_disks(min_size: int, test_mode: bool = False) -> list[Disk]: if value.replace('"', "").strip() != "" else None for key, value in ( - key_value.split("=") for key_value in line.strip().split('" ') + key_value.split("=") for key_value in clean_line.split('" ') ) } logger.debug(f"Scan_disks() using: blk_dev_properties={blk_dev_properties}") diff --git a/src/rockstor/system/tests/test_osi.py b/src/rockstor/system/tests/test_osi.py index 0f5c0713f..9bfbde709 100644 --- a/src/rockstor/system/tests/test_osi.py +++ b/src/rockstor/system/tests/test_osi.py @@ -1638,6 +1638,90 @@ def test_scan_disks_27_plus_disks_regression_issue(self): "expected = ({}).".format(returned, expected), ) + def test_scan_disks_lsblk_parse_fail(self): + """ + Testing releases 5.0.9-0 & 5.0.10-0 failed lsblk output parsing when values began with a space. + This test can collect other such parsing failures to avoid similar regressions of the dict comp + approach to extracting disk properties from lsblks output. + TO RUN: + cd /opt/rockstor/src/rockstor/system/tests + /opt/rockstor/.venv/bin/python -m unittest test_osi.OSITests.test_scan_disks_lsblk_parse_fail + """ + # E.g 'VENDOR=" "' failed with: "ValueError: not enough values to unpack (expected 2, got 1)" + # We use dictionary comprehension to parse lsblk output and rely on: 'line.strip().split('" ')' + # this breaks when a disk property begins with a space. + # See: https://github.com/rockstor/rockstor-core/issues/2853 + # Reported/encountered/addressed failure concerns values consisting solely of whitespace. + out = [ + [ + 'NAME="/dev/fd0" MODEL="" SERIAL="" SIZE="4K" TRAN="" VENDOR="" HCTL="" TYPE="disk" FSTYPE="" LABEL="" UUID=""', # noqa E501 + 'NAME="/dev/sda" MODEL="ST18000NM000J-2T" SERIAL="ZR54RJQ2" SIZE="16.4T" TRAN="" VENDOR=" " HCTL="1:0:0:0" TYPE="disk" FSTYPE="btrfs" LABEL="Pool" UUID="f15ef88e-c734-49a9-9f5a-3bfaf40656d0"', # noqa E501 + 'NAME="/dev/sdb" MODEL="Virtual Disk" SERIAL="6002248008e48f45909f67474b658d2a" SIZE="32G" TRAN="" VENDOR="Msft " HCTL="0:0:0:0" TYPE="disk" FSTYPE="" LABEL="" UUID=""', # noqa E501 + 'NAME="/dev/sdb1" MODEL="" SERIAL="" SIZE="2M" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="" LABEL="" UUID=""', # noqa E501 + 'NAME="/dev/sdb2" MODEL="" SERIAL="" SIZE="64M" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="vfat" LABEL="EFI" UUID="1381-2428"', # noqa E501 + 'NAME="/dev/sdb3" MODEL="" SERIAL="" SIZE="2G" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="swap" LABEL="SWAP" UUID="ee6de8d5-eb1c-4d19-943c-0d24ecee92a7"', # noqa E501 + 'NAME="/dev/sdb4" MODEL="" SERIAL="" SIZE="29.9G" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="btrfs" LABEL="ROOT" UUID="46befa9e-b399-480d-b10d-338a4c875615"', # noqa E501 + "", + ] + ] + err = [[""]] + rc = [0] + expected_result = [ + [ + Disk( + name="/dev/sda", + model="ST18000NM000J-2T", + serial="ZR54RJQ2", + size=17609365913, + transport=None, + vendor=None, + hctl="1:0:0:0", + type="disk", + fstype="btrfs", + label="Pool", + uuid="f15ef88e-c734-49a9-9f5a-3bfaf40656d0", + parted=False, + root=True, + partitions={}, + ), + Disk( + name="/dev/sdb", + model="Virtual Disk", + serial="6002248008e48f45909f67474b658d2a", + size=31352422, + transport=None, + vendor="Msft", + hctl="0:0:0:0", + type="disk", + fstype="btrfs", + label="ROOT", + uuid="46befa9e-b399-480d-b10d-338a4c875615", + parted=True, + root=False, + partitions={"/dev/sdb4": "btrfs"}, + ), + ] + ] + # As all serials are available via the lsblk we can avoid mocking + # get_device_serial() + # And given no bcache we can also avoid mocking + # get_bcache_device_type() + # Iterate the test data sets for run_command running lsblk. + for o, e, r, expected in zip(out, err, rc, expected_result): + self.mock_run_command.return_value = (o, e, r) + # itemgetter(0) referenced the first item within our Disk + # collection by which to sort (key) ie name. N.B. 'name' failed. + expected.sort(key=operator.itemgetter(0)) + returned = scan_disks(5242880, test_mode=True) + returned.sort(key=operator.itemgetter(0)) + self.assertListEqual( + returned, + expected, + msg="Regression in parsing lsblk output:\n " + "returned = ({}).\n " + "expected = ({}).".format(returned, expected), + ) + def test_scan_disks_luks_sys_disk(self): """ Test to ensure scan_disks() correctly identifies the CentOS, default