Accept hostname in reserve_node in addition to provision_node

With the allocation API we will need to provide the hostname as
the allocation name. Thus, we have to do it earlier.

Change-Id: I8afd8af23ad929fd9768e95a82fecd114fdcbfd9

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)

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)}

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.

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
-                return node.name
-            else:
-                return node.id
 
         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
+        :param hostname: Hostname to assign to the instance. If provided,
-            node's name or UUID.
+            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)
+            cd = config.build_configdrive(node)
-            # TODO(dtantsur): move this to openstacksdk?
-            if not isinstance(cd, six.string_types):
-                cd = cd.decode('utf-8')
             LOG.debug('Starting provisioning of node %s', _utils.log_res(node))
             self.connection.baremetal.set_node_provision_state(
                 node, 'active', config_drive=cd)

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:

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" %

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'),

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]

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.configdrive_mock.assert_called_once_with(mock.ANY, self.node)
-                                                      self.node.name)
         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(
+        config.build_configdrive.assert_called_once_with(self.node)
-            self.node, self.node.name)
         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,
+        self.configdrive_mock.assert_called_once_with(mock.ANY, self.node)
-                                                      hostname)
+        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)

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 = (

releasenotes/notes/reserve-hostname-85d02321156bde3b.yaml

@@ -0,0 +1,4 @@
+---
+features:
+  - |
+    The ``reserve_node`` call now also accepts ``hostname``.