✨ feat: introduce NodeReadinessEvaluation (NRE) CRD and controller #345
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,267 @@ | ||
| /* | ||
| Copyright The Kubernetes Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package v1alpha1 | ||
|
|
||
| import ( | ||
| corev1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| ) | ||
|
|
||
| // NodeEvaluationState indicates the overall readiness/availability state of the node based on all rules. | ||
| // +kubebuilder:validation:Enum=Available;NotAvailable | ||
| type NodeEvaluationState string | ||
|
|
||
| const ( | ||
| // NodeEvaluationStateAvailable indicates the node has satisfied all applicable rules and is available for scheduling. | ||
| NodeEvaluationStateAvailable NodeEvaluationState = "Available" | ||
|
|
||
| // NodeEvaluationStateNotAvailable indicates one or more applicable rules are currently not satisfied. | ||
| NodeEvaluationStateNotAvailable NodeEvaluationState = "NotAvailable" | ||
| ) | ||
|
|
||
| // RuleStatus defines the result of evaluating a NodeReadinessRule's criteria against a Node. | ||
| // Rule-configuration faults (e.g. an invalid NodeSelector) are reported on the | ||
| // NodeReadinessRule's own status conditions, not here. | ||
| // +kubebuilder:validation:Enum=Satisfied;Unsatisfied | ||
| type RuleStatus string | ||
|
|
||
| const ( | ||
| // RuleStatusSatisfied indicates that the Node successfully met all conditions | ||
| // defined in the NodeReadinessRule. The controller will ensure the corresponding | ||
| // taint is removed so the node is unblocked. | ||
| RuleStatusSatisfied RuleStatus = "Satisfied" | ||
|
|
||
| // RuleStatusUnsatisfied indicates that one or more conditions defined in the | ||
| // NodeReadinessRule were not met. The controller will ensure the corresponding | ||
| // taint remains present to block scheduling. | ||
| RuleStatusUnsatisfied RuleStatus = "Unsatisfied" | ||
| ) | ||
|
|
||
| // NodeReadinessEvaluationSpec defines the desired state of NodeReadinessEvaluation. | ||
| type NodeReadinessEvaluationSpec struct { | ||
| // nodeName specifies the exact name of the target Kubernetes Node. | ||
| // This object establishes a strict 1:1 relationship with the specified node, | ||
| // acting as the single source of truth for all rules and statuses applied to it. | ||
| // Because it binds this resource to a specific physical or virtual machine, it cannot be changed once set. | ||
| // | ||
| // The validation constraints enforce standard Kubernetes resource naming | ||
| // (RFC 1123 DNS Subdomain format), as defined in upstream apimachinery: | ||
| // https://github.com/kubernetes/apimachinery/blob/master/pkg/util/validation/validation.go#L198 | ||
| // | ||
| // +required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=253 | ||
| // +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$` | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="nodeName is immutable and cannot be changed once set" | ||
| NodeName string `json:"nodeName,omitempty"` | ||
|
Contributor
There was a problem hiding this comment. Considering the fact that spec is usually meant for defining the desired state of a CRD, may I know if you considered the trade-offs while putting NodeName as a spec? My first thought was that we will be better off using 1:1 metadata.name for node and nre. With on-going updates, the line maybe slightly misplaced, putting the context in the comment only NodeName string `json:"nodeName,omitempty"`
Contributor
Author
There was a problem hiding this comment. Yeah, The idea was to keep 1:1 mapping only and currently even the metadata.name is also set to nodename, We should be good to drop this field, but only thing is then the spec will become empty so I thought to keep the nodename as immutable field for better clarity.
Contributor
There was a problem hiding this comment. Yes that's also right, spec will be empty. But unless there's some guideline or practice in common projects not to do so, we should be good I guess. |
||
| } | ||
|
|
||
| // NodeReadinessEvaluationStatus defines the observed state of NodeReadinessEvaluation. | ||
| // +kubebuilder:validation:MinProperties=1 | ||
| type NodeReadinessEvaluationStatus struct { | ||
| // conditions represent the latest available observations of the node's readiness evaluation state. | ||
| // Known condition types are: | ||
| // - "Evaluated": indicates whether the controller successfully evaluated all rules without errors. | ||
|
Contributor
There was a problem hiding this comment. Is it possible to get Ready=False AND Evaluated=True on a NREStatus? I'm trying to see whether Evaluated is functionally identical signal as Ready or how it should be interpreted.
Contributor
There was a problem hiding this comment. Do I understand your comments correctly: you think Evaluated=False suggests 'errors' processing this node with NRC, whereas Ready=False means Taint is present (pending readiness on this node). So a normal user would subscribe to "kubectl wait --for=condition=Ready". who / how do you see Evaluated should be consumed? and could you also clarify what 'errors' do you see this to be. is it like permission (for eg: to remove taint) or something else?
Contributor
Author
There was a problem hiding this comment. Yes Correct, What I thought is, when the Ready/Available is False, User can check the status of Evaluated
|
||
| // - "Available": indicates whether the node has satisfied all rules, has zero taints applied, and is available for scheduling. | ||
| // | ||
| // +optional | ||
| // +listType=map | ||
| // +listMapKey=type | ||
| // +kubebuilder:validation:MaxItems=8 | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` | ||
|
|
||
| // state indicates the overall readiness state of the node based on all applicable rules. | ||
| // It acts as a top-level health indicator for this node's readiness evaluation. | ||
| // | ||
| // +optional | ||
| State NodeEvaluationState `json:"state,omitempty"` | ||
|
|
||
| // rules contains the evaluation outcomes for all rules applicable to this node. | ||
| // Each entry is keyed by ruleName, allowing independent per-rule updates by | ||
| // parallel rule-workers without last-write-wins conflicts (listType=map). | ||
| // | ||
| // +optional | ||
| // +listType=map | ||
| // +listMapKey=ruleName | ||
| // +kubebuilder:validation:MaxItems=100 | ||
|
Contributor
There was a problem hiding this comment. NA - I'm not suggesting to change this. but I'll be worried about a node having to deal with 100 taints!~ .
Contributor
Author
There was a problem hiding this comment. True that, do you think as a controller we should provide some recommendation? or controller optimal performance when the Node has X taints? |
||
| Rules []RuleEvaluation `json:"rules,omitempty"` | ||
| } | ||
|
|
||
| // RuleEvaluation defines the outcome of evaluating a single NodeReadinessRule against this Node. | ||
| type RuleEvaluation struct { | ||
|
Contributor
There was a problem hiding this comment. Personally I think ConditionPolicy is a value that may be worth adding here. |
||
| // ruleName is the name of the NodeReadinessRule. | ||
| // This field is the map key for status.rules (listType=map) and must always be present. | ||
| // | ||
| // +required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=253 | ||
| RuleName string `json:"ruleName,omitempty"` | ||
|
|
||
| // ruleUID is the UID of the NodeReadinessRule. | ||
| // If the rule is deleted and recreated with the same name, the UID will differ, | ||
| // allowing the controller to detect and replace stale evaluation entries. | ||
| // | ||
| // +required | ||
| RuleUID types.UID `json:"ruleUID,omitempty"` | ||
|
|
||
| // ruleStatus indicates the overall outcome of the rule's criteria against the Node. | ||
| // | ||
| // +required | ||
| RuleStatus RuleStatus `json:"ruleStatus,omitempty"` | ||
|
|
||
|
Contributor
There was a problem hiding this comment. should we add the taintKey/effect here to help for self-reporting (instead of the active taints 'count' at the summary)? I dunno, it would be redundant to rule.spec.taint, but for the headlamp kind of clients we are building, the CR may give a singular view of the readiness state on a 'node' level, without have to lookup again on the rule).
Contributor
Author
There was a problem hiding this comment. We can add, its provides better clarity and better UX, avoid looks up |
||
| // taintStatus reflects the observed state of the rule's specified taint on the Node (Present/Absent). | ||
| // | ||
| // +required | ||
| TaintStatus TaintStatus `json:"taintStatus,omitempty"` | ||
|
|
||
| // taintKey is the key of the taint managed by this rule, stamped at evaluation | ||
| // time so this entry is self-contained without requiring a lookup of the rule. | ||
| // Matches rule.spec.taint.key. | ||
| // | ||
| // +required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=253 | ||
| TaintKey string `json:"taintKey,omitempty"` | ||
|
|
||
| // taintEffect is the effect of the taint managed by this rule, stamped at | ||
| // evaluation time so this entry is self-contained without requiring a lookup | ||
| // of the rule. Matches rule.spec.taint.effect. | ||
| // | ||
| // +required | ||
| // +kubebuilder:validation:Enum=NoSchedule;PreferNoSchedule;NoExecute | ||
| TaintEffect corev1.TaintEffect `json:"taintEffect,omitempty"` | ||
|
Contributor
There was a problem hiding this comment. Also do we keep Taint key and effect separate for cases where people may be interested in filtering by the effect?
Contributor
Author
There was a problem hiding this comment. Yes, Thats the idea. |
||
|
|
||
| // conditionPolicy is the aggregation policy used when evaluating the conditions list, | ||
| // stamped at evaluation time so this entry is self-contained without requiring a lookup | ||
| // of the rule. Matches rule.spec.conditionPolicy. | ||
| // | ||
| // +required | ||
| ConditionPolicy ConditionPolicy `json:"conditionPolicy,omitempty"` | ||
|
|
||
| // reason contains a concise, machine-readable string detailing the primary outcome. | ||
| // | ||
| // +optional | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=256 | ||
| Reason string `json:"reason,omitempty"` | ||
|
|
||
| // message is a comprehensive, human-readable explanation providing further context. | ||
| // | ||
| // +optional | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=10240 | ||
|
Contributor
There was a problem hiding this comment. is it intentionally 10K?
Contributor
There was a problem hiding this comment. I was surprised at this thinking why should they be this long, but looks like k8s api gives 32k for Message and 1024! for Reason as well: https://github.com/kubernetes/apimachinery/blob/50d9b4a672b474db2e0bf61c968d87465b55eb56/pkg/apis/meta/v1/types.go#L1702. so ours is fine! |
||
| Message string `json:"message,omitempty"` | ||
|
|
||
| // readinessConditions provides a detailed breakdown of each condition evaluation | ||
| // for this Node. This allows for granular debugging of which specific criteria passed/failed. | ||
| // | ||
| // +optional | ||
| // +listType=map | ||
| // +listMapKey=type | ||
| // +kubebuilder:validation:MaxItems=32 | ||
| ReadinessConditions []ConditionEvaluationResult `json:"readinessConditions,omitempty"` | ||
|
Contributor
There was a problem hiding this comment. I would be against reusing the old ConditionEvaluationResult. When adding default status, I ran into an issue of not being able to properly provide visibility into what's the actual status and what's the status we are calculating after applying default status, because of the name 'CurrentStatus'. Now that we have a chance to introduce new API, I think replacing 'CurrentStatus' with something like 'EffectiveStatus' and 'ObservedStatus' can improve the observability.
Contributor
Author
There was a problem hiding this comment. I tend to agree with you, I think instead of currentState we should rename it to ObservedState and we can possibly add another field as EffectiveState which can help the user to understand the effective state which will be ObservedState if present or Default state if CurrentState not present
Contributor
There was a problem hiding this comment. Ah About the NRE and NRR, not quite sure about that. My personal preference would have been to do directly in NRR but obviously not possible due to schema constraints breaking existing schemas, so thought while introducing a new one, should be better doing it from day 1. Anyway that's your call. |
||
|
|
||
| // lastEvaluatedAt records the exact moment the controller most recently assessed this rule. | ||
| // | ||
| // +required | ||
| LastEvaluatedAt metav1.Time `json:"lastEvaluatedAt,omitempty,omitzero"` | ||
|
|
||
| // firstEvaluatedAt is the time the rule was first assessed against this node. | ||
| // | ||
| // +optional | ||
| FirstEvaluatedAt *metav1.Time `json:"firstEvaluatedAt,omitempty"` | ||
|
Contributor
There was a problem hiding this comment. nit: is this for observing possible rule-reconciliation delay on the node? I think the other TaintAt times are also set at the first rule-reconciliation times. but I think I'm okay to keep this as it may bring some additional insights on rule's actions later.
Contributor
Author
There was a problem hiding this comment. Yes, This is the time the NRC saw the rule. helps to see how long it took to evaluate the rule on tainted node |
||
|
|
||
| // taintObservedAt is the time NRC first observed the taint present on the node, | ||
| // regardless of whether NRC applied it or it was pre-existing (e.g. via --register-with-taints). | ||
| // This marks the beginning of the node being blocked by this rule and is the correct | ||
| // start time for computing time-to-unblock SLIs. | ||
| // | ||
| // +optional | ||
| TaintObservedAt *metav1.Time `json:"taintObservedAt,omitempty"` | ||
|
|
||
| // taintAddedAt is the time NRC itself first applied the taint to the node in the | ||
| // current taint lifecycle (i.e., since the last removal). This field is nil when | ||
| // the taint was pre-existing and NRC adopted it rather than creating it. | ||
| // The value is set once on the Absent→Present transition and carried forward on | ||
| // subsequent reconciles; it is not the timestamp of the most recent reconcile. | ||
| // Use taintObservedAt for "how long has the node been blocked"; use this field | ||
| // to measure NRC's own apply latency (taintAddedAt - firstEvaluatedAt). | ||
| // | ||
| // +optional | ||
| TaintAddedAt *metav1.Time `json:"taintAddedAt,omitempty"` | ||
|
Contributor
There was a problem hiding this comment. do we update this in continuous when Taint is reapplied?
Contributor
There was a problem hiding this comment. or should we split into FirstAppliedAt and CurrentTaintAppliedAt?
Contributor
Author
There was a problem hiding this comment. so currently we support only one taint lifecycle Absent->Present cycle, do you expect to support Absent→Present→Absent→Present cycle as well, does it have any impact on telemetry? |
||
|
|
||
| // taintRemovedAt is the time the controller successfully removed the taint | ||
| // after the node satisfied all conditions. | ||
| // | ||
| // +optional | ||
| TaintRemovedAt *metav1.Time `json:"taintRemovedAt,omitempty"` | ||
| } | ||
|
|
||
| // +kubebuilder:object:root=true | ||
| // +kubebuilder:subresource:status | ||
| // +kubebuilder:resource:scope=Cluster,shortName=nre | ||
| // +kubebuilder:printcolumn:name="Node",type=string,JSONPath=`.spec.nodeName`,description="The name of the target Node." | ||
| // +kubebuilder:selectablefield:JSONPath=`.spec.nodeName` | ||
| // +kubebuilder:printcolumn:name="State",type=string,JSONPath=`.status.state`,description="The overall readiness evaluation state of the node." | ||
|
Contributor
There was a problem hiding this comment. ah, now I see where do you see these enums for state. could we use Status=(?Condition==Ready).status?
Contributor
Author
There was a problem hiding this comment. Array filtering was not supported when I check last time, but I can see if there are any alternatives if we really want to drop the state field. |
||
| // +kubebuilder:selectablefield:JSONPath=`.status.state` | ||
| // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`,description="The age of this resource." | ||
|
|
||
| // NodeReadinessEvaluation is the Schema for the NodeReadinessEvaluations API. | ||
| // Each instance maps 1:1 to a Node and folds the outcomes of all applicable | ||
| // NodeReadinessRules for that node into a single object. | ||
| // An ownerReference to the corresponding Node is set for automatic garbage | ||
| // collection when the node is deleted. | ||
| type NodeReadinessEvaluation struct { | ||
| metav1.TypeMeta `json:",inline"` | ||
|
|
||
| // metadata is a standard object metadata. | ||
| // More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata | ||
| // | ||
| // +optional | ||
| metav1.ObjectMeta `json:"metadata,omitempty,omitzero"` | ||
|
|
||
| // spec defines the desired state of NodeReadinessEvaluation. | ||
| // | ||
| // +required | ||
| Spec NodeReadinessEvaluationSpec `json:"spec,omitempty,omitzero"` | ||
|
|
||
| // status defines the observed state of NodeReadinessEvaluation. | ||
| // | ||
| // +optional | ||
| Status NodeReadinessEvaluationStatus `json:"status,omitempty,omitzero"` | ||
| } | ||
|
|
||
| // +kubebuilder:object:root=true | ||
|
|
||
| // NodeReadinessEvaluationList contains a list of NodeReadinessEvaluation. | ||
| type NodeReadinessEvaluationList struct { | ||
| metav1.TypeMeta `json:",inline"` | ||
|
|
||
| // metadata is the standard list's metadata. | ||
| // More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#lists-and-simple-kinds | ||
| // | ||
| // +optional | ||
| metav1.ListMeta `json:"metadata,omitempty"` | ||
|
|
||
| // items is the list of NodeReadinessEvaluation. | ||
| Items []NodeReadinessEvaluation `json:"items"` | ||
| } | ||
|
|
||
| func init() { | ||
| objectTypes = append(objectTypes, &NodeReadinessEvaluation{}, &NodeReadinessEvaluationList{}) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to compare with the states we introduced here: #344.
We added new state conventions for nodes: 'held' vs 'released' - but I think we saw them as "node's" state per rule. these are rule's state so "satisfied" makes sense. is that how you're thinking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Correct, Its the Rule's State