openstack
/
octavia
Code Issues Proposed changes

Fix session_persistence deletion bug 

Our code for updating a pool in the repository was off: You could
potentially update other pool parameters (ie. not session_persistence)
and it would unexpectedly clear the session_persistence. This patch
corrects this problem and cleans up the code dealing with session
persistence so that it's more understandable what it's doing.

There was also a bug in Neutron-LBaaS' handling of session_persistence
which was making troubleshooting this problem more difficult. This patch
is a prerequisite for the Neutron-LBaaS bugfix.

Beyond this, this patch also cleans up calls to
repository.update_pool_and_sp to not require the sp_dict parameter
(which, it turns out, did not need to be split out of the pool data
structure prior to calling this method anyway).

Closes-Bug: #1547157
Change-Id: Idcf12e463fbaa3a61a211f13986d8472f52036d2
This commit is contained in:
Stephen Balukoff 2016-03-02 03:12:30 -08:00
parent 972cdba6ee
commit 571d874faa
8 changed files with 69 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
octavia/api/v1/controllers/pool.py
View File

@ -89,7 +89,7 @@ class PoolsController(base.BaseController):
raise exceptions.ImmutableObject(resource=db_lb._name(),
id=self.load_balancer_id)
def _validate_create_pool(self, session, sp_dict, pool_dict):
def _validate_create_pool(self, session, pool_dict):
"""Validate creating pool on load balancer.
Update database for load balancer and (optional) listener based on
@ -97,8 +97,7 @@ class PoolsController(base.BaseController):
"""
try:
db_pool = self.repositories.create_pool_on_load_balancer(
session, pool_dict, listener_id=self.listener_id,
sp_dict=sp_dict)
session, pool_dict, listener_id=self.listener_id)
except odb_exceptions.DBDuplicateEntry as de:
if ['id'] == de.columns:
raise exceptions.IDAlreadyExists()
@ -151,11 +150,10 @@ class PoolsController(base.BaseController):
raise exceptions.DuplicatePoolEntry()
self._test_lb_and_listener_statuses(context.session)
sp_dict = pool_dict.pop('session_persistence', None)
pool_dict['operating_status'] = constants.OFFLINE
pool_dict['load_balancer_id'] = self.load_balancer_id
return self._validate_create_pool(context.session, sp_dict, pool_dict)
return self._validate_create_pool(context.session, pool_dict)
@wsme_pecan.wsexpose(pool_types.PoolResponse, wtypes.text,
body=pool_types.PoolPUT, status_code=202)

4
octavia/api/v1/handlers/controller_simulator/handler.py
View File

@ -196,9 +196,7 @@ def simulate_controller(data_model, delete=False, update=False, create=False):
db_pool = repo.pool.get(db_api.get_session(), id=pool.id)
pool_dict = pool.to_dict()
pool_dict['operating_status'] = db_pool.operating_status
sp_dict = pool_dict.pop('session_persistence', None)
repo.update_pool_and_sp(db_api.get_session(), pool.id,
pool_dict, sp_dict)
repo.update_pool_and_sp(db_api.get_session(), pool.id, pool_dict)
elif create:
repo.pool.update(db_api.get_session(), pool.id,
operating_status=constants.ONLINE)

5
octavia/common/data_models.py
View File

@ -193,7 +193,10 @@ class Pool(BaseDataModel):
def update(self, update_dict):
for key, value in update_dict.items():
if key == 'session_persistence':
if self.session_persistence is not None:
if value is None or value == {}:
if self.session_persistence is not None:
self.session_persistence.delete()
elif self.session_persistence is not None:
self.session_persistence.update(value)
else:
value.update({'pool_id': self.id})

5
octavia/controller/worker/tasks/database_tasks.py
View File

@ -1064,9 +1064,8 @@ class UpdatePoolInDB(BaseDatabaseTask):
"""
LOG.debug("Update DB for pool id: %s ", pool.id)
sp_dict = update_dict.pop('session_persistence', None)
self.repos.update_pool_and_sp(db_apis.get_session(), pool.id,
update_dict, sp_dict)
update_dict)
def revert(self, pool, *args, **kwargs):
"""Mark the pool ERROR since the update couldn't happen
@ -1078,7 +1077,7 @@ class UpdatePoolInDB(BaseDatabaseTask):
"for pool id %s"), pool.id)
# TODO(johnsom) fix this to set the upper ojects to ERROR
self.repos.update_pool_and_sp(db_apis.get_session(),
pool.id, {'enabled': 0}, None)
pool.id, {'enabled': 0})
class UpdateL7PolicyInDB(BaseDatabaseTask):

32
octavia/db/repositories.py
View File

@ -160,21 +160,21 @@ class Repositories(object):
return self.load_balancer.get(session, id=lb.id)
def create_pool_on_load_balancer(self, session, pool_dict,
listener_id=None, sp_dict=None):
listener_id=None):
"""Inserts a pool and session persistence entity into the database.
:param session: A Sql Alchemy database session.
:param pool_dict: Dictionary representation of a pool
:param listener_id: Optional listener id that will
reference this pool as its default_pool_id
:param sp_dict: Dictionary representation of a session persistence
:returns: octavia.common.data_models.Pool
"""
with session.begin(subtransactions=True):
if not pool_dict.get('id'):
pool_dict['id'] = uuidutils.generate_uuid()
sp_dict = pool_dict.pop('session_persistence', None)
db_pool = self.pool.create(session, **pool_dict)
if sp_dict:
if sp_dict is not None and sp_dict != {}:
sp_dict['pool_id'] = pool_dict['id']
self.session_persistence.create(session, **sp_dict)
if listener_id:
@ -182,31 +182,31 @@ class Repositories(object):
default_pool_id=pool_dict['id'])
return self.pool.get(session, id=db_pool.id)
def update_pool_and_sp(self, session, pool_id, pool_dict, sp_dict):
def update_pool_and_sp(self, session, pool_id, pool_dict):
"""Updates a pool and session persistence entity in the database.
:param session: A Sql Alchemy database session.
:param pool_dict: Dictionary representation of a pool
:param sp_dict: Dictionary representation of a session persistence
:returns: octavia.common.data_models.Pool
"""
with session.begin(subtransactions=True):
# If only the session persistence is being updated, this will be
# empty
if len(pool_dict.keys()) > 0:
self.pool.update(session, pool_id, **pool_dict)
if sp_dict:
if self.session_persistence.exists(session, pool_id):
if 'session_persistence' in pool_dict.keys():
sp_dict = pool_dict.pop('session_persistence')
if sp_dict is None or sp_dict == {}:
if self.session_persistence.exists(session, pool_id):
self.session_persistence.delete(session,
pool_id=pool_id)
elif self.session_persistence.exists(session, pool_id):
self.session_persistence.update(session, pool_id,
**sp_dict)
else:
sp_dict['pool_id'] = pool_id
self.session_persistence.create(session, **sp_dict)
db_pool = self.pool.get(session, id=pool_id)
if db_pool.session_persistence is not None and not sp_dict:
self.session_persistence.delete(session, pool_id=pool_id)
db_pool = self.pool.get(session, id=pool_id)
return db_pool
# If only the session_persistence is being updated, this will be
# empty
if len(pool_dict.keys()) > 0:
self.pool.update(session, pool_id, **pool_dict)
return self.pool.get(session, id=pool_id)
def test_and_set_lb_and_listeners_prov_status(self, session, lb_id,
lb_prov_status,

23
octavia/tests/functional/api/v1/test_pool.py
View File

@ -433,6 +433,29 @@ class TestPool(base.BaseAPITest):
self.listener.get('id'),
constants.ACTIVE, constants.ONLINE)
def test_update_preserve_session_persistence(self):
sp = {"type": constants.SESSION_PERSISTENCE_HTTP_COOKIE,
"cookie_name": "test_cookie_name"}
api_pool = self.create_pool(self.lb.get('id'),
self.listener.get('id'),
constants.PROTOCOL_HTTP,
constants.LB_ALGORITHM_ROUND_ROBIN,
session_persistence=sp)
self.set_lb_status(lb_id=self.lb.get('id'))
pool_update = {'lb_algorithm': constants.LB_ALGORITHM_SOURCE_IP}
api_pool = self.put(self.pool_path.format(pool_id=api_pool.get('id')),
body=pool_update).json
self.assert_correct_lb_status(self.lb.get('id'),
constants.PENDING_UPDATE,
constants.ONLINE)
self.assert_correct_listener_status(self.lb.get('id'),
self.listener.get('id'),
constants.PENDING_UPDATE,
constants.ONLINE)
response = self.get(self.pool_path.format(
pool_id=api_pool.get('id'))).json
self.assertEqual(sp, response.get('session_persistence'))
def test_update_bad_session_persistence(self):
self.skip('This test should pass after a validation layer.')
sp = {"type": constants.SESSION_PERSISTENCE_HTTP_COOKIE,

27
octavia/tests/functional/db/test_repositories.py
View File

@ -170,8 +170,9 @@ class AllRepositoriesTest(base.OctaviaDBTestBase):
sp = {'type': constants.SESSION_PERSISTENCE_HTTP_COOKIE,
'cookie_name': 'cookie_monster',
'pool_id': pool['id']}
pool.update({'session_persistence': sp})
pool_dm = self.repos.create_pool_on_load_balancer(
self.session, pool, listener_id=self.listener.id, sp_dict=sp)
self.session, pool, listener_id=self.listener.id)
pool_dm_dict = pool_dm.to_dict()
del pool_dm_dict['members']
del pool_dm_dict['health_monitor']
@ -203,7 +204,7 @@ class AllRepositoriesTest(base.OctaviaDBTestBase):
self.session, pool, listener_id=self.listener.id)
update_pool = {'protocol': constants.PROTOCOL_TCP, 'name': 'up_pool'}
new_pool_dm = self.repos.update_pool_and_sp(
self.session, pool_dm.id, update_pool, None)
self.session, pool_dm.id, update_pool)
pool_dm_dict = new_pool_dm.to_dict()
del pool_dm_dict['members']
del pool_dm_dict['health_monitor']
@ -226,12 +227,14 @@ class AllRepositoriesTest(base.OctaviaDBTestBase):
sp = {'type': constants.SESSION_PERSISTENCE_HTTP_COOKIE,
'cookie_name': 'cookie_monster',
'pool_id': pool['id']}
pool.update({'session_persistence': sp})
pool_dm = self.repos.create_pool_on_load_balancer(
self.session, pool, listener_id=self.listener.id, sp_dict=sp)
self.session, pool, listener_id=self.listener.id)
update_pool = {'protocol': constants.PROTOCOL_TCP, 'name': 'up_pool'}
update_sp = {'type': constants.SESSION_PERSISTENCE_SOURCE_IP}
update_pool.update({'session_persistence': update_sp})
new_pool_dm = self.repos.update_pool_and_sp(
self.session, pool_dm.id, update_pool, update_sp)
self.session, pool_dm.id, update_pool)
pool_dm_dict = new_pool_dm.to_dict()
del pool_dm_dict['members']
del pool_dm_dict['health_monitor']
@ -260,8 +263,9 @@ class AllRepositoriesTest(base.OctaviaDBTestBase):
update_pool = {'protocol': constants.PROTOCOL_TCP, 'name': 'up_pool'}
update_sp = {'type': constants.SESSION_PERSISTENCE_HTTP_COOKIE,
'cookie_name': 'monster_cookie'}
update_pool.update({'session_persistence': update_sp})
new_pool_dm = self.repos.update_pool_and_sp(
self.session, pool_dm.id, update_pool, update_sp)
self.session, pool_dm.id, update_pool)
sp_dm_dict = new_pool_dm.session_persistence.to_dict()
del sp_dm_dict['pool']
update_sp['pool_id'] = pool_dm.id
@ -277,9 +281,10 @@ class AllRepositoriesTest(base.OctaviaDBTestBase):
'id': uuidutils.generate_uuid()}
pool_dm = self.repos.create_pool_on_load_balancer(
self.session, pool, listener_id=self.listener.id)
update_pool = {'protocol': constants.PROTOCOL_TCP, 'name': 'up_pool'}
update_pool = {'protocol': constants.PROTOCOL_TCP, 'name': 'up_pool',
'session_persistence': None}
new_pool_dm = self.repos.update_pool_and_sp(
self.session, pool_dm.id, update_pool, None)
self.session, pool_dm.id, update_pool)
self.assertIsNone(new_pool_dm.session_persistence)
def test_update_pool_with_existing_sp_delete_sp(self):
@ -292,11 +297,13 @@ class AllRepositoriesTest(base.OctaviaDBTestBase):
sp = {'type': constants.SESSION_PERSISTENCE_HTTP_COOKIE,
'cookie_name': 'cookie_monster',
'pool_id': pool['id']}
pool.update({'session_persistence': sp})
pool_dm = self.repos.create_pool_on_load_balancer(
self.session, pool, listener_id=self.listener.id, sp_dict=sp)
update_pool = {'protocol': constants.PROTOCOL_TCP, 'name': 'up_pool'}
self.session, pool, listener_id=self.listener.id)
update_pool = {'protocol': constants.PROTOCOL_TCP, 'name': 'up_pool',
'session_persistence': {}}
new_pool_dm = self.repos.update_pool_and_sp(
self.session, pool_dm.id, update_pool, None)
self.session, pool_dm.id, update_pool)
self.assertIsNone(new_pool_dm.session_persistence)

6
octavia/tests/unit/controller/worker/tasks/test_database_tasks.py
View File

@ -1181,7 +1181,7 @@ class TestDatabaseTasks(base.TestCase):
repo.Repositories.update_pool_and_sp.assert_called_once_with(
'TEST',
POOL_ID,
update_dict, sp_dict)
update_dict)
# Test the revert
@ -1192,7 +1192,7 @@ class TestDatabaseTasks(base.TestCase):
repo.Repositories.update_pool_and_sp.assert_called_once_with(
'TEST',
POOL_ID,
{'enabled': 0}, None)
{'enabled': 0})
@mock.patch('octavia.db.repositories.L7PolicyRepository.update')
def test_update_l7policy_in_db(self,
@ -1448,4 +1448,4 @@ class TestDatabaseTasks(base.TestCase):
# Test the revert
mock_listener_repo_update.reset_mock()
update_server_group_info.revert(LB_ID, SERVER_GROUP_ID)
update_server_group_info.revert(LB_ID, SERVER_GROUP_ID)