Skip to content

Commit 2ba4d84

Browse files
committed
docs: renumber to ADR-020 and address review feedback
Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
1 parent 3cc2046 commit 2ba4d84

1 file changed

Lines changed: 32 additions & 14 deletions

File tree

docs/design/021-snapshot-agent-run-isolation.md renamed to docs/design/020-snapshot-agent-run-isolation.md

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# ADR-021: Snapshot Agent Run Isolation
1+
# ADR-020: Snapshot Agent Run Isolation
22

33
## Status
44

@@ -8,9 +8,10 @@
88
## Decision Summary
99

1010
Every `aicr snapshot` and `aicr validate` invocation generates a run ID and
11-
suffixes it onto every Kubernetes object it creates. User-supplied resource
12-
names are **prefixes**, never exact names, so no object aicr creates can collide
13-
with another run's.
11+
suffixes it onto every **run-owned** object. User-supplied resource names are
12+
**prefixes**, never exact names, so no run-owned object can collide with another
13+
run's. The Namespace and an explicitly requested output ConfigMap are shared by
14+
design and are not suffixed.
1415

1516
Object lifecycles fall into exactly three classes — **run-owned**, **ensured**,
1617
and **delivered** — and every object the snapshot agent touches belongs to one.
@@ -69,6 +70,12 @@ unit tests stay deterministic. The run ID is logged at `slog.Info` on deploy; it
6970
timestamp prefix makes runs sortable in `kubectl get` output and in orphan
7071
triage.
7172

73+
**One run ID per invocation.** `aicr validate` both collects a snapshot and
74+
deploys validation Jobs, and both must carry the same ID. It is generated once at
75+
the top of the command and passed into collection. Today the two are independent
76+
`pkg/cli/validate.go:191` collects, `:265` generates — which would split names,
77+
labels, logs, and cleanup across two IDs.
78+
7279
### 2. Every user-supplied name is a prefix
7380

7481
`--job-name`, `--service-account-name`, and their `pkg/config` and SDK
@@ -109,8 +116,10 @@ here.
109116
| **Ensured** | Namespace | Created if absent, labeled `managed-by`, never deleted, never suffixed. |
110117
| **Delivered** | Explicit `cm://namespace/name` output ConfigMap | The user's artifact. Written on purpose, never deleted, never suffixed. |
111118

112-
aicr deletes an object if and only if it is run-owned. Exactly one object kind
113-
falls in each of the other two classes.
119+
At runtime aicr deletes an object if and only if it is run-owned. Exactly one
120+
object kind falls in each of the other two classes. `tools/cleanup` sits outside
121+
this boundary as a developer utility; its name-based removal of the legacy
122+
`aicr-node-reader` pair is a one-time migration step, not run cleanup.
114123

115124
### 4. The Namespace stays user-specified
116125

@@ -131,12 +140,16 @@ and is not renamed to `aicr-<runID>`:
131140
### 5. Pod selection by Job ownership
132141

133142
`Deploy` records the UID returned by the Job `Create`. `findPodName` and
134-
`findOrWatchPodName` (`wait.go:111-149`) select on
135-
`app.kubernetes.io/name=aicr,aicr.run/run-id=<runID>`, then reject any pod whose
136-
`batch.kubernetes.io/controller-uid` does not match. Existing
137-
`DeletionTimestamp` / `PodFailed` / youngest-first filtering is retained as a
138-
tiebreaker. The UID is held in an `atomic.Pointer`, since it is written in
139-
`Deploy` and read from the log-streaming goroutine spawned afterwards.
143+
`findOrWatchPodName` (`wait.go:111-149`) narrow by
144+
`app.kubernetes.io/name=aicr,aicr.run/run-id=<runID>`, then confirm ownership.
145+
Existing `DeletionTimestamp` / `PodFailed` / youngest-first filtering is retained
146+
as a tiebreaker.
147+
148+
**Ownership must be established from the pod's controlling `ownerReference`, not
149+
from a label.** Pod labels — `batch.kubernetes.io/controller-uid` included — are
150+
writable by anything that can update pods in the namespace, so they narrow the
151+
candidate set but cannot authorize it. The authority is a `controller: true`
152+
`ownerReference` of kind `Job` carrying the recorded Job UID.
140153

141154
### 6. Cleanup is ownership-scoped
142155

@@ -153,6 +166,10 @@ which per-run naming would otherwise turn into one leaked object per run.
153166
`CheckPermissions` (`permissions.go:48-64`) gains the `configmaps: delete` verb
154167
this needs.
155168

169+
The staging ConfigMap is created by the in-pod agent, not the controller, so its
170+
UID is not captured at create time. It must be recorded when the controller reads
171+
it so its deletion is UID-pinned like every other run-owned object.
172+
156173
### 7. Labels
157174

158175
Applied to all seven objects **and to the Job's pod template** — Job
@@ -203,8 +220,9 @@ selectors whose value now changes per run.
203220

204221
- The documented concurrency contract becomes true, including for SDK callers
205222
following the current documented example.
206-
- Every existing invocation still succeeds. No new failure modes, error codes, or
207-
flags are introduced.
223+
- Every existing invocation still succeeds: no new flags, no new user-facing
224+
error codes, and no new way for a well-formed command to fail. The one
225+
observable behavior change is the ServiceAccount drift noted below.
208226
- The `aicr validate` / `aicr snapshot` cross-command collision is fixed.
209227
- Least-privilege improves: a `DiscoverNetwork` run's mutating cluster
210228
permissions bind to an identity that exists for one run and is revoked at

0 commit comments

Comments
 (0)