From e92f0c3addee7dc5900114f844bb03673df633cf Mon Sep 17 00:00:00 2001 From: David Festal Date: Tue, 5 Jun 2018 16:23:01 +0200 Subject: [PATCH] Watch connection manager never closed when trying to delete a non-existing POD (#9932) Fix the root cause of a recurring 1006 web-socket error. The fixed bug is described / discussed in the following issue: https://github.com/redhat-developer/rh-che/issues/672 Signed-off-by: David Festal --- .../kubernetes/namespace/KubernetesPods.java | 20 +++++- .../namespace/KubernetesNamespaceTest.java | 68 +++++++++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesPods.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesPods.java index 8f48936745e..4a038bebe44 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesPods.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesPods.java @@ -15,6 +15,7 @@ import static org.eclipse.che.workspace.infrastructure.kubernetes.Constants.CHE_WORKSPACE_ID_LABEL; import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesObjectUtil.putLabel; +import com.google.common.annotations.VisibleForTesting; import io.fabric8.kubernetes.api.model.DoneablePod; import io.fabric8.kubernetes.api.model.Event; import io.fabric8.kubernetes.api.model.ObjectReference; @@ -553,14 +554,19 @@ public void delete() throws InfrastructureException { } } - private CompletableFuture doDelete(String name) throws InfrastructureException { + @VisibleForTesting + CompletableFuture doDelete(String name) throws InfrastructureException { + Watch toCloseOnException = null; try { final PodResource podResource = clientFactory.create(workspaceId).pods().inNamespace(namespace).withName(name); final CompletableFuture deleteFuture = new CompletableFuture<>(); final Watch watch = podResource.watch(new DeleteWatcher(deleteFuture)); - - podResource.delete(); + toCloseOnException = watch; + Boolean deleteSucceeded = podResource.delete(); + if (deleteSucceeded == null || !deleteSucceeded) { + deleteFuture.complete(null); + } return deleteFuture.whenComplete( (v, e) -> { if (e != null) { @@ -569,7 +575,15 @@ private CompletableFuture doDelete(String name) throws InfrastructureExcep watch.close(); }); } catch (KubernetesClientException ex) { + if (toCloseOnException != null) { + toCloseOnException.close(); + } throw new KubernetesInfrastructureException(ex); + } catch (Exception e) { + if (toCloseOnException != null) { + toCloseOnException.close(); + } + throw e; } } diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java index 08dd070e531..e65b2c5e43f 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java @@ -20,6 +20,8 @@ import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertTrue; +import static org.testng.Assert.fail; import io.fabric8.kubernetes.api.model.DoneableNamespace; import io.fabric8.kubernetes.api.model.DoneableServiceAccount; @@ -33,9 +35,12 @@ import io.fabric8.kubernetes.client.Watcher.Action; import io.fabric8.kubernetes.client.dsl.MixedOperation; import io.fabric8.kubernetes.client.dsl.NonNamespaceOperation; +import io.fabric8.kubernetes.client.dsl.PodResource; import io.fabric8.kubernetes.client.dsl.Resource; +import java.util.concurrent.TimeUnit; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory; +import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesInfrastructureException; import org.mockito.Mock; import org.mockito.stubbing.Answer; import org.mockito.testng.MockitoTestNGListener; @@ -203,6 +208,69 @@ public void testStopsWaitingServiceAccountEventJustAfterEventReceived() throws E verify(serviceAccountResource).watch(any()); } + @Test + public void testDeleteNonExistingPodBeforeWatch() throws Exception { + final MixedOperation mixedOperation = mock(MixedOperation.class); + final NonNamespaceOperation namespaceOperation = mock(NonNamespaceOperation.class); + final PodResource podResource = mock(PodResource.class); + doReturn(mixedOperation).when(kubernetesClient).pods(); + when(mixedOperation.inNamespace(anyString())).thenReturn(namespaceOperation); + when(namespaceOperation.withName(anyString())).thenReturn(podResource); + + doReturn(Boolean.FALSE).when(podResource).delete(); + Watch watch = mock(Watch.class); + doReturn(watch).when(podResource).watch(any()); + + new KubernetesPods("", "", clientFactory).doDelete("nonExistingPod").get(5, TimeUnit.SECONDS); + + verify(watch).close(); + } + + @Test + public void testDeletePodThrowingKubernetesClientExceptionShouldCloseWatch() throws Exception { + final MixedOperation mixedOperation = mock(MixedOperation.class); + final NonNamespaceOperation namespaceOperation = mock(NonNamespaceOperation.class); + final PodResource podResource = mock(PodResource.class); + doReturn(mixedOperation).when(kubernetesClient).pods(); + when(mixedOperation.inNamespace(anyString())).thenReturn(namespaceOperation); + when(namespaceOperation.withName(anyString())).thenReturn(podResource); + + doThrow(KubernetesClientException.class).when(podResource).delete(); + Watch watch = mock(Watch.class); + doReturn(watch).when(podResource).watch(any()); + + try { + new KubernetesPods("", "", clientFactory).doDelete("nonExistingPod").get(5, TimeUnit.SECONDS); + } catch (KubernetesInfrastructureException e) { + assertTrue(e.getCause() instanceof KubernetesClientException); + verify(watch).close(); + return; + } + fail("The exception should have been rethrown"); + } + + @Test + public void testDeletePodThrowingAnyExceptionShouldCloseWatch() throws Exception { + final MixedOperation mixedOperation = mock(MixedOperation.class); + final NonNamespaceOperation namespaceOperation = mock(NonNamespaceOperation.class); + final PodResource podResource = mock(PodResource.class); + doReturn(mixedOperation).when(kubernetesClient).pods(); + when(mixedOperation.inNamespace(anyString())).thenReturn(namespaceOperation); + when(namespaceOperation.withName(anyString())).thenReturn(podResource); + + doThrow(RuntimeException.class).when(podResource).delete(); + Watch watch = mock(Watch.class); + doReturn(watch).when(podResource).watch(any()); + + try { + new KubernetesPods("", "", clientFactory).doDelete("nonExistingPod").get(5, TimeUnit.SECONDS); + } catch (RuntimeException e) { + verify(watch).close(); + return; + } + fail("The exception should have been rethrown"); + } + private MetadataNested prepareCreateNamespaceRequest() { DoneableNamespace namespace = mock(DoneableNamespace.class); MetadataNested metadataNested = mock(MetadataNested.class);