diff --git a/internal/api/client.go b/internal/api/client.go index d5ed292a..2f27589e 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -402,6 +402,11 @@ type ProvisionedClient struct { // ClusterID is the kube-system namespace UID this client is anchored to // (RFC-0001 §6.3 / backend#883). Empty on legacy / not-yet-backfilled clients. ClusterID string `json:"cluster_id"` + // NumRunningExperiments is how many training runs are active on this client + // right now. `tracebloc delete`'s work-guard blocks on this — not on Status + // (== online, which a healthy client always is) — so a live-but-idle + // environment can still be offboarded after the typed-name confirm. + NumRunningExperiments int `json:"num_running_experiments"` } // CreateClientRequest is the POST /edge-device/ body. The account is stamped diff --git a/internal/cli/delete.go b/internal/cli/delete.go index 1e112f98..e48e5e53 100644 --- a/internal/cli/delete.go +++ b/internal/cli/delete.go @@ -125,49 +125,46 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er ns = prof.ActiveClientNamespace } - // The three-way scope summary — shown BEFORE the confirm so the user sees - // exactly what is removed, kept, and left alone (RFC-0001 §7.10). - renderOffboardSummary(p, name, ns, o.keepData) - - // Live-work guard (inherits §7.4): refuse if tracebloc still reports this - // client online / with a running job, unless --force. The heartbeat is - // advisory, so this is a courtesy stop, not the safety — the teardown itself - // is the real gate. A lookup failure is non-fatal (the client may be - // unreachable precisely because it's being retired); warn and continue. + // Work-guard: refuse to offboard while training runs are ACTIVE (offboarding + // would kill them), unless --force. Block on RUNNING experiments, NOT on + // "online": a healthy environment is always online (it heartbeats), so + // blocking on that would make offboard impossible without --force and mislead + // ("stop your training jobs" when there are none). A lookup failure is + // non-fatal (the environment may be unreachable precisely because it's being + // retired); warn and continue — the typed-name confirm and the teardown are + // the real gates. 426/401/403 won't recover by continuing, so fail fast. if !o.force { - if st, found, lerr := lookupClientStatus(ctx, client, prof.ActiveClientID); lerr != nil { - // A 426 (CLI too old) won't recover by continuing — the whole offboard - // talks to the same backend, so fail fast with the upgrade message rather - // than imply the guard was merely skipped. + pc, lerr := client.GetClient(ctx, id) + switch { + case lerr != nil: var ue *api.UpgradeRequiredError if errors.As(lerr, &ue) { return &exitError{code: exitFailure, err: lerr} } - // A 401/403 means the signed-in credential is revoked/expired — it won't - // recover by continuing either, and every later step (the revoke included) - // hits the same backend. Fail fast and point at sign-in rather than march - // the user through the confirm only to fail at revoke. Mirrors the --wait - // poll loop's auth handling in client.go. var ae *api.APIError if errors.As(lerr, &ae) && (ae.StatusCode == http.StatusUnauthorized || ae.StatusCode == http.StatusForbidden) { return &exitError{code: exitFailure, err: errors.New( "tracebloc rejected your credentials — run `tracebloc login`, then retry `tracebloc delete`")} } - // Other errors (5xx/429/network) are transient: warn and continue, since - // the teardown is the real gate. - p.Hintf("Couldn't check whether this client is still online (%v) — continuing; pass --force to skip this check.", lerr) - } else if !found { + p.Hintf("Couldn't check for active training runs (%v) — continuing; the confirmation below still guards you.", lerr) + case pc == nil: // The stored id isn't among this account's clients — likely a stale - // pointer or the wrong account/env. Don't silently pass the guard: warn, - // then continue (the revoke below will 403/404 if it isn't really ours). - p.Hintf("This client isn't in the signed-in account's client list — continuing; if that's unexpected, check you're logged into the right account/env.") - } else if st == clientStatusOnline { + // pointer or the wrong account. Warn, then continue (the revoke below + // will 403/404 if it isn't really ours). + p.Hintf("This secure environment isn't in the signed-in account — continuing; if that's unexpected, check you're logged into the right account.") + case pc.NumRunningExperiments > 0: return &exitError{code: exitFailure, err: fmt.Errorf( - "client %q is still online (tracebloc reports it running) — stop its training jobs first, "+ - "or pass --force to offboard anyway", name)} + "your secure environment %q has %d training run(s) active — offboarding would stop them. "+ + "Wait for them to finish or cancel them, then run `tb delete` again — or `tb delete --force` to stop them and offboard now", + name, pc.NumRunningExperiments)} } } + // The three-way scope PREVIEW — shown after the work-guard and before the + // typed-name confirm, so the user sees exactly what WILL be removed, kept, and + // left alone before committing to anything (RFC-0001 §7.10). + renderOffboardSummary(p, name, o.keepData) + // Typed-client-name confirmation (not [y/N]): the local data wipe is // irreversible, so require the user to type the client's name. --yes skips it // for automation. @@ -233,7 +230,7 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er p.Hintf("Couldn't revoke the credential server-side (%v) — continuing with local teardown. "+ "The credential may still be live on tracebloc; revoke it from the dashboard if needed.", rerr) } else { - p.Successf("Revoked this machine's credential (client %q kept on tracebloc as a record).", name) + p.Successf("Revoked this machine's credential — your secure environment %q stays on tracebloc as a record.", name) } // The teardown steps below are best-effort. (The credential is revoked when the @@ -266,7 +263,7 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er p.Warnf("Chart uninstall reported: %v", uerr) degraded = true } else { - p.Successf("Uninstalled the Helm release %s.", ns) + p.Successf("Uninstalled tracebloc.") } } else { // No cached namespace (a pre-cache config, or none passed) — we can't name @@ -282,15 +279,22 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er p.Warnf("Cluster teardown reported: %v", terr) degraded = true } else { - p.Successf("Deleted the local cluster %q.", nodeboot.ClusterName) + p.Successf("Removed the local environment.") } // 4. Reclaim the tracebloc container images (SCOPED to ghcr.io/tracebloc/*, // never a blanket prune — best-effort). if perr := pruneImages(ctx); perr != nil { - p.Warnf("Image reclaim reported: %v", perr) + // Pure disk cleanup — a failure here changes nothing about the offboard + // (it's excluded from `degraded`). Keep it a quiet, plain note; don't leak + // the raw docker/daemon error at the user. + // `docker image prune` would NOT help here: it only removes dangling + // (untagged) images, and these are tagged ghcr.io/tracebloc/* refs. Point + // at the scoped removal that actually matches the failure mode — the same + // reference PruneImages targets — never a blanket prune. + p.Infof("Some tracebloc images couldn't be reclaimed (harmless) — remove them later with `docker rmi $(docker images ghcr.io/tracebloc/* --format '{{.Repository}}:{{.Tag}}')`.") } else { - p.Successf("Reclaimed the tracebloc container images.") + p.Successf("Reclaimed tracebloc's downloaded images.") } // 5. Spare or wipe ~/.tracebloc. The active-client pointer was already cleared @@ -339,32 +343,27 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er return nil } -// renderOffboardSummary prints the removed / retained / left three-way summary -// (RFC-0001 §7.10 mock) shown before the typed confirm. -func renderOffboardSummary(p *ui.Printer, name, ns string, keepData bool) { - p.Banner("tracebloc", "offboard this machine") - - p.Section("Removed from this machine") - p.Infof("This machine's credential (revoked — tracebloc can no longer see it)") - if ns != "" { - p.Infof("The Helm release %q and the local cluster %q", ns, nodeboot.ClusterName) - } else { - p.Infof("The Helm release and the local cluster %q", nodeboot.ClusterName) - } - p.Infof("The tracebloc container images (ghcr.io/tracebloc/*)") +// renderOffboardSummary prints the three-way PREVIEW — what will be removed, +// kept, and left alone — shown after the work-guard and before the typed confirm +// (RFC-0001 §7.10 mock). Plain language only; the Kubernetes/Helm/registry +// specifics stay out of the user's way. +func renderOffboardSummary(p *ui.Printer, name string, keepData bool) { + p.Section("This will remove") + p.Infof("This machine's credential — so tracebloc can no longer reach it") + p.Infof("Your secure environment %q and everything it runs on this machine", name) + p.Infof("tracebloc's downloaded images") if keepData { - p.Infof("The CLI itself (local data + config kept: --keep-data)") + p.Infof("The tb CLI (your local data & config are kept — --keep-data)") } else { - p.Infof("Local data + config (~/.tracebloc) and the CLI itself — irreversible") + p.Infof("Your local data & config (~/.tracebloc) and the tb CLI — can't be undone") } - p.Section("Kept on tracebloc, as a record") + p.Section("Kept on tracebloc") p.Infof("Your use cases and the models trained here") - p.Infof("The datasets' catalog entries (marked unavailable, not deleted)") + p.Infof("Your dataset records (marked unavailable, not deleted)") - p.Section("Left in place (system-wide)") - p.Infof("Docker, kubectl, k3d, helm — remove yourself if unused") - p.Infof("(On a GPU box: NVIDIA drivers + container toolkit)") + p.Section("Left alone") + p.Infof("Docker and related tools — remove them yourself if you no longer need them") } // removeHostDataDir deletes the tracebloc host data directory (~/.tracebloc, or diff --git a/internal/cli/delete_test.go b/internal/cli/delete_test.go index c8d89a70..9b9948a0 100644 --- a/internal/cli/delete_test.go +++ b/internal/cli/delete_test.go @@ -554,21 +554,23 @@ func TestDelete_KubeconfigContext_ReachHelm(t *testing.T) { } } -// (d) A running/online client → refuse unless --force. +// (d) A client with ACTIVE training runs → refuse unless --force. (Being merely +// "online" no longer blocks — a healthy environment always heartbeats.) func TestDelete_RunningJob_RefusesUnlessForce(t *testing.T) { newHandler := func() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { switch { case r.Method == http.MethodGet && r.URL.Path == "/edge-device/5/": - // status 1 = online (a running client). - _, _ = w.Write([]byte(`{"id":5,"first_name":"gpu-box-01","namespace":"gpu-box-01","status":1}`)) + // 2 active training runs — the work-guard blocks on this, NOT on + // status==online (which a healthy client always reports). + _, _ = w.Write([]byte(`{"id":5,"first_name":"gpu-box-01","namespace":"gpu-box-01","status":1,"num_running_experiments":2}`)) case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke"): w.WriteHeader(http.StatusOK) } } } - t.Run("online without --force refuses, no revoke", func(t *testing.T) { + t.Run("active training without --force refuses, no revoke", func(t *testing.T) { revoked := false withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke") { @@ -582,18 +584,24 @@ func TestDelete_RunningJob_RefusesUnlessForce(t *testing.T) { var out bytes.Buffer err := runDelete(context.Background(), ui.New(&out), typedNamePrompter{reply: "gpu-box-01"}, deleteOpts{}) - if err == nil || !strings.Contains(err.Error(), "still online") { - t.Fatalf("want online refusal, got %v", err) + if err == nil || !strings.Contains(err.Error(), "training run") { + t.Fatalf("want an active-training refusal, got %v", err) } if revoked { - t.Error("revoke must NOT run when the online guard refuses") + t.Error("revoke must NOT run when the work-guard refuses") } if len(fn.calls) != 0 { t.Errorf("no teardown on refusal, got: %v", fn.calls) } + // The removal PREVIEW must NOT have printed — the work-guard runs first, so + // a blocked offboard never shows a "here's what I'll remove" manifest (the + // bug: the manifest used to print before the guard, then error out). + if strings.Contains(out.String(), "This will remove") { + t.Errorf("preview must not print when the work-guard blocks:\n%s", out.String()) + } }) - t.Run("--force offboards an online client", func(t *testing.T) { + t.Run("--force offboards despite active training", func(t *testing.T) { revoked := false withClientBackend(t, func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/revoke") { @@ -610,7 +618,7 @@ func TestDelete_RunningJob_RefusesUnlessForce(t *testing.T) { t.Fatalf("offboard --force: %v", err) } if !revoked { - t.Error("--force should proceed to revoke an online client") + t.Error("--force should proceed to revoke despite active training") } }) } @@ -635,13 +643,15 @@ func TestDelete_ShowsRetainedAndLeftCopy(t *testing.T) { } s := out.String() for _, want := range []string{ - "Kept on tracebloc, as a record", + "This will remove", + "Your secure environment", + "Kept on tracebloc", "Your use cases and the models trained here", - "Left in place (system-wide)", - "Docker, kubectl, k3d, helm — remove yourself if unused", + "Left alone", + "Docker and related tools", } { if !strings.Contains(s, want) { - t.Errorf("output missing RETAINED/LEFT copy %q:\n%s", want, s) + t.Errorf("output missing preview copy %q:\n%s", want, s) } } } diff --git a/internal/nodeboot/nodeboot.go b/internal/nodeboot/nodeboot.go index 8842dd2b..82599d84 100644 --- a/internal/nodeboot/nodeboot.go +++ b/internal/nodeboot/nodeboot.go @@ -102,8 +102,10 @@ func UninstallChart(ctx context.Context, namespace, kubeconfig, kubeContext stri return err } -// PruneImages reclaims the tracebloc container images pulled during install — -// `docker images --filter=reference="ghcr.io/tracebloc/*" -q | docker rmi`. It is +// PruneImages reclaims the tracebloc container images pulled during install by +// reference — `docker images --filter=reference="ghcr.io/tracebloc/*" --format +// {{.Repository}}:{{.Tag}} | docker rmi` (by repo:tag, NOT image ID: a shared ID +// refuses `docker rmi ` — see the body). It is // SCOPED to the tracebloc image reference and best-effort by design (RFC-0001 // §7.10): reclaiming disk is a nice-to-have on offboard, not a hard step, so a // docker failure or an image still in use by a container is not fatal. It is @@ -112,23 +114,36 @@ func UninstallChart(ctx context.Context, namespace, kubeconfig, kubeContext stri // // No matching images (nothing to reclaim) is a clean no-op, not an error. func PruneImages(ctx context.Context) error { - out, err := run(ctx, "docker", "images", "--filter=reference="+imageReference, "-q") + // List by REFERENCE (repo:tag), not image ID (-q). An image ID shared across + // multiple repositories refuses `docker rmi ` ("must be forced — image is + // referenced in multiple repositories"). Removing by reference untags only OUR + // tracebloc references — Docker deletes the underlying image only when nothing + // else references it — so we never need a force that could evict an image a + // non-tracebloc workload shares (cli delete image-reclaim bug). + out, err := run(ctx, "docker", "images", "--filter=reference="+imageReference, + "--format", "{{.Repository}}:{{.Tag}}") if err != nil { return err } - // De-duplicate the image IDs: a single image tagged multiple times - // (ghcr.io/tracebloc/x:1 and :latest) lists its ID once per tag, and passing - // the same ID twice to `docker rmi` makes the second reference error. - ids := dedupeLines(out) - if len(ids) == 0 { + // Dedupe and drop dangling refs: an image that lost its tag prints "" + // for the repo or tag, which can't be removed by reference (`docker image + // prune` reclaims those). Best-effort, so skipping them is fine. + var refs []string + for _, ref := range dedupeLines(out) { + if strings.Contains(ref, "") { + continue + } + refs = append(refs, ref) + } + if len(refs) == 0 { return nil // nothing tracebloc-owned to reclaim } - _, err = run(ctx, "docker", append([]string{"rmi"}, ids...)...) + _, err = run(ctx, "docker", append([]string{"rmi"}, refs...)...) return err } -// dedupeLines splits combined `docker images -q` output into unique, non-empty, -// order-preserving lines (image IDs). +// dedupeLines splits combined `docker images` output into unique, non-empty, +// order-preserving lines (image IDs or repo:tag references). func dedupeLines(out string) []string { seen := map[string]struct{}{} var ids []string diff --git a/internal/nodeboot/nodeboot_test.go b/internal/nodeboot/nodeboot_test.go index 4d007c8f..376210db 100644 --- a/internal/nodeboot/nodeboot_test.go +++ b/internal/nodeboot/nodeboot_test.go @@ -195,40 +195,66 @@ func TestUninstallChart(t *testing.T) { } func TestPruneImages(t *testing.T) { - t.Run("removes tracebloc images, scoped + deduped", func(t *testing.T) { + // Removal is by REFERENCE (repo:tag), not image ID (-q): an ID shared across + // repos refuses `docker rmi ` ("must be forced"). Removing by reference + // untags only our refs and never needs a force. + const listCmd = `docker images --filter=reference=ghcr.io/tracebloc/* --format {{.Repository}}:{{.Tag}}` + + t.Run("removes tracebloc images by reference, scoped + deduped", func(t *testing.T) { f := newFakeRunner() - // Two tags of the same image list the ID twice; PruneImages must pass it once. - f.on(`docker images --filter=reference=ghcr.io/tracebloc/* -q`, "aaa111\nbbb222\naaa111\n", nil) + // A reference listed twice must be passed to rmi once. + f.on(listCmd, "ghcr.io/tracebloc/jobs-manager:1.9.5\nghcr.io/tracebloc/requests-proxy:1.9.5\nghcr.io/tracebloc/jobs-manager:1.9.5\n", nil) f.install(t) if err := PruneImages(context.Background()); err != nil { t.Fatalf("unexpected error: %v", err) } want := []string{ - `docker images --filter=reference=ghcr.io/tracebloc/* -q`, - "docker rmi aaa111 bbb222", + listCmd, + "docker rmi ghcr.io/tracebloc/jobs-manager:1.9.5 ghcr.io/tracebloc/requests-proxy:1.9.5", } if !reflect.DeepEqual(f.calls, want) { t.Fatalf("calls = %v, want %v", f.calls, want) } - // Guard the SCOPED contract: an offboard must never blanket-prune. + // SCOPED contract: never a blanket prune, and never a force (which could + // evict an image a non-tracebloc workload shares — the whole reason we + // switched from -q/ID to by-reference). for _, c := range f.calls { if strings.Contains(c, "system prune") { t.Fatalf("PruneImages must never run `docker system prune`; got %q", c) } + if strings.Contains(c, "rmi") && (strings.Contains(c, " -f") || strings.Contains(c, "--force")) { + t.Fatalf("PruneImages must never force-remove; got %q", c) + } + } + }) + + t.Run("dangling references are skipped", func(t *testing.T) { + f := newFakeRunner() + f.on(listCmd, "ghcr.io/tracebloc/jobs-manager:1.9.5\n:\nghcr.io/tracebloc/x:latest\n", nil) + f.install(t) + + if err := PruneImages(context.Background()); err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := []string{ + listCmd, + "docker rmi ghcr.io/tracebloc/jobs-manager:1.9.5 ghcr.io/tracebloc/x:latest", + } + if !reflect.DeepEqual(f.calls, want) { + t.Fatalf("calls = %v, want %v (must skip )", f.calls, want) } }) t.Run("no matching images is a clean no-op", func(t *testing.T) { f := newFakeRunner() - f.on(`docker images --filter=reference=ghcr.io/tracebloc/* -q`, "\n \n", nil) + f.on(listCmd, "\n \n", nil) f.install(t) if err := PruneImages(context.Background()); err != nil { t.Fatalf("unexpected error: %v", err) } - // No rmi when there's nothing to remove. - want := []string{`docker images --filter=reference=ghcr.io/tracebloc/* -q`} + want := []string{listCmd} // no rmi when there's nothing to remove if !reflect.DeepEqual(f.calls, want) { t.Fatalf("calls = %v, want %v", f.calls, want) } @@ -236,12 +262,12 @@ func TestPruneImages(t *testing.T) { t.Run("best-effort: rmi failure surfaces to the caller", func(t *testing.T) { f := newFakeRunner() - f.on(`docker images --filter=reference=ghcr.io/tracebloc/* -q`, "aaa111", nil) - f.on("docker rmi aaa111", "image is being used by running container", errors.New("exit 1")) + f.on(listCmd, "ghcr.io/tracebloc/x:1", nil) + f.on("docker rmi ghcr.io/tracebloc/x:1", "image is being used by running container", errors.New("exit 1")) f.install(t) // PruneImages returns the error; the CALLER (tracebloc delete) treats it as - // best-effort and only warns — that policy lives in the command, not here. + // best-effort and only notes it — that policy lives in the command, not here. if err := PruneImages(context.Background()); err == nil { t.Fatal("want the rmi error surfaced, got nil") } @@ -249,7 +275,7 @@ func TestPruneImages(t *testing.T) { t.Run("listing failure surfaces", func(t *testing.T) { f := newFakeRunner() - f.on(`docker images --filter=reference=ghcr.io/tracebloc/* -q`, "", errors.New("docker daemon not running")) + f.on(listCmd, "", errors.New("docker daemon not running")) f.install(t) if err := PruneImages(context.Background()); err == nil {