Item passed in is modified by replace or patch calls

[shawkins]

The item passed in can have the resourceVersion set as a side effect of replace or patch calls. Is it expected and/or should it be part of the javadoc?

[manusa]

Current behavior is that when you replace a Resource, if the resourceVersion is provided, then we are using this as an Optimistic Lock mechanism. I'm not sure if this is documented for the replace operation, but (IMHO) this is how it should be.

For the patch operation, it makes sense to me that if we want to patch only a few fields of a given resource, we should be able to provide only those fields and a PATCH request should be sent only with those fields. I tested this in our current version with a simple operation:

client..apps()deployments().withName("whatever").patch(new DeploymentBuilder().withNewSpec().withReplicas(1).endSpec().build())

but it's not working as expected.

In first place, metadata needs to be non-null (because there is this side-effect, which you mention, trying to align the resourceVersion with the one in the server). Then, there are issues because the name needs to be provided too.

So, in scope of this issue, we should probably revisit and properly document the current PATCH implementations and see how we are aligning with the different strategies and expectations.

[shawkins]

In first place, metadata needs to be non-null (because there is this side-effect, which you mention, trying to align the resourceVersion with the one in the server). Then, there are issues because the name needs to be provided too.

Yes, I'm confused by the expectations of the PATCH call as it exists today. #3067 addresses correctly determining the patch based upon a baseline object.

#3067 does not require metadata to exist, but I don't think that helps much. What you are looking for in terms of a patch call is more of a "take this possibly incomplete object and determine what I'm trying to patch" - that seems problematic especially in terms of null fields. I think this would require using the builder directly:

client..apps()deployments().withName("whatever").patch(new DeploymentBuilder().withNewSpec().withReplicas(1).endSpec())

Then you'd need a fluent to diff framework based only upon what fields were modified. That seems like a lot of work though. It would be better to just use edit right?

client..apps()deployments().withName("whatever").edit(d->new DeploymentBuilder(d).editSpec().withReplicas(1).endSpec().build())

So, in scope of this issue, we should probably revisit and properly document the current PATCH implementations and see how we are aligning with the different strategies and expectations.

Yes, that would be good. Specific to this issue, I just want to make sure it's known when/why the passed in object is being modified.

[manusa]

The thing, is that with a Patch (depending on the strategy), we should not be diffing anything. Just send fields that should be idempotently set in the server without caring about resource version or other fields that might have changed.

We'll further analyze and document the current PATCH support to see if we're missing something and create issues accordingly.

[shawkins]

The thing, is that with a Patch (depending on the strategy), we should not be diffing anything. Just send fields that should be idempotently set in the server without caring about resource version or other fields that might have changed.

That is exactly what I'm enforcing with #3067 - with that change an edit shown above will behave exactly as you want - it will only send those modifications as part of the patch and nothing more.

The problem with the current patch implementation (which is used under an edit call) is that it will diff the edit against the latest on the server - not the object that was passed into the edit operation. That creates a small window for concurrent modifications to cause the wrong patch to be sent.

[shawkins]

We'll further analyze and document the current PATCH support to see if we're missing something and create issues accordingly.

My take is that patch(item) currently behaves more like a go client apply - but without the ability to specify the ApplyOptions.

[shawkins]

Now that the dust has settled a bit from other prs, I think it's possible to move forward here.

Specific to this issue I'll look to remove the possibility that a passed in item or the context item gets modified as a side effect of an operation - that will use the clone logic that has already been added.

Current behavior is that when you replace a Resource, if the resourceVersion is provided, then we are using this as an Optimistic Lock mechanism. I'm not sure if this is documented for the replace operation, but (IMHO) this is how it should be.

With some other recent changes, we will now first check the item for a resourceVersion. However there is no Javadoc about this or using the lockResourceVersion method. I'll add that.

For the patch operation, it makes sense to me that if we want to patch only a few fields of a given resource, we should be able to provide only those fields and a PATCH request should be sent only with those fields. I tested this in our current version with a simple operation:

That is now possible with patch(PatchContext.of(type), item) - where type can be STRATEGIC_MERGE or JSON_MERGE. Of course a limitation is that it only works for adding things. To delete things we either need a builder to track what fields have been set or to do a diff - a diff is used when specifying type JSON. It seemed simplest to leave that consideration out initially. Is this something that should be revisited?

Yes, I'm confused by the expectations of the PATCH call as it exists today.

I might be missing something, but I'd say patch(item) should be deprecated - it's unclear to me why you would use it given you could use edit or replace instead. edits just call patch(item), but it gives you a little more safety of being clear which baseline you are modifying - it can still fail due to (#3206) or blow away concurrent modifications, it's just not as likely as patch(item). If one is not worried about concurrent changes, which patch(item) doesn't handle gracefully in any case, it's just a replace that doesn't retry. @manusa @rohanKanojia WDYT?

[shawkins]

Specific to this issue I'll look to remove the possibility that a passed in item or the context item gets modified as a side effect of an operation - that will use the clone logic that has already been added.

After looking things over it seems simpler to just update the javadocs to this possibility - more than likely these modifications aren't meaningful. I should add that the concern about modification is related to #3076. Our operator was creating a new version of a CustomResource off of one from the informer cache with a builder, then doing a patch would cause the resourceVersion of the cache entry to change unexpectedly.