From 85a3186d50409f33663cb05d5f48c5f8730fc6b5 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 18 Jun 2019 09:01:47 +0200 Subject: [PATCH] Use openstacksdk for accessing Swift This change replaces the swiftclient dependency with openstacksdk. The ultimate goal is to use only openstacksdk for all cross-service interactions. The configuration option max_retires has been broken for several releases since commit 7e8cdc0d0f2bff863e1dc8fac12051187a9862b7, this change deprecates it. Change-Id: I635519f88e90f4267ff7a9cb063a697ff39e4710 --- ironic_inspector/common/swift.py | 36 ++--- ironic_inspector/conf/opts.py | 1 - ironic_inspector/conf/swift.py | 5 +- ironic_inspector/test/unit/test_swift.py | 142 +++++++----------- lower-constraints.txt | 3 +- .../swift-max-retries-dfaecb74bd3aba9a.yaml | 5 + requirements.txt | 2 +- 7 files changed, 77 insertions(+), 117 deletions(-) create mode 100644 releasenotes/notes/swift-max-retries-dfaecb74bd3aba9a.yaml diff --git a/ironic_inspector/common/swift.py b/ironic_inspector/common/swift.py index 44e1190cd..716824004 100644 --- a/ironic_inspector/common/swift.py +++ b/ironic_inspector/common/swift.py @@ -15,9 +15,9 @@ import json +import openstack +from openstack import exceptions as os_exc from oslo_config import cfg -from swiftclient import client as swift_client -from swiftclient import exceptions as swift_exceptions from ironic_inspector.common.i18n import _ from ironic_inspector.common import keystone @@ -53,19 +53,9 @@ class SwiftAPI(object): if not SWIFT_SESSION: SWIFT_SESSION = keystone.get_session('swift') - adapter = keystone.get_adapter('swift', session=SWIFT_SESSION) - except Exception as exc: - raise utils.Error(_("Could not create an adapter to connect to " - "the object storage service: %s") % exc) - - # TODO(pas-ha) reverse-construct SSL-related session options here - params = { - 'os_options': { - 'object_storage_url': adapter.get_endpoint()}} - - try: - self.connection = swift_client.Connection(session=SWIFT_SESSION, - **params) + self.connection = openstack.connection.Connection( + session=SWIFT_SESSION, + oslo_conf=CONF).object_store except Exception as exc: raise utils.Error(_("Could not connect to the object storage " "service: %s") % exc) @@ -82,8 +72,8 @@ class SwiftAPI(object): :raises: utils.Error, if any operation with Swift fails. """ try: - self.connection.put_container(container) - except swift_exceptions.ClientException as e: + self.connection.create_container(container) + except os_exc.SDKException as e: err_msg = (_('Swift failed to create container %(container)s. ' 'Error was: %(error)s') % {'container': container, 'error': e}) @@ -94,11 +84,9 @@ class SwiftAPI(object): headers['X-Delete-After'] = CONF.swift.delete_after try: - obj_uuid = self.connection.put_object(container, - object, - data, - headers=headers) - except swift_exceptions.ClientException as e: + obj_uuid = self.connection.create_object( + container, object, data=data, headers=headers) + except os_exc.SDKException as e: err_msg = (_('Swift failed to create object %(object)s in ' 'container %(container)s. Error was: %(error)s') % {'object': object, 'container': container, 'error': e}) @@ -115,8 +103,8 @@ class SwiftAPI(object): :raises: utils.Error, if the Swift operation fails. """ try: - headers, obj = self.connection.get_object(container, object) - except swift_exceptions.ClientException as e: + obj = self.connection.download_object(object, container=container) + except os_exc.SDKException as e: err_msg = (_('Swift failed to get object %(object)s in ' 'container %(container)s. Error was: %(error)s') % {'object': object, 'container': container, 'error': e}) diff --git a/ironic_inspector/conf/opts.py b/ironic_inspector/conf/opts.py index 545a9d55e..ed9e13868 100644 --- a/ironic_inspector/conf/opts.py +++ b/ironic_inspector/conf/opts.py @@ -31,7 +31,6 @@ def set_config_defaults(): 'requests=WARNING', 'urllib3.connectionpool=WARNING', 'keystonemiddleware=WARNING', - 'swiftclient=WARNING', 'keystoneauth=WARNING', 'ironicclient=WARNING']) set_cors_middleware_defaults() diff --git a/ironic_inspector/conf/swift.py b/ironic_inspector/conf/swift.py index c35653d8c..581a763ff 100644 --- a/ironic_inspector/conf/swift.py +++ b/ironic_inspector/conf/swift.py @@ -23,9 +23,8 @@ SERVICE_TYPE = 'object-store' _OPTS = [ cfg.IntOpt('max_retries', - default=2, - help=_('Maximum number of times to retry a Swift request, ' - 'before failing.')), + help=_('This option is deprecated and has no effect.'), + deprecated_for_removal=True), cfg.IntOpt('delete_after', default=0, help=_('Number of seconds that the Swift object will last ' diff --git a/ironic_inspector/test/unit/test_swift.py b/ironic_inspector/test/unit/test_swift.py index 2d4098778..d6a701ab8 100644 --- a/ironic_inspector/test/unit/test_swift.py +++ b/ironic_inspector/test/unit/test_swift.py @@ -15,10 +15,9 @@ # Mostly copied from ironic/tests/test_swift.py from keystoneauth1 import exceptions as ks_exc -from keystoneauth1 import loading as kloading import mock -from swiftclient import client as swift_client -from swiftclient import exceptions as swift_exception +import openstack +from openstack import exceptions as os_exc from ironic_inspector.common import keystone from ironic_inspector.common import swift @@ -26,128 +25,99 @@ from ironic_inspector.test import base as test_base from ironic_inspector import utils -class BaseTest(test_base.NodeTest): - def setUp(self): - super(BaseTest, self).setUp() - self.all_macs = self.macs + ['DE:AD:BE:EF:DE:AD'] - self.pxe_mac = self.macs[1] - self.data = { - 'ipmi_address': self.bmc_address, - 'cpus': 2, - 'cpu_arch': 'x86_64', - 'memory_mb': 1024, - 'local_gb': 20, - 'interfaces': { - 'em1': {'mac': self.macs[0], 'ip': '1.2.0.1'}, - 'em2': {'mac': self.macs[1], 'ip': '1.2.0.2'}, - 'em3': {'mac': self.all_macs[2]}, - }, - 'boot_interface': '01-' + self.pxe_mac.replace(':', '-'), - } - - -@mock.patch.object(keystone, 'get_adapter', autospec=True) -@mock.patch.object(keystone, 'register_auth_opts') -@mock.patch.object(keystone, 'get_session') -@mock.patch.object(swift_client, 'Connection', autospec=True) -class SwiftTestCase(BaseTest): +@mock.patch.object(keystone, 'get_session', autospec=True) +@mock.patch.object(openstack.connection, 'Connection', autospec=True) +class SwiftTestCase(test_base.NodeTest): def setUp(self): super(SwiftTestCase, self).setUp() swift.reset_swift_session() - self.swift_exception = swift_exception.ClientException('', '') - self.cfg.config(group='swift', max_retries=2) - # NOTE(aarefiev) register keystoneauth dynamic options - adapter_opts = kloading.get_adapter_conf_options( - include_deprecated=False) - self.cfg.register_opts(adapter_opts, 'swift') self.addCleanup(swift.reset_swift_session) - def test___init__(self, connection_mock, load_mock, - opts_mock, adapter_mock): - fake_endpoint = "http://localhost:6000" - adapter_mock.return_value.get_endpoint.return_value = fake_endpoint + def test___init__(self, connection_mock, load_mock): swift.SwiftAPI() connection_mock.assert_called_once_with( session=load_mock.return_value, - os_options={'object_storage_url': fake_endpoint}) + oslo_conf=swift.CONF) - def test___init__keystone_failure(self, connection_mock, load_mock, - opts_mock, adapter_mock): - adapter_mock.side_effect = ks_exc.MissingRequiredOptions([]) - self.assertRaisesRegex(utils.Error, 'Could not create an adapter', + def test___init__keystone_failure(self, connection_mock, load_mock): + load_mock.side_effect = ks_exc.MissingRequiredOptions([]) + self.assertRaisesRegex(utils.Error, 'Could not connect', swift.SwiftAPI) self.assertFalse(connection_mock.called) - def test___init__swift_failure(self, connection_mock, load_mock, - opts_mock, adapter_mock): - fake_endpoint = "http://localhost:6000" - adapter_mock.return_value.get_endpoint.return_value = fake_endpoint + def test___init__sdk_failure(self, connection_mock, load_mock): connection_mock.side_effect = RuntimeError() self.assertRaisesRegex(utils.Error, 'Could not connect', swift.SwiftAPI) connection_mock.assert_called_once_with( session=load_mock.return_value, - os_options={'object_storage_url': fake_endpoint}) + oslo_conf=swift.CONF) - def test_create_object(self, connection_mock, load_mock, - opts_mock, adapter_mock): + def test_create_object(self, connection_mock, load_mock): swiftapi = swift.SwiftAPI() - connection_obj_mock = connection_mock.return_value - - connection_obj_mock.put_object.return_value = 'object-uuid' + swift_mock = connection_mock.return_value.object_store + swift_mock.create_object.return_value = 'object-uuid' object_uuid = swiftapi.create_object('object', 'some-string-data') - connection_obj_mock.put_container.assert_called_once_with('ironic-' - 'inspector') - connection_obj_mock.put_object.assert_called_once_with( - 'ironic-inspector', 'object', 'some-string-data', headers=None) + swift_mock.create_container.assert_called_once_with('ironic-inspector') + swift_mock.create_object.assert_called_once_with( + 'ironic-inspector', 'object', + data='some-string-data', headers=None) + self.assertEqual('object-uuid', object_uuid) + + def test_create_object_with_delete_after(self, connection_mock, load_mock): + swift.CONF.set_override('delete_after', 60, group='swift') + + swiftapi = swift.SwiftAPI() + swift_mock = connection_mock.return_value.object_store + swift_mock.create_object.return_value = 'object-uuid' + + object_uuid = swiftapi.create_object('object', 'some-string-data') + + swift_mock.create_container.assert_called_once_with('ironic-inspector') + swift_mock.create_object.assert_called_once_with( + 'ironic-inspector', 'object', + data='some-string-data', headers={'X-Delete-After': 60}) self.assertEqual('object-uuid', object_uuid) def test_create_object_create_container_fails( - self, connection_mock, load_mock, opts_mock, adapter_mock): + self, connection_mock, load_mock): swiftapi = swift.SwiftAPI() - connection_obj_mock = connection_mock.return_value - connection_obj_mock.put_container.side_effect = self.swift_exception + swift_mock = connection_mock.return_value.object_store + swift_mock.create_container.side_effect = os_exc.SDKException self.assertRaises(utils.Error, swiftapi.create_object, 'object', 'some-string-data') - connection_obj_mock.put_container.assert_called_once_with('ironic-' - 'inspector') - self.assertFalse(connection_obj_mock.put_object.called) + swift_mock.create_container.assert_called_once_with('ironic-inspector') + self.assertFalse(swift_mock.create_object.called) - def test_create_object_put_object_fails(self, connection_mock, load_mock, - opts_mock, adapter_mock): + def test_create_object_put_object_fails(self, connection_mock, load_mock): swiftapi = swift.SwiftAPI() - connection_obj_mock = connection_mock.return_value - connection_obj_mock.put_object.side_effect = self.swift_exception + swift_mock = connection_mock.return_value.object_store + swift_mock.create_object.side_effect = os_exc.SDKException self.assertRaises(utils.Error, swiftapi.create_object, 'object', 'some-string-data') - connection_obj_mock.put_container.assert_called_once_with('ironic-' - 'inspector') - connection_obj_mock.put_object.assert_called_once_with( - 'ironic-inspector', 'object', 'some-string-data', headers=None) + swift_mock.create_container.assert_called_once_with('ironic-inspector') + swift_mock.create_object.assert_called_once_with( + 'ironic-inspector', 'object', + data='some-string-data', headers=None) - def test_get_object(self, connection_mock, load_mock, - opts_mock, adapter_mock): + def test_get_object(self, connection_mock, load_mock): swiftapi = swift.SwiftAPI() - connection_obj_mock = connection_mock.return_value - - expected_obj = self.data - connection_obj_mock.get_object.return_value = ('headers', expected_obj) + swift_mock = connection_mock.return_value.object_store swift_obj = swiftapi.get_object('object') - connection_obj_mock.get_object.assert_called_once_with( - 'ironic-inspector', 'object') - self.assertEqual(expected_obj, swift_obj) + swift_mock.download_object.assert_called_once_with( + 'object', container='ironic-inspector') + self.assertIs(swift_mock.download_object.return_value, swift_obj) - def test_get_object_fails(self, connection_mock, load_mock, - opts_mock, adapter_mock): + def test_get_object_fails(self, connection_mock, load_mock): swiftapi = swift.SwiftAPI() - connection_obj_mock = connection_mock.return_value - connection_obj_mock.get_object.side_effect = self.swift_exception + swift_mock = connection_mock.return_value.object_store + swift_mock.download_object.side_effect = os_exc.SDKException self.assertRaises(utils.Error, swiftapi.get_object, 'object') - connection_obj_mock.get_object.assert_called_once_with( - 'ironic-inspector', 'object') + swift_mock.download_object.assert_called_once_with( + 'object', container='ironic-inspector') diff --git a/lower-constraints.txt b/lower-constraints.txt index 5af2f3c06..89c9d5f7c 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -56,7 +56,7 @@ munch==2.2.0 netaddr==0.7.18 netifaces==0.10.6 openstackdocstheme==1.18.1 -openstacksdk==0.12.0 +openstacksdk==0.30.0 os-api-ref==1.4.0 os-client-config==1.29.0 os-service-types==1.2.0 @@ -97,7 +97,6 @@ python-ironicclient==2.3.0 python-keystoneclient==3.15.0 python-mimeparse==1.6.0 python-subunit==1.2.0 -python-swiftclient==3.2.0 pytz==2013.6 PyYAML==3.12 reno==2.5.0 diff --git a/releasenotes/notes/swift-max-retries-dfaecb74bd3aba9a.yaml b/releasenotes/notes/swift-max-retries-dfaecb74bd3aba9a.yaml new file mode 100644 index 000000000..0bcd1fd66 --- /dev/null +++ b/releasenotes/notes/swift-max-retries-dfaecb74bd3aba9a.yaml @@ -0,0 +1,5 @@ +--- +deprecations: + - | + The configuration option ``[swift]max_retries`` is deprecated. It has been + doing nothing for a few releases already. diff --git a/requirements.txt b/requirements.txt index d750d3330..ae6ea25be 100644 --- a/requirements.txt +++ b/requirements.txt @@ -16,8 +16,8 @@ keystonemiddleware>=4.17.0 # Apache-2.0 netaddr>=0.7.18 # BSD pbr!=2.1.0,>=2.0.0 # Apache-2.0 python-ironicclient>=2.3.0 # Apache-2.0 -python-swiftclient>=3.2.0 # Apache-2.0 pytz>=2013.6 # MIT +openstacksdk>=0.30.0 # Apache-2.0 oslo.concurrency>=3.26.0 # Apache-2.0 oslo.config>=5.2.0 # Apache-2.0 oslo.context>=2.19.2 # Apache-2.0