PUT instead of PATCH used for UpdateControl.patchResourceAndStatus(resource)

[codepitbull]

Bug Report

I wanted to patch both status and resource returning UpdateControl.patchResourceAndStatus(resource) from the reconcile.
My tests became flaky with an occasional the object has been modified; please apply your changes to the latest version and try again..

What did you do?

Use UpdateControl.patchResourceAndStatus(resource) to get both, status and resource patched.

What did you expect to see?

Patch should be sued for status und resource

What did you see instead? Under which circumstances?

This is either a bug or the method-name implies a wrong operation being used.

In ReconciliationDeispatcher.reconcileExecution updateResource is used, which ignores the provided patch-flag.

So only the status gets patched, not the resource.

Environment

All k8s-versions.

Possible Solution

If the intention of the method is to only patch the status and to use put for the resource it should be reflected in the method name.
If not then I guess I found a bug ;)

[csviri]

Hi @codepitbull , yep that is probably wrong implementation or wrong naming.

However just few remarks, and clarifications:

1. in general controller should not change the spec of it's own CR, I assume you are rather adding some label or changing the metadata (what is fine). Could you pls describe your use case?
2. For some reason (exceptional cases) still want to update the spec, update with optimistic locking is still the preferred way.

So patching (without optimistic locking) is preferred really when there needs to be a metadata change, and for that use case this should be fixed. If remember correctly the original thinking was that update is more general since it covers (2.) but even if it fails, it will be retried and eventually work.

But again I agree that naming is misleading, I guess best way is to fix this behavior for next minor release, and also a and updateResourcePatchStatus method to migrate to, if somebody intentionally used the way it is now.

[codepitbull]

In my case I am applying an annotation from inside the reconcile-loop.
This is done for the following reason:

• Our custom resource configures a service which in turn connects to a remote service
• We have seen multiple situations where the remote service is wrongly configured, something we can't fix.
• If we discover such a case the CR is annotated which causes it to scale down its deplyoments

The annotation is used because SRE should be able to reenable everything without going into the CR itself but rely on a well known annotation.
The other way round allows SRE to deactivate a problematic CR without deleting anything or having to get into the resources, allowing us to debug the last known state a bit more easy.

So, no spec-change involved, just metadata :)

[csviri]

without going into the CR itself

you mean changing the spec?

Just one other aspect I forgot to mention (will update the docs regarding to this). There is another reason while the default behavior is update with optimistic locking. In this case the framework can guarantee that the update will be present in the next reconciliation. If it's without optimistic locking it's not. So it can happen that you won't see the update in the next reconiliation in the metadata. Therefore we made this decision. Since usually even if its in metadata you implement logic based on this - if understand correctly, in this case the scale down logic.

[csviri]

So if your reconciliation logic depends on those labels, you are probably doing it right now. The method name is still misleading though.

[csviri]

@codepitbull preparing an update for this. But we can also take a look closer to your use case more in detail, even in a call if you like.

[csviri]

For now created a PR, that makes the API more clear.
Also added am issue to discuss if we should add patch methods for the resource (not status): #1763
For now pls use the Kubernetes client directly for that.
thx for the issue!