From f76fa1ae330e933e86c666b857acfee06f675adb Mon Sep 17 00:00:00 2001
From: Sergey Kraynev <sergejyit@gmail.com>
Date: Wed, 23 Oct 2024 17:57:44 +0400
Subject: [PATCH] Remove amphora_health record on revert CreateAmphoraInDB

Record in amphora_health created by first UDP request from booted
amphora.
It's possible to run creation/failover LB where Amphora will be booted
and reverted on the final tasks (e.g. AmphoraPostVIPPlug) as result
record in amphora_health will be created, but not be removed on revert.
This patch adds logic to remove amphora_health record on revert flow.

Closes-Bug: 2085530

Change-Id: Ia8b4612cd25a68b44d55b113f704756818eb8a5b
---
 .../worker/v2/tasks/database_tasks.py         | 16 +++++----
 .../worker/v2/tasks/test_database_tasks.py    | 34 +++++++++++++++----
 ...ow_on_amphora_revert-082f94459ecacaa2.yaml |  7 ++++
 3 files changed, 45 insertions(+), 12 deletions(-)
 create mode 100644 releasenotes/notes/delete_amphora_health_row_on_amphora_revert-082f94459ecacaa2.yaml

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.