From 49ad990ec9c375371db09a4bd90b14e4cdce6602 Mon Sep 17 00:00:00 2001 From: David Festal Date: Mon, 4 Jun 2018 14:33:39 +0200 Subject: [PATCH 1/6] Fix the root cause of a recurring 1006 web-socket error. Signed-off-by: David Festal --- .../kubernetes/namespace/KubernetesPods.java | 34 +++++++++++++------ 1 file changed, 24 insertions(+), 10 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..5315fdeaef2 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 @@ -554,22 +554,36 @@ public void delete() throws InfrastructureException { } private 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<>(); + CompletableFuture deleteFuture = new CompletableFuture<>(); final Watch watch = podResource.watch(new DeleteWatcher(deleteFuture)); - - podResource.delete(); - return deleteFuture.whenComplete( - (v, e) -> { - if (e != null) { - LOG.warn("Failed to remove pod {} cause {}", name, e.getMessage()); - } - watch.close(); - }); + toCloseOnException = watch; + deleteFuture = + deleteFuture.whenComplete( + (v, e) -> { + if (e != null) { + LOG.warn("Failed to remove pod {} cause {}", name, e.getMessage()); + } + watch.close(); + }); + Boolean deleteSucceeded = podResource.delete(); + if (deleteSucceeded == null || !deleteSucceeded) { + watch.close(); + } + return deleteFuture; } catch (KubernetesClientException ex) { + if (toCloseOnException != null) { + toCloseOnException.close(); + } throw new KubernetesInfrastructureException(ex); + } catch (Exception e) { + if (toCloseOnException != null) { + toCloseOnException.close(); + } + throw e; } } From 3cfb53823d2221af36db9e30b0f4f0f8a472d939 Mon Sep 17 00:00:00 2001 From: David Festal Date: Mon, 4 Jun 2018 20:50:27 +0200 Subject: [PATCH 2/6] Added a test for the fixed use-case Signed-off-by: David Festal --- .../namespace/KubernetesNamespaceTest.java | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) 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..aa9872e02cd 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 @@ -33,7 +33,11 @@ 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.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory; import org.mockito.Mock; @@ -203,6 +207,33 @@ public void testStopsWaitingServiceAccountEventJustAfterEventReceived() throws E verify(serviceAccountResource).watch(any()); } + @Test + public void testDeleteNonExistingPodBeforeWatch() { + System.out.println( + "Start the test of testDeleteNonExistingPodBeforeWatch testDeleteNonExistingPodBeforeWatch testDeleteNonExistingPodBeforeWatch"); + 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()); + + try { + new KubernetesPods("", "", clientFactory).doDelete("nonExistingPod").get(5, TimeUnit.SECONDS); + } catch (InfrastructureException + | InterruptedException + | ExecutionException + | TimeoutException e) { + e.printStackTrace(); + } + + verify(watch).close(); + } + private MetadataNested prepareCreateNamespaceRequest() { DoneableNamespace namespace = mock(DoneableNamespace.class); MetadataNested metadataNested = mock(MetadataNested.class); From bbabfdff677a20c440cf9bfefff624a51aa82759 Mon Sep 17 00:00:00 2001 From: David Festal Date: Mon, 4 Jun 2018 20:52:43 +0200 Subject: [PATCH 3/6] call `deleteFuture.complete()` instead of closing the watch directly as proposed by @sleshchenko Signed-off-by: David Festal --- .../kubernetes/namespace/KubernetesPods.java | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 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 5315fdeaef2..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,27 +554,26 @@ 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); - CompletableFuture deleteFuture = new CompletableFuture<>(); + final CompletableFuture deleteFuture = new CompletableFuture<>(); final Watch watch = podResource.watch(new DeleteWatcher(deleteFuture)); toCloseOnException = watch; - deleteFuture = - deleteFuture.whenComplete( - (v, e) -> { - if (e != null) { - LOG.warn("Failed to remove pod {} cause {}", name, e.getMessage()); - } - watch.close(); - }); Boolean deleteSucceeded = podResource.delete(); if (deleteSucceeded == null || !deleteSucceeded) { - watch.close(); + deleteFuture.complete(null); } - return deleteFuture; + return deleteFuture.whenComplete( + (v, e) -> { + if (e != null) { + LOG.warn("Failed to remove pod {} cause {}", name, e.getMessage()); + } + watch.close(); + }); } catch (KubernetesClientException ex) { if (toCloseOnException != null) { toCloseOnException.close(); From f80c84aa8bdf8fd5137adea1dff8758e098576b4 Mon Sep 17 00:00:00 2001 From: David Festal Date: Tue, 5 Jun 2018 09:57:45 +0200 Subject: [PATCH 4/6] remove unnecessary debug message Signed-off-by: David Festal --- .../kubernetes/namespace/KubernetesNamespaceTest.java | 2 -- 1 file changed, 2 deletions(-) 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 aa9872e02cd..c1d31f339a6 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 @@ -209,8 +209,6 @@ public void testStopsWaitingServiceAccountEventJustAfterEventReceived() throws E @Test public void testDeleteNonExistingPodBeforeWatch() { - System.out.println( - "Start the test of testDeleteNonExistingPodBeforeWatch testDeleteNonExistingPodBeforeWatch testDeleteNonExistingPodBeforeWatch"); final MixedOperation mixedOperation = mock(MixedOperation.class); final NonNamespaceOperation namespaceOperation = mock(NonNamespaceOperation.class); final PodResource podResource = mock(PodResource.class); From 000e135dca1ce415cc781a38c3027d93524f7886 Mon Sep 17 00:00:00 2001 From: David Festal Date: Tue, 5 Jun 2018 10:01:06 +0200 Subject: [PATCH 5/6] Change according to @sleshchenko comment [here](https://github.com/eclipse/che/pull/9932#discussion_r192953713) Signed-off-by: David Festal --- .../namespace/KubernetesNamespaceTest.java | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) 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 c1d31f339a6..64aac38956e 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 @@ -35,9 +35,7 @@ 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.ExecutionException; import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory; import org.mockito.Mock; @@ -208,7 +206,7 @@ public void testStopsWaitingServiceAccountEventJustAfterEventReceived() throws E } @Test - public void testDeleteNonExistingPodBeforeWatch() { + public void testDeleteNonExistingPodBeforeWatch() throws Exception { final MixedOperation mixedOperation = mock(MixedOperation.class); final NonNamespaceOperation namespaceOperation = mock(NonNamespaceOperation.class); final PodResource podResource = mock(PodResource.class); @@ -220,14 +218,7 @@ public void testDeleteNonExistingPodBeforeWatch() { Watch watch = mock(Watch.class); doReturn(watch).when(podResource).watch(any()); - try { - new KubernetesPods("", "", clientFactory).doDelete("nonExistingPod").get(5, TimeUnit.SECONDS); - } catch (InfrastructureException - | InterruptedException - | ExecutionException - | TimeoutException e) { - e.printStackTrace(); - } + new KubernetesPods("", "", clientFactory).doDelete("nonExistingPod").get(5, TimeUnit.SECONDS); verify(watch).close(); } From 3892ce58afba8bd9e09d609d638eb179b178d428 Mon Sep 17 00:00:00 2001 From: David Festal Date: Tue, 5 Jun 2018 10:27:26 +0200 Subject: [PATCH 6/6] Add 2 tests to validate that the watch is closed even on exception Signed-off-by: David Festal --- .../namespace/KubernetesNamespaceTest.java | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) 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 64aac38956e..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; @@ -38,6 +40,7 @@ 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; @@ -223,6 +226,51 @@ public void testDeleteNonExistingPodBeforeWatch() throws Exception { 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);