From 4acbf4fee389129611541b6d7db999dd5ddb45ad Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Thu, 12 Jul 2018 15:43:15 -0700 Subject: [PATCH] ironic: provide facilities to gracefully navigate versions For quite some time, the nova virt driver has invoked the ironic python client library with a static microversion pin which causes operators to have a failed nova-compute process upon out of order upgrades, as well as causes ironic's grenade CI jobs to break every cycle when the pin is changed to a version released in the current cycle due to established community agreement as well as the grenade testing framework limitations. In order to gracefully navigate this, python-ironicclient has accepted a list of possible versions to negotiate since 2.2.0. This patch provies a mechanism to validate if we are able to send a request that was added at a specific version, allowing for minimal code changes and for logic to only be centered around newer features being added. Change-Id: I440689a246538fbc8200687e40480d837b87eb7b Closes-Bug: #1739440 --- nova/exception.py | 4 ++ .../unit/virt/ironic/test_client_wrapper.py | 6 +-- nova/tests/unit/virt/ironic/test_driver.py | 28 ++++++++++++++ nova/tests/unit/virt/ironic/utils.py | 3 ++ nova/virt/ironic/client_wrapper.py | 38 ++++++++++++++++++- nova/virt/ironic/driver.py | 22 +++++++++++ 6 files changed, 97 insertions(+), 4 deletions(-) diff --git a/nova/exception.py b/nova/exception.py index 4a63f4c579..b8a807febb 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -2295,3 +2295,7 @@ class CertificateValidationNotYetAvailable(NovaException): msg_fmt = _("Image signature certificate validation support is " "not yet available.") code = 409 + + +class IronicAPIVersionNotAvailable(NovaException): + msg_fmt = _('Ironic API version %(version)s is not available.') diff --git a/nova/tests/unit/virt/ironic/test_client_wrapper.py b/nova/tests/unit/virt/ironic/test_client_wrapper.py index c6f66d982e..79046fb3e4 100644 --- a/nova/tests/unit/virt/ironic/test_client_wrapper.py +++ b/nova/tests/unit/virt/ironic/test_client_wrapper.py @@ -86,7 +86,7 @@ class IronicClientWrapperTestCase(test.NoDBTestCase): expected = {'session': 'session', 'max_retries': CONF.ironic.api_max_retries, 'retry_interval': CONF.ironic.api_retry_interval, - 'os_ironic_api_version': '1.37', + 'os_ironic_api_version': ['1.37', '1.37'], 'ironic_url': self.get_ksa_adapter.return_value.get_endpoint.return_value} mock_ir_cli.assert_called_once_with(1, **expected) @@ -112,7 +112,7 @@ class IronicClientWrapperTestCase(test.NoDBTestCase): expected = {'session': 'session', 'max_retries': CONF.ironic.api_max_retries, 'retry_interval': CONF.ironic.api_retry_interval, - 'os_ironic_api_version': '1.37', + 'os_ironic_api_version': ['1.37', '1.37'], 'ironic_url': None} mock_ir_cli.assert_called_once_with(1, **expected) @@ -130,7 +130,7 @@ class IronicClientWrapperTestCase(test.NoDBTestCase): expected = {'session': 'session', 'max_retries': CONF.ironic.api_max_retries, 'retry_interval': CONF.ironic.api_retry_interval, - 'os_ironic_api_version': '1.37', + 'os_ironic_api_version': ['1.37', '1.37'], 'ironic_url': endpoint} mock_ir_cli.assert_called_once_with(1, **expected) diff --git a/nova/tests/unit/virt/ironic/test_driver.py b/nova/tests/unit/virt/ironic/test_driver.py index 36b7b341b1..b09f98d28f 100644 --- a/nova/tests/unit/virt/ironic/test_driver.py +++ b/nova/tests/unit/virt/ironic/test_driver.py @@ -2816,6 +2816,34 @@ class IronicDriverSyncTestCase(IronicDriverTestCase): self.driver._pike_flavor_migration([uuids.node1]) mock_normalize.assert_not_called() + def test__can_send_version(self): + self.assertIsNone( + self.driver._can_send_version( + min_version='%d.%d' % cw.IRONIC_API_VERSION)) + + def test__can_send_version_too_new(self): + self.assertRaises(exception.IronicAPIVersionNotAvailable, + self.driver._can_send_version, + min_version='%d.%d' % (cw.IRONIC_API_VERSION[0], + cw.IRONIC_API_VERSION[1] + 1)) + + def test__can_send_version_too_old(self): + self.assertRaises( + exception.IronicAPIVersionNotAvailable, + self.driver._can_send_version, + max_version='%d.%d' % (cw.PRIOR_IRONIC_API_VERSION[0], + cw.PRIOR_IRONIC_API_VERSION[1] - 1)) + + @mock.patch.object(cw.IronicClientWrapper, 'current_api_version', + autospec=True) + @mock.patch.object(cw.IronicClientWrapper, 'is_api_version_negotiated', + autospec=True) + def test__can_send_version_not_negotiated(self, mock_is_negotiated, + mock_api_version): + mock_is_negotiated.return_value = False + self.assertIsNone(self.driver._can_send_version()) + self.assertFalse(mock_api_version.called) + @mock.patch.object(instance_metadata, 'InstanceMetadata') @mock.patch.object(configdrive, 'ConfigDriveBuilder') diff --git a/nova/tests/unit/virt/ironic/utils.py b/nova/tests/unit/virt/ironic/utils.py index d98658e658..0e5e318194 100644 --- a/nova/tests/unit/virt/ironic/utils.py +++ b/nova/tests/unit/virt/ironic/utils.py @@ -14,6 +14,7 @@ # under the License. from nova import objects +from nova.virt.ironic import client_wrapper from nova.virt.ironic import ironic_states @@ -220,3 +221,5 @@ class FakeClient(object): port = FakePortClient() portgroup = FakePortgroupClient() volume_target = FakeVolumeTargetClient() + current_api_version = '%d.%d' % client_wrapper.IRONIC_API_VERSION + is_api_version_negotiated = True diff --git a/nova/virt/ironic/client_wrapper.py b/nova/virt/ironic/client_wrapper.py index 3f767af0e0..cc40e932a8 100644 --- a/nova/virt/ironic/client_wrapper.py +++ b/nova/virt/ironic/client_wrapper.py @@ -33,6 +33,13 @@ IRONIC_GROUP = nova.conf.ironic.ironic_group # The API version required by the Ironic driver IRONIC_API_VERSION = (1, 37) +# NOTE(TheJulia): This version should ALWAYS be the _last_ release +# supported version of the API version used by nova. If a feature +# needs 1.38 to be negotiated to operate properly, then the version +# above should be updated, and this version should only be changed +# once a cycle to the API version desired for features merging in +# that cycle. +PRIOR_IRONIC_API_VERSION = (1, 37) class IronicClientWrapper(object): @@ -85,7 +92,12 @@ class IronicClientWrapper(object): kwargs = {} kwargs['max_retries'] = max_retries kwargs['retry_interval'] = retry_interval - kwargs['os_ironic_api_version'] = '%d.%d' % IRONIC_API_VERSION + # NOTE(TheJulia): The ability for a list of available versions to be + # accepted was added in python-ironicclient 2.2.0. The highest + # available version will be utilized by the client for the lifetime + # of the client. + kwargs['os_ironic_api_version'] = [ + '%d.%d' % IRONIC_API_VERSION, '%d.%d' % PRIOR_IRONIC_API_VERSION] # NOTE(clenimar/efried): by default, the endpoint is taken from the # service catalog. Use `endpoint_override` if you want to override it. @@ -156,3 +168,27 @@ class IronicClientWrapper(object): # 0.8.0 client = self._get_client(retry_on_conflict=retry_on_conflict) return self._multi_getattr(client, method)(*args, **kwargs) + + @property + def current_api_version(self): + """Value representing the negotiated API client version. + + This value represents the current negotiated API version that + is being utilized by the client to permit the caller to make + decisions based upon that version. + + :returns: The highest available negotiatable version or None + if a version has not yet been negotiated by the underlying + client library. + """ + return self._get_client().current_api_version + + @property + def is_api_version_negotiated(self): + """Boolean to indicate if the client version has been negotiated. + + :returns: True if the underlying client library has completed API + version negotiation. Otherwise the value returned is + False. + """ + return self._get_client().is_api_version_negotiated diff --git a/nova/virt/ironic/driver.py b/nova/virt/ironic/driver.py index d80c3f3f9a..c848cbdfe8 100644 --- a/nova/virt/ironic/driver.py +++ b/nova/virt/ironic/driver.py @@ -19,6 +19,7 @@ A driver wrapping the Ironic API, such that Nova may provision bare metal resources. """ import base64 +from distutils import version import gzip import shutil import tempfile @@ -1954,3 +1955,24 @@ class IronicDriver(virt_driver.ComputeDriver): instance=instance) return vif_id return None + + def _can_send_version(self, min_version=None, max_version=None): + """Validate if the suppplied version is available in the API.""" + # NOTE(TheJulia): This will effectively just be a pass if no + # version negotiation has occured, since there is no way for + # us to know without explicitly otherwise requesting that + # back-end negotiation occurs. This is a capability that is + # present in python-ironicclient, however it may not be needed + # in this case. + if self.ironicclient.is_api_version_negotiated: + current_api_version = self.ironicclient.current_api_version + if (min_version and + version.StrictVersion(current_api_version) < + version.StrictVersion(min_version)): + raise exception.IronicAPIVersionNotAvailable( + version=min_version) + if (max_version and + version.StrictVersion(current_api_version) > + version.StrictVersion(max_version)): + raise exception.IronicAPIVersionNotAvailable( + version=max_version)