KCP triggers rollout after upgrade

[sbueringer]

As described in #8101 our e2e-main test is currently flaky. The test case which is responsible for that is the clusterctl upgrade test (v0.3 => main). The problem is that once we upgrade Cluster API to the version from main, Machines are rolled out. After a bit of investigation we found out that the KCP controller triggers the rollout.

[Spoiler alert] The test is failing roughly since 2 weeks ago when we merged #7772

Some Context:

• We introduced a new ImagePullPolicy field in KubeadmConfigSpec.InitConfiguration.NodeRegistration and KubeadmConfigSpec.JoinConfiguration.NodeRegistration
• The ImagePullPolicy field has a default value (IfNotPresent) which is applied via OpenAPI
• The defaulting happens automatically as soon as there is any kind of update on KCP and KubeadmConfig objects
• When KCP checks if it needs to rollout a machine it also compares the KubeadmConfigSpec in KCP with the KubeadmConfig of existing Machines

Example error case:

• We install Cluster API v0.3
• We create the cluster including the KCP object
• Cluster API is upgraded (to the "main" version)
• ImagePullPolicy is defaulted in the KubeadmConfig of the Machine. It is not defaulted on the KCP object.
  • Because there was an update on the Machine which triggered defaulting, but not on the KCP object.
• KCP controller reconcile detects a diff and triggers a rollout
  • Diff is: InitConfiguration.NodeRegistration.ImagePullPolicy: IfNotPresent != ""

The solution is to implement defaulting in a way that it doesn't make a difference when we calculate if we need a rollout or not:

• Move the defaulting from OpenAPI to the default func: 998f5f3
  • Context: the default func is called on both KCP and KubeadmConfig before diffing

P.S. This bug doesn't affect any released version of Cluster API as #7772 was just merged on main.

/kind bug
/area control-plane
[One or more /area label. See https://github.com/kubernetes-sigs/cluster-api/labels?q=area for the list of labels]

[sbueringer]

/cc @killianmuldoon @fabriziopandini @ykakarap @furkatgofurov7

[sbueringer]

/triage accepted

[ykakarap]

Thanks for the analysis 🙏🏼 .
This is similar to when we had unexpected rollouts in KCP after adding defaulting to format. Old issue.

We should consider adding a linter check to enforce that we don't accidentally add defaulting using OpenAPI in KCP to avoid have the same problem again in the future.

[sbueringer]

@ykakarap Great idea. I'll open a follow-up issue for the linter.

[vincepri]

If I understood this correctly, the issue happens because the diffing logic in KCP does not take into account the OpenAI defaulting logic?

[ykakarap]

If I understood this correctly, the issue happens because the diffing logic in KCP does not take into account the OpenAI defaulting logic?

I believe you mean OpenAPI 😅. Yes. KCP logic runs the defaulting webhook code but that does not take care of applying any of the OpenAPI defaulting.

[sbueringer]

Yup and then it's just racy when/if defaulting is done on the KCP and KubeadmConfig objects in the apiserver

[vincepri]

Yeah OpenAPI 😄 , I'm wondering if we can have a way to disable the use for any tree of fields that we compare in code

[sbueringer]

Hm I think on our side in CAPI we could have either a linter (based on controller-tools libraries or just parsing the final CRDs) or maybe a unit test.

I guess the most hacky / trivial thing is just to write a small go binary which parses the relevant CRDs and then fails if it finds a default value in a specific sub-tree.

On controller-tools side I could imagine some sort of marker which disallows the default marker for a struct and everything below.

[sbueringer]

/reopen

for follow-ups

[k8s-ci-robot]

@sbueringer: Reopened this issue.

In response to this:

/reopen

for follow-ups

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.

[furkatgofurov7]

@sbueringer @killianmuldoon thanks for the great effort on fixing the tests, perhaps we can close this as well (assuming follow-up #8139 is also merged)

[sbueringer]

I was referring to additional measures to ensure this doesn't happen again with follow-ups.

But I've created a separate issue for that now: #8147

Makes sense to close this issue to signal that the issue is resolved

/close

[k8s-ci-robot]

@sbueringer: Closing this issue.

In response to this:

I was referring to additional measures to ensure this doesn't happen again with follow-ups.

But I've created a separate issue for that now: #8147

Makes sense to close this issue to signal that the issue is resolved

/close

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.