Skip to content

imrpove: replacing confusing UpdateControl method #1762

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

Merged
merged 7 commits into from
Feb 9, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,27 @@ public class UpdateControl<P extends HasMetadata> extends BaseControl<UpdateCont
private final P resource;
private final boolean updateStatus;
private final boolean updateResource;
private final boolean patch;
private final boolean patchStatus;

private UpdateControl(
P resource, boolean updateStatus, boolean updateResource, boolean patch) {
P resource, boolean updateStatus, boolean updateResource, boolean patchStatus) {
if ((updateResource || updateStatus) && resource == null) {
throw new IllegalArgumentException("CustomResource cannot be null in case of update");
}
this.resource = resource;
this.updateStatus = updateStatus;
this.updateResource = updateResource;
this.patch = patch;
this.patchStatus = patchStatus;
}

/**
* Creates an update control instance that instructs the framework to do an update on resource
* itself, not on the status. Note that usually as a results of a reconciliation should be a
* status update not an update to the resource itself.
*
* Using this update makes sure that the resource in the next reconciliation is the updated one -
* this is not guaranteed by default if you do an update on a resource by the Kubernetes client.
*
* @param <T> custom resource type
* @param customResource customResource to use for update
* @return initialized update control
Expand Down Expand Up @@ -73,6 +76,9 @@ public static <T extends HasMetadata> UpdateControl<T> updateStatus(T customReso
* As a results of this there will be two call to K8S API. First the custom resource will be
* updates then the status sub-resource.
*
* Using this update makes sure that the resource in the next reconciliation is the updated one -
* this is not guaranteed by default if you do an update on a resource by the Kubernetes client.
*
* @param <T> resource type
* @param customResource - custom resource to use in both API calls
* @return UpdateControl instance
Expand All @@ -82,11 +88,36 @@ public static <T extends HasMetadata> UpdateControl<T> updateResourceAndStatus(
return new UpdateControl<>(customResource, true, true, false);
}

public static <T extends HasMetadata> UpdateControl<T> patchResourceAndStatus(
/**
* Updates the resource - with optimistic locking - and patches the status without optimistic
* locking in place.
*
* Note that using this method, it is not guaranteed that the most recent updated resource will be
* in case for next reconciliation.
*
* @param customResource to update
* @return UpdateControl instance
* @param <T> resource type
*/
public static <T extends HasMetadata> UpdateControl<T> updateResourceAndPatchStatus(
T customResource) {
return new UpdateControl<>(customResource, true, true, true);
}

/**
* Marked for removal, because of confusing name. It does not patch the resource but rather
* updates it.
*
* @deprecated use {@link UpdateControl#updateResourceAndPatchStatus(HasMetadata)}
*
* @param customResource to update
* @return UpdateControl instance
* @param <T> resource type
*/
@Deprecated(forRemoval = true)
public static <T extends HasMetadata> UpdateControl<T> patchResourceAndStatus(T customResource) {
return updateResourceAndStatus(customResource);
}

public static <T extends HasMetadata> UpdateControl<T> noUpdate() {
return new UpdateControl<>(null, false, false, false);
Expand All @@ -104,8 +135,8 @@ public boolean isUpdateResource() {
return updateResource;
}

public boolean isPatch() {
return patch;
public boolean isPatchStatus() {
return patchStatus;
}

public boolean isNoUpdate() {
Expand All @@ -115,4 +146,5 @@ public boolean isNoUpdate() {
public boolean isUpdateResourceAndStatus() {
return updateResource && updateStatus;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ private PostExecutionControl<P> handleDispatch(ExecutionScope<P> executionScope)
Context<P> context =
new DefaultContext<>(executionScope.getRetryInfo(), controller, originalResource);
if (markedForDeletion) {
return handleCleanup(originalResource, resourceForExecution, context);
return handleCleanup(resourceForExecution, context);
} else {
return handleReconcile(executionScope, resourceForExecution, originalResource, context);
}
Expand Down Expand Up @@ -147,23 +147,24 @@ private PostExecutionControl<P> reconcileExecution(ExecutionScope<P> executionSc
.setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion());
updatedCustomResource =
updateStatusGenerationAware(updateControl.getResource(), originalResource,
updateControl.isPatch());
updateControl.isPatchStatus());
} else if (updateControl.isUpdateStatus()) {
updatedCustomResource =
updateStatusGenerationAware(updateControl.getResource(), originalResource,
updateControl.isPatch());
updateControl.isPatchStatus());
} else if (updateControl.isUpdateResource()) {
updatedCustomResource =
updateCustomResource(updateControl.getResource());
if (shouldUpdateObservedGenerationAutomatically(updatedCustomResource)) {
updatedCustomResource =
updateStatusGenerationAware(updateControl.getResource(), originalResource,
updateControl.isPatch());
updateControl.isPatchStatus());
}
} else if (updateControl.isNoUpdate()
&& shouldUpdateObservedGenerationAutomatically(resourceForExecution)) {
updatedCustomResource =
updateStatusGenerationAware(originalResource, originalResource, updateControl.isPatch());
updateStatusGenerationAware(originalResource, originalResource,
updateControl.isPatchStatus());
}
return createPostExecutionControl(updatedCustomResource, updateControl);
}
Expand Down Expand Up @@ -259,7 +260,7 @@ private PostExecutionControl<P> createPostExecutionControl(P updatedCustomResour
UpdateControl<P> updateControl) {
PostExecutionControl<P> postExecutionControl;
if (updatedCustomResource != null) {
if (updateControl.isUpdateStatus() && updateControl.isPatch()) {
if (updateControl.isUpdateStatus() && updateControl.isPatchStatus()) {
postExecutionControl =
PostExecutionControl.customResourceStatusPatched(updatedCustomResource);
} else {
Expand All @@ -279,7 +280,7 @@ private void updatePostExecutionControlWithReschedule(
}


private PostExecutionControl<P> handleCleanup(P originalResource, P resource,
private PostExecutionControl<P> handleCleanup(P resource,
Context<P> context) {
log.debug(
"Executing delete for resource: {} with version: {}",
Expand Down Expand Up @@ -317,8 +318,10 @@ private P updateCustomResourceWithFinalizer(P resourceForExecution, P originalRe
}

private P updateCustomResource(P resource) {
log.debug("Updating resource: {} with version: {}", getUID(resource), getVersion(resource));
log.debug("Updating resource: {} with version: {}", getUID(resource),
getVersion(resource));
log.trace("Resource before update: {}", resource);

return customResourceFacade.updateResource(resource);
}

Expand Down