⚠️ Restructure MHC fields in MHC, Cluster and ClusterClass CRDs #12504

sbueringer · 2025-07-18T10:17:06Z

Signed-off-by: Stefan Büringer buringerst@vmware.com

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #12497

sbueringer · 2025-07-18T10:24:10Z

api/core/v1beta2/machinehealthcheck_types.go

+
+// MachineHealthCheckRemediationTriggerIf configures if remediations are triggered.
+// +kubebuilder:validation:MinProperties=1
+type MachineHealthCheckRemediationTriggerIf struct {


MHC now looks like this

spec: checks: nodeStartupTimeoutSeconds: 0 unhealthyNodeConditions: [] remediation: templateRef: {} triggerIf: unhealthyLessThanOrEqualTo: 100% unhealthyInRange: "[1-4]"

Open point from Joel:

remediation.triggerIf, not sure if other names were considered, but could also be triggerWhen, or triggerIfNodes/triggerWhenNodes - I got thinking about whether nodes should be in the context, since it's not in the child fields?

I would prefer triggerIf, we have things like enabledIf in our APIs already.

Regarding adding Node, I think if we add it we should consider adding it to the child fields, like we already have below checks. On the other side here "unhealthy" applies more to Machines or actually Machines and Nodes? So probably keeping the current names would be better

cc @fabriziopandini @JoelSpeed Let's finalize this :)

I'm leaning toward triggerWhen, it seems more precise in this case

First google result, not sure if true

We use "when" when we know something is going to happen, there's 100% certainty here. This is a key point, it's going to happen.

"If," on the other hand, we use "if" if there's only a chance something is going to happen. There's only a chance, it is not determined, it is not something that has been decided.

Another one

If is used to discuss conditional or uncertain events, indicating the possibility that an event may or may not happen. On the other hand, when is used when you're certain that an event will happen, and it pertains to timing rather than conditionality.

On the other side here "unhealthy" applies more to Machines or actually Machines and Nodes? So probably keeping the current names would be better

I agree with this.

"If," on the other hand, we use "if" if there's only a chance something is going to happen. There's only a chance, it is not determined, it is not something that has been decided.

I see it like this.

I'm happy to leave as is, it seems that's preferred by Stefan and Alberto at least, and the enabledIf consistency is positive.

sbueringer · 2025-07-18T10:26:18Z

/test pull-cluster-api-e2e-main-gke

sbueringer · 2025-07-18T10:26:47Z

/hold
Will push another commit for docs

sbueringer · 2025-07-18T10:35:48Z

/assign @JoelSpeed @fabriziopandini

api/core/v1beta2/cluster_types.go

fabriziopandini · 2025-07-18T14:57:37Z

api/core/v1beta2/machinehealthcheck_types.go

+
+// MachineHealthCheckRemediationTriggerIf configures if remediations are triggered.
+// +kubebuilder:validation:MinProperties=1
+type MachineHealthCheckRemediationTriggerIf struct {


I'm leaning toward triggerWhen, it seems more precise in this case

api/core/v1beta2/cluster_types.go

sbueringer · 2025-07-18T15:31:40Z

/test pull-cluster-api-e2e-main-gke

exp/topology/scope/blueprint.go

Signed-off-by: Stefan Büringer buringerst@vmware.com

api/core/v1beta2/machinehealthcheck_types.go

enxebre · 2025-07-21T10:53:37Z

api/core/v1beta2/machinehealthcheck_types.go

+	// triggerIf configures if remediations are triggered.
+	// If this field is not set, remediations are always triggered.
+	// +optional
+	TriggerIf MachineHealthCheckRemediationTriggerIf `json:"triggerIf,omitempty,omitzero"`


as an optional struct, shouldn't this be a pointer?

same question for Remediation, checks... or did we agree on something else?

Why should an optional struct be a pointer?

From an API best practices perspective pointer + omitempty or non-pointer + omitempty,omitzero works

(we did this for many fields in the last few weeks, see e.g.: #12482, #12489)

(cc @JoelSpeed)

From an API best practices perspective pointer + omitempty or non-pointer + omitempty,omitzero works

This is quite a new thing, with Go 1.24 we can now use omitzero. So, as long as there is no valid use case for a {} meaning something different than omitted, the correct thing now is to be not a pointer, and have omitzero. This should make coding simpler for folks over time as we will have far fewer dereference points for structs

ack. Does this then set a hard dep on clients needing to be on go 1.24?

Yes. But we already have that because we rely on controller-runtime (>=v0.21) and k8.io (>=v0.33) versions that require Go 1.24

sbueringer · 2025-07-21T11:57:57Z

@fabriziopandini @JoelSpeed @enxebre PTAL, thx! :)

sbueringer · 2025-07-21T11:58:10Z

/test pull-cluster-api-e2e-main-gke

sbueringer · 2025-07-21T13:49:27Z

/test pull-cluster-api-e2e-main-gke

api/core/v1beta2/machinehealthcheck_types.go

api/core/v1beta2/cluster_types.go

sbueringer · 2025-07-22T07:39:29Z

/test pull-cluster-api-e2e-main-gke

k8s-ci-robot · 2025-07-22T07:43:29Z

@sbueringer: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-apidiff-main	`c81ae92`	link	false	`/test pull-cluster-api-apidiff-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

fabriziopandini · 2025-07-22T10:15:33Z

Great work
/lgtm

k8s-ci-robot · 2025-07-22T10:15:40Z

LGTM label has been added.

Git tree hash: fde147487d6305cee130dcf4e21c0ee9a9d5ec6a

sbueringer · 2025-07-22T10:22:12Z

/hold cancel
Doc commit was pushed a while back

fabriziopandini · 2025-07-22T10:23:43Z

(after a quick sync with Stefan)

/approve

So we can keep working on other changes in #12497 before getting to near to the code freeze.

If there will be some late feedback, we will address then in a follow up

k8s-ci-robot · 2025-07-22T10:23:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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~~ [fabriziopandini]

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 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 18, 2025

k8s-ci-robot requested review from elmiko and sivchari July 18, 2025 10:17

k8s-ci-robot added do-not-merge/needs-area PR is missing an area label size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 18, 2025

sbueringer force-pushed the pr-field-reshuffling branch from 16d7870 to a7f8d81 Compare July 18, 2025 10:20

sbueringer added area/clusterclass Issues or PRs related to clusterclass area/machinehealthcheck Issues or PRs related to machinehealthchecks area/cluster Issues or PRs related to clusters labels Jul 18, 2025

k8s-ci-robot removed do-not-merge/needs-area PR is missing an area label labels Jul 18, 2025

sbueringer commented Jul 18, 2025

View reviewed changes

sbueringer changed the title ~~[WIP] ⚠️ Restructure MHC fields in MHC, Cluster and ClusterClass CRDs~~ ⚠️ Restructure MHC fields in MHC, Cluster and ClusterClass CRDs Jul 18, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 18, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2025

k8s-ci-robot assigned fabriziopandini and JoelSpeed Jul 18, 2025

fabriziopandini reviewed Jul 18, 2025

View reviewed changes

exp/topology/scope/blueprint.go Outdated Show resolved Hide resolved

sbueringer added 3 commits July 21, 2025 11:47

Restructure MHC fields in MHC, Cluster and ClusterClass CRDs

4a666de

Signed-off-by: Stefan Büringer buringerst@vmware.com

Fix review findings

576a4a9

Fix review findings

6090a63

enxebre reviewed Jul 21, 2025

View reviewed changes

api/core/v1beta2/machinehealthcheck_types.go Show resolved Hide resolved

enxebre reviewed Jul 21, 2025

View reviewed changes

api/core/v1beta2/machinehealthcheck_types.go Show resolved Hide resolved

enxebre reviewed Jul 21, 2025

View reviewed changes

sbueringer added 3 commits July 21, 2025 13:28

Remove Machine from Cluster & ClusterClass health check structs

23818a8

Add migration documentation

9194d44

Improve godoc

e4ed962

sbueringer force-pushed the pr-field-reshuffling branch from 1644530 to e4ed962 Compare July 21, 2025 11:56

sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 21, 2025

fabriziopandini reviewed Jul 21, 2025

View reviewed changes

fabriziopandini mentioned this pull request Jul 21, 2025

⚠️ Rename ClusterClass MachineInfrastructure #12511

Closed

Adjust godoc comments

c81ae92

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2025

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2025

k8s-ci-robot merged commit f3a9726 into kubernetes-sigs:main Jul 22, 2025
19 of 20 checks passed

k8s-ci-robot added this to the v1.11 milestone Jul 22, 2025

sbueringer deleted the pr-field-reshuffling branch July 22, 2025 11:31

⚠️ Restructure MHC fields in MHC, Cluster and ClusterClass CRDs #12504

⚠️ Restructure MHC fields in MHC, Cluster and ClusterClass CRDs #12504

Uh oh!

Conversation

sbueringer commented Jul 18, 2025

Uh oh!

sbueringer Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabriziopandini Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enxebre Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer commented Jul 18, 2025

Uh oh!

sbueringer commented Jul 18, 2025

Uh oh!

sbueringer commented Jul 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fabriziopandini Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbueringer commented Jul 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

enxebre Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enxebre Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbueringer Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

enxebre Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbueringer commented Jul 21, 2025

Uh oh!

sbueringer commented Jul 21, 2025

Uh oh!

sbueringer commented Jul 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbueringer commented Jul 22, 2025

Uh oh!

k8s-ci-robot commented Jul 22, 2025

Uh oh!

fabriziopandini commented Jul 22, 2025

Uh oh!

sbueringer Jul 18, 2025 •

edited

Loading

sbueringer Jul 18, 2025 •

edited

Loading

enxebre Jul 21, 2025 •

edited

Loading

enxebre Jul 21, 2025 •

edited

Loading

sbueringer Jul 21, 2025 •

edited

Loading

sbueringer Jul 21, 2025 •

edited

Loading