From 5785b9755e76dbf3a8ff58b6c91a5c65400685ec Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Mon, 9 Jul 2018 18:36:39 -0700 Subject: [PATCH] Cleanup Octavia create VIP ports on LB delete This patch updates the Octavia load balancer VIP handling to deallocate the VIP if it was created by Octavia. User provided or provider driver supplied VIPs will not be deallocated. Change-Id: Idb62a53197975a4aa52fbc3fb5ee192f261b08d8 --- octavia/api/drivers/driver_lib.py | 19 +++++++++-- octavia/api/v2/controllers/load_balancer.py | 13 ++++--- octavia/common/data_models.py | 3 +- ...5e_add_octavia_owned_vip_column_to_vip_.py | 34 +++++++++++++++++++ octavia/db/models.py | 1 + .../tests/functional/db/test_repositories.py | 2 +- .../tests/unit/api/drivers/test_driver_lib.py | 29 +++++++++++++++- 7 files changed, 90 insertions(+), 11 deletions(-) create mode 100644 octavia/db/migration/alembic_migrations/versions/ebbcc72b4e5e_add_octavia_owned_vip_column_to_vip_.py diff --git a/octavia/api/drivers/driver_lib.py b/octavia/api/drivers/driver_lib.py index 242b995a68..2f605cfa3b 100644 --- a/octavia/api/drivers/driver_lib.py +++ b/octavia/api/drivers/driver_lib.py @@ -14,6 +14,7 @@ from octavia.api.drivers import exceptions as driver_exceptions from octavia.common import constants as consts +from octavia.common import utils from octavia.db import api as db_apis from octavia.db import repositories as repo @@ -33,6 +34,16 @@ class DriverLibrary(object): self.db_session = db_apis.get_session() super(DriverLibrary, self).__init__(**kwargs) + def _check_for_lb_vip_deallocate(self, repo, lb_id): + lb = repo.get(self.db_session, id=lb_id) + if lb.vip.octavia_owned: + vip = lb.vip + # We need a backreference + vip.load_balancer = lb + # Only lookup the network driver if we have a VIP to deallocate + network_driver = utils.get_network_driver() + network_driver.deallocate_vip(vip) + def _process_status_update(self, repo, object_name, record, delete_record=False): # Zero it out so that if the ID is missing from a record we do not @@ -43,11 +54,13 @@ class DriverLibrary(object): record_kwargs = {} prov_status = record.get(consts.PROVISIONING_STATUS, None) if prov_status: - if prov_status == consts.DELETED and delete_record: + if (prov_status == consts.DELETED and + object_name == consts.LOADBALANCERS): + self._check_for_lb_vip_deallocate(repo, record_id) + elif prov_status == consts.DELETED and delete_record: repo.delete(self.db_session, id=record_id) return - else: - record_kwargs[consts.PROVISIONING_STATUS] = prov_status + record_kwargs[consts.PROVISIONING_STATUS] = prov_status op_status = record.get(consts.OPERATING_STATUS, None) if op_status: record_kwargs[consts.OPERATING_STATUS] = op_status diff --git a/octavia/api/v2/controllers/load_balancer.py b/octavia/api/v2/controllers/load_balancer.py index 6bc897f370..c42c583bd7 100644 --- a/octavia/api/v2/controllers/load_balancer.py +++ b/octavia/api/v2/controllers/load_balancer.py @@ -289,6 +289,7 @@ class LoadBalancersController(base.BaseController): lock_session, lb_dict, vip_dict) # See if the provider driver wants to create the VIP port + octavia_owned = False try: provider_vip_dict = driver_utils.vip_dict_to_provider_dict( vip_dict) @@ -302,13 +303,15 @@ class LoadBalancersController(base.BaseController): vip = self._create_vip_port_if_not_exist(db_lb) LOG.info('Created VIP port %s for provider %s.', vip.port_id, driver.name) + # If a port_id wasn't passed in and we made it this far + # we created the VIP + if 'port_id' not in vip_dict or not vip_dict['port_id']: + octavia_owned = True self.repositories.vip.update( - lock_session, db_lb.id, - ip_address=vip.ip_address, - port_id=vip.port_id, - network_id=vip.network_id, - subnet_id=vip.subnet_id) + lock_session, db_lb.id, ip_address=vip.ip_address, + port_id=vip.port_id, network_id=vip.network_id, + subnet_id=vip.subnet_id, octavia_owned=octavia_owned) if listeners or pools: db_pools, db_lists = self._graph_create( diff --git a/octavia/common/data_models.py b/octavia/common/data_models.py index f0af12c8ff..38d6067303 100644 --- a/octavia/common/data_models.py +++ b/octavia/common/data_models.py @@ -471,7 +471,7 @@ class Vip(BaseDataModel): def __init__(self, load_balancer_id=None, ip_address=None, subnet_id=None, network_id=None, port_id=None, - load_balancer=None, qos_policy_id=None): + load_balancer=None, qos_policy_id=None, octavia_owned=None): self.load_balancer_id = load_balancer_id self.ip_address = ip_address self.subnet_id = subnet_id @@ -479,6 +479,7 @@ class Vip(BaseDataModel): self.port_id = port_id self.load_balancer = load_balancer self.qos_policy_id = qos_policy_id + self.octavia_owned = octavia_owned class SNI(BaseDataModel): diff --git a/octavia/db/migration/alembic_migrations/versions/ebbcc72b4e5e_add_octavia_owned_vip_column_to_vip_.py b/octavia/db/migration/alembic_migrations/versions/ebbcc72b4e5e_add_octavia_owned_vip_column_to_vip_.py new file mode 100644 index 0000000000..b595bdb0df --- /dev/null +++ b/octavia/db/migration/alembic_migrations/versions/ebbcc72b4e5e_add_octavia_owned_vip_column_to_vip_.py @@ -0,0 +1,34 @@ +# Copyright 2018 Rackspace, US Inc. +# +# 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. +"""Add Octavia owned VIP column to VIP table + +Revision ID: ebbcc72b4e5e +Revises: 0f242cf02c74 +Create Date: 2018-07-09 17:25:30.137527 + +""" + +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision = 'ebbcc72b4e5e' +down_revision = '0f242cf02c74' + + +def upgrade(): + op.add_column( + u'vip', + sa.Column(u'octavia_owned', sa.Boolean(), nullable=True) + ) diff --git a/octavia/db/models.py b/octavia/db/models.py index d0703afbd2..d7adff989b 100644 --- a/octavia/db/models.py +++ b/octavia/db/models.py @@ -389,6 +389,7 @@ class Vip(base_models.BASE): backref=orm.backref("vip", uselist=False, cascade="delete")) qos_policy_id = sa.Column(sa.String(36), nullable=True) + octavia_owned = sa.Column(sa.Boolean(), nullable=True) class Listener(base_models.BASE, base_models.IdMixin, diff --git a/octavia/tests/functional/db/test_repositories.py b/octavia/tests/functional/db/test_repositories.py index 38ea8fbf1c..df88e3a94f 100644 --- a/octavia/tests/functional/db/test_repositories.py +++ b/octavia/tests/functional/db/test_repositories.py @@ -135,7 +135,7 @@ class AllRepositoriesTest(base.OctaviaDBTestBase): 'port_id': uuidutils.generate_uuid(), 'subnet_id': uuidutils.generate_uuid(), 'network_id': uuidutils.generate_uuid(), - 'qos_policy_id': None} + 'qos_policy_id': None, 'octavia_owned': True} lb_dm = self.repos.create_load_balancer_and_vip(self.session, lb, vip) lb_dm_dict = lb_dm.to_dict() del lb_dm_dict['vip'] diff --git a/octavia/tests/unit/api/drivers/test_driver_lib.py b/octavia/tests/unit/api/drivers/test_driver_lib.py index ee3797864b..8b53ec0aa9 100644 --- a/octavia/tests/unit/api/drivers/test_driver_lib.py +++ b/octavia/tests/unit/api/drivers/test_driver_lib.py @@ -70,7 +70,28 @@ class TestDriverLib(base.TestCase): "total_connections": 100}] self.listener_stats_dict = {"listeners": listener_stats_list} - def test_process_status_update(self): + @mock.patch('octavia.common.utils.get_network_driver') + def test_check_for_lb_vip_deallocate(self, mock_get_driver): + mock_repo = mock.MagicMock() + mock_lb = mock.MagicMock() + + # Test VIP not owned by Octavia + mock_lb.vip.octavia_owned = False + mock_repo.get.return_value = mock_lb + self.driver_lib._check_for_lb_vip_deallocate(mock_repo, 4) + mock_get_driver.assert_not_called() + + # Test VIP is owned by Octavia + mock_lb.vip.octavia_owned = True + mock_repo.get.return_value = mock_lb + mock_net_driver = mock.MagicMock() + mock_get_driver.return_value = mock_net_driver + self.driver_lib._check_for_lb_vip_deallocate(mock_repo, 4) + mock_net_driver.deallocate_vip.assert_called_once_with(mock_lb.vip) + + @mock.patch('octavia.api.drivers.driver_lib.DriverLibrary.' + '_check_for_lb_vip_deallocate') + def test_process_status_update(self, mock_deallocate): mock_repo = mock.MagicMock() list_dict = {"id": 2, constants.PROVISIONING_STATUS: constants.ACTIVE, constants.OPERATING_STATUS: constants.ONLINE} @@ -129,6 +150,12 @@ class TestDriverLib(base.TestCase): mock_repo.delete.assert_called_once_with(self.mock_session, id=2) mock_repo.update.assert_not_called() + # Test with LB Delete + mock_repo.reset_mock() + self.driver_lib._process_status_update( + mock_repo, constants.LOADBALANCERS, list_deleted_dict) + mock_deallocate.assert_called_once_with(mock_repo, 2) + # Test with an exception mock_repo.reset_mock() mock_repo.update.side_effect = Exception('boom')