diff --git a/candy/plugin-check/bed_debug_hint_test.go b/candy/plugin-check/bed_debug_hint_test.go index 65f7920..530d1a6 100644 --- a/candy/plugin-check/bed_debug_hint_test.go +++ b/candy/plugin-check/bed_debug_hint_test.go @@ -43,3 +43,27 @@ func TestDebugRetentionNotice_VMHintsUseDeployDomain(t *testing.T) { t.Errorf("destroy hint is missing the --domain the cleanup path uses:\n%s", got) } } + +// TestDebugRetentionNotice_LocalHintUsesDeployDel pins the local half of the same +// operator-facing guarantee (opencharly/plugin-check#129): a kind:local apply is torn +// down with the deploy-tree verb `charly deploy del`, exactly what the cleanup path +// issues, never the container verb `charly remove` — which dies with `no container +// engine found` on the host venues a local bed actually runs on. +func TestDebugRetentionNotice_LocalHintUsesDeployDel(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + printDebugRetentionNotice(w, "check-local-bed", spec.CheckBedReply{IsLocal: true}, "") + _ = w.Close() + buf := make([]byte, 4096) + n, _ := r.Read(buf) + got := string(buf[:n]) + + if !strings.Contains(got, "charly deploy del check-local-bed") { + t.Errorf("local hint does not print the deploy verb the cleanup path issues:\n%s", got) + } + if strings.Contains(got, "charly remove") { + t.Errorf("local hint still prints the container verb `charly remove`:\n%s", got) + } +} diff --git a/candy/plugin-check/bed_local_teardown_test.go b/candy/plugin-check/bed_local_teardown_test.go new file mode 100644 index 0000000..a39e831 --- /dev/null +++ b/candy/plugin-check/bed_local_teardown_test.go @@ -0,0 +1,84 @@ +package check + +import ( + "os" + "strings" + "testing" +) + +// TestLocalBedTeardownNeverUsesTheContainerVerb is the regression guard for +// opencharly/plugin-check#129. A kind:local (`host: local`) bed was torn down with the +// CONTAINER verb `charly remove --purge` in BOTH cleanup switches, because each switch +// tested d.IsExternal alone and let d.IsLocal fall through into the pod arm. The arm is +// reached only after every authored assertion has passed, so a host venue with no +// container engine (a kind:local venue's normal shape) could never pass a bed — the +// run ended FAIL (steps=N) with `no container engine found (install podman or docker)` +// at the LAST step. +// +// The test reads bed_run.go and requires the ONE teardown decision to own both halves: +// - no switch may test d.IsExternal alone (the fall-through this defect is made of); +// - the pre-run switch and the final cleanup switch must both call +// bedRootTeardownArgs(d, name, isKubeVirt); +// - the failure-path retention hint for a local bed must print `charly deploy del`, +// the verb the cleanup path itself issues, never `charly remove`. +// +// It is a source-structure test on purpose: the two switches live inside the bed-run +// closure and are only reachable after a full deploy, so a unit test cannot walk them +// without a venue. The behavioural half of the same predicate is pinned in +// bed_root_teardown_test.go. +func TestLocalBedTeardownNeverUsesTheContainerVerb(t *testing.T) { + src, err := os.ReadFile("bed_run.go") + if err != nil { + t.Fatalf("reading bed_run.go: %v", err) + } + text := string(src) + + preRunCalls, cleanupCalls := 0, 0 + for i, line := range strings.Split(text, "\n") { + // The defect's signature: an in-place arm guarded by IsExternal ONLY, so a + // local bed falls through to the container arm below it. + if strings.Contains(line, "if d.IsExternal {") { + t.Errorf("bed_run.go:%d tests IsExternal alone — a local bed falls through to the container arm: %s", + i+1, strings.TrimSpace(line)) + } + if strings.Contains(line, "bestEffort(bedRootTeardownArgs(d, name, isKubeVirt)...)") { + preRunCalls++ + } + if strings.Contains(line, `step("cleanup", bedRootTeardownArgs(d, name, isKubeVirt)...)`) { + cleanupCalls++ + } + } + if preRunCalls != 1 { + t.Errorf("pre-run cleanup routes through bedRootTeardownArgs at %d sites, want 1 — the pre-run and final teardown decisions must be the SAME helper", preRunCalls) + } + if cleanupCalls != 1 { + t.Errorf("final cleanup routes through bedRootTeardownArgs at %d sites, want 1 — the pre-run and final teardown decisions must be the SAME helper", cleanupCalls) + } + + hint := localRetentionHint(text) + if hint == "" { + t.Fatal("could not locate the local retention hint in bed_run.go") + } + if !strings.Contains(hint, "charly deploy del") { + t.Errorf("local retention hint does not print the deploy verb:\n%s", hint) + } + if strings.Contains(hint, "charly remove") { + t.Errorf("local retention hint still prints the container verb `charly remove`:\n%s", hint) + } +} + +// localRetentionHint returns the `case d.IsLocal:` block of printDebugRetentionNotice — +// the operator-facing hint printed when a FAILED local bed's apply is left in place. +func localRetentionHint(src string) string { + const marker = "local apply left in place for debugging" + i := strings.Index(src, marker) + if i < 0 { + return "" + } + start := strings.LastIndex(src[:i], "case d.IsLocal:") + end := strings.Index(src[i:], "case d.IsExternal:") + if start < 0 || end < 0 { + return "" + } + return src[start : i+end] +} diff --git a/candy/plugin-check/bed_root_teardown_test.go b/candy/plugin-check/bed_root_teardown_test.go new file mode 100644 index 0000000..e36ef73 --- /dev/null +++ b/candy/plugin-check/bed_root_teardown_test.go @@ -0,0 +1,67 @@ +package check + +import ( + "reflect" + "testing" + + "github.com/opencharly/spec/spec" +) + +// TestBedRootTeardownArgsForEveryVenue pins the ONE root-teardown decision both cleanup +// switches read (opencharly/plugin-check#129). An in-place venue — kind:local (host-rooted) +// or external-in-place — has no container lifecycle, so it must be torn down with the +// deploy-tree verb `charly deploy del` and must NEVER reach the container verb +// `charly remove`. A kubevirt root is also `deploy del` (its plugin owns the CR teardown); +// only a container pod keeps `remove --purge` (the sole path to purgeDeployArtifacts). +// +// The mutant this kills: restoring the pre-fix predicate (an arm that tests IsExternal +// only) makes the local row produce `["remove", …]` and fails the test. +func TestBedRootTeardownArgsForEveryVenue(t *testing.T) { + const name = "check-k3s-shared-containerd" + cases := []struct { + name string + d spec.CheckBedReply + kubevirt bool + want []string + }{ + {"kind:local host venue → deploy del", spec.CheckBedReply{IsLocal: true}, false, []string{"deploy", "del", name}}, + {"external in-place → deploy del", spec.CheckBedReply{IsExternal: true}, false, []string{"deploy", "del", name}}, + {"kubevirt root → deploy del", spec.CheckBedReply{}, true, []string{"deploy", "del", name}}, + {"container pod → remove --purge", spec.CheckBedReply{}, false, []string{"remove", name, "--purge"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := bedRootTeardownArgs(tc.d, name, tc.kubevirt) + if !reflect.DeepEqual(got, tc.want) { + t.Fatalf("bedRootTeardownArgs(%+v, kubevirt=%v) = %v, want %v", tc.d, tc.kubevirt, got, tc.want) + } + // The defect's user-visible symptom: a local bed invoking the container verb. + if (tc.d.IsLocal || tc.d.IsExternal) && got[0] == "remove" { + t.Errorf("in-place venue %+v invoked the container verb %q; it has no container to remove", tc.d, got[0]) + } + }) + } +} + +// TestBedInPlaceCoversBothInPlaceVenues pins the shared predicate: only a local or an +// external-in-place venue is in place. A pod must not be, or its `charly config`/`start` +// bring-up would be skipped as well. +func TestBedInPlaceCoversBothInPlaceVenues(t *testing.T) { + cases := []struct { + name string + d spec.CheckBedReply + want bool + }{ + {"kind:local is in place", spec.CheckBedReply{IsLocal: true}, true}, + {"external-in-place is in place", spec.CheckBedReply{IsExternal: true}, true}, + {"local external is in place", spec.CheckBedReply{IsLocal: true, IsExternal: true}, true}, + {"container pod is NOT in place", spec.CheckBedReply{}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := bedInPlace(tc.d); got != tc.want { + t.Fatalf("bedInPlace(%+v) = %v, want %v", tc.d, got, tc.want) + } + }) + } +} diff --git a/candy/plugin-check/bed_run.go b/candy/plugin-check/bed_run.go index cac79aa..1075838 100644 --- a/candy/plugin-check/bed_run.go +++ b/candy/plugin-check/bed_run.go @@ -312,6 +312,33 @@ func vmBedDestroyArgs(d spec.CheckBedReply, opts bedRunOpts, snap *spec.VmSnapsh return argv } +// bedInPlace reports whether the bed's venue is applied IN PLACE: a kind:local +// (host-rooted) deploy or an external-in-place substrate (spec/deploy's HostRooted and +// ExternalInPlaceVenue). Neither has a container/VM lifecycle — their candies are applied +// during `charly deploy add` — so their teardown is the deploy-tree verb +// `charly deploy del`, never the container verb `charly remove`. This is the ONE +// predicate every call site reads (R2): the isInPlace gate and the root-teardown helper +// below, so a local bed cannot fall through into a pod arm again +// (opencharly/plugin-check#129). +func bedInPlace(d spec.CheckBedReply) bool { + return d.IsLocal || d.IsExternal +} + +// bedRootTeardownArgs is the ONE root-venue teardown command for a NON-VM bed; the VM's +// own `vm destroy` arm (which carries the domain/disk flags) stays separate. A kubevirt +// root (its plugin owns the CR teardown) and both in-place venues are torn down with the +// deploy verb `charly deploy del`; a container pod keeps `remove --purge`, which is the +// ONLY path that reaches purgeDeployArtifacts (named volumes, gocryptfs volumes and the +// synthesized -overlay images). Both cleanup switches call this helper, so the two +// halves of a bed run cannot drift — and a local bed cannot reach the container verb +// (opencharly/plugin-check#129). +func bedRootTeardownArgs(d spec.CheckBedReply, name string, isKubeVirt bool) []string { + if isKubeVirt || bedInPlace(d) { + return []string{"deploy", "del", name} + } + return []string{"remove", name, "--purge"} +} + // keepVenueOnSuccess is the SUCCESS tail's retention decision, and it is deliberately the // PRE-CHANGE gate exactly: `--keep` / `--keep-venue` / a bed's `keep_venue:` policy (all of which // force opts.Keep). It carries no other arm, so a passing run tears its venue down exactly as it @@ -726,17 +753,11 @@ func runCheckBed(ctx context.Context, ex *sdk.Executor, name string, opts bedRun if opts.Anchor == "" { bestEffort("vm", "destroy", d.VMTemplate, "--domain", d.BedDomain, "--if-exists") } - case isKubeVirt: - // A kubevirt bed's teardown is `charly deploy del` — candy/plugin-kubevirt's - // OpPostTeardown removes the VirtualMachine CR and stops the managed port-forward - // (the libvirt-domain teardown does not apply). Same in-place external shape. - bestEffort("deploy", "del", name) default: - if d.IsExternal { - bestEffort("deploy", "del", name) - } else { - bestEffort("remove", name, "--purge") - } + // Non-VM roots: kubevirt, in-place local/external and pod all take the ONE + // teardown decision (bedRootTeardownArgs) — a kubevirt CR and an in-place venue + // are `deploy del`, a container pod is `remove --purge`, never the reverse. + bestEffort(bedRootTeardownArgs(d, name, isKubeVirt)...) _ = deploykit.TearDownMembers(ctx, &bedNode) } @@ -960,12 +981,11 @@ func runCheckBed(ctx context.Context, ex *sdk.Executor, name string, opts bedRun switch { case d.IsVM: targetErr = step("cleanup", vmBedDestroyArgs(d, opts, bedNode.Snapshot)...) - case isKubeVirt: - targetErr = step("cleanup", "deploy", "del", name) - case d.IsExternal: - targetErr = step("cleanup", "deploy", "del", name) default: - targetErr = step("cleanup", "remove", name, "--purge") + // Every non-VM root takes the ONE teardown decision — kubevirt and both + // in-place venues via `deploy del`, a container pod via `remove --purge`. + // A local bed has no container engine to remove from (#129). + targetErr = step("cleanup", bedRootTeardownArgs(d, name, isKubeVirt)...) } membersErr := phase("cleanup-members", func() error { return deploykit.TearDownMembers(ctx, &bedNode) @@ -1050,8 +1070,9 @@ func runCheckBed(ctx context.Context, ex *sdk.Executor, name string, opts bedRun // isInPlace unifies local + in-place-external: they apply candies in place during // `charly deploy add` (no container/VM lifecycle — no `charly config`/`charly - // start`, teardown via `charly deploy del`). - isInPlace := d.IsLocal || d.IsExternal + // start`, teardown via `charly deploy del`). ONE predicate (bedInPlace) with the + // cleanup switches above, so the bring-up and teardown halves cannot drift. + isInPlace := bedInPlace(d) // Steps 1+2: image build + check box. A VM/kubevirt substrate is an image-backed // CR (no box build); a kind:local deploy carries no image. An EXTERNAL-IN-PLACE @@ -1550,8 +1571,10 @@ func printDebugRetentionNotice(w io.Writer, name string, d spec.CheckBedReply, r " destroy: charly vm destroy %s --domain %s\n", name, "charly-"+d.BedDomain, live, d.BedDomain, d.VMTemplate, d.BedDomain) case d.IsLocal: + // A local apply has no container to remove: its teardown is the deploy-tree verb, + // the same one the cleanup path issues (bedRootTeardownArgs). fmt.Fprintf(w, "\n[charly check run] bed %q FAILED — local apply left in place for debugging.\n"+ - " destroy: charly remove %s\n", name, name) + " destroy: charly deploy del %s\n", name, name) case d.IsExternal: fmt.Fprintf(w, "\n[charly check run] bed %q FAILED — external deploy apply left in place for debugging.\n"+ " destroy: charly deploy del %s\n", name, name)