From 0f1102b3fd418a3d5b35be5ef1deb6a3eb0ba51b Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Mon, 28 Jan 2019 11:49:51 +0100 Subject: [PATCH] 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 | 2 +- tripleo_common/actions/baremetal_deploy.py | 84 +++++++------------ .../tests/actions/test_baremetal_deploy.py | 53 ++++++++++-- 3 files changed, 77 insertions(+), 62 deletions(-) diff --git a/requirements.txt b/requirements.txt index 7a17e7132..6628c2fca 100644 --- a/requirements.txt +++ b/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 diff --git a/tripleo_common/actions/baremetal_deploy.py b/tripleo_common/actions/baremetal_deploy.py index 64dc194e6..f5a929921 100644 --- a/tripleo_common/actions/baremetal_deploy.py +++ b/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 - # is bumped to 0.9.0. - except Exception: + # TODO(dtantsur): replace Error with a specific exception + except (sdk_exc.ResourceNotFound, metalsmith.exceptions.Error): 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: - 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}) + 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): - if image.startswith('http://') or image.startswith('https://'): - return 'http' - elif image.startswith('file://'): - return 'file' - else: - return 'glance' +def _get_source(instance): + return sources.detect(image=instance.get('image'), + kernel=instance.get('image_kernel'), + ramdisk=instance.get('image_ramdisk'), + checksum=instance.get('image_checksum')) diff --git a/tripleo_common/tests/actions/test_baremetal_deploy.py b/tripleo_common/tests/actions/test_baremetal_deploy.py index ec7780c36..7ed0a76d0 100644 --- a/tripleo_common/tests/actions/test_baremetal_deploy.py +++ b/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):