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.

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

Allow specifying PropagationPolicy when using deleteExisting #2676

Merged
merged 5 commits into from
Dec 21, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* Fix #2656: Binding operations can be instantiated

#### Improvements
* Fix: Allow specifying PropagationPolicy when using deleteExisting
* Fix: Adds a convenience method for referring to Cache keys by namespace and name rather than item
* Fix: CustomResourceDefinitionContext.fromCrd support for v1 CustomResourceDefinition
* Fix #2642: Update kubernetes-examples to use apps/v1 Deployment rather than extensions/v1beta1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,5 @@
/**
* Created by iocanel on 9/15/16.
*/
public interface CascadingDeletable extends Deletable, Cascading<Deletable> {
public interface CascadingDeletable<T> extends Deletable, Cascading<Deletable>, Recreateable<Applicable<T>> {
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
public interface ListVisitFromServerGetDeleteRecreateWaitApplicable<T> extends Visitable<ListVisitFromServerGetDeleteRecreateWaitApplicable<T>>,
FromServerGettable<List<T>>,
RecreateApplicable<List<T>, T>,
CascadingDeletable,
CascadingDeletable<List<T>>,
Waitable<List<T>, T>,
GracePeriodConfigurable<CascadingDeletable>,
PropagationPolicyConfigurable<CascadingDeletable> {
GracePeriodConfigurable<CascadingDeletable<List<T>>>,
PropagationPolicyConfigurable<CascadingDeletable<List<T>>> {
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@

public interface VisitFromServerGetWatchDeleteRecreateWaitApplicable<T> extends Visitable<VisitFromServerGetWatchDeleteRecreateWaitApplicable<T>>,
FromServerGettable<T>, RecreateApplicable<T, T>,
CascadingDeletable,
CascadingDeletable<T>,
Watchable<Watcher<T>>,
Waitable<T, T>,
GracePeriodConfigurable<CascadingDeletable>,
PropagationPolicyConfigurable<CascadingDeletable> {
GracePeriodConfigurable<CascadingDeletable<T>>,
PropagationPolicyConfigurable<CascadingDeletable<T>> {
}
Original file line number Diff line number Diff line change
Expand Up @@ -200,12 +200,12 @@ public VisitFromServerGetWatchDeleteRecreateWaitApplicable<HasMetadata> accept(V
}

@Override
public CascadingDeletable withGracePeriod(long gracePeriodSeconds) {
public CascadingDeletable<HasMetadata> 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<HasMetadata> withPropagationPolicy(DeletionPropagation propagationPolicy) {
return new NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl(client, config, fallbackNamespace, explicitNamespace, fromServer, true, visitors, item, gracePeriodSeconds, propagationPolicy, cascading, watchRetryInitialBackoffMillis, watchRetryBackoffMultiplier);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -382,12 +382,12 @@ public ListVisitFromServerGetDeleteRecreateWaitApplicable<HasMetadata> 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<List<HasMetadata>> 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<List<HasMetadata>> withPropagationPolicy(DeletionPropagation propagationPolicy)
{
return new NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl(client, config, fallbackNamespace, explicitNamespace, fromServer, true, visitors, item, null, null, gracePeriodSeconds, propagationPolicy, cascading, watchRetryInitialBackoffMillis, watchRetryBackoffMultiplier);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,32 +23,75 @@
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<T extends HasMetadata> {
private static final Logger LOG = LoggerFactory.getLogger(DeleteAndCreateHelper.class);

private final UnaryOperator<T> createTask;
private final Function<T, Boolean> awaitDeleteTask;
private final Function<T, Boolean> deleteTask;

public DeleteAndCreateHelper(UnaryOperator<T> createTask, Function<T, Boolean> deleteTask) {
public DeleteAndCreateHelper(UnaryOperator<T> createTask, Function<T, Boolean> deleteTask, Function<T, Boolean> awaitDeleteTask) {
this.createTask = createTask;
this.awaitDeleteTask = awaitDeleteTask;
this.deleteTask = deleteTask;
}

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) {
Copy link
Contributor Author

@bbeaudreault bbeaudreault Dec 17, 2020

Choose a reason for hiding this comment

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

I don't think this is related to my original change to support PropagationPolicy, but my new integration test caught this issue. The default grace period is 30s, so unless you explicitly specify 0 seconds (which we don't here and probably shouldn't if we could) it's possible that the DELETE call returns a 202 instead of 200, meaning it hasn't actually been deleted yet.

BaseOperation/OperationSupport/etc assume that a non-error response means the item was deleted. This is sort of true, but not really since it may have actually been queued for asynchronous deletion (per the 202 response). I looked into whether I could pipe the response Status through as the return value of the various delete() functions, but that proved to be a significant change (and probably a breaking one, since right now we expect to return a simple Boolean).

The simpler fix here seems to be to add a waiting period as I did here. Alternatively we could continually retry createTask for a period of time, but this might be less impactful on API servers?

// 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.
Comment on lines +58 to +59
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this comment still accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, still accurate. All of the stuff I mentioned about DELETE returning a 200 or 202 still applies. So we still need to wait for the deletion to occur before creating, otherwise we'll get a conflict.

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;
}
return createTask.apply(item);
}

public static HasMetadata deleteAndCreateItem(OkHttpClient client, Config config, HasMetadata meta, ResourceHandler<HasMetadata, HasMetadataVisitiableBuilder> h, String namespaceToUse, DeletionPropagation propagationPolicy) {
DeleteAndCreateHelper<HasMetadata> 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 <T extends HasMetadata> Function<T, Boolean> waitUntilDeletedOrInterrupted(OkHttpClient client, Config config, ResourceHandler<HasMetadata, HasMetadataVisitiableBuilder> h, String namespaceToUse) {
return m -> {
try {
return h.waitUntilCondition(client, config, namespaceToUse, m, Objects::isNull, 30 , TimeUnit.SECONDS) == null;
bbeaudreault marked this conversation as resolved.
Show resolved Hide resolved
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
LOG.warn("interrupted waiting for item to be deleted, assuming not deleted");
return false;
}
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -47,6 +48,7 @@ void testDeleteAndCreate() {
.thenReturn(getPod());
DeleteAndCreateHelper<Pod> podDeleteAndCreateHelper = new DeleteAndCreateHelper<>(
createPodTask,
deletePodTask,
p -> true
);

Expand All @@ -55,26 +57,118 @@ void testDeleteAndCreate() {

// Then
assertNotNull(podCreated);
assertTrue(deletePodTask.apply(podCreated));
assertTrue(wasPodDeleted.get());
}

@Test
void testDeleteAndCreateWhenDeletionFailed() {
// Given
AtomicBoolean wasPodDeleted = new AtomicBoolean(false);
Function<Pod, Boolean> deletePodTask = p -> {
wasPodDeleted.set(true);
return false;
};
UnaryOperator<Pod> createPodTask = Mockito.mock(UnaryOperator.class, Mockito.RETURNS_DEEP_STUBS);
when(createPodTask.apply(any())).thenAnswer(invocation -> invocation.getArgument(0));

DeleteAndCreateHelper<Pod> 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<Pod, Boolean> deletePodTask = p -> {
wasPodDeleted.set(true);
return false;
};

UnaryOperator<Pod> 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<Pod> podDeleteAndCreateHelper = new DeleteAndCreateHelper<>(
createPodTask,
deletePodTask,
p -> {
throw new RuntimeException("should not be called because creation will succeed first");
}
);

// When
Pod podToDeleteAndCreate = getPod();
assertThrows(KubernetesClientException.class,() -> podDeleteAndCreateHelper.deleteAndCreate(podToDeleteAndCreate));
}

@Test
void testDeleteAndCreateWhenDeletionSucceedsButNotFinishedInTime() {
// Given
UnaryOperator<Pod> 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<Pod> podDeleteAndCreateHelper = new DeleteAndCreateHelper<>(
createPodTask,
p -> false
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<Pod, Boolean> deletePodTask = p -> {
wasPodDeleted.set(true);
return true;
};

AtomicBoolean awaitedDeletion = new AtomicBoolean(false);
Function<Pod, Boolean> awaitDeletionTask = p -> {
awaitedDeletion.set(true);
return true;
};

UnaryOperator<Pod> 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<Pod> 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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down
Loading