From 173ab63c4a711bfff3a3ad1f5254d366a38176f8 Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Tue, 30 Jun 2020 12:02:00 -0400 Subject: [PATCH 1/4] Make PropagationPolicy configurable when deletingExisting is configured await for deletion if the just-deleted resource still exists when trying to create --- CHANGELOG.md | 1 + .../client/dsl/CascadingDeletable.java | 2 +- ...ServerGetDeleteRecreateWaitApplicable.java | 6 +- ...rGetWatchDeleteRecreateWaitApplicable.java | 6 +- ...WatchDeleteRecreateWaitApplicableImpl.java | 4 +- ...hDeleteRecreateWaitApplicableListImpl.java | 4 +- .../client/utils/DeleteAndCreateHelper.java | 41 +++++++++++- .../utils/DeleteAndCreateHelperTest.java | 63 ++++++++++++++++++- .../io/fabric8/kubernetes/ResourceIT.java | 61 ++++++++++++++++++ .../kubernetes/client/mock/ResourceTest.java | 27 ++++++++ 10 files changed, 199 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e474e1132a..ae99b12ff42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ * Fix #2656: Binding operations can be instantiated #### Improvements +* Fix: Allow specifying PropagationPolicy when using deleteExisting * Fix: CustomResourceDefinitionContext.fromCrd support for v1 CustomResourceDefinition * Fix #2642: Update kubernetes-examples to use apps/v1 Deployment rather than extensions/v1beta1 diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/CascadingDeletable.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/CascadingDeletable.java index e670a6559cb..661856e7ee4 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/CascadingDeletable.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/CascadingDeletable.java @@ -18,5 +18,5 @@ /** * Created by iocanel on 9/15/16. */ -public interface CascadingDeletable extends Deletable, Cascading { +public interface CascadingDeletable extends Deletable, Cascading, Recreateable> { } diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/ListVisitFromServerGetDeleteRecreateWaitApplicable.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/ListVisitFromServerGetDeleteRecreateWaitApplicable.java index 6be63078d01..9f8a527aac5 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/ListVisitFromServerGetDeleteRecreateWaitApplicable.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/ListVisitFromServerGetDeleteRecreateWaitApplicable.java @@ -26,8 +26,8 @@ public interface ListVisitFromServerGetDeleteRecreateWaitApplicable extends Visitable>, FromServerGettable>, RecreateApplicable, T>, - CascadingDeletable, + CascadingDeletable>, Waitable, T>, - GracePeriodConfigurable, - PropagationPolicyConfigurable { + GracePeriodConfigurable>>, + PropagationPolicyConfigurable>> { } diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/VisitFromServerGetWatchDeleteRecreateWaitApplicable.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/VisitFromServerGetWatchDeleteRecreateWaitApplicable.java index c2d656b7b6a..ced2a37f755 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/VisitFromServerGetWatchDeleteRecreateWaitApplicable.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/VisitFromServerGetWatchDeleteRecreateWaitApplicable.java @@ -25,9 +25,9 @@ public interface VisitFromServerGetWatchDeleteRecreateWaitApplicable extends Visitable>, FromServerGettable, RecreateApplicable, - CascadingDeletable, + CascadingDeletable, Watchable>, Waitable, - GracePeriodConfigurable, - PropagationPolicyConfigurable { + GracePeriodConfigurable>, + PropagationPolicyConfigurable> { } diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl.java index 03e23cb9ccd..536b4f7589f 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl.java @@ -200,12 +200,12 @@ public VisitFromServerGetWatchDeleteRecreateWaitApplicable accept(V } @Override - public CascadingDeletable withGracePeriod(long gracePeriodSeconds) { + public CascadingDeletable withGracePeriod(long gracePeriodSeconds) { return new NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl(client, config, fallbackNamespace, explicitNamespace, fromServer, true, visitors, item, gracePeriodSeconds, propagationPolicy, cascading, watchRetryInitialBackoffMillis, watchRetryBackoffMultiplier); } @Override - public CascadingDeletable withPropagationPolicy(DeletionPropagation propagationPolicy) { + public CascadingDeletable withPropagationPolicy(DeletionPropagation propagationPolicy) { return new NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl(client, config, fallbackNamespace, explicitNamespace, fromServer, true, visitors, item, gracePeriodSeconds, propagationPolicy, cascading, watchRetryInitialBackoffMillis, watchRetryBackoffMultiplier); } diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java index 1f90a43487f..e86e74f4617 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java @@ -382,12 +382,12 @@ public ListVisitFromServerGetDeleteRecreateWaitApplicable accept(Vi return new NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl(client, config, fallbackNamespace, explicitNamespace, fromServer, true, newVisitors, item, null, null, gracePeriodSeconds, propagationPolicy, cascading, watchRetryInitialBackoffMillis, watchRetryBackoffMultiplier); } - @Override public CascadingDeletable withGracePeriod(long gracePeriodSeconds) + @Override public CascadingDeletable> withGracePeriod(long gracePeriodSeconds) { return new NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl(client, config, fallbackNamespace, explicitNamespace, fromServer, true, visitors, item, null, null, gracePeriodSeconds, propagationPolicy, cascading, watchRetryInitialBackoffMillis, watchRetryBackoffMultiplier); } - @Override public CascadingDeletable withPropagationPolicy(DeletionPropagation propagationPolicy) + @Override public CascadingDeletable> withPropagationPolicy(DeletionPropagation propagationPolicy) { return new NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl(client, config, fallbackNamespace, explicitNamespace, fromServer, true, visitors, item, null, null, gracePeriodSeconds, propagationPolicy, cascading, watchRetryInitialBackoffMillis, watchRetryBackoffMultiplier); } diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelper.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelper.java index da3502579c5..b26d7463f25 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelper.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelper.java @@ -23,15 +23,25 @@ import io.fabric8.kubernetes.client.ResourceHandler; import okhttp3.OkHttpClient; +import java.net.HttpURLConnection; +import java.util.Objects; +import java.util.concurrent.TimeUnit; import java.util.function.Function; import java.util.function.UnaryOperator; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + public class DeleteAndCreateHelper { + private static final Logger LOG = LoggerFactory.getLogger(DeleteAndCreateHelper.class); + private final UnaryOperator createTask; + private final Function awaitDeleteTask; private final Function deleteTask; - public DeleteAndCreateHelper(UnaryOperator createTask, Function deleteTask) { + public DeleteAndCreateHelper(UnaryOperator createTask, Function deleteTask, Function awaitDeleteTask) { this.createTask = createTask; + this.awaitDeleteTask = awaitDeleteTask; this.deleteTask = deleteTask; } @@ -40,15 +50,40 @@ public T deleteAndCreate(T item) { if (Boolean.FALSE.equals(deleted)) { throw new KubernetesClientException("Failed to delete existing item:" + item.getMetadata().getName()); } - return createTask.apply(item); + try { + return createTask.apply(item); + } catch (KubernetesClientException e) { + // depending on the grace period, the object might not actually be deleted by the time we try to create + // if that's the case, give it some time. + if (e.getCode() == HttpURLConnection.HTTP_CONFLICT) { + if (Boolean.FALSE.equals(awaitDeleteTask.apply(item))) { + throw new KubernetesClientException("Timed out waiting for item to be deleted before recreating: " + item.getMetadata().getName(), e); + } + return createTask.apply(item); + } + throw e; + } } public static HasMetadata deleteAndCreateItem(OkHttpClient client, Config config, HasMetadata meta, ResourceHandler h, String namespaceToUse, DeletionPropagation propagationPolicy) { DeleteAndCreateHelper deleteAndCreateHelper = new DeleteAndCreateHelper<>( m -> h.create(client, config, namespaceToUse, m), - m -> h.delete(client, config, namespaceToUse, propagationPolicy, m) + m -> h.delete(client, config, namespaceToUse, propagationPolicy, m), + waitUntilDeletedOrInterrupted(client, config, h, namespaceToUse) ); return deleteAndCreateHelper.deleteAndCreate(meta); } + + private static Function waitUntilDeletedOrInterrupted(OkHttpClient client, Config config, ResourceHandler h, String namespaceToUse) { + return m -> { + try { + return h.waitUntilCondition(client, config, namespaceToUse, m, Objects::isNull, 30 , TimeUnit.SECONDS) == null; + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + LOG.warn("interrupted waiting for item to be deleted, assuming not deleted"); + return false; + } + }; + } } diff --git a/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelperTest.java b/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelperTest.java index f0fa06b33a8..8a88edadd9a 100644 --- a/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelperTest.java +++ b/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelperTest.java @@ -47,6 +47,7 @@ void testDeleteAndCreate() { .thenReturn(getPod()); DeleteAndCreateHelper podDeleteAndCreateHelper = new DeleteAndCreateHelper<>( createPodTask, + deletePodTask, p -> true ); @@ -55,7 +56,7 @@ void testDeleteAndCreate() { // Then assertNotNull(podCreated); - assertTrue(deletePodTask.apply(podCreated)); + assertTrue(wasPodDeleted.get()); } @Test @@ -67,7 +68,10 @@ void testDeleteAndCreateWhenDeletionFailed() { HttpURLConnection.HTTP_CONFLICT, new StatusBuilder().withCode(HttpURLConnection.HTTP_CONFLICT).build())); DeleteAndCreateHelper podDeleteAndCreateHelper = new DeleteAndCreateHelper<>( createPodTask, - p -> false + p -> false, + p -> { + throw new RuntimeException("should not be called because deletion failed"); + } ); // When @@ -75,6 +79,61 @@ void testDeleteAndCreateWhenDeletionFailed() { assertThrows(KubernetesClientException.class,() -> podDeleteAndCreateHelper.deleteAndCreate(podToDeleteAndCreate)); } + @Test + void testDeleteAndCreateWhenDeletionSucceedsButNotFinishedInTime() { + // Given + UnaryOperator createPodTask = Mockito.mock(UnaryOperator.class, Mockito.RETURNS_DEEP_STUBS); + when(createPodTask.apply(any())).thenThrow(new KubernetesClientException("The POST operation could not be completed at " + + "this time, please try again", + HttpURLConnection.HTTP_CONFLICT, new StatusBuilder().withCode(HttpURLConnection.HTTP_CONFLICT).build())); + DeleteAndCreateHelper podDeleteAndCreateHelper = new DeleteAndCreateHelper<>( + createPodTask, + p -> true, // deletion succeeds + p -> false // but doesn't finish in time + ); + + // When + Pod podToDeleteAndCreate = getPod(); + assertThrows(KubernetesClientException.class,() -> podDeleteAndCreateHelper.deleteAndCreate(podToDeleteAndCreate)); + } + + @Test + void testDeleteAndCreateAfterWaitingForItemToBeDeleted() { + // Given + AtomicBoolean wasPodDeleted = new AtomicBoolean(false); + Function deletePodTask = p -> { + wasPodDeleted.set(true); + return true; + }; + + AtomicBoolean awaitedDeletion = new AtomicBoolean(false); + Function awaitDeletionTask = p -> { + awaitedDeletion.set(true); + return true; + }; + + UnaryOperator createPodTask = Mockito.mock(UnaryOperator.class, Mockito.RETURNS_DEEP_STUBS); + when(createPodTask.apply(any())) + .thenThrow(new KubernetesClientException("The POST operation could not be completed at " + + "this time, please try again", + HttpURLConnection.HTTP_CONFLICT, new StatusBuilder().withCode(HttpURLConnection.HTTP_CONFLICT).build())) + .thenReturn(getPod()); + + DeleteAndCreateHelper podDeleteAndCreateHelper = new DeleteAndCreateHelper<>( + createPodTask, + deletePodTask, + awaitDeletionTask + ); + + // When + Pod podCreated = podDeleteAndCreateHelper.deleteAndCreate(getPod()); + + // Then + assertNotNull(podCreated); + assertTrue(wasPodDeleted.get()); + assertTrue(awaitedDeletion.get()); + } + private Pod getPod() { return new PodBuilder() .withNewMetadata().withName("p1").endMetadata() diff --git a/kubernetes-itests/src/test/java/io/fabric8/kubernetes/ResourceIT.java b/kubernetes-itests/src/test/java/io/fabric8/kubernetes/ResourceIT.java index ba56829c552..bc0a65523fc 100644 --- a/kubernetes-itests/src/test/java/io/fabric8/kubernetes/ResourceIT.java +++ b/kubernetes-itests/src/test/java/io/fabric8/kubernetes/ResourceIT.java @@ -30,6 +30,7 @@ import io.fabric8.kubernetes.api.model.ServiceBuilder; import io.fabric8.kubernetes.api.model.apps.Deployment; import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder; +import io.fabric8.kubernetes.api.model.apps.ReplicaSetList; import io.fabric8.kubernetes.client.KubernetesClient; import org.arquillian.cube.kubernetes.api.Session; import org.arquillian.cube.kubernetes.impl.requirement.RequiresKubernetes; @@ -49,6 +50,7 @@ import static junit.framework.TestCase.assertNotNull; import static org.awaitility.Awaitility.await; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; @RunWith(ArquillianConditionalRunner.class) @@ -159,6 +161,65 @@ public void delete() { assertTrue(client.resource(pod1).inNamespace(session.getNamespace()).delete()); } + @Test + public void testDeleteExistingWithOrphanDeletion() { + // Create Deployment + client.resource(deployment).inNamespace(session.getNamespace()).createOrReplace(); + await().atMost(30, TimeUnit.SECONDS).until(resourceIsReady(deployment)); + + // get creationTimestamp of underlying replicaset. we expect this NOT to match later, meaning the orphan WAS deleted. + ReplicaSetList replicaSetList = client.apps().replicaSets().inNamespace(session.getNamespace()).withLabel("run", "deploy1").list(); + assertEquals(1, replicaSetList.getItems().size()); + String replicaSetCreationTimestamp = replicaSetList.getItems().get(0).getMetadata().getCreationTimestamp(); + + // Recreate deployment with deleteExisting + client.resource(deployment).inNamespace(session.getNamespace()).withPropagationPolicy(DeletionPropagation.FOREGROUND).deletingExisting().createOrReplace(); + await().atMost(30, TimeUnit.SECONDS).until(resourceIsReady(deployment)); + + // check that creationTimestamp DOES NOT MATCH original, meaning the orphan WAS deleted + replicaSetList = client.apps().replicaSets().inNamespace(session.getNamespace()).withLabel("run", "deploy1").list(); + assertEquals(1, replicaSetList.getItems().size()); + assertNotEquals(replicaSetCreationTimestamp, replicaSetList.getItems().get(0).getMetadata().getCreationTimestamp()); + + // cleanup + assertEquals(true, client.resource(deployment).inNamespace(session.getNamespace()).delete()); + // Check whether child resources are also deleted + await().atMost(30, TimeUnit.SECONDS) + .until(() -> client.apps().replicaSets().inNamespace(session.getNamespace()).withLabel("run", "deploy1").list().getItems().size() == 0); + } + + @Test + public void testDeleteExistingWithoutOrphanDeletion() { + // Create Deployment + client.resource(deployment).inNamespace(session.getNamespace()).createOrReplace(); + await().atMost(30, TimeUnit.SECONDS).until(resourceIsReady(deployment)); + + // get creationTimestamp of underlying replicaset. we expect this to match later, meaning the orphan was not deleted. + ReplicaSetList replicaSetList = client.apps().replicaSets().inNamespace(session.getNamespace()).withLabel("run", "deploy1").list(); + assertEquals(1, replicaSetList.getItems().size()); + String replicaSetCreationTimestamp = replicaSetList.getItems().get(0).getMetadata().getCreationTimestamp(); + + // Recreate deployment with deleteExisting + client.resource(deployment) + .inNamespace(session.getNamespace()) + .withPropagationPolicy(DeletionPropagation.ORPHAN) + .deletingExisting() + .createOrReplace(); + + await().atMost(30, TimeUnit.SECONDS).until(resourceIsReady(deployment)); + + // check that creationTimestamp matches original, meaning the orphan was not deleted + replicaSetList = client.apps().replicaSets().inNamespace(session.getNamespace()).withLabel("run", "deploy1").list(); + assertEquals(1, replicaSetList.getItems().size()); + assertEquals(replicaSetCreationTimestamp, replicaSetList.getItems().get(0).getMetadata().getCreationTimestamp()); + + // cleanup + assertEquals(true, client.resource(deployment).inNamespace(session.getNamespace()).delete()); + // Check whether child resources are also deleted + await().atMost(30, TimeUnit.SECONDS) + .until(() -> client.apps().replicaSets().inNamespace(session.getNamespace()).withLabel("run", "deploy1").list().getItems().size() == 0); + } + @Test public void testDeletionWithOrphanDeletion() { // Create Deployment diff --git a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceTest.java b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceTest.java index e96c71d4932..b33dd177eb7 100644 --- a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceTest.java +++ b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceTest.java @@ -16,6 +16,8 @@ package io.fabric8.kubernetes.client.mock; +import io.fabric8.kubernetes.api.model.DeleteOptions; +import io.fabric8.kubernetes.api.model.DeletionPropagation; import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.Pod; import io.fabric8.kubernetes.api.model.PodBuilder; @@ -33,6 +35,7 @@ import io.fabric8.kubernetes.client.dsl.PodResource; import io.fabric8.kubernetes.client.internal.readiness.Readiness; import io.fabric8.kubernetes.client.server.mock.KubernetesServer; +import io.fabric8.kubernetes.client.utils.Serialization; import okhttp3.mockwebserver.RecordedRequest; import org.assertj.core.api.InstanceOfAssertFactories; import org.junit.Assert; @@ -118,6 +121,30 @@ void testCreateOrReplaceWithDeleteExisting() throws Exception { assertEquals("POST", request.getMethod()); } + @Test + void itPassesPropagationPolicyWithDeleteExisting() throws InterruptedException { + Pod pod1 = new PodBuilder().withNewMetadata().withName("pod1").withNamespace("test").and().build(); + + server.expect().delete().withPath("/api/v1/namespaces/ns1/pods/pod1").andReturn(HttpURLConnection.HTTP_OK, pod1).once(); + server.expect().post().withPath("/api/v1/namespaces/ns1/pods").andReturn(HttpURLConnection.HTTP_CREATED, pod1).once(); + + KubernetesClient client = server.getClient(); + HasMetadata response = client.resource(pod1).inNamespace("ns1").withPropagationPolicy(DeletionPropagation.FOREGROUND).deletingExisting().createOrReplace(); + assertEquals(pod1, response); + + assertEquals(2, server.getMockServer().getRequestCount()); + + RecordedRequest deleteRequest = server.getMockServer().takeRequest(); + assertEquals("/api/v1/namespaces/ns1/pods/pod1", deleteRequest.getPath()); + assertEquals("DELETE", deleteRequest.getMethod()); + DeleteOptions deleteOptions = Serialization.unmarshal(deleteRequest.getBody().readUtf8(), DeleteOptions.class); + assertEquals("Foreground", deleteOptions.getPropagationPolicy()); + + RecordedRequest postRequest = server.getLastRequest(); + assertEquals("/api/v1/namespaces/ns1/pods", postRequest.getPath()); + assertEquals("POST", postRequest.getMethod()); + } + @Test void testCreateOrReplaceWithDeleteExistingWithCreateFailed() { // Given From 7903ebee2d26a83bf8481fa6a5c5a5a258eb9289 Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Fri, 18 Dec 2020 08:06:36 -0500 Subject: [PATCH 2/4] trigger build From 2703800ee7b9a7b535e79fb4195d367050074d12 Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Fri, 18 Dec 2020 18:08:05 -0500 Subject: [PATCH 3/4] fix resource test. fix DeleteAndCreateHelper so we don't fail on deleted=false, and update tests --- .../client/utils/DeleteAndCreateHelper.java | 12 +++++- .../utils/DeleteAndCreateHelperTest.java | 41 +++++++++++++++++-- .../kubernetes/client/mock/ResourceTest.java | 2 +- 3 files changed, 49 insertions(+), 6 deletions(-) diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelper.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelper.java index b26d7463f25..eedd395419e 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelper.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelper.java @@ -47,20 +47,28 @@ public DeleteAndCreateHelper(UnaryOperator createTask, Function d public T deleteAndCreate(T item) { Boolean deleted = deleteTask.apply(item); - if (Boolean.FALSE.equals(deleted)) { - throw new KubernetesClientException("Failed to delete existing item:" + item.getMetadata().getName()); + if (!deleted) { + LOG.debug("did not delete because item did not exist, continuing to create {}", item.getMetadata().getName()); } + try { return createTask.apply(item); } catch (KubernetesClientException e) { // depending on the grace period, the object might not actually be deleted by the time we try to create // if that's the case, give it some time. if (e.getCode() == HttpURLConnection.HTTP_CONFLICT) { + if (!deleted) { + LOG.error("there was no item to delete, but received HTTP_CONFLICT response upon creation of item {}", item.getMetadata().getName(), e); + throw e; + } + if (Boolean.FALSE.equals(awaitDeleteTask.apply(item))) { throw new KubernetesClientException("Timed out waiting for item to be deleted before recreating: " + item.getMetadata().getName(), e); } + return createTask.apply(item); } + throw e; } } diff --git a/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelperTest.java b/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelperTest.java index 8a88edadd9a..4851c9f5d5e 100644 --- a/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelperTest.java +++ b/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelperTest.java @@ -27,11 +27,12 @@ import java.util.function.Function; import java.util.function.UnaryOperator; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; class DeleteAndCreateHelperTest { @Test @@ -62,15 +63,49 @@ void testDeleteAndCreate() { @Test void testDeleteAndCreateWhenDeletionFailed() { // Given + AtomicBoolean wasPodDeleted = new AtomicBoolean(false); + Function deletePodTask = p -> { + wasPodDeleted.set(true); + return false; + }; + UnaryOperator createPodTask = Mockito.mock(UnaryOperator.class, Mockito.RETURNS_DEEP_STUBS); + when(createPodTask.apply(any())).thenAnswer(invocation -> invocation.getArgument(0)); + + DeleteAndCreateHelper podDeleteAndCreateHelper = new DeleteAndCreateHelper<>( + createPodTask, + deletePodTask, + p -> { + throw new RuntimeException("should not be called because creation will succeed first"); + } + ); + + // When + Pod podToDeleteAndCreate = getPod(); + Pod result = podDeleteAndCreateHelper.deleteAndCreate(podToDeleteAndCreate); + + // Then + assertEquals(podToDeleteAndCreate, result); + verify(createPodTask).apply(podToDeleteAndCreate); + } + + @Test + void testThrowExceptionOnConflictAfterNoDelete() { + // Given + AtomicBoolean wasPodDeleted = new AtomicBoolean(false); + Function deletePodTask = p -> { + wasPodDeleted.set(true); + return false; + }; + UnaryOperator createPodTask = Mockito.mock(UnaryOperator.class, Mockito.RETURNS_DEEP_STUBS); when(createPodTask.apply(any())).thenThrow(new KubernetesClientException("The POST operation could not be completed at " + "this time, please try again", HttpURLConnection.HTTP_CONFLICT, new StatusBuilder().withCode(HttpURLConnection.HTTP_CONFLICT).build())); DeleteAndCreateHelper podDeleteAndCreateHelper = new DeleteAndCreateHelper<>( createPodTask, - p -> false, + deletePodTask, p -> { - throw new RuntimeException("should not be called because deletion failed"); + throw new RuntimeException("should not be called because creation will succeed first"); } ); diff --git a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceTest.java b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceTest.java index b33dd177eb7..c8649b18567 100644 --- a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceTest.java +++ b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceTest.java @@ -140,7 +140,7 @@ void itPassesPropagationPolicyWithDeleteExisting() throws InterruptedException { DeleteOptions deleteOptions = Serialization.unmarshal(deleteRequest.getBody().readUtf8(), DeleteOptions.class); assertEquals("Foreground", deleteOptions.getPropagationPolicy()); - RecordedRequest postRequest = server.getLastRequest(); + RecordedRequest postRequest = server.getMockServer().takeRequest(); assertEquals("/api/v1/namespaces/ns1/pods", postRequest.getPath()); assertEquals("POST", postRequest.getMethod()); } From 7d3a46e9f22eddf2bb39432f7f86d2f66336f3b2 Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Mon, 21 Dec 2020 07:00:36 -0500 Subject: [PATCH 4/4] Make it a constant --- .../kubernetes/client/utils/DeleteAndCreateHelper.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelper.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelper.java index eedd395419e..6b8040960aa 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelper.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/DeleteAndCreateHelper.java @@ -34,7 +34,8 @@ public class DeleteAndCreateHelper { private static final Logger LOG = LoggerFactory.getLogger(DeleteAndCreateHelper.class); - + private static final int MAX_WAIT_SECONDS = 30; + private final UnaryOperator createTask; private final Function awaitDeleteTask; private final Function deleteTask; @@ -86,7 +87,7 @@ public static HasMetadata deleteAndCreateItem(OkHttpClient client, Config config private static Function waitUntilDeletedOrInterrupted(OkHttpClient client, Config config, ResourceHandler h, String namespaceToUse) { return m -> { try { - return h.waitUntilCondition(client, config, namespaceToUse, m, Objects::isNull, 30 , TimeUnit.SECONDS) == null; + return h.waitUntilCondition(client, config, namespaceToUse, m, Objects::isNull, MAX_WAIT_SECONDS , TimeUnit.SECONDS) == null; } catch (InterruptedException e) { Thread.currentThread().interrupt(); LOG.warn("interrupted waiting for item to be deleted, assuming not deleted");