Consider critical-components-not-ready taint when machine joins first time

[SimonKienzler]

What this PR does / why we need it:

A node taint (node.gardener.cloud/critical-components-not-ready) is introduced in gardener/gardener#7406 as part of gardener/gardener#7117. This taint needs to be considered by the MCM when it checks the machine health, as a machine is only supposed to be Ready if the introduced taint is not present.

Co-Authored-By: @timebertt

Which issue(s) this PR fixes:
Part of gardener/gardener#7117

Special notes for your reviewer:

The change has been tested locally in combination with with machine-controller-manager-provider-local in order to verify correct behaviour.

Release note:

Machine object won't turn from `Pending`  to `Running` state if `node.gardener.cloud/critical-components-not-ready` taint is there on the corresponding node.

[gardener-robot]

@SimonKienzler Thank you for your contribution.

[gardener-robot-ci-3]

Thank you @SimonKienzler for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

[himanshu-kun]

From the gardener issue I understand that this taint is relevant only during the first time the machine becomes Running. Current changes in the PR also lets the taint affect the health mechanism of MCM. Health mechanism is only relevant after the machines have become Running once. So I have suggested changes , not to affect that.
Please let me know if you think otherwise.

[SimonKienzler]

@himanshu-kun thanks for your feedback, I gave it a go - please let me know what you think.

[himanshu-kun]

could you also add 3 test cases in a different Context block under ##General Machine Health Reconciliation in pkg/util/provider/machinecontroller/machine_util_test.go

• "Pending machine which is healthy but node without taint shouldn't be marked Running"
• "Pending machine which is healthy and node has taint should be marked Running"
• "Unknown machine which is healthy , and node with the taint should be marked Running"

[SimonKienzler]

@himanshu-kun I added the test cases. ##General Machine Health Reconciliation is a DescribeTable test, I'm not aware of a way to subdivide the entries into different Contexts. Hope it's alright like this.

[himanshu-kun]

Asked for one test case. otherwise looks good.

[himanshu-kun]

@SimonKienzler could you allow me editing rights to this PR. I have some changes which I'd like to make

[SimonKienzler]

@himanshu-kun Unfortunately, I cannot (see https://github.com/orgs/community/discussions/5634). Can we make it work in another way?

[timebertt]

/assign
I will take over this PR while @SimonKienzler is unavailable.

[timebertt]

@himanshu-kun I cherry-picked your commits. I think, this PR should be ready to merge :)

[himanshu-kun]

We discussed internally and have a common concern that should the node.gardener.cloud/critical-components-not-ready be deciding the Phase of a machine at all?
Running machine itself means that the node is healthy with all the relevant node conditions true, kubelet runnning etc. Taint is just a constraint from the kubernetes side for scheduler. From what I can think of , keeping the machine Pending because of the taint, has only one benefit that the EveryNodeReady condition of shoot would not turn green until critical shoot system components are ready. But then we also have SystemComponentsRunning condition for all system components.
@timebertt do you see any other benefits which I might have missed?

[timebertt]

I would say this is necessary to prevent rolling updates from tumbling. Otherwise, mcm would continue the rolling update even if the customer workload cannot be scheduled on the node yet because it is not fully ready.

From gardener/gardener#7117:

make machine-controller-manager aware of the new checks and only continue rolling updates once all node-critical pods got ready in addition to waiting for the Ready condition (already done today)

[timebertt]

@himanshu-kun please tell me if there is anything missing to merge this PR :)

[himanshu-kun]

No there isn't @timebertt . thanks for the explanation.

[himanshu-kun]

/lgtm