Default MHC fields defined in a ClusterClass

[killianmuldoon]

There is an open PR to include MachineHealthCheck in the ClusterClass proposal #5125 .

The MHC spec in a ClusterClass, according to the proposal would allow an empty MachineHealthCheck to be added to the ClusterClass and sensible defaults would be added to the object before it is created. This issue covers issues related to defaulting including:

1. Which fields should be defaulted and to what values?

type MachineHealthCheckSpec struct {
    ClusterName         string                  `json:"clusterName"`
    Selector            v1.LabelSelector    `json:"selector"`
    UnhealthyConditions []UnhealthyCondition    `json:"unhealthyConditions"`
    MaxUnhealthy        *intstr.IntOrString     `json:"maxUnhealthy,omitempty"`
    UnhealthyRange      *string                 `json:"unhealthyRange,omitempty"`
    NodeStartupTimeout  *v1.Duration        `json:"nodeStartupTimeout,omitempty"`
    RemediationTemplate *v1.ObjectReference `json:"remediationTemplate,omitempty"`
}

• ClusterName and Selector MUST be set by the ClusterClass reconciler as that's the only place the information will be available.

2. Where should the defaulting be done? relevant discussion here
   • In the MHC webhook (adding additional defaulting on top of what exists today
   • In the ClusterClass webhook (makes defaulting for vanilla MHC and ClusterClass MHC different, also adds additional non-user specificed values to the ClusterClass.
   • In the ClusterClass controller (defaulting during reconciliation)

[sbueringer]

/area topology

[fabriziopandini]

my 2 cents.

• There must not be any different default rules/behaviors between a topology-owned MHC and a user-created MHC. they should be identical.
• Corollary: MHC defaulting must be a responsibility of the MHC webhook.
• Topology controller, when reconciling a topology-owned MHC must enforce ClusterName, Selector, and the values specifically defined in the ClusterClass (and not everything else)

[fabriziopandini]

/milestone v1.1

[killianmuldoon]

A decision has been made to include no additional defaulting in the current proposal and implementation. The proposal has been merged with that design. Defaulting may still be included in a future iteration of the MachineHealthCheck generation code. This should be removed from milestone 1.1.

[fabriziopandini]

/milestone Next
Given that it is TBH when we want to come back to this issue

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[fabriziopandini]

/remove-lifecycle stale
Let's keep this around a little bit more to see if there are feedback from users

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[fabriziopandini]

/remove-lifecycle stale

[fabriziopandini]

As per 29/7 discussion we are not going to implement this
/close

[k8s-ci-robot]

@fabriziopandini: Closing this issue.

In response to this:

As per 29/7 discussion we are not going to implement this
/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.