Skip to content

Commit

Permalink
5.0.9-0 & 5.0.10-0 lsblk whitespace only values - not enough values t…
Browse files Browse the repository at this point in the history
…o 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.
  • Loading branch information
phillxnet committed Jun 20, 2024
1 parent 3f8d02b commit e6995a2
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 1 deletion.
4 changes: 3 additions & 1 deletion src/rockstor/system/osi.py
Original file line number Diff line number Diff line change
Expand Up @@ -327,14 +327,16 @@ 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"
else "transport": value.replace('"', "").strip()
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}")
Expand Down
84 changes: 84 additions & 0 deletions src/rockstor/system/tests/test_osi.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit e6995a2

Please sign in to comment.