Skip to content

fix: use update instead of replace in DR #2006

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

Merged
merged 2 commits into from
Aug 14, 2023
Merged

fix: use update instead of replace in DR #2006

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b99a8b7
fix: use update instead of replace in DR
csviri Aug 8, 2023
6aa8b3b
Merge branch 'main' into dr-fix-update-no-replace
csviri Aug 14, 2023
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
2 changes: 1 addition & 1 deletion ...javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ public R update(R actual, R target, P primary, Context<P> context) {
.forceConflicts().serverSideApply();
} else {
var updatedActual = updaterMatcher.updateResource(actual, target, context);
updatedResource = prepare(updatedActual, primary, "Updating").replace();
updatedResource = prepare(updatedActual, primary, "Updating").update();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may need to be an explicitly locked replace, or a patch. One subtle difference between update and replace is that replace does some modifications to the resource (Services, Jobs, and OpenShift RoleBindings) based upon the present state - see HasMetadataOperation.modifyItemForReplaceOrPatch. The intention is to remove that once replace is gone.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This does not makes up to me, we such changes are done for the resources:

https://github.com/fabric8io/kubernetes-client/blob/0c7d5150702387c1aeca66facb98508d590934f2/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/batch/v1/JobOperationsImpl.java#L163-L175

Shoudn't be this the responsibility of the user to fill those values?

I don't see why should be this patch or replace instead of update because of this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it's not clear why these resources have a special treatment to me either…

Copy link
Collaborator

@shawkins shawkins Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The root issue is that PUT has side effects. Service is the poster child for this - if you attempt a PUT and the clusterIP is not populated, then it will be allocated, which will then conflict with the existing one. If you use an empty string it will complain that the field is immutable - people have complained about this for years kubernetes/kubernetes#91459 So I guess that in the past they wanted to smooth this behavior out in the fabric8 client.

In the last couple of years when users complain of new situations like this that don't work with createOrReplace we have been telling them to manually do something like the proposed createOr, or more recently to use serverSideApply.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, things can get messy fast when you get into discussion of HTTP verbs semantics :)
I do agree with one of the commenters that PUT should be idempotent so regardless of what controllers do, if they accepted one resource as valid at one point in time, they should accept that same resource again if re-PUT (and possibly return the existing one), which doesn't appear to be the case here…

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The root issue is that PUT has side effects. Service is the poster child for this - if you attempt a PUT and the clusterIP is not populated, then it will be allocated, which will then conflict with the existing one. If you use an empty string it will complain that the field is immutable - people have complained about this for years kubernetes/kubernetes#91459 So I guess that in the past they wanted to smooth this behavior out in the fabric8 client.
In the last couple of years when users complain of new situations like this that don't work with createOrReplace we have been telling them to manually do something like the proposed createOr, or more recently to use serverSideApply.

Yeah, I think this workaround for example can nice used:
First load the existing service that contains the current clusterIP. Set the old clusterIp to the updated V1Service.

For these special cases would rather prepare some default implementations in dependent resources, rather than solving it on client level here. So would anyways stick with the update.

}
log.debug("Resource version after update: {}",
updatedResource.getMetadata().getResourceVersion());
Expand Down