From 05c0e07a9d7f9159d08e8862ac9b2d3bcc00a5b1 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Thu, 21 Jan 2021 16:23:33 +0000 Subject: [PATCH 1/5] Add option to destroy storage to destroy_model() call If a sequence of bundles (e.g. for func) creates storage, if destroy_storage is not used, then the model (possibly) leaves the storage behind. This patch enables the destroy_storage feature in destroy_model() call in python_libjuju. --- zaza/controller.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/zaza/controller.py b/zaza/controller.py index 5b8380605..bd2686bf0 100644 --- a/zaza/controller.py +++ b/zaza/controller.py @@ -54,7 +54,10 @@ async def async_destroy_model(model_name): controller = Controller() await controller.connect() logging.debug("Destroying model {}".format(model_name)) - await controller.destroy_model(model_name, force=True, max_wait=600) + await controller.destroy_model(model_name, + destroy_storage=True, + force=True, + max_wait=600) # The model ought to be destroyed by now. Let's make sure, and if not, # raise an error. Even if the model has been destroyed, it's still hangs # around in the .list_models() for a little while; retry until it goes From 9b4bfdff06f9ea1d7e463365c888d8162034e0f7 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Mon, 25 Jan 2021 12:19:24 +0000 Subject: [PATCH 2/5] Fix test to work with additional destroy_storage parameter --- unit_tests/test_zaza_controller.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/unit_tests/test_zaza_controller.py b/unit_tests/test_zaza_controller.py index 57a9db740..da4dc8e1a 100644 --- a/unit_tests/test_zaza_controller.py +++ b/unit_tests/test_zaza_controller.py @@ -37,7 +37,8 @@ async def _list_models(): async def _add_model(model_name, config=None): return self.model1 - async def _destroy_model(model_name, force=False, max_wait=None): + async def _destroy_model(model_name, destroy_storage=False, + force=False, max_wait=None): if model_name in self.models: self.models.remove(model_name) return @@ -90,7 +91,8 @@ def test_add_model_config(self): def test_destroy_model(self): controller.destroy_model(self.model1.info.name) self.Controller_mock.destroy_model.assert_called_once_with( - self.model1.info.name, force=True, max_wait=600) + self.model1.info.name, destroy_storage=True, + force=True, max_wait=600) def test_get_cloud(self): self.assertEqual( From 4cccbdc9786acbcdc9485394c0b696e4fde4712e Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Wed, 27 Jan 2021 10:51:55 +0000 Subject: [PATCH 3/5] Really destroy instances when destroying model on openstack provider Essentially, the destroy-model with --force (via libjuju) doesn't reap the instances leading to CI failures if multiple bundles are used as it relies on proper clean-up of the instances and their vips. This patch uses the undercloud OpenStack directly to remove the instances that used to belong to the model that was just destroyed. Only has an affect on the openstack provider. --- zaza/charm_lifecycle/destroy.py | 13 ++ zaza/utilities/openstack_provider.py | 286 +++++++++++++++++++++++++++ 2 files changed, 299 insertions(+) create mode 100644 zaza/utilities/openstack_provider.py diff --git a/zaza/charm_lifecycle/destroy.py b/zaza/charm_lifecycle/destroy.py index ea7e5c926..fcaad20fd 100644 --- a/zaza/charm_lifecycle/destroy.py +++ b/zaza/charm_lifecycle/destroy.py @@ -14,19 +14,32 @@ """Run destroy phase.""" import argparse +import logging import sys import zaza.controller import zaza.utilities.cli as cli_utils +import zaza.utilities.juju as juju_utils def destroy(model_name): """Run all steps to cleaup after a test run. + Note: on the OpenStack provider we also verify after the destroy model call + that the instances associated with the model really are gone. Reap any + instances that have the model name in them before returning. + Bug: #xxxxxxx + :param model: Name of model to remove :type bundle: str """ zaza.controller.destroy_model(model_name) + # get the keystone overcloud + if juju_utils.get_provider_type() == "openstack": + # only import openstack_provider if it's needed. This avoids forcing + # zaza to have dependencies for providers that the user isn't using. + import zaza.utilities.openstack_provider as op + op.clean_up_instances(model_name) def parse_args(args): diff --git a/zaza/utilities/openstack_provider.py b/zaza/utilities/openstack_provider.py new file mode 100644 index 000000000..fe2780d80 --- /dev/null +++ b/zaza/utilities/openstack_provider.py @@ -0,0 +1,286 @@ +# Copyright 2021 Canonical Ltd. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Functions to work with the openstack provider.""" + +import logging +import os +import tenacity + +from keystoneauth1 import session +from keystoneauth1.identity import ( + v3, + v2, +) +from novaclient import client as novaclient_client + + +class MissingOSAthenticationException(Exception): + """Exception when some data needed to authenticate is missing.""" + + pass + + +def get_undercloud_keystone_session(verify=None): + """Return Under cloud keystone session. + + :param verify: Control TLS certificate verification behaviour + :type verify: any + :returns keystone_session: keystoneauth1.session.Session object + :rtype: keystoneauth1.session.Session + """ + return get_keystone_session(get_undercloud_auth(), + verify=verify) + + +def get_keystone_session(openrc_creds, scope='PROJECT', verify=None): + """Return keystone session. + + :param openrc_creds: OpenStack RC credentials + :type openrc_creds: dict + :param verify: Control TLS certificate verification behaviour + :type verify: any (True - use system certs, + False - do not verify, + None - defer to requests library to find certs, + str - path to a CA cert bundle) + :param scope: Authentication scope: PROJECT or DOMAIN + :type scope: string + :returns: Keystone session object + :rtype: keystoneauth1.session.Session object + """ + keystone_creds = get_ks_creds(openrc_creds, scope=scope) + if not verify and openrc_creds.get('OS_CACERT'): + verify = openrc_creds['OS_CACERT'] + if openrc_creds.get('API_VERSION', 2) == 2: + auth = v2.Password(**keystone_creds) + else: + auth = v3.Password(**keystone_creds) + return session.Session(auth=auth, verify=verify) + + +def get_ks_creds(cloud_creds, scope='PROJECT'): + """Return the credentials for authenticating against keystone. + + :param cloud_creds: OpenStack RC environment credentials + :type cloud_creds: dict + :param scope: Authentication scope: PROJECT or DOMAIN + :type scope: string + :returns: Credentials dictionary + :rtype: dict + """ + if cloud_creds.get('API_VERSION', 2) == 2: + auth = { + 'username': cloud_creds['OS_USERNAME'], + 'password': cloud_creds['OS_PASSWORD'], + 'auth_url': cloud_creds['OS_AUTH_URL'], + 'tenant_name': (cloud_creds.get('OS_PROJECT_NAME') or + cloud_creds['OS_TENANT_NAME']), + } + else: + if scope == 'DOMAIN': + auth = { + 'username': cloud_creds['OS_USERNAME'], + 'password': cloud_creds['OS_PASSWORD'], + 'auth_url': cloud_creds['OS_AUTH_URL'], + 'user_domain_name': cloud_creds['OS_USER_DOMAIN_NAME'], + 'domain_name': cloud_creds['OS_DOMAIN_NAME'], + } + else: + auth = { + 'username': cloud_creds['OS_USERNAME'], + 'password': cloud_creds['OS_PASSWORD'], + 'auth_url': cloud_creds['OS_AUTH_URL'], + 'user_domain_name': cloud_creds['OS_USER_DOMAIN_NAME'], + 'project_domain_name': cloud_creds['OS_PROJECT_DOMAIN_NAME'], + 'project_name': cloud_creds['OS_PROJECT_NAME'], + } + return auth + + +def get_undercloud_auth(): + """Get undercloud OpenStack authentication settings from environment. + + :returns: Dictionary of authentication settings + :rtype: dict + """ + os_auth_url = os.environ.get('OS_AUTH_URL') + if os_auth_url: + api_version = os_auth_url.split('/')[-1].replace('v', '') + else: + logging.error('Missing OS authentication setting: OS_AUTH_URL') + # raise exceptions.MissingOSAthenticationException( + raise MissingOSAthenticationException( + 'One or more OpenStack authentication variables could ' + 'be found in the environment. Please export the OS_* ' + 'settings into the environment.') + + logging.info('AUTH_URL: {}, api_ver: {}'.format(os_auth_url, api_version)) + + if api_version == '2.0': + # V2 + logging.info('Using keystone API V2 for undercloud auth') + auth_settings = { + 'OS_AUTH_URL': os.environ.get('OS_AUTH_URL'), + 'OS_TENANT_NAME': os.environ.get('OS_TENANT_NAME'), + 'OS_USERNAME': os.environ.get('OS_USERNAME'), + 'OS_PASSWORD': os.environ.get('OS_PASSWORD'), + 'OS_REGION_NAME': os.environ.get('OS_REGION_NAME'), + 'API_VERSION': 2, + } + elif api_version >= '3': + # V3 or later + logging.info('Using keystone API V3 (or later) for undercloud auth') + domain = os.environ.get('OS_DOMAIN_NAME') + auth_settings = { + 'OS_AUTH_URL': os.environ.get('OS_AUTH_URL'), + 'OS_USERNAME': os.environ.get('OS_USERNAME'), + 'OS_PASSWORD': os.environ.get('OS_PASSWORD'), + 'OS_REGION_NAME': os.environ.get('OS_REGION_NAME'), + 'API_VERSION': 3, + } + if domain: + auth_settings['OS_DOMAIN_NAME'] = domain + else: + auth_settings['OS_USER_DOMAIN_NAME'] = ( + os.environ.get('OS_USER_DOMAIN_NAME')) + auth_settings['OS_PROJECT_NAME'] = ( + os.environ.get('OS_PROJECT_NAME')) + auth_settings['OS_PROJECT_DOMAIN_NAME'] = ( + os.environ.get('OS_PROJECT_DOMAIN_NAME')) + os_project_id = os.environ.get('OS_PROJECT_ID') + if os_project_id is not None: + auth_settings['OS_PROJECT_ID'] = os_project_id + + _os_cacert = os.environ.get('OS_CACERT') + if _os_cacert: + auth_settings.update({'OS_CACERT': _os_cacert}) + + # Validate settings + for key, settings in list(auth_settings.items()): + if settings is None: + logging.error('Missing OS authentication setting: {}' + ''.format(key)) + # raise exceptions.MissingOSAthenticationException( + raise MissingOSAthenticationException( + 'One or more OpenStack authentication variables could ' + 'be found in the environment. Please export the OS_* ' + 'settings into the environment.') + + return auth_settings + + +# Nova utilities +def get_nova_session_client(session, version=2): + """Return novaclient authenticated by keystone session. + + :param session: Keystone session object + :type session: keystoneauth1.session.Session object + :param version: Version of client to request. + :type version: float + :returns: Authenticated novaclient + :rtype: novaclient.Client object + """ + return novaclient_client.Client(version, session=session) + + +# Manage resources +def delete_resource(resource, resource_id, msg="resource"): + """Delete an openstack resource. + + Delete an openstack resource, such as one instance, keypair, + image, volume, stack, etc., and confirm deletion within max wait time. + + :param resource: pointer to os resource type, ex:glance_client.images + :type resource: str + :param resource_id: unique name or id for the openstack resource + :type resource_id: str + :param msg: text to identify purpose in logging + :type msg: str + """ + logging.debug('Deleting OpenStack resource ' + '{} ({})'.format(resource_id, msg)) + resource.delete(resource_id) + resource_removed(resource, resource_id, msg) + + +def _resource_removed(resource, resource_id, msg="resource"): + """Wait for an openstack resource to no longer be present. + + :param resource: pointer to os resource type, ex: heat_client.stacks + :type resource: str + :param resource_id: unique id for the openstack resource + :type resource_id: str + :param msg: text to identify purpose in logging + :type msg: str + :raises: AssertionError + """ + matching = [r for r in resource.list() if r.id == resource_id] + logging.debug("{}: resource {} still present".format(msg, resource_id)) + assert len(matching) == 0 + + +def resource_removed(resource, + resource_id, + msg='resource', + wait_exponential_multiplier=1, + wait_iteration_max_time=60, + stop_after_attempt=8): + """Wait for an openstack resource to no longer be present. + + :param resource: pointer to os resource type, ex: heat_client.stacks + :type resource: str + :param resource_id: unique id for the openstack resource + :type resource_id: str + :param msg: text to identify purpose in logging + :type msg: str + :param wait_exponential_multiplier: Wait 2^x * wait_exponential_multiplier + seconds between each retry + :type wait_exponential_multiplier: int + :param wait_iteration_max_time: Wait a max of wait_iteration_max_time + between retries. + :type wait_iteration_max_time: int + :param stop_after_attempt: Stop after stop_after_attempt retires. + :type stop_after_attempt: int + :raises: AssertionError + """ + retryer = tenacity.Retrying( + wait=tenacity.wait_exponential( + multiplier=wait_exponential_multiplier, + max=wait_iteration_max_time), + reraise=True, + stop=tenacity.stop_after_attempt(stop_after_attempt)) + retryer( + _resource_removed, + resource, + resource_id, + msg) + + +def clean_up_instances(model_name): + """ + TODO: fill this out + """ + session = get_undercloud_keystone_session() + nova_client = get_nova_session_client(session) + servers = list(nova_client.servers.list()) + if servers: + logging.warning("Having to clean-up {} servers after supposed destroy" + .format(len(servers))) + for server in servers: + if model_name in server.name: + logging.info('Removing server {}'.format(server.name)) + delete_resource( + nova_client.servers, + server.id, + msg="server") From a90e306c35ffb11a1c4398bf9cd19b08fdc5ae7b Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Thu, 28 Jan 2021 15:37:16 +0000 Subject: [PATCH 4/5] Add tests for new openstack_provider and usage thereof --- .../test_zaza_charm_lifecycle_destroy.py | 20 ++ .../test_zaza_utilities_openstack_provider.py | 205 ++++++++++++++++++ zaza/charm_lifecycle/destroy.py | 7 +- zaza/controller.py | 3 +- zaza/utilities/openstack_provider.py | 33 ++- 5 files changed, 256 insertions(+), 12 deletions(-) create mode 100644 unit_tests/test_zaza_utilities_openstack_provider.py diff --git a/unit_tests/test_zaza_charm_lifecycle_destroy.py b/unit_tests/test_zaza_charm_lifecycle_destroy.py index 7762ba2ce..320129fa0 100644 --- a/unit_tests/test_zaza_charm_lifecycle_destroy.py +++ b/unit_tests/test_zaza_charm_lifecycle_destroy.py @@ -20,8 +20,28 @@ class TestCharmLifecycleDestroy(ut_utils.BaseTestCase): def test_destroy(self): self.patch_object(lc_destroy.zaza.controller, 'destroy_model') + self.patch_object(lc_destroy.model, 'get_status', + return_value={'machines': "the-machines"}) + self.patch_object(lc_destroy.juju_utils, 'get_provider_type', + return_value="maas") + self.patch("zaza.utilities.openstack_provider.clean_up_instances", + name='clean_up_instances') lc_destroy.destroy('doomed') self.destroy_model.assert_called_once_with('doomed') + self.clean_up_instances.assert_not_called() + + def test_destroy_on_openstack_provider(self): + self.patch_object(lc_destroy.zaza.controller, 'destroy_model') + self.patch_object(lc_destroy.model, 'get_status', + return_value={'machines': "the-machines"}) + self.patch_object(lc_destroy.juju_utils, 'get_provider_type', + return_value="openstack") + self.patch("zaza.utilities.openstack_provider.clean_up_instances", + name='clean_up_instances') + lc_destroy.destroy('doomed') + self.destroy_model.assert_called_once_with('doomed') + self.clean_up_instances.assert_called_once_with( + 'doomed', 'the-machines') def test_parser(self): args = lc_destroy.parse_args(['-m', 'doomed']) diff --git a/unit_tests/test_zaza_utilities_openstack_provider.py b/unit_tests/test_zaza_utilities_openstack_provider.py new file mode 100644 index 000000000..4e88666fe --- /dev/null +++ b/unit_tests/test_zaza_utilities_openstack_provider.py @@ -0,0 +1,205 @@ +# Copyright 2018 Canonical Ltd. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import mock + +import unit_tests.utils as ut_utils +import zaza.utilities.openstack_provider as openstack_provider + + +class TestOpenStackUtils(ut_utils.BaseTestCase): + + def setUp(self): + super(TestOpenStackUtils, self).setUp() + self.port_name = "port_name" + self.net_uuid = "net_uuid" + self.project_id = "project_uuid" + self.ext_net = "ext_net" + self.private_net = "private_net" + self.port = { + "port": {"id": "port_id", + "name": self.port_name, + "network_id": self.net_uuid}} + self.ports = {"ports": [self.port["port"]]} + self.floatingip = { + "floatingip": {"id": "floatingip_id", + "floating_network_id": self.net_uuid, + "port_id": "port_id"}} + self.floatingips = {"floatingips": [self.floatingip["floatingip"]]} + self.address_scope_name = "address_scope_name" + self.address_scope = { + "address_scope": {"id": "address_scope_id", + "name": self.address_scope_name, + "shared": True, + "ip_version": 4, + "tenant_id": self.project_id}} + self.address_scopes = { + "address_scopes": [self.address_scope["address_scope"]]} + + self.network = { + "network": {"id": "network_id", + "name": self.ext_net, + "tenant_id": self.project_id, + "router:external": True, + "provider:physical_network": "physnet1", + "provider:network_type": "flat"}} + + self.networks = { + "networks": [self.network["network"]]} + + self.agents = { + "agents": [ + { + 'id': '7f3afd5b-ff6d-4df3-be0e-3d9651e71873', + 'binary': 'neutron-bgp-dragent', + }]} + + self.bgp_speakers = { + "bgp_speakers": [ + { + 'id': '07a0798d-c29c-4a92-8fcb-c1ec56934729', + }]} + + self.neutronclient = mock.MagicMock() + self.neutronclient.list_ports.return_value = self.ports + self.neutronclient.create_port.return_value = self.port + + self.neutronclient.list_floatingips.return_value = self.floatingips + self.neutronclient.create_floatingip.return_value = self.floatingip + + self.neutronclient.list_address_scopes.return_value = ( + self.address_scopes) + self.neutronclient.create_address_scope.return_value = ( + self.address_scope) + + self.neutronclient.list_networks.return_value = self.networks + self.neutronclient.create_network.return_value = self.network + + self.neutronclient.list_agents.return_value = self.agents + self.neutronclient.list_bgp_speaker_on_dragent.return_value = \ + self.bgp_speakers + + def test_get_undercloud_keystone_session(self): + self.patch_object(openstack_provider, "get_keystone_session") + self.patch_object(openstack_provider, "get_undercloud_auth") + _auth = "FAKE_AUTH" + self.get_undercloud_auth.return_value = _auth + + openstack_provider.get_undercloud_keystone_session() + self.get_keystone_session.assert_called_once_with(_auth, verify=None) + + def test_get_nova_session_client(self): + session_mock = mock.MagicMock() + self.patch_object(openstack_provider.novaclient_client, "Client") + openstack_provider.get_nova_session_client(session_mock) + self.Client.assert_called_once_with(2, session=session_mock) + self.Client.reset_mock() + openstack_provider.get_nova_session_client(session_mock, version=2.56) + self.Client.assert_called_once_with(2.56, session=session_mock) + + def test__resource_removed(self): + resource_mock = mock.MagicMock() + resource_mock.list.return_value = [mock.MagicMock(id='ba8204b0')] + openstack_provider._resource_removed(resource_mock, 'e01df65a') + + def test__resource_removed_fail(self): + resource_mock = mock.MagicMock() + resource_mock.list.return_value = [mock.MagicMock(id='e01df65a')] + with self.assertRaises(AssertionError): + openstack_provider._resource_removed(resource_mock, 'e01df65a') + + def test_resource_removed(self): + self.patch_object(openstack_provider, "_resource_removed") + self._resource_removed.return_value = True + openstack_provider.resource_removed('resource', 'e01df65a') + self._resource_removed.assert_called_once_with( + 'resource', + 'e01df65a', + 'resource') + + def test_resource_removed_custom_retry(self): + self.patch_object(openstack_provider, "_resource_removed") + + def _retryer(f, arg1, arg2, arg3): + f(arg1, arg2, arg3) + self.patch_object( + openstack_provider.tenacity, + "Retrying", + return_value=_retryer) + saa_mock = mock.MagicMock() + self.patch_object( + openstack_provider.tenacity, + "stop_after_attempt", + return_value=saa_mock) + we_mock = mock.MagicMock() + self.patch_object( + openstack_provider.tenacity, + "wait_exponential", + return_value=we_mock) + self._resource_removed.return_value = True + openstack_provider.resource_removed( + 'resource', + 'e01df65a', + wait_exponential_multiplier=2, + wait_iteration_max_time=20, + stop_after_attempt=2) + self._resource_removed.assert_called_once_with( + 'resource', + 'e01df65a', + 'resource') + self.Retrying.assert_called_once_with( + wait=we_mock, + reraise=True, + stop=saa_mock) + + def test_delete_resource(self): + resource_mock = mock.MagicMock() + self.patch_object(openstack_provider, "resource_removed") + openstack_provider.delete_resource(resource_mock, 'e01df65a') + resource_mock.delete.assert_called_once_with('e01df65a') + self.resource_removed.assert_called_once_with( + resource_mock, + 'e01df65a', + 'resource') + + def test_get_keystone_session(self): + self.patch_object(openstack_provider, "session") + self.patch_object(openstack_provider, "v2") + _auth = mock.MagicMock() + self.v2.Password.return_value = _auth + _openrc = { + "OS_AUTH_URL": "https://keystone:5000", + "OS_USERNAME": "myuser", + "OS_PASSWORD": "pass", + "OS_TENANT_NAME": "tenant", + } + openstack_provider.get_keystone_session(_openrc) + self.session.Session.assert_called_once_with(auth=_auth, verify=None) + + def test_get_keystone_session_tls(self): + self.patch_object(openstack_provider, "session") + self.patch_object(openstack_provider, "v2") + _auth = mock.MagicMock() + self.v2.Password.return_value = _auth + _cacert = "/tmp/cacert" + _openrc = { + "OS_AUTH_URL": "https://keystone:5000", + "OS_USERNAME": "myuser", + "OS_PASSWORD": "pass", + "OS_TENANT_NAME": "tenant", + "OS_CACERT": _cacert, + } + openstack_provider.get_keystone_session(_openrc) + self.session.Session.assert_called_once_with( + auth=_auth, verify=_cacert) diff --git a/zaza/charm_lifecycle/destroy.py b/zaza/charm_lifecycle/destroy.py index fcaad20fd..da4f7f896 100644 --- a/zaza/charm_lifecycle/destroy.py +++ b/zaza/charm_lifecycle/destroy.py @@ -14,12 +14,12 @@ """Run destroy phase.""" import argparse -import logging import sys import zaza.controller import zaza.utilities.cli as cli_utils import zaza.utilities.juju as juju_utils +import zaza.model as model def destroy(model_name): @@ -28,18 +28,19 @@ def destroy(model_name): Note: on the OpenStack provider we also verify after the destroy model call that the instances associated with the model really are gone. Reap any instances that have the model name in them before returning. - Bug: #xxxxxxx + Bug: https://bugs.launchpad.net/juju/+bug/1913418 :param model: Name of model to remove :type bundle: str """ + machines = model.get_status()["machines"] zaza.controller.destroy_model(model_name) # get the keystone overcloud if juju_utils.get_provider_type() == "openstack": # only import openstack_provider if it's needed. This avoids forcing # zaza to have dependencies for providers that the user isn't using. import zaza.utilities.openstack_provider as op - op.clean_up_instances(model_name) + op.clean_up_instances(model_name, machines) def parse_args(args): diff --git a/zaza/controller.py b/zaza/controller.py index bd2686bf0..66eca606c 100644 --- a/zaza/controller.py +++ b/zaza/controller.py @@ -64,8 +64,7 @@ async def async_destroy_model(model_name): # away, or that fails. for attempt in tenacity.Retrying( stop=tenacity.stop_after_attempt(20), - wait=tenacity.wait_exponential( - multiplier=1, min=2, max=20), + wait=tenacity.wait_fixed(10), retry=tenacity.retry_if_exception_type( zaza.utilities.exceptions.DestroyModelFailed)): with attempt: diff --git a/zaza/utilities/openstack_provider.py b/zaza/utilities/openstack_provider.py index fe2780d80..6f6e33bd0 100644 --- a/zaza/utilities/openstack_provider.py +++ b/zaza/utilities/openstack_provider.py @@ -267,20 +267,39 @@ def resource_removed(resource, msg) -def clean_up_instances(model_name): - """ - TODO: fill this out +def clean_up_instances(model_name, machines): + """Clean up any remaining instances that might exist on OpenStack. + + This is used after delete to remove any instances that might exists after + destroy model. It does this by matching the model name to the OpenStack + instance name, where the model_name is a part of the name. + + :param model_name: the model to destroy. + :type model_name: str + :param machines: the value of get_status(model_name)['machines'] prior to + model deletion. + :type machines: List[???] """ + machine_ids = [d.instance_id for d in machines.values()] session = get_undercloud_keystone_session() nova_client = get_nova_session_client(session) - servers = list(nova_client.servers.list()) + servers = [s for s in nova_client.servers.list() if s.id in machine_ids] if servers: - logging.warning("Having to clean-up {} servers after supposed destroy" + logging.warning("Possibly having to clean-up {} servers after " + " destroy - due to async they may already be gone." .format(len(servers))) for server in servers: - if model_name in server.name: - logging.info('Removing server {}'.format(server.name)) + try: delete_resource( nova_client.servers, server.id, msg="server") + logging.info("Removed server {} - id:{}" + .format(server.name, server.id)) + except novaclient_client.exceptions.NotFound: + # Due to the async nature of all the bits of technology, + # sometimes OpenStack will report the server existing despite + # having removed it. We get this exception if it was going + # depite being in the list, so just ignore this error. + logging.info("Server {} already removed - race due to async." + " id:{}" .format(server.name, server.id)) From da50602e2bbcfd4a8328f6d7e8c5fa495da96595 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Fri, 29 Jan 2021 10:33:47 +0000 Subject: [PATCH 5/5] Update docstrings based on review feedback --- unit_tests/test_zaza_utilities_openstack_provider.py | 2 +- zaza/utilities/openstack_provider.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/unit_tests/test_zaza_utilities_openstack_provider.py b/unit_tests/test_zaza_utilities_openstack_provider.py index 4e88666fe..a16a29a2e 100644 --- a/unit_tests/test_zaza_utilities_openstack_provider.py +++ b/unit_tests/test_zaza_utilities_openstack_provider.py @@ -1,4 +1,4 @@ -# Copyright 2018 Canonical Ltd. +# Copyright 2021 Canonical Ltd. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. diff --git a/zaza/utilities/openstack_provider.py b/zaza/utilities/openstack_provider.py index 6f6e33bd0..33e9f2d05 100644 --- a/zaza/utilities/openstack_provider.py +++ b/zaza/utilities/openstack_provider.py @@ -33,7 +33,7 @@ class MissingOSAthenticationException(Exception): def get_undercloud_keystone_session(verify=None): - """Return Under cloud keystone session. + """Return Undercloud keystone session. :param verify: Control TLS certificate verification behaviour :type verify: any @@ -215,7 +215,7 @@ def delete_resource(resource, resource_id, msg="resource"): def _resource_removed(resource, resource_id, msg="resource"): - """Wait for an openstack resource to no longer be present. + """Raise AssertError if a resource is still longer present. :param resource: pointer to os resource type, ex: heat_client.stacks :type resource: str