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):