Skip to content

Commit

Permalink
regression - nvme system disk not identified #1951
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@phillxnet
phillxnet committed Aug 14, 2018
1 parent befe016 commit 28b35c6
Show file tree
Hide file tree
Showing 2 changed files with 145 additions and 12 deletions.
23 changes: 14 additions & 9 deletions src/rockstor/system/osi.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
134 changes: 131 additions & 3 deletions src/rockstor/system/tests/test_osi.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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,
Expand All @@ -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()
Expand Down Expand Up @@ -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))

0 comments on commit 28b35c6

Please sign in to comment.