Watch connection manager never closed when trying to delete a non-exi…

…sting POD (eclipse-che#9932) Fix the root cause of a recurring 1006 web-socket error. The fixed bug is described / discussed in the following issue: redhat-developer/rh-che#672 Signed-off-by: David Festal <dfestal@redhat.com>
hbhargav · Dec 4, 2018 · e92f0c3 · e92f0c3
1 parent 43adc44
commit e92f0c3
Show file tree

Hide file tree

Showing 2 changed files with 85 additions and 3 deletions.
diff --git a/...in/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesPods.java b/...in/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<Void> doDelete(String name) throws InfrastructureException {
+  @VisibleForTesting
+  CompletableFuture<Void> doDelete(String name) throws InfrastructureException {
+    Watch toCloseOnException = null;
     try {
       final PodResource<Pod, DoneablePod> podResource =
           clientFactory.create(workspaceId).pods().inNamespace(namespace).withName(name);
       final CompletableFuture<Void> 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<Void> 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/...rg/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java b/...rg/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);