Make healthcheck consistent chunked - #2162
Conversation
/healthz deliberately reports liveness only, so a controller that is alive but failing every sync is invisible to it. These two metrics, controller_sync_last_success and controller_sync_failures_total, are the signal for that case, and RecordSyncResult is the single entry point the controllers will report through. Both series are created on the first report either way, so a controller that has never succeeded still shows up at 0 instead of matching an empty vector.
Previously fullPolicySync logged its failures and returned nothing, so Run had no outcome to report and a failed sync silently skipped its heartbeat. Both implementations now return their abort paths, and the shared syncAndReport wrapper beats at sync start and end regardless of the outcome, recording the result through metrics.RecordSyncResult instead of gating liveness on it. The nftables implementation additionally records per-policy transaction failures, which deliberately don't abort the sync (their pods fail closed), so a partial failure still counts as a failed sync.
|
| Filename | Overview |
|---|---|
| pkg/healthcheck/health_controller.go | Introduces the shared unconditional start/end heartbeat wrapper. |
| pkg/metrics/metrics_controller.go | Adds centralized synchronization outcome reporting around the heartbeat wrapper. |
| pkg/controllers/proxy/network_services_controller.go | Covers initial, requested, and periodic proxy reconciliations with consistent heartbeat and error reporting. |
| pkg/controllers/routing/network_routes_controller.go | Extracts periodic routing reconciliation and aggregates failures across all reconciliation stages. |
| pkg/controllers/netpol/npc_iptables.go | Returns policy-sync failures and reports them through the shared observed-sync path. |
| pkg/controllers/netpol/npc_nftables.go | Records partial per-policy transaction failures while preserving fail-closed programming. |
| pkg/controllers/lballoc/lballoc.go | Routes allocator walks through the common dual-heartbeat contract. |
| pkg/routes/route_sync.go | Applies centralized heartbeat and metric reporting to route synchronization. |
| docs/health.md | Documents that health checks measure loop liveness rather than reconciliation correctness. |
| docs/metrics.md | Documents sync-success and failure metrics with example alerts. |
Reviews (4): Last reviewed commit: "fact(health): consolidate RunSync logic" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Fixes: #2163 |
syncIpvsServices and its setup helpers used to log failures and return nil, so the controller reported a clean sync no matter what the kernel actually accepted. Every step now joins its failures rather than dropping them or bailing on the first one, so one broken Service can't keep the rest from programming while the sync still records as failed. With a real outcome to report, Run now records it through metrics.RecordSyncResult and beats at both sync start and end unconditionally. The initial sync also comes up degraded rather than fatal, because a restart can't fix a rejected service, and it beats at both ends too, so a slow-but-progressing first sync on a large cluster isn't mistaken for a wedged loop.
This finishes standardizing what a heartbeat means: the main syncing loop is not wedged. NRC, the route syncer, and the load balancer allocator now all beat at iteration start and end unconditionally, because gating the beat on sync success let a stuck BGP peer or an unreplaceable route get the container killed over a problem a restart can't fix. Sync outcomes are reported through the sync metrics instead. Along the way advertiseVIPs, withdrawVIPs and syncInternalPeers return their failures rather than dropping them, syncInternalPeers defers peer deletion while any Node fails to parse (so a transient address outage can't tear down a working peer), and failed peer deletions stay in activeNodes so a later pass retries them. Also documents the new heartbeat contract and the accompanying alert guidance in docs/health.md, and adds tests for the health controller's staleness windows.
ea7ff91 to
2e525b7
Compare
| This is worth being explicit about, because the two shipped daemonsets wire `/healthz` as a **livenessProbe** with | ||
| `periodSeconds: 3` and `failureThreshold: 3`. Roughly nine seconds of unhealthy and the kubelet kills the container, | ||
| so what a heartbeat means decides when kube-router gets restarted. |
There was a problem hiding this comment.
My gut feeling says this belongs into the respective DaemonSets as a comment? Feels a bit off as the first paragraph in this section? I'd try to detangle the whole section a bit from the concrete Pod's livenessProbe details, or at least defer those things into a separate section at the end of this doc. "What it means for the kube-router DaemonSet" or something along those lines.
There was a problem hiding this comment.
I added a comment to each of the daemonsets we provide that contained a livenessProbe.
| // syncAndReport runs one full policy sync, beating at both the start and the end so the health | ||
| // deadline budgets a full sync period per iteration instead of the first sync's duration, and | ||
| // records the outcome unconditionally so the beats assert only that this goroutine isn't wedged | ||
| func (npc *NetworkPolicyControllerBase) syncAndReport(fullPolicySync func() error) { |
There was a problem hiding this comment.
I'd rather have expected such a helper in the healthcheck package à la
func RunWithBeats(ch, component, f) {
healthcheck.SendHeartBeat(ch, component)
defer healthcheck.SendHeartBeat(ch, component)
f()
}
There was a problem hiding this comment.
This is a really good point and something I honestly should have caught, especially since the original implementation had the same core set of logic strewn across the code base.
I've now consolidated the logic into a healthcheck.RunSync() which does the heart-beating surrounding a sync. And a metrics.RunObservedSync() which calls RecordSyncResult() after a healthcheck.RunSync().
As a bonus this also forced many of the more lengthy select clauses in some controllers to actually get a function written that encapsulates the logic, something that I haven't liked for a bit now.
| klog.Errorf("Aborting sync. Failed to run iptables-restore: %v\n%s", | ||
| err.Error(), npc.filterTableRules[ipFamily].String()) | ||
| return | ||
| klog.Errorf("iptables-restore for %v failed against the following rule set:\n%s", |
There was a problem hiding this comment.
Wouldn't this end up twice in the logs? I'm assuming that the caller will log the returned error eventually.
There was a problem hiding this comment.
It does sorta go into the logs twice, but I like this one because it includes the full tables rule that failed whereas the upper error does not.
So I kept it, but I rephrased the error just a bit to make it more apparent what the value of it is.
This consolidates the RunSync() logic into the healthcontroller so that the pattern of beat -> sync -> beat is codified rather than allowing it to be strewn across the code base. Additionally, we add another function from the metrics package that is called RunObservedSync() which codifies the pattern of publishing metrics about the sync after the RunSync() function has been called.
298b30e to
8a61466
Compare
2 participants
FYI @twz123 / @rbrtbnfgl
What type of PR is this?
bug
What this PR does / why we need it:
Standardizes what a heartbeat means across NPC, NSC, NRC, the route syncer, and the load balancer allocator: every
controller now beats at iteration start and end, unconditionally, instead of skipping the beat when a sync failed.
Gating liveness on sync success was turning unfixable problems (a stuck BGP peer, a rejected Service, an
unreplaceable route) into a CrashLoopBackOff, which takes kube-router off the node instead of leaving a
stale-but-working rule set in place.
Sync outcomes are no longer silently dropped. They're reported through two new metrics,
kube_router_controller_sync_last_success and kube_router_controller_sync_failures_total, via a single
RecordSyncResult entry point. docs/health.md documents the new contract and includes alert guidance for the
staleness case /healthz intentionally no longer catches.
Also fixes a few related bugs found along the way: advertiseVIPs/withdrawVIPs/syncInternalPeers now return
their failures instead of dropping them, and syncInternalPeers defers peer deletion while any Node fails to
parse so a transient address outage can't tear down a working peer.
Which issue(s) this PR is related to:
None
Was AI used during the creation of this PR?
failure-propagation approach were specified up front, Claude worked out the per-controller implementation
What, if any, amount of integration testing was done with this change in a Kubernetes environment?
Deployed to a live cluster and validated failure scenarios directly: failed a controller's sync and confirmed
kube_router_controller_sync_failures_total incremented while /healthz kept returning healthy for that controller.
Does this PR introduce a breaking change?
Anything else the reviewer should know that wasn't already covered?
docs/health.md has a new section spelling out exactly what a heartbeat does and doesn't assert, plus sample
Prometheus alert rules for the two new metrics - worth a read before reviewing the code changes.