Merge lp:~newell-jensen/maas/fix-1694767 into lp:~maas-committers/maas/trunk

[Newell Jensen]

Fixes RSD pod driver conditional to check that the smallest available block size is larger than the requested block device size in get_pod_machine_local_storages. Adds tests to check for successful and unsuccessful mapping of block device tags.

[Mike Pontillo (mpontillo)]

I'm not sure I fully understand this branch. It might be fine as-is, but I can't really tell given the lack of context.

I think it would be clearer if you add some unit tests showing more specific expected success and failure scenarios.

review: Needs Information

[Mike Pontillo (mpontillo)]

Also, you'll need to set a commit message. ;-)

[Mike Pontillo (mpontillo)]

Thanks for updating the tests and making the code more robust. It makes a little more sense to me now. Unfortunately, the log message raised more questions. =) See comment below.

review: Needs Information

[Newell Jensen (newell-jensen)]

See inline response. I agree that the message should definitely be more descriptive in terms of what is going on and will update it to give the user more information. In particular see what I say about (3) below.

[Mike Pontillo (mpontillo)]

Looks good; thanks for the fixes!

review: Approve

=== modified file 'src/provisioningserver/drivers/pod/rsd.py'
--- src/provisioningserver/drivers/pod/rsd.py	2017-06-05 17:31:23 +0000
+++ src/provisioningserver/drivers/pod/rsd.py	2017-06-07 00:15:32 +0000
@@ -474,12 +474,13 @@
         """Get pod machine local strorages."""
         local_drives = node_data.get('Links', {}).get('LocalDrives', [])
         for local_drive in local_drives:
+            local_drive_endpoint = local_drive['@odata.id']
             discovered_machine_block_device = (
                 DiscoveredMachineBlockDevice(
                     model='', serial='', size=0))
             drive_data, _ = yield self.redfish_request(
-                b"GET", join(url, local_drive[
-                    '@odata.id'].lstrip('/').encode('utf-8')), headers)
+                b"GET", join(url, local_drive_endpoint.lstrip(
+                    '/').encode('utf-8')), headers)
             discovered_machine_block_device.model = drive_data['Model']
             discovered_machine_block_device.serial = drive_data['SerialNumber']
             discovered_machine_block_device.size = float(
@@ -489,26 +490,33 @@
             # block devices. This ensures that the composed machine has the
             # requested tags on the block device.
             if request is not None:
-                # Iterate over all the request's block devices and pick first
+                # Iterate over all the request's block devices and pick
                 # device that has 'local' flag set with smallest adequate size.
                 chosen_block_device = None
                 smallest_size = discovered_machine_block_device.size
                 for block_device in request.block_devices:
                     if ('local' in block_device.tags and
-                            block_device.size >= smallest_size):
+                            smallest_size >= block_device.size):
                         smallest_size = block_device.size
                         chosen_block_device = block_device
                 if chosen_block_device is not None:
                     discovered_machine_block_device.tags = (
                         chosen_block_device.tags)
                     # Delete this from the request's block devices as it
-                    # is not longer needed.
+                    # is no longer needed.
                     request.block_devices.remove(chosen_block_device)
                 else:
                     # Log the fact that we did not find a chosen block device
-                    # to set the tags.
-                    maaslog.info(
-                        "Unable to set tags for local drive: %r" % local_drive)
+                    # to set the tags.  If the RSD is performing the way it
+                    # should, the user should not be seeing this as the
+                    # allocation process for composition should have failed.
+                    # Warn the user of this.
+                    maaslog.warning(
+                        "Requested disk size is larger than %d GiB, which "
+                        "drive '%r' contains.  RSD allocation should have "
+                        "failed.  Please report this to your RSD Pod "
+                        "administrator." % (
+                            smallest_size / (1024 ** 3), local_drive_endpoint))
 
             if 'local' not in discovered_machine_block_device.tags:
                 discovered_machine_block_device.tags.append('local')

=== modified file 'src/provisioningserver/drivers/pod/tests/test_rsd.py'
--- src/provisioningserver/drivers/pod/tests/test_rsd.py	2017-06-05 17:31:23 +0000
+++ src/provisioningserver/drivers/pod/tests/test_rsd.py	2017-06-07 00:15:32 +0000
@@ -1312,14 +1312,20 @@
         headers = driver.make_auth_headers(**context)
         request = make_requested_machine()
         # Set the tags on the requested machine's block devices
-        # and the size.
-        for idx in range(3):
-            request.block_devices[idx].size = 200 * 1024 ** 3
-            request.block_devices[idx].tags = ['local', 'testing tags']
+        # and the size.  First device will be a device that has
+        # its tags mapped, while the second one will be at a size
+        # that is bigger than the available local disk size so its
+        # tags will not be mapped.
+        request.block_devices[0].size = 100 * 1024 ** 3
+        request.block_devices[0].tags = ['local', 'tags mapped']
+        request.block_devices[1].size = 200 * 1024 ** 3
+        request.block_devices[1].tags = ['local', 'tags not mapped']
+        local_drive = "/redfish/v1/Systems/1/Adapters/3/Devices/3"
         node_data = SAMPLE_JSON_NODE
         discovered_machine = make_discovered_machine(block_devices=[])
         mock_redfish_request = self.patch(driver, 'redfish_request')
         mock_redfish_request.return_value = (SAMPLE_JSON_DEVICE, None)
+        self.patch_autospec(rsd_module.maaslog, 'warning')
 
         yield driver.get_pod_machine_local_storages(
             node_data, url, headers, discovered_machine, request)
@@ -1330,7 +1336,7 @@
                     serial=Equals('CVLI310601PY120E'),
                     size=Equals(119999999999.99997),
                     block_size=Equals(512),
-                    tags=Equals(['local', 'testing tags', 'ssd']),
+                    tags=Equals(['local', 'tags mapped', 'ssd']),
                     type=Equals(BlockDeviceType.PHYSICAL),
                 ),
                 MatchesStructure(
@@ -1338,9 +1344,16 @@
                     serial=Equals('CVLI310601PY120E'),
                     size=Equals(119999999999.99997),
                     block_size=Equals(512),
-                    tags=Equals(['local', 'testing tags', 'ssd']),
+                    tags=Equals(['local', 'ssd']),
                     type=Equals(BlockDeviceType.PHYSICAL),
                 )]))
+        self.assertThat(rsd_module.maaslog.warning, MockCalledOnceWith(
+            "Requested disk size is larger than %d GiB, which "
+            "drive '%r' contains.  RSD allocation should have "
+            "failed.  Please report this to your RSD Pod "
+            "administrator." % (
+                discovered_machine.block_devices[1].size / (1024 ** 3),
+                local_drive)))
 
     def test__get_pod_machine_remote_storages(self):
         driver = RSDPodDriver()