From 28b35c699ba0a8e5e02640fd7d66b6d84f536bc0 Mon Sep 17 00:00:00 2001 From: Philip Guyton Date: Tue, 14 Aug 2018 18:22:58 +0100 Subject: [PATCH] regression - nvme system disk not identified #1951 Address nvme system disk regression, not recognised as attached, by improving the partition regex mechanism initially enhanced in commit acbd6a5 (pr #1946) which created the indicated regression. Summary: - Unit test added to instantiate observed nvme system disk regression and expected behaviour. - Simplify regex match conditional, for root partition identification, so that it again incorporates nvme device names: with root in partition. - Establish conditional partition regex prior to above match. - Enhance existing btrfs-in-partition unit test to catch a regression observed prior to establishing the above conditional regex that affected the 42nd scsi type drive 'sdap' if that drive had a btrfs partition concurrent with an 'sda' root drive name (and the prior regex's were 'or' combined). - Add debug logging of sys partition info inheritance from base device. - Add debug logging of btrfs in partition device skip: we only surface the base device. - Minor cosmetic bracket changes and comment updates. - Added TODO: on future option re base device identification from partition device name. --- src/rockstor/system/osi.py | 23 +++-- src/rockstor/system/tests/test_osi.py | 134 +++++++++++++++++++++++++- 2 files changed, 145 insertions(+), 12 deletions(-) diff --git a/src/rockstor/system/osi.py b/src/rockstor/system/osi.py index db2ba8f67..42693d3b4 100644 --- a/src/rockstor/system/osi.py +++ b/src/rockstor/system/osi.py @@ -365,7 +365,7 @@ def scan_disks(min_size, test_mode=False): # This dict will be populated when we find our partitions and back # port their names and fstype (as values). if ((not is_root_disk and not is_partition) or - (is_btrfs)): + is_btrfs): # We have a non system disk that is not a partition # or # We have a device that is btrfs formatted @@ -375,12 +375,15 @@ def scan_disks(min_size, test_mode=False): # a btrfs file system # Regex to identify a partition on the base_root_disk. # Root on 'sda3' gives base_root_disk 'sda'. - # Match partitions of eg 'sda' with >= one additional digit. - part_regex = base_root_disk + '\d+' - bios_md_part = base_root_disk + 'p\d+' # ie md126p3 - if (re.match(part_regex, dmap['NAME']) is not None) or \ - (dmap['TYPE'] == 'md' and - re.match(bios_md_part, dmap['NAME']) is not None): + if re.match('sd|vd', dmap['NAME']) is not None: + # eg 'sda' or 'vda' with >= one additional digit, + part_regex = base_root_disk + '\d+' + else: + # md126 or nvme0n1 with 'p' + >= one additional digit eg: + # md126p3 or nvme0n1p4; also mmcblk0p2 for base mmcblk0. + part_regex = base_root_disk + 'p\d+' + if re.match(part_regex, dmap['NAME']) is not None: + logger.debug('--- Inheriting base_root_disk info ---') # We are assuming that a partition with a btrfs fs on is # our root if it's name begins with our base system disk # name. Now add the properties we stashed when looking at @@ -413,9 +416,9 @@ def scan_disks(min_size, test_mode=False): # NOT on the system disk. # Most likely a current btrfs data drive or one we could # import. - # As we don't understand / support btrfs in partitions - # then ignore / skip this btrfs device if it's a partition + # Ignore / skip this btrfs device if it's a partition if is_partition: + logger.debug('-- Skipping non root btrfs partition -') continue # No more continues so the device we have is to be passed to our db # entry system views/disk.py ie _update_disk_state() @@ -901,6 +904,8 @@ def root_disk(): single character. :return: sdX type device name (without path) where root is mounted. """ + # TODO: Consider 'lsblk -no pkname devname' rather than parse and strip. + # -n = no headings, -o specify output (pkname = Parent Kernel Name) with open('/proc/mounts') as fo: for line in fo.readlines(): fields = line.split() diff --git a/src/rockstor/system/tests/test_osi.py b/src/rockstor/system/tests/test_osi.py index 5f7baf244..449dc1777 100644 --- a/src/rockstor/system/tests/test_osi.py +++ b/src/rockstor/system/tests/test_osi.py @@ -1002,6 +1002,8 @@ def test_scan_disks_btrfs_in_partition(self): data disk (for btrfs in partition) virtio with serial "serial-1" prepared as follows with regard to partition / formatting: + First data set: + yum install dosfstools parted -a optimal /dev/disk/by-id/virtio-serial-1 mklabel msdos @@ -1011,8 +1013,22 @@ def test_scan_disks_btrfs_in_partition(self): mkfs.fat -s2 -F 32 /dev/disk/by-id/virtio-serial-1-part1 mkfs.btrfs -L btrfs-in-partition /dev/disk/by-id/virtio-serial-1-part2 - Should result in appropriate "partitions=" entry for base device of - /dev/disk/by-id/virtio-serial-1 (/dev/vda in below test data). + Second data set (42nd scsi drive with a btrfs partiiton plus sda root): + + parted -a optimal /dev/sdap + mklabel msdos + mkpart primary fat32 1 50% + mkpart primary ext2 50% 100% + quit + mkfs.fat -s2 -F 32 /dev/sdap1 + mkfs.btrfs -L btrfs-in-partition /dev/sdap2 + + Should result in appropriate "partitions=" entry for base devices of + /dev/disk/by-id/virtio-serial-1 (/dev/vda in below test data) and + /dev/sdap + + The second data set is to check for a regression re false positive when + root on sda, ie 'Regex to identify a partition on the base_root_disk.' """ out = [[ 'NAME="sr0" MODEL="QEMU DVD-ROM " SERIAL="QM00001" SIZE="1024M" TRAN="ata" VENDOR="QEMU " HCTL="0:0:0:0" TYPE="rom" FSTYPE="" LABEL="" UUID=""', # noqa E501 @@ -1023,9 +1039,22 @@ def test_scan_disks_btrfs_in_partition(self): 'NAME="vda" MODEL="" SERIAL="" SIZE="8G" TRAN="" VENDOR="0x1af4" HCTL="" TYPE="disk" FSTYPE="" LABEL="" UUID=""', # noqa E501 'NAME="vda2" MODEL="" SERIAL="" SIZE="4G" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="btrfs" LABEL="btrfs-in-partition" UUID="55284332-af66-4ca0-9647-99d9afbe0ec5"', # noqa E501 'NAME="vda1" MODEL="" SERIAL="" SIZE="4G" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="vfat" LABEL="" UUID="8F05-D915"', # noqa E501 - '']] + ''], [ + 'NAME="sr0" MODEL="QEMU DVD-ROM " SERIAL="QM00001" SIZE="1024M" TRAN="ata" VENDOR="QEMU " HCTL="0:0:0:0" TYPE="rom" FSTYPE="" LABEL="" UUID=""', # noqa E501 + 'NAME="sda" MODEL="QEMU HARDDISK " SERIAL="QM00005" SIZE="8G" TRAN="sata" VENDOR="ATA " HCTL="2:0:0:0" TYPE="disk" FSTYPE="" LABEL="" UUID=""', # noqa E501 + 'NAME="sda2" MODEL="" SERIAL="" SIZE="820M" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="swap" LABEL="" UUID="aaf61037-23b1-4c3b-81ca-6d07f3ed922d"', # noqa E501 + 'NAME="sda3" MODEL="" SERIAL="" SIZE="6.7G" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="btrfs" LABEL="rockstor_rockstor" UUID="355f53a4-24e1-465e-95f3-7c422898f542"', # noqa E501 + 'NAME="sda1" MODEL="" SERIAL="" SIZE="500M" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="ext4" LABEL="" UUID="04ce9f16-a0a0-4db8-8719-1083a0d4f381"', # noqa E501 + 'NAME="sdap" MODEL="QEMU HARDDISK " SERIAL="42nd-scsi" SIZE="8G" TRAN="sata" VENDOR="ATA " HCTL="3:0:0:0" TYPE="disk" FSTYPE="" LABEL="" UUID=""', # noqa E501 + 'NAME="sdap2" MODEL="" SERIAL="" SIZE="4G" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="btrfs" LABEL="btrfs-in-partition" UUID="55284332-af66-4ca0-9647-99d9afbe0ec5"', # noqa E501 + 'NAME="sdap1" MODEL="" SERIAL="" SIZE="4G" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="vfat" LABEL="" UUID="8F05-D915"', # noqa E501 + '' + ]] err = [['']] rc = [0] + # Second lsblk moc output is a duplicate of our first set for err, rc. + err.append(err[0]) + rc.append(0) expected_result = [[ # Note partitions entry within vda, consistent with cli prep. Disk(name='vda', model=None, serial='serial-1', size=4194304, @@ -1038,6 +1067,18 @@ def test_scan_disks_btrfs_in_partition(self): type='part', fstype='btrfs', label='rockstor_rockstor', uuid='355f53a4-24e1-465e-95f3-7c422898f542', parted=True, root=True, partitions={}) + ], [ + # Note sdap (42nd disk) hand crafted from above vda entry + Disk(name='sdap', model='QEMU HARDDISK', serial='42nd-scsi', + size=4194304, transport='sata', vendor='ATA', hctl='3:0:0:0', + type='disk', fstype='btrfs', label='btrfs-in-partition', + uuid='55284332-af66-4ca0-9647-99d9afbe0ec5', parted=True, + root=False, partitions={'sdap1': 'vfat', 'sdap2': 'btrfs'}), + Disk(name='sda3', model='QEMU HARDDISK', serial='QM00005', + size=7025459, transport='sata', vendor='ATA', hctl='2:0:0:0', + type='part', fstype='btrfs', label='rockstor_rockstor', + uuid='355f53a4-24e1-465e-95f3-7c422898f542', parted=True, + root=True, partitions={}) ]] # No LUKS or bcache mocking necessary as none in test data. # Establish dynamic mock behaviour for get_disk_serial() @@ -1423,3 +1464,90 @@ def dyn_disk_serial_return(*args, **kwargs): 'returned = ({}).\n ' 'expected = ({}).'.format(returned, expected)) + + def test_scan_disks_nvme_sys_disk(self): + """ + Post pr #1925 https://github.com/rockstor/rockstor-core/pull/1946 + a regression was observed for nvme system disk installs. Essentially + they were no longer recognized as attached, ie scan_disks() did not + return them. As a result they became detached on existing installs. + Thanks to forum member Jorma_Tuomainen in the following forum thread + for supplying this instance data to be used as a regression test set. + """ + # Test data based on 2 data drives (sdb, sdb) and an nvme system drive + # /dev/nvme0n1 as the base device. + out = [[ + 'NAME="sdb" MODEL="WDC WD100EFAX-68" SERIAL="7PKNDX1C" SIZE="9.1T" TRAN="sata" VENDOR="ATA " HCTL="1:0:0:0" TYPE="disk" FSTYPE="btrfs" LABEL="Data" UUID="d2f76ce6-85fd-4615-b4f8-77e1b6a69c60"', # noqa E501 + 'NAME="sda" MODEL="WDC WD100EFAX-68" SERIAL="7PKP0MNC" SIZE="9.1T" TRAN="sata" VENDOR="ATA " HCTL="0:0:0:0" TYPE="disk" FSTYPE="btrfs" LABEL="Data" UUID="d2f76ce6-85fd-4615-b4f8-77e1b6a69c60"', # noqa E501 + 'NAME="nvme0n1" MODEL="INTEL SSDPEKKW128G7 " SERIAL="BTPY72910KCW128A" SIZE="119.2G" TRAN="" VENDOR="" HCTL="" TYPE="disk" FSTYPE="" LABEL="" UUID=""', # noqa E501 + 'NAME="nvme0n1p3" MODEL="" SERIAL="" SIZE="7.8G" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="swap" LABEL="" UUID="d33115d8-3d8c-4f65-b560-8ebf72d08fbc"', # noqa E501 + 'NAME="nvme0n1p1" MODEL="" SERIAL="" SIZE="200M" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="vfat" LABEL="" UUID="53DC-1323"', # noqa E501 + 'NAME="nvme0n1p4" MODEL="" SERIAL="" SIZE="110.8G" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="btrfs" LABEL="rockstor_rockstor00" UUID="4a05477f-cd4a-4614-b264-d029d98928ab"', # noqa E501 + 'NAME="nvme0n1p2" MODEL="" SERIAL="" SIZE="500M" TRAN="" VENDOR="" HCTL="" TYPE="part" FSTYPE="ext4" LABEL="" UUID="497a9eda-a655-4fc4-bad8-2d9aa8661980"', # noqa E501 + '']] + err = [['']] + rc = [0] + # Second lsblk moc output is a duplicate of our first set. + out.append(out[0]) + err.append(err[0]) + rc.append(0) + # Setup expected results + expected_result = [[ + Disk(name='sda', model='WDC WD100EFAX-68', serial='7PKP0MNC', + size=9771050598, transport='sata', vendor='ATA', + hctl='0:0:0:0', type='disk', fstype='btrfs', label='Data', + uuid='d2f76ce6-85fd-4615-b4f8-77e1b6a69c60', parted=False, + root=False, partitions={}), + Disk(name='sdb', model='WDC WD100EFAX-68', serial='7PKNDX1C', + size=9771050598, transport='sata', vendor='ATA', + hctl='1:0:0:0', type='disk', fstype='btrfs', label='Data', + uuid='d2f76ce6-85fd-4615-b4f8-77e1b6a69c60', parted=False, + root=False, partitions={}) + ], [ + Disk(name='nvme0n1p4', model='INTEL SSDPEKKW128G7', + serial='BTPY72910KCW128A', size=116182220, transport=None, + vendor=None, hctl=None, type='part', fstype='btrfs', + label='rockstor_rockstor00', + uuid='4a05477f-cd4a-4614-b264-d029d98928ab', parted=True, + root=True, partitions={}), + Disk(name='sda', model='WDC WD100EFAX-68', serial='7PKP0MNC', + size=9771050598, transport='sata', vendor='ATA', + hctl='0:0:0:0', type='disk', fstype='btrfs', label='Data', + uuid='d2f76ce6-85fd-4615-b4f8-77e1b6a69c60', parted=False, + root=False, partitions={}), + Disk(name='sdb', model='WDC WD100EFAX-68', serial='7PKNDX1C', + size=9771050598, transport='sata', vendor='ATA', + hctl='1:0:0:0', type='disk', fstype='btrfs', label='Data', + uuid='d2f76ce6-85fd-4615-b4f8-77e1b6a69c60', parted=False, + root=False, partitions={}) + ]] + # Second expected instance is where the nvme system disk is identified. + # 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() + # Ensure we correctly mock our root_disk value away from file default + # of sda as we now have a root_disk on an nvme device. + self.mock_root_disk.return_value = 'nvme0n1' + # 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(1048576, test_mode=True) + returned.sort(key=operator.itemgetter(0)) + # TODO: Would be nice to have differences found shown. + if len(expected) == 2: # no system disk only the 2 data disks + self.assertNotEqual(returned, expected, + msg='Nvme sys disk missing regression:\n ' + 'returned = ({}).\n ' + 'expected = ({}).'.format(returned, + expected)) + if len(expected) == 3: + # assumed to be our correctly reported 1 x sys + 2 x data disks + self.assertEqual(returned, expected, + msg='Un-expected scan_disks() result:\n ' + 'returned = ({}).\n ' + 'expected = ({}).'.format(returned, + expected))