docs: add observability design - #344
Conversation
TAG=agy CONV=196812f1-9aa8-42af-991d-51a74392b7bd
kubernetes-prow
Bot
added
kind/documentation
|
@ajaysundark: The label(s) DetailsIn response to this:
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-sigs/prow repository. |
✅ Deploy Preview for node-readiness-controller canceled.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ajaysundark The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
cncf-cla: yes
kubernetes-prow
Bot
added
the
approved
kubernetes-prow
Bot
added
the
size/L
|
/cc @rawadhossain |
|
@ajaysundark: GitHub didn't allow me to request PR reviews from the following users: rawadhossain. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
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-sigs/prow repository. |
Karthik-K-N
left a comment
Karthik-K-N
left a comment
There was a problem hiding this comment.
can we move this /docs/proposals
so in future we can keep all proposals at one place?
|
|
||
| ## Design | ||
|
|
||
| ### 1. Personas |
There was a problem hiding this comment.
Should we add user stories like personas?
There was a problem hiding this comment.
User-stories felt bit redundant to me. I captured as 'questions' in SLIs/SLOs table below. Do you think I could may be extract that out at the top?
There was a problem hiding this comment.
Yeah, What I thought is if any reader goes through this doc they can relate themself to any user story so the expectations may be clear.
|
|
||
| * Add granular per-rule metrics. | ||
| * Build a Grafana dashboard for fleet-wide SLOs. | ||
|
|
There was a problem hiding this comment.
we are also building Scrape-Time Collector, isn't a goal of this exercise?
There was a problem hiding this comment.
yes, that's a good point. it could be a goal.
|
|
||
| --- | ||
|
|
||
| ### 4. New Scrape-Time Collector Surface |
There was a problem hiding this comment.
is it mandatory requirement or performance improvement?
There was a problem hiding this comment.
Not measured individually. But I had few other optimizations like batched status updates (something similar to endpointslice-updates-batch-period in kcm, and it showed significant perf difference in my 5k+ node testing.
I'll add it in my list to present it to one of our discussions
AvineshTripathi
left a comment
AvineshTripathi
left a comment
There was a problem hiding this comment.
added some questions, rest lgtm
| We design observability around three distinct personas. Each persona asks different questions and looks at different interfaces. | ||
|
|
||
| 1. **Infrastructure Owners (Cluster Operators)**: These operators run the cluster and manage the node lifecycle. They need to know if the fleet is healthy and if the controller is working. | ||
| 2. **Component / Rule Owners**: These are teams that own the infrastructure components that gate node readiness, like CNI plugins or GPU drivers. They need to know whether / which specific check is failing. |
There was a problem hiding this comment.
| 2. **Component / Rule Owners**: These are teams that own the infrastructure components that gate node readiness, like CNI plugins or GPU drivers. They need to know whether / which specific check is failing. | |
| 2. **Component / Rule Owners**: These are teams that own the infrastructure components that gate node readiness, like CNI plugins or GPU drivers. They need to know whether / which specific component is delaying the readiness. |
A check could be a generic check. I am fine with either but feel above suggestion fits more. Thoughts?
There was a problem hiding this comment.
yeah, that reads a little better, but also underwhelms the need for same rule granularity signals.
For instance, I was thinking to capture that when multiple conditions in a rule map to a network readiness-rule, the rule owner would also want to distinguish whether the latency is from IPAMReady or CNIReady condition -- note, kube_node_status_condition (KSM) already gives this per-condition observability,.
^^May be identifying 'rule owner' identical to 'component owner' is not the best fit. As a rule may include multiple components as we support multiple conditions.
There was a problem hiding this comment.
That's a valid point. How about component(s) or one or more components maybe
|
|
||
| #### Metric vs status vs event vs log decisions | ||
| * Metrics to track aggregate cluster rates, durations, and counts over time. NRC will never use node names as metric labels for scale concerns. | ||
| * Custom Resource status (`NodeReadinessRule.status`) will hold current rule health and evaluation summaries. New `NodeReadinessEvaluation` api will also be implemented for the individual node readiness status. |
There was a problem hiding this comment.
are we referring NodeReadinessRule.status to this or the new api?
There was a problem hiding this comment.
both. This is recommending us to follow best-practices[1] for including a metav1.Condition[] in rule.status; I'm imagining something like "Progressing" to show rule reconciliation in-progress, "Ready" when complete.. This is helpful to tell controller's status on whether there are still some nodes in the process queue for handling a rule.
@Karthik-K-N also brought a point on the need to align this convention, to help operators to use standard workflow like:
kubectl wait --for=condition=ready node-readiness-rule ..
I agree it would be necessary for us to also define these here in a document. But I wanted to get the initial thoughts out and add more details to agree on the 'states'
[1] ref-
|
|
||
| ### 2. Signals | ||
|
|
||
| We identify four telemetry surface in NRC: metric, status, event, or log and will follow below principles to assign each observability need to the right channel. |
There was a problem hiding this comment.
do we consider annotations as a signal for bootstraping currently?
There was a problem hiding this comment.
That's a valid question maybe worth even clarifying at the doc. Annotation is 'metadata' and not observability signal. This is suppose to be treated only as internal state of controller. ie: no user can design a system on top based on the annotation we maintain as that could change.
| | **Rule discovery (count, mode)** | Metric (`node_readiness_rules`) + `kubectl get nrr` | While `kubectl get` provides an API-level list, this gauge gives operators a controller-side view. The existing `node_readiness_rules_total` gauge violates naming conventions and ignores rule modes. We deprecate it in favor of `node_readiness_rules` without the `_total` suffix, adding labels to slice by `enforcement_mode` and `dry_run`. | | ||
| | **Selector match (nodes grouped per rule)** | Metric (`node_readiness_rule_matched_nodes`) | Tracks how many nodes match each rule selector. Because we are removing per-node lists from `NodeReadinessRule.status` for scalability, this scrape-time gauge collector preserves visibility into selector reach. A zero value signals a misconfigured selector. | | ||
| | **Nodes held or released per rule** | Metric (`node_readiness_rule_nodes`) | Shows the direct capacity impact of readiness gating on the cluster. The existing `node_readiness_nodes_by_state` gauge requires scale testing and tuning. We replace it with an always-on scrape-time collector that reads ground-truth `spec.taints` directly from the node cache. | | ||
| | **Blocking conditions per rule** | Metric (`node_readiness_blocked_nodes`) | Assesses which infrastructure component is blocking nodes across the fleet. Today, `node_readiness_condition_failures_total` counts an unsatisfied condition during normal bootstrapping as a failure, which inflates rates with every reconcile. We replace it with a `node_readiness_blocked_nodes{rule,condition}` gauge. Per-node drill-downs will live in the upcoming `NodeReadinessEvaluation` CRD. | |
There was a problem hiding this comment.
This proposal aims to further break down the earlier node_readiness_nodes_by_state to three more explicit per-node interested metrics:
- node_readiness_rule_matched_nodes (how nodes are mapped against our rules)
- node_readiness_rule_nodes (what are their current status as seen by controller - held vs released as opposed to ‘ready, not_ready, bootstrapping’ as it felt we are conflating with kubelet’s Ready. Also, bootstrapping doesn’t apply for ‘continuous’)
- node_readiness_blocked_nodes (what is pending on “held”)
| | `node_readiness_blocked_nodes` | gauge | `rule`, `condition` | Number of nodes blocked by each required condition. | ALPHA | | ||
| | `node_readiness_taint_operations_total` | counter | `rule`, `operation` (`add` or `remove`) | Total taint additions and removals performed by the controller. | BETA (STABLE candidate) | | ||
| | `node_readiness_failures_total` | counter | `rule`, `reason` | Total operational failures across evaluation, taint operations, and status patches. | BETA | | ||
| | `node_readiness_api_conflicts_total` | counter | `rule`, `operation` | Number of API write conflicts per retry attempt. | ALPHA | |
There was a problem hiding this comment.
is this needed? acc to the discussion here i think this can go as logs instead. wdyt?
| |---|---|---|---|---| | ||
| | `node_readiness_reporter_build_info` | gauge | `version` | Reporter binary version to track fleet version skew. | ALPHA | | ||
| | `node_readiness_reporter_check_duration_seconds` | histogram | none | Duration of health probe checks. | ALPHA | | ||
| | `node_readiness_reporter_checks_total` | counter | `result` (`healthy`, `unhealthy`, or `error`) | Total probe check results over time. | ALPHA | |
There was a problem hiding this comment.
will have labels (healthy, unhealthy, or error)?
There was a problem hiding this comment.
yes, I'm thinking they are good representative state generalizable across other reporters (eg: NRD) in the future as well. I am not sure whether more granular states / categorical counts would be needed for the http health-checks, but these three seemed good to be a starting point. This is currently proposed ALPHA so we could come back to analyze this again in the future when we decide promote to beta.
rawadhossain
left a comment
rawadhossain
left a comment
There was a problem hiding this comment.
Thanks for putting this together! left a few questions. Overall looks good to me.
| | `node_readiness_bootstrap_duration_seconds` | Rename and reshape | Migrate into `node_readiness_bootstrap_hold_duration_seconds{rule, taint_origin="adopted"}` histogram. | | ||
| | `node_readiness_reconciliation_latency_seconds` | Rename and reshape | Rename to `node_readiness_enforcement_latency_seconds` to prevent collision with `controller-runtime` reconcile metrics. Normalize operation values to `add` and `remove`. | | ||
| | `node_readiness_condition_failures_total` | Delete | Replace state-as-counter metric with the `node_readiness_blocked_nodes` gauge. | | ||
| | `node_readiness_rule_last_reconciliation_timestamp_seconds` | Delete | Remove timestamp gauge. Unchanged rules are still actively enforced under `GenerationChangedPredicate` without resyncs. | |
There was a problem hiding this comment.
right now this is metric (node_readiness_rule_last_reconciliation_timestamp_seconds) exists for every rule, even if a rule has zero matching nodes, and some of our tooling relies on that. If we remove it, is there another metric that provides the same guarantee, or is that something we need to figure out?
| |---|---|---|---|---| | ||
| | `node_readiness_build_info` | gauge | `version` | Controller binary version for managed-export monitoring. | ALPHA | | ||
| | `node_readiness_rules` | gauge | `enforcement_mode`, `dry_run` | Number of rules by enforcement mode and dry-run state. | BETA | | ||
| | `node_readiness_rule_matched_nodes` | gauge | `rule` | Number of nodes matching the rule selector. | BETA | |
There was a problem hiding this comment.
Since this metric (node_readiness_rule_matched_nodes) is tied to the new collector, should this wait until the collector is added, or is the idea to implement it using the current approach first and migrate it later?
|
|
||
| | Persona | Question | SLI / SLO | Alerts and health checks | | ||
| |---|---|---|---| | ||
| | NRC cluster operators | Are there nodes stuck in a bootstrapping state? | **Nodes held:** `sum by (rule) (node_readiness_rule_nodes{state="held"})` | **Node Capacity held:** `sum(node_readiness_rule_nodes{state="held"}) > 0` and zero bootstrap completions for 30m.<br><br>**Error budget:** `rate(node_readiness_failures_total[10m]) > 0` for 15m.<br><br>**Controller health:** `up == 0`, `workqueue_depth > 1000`. | |
There was a problem hiding this comment.
Would it make more sense for this to scale with fleet size or MaxConcurrentReconciles, rather than a fixed value? Queue depth can vary quite a bit depending on how many nodes enter the queue at once, so 1000 might mean different things for different cluster sizes. wdyt?
|
two things from the code side that might be worth folding into the doc. the scrape-time collector reading second, |
5 participants
Description
Add Observability Design Document for Node Readiness Controller
Assisted by: AGY / Gemini
Related Issue
None
Type of Change
/kind documentation
/kind feature
/kind design
Testing
NA
Checklist
make testpassesmake lintpassesDoes this PR introduce a user-facing change?