nova-less-deploy: updates for metalsmith >= 0.9.0

* Replace image detection with the call from metalsmith
* Validate image parameters early to detect trivial errors
* Add support for specifying subnets in NICs
* Use more specific exceptions when checking for existing instances
* Drop compatibility code for nodes in < 0.9.0

Change-Id: I719082c0845b517172c309838e37a5e38a04c04c
Implements: blueprint nova-less-deploy

requirements.txt

@@ -28,5 +28,5 @@ python-keystoneclient>=3.8.0 # Apache-2.0
 keystoneauth1>=3.4.0 # Apache-2.0
 tenacity>=4.4.0 # Apache-2.0
 futures>=3.0.0;python_version=='2.7' or python_version=='2.6'  # BSD
-metalsmith>=0.8.0 # Apache-2.0
+metalsmith>=0.9.0 # Apache-2.0
 jsonschema<3.0.0,>=2.6.0 # MIT

tripleo_common/actions/baremetal_deploy.py

@@ -18,6 +18,7 @@ import jsonschema
 import metalsmith
 from metalsmith import sources
 from mistral_lib import actions
+from openstack import exceptions as sdk_exc
 import six
 
 from tripleo_common.actions import base
@@ -51,6 +52,7 @@ _INSTANCES_INPUT_SCHEMA = {
                                    'network': {'type': 'string'},
                                    'port': {'type': 'string'},
                                    'fixed_ip': {'type': 'string'},
+                                   'subnet': {'type': 'string'},
                                },
                                'additionalProperties': False}},
             'profile': {'type': 'string'},
@@ -89,10 +91,16 @@ class CheckExistingInstancesAction(base.TripleOAction):
         for request in self.instances:
             try:
                 instance = provisioner.show_instance(request['hostname'])
-            # TODO(dtantsur): use openstacksdk exceptions when metalsmith
+            # TODO(dtantsur): replace Error with a specific exception
-            # is bumped to 0.9.0.
+            except (sdk_exc.ResourceNotFound, metalsmith.exceptions.Error):
-            except Exception:
                 not_found.append(request)
+            except Exception as exc:
+                message = ('Failed to request instance information for '
+                           'hostname %s' % request['hostname'])
+                LOG.exception(message)
+                return actions.Result(
+                    error="%s. %s: %s" % (message, type(exc).__name__, exc)
+                )
             else:
                 # NOTE(dtantsur): metalsmith can match instances by node names,
                 # provide a safeguard to avoid conflicts.
@@ -166,13 +174,7 @@ class ReserveNodesAction(base.TripleOAction):
                     traits=instance.get('traits'))
                 LOG.info('Reserved node %s for instance %s', node, instance)
                 nodes.append(node)
-                try:
+                result.append({'node': node.id, 'instance': instance})
-                    node_id = node.id
-                except AttributeError:
-                    # TODO(dtantsur): transition from ironicclient to
-                    # openstacksdk, remove when metalsmith is bumped to 0.9.0
-                    node_id = node.uuid
-                result.append({'node': node_id, 'instance': instance})
         except Exception as exc:
             LOG.exception('Provisioning failed, cleaning up')
             # Remove all reservations on failure
@@ -204,45 +206,10 @@ class DeployNodeAction(base.TripleOAction):
         self.default_network = default_network
         self.default_root_size = default_root_size
 
-    def _get_image(self):
-        # TODO(dtantsur): move this logic to metalsmith in 0.9.0
-        image = self.instance.get('image', self.default_image)
-        image_type = _link_type(image)
-        if image_type == 'glance':
-            return sources.GlanceImage(image)
-        else:
-            checksum = self.instance.get('image_checksum')
-            if (checksum and image_type == 'http' and
-                    _link_type(checksum) == 'http'):
-                kwargs = {'checksum_url': checksum}
-            else:
-                kwargs = {'checksum': checksum}
-
-            whole_disk_image = not (self.instance.get('image_kernel') or
-                                    self.instance.get('image_ramdisk'))
-
-            if whole_disk_image:
-                if image_type == 'http':
-                    return sources.HttpWholeDiskImage(image, **kwargs)
-                else:
-                    return sources.FileWholeDiskImage(image, **kwargs)
-            else:
-                if image_type == 'http':
-                    return sources.HttpPartitionImage(
-                        image,
-                        kernel_url=self.instance.get('image_kernel'),
-                        ramdisk_url=self.instance.get('image_ramdisk'),
-                        **kwargs)
-                else:
-                    return sources.FilePartitionImage(
-                        image,
-                        kernel_location=self.instance.get('image_kernel'),
-                        ramdisk_location=self.instance.get('image_ramdisk'),
-                        **kwargs)
-
     def run(self, context):
         try:
-            _validate_instances([self.instance])
+            _validate_instances([self.instance],
+                                default_image=self.default_image)
         except Exception as exc:
             LOG.error('Failed to validate the request. %s', exc)
             return actions.Result(error=six.text_type(exc))
@@ -252,11 +219,12 @@ class DeployNodeAction(base.TripleOAction):
         LOG.debug('Starting provisioning of %s on node %s',
                   self.instance, self.node)
         try:
+            image = _get_source(self.instance)
             instance = provisioner.provision_node(
                 self.node,
                 config=self.config,
                 hostname=self.instance['hostname'],
-                image=self._get_image(),
+                image=image,
                 nics=self.instance.get('nics',
                                        [{'network': self.default_network}]),
                 root_size_gb=self.instance.get('root_size_gb',
@@ -327,16 +295,22 @@ class UndeployInstanceAction(base.TripleOAction):
         LOG.info('Successfully unprovisioned %s', instance.hostname)
 
 
-def _validate_instances(instances):
+def _validate_instances(instances, default_image='overcloud-full'):
     for inst in instances:
         if inst.get('name') and not inst.get('hostname'):
             inst['hostname'] = inst['name']
 
+        # Set the default image so that the source validation can succeed.
+        inst.setdefault('image', default_image)
+
     jsonschema.validate(instances, _INSTANCES_INPUT_SCHEMA)
 
     hostnames = set()
     names = set()
     for inst in instances:
+        # NOTE(dtantsur): validate image parameters
+        _get_source(inst)
+
         if inst['hostname'] in hostnames:
             raise ValueError('Hostname %s is used more than once' %
                              inst['hostname'])
@@ -360,10 +334,8 @@ def _release_nodes(provisioner, nodes):
             LOG.info('Removed reservation from node %s', node)
 
 
-def _link_type(image):
+def _get_source(instance):
-    if image.startswith('http://') or image.startswith('https://'):
+    return sources.detect(image=instance.get('image'),
-        return 'http'
+                          kernel=instance.get('image_kernel'),
-    elif image.startswith('file://'):
+                          ramdisk=instance.get('image_ramdisk'),
-        return 'file'
+                          checksum=instance.get('image_checksum'))
-    else:
-        return 'glance'

tripleo_common/tests/actions/test_baremetal_deploy.py

@@ -12,8 +12,10 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import metalsmith
 from metalsmith import sources
 import mock
+from openstack import exceptions as sdk_exc
 
 from tripleo_common.actions import baremetal_deploy
 from tripleo_common.tests import base
@@ -195,6 +197,34 @@ class TestDeployNode(base.TestCase):
         self.assertEqual([], config.ssh_keys)
         self.assertEqual('heat-admin', config.users[0]['name'])
 
+    def test_success_advanced_nic(self, mock_pr):
+        action = baremetal_deploy.DeployNodeAction(
+            instance={'hostname': 'host1',
+                      'nics': [{'subnet': 'ctlplane-subnet'},
+                               {'network': 'ctlplane',
+                                'fixed_ip': '10.0.0.2'}]},
+            node='1234'
+        )
+        result = action.run(mock.Mock())
+
+        pr = mock_pr.return_value
+        self.assertEqual(
+            pr.provision_node.return_value.to_dict.return_value,
+            result)
+        pr.provision_node.assert_called_once_with(
+            '1234',
+            image=mock.ANY,
+            nics=[{'subnet': 'ctlplane-subnet'},
+                  {'network': 'ctlplane', 'fixed_ip': '10.0.0.2'}],
+            hostname='host1',
+            root_size_gb=49,
+            swap_size_mb=None,
+            config=mock.ANY,
+        )
+        config = pr.provision_node.call_args[1]['config']
+        self.assertEqual([], config.ssh_keys)
+        self.assertEqual('heat-admin', config.users[0]['name'])
+
     def test_success(self, mock_pr):
         pr = mock_pr.return_value
         action = baremetal_deploy.DeployNodeAction(
@@ -228,8 +258,6 @@ class TestDeployNode(base.TestCase):
         self.assertIsInstance(source, sources.GlanceImage)
         # TODO(dtantsur): check the image when it's a public field
 
-    # NOTE(dtantsur): limited coverage for source detection since this code is
-    # being moved to metalsmith in 0.9.0.
     def test_success_http_partition_image(self, mock_pr):
         action = baremetal_deploy.DeployNodeAction(
             instance={'hostname': 'host1',
@@ -363,12 +391,14 @@ class TestCheckExistingInstances(base.TestCase):
         pr = mock_pr.return_value
         instances = [
             {'hostname': 'host1'},
+            {'hostname': 'host3'},
             {'hostname': 'host2', 'resource_class': 'compute',
              'capabilities': {'answer': '42'}}
         ]
         existing = mock.MagicMock(hostname='host2')
         pr.show_instance.side_effect = [
-            RuntimeError('not found'),
+            sdk_exc.ResourceNotFound(""),
+            metalsmith.exceptions.Error(""),
             existing,
         ]
         action = baremetal_deploy.CheckExistingInstancesAction(instances)
@@ -376,10 +406,11 @@ class TestCheckExistingInstances(base.TestCase):
 
         self.assertEqual({
             'instances': [existing.to_dict.return_value],
-            'not_found': [{'hostname': 'host1'}]
+            'not_found': [{'hostname': 'host1', 'image': 'overcloud-full'},
+                          {'hostname': 'host3', 'image': 'overcloud-full'}]
         }, result)
         pr.show_instance.assert_has_calls([
-            mock.call('host1'), mock.call('host2')
+            mock.call(host) for host in ['host1', 'host3', 'host2']
         ])
 
     def test_missing_hostname(self, mock_pr):
@@ -404,6 +435,18 @@ class TestCheckExistingInstances(base.TestCase):
         self.assertIn("hostname host1 was not found", result.error)
         mock_pr.return_value.show_instance.assert_called_once_with('host1')
 
+    def test_unexpected_error(self, mock_pr):
+        instances = [
+            {'hostname': 'host%d' % i} for i in range(3)
+        ]
+        mock_pr.return_value.show_instance.side_effect = RuntimeError('boom')
+        action = baremetal_deploy.CheckExistingInstancesAction(instances)
+        result = action.run(mock.Mock())
+
+        self.assertIn("hostname host0", result.error)
+        self.assertIn("RuntimeError: boom", result.error)
+        mock_pr.return_value.show_instance.assert_called_once_with('host0')
+
 
 @mock.patch.object(baremetal_deploy, '_provisioner', autospec=True)
 class TestWaitForDeployment(base.TestCase):