Repository navigation
✨ feat: introduce NodeReadinessEvaluation (NRE) CRD and controller - #345
Karthik-K-N wants to merge 4 commits into
Conversation
✅ Deploy Preview for node-readiness-controller canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Karthik-K-N The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| // - Node objects (conditions, taints, labels) | ||
| // - NodeReadinessRule objects (enqueues all nodes matching the changed rule) | ||
| func (r *NodeReadinessEvaluationReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| return ctrl.NewControllerManagedBy(mgr). |
There was a problem hiding this comment.
@Karthik-K-N IIUC, this would trigger two parallel reconciliations for node updates (NodeReconciler and NREReconciler) and one is handling the output of the other. I wonder if a separate controller for NRE is a better fitting pattern for this or should be bridged into NodeReconciler's watch?
There was a problem hiding this comment.
yes I thought about that and I missed to point out why I chose this way, here are my thoughts
- Initially tried combining both into the NodeReconciler, but then when I checked with best practices of writing controller, if we do so we will be merging two different operations into one, Node controller adding/removing taints,NRE just storing the result, One failure should not cause requeue for other and I think NodeReconcile should be as quick as possible as it affects the workloads.
- Followed the existing pattern of having separate controllers like NRR, Node and NRE
- Even if there is race and NRE reconciles first and later the Node, Since the NRE also watches for condition, taint and label, if something updated by the Node Rec then eventually it triggers the NRE. We will be eventually consistent.
These are the thoughts and I lean towards keeping them separate, but let me know what do you think.
There was a problem hiding this comment.
My concern with another reconciler is that we are reintroducing a shared (Node) state again between two controllers. looking at buildRuleEvaluation it is very similar to evaluateRuleForNode. reevaluating the node status between two controllers with caches seem like a bad idea (and that seems like repeating what we did and trying to change with rule.status). :/
I saw #343 emitting individual nodeStatusDelta in rule-reconciliation, which could make adding NRE even cheaper than before. Happy to discuss more / hear your thoughts further on this.
|
i tested this branch and found two behaviours worth knowing about, plus one interaction with #315. all three are reproduced with runnable tests, happy to share them. first, a rule-creation window that never heals. the NRE fan-out fires on the rule Create event (GenerationChangedPredicate only filters updates), but the shared ruleCache is only populated on the rule controller's second reconcile pass, because the first one adds the finalizer and returns RequeueAfter one second. so the NRE reconciles for every matching node run against a cache that cannot contain the new rule yet. for rules whose evaluation then changes a node, the taint write re-triggers the node watch and everything heals. but for a rule whose conditions are already satisfied everywhere, nothing mutates any node, the rule's status patch doesn't bump generation, heartbeats don't pass the node predicate (it compares only condition type to status), and informer resyncs are filtered as no-ops. the NRE just permanently misses the rule until some unrelated node change. deletion is fine, reconcileDelete empties the cache before dropping the finalizer. i think this is the concrete version of the question ajaysundark raised above: the reconciler's correctness depends on another controller's queue having run first. listing rules from the informer inside Reconcile instead of reading the private cache would remove the ordering dependency entirely. second, this branch merges cleanly with #315, and the two disagree once combined. evaluateRuleForNode there honours conditionPolicy anyOf, while buildRuleEvaluation here ANDs every condition unconditionally. an anyOf rule with one of two conditions satisfied ends up enforced as satisfied (no taint) while the NRE for the same node reports it Unmatched. a shared evaluation helper honouring GetConditionPolicy() in both paths would keep them from drifting. small one: nothing currently produces RuleStatusError, so the Errors count, State Pending, and the Evaluated False condition are unreachable in this iteration. fine if that's intentional groundwork, might deserve a TODO. |
The controller side of the NRE proposal is still a draft, so nothing populates these objects yet. Writing them here makes the per-node datasource measurable now, since the payload is decided by the schema and by how many rules apply to a node. Refs kubernetes-sigs/node-readiness-controller#345
|
@Karthik-K-N is this ready for review? |
yes , its ready for review |
|
The rule-creation window I flagged here on Aug 11 has a second victim now, and this one is worse because it's the reporting surface. Walked the sequence on your head The Aug 11 fix suggestion covers both consumers, and it got cheaper to justify: have this controller read rules through the informer instead of the private cache, and the ordering dependency disappears rather than getting patched around. Separately, I reproduced prow's lint failure locally so it can be named precisely: gocritic What I checked that holds: rule deletions do reach this controller (in controller-runtime v0.24.1 a nil DeleteFunc defaults to passing, so GenerationChangedPredicate only filters updates), and your D1 shows the prune works once a reconcile runs; the CEL immutability on nodeSelector makes mapping by selector safe across rule updates; node deletion is covered by the ownerReference; and the single-writer separation is real, this reconciler writes nothing but its own status. One RBAC nit: the marker asks |
|
|
||
| for _, r := range status.Rules { | ||
| switch r.RuleStatus { | ||
| case readinessv1alpha1.RuleStatusMatched: |
There was a problem hiding this comment.
The Matched / Unmatched rules on status is bit unclear to me. Sorry I missed to get this at the doc, will look into it again.
There was a problem hiding this comment.
So the RuleStatus field helps to understand "whether node satisfy every condition listed in the rule"
Matched - All rule.Spec.Conditions are satisfied - ideally no taint
Unmatched - Any condition is not satisfied - there is taint on node
Do you think we should use better field name to convey this?
Instead of Matched should I change that to RuleStatusSatisfied means the rule is satisfied by the node?
|
|
||
| status.Summary = readinessv1alpha1.EvaluationSummary{ | ||
| MatchedRules: &matched, | ||
| UnmatchedRules: &unmatched, |
There was a problem hiding this comment.
does 'unmatched' (node-selector escaped?) add value to the per-node status? As a node-owner I would less-likely be interested in rules that are not affecting my node.
Imagine a large cluster with many different node-pools. Setting the status of a node in pool-A for other rules associated with every other pools may be rather confusing to the user.
There was a problem hiding this comment.
True, I think we can drop the UnmatchedRules field.
| // The slice is owned and fully replaced by the controller on each reconcile (listType=atomic). | ||
| // | ||
| // +optional | ||
| // +listType=atomic |
There was a problem hiding this comment.
listType=map with Key=ruleName?
There was a problem hiding this comment.
we would need this for multiple rule-workers working in parallel. and ref is not compatible with map, so it'd be good to consider flattening the rule references here.
| // 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" |
There was a problem hiding this comment.
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.
Yes, Correct, Its the Rule's State
| RuleStatusUnsatisfied RuleStatus = "Unsatisfied" | ||
|
|
||
| // RuleStatusError indicates that a programmatic or configuration error occurred | ||
| // during the evaluation process (e.g., an invalid or unparseable NodeSelector). |
There was a problem hiding this comment.
how will unparseable NodeSelector error is attributable to any specific node? I think rule-configuration error on the node's evaluation error is confusing.
I could imagine a more generic 'Blocked' for RuleStatus on the node - but then the question about - when do we decide unsatisfied vs blocked comes up as we do not have any 'time' budget on the rule-reconciliation.
There was a problem hiding this comment.
The Error is not attributed to Node rather the particular Rule of the Node. But yes having unparseble Nodeselector means it belong to all Nodes and which does not makes sense, I just removed this state. I think with this we no more need NodeEvaluationStatePending Node state as well, I just removed that.
| // | ||
| // +required | ||
| RuleStatus RuleStatus `json:"ruleStatus,omitempty"` | ||
|
|
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).
There was a problem hiding this comment.
We can add, its provides better clarity and better UX, avoid looks up
| // | ||
| // +optional | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=10240 |
There was a problem hiding this comment.
is it intentionally 10K?
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!
| // NRC's own apply latency (taintAddedAt - firstEvaluatedAt). | ||
| // | ||
| // +optional | ||
| TaintAddedAt *metav1.Time `json:"taintAddedAt,omitempty"` |
There was a problem hiding this comment.
do we update this in continuous when Taint is reapplied?
There was a problem hiding this comment.
or should we split into FirstAppliedAt and CurrentTaintAppliedAt?
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?
| // firstEvaluatedAt is the time the rule was first assessed against this node. | ||
| // | ||
| // +optional | ||
| FirstEvaluatedAt *metav1.Time `json:"firstEvaluatedAt,omitempty"` |
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.
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
| // lastEvaluationTime records the exact moment the controller most recently assessed this rule. | ||
| // | ||
| // +required | ||
| LastEvaluationTime metav1.Time `json:"lastEvaluationTime,omitempty,omitzero"` |
There was a problem hiding this comment.
This means we are updating all the matching nodes' RuleEvals, irrespective of whether they are satisfied.
Rule-reconciliations should be less frequent - only on rule modifications - so this seems fine, and 'freshness' details of evaluations maybe helpful.
| // +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." |
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?
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.
AnuragThePathak
left a comment
There was a problem hiding this comment.
Haven't got a chance to look into the controller code, which I will get back soon.
About the API, loved the recent update, to drop error and loading states (loading was unnecessary anyway for our controller as it's not time taking by nature). Error being displayed at the rule level solved a lot of problems.
| // +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"` |
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"`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.
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.
| // +listType=map | ||
| // +listMapKey=type | ||
| // +kubebuilder:validation:MaxItems=32 | ||
| ReadinessConditions []ConditionEvaluationResult `json:"readinessConditions,omitempty"` |
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.
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
but
I think if we do this we should do it at both NRE and NRR level to avoid further confusion and keep them simple, Given that both are bit dependent or it may give two different results to user. I can take this up as a follow up task.
There was a problem hiding this comment.
Ah state suffix would be confusing I guess, status would be more consistent.
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.
4cdd298 to
6c00c70
Compare
| // lastEvaluationTime records the exact moment the controller most recently assessed this rule. | ||
| // | ||
| // +required | ||
| LastEvaluationTime metav1.Time `json:"lastEvaluationTime,omitempty,omitzero"` |
There was a problem hiding this comment.
Randomly realized that we are putting suffix of "-At" everywhere except this one.
There was a problem hiding this comment.
Nice catch, I updated it to LastEvaluatedAt
| } | ||
|
|
||
| // RuleEvaluation defines the outcome of evaluating a single NodeReadinessRule against this Node. | ||
| type RuleEvaluation struct { |
There was a problem hiding this comment.
Personally I think ConditionPolicy is a value that may be worth adding here.
| // | ||
| // +required | ||
| // +kubebuilder:validation:Enum=NoSchedule;PreferNoSchedule;NoExecute | ||
| TaintEffect corev1.TaintEffect `json:"taintEffect,omitempty"` |
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?
There was a problem hiding this comment.
Yes, Thats the idea.
| } | ||
|
|
||
| ruleStatus := readinessv1alpha1.RuleStatusSatisfied | ||
| if !allConditionsSatisfied { |
There was a problem hiding this comment.
You are completely ignoring conditionPolicy. Have to update that.
There was a problem hiding this comment.
I have rebased and updated the code. Thank you
6c00c70 to
7ed55ff
Compare
7ed55ff to
99a1ca2
Compare
vishnukothakapu
left a comment
There was a problem hiding this comment.
Reviewing from the Headlamp plugin side, it looks good! The new NRE CRD with its 1:1 node mapping is exactly what we need for the UI. I've left a couple of quick inline suggestions (around DryRun and rule generations) to make the data even more powerful for the frontend
| // Rebuild the full rules slice from scratch on every reconcile. | ||
| // listType=atomic means the controller owns the whole slice. | ||
| newRules := make([]readinessv1alpha1.RuleEvaluation, 0, len(applicableRules)) | ||
| for _, rule := range applicableRules { |
There was a problem hiding this comment.
I noticed in that we are skipping rules if rule.Spec.DryRun is true. From a Headlamp UI/Plugin perspective, DryRun is actually when users most want to look at the evaluation data to see what would happen if they enabled the rule.
Instead of skipping DryRun rules entirely, could we evaluate them and add a DryRun: true boolean to the RuleEvaluation struct? That way, in the Headlamp UI, we can show a special "Dry Run: Satisfied / Unsatisfied" badge on the Node without users getting confused about why the taint isn't actually applied.
There was a problem hiding this comment.
yeah, dryrun handling as a whole could use some improvement. @Karthik-K-N Can we consider evaluating this as a follow-up to this PR?
| // allowing the controller to detect and replace stale evaluation entries. | ||
| // | ||
| // +required | ||
| RuleUID types.UID `json:"ruleUID,omitempty"` |
There was a problem hiding this comment.
Should we also track ObservedRuleGeneration here? If a rule is edited, the UID stays the same. Having the generation would let our Headlamp plugin easily flag when a node's evaluation is out-of-sync with the latest rule updates (e.g., showing a 'Pending' badge)
| } | ||
| } | ||
|
|
||
| if r.EnableNRE { |
There was a problem hiding this comment.
I remember we discussed an option where the node-reconciler could watch the node-keys and handle updates as sole-writer. We could investigate if that's a better path in general. but not blocking this PR
There was a problem hiding this comment.
sure, will work on that as a follow up item
| prevRules := make([]readinessv1alpha1.RuleEvaluation, len(nre.Status.Rules)) | ||
| copy(prevRules, nre.Status.Rules) | ||
|
|
||
| patch := client.MergeFrom(nre.DeepCopy()) |
There was a problem hiding this comment.
Does this need optimistic lock? xref previous issues we have been dealing with #343
There was a problem hiding this comment.
yes, I think its better to use that to keep consistency, Can I take that in immediate follow up PR?
Description
Introduces the
NodeReadinessEvaluation(NRE) custom resource and its dedicated reconciler. NRE provides a per-node, read-only mirror of the evaluated state of every applicableNodeReadinessRule, giving operators a single object tokubectl get nre <node-name>to see the full readiness picture of any node without having to cross-reference multiple rule statuses.Discussion document: https://docs.google.com/document/d/1DOP1G6i__nQN8qbhlSvdswkUquMeSKQY4LU554AbcgM/edit?usp=sharing
Feature Flag
The controller is opt-in via:
--enable-node-readiness-evaluationRelated Issue
Type of Change
Testing
Checklist
make testpassesmake lintpassesDoes this PR introduce a user-facing change?
Doc #(issue)