From 1ad32d7cae56a093b0f1673d54957f370324a224 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 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 4b8b198fecee802e1031c014f811eb766329a1a0) (cherry picked from commit 4039d35ce221b58490d4aa5b3497e29dbf7664ac) (cherry picked from commit 844f1348ea3d83f9fb5f920b64c9234dbdf9948d) (cherry picked from commit cf2a8bdf88d544e47d2aa2e43388aeb3de99e21b) (cherry picked from commit 95690e251d5c89db63e9a81abc6f71ff5d1cf183) --- .../worker/v1/tasks/amphora_driver_tasks.py | 8 -- .../worker/v1/tasks/database_tasks.py | 47 +---------- .../v1/tasks/test_amphora_driver_tasks.py | 37 +-------- .../worker/v1/tasks/test_database_tasks.py | 78 ++----------------- ...ing-status-on-errors-7f3736ef6e94d453.yaml | 9 +++ 5 files changed, 23 insertions(+), 156 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 b1fc0f48bd..fc9f496d95 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 a664eaef6c..33f26cd94b 100644 --- a/octavia/controller/worker/v1/tasks/database_tasks.py +++ b/octavia/controller/worker/v1/tasks/database_tasks.py @@ -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. 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 e25b0ba1e9..cb9cd951bf 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 98d45580c7..326cadb8dc 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 @@ -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, 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.