🐛 Fix cluster reconcilation predicates

[Unix4ever]

The current implementation for ClusterUpdateUnpaused is filtering all
cluster updates except for the case when spec.paused is updated from
true to false.

As all Cluster updates do not trigger Machines reconcilation,
setting ControlPlaneInitialized to True does not start workload nodes
watch in MachinesController.

That leads to cluster deployment being stuck and hanging for 15 minutes.

Signed-off-by: Artem Chernyshev [email protected]

[k8s-ci-robot]

Hi @Unix4ever. 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.

[ykakarap]

/ok-to-test

[ykakarap]

As all Cluster updates do not trigger Machines reconcilation,
setting ControlPlaneInitialized to True does not start workload nodes
watch in MachinesController

ControlPlaneInitialized is no longer part of the Cluster type in the latest type (v1beta1).
Looks like ControlPlaneInitialized is part of the v1alpha3 Cluster type which is pretty old.

I am not sure if this fix to main (currently on the v1beta1) types would address your problem.

[ykakarap]

As all Cluster updates do not trigger Machines reconcilation,
setting ControlPlaneInitialized to True does not start workload nodes
watch in MachinesController

ControlPlaneInitialized is no longer part of the Cluster type in the latest type (v1beta1).
Looks like ControlPlaneInitialized is part of the v1alpha3 Cluster type which is pretty old.

I am not sure if this fix to main (currently on the v1beta1) types would address your problem.

[Unix4ever]

I was testing that on v1beta1 and it actually fixes my problem.
And I was talking about the condition: https://github.com/kubernetes-sigs/cluster-api/blob/main/api/v1beta1/condition_consts.go#L65 , not about the field in status.
I don't see it being deprecated or something.

And I still see it being set after controlplane resource initialized flag changes to true.

[ykakarap]

I was testing that on v1beta1 and it actually fixes my problem. And I was talking about the condition: https://github.com/kubernetes-sigs/cluster-api/blob/main/api/v1beta1/condition_consts.go#L65 , not about the field in status. I don't see it being deprecated or something.

And I still see it being set after controlplane resource initialized flag changes to true.

You are right. My bad, I missed the condition. It is clear that the current behavior does not reconcile machines if changes are made to the cluster if it is already unpaused. I am LGTM for the changes however I want to get @fabriziopandini's opinion here on two things:

1. Was there any historical reason on limiting the predicate to only cluster transitions from paused to unpaused instead of being a generic "check if cluster is unpaused". Asking because the comments on the function are pretty clear in that they only want to limit to transition.

2. This change will change the behavior of the function as it currently is. Since this is an exposed function and part of the code API is it okay to change this behavior or should we ideally create a new predicate that checks if the Cluster is unpaused and use that instead?

[sbueringer]

I think we should not change ClusterUpdateUnpaused as the intention seems pretty clear that it should only capture update events which explicitly change the pause field from true to false. One level above in ClusterUnpaused the intention seems to be similar.

What I'm absolutely not sure about is what the intention in the controllers is.

So concrete, in the MachineController do we always want to reconcile on Cluster events if the cluster is not paused or do we explicitly only want to reconcile if an unpause occurred.

If we always want to reconcile on update events of unpaused cluster we should introduce a new predicate (e.g. ClusterNotPaused and use that accordingly.

At a first glance it seems reasonable to do that, but I'm not aware of the history and as this triggers a lot of additional reconciles we should be really sure.

@vincepri @fabriziopandini WDYT?

[Unix4ever]

Ok, I've introduced a new predicate and injected it using or in the predicates list of the machine_controller.go.

So concrete, in the MachineController do we always want to reconcile on Cluster events if the cluster is not paused or do we > explicitly only want to reconcile if an unpause occurred.

Pretty long time ago I had another attempt to fix that issue in the machine controller and back then I've got the following answer:
#5884 (comment)

Judging from the answer I think the intention was to react on any cluster updates.

[vincepri]

Hey folks just reading through this thread and was hoping to provide a bit more clarity on why the predicates were built this way.

The Machine controller watches all Cluster objects, when a Cluster has an event (regardless of filter) is fed through the watch informers to controller runtime. A predicate can be used in this scenario to filter out some events and prevent reconciliation.

In the beginning of the project, the Machine controller was watching every event on the Cluster object, regardless on the type; this caused lots of frequent reconciliations which ultimately caused the system to reconcile Machine too often.

The current predicate ClusterUpdateUnpaused is there because we always want to make sure to reconcile each Machine right after the Cluster object spec.paused field was changed from true to false. This is a common scenario that happens during clusterctl move operations (or backup/restore), it can also be used when performing manual maintenance on a cluster.

Going back to the original problem reported, it sounds like that ControlPlaneInitialized on the Cluster object isn't triggering reconciliation of Machines which causes a delay. Usually though, ControlPlaneInitialized when using machine-based Clusters should receive events when the Machine itself gets created and/or joins the cluster and status.nodeRef is populated. Is this event not coming through?

@Unix4ever Can you also clarify what actually gets stuck for 15 minutes?

[smira]

Going back to the original problem reported, it sounds like that ControlPlaneInitialized on the Cluster object isn't triggering reconciliation of Machines which causes a delay. Usually though, ControlPlaneInitialized when using machine-based Clusters should receive events when the Machine itself gets created and/or joins the cluster and status.nodeRef is populated. Is this event not coming through?

@vincepri I believe the actual problem is that MachineController doesn't start watching workload cluster Node resources until the control plane is initialized. So there will be reconcile triggered when the provider ID is set, and Machine never becomes ready. This is a problem only on the initial cluster creation, as once MachineController is triggered at least once, it starts watching workload cluster Nodes, and things are back to normal. But with the current predicate, if MachineController reconciles a Machine before control plane is initialized, it will be stuck "forever" as no event will trigger a reconcilation as Nodes are not watched. So probably predicate could be updated to reconcile only on changes to the Cluster control plane initizlied field to keep the number of reconciles low.

[vincepri]

That makes sense, thanks for digging into it more @smira! @Unix4ever Do you have some time to try to add another predicate that allows the Machine controller to reconcile when ControlPlaneInitialized is set to true?

[Unix4ever]

That makes sense, thanks for digging into it more @smira! @Unix4ever Do you have some time to try to add another predicate that allows the Machine controller to reconcile when ControlPlaneInitialized is set to true?

Yeah, updated and tested that it still works.

[ykakarap]

/lgtm

[smira]

if/when this gets merged, can we please backport it to 1.1.x branch?

[vincepri]

/approve
/cherry-pick release-1.1

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vincepri]

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

[vincepri]

/cherry-pick release-1.1

[k8s-infra-cherrypick-robot]

@vincepri: new pull request created: #6488

In response to this:

/cherry-pick release-1.1

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.