From 74522a1a224a5379649645ad57e916fe003638bd Mon Sep 17 00:00:00 2001 From: Monty Taylor Date: Tue, 20 Jun 2017 16:10:01 -0500 Subject: [PATCH] Fix delete_ips on delete_server and add tests This codepath was basically completely untested and essentially completely broken. Fix the break, but also add tests for it. Also add tests for config_drive while we're in there. Change-Id: I9f44474000213bab9e08266a376e41fecc4fbc1f --- .../fix-delete-ips-1d4eebf7bc4d4733.yaml | 6 + shade/openstackcloud.py | 17 ++- shade/tests/functional/test_compute.py | 17 +++ shade/tests/unit/test_delete_server.py | 123 ++++++++++++++++++ 4 files changed, 158 insertions(+), 5 deletions(-) create mode 100644 releasenotes/notes/fix-delete-ips-1d4eebf7bc4d4733.yaml diff --git a/releasenotes/notes/fix-delete-ips-1d4eebf7bc4d4733.yaml b/releasenotes/notes/fix-delete-ips-1d4eebf7bc4d4733.yaml new file mode 100644 index 000000000..7d8199dee --- /dev/null +++ b/releasenotes/notes/fix-delete-ips-1d4eebf7bc4d4733.yaml @@ -0,0 +1,6 @@ +--- +issues: + - Fixed the logic in delete_ips and added regression + tests to cover it. The old logic was incorrectly looking + for floating ips using port syntax. It was also not + swallowing errors when it should. diff --git a/shade/openstackcloud.py b/shade/openstackcloud.py index 3efef0832..ef818235c 100644 --- a/shade/openstackcloud.py +++ b/shade/openstackcloud.py @@ -5895,11 +5895,18 @@ class OpenStackCloud( # addresses dict? If not, skip this. server_floats = meta.find_nova_interfaces( server['addresses'], ext_tag='floating') - if not server_floats: - return - ips = self.search_floating_ips(filters={ - 'device_id': server['id']}) - for ip in ips: + for fip in server_floats: + try: + ip = self.get_floating_ip(id=None, filters={ + 'floating_ip_address': fip['addr']}) + except OpenStackCloudURINotFound: + # We're deleting. If it doesn't exist - awesome + # NOTE(mordred) If the cloud is a nova FIP cloud but + # floating_ip_source is set to neutron, this + # can lead to a FIP leak. + continue + if not ip: + continue deleted = self.delete_floating_ip( ip['id'], retry=delete_ip_retry) if not deleted: diff --git a/shade/tests/functional/test_compute.py b/shade/tests/functional/test_compute.py index 45883c782..d52dfedf7 100644 --- a/shade/tests/functional/test_compute.py +++ b/shade/tests/functional/test_compute.py @@ -71,6 +71,23 @@ class TestCompute(base.BaseFunctionalTestCase): self.user_cloud.delete_server(self.server_name, wait=True)) self.assertIsNone(self.user_cloud.get_server(self.server_name)) + def test_create_and_delete_server_auto_ip_delete_ips(self): + self.addCleanup(self._cleanup_servers_and_volumes, self.server_name) + server = self.user_cloud.create_server( + name=self.server_name, + image=self.image, + flavor=self.flavor, + auto_ip=True, + wait=True) + self.assertEqual(self.server_name, server['name']) + self.assertEqual(self.image.id, server['image']['id']) + self.assertEqual(self.flavor.id, server['flavor']['id']) + self.assertIsNotNone(server['adminPass']) + self.assertTrue( + self.user_cloud.delete_server( + self.server_name, wait=True, delete_ips=True)) + self.assertIsNone(self.user_cloud.get_server(self.server_name)) + def test_attach_detach_volume(self): server_name = self.getUniqueString() self.addCleanup(self._cleanup_servers_and_volumes, server_name) diff --git a/shade/tests/unit/test_delete_server.py b/shade/tests/unit/test_delete_server.py index bcbe0497b..0e558e509 100644 --- a/shade/tests/unit/test_delete_server.py +++ b/shade/tests/unit/test_delete_server.py @@ -16,6 +16,7 @@ test_delete_server Tests for the `delete_server` command. """ +import uuid from shade import exc as shade_exc from shade.tests import fakes @@ -140,3 +141,125 @@ class TestDeleteServer(base.RequestsMockTestCase): self.assertTrue(self.cloud.delete_server('porky', wait=False)) self.assert_calls() + + def test_delete_server_delete_ips(self): + """ + Test that deleting server and fips works + """ + server = fakes.make_fake_server('1234', 'porky', 'ACTIVE') + fip_id = uuid.uuid4().hex + + self.register_uris([ + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', append=['servers', 'detail']), + json={'servers': [server]}), + dict(method='GET', + uri=self.get_mock_url( + 'network', 'public', append=['v2.0', 'floatingips.json'], + qs_elements=['floating_ip_address=172.24.5.5']), + complete_qs=True, + json={'floatingips': [{ + 'router_id': 'd23abc8d-2991-4a55-ba98-2aaea84cc72f', + 'tenant_id': '4969c491a3c74ee4af974e6d800c62de', + 'floating_network_id': '376da547-b977-4cfe-9cba7', + 'fixed_ip_address': '10.0.0.4', + 'floating_ip_address': '172.24.5.5', + 'port_id': 'ce705c24-c1ef-408a-bda3-7bbd946164ac', + 'id': fip_id, + 'status': 'ACTIVE'}]}), + dict(method='DELETE', + uri=self.get_mock_url( + 'network', 'public', + append=['v2.0', 'floatingips', + '{fip_id}.json'.format(fip_id=fip_id)])), + dict(method='GET', + uri=self.get_mock_url( + 'network', 'public', append=['v2.0', 'floatingips.json']), + complete_qs=True, + json={'floatingips': []}), + dict(method='DELETE', + uri=self.get_mock_url( + 'compute', 'public', append=['servers', '1234'])), + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', append=['servers', 'detail']), + json={'servers': []}), + ]) + self.assertTrue(self.cloud.delete_server( + 'porky', wait=True, delete_ips=True)) + + self.assert_calls() + + def test_delete_server_delete_ips_bad_neutron(self): + """ + Test that deleting server with a borked neutron doesn't bork + """ + server = fakes.make_fake_server('1234', 'porky', 'ACTIVE') + + self.register_uris([ + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', append=['servers', 'detail']), + json={'servers': [server]}), + dict(method='GET', + uri=self.get_mock_url( + 'network', 'public', append=['v2.0', 'floatingips.json'], + qs_elements=['floating_ip_address=172.24.5.5']), + complete_qs=True, + status_code=404), + dict(method='DELETE', + uri=self.get_mock_url( + 'compute', 'public', append=['servers', '1234'])), + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', append=['servers', 'detail']), + json={'servers': []}), + ]) + self.assertTrue(self.cloud.delete_server( + 'porky', wait=True, delete_ips=True)) + + self.assert_calls() + + def test_delete_server_delete_fips_nova(self): + """ + Test that deleting server with a borked neutron doesn't bork + """ + self.cloud._floating_ip_source = 'nova' + server = fakes.make_fake_server('1234', 'porky', 'ACTIVE') + + self.register_uris([ + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', append=['servers', 'detail']), + json={'servers': [server]}), + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', append=['os-floating-ips']), + json={'floating_ips': [ + { + 'fixed_ip': None, + 'id': 1, + 'instance_id': None, + 'ip': '172.24.5.5', + 'pool': 'nova' + }]}), + dict(method='DELETE', + uri=self.get_mock_url( + 'compute', 'public', append=['os-floating-ips', '1'])), + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', append=['os-floating-ips']), + json={'floating_ips': []}), + dict(method='DELETE', + uri=self.get_mock_url( + 'compute', 'public', append=['servers', '1234'])), + dict(method='GET', + uri=self.get_mock_url( + 'compute', 'public', append=['servers', 'detail']), + json={'servers': []}), + ]) + self.assertTrue(self.cloud.delete_server( + 'porky', wait=True, delete_ips=True)) + + self.assert_calls()