From a4da3ef22001e1229541a3bdfe232f6967987cb9 Mon Sep 17 00:00:00 2001
From: Gregory Thiemonge <gthiemon@redhat.com>
Date: Thu, 5 Sep 2019 14:52:02 +0200
Subject: [PATCH] Fix cleanup of expired load balancer entries

Fix a bad query filter, deleted load balancers have a DELETED
provisioning_status (instead of operating_status).

Add a functional test that checks that deleted and expired load
balancers are correctly selected by get_all_deleted_expiring function.

Fix a similar test for amphorae that was buggy and not enabled (because
of missing test_ prefix).

Change-Id: I0ce2eabfd4dd41210312ea3b7f6274c9a6d50e44
Story: 2006496
Task: 36458
---
 octavia/db/repositories.py                    |  2 +-
 .../tests/functional/db/test_repositories.py  | 37 ++++++++++++++-----
 ...dbalancer-db-cleanup-61ee81a4fd597067.yaml |  5 +++
 3 files changed, 33 insertions(+), 11 deletions(-)
 create mode 100644 releasenotes/notes/fix-loadbalancer-db-cleanup-61ee81a4fd597067.yaml

diff --git a/octavia/db/repositories.py b/octavia/db/repositories.py
index 45532d94c0..9e22d9305a 100644
--- a/octavia/db/repositories.py
+++ b/octavia/db/repositories.py
@@ -196,7 +196,7 @@ class BaseRepository(object):
         if hasattr(self.model_class, 'status'):
             query = query.filter_by(status=consts.DELETED)
         else:
-            query = query.filter_by(operating_status=consts.DELETED)
+            query = query.filter_by(provisioning_status=consts.DELETED)
         # Do not load any relationship
         query = query.options(noload('*'))
         model_list = query.all()
diff --git a/octavia/tests/functional/db/test_repositories.py b/octavia/tests/functional/db/test_repositories.py
index 6a896d6392..17668edc10 100644
--- a/octavia/tests/functional/db/test_repositories.py
+++ b/octavia/tests/functional/db/test_repositories.py
@@ -2661,13 +2661,17 @@ class HealthMonitorRepositoryTest(BaseRepositoryTest):
 
 class LoadBalancerRepositoryTest(BaseRepositoryTest):
 
-    def create_loadbalancer(self, lb_id):
-        lb = self.lb_repo.create(self.session, id=lb_id,
-                                 project_id=self.FAKE_UUID_2, name="lb_name",
-                                 description="lb_description",
-                                 provisioning_status=constants.ACTIVE,
-                                 operating_status=constants.ONLINE,
-                                 enabled=True, tags=['test_tag'])
+    def create_loadbalancer(self, lb_id, **overrides):
+        settings = dict(
+            id=lb_id,
+            project_id=self.FAKE_UUID_2, name="lb_name",
+            description="lb_description",
+            provisioning_status=constants.ACTIVE,
+            operating_status=constants.ONLINE,
+            enabled=True, tags=['test_tag'],
+        )
+        settings.update(**overrides)
+        lb = self.lb_repo.create(self.session, **settings)
         return lb
 
     def test_get(self):
@@ -2936,6 +2940,20 @@ class LoadBalancerRepositoryTest(BaseRepositoryTest):
         lb = self.lb_repo.get(self.session, id=lb_id)
         self.assertEqual(constants.PENDING_UPDATE, lb.provisioning_status)
 
+    def test_get_all_deleted_expiring_load_balancer(self):
+        exp_age = datetime.timedelta(seconds=self.FAKE_EXP_AGE)
+        updated_at = datetime.datetime.utcnow() - exp_age
+        lb1 = self.create_loadbalancer(
+            self.FAKE_UUID_1, updated_at=updated_at,
+            provisioning_status=constants.DELETED)
+        lb2 = self.create_loadbalancer(
+            self.FAKE_UUID_2, provisioning_status=constants.DELETED)
+
+        expiring_ids = self.lb_repo.get_all_deleted_expiring(
+            self.session, exp_age=exp_age)
+        self.assertIn(lb1.id, expiring_ids)
+        self.assertNotIn(lb2.id, expiring_ids)
+
 
 class VipRepositoryTest(BaseRepositoryTest):
 
@@ -3175,7 +3193,7 @@ class AmphoraRepositoryTest(BaseRepositoryTest):
         self.assertIsNotNone(lb)
         self.assertEqual(self.lb, lb)
 
-    def get_all_deleted_expiring_amphora(self):
+    def test_get_all_deleted_expiring_amphora(self):
         exp_age = datetime.timedelta(seconds=self.FAKE_EXP_AGE)
         updated_at = datetime.datetime.utcnow() - exp_age
         amphora1 = self.create_amphora(
@@ -3183,9 +3201,8 @@ class AmphoraRepositoryTest(BaseRepositoryTest):
         amphora2 = self.create_amphora(
             self.FAKE_UUID_2, status=constants.DELETED)
 
-        expiring_list = self.amphora_repo.get_all_deleted_expiring(
+        expiring_ids = self.amphora_repo.get_all_deleted_expiring(
             self.session, exp_age=exp_age)
-        expiring_ids = [amp.id for amp in expiring_list]
         self.assertIn(amphora1.id, expiring_ids)
         self.assertNotIn(amphora2.id, expiring_ids)
 
diff --git a/releasenotes/notes/fix-loadbalancer-db-cleanup-61ee81a4fd597067.yaml b/releasenotes/notes/fix-loadbalancer-db-cleanup-61ee81a4fd597067.yaml
new file mode 100644
index 0000000000..c893ddd8f8
--- /dev/null
+++ b/releasenotes/notes/fix-loadbalancer-db-cleanup-61ee81a4fd597067.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fix an issue that prevented the cleanup of load balancer entries in the
+    database by the Octavia housekeeper service.