From 535dc7c69885d661fed2a59ac7d4b8b3ce89e064 Mon Sep 17 00:00:00 2001
From: Carlos Goncalves <cgoncalves@redhat.com>
Date: Thu, 21 Nov 2019 22:31:26 +0100
Subject: [PATCH] Fix load balancer update with provider filtered params

When only setting tags to an existing load balancer, the amphora driver
would try to apply QoS policy on VRRP ports even if no policy is
defined. This raised an NetworkException that led load balancers to go
into ERROR.

Task: 37589
Story: 2006922

Change-Id: I48315f8f293811e1d99584ea36da05c4211cf275
(cherry picked from commit 2cfcd71a8659583d91cda3e488c60ded078c9677)
---
 .../controller/worker/v1/tasks/network_tasks.py  |  2 +-
 .../controller/worker/v2/tasks/network_tasks.py  |  2 +-
 .../worker/v1/tasks/test_network_tasks.py        | 16 ++++++++++++++++
 .../worker/v2/tasks/test_network_tasks.py        | 16 ++++++++++++++++
 ...-lb-update-with-no-data-abefe7860b8fb4c7.yaml |  5 +++++
 5 files changed, 39 insertions(+), 2 deletions(-)
 create mode 100644 releasenotes/notes/fix-lb-update-with-no-data-abefe7860b8fb4c7.yaml

diff --git a/octavia/controller/worker/v1/tasks/network_tasks.py b/octavia/controller/worker/v1/tasks/network_tasks.py
index 0f1535f88d..f2831dcea7 100644
--- a/octavia/controller/worker/v1/tasks/network_tasks.py
+++ b/octavia/controller/worker/v1/tasks/network_tasks.py
@@ -594,7 +594,7 @@ class ApplyQos(BaseNetworkTask):
         """Apply qos policy on the vrrp ports which are related with vip."""
         qos_policy_id = loadbalancer.vip.qos_policy_id
         if not qos_policy_id and (
-            update_dict and (
+            not update_dict or (
                 'vip' not in update_dict or
                 'qos_policy_id' not in update_dict['vip'])):
             return
diff --git a/octavia/controller/worker/v2/tasks/network_tasks.py b/octavia/controller/worker/v2/tasks/network_tasks.py
index 0f1535f88d..f2831dcea7 100644
--- a/octavia/controller/worker/v2/tasks/network_tasks.py
+++ b/octavia/controller/worker/v2/tasks/network_tasks.py
@@ -594,7 +594,7 @@ class ApplyQos(BaseNetworkTask):
         """Apply qos policy on the vrrp ports which are related with vip."""
         qos_policy_id = loadbalancer.vip.qos_policy_id
         if not qos_policy_id and (
-            update_dict and (
+            not update_dict or (
                 'vip' not in update_dict or
                 'qos_policy_id' not in update_dict['vip'])):
             return
diff --git a/octavia/tests/unit/controller/worker/v1/tasks/test_network_tasks.py b/octavia/tests/unit/controller/worker/v1/tasks/test_network_tasks.py
index 912c67b4ba..896b93a473 100644
--- a/octavia/tests/unit/controller/worker/v1/tasks/test_network_tasks.py
+++ b/octavia/tests/unit/controller/worker/v1/tasks/test_network_tasks.py
@@ -571,6 +571,22 @@ class TestNetworkTasks(base.TestCase):
             t_constants.MOCK_QOS_POLICY_ID1, mock.ANY)
         self.assertEqual(2, mock_driver.apply_qos_on_port.call_count)
 
+        mock_driver.reset_mock()
+        update_dict = {'description': 'fool',
+                       'vip': {
+                           'qos_policy_id': t_constants.MOCK_QOS_POLICY_ID1}}
+        tmp_lb.amphorae = AMPS_DATA
+        tmp_lb.topology = constants.TOPOLOGY_ACTIVE_STANDBY
+        net.execute(tmp_lb, update_dict=update_dict)
+        mock_driver.apply_qos_on_port.assert_called_with(
+            t_constants.MOCK_QOS_POLICY_ID1, mock.ANY)
+        self.assertEqual(2, mock_driver.apply_qos_on_port.call_count)
+
+        mock_driver.reset_mock()
+        update_dict = {}
+        net.execute(null_qos_lb, update_dict=update_dict)
+        self.assertEqual(0, mock_driver.apply_qos_on_port.call_count)
+
         # revert
         mock_driver.reset_mock()
         tmp_lb.amphorae = [AMPS_DATA[0]]
diff --git a/octavia/tests/unit/controller/worker/v2/tasks/test_network_tasks.py b/octavia/tests/unit/controller/worker/v2/tasks/test_network_tasks.py
index d15392fe8e..2fbed96bcf 100644
--- a/octavia/tests/unit/controller/worker/v2/tasks/test_network_tasks.py
+++ b/octavia/tests/unit/controller/worker/v2/tasks/test_network_tasks.py
@@ -571,6 +571,22 @@ class TestNetworkTasks(base.TestCase):
             t_constants.MOCK_QOS_POLICY_ID1, mock.ANY)
         self.assertEqual(2, mock_driver.apply_qos_on_port.call_count)
 
+        mock_driver.reset_mock()
+        update_dict = {'description': 'fool',
+                       'vip': {
+                           'qos_policy_id': t_constants.MOCK_QOS_POLICY_ID1}}
+        tmp_lb.amphorae = AMPS_DATA
+        tmp_lb.topology = constants.TOPOLOGY_ACTIVE_STANDBY
+        net.execute(tmp_lb, update_dict=update_dict)
+        mock_driver.apply_qos_on_port.assert_called_with(
+            t_constants.MOCK_QOS_POLICY_ID1, mock.ANY)
+        self.assertEqual(2, mock_driver.apply_qos_on_port.call_count)
+
+        mock_driver.reset_mock()
+        update_dict = {}
+        net.execute(null_qos_lb, update_dict=update_dict)
+        self.assertEqual(0, mock_driver.apply_qos_on_port.call_count)
+
         # revert
         mock_driver.reset_mock()
         tmp_lb.amphorae = [AMPS_DATA[0]]
diff --git a/releasenotes/notes/fix-lb-update-with-no-data-abefe7860b8fb4c7.yaml b/releasenotes/notes/fix-lb-update-with-no-data-abefe7860b8fb4c7.yaml
new file mode 100644
index 0000000000..ffd8b0dac0
--- /dev/null
+++ b/releasenotes/notes/fix-lb-update-with-no-data-abefe7860b8fb4c7.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fixed an issue where load balancers would go into ERROR when
+    setting data not visible to providers (e.g. tags).