Skip to content

feat: Add selector and bootstrap observability metrics #286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rawadhossain wants to merge 1 commit into
base: main
Choose a base branch
from
+189 −0
Open

feat: Add selector and bootstrap observability metrics #286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions internal/controller/node_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,7 @@ func (r *RuleReadinessController) markBootstrapCompleted(ctx context.Context, no
switch {
case err != nil:
log.Error(err, "Failed to mark bootstrap completed", "node", nodeName, "rule", ruleName)
metrics.BootstrapCompletionErrors.WithLabelValues(ruleName).Inc()
case marked:
log.Info("Marked bootstrap completed", "node", nodeName, "rule", ruleName)
metrics.BootstrapCompleted.WithLabelValues(ruleName).Inc()
Expand Down
18 changes: 18 additions & 0 deletions internal/controller/nodereadinessrule_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,15 @@ func (r *RuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
// Update rule cache (after cleanup)
r.Controller.updateRuleCache(ctx, rule)

// Update selector_matched_nodes_total from the node list we already fetched.
var matchedCount int
for i := range nodeList.Items {

@AvineshTripathi AvineshTripathi Jul 4, 2026

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.

we already do the same thing inside processAllNodesForRule here

@rawadhossain rawadhossain Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I intentionally kept it before the dry-run check so the gauge gets updated for both dry-run and normal rules in one place. If I move it into processAllNodesForRule, dryrun rules would never update the metric as they go through processDryRun. Open to refactor it if you think splitting it across both paths is cleaner.

if r.Controller.ruleAppliesTo(ctx, rule, &nodeList.Items[i]) {
matchedCount++
}
}
metrics.SelectorMatchedNodes.WithLabelValues(rule.Name).Set(float64(matchedCount))

// Handle dry run
if rule.Spec.DryRun {
if err := r.Controller.processDryRun(ctx, rule, nodeList); err != nil {
Expand Down Expand Up @@ -209,6 +218,9 @@ func (r *RuleReconciler) reconcileDelete(ctx context.Context, rule *readinessv1a
metrics.RuleLastReconciliationTime.DeleteLabelValues(rule.Name)
metrics.BootstrapCompleted.DeleteLabelValues(rule.Name)
metrics.BootstrapDuration.DeleteLabelValues(rule.Name)
metrics.SelectorMatchedNodes.DeleteLabelValues(rule.Name)
metrics.BootstrapCompletionErrors.DeleteLabelValues(rule.Name)
metrics.BootstrapNRCDuration.DeleteLabelValues(rule.Name)

// For multi-label metrics, use DeletePartialMatch to wipe all combinations
metrics.NodesByState.DeletePartialMatch(ruleLabel)
Expand Down Expand Up @@ -419,6 +431,8 @@ func (r *RuleReadinessController) evaluateRuleForNode(ctx context.Context, rule
if duration > 0 {
metrics.BootstrapDuration.WithLabelValues(rule.Name).Observe(duration)
}
// TODO: pending decision — record BootstrapNRCDuration from readiness.k8s.io/taint-applied-<rule> to time.Now(); skip if missing.

@AvineshTripathi AvineshTripathi Jul 4, 2026

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.

This PR introduces a new format of bootstrap annotation where the value is a JSON. Instead of creating a new annotation readiness.k8s.io/taint-applied-<rule> we could just add a field in the json value.

@rawadhossain rawadhossain Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I like that approach better too. As it was still in discussion, I went with the annotation one.

Since #224 is still pending, I'll remove bootstrap_nrc_duration_seconds metric from here entirely and implement JSON value at taint then open a PR once #224 merges. So the other two metrics are not blocked.

// Alternative: dedicated status condition (needs nodes/status RBAC).
} else {
log.V(4).Info("Skipping bootstrap duration metric for legacy node or missing transition",
"node", node.Name,
Expand All @@ -437,8 +451,12 @@ func (r *RuleReadinessController) evaluateRuleForNode(ctx context.Context, rule
// Record add taint latency and taint operation counter
metrics.TaintOperations.WithLabelValues(rule.Name, "add").Inc()
recordLatency("add_taint")
// TODO: pending decision — on first taint add, patch readiness.k8s.io/taint-applied-<rule> with time.Now() when absent (same r.Patch(), no extra RBAC).
// Alternative: dedicated status condition (needs nodes/status RBAC).

case !shouldRemoveTaint && currentlyHasTaint:
// Adopted taints were already on the node — we never wrote taint-applied-<rule>,
// so BootstrapNRCDuration is skipped at completion.
if isFirstEvaluation {
log.Info("Adopting pre-existing taint", "node", node.Name, "rule", rule.Name, "taint", rule.Spec.Taint.Key)

Expand Down
138 changes: 138 additions & 0 deletions internal/controller/nodereadinessrule_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ func histogramSampleCount(histogram interface{ Write(*dto.Metric) error }) uint6
return metric.GetHistogram().GetSampleCount()
}

func gaugeValue(gauge interface{ Write(*dto.Metric) error }) float64 {
metric := &dto.Metric{}
Expect(gauge.Write(metric)).To(Succeed())
return metric.GetGauge().GetValue()
}

// errorInjectingClient forces Patch to fail for selected nodes.
type errorInjectingClient struct {
client.Client
Expand Down Expand Up @@ -2229,4 +2235,136 @@ var _ = Describe("NodeReadinessRule Controller", func() {
Expect(failedNames).NotTo(ContainElement("stale-recovery-node"))
})
})

Context("Metric: selector_matched_nodes_total", func() {
It("should set gauge to matched node count after reconcile", func() {
ruleName := "sel-gauge-match-rule"
matchLabel := "sel-gauge-match"

nodeA := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "sel-gauge-node-a", Labels: map[string]string{matchLabel: "true"}}}
nodeB := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "sel-gauge-node-b", Labels: map[string]string{matchLabel: "true"}}}
nodeC := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "sel-gauge-node-c", Labels: map[string]string{"other": "label"}}}
for _, n := range []*corev1.Node{nodeA, nodeB, nodeC} {
Expect(k8sClient.Create(ctx, n)).To(Succeed())
}
defer func() {
for _, n := range []*corev1.Node{nodeA, nodeB, nodeC} {
_ = k8sClient.Delete(ctx, n)
}
}()

rule := &nodereadinessiov1alpha1.NodeReadinessRule{
ObjectMeta: metav1.ObjectMeta{Name: ruleName, Finalizers: []string{finalizerName}},
Spec: nodereadinessiov1alpha1.NodeReadinessRuleSpec{
Conditions: []nodereadinessiov1alpha1.ConditionRequirement{{Type: "Ready", RequiredStatus: corev1.ConditionTrue}},
Taint: corev1.Taint{Key: "readiness.k8s.io/sel-gauge-taint", Effect: corev1.TaintEffectNoSchedule},
NodeSelector: metav1.LabelSelector{MatchLabels: map[string]string{matchLabel: "true"}},
EnforcementMode: nodereadinessiov1alpha1.EnforcementModeContinuous,
},
}
Expect(k8sClient.Create(ctx, rule)).To(Succeed())
defer func() { _ = k8sClient.Delete(ctx, rule) }()

_, err := ruleReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: ruleName}})
Expect(err).NotTo(HaveOccurred())

gauge, err := metrics.SelectorMatchedNodes.GetMetricWith(prometheus.Labels{"rule": ruleName})
Expect(err).NotTo(HaveOccurred())
Expect(gaugeValue(gauge)).To(Equal(2.0))
})

It("should set gauge to 0 when no nodes match the selector", func() {
ruleName := "sel-gauge-nomatch-rule"

rule := &nodereadinessiov1alpha1.NodeReadinessRule{
ObjectMeta: metav1.ObjectMeta{Name: ruleName, Finalizers: []string{finalizerName}},
Spec: nodereadinessiov1alpha1.NodeReadinessRuleSpec{
Conditions: []nodereadinessiov1alpha1.ConditionRequirement{{Type: "Ready", RequiredStatus: corev1.ConditionTrue}},
Taint: corev1.Taint{Key: "readiness.k8s.io/sel-nomatch-taint", Effect: corev1.TaintEffectNoSchedule},
NodeSelector: metav1.LabelSelector{MatchLabels: map[string]string{"definitely-no-such-label": "true"}},
EnforcementMode: nodereadinessiov1alpha1.EnforcementModeContinuous,
},
}
Expect(k8sClient.Create(ctx, rule)).To(Succeed())
defer func() { _ = k8sClient.Delete(ctx, rule) }()

_, err := ruleReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: ruleName}})
Expect(err).NotTo(HaveOccurred())

gauge, err := metrics.SelectorMatchedNodes.GetMetricWith(prometheus.Labels{"rule": ruleName})
Expect(err).NotTo(HaveOccurred())
Expect(gaugeValue(gauge)).To(Equal(0.0))
})

It("should clean up label values on rule deletion", func() {
ruleName := "sel-gauge-del-rule"
matchLabel := "sel-gauge-del"

node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "sel-gauge-del-node", Labels: map[string]string{matchLabel: "true"}}}
Expect(k8sClient.Create(ctx, node)).To(Succeed())
defer func() { _ = k8sClient.Delete(ctx, node) }()

rule := &nodereadinessiov1alpha1.NodeReadinessRule{
ObjectMeta: metav1.ObjectMeta{Name: ruleName, Finalizers: []string{finalizerName}},
Spec: nodereadinessiov1alpha1.NodeReadinessRuleSpec{
Conditions: []nodereadinessiov1alpha1.ConditionRequirement{{Type: "Ready", RequiredStatus: corev1.ConditionTrue}},
Taint: corev1.Taint{Key: "readiness.k8s.io/sel-del-taint", Effect: corev1.TaintEffectNoSchedule},
NodeSelector: metav1.LabelSelector{MatchLabels: map[string]string{matchLabel: "true"}},
EnforcementMode: nodereadinessiov1alpha1.EnforcementModeContinuous,
},
}
Expect(k8sClient.Create(ctx, rule)).To(Succeed())

_, err := ruleReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: ruleName}})
Expect(err).NotTo(HaveOccurred())

gauge, err := metrics.SelectorMatchedNodes.GetMetricWith(prometheus.Labels{"rule": ruleName})
Expect(err).NotTo(HaveOccurred())
Expect(gaugeValue(gauge)).To(Equal(1.0), "gauge should be 1 before deletion")

Expect(k8sClient.Delete(ctx, rule)).To(Succeed())

// Deletion reconcile triggers reconcileDelete, which calls DeleteLabelValues.
Eventually(func() bool {
_, err := ruleReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Name: ruleName}})
Expect(err).NotTo(HaveOccurred())
readinessController.ruleCacheMutex.RLock()
_, exists := readinessController.ruleCache[ruleName]
readinessController.ruleCacheMutex.RUnlock()
return !exists
}).Should(BeTrue())

// After DeleteLabelValues, GetMetricWith allocates a fresh gauge at 0 (old value removed).
freshGauge, err := metrics.SelectorMatchedNodes.GetMetricWith(prometheus.Labels{"rule": ruleName})
Expect(err).NotTo(HaveOccurred())
Expect(gaugeValue(freshGauge)).To(Equal(0.0))
})
})

Context("Metric: bootstrap_completion_errors_total", func() {
It("should increment counter when annotation write fails", func() {
ruleName := "bce-error-rule"

before := counterValue(metrics.BootstrapCompletionErrors.WithLabelValues(ruleName))

readinessController.markBootstrapCompleted(ctx, "nonexistent-node-for-bce-test", ruleName)

Expect(counterValue(metrics.BootstrapCompletionErrors.WithLabelValues(ruleName))).To(Equal(before + 1))
})

It("should not increment counter on successful annotation write", func() {
nodeName := "bce-success-node"
ruleName := "bce-success-rule"

node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}
Expect(k8sClient.Create(ctx, node)).To(Succeed())
defer func() { _ = k8sClient.Delete(ctx, node) }()

before := counterValue(metrics.BootstrapCompletionErrors.WithLabelValues(ruleName))

readinessController.markBootstrapCompleted(ctx, nodeName, ruleName)

Expect(counterValue(metrics.BootstrapCompletionErrors.WithLabelValues(ruleName))).To(Equal(before))
})
})
})
32 changes: 32 additions & 0 deletions internal/metrics/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,35 @@ var (
},
[]string{"rule"},
)

// SelectorMatchedNodes tracks how many nodes match a rule's NodeSelector.
SelectorMatchedNodes = prometheus.NewGaugeVec(
prometheus.GaugeOpts{
Name: "node_readiness_selector_matched_nodes_total",
Help: "Number of nodes matched by a rule's NodeSelector",
},
[]string{"rule"},
)

// BootstrapCompletionErrors tracks failures writing the bootstrap-completion annotation.
BootstrapCompletionErrors = prometheus.NewCounterVec(

@AvineshTripathi AvineshTripathi Jul 4, 2026

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.

I couldn't find this in the doc. Where are we planning to use these metrics? I feel counter without reason of failure won't be that useful. Please correct me if I am understanding this wrongly

@rawadhossain rawadhossain Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes it wasnt there in the doc, I actually found this gap while implementing the metrics.

Right now, if writing the bootstrap annotation fails after all retries are exhausted, the node gets excluded from bootstrap_duration_seconds with no signal anywhere. This counter makes that failure visible.
Good point on the reason label, I’ll add it so it's explicit and easy to extend later if needed.

prometheus.CounterOpts{
Name: "node_readiness_bootstrap_completion_errors_total",
Help: "Total failures writing the bootstrap-completion annotation",
},
[]string{"rule"},
)

// BootstrapNRCDuration tracks time from NRC taint application to bootstrap completion.
// Unlike bootstrap_duration_seconds, this excludes pre-NRC node boot time.
BootstrapNRCDuration = prometheus.NewHistogramVec(

@AvineshTripathi AvineshTripathi Jul 4, 2026

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.

missing implementation here or is this the pending decision one?

@rawadhossain rawadhossain Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes its the pending one, was waiting for the decision.

Btw I think it's cleaner to remove this metric from the PR for now and open a separate PR once PR #224 merges. what do you say?

prometheus.HistogramOpts{
Name: "node_readiness_bootstrap_nrc_duration_seconds",
Help: "Time from NRC taint application to bootstrap completion for bootstrap-only rules",
Buckets: []float64{1, 5, 10, 30, 60, 120, 300, 600, 1200}, // 1s to 20min
},
[]string{"rule"},
)
)

func init() {
Expand All @@ -131,4 +160,7 @@ func init() {
metrics.Registry.MustRegister(NodesByState)
metrics.Registry.MustRegister(ConditionEvaluationFailures)
metrics.Registry.MustRegister(RuleLastReconciliationTime)
metrics.Registry.MustRegister(SelectorMatchedNodes)
metrics.Registry.MustRegister(BootstrapCompletionErrors)
metrics.Registry.MustRegister(BootstrapNRCDuration)
}