fix: use update instead of replace in DR #2006
Conversation
|
cc @shawkins |
I don't think the resource version can be null, since we here update a cloned |
Are saying that the user should know to leave the resourceVersion populated on the target they create from the desired method? And that if they set it as null, that it will be an unlocked update? Shouldn't SSA work the same way? My guess was that you are trying to force locking, so I'm proposing https://github.com/operator-framework/java-operator-sdk/pull/2005/files#diff-aa20588ab4b1ff4f171a897d4042d1b055c02737b067d09aaeb9cfd770adf3a0R131 |
|
In generic case, here the logic works that we clone the actual resource (from cache), replacing the spec and annotations ans labels. So the resource version is always present in the result. |
no since that is not aware of the current state. Normally there should not be optimistick locking for SSA, just do it because of event recording, althoug will check but we might able to live without that too. |
|
Which issue(s) is this supposed to address? Is this about patching vs. sending a full version of the resource we're trying to update? |
Sorry I hadn't realized there was that additional complexity. So basically every resource that lacks a spec, or ones that you want to manipulate a subresource, you have create a matcher for. And if new mutable fields are added, they must also be added to the matcher.
That's not what I was thinking, my comment was based upon the possiblity that the resourceVersion in the non-SSA case could be null. |
This is about sending the full update. (non SSA). Without this having under optimistick locking, this part becomes very fuzzy: Since it could happen that there was an other update happening, from other party, and we would simply override it. Also makes the eventing easier to reason about, but that might be not necessariy. Will create a separate issue to discuss that situation. |
| @@ -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(); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This does not makes up to me, we such changes are done for the resources:
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.
There was a problem hiding this comment.
Yes, it's not clear why these resources have a special treatment to me either…
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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…
There was a problem hiding this comment.
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.
No description provided.