From 4c3e338273b8ac31c33e1507b8794be4eea36b08 Mon Sep 17 00:00:00 2001 From: Yash Gupta Date: Tue, 3 Sep 2019 14:14:40 +0900 Subject: [PATCH] Reuse utils.get_lbaas_spec in lb handler LoadBalancerHandler._get_lbaas_spec is identical to utils.get_lbaas_spec, which is used by LBaaSSpecHandler, so we can reuse the function from utils instead of duplication of code. Note that the passed k8s object is endpoint in case of LoadBalancerHandler and service in case of LBaaSSpecHandler (but same annotation used in both cases, so a common function should be enough) Change-Id: I124109f79bcdefcc4948eb35b4bbb4a9ca87c43b Signed-off-by: Yash Gupta --- .../controller/handlers/ingress_lbaas.py | 3 ++- kuryr_kubernetes/controller/handlers/lbaas.py | 15 +----------- .../controller/handlers/test_ingress_lbaas.py | 5 ++-- .../unit/controller/handlers/test_lbaas.py | 24 +++++++++++-------- kuryr_kubernetes/utils.py | 5 ++-- 5 files changed, 23 insertions(+), 29 deletions(-) diff --git a/kuryr_kubernetes/controller/handlers/ingress_lbaas.py b/kuryr_kubernetes/controller/handlers/ingress_lbaas.py index 699dc7b2c..4c45b42de 100644 --- a/kuryr_kubernetes/controller/handlers/ingress_lbaas.py +++ b/kuryr_kubernetes/controller/handlers/ingress_lbaas.py @@ -23,6 +23,7 @@ from kuryr_kubernetes.controller.drivers import base as drv_base from kuryr_kubernetes.controller.handlers import lbaas as h_lbaas from kuryr_kubernetes.controller.ingress import ingress_ctl from kuryr_kubernetes.objects import lbaas as obj_lbaas +from kuryr_kubernetes import utils LOG = logging.getLogger(__name__) @@ -55,7 +56,7 @@ class IngressLoadBalancerHandler(h_lbaas.LoadBalancerHandler): LOG.info("No L7 router found - do nothing") return - lbaas_spec = self._get_lbaas_spec(endpoints) + lbaas_spec = utils.get_lbaas_spec(endpoints) if self._should_ignore(endpoints, lbaas_spec): return diff --git a/kuryr_kubernetes/controller/handlers/lbaas.py b/kuryr_kubernetes/controller/handlers/lbaas.py index 79c05f82c..141e6dd94 100644 --- a/kuryr_kubernetes/controller/handlers/lbaas.py +++ b/kuryr_kubernetes/controller/handlers/lbaas.py @@ -15,7 +15,6 @@ from kuryr.lib._i18n import _ from oslo_log import log as logging -from oslo_serialization import jsonutils from kuryr_kubernetes import clients from kuryr_kubernetes import config @@ -171,7 +170,7 @@ class LoadBalancerHandler(k8s_base.ResourceEventHandler): config.CONF.kubernetes.endpoints_driver_octavia_provider) def on_present(self, endpoints): - lbaas_spec = self._get_lbaas_spec(endpoints) + lbaas_spec = utils.get_lbaas_spec(endpoints) if self._should_ignore(endpoints, lbaas_spec): LOG.debug("Ignoring Kubernetes endpoints %s", endpoints['metadata']['name']) @@ -622,15 +621,3 @@ class LoadBalancerHandler(k8s_base.ResourceEventHandler): lbaas_state.loadbalancer = lb return changed - - def _get_lbaas_spec(self, endpoints): - # TODO(ivc): same as '_get_lbaas_state' - try: - annotations = endpoints['metadata']['annotations'] - annotation = annotations[k_const.K8S_ANNOTATION_LBAAS_SPEC] - except KeyError: - return None - obj_dict = jsonutils.loads(annotation) - obj = obj_lbaas.LBaaSServiceSpec.obj_from_primitive(obj_dict) - LOG.debug("Got LBaaSServiceSpec from annotation: %r", obj) - return obj diff --git a/kuryr_kubernetes/tests/unit/controller/handlers/test_ingress_lbaas.py b/kuryr_kubernetes/tests/unit/controller/handlers/test_ingress_lbaas.py index b9c679a82..7be612cbf 100644 --- a/kuryr_kubernetes/tests/unit/controller/handlers/test_ingress_lbaas.py +++ b/kuryr_kubernetes/tests/unit/controller/handlers/test_ingress_lbaas.py @@ -33,14 +33,15 @@ class TestIngressLoadBalancerHandler(t_lbaas.TestLoadBalancerHandler): self.assertEqual(mock.sentinel.drv_lbaas, handler._drv_lbaas) - def test_on_present_no_ing_ctrlr(self): + @mock.patch('kuryr_kubernetes.utils.get_lbaas_spec') + def test_on_present_no_ing_ctrlr(self, m_get_lbaas_spec): endpoints = mock.sentinel.endpoints m_handler = mock.Mock(spec=h_ing_lbaas.IngressLoadBalancerHandler) m_handler._l7_router = None h_ing_lbaas.IngressLoadBalancerHandler.on_present(m_handler, endpoints) - m_handler._get_lbaas_spec.assert_not_called() + m_get_lbaas_spec.assert_not_called() m_handler._should_ignore.assert_not_called() def test_should_ignore(self): diff --git a/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py b/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py index daf0266b5..4e67fe986 100644 --- a/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py +++ b/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py @@ -441,9 +441,11 @@ class TestLoadBalancerHandler(test_base.TestCase): self.assertEqual(mock.sentinel.drv_lb_ip, handler._drv_service_pub_ip) self.assertEqual('ovn', handler._lb_provider) + @mock.patch('kuryr_kubernetes.utils.get_lbaas_spec') @mock.patch('kuryr_kubernetes.utils.set_lbaas_state') @mock.patch('kuryr_kubernetes.utils.get_lbaas_state') - def test_on_present(self, m_get_lbaas_state, m_set_lbaas_state): + def test_on_present(self, m_get_lbaas_state, m_set_lbaas_state, + m_get_lbaas_spec): lbaas_spec = mock.sentinel.lbaas_spec lbaas_spec.type = 'DummyType' lbaas_spec.lb_ip = "1.2.3.4" @@ -461,7 +463,7 @@ class TestLoadBalancerHandler(test_base.TestCase): m_drv_service_pub_ip.associate_pub_ip.return_value = True m_handler = mock.Mock(spec=h_lbaas.LoadBalancerHandler) - m_handler._get_lbaas_spec.return_value = lbaas_spec + m_get_lbaas_spec.return_value = lbaas_spec m_handler._should_ignore.return_value = False m_get_lbaas_state.return_value = lbaas_state m_handler._sync_lbaas_members.return_value = True @@ -469,7 +471,7 @@ class TestLoadBalancerHandler(test_base.TestCase): h_lbaas.LoadBalancerHandler.on_present(m_handler, endpoints) - m_handler._get_lbaas_spec.assert_called_once_with(endpoints) + m_get_lbaas_spec.assert_called_once_with(endpoints) m_handler._should_ignore.assert_called_once_with(endpoints, lbaas_spec) m_get_lbaas_state.assert_called_once_with(endpoints) m_handler._sync_lbaas_members.assert_called_once_with( @@ -485,10 +487,11 @@ class TestLoadBalancerHandler(test_base.TestCase): lbaas_state.service_pub_ip_info = None return True + @mock.patch('kuryr_kubernetes.utils.get_lbaas_spec') @mock.patch('kuryr_kubernetes.utils.set_lbaas_state') @mock.patch('kuryr_kubernetes.utils.get_lbaas_state') - def test_on_present_loadbalancer_service(self, m_get_lbaas_state, - m_set_lbaas_state): + def test_on_present_loadbalancer_service( + self, m_get_lbaas_state, m_set_lbaas_state, m_get_lbaas_spec): lbaas_spec = mock.sentinel.lbaas_spec lbaas_spec.type = 'LoadBalancer' lbaas_spec.lb_ip = "1.2.3.4" @@ -511,7 +514,7 @@ class TestLoadBalancerHandler(test_base.TestCase): m_drv_service_pub_ip.associate_pub_ip.return_value = True m_handler = mock.Mock(spec=h_lbaas.LoadBalancerHandler) - m_handler._get_lbaas_spec.return_value = lbaas_spec + m_get_lbaas_spec.return_value = lbaas_spec m_handler._should_ignore.return_value = False m_get_lbaas_state.return_value = lbaas_state m_handler._sync_lbaas_members = self._fake_sync_lbaas_members @@ -519,17 +522,18 @@ class TestLoadBalancerHandler(test_base.TestCase): h_lbaas.LoadBalancerHandler.on_present(m_handler, endpoints) - m_handler._get_lbaas_spec.assert_called_once_with(endpoints) + m_get_lbaas_spec.assert_called_once_with(endpoints) m_handler._should_ignore.assert_called_once_with(endpoints, lbaas_spec) m_get_lbaas_state.assert_called_once_with(endpoints) m_set_lbaas_state.assert_called_once_with( endpoints, lbaas_state) m_handler._update_lb_status.assert_called() + @mock.patch('kuryr_kubernetes.utils.get_lbaas_spec') @mock.patch('kuryr_kubernetes.utils.set_lbaas_state') @mock.patch('kuryr_kubernetes.utils.get_lbaas_state') def test_on_present_rollback(self, m_get_lbaas_state, - m_set_lbaas_state): + m_set_lbaas_state, m_get_lbaas_spec): lbaas_spec = mock.sentinel.lbaas_spec lbaas_spec.type = 'ClusterIp' lbaas_spec.lb_ip = '1.2.3.4' @@ -546,7 +550,7 @@ class TestLoadBalancerHandler(test_base.TestCase): endpoints = mock.sentinel.endpoints m_handler = mock.Mock(spec=h_lbaas.LoadBalancerHandler) - m_handler._get_lbaas_spec.return_value = lbaas_spec + m_get_lbaas_spec.return_value = lbaas_spec m_handler._should_ignore.return_value = False m_get_lbaas_state.return_value = lbaas_state m_handler._sync_lbaas_members.return_value = True @@ -555,7 +559,7 @@ class TestLoadBalancerHandler(test_base.TestCase): m_handler._drv_service_pub_ip = m_drv_service_pub_ip h_lbaas.LoadBalancerHandler.on_present(m_handler, endpoints) - m_handler._get_lbaas_spec.assert_called_once_with(endpoints) + m_get_lbaas_spec.assert_called_once_with(endpoints) m_handler._should_ignore.assert_called_once_with(endpoints, lbaas_spec) m_get_lbaas_state.assert_called_once_with(endpoints) m_handler._sync_lbaas_members.assert_called_once_with( diff --git a/kuryr_kubernetes/utils.py b/kuryr_kubernetes/utils.py index 05035af0f..a6a27b722 100644 --- a/kuryr_kubernetes/utils.py +++ b/kuryr_kubernetes/utils.py @@ -242,9 +242,10 @@ def has_kuryr_crd(crd_url): return True -def get_lbaas_spec(service): +def get_lbaas_spec(k8s_object): + # k8s_object can be service or endpoint try: - annotations = service['metadata']['annotations'] + annotations = k8s_object['metadata']['annotations'] annotation = annotations[constants.K8S_ANNOTATION_LBAAS_SPEC] except KeyError: return None