diff --git a/octavia/controller/worker/v2/tasks/database_tasks.py b/octavia/controller/worker/v2/tasks/database_tasks.py index e78ef41d7f..48b4d92d46 100644 --- a/octavia/controller/worker/v2/tasks/database_tasks.py +++ b/octavia/controller/worker/v2/tasks/database_tasks.py @@ -132,13 +132,17 @@ class CreateAmphoraInDB(BaseDatabaseTask): LOG.warning("Reverting create amphora in DB for amp id %s ", result) # Delete the amphora for now. May want to just update status later - try: - with db_apis.session().begin() as session: + with db_apis.session().begin() as session: + try: self.amphora_repo.delete(session, id=result) - except Exception as e: - LOG.error("Failed to delete amphora %(amp)s " - "in the database due to: " - "%(except)s", {'amp': result, 'except': str(e)}) + except Exception as e: + LOG.error("Failed to delete amphora %(amp)s " + "in the database due to: " + "%(except)s", {'amp': result, 'except': str(e)}) + try: + self.amp_health_repo.delete(session, amphora_id=result) + except Exception: + pass class MarkLBAmphoraeDeletedInDB(BaseDatabaseTask): 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 8b471b8bcb..25d2670851 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 @@ -184,7 +184,9 @@ class TestDatabaseTasks(base.TestCase): @mock.patch('octavia.db.repositories.AmphoraRepository.create', return_value=_db_amphora_mock) + @mock.patch('octavia.db.repositories.AmphoraHealthRepository.delete') def test_create_amphora_in_db(self, + mock_amphora_health_repo_delete, mock_create, mock_generate_uuid, mock_LOG, @@ -210,23 +212,43 @@ class TestDatabaseTasks(base.TestCase): # Test the revert create_amp_in_db.revert(_tf_failure_mock) - self.assertFalse(mock_amphora_repo_delete.called) + mock_amphora_repo_delete.assert_not_called() + mock_amphora_health_repo_delete.assert_not_called() + amp_id = 'AMP' mock_amphora_repo_delete.reset_mock() - create_amp_in_db.revert(result='AMP') + mock_amphora_health_repo_delete.reset_mock() + create_amp_in_db.revert(result=amp_id) self.assertTrue(mock_amphora_repo_delete.called) + self.assertTrue(mock_amphora_health_repo_delete.called) mock_amphora_repo_delete.assert_called_once_with( mock_session, - id='AMP') + id=amp_id) + mock_amphora_health_repo_delete.assert_called_once_with( + mock_session, + amphora_id=amp_id) + mock_LOG.error.assert_not_called() + mock_LOG.debug.assert_not_called() # Test revert with exception mock_amphora_repo_delete.reset_mock() - mock_amphora_repo_delete.side_effect = Exception('fail') - create_amp_in_db.revert(result='AMP') + mock_amphora_health_repo_delete.reset_mock() + err1_msg, err2_msg = ('fail', 'fail2') + mock_amphora_repo_delete.side_effect = Exception(err1_msg) + mock_amphora_health_repo_delete.side_effect = Exception(err2_msg) + create_amp_in_db.revert(result=amp_id) self.assertTrue(mock_amphora_repo_delete.called) + self.assertTrue(mock_amphora_health_repo_delete.called) mock_amphora_repo_delete.assert_called_once_with( mock_session, - id='AMP') + id=amp_id) + mock_amphora_health_repo_delete.assert_called_once_with( + mock_session, + amphora_id=amp_id) + mock_LOG.error.assert_called_once_with( + "Failed to delete amphora %(amp)s " + "in the database due to: " + "%(except)s", {'amp': amp_id, 'except': err1_msg}) @mock.patch('octavia.db.repositories.ListenerRepository.delete') def test_delete_listener_in_db(self, diff --git a/releasenotes/notes/delete_amphora_health_row_on_amphora_revert-082f94459ecacaa2.yaml b/releasenotes/notes/delete_amphora_health_row_on_amphora_revert-082f94459ecacaa2.yaml new file mode 100644 index 0000000000..39b7266fb8 --- /dev/null +++ b/releasenotes/notes/delete_amphora_health_row_on_amphora_revert-082f94459ecacaa2.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Remove record in amphora_health table on revert. It's necessary, because + record in amphora table for corresponding amphora also deleted. + It allows to avoid false positive react of failover threshold due to + orphan records in amphora_health table.