net: finish the backend-readiness gate follow-ups from #603
- Dominant language
- Go
- Stars
- 28
- Forks
- 11
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 55
Description
Follow-ups from the review of #603, which gated the net admission webhooks and
the aggregated `APIService` on the net controller actually serving.
These four were deliberately deferred from that PR: each needs design work or
touches the render pipeline, and #603 was already large. They are grouped here
because they are small individually and share context, so one person should do
them together.
Line references are against `main` after #603 merged.
---
## 1. The net controller can still sit "leader but never functionally ready"
#603 made Service endpoint publication conditional on the site controller
reaching `Ready()`, and made `NewSiteController` failure fatal, which covers the
common trigger. What is still uncovered is `siteCtrl.Run` never returning:
- `internal/net/controller/site_controller.go:788` blocks in
`cache.WaitForCacheSync`. If an informer never syncs, `Run` never returns and
`Ready()` never closes.
- `cmd/unbounded-net-controller/main.go` waits on `siteCtrl.Ready()` or
`ctx.Done()` with no third case, so `onReady` is never called.
- Result: the leader publishes no endpoint, the operator withholds all three
registrations indefinitely, the pod still passes its readiness probe
(readiness is deliberately leadership-independent), and nothing restarts it.
This is rarer on the operator-managed path than it first appears, because
`BootstrapCRDs` applies and waits for `Established` on every required CRD before
the manager starts (`internal/operator/bootstrap.go:47-79`,
`cmd/unbounded-operator/main.go:180` versus `:184`/`:262`). It remains reachable
via `make -C hack/net deploy-direct`, which applies only `deploy/net/crd/`
(`hack/net/Makefile:55-56`) while the Site CRD ships with machina, and via
out-of-band CRD deletion.
**Do:**
- Add a bounded wait to the `onReady` watcher in `main.go`: a third
`case <-time.After(siteControllerReadyTimeout)` that logs loudly and exits so
the pod restarts and the lease moves. Suggested default 5 minutes, plumbed
through `config.Config` alongside the other leader-election timings.
- Make `siteCtrl.Run` returning an error exit the process too, but only when
`ctx.Err() == nil`, so graceful shutdown stays silent.
- Add a `controller_ready` gauge next to `leader_is_leader`
(`cmd/unbounded-net-controller/metrics.go:42`) so "leader but not functionally
ready" is alertable rather than only visible as `NetReady=False` on the Site.
**Acceptance:** a `runAsLeader` whose `runFunc` never calls `onReady` reaches the
timeout path in a unit test; cancellation before readiness does not trigger it.
---
## 2. `controller.healthPort` is not actually configurable
`deploy/net/01-configmap.yaml.tmpl:19` exposes `controller.healthPort`, and the
operator preserves user edits to that ConfigMap
(`TestEnsureConfigPreservesExistingPayload`). The controller honours it: the
HTTPS listener binds `cfg.HealthPort` (`cmd/unbounded-net-controller/server.go:136-138`)
and the published endpoints carry it (`health_state.go:265`).
But every object that points at the Service hardcodes 9999:
- `deploy/net/controller/03-deployment.yaml.tmpl:125` (containerPort)
- `deploy/net/controller/04-service.yaml.tmpl:18-19`
- `deploy/net/controller/06-validatingwebhook.yaml.tmpl:24`
- `deploy/net/controller/07-apiservice.yaml.tmpl:21`
- `deploy/net/controller/08-mutatingwebhook.yaml.tmpl:25`
So setting the field moves the listener and breaks every caller. #603 removed
the operator-side symptom by matching the endpoint port by name rather than
number, but the underlying inconsistency remains.
**Do** one of:
- Template the port into all of the above from the same source as
`01-configmap.yaml.tmpl`, so the field works end to end, **or**
- Remove or document `controller.healthPort` as fixed, and drop the plumbing.
Templating is preferred if the render pipeline can carry the value cleanly;
otherwise documenting it as fixed is the honest answer. Either way add a test
pinning the manifests and the config default to the same value, in the style of
the existing drift-guard tests.
---
## 3. Cleanups left behind by #603
Each is a few lines.
- **Dead `isReady`.** `cmd/unbounded-net-controller/health_state.go` still
declares `isReady`, which has no production caller after `server.go:167`
switched to `readinessStatus`. Delete it and point
`TestHealthStateHelpersAndLeaderInfo` and `TestHealthStateReadinessFailure` at
`readinessStatus` directly.
- **Buried timing defaults.** `publishServiceEndpoints` in `health_state.go`
defaults `endpointRetryPeriod` to 1s and `endpointRefreshPeriod` to 30s inside
the function. Nothing in production sets those fields, so those buried
literals are the real production values. Promote them to named package
constants.
- **Leftover test scaffolding.** `h.setLeader(true)` in
`TestHealthStateHelpersAndLeaderInfo` and `TestHealthStateReadinessFailure`
(`health_state_test.go`), and `health.setLeader(true)` in
`TestRegisterProbeHandlers` (`server_routes_test.go`), are no-ops: readiness is
deliberately independent of leadership. They are residue from the intermediate
commit reverted by `3b016e8a` and imply a dependency that does not exist.
Remove them.
Do **not** remove the `setLeader` calls in
`TestHealthStateReadinessIsIndependentOfLeadership` or the `setLeader(false)`
in `TestRegisterProbeHandlers`: those are load-bearing assertions about
leadership loss clearing `controllerReady` and about standby replicas staying
Ready.
---
## 4. Document the Site component conditions
There is no reference page for Site component conditions today;
`docs/content/reference/` covers only `workload-overrides.md` for operator
behaviour. #603 introduced user-visible surface that is currently undocumented:
- The `BackendNotReady` reason on `NetReady`
(`internal/operator/component/result.go`), what it means, and that it is
expected transiently during a net rollout.
- The two polling intervals the net component requests while withholding
(`backendPollInterval` and `backendIdlePollInterval` in
`internal/operator/components/net/activation.go`), and why the operator
re-checks rather than watching Deployment status or endpoints.
- The operator to net-controller version coupling. The gate reads the published
endpoint's `targetRef`, which controllers released before #603 do not set.
Pinning the net controller image through the override ConfigMap
(`internal/operator/override/allowlist.go:56-60`, documented at
`docs/content/reference/workload-overrides.md:554-563`) survives operator
upgrades indefinitely, so this is a supported configuration users need to
understand. #603 added a compatibility path for a nil `targetRef`; document
what it does and does not verify.
Either add a "Site conditions" section to an existing reference page or create a
new one, whichever fits the docs structure better.
---
## Acceptance criteria
- [ ] A wedged site controller cannot leave the process running as a leader that
never publishes its endpoint.
- [ ] `controller.healthPort` either works end to end or no longer exists as a
knob, with a drift-guard test either way.
- [ ] The three cleanups in section 3 are applied.
- [ ] `BackendNotReady`, the poll behaviour, and the version coupling are
documented.
- [ ] `make fmt`, `make lint`, and `go test -race ./...` pass.
Contributor guide
Research direction
Start with cmd/unbounded-net-controller/main.go, metrics.go, health_state.go, and their named tests to trace readiness timeout and cleanup behavior. Then inspect the deployment, Service, webhook, APIService, ConfigMap templates and existing drift-guard tests, followed by docs/content/reference/; done means all four sections are implemented, documented, and make fmt, make lint, and go test -race ./... pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, kubernetes
- Domain
- api, backend, devops, documentation, testing
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100