Improve patchHelper to allow atomically updating a set of fields

[sbueringer]

Today patchHelper calculates patches by comparing two objects (let's call them original & desired). The patch that is send to the apiserver then only contains the delta.

This works fine as long as the original object is up-to-date. The problems start when the original object is outdated, e.g. because we read it from the controller-runtime cache and the object was stale. Please note that when patching status & spec the patch helper does not use optimistic locking.

An example:

original KCP object (stale)

status:
  replicas: 3
  readyReplicas: 3

desired KCP object

status:
  replicas: 4
  readyReplicas: 3

=> patchHelper will send a patch that sets .status.replicas = 4. readyReplicas is not included because patchHelper couldn't see a delta.

But the actual KCP object was:

status:
  replicas: 3
  readyReplicas: 2

So after patching we will end up with the following KCP object:

status:
  replicas: 4
  readyReplicas: 2

At no point did KCP calculate this combination of counter fields, but we still end up with this state in the apiserver because the patch calculation was based on a stale object. This can happen with various values for the counter fields and it gets problematic when other controllers are reading the KCP status and then making decisions based on it, e.g. they might think KCP has been already entirely upgraded if all replica fields are 3, but this might not actually be the case.

This might sound like a contrived example, but we actually had this problem and it's super hard to debug. The Cluster topology controller read KCP status, determined KCP was already entirely upgraded and then triggered worker upgrades, even though the KCP ugprade was still in progress.

One way to solve this would be to not read KCP from the cache and read it from the apiserver directly, but this is not great for scalability and puts more load on the kube-apiserver.

My proposal would be to add an option to patchHelper to ensure a certain set of fields is always part of the patch, if one of the fields changed. Based on the example above this option would be used to ensure replicas & readyReplicas are always patched together.

Somewhat related to ongoing work in #11105

[k8s-ci-robot]

This issue is currently awaiting triage.

If CAPI contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[sbueringer]

@vincepri @fabriziopandini @chrischdi Opinions?

[mboersma]

One way to solve this would be to not read KCP from the cache and read it from the apiserver directly

Even this change would just minimize the chances of getting an outdated object, but not eliminate the problem, right? I think we would need to go with an optimistic locking approach, or your latter proposal of identifying which sets of change must be atomic.

to add an option to patchHelper to ensure a certain set of fields is always part of the patch, if one of the fields changed

Would this be an additional argument or arguments the caller needs to supply when calling patchHelper? Sort of an opt-in approach. Or would patchHelper have knowledge of correlated field requirements for CAPI objects?

[vincepri]

My proposal would be to add an option to patchHelper to ensure a certain set of fields is always part of the patch, if one of the fields changed. Based on the example above this option would be used to ensure replicas & readyReplicas are always patched together.

In some ways though it feels like we might want to enable optimistic locking? That kind of guarantees what you mention above, although has other side effects.