reconciler: scope the drift check to svcforge's own releases
ci / lint (push) Failing after 10s
ci / types (push) Has been skipped
ci / unit (push) Has been skipped
ci / integration (push) Has been skipped
ci / security (push) Has been skipped
ci / dockerfile (push) Has been skipped
ci / chart (push) Has been skipped
ci / image (api) (push) Has been skipped
ci / image (reconciler) (push) Has been skipped
ci / image (worker) (push) Has been skipped
ci / bump (push) Has been skipped
ci / lint (push) Failing after 10s
ci / types (push) Has been skipped
ci / unit (push) Has been skipped
ci / integration (push) Has been skipped
ci / security (push) Has been skipped
ci / dockerfile (push) Has been skipped
ci / chart (push) Has been skipped
ci / image (api) (push) Has been skipped
ci / image (reconciler) (push) Has been skipped
ci / image (worker) (push) Has been skipped
ci / bump (push) Has been skipped
check_drift diffs helm against the database in both directions, and the second one is `live - known` -> drift.orphan_release at ERROR. `live` was every release in the cluster, so argocd, longhorn, gitea and cert-manager were all reported as orphans svcforge is failing to account for, on every sweep. They are not orphans; they were never svcforge's to know about. install() now stamps app.kubernetes.io/managed-by=svcforge and list_releases() selects on it. Releases provisioned before this see one sweep as missing and get re-provisioned, which is safe by construction — `helm upgrade --install` against a deterministic release name — and that re-provision applies the label. This does NOT fix the timeout, and the docstring says so. Measured, not assumed: unscoped 23 releases in 4392ms, scoped to 0 in 3988ms. `--selector` is not pushed down as a server-side selector, so helm still fetches and decompresses every release secret and filters what it already parsed. Around 10%, not the order of magnitude the flag's shape suggests. The 330s timeouts need CPU for the container or a different read path. The two sides of the label are asserted against each other rather than a literal, so a rename that updates only one fails in tests instead of in production as a silently empty drift check. Control-tested: renaming the read side alone fails two of the three.
This commit is contained in:
@@ -45,6 +45,16 @@ _RUN_TIMEOUT_MARGIN_S = 30
|
|||||||
|
|
||||||
_STDERR_TAIL_BYTES = 2048
|
_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):
|
class HelmError(SvcforgeError, RuntimeError):
|
||||||
"""Non-zero exit. str(self) is the stderr tail that lands in instances.error.
|
"""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
|
# and one fewer set of vendored Go CVEs to track. Idempotent: existing
|
||||||
# namespaces are left alone.
|
# namespaces are left alone.
|
||||||
"--create-namespace",
|
"--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",
|
"--version",
|
||||||
entry.chart_version,
|
entry.chart_version,
|
||||||
"--values",
|
"--values",
|
||||||
@@ -232,8 +248,42 @@ class HelmProvisioner:
|
|||||||
await self._run_helm(argv)
|
await self._run_helm(argv)
|
||||||
|
|
||||||
async def list_releases(self) -> list[ReleaseInfo]:
|
async def list_releases(self) -> list[ReleaseInfo]:
|
||||||
"""Every release helm knows about, in every namespace. The reconciler's view of reality."""
|
"""Every release SVCFORGE provisioned, in every namespace. The reconciler's view of reality.
|
||||||
argv = self._base_argv("list", "--all-namespaces", "--output", "json")
|
|
||||||
|
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)
|
raw = await self._run_helm(argv)
|
||||||
try:
|
try:
|
||||||
parsed: Any = json.loads(raw or "[]")
|
parsed: Any = json.loads(raw or "[]")
|
||||||
|
|||||||
@@ -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")
|
||||||
Reference in New Issue
Block a user