diff --git a/metalsmith/_cmd.py b/metalsmith/_cmd.py index 7a81471..e567149 100644 --- a/metalsmith/_cmd.py +++ b/metalsmith/_cmd.py @@ -71,14 +71,14 @@ def _do_deploy(api, args, formatter): conductor_group=args.conductor_group, capabilities=capabilities, traits=args.trait, - candidates=args.candidate) + candidates=args.candidate, + hostname=args.hostname) instance = api.provision_node(node, image=source, nics=args.nics, root_size_gb=args.root_size, swap_size_mb=args.swap_size, config=config, - hostname=args.hostname, netboot=args.netboot, wait=wait) formatter.deploy(instance) diff --git a/metalsmith/_config.py b/metalsmith/_config.py index 0b6a1ec..240ef15 100644 --- a/metalsmith/_config.py +++ b/metalsmith/_config.py @@ -60,13 +60,14 @@ class InstanceConfig(object): kwargs.setdefault('ssh_authorized_keys', self.ssh_keys) self.users.append(kwargs) - def build_configdrive(self, node, hostname): + def build_configdrive(self, node): """Make the config drive. :param node: `Node` object. - :param hostname: instance hostname. :return: configdrive contents as a base64-encoded string. """ + hostname = node.instance_info.get(_utils.HOSTNAME_FIELD) + # NOTE(dtantsur): CirrOS does not understand lists if isinstance(self.ssh_keys, list): ssh_keys = {str(i): v for i, v in enumerate(self.ssh_keys)} diff --git a/metalsmith/_instance.py b/metalsmith/_instance.py index eef0092..ef27e09 100644 --- a/metalsmith/_instance.py +++ b/metalsmith/_instance.py @@ -94,7 +94,7 @@ class Instance(object): @property def hostname(self): """Node's hostname.""" - return self._node.instance_info.get(_utils.GetNodeMixin.HOSTNAME_FIELD) + return self._node.instance_info.get(_utils.HOSTNAME_FIELD) def ip_addresses(self): """Returns IP addresses for this instance. diff --git a/metalsmith/_provisioner.py b/metalsmith/_provisioner.py index bcfe4dc..240ea50 100644 --- a/metalsmith/_provisioner.py +++ b/metalsmith/_provisioner.py @@ -34,7 +34,8 @@ LOG = logging.getLogger(__name__) _CREATED_PORTS = 'metalsmith_created_ports' _ATTACHED_PORTS = 'metalsmith_attached_ports' -_PRESERVE_INSTANCE_INFO_KEYS = {'capabilities', 'traits'} +_PRESERVE_INSTANCE_INFO_KEYS = {'capabilities', 'traits', + _utils.HOSTNAME_FIELD} class Provisioner(_utils.GetNodeMixin): @@ -67,7 +68,7 @@ class Provisioner(_utils.GetNodeMixin): def reserve_node(self, resource_class, conductor_group=None, capabilities=None, traits=None, candidates=None, - predicate=None): + predicate=None, hostname=None): """Find and reserve a suitable node. Example:: @@ -88,10 +89,13 @@ class Provisioner(_utils.GetNodeMixin): :param predicate: Custom predicate to run on nodes. A callable that accepts a node and returns ``True`` if it should be included, ``False`` otherwise. Any exceptions are propagated to the caller. + :param hostname: Hostname to assign to the instance. Defaults to the + node's name or UUID. :return: reserved `Node` object. :raises: :py:class:`metalsmith.exceptions.ReservationFailed` """ capabilities = capabilities or {} + self._check_hostname(hostname) if candidates: nodes = [self._get_node(node) for node in candidates] @@ -127,7 +131,8 @@ class Provisioner(_utils.GetNodeMixin): if traits: instance_info['traits'] = traits reserver = _scheduler.IronicReserver(self.connection, - instance_info) + instance_info, + hostname) node = _scheduler.schedule_node(nodes, filters, reserver, dry_run=self._dry_run) @@ -170,33 +175,25 @@ class Provisioner(_utils.GetNodeMixin): return node - def _check_hostname(self, node, hostname): + def _check_hostname(self, hostname, node=None): """Check the provided host name. - If the ``hostname`` is not provided, use either the name or the UUID, - whichever is appropriate for a host name. - - :return: appropriate hostname :raises: ValueError on inappropriate value of ``hostname`` """ if hostname is None: - if node.name and _utils.is_hostname_safe(node.name): - return node.name - else: - return node.id + return if not _utils.is_hostname_safe(hostname): raise ValueError("%s cannot be used as a hostname" % hostname) existing = self._find_node_by_hostname(hostname) - if existing is not None and existing.id != node.id: + if (existing is not None and node is not None + and existing.id != node.id): raise ValueError("The following node already uses hostname " "%(host)s: %(node)s" % {'host': hostname, 'node': _utils.log_res(existing)}) - return hostname - def provision_node(self, node, image, nics=None, root_size_gb=None, swap_size_mb=None, config=None, hostname=None, netboot=False, capabilities=None, traits=None, @@ -234,8 +231,8 @@ class Provisioner(_utils.GetNodeMixin): to specify it for a whole disk image. :param config: :py:class:`metalsmith.InstanceConfig` object with the configuration to pass to the instance. - :param hostname: Hostname to assign to the instance. Defaults to the - node's name or UUID. + :param hostname: Hostname to assign to the instance. If provided, + overrides the ``hostname`` passed to ``reserve_node``. :param netboot: Whether to use networking boot for final instances. :param capabilities: Requested capabilities of the node. If present, overwrites the capabilities set by :meth:`reserve_node`. @@ -261,7 +258,7 @@ class Provisioner(_utils.GetNodeMixin): nics = _nics.NICs(self.connection, node, nics) try: - hostname = self._check_hostname(node, hostname) + self._check_hostname(hostname, node=node) root_size_gb = _utils.get_root_disk(root_size_gb, node) image._validate(self.connection) @@ -283,7 +280,12 @@ class Provisioner(_utils.GetNodeMixin): instance_info = self._clean_instance_info(node.instance_info) instance_info['root_gb'] = root_size_gb instance_info['capabilities'] = capabilities - instance_info[self.HOSTNAME_FIELD] = hostname + if hostname: + instance_info[_utils.HOSTNAME_FIELD] = hostname + elif not instance_info.get(_utils.HOSTNAME_FIELD): + instance_info[_utils.HOSTNAME_FIELD] = _utils.default_hostname( + node) + extra = node.extra.copy() extra[_CREATED_PORTS] = nics.created_ports extra[_ATTACHED_PORTS] = nics.attached_ports @@ -303,10 +305,7 @@ class Provisioner(_utils.GetNodeMixin): LOG.debug('Generating a configdrive for node %s', _utils.log_res(node)) - cd = config.build_configdrive(node, hostname) - # TODO(dtantsur): move this to openstacksdk? - if not isinstance(cd, six.string_types): - cd = cd.decode('utf-8') + cd = config.build_configdrive(node) LOG.debug('Starting provisioning of node %s', _utils.log_res(node)) self.connection.baremetal.set_node_provision_state( node, 'active', config_drive=cd) diff --git a/metalsmith/_scheduler.py b/metalsmith/_scheduler.py index fd943b7..74350d0 100644 --- a/metalsmith/_scheduler.py +++ b/metalsmith/_scheduler.py @@ -261,10 +261,11 @@ class CustomPredicateFilter(Filter): class IronicReserver(Reserver): - def __init__(self, connection, instance_info=None): + def __init__(self, connection, instance_info=None, hostname=None): self._connection = connection self._failed_nodes = [] self._iinfo = instance_info or {} + self.hostname = hostname def validate(self, node): try: @@ -276,10 +277,17 @@ class IronicReserver(Reserver): LOG.warning(message) raise exceptions.ValidationFailed(message) + def _get_hostname(self, node): + if self.hostname is None: + return _utils.default_hostname(node) + else: + return self.hostname + def __call__(self, node): try: self.validate(node) iinfo = dict(node.instance_info or {}, **self._iinfo) + iinfo[_utils.HOSTNAME_FIELD] = self._get_hostname(node) return self._connection.baremetal.update_node( node, instance_id=node.id, instance_info=iinfo) except sdk_exc.SDKException: diff --git a/metalsmith/_utils.py b/metalsmith/_utils.py index 1aaccf7..ef039b9 100644 --- a/metalsmith/_utils.py +++ b/metalsmith/_utils.py @@ -107,11 +107,19 @@ class DuplicateHostname(sdk_exc.SDKException, exceptions.Error): pass +HOSTNAME_FIELD = 'metalsmith_hostname' + + +def default_hostname(node): + if node.name and is_hostname_safe(node.name): + return node.name + else: + return node.id + + class GetNodeMixin(object): """A helper mixin for getting nodes with hostnames.""" - HOSTNAME_FIELD = 'metalsmith_hostname' - _node_list = None def _available_nodes(self): @@ -128,7 +136,7 @@ class GetNodeMixin(object): """A helper to find a node by metalsmith hostname.""" nodes = self._node_list or self._nodes_for_lookup() existing = [n for n in nodes - if n.instance_info.get(self.HOSTNAME_FIELD) == hostname] + if n.instance_info.get(HOSTNAME_FIELD) == hostname] if len(existing) > 1: raise DuplicateHostname( "More than one node found with hostname %(host)s: %(nodes)s" % diff --git a/metalsmith/test/test_cmd.py b/metalsmith/test/test_cmd.py index 59235be..5dd9a77 100644 --- a/metalsmith/test/test_cmd.py +++ b/metalsmith/test/test_cmd.py @@ -46,7 +46,8 @@ class TestDeploy(testtools.TestCase): conductor_group=None, capabilities={}, traits=[], - candidates=None) + candidates=None, + hostname=None) reserve_defaults.update(reserve_args) provision_defaults = dict(image=mock.ANY, @@ -54,7 +55,6 @@ class TestDeploy(testtools.TestCase): root_size_gb=None, swap_size_mb=None, config=mock.ANY, - hostname=None, netboot=False, wait=1800) provision_defaults.update(provision_args) @@ -369,7 +369,7 @@ class TestDeploy(testtools.TestCase): args = ['deploy', '--network', 'mynet', '--image', 'myimg', '--hostname', 'host', '--resource-class', 'compute'] - self._check(mock_pr, args, {}, {'hostname': 'host'}) + self._check(mock_pr, args, {'hostname': 'host'}, {}) self.mock_print.assert_has_calls([ mock.call(mock.ANY, node='123', state='ACTIVE'), diff --git a/metalsmith/test/test_config.py b/metalsmith/test/test_config.py index 3ef6c14..4592f18 100644 --- a/metalsmith/test/test_config.py +++ b/metalsmith/test/test_config.py @@ -20,6 +20,7 @@ from openstack.baremetal import configdrive import testtools from metalsmith import _config +from metalsmith import _utils class TestInstanceConfig(testtools.TestCase): @@ -38,9 +39,11 @@ class TestInstanceConfig(testtools.TestCase): 'files': [], 'meta': {}} expected_m.update(expected_metadata) + self.node.instance_info = {_utils.HOSTNAME_FIELD: + expected_m.get('hostname')} with mock.patch.object(configdrive, 'build', autospec=True) as mb: - result = config.build_configdrive(self.node, "example.com") + result = config.build_configdrive(self.node) mb.assert_called_once_with(expected_m, mock.ANY) self.assertIs(result, mb.return_value) user_data = mb.call_args[0][1] diff --git a/metalsmith/test/test_provisioner.py b/metalsmith/test/test_provisioner.py index e4e7e63..c6ffc38 100644 --- a/metalsmith/test/test_provisioner.py +++ b/metalsmith/test/test_provisioner.py @@ -103,7 +103,9 @@ class TestReserveNode(Base): kwargs.setdefault('instance_id', None) kwargs.setdefault('is_maintenance', False) kwargs.setdefault('resource_class', self.RSC) - return mock.Mock(spec=NODE_FIELDS, **kwargs) + result = mock.Mock(spec=NODE_FIELDS, **kwargs) + result.name = kwargs.get('name') + return result def test_no_nodes(self): self.api.baremetal.nodes.return_value = [] @@ -120,7 +122,30 @@ class TestReserveNode(Base): self.assertIn(node, nodes) self.api.baremetal.update_node.assert_called_once_with( - node, instance_id=node.id, instance_info={}) + node, instance_id=node.id, + instance_info={'metalsmith_hostname': node.id}) + + def test_with_hostname(self): + nodes = [self._node()] + self.api.baremetal.nodes.return_value = nodes + + node = self.pr.reserve_node(self.RSC, hostname='example.com') + + self.assertIn(node, nodes) + self.api.baremetal.update_node.assert_called_once_with( + node, instance_id=node.id, + instance_info={'metalsmith_hostname': 'example.com'}) + + def test_with_name_as_hostname(self): + nodes = [self._node(name='example.com')] + self.api.baremetal.nodes.return_value = nodes + + node = self.pr.reserve_node(self.RSC) + + self.assertIn(node, nodes) + self.api.baremetal.update_node.assert_called_once_with( + node, instance_id=node.id, + instance_info={'metalsmith_hostname': 'example.com'}) def test_with_capabilities(self): nodes = [ @@ -135,7 +160,8 @@ class TestReserveNode(Base): self.assertIs(node, expected) self.api.baremetal.update_node.assert_called_once_with( node, instance_id=node.id, - instance_info={'capabilities': {'answer': '42'}}) + instance_info={'capabilities': {'answer': '42'}, + 'metalsmith_hostname': node.id}) def test_with_traits(self): nodes = [self._node(properties={'local_gb': 100}, traits=traits) @@ -149,7 +175,8 @@ class TestReserveNode(Base): self.assertIs(node, expected) self.api.baremetal.update_node.assert_called_once_with( node, instance_id=node.id, - instance_info={'traits': ['foo', 'answer:42']}) + instance_info={'traits': ['foo', 'answer:42'], + 'metalsmith_hostname': node.id}) def test_custom_predicate(self): nodes = [self._node(properties={'local_gb': i}) @@ -162,7 +189,8 @@ class TestReserveNode(Base): self.assertEqual(node, nodes[1]) self.api.baremetal.update_node.assert_called_once_with( - node, instance_id=node.id, instance_info={}) + node, instance_id=node.id, + instance_info={'metalsmith_hostname': node.id}) def test_custom_predicate_false(self): nodes = [self._node() for _ in range(3)] @@ -184,7 +212,8 @@ class TestReserveNode(Base): self.assertEqual(node, nodes[0]) self.assertFalse(self.api.baremetal.nodes.called) self.api.baremetal.update_node.assert_called_once_with( - node, instance_id=node.id, instance_info={}) + node, instance_id=node.id, + instance_info={'metalsmith_hostname': node.id}) def test_provided_nodes(self): nodes = [self._node(), self._node()] @@ -194,7 +223,8 @@ class TestReserveNode(Base): self.assertEqual(node, nodes[0]) self.assertFalse(self.api.baremetal.nodes.called) self.api.baremetal.update_node.assert_called_once_with( - node, instance_id=node.id, instance_info={}) + node, instance_id=node.id, + instance_info={'metalsmith_hostname': node.id}) def test_nodes_filtered(self): nodes = [self._node(resource_class='banana'), @@ -210,7 +240,8 @@ class TestReserveNode(Base): self.assertFalse(self.api.baremetal.nodes.called) self.api.baremetal.update_node.assert_called_once_with( node, instance_id=node.id, - instance_info={'capabilities': {'cat': 'meow'}}) + instance_info={'capabilities': {'cat': 'meow'}, + 'metalsmith_hostname': node.id}) def test_nodes_filtered_by_conductor_group(self): nodes = [self._node(conductor_group='loc1'), @@ -230,7 +261,8 @@ class TestReserveNode(Base): self.assertFalse(self.api.baremetal.nodes.called) self.api.baremetal.update_node.assert_called_once_with( node, instance_id=node.id, - instance_info={'capabilities': {'cat': 'meow'}}) + instance_info={'capabilities': {'cat': 'meow'}, + 'metalsmith_hostname': node.id}) def test_provided_nodes_no_match(self): nodes = [ @@ -262,7 +294,7 @@ class TestProvisionNode(Base): 'image_source': self.image.id, 'root_gb': 99, # 100 - 1 'capabilities': {'boot_option': 'local'}, - _utils.GetNodeMixin.HOSTNAME_FIELD: 'control-0' + _utils.HOSTNAME_FIELD: 'control-0' } self.extra = { 'metalsmith_created_ports': [ @@ -291,8 +323,7 @@ class TestProvisionNode(Base): self.api.baremetal.update_node.assert_called_once_with( self.node, instance_info=self.instance_info, extra=self.extra) self.api.baremetal.validate_node.assert_called_once_with(self.node) - self.configdrive_mock.assert_called_once_with(mock.ANY, self.node, - self.node.name) + self.configdrive_mock.assert_called_once_with(mock.ANY, self.node) self.api.baremetal.set_node_provision_state.assert_called_once_with( self.node, 'active', config_drive=mock.ANY) self.assertFalse(self.api.network.delete_port.called) @@ -346,8 +377,7 @@ class TestProvisionNode(Base): self.assertEqual(inst.uuid, self.node.id) self.assertEqual(inst.node, self.node) - config.build_configdrive.assert_called_once_with( - self.node, self.node.name) + config.build_configdrive.assert_called_once_with(self.node) self.api.network.create_port.assert_called_once_with( network_id=self.api.network.find_network.return_value.id) self.api.baremetal.attach_vif_to_node.assert_called_once_with( @@ -369,7 +399,7 @@ class TestProvisionNode(Base): inst = self.pr.provision_node(self.node, 'image', [{'network': 'network'}], hostname=hostname) - self.instance_info[_utils.GetNodeMixin.HOSTNAME_FIELD] = hostname + self.instance_info[_utils.HOSTNAME_FIELD] = hostname self.assertEqual(inst.uuid, self.node.id) self.assertEqual(inst.node, self.node) @@ -381,8 +411,31 @@ class TestProvisionNode(Base): self.api.baremetal.update_node.assert_called_once_with( self.node, instance_info=self.instance_info, extra=self.extra) self.api.baremetal.validate_node.assert_called_once_with(self.node) - self.configdrive_mock.assert_called_once_with(mock.ANY, self.node, - hostname) + self.configdrive_mock.assert_called_once_with(mock.ANY, self.node) + self.api.baremetal.set_node_provision_state.assert_called_once_with( + self.node, 'active', config_drive=mock.ANY) + self.assertFalse( + self.api.baremetal.wait_for_nodes_provision_state.called) + self.assertFalse(self.api.network.delete_port.called) + + def test_existing_hostname(self): + hostname = 'control-0.example.com' + self.node.instance_info[_utils.HOSTNAME_FIELD] = hostname + inst = self.pr.provision_node(self.node, 'image', + [{'network': 'network'}]) + self.instance_info[_utils.HOSTNAME_FIELD] = hostname + + self.assertEqual(inst.uuid, self.node.id) + self.assertEqual(inst.node, self.node) + + self.api.network.create_port.assert_called_once_with( + network_id=self.api.network.find_network.return_value.id) + self.api.baremetal.attach_vif_to_node.assert_called_once_with( + self.node, self.api.network.create_port.return_value.id) + self.api.baremetal.update_node.assert_called_once_with( + self.node, instance_info=self.instance_info, extra=self.extra) + self.api.baremetal.validate_node.assert_called_once_with(self.node) + self.configdrive_mock.assert_called_once_with(mock.ANY, self.node) self.api.baremetal.set_node_provision_state.assert_called_once_with( self.node, 'active', config_drive=mock.ANY) self.assertFalse( @@ -393,7 +446,7 @@ class TestProvisionNode(Base): self.node.name = 'node_1' inst = self.pr.provision_node(self.node, 'image', [{'network': 'network'}]) - self.instance_info[_utils.GetNodeMixin.HOSTNAME_FIELD] = '000' + self.instance_info[_utils.HOSTNAME_FIELD] = '000' self.assertEqual(inst.uuid, self.node.id) self.assertEqual(inst.node, self.node) diff --git a/metalsmith/test/test_scheduler.py b/metalsmith/test/test_scheduler.py index 4299ab9..66262a6 100644 --- a/metalsmith/test/test_scheduler.py +++ b/metalsmith/test/test_scheduler.py @@ -204,6 +204,8 @@ class TestIronicReserver(testtools.TestCase): super(TestIronicReserver, self).setUp() self.node = mock.Mock(spec=['id', 'name', 'instance_info'], instance_info={}) + self.node.id = 'abcd' + self.node.name = None self.api = mock.Mock(spec=['baremetal']) self.api.baremetal = mock.Mock(spec=['update_node', 'validate_node']) self.api.baremetal.update_node.side_effect = ( @@ -220,7 +222,27 @@ class TestIronicReserver(testtools.TestCase): self.api.baremetal.validate_node.assert_called_with( self.node, required=('power', 'management')) self.api.baremetal.update_node.assert_called_once_with( - self.node, instance_id=self.node.id, instance_info={}) + self.node, instance_id=self.node.id, + instance_info={'metalsmith_hostname': 'abcd'}) + + def test_name_as_hostname(self): + self.node.name = 'example.com' + self.assertEqual(self.node, self.reserver(self.node)) + self.api.baremetal.validate_node.assert_called_with( + self.node, required=('power', 'management')) + self.api.baremetal.update_node.assert_called_once_with( + self.node, instance_id=self.node.id, + instance_info={'metalsmith_hostname': 'example.com'}) + + def test_name_cannot_be_hostname(self): + # This should not ever happen, but checking just in case + self.node.name = 'banana!' + self.assertEqual(self.node, self.reserver(self.node)) + self.api.baremetal.validate_node.assert_called_with( + self.node, required=('power', 'management')) + self.api.baremetal.update_node.assert_called_once_with( + self.node, instance_id=self.node.id, + instance_info={'metalsmith_hostname': 'abcd'}) def test_with_instance_info(self): self.reserver = _scheduler.IronicReserver(self.api, @@ -230,7 +252,17 @@ class TestIronicReserver(testtools.TestCase): self.node, required=('power', 'management')) self.api.baremetal.update_node.assert_called_once_with( self.node, instance_id=self.node.id, - instance_info={'cat': 'meow'}) + instance_info={'cat': 'meow', 'metalsmith_hostname': 'abcd'}) + + def test_with_hostname(self): + self.reserver = _scheduler.IronicReserver(self.api, + hostname='example.com') + self.assertEqual(self.node, self.reserver(self.node)) + self.api.baremetal.validate_node.assert_called_with( + self.node, required=('power', 'management')) + self.api.baremetal.update_node.assert_called_once_with( + self.node, instance_id=self.node.id, + instance_info={'metalsmith_hostname': 'example.com'}) def test_reservation_failed(self): self.api.baremetal.update_node.side_effect = ( @@ -240,7 +272,8 @@ class TestIronicReserver(testtools.TestCase): self.api.baremetal.validate_node.assert_called_with( self.node, required=('power', 'management')) self.api.baremetal.update_node.assert_called_once_with( - self.node, instance_id=self.node.id, instance_info={}) + self.node, instance_id=self.node.id, + instance_info={'metalsmith_hostname': 'abcd'}) def test_validation_failed(self): self.api.baremetal.validate_node.side_effect = ( diff --git a/releasenotes/notes/reserve-hostname-85d02321156bde3b.yaml b/releasenotes/notes/reserve-hostname-85d02321156bde3b.yaml new file mode 100644 index 0000000..3567539 --- /dev/null +++ b/releasenotes/notes/reserve-hostname-85d02321156bde3b.yaml @@ -0,0 +1,4 @@ +--- +features: + - | + The ``reserve_node`` call now also accepts ``hostname``.