Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions api/v1alpha1/nodereadinessrule_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,16 @@ type NodeEvaluation struct {
//
// +required
LastEvaluationTime metav1.Time `json:"lastEvaluationTime,omitempty,omitzero"`

// taintAppliedAt is the timestamp when the controller applied the readiness taint to this Node.
//
// +optional
TaintAppliedAt metav1.Time `json:"taintAppliedAt,omitempty,omitzero"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @Karthik-K-N for note as we should keep this field name and semantics same when we split NodeEvaluation status

Comment thread
ajaysundark marked this conversation as resolved.

// taintObservedAt is the timestamp when the readiness taint was first observed on this Node.
//
// +optional
TaintObservedAt metav1.Time `json:"taintObservedAt,omitempty,omitzero"`
}

// ConditionEvaluationResult provides a detailed report of the comparison between
Expand Down
2 changes: 2 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions config/crd/bases/readiness.node.x-k8s.io_nodereadinessrules.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,16 @@ spec:
minLength: 1
pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
type: string
taintAppliedAt:
description: taintAppliedAt is the timestamp when the controller
applied the readiness taint to this Node.
format: date-time
type: string
taintObservedAt:
description: taintObservedAt is the timestamp when the readiness
taint was first observed on this Node.
format: date-time
type: string
taintStatus:
description: taintStatus represents the taint status on the
Node, one of Present, Absent.
Expand Down
19 changes: 19 additions & 0 deletions docs/book/src/operations/monitoring.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,25 @@ Total number of failure events recorded by the controller.
| `rule` | `NodeReadinessRule` name | Any rule name |
| `reason` | Failure label recorded by the controller | `EvaluationError`, `AddTaintError`, `RemoveTaintError` |

### `node_readiness_bootstrap_hold_duration_seconds`

Time from readiness taint application or observation to bootstrap completion.

| Property | Value |
| --- | --- |
| Type | `histogram` |
| Labels | `rule`, `taint_origin` |
| Buckets | `1, 5, 10, 30, 60, 120, 300, 600, 1200, 1800, 3600` |
| Recorded when | The controller marks bootstrap as completed for a node under a bootstrap-only rule. |

#### Labels

| Label | Description | Values |
| --- | --- | --- |
| `rule` | `NodeReadinessRule` name | Any rule name |
| `taint_origin` | Origin of the readiness taint's anchor timestamp | `controller`, `adopted` |


### `node_readiness_build_info`

*Available starting from the v0.6.0 release.*
Expand Down
12 changes: 7 additions & 5 deletions internal/controller/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,21 @@ import (
readinessv1alpha1 "sigs.k8s.io/node-readiness-controller/api/v1alpha1"
)

//nolint:godot
const (
// bootstrapAnnotationPrefix is the common prefix for all bootstrap completion
// annotations on a Node. The suffix is the rule's metadata.uid (RFC 4122 UUID,
// ~36 chars), which is immutable for the object's lifetime and globally unique.
//
// Full key format: readiness.k8s.io/bootstrap-completed-<ruleUID>
// Value format: {"rule-name":"<ruleName>"} (for human readability)
// Value format: {"rule-name":"<ruleName>"} (for human readability).
bootstrapAnnotationPrefix = "readiness.k8s.io/bootstrap-completed-"
)

// bootstrapAnnotationPayload is the JSON value stored in a bootstrap-completion annotation.
type bootstrapAnnotationPayload struct {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, TaintAppliedAt and Completed are better suited for the dedicated NodeEvaluation 'status' api we planned. Can we discuss this during our next sync on how we could shape it in the alpha2 api?

RuleName string `json:"rule-name"`
}

// bootstrapAnnotationKey returns the annotation key for a rule's bootstrap
// completion state, using the rule's UID as the suffix.
func bootstrapAnnotationKey(uid types.UID) string {
Expand All @@ -45,9 +49,7 @@ func bootstrapAnnotationKey(uid types.UID) string {
// bootstrapAnnotationValue returns the JSON-encoded value to store in the
// bootstrap annotation. It includes the rule name for human readability.
func bootstrapAnnotationValue(ruleName string) string {
v := struct {
RuleName string `json:"rule-name"`
}{RuleName: ruleName}
v := bootstrapAnnotationPayload{RuleName: ruleName}
b, err := json.Marshal(v)
if err != nil {
return `{"rule-name":""}` // should never happen
Expand Down
139 changes: 139 additions & 0 deletions internal/controller/node_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"context"
"errors"
"fmt"
"strings"
"time"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -147,6 +149,16 @@ func (r *RuleReadinessController) processNodeAgainstAllRules(ctx context.Context
continue
}

// Recover a missing TaintAppliedAt/TaintObservedAt anchor before evaluating the rule.
// Skip repeated recovery checks once attempts are exhausted.
if rule.Spec.EnforcementMode == readinessv1alpha1.EnforcementModeBootstrapOnly &&
r.hasTaintBySpec(node, rule.Spec.Taint) &&
r.taintAnchorMissing(rule, node.Name) &&
r.shouldAttemptTaintAppliedAtRecovery(rule.Name, node.Name) {
recovered := r.recoverTaintAppliedAtFromAPI(ctx, rule, node.Name)
r.recordTaintAppliedAtRecoveryOutcome(rule.Name, node.Name, recovered)
}

log.Info("Evaluating rule for node",
"node", node.Name,
"rule", rule.Name,
Expand Down Expand Up @@ -192,6 +204,12 @@ func (r *RuleReadinessController) processNodeAgainstAllRules(ctx context.Context
found := false
for i := range latestRule.Status.NodeEvaluations {
if latestRule.Status.NodeEvaluations[i].NodeName == node.Name {
if currEval.TaintAppliedAt.IsZero() && !latestRule.Status.NodeEvaluations[i].TaintAppliedAt.IsZero() {
currEval.TaintAppliedAt = latestRule.Status.NodeEvaluations[i].TaintAppliedAt
}
if currEval.TaintObservedAt.IsZero() && !latestRule.Status.NodeEvaluations[i].TaintObservedAt.IsZero() {
currEval.TaintObservedAt = latestRule.Status.NodeEvaluations[i].TaintObservedAt
}
latestRule.Status.NodeEvaluations[i] = currEval
found = true
break
Expand Down Expand Up @@ -250,6 +268,127 @@ func (r *RuleReadinessController) processNodeAgainstAllRules(ctx context.Context
return errors.Join(errs...)
}

const maxTaintAnchorRecoveryAttempts = 2

// Reports whether the cached evaluation is missing TaintAppliedAt or TaintObservedAt.
func (r *RuleReadinessController) taintAnchorMissing(rule *readinessv1alpha1.NodeReadinessRule, nodeName string) bool {
r.ruleCacheMutex.Lock()
defer r.ruleCacheMutex.Unlock()

prevEval := r.getPreviousNodeEvaluation(rule, nodeName)
return prevEval == nil || prevEval.TaintAppliedAt.IsZero() && prevEval.TaintObservedAt.IsZero()
}

// Reports whether recovery should still be attempted.
func (r *RuleReadinessController) shouldAttemptTaintAppliedAtRecovery(ruleName, nodeName string) bool {
r.taintAnchorRecoveryMutex.Lock()
defer r.taintAnchorRecoveryMutex.Unlock()

return r.taintAnchorRecoveryAttempts[ruleName+"/"+nodeName] < maxTaintAnchorRecoveryAttempts
}

// Records the outcome of a recovery attempt.
func (r *RuleReadinessController) recordTaintAppliedAtRecoveryOutcome(ruleName, nodeName string, recovered bool) {
key := ruleName + "/" + nodeName

r.taintAnchorRecoveryMutex.Lock()
defer r.taintAnchorRecoveryMutex.Unlock()

if recovered {
delete(r.taintAnchorRecoveryAttempts, key)
return
}
if r.taintAnchorRecoveryAttempts == nil {
r.taintAnchorRecoveryAttempts = make(map[string]int)
}
r.taintAnchorRecoveryAttempts[key]++
}

// Clears recovery tracking for a deleted rule.
func (r *RuleReadinessController) clearTaintAppliedAtRecoveryForRule(ruleName string) {
prefix := ruleName + "/"

r.taintAnchorRecoveryMutex.Lock()
defer r.taintAnchorRecoveryMutex.Unlock()

for key := range r.taintAnchorRecoveryAttempts {
if strings.HasPrefix(key, prefix) {
delete(r.taintAnchorRecoveryAttempts, key)
}
}
}

// Clears recovery tracking for a rule/node pair.
func (r *RuleReadinessController) clearTaintAppliedAtRecoveryForNode(ruleName, nodeName string) {
r.taintAnchorRecoveryMutex.Lock()
defer r.taintAnchorRecoveryMutex.Unlock()

delete(r.taintAnchorRecoveryAttempts, ruleName+"/"+nodeName)
}

// Recovers a missing TaintAppliedAt/TaintObservedAt from the API and updates the cached rule.
// Returns true if an existing anchor was found.
func (r *RuleReadinessController) recoverTaintAppliedAtFromAPI(ctx context.Context, rule *readinessv1alpha1.NodeReadinessRule, nodeName string) bool {
log := ctrl.LoggerFrom(ctx)

const (
attempts = 3
delay = 500 * time.Millisecond
)

for i := range attempts {
if i > 0 {
select {
case <-ctx.Done():
return false
case <-time.After(delay):
}
}

latestRule := &readinessv1alpha1.NodeReadinessRule{}
if err := r.Get(ctx, client.ObjectKey{Name: rule.Name}, latestRule); err != nil {
log.V(4).Info("Failed to refresh rule for TaintAppliedAt recovery",
"rule", rule.Name, "node", nodeName, "error", err.Error())
continue
}

for _, eval := range latestRule.Status.NodeEvaluations {
if eval.NodeName != nodeName {
continue
}
if eval.TaintAppliedAt.IsZero() && eval.TaintObservedAt.IsZero() {
continue
}

r.ruleCacheMutex.Lock()
nodeEval := r.getOrCreateNodeEvaluation(rule, nodeName)
if nodeEval.TaintAppliedAt.IsZero() && !eval.TaintAppliedAt.IsZero() {
nodeEval.TaintAppliedAt = eval.TaintAppliedAt
}
if nodeEval.TaintObservedAt.IsZero() && !eval.TaintObservedAt.IsZero() {
nodeEval.TaintObservedAt = eval.TaintObservedAt
}

if cachedRule, ok := r.ruleCache[rule.Name]; ok {
cachedNodeEval := r.getOrCreateNodeEvaluation(cachedRule, nodeName)
if cachedNodeEval.TaintAppliedAt.IsZero() && !eval.TaintAppliedAt.IsZero() {
cachedNodeEval.TaintAppliedAt = eval.TaintAppliedAt
}
if cachedNodeEval.TaintObservedAt.IsZero() && !eval.TaintObservedAt.IsZero() {
cachedNodeEval.TaintObservedAt = eval.TaintObservedAt
}
}
r.ruleCacheMutex.Unlock()

log.V(4).Info("Recovered taint anchor(s) from API into stale cache entry",
"rule", rule.Name, "node", nodeName,
"taintAppliedAt", eval.TaintAppliedAt, "taintObservedAt", eval.TaintObservedAt)
return true
}
}
return false
}

// getConditionStatus gets the status of a condition on a node.
// If the condition is not present, defaultStatus is returned with found=false.
func (r *RuleReadinessController) getConditionStatus(
Expand Down
91 changes: 91 additions & 0 deletions internal/controller/node_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,97 @@ var _ = Describe("Node Controller", func() {
return false
}, time.Second*2).Should(BeFalse())
})

It("should not retry recovery for an adopted taint", func() {
// The node already has the taint, so the first reconcile takes the adopt path.
// TaintObservedAt is set while TaintAppliedAt stays zero.
_, err := nodeReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName})
Expect(err).NotTo(HaveOccurred())

updatedRule := &nodereadinessiov1alpha1.NodeReadinessRule{}
Expect(k8sClient.Get(ctx, types.NamespacedName{Name: ruleName}, updatedRule)).To(Succeed())

var eval *nodereadinessiov1alpha1.NodeEvaluation
for i := range updatedRule.Status.NodeEvaluations {
if updatedRule.Status.NodeEvaluations[i].NodeName == nodeName {
eval = &updatedRule.Status.NodeEvaluations[i]
}
}
Expect(eval).NotTo(BeNil())
Expect(eval.TaintAppliedAt.IsZero()).To(BeTrue(), "TaintAppliedAt must stay zero for an adopted taint")
Expect(eval.TaintObservedAt.IsZero()).To(BeFalse(), "TaintObservedAt must be stamped for an adopted taint")
Expect(readinessController.taintAnchorMissing(updatedRule, nodeName)).To(BeFalse())

// Refresh the cache and record the recovery attempts so far.
readinessController.ruleCache[ruleName] = updatedRule
readinessController.taintAnchorRecoveryMutex.Lock()
attemptsBefore := readinessController.taintAnchorRecoveryAttempts[ruleName+"/"+nodeName]
readinessController.taintAnchorRecoveryMutex.Unlock()

_, err = nodeReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName})
Expect(err).NotTo(HaveOccurred())

readinessController.taintAnchorRecoveryMutex.Lock()
attemptsAfter := readinessController.taintAnchorRecoveryAttempts[ruleName+"/"+nodeName]
readinessController.taintAnchorRecoveryMutex.Unlock()
Expect(attemptsAfter).To(Equal(attemptsBefore),
"no additional recovery attempt should have been made once TaintObservedAt is known")
})

It("should self-heal the rule cache after recovering the taint anchor", func() {
// Reconcile #1: adopt the existing taint and persist TaintObservedAt to the API.
_, err := nodeReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName})
Expect(err).NotTo(HaveOccurred())

readCachedEval := func() *nodereadinessiov1alpha1.NodeEvaluation {
readinessController.ruleCacheMutex.RLock()
defer readinessController.ruleCacheMutex.RUnlock()
cachedRule, ok := readinessController.ruleCache[ruleName]
if !ok {
return nil
}
for i := range cachedRule.Status.NodeEvaluations {
if cachedRule.Status.NodeEvaluations[i].NodeName == nodeName {
return &cachedRule.Status.NodeEvaluations[i]
}
}
return nil
}

Expect(readCachedEval()).To(BeNil(),
"the persistent cache should still be stale immediately after the adopt reconcile")

// Reconcile #2: recover TaintObservedAt from the API and update the cache.
_, err = nodeReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName})
Expect(err).NotTo(HaveOccurred())

cachedEval := readCachedEval()
Expect(cachedEval).NotTo(BeNil(),
"the persistent cache should now contain a NodeEvaluation for this node")
Expect(cachedEval.TaintObservedAt.IsZero()).To(BeFalse(),
"the recovered TaintObservedAt should have been written into r.ruleCache directly")
Expect(cachedEval.TaintAppliedAt.IsZero()).To(BeTrue())

readinessController.ruleCacheMutex.RLock()
cachedRule := readinessController.ruleCache[ruleName]
readinessController.ruleCacheMutex.RUnlock()
Expect(readinessController.taintAnchorMissing(cachedRule, nodeName)).To(BeFalse(),
"the self-healed cache entry must no longer report the anchor as missing")

readinessController.taintAnchorRecoveryMutex.Lock()
attemptsBefore := readinessController.taintAnchorRecoveryAttempts[ruleName+"/"+nodeName]
readinessController.taintAnchorRecoveryMutex.Unlock()

// Reconcile #3: the cache is already healed, so no further recovery is needed.
_, err = nodeReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName})
Expect(err).NotTo(HaveOccurred())

readinessController.taintAnchorRecoveryMutex.Lock()
attemptsAfter := readinessController.taintAnchorRecoveryAttempts[ruleName+"/"+nodeName]
readinessController.taintAnchorRecoveryMutex.Unlock()
Expect(attemptsAfter).To(Equal(attemptsBefore),
"no further recovery attempt should be needed once the cache has self-healed")
})
})

When("in continuous mode", func() {
Expand Down
Loading