Repository navigation
fix: Expire producer health reports a node stops renewing - #1139
Conversation
Each node writes the health of the Spread producers it hosts into the endpoint's status.producers, and every node routes on those entries. An entry stayed until its own node replaced it, so a node that went down, or whose galactic-router stopped, left its producers reported as they last were. A remote consumer kept sending new flows to a lost node's producers while their attachments still read ready, and a producer reported Unhealthy just before its router stopped stayed drained after it recovered. Each entry now carries renewTime. Every node renews its entries every 15 seconds, even when no decision changed, and skips the write while its entries are unchanged and younger than that. An entry is current for 60 seconds after its renewTime; after that, or with no renewTime, it is expired. When the planner and the programming plan pick producers, they read an expired entry this way: - It never drains a producer in a node's local set. - A remote consumer goes to the first producer currently reported Healthy or Unknown, or not reported at all. Failing that, it goes to the first producer whose entry expired, and then to the first producer, as it already did when every producer is Unhealthy. ServiceRoutePolicyReconciler requeues for the next entry expiry, so a node with no intents yet picks up a remote producer that moves onto it. It ignores endpoint updates that only renew entries still current, so the periodic renewal does not reconcile every policy on every node. A router without renewal writes no renewTime, so during a rollout, upgraded nodes rank the producers of nodes not yet upgraded after currently reported ones. A rolled-back router ignores the field. The private services guide documents the timing, the ordering and this rollout behavior. Resolves #1099
The ServiceEndpoint watch filter skipped any update that changed only the renewTime of current status.producers entries. An update that clears renewTime, as a rolled-back router's apply does, or that leaves it expired, turns a current entry into an expired one, and the filter treated it as a renewal, so the change waited for the next expiry requeue. The filter now passes an update unless every entry is current both before and after it. The ServiceEndpoint CRD is regenerated with the repository's controller-gen v0.21.0, so its version annotation matches the other CRDs. The private services guide states that the CRD has to be applied before galactic-router is upgraded, since the API server otherwise prunes renewTime from every report, and that node clocks have to stay in sync, since each node compares renewTime with its own clock. Related to #1099
|
Second-pass review on head 7b2c8f8 agreed. pr-rereviewer: VERDICT merge. It verified that all four first-pass findings are fixed: the CRD was regenerated with controller-gen v0.21.0 and the diff is only renewTime and descriptions; the CRD-before-router rollout order is documented; the watch filter passes an update unless every producer report is current in both the old and new object, and its test fails when the fix is reverted; clock sync is documented. pr-conventions-reviewer: VERDICT merge. Nits only: overlap with open PRs #845 and #1138 is limited to generated deepcopy and API doc files, so whichever lands second rebases and regenerates; the two commits stay split because each builds and tests alone and the repository refuses squash merges. Ran locally on the head's content: golangci-lint, yamlfmt lint, task build, task test:unit, task test:e2e and task docs:check-generated on the first commit; after the second commit, lint, race unit tests for internal/controller, internal/serviceroute and api/network/v1alpha1, docs:check-generated and yamlfmt again, all clean. No findings remained to apply in this pass. CI on 7b2c8f8: every check passed (build, lint, unit, root unit, e2e, docs, manifests, service API bundle, image and bundle publishes, CLA). Auto-merge is enabled with the merge method, the only one the ruleset allows. The ruleset requires one code-owner approval on the last push, so the PR waits for a human approval before it lands. |
Summary
Spread endpoints routed on producer health reports that never expired, so a lost node's producers kept receiving other nodes' consumers, and a producer reported Unhealthy just before its router stopped stayed drained after it recovered.
Each node now renews its reports every 15 seconds, and a report not renewed for 60 seconds is expired. An expired report drains nothing, and remote consumers go to a currently reported producer before one whose report expired.
Apply the updated CRD before upgrading the router, because the API server otherwise drops the new field and every report reads as expired.
Test plan
Fixes #1099