From 8018bf9124a17bc386ee06cc0a4ea9c36c3bbb55 Mon Sep 17 00:00:00 2001
From: Mark Goddard <mark@stackhpc.com>
Date: Fri, 22 Jan 2021 15:22:45 +0000
Subject: [PATCH] Fix cluster deletion when load balancers don't exist

During cluster deletion, magnum tries to delete the cluster's load
balancers in advance of deleting the heat stack. If these load balancers
do not exist for some reason, the cluster deletion will fail with an
error such as the following:

    Failed to pre-delete resources for cluster
    748b628a-2cd8-456f-8aee-c93804b2099b, error: list indices must be
    integers or slices, not str.

This happens because the heat stack has the physical_resource_id set to
None for the load balancer, which causes the load_balancer_show method
of octavia client to GET all load balancers, rather than just one. The
returned data is a list, rather than a dict, leading to the error above.

This change fixes the issue by checking if physical_resource_id is set
to None, and skipping the load balancer deletion if so.

Change-Id: I8f4ca497a01ad04db6cb6c4bc81caed0d714b5a6
Story: 2008548
Task: 41669
---
 magnum/common/octavia.py                      |  2 ++
 magnum/tests/unit/common/test_octavia.py      | 24 +++++++++++++++++++
 .../notes/story-2008548-65a571ad15451937.yaml |  6 +++++
 3 files changed, 32 insertions(+)
 create mode 100644 releasenotes/notes/story-2008548-65a571ad15451937.yaml

diff --git a/magnum/common/octavia.py b/magnum/common/octavia.py
index 5c65d9ee61..f069a4c61d 100644
--- a/magnum/common/octavia.py
+++ b/magnum/common/octavia.py
@@ -111,6 +111,8 @@ def delete_loadbalancers(context, cluster):
             filters={"type": lb_resource_type})
         for lb_res in lb_resources:
             lb_id = lb_res.physical_resource_id
+            if not lb_id:
+                continue
             try:
                 lb = octavia_client.load_balancer_show(lb_id)
                 lbs.append(lb)
diff --git a/magnum/tests/unit/common/test_octavia.py b/magnum/tests/unit/common/test_octavia.py
index 09df86ecff..675a328d21 100644
--- a/magnum/tests/unit/common/test_octavia.py
+++ b/magnum/tests/unit/common/test_octavia.py
@@ -145,3 +145,27 @@ class OctaviaTest(base.TestCase):
             self.context,
             self.cluster
         )
+
+    @mock.patch("magnum.common.neutron.delete_floatingip")
+    @mock.patch('magnum.common.clients.OpenStackClients')
+    def test_delete_loadbalancers_already_deleted(self, mock_clients,
+                                                  mock_delete_fip):
+        mock_octavia_client = mock.MagicMock()
+        mock_octavia_client.load_balancer_list.return_value = {
+            "loadbalancers": []
+        }
+
+        mock_heat_client = mock.MagicMock()
+        mock_heat_client.resources.list.return_value = [
+            TestHeatLBResource(None)
+        ]
+
+        osc = mock.MagicMock()
+        mock_clients.return_value = osc
+        osc.octavia.return_value = mock_octavia_client
+        osc.heat.return_value = mock_heat_client
+
+        octavia.delete_loadbalancers(self.context, self.cluster)
+
+        self.assertFalse(mock_octavia_client.load_balancer_show.called)
+        self.assertFalse(mock_octavia_client.load_balancer_delete.called)
diff --git a/releasenotes/notes/story-2008548-65a571ad15451937.yaml b/releasenotes/notes/story-2008548-65a571ad15451937.yaml
new file mode 100644
index 0000000000..f69e6de453
--- /dev/null
+++ b/releasenotes/notes/story-2008548-65a571ad15451937.yaml
@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Fixes an issue with cluster deletion if load balancers do not exist. See
+    `story 2008548 <https://storyboard.openstack.org/#!/story/2008548>` for
+    details.