Re-enable metrics-server via hostNetwork (kubectl top under Cilium)

[jwbron]

Summary

kubectl top doesn't work on egg's k3s clusters: #2703 disabled metrics-server with --disable=metrics-server because, under our Cilium datapath, its pod can't reach the kubelet on the node IP — it never becomes Ready, and the perpetually-unavailable v1beta1.metrics.k8s.io APIService wedges all namespace deletion (namespaces stuck Terminating).

That disable was treated as permanent — but the root cause is the network namespace, not an inherent Cilium limitation. The kubelet is perfectly reachable from the host (curl -k https://<nodeIP>:10250 → 401). Running metrics-server with hostNetwork: true puts it on that same path, so it scrapes fine and kubectl top works — without re-triggering the namespace-deletion wedge (the APIService stays Available).

This also fixes a latent inconsistency: docs/deploy/resource-sizing.md ships a kubectl top sampling loop that couldn't work given the disable.

What changed

• k8s/addons/metrics-server.yaml — vendored upstream metrics-server v0.8.1, with exactly three egg modifications (all marked egg:): hostNetwork: true, dnsPolicy: ClusterFirstWithHostNet, and --secure-port / containerPort 10250 → 4443 (10250 is the kubelet's; on hostNetwork the pod shares node ports). Port stays named https, so the Service targetPort and probes follow unchanged.
• scripts/install-metrics-server.sh — idempotent apply + verification (rollout → APIService Available → real kubectl top nodes), matching install-cilium.sh conventions. Refuses if k3s's bundled metrics-server addon is still present (it would fight ours).
• Makefile k3s-setup + CI test-integration.yml — run the script after the node is Ready. We keep --disable=metrics-server (so k3s's broken bundled one doesn't reconcile-fight ours) and ship our own.
• docs/guides/deployment.md — updated the --disable=metrics-server rationale to describe the bundled-vs-hostNetwork split.

Design notes

• Keep --disable + ship our own rather than un-disable + patch: k3s reconciles its bundled manifest from /var/lib/rancher/k3s/server/manifests, so patching it in place loses to the addon controller.
• Tail risk: re-introducing metrics-server means a window where the pod is down (reboot/rollout/OOM) could briefly re-open the namespace-deletion wedge. hostNetwork keeps it Available in steady state; maxUnavailable: 0 (upstream default, retained) avoids a gap during rollouts.

Testing

Verified end-to-end on a live single-node Cilium k3s cluster:

• scripts/install-metrics-server.sh → pod 1/1, v1beta1.metrics.k8s.io Available=True, kubectl top nodes/pods -n egg-system return data.
• kubectl apply --dry-run=server clean; yamllint + shellcheck clean on the new files.

Note: make lint currently fails on a pre-existing ruff issue in orchestrator/tests/test_cli.py, unrelated to this PR (not in the diff).

[egg-reviewer]

No agent-mode design concerns — this is k8s/Cilium infrastructure (vendored metrics-server with hostNetwork: true, install script, Makefile/CI wiring). No agent prompts, LLM calls, output formats, or sandbox boundaries touched.

— Authored by egg

[egg-reviewer]

Review summary

The hostNetwork approach is sound — under egg's Cilium datapath the host netns reaches the kubelet (verified by upstream issues against kubeProxyReplacement=false + bpf.hostLegacyRouting=true), and the vendored-manifest + idempotent-script + bundled-addon-guard pattern matches install-cilium.sh conventions cleanly. The maxUnavailable: 0 retention, however, becomes a deadlock when combined with hostNetwork: true on a single-replica deployment, and the doc comment that claims "byte-for-byte upstream" misrepresents the diff. Details below.

Blocking

1. Rolling-update strategy will deadlock on the first attempted rollout (k8s/addons/metrics-server.yaml:155-171).

The deployment has, effectively:

• replicas: 1 (default — no explicit field)
• strategy.rollingUpdate.maxUnavailable: 0 (explicit, copied from upstream)
• strategy.rollingUpdate.maxSurge: 25% (default — no explicit field; ceil(0.25·1) = 1)
• hostNetwork: true with containerPort: 4443

On any rollout (image bump, args change, env update, kubectl rollout restart), the rolling-update controller wants to bring up a second pod before deleting the old one (that is precisely what maxUnavailable: 0 + maxSurge ≥ 1 mandates). The scheduler does not consider containerPort for hostNetwork pods (only hostPort is a scheduler predicate), so on a single-node k3s cluster both replicas are placed on the same node, the new pod attempts to bind 0.0.0.0:4443, fails with address already in use, and CrashLoopBackOffs forever. Because maxUnavailable: 0, the old pod is never evicted to make room. The rollout is stuck, the metrics-server image cannot be upgraded, and the only escape is to manually kubectl delete pod — which is exactly the gap-during-restart the strategy was meant to prevent.

This is not theoretical: it is the standard outcome of hostNetwork + maxUnavailable=0 + single-replica + no pod-anti-affinity on a single-node cluster. Initial install works because there is no old pod, and CI never tests a rollout — so the green checks here are not evidence the strategy is safe.

The PR description leans on this strategy: "maxUnavailable: 0 (upstream default, retained) avoids a gap during rollouts." That is true in upstream's multi-replica non-hostNetwork shape, but the moment you flip hostNetwork: true you lose the property — the strategy now turns a bounded gap into an indefinite deadlock. The two changes have to be reasoned about together.

Fix one of:

• strategy: { type: Recreate } — old pod is deleted before new pod starts; brief unavailability window (~25s including probe initial delay), bounded.
• strategy: { rollingUpdate: { maxSurge: 0, maxUnavailable: 1 } } — same semantics, less drastic edit.

Both reintroduce the "gap during rollout" that hostNetwork pods cannot avoid on a single node. That gap is the correct tradeoff: a few seconds of APIService=Unavailable during pod restart is vastly better than a deployment that cannot be rolled forward. The namespace-deletion wedge only triggers when the APIService stays unavailable across a discovery cycle long enough for a namespace deletion to land — a 25s restart is well below that risk surface.

When you fix this, please also update the design-notes paragraph in the PR body so the reasoning matches the manifest.

Non-blocking

2. "byte-for-byte upstream v0.8.1" claim in the header comment is inaccurate (k8s/addons/metrics-server.yaml:32).

Diffing /tmp/upstream-ms.yaml (the verified 4a672c4891… SHA) against the vendored copy shows that every YAML list in the file has been re-indented by two extra spaces — rules, subjects, the Service ports, the container spec, volumes, the args list. This is YAML-cosmetic (semantically identical to upstream) but it means a future maintainer who wants to do a clean diff against v0.9.0's components.yaml will see ~100 noise lines on top of the real changes, which directly undermines the "search for egg:" audit story the comment promises. Either keep upstream's indentation verbatim (preferred — it's what the comment claims) or change the comment from "byte-for-byte upstream" to "semantically equivalent to upstream; see the egg: markers for the substantive changes."

3. Final kubectl top nodes smoke-test deadline is tight (scripts/install-metrics-server.sh:98-105).

With --metric-resolution=15s, the first scrape can land up to 15s after readiness, and the 30s deadline starts only after the APIService reports Available — which itself can lag pod-Ready by a scrape cycle. In steady-state runs this is fine; on a cold k3s where the kubelet's cAdvisor is still spinning up it could flake. Bumping to 60s (or polling every 2s instead of 3s) costs nothing in the happy path and removes a future flake source. Not blocking — the script is run once per cluster install.

4. --kubelet-insecure-tls is intentionally omitted, but the failure mode if k3s ever changes default cert provisioning is silent (k8s/addons/metrics-server.yaml:177).

This relies on k3s auto-rotating kubelet serving certs against the cluster CA. That works today, but it's a property of k3s's defaults — if a future k3s release flips back to self-signed kubelet certs (or serverTLSBootstrap is ever explicitly disabled in an INSTALL_K3S_EXEC arg), scrapes will start returning x509: certificate signed by unknown authority and the pod will go NotReady → wedge risk reopens. Since this is internal cluster traffic on an internal-only metrics surface, adding --kubelet-insecure-tls is the standard defense-in-depth move; the security cost is roughly zero (the kubelet endpoint is already authenticated via the metrics-server SA token, not the cert).

5. PR description references docs/deploy/resource-sizing.md but the file isn't in the diff.

The description says this PR "fixes a latent inconsistency: docs/deploy/resource-sizing.md ships a kubectl top sampling loop that couldn't work given the disable." That doc is now functional once metrics-server is back — but it would be worth either (a) adding a one-line note at the top of resource-sizing.md that points to k8s/addons/metrics-server.yaml for the requirement, or (b) dropping that line from the PR description so the claim matches the diff. Currently a reader following the PR-description link finds an unchanged file.

6. replicas: 1 is implicit.

Worth making explicit alongside any strategy: change (issue 1), so the relationship between replica count and rollout strategy is legible at a glance. Minor.

— Authored by egg

[james-in-a-box]

Thanks for the careful review — the rolling-update deadlock especially. Per-item disposition:

Blocking

1. Rolling-update deadlock under hostNetwork + single replica — fixed-in-PR (commit bc6c11b)
Switched the Deployment from rollingUpdate + maxUnavailable:0 to strategy: Recreate. Picked Recreate over maxSurge:0, maxUnavailable:1 because the semantic ("delete the old pod, then create the new one") matches what a single-replica hostNetwork deployment can actually do — and is the most legible to a future maintainer who sees hostNetwork: true and asks "wait, how does this roll?" The bounded ~25s gap during rollouts is the correct tradeoff vs. an indefinite deadlock that can only be unstuck with kubectl delete pod. Also made replicas: 1 explicit so the strategy/replica relationship is visible at a glance (item #6).

The header comment in k8s/addons/metrics-server.yaml now documents this as the third coupled egg: change, with the same reasoning you laid out in the review. (PR-body edit denied by the gateway — repo owner can adjust the design-notes paragraph if desired; the manifest and its inline rationale are the authoritative source.)

Non-blocking

2. "byte-for-byte upstream v0.8.1" claim is inaccurate — fixed-in-PR (commit bc6c11b)
Updated the header comment to "semantically equivalent to upstream v0.8.1" and explicitly called out the YAML-cosmetic reindent + the strategy/insecure-tls modifications as the four substantive deltas, with a hint to normalize indentation before diffing against a newer upstream. Kept the editor-applied reindent rather than reverting it — reverting would be 100+ lines of noise that the maintainer's git blame would have to step through, and the comment now sets the right expectation for the audit story.

3. kubectl top nodes smoke-test deadline is tight — fixed-in-PR (commit bc6c11b)
Bumped from 30s → 60s and the poll interval from 3s → 2s. Removes the cold-k3s cAdvisor flake source you flagged; zero cost in the happy path since the script exits as soon as the first scrape lands.

4. --kubelet-insecure-tls defense-in-depth — fixed-in-PR (commit bc6c11b)
Added the flag with an inline comment explaining the "no-op today, prevents silent x509 wedge tomorrow" reasoning (k3s flipping back to self-signed kubelet certs would otherwise re-open exactly the namespace-deletion wedge this PR is plugging). Endpoint authentication is unchanged — still via the metrics-server SA token.

5. PR description references docs/deploy/resource-sizing.md but the file isn't in the diff — fixed-in-PR (commit bc6c11b)
Took option (a): added a one-line > **Requires kubectl top.** admonition at the top of docs/deploy/resource-sizing.md linking to k8s/addons/metrics-server.yaml. A reader following the sampling loops now sees the dependency immediately. (PR-body edit denied by the gateway — see item #1.)

6. replicas: 1 implicit — fixed-in-PR (commit bc6c11b)
Made explicit alongside the Recreate strategy in the Deployment spec, with a comment tying the two together.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns. The delta since the prior review (commit bc6c11b) is entirely k8s/Cilium infrastructure — Deployment strategy: Recreate + explicit replicas: 1, --kubelet-insecure-tls, the smoke-test deadline bump in scripts/install-metrics-server.sh, and the header-comment correction in k8s/addons/metrics-server.yaml. None of these touch agent prompts, LLM calls, output formats, or sandbox boundaries.

— Authored by egg

[egg-reviewer]

Re-review summary

All six items from the previous review are addressed. The blocking deadlock fix is the right call, and the non-blocking items are handled cleanly. Approving.

Blocking item — fixed

1. Rolling-update deadlock under hostNetwork + single replica — fixed via strategy: { type: Recreate } (k8s/addons/metrics-server.yaml:187-188) with replicas: 1 made explicit (:179). Recreate is the right choice over maxSurge:0, maxUnavailable:1 here: the "delete then create" semantics are self-evident when paired with hostNetwork: true, and a future maintainer doesn't have to reconstruct why maxSurge:0 is load-bearing. The bounded gap (~25s under a clean shutdown; default terminationGracePeriodSeconds=30s is the upper bound on the SIGTERM phase, plus readinessProbe.initialDelaySeconds=20s before the first probe) is comfortably below the namespace-controller's discovery cycle, so the wedge this PR is plugging stays plugged across rollouts.

Non-blocking items — all addressed

• #2 — "byte-for-byte upstream" claim — fixed (k8s/addons/metrics-server.yaml:49-52). Header now reads "semantically equivalent to upstream v0.8.1", calls out the YAML-cosmetic reindent, and tells a future maintainer to normalize indentation before diffing against a newer upstream release. Audit story matches the diff.
• #3 — kubectl top nodes smoke-test deadline — fixed (scripts/install-metrics-server.sh:100,107). 30s → 60s deadline + 3s → 2s poll, with an inline comment tying the deadline to --metric-resolution=15s and cAdvisor cold-start.
• #4 — --kubelet-insecure-tls defense-in-depth — fixed (k8s/addons/metrics-server.yaml:206). Added with inline rationale.
• #5 — docs/deploy/resource-sizing.md admonition — fixed (docs/deploy/resource-sizing.md:3). One-line > **Requires kubectl top.** admonition links to the manifest. Reader following the sampling loops now sees the dependency immediately.
• #6 — replicas: 1 implicit — fixed (k8s/addons/metrics-server.yaml:179) with a comment tying the count to the strategy.

Non-blocking observation on this iteration

Minor comment-accuracy nit (k8s/addons/metrics-server.yaml:46-48). The inline rationale for --kubelet-insecure-tls says "Security cost ~zero: the kubelet endpoint is already authenticated via the metrics-server SA token, and this is internal cluster traffic on an internal-only metrics surface." The SA token is the client→server auth (metrics-server proving identity to kubelet); --kubelet-insecure-tls skips the server→client verification (metrics-server confirming kubelet's identity). These are independent properties — the SA-token argument doesn't logically discharge the cost of skipping cert verification. The right argument is the second clause alone: on hostNetwork this is loopback-equivalent traffic on a single host, so there's no realistic interception surface. Pure comment hygiene; the flag and its placement are correct. Optional follow-up — not worth a separate PR.

Everything else end-to-end traces cleanly: make k3s-setup → install-metrics-server.sh (bundled-addon guard → apply → rollout wait → APIService Available wait → kubectl top smoke test) → kubectl top nodes/pods works → docs/deploy/resource-sizing.md sampling loops are reproducible.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. Picking up the one observation:

Comment-accuracy nit on --kubelet-insecure-tls rationale (k8s/addons/metrics-server.yaml:46-48) — fixed-in-PR (commit 0c0c244)
You're right that the SA-token clause was off-target: that's client→server auth (metrics-server proving identity to the kubelet), and the flag this PR adds skips server→client cert verification (metrics-server confirming the kubelet's identity) — independent properties. Rewrote the header to lead with the actual reason the cost is ~zero here (hostNetwork makes it loopback-equivalent on a single host, no realistic interception surface), and parenthetically noted that the SA-token client→server path is unchanged and independent so a future reader doesn't wonder if the flag affected it.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns. The only delta since the prior review (commit bc6c11b) is a comment-accuracy correction in k8s/addons/metrics-server.yaml about the auth-direction reasoning for --kubelet-insecure-tls (commit 0c0c244). Pure comment hygiene in a k8s addon — no agent prompts, LLM calls, output formats, or sandbox boundaries are touched.

— Authored by egg

[egg-reviewer]

Re-review summary

The only delta since the prior review (bc6c11b) is commit 0c0c244, which rewrites the --kubelet-insecure-tls rationale in k8s/addons/metrics-server.yaml:42-51. This was the lone non-blocking observation from my previous re-review, and it is correctly resolved.

Verified

• Auth direction is now stated correctly (k8s/addons/metrics-server.yaml:46-51). The header now says the flag "skips server→client cert verification (metrics-server confirming the kubelet's identity)" — that matches what --kubelet-insecure-tls actually does (it disables the metrics-server client's verification of the kubelet's serving cert; it does not touch the SA-token bearer auth in the other direction).
• The cost-is-~zero argument now stands on its own merits. The new wording leads with the right justification — "on hostNetwork this is loopback-equivalent traffic on a single host — no realistic interception surface" — rather than leaning on the SA-token clause, which was orthogonal. That is exactly the correction I asked for.
• The parenthetical disambiguation prevents future confusion (:50-51). Explicitly noting that the client→server SA-token path is "unchanged and independent" keeps a future reader from re-conflating the two directions and concluding the flag affected authentication. Good prophylactic note.
• Nothing else moved. Diff is comment-only (no manifest semantics, no script changes, no doc shuffle); CI surface is the same as the prior green run; the four egg: markers the trailing comment promises are still the four below.

No new issues. Approving.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Re-review on commit 0c0c244 had no new actionable items — review bot returned "No new issues. Approving." and agent-mode-design returned verdict=approve. Nothing to fix this round.

For completeness, all prior feedback is accounted for:

• Blocking Phases 1-2: Repository setup, docs, and gateway extraction (partial) #1 — rolling-update deadlock under hostNetwork + single replica — fixed-in-PR (commit bc6c11b) (Deployment → strategy: Recreate, replicas: 1 made explicit)
• Phase 1: Repository setup and CI infrastructure #2 — "byte-for-byte upstream v0.8.1" claim — fixed-in-PR (commit bc6c11b) (header rewritten to "semantically equivalent", reindent + substantive deltas called out)
• Phase 1.5: Documentation extraction #3 — kubectl top nodes smoke-test deadline tight — fixed-in-PR (commit bc6c11b) (30s → 60s, 3s → 2s poll)
• Phase 2: Gateway module extraction #4 — --kubelet-insecure-tls defense-in-depth — fixed-in-PR (commit bc6c11b) (flag added with inline rationale)
• Phase 3: Container extraction #5 — docs/deploy/resource-sizing.md admonition — fixed-in-PR (commit bc6c11b) (one-line > **Requires kubectl top.** linking to the manifest)
• Phase 4: CLI implementation #6 — replicas: 1 implicit — fixed-in-PR (commit bc6c11b) (made explicit alongside Recreate)
• Comment-accuracy nit on --kubelet-insecure-tls auth direction — fixed-in-PR (commit 0c0c244) (rewrote rationale to lead with the hostNetwork loopback-equivalent argument; parenthetically noted SA-token path is independent)

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

10 previous review(s) hidden.