From d9455f6d8e09d4a80e2f3f03596985c598d19aa9 Mon Sep 17 00:00:00 2001 From: Monty Taylor Date: Thu, 21 Apr 2016 10:12:14 -0500 Subject: [PATCH] Fail if FIP doens't have the requested port_id When we create a FIP on a port, it should return with the port in question. Also, it turns out we can wait for the floating IP to reach ACTIVE status. Finally, our normalize method for fips was producing inconsistent values depending on create or list - so fix that. Change-Id: I56b4136a77dd61b6ef1832759b8169dd53aa49da --- shade/_utils.py | 24 +++--- shade/openstackcloud.py | 83 +++++++++++++++++--- shade/tests/unit/test_floating_ip_neutron.py | 31 ++++++-- 3 files changed, 110 insertions(+), 28 deletions(-) diff --git a/shade/_utils.py b/shade/_utils.py index ccc13b5f1..25285d255 100644 --- a/shade/_utils.py +++ b/shade/_utils.py @@ -313,15 +313,21 @@ def normalize_neutron_floating_ips(ips): ] """ - ret = [dict( - id=ip['id'], - fixed_ip_address=ip.get('fixed_ip_address'), - floating_ip_address=ip['floating_ip_address'], - network=ip['floating_network_id'], - attached=(ip.get('port_id') is not None and - ip.get('port_id') != ''), - status=ip['status'] - ) for ip in ips] + ret = [] + for ip in ips: + network_id = ip.get('floating_network_id', ip.get('network')) + ret.append(dict( + id=ip['id'], + fixed_ip_address=ip.get('fixed_ip_address'), + floating_ip_address=ip['floating_ip_address'], + network=network_id, + floating_network_id=network_id, + port_id=ip.get('port_id'), + router_id=ip.get('router_id'), + attached=(ip.get('port_id') is not None and + ip.get('port_id') != ''), + status=ip['status'], + )) return meta.obj_list_to_dict(ret) diff --git a/shade/openstackcloud.py b/shade/openstackcloud.py index c5c3785f8..4dc9e63b7 100644 --- a/shade/openstackcloud.py +++ b/shade/openstackcloud.py @@ -3165,7 +3165,8 @@ class OpenStackCloud(object): return [f_ip] def create_floating_ip(self, network=None, server=None, - fixed_address=None, nat_destination=None): + fixed_address=None, nat_destination=None, + wait=False, timeout=60): """Allocate a new floating IP from a network or a pool. :param network: Nova pool name or Neutron network name or id @@ -3177,6 +3178,12 @@ class OpenStackCloud(object): :param nat_destination: (optional) Name or id of the network that the fixed IP to attach the floating IP to should be on. + :param wait: (optional) Whether to wait for the IP to be active. + Defaults to False. Only applies if a server is + provided. + :param timeout: (optional) How long to wait for the IP to be active. + Defaults to 60. Only applies if a server is + provided. :returns: a floating IP address @@ -3184,13 +3191,11 @@ class OpenStackCloud(object): """ if self._use_neutron_floating(): try: - f_ips = _utils.normalize_neutron_floating_ips( - [self._neutron_create_floating_ip( - network_name_or_id=network, server=server, - fixed_address=fixed_address, - nat_destination=nat_destination)] - ) - return f_ips[0] + return self._neutron_create_floating_ip( + network_name_or_id=network, server=server, + fixed_address=fixed_address, + nat_destination=nat_destination, + wait=wait, timeout=timeout) except OpenStackCloudURINotFound as e: self.log.debug( "Something went wrong talking to neutron API: " @@ -3202,9 +3207,15 @@ class OpenStackCloud(object): [self._nova_create_floating_ip(pool=network)]) return f_ips[0] + def _submit_create_fip(self, kwargs): + # Split into a method to aid in test mocking + return _utils.normalize_neutron_floating_ips( + [self.manager.submitTask(_tasks.NeutronFloatingIPCreate( + body={'floatingip': kwargs}))['floatingip']])[0] + def _neutron_create_floating_ip( self, network_name_or_id=None, server=None, - fixed_address=None, nat_destination=None): + fixed_address=None, nat_destination=None, wait=False, timeout=60): with _utils.neutron_exceptions( "unable to create floating IP for net " "{0}".format(network_name_or_id)): @@ -3223,6 +3234,7 @@ class OpenStackCloud(object): kwargs = { 'floating_network_id': networks[0]['id'], } + port = None if server: (port, fixed_ip_address) = self._get_free_fixed_port( server, fixed_address=fixed_address, @@ -3230,8 +3242,37 @@ class OpenStackCloud(object): if port: kwargs['port_id'] = port['id'] kwargs['fixed_ip_address'] = fixed_ip_address - return self.manager.submitTask(_tasks.NeutronFloatingIPCreate( - body={'floatingip': kwargs}))['floatingip'] + fip = self._submit_create_fip(kwargs) + fip_id = fip['id'] + + if port: + if fip['port_id'] != port['id']: + raise OpenStackCloudException( + "Attempted to create FIP on port {port} for server" + " {server} but something went wrong".format( + port=port['id'], server=server['id'])) + # The FIP is only going to become active in this context + # when we've attached it to something, which only occurs + # if we've provided a port as a parameter + if wait: + try: + for count in _utils._iterate_timeout( + timeout, + "Timeout waiting for the floating IP" + " to be ACTIVE"): + fip = self.get_floating_ip(fip_id) + if fip['status'] == 'ACTIVE': + break + except OpenStackCloudTimeout: + self.log.error( + "Timed out on floating ip {fip} becoming active." + " Deleting".format(fip=fip_id)) + try: + self.delete_floating_ip(fip_id) + except Exception: + pass + raise + return fip def _nova_create_floating_ip(self, pool=None): with _utils.shade_exceptions( @@ -3618,9 +3659,17 @@ class OpenStackCloud(object): if reuse: f_ip = self.available_floating_ip(network=network) else: + start_time = time.time() f_ip = self.create_floating_ip( - network=network, nat_destination=nat_destination) + network=network, nat_destination=nat_destination, + wait=wait, timeout=timeout) + timeout = timeout - (time.time() - start_time) + # We run attach as a second call rather than in the create call + # because there are code flows where we will not have an attached + # FIP yet. However, even if it was attached in the create, we run + # the attach function below to get back the server dict refreshed + # with the FIP information. return self._attach_ip_to_server( server=server, floating_ip=f_ip, fixed_address=fixed_address, wait=wait, timeout=timeout) @@ -3685,7 +3734,10 @@ class OpenStackCloud(object): if reuse: f_ip = self.available_floating_ip() else: - f_ip = self.create_floating_ip(server=server) + start_time = time.time() + f_ip = self.create_floating_ip( + server=server, wait=wait, timeout=timeout) + timeout = timeout - (time.time() - start_time) if server: # This gets passed in for both nova and neutron # but is only meaninful for the neutron logic branch @@ -3693,6 +3745,11 @@ class OpenStackCloud(object): created = True try: + # We run attach as a second call rather than in the create call + # because there are code flows where we will not have an attached + # FIP yet. However, even if it was attached in the create, we run + # the attach function below to get back the server dict refreshed + # with the FIP information. return self._attach_ip_to_server( server=server, floating_ip=f_ip, wait=wait, timeout=timeout, skip_attach=skip_attach) diff --git a/shade/tests/unit/test_floating_ip_neutron.py b/shade/tests/unit/test_floating_ip_neutron.py index 85fb977ea..80dbe9536 100644 --- a/shade/tests/unit/test_floating_ip_neutron.py +++ b/shade/tests/unit/test_floating_ip_neutron.py @@ -19,6 +19,7 @@ test_floating_ip_neutron Tests Floating IP resource methods for Neutron """ +import mock from mock import patch import os_client_config @@ -340,20 +341,22 @@ class TestFloatingIP(base.TestCase): mock_keystone_session, mock_nova_client): mock_has_service.return_value = True - mock__neutron_create_floating_ip.return_value = \ - self.mock_floating_ip_list_rep['floatingips'][0] + fip = _utils.normalize_neutron_floating_ips( + self.mock_floating_ip_list_rep['floatingips'])[0] + mock__neutron_create_floating_ip.return_value = fip mock_keystone_session.get_project_id.return_value = \ '4969c491a3c74ee4af974e6d800c62df' + fake_server = meta.obj_to_dict(fakes.FakeServer('1234', '', 'ACTIVE')) self.client.add_ips_to_server( - dict(id='1234'), ip_pool='my-network', reuse=False) + fake_server, ip_pool='my-network', reuse=False) mock__neutron_create_floating_ip.assert_called_once_with( network_name_or_id='my-network', server=None, - fixed_address=None, nat_destination=None) + fixed_address=None, nat_destination=None, wait=False, timeout=60) mock_attach_ip_to_server.assert_called_once_with( - server={'id': '1234'}, fixed_address=None, - floating_ip=self.floating_ip, wait=False, timeout=60) + server=fake_server, fixed_address=None, + floating_ip=fip, wait=False, timeout=mock.ANY) @patch.object(OpenStackCloud, 'keystone_session') @patch.object(OpenStackCloud, '_neutron_create_floating_ip') @@ -564,3 +567,19 @@ class TestFloatingIP(base.TestCase): mock_delete_floating_ip.assert_called_once_with( id="this-is-a-floating-ip-id", timeout=60, wait=False) + + @patch.object(OpenStackCloud, '_submit_create_fip') + @patch.object(OpenStackCloud, '_get_free_fixed_port') + @patch.object(OpenStackCloud, 'get_external_networks') + def test_create_floating_ip_no_port( + self, mock_get_ext_nets, mock_get_free_fixed_port, + mock_submit_create_fip): + fake_port = dict(id='port-id') + mock_get_ext_nets.return_value = [self.mock_get_network_rep] + mock_get_free_fixed_port.return_value = (fake_port, '10.0.0.2') + mock_submit_create_fip.return_value = dict(port_id=None) + + self.assertRaises( + exc.OpenStackCloudException, + self.client._neutron_create_floating_ip, + server=dict(id='some-server'))