From 17f2c674916584d70ba11fd2d8ba3bae439d5b62 Mon Sep 17 00:00:00 2001 From: Gregory Thiemonge Date: Thu, 28 Oct 2021 22:13:26 +0200 Subject: [PATCH] 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 Change-Id: I48b0f5a773209b1c1b056d71c0da05d6fd82ca73 (cherry picked from commit 4b8b198fecee802e1031c014f811eb766329a1a0) (cherry picked from commit 4039d35ce221b58490d4aa5b3497e29dbf7664ac) (cherry picked from commit 844f1348ea3d83f9fb5f920b64c9234dbdf9948d) --- .../worker/v1/tasks/amphora_driver_tasks.py | 8 -- .../worker/v1/tasks/database_tasks.py | 46 +-------- .../worker/v2/flows/amphora_flows.py | 4 + .../worker/v2/tasks/amphora_driver_tasks.py | 10 -- .../worker/v2/tasks/database_tasks.py | 81 +--------------- .../v1/tasks/test_amphora_driver_tasks.py | 37 +------- .../worker/v1/tasks/test_database_tasks.py | 78 ++-------------- .../worker/v2/flows/test_amphora_flows.py | 6 +- .../v2/tasks/test_amphora_driver_tasks.py | 37 +------- .../worker/v2/tasks/test_database_tasks.py | 93 +++---------------- ...ing-status-on-errors-7f3736ef6e94d453.yaml | 9 ++ 11 files changed, 48 insertions(+), 361 deletions(-) create mode 100644 releasenotes/notes/fix-provisioning-status-on-errors-7f3736ef6e94d453.yaml diff --git a/octavia/controller/worker/v1/tasks/amphora_driver_tasks.py b/octavia/controller/worker/v1/tasks/amphora_driver_tasks.py index 52c5ba01ea..f2fb420de3 100644 --- a/octavia/controller/worker/v1/tasks/amphora_driver_tasks.py +++ b/octavia/controller/worker/v1/tasks/amphora_driver_tasks.py @@ -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.""" diff --git a/octavia/controller/worker/v1/tasks/database_tasks.py b/octavia/controller/worker/v1/tasks/database_tasks.py index e42f6efaba..7a8203668a 100644 --- a/octavia/controller/worker/v1/tasks/database_tasks.py +++ b/octavia/controller/worker/v1/tasks/database_tasks.py @@ -1070,10 +1070,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.""" @@ -1133,17 +1129,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. @@ -1165,17 +1150,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. @@ -1209,12 +1183,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([listener.id + LOG.warning("Reverting mark listeners active in DB " + "for listener ids: %(list)s", + {'list': ', '.join([listener.id for listener in listeners])}) - self.task_utils.mark_loadbalancer_prov_status_error(loadbalancer.id) for listener in listeners: self.task_utils.mark_listener_prov_status_error(listener.id) @@ -1301,18 +1273,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. diff --git a/octavia/controller/worker/v2/flows/amphora_flows.py b/octavia/controller/worker/v2/flows/amphora_flows.py index 1ae186fbc1..4b4bbdc60a 100644 --- a/octavia/controller/worker/v2/flows/amphora_flows.py +++ b/octavia/controller/worker/v2/flows/amphora_flows.py @@ -578,6 +578,10 @@ class AmphoraFlows(object): failover_amp_flow = linear_flow.Flow( constants.FAILOVER_AMPHORA_FLOW) + # Revert LB to provisioning_status ERROR if this flow goes wrong + failover_amp_flow.add(lifecycle_tasks.LoadBalancerToErrorOnRevertTask( + requires=constants.LOADBALANCER)) + # Revert amphora to status ERROR if this flow goes wrong failover_amp_flow.add(lifecycle_tasks.AmphoraToErrorOnRevertTask( requires=constants.AMPHORA, diff --git a/octavia/controller/worker/v2/tasks/amphora_driver_tasks.py b/octavia/controller/worker/v2/tasks/amphora_driver_tasks.py index fdf91d23d1..c06e00ea51 100644 --- a/octavia/controller/worker/v2/tasks/amphora_driver_tasks.py +++ b/octavia/controller/worker/v2/tasks/amphora_driver_tasks.py @@ -359,8 +359,6 @@ class AmphoraPostVIPPlug(BaseAmphoraTask): return LOG.warning("Reverting post vip plug.") self.task_utils.mark_amphora_status_error(amphora.get(constants.ID)) - self.task_utils.mark_loadbalancer_prov_status_error( - loadbalancer[constants.LOADBALANCER_ID]) class AmphoraePostVIPPlug(BaseAmphoraTask): @@ -376,14 +374,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[constants.LOADBALANCER_ID]) - class AmphoraCertUpload(BaseAmphoraTask): """Upload a certificate to the amphora.""" diff --git a/octavia/controller/worker/v2/tasks/database_tasks.py b/octavia/controller/worker/v2/tasks/database_tasks.py index 9b8e81c098..a875191536 100644 --- a/octavia/controller/worker/v2/tasks/database_tasks.py +++ b/octavia/controller/worker/v2/tasks/database_tasks.py @@ -1140,12 +1140,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[constants.LOADBALANCER_ID]) - self.task_utils.mark_loadbalancer_prov_status_error( - loadbalancer[constants.LOADBALANCER_ID]) - class MarkLBActiveInDBByListener(BaseDatabaseTask): """Mark the load balancer active in the DB using a listener dict. @@ -1166,22 +1160,6 @@ class MarkLBActiveInDBByListener(BaseDatabaseTask): listener[constants.LOADBALANCER_ID], provisioning_status=constants.ACTIVE) - def revert(self, listener, *args, **kwargs): - """Mark the load balancer as broken and ready to be cleaned up. - - This also puts all sub-objects of the load balancer to ERROR state if - self.mark_subobjects is True - - :param listener: Listener dictionary - :returns: None - """ - - LOG.warning("Reverting mark load balancer active in DB " - "for load balancer id %s", - listener[constants.LOADBALANCER_ID]) - self.task_utils.mark_loadbalancer_prov_status_error( - listener[constants.LOADBALANCER_ID]) - class UpdateLBServerGroupInDB(BaseDatabaseTask): """Update the server group id info for load balancer in DB.""" @@ -1241,19 +1219,6 @@ class MarkLBDeletedInDB(BaseDatabaseTask): loadbalancer[constants.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[constants.LOADBALANCER_ID]) - self.task_utils.mark_loadbalancer_prov_status_error( - loadbalancer[constants.LOADBALANCER_ID]) - class MarkLBPendingDeleteInDB(BaseDatabaseTask): """Mark the load balancer pending delete in the DB. @@ -1275,19 +1240,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[constants.LOADBALANCER_ID]) - self.task_utils.mark_loadbalancer_prov_status_error( - loadbalancer[constants.LOADBALANCER_ID]) - class MarkLBAndListenersActiveInDB(BaseDatabaseTask): """Mark the load balancer and specified listeners active in the DB. @@ -1326,20 +1278,11 @@ class MarkLBAndListenersActiveInDB(BaseDatabaseTask): :param listeners: Listener objects that failed to update :returns: None """ - lb_id = None - if loadbalancer_id: - lb_id = loadbalancer_id - elif listeners: - lb_id = listeners[0][constants.LOADBALANCER_ID] - - if lb_id: - lists = ', '.join([listener[constants.LISTENER_ID] - for listener in listeners]) - LOG.warning("Reverting mark load balancer and listeners active in " - "DB for load balancer id %(LB)s and listener ids: " - "%(list)s", {'LB': lb_id, - 'list': lists}) - self.task_utils.mark_loadbalancer_prov_status_error(lb_id) + lists = ', '.join([listener[constants.LISTENER_ID] + for listener in listeners]) + LOG.warning("Reverting mark listeners active in " + "DB for listener ids: " + "%(list)s", {'list': lists}) for listener in listeners: self.task_utils.mark_listener_prov_status_error( @@ -1430,20 +1373,6 @@ class UpdateLoadbalancerInDB(BaseDatabaseTask): loadbalancer[constants.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[constants.LOADBALANCER_ID]) - - self.task_utils.mark_loadbalancer_prov_status_error( - loadbalancer[constants.LOADBALANCER_ID]) - class UpdateHealthMonInDB(BaseDatabaseTask): """Update the health monitor in the DB. diff --git a/octavia/tests/unit/controller/worker/v1/tasks/test_amphora_driver_tasks.py b/octavia/tests/unit/controller/worker/v1/tasks/test_amphora_driver_tasks.py index fa92aae320..aff0bae77f 100644 --- a/octavia/tests/unit/controller/worker/v1/tasks/test_amphora_driver_tasks.py +++ b/octavia/tests/unit/controller/worker/v1/tasks/test_amphora_driver_tasks.py @@ -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, diff --git a/octavia/tests/unit/controller/worker/v1/tasks/test_database_tasks.py b/octavia/tests/unit/controller/worker/v1/tasks/test_database_tasks.py index a4c5f1262d..f905096ff9 100644 --- a/octavia/tests/unit/controller/worker/v1/tasks/test_database_tasks.py +++ b/octavia/tests/unit/controller/worker/v1/tasks/test_database_tasks.py @@ -1043,10 +1043,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() @@ -1061,10 +1058,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) @@ -1128,10 +1122,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 @@ -1139,10 +1130,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, @@ -1174,10 +1162,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, @@ -1280,10 +1265,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, @@ -1340,20 +1322,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, @@ -1373,25 +1349,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, @@ -1449,25 +1406,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, diff --git a/octavia/tests/unit/controller/worker/v2/flows/test_amphora_flows.py b/octavia/tests/unit/controller/worker/v2/flows/test_amphora_flows.py index 8cf8996513..a55578ab77 100644 --- a/octavia/tests/unit/controller/worker/v2/flows/test_amphora_flows.py +++ b/octavia/tests/unit/controller/worker/v2/flows/test_amphora_flows.py @@ -362,10 +362,11 @@ class TestAmphoraFlows(base.TestCase): self.assertIsInstance(amp_flow, flow.Flow) self.assertIn(constants.LOADBALANCER_ID, amp_flow.requires) + self.assertIn(constants.LOADBALANCER, amp_flow.requires) self.assertIn(constants.VIP_SG_ID, amp_flow.provides) - self.assertEqual(1, len(amp_flow.requires)) + self.assertEqual(2, len(amp_flow.requires)) self.assertEqual(1, len(amp_flow.provides)) def test_get_failover_flow_spare(self, mock_get_net_driver): @@ -376,10 +377,11 @@ class TestAmphoraFlows(base.TestCase): self.assertIsInstance(amp_flow, flow.Flow) self.assertIn(constants.LOADBALANCER_ID, amp_flow.requires) + self.assertIn(constants.LOADBALANCER, amp_flow.requires) self.assertIn(constants.VIP_SG_ID, amp_flow.provides) - self.assertEqual(1, len(amp_flow.requires)) + self.assertEqual(2, len(amp_flow.requires)) self.assertEqual(1, len(amp_flow.provides)) def test_cert_rotate_amphora_flow(self, mock_get_net_driver): diff --git a/octavia/tests/unit/controller/worker/v2/tasks/test_amphora_driver_tasks.py b/octavia/tests/unit/controller/worker/v2/tasks/test_amphora_driver_tasks.py index 28de5af50e..723048a3e5 100644 --- a/octavia/tests/unit/controller/worker/v2/tasks/test_amphora_driver_tasks.py +++ b/octavia/tests/unit/controller/worker/v2/tasks/test_amphora_driver_tasks.py @@ -557,10 +557,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) @@ -574,10 +571,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) @@ -664,33 +658,6 @@ class TestAmphoraDriverTasks(base.TestCase): _db_amphora_mock, _db_load_balancer_mock, amphorae_net_config_mock, vip_subnet=vip_subnet, vrrp_port=vrrp_port) - # 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, diff --git a/octavia/tests/unit/controller/worker/v2/tasks/test_database_tasks.py b/octavia/tests/unit/controller/worker/v2/tasks/test_database_tasks.py index 5641b572dc..690d16b52d 100644 --- a/octavia/tests/unit/controller/worker/v2/tasks/test_database_tasks.py +++ b/octavia/tests/unit/controller/worker/v2/tasks/test_database_tasks.py @@ -1115,10 +1115,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 LB_ID from listeners mock_loadbalancer_repo_update.reset_mock() @@ -1130,10 +1127,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 no LB_ID mock_loadbalancer_repo_update.reset_mock() @@ -1156,10 +1150,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) @@ -1223,10 +1214,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 @@ -1234,10 +1222,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_by_listener(self, @@ -1263,10 +1248,7 @@ class TestDatabaseTasks(base.TestCase): mock_loadbalancer_repo_update.reset_mock() mark_loadbalancer_active.revert(listener_dict) - 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 @@ -1274,10 +1256,7 @@ class TestDatabaseTasks(base.TestCase): mock_loadbalancer_repo_update.side_effect = Exception('fail') mark_loadbalancer_active.revert(listener_dict) - 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) @mock.patch('octavia.db.repositories.LoadBalancerRepository.get') @@ -1312,10 +1291,7 @@ class TestDatabaseTasks(base.TestCase): mock_listener_repo_update.reset_mock() mark_lb_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(2, repo.ListenerRepository.update.call_count) repo.ListenerRepository.update.has_calls( [mock.call('TEST', listeners[0].id, @@ -1407,10 +1383,7 @@ class TestDatabaseTasks(base.TestCase): mock_l7r_repo_update.reset_mock() mark_lb_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(2, repo.ListenerRepository.update.call_count) repo.ListenerRepository.update.has_calls( [mock.call('TEST', listeners[0].id, @@ -1456,20 +1429,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, @@ -1489,25 +1456,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, @@ -1565,25 +1513,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, diff --git a/releasenotes/notes/fix-provisioning-status-on-errors-7f3736ef6e94d453.yaml b/releasenotes/notes/fix-provisioning-status-on-errors-7f3736ef6e94d453.yaml new file mode 100644 index 0000000000..f4688bb3e9 --- /dev/null +++ b/releasenotes/notes/fix-provisioning-status-on-errors-7f3736ef6e94d453.yaml @@ -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.