diff --git a/libs/svcforge_core/svcforge_core/adapters/helm.py b/libs/svcforge_core/svcforge_core/adapters/helm.py index ab1ee80..efab270 100644 --- a/libs/svcforge_core/svcforge_core/adapters/helm.py +++ b/libs/svcforge_core/svcforge_core/adapters/helm.py @@ -45,6 +45,16 @@ _RUN_TIMEOUT_MARGIN_S = 30 _STDERR_TAIL_BYTES = 2048 +# The label every release svcforge provisions carries, and the only thing that lets the +# reconciler tell its own releases from the rest of the cluster's. Written by install(), +# read by list_releases(). Changing either value without the other silently empties the +# reconciler's view of reality, which reads as "no drift" rather than as an error. +# +# `app.kubernetes.io/managed-by` is the standard key for exactly this, so anyone reading +# the cluster with kubectl gets the same answer the reconciler does. +MANAGED_BY_LABEL = "app.kubernetes.io/managed-by" +MANAGED_BY_VALUE = "svcforge" + class HelmError(SvcforgeError, RuntimeError): """Non-zero exit. str(self) is the stderr tail that lands in instances.error. @@ -206,6 +216,12 @@ class HelmProvisioner: # and one fewer set of vendored Go CVEs to track. Idempotent: existing # namespaces are left alone. "--create-namespace", + # Stamps MANAGED_BY_LABEL onto the release, which is what makes + # list_releases() able to ask for svcforge's releases and nobody else's. + # Without it the reconciler has to list every release in the cluster and + # sort out ownership afterwards, which it cannot actually do. + "--labels", + f"{MANAGED_BY_LABEL}={MANAGED_BY_VALUE}", "--version", entry.chart_version, "--values", @@ -232,8 +248,42 @@ class HelmProvisioner: await self._run_helm(argv) async def list_releases(self) -> list[ReleaseInfo]: - """Every release helm knows about, in every namespace. The reconciler's view of reality.""" - argv = self._base_argv("list", "--all-namespaces", "--output", "json") + """Every release SVCFORGE provisioned, in every namespace. The reconciler's view of reality. + + Scoped by label, and the scope is load-bearing twice over. + + Correctness first. The reconciler diffs this against the database in both directions, + and the second direction is `live - known` -> `drift.orphan_release` at ERROR. + Unscoped, `live` is every release in the cluster, so argocd, longhorn, gitea and + cert-manager are all reported as orphans svcforge is failing to account for, every + sweep. They are not orphans. They were never svcforge's to know about. + + What this does NOT fix is cost, and that was measured rather than assumed: + unscoped 23 releases in 4392ms, scoped to 0 releases in 3988ms. `--selector` is not + pushed down as a server-side label selector — helm fetches and decompresses every + release secret in the cluster regardless, then filters what it already parsed. The + saving is around 10%, not the order of magnitude the shape of the flag suggests. + + That matters because this call was timing out at 330s on every tick, with the CPU + request mutated to 0 by a cluster policy, against ~4s given real CPU. Scoping does + not rescue that. A check that never completes reports no drift, which looks exactly + like no drift existing, so the timeout needs its own fix — CPU for the container, or + reading the release secrets directly with a label selector, which the API server + answers in 141ms because it never decompresses anything. + + Releases provisioned before the label existed will not match, so the first sweep + after this ships sees them as missing and re-provisions. That is safe by + construction — provisioning is `helm upgrade --install` against a deterministic + release name — and the re-provision is what applies the label. + """ + argv = self._base_argv( + "list", + "--all-namespaces", + "--selector", + f"{MANAGED_BY_LABEL}={MANAGED_BY_VALUE}", + "--output", + "json", + ) raw = await self._run_helm(argv) try: parsed: Any = json.loads(raw or "[]") diff --git a/tests/unit/test_helm_argv.py b/tests/unit/test_helm_argv.py new file mode 100644 index 0000000..4ab816b --- /dev/null +++ b/tests/unit/test_helm_argv.py @@ -0,0 +1,93 @@ +"""The helm argv that the reconciler's correctness depends on. + +install() writes a label; list_releases() reads it back. Neither is checked by anything at +runtime — if the two ever disagree, `helm list --selector` matches nothing, the reconciler +sees an empty cluster, and every ready instance looks like it lost its release. That failure +is silent and reads as "no drift", so it gets a test rather than a comment. + +These assert argv, not behaviour against a real cluster: tests/e2e covers that. The point +here is that the two sides of the label agree, and that neither drops out under a refactor. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from svcforge_core.adapters.helm import ( + MANAGED_BY_LABEL, + MANAGED_BY_VALUE, + HelmProvisioner, +) +from svcforge_core.domain.models import CatalogEntry, SizeSpec + +ENTRY = CatalogEntry( + service_type="redis", + chart="oci://example/redis", + chart_version="1.2.3", + sizes={"small": SizeSpec(replicas=1, resources={})}, +) + + +def _capture(monkeypatch: pytest.MonkeyPatch) -> list[list[str]]: + """Record every argv HelmProvisioner would exec, and run none of them.""" + seen: list[list[str]] = [] + + async def fake_run_helm(self: HelmProvisioner, argv: list[str]) -> str: + seen.append(list(argv)) + return "[]" + + monkeypatch.setattr(HelmProvisioner, "_run_helm", fake_run_helm, raising=True) + return seen + + +def _pair(argv: list[str], flag: str) -> str | None: + """The value following `flag`, or None. Positional, because helm takes `--flag value`.""" + return argv[argv.index(flag) + 1] if flag in argv else None + + +@pytest.mark.asyncio +async def test_install_labels_the_release_as_svcforge_managed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + seen = _capture(monkeypatch) + await HelmProvisioner(kubeconfig=Path("/dev/null")).install( + "acme-redis", "tenant-acme", ENTRY, {"replicas": 1} + ) + + assert len(seen) == 1 + assert _pair(seen[0], "--labels") == f"{MANAGED_BY_LABEL}={MANAGED_BY_VALUE}" + + +@pytest.mark.asyncio +async def test_list_releases_asks_only_for_svcforge_releases( + monkeypatch: pytest.MonkeyPatch, +) -> None: + seen = _capture(monkeypatch) + + await HelmProvisioner(kubeconfig=Path("/dev/null")).list_releases() + + assert len(seen) == 1 + argv = seen[0] + assert _pair(argv, "--selector") == f"{MANAGED_BY_LABEL}={MANAGED_BY_VALUE}" + # Still every namespace. Tenants get their own, so scoping to one would hide releases + # rather than the cluster's; the label is what narrows this, not the namespace. + assert "--all-namespaces" in argv + + +@pytest.mark.asyncio +async def test_the_label_written_is_the_label_read(monkeypatch: pytest.MonkeyPatch) -> None: + """The regression that motivated this file. + + Asserted against each other rather than against a literal on both sides, so a rename + that updates only one of install/list_releases fails here instead of in production as + an empty drift check. + """ + seen = _capture(monkeypatch) + prov = HelmProvisioner(kubeconfig=Path("/dev/null")) + + await prov.install("acme-redis", "tenant-acme", ENTRY, {}) + await prov.list_releases() + + assert _pair(seen[0], "--labels") == _pair(seen[1], "--selector")