From 03c9571bbfd7834391dc276f7a464fafb50d57e7 Mon Sep 17 00:00:00 2001 From: Konstantins Feofantovs Date: Fri, 28 Aug 2026 22:49:37 +0300 Subject: [PATCH] =?UTF-8?q?fix:=20the=20update=20strategy=20is=20named=20f?= =?UTF-8?q?or=20the=20field,=20and=20absence=20claims=20nothing=20(ADR=200?= =?UTF-8?q?048=20=C2=A72)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two corrections to a decision that was a day old, both found by running the agent against a real cluster rather than by review. `rollout` is renamed `update_strategy`. `Rollout` is the kind name of the Argo custom resource, which the agent already reports in `workload_kind`; a record carrying `workload_kind: Rollout` beside a field called `rollout` put two meanings of one word in one object. `updateStrategy` is Kubernetes' own field name on StatefulSet and DaemonSet. The `unread` marker is removed. It claimed that a workload of an unknown kind has a strategy the agent failed to read — a judgement the agent cannot make, and one that needs a complete list of owner kinds that do not update themselves. The list omitted `Node`, so every static control-plane pod on this kind cluster was reported as having an unread strategy: "workload_kind": "Node", "container": "kube-apiserver", "workload": { "rollout": { "unread": true } } Absence now means the agent did not read a strategy, and nothing more. What distinguishes a Job from an Argo Rollout is `workload_kind`, which already ships; what a kind implies is a rendering (ADR 0004). ADR 0048 §2 is corrected in place with a dated note, per the owner's decision; `security.md` and `backend-requirements.md` follow. make test, lint, chart-lint: green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011YRKuL97KRndaopfpixrhp --- cmd/agent/main.go | 2 +- ...0048-findings-name-the-fields-they-need.md | 51 ++++++---- docs/backend-requirements.md | 10 +- docs/security.md | 2 +- .../{rollout.go => updatestrategy.go} | 96 ++++++++----------- ...rollout_test.go => updatestrategy_test.go} | 84 +++++++--------- internal/metadata/workload.go | 22 ++--- internal/metadata/workload_test.go | 20 ++-- internal/sink/spool_test.go | 12 +-- .../testdata/workload-metadata.golden.json | 11 +-- test/e2e/policy_test.go | 18 ++-- 11 files changed, 153 insertions(+), 175 deletions(-) rename internal/collector/{rollout.go => updatestrategy.go} (51%) rename internal/collector/{rollout_test.go => updatestrategy_test.go} (61%) diff --git a/cmd/agent/main.go b/cmd/agent/main.go index 5f43cdc..a9ee62f 100644 --- a/cmd/agent/main.go +++ b/cmd/agent/main.go @@ -363,7 +363,7 @@ func run(ctx context.Context, logger *slog.Logger, clientset kubernetes.Interfac // One capture instant for both payloads of a flush: they describe the // same cluster state and are joined against each other downstream. capturedAt := time.Now() - records := metadata.Aggregate(podWatcher.Pods(), podWatcher.Rollouts()) + records := metadata.Aggregate(podWatcher.Pods(), podWatcher.UpdateStrategies()) if err := spool.WriteWorkloadMetadata(capturedAt, records); err != nil { logger.Error("spooling workload metadata", "error", err) } diff --git a/docs/adr/0048-findings-name-the-fields-they-need.md b/docs/adr/0048-findings-name-the-fields-they-need.md index eebb6c8..af529cb 100644 --- a/docs/adr/0048-findings-name-the-fields-they-need.md +++ b/docs/adr/0048-findings-name-the-fields-they-need.md @@ -11,6 +11,10 @@ Services to `workload_policy`, and a version to each module in `go_build`. Adds one resource to the ClusterRole (`services`). Extends the informer transform of ADR 0046 to the probe handlers, which were being cached whole. +*Corrected 2026-08-28, the day it was accepted and before any release: §2 first +named the field `rollout` and gave unread kinds an `unread` marker. The first +run against a real cluster refuted both — see §2.* + ## Context Every decision here so far has answered the same question one fact at a time: @@ -62,22 +66,35 @@ The kind is kept and the target is not, because the finding asks whether two probes are the same check, not what either calls. The numbers are the API server's defaulted ones, which are the numbers the kubelet will use. -**2. The workload's update strategy ships in a `workload` scope block.** Type, +**2. The workload's update strategy ships as `workload.update_strategy`.** Type, `maxUnavailable`, `maxSurge`, a StatefulSet's `partition`, and `minReadySeconds`, read from the Deployment, StatefulSet or DaemonSet the pods -belong to. - -Absence has to answer two different questions, and one flag separates them. A -Job, a CronJob, a bare ReplicaSet and an owner-less pod replace no replicas, so -they carry no rollout and that absence is the fact. A pod managed by a custom -resource — an Argo Rollout, a Knative Revision, an in-house operator — is the -opposite case: it almost certainly *does* roll, and the agent holds no RBAC to -read how (`docs/security.md` §11). Reporting nothing there would state "this -does not roll", which is worse than silence. Those records carry -`rollout: {unread: true}` and nothing else — ADR 0013's rule that the agent -reports what it observed rather than what it concluded, applied to a structural -payload. This is the case a first-hour report must not get wrong quietly: Argo -Rollouts is common, and the finding is about exactly the workloads it manages. +belong to. Those three kinds and no others. + +**The field is not called `rollout`, and that is a decision rather than a +preference.** `Rollout` is the kind name of the Argo custom resource, which this +agent already reports in `workload_kind`. One record carrying +`workload_kind: Rollout` beside a field called `rollout` puts two meanings of +one word in one object, and the generic English noun is the one that loses. +`updateStrategy` is Kubernetes' own field name on StatefulSet and DaemonSet, so +it is borrowed rather than invented. + +**Absence means the agent did not read a strategy, and claims nothing beyond +that.** The first version of this decision tried to say more: kinds known to +replace no replicas carried nothing, and everything else carried +`rollout: {unread: true}` so that a Job could be told from an Argo Rollout. That +was wrong twice over, and one run against a real cluster showed both. It needs a +complete list of owner kinds that do not update themselves, which nobody has — +the list omitted `Node`, so every static control-plane pod on a self-managed +cluster was reported as having an update strategy the agent had failed to read. +And the claim itself is a judgement: whether an unknown custom resource updates +its replicas is not something the agent observes, and this agent reduces rather +than concludes (ADR 0004). + +What distinguishes the cases is already in the record. `workload_kind` ships in +the key, the kinds this agent reads are three and are named in +`docs/backend-requirements.md`, and what any other kind implies is a rendering +question for the side that renders. It is a fact of the workload, and the record's key is narrower than that, so it is nested and not flattened (ADR 0014) — a `workload` object beside the existing @@ -161,9 +178,9 @@ No CRD is read, and the closed ClusterRole that §9 of `docs/security.md` leans on gains no vendor resource. Reading Argo Rollouts properly is a larger decision than a field — a first custom-resource grant, an informer for a kind that may not exist in the cluster, and a strategy whose canary and blue-green shapes are not -this struct — so it is left to its own ADR. What this one owes is that the gap is -legible from the payload rather than inferred from a list of kinds the backend -would have to keep in step with the agent. +this struct — so it is left to its own ADR. Until then the rollout dimension of a +first-hour report is blank for an Argo-managed workload, and blank is what the +payload says. **Not changed.** No new payload kind, no registry row, no change to any natural key, no verb that writes, no new privilege on the node, and nothing new is read diff --git a/docs/backend-requirements.md b/docs/backend-requirements.md index 91750e8..2a027c0 100644 --- a/docs/backend-requirements.md +++ b/docs/backend-requirements.md @@ -279,11 +279,11 @@ was taken away, which is the normal state. **`workload_metadata.workload` and `.pod` are two different scopes, and neither is per record** (ADR 0014). A pod's containers each produce a record repeating both blocks, so **the backend MUST NOT sum either across one workload's -records**. `rollout` is absent for a kind that replaces no replicas — a Job, a -CronJob, a bare ReplicaSet, an owner-less pod — and MUST NOT be rendered as one -that replaces everything at once. A `rollout` carrying only `unread: true` is a -workload managed by a custom resource the agent holds no RBAC for, such as an -Argo Rollout: **unobserved, not absent**, and MUST NOT be read as no strategy. +records**. `workload.update_strategy` is present only for `workload_kind` of +`Deployment`, `StatefulSet` or `DaemonSet` — the kinds the agent reads. For every +other kind it is absent, and **the backend MUST NOT read that absence as "this +workload has no update strategy"**: an Argo Rollout has one and the agent holds +no RBAC for it. Absence says only that the agent did not read one. **`workload_metadata.pod.unscheduled` explains the replica shortfall, it does not restate it.** The gap between `replicas` and the sum of `nodes` has always diff --git a/docs/security.md b/docs/security.md index add4cf2..dda83a5 100644 --- a/docs/security.md +++ b/docs/security.md @@ -490,7 +490,7 @@ test fails if a kind ships without appearing here (ADR 0022). | `pod_disruptions` | Per hour: the pods the *cluster* removed — preempted, evicted under node pressure, drained, or removed via the eviction API — with the node and the instant | **yes** | | `job_runs` | Per hour: each Job that finished — when it started and finished, whether it succeeded, the failure reason, its pod success/failure counts, and its declared `parallelism`/`completions`/`backoffLimit` | **yes** | | `deployment_revisions` | Per flush: each ReplicaSet of a collected Deployment — its revision number, when it was created, its desired/current/ready replica counts, and each container's image reference | **yes** | -| `workload_metadata` | Declared shape per (namespace, workload, container, image digest): image, requests, limits, ports, probe schedules without what any probe checks, the five runtime knobs of the field-minimization list below, QoS, replica counts by phase, by node, and by unscheduled reason, the pod's reduced placement constraints (ADR 0031), and how the workload replaces its own replicas — or, for a controller the agent does not read, that it did not read it ([ADR 0048](adr/0048-findings-name-the-fields-they-need.md)) | no | +| `workload_metadata` | Declared shape per (namespace, workload, container, image digest): image, requests, limits, ports, probe schedules without what any probe checks, the five runtime knobs of the field-minimization list below, QoS, replica counts by phase, by node, and by unscheduled reason, the pod's reduced placement constraints (ADR 0031), and — for a Deployment, StatefulSet or DaemonSet — the update strategy by which the workload replaces its own replicas ([ADR 0048](adr/0048-findings-name-the-fields-they-need.md)). A workload of any other kind carries no strategy and nothing standing in for one | no | | `workload_policy` | Per collected workload, what bounds it from outside its own spec: disruption budgets covering it, autoscalers driving it, the volume claims its pods mount (ADR 0032), and the Services routing to it (ADR 0048); plus `unavailable_sources`, naming any of those the agent could not read — never granted, or no longer being fed (ADR 0033, ADR 0035) | not directly — but a claim is named, and a StatefulSet's claim is `