Consider dropping MachineHealthCheck conditions validations

[rikatz]

What would you like to be added (User Story)?

As an operator, I want to use Machine Health Check to detect when my node was deleted, but without relying on any other additional conditions.

Detailed Description

As today cloud providers can detect if a node was deleted from the underlying infrastructure (eg. vsphere-cloud-provider) and delete the node from Kubernetes cluster (kubectl delete node), and as Machine Health Check today also checks if the node is present, otherwise it reconciles, this is enough for a basic cloud health check and no further condition should be required

This way, the request is to consider dropping the "existence" of conditions on a MachineHealthCheck and make MHC controller be able to reconcile NodeNotFound errors only

Anything else you would like to add?

No response

Label(s) to be applied

/kind feature

[enxebre]

This way, the request is to consider dropping the "existence" of conditions on a MachineHealthCheck and make MHC controller be able to reconcile NodeNotFound errors only

We already support the missing node scenario

cluster-api/internal/controllers/machinehealthcheck/machinehealthcheck_targets.go

Line 108 in 806b10f

// the node does not exist

Are you suggesting something different?

[rikatz]

The issue is not the missing node scenario :) Is that validation webhook forces me to set conditions on MHC, even if I just want to check the missing node scenario and nothing else.

So today I need to set some random condition in https://doc.crds.dev/github.com/kubernetes-sigs/cluster-api/cluster.x-k8s.io/MachineHealthCheck/v1beta1@v1.6.0-beta.0#spec-unhealthyConditions just because I don't care about unhealthy Conditions, but just with node deleted scenario

[sbueringer]

I'm +1 to drop the validation:

cluster-api/api/v1beta1/machinehealthcheck_types.go

Line 50 in 8193dec

// +kubebuilder:validation:MinItems=1

which basically will make unhealthyConditions an optional field (or at least allow to set it to an empty array).

I don't see a reason why it should be mandatory, but maybe I'm missing something.

@fabriziopandini Opinions?

Note: We should also check for validations in other places, like e.g. this one for ClusterClass / Cluster.spec.topology...:

cluster-api/internal/webhooks/machinehealthcheck.go

Lines 200 to 205 in 8193dec

	if len(m.Spec.UnhealthyConditions) == 0 {
	allErrs = append(allErrs, field.Forbidden(
	fldPath.Child("unhealthyConditions"),
	"must have at least one entry",
	))
	}

[enxebre]

The issue is not the missing node scenario :) Is that validation webhook forces me to set conditions on MHC, even if I just want to check the missing node scenario and nothing else.

Ah, understood. I'm +1 as well for specific conditions not being a requirement, I have seen the use case a few times where the api consumer needs to set a "fake" condition just to satisfy the req.

[fabriziopandini]

I'm generally ok, but it would be great if we find some way to surface that MHC remediates if the node goes away
e.g I'm not sure this is documented in our book or in the description of our API fields

[enxebre]

/triage accepted