-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Changes from 4 commits
173ab63
c7ae84f
7903ebe
2703800
7d3a46e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
// 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this comment still accurate? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
}; | ||
} | ||
} |
There was a problem hiding this comment.
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?