crossplane
pkg/resource: consistent applicators
Description of your changes
This PR does two things:
- the update applicator is deprecated. The override of the resource version is removed, i.e. the update applicator will correctly conflict with any other actor writing to the object when the base version of the reconcile (from the informer cache) is outdated. With that the updates follow the optimistic concurrency protocol of the API: get, modify object, update, retry on conflict.
- the patching applicator is changed to compute patches between the base version of the reconcile (from the informer cache) and the object to be applied. Because the applicator has no access to the former, it does another call against the cached client to get the current version. If that matches the resource version of the object to be applied, we are fine and can apply the computed patch to the apiserver. If the resource version differs, this is equivalent to a conflict error because the world has moved on during reconcile. That's returned as a conflict error to the reconcile loop.
Fixes #483:
- [x] Read and followed Crossplane's contribution process.
- [x] Run
make reviewable testto ensure this PR is ready for review.
How has this code been tested
- [x] proof PR for crossplane created: https://github.com/crossplane/crossplane/pull/4562
@sttts Could you update the PR description with a little more detail, please. Between your initial issue description and my lengthy comment we touch on quite a few things, and it's not obvious to me what parts of https://github.com/crossplane/crossplane-runtime/issues/483 this is intending to solve and how.
This needs a proof PR against Crossplane. I have the suspicion that it breaks the composite reconciler as it relies on the inconsistent, additive behaviour.
Perhaps we can add a few e2e tests to validate that? (in crossplane PR ofcourse)
This needs a proof PR against Crossplane
Agreed. I haven't had time to review closely. I like the direction but suspect it could be breaking in a few places.
This PR does two things
@sttts +1 for deprecating the APIUpdatingApplicator. That makes sense to me.
With regard to the APIPatchingApplicator changes, my understanding (as written up in https://github.com/crossplane/crossplane-runtime/issues/483#issuecomment-1636432560) is that this applicator does not have an optimistic concurrency issue at the moment. Is that understanding wrong?
Is the fact that the APIPatchingApplicator now computes patches between the base version of the resource and the object to be applied addressing an optimistic concurrency issue? Or is it instead intended to allow us to support deleting object fields, since it now creates a proper merge patch?
applicator does not have an optimistic concurrency issue at the moment. Is that understanding wrong?
It has a consistency issue though :) It does not apply what you tell it to apply. It additively merges your changes into the version on the server.
Or is it instead intended to allow us to support deleting object fields, since it now creates a proper merge patch?
Yes, it is.
But this of course means that it wipes everything not in the passed object. E.g. if you override the spec in the composition controller code to be equal to that in the Composition, it will wipe everything that a provider adds to the spec. To work around that we have to switch the compisition controller to server side apply. But that means that we will get rid of the patching applicator in that controller and talk directly via SSA to the server.
Or is it instead intended to allow us to support deleting object fields, since it now creates a proper merge patch?
Yes, it is.
But this of course means that it wipes everything not in the passed object. E.g. if you override the spec in the composition controller code to be equal to that in the
Composition, it will wipe everything that a provider adds to the spec. To work around that we have to switch the compisition controller to server side apply. But that means that we will get rid of the patching applicator in that controller and talk directly via SSA to the server.
The logic in the composite controller (also in the claim controller) heavily relies on additive behavior currently. Hence, it wouldn't be possible to switch to the patching applicator introduced in this PR without a good amount of refactoring/testing, which could be a similar amount of effort as switching to Server Side Apply. IIUC, this will work quite similar to the extract flow mentioned here. So, I am wondering if it is worth the effort to switch to this or if should we simply invest in Server Side Apply instead 🤔
The logic in the composite controller (also in the claim controller) heavily relies on additive behavior currently. Hence, it wouldn't be possible to switch to the patching applicator introduced in this PR without a good amount of refactoring/testing, which could be a similar amount of effort as switching to Server Side Apply. IIUC, this will work quite similar to the extract flow mentioned here. So, I am wondering if it is worth the effort to switch to this or if should we simply invest in Server Side Apply instead 🤔
Hence, AdditiveMergePatchApplyOption to make this old behaviour explicit. We can fix the compositor with SSA in a follow-up.
The logic in the composite controller (also in the claim controller) heavily relies on additive behavior currently. Hence, it wouldn't be possible to switch to the patching applicator introduced in this PR without a good amount of refactoring/testing, which could be a similar amount of effort as switching to Server Side Apply.
Independently from the strategy, we need to create crossplane e2e tests, covering at least major usecases advocated to users - only then we can switch with confidence to an improved applicator/server-side apply.
