From d14ebc2cdf763e83998b2baa151800fb518a1756 Mon Sep 17 00:00:00 2001 From: Brandon Logan Date: Tue, 15 Sep 2015 17:27:39 -0500 Subject: [PATCH] Fix issues uncovered by neutron lbaas tempest tests Updating a listener caused issues because SNI was not being appropriately handles on a listener update. This has been "fixed" but another patch will be needed to make sure that every listener update does not clear out the list. I think it is best to leave that for another review though. This also fixes the bug where security groups were not getting cleaned up when a load balancer is deleted. Since neutron does not synchronously remove a security group from a port, a retry mechanism had to be used and thus more config options to tune the interval and max attempts. Change-Id: I0434b8ced144ab08413b91569bd008295ef1784e Closes-Bug: #1464953 --- etc/octavia.conf | 4 ++ octavia/common/config.py | 6 +++ .../controller/worker/tasks/network_tasks.py | 8 ++- octavia/db/repositories.py | 11 ++++ .../drivers/neutron/allowed_address_pairs.py | 50 ++++++++++++++++++- .../neutron/test_allowed_address_pairs.py | 37 ++++++++++++-- 6 files changed, 111 insertions(+), 5 deletions(-) diff --git a/etc/octavia.conf b/etc/octavia.conf index 1ea60041a3..fbf80d3934 100644 --- a/etc/octavia.conf +++ b/etc/octavia.conf @@ -71,6 +71,10 @@ [networking] # Network to communicate with amphora # lb_network_name = +# The maximum attempts to retry an action with the networking service. +# max_retries = 5 +# Seconds to wait before retrying an action with the networking service. +# retry_interval = 1 [haproxy_amphora] # base_path = /var/lib/octavia diff --git a/octavia/common/config.py b/octavia/common/config.py index 11e3ce9c65..661af67d0a 100644 --- a/octavia/common/config.py +++ b/octavia/common/config.py @@ -97,6 +97,12 @@ amphora_agent_opts = [ networking_opts = [ cfg.StrOpt('lb_network_name', help=_('Name of amphora internal network')), + cfg.IntOpt('max_retries', default=5, + help=_('The maximum attempts to retry an action with the ' + 'networking service.')), + cfg.IntOpt('retry_interval', default=1, + help=_('Seconds to wait before retrying an action with the ' + 'networking service.')) ] healthmanager_opts = [ diff --git a/octavia/controller/worker/tasks/network_tasks.py b/octavia/controller/worker/tasks/network_tasks.py index bbb1312e78..c104566d41 100644 --- a/octavia/controller/worker/tasks/network_tasks.py +++ b/octavia/controller/worker/tasks/network_tasks.py @@ -339,7 +339,13 @@ class DeallocateVIP(BaseNetworkTask): LOG.debug("Deallocating a VIP %s", loadbalancer.vip.ip_address) - self.network_driver.deallocate_vip(loadbalancer.vip) + # NOTE(blogan): this is kind of ugly but sufficient for now. Drivers + # will need access to the load balancer that the vip is/was attached + # to. However the data model serialization for the vip does not give a + # backref to the loadbalancer if accessed through the loadbalancer. + vip = loadbalancer.vip + vip.load_balancer = loadbalancer + self.network_driver.deallocate_vip(vip) return diff --git a/octavia/db/repositories.py b/octavia/db/repositories.py index abc778b946..e05ddb58bd 100644 --- a/octavia/db/repositories.py +++ b/octavia/db/repositories.py @@ -301,6 +301,17 @@ class ListenerRepository(BaseRepository): listener = self.get(session, id=id) return bool(listener.default_pool) + def update(self, session, id, **model_kwargs): + with session.begin(subtransactions=True): + listener_db = session.query(self.model_class).filter_by( + id=id).first() + sni_containers = model_kwargs.pop('sni_containers', []) + for container_ref in sni_containers: + sni = models.SNI(listener_id=id, + tls_certificate_id=container_ref) + listener_db.sni_containers.append(sni) + listener_db.update(model_kwargs) + class ListenerStatisticsRepository(BaseRepository): model_class = models.ListenerStatistics diff --git a/octavia/network/drivers/neutron/allowed_address_pairs.py b/octavia/network/drivers/neutron/allowed_address_pairs.py index 0662724137..f856675ee8 100644 --- a/octavia/network/drivers/neutron/allowed_address_pairs.py +++ b/octavia/network/drivers/neutron/allowed_address_pairs.py @@ -12,14 +12,17 @@ # License for the specific language governing permissions and limitations # under the License. +import time + from neutronclient.common import exceptions as neutron_client_exceptions from novaclient import exceptions as nova_client_exceptions +from oslo_config import cfg from oslo_log import log as logging from octavia.common import clients from octavia.common import constants from octavia.common import data_models -from octavia.i18n import _LE, _LI +from octavia.i18n import _LE, _LI, _LW from octavia.network import base from octavia.network.drivers.neutron import base as neutron_base from octavia.network.drivers.neutron import utils @@ -134,6 +137,35 @@ class AllowedAddressPairsDriver(neutron_base.BaseNeutronDriver): except base.NetworkException as e: raise base.PlugVIPException(str(e)) + def _delete_vip_security_group(self, sec_grp): + """Deletes a security group in neutron. + + Retries upon an exception because removing a security group from + a neutron port does not happen immediately. + """ + attempts = 0 + while attempts <= cfg.CONF.networking.max_retries: + try: + self.neutron_client.delete_security_group(sec_grp) + LOG.info(_LI("Deleted security group {0}.").format( + sec_grp)) + return + except neutron_client_exceptions.NotFound: + message = _LI("Security group {0} not found, will assume it is" + " already deleted").format(sec_grp) + LOG.info(message) + return + except Exception: + message = _LW("Attempt {0} to remove security group {1} " + "failed.").format(attempts + 1, sec_grp) + LOG.warning(message) + attempts += 1 + time.sleep(cfg.CONF.networking.retry_interval) + message = _LE("All attempts to remove security group {0} have " + "failed.").format(sec_grp) + LOG.exception(message) + raise base.DeallocateVIPException(message) + def deallocate_vip(self, vip): try: port = self.get_port(vip.port_id) @@ -144,6 +176,18 @@ class AllowedAddressPairsDriver(neutron_base.BaseNeutronDriver): if port.device_owner != OCTAVIA_OWNER: LOG.info(_LI("Port {0} will not be deleted by Octavia as it was " "not created by Octavia.").format(vip.port_id)) + if self.sec_grp_enabled: + sec_grp = self._get_lb_security_group(vip.load_balancer.id) + sec_grp = sec_grp.get('id') + LOG.info(_LI("Removing security group {0} from port " + "{1}").format(sec_grp, vip.port_id)) + raw_port = self.neutron_client.show_port(port.id) + sec_grps = raw_port.get('port', {}).get('security_groups', []) + if sec_grp in sec_grps: + sec_grps.remove(sec_grp) + port_update = {'port': {'security_groups': sec_grps}} + self.neutron_client.update_port(port.id, port_update) + self._delete_vip_security_group(sec_grp) return try: self.neutron_client.delete_port(vip.port_id) @@ -152,6 +196,10 @@ class AllowedAddressPairsDriver(neutron_base.BaseNeutronDriver): 'neutron').format(port_id=vip.port_id) LOG.exception(message) raise base.DeallocateVIPException(message) + if self.sec_grp_enabled: + sec_grp = self._get_lb_security_group(vip.load_balancer.id) + sec_grp = sec_grp.get('id') + self._delete_vip_security_group(sec_grp) def plug_vip(self, load_balancer, vip): if self.sec_grp_enabled: diff --git a/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py b/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py index 401c043d90..759c9c07a5 100644 --- a/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py +++ b/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py @@ -97,11 +97,25 @@ class TestAllowedAddressPairsDriver(base.TestCase): self.assertEqual([], unpluggers) def test_deallocate_vip(self): - vip = data_models.Vip(port_id='1') + lb = dmh.generate_load_balancer_tree() + lb.vip.load_balancer = lb + vip = lb.vip + sec_grp_id = 'lb-sec-grp1' show_port = self.driver.neutron_client.show_port show_port.return_value = {'port': { 'device_owner': allowed_address_pairs.OCTAVIA_OWNER}} + delete_port = self.driver.neutron_client.delete_port + delete_sec_grp = self.driver.neutron_client.delete_security_group + list_security_groups = self.driver.neutron_client.list_security_groups + security_groups = { + 'security_groups': [ + {'id': sec_grp_id} + ] + } + list_security_groups.return_value = security_groups self.driver.deallocate_vip(vip) + delete_port.assert_called_once_with(vip.port_id) + delete_sec_grp.assert_called_once_with(sec_grp_id) def test_deallocate_vip_when_delete_port_fails(self): vip = data_models.Vip(port_id='1') @@ -121,12 +135,29 @@ class TestAllowedAddressPairsDriver(base.TestCase): self.driver.deallocate_vip, vip) def test_deallocate_vip_when_port_not_owned_by_octavia(self): - vip = data_models.Vip(port_id='1') + lb = dmh.generate_load_balancer_tree() + lb.vip.load_balancer = lb + vip = lb.vip + sec_grp_id = 'lb-sec-grp1' show_port = self.driver.neutron_client.show_port show_port.return_value = {'port': { - 'device_owner': 'neutron:LOADBALANCERV2'}} + 'id': vip.port_id, + 'device_owner': 'neutron:LOADBALANCERV2', + 'security_groups': [sec_grp_id]}} delete_port = self.driver.neutron_client.delete_port + update_port = self.driver.neutron_client.update_port + delete_sec_grp = self.driver.neutron_client.delete_security_group + list_security_groups = self.driver.neutron_client.list_security_groups + security_groups = { + 'security_groups': [ + {'id': sec_grp_id} + ] + } + list_security_groups.return_value = security_groups self.driver.deallocate_vip(vip) + expected_port_update = {'port': {'security_groups': []}} + update_port.assert_called_once_with(vip.port_id, expected_port_update) + delete_sec_grp.assert_called_once_with(sec_grp_id) self.assertFalse(delete_port.called) def test_deallocate_vip_when_vip_port_not_found(self):