Fix LB set in ERROR too early in the revert flow

When an error occurs in a flow, the provisioning status of the load
balancer should be set to ERROR in the revert method of the first task
of the flow. This update acts as an unlock of the LB object and cannot
occur in any other revert method because the API might consider the LB
as mutable before finishing a task/flow.

Remove all occurrences of mark_loadbalancer_prov_status_error calls in
revert method of tasks that are not specifically designed for unlocking
the load balancers. Add a LoadBalancerToErrorOnRevertTask task in the
amphora failover flow to prevent a LB to be in an immutable state.

Story 2009651
Task 43810
Story 2009652
Task 43811

Note for stable/train: the code of the amphorav2 is not updated in this
backport, the source files exist in train but the feature was added
ussuri. Backporting this patch creates many merge conflicts and doesn't
provide anything for train users.

Conflicts:
	octavia/controller/worker/v1/tasks/database_tasks.py
	octavia/controller/worker/v2/flows/amphora_flows.py
	octavia/controller/worker/v2/tasks/amphora_driver_tasks.py
	octavia/controller/worker/v2/tasks/database_tasks.py
	octavia/tests/unit/controller/worker/v2/flows/test_amphora_flows.py
	octavia/tests/unit/controller/worker/v2/tasks/test_amphora_driver_tasks.py
	octavia/tests/unit/controller/worker/v2/tasks/test_database_tasks.py

Change-Id: I48b0f5a773209b1c1b056d71c0da05d6fd82ca73
(cherry picked from commit 4b8b198fec)
(cherry picked from commit 4039d35ce2)
(cherry picked from commit 844f1348ea)
(cherry picked from commit cf2a8bdf88)
(cherry picked from commit 95690e251d)
This commit is contained in:
Gregory Thiemonge 2021-10-28 22:13:26 +02:00
parent e117cda177
commit 1ad32d7cae
5 changed files with 23 additions and 156 deletions

View File

@ -254,7 +254,6 @@ class AmphoraPostVIPPlug(BaseAmphoraTask):
return
LOG.warning("Reverting post vip plug.")
self.task_utils.mark_amphora_status_error(amphora.id)
self.task_utils.mark_loadbalancer_prov_status_error(loadbalancer.id)
class AmphoraePostVIPPlug(BaseAmphoraTask):
@ -268,13 +267,6 @@ class AmphoraePostVIPPlug(BaseAmphoraTask):
loadbalancer,
amphorae_network_config)
def revert(self, result, loadbalancer, *args, **kwargs):
"""Handle a failed amphora vip plug notification."""
if isinstance(result, failure.Failure):
return
LOG.warning("Reverting amphorae post vip plug.")
self.task_utils.mark_loadbalancer_prov_status_error(loadbalancer.id)
class AmphoraCertUpload(BaseAmphoraTask):
"""Upload a certificate to the amphora."""

View File

@ -1064,10 +1064,6 @@ class MarkLBActiveInDB(BaseDatabaseTask):
LOG.warning("Error updating listener %s provisioning "
"status", listener.id)
LOG.warning("Reverting mark load balancer deleted in DB "
"for load balancer id %s", loadbalancer.id)
self.task_utils.mark_loadbalancer_prov_status_error(loadbalancer.id)
class UpdateLBServerGroupInDB(BaseDatabaseTask):
"""Update the server group id info for load balancer in DB."""
@ -1127,17 +1123,6 @@ class MarkLBDeletedInDB(BaseDatabaseTask):
loadbalancer.id,
provisioning_status=constants.DELETED)
def revert(self, loadbalancer, *args, **kwargs):
"""Mark the load balancer as broken and ready to be cleaned up.
:param loadbalancer: Load balancer object that failed to update
:returns: None
"""
LOG.warning("Reverting mark load balancer deleted in DB "
"for load balancer id %s", loadbalancer.id)
self.task_utils.mark_loadbalancer_prov_status_error(loadbalancer.id)
class MarkLBPendingDeleteInDB(BaseDatabaseTask):
"""Mark the load balancer pending delete in the DB.
@ -1159,17 +1144,6 @@ class MarkLBPendingDeleteInDB(BaseDatabaseTask):
provisioning_status=(constants.
PENDING_DELETE))
def revert(self, loadbalancer, *args, **kwargs):
"""Mark the load balancer as broken and ready to be cleaned up.
:param loadbalancer: Load balancer object that failed to update
:returns: None
"""
LOG.warning("Reverting mark load balancer pending delete in DB "
"for load balancer id %s", loadbalancer.id)
self.task_utils.mark_loadbalancer_prov_status_error(loadbalancer.id)
class MarkLBAndListenersActiveInDB(BaseDatabaseTask):
"""Mark the load balancer and specified listeners active in the DB.
@ -1203,11 +1177,10 @@ class MarkLBAndListenersActiveInDB(BaseDatabaseTask):
:returns: None
"""
LOG.warning("Reverting mark load balancer and listeners active in DB "
"for load balancer id %(LB)s and listener ids: %(list)s",
{'LB': loadbalancer.id,
'list': ', '.join([l.id for l in listeners])})
self.task_utils.mark_loadbalancer_prov_status_error(loadbalancer.id)
LOG.warning("Reverting mark listeners active in DB "
"for listener ids: %(list)s",
{'list': ', '.join([listener.id
for listener in listeners])})
for listener in listeners:
self.task_utils.mark_listener_prov_status_error(listener.id)
@ -1294,18 +1267,6 @@ class UpdateLoadbalancerInDB(BaseDatabaseTask):
self.loadbalancer_repo.update(db_apis.get_session(), loadbalancer.id,
**update_dict)
def revert(self, loadbalancer, *args, **kwargs):
"""Mark the loadbalancer ERROR since the update couldn't happen
:param loadbalancer: The load balancer that couldn't be updated
:returns: None
"""
LOG.warning("Reverting update loadbalancer in DB "
"for loadbalancer id %s", loadbalancer.id)
self.task_utils.mark_loadbalancer_prov_status_error(loadbalancer.id)
class UpdateHealthMonInDB(BaseDatabaseTask):
"""Update the health monitor in the DB.

View File

@ -496,10 +496,7 @@ class TestAmphoraDriverTasks(base.TestCase):
_session_mock,
id=AMP_ID,
status=constants.ERROR)
repo.LoadBalancerRepository.update.assert_called_once_with(
_session_mock,
id=LB_ID,
provisioning_status=constants.ERROR)
repo.LoadBalancerRepository.update.assert_not_called()
self.assertIsNone(amp)
@ -513,10 +510,7 @@ class TestAmphoraDriverTasks(base.TestCase):
_session_mock,
id=AMP_ID,
status=constants.ERROR)
repo.LoadBalancerRepository.update.assert_called_once_with(
_session_mock,
id=LB_ID,
provisioning_status=constants.ERROR)
repo.LoadBalancerRepository.update.assert_not_called()
self.assertIsNone(amp)
@ -546,33 +540,6 @@ class TestAmphoraDriverTasks(base.TestCase):
mock_driver.post_vip_plug.assert_called_once_with(
_amphora_mock, _LB_mock, amphorae_net_config_mock)
# Test revert
amp = amphora_post_vip_plug_obj.revert(None, _LB_mock)
repo.LoadBalancerRepository.update.assert_called_once_with(
_session_mock,
id=LB_ID,
provisioning_status=constants.ERROR)
self.assertIsNone(amp)
# Test revert with exception
repo.LoadBalancerRepository.update.reset_mock()
mock_loadbalancer_repo_update.side_effect = Exception('fail')
amp = amphora_post_vip_plug_obj.revert(None, _LB_mock)
repo.LoadBalancerRepository.update.assert_called_once_with(
_session_mock,
id=LB_ID,
provisioning_status=constants.ERROR)
self.assertIsNone(amp)
# Test revert when this task failed
repo.AmphoraRepository.update.reset_mock()
amp = amphora_post_vip_plug_obj.revert(
failure.Failure.from_exception(Exception('boom')), _amphora_mock,
None)
repo.AmphoraRepository.update.assert_not_called()
def test_amphora_cert_upload(self,
mock_driver,
mock_generate_uuid,

View File

@ -998,10 +998,7 @@ class TestDatabaseTasks(base.TestCase):
'TEST',
id=LISTENER_ID,
provisioning_status=constants.ERROR)
repo.LoadBalancerRepository.update.assert_called_once_with(
'TEST',
id=LB_ID,
provisioning_status=constants.ERROR)
repo.LoadBalancerRepository.update.assert_not_called()
# Test the revert with exceptions
mock_loadbalancer_repo_update.reset_mock()
@ -1016,10 +1013,7 @@ class TestDatabaseTasks(base.TestCase):
'TEST',
id=LISTENER_ID,
provisioning_status=constants.ERROR)
repo.LoadBalancerRepository.update.assert_called_once_with(
'TEST',
id=LB_ID,
provisioning_status=constants.ERROR)
repo.LoadBalancerRepository.update.assert_not_called()
@mock.patch('octavia.common.tls_utils.cert_parser.get_cert_expiration',
return_value=_cert_mock)
@ -1083,10 +1077,7 @@ class TestDatabaseTasks(base.TestCase):
mock_loadbalancer_repo_update.reset_mock()
mark_loadbalancer_active.revert(self.loadbalancer_mock)
repo.LoadBalancerRepository.update.assert_called_once_with(
'TEST',
id=LB_ID,
provisioning_status=constants.ERROR)
repo.LoadBalancerRepository.update.assert_not_called()
self.assertEqual(0, repo.ListenerRepository.update.call_count)
# Test the revert with exception
@ -1094,10 +1085,7 @@ class TestDatabaseTasks(base.TestCase):
mock_loadbalancer_repo_update.side_effect = Exception('fail')
mark_loadbalancer_active.revert(self.loadbalancer_mock)
repo.LoadBalancerRepository.update.assert_called_once_with(
'TEST',
id=LB_ID,
provisioning_status=constants.ERROR)
repo.LoadBalancerRepository.update.assert_not_called()
self.assertEqual(0, repo.ListenerRepository.update.call_count)
def test_mark_LB_active_in_db_and_listeners(self,
@ -1129,10 +1117,7 @@ class TestDatabaseTasks(base.TestCase):
mock_listener_repo_update.reset_mock()
mark_lb_active.revert(lb)
repo.LoadBalancerRepository.update.assert_called_once_with(
'TEST',
id=lb.id,
provisioning_status=constants.ERROR)
repo.LoadBalancerRepository.update.assert_not_called()
self.assertEqual(2, repo.ListenerRepository.update.call_count)
repo.ListenerRepository.update.has_calls(
[mock.call('TEST', listeners[0].id,
@ -1235,10 +1220,7 @@ class TestDatabaseTasks(base.TestCase):
mock_l7r_repo_update.reset_mock()
mark_lb_active.revert(lb)
repo.LoadBalancerRepository.update.assert_called_once_with(
'TEST',
id=lb.id,
provisioning_status=constants.ERROR)
repo.LoadBalancerRepository.update.assert_not_called()
self.assertEqual(2, repo.ListenerRepository.update.call_count)
repo.ListenerRepository.update.has_calls(
[mock.call('TEST', listeners[0].id,
@ -1295,20 +1277,14 @@ class TestDatabaseTasks(base.TestCase):
mock_loadbalancer_repo_update.reset_mock()
mark_loadbalancer_deleted.revert(self.loadbalancer_mock)
repo.LoadBalancerRepository.update.assert_called_once_with(
'TEST',
id=LB_ID,
provisioning_status=constants.ERROR)
repo.LoadBalancerRepository.update.assert_not_called()
# Test the revert with exception
mock_loadbalancer_repo_update.reset_mock()
mock_loadbalancer_repo_update.side_effect = Exception('fail')
mark_loadbalancer_deleted.revert(self.loadbalancer_mock)
repo.LoadBalancerRepository.update.assert_called_once_with(
'TEST',
id=LB_ID,
provisioning_status=constants.ERROR)
repo.LoadBalancerRepository.update.assert_not_called()
def test_mark_LB_pending_deleted_in_db(self,
mock_generate_uuid,
@ -1328,25 +1304,6 @@ class TestDatabaseTasks(base.TestCase):
LB_ID,
provisioning_status=constants.PENDING_DELETE)
# Test the revert
mock_loadbalancer_repo_update.reset_mock()
mark_loadbalancer_pending_delete.revert(self.loadbalancer_mock)
repo.LoadBalancerRepository.update.assert_called_once_with(
'TEST',
id=LB_ID,
provisioning_status=constants.ERROR)
# Test the revert with exception
mock_loadbalancer_repo_update.reset_mock()
mock_loadbalancer_repo_update.side_effect = Exception('fail')
mark_loadbalancer_pending_delete.revert(self.loadbalancer_mock)
repo.LoadBalancerRepository.update.assert_called_once_with(
'TEST',
id=LB_ID,
provisioning_status=constants.ERROR)
@mock.patch('octavia.db.repositories.HealthMonitorRepository.update')
def test_update_health_monitor_in_db(self,
mock_health_mon_repo_update,
@ -1404,25 +1361,6 @@ class TestDatabaseTasks(base.TestCase):
LB_ID,
name='test', description='test2')
# Test the revert
mock_loadbalancer_repo_update.reset_mock()
update_load_balancer.revert(self.loadbalancer_mock)
repo.LoadBalancerRepository.update.assert_called_once_with(
'TEST',
id=LB_ID,
provisioning_status=constants.ERROR)
# Test the revert with exception
mock_loadbalancer_repo_update.reset_mock()
mock_loadbalancer_repo_update.side_effect = Exception('fail')
update_load_balancer.revert(self.loadbalancer_mock)
repo.LoadBalancerRepository.update.assert_called_once_with(
'TEST',
id=LB_ID,
provisioning_status=constants.ERROR)
@mock.patch('octavia.db.repositories.VipRepository.update')
def test_update_vip_in_db_during_update_loadbalancer(self,
mock_vip_update,

View File

@ -0,0 +1,9 @@
---
fixes:
- |
Fix an issue with the provisioning status of a load balancer that was set
to ERROR too early when an error occurred, making the load balancer mutable
while the execution of the tasks for this resources haven't finished yet.
- |
Fix an issue that could set the provisioning status of a load balancer to a
PENDING_UPDATE state when an error occurred in the amphora failover flow.