From f2e3ffb585a8c98f6994373dc66cac1c549ae659 Mon Sep 17 00:00:00 2001 From: Roman Dobosz Date: Fri, 4 Sep 2020 12:40:19 +0200 Subject: [PATCH] Catch exceptions for deleted pod. It might happen, that during adding and instantly removing pod, vif handler will not be able to set the finalizer for such object, resulting in error. In this patch we prevent for such case. Closes-Bug: 1894212 Change-Id: I5b3b4b36ae0c2fca7c16153f75c62607efbc3ce4 --- .../controller/handlers/kuryrport.py | 9 +++++++-- kuryr_kubernetes/controller/handlers/lbaas.py | 5 +++-- kuryr_kubernetes/controller/handlers/vif.py | 16 ++++++++++++++-- kuryr_kubernetes/k8s_client.py | 15 +++++++++------ .../tests/unit/controller/handlers/test_vif.py | 17 +++++++++++++++++ 5 files changed, 50 insertions(+), 12 deletions(-) diff --git a/kuryr_kubernetes/controller/handlers/kuryrport.py b/kuryr_kubernetes/controller/handlers/kuryrport.py index 6567fc551..c533ddc4f 100644 --- a/kuryr_kubernetes/controller/handlers/kuryrport.py +++ b/kuryr_kubernetes/controller/handlers/kuryrport.py @@ -133,8 +133,13 @@ class KuryrPortHandler(k8s_base.ResourceEventHandler): LOG.error("Pod %s/%s doesn't exists, deleting orphaned KuryrPort", namespace, name) # TODO(gryf): Free resources - self.k8s.remove_finalizer(kuryrport_crd, - constants.KURYRPORT_FINALIZER) + try: + self.k8s.remove_finalizer(kuryrport_crd, + constants.KURYRPORT_FINALIZER) + except k_exc.K8sClientException as ex: + LOG.exception("Failed to remove finalizer from KuryrPort %s", + ex) + raise return if 'deletionTimestamp' not in pod['metadata']: diff --git a/kuryr_kubernetes/controller/handlers/lbaas.py b/kuryr_kubernetes/controller/handlers/lbaas.py index feb8186d7..45f6dd62d 100644 --- a/kuryr_kubernetes/controller/handlers/lbaas.py +++ b/kuryr_kubernetes/controller/handlers/lbaas.py @@ -56,7 +56,8 @@ class ServiceHandler(k8s_base.ResourceEventHandler): k8s = clients.get_kubernetes_client() loadbalancer_crd = k8s.get_loadbalancer_crd(service) try: - self._patch_service_finalizer(service) + if not self._patch_service_finalizer(service): + return except k_exc.K8sClientException as ex: LOG.exception("Failed to set service finalizer: %s", ex) raise @@ -100,7 +101,7 @@ class ServiceHandler(k8s_base.ResourceEventHandler): def _patch_service_finalizer(self, service): k8s = clients.get_kubernetes_client() - k8s.add_finalizer(service, k_const.SERVICE_FINALIZER) + return k8s.add_finalizer(service, k_const.SERVICE_FINALIZER) def on_finalize(self, service): k8s = clients.get_kubernetes_client() diff --git a/kuryr_kubernetes/controller/handlers/vif.py b/kuryr_kubernetes/controller/handlers/vif.py index bd43c4f80..7ffee265e 100644 --- a/kuryr_kubernetes/controller/handlers/vif.py +++ b/kuryr_kubernetes/controller/handlers/vif.py @@ -75,7 +75,15 @@ class VIFHandler(k8s_base.ResourceEventHandler): # subsequent updates of the pod, add_finalizer will ignore this if # finalizer exists. k8s = clients.get_kubernetes_client() - k8s.add_finalizer(pod, constants.POD_FINALIZER) + + try: + if not k8s.add_finalizer(pod, constants.POD_FINALIZER): + # NOTE(gryf) It might happen that pod will be deleted even + # before we got here. + return + except k_exc.K8sClientException as ex: + LOG.exception("Failed to add finalizer to pod object: %s", ex) + raise if self._move_annotations_to_crd(pod): return @@ -114,7 +122,11 @@ class VIFHandler(k8s_base.ResourceEventHandler): kp = k8s.get(KURYRPORT_URI.format(ns=pod["metadata"]["namespace"], crd=pod["metadata"]["name"])) except k_exc.K8sResourceNotFound: - k8s.remove_finalizer(pod, constants.POD_FINALIZER) + try: + k8s.remove_finalizer(pod, constants.POD_FINALIZER) + except k_exc.K8sClientException as ex: + LOG.exception('Failed to remove finalizer from pod: %s', ex) + raise return if 'deletionTimestamp' in kp['metadata']: diff --git a/kuryr_kubernetes/k8s_client.py b/kuryr_kubernetes/k8s_client.py index 02dc035ce..06aec34cd 100644 --- a/kuryr_kubernetes/k8s_client.py +++ b/kuryr_kubernetes/k8s_client.py @@ -227,7 +227,7 @@ class K8sClient(object): # duplication, but I don't see a nice way to avoid it. def add_finalizer(self, obj, finalizer): if finalizer in obj['metadata'].get('finalizers', []): - return obj + return True path = obj['metadata']['selfLink'] LOG.debug(f"Add finalizer {finalizer} to {path}") @@ -250,15 +250,18 @@ class K8sClient(object): verify=self.verify_server) if response.ok: - return response.json() + return True try: self._raise_from_response(response) + except (exc.K8sForbidden, exc.K8sResourceNotFound): + # Object is being deleting or gone. Return. + return False except exc.K8sConflict: obj = self.get(path) if finalizer in obj['metadata'].get('finalizers', []): # Finalizer is there, return. - return obj + return True # If after 3 iterations there's still conflict, just raise. self._raise_from_response(response) @@ -275,7 +278,7 @@ class K8sClient(object): finalizers.remove(finalizer) except ValueError: # Finalizer is not there, return. - return obj + return True data = { 'metadata': { @@ -289,7 +292,7 @@ class K8sClient(object): verify=self.verify_server) if response.ok: - return response.json() + return True try: try: @@ -298,7 +301,7 @@ class K8sClient(object): obj = self.get(path) except exc.K8sResourceNotFound: # Object is gone already, stop. - return None + return False # If after 3 iterations there's still conflict, just raise. self._raise_from_response(response) diff --git a/kuryr_kubernetes/tests/unit/controller/handlers/test_vif.py b/kuryr_kubernetes/tests/unit/controller/handlers/test_vif.py index 023aeefc8..c78129ca4 100644 --- a/kuryr_kubernetes/tests/unit/controller/handlers/test_vif.py +++ b/kuryr_kubernetes/tests/unit/controller/handlers/test_vif.py @@ -308,6 +308,23 @@ class TestVIFHandler(test_base.TestCase): self._request_additional_vifs.assert_not_called() self._activate_vif.assert_not_called() + @mock.patch('kuryr_kubernetes.clients.get_kubernetes_client') + @mock.patch('kuryr_kubernetes.controller.drivers.utils.is_host_network') + def test_on_present_pod_finalizer_exception(self, m_host_network, + m_get_k8s_client): + m_host_network.return_value = False + self._matc.return_value = True + k8s = mock.MagicMock() + k8s.add_finalizer.side_effect = k_exc.K8sClientException + m_get_k8s_client.return_value = k8s + + self.assertRaises(k_exc.K8sClientException, + h_vif.VIFHandler.on_present, self._handler, + self._pod) + + k8s.add_finalizer.assert_called_once_with(self._pod, + k_const.POD_FINALIZER) + @mock.patch('kuryr_kubernetes.clients.get_kubernetes_client') @mock.patch('kuryr_kubernetes.controller.drivers.utils.get_kuryrport') def test_on_finalize_crd(self, m_get_kuryrport, m_get_k8s_client):