🐛 Do not update MS status when unable to get workload cluster or machine node

[jessehu]

What this PR does / why we need it:
The ErrClusterLocked error causes the MD.Status.ReadyReplicas changes from 3 to 0 and after about 90s it will be changed back to 3. The reason is updateStatus() in machineset_controller.go ignores the error returned by getMachineNode() and treats the Node as not ready. Setting MD.Status.ReadyReplicas from 3 to 0 is unreasonable and causes issues in our project on top of CAPI.

Which issue(s) this PR fixes:
Fixes #10195

Changes
Do not update any fields of MS.Status when unable to get workload cluster or machine Node due to ErrClusterLocked or any other errors. Because the ErrClusterLocked error can be recovered soon after reconciling again, and the error that cannot get machine Node (e.g. network issue, or apiserver unavailable temporarily) should also be recovered soon after reconciling again.
Please refer to more discussion on #10229 (comment).

Test:

• The UT code for existing MS.Status update logic does not contain fake Reconciler.Tracker object and not consider the getNode logic. So it needs more changes to add UT for this error scenario.
• Ran e2e test for 3 CP & 3 Worker cluster creation with updating CAPI components version(based on CAPI 1.5.2) after the cluster created, observed the following expected behavior:
• MD.Status.ReadyReplicas won't change from 3 to 0 when getMachineNode() hit ErrClusterLocked error. The log "Requeuing because another worker has the lock on the ClusterCacheTracker" was printed.

@sbueringer @fabriziopandini please kindly take a look again. Thanks for you time!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign chrischdi for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @jessehu. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[jessehu]

/area machineset

[sbueringer]

/ok-to-test

[sbueringer]

I think this doesn't address:

For MachineSet specifically: It definitely solves the "happy path", but it makes the "unhappy path" worse by freezing the status indefinitely. Without the changes the ready and available replicas were dropping to 0 when the communication broke down. Probably a matter of preference if in case of the communication breakdown we prefer to keep ready and available replicas as is or drop them to 0. But another side effect is also that we don't update ObservedGeneration anymore, which definitely seems wrong (similar for the conditions)

Specifically:

"unhappy path" worse by freezing the status indefinitely

But another side effect is also that we don't update ObservedGeneration anymore, which definitely seems wrong (similar for the conditions)

[jessehu]

Thanks @sbueringer.

"unhappy path" worse by freezing the status indefinitely

This PR does not update any fields of MS.Status when unable to get workload cluster or machine Node due to ErrClusterLocked or any other errors. Because the ErrClusterLocked error can be recovered soon after reconciling again, and the error that cannot get machine Node (e.g. network issue, or apiserver unavailable temporarily) should also be recovered soon after reconciling again. Are there other "unhappy path" you meant?

But another side effect is also that we don't update ObservedGeneration anymore, which definitely seems wrong (similar for the conditions).

Since this PR returns error at line 891, so it won't update ms.Status at line 904 and newStatus.ObservedGeneration at line 924. This is as expected.

[sbueringer]

and the error that cannot get machine Node (e.g. network issue, or apiserver unavailable temporarily) should also be recovered soon after reconciling again. Are there other "unhappy path" you meant?

Yup, apiserver down for a longer period of time / indefinitely

[jessehu]

In case the apiserver goes down for a longer period of time or indefinitely, KCP controller should catch it and handle correctly.

[sbueringer]

KCP won't always be able to recover (if that's what you meant)

[jessehu]

Hi @fabriziopandini as discussed with @sbueringer in current thread, IMHO this PR provides a simple patch to solve the MS status update issue described in #10195, and there would be no need to introduce the suggested handling code (provided by @sbueringer and IMHO a little complex)

1. I had replied my thought in 🐛 Do not update KCP and MS status when unable to get workload cluster #10229 (comment) and also removed the KCP status handling code as you suggested.
2. In case the apiserver goes down for a longer period of time or indefinitely, the MS status won't be updated, but KCP controller should catch it and handle correctly. In this case updating MS status won't help.

BTW I was on vacation last week so sorry for the late reply. Thanks!

[fabriziopandini]

I will look into it as soon as possible, but we already invested a lot of time in trying to find an acceptable way forward.

[sbueringer]

I tried to express above that it's not acceptable that the MS status is permanently not updated anymore when a workload cluster stays unreachable

[jessehu]

As I described that
2) in case the apiserver goes down for a longer period of time or indefinitely, the MS status won't be updated, but KCP controller should catch it and handle correctly. In this case updating MS status won't help.
3) Even if we want to update MS status in this special case, it would be not good to set th MS ready replicas to 0, instead IMHO a new status UnknownReplicas should be added (but this changes the current API definition and is not a trivial change).

[sbueringer]

Sure updating the MS status won't contribute to KCP trying to remediate the problem, but it will actually keep the status up-to-date. The status is much more than just the replica fields