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

@@ -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

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 = [

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
 
 

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

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:

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):