Ironic: Lightweight fetching of nodes

The Ironic API version >= 1.8 supports fetching only a small number of
a resource instead of it's full representation. This patch is pinning
the API version for Ironic at 1.8 so the nova driver can benefit from
this feature by fetching only the required fields for the deployment,
making it more lightweight and improving the readability of the logs.

The configuration option "api_version" was marked as deprecated and
should be removed in the future. The only possible value for that
configuration was "1" (because Ironic only have 1 API version) and the
ironic team came to an agreement that setting the API version via
configuration option should not be supported anymore.

Closes-Bug: #1458934
Change-Id: Iebfa06da6811889e11fd4edf15d47ca4e0feea6f

nova/conf/ironic.py

@@ -22,7 +22,9 @@
 api_version = cfg.IntOpt(
     'api_version',
     default=1,
-    help='Version of Ironic API service endpoint.')
+    deprecated_for_removal=True,
+    help='Version of Ironic API service endpoint. '
+         'DEPRECATED: Setting the API version is not possible anymore.')
 
 api_endpoint = cfg.StrOpt(
     'api_endpoint',

nova/tests/unit/virt/ironic/test_client_wrapper.py

@@ -73,7 +73,8 @@ def test__get_client_no_auth_token(self, mock_ir_cli):
                     'os_endpoint_type': 'public',
                     'ironic_url': CONF.ironic.api_endpoint,
                     'max_retries': CONF.ironic.api_max_retries,
-                    'retry_interval': CONF.ironic.api_retry_interval}
+                    'retry_interval': CONF.ironic.api_retry_interval,
+                    'os_ironic_api_version': '1.8'}
         mock_ir_cli.assert_called_once_with(CONF.ironic.api_version,
                                             **expected)
 
@@ -86,7 +87,8 @@ def test__get_client_with_auth_token(self, mock_ir_cli):
         expected = {'os_auth_token': 'fake-token',
                     'ironic_url': CONF.ironic.api_endpoint,
                     'max_retries': CONF.ironic.api_max_retries,
-                    'retry_interval': CONF.ironic.api_retry_interval}
+                    'retry_interval': CONF.ironic.api_retry_interval,
+                    'os_ironic_api_version': '1.8'}
         mock_ir_cli.assert_called_once_with(CONF.ironic.api_version,
                                             **expected)
 

nova/tests/unit/virt/ironic/test_driver.py

@@ -526,7 +526,8 @@ def test_instance_exists(self, mock_call):
                                                    uuid=self.instance_uuid)
         self.assertTrue(self.driver.instance_exists(instance))
         mock_call.assert_called_once_with('node.get_by_instance_uuid',
-                                          self.instance_uuid)
+                                          self.instance_uuid,
+                                          fields=ironic_driver._NODE_FIELDS)
 
     @mock.patch.object(cw.IronicClientWrapper, 'call')
     def test_instance_exists_fail(self, mock_call):
@@ -535,7 +536,8 @@ def test_instance_exists_fail(self, mock_call):
                                                    uuid=self.instance_uuid)
         self.assertFalse(self.driver.instance_exists(instance))
         mock_call.assert_called_once_with('node.get_by_instance_uuid',
-                                          self.instance_uuid)
+                                          self.instance_uuid,
+                                          fields=ironic_driver._NODE_FIELDS)
 
     @mock.patch.object(cw.IronicClientWrapper, 'call')
     @mock.patch.object(objects.Instance, 'get_by_uuid')
@@ -591,7 +593,8 @@ def test_node_is_available_empty_cache_empty_list(self, mock_get,
         mock_get.return_value = node
         mock_list.return_value = []
         self.assertTrue(self.driver.node_is_available(node.uuid))
-        mock_get.assert_called_with(node.uuid)
+        mock_get.assert_called_with(node.uuid,
+                                    fields=ironic_driver._NODE_FIELDS)
         mock_list.assert_called_with(detail=True, limit=0)
 
         mock_get.side_effect = ironic_exception.NotFound
@@ -722,7 +725,8 @@ def test_get_available_resource(self, mock_nr, mock_list, mock_get):
         result = self.driver.get_available_resource(node.uuid)
         self.assertEqual(fake_resource, result)
         mock_nr.assert_called_once_with(node)
-        mock_get.assert_called_once_with(node.uuid)
+        mock_get.assert_called_once_with(node.uuid,
+                                         fields=ironic_driver._NODE_FIELDS)
 
     @mock.patch.object(FAKE_CLIENT.node, 'get')
     @mock.patch.object(FAKE_CLIENT.node, 'list')
@@ -823,7 +827,8 @@ def _test_spawn(self, mock_sf, mock_pvifs, mock_adf, mock_wait_active,
 
         self.driver.spawn(self.ctx, instance, image_meta, [], None)
 
-        mock_node.get.assert_called_once_with(node_uuid)
+        mock_node.get.assert_called_once_with(
+            node_uuid, fields=ironic_driver._NODE_FIELDS)
         mock_node.validate.assert_called_once_with(node_uuid)
         mock_adf.assert_called_once_with(node, instance,
                                          test.MatchType(objects.ImageMeta),
@@ -1020,7 +1025,8 @@ def test_spawn_node_driver_validation_fail(self, mock_node,
 
         self.assertRaises(exception.ValidationError, self.driver.spawn,
                           self.ctx, instance, image_meta, [], None)
-        mock_node.get.assert_called_once_with(node_uuid)
+        mock_node.get.assert_called_once_with(
+            node_uuid, fields=ironic_driver._NODE_FIELDS)
         mock_node.validate.assert_called_once_with(node_uuid)
 
     @mock.patch.object(configdrive, 'required_by')
@@ -1048,7 +1054,8 @@ class TestException(Exception):
         self.assertRaises(TestException, self.driver.spawn,
                           self.ctx, instance, image_meta, [], None)
 
-        mock_node.get.assert_called_once_with(node_uuid)
+        mock_node.get.assert_called_once_with(
+            node_uuid, fields=ironic_driver._NODE_FIELDS)
         mock_node.validate.assert_called_once_with(node_uuid)
         mock_cleanup_deploy.assert_called_with(self.ctx, node, instance, None,
                                                flavor=flavor)
@@ -1076,7 +1083,8 @@ def test_spawn_node_trigger_deploy_fail(self, mock_cleanup_deploy,
         self.assertRaises(exception.NovaException, self.driver.spawn,
                           self.ctx, instance, image_meta, [], None)
 
-        mock_node.get.assert_called_once_with(node_uuid)
+        mock_node.get.assert_called_once_with(
+            node_uuid, fields=ironic_driver._NODE_FIELDS)
         mock_node.validate.assert_called_once_with(node_uuid)
         mock_cleanup_deploy.assert_called_once_with(self.ctx, node,
                                                     instance, None,
@@ -1105,7 +1113,8 @@ def test_spawn_node_trigger_deploy_fail2(self, mock_cleanup_deploy,
                           self.driver.spawn,
                           self.ctx, instance, image_meta, [], None)
 
-        mock_node.get.assert_called_once_with(node_uuid)
+        mock_node.get.assert_called_once_with(
+            node_uuid, fields=ironic_driver._NODE_FIELDS)
         mock_node.validate.assert_called_once_with(node_uuid)
         mock_cleanup_deploy.assert_called_once_with(self.ctx, node,
                                                     instance, None,
@@ -1184,7 +1193,8 @@ def fake_set_provision_state(*_):
         mock_node.set_provision_state.side_effect = fake_set_provision_state
         self.driver.destroy(self.ctx, instance, network_info, None)
 
-        mock_node.get_by_instance_uuid.assert_called_with(instance.uuid)
+        mock_node.get_by_instance_uuid.assert_called_with(
+            instance.uuid, fields=ironic_driver._NODE_FIELDS)
         mock_cleanup_deploy.assert_called_with(self.ctx, node,
                                                instance, network_info)
 
@@ -1279,7 +1289,8 @@ def test_destroy_unassociate_fail(self, mock_node):
                           self.ctx, instance, None, None)
         mock_node.set_provision_state.assert_called_once_with(node_uuid,
                                                               'deleted')
-        mock_node.get_by_instance_uuid.assert_called_with(instance.uuid)
+        mock_node.get_by_instance_uuid.assert_called_with(
+            instance.uuid, fields=ironic_driver._NODE_FIELDS)
 
     @mock.patch.object(loopingcall, 'FixedIntervalLoopingCall')
     @mock.patch.object(ironic_driver, '_validate_instance_and_node')
@@ -1364,7 +1375,8 @@ def test_plug_vifs(self, mock__plug_vifs, mock_get):
         network_info = utils.get_test_network_info()
         self.driver.plug_vifs(instance, network_info)
 
-        mock_get.assert_called_once_with(node_uuid)
+        mock_get.assert_called_once_with(node_uuid,
+                                         fields=ironic_driver._NODE_FIELDS)
         mock__plug_vifs.assert_called_once_with(node, instance, network_info)
 
     @mock.patch.object(FAKE_CLIENT.port, 'update')
@@ -1433,7 +1445,8 @@ def test_unplug_vifs(self, mock_node, mock_update):
                                 utils.get_test_network_info())
 
         # asserts
-        mock_node.get.assert_called_once_with(node_uuid)
+        mock_node.get.assert_called_once_with(
+            node_uuid, fields=ironic_driver._NODE_FIELDS)
         mock_node.list_ports.assert_called_once_with(node_uuid, detail=True)
         mock_update.assert_called_once_with(port.uuid, expected_patch)
 
@@ -1449,7 +1462,8 @@ def test_unplug_vifs_port_not_associated(self, mock_node, mock_update):
         instance = fake_instance.fake_instance_obj(self.ctx, node=node_uuid)
         self.driver.unplug_vifs(instance, utils.get_test_network_info())
 
-        mock_node.get.assert_called_once_with(node_uuid)
+        mock_node.get.assert_called_once_with(
+            node_uuid, fields=ironic_driver._NODE_FIELDS)
         mock_node.list_ports.assert_called_once_with(node_uuid, detail=True)
         # assert port.update() was not called
         self.assertFalse(mock_update.called)

nova/tests/unit/virt/ironic/utils.py

@@ -92,10 +92,10 @@ class FakeNodeClient(object):
     def list(self, detail=False):
         return []
 
-    def get(self, node_uuid):
+    def get(self, node_uuid, fields=None):
         pass
 
-    def get_by_instance_uuid(self, instance_uuid):
+    def get_by_instance_uuid(self, instance_uuid, fields=None):
         pass
 
     def list_ports(self, node_uuid):

nova/virt/ironic/client_wrapper.py

@@ -30,6 +30,9 @@
 
 ironic = None
 
+# The API version required by the Ironic driver
+IRONIC_API_VERSION = (1, 8)
+
 
 class IronicClientWrapper(object):
     """Ironic client wrapper class that encapsulates retry logic."""
@@ -82,8 +85,9 @@ def _get_client(self, retry_on_conflict=True):
         # Retries for Conflict exception
         kwargs['max_retries'] = max_retries
         kwargs['retry_interval'] = retry_interval
+        kwargs['os_ironic_api_version'] = '%d.%d' % IRONIC_API_VERSION
         try:
-            cli = ironic.client.get_client(CONF.ironic.api_version, **kwargs)
+            cli = ironic.client.get_client(IRONIC_API_VERSION[0], **kwargs)
             # Cache the client so we don't have to reconstruct and
             # reauthenticate it every time we need it.
             if retry_on_conflict:

nova/virt/ironic/driver.py

@@ -74,6 +74,10 @@
                        ironic_states.ERROR, ironic_states.DEPLOYWAIT,
                        ironic_states.DEPLOYING)
 
+_NODE_FIELDS = ('uuid', 'power_state', 'target_power_state', 'provision_state',
+                'target_provision_state', 'last_error', 'maintenance',
+                'properties', 'instance_uuid')
+
 
 def map_power_state(state):
     try:
@@ -90,7 +94,8 @@ def _validate_instance_and_node(ironicclient, instance):
     node, and return the node.
     """
     try:
-        return ironicclient.call("node.get_by_instance_uuid", instance.uuid)
+        return ironicclient.call('node.get_by_instance_uuid', instance.uuid,
+                                 fields=_NODE_FIELDS)
     except ironic.exc.NotFound:
         raise exception.InstanceNotFound(instance_id=instance.uuid)
 
@@ -159,6 +164,11 @@ def __init__(self, virtapi, read_only=False):
 
         self.ironicclient = client_wrapper.IronicClientWrapper()
 
+    def _get_node(self, node_uuid):
+        """Get a node by its UUID."""
+        return self.ironicclient.call('node.get', node_uuid,
+                                      fields=_NODE_FIELDS)
+
     def _node_resources_unavailable(self, node_obj):
         """Determine whether the node's resources are in an acceptable state.
 
@@ -446,7 +456,7 @@ def _get_hypervisor_type(self):
 
     def _get_hypervisor_version(self):
         """Returns the version of the Ironic API service endpoint."""
-        return CONF.ironic.api_version
+        return client_wrapper.IRONIC_API_VERSION[0]
 
     def instance_exists(self, instance):
         """Checks the existence of an instance.
@@ -525,7 +535,7 @@ def node_is_available(self, nodename):
         # NOTE(comstud): Fallback and check Ironic. This case should be
         # rare.
         try:
-            self.ironicclient.call("node.get", nodename)
+            self._get_node(nodename)
             return True
         except ironic.exc.NotFound:
             return False
@@ -585,7 +595,7 @@ def get_available_resource(self, nodename):
         else:
             LOG.debug("Node %(node)s not found in cache, age: %(age)s",
                       {'node': nodename, 'age': cache_age})
-            node = self.ironicclient.call("node.get", nodename)
+            node = self._get_node(nodename)
         return self._node_resource(node)
 
     def get_info(self, instance):
@@ -643,7 +653,7 @@ def macs_for_instance(self, instance):
             MAC addresses'.
         """
         try:
-            node = self.ironicclient.call("node.get", instance.node)
+            node = self._get_node(instance.node)
         except ironic.exc.NotFound:
             return None
         ports = self.ironicclient.call("node.list_ports", node.uuid)
@@ -712,7 +722,7 @@ def spawn(self, context, instance, image_meta, injected_files,
                 _("Ironic node uuid not supplied to "
                   "driver for instance %s.") % instance.uuid)
 
-        node = self.ironicclient.call("node.get", node_uuid)
+        node = self._get_node(node_uuid)
         flavor = instance.flavor
 
         self._add_driver_fields(node, instance, image_meta, flavor)
@@ -1071,7 +1081,7 @@ def plug_vifs(self, instance, network_info):
         :param network_info: Instance network information.
 
         """
-        node = self.ironicclient.call("node.get", instance.node)
+        node = self._get_node(instance.node)
         self._plug_vifs(node, instance, network_info)
 
     def unplug_vifs(self, instance, network_info):
@@ -1081,7 +1091,7 @@ def unplug_vifs(self, instance, network_info):
         :param network_info: Instance network information.
 
         """
-        node = self.ironicclient.call("node.get", instance.node)
+        node = self._get_node(instance.node)
         self._unplug_vifs(node, instance, network_info)
 
     def rebuild(self, context, instance, image_meta, injected_files,
@@ -1133,7 +1143,7 @@ def rebuild(self, context, instance, image_meta, injected_files,
         instance.save(expected_task_state=[task_states.REBUILDING])
 
         node_uuid = instance.node
-        node = self.ironicclient.call("node.get", node_uuid)
+        node = self._get_node(node_uuid)
 
         self._add_driver_fields(node, instance, image_meta, instance.flavor,
                                 preserve_ephemeral)
@@ -1174,7 +1184,7 @@ def network_binding_host_id(self, context, instance):
         :returns: a string representing the host ID
         """
 
-        node = self.ironicclient.call("node.get", instance.node)
+        node = self._get_node(instance.node)
         if getattr(node, 'network_provider', 'none') == 'none':
             # flat network, go ahead and allow the port to be bound
             return super(IronicDriver, self).network_binding_host_id(