Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Watch connection manager never closed when trying to delete a non-existing POD #9932

Merged
merged 6 commits into from Jun 5, 2018
Merged

Watch connection manager never closed when trying to delete a non-existing POD #9932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
49ad990
Fix the root cause of a recurring 1006 web-socket error.
davidfestal Jun 4, 2018
3cfb538
Added a test for the fixed use-case
davidfestal Jun 4, 2018
bbabfdf
call `deleteFuture.complete()` instead of closing the watch directly
davidfestal Jun 4, 2018
f80c84a
remove unnecessary debug message
davidfestal Jun 5, 2018
000e135
Change according to @sleshchenko comment
davidfestal Jun 5, 2018
3892ce5
Add 2 tests to validate that the watch is closed even on exception
davidfestal Jun 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 17 additions & 3 deletions ...in/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesPods.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can propose you to rewrite this method in the following way:

@VisibleForTesting
  CompletableFuture<Void> doDelete(String name) {
    final CompletableFuture<Void> deleteFuture = new CompletableFuture<>();
    try {
      final PodResource<Pod, DoneablePod> podResource =
          clientFactory.create(workspaceId).pods().inNamespace(namespace).withName(name);

      Watch watch = podResource.watch(new DeleteWatcher(deleteFuture));
      //watch is opened - register callback to close it
      deleteFuture.whenComplete((v, e) -> watch.close());

      Boolean deleteSucceeded = podResource.delete();
      if (deleteSucceeded == null || !deleteSucceeded) {
        deleteFuture.complete(null);
      }
    } catch (KubernetesClientException ex) {
      deleteFuture.completeExceptionally(new KubernetesInfrastructureException(ex));
    } catch (Exception e) {
      deleteFuture.completeExceptionally(new InternalInfrastructureException(e.getMessage(), e));
    }

    return deleteFuture;
  }

Why it can be better - because in a case when an exception occurs during removing many pods - then there will be trying to remove all pods. With current approach - if an exception occurs then no more pods will be removed. And I think it would be better to try to clean up resources as many as we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind if we do this change in another PR ?
I'd prefer keep the behavior as much identical as possible to what was previously, and only fix the bug of the non-closed watch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidfestal Makes sense. I'm OK with that

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) {
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be better to wrap this exception into InternalInfastructureException

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, honestly I prefer re-throwing exactly the same exception, so that there is no behavior change compared to the previous implementation that used to only catch KubernetesClientException exceptions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's the same as was before. But only InfrastructureException is declared as throwing and it would be safer to wrap other exception in InternalInfastructureException.
InternalInfastructureException is special kind of exception that should wrap unexpected exceptions like NullPointerException which happen because of developer fault.
I would recommend wrapping but you can leave it as it is since it doesn't change current approach.

}
}

Expand Down
68 changes: 68 additions & 0 deletions ...rg/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java
View file
Open in desktop
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down