Merge lp:~ltrager/maas/lp1685361 into lp:~maas-committers/maas/trunk

[Lee Trager]

Fix running commissioning and testing on Trusty

  * The new Python script runner used paramater expansion before named arguments which is a feature available in Python 3.5+.
  * 00-maas-07-block-devices sorted results using the -x flag which is not available in Trusty. Sorted is now done with Python.
  * If a commissioning script failed the node goes into failed commissioning and invalidates the hosts OAUTH tokens. The runner was still running and sending test results. Now if a commissioning script fails no test scripts are run and the metadata server marks all pending tests as aborted.

MAAS can now use Trusty for commissioning and testing. The smartctl and badblock tests used the SERIAL column to help users identity drive. The trusty version of lsblk does not support outputting this column. The tests now try with the COLUMN argument, then try again without it as they can still run without that information. The ntp test is failing due to the ntp package not being installed. That means ntp isn't being configured, I'm not sure if that is a bug or unsupported on Trusty. If its unsupported I can fix it by install ntp in the test. The stress-ng tests also fail due to unsupported arguments used with stress-ng. I could look into switching to using the snap for stress-ng.

[Blake Rouse (blake-rouse)]

Looks good. Thanks for fixing this issue glad to see it work on Trusty. Did you fix this in 2.1 as well? For the block device listing that is.

review: Approve

=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py	2017-04-06 16:08:19 +0000
+++ src/metadataserver/api.py	2017-04-22 03:04:16 +0000
@@ -495,12 +495,18 @@
         }
         target_status = signaling_statuses.get(status)
 
-        if target_status is not None:
-            node.status_expires = None
-
-        # Recalculate tags when commissioning ends.
         if target_status in [NODE_STATUS.READY, NODE_STATUS.TESTING]:
+            # Recalculate tags when commissioning ends.
             populate_tags_for_single_node(Tag.objects.all(), node)
+        elif (target_status == NODE_STATUS.FAILED_COMMISSIONING and
+                node.current_testing_script_set is not None):
+            # If commissioning failed testing doesn't run, mark any pending
+            # scripts as aborted.
+            qs = node.current_testing_script_set.scriptresult_set.filter(
+                status=SCRIPT_STATUS.PENDING)
+            for script_result in qs:
+                script_result.status = SCRIPT_STATUS.ABORTED
+                script_result.save(update_fields=['status'])
 
         return target_status
 

=== modified file 'src/metadataserver/builtin_scripts/badblocks.py'
--- src/metadataserver/builtin_scripts/badblocks.py	2017-03-29 23:38:04 +0000
+++ src/metadataserver/builtin_scripts/badblocks.py	2017-04-22 03:04:16 +0000
@@ -77,11 +77,20 @@
         iscsi_drives = re.findall(
             'Attached scsi disk (?P<disk>\w+)', output.decode('utf-8'))
 
-    lsblk_output = check_output(
-        [
-            'lsblk', '--exclude', '1,2,7', '-d', '-P', '-o',
-            'NAME,RO,MODEL,SERIAL',
-        ], timeout=TIMEOUT).decode('utf-8')
+    try:
+        lsblk_output = check_output(
+            [
+                'lsblk', '--exclude', '1,2,7', '-d', '-P', '-o',
+                'NAME,RO,MODEL,SERIAL',
+            ], timeout=TIMEOUT).decode('utf-8')
+    except CalledProcessError:
+        # The SERIAL column is unsupported in the Trusty version of lsblk. Try
+        # again without it.
+        lsblk_output = check_output(
+            [
+                'lsblk', '--exclude', '1,2,7', '-d', '-P', '-o',
+                'NAME,RO,MODEL',
+            ], timeout=TIMEOUT).decode('utf-8')
     drives = []
     for line in lsblk_output.splitlines():
         drive = {}
@@ -115,7 +124,10 @@
         dashes = '-' * int((80.0 - (2 + len(thread.drive['PATH']))) / 2)
         print('%s %s %s' % (dashes, thread.drive['PATH'], dashes))
         print('Model:  %s' % thread.drive['MODEL'])
-        print('Serial: %s' % thread.drive['SERIAL'])
+        # The SERIAL column is only available in newer versions of lsblk. This
+        # can be removed with Trusty support.
+        if 'SERIAL' in thread.drive:
+            print('Serial: %s' % thread.drive['SERIAL'])
         print()
 
         if thread.returncode != 0:

=== modified file 'src/metadataserver/builtin_scripts/smartctl.py'
--- src/metadataserver/builtin_scripts/smartctl.py	2017-03-30 00:45:53 +0000
+++ src/metadataserver/builtin_scripts/smartctl.py	2017-04-22 03:04:16 +0000
@@ -148,11 +148,20 @@
                     if m is not None:
                         drives.append(drive_with_device_type)
 
-    all_drives = check_output(
-        [
-            'lsblk', '--exclude', '1,2,7', '-d', '-l', '-o',
-            'NAME,MODEL,SERIAL', '-x', 'NAME',
-        ], timeout=TIMEOUT, stderr=DEVNULL).decode('utf-8').splitlines()
+    try:
+        all_drives = check_output(
+            [
+                'lsblk', '--exclude', '1,2,7', '-d', '-l', '-o',
+                'NAME,MODEL,SERIAL', '-x', 'NAME',
+            ], timeout=TIMEOUT, stderr=DEVNULL).decode('utf-8').splitlines()
+    except CalledProcessError:
+        # The SERIAL column and sorting(-x) are unsupported in the Trusty
+        # version of lsblk. Try again without them.
+        all_drives = check_output(
+            [
+                'lsblk', '--exclude', '1,2,7', '-d', '-l', '-o',
+                'NAME,MODEL',
+            ], timeout=TIMEOUT, stderr=DEVNULL).decode('utf-8').splitlines()
     supported_drives = iscsi_drives + [
         drive[0].split('/')[-1] for drive in drives]
     unsupported_drives = [

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2017-04-06 16:08:19 +0000
+++ src/metadataserver/tests/test_api.py	2017-04-22 03:04:16 +0000
@@ -1435,7 +1435,7 @@
         self.assertEqual(http.client.OK, response.status_code)
         self.assertEqual(NODE_STATUS.READY, reload_object(node).status)
 
-    def test_signaling_commissioning_failure_makes_node_failed_Tests(self):
+    def test_signaling_commissioning_failure_makes_node_failed_tests(self):
         node = factory.make_Node(
             status=NODE_STATUS.COMMISSIONING, with_empty_script_sets=True)
         client = make_node_client(node=node)
@@ -1443,6 +1443,8 @@
         self.assertEqual(http.client.OK, response.status_code)
         self.assertEqual(
             NODE_STATUS.FAILED_COMMISSIONING, reload_object(node).status)
+        for script_result in node.current_testing_script_set:
+            self.assertEqual(SCRIPT_STATUS.ABORTED, script_result.status)
 
     def test_signaling_commissioning_failure_does_not_populate_tags(self):
         populate_tags_for_single_node = self.patch(

=== modified file 'src/metadataserver/user_data/templates/snippets/maas_run_remote_scripts.py'
--- src/metadataserver/user_data/templates/snippets/maas_run_remote_scripts.py	2017-04-13 01:15:22 +0000
+++ src/metadataserver/user_data/templates/snippets/maas_run_remote_scripts.py	2017-04-22 03:04:16 +0000
@@ -103,8 +103,9 @@
         timeout_seconds = script.get('timeout_seconds')
 
         signal_wrapper(
-            **args, error='Starting %s [%d/%d]' % (
-                script['name'], i, len(scripts)))
+            error='Starting %s [%d/%d]' % (
+                script['name'], i, len(scripts)),
+            **args)
 
         script_path = os.path.join(scripts_dir, script['path'])
         combined_path = os.path.join(out_dir, script['name'])
@@ -142,8 +143,9 @@
                 stderr_name: result,
             }
             signal_wrapper(
-                **args, error='Failed to execute %s [%d/%d]: %d' % (
-                    script['name'], i, total_scripts, args['exit_status']))
+                error='Failed to execute %s [%d/%d]: %d' % (
+                    script['name'], i, total_scripts, args['exit_status']),
+                **args)
         except TimeoutExpired:
             fail_count += 1
             args['status'] = 'TIMEDOUT'
@@ -153,9 +155,10 @@
                 stderr_name: open(stderr_path, 'rb').read(),
             }
             signal_wrapper(
-                **args, error='Timeout(%s) expired on %s [%d/%d]' % (
+                error='Timeout(%s) expired on %s [%d/%d]' % (
                     str(timedelta(seconds=timeout_seconds)), script['name'], i,
-                    total_scripts))
+                    total_scripts),
+                **args)
         else:
             if proc.returncode != 0:
                 fail_count += 1
@@ -166,8 +169,9 @@
                 stderr_name: open(stderr_path, 'rb').read(),
             }
             signal_wrapper(
-                **args, error='Finished %s [%d/%d]: %d' % (
-                    script['name'], i, len(scripts), args['exit_status']))
+                error='Finished %s [%d/%d]: %d' % (
+                    script['name'], i, len(scripts), args['exit_status']),
+                **args)
 
     # Signal failure after running commissioning or testing scripts so MAAS
     # transisitions the node into FAILED_COMMISSIONING or FAILED_TESTING.
@@ -188,6 +192,8 @@
     if commissioning_scripts is not None:
         fail_count += run_scripts(
             url, creds, scripts_dir, out_dir, commissioning_scripts)
+        if fail_count != 0:
+            return
 
     testing_scripts = scripts.get('testing_scripts')
     if testing_scripts is not None:

=== modified file 'src/metadataserver/user_data/templates/snippets/tests/test_maas_run_remote_scripts.py'
--- src/metadataserver/user_data/templates/snippets/tests/test_maas_run_remote_scripts.py	2017-04-13 01:19:43 +0000
+++ src/metadataserver/user_data/templates/snippets/tests/test_maas_run_remote_scripts.py	2017-04-22 03:04:16 +0000
@@ -21,6 +21,7 @@
     MockAnyCall,
     MockCalledOnce,
     MockCalledOnceWith,
+    MockNotCalled,
 )
 from maastesting.testcase import MAASTestCase
 from snippets import maas_run_remote_scripts
@@ -117,6 +118,24 @@
             mock_signal,
             MockAnyCall(None, None, 'OK', 'All scripts successfully ran'))
 
+    def test_run_scripts_from_metadata_doesnt_run_tests_on_commiss_fail(self):
+        scripts_dir = self.useFixture(TempDirectory()).path
+        mock_run_scripts = self.patch(maas_run_remote_scripts, 'run_scripts')
+        mock_run_scripts.return_value = random.randint(1, 100)
+        mock_signal = self.patch(maas_run_remote_scripts, 'signal')
+        index_json = self.make_index_json(scripts_dir)
+
+        # Don't need to give the url, creds, or out_dir as we're not running
+        # the scripts and sending the results.
+        run_scripts_from_metadata(None, None, scripts_dir, None)
+
+        self.assertThat(
+            mock_run_scripts,
+            MockCalledOnceWith(
+                None, None, scripts_dir, None,
+                index_json['commissioning_scripts']))
+        self.assertThat(mock_signal, MockNotCalled())
+
     def test_run_scripts_from_metadata_does_nothing_on_empty(self):
         scripts_dir = self.useFixture(TempDirectory()).path
         self.patch(maas_run_remote_scripts, 'run_scripts')

=== modified file 'src/provisioningserver/refresh/node_info_scripts.py'
--- src/provisioningserver/refresh/node_info_scripts.py	2017-04-06 00:39:19 +0000
+++ src/provisioningserver/refresh/node_info_scripts.py	2017-04-22 03:04:16 +0000
@@ -457,7 +457,7 @@
     blockdevs = []
     block_list = check_output((
         "lsblk", "--exclude", "1,2,7", "-d", "-P",
-        "-o", "NAME,RO,RM,MODEL,ROTA,MAJ:MIN", "-x", "MAJ:MIN"))
+        "-o", "NAME,RO,RM,MODEL,ROTA,MAJ:MIN"))
     block_list = block_list.decode("utf-8")
     for blockdev in block_list.splitlines():
         tokens = shlex.split(blockdev)
@@ -467,6 +467,13 @@
             current_block[k] = v.strip()
         blockdevs.append(current_block)
 
+    # Sort drives by MAJ:MIN so MAAS picks the correct boot drive.
+    # lsblk -x MAJ:MIN can't be used as the -x flag only appears in
+    # lsblk 2.71.1 or newer which is unavailable on Trusty. See LP:1673724
+    blockdevs = sorted(
+        blockdevs,
+        key=lambda blockdev: [int(i) for i in blockdev['MAJ:MIN'].split(':')])
+
     # Grab the device path, serial number, and sata connection.
     UDEV_MAPPINGS = {
         "DEVNAME": "PATH",

=== modified file 'src/provisioningserver/refresh/tests/test_node_info_scripts.py'
--- src/provisioningserver/refresh/tests/test_node_info_scripts.py	2017-04-05 14:35:05 +0000
+++ src/provisioningserver/refresh/tests/test_node_info_scripts.py	2017-04-22 03:04:16 +0000
@@ -338,16 +338,19 @@
     @typed
     def make_lsblk_output(
             self, name=None, read_only=False, removable=False,
-            model=None, rotary=True) -> bytes:
+            model=None, rotary=True, maj_min=None) -> bytes:
         if name is None:
             name = factory.make_name('name')
         if model is None:
             model = factory.make_name('model')
+        if maj_min is None:
+            maj_min = (random.randint(0, 255), random.randint(0, 255))
         read_only = "1" if read_only else "0"
         removable = "1" if removable else "0"
         rotary = "1" if rotary else "0"
-        output = 'NAME="%s" RO="%s" RM="%s" MODEL="%s" ROTA="%s"' % (
-            name, read_only, removable, model, rotary)
+        output = (
+            'NAME="%s" RO="%s" RM="%s" MODEL="%s" ROTA="%s" MAJ:MIN="%s"' % (
+                name, read_only, removable, model, rotary, '%s:%s' % maj_min))
         return output.encode("ascii")
 
     @typed
@@ -390,13 +393,37 @@
         gather_physical_block_devices(dev_disk_byid=dev_disk_byid)
         return json.loads(output.getvalue())
 
+    def make_output(
+            self, name, maj_min, model, serial, size, block_size,
+            drive_path=None, device_id_path=None, rotary=True,
+            removable=False, read_only=False, sata=True):
+        if drive_path is None:
+            drive_path = '/dev/%s' % name
+        ret = {
+            'NAME': name,
+            'PATH': drive_path,
+            'MAJ:MIN': '%s:%s' % maj_min,
+            'RO': '1' if read_only else '0',
+            'RM': '1' if removable else '0',
+            'MODEL': model,
+            'ROTA': '1' if rotary else '0',
+            'SATA': '1' if sata else '0',
+            'SERIAL': serial,
+            'SIZE': str(size),
+            'BLOCK_SIZE': str(block_size),
+            'RPM': '5400',
+        }
+        if device_id_path is not None:
+            ret['ID_PATH'] = device_id_path
+        return ret
+
     def test__calls_lsblk(self):
         check_output = self.patch(subprocess, "check_output")
         check_output.return_value = b""
         self.call_gather_physical_block_devices()
         self.assertThat(check_output, MockCalledOnceWith((
             "lsblk", "--exclude", "1,2,7", "-d", "-P",
-            "-o", "NAME,RO,RM,MODEL,ROTA,MAJ:MIN", "-x", "MAJ:MIN")))
+            "-o", "NAME,RO,RM,MODEL,ROTA,MAJ:MIN")))
 
     def test__returns_empty_list_when_no_disks(self):
         check_output = self.patch(subprocess, "check_output")
@@ -416,7 +443,7 @@
         self.assertThat(check_output, MockCallsMatch(
             call((
                 "lsblk", "--exclude", "1,2,7", "-d", "-P",
-                "-o", "NAME,RO,RM,MODEL,ROTA,MAJ:MIN", "-x", "MAJ:MIN")),
+                "-o", "NAME,RO,RM,MODEL,ROTA,MAJ:MIN")),
             call(("udevadm", "info", "-q", "all", "-n", name))))
 
     def test__returns_empty_list_when_cdrom_only(self):
@@ -448,7 +475,7 @@
         self.assertThat(check_output, MockCallsMatch(
             call((
                 "lsblk", "--exclude", "1,2,7", "-d", "-P",
-                "-o", "NAME,RO,RM,MODEL,ROTA,MAJ:MIN", "-x", "MAJ:MIN")),
+                "-o", "NAME,RO,RM,MODEL,ROTA,MAJ:MIN")),
             call(("udevadm", "info", "-q", "all", "-n", name)),
             call(("sudo", "-n", "blockdev", "--getsize64", "/dev/%s" % name)),
             call(("sudo", "-n", "blockdev", "--getbsz", "/dev/%s" % name))))
@@ -471,48 +498,52 @@
         self.assertThat(check_output, MockCallsMatch(
             call((
                 "lsblk", "--exclude", "1,2,7", "-d", "-P",
-                "-o", "NAME,RO,RM,MODEL,ROTA,MAJ:MIN", "-x", "MAJ:MIN")),
+                "-o", "NAME,RO,RM,MODEL,ROTA,MAJ:MIN")),
             call(("udevadm", "info", "-q", "all", "-n", name)),
             call(("blockdev", "--getsize64", "/dev/%s" % name)),
             call(("blockdev", "--getbsz", "/dev/%s" % name))))
 
-    def test__returns_block_device(self):
-        name = factory.make_name('name')
-        model = factory.make_name('model')
-        serial = factory.make_name('serial')
-        size = random.randint(3000 * 1000, 1000 * 1000 * 1000)
-        block_size = random.choice([512, 1024, 4096])
-        check_output = self.patch(subprocess, "check_output")
-
+    def test__returns_sorted_block_devices(self):
+        output = []
+        check_output_side_effects = []
         # Create simulated /dev tree
         devroot = self.make_dir()
         os.mkdir(os.path.join(devroot, 'disk'))
         byidroot = os.path.join(devroot, 'disk', 'by_id')
         os.mkdir(byidroot)
-        os.mknod(os.path.join(devroot, name))
-        os.symlink(os.path.join(devroot, name),
-                   os.path.join(byidroot, 'deviceid'))
-
-        check_output.side_effect = [
-            self.make_lsblk_output(name=name, model=model),
-            self.make_udevadm_output(name, serial=serial, dev=devroot),
-            b'%d' % size,
-            b'%d' % block_size,
+        for _ in range(3):
+            name = factory.make_name('name')
+            model = factory.make_name('model')
+            serial = factory.make_name('serial')
+            size = random.randint(3000 * 1000, 1000 * 1000 * 1000)
+            block_size = random.choice([512, 1024, 4096])
+            maj_min = (random.randint(0, 255), random.randint(0, 255))
+
+            # Create simulated /dev tree
+            drive_path = os.path.join(devroot, name)
+            os.mknod(drive_path)
+            device_id_path = os.path.join(
+                byidroot, factory.make_name('deviceid'))
+            os.symlink(drive_path, device_id_path)
+
+            check_output_side_effects += [
+                self.make_lsblk_output(
+                    name=name, model=model, maj_min=maj_min),
+                self.make_udevadm_output(name, serial=serial, dev=devroot),
+                b'%d' % size,
+                b'%d' % block_size,
             ]
-        self.assertEqual([{
-            "NAME": name,
-            "PATH": os.path.join(devroot, name),
-            "ID_PATH": os.path.join(byidroot, 'deviceid'),
-            "RO": "0",
-            "RM": "0",
-            "MODEL": model,
-            "ROTA": "1",
-            "SATA": "1",
-            "SERIAL": serial,
-            "SIZE": "%s" % size,
-            "BLOCK_SIZE": "%s" % block_size,
-            "RPM": "5400",
-            }], self.call_gather_physical_block_devices(byidroot))
+            output.append(
+                self.make_output(
+                    name, maj_min, model, serial, size, block_size,
+                    drive_path, device_id_path))
+
+        check_output = self.patch(subprocess, "check_output")
+        check_output.side_effect = check_output_side_effects
+
+        for ref, out in zip(
+                output, self.call_gather_physical_block_devices(byidroot)):
+            self.assertDictEqual(ref, out)
 
     def test__removes_duplicate_block_device_same_serial_and_model(self):
         """Multipath disks get multiple IDs, but same serial/model is same
@@ -522,107 +553,51 @@
         serial = factory.make_name('serial')
         size = random.randint(3000 * 1000, 1000 * 1000 * 1000)
         block_size = random.choice([512, 1024, 4096])
-        check_output = self.patch(subprocess, "check_output")
-
-        name2 = factory.make_name('name')
-
-        # Create simulated /dev tree.
-        devroot = self.make_dir()
-        os.mkdir(os.path.join(devroot, 'disk'))
-        byidroot = os.path.join(devroot, 'disk', 'by_id')
-        os.mkdir(byidroot)
-
-        os.mknod(os.path.join(devroot, name))
-        os.symlink(os.path.join(devroot, name),
-                   os.path.join(byidroot, 'deviceid'))
-
-        os.mknod(os.path.join(devroot, name2))
-        os.symlink(os.path.join(devroot, name2),
-                   os.path.join(byidroot, 'deviceid2'))
-
-        check_output.side_effect = [
-            b"\n".join([
-                self.make_lsblk_output(name=name, model=model),
-                self.make_lsblk_output(name=name2, model=model)]),
-            self.make_udevadm_output(name, serial=serial, dev=devroot),
-            self.make_udevadm_output(name2, serial=serial, dev=devroot),
-            b'%d' % size,
-            b'%d' % block_size,
-            b'%d' % size,
-            b'%d' % block_size,
-        ]
-
-        self.assertEqual([{
-            "NAME": name,
-            "PATH": os.path.join(devroot, name),
-            "ID_PATH": os.path.join(byidroot, 'deviceid'),
-            "RO": "0",
-            "RM": "0",
-            "MODEL": model,
-            "ROTA": "1",
-            "SATA": "1",
-            "SERIAL": serial,
-            "SIZE": "%s" % size,
-            "BLOCK_SIZE": "%s" % block_size,
-            "RPM": "5400",
-            }], self.call_gather_physical_block_devices(byidroot))
-
-    def test__removes_duplicate_block_device_same_serial_blank_model(self):
-        """Multipath disks get multiple IDs, but same serial is same device."""
-        name = factory.make_name('name')
-        model = ""
-        serial = factory.make_name('serial')
-        size = random.randint(3000 * 1000, 1000 * 1000 * 1000)
-        block_size = random.choice([512, 1024, 4096])
-        check_output = self.patch(subprocess, "check_output")
-
-        name2 = factory.make_name('name')
-
-        # Create simulated /dev tree.
-        devroot = self.make_dir()
-        os.mkdir(os.path.join(devroot, 'disk'))
-        byidroot = os.path.join(devroot, 'disk', 'by_id')
-        os.mkdir(byidroot)
-
-        os.mknod(os.path.join(devroot, name))
-        os.symlink(os.path.join(devroot, name),
-                   os.path.join(byidroot, 'deviceid'))
-
-        os.mknod(os.path.join(devroot, name2))
-        os.symlink(os.path.join(devroot, name2),
-                   os.path.join(byidroot, 'deviceid2'))
-
-        check_output.side_effect = [
-            b"\n".join([
-                self.make_lsblk_output(name=name, model=model),
-                self.make_lsblk_output(name=name2, model=model)]),
-            self.make_udevadm_output(name, serial=serial, dev=devroot),
-            self.make_udevadm_output(name2, serial=serial, dev=devroot),
-            b'%d' % size,
-            b'%d' % block_size,
-            b'%d' % size,
-            b'%d' % block_size,
-        ]
-
-        self.assertEqual([{
-            "NAME": name,
-            "PATH": os.path.join(devroot, name),
-            "ID_PATH": os.path.join(byidroot, 'deviceid'),
-            "RO": "0",
-            "RM": "0",
-            "MODEL": model,
-            "ROTA": "1",
-            "SATA": "1",
-            "SERIAL": serial,
-            "SIZE": "%s" % size,
-            "BLOCK_SIZE": "%s" % block_size,
-            "RPM": "5400",
-            }], self.call_gather_physical_block_devices(byidroot))
+        maj_min = (random.randint(0, 255), random.randint(0, 255))
+        check_output = self.patch(subprocess, "check_output")
+
+        name2 = factory.make_name('name')
+
+        # Create simulated /dev tree.
+        devroot = self.make_dir()
+        os.mkdir(os.path.join(devroot, 'disk'))
+        byidroot = os.path.join(devroot, 'disk', 'by_id')
+        os.mkdir(byidroot)
+
+        drive_path = os.path.join(devroot, name)
+        os.mknod(drive_path)
+        device_id_path = os.path.join(byidroot, 'deviceid')
+        os.symlink(os.path.join(devroot, name), device_id_path)
+
+        os.mknod(os.path.join(devroot, name2))
+        device_id_path2 = os.path.join(byidroot, 'deviceid2')
+        os.symlink(os.path.join(devroot, name2), device_id_path2)
+
+        check_output.side_effect = [
+            b"\n".join([
+                self.make_lsblk_output(
+                    name=name, model=model, maj_min=maj_min),
+                self.make_lsblk_output(
+                    name=name2, model=model, maj_min=maj_min)]),
+            self.make_udevadm_output(name, serial=serial, dev=devroot),
+            self.make_udevadm_output(name2, serial=serial, dev=devroot),
+            b'%d' % size,
+            b'%d' % block_size,
+            b'%d' % size,
+            b'%d' % block_size,
+        ]
+
+        self.assertItemsEqual(
+            [self.make_output(
+                name, maj_min, model, serial, size, block_size, drive_path,
+                device_id_path)],
+            self.call_gather_physical_block_devices(byidroot))
 
     def test__keeps_block_device_same_serial_different_model(self):
         """Multipath disks get multiple IDs, but same serial is same device."""
         name = factory.make_name('name')
         model = factory.make_name('model')
+        maj_min = (0, 0)
         serial = factory.make_name('serial')
         size = random.randint(3000 * 1000, 1000 * 1000 * 1000)
         block_size = random.choice([512, 1024, 4096])
@@ -630,6 +605,7 @@
 
         name2 = factory.make_name('name')
         model2 = factory.make_name('model')
+        maj_min2 = (1, 1)
 
         # Create simulated /dev tree.
         devroot = self.make_dir()
@@ -637,18 +613,23 @@
         byidroot = os.path.join(devroot, 'disk', 'by_id')
         os.mkdir(byidroot)
 
-        os.mknod(os.path.join(devroot, name))
-        os.symlink(os.path.join(devroot, name),
-                   os.path.join(byidroot, 'deviceid'))
+        drive_path = os.path.join(devroot, name)
+        os.mknod(drive_path)
+        device_id_path = os.path.join(byidroot, 'deviceid')
+        os.symlink(os.path.join(devroot, name), device_id_path)
 
-        os.mknod(os.path.join(devroot, name2))
-        os.symlink(os.path.join(devroot, name2),
-                   os.path.join(byidroot, 'deviceid2'))
+        drive_path2 = os.path.join(devroot, name2)
+        os.mknod(drive_path2)
+        device_id_path2 = os.path.join(byidroot, 'deviceid2')
+        os.symlink(os.path.join(devroot, name2), device_id_path2)
 
         check_output.side_effect = [
             b"\n".join([
-                self.make_lsblk_output(name=name, model=model),
-                self.make_lsblk_output(name=name2, model=model2)]),
+                self.make_lsblk_output(
+                    name=name, model=model, maj_min=maj_min),
+                self.make_lsblk_output(
+                    name=name2, model=model2, maj_min=maj_min2)
+                ]),
             self.make_udevadm_output(name, serial=serial, dev=devroot),
             self.make_udevadm_output(name2, serial=serial, dev=devroot),
             b'%d' % size,
@@ -657,44 +638,29 @@
             b'%d' % block_size,
         ]
 
-        self.assertEqual([{
-            "NAME": name,
-            "PATH": os.path.join(devroot, name),
-            "ID_PATH": os.path.join(byidroot, 'deviceid'),
-            "RO": "0",
-            "RM": "0",
-            "MODEL": model,
-            "ROTA": "1",
-            "SATA": "1",
-            "SERIAL": serial,
-            "SIZE": "%s" % size,
-            "BLOCK_SIZE": "%s" % block_size,
-            "RPM": "5400",
-        }, {
-            "NAME": name2,
-            "PATH": os.path.join(devroot, name2),
-            "ID_PATH": os.path.join(byidroot, 'deviceid2'),
-            "RO": "0",
-            "RM": "0",
-            "MODEL": model2,
-            "ROTA": "1",
-            "SATA": "1",
-            "SERIAL": serial,
-            "SIZE": "%s" % size,
-            "BLOCK_SIZE": "%s" % block_size,
-            "RPM": "5400",
-        }], self.call_gather_physical_block_devices(byidroot))
+        for ref, out in zip(
+                [
+                    self.make_output(
+                        name, maj_min, model, serial, size, block_size,
+                        drive_path, device_id_path),
+                    self.make_output(
+                        name2, maj_min2, model2, serial, size, block_size,
+                        drive_path2, device_id_path2),
+                ], self.call_gather_physical_block_devices(byidroot)):
+            self.assertDictEqual(ref, out)
 
     def test__keeps_block_device_blank_serial_same_model(self):
         """Multipath disks get multiple IDs, but same serial is same device."""
         name = factory.make_name('name')
         model = factory.make_name('model')
+        maj_min = (0, 0)
         serial = ''
         size = random.randint(3000 * 1000, 1000 * 1000 * 1000)
         block_size = random.choice([512, 1024, 4096])
         check_output = self.patch(subprocess, "check_output")
 
         name2 = factory.make_name('name')
+        maj_min2 = (1, 1)
 
         # Create simulated /dev tree.
         devroot = self.make_dir()
@@ -702,18 +668,22 @@
         byidroot = os.path.join(devroot, 'disk', 'by_id')
         os.mkdir(byidroot)
 
-        os.mknod(os.path.join(devroot, name))
-        os.symlink(os.path.join(devroot, name),
-                   os.path.join(byidroot, 'deviceid'))
+        drive_path = os.path.join(devroot, name)
+        os.mknod(drive_path)
+        device_id_path = os.path.join(byidroot, 'deviceid')
+        os.symlink(os.path.join(devroot, name), device_id_path)
 
-        os.mknod(os.path.join(devroot, name2))
-        os.symlink(os.path.join(devroot, name2),
-                   os.path.join(byidroot, 'deviceid2'))
+        drive_path2 = os.path.join(devroot, name2)
+        os.mknod(drive_path2)
+        device_id_path2 = os.path.join(byidroot, 'deviceid2')
+        os.symlink(os.path.join(devroot, name2), device_id_path2)
 
         check_output.side_effect = [
             b"\n".join([
-                self.make_lsblk_output(name=name, model=model),
-                self.make_lsblk_output(name=name2, model=model)]),
+                self.make_lsblk_output(
+                    name=name, model=model, maj_min=maj_min),
+                self.make_lsblk_output(
+                    name=name2, model=model, maj_min=maj_min2)]),
             self.make_udevadm_output(name, serial=serial, dev=devroot),
             self.make_udevadm_output(name2, serial=serial, dev=devroot),
             b'%d' % size,
@@ -722,38 +692,22 @@
             b'%d' % block_size,
         ]
 
-        self.assertEqual([{
-            "NAME": name,
-            "PATH": os.path.join(devroot, name),
-            "ID_PATH": os.path.join(byidroot, 'deviceid'),
-            "RO": "0",
-            "RM": "0",
-            "MODEL": model,
-            "ROTA": "1",
-            "SATA": "1",
-            "SERIAL": serial,
-            "SIZE": "%s" % size,
-            "BLOCK_SIZE": "%s" % block_size,
-            "RPM": "5400",
-        }, {
-            "NAME": name2,
-            "PATH": os.path.join(devroot, name2),
-            "ID_PATH": os.path.join(byidroot, 'deviceid2'),
-            "RO": "0",
-            "RM": "0",
-            "MODEL": model,
-            "ROTA": "1",
-            "SATA": "1",
-            "SERIAL": serial,
-            "SIZE": "%s" % size,
-            "BLOCK_SIZE": "%s" % block_size,
-            "RPM": "5400",
-        }], self.call_gather_physical_block_devices(byidroot))
+        for ref, out in zip(
+                [
+                    self.make_output(
+                        name, maj_min, model, serial, size, block_size,
+                        drive_path, device_id_path),
+                    self.make_output(
+                        name2, maj_min2, model, serial, size, block_size,
+                        drive_path2, device_id_path2),
+                ], self.call_gather_physical_block_devices(byidroot)):
+            self.assertDictEqual(ref, out)
 
     def test__returns_block_device_without_id_path(self):
         """Block devices without by-id links should not have ID_PATH key"""
         name = factory.make_name('name')
         model = factory.make_name('model')
+        maj_min = (random.randint(0, 255), random.randint(0, 255))
         serial = factory.make_name('serial')
         size = random.randint(3000 * 1000, 1000 * 1000 * 1000)
         block_size = random.choice([512, 1024, 4096])
@@ -764,158 +718,113 @@
         os.mkdir(os.path.join(devroot, 'disk'))
         byidroot = os.path.join(devroot, 'disk', 'by_id')
         os.mkdir(byidroot)
-        os.mknod(os.path.join(devroot, name))
+        drive_path = os.path.join(devroot, name)
+        os.mknod(drive_path)
 
         check_output.side_effect = [
-            self.make_lsblk_output(name=name, model=model),
+            self.make_lsblk_output(name=name, model=model, maj_min=maj_min),
             self.make_udevadm_output(name, serial=serial, dev=devroot),
             b'%d' % size,
             b'%d' % block_size,
             ]
-        self.assertEqual([{
-            "NAME": name,
-            "PATH": os.path.join(devroot, name),
-            "RO": "0",
-            "RM": "0",
-            "MODEL": model,
-            "ROTA": "1",
-            "SATA": "1",
-            "SERIAL": serial,
-            "SIZE": "%s" % size,
-            "BLOCK_SIZE": "%s" % block_size,
-            "RPM": "5400",
-            }], self.call_gather_physical_block_devices(byidroot))
+        for ref, out in zip(
+                [
+                    self.make_output(
+                        name, maj_min, model, serial, size, block_size,
+                        drive_path),
+                ], self.call_gather_physical_block_devices(byidroot)):
+            self.assertDictEqual(ref, out)
 
     def test__returns_block_device_readonly(self):
         name = factory.make_name('name')
         model = factory.make_name('model')
+        maj_min = (random.randint(0, 255), random.randint(0, 255))
         serial = factory.make_name('serial')
         size = random.randint(3000 * 1000, 1000 * 1000 * 1000)
         block_size = random.choice([512, 1024, 4096])
         check_output = self.patch(subprocess, "check_output")
         check_output.side_effect = [
-            self.make_lsblk_output(name=name, model=model, read_only=True),
+            self.make_lsblk_output(
+                name=name, model=model, read_only=True, maj_min=maj_min),
             self.make_udevadm_output(name, serial=serial),
             b'%d' % size,
             b'%d' % block_size,
             ]
-        self.assertEqual([{
-            "NAME": name,
-            "PATH": "/dev/%s" % name,
-            "RO": "1",
-            "RM": "0",
-            "MODEL": model,
-            "ROTA": "1",
-            "SATA": "1",
-            "SERIAL": serial,
-            "SIZE": "%s" % size,
-            "BLOCK_SIZE": "%s" % block_size,
-            "RPM": "5400",
-            }], self.call_gather_physical_block_devices())
+        for ref, out in zip(
+                [
+                    self.make_output(
+                        name, maj_min, model, serial, size,
+                        block_size, read_only=True),
+                ], self.call_gather_physical_block_devices()):
+            self.assertDictEqual(ref, out)
 
     def test__returns_block_device_ssd(self):
         name = factory.make_name('name')
         model = factory.make_name('model')
+        maj_min = (random.randint(0, 255), random.randint(0, 255))
         serial = factory.make_name('serial')
         size = random.randint(3000 * 1000, 1000 * 1000 * 1000)
         block_size = random.choice([512, 1024, 4096])
         check_output = self.patch(subprocess, "check_output")
         check_output.side_effect = [
-            self.make_lsblk_output(name=name, model=model, rotary=False),
+            self.make_lsblk_output(
+                name=name, model=model, rotary=False, maj_min=maj_min),
             self.make_udevadm_output(name, serial=serial),
             b'%d' % size,
             b'%d' % block_size,
             ]
-        self.assertEqual([{
-            "NAME": name,
-            "PATH": "/dev/%s" % name,
-            "RO": "0",
-            "RM": "0",
-            "MODEL": model,
-            "ROTA": "0",
-            "SATA": "1",
-            "SERIAL": serial,
-            "SIZE": "%s" % size,
-            "BLOCK_SIZE": "%s" % block_size,
-            "RPM": "5400",
-            }], self.call_gather_physical_block_devices())
+        for ref, out in zip(
+                [
+                    self.make_output(
+                        name, maj_min, model, serial, size, block_size,
+                        rotary=False),
+                ], self.call_gather_physical_block_devices()):
+            self.assertDictEqual(ref, out)
 
     def test__returns_block_device_not_sata(self):
         name = factory.make_name('name')
         model = factory.make_name('model')
+        maj_min = (random.randint(0, 255), random.randint(0, 255))
         serial = factory.make_name('serial')
         size = random.randint(3000 * 1000, 1000 * 1000 * 1000)
         block_size = random.choice([512, 1024, 4096])
         check_output = self.patch(subprocess, "check_output")
         check_output.side_effect = [
-            self.make_lsblk_output(name=name, model=model),
+            self.make_lsblk_output(name=name, model=model, maj_min=maj_min),
             self.make_udevadm_output(name, serial=serial, sata=False),
             b'%d' % size,
             b'%d' % block_size,
             ]
-        self.assertEqual([{
-            "NAME": name,
-            "PATH": "/dev/%s" % name,
-            "RO": "0",
-            "RM": "0",
-            "MODEL": model,
-            "ROTA": "1",
-            "SATA": "0",
-            "SERIAL": serial,
-            "SIZE": "%s" % size,
-            "BLOCK_SIZE": "%s" % block_size,
-            "RPM": "5400",
-            }], self.call_gather_physical_block_devices())
+        for ref, out in zip(
+                [
+                    self.make_output(
+                        name, maj_min, model, serial, size, block_size,
+                        sata=False),
+                ], self.call_gather_physical_block_devices()):
+            self.assertDictEqual(ref, out)
 
     def test__returns_block_device_removable(self):
         name = factory.make_name('name')
         model = factory.make_name('model')
+        maj_min = (random.randint(0, 255), random.randint(0, 255))
         serial = factory.make_name('serial')
         size = random.randint(3000 * 1000, 1000 * 1000 * 1000)
         block_size = random.choice([512, 1024, 4096])
         check_output = self.patch(subprocess, "check_output")
         check_output.side_effect = [
-            self.make_lsblk_output(name=name, model=model, removable=True),
+            self.make_lsblk_output(
+                name=name, model=model, removable=True, maj_min=maj_min),
             self.make_udevadm_output(name, serial=serial),
             b'%d' % size,
             b'%d' % block_size,
             ]
-        self.assertEqual([{
-            "NAME": name,
-            "PATH": "/dev/%s" % name,
-            "RO": "0",
-            "RM": "1",
-            "MODEL": model,
-            "ROTA": "1",
-            "SATA": "1",
-            "SERIAL": serial,
-            "SIZE": "%s" % size,
-            "BLOCK_SIZE": "%s" % block_size,
-            "RPM": "5400",
-            }], self.call_gather_physical_block_devices())
-
-    def test__returns_multiple_block_devices_in_order(self):
-        names = [factory.make_name('name') for _ in range(3)]
-        lsblk = [
-            self.make_lsblk_output(name=name)
-            for name in names
-            ]
-        call_outputs = []
-        call_outputs.append(b"\n".join(lsblk))
-        for name in names:
-            call_outputs.append(self.make_udevadm_output(name))
-        for name in names:
-            call_outputs.append(
-                b"%d" % random.randint(1000 * 1000, 1000 * 1000 * 1000))
-            call_outputs.append(
-                b"%d" % random.choice([512, 1024, 4096]))
-        check_output = self.patch(subprocess, "check_output")
-        check_output.side_effect = call_outputs
-        device_names = [
-            block_info['NAME']
-            for block_info in self.call_gather_physical_block_devices()
-            ]
-        self.assertEqual(names, device_names)
+        for ref, out in zip(
+                [
+                    self.make_output(
+                        name, maj_min, model, serial, size, block_size,
+                        removable=True),
+                ], self.call_gather_physical_block_devices()):
+            self.assertDictEqual(ref, out)
 
     def test__quietly_exits_in_container(self):
         script_dir = self.useFixture(TempDirectory()).path