diff --git a/.github/workflows/runtime-proof.yml b/.github/workflows/runtime-proof.yml index 37b5771c..27682020 100644 --- a/.github/workflows/runtime-proof.yml +++ b/.github/workflows/runtime-proof.yml @@ -415,6 +415,13 @@ jobs: - name: feint run sweeps the machines it created, and only those run: sudo tools/conformance/run-sweep.sh http://127.0.0.1:4599 + # The host half of a reset (#853). Its third assertion is the + # product: a reset that swept both machines would return an + # operator to a checkpoint whose servers answer nothing, which is + # worse than no reset at all. + - name: a reset reconciles the host and touches nothing else + run: sudo tools/conformance/reset-host.sh http://127.0.0.1:4599 + # The end of the chain the emulator exists for: a key registered through # the API, a server booted by the official client, a real shell on the # provider's own default login — three providers, three logins. diff --git a/README.fr.md b/README.fr.md index f9afee5f..e1c40178 100644 --- a/README.fr.md +++ b/README.fr.md @@ -666,6 +666,8 @@ n'est une étape dans le travail de qui que ce soit. | `feint doctor` | diagnostique l'hôte : le port, le runtime, les clients | | `feint env ` | l'environnement dont un vrai client de ce provider a besoin | | `feint snapshot` | nomme l'état d'un émulateur qui tourne et y revient : `save`, `load`, `list`, `rm` | +| `feint checkpoint` | nomme l'état d'un émulateur qui tourne : plan de contrôle, octets, et ce qu'un client peut en observer | +| `feint reset` | revient à ce point, retire ce qu'il ne contient pas, et dit si le laboratoire a convergé | | `feint images` | construit les images de machines, qui portent un démon ssh pour répondre sans dépôt de paquets | | `feint clean` | retire les machines, réseaux et jeux de règles que l'émulateur a créés | | `feint version` | affiche la version | diff --git a/README.md b/README.md index b94e2303..17b7a64f 100644 --- a/README.md +++ b/README.md @@ -1064,6 +1064,8 @@ workflow. | `feint env ` | the environment a real client needs, for `eval` | | `feint doctor` | diagnose the host: the port, the machine runtime, the clients, the ssh trap | | `feint snapshot` | `save`, `load`, `list`, `rm` — name a running emulator's state and come back to it | +| `feint checkpoint` | name the state of a running emulator — control plane, object bytes, and what a client can observe of both | +| `feint reset` | return to that checkpoint, take down what it does not hold, and say whether the lab converged | | `feint serve` | serve the three emulated clouds on one port, in the foreground | | `feint images` | build the machine images, which carry an ssh daemon so a machine answers without a package repository | | `feint clean` | remove every machine, network and rule set the emulator created | diff --git a/internal/cli/checkpoint.go b/internal/cli/checkpoint.go new file mode 100644 index 00000000..d3d7fdf7 --- /dev/null +++ b/internal/cli/checkpoint.go @@ -0,0 +1,407 @@ +package cli + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "strings" + "time" + + "github.com/stephrobert/feint/internal/core/machine" +) + +// `feint checkpoint` and `feint reset`: transaction-like test isolation (#853). +// +// feint checkpoint baseline +// # run tests, create resources, fail halfway +// feint reset baseline +// +// The emulator owns the state transitions, and these two own the VERDICT. +// That split is the whole design, and #853 states why in one line: "reset is +// not successful because JSON was restored". So a reset here is judged by +// comparing what a client can observe before and after, never by the status +// code of the call that performed it. +// +// ## What a checkpoint records locally, and why +// +// The emulator holds the state. These commands additionally record the +// OBSERVED state beside the instance, because that is what a later reset is +// measured against. Deriving it from the restored store instead would compare +// a restore to itself: the containers and the bytes would be out of the +// picture, which is exactly the half that can diverge. +// +// ## The exit code +// +// 0 when the observable state converged, 1 when it did not, with every +// difference named. Not 2: this repository reserves that for upstream drift, +// and a reset that failed to deliver its guarantee is an error in the ordinary +// sense. A reset that printed differences and exited 0 would be a gate nobody +// can build on. + +// checkpointStateVersion is the shape of the file a checkpoint records beside +// its instance. Frozen from the start, like the emulator's own schemas: a +// suite that reads it is a consumer on day one. +const checkpointStateVersion = 1 + +// recordedCheckpoint is what `feint checkpoint` writes and `feint reset` reads. +type recordedCheckpoint struct { + SchemaVersion int `json:"schema_version"` + Name string `json:"name"` + Taken string `json:"taken"` + Runtime string `json:"runtime"` + State observed `json:"state"` +} + +// checkpointArgName takes the leading name off the arguments, so that the +// flags behind it are still flags. +// +// Shared with the refusals a name must survive: the byte store writes a +// directory per checkpoint, so a name is a path component before it is a +// name, and the emulator refuses the same shapes at its own door. Refusing +// here too means the operator hears it before a request is sent. +// +// TestACheckpointNameIsTakenBeforeTheFlags fails without this. +func checkpointArgName(args []string, stderr io.Writer, verb string) (string, []string, bool) { + if len(args) == 0 || strings.HasPrefix(args[0], "-") { + fmt.Fprintf(stderr, "feint %s: name one checkpoint: feint %s [flags]\n", verb, verb) + return "", nil, false + } + name := args[0] + if name == "." || name == ".." || strings.ContainsAny(name, `/\`) { + fmt.Fprintf(stderr, "feint %s: %q is not a checkpoint name: no path separators, no . or ..\n", verb, name) + return "", nil, false + } + return name, args[1:], true +} + +// checkpointCommand takes a named checkpoint of a running lab. +func checkpointCommand(args []string, stdout, stderr io.Writer) int { + // The name comes first and the flags after it, the way `feint snapshot + // save --addr …` already reads. Go's flag package stops at the + // first non-flag argument, so a name parsed by it swallows every flag + // behind it into the positionals — which is exactly what the first + // measurement of this command did, refusing `feint checkpoint baseline + // --addr 127.0.0.1:4621` on its own usage line. + // The flag set is built BEFORE the name is taken off, and that order is + // not cosmetic: `feint checkpoint -h` must print the flags, and the first + // version refused on its own usage line instead. The frozen-surface + // observation found it, because a verb that builds no flag set freezes + // nothing. + fs := newFlagSet("checkpoint") + addr := fs.String("addr", DefaultAddr, "listen address of the emulator to checkpoint") + if len(args) != 0 && strings.HasPrefix(args[0], "-") { + // -h, --help, or a mistyped flag: let the flag set answer, which is + // the only thing that can print the usage. + if err := fs.Parse(args); err != nil { + return exitError + } + } + name, rest, ok := checkpointArgName(args, stderr, "checkpoint") + if !ok { + return exitError + } + if err := fs.Parse(rest); err != nil { + return exitError + } + if fs.NArg() != 0 { + fmt.Fprintf(stderr, "feint checkpoint: unexpected argument %q; one name, then flags\n", fs.Arg(0)) + return exitError + } + endpoint := "http://" + *addr + + // The emulator's half first: a checkpoint whose state was observed and + // whose store was never taken is a checkpoint a reset cannot honour, and + // the observation would read as a success. + answer, err := callCheckpoint(http.MethodPost, endpoint+"/_feint/checkpoint/"+name) + if err != nil { + fmt.Fprintf(stderr, "feint checkpoint: %v\n", err) + return exitError + } + + state, runtimeName, err := observeFor(endpoint) + if err != nil { + fmt.Fprintf(stderr, "feint checkpoint: the checkpoint was taken and could not be read back: %v\n", err) + return exitError + } + if err := writeRecordedCheckpoint(*addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion, Name: name, + Taken: time.Now().UTC().Format(time.RFC3339), Runtime: runtimeName, State: state, + }); err != nil { + fmt.Fprintf(stderr, "feint checkpoint: %v\n", err) + return exitError + } + + fmt.Fprintf(stdout, "checkpoint %q: %d resource(s), %d object(s), %d machine(s)\n", + name, len(state.Resources), len(state.Objects), len(state.Machines)) + if carried, _ := answer["bytes"].(bool); !carried { + // Said rather than left out: a reader who resets expecting bytes back + // would otherwise find none and blame the reset. + fmt.Fprintln(stdout, " bytes: this emulator serves no object store, so none are part of it") + } + return exitOK +} + +// resetCommand returns a lab to a checkpoint and judges whether it converged. +func resetCommand(args []string, stdout, stderr io.Writer) int { + // The name is optional here, so the split is by shape rather than by + // position: a leading argument that does not start with a dash is the + // name, everything else is flags. Same reason as above. + fs := newFlagSet("reset") + addr := fs.String("addr", DefaultAddr, "listen address of the emulator to reset") + asked, rest := "", args + if len(args) != 0 && !strings.HasPrefix(args[0], "-") { + asked, rest = args[0], args[1:] + } + if err := fs.Parse(rest); err != nil { + return exitError + } + if fs.NArg() != 0 { + fmt.Fprintf(stderr, "feint reset: unexpected argument %q; one name at most, then flags\n", fs.Arg(0)) + return exitError + } + endpoint := "http://" + *addr + + recorded, err := readRecordedCheckpoint(*addr) + if err != nil { + fmt.Fprintf(stderr, "feint reset: %v\n", err) + fmt.Fprintf(stderr, " Take one first: feint checkpoint --addr %s\n", *addr) + return exitError + } + name := recorded.Name + if asked != "" { + name = asked + } + if name != recorded.Name { + // Refused rather than guessed. The recorded state is what a verdict + // compares against, and comparing one checkpoint's emulator state to + // another's observation would report differences that are not the + // reset's fault. + fmt.Fprintf(stderr, "feint reset: this instance recorded %q, not %q, so a verdict on %q would compare the wrong states\n", + recorded.Name, name, name) + return exitError + } + + answer, err := callCheckpoint(http.MethodPost, endpoint+"/_feint/reset/"+name) + if err != nil { + fmt.Fprintf(stderr, "feint reset: %v\n", err) + return exitError + } + reportReconciliation(stdout, answer) + + state, _, err := observeFor(endpoint) + if err != nil { + fmt.Fprintf(stderr, "feint reset: the reset ran and could not be read back: %v\n", err) + return exitError + } + + // The verdict, and the only one worth having: what a client can observe, + // compared with what it could observe at the checkpoint. + differences := recorded.State.diff(state) + if len(differences) == 0 { + fmt.Fprintf(stdout, "reset %q: the lab converged\n", name) + return exitOK + } + fmt.Fprintf(stderr, "feint reset: %q did not converge; %d difference(s) remain:\n", name, len(differences)) + for _, difference := range differences { + fmt.Fprintf(stderr, " %s\n", difference) + } + if recreates, _ := answer["recreates"].(bool); !recreates { + fmt.Fprintln(stderr, " This reset removes what the checkpoint does not hold and recreates nothing:") + fmt.Fprintln(stderr, " a machine the checkpoint had and the host no longer does needs a boot only") + fmt.Fprintln(stderr, " its pack can render, and docs/limits.md says so.") + } + return exitError +} + +// reportReconciliation prints what the emulator says it did, before the +// verdict says whether it was enough. +func reportReconciliation(stdout io.Writer, answer map[string]any) { + destroyed := stringsOf(answer["destroyed"]) + skipped := stringsOf(answer["skipped"]) + if len(destroyed) != 0 { + fmt.Fprintf(stdout, " destroyed: %s\n", strings.Join(destroyed, ", ")) + } + if len(skipped) != 0 { + // A record nobody could act on is what an operator needs to hear + // about: it is the difference between "nothing to do" and "this run + // has no runtime". + fmt.Fprintf(stdout, " not acted on: %s\n", strings.Join(skipped, ", ")) + } +} + +func stringsOf(value any) []string { + items, ok := value.([]any) + if !ok { + return nil + } + out := make([]string, 0, len(items)) + for _, item := range items { + if text, ok := item.(string); ok { + out = append(out, text) + } + } + return out +} + +// observeFor reads the observable state, resolving the runtime the emulator +// actually declares. +// +// From /_feint/health rather than from a flag, for the reason the run receipt +// does the same: a capability is declared by the driver, and asking the host +// for a mode nobody is running would refuse on a station that has none. +func observeFor(endpoint string) (observed, string, error) { + name, _, _ := labHealth(endpoint) + rt := machine.Runtime{} + if name != "" && name != "none" && name != "off" { + resolved, err := resolveRuntime(name, io.Discard) + if err != nil { + // Not fatal: the host half stays empty and the verdict says so by + // comparing fewer axes. Refusing here would make a reset + // impossible on a station whose runtime went away between the + // checkpoint and now, which is the moment a reset is most needed. + return mustObserve(endpoint, rt), name, nil + } + rt = resolved + } + return mustObserve(endpoint, rt), name, nil +} + +func mustObserve(endpoint string, rt machine.Runtime) observed { + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + state, err := observeLab(ctx, endpoint, rt) + if err != nil { + // Returned empty rather than propagated, because the caller already + // distinguishes "could not read it back" from "it did not converge" + // and an empty state would make every axis differ. + return observed{} + } + return state +} + +// callCheckpoint drives one of the emulator's checkpoint endpoints. +func callCheckpoint(method, url string) (map[string]any, error) { + req, err := http.NewRequestWithContext(context.Background(), method, url, nil) + if err != nil { + return nil, err + } + client := &http.Client{Timeout: 2 * time.Minute} + res, err := client.Do(req) + if err != nil { + return nil, fmt.Errorf("the emulator did not answer: %w", err) + } + defer func() { _ = res.Body.Close() }() + var body map[string]any + _ = json.NewDecoder(io.LimitReader(res.Body, maxStateBody)).Decode(&body) + if res.StatusCode != http.StatusOK { + if message, ok := body["error"].(string); ok { + return nil, fmt.Errorf("%s", message) + } + return nil, fmt.Errorf("the emulator answered %d", res.StatusCode) + } + return body, nil +} + +// checkpointPath is where the observed state lives: beside the instance +// record, because it belongs to that emulator and goes when it does. +func checkpointPath(addr string) (string, error) { + dir, err := instanceDir(addr) + if err != nil { + return "", err + } + return filepath.Join(dir, "checkpoint.json"), nil +} + +func writeRecordedCheckpoint(addr string, recorded recordedCheckpoint) error { + path, err := checkpointPath(addr) + if err != nil { + return err + } + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return err + } + body, err := json.MarshalIndent(recorded, "", " ") + if err != nil { + return err + } + return os.WriteFile(path, append(body, '\n'), 0o600) +} + +func readRecordedCheckpoint(addr string) (recordedCheckpoint, error) { + path, err := checkpointPath(addr) + if err != nil { + return recordedCheckpoint{}, err + } + body, err := os.ReadFile(filepath.Clean(path)) + if err != nil { + return recordedCheckpoint{}, fmt.Errorf("no checkpoint recorded for %s", addr) + } + var recorded recordedCheckpoint + if err := json.Unmarshal(body, &recorded); err != nil { + return recordedCheckpoint{}, fmt.Errorf("the recorded checkpoint is unreadable: %w", err) + } + if recorded.SchemaVersion != checkpointStateVersion { + return recordedCheckpoint{}, fmt.Errorf( + "the recorded checkpoint is version %d and this binary reads %d", + recorded.SchemaVersion, checkpointStateVersion) + } + return recorded, nil +} + +// takeSessionCheckpoint names the state a `feint run --reset-after` returns to. +// +// It is the lab's own session identity, not a name the operator typed: two +// runs against one shared emulator must not be able to reset each other's +// state, and a fixed name would let them. +// +// TestASessionCheckpointIsNamedAfterItsRun fails without this. +func takeSessionCheckpoint(endpoint, session, addr string) error { + if _, err := callCheckpoint(http.MethodPost, endpoint+"/_feint/checkpoint/"+session); err != nil { + return fmt.Errorf("the session checkpoint could not be taken: %w", err) + } + state, runtimeName, err := observeFor(endpoint) + if err != nil { + return fmt.Errorf("the session checkpoint could not be read back: %w", err) + } + return writeRecordedCheckpoint(addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion, Name: session, + Taken: time.Now().UTC().Format(time.RFC3339), Runtime: runtimeName, State: state, + }) +} + +// resetToSession returns the lab to its session checkpoint and answers a +// verdict plus whatever is left over. +// +// It never returns an error to the caller, and that is deliberate: the child +// has already run, and its exit code is what a caller builds on. A reset that +// could not even be attempted must be REPORTED rather than swallowed, and +// rather than raised into the child's verdict where it would read as a failing +// test. So the verdict is a word the receipt carries, and the four words are +// not interchangeable — "not attempted", "failed", "unverified" and +// "converged" send a reader to four different places. +// +// TestAFailedResetAfterIsReportedAndNotRaised fails without this. +func resetToSession(endpoint, session, addr string, stderr io.Writer) (string, []string) { + recorded, err := readRecordedCheckpoint(addr) + if err != nil || recorded.Name != session { + fmt.Fprintf(stderr, "feint run: --reset-after has no session checkpoint to return to: %v\n", err) + return "not attempted", nil + } + if _, err := callCheckpoint(http.MethodPost, endpoint+"/_feint/reset/"+session); err != nil { + fmt.Fprintf(stderr, "feint run: the reset failed: %v\n", err) + return "failed", nil + } + state, _, err := observeFor(endpoint) + if err != nil { + fmt.Fprintf(stderr, "feint run: the reset ran and could not be read back: %v\n", err) + return "unverified", nil + } + if differences := recorded.State.diff(state); len(differences) != 0 { + return "did not converge", differences + } + return "converged", nil +} diff --git a/internal/cli/checkpoint_test.go b/internal/cli/checkpoint_test.go new file mode 100644 index 00000000..48d21217 --- /dev/null +++ b/internal/cli/checkpoint_test.go @@ -0,0 +1,133 @@ +package cli + +import ( + "bytes" + "strings" + "testing" +) + +// TestACheckpointNameIsTakenBeforeTheFlags is cited by checkpointArgName. +// +// It exists because the first version of these commands let Go's flag package +// parse the name, and flag stops at the first non-flag argument: every flag +// behind the name became a positional. The first real measurement of +// `feint checkpoint baseline --addr 127.0.0.1:4621` was refused by the +// command's own usage line, which is a defect no unit test of the parsing +// would have found, because I would have written the test with the flags +// first. +func TestACheckpointNameIsTakenBeforeTheFlags(t *testing.T) { + var stderr bytes.Buffer + + name, rest, ok := checkpointArgName([]string{"baseline", "--addr", "127.0.0.1:4621"}, &stderr, "checkpoint") + if !ok { + t.Fatalf("a name followed by flags was refused: %s", stderr.String()) + } + if name != "baseline" { + t.Errorf("name %q", name) + } + if len(rest) != 2 || rest[0] != "--addr" { + t.Errorf("the flags did not survive the split: %v", rest) + } + + // No name at all, and a flag where the name should be: both are the same + // mistake from the operator's side and both must say what to type. + for _, probe := range [][]string{nil, {"--addr", "127.0.0.1:4599"}} { + stderr.Reset() + if _, _, ok := checkpointArgName(probe, &stderr, "checkpoint"); ok { + t.Errorf("%v was accepted as a name", probe) + } + if !strings.Contains(stderr.String(), "") { + t.Errorf("the refusal for %v does not say what to type: %s", probe, stderr.String()) + } + } + + // A name that is a path component before it is a name. The emulator + // refuses the same shapes at its own door; refusing here too means the + // operator hears it before a request leaves. + for _, bad := range []string{".", "..", "a/b", `a\b`} { + stderr.Reset() + if _, _, ok := checkpointArgName([]string{bad}, &stderr, "checkpoint"); ok { + t.Errorf("the name %q was accepted", bad) + } + if !strings.Contains(stderr.String(), "path separators") { + t.Errorf("the refusal of %q does not say why: %s", bad, stderr.String()) + } + } +} + +// TestAResetRefusesToJudgeAgainstAnotherCheckpointsState is the other half of +// the verdict's honesty. +// +// The recorded observable state belongs to ONE checkpoint. Comparing the +// emulator's restore of one against the observation of another would report +// differences that are not the reset's fault, and an operator would chase +// them. So a name that disagrees with the record is refused rather than +// guessed. +func TestAResetRefusesToJudgeAgainstAnotherCheckpointsState(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + const addr = "127.0.0.1:4599" + if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion, Name: "baseline", + }); err != nil { + t.Fatalf("record a checkpoint: %v", err) + } + + var stdout, stderr bytes.Buffer + if code := resetCommand([]string{"something-else", "--addr", addr}, &stdout, &stderr); code != exitError { + t.Fatalf("a reset against another checkpoint's state was allowed (%d)", code) + } + for _, want := range []string{"baseline", "something-else", "compare the wrong states"} { + if !strings.Contains(stderr.String(), want) { + t.Errorf("the refusal does not mention %q: %s", want, stderr.String()) + } + } + + // And with no record at all it says how to make one, rather than failing + // on a missing file an operator did not know existed. + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + stderr.Reset() + if code := resetCommand([]string{"--addr", addr}, &stdout, &stderr); code != exitError { + t.Fatalf("a reset with no checkpoint was allowed (%d)", code) + } + if !strings.Contains(stderr.String(), "feint checkpoint") { + t.Errorf("the refusal does not say how to take one: %s", stderr.String()) + } +} + +// TestARecordedCheckpointRefusesAVersionItCannotRead is the forward +// compatibility half. +// +// The file is a published shape — a suite that resets between tests reads it — +// so a binary meeting a version it does not know must say so rather than +// decode what it can and compare against a half-read state. A verdict built +// on a partially understood record is worse than no verdict. +func TestARecordedCheckpointRefusesAVersionItCannotRead(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + const addr = "127.0.0.1:4599" + if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion + 1, Name: "from-the-future", + }); err != nil { + t.Fatalf("record a checkpoint: %v", err) + } + if _, err := readRecordedCheckpoint(addr); err == nil { + t.Fatal("a record from a later version was read anyway") + } else if !strings.Contains(err.Error(), "version") { + t.Errorf("the refusal does not say it is a version: %v", err) + } + + // The round trip of the current version, so this cannot be satisfied by + // refusing everything. + if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion, Name: "baseline", + State: observed{Machines: []string{"feint-scw-a"}}, + }); err != nil { + t.Fatalf("record a checkpoint: %v", err) + } + back, err := readRecordedCheckpoint(addr) + if err != nil { + t.Fatalf("read it back: %v", err) + } + if back.Name != "baseline" || len(back.State.Machines) != 1 { + t.Errorf("the record did not survive the round trip: %+v", back) + } +} diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 5a0b849e..b25dfcd0 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -249,7 +249,7 @@ const ( // TestTheFrozenSurfacesStillMatchTheirFixture, and a fixture regenerated // without bumping this constant fails TestASurfaceChangeDemandsItsVersionBump. // The procedure for a deliberate change is in RELEASING.md ("Frozen surfaces"). -const cliSurfaceVersion = 30 +const cliSurfaceVersion = 32 // Run executes one command and returns the process exit code. func Run(args []string, stdout, stderr io.Writer) int { @@ -316,6 +316,10 @@ func Run(args []string, stdout, stderr io.Writer) int { return images(args[2:], stdout, stderr) case "snapshot": return snapshot(args[2:], stdout, stderr) + case "checkpoint": + return checkpointCommand(args[2:], stdout, stderr) + case "reset": + return resetCommand(args[2:], stdout, stderr) case "version", "--version", "-v": // The flag forms are aliases, not a second way of doing it: a user who // types `feint --version` and gets "unknown command" concludes the @@ -531,7 +535,8 @@ Usage: behind it on the network. feint run [--provider scaleway] [--endpoint http://127.0.0.1:4599] - [--addr 127.0.0.1:0] [--keep-on-failure] [--json] + [--addr 127.0.0.1:0] [--keep-on-failure] [--reset-after] + [--json ] [--unsafe-real-credentials] -- [args...] Run one command against a local cloud this command owns. With no --endpoint it starts an emulator, waits for it, @@ -560,12 +565,35 @@ Usage: --json writes the receipt as JSON to a path, or to stdout with a single dash; stdout otherwise belongs to your command alone. The exit code is the child's. + --reset-after takes a checkpoint once the lab is ready + and returns to it when the command ends, however it ends: + success, failure, or a signal this command relayed. It is + refused with --keep-on-failure, which keeps the state it + would erase. --runtime asks the host for a machine runtime before anything starts, with the refusal up makes. off is the default: starting containers is a side effect on this station, so it is asked for, and it needs a lab this command owns. + feint checkpoint [--addr 127.0.0.1:4599] + Name the state of a running emulator: the control plane + and, when one is armed, the object bytes. It also records + what a client can OBSERVE at that moment, beside the + instance, because that is what a later reset is measured + against. + + feint reset [] [--addr 127.0.0.1:4599] + Return to that checkpoint and say whether the lab + converged. The store is restored, the bytes are restored, + and the machines of records the checkpoint does not hold + are taken down through the ownership checks. Then the + observable state is read again and compared: exit 0 when + it matches, 1 with every remaining difference named. A + reset is not successful because JSON was restored. + It recreates nothing: a machine the checkpoint had and the + host no longer does needs a boot only its pack can render. + feint transcript [--shape OP [--against emu.jsonl]] [--sanitise corpus.jsonl --contract contracts/.json] [--format text|json] diff --git a/internal/cli/observable.go b/internal/cli/observable.go new file mode 100644 index 00000000..7c607f62 --- /dev/null +++ b/internal/cli/observable.go @@ -0,0 +1,373 @@ +package cli + +import ( + "context" + "encoding/json" + "encoding/xml" + "fmt" + "io" + "net/http" + "slices" + "strings" + "time" + + "github.com/stephrobert/feint/internal/core/machine" +) + +// The observable state of a lab, normalised so that two reads of the same +// state are equal. +// +// This exists before `feint checkpoint` and `feint reset` (#853) because +// without it those two commands cannot be honest. The issue says so in one +// line — "reset is not successful because JSON was restored" — and a reset +// that reported success from the absence of an error would be exactly the +// verdict this repository refuses: an operation's own opinion of itself. +// +// So the verdict is a COMPARISON, and this file is the thing compared. +// +// ## What is in it, and why those three +// +// - the control plane, from /_feint/resources: what the API says exists; +// - the bytes, read THROUGH the doors: what a client can fetch; +// - the host, from Runtime.Survey: what the emulator actually created. +// +// Any two of the three without the third is a reset that can lie. Restoring +// the store alone leaves containers running that the API no longer mentions; +// restoring the store and the host leaves bytes from a later test in a bucket +// the checkpoint had empty. +// +// ## Why the bytes are read through the door rather than from the store +// +// The authoritative view of an object is the one a client gets, and this +// project's whole claim is that a client cannot tell the difference. A +// comparison that read the store directly would pass while the door served +// something else, which is the half that matters to a test using it. +// +// It does mean the comparison depends on the door answering. That is not a +// circularity worth avoiding: bytes a door cannot serve are bytes no client +// can observe, and a reset whose doors are broken has not converged. +// +// ## What is deliberately NOT in it +// +// Created and Updated timestamps, because a restored resource carries the +// instant it was restored and a comparison including them could never be +// equal. What survives of time is ordering, and the store already keeps that +// in the identifiers. Attrs are not compared field by field either: a pack's +// own view can hold a derived value, and a reset is about the state, not about +// how a pack renders it. The identity, the kind and the state are what a test +// builds on. +// +// TestTwoReadsOfOneLabAreEqual and TestADifferenceIsNamedWhereverItIs fail +// without this. + +// observed is one lab's normalised state. +type observed struct { + Resources []observedResource `json:"resources"` + Objects []observedObject `json:"objects"` + Machines []string `json:"machines"` + Networks []string `json:"networks"` + Firewalls []string `json:"firewalls"` +} + +// observedResource is a control-plane record, without what cannot be equal. +type observedResource struct { + Kind string `json:"kind"` + ID string `json:"id"` + State string `json:"state,omitempty"` + // Backing is the host object this record names, and it is what ties the + // two halves together: a resource whose container changed under it is a + // resource a reset did not reconcile, even when both lists are the same + // length. + Backing string `json:"backing,omitempty"` +} + +// observedObject is one object, identified by its content rather than by a +// length: two objects of the same size are not the same object, and a reset +// that swapped them would otherwise compare equal. +type observedObject struct { + Door string `json:"door"` + Bucket string `json:"bucket"` + Key string `json:"key"` + Digest string `json:"digest"` + Size int64 `json:"size"` +} + +// observeLab reads the three halves and normalises them. +// +// A runtime that answers nothing is not an error: `--vm off` is the ordinary +// case and its host half is legitimately empty. An unreachable emulator IS an +// error, because then nothing was observed and a comparison would be between +// two silences. +func observeLab(ctx context.Context, endpoint string, rt machine.Runtime) (observed, error) { + var state observed + + inventory, err := readInventoryView(endpoint) + if err != nil { + return observed{}, fmt.Errorf("reading the control plane: %w", err) + } + for _, view := range inventory { + state.Resources = append(state.Resources, observedResource{ + Kind: view.Kind, ID: view.ID, State: view.State, + Backing: backingOf(view.Runtime), + }) + } + + objects, err := observeObjects(ctx, endpoint) + if err != nil { + return observed{}, fmt.Errorf("reading the bytes: %w", err) + } + state.Objects = objects + + // Survey names only what the emulator labelled, and its own test asserts + // it issues no mutating command. A driver that cannot survey answers + // asked=false, and the host half stays empty rather than guessed. + if left, asked, err := rt.Survey(ctx); err != nil { + return observed{}, fmt.Errorf("surveying the host: %w", err) + } else if asked { + state.Machines = append(state.Machines, left.Machines...) + state.Networks = append(state.Networks, left.Networks...) + state.Firewalls = append(state.Firewalls, left.Firewalls...) + } + + state.normalise() + return state, nil +} + +// normalise sorts every list so that two reads of one state are equal. +func (o *observed) normalise() { + slices.SortFunc(o.Resources, func(a, b observedResource) int { + if c := strings.Compare(a.Kind, b.Kind); c != 0 { + return c + } + return strings.Compare(a.ID, b.ID) + }) + slices.SortFunc(o.Objects, func(a, b observedObject) int { + for _, pair := range [][2]string{{a.Door, b.Door}, {a.Bucket, b.Bucket}, {a.Key, b.Key}} { + if c := strings.Compare(pair[0], pair[1]); c != 0 { + return c + } + } + return 0 + }) + slices.Sort(o.Machines) + slices.Sort(o.Networks) + slices.Sort(o.Firewalls) +} + +// backingOf answers the host object a record names, whatever key the pack +// used for it. +// +// Sorted and joined rather than read from one known key: the Runtime map is a +// pack's own vocabulary, three packs spell it differently, and a hardcoded key +// here would silently observe nothing for the fourth pack. What matters for a +// comparison is that the value changes when the host object changes. +func backingOf(runtime map[string]string) string { + if len(runtime) == 0 { + return "" + } + pairs := make([]string, 0, len(runtime)) + for key, value := range runtime { + pairs = append(pairs, key+"="+value) + } + slices.Sort(pairs) + return strings.Join(pairs, " ") +} + +// readInventoryView fetches /_feint/resources with the fields a comparison +// needs. up.go's readInventory drops the identity and the state, which are the +// two a reset is judged on, so this reads the fuller view rather than widening +// a struct three ready conditions already depend on. +func readInventoryView(endpoint string) ([]emulatorResourceView, error) { + client := &http.Client{Timeout: 10 * time.Second} + res, err := client.Get(strings.TrimSuffix(endpoint, "/") + "/_feint/resources") + if err != nil { + return nil, err + } + defer func() { _ = res.Body.Close() }() + if res.StatusCode != http.StatusOK { + return nil, fmt.Errorf("the inventory answered %d", res.StatusCode) + } + var body struct { + Resources []emulatorResourceView `json:"resources"` + } + if err := json.NewDecoder(io.LimitReader(res.Body, maxStateBody)).Decode(&body); err != nil { + return nil, err + } + return body.Resources, nil +} + +// emulatorResourceView is the part of emulator.ResourceView a comparison uses. +// Declared here rather than imported so that a field added upstream cannot +// silently join the comparison: a reset's verdict must change only when +// somebody decides it does. +type emulatorResourceView struct { + ID string `json:"id"` + Kind string `json:"kind"` + State string `json:"state"` + Runtime map[string]string `json:"runtime"` +} + +// observeObjects lists every door's buckets and their objects, through the +// doors. +// +// An emulator serving no doors answers an empty list and no error: object +// storage is opt-in, and a lab without it has no bytes to converge. +func observeObjects(ctx context.Context, endpoint string) ([]observedObject, error) { + report, err := fetchS3(endpoint) + if err != nil { + return nil, err + } + if !report.Enabled || len(report.Doors) == 0 { + return nil, nil + } + client := &http.Client{Timeout: 30 * time.Second} + var out []observedObject + for _, door := range report.Doors { + base := strings.TrimSuffix(endpoint, "/") + door.Path + buckets, err := listBuckets(ctx, client, base) + if err != nil { + return nil, fmt.Errorf("%s: %w", door.Path, err) + } + for _, bucket := range buckets { + objects, err := listObjects(ctx, client, base, bucket) + if err != nil { + return nil, fmt.Errorf("%s%s: %w", door.Path, bucket, err) + } + for _, object := range objects { + out = append(out, observedObject{ + Door: strings.Trim(door.Path, "/"), Bucket: bucket, + Key: object.Key, Digest: object.ETag, Size: object.Size, + }) + } + } + } + return out, nil +} + +type bucketListing struct { + Buckets struct { + Bucket []struct { + Name string `xml:"Name"` + } `xml:"Bucket"` + } `xml:"Buckets"` +} + +type objectListing struct { + Contents []struct { + Key string `xml:"Key"` + ETag string `xml:"ETag"` + Size int64 `xml:"Size"` + } `xml:"Contents"` + IsTruncated bool `xml:"IsTruncated"` + NextToken string `xml:"NextContinuationToken"` +} + +func listBuckets(ctx context.Context, client *http.Client, base string) ([]string, error) { + var listing bucketListing + if err := getXML(ctx, client, base, &listing); err != nil { + return nil, err + } + names := make([]string, 0, len(listing.Buckets.Bucket)) + for _, bucket := range listing.Buckets.Bucket { + names = append(names, bucket.Name) + } + slices.Sort(names) + return names, nil +} + +func listObjects(ctx context.Context, client *http.Client, base, bucket string) ([]struct { + Key string `xml:"Key"` + ETag string `xml:"ETag"` + Size int64 `xml:"Size"` +}, error, +) { + var all []struct { + Key string `xml:"Key"` + ETag string `xml:"ETag"` + Size int64 `xml:"Size"` + } + // Paginated, because a comparison that read the first page only would + // report two different stores as equal — the exact shape of a verdict + // measuring its own window rather than its subject. + token := "" + for page := 0; page < 1000; page++ { + url := base + bucket + "?list-type=2" + if token != "" { + url += "&continuation-token=" + token + } + var listing objectListing + if err := getXML(ctx, client, url, &listing); err != nil { + return nil, err + } + all = append(all, listing.Contents...) + if !listing.IsTruncated || listing.NextToken == "" { + return all, nil + } + token = listing.NextToken + } + return nil, fmt.Errorf("%s did not stop paginating", bucket) +} + +func getXML(ctx context.Context, client *http.Client, url string, into any) error { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return err + } + res, err := client.Do(req) + if err != nil { + return err + } + defer func() { _ = res.Body.Close() }() + if res.StatusCode != http.StatusOK { + return fmt.Errorf("answered %d", res.StatusCode) + } + return xml.NewDecoder(io.LimitReader(res.Body, maxStateBody)).Decode(into) +} + +// diff names every difference, in both directions. +// +// Every one of them, and not the first: a reset reports exactly what it could +// not reconcile (#853), and a verdict that stopped at the first difference +// would send an operator round the loop once per problem. The wording says +// which side a thing is on, because "extra" and "missing" need opposite fixes. +func (o observed) diff(other observed) []string { + var out []string + + resourceKey := func(r observedResource) string { + return r.Kind + " " + r.ID + " state=" + r.State + " backing=[" + r.Backing + "]" + } + objectKey := func(ob observedObject) string { + return ob.Door + "/" + ob.Bucket + "/" + ob.Key + " digest=" + ob.Digest + } + + out = append(out, compareSets("resource", mapAll(o.Resources, resourceKey), mapAll(other.Resources, resourceKey))...) + out = append(out, compareSets("object", mapAll(o.Objects, objectKey), mapAll(other.Objects, objectKey))...) + out = append(out, compareSets("machine", o.Machines, other.Machines)...) + out = append(out, compareSets("network", o.Networks, other.Networks)...) + out = append(out, compareSets("firewall", o.Firewalls, other.Firewalls)...) + return out +} + +func mapAll[T any](items []T, key func(T) string) []string { + out := make([]string, 0, len(items)) + for _, item := range items { + out = append(out, key(item)) + } + return out +} + +// compareSets answers what is extra on the right and what is missing from it. +func compareSets(kind string, want, got []string) []string { + var out []string + for _, item := range got { + if !slices.Contains(want, item) { + out = append(out, kind+" present and not in the checkpoint: "+item) + } + } + for _, item := range want { + if !slices.Contains(got, item) { + out = append(out, kind+" in the checkpoint and not present: "+item) + } + } + return out +} diff --git a/internal/cli/observable_test.go b/internal/cli/observable_test.go new file mode 100644 index 00000000..4c8d4408 --- /dev/null +++ b/internal/cli/observable_test.go @@ -0,0 +1,192 @@ +package cli + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stephrobert/feint/internal/core/machine" + "github.com/stephrobert/feint/internal/s3" +) + +// s3ReportVersionForTest keeps the fake honest: the reader refuses any other +// version, so the fake follows the constant rather than a copy of its value. +var s3ReportVersionForTest = fmt.Sprint(s3.ReportVersion) + +// The observable state a reset is judged against (#853). +// +// These tests exist before the commands do, and that order is the point: the +// issue refuses a reset that reports success because no error occurred — +// "reset is not successful because JSON was restored" — so the verdict has to +// be a comparison, and a comparison is only worth the normalisation under it. + +// fakeLab serves the three endpoints observeLab reads, with the bytes of one +// door, so that the reader can be driven without starting an emulator (`start` +// re-execs os.Executable(), which under `go test` is the test binary). +func fakeLab(t *testing.T, objects map[string]map[string]string) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("/_feint/resources", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"count":2,"resources":[ + {"id":"b","kind":"instance/server","state":"running", + "runtime":{"container":"feint-scw-b"},"created":"2026-11-11T09:00:00Z"}, + {"id":"a","kind":"instance/ip","state":"available","created":"2026-11-11T09:00:01Z"}]}`)) + }) + mux.HandleFunc("/_feint/s3", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + // The whole report, because fetchS3 validates it: a version, a + // non-nil request list and limits that agree with the activation. + // A fake that answered less would fail on the reader's own guard + // rather than on what this test is about. + _, _ = w.Write([]byte(`{"schema_version":` + s3ReportVersionForTest + `, + "enabled":true,"max_blob_bytes":1073741824,"max_object_bytes":268435456, + "http_requests":{"GET":1,"PUT":1}, + "doors":[{"provider":"scaleway","region":"fr-par", + "path":"/_feint/s3/scaleway/fr-par/"}]}`)) + }) + mux.HandleFunc("/_feint/s3/scaleway/fr-par/", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/xml") + rest := strings.TrimPrefix(r.URL.Path, "/_feint/s3/scaleway/fr-par/") + if rest == "" { + var body strings.Builder + body.WriteString("") + for bucket := range objects { + body.WriteString("" + bucket + "") + } + body.WriteString("") + _, _ = w.Write([]byte(body.String())) + return + } + var body strings.Builder + body.WriteString("false") + for key, digest := range objects[strings.TrimSuffix(rest, "/")] { + body.WriteString("" + key + "" + digest + + "7") + } + body.WriteString("") + _, _ = w.Write([]byte(body.String())) + }) + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + return server +} + +// TestTwoReadsOfOneLabAreEqual is cited by the observable state's doc. +// +// Without this there is no verdict at all: a comparison whose two sides +// disagree about ordering reports a difference on every reset and an operator +// learns to ignore it. The fake answers its resources out of order and its +// objects from a map, which has no order at all, precisely so the sort is the +// thing under test. +func TestTwoReadsOfOneLabAreEqual(t *testing.T) { + lab := fakeLab(t, map[string]map[string]string{ + "images": {"disk.qcow2": "d41d8", "boot.img": "a1b2c"}, + "backup": {"db.sql": "99aa0"}, + }) + rt := machine.Use(surveyingDriver{left: machine.Leftovers{ + Machines: []string{"feint-scw-b", "feint-scw-a"}, + Networks: []string{"scw-net-2", "scw-net-1"}, + }}) + + first, err := observeLab(t.Context(), lab.URL, rt) + if err != nil { + t.Fatalf("first read: %v", err) + } + second, err := observeLab(t.Context(), lab.URL, rt) + if err != nil { + t.Fatalf("second read: %v", err) + } + if diff := first.diff(second); len(diff) != 0 { + t.Fatalf("two reads of one lab differ:\n%s", strings.Join(diff, "\n")) + } + + // And it actually observed something: a comparison between two empty + // states is equal too, and would pass this test while measuring nothing. + if len(first.Resources) != 2 { + t.Errorf("resources %+v", first.Resources) + } + if len(first.Objects) != 3 { + t.Errorf("objects %+v", first.Objects) + } + if len(first.Machines) != 2 || len(first.Networks) != 2 { + t.Errorf("host half %v %v", first.Machines, first.Networks) + } + // Sorted, not merely stable: the identity of a resource is its kind and + // id, and a reader of the JSON must not have to sort it again. + if first.Resources[0].ID != "a" || first.Objects[0].Bucket != "backup" { + t.Errorf("not sorted: %+v %+v", first.Resources, first.Objects) + } + // The backing is carried, because a resource whose container changed + // under it is a resource a reset did not reconcile even when the lists + // are the same length. + if first.Resources[1].Backing != "container=feint-scw-b" { + t.Errorf("backing %q", first.Resources[1].Backing) + } +} + +// TestADifferenceIsNamedWhereverItIs is cited by the observable state's doc. +// +// #853 requires a reset to report "exactly what could not be reconciled", so +// one difference per axis is asserted here, in both directions. A verdict that +// found the extra container but not the changed digest would send an operator +// round the loop once per problem, and one that stopped at the first would do +// it every time. +func TestADifferenceIsNamedWhereverItIs(t *testing.T) { + checkpoint := observed{ + Resources: []observedResource{{Kind: "instance/server", ID: "a", State: "running"}}, + Objects: []observedObject{{Door: "d", Bucket: "b", Key: "k", Digest: "before"}}, + Machines: []string{"feint-scw-a"}, + Networks: []string{"scw-net-1"}, + Firewalls: []string{"scw-acl-1"}, + } + now := observed{ + // The state moved, which is a difference even though the resource is + // the same one: a reset that left a stopped server stopped has not + // converged. + Resources: []observedResource{ + {Kind: "instance/server", ID: "a", State: "stopped"}, + {Kind: "instance/ip", ID: "extra", State: "available"}, + }, + // Same door, same bucket, same key, different content. A comparison + // on sizes would call these equal. + Objects: []observedObject{{Door: "d", Bucket: "b", Key: "k", Digest: "after"}}, + Machines: []string{"feint-scw-a", "feint-scw-extra"}, + Networks: nil, + Firewalls: []string{"scw-acl-1"}, + } + + diff := checkpoint.diff(now) + joined := strings.Join(diff, "\n") + for _, want := range []string{ + "state=stopped", // the moved state + "instance/ip extra", // the resource nobody checkpointed + "digest=after", // the content that changed + "digest=before", // and the content that is gone + "feint-scw-extra", // the machine in excess + "scw-net-1", // the network a reset failed to bring back + } { + if !strings.Contains(joined, want) { + t.Errorf("the verdict never mentions %q:\n%s", want, joined) + } + } + // Both directions are worded apart, because "extra" and "missing" need + // opposite fixes and a single word for both sends the reader the wrong way. + if !strings.Contains(joined, "present and not in the checkpoint") || + !strings.Contains(joined, "in the checkpoint and not present") { + t.Errorf("the verdict does not say which side a thing is on:\n%s", joined) + } + // And an unchanged axis says nothing at all: a verdict that listed the + // firewall that did converge would bury the five that did not. + if strings.Contains(joined, "scw-acl-1") { + t.Errorf("an axis that converged was reported anyway:\n%s", joined) + } + + // Equal states produce no verdict, which is the half that makes a second + // reset a no-op (#853). + if diff := checkpoint.diff(checkpoint); len(diff) != 0 { + t.Errorf("a state differs from itself:\n%s", strings.Join(diff, "\n")) + } +} diff --git a/internal/cli/reconcile_discipline_test.go b/internal/cli/reconcile_discipline_test.go new file mode 100644 index 00000000..b60e67cf --- /dev/null +++ b/internal/cli/reconcile_discipline_test.go @@ -0,0 +1,60 @@ +package cli + +import ( + "testing" + + "github.com/stephrobert/feint/internal/core/emulator" +) + +// TestAPackThatDrivesMachinesReconcilesBothWays is cited by +// emulator.MissingReconciler, and it exists because of a defect that happened +// while that interface was being written. +// +// The first version carried both halves on ONE interface. Adding the second +// method turned every pack into a non-implementer: the type assertion in the +// reset handler failed, the reconciliation silently stopped happening, and the +// call still answered 200 with a clean-looking body. Nothing logged it. The +// only reason it surfaced is that a test asserted which records a pack had +// been HANDED rather than that the call succeeded. +// +// Splitting the interface in two removes that class of defect for the future — +// a pack gains one half when it is ready and keeps the other working — and +// this test closes the other side of it: a pack that drives machines must +// satisfy both, or a reset would quietly do half its job for that provider. +// +// The subject is "drives machines", and it is read from the pack itself rather +// than from a list of provider names: a fourth pack must inherit this without +// anyone editing a list here, which is rule 5 applied to a test. +func TestAPackThatDrivesMachinesReconcilesBothWays(t *testing.T) { + srv, _, err := newServer(nil) + if err != nil { + t.Fatalf("build the emulator: %v", err) + } + + examined := 0 + for _, pack := range srv.Packs() { + gone, reconciles := pack.(emulator.Reconciler) + missing, starts := pack.(emulator.MissingReconciler) + if !reconciles && !starts { + // A pack that drives no machine implements neither, which is the + // ordinary case and not a failure. + continue + } + examined++ + if !reconciles { + t.Errorf("%s can start machines again and cannot take them down: a reset would "+ + "leave every machine the checkpoint does not hold running", pack.Name()) + } + if !starts { + t.Errorf("%s can take machines down and cannot start them again: a reset would "+ + "return an operator to a checkpoint whose servers answer nothing", pack.Name()) + } + _, _ = gone, missing + } + + // A denominator, because a loop over nothing passes silently and would + // have passed before either half existed. + if examined < 3 { + t.Fatalf("only %d pack(s) reconcile at all; this build drives machines for three", examined) + } +} diff --git a/internal/cli/reset_after_test.go b/internal/cli/reset_after_test.go new file mode 100644 index 00000000..309c0ba4 --- /dev/null +++ b/internal/cli/reset_after_test.go @@ -0,0 +1,250 @@ +package cli + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// `feint run --reset-after` (#853's crash behaviour), in the parts a unit test +// can reach. +// +// What it asks for is a reset attempted on command success, command failure +// and a relayed signal, and it concedes the fourth case in the same breath: a +// hard kill cannot promise anything. So these tests hold the decisions, and +// the three ways out are driven against the built binary where the signal is +// real. + +// TestResetAfterAndKeepOnFailureAreRefusedTogether is cited by runCommand. +// +// Two flags that mean opposite things about a failed run: one erases the +// state, the other keeps it for inspection. Refused rather than silently +// ordered, because either order is wrong for somebody and a reader cannot tell +// which they got. +func TestResetAfterAndKeepOnFailureAreRefusedTogether(t *testing.T) { + server := reportServer(t, door("scaleway", "fr-par")) + var stdout, stderr bytes.Buffer + code := runCommand([]string{ + "--endpoint", server.URL, "--reset-after", "--keep-on-failure", "--", "true", + }, &stdout, &stderr) + if code != exitError { + t.Fatalf("both flags together were accepted (%d)", code) + } + for _, want := range []string{"--reset-after", "--keep-on-failure", "pick one"} { + if !strings.Contains(stderr.String(), want) { + t.Errorf("the refusal does not mention %q: %s", want, stderr.String()) + } + } + + // And each alone is not refused, so this cannot be satisfied by refusing + // the pair's members. + for _, flag := range []string{"--reset-after", "--keep-on-failure"} { + stdout.Reset() + stderr.Reset() + if code := runCommand([]string{ + "--endpoint", server.URL, flag, "--", "true", + }, &stdout, &stderr); code == exitError && strings.Contains(stderr.String(), "pick one") { + t.Errorf("%s alone was refused as a pair: %s", flag, stderr.String()) + } + } +} + +// TestAFailedResetAfterIsReportedAndNotRaised is cited by resetToSession. +// +// The child has already run when a reset is attempted, and its exit code is +// what a caller builds on. A reset that could not be attempted must be +// reported rather than swallowed, and rather than raised into the child's +// verdict where it would read as a failing test. The four words are not +// interchangeable: they send a reader to four different places. +func TestAFailedResetAfterIsReportedAndNotRaised(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + const addr = "127.0.0.1:4599" + + t.Run("no session checkpoint recorded", func(t *testing.T) { + var stderr bytes.Buffer + verdict, left := resetToSession("http://"+addr, "feint-lab-1", addr, &stderr) + if verdict != "not attempted" { + t.Errorf("verdict %q", verdict) + } + if len(left) != 0 { + t.Errorf("leftovers %v for a reset that never ran", left) + } + if stderr.Len() == 0 { + t.Error("nothing was reported") + } + }) + + t.Run("the emulator refuses the reset", func(t *testing.T) { + refusing := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"error":"the control plane could not be restored"}`)) + })) + defer refusing.Close() + if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion, Name: "feint-lab-1", + }); err != nil { + t.Fatal(err) + } + var stderr bytes.Buffer + verdict, _ := resetToSession(refusing.URL, "feint-lab-1", addr, &stderr) + if verdict != "failed" { + t.Errorf("verdict %q", verdict) + } + // The emulator's own words, not a summary of them: a reset refused + // because the store could not be restored and one refused because the + // bytes could not be are different problems. + if !strings.Contains(stderr.String(), "control plane could not be restored") { + t.Errorf("the emulator's reason was dropped: %s", stderr.String()) + } + }) + + t.Run("a session that is not this run's is not reset", func(t *testing.T) { + // Two runs against one shared emulator must not reset each other's + // state. The recorded name decides, and a mismatch is "not attempted" + // rather than a reset of somebody else's checkpoint. + if err := writeRecordedCheckpoint(addr, recordedCheckpoint{ + SchemaVersion: checkpointStateVersion, Name: "feint-lab-OTHER", + }); err != nil { + t.Fatal(err) + } + var stderr bytes.Buffer + verdict, _ := resetToSession("http://"+addr, "feint-lab-1", addr, &stderr) + if verdict != "not attempted" { + t.Errorf("verdict %q for another run's session", verdict) + } + }) +} + +// TestASessionCheckpointIsNamedAfterItsRun is cited by takeSessionCheckpoint. +// +// The name is the lab's own session identity and not one the operator typed, +// so that two runs against one shared emulator cannot reset each other's +// state. A fixed name would let them, and the failure would look like a +// flaky test rather than a collision. +func TestASessionCheckpointIsNamedAfterItsRun(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + const addr = "127.0.0.1:4599" + + var asked []string + lab := checkpointingLab(t, &asked) + if err := takeSessionCheckpoint(lab.URL, "feint-lab-4021", addr); err != nil { + t.Fatalf("take the session checkpoint: %v", err) + } + if len(asked) != 1 || asked[0] != "/_feint/checkpoint/feint-lab-4021" { + t.Fatalf("the emulator was asked for %v", asked) + } + recorded, err := readRecordedCheckpoint(addr) + if err != nil { + t.Fatalf("read the record back: %v", err) + } + if recorded.Name != "feint-lab-4021" { + t.Errorf("the record names %q", recorded.Name) + } + if recorded.Taken == "" { + t.Error("the record carries no time, so two runs cannot be told apart in a log") + } +} + +// checkpointingLab answers the endpoints takeSessionCheckpoint drives, and +// records which paths were asked for. +func checkpointingLab(t *testing.T, asked *[]string) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("/_feint/checkpoint/", func(w http.ResponseWriter, r *http.Request) { + *asked = append(*asked, r.URL.Path) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"schema_version":1,"name":"x","resources":0,"bytes":false}`)) + }) + mux.HandleFunc("/_feint/health", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"machines":"none","capabilities":null,"enforced":{}}`)) + }) + mux.HandleFunc("/_feint/resources", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"count":0,"resources":[]}`)) + }) + mux.HandleFunc("/_feint/s3", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"schema_version":2,"enabled":false,"doors":[],"http_requests":{}}`)) + }) + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + return server +} + +// TestARunWhoseResetDidNotConvergeFails is cited by runCommand's exit code. +// +// The expensive case, and the one the falsification caught my tests not +// covering: a shared emulator left dirty by a run that reported success. The +// caller asked for the lab to be left as found, and a quiet 0 is how the next +// test inherits the mess and blames itself. +// +// The child passes on purpose. A reset's failure must not be hidden behind a +// failing test, and it must not be hidden behind a passing one either. +func TestARunWhoseResetDidNotConvergeFails(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + + // A lab that answers one more resource after the reset than it did at the + // checkpoint: the reset ran, the emulator said ok, and the state did not + // come back. That is exactly the shape #853 refuses to call a success. + var resourceReads int + mux := http.NewServeMux() + mux.HandleFunc("/_feint/resources", func(w http.ResponseWriter, _ *http.Request) { + resourceReads++ + if resourceReads == 1 { + _, _ = w.Write([]byte(`{"count":0,"resources":[]}`)) + return + } + _, _ = w.Write([]byte(`{"count":1,"resources":[` + + `{"id":"leftover","kind":"instance/server","state":"running"}]}`)) + }) + mux.HandleFunc("/_feint/checkpoint/", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"schema_version":1,"name":"x","resources":0,"bytes":false}`)) + }) + mux.HandleFunc("/_feint/reset/", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"schema_version":1,"name":"x","resources":0,` + + `"destroyed":[],"skipped":[],"recreated":[],"recreates":false}`)) + }) + mux.HandleFunc("/_feint/health", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"machines":"none","capabilities":null,"enforced":{}}`)) + }) + mux.HandleFunc("/_feint/trace", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"schema_version":1,"count":0,"kept":256,"exchanges":[]}`)) + }) + mux.HandleFunc("/_feint/s3", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"schema_version":2,"enabled":true,` + + `"max_blob_bytes":1073741824,"max_object_bytes":268435456,` + + `"http_requests":{"GET":1},"doors":[{"provider":"scaleway","region":"fr-par",` + + `"path":"/_feint/s3/scaleway/fr-par/"}]}`)) + }) + mux.HandleFunc("/_feint/s3/scaleway/fr-par/", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("")) + }) + lab := httptest.NewServer(mux) + defer lab.Close() + + var stdout, stderr bytes.Buffer + code := runCommand([]string{ + "--endpoint", lab.URL, "--reset-after", "--json", "-", "--", "true", + }, &stdout, &stderr) + if code == exitOK { + t.Fatalf("a run whose reset left a resource behind reported success\nstdout:%s\nstderr:%s", + stdout.String(), stderr.String()) + } + + // And the receipt says which it was, because "the test failed" and "the + // lab was left dirty" send a reader to two different places. + var receipt runReceipt + if err := json.Unmarshal(stdout.Bytes(), &receipt); err != nil { + t.Fatalf("stdout is not the receipt alone: %v\n%s", err, stdout.String()) + } + if receipt.Reset != "did not converge" { + t.Errorf("the receipt reports reset=%q", receipt.Reset) + } + if receipt.Exit != exitOK { + t.Errorf("the receipt blames the child, which passed: exit=%d", receipt.Exit) + } + if len(receipt.Unreconciled) == 0 { + t.Error("the receipt names nothing it could not reconcile, which is what #853 asks for") + } +} diff --git a/internal/cli/run.go b/internal/cli/run.go index 008beb30..8e41a8ed 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -81,6 +81,7 @@ func runCommand(args []string, stdout io.Writer, stderr io.Writer) int { provider := fs.String("provider", "", "resolve only this provider's client environment into the child; empty resolves every pack's") addr := fs.String("addr", "127.0.0.1:0", "the proxy's listen address; the default takes a free loopback port and prints it") runtimeMode := fs.String("runtime", "off", "the machine runtime the lab starts with: off, incus, incus-vm, incus-ovn, auto. Anything but off starts containers on this host, so it is asked for") + resetAfter := fs.Bool("reset-after", false, "take a checkpoint once the lab is ready and return to it when the command ends, however it ends (#853). Refused with --keep-on-failure, which preserves the state this would erase") keep := fs.Bool("keep-on-failure", false, "when the child fails, leave the lab running and print how to reach and remove it") receiptPath := fs.String("json", "", "write the receipt as JSON to this path; `-` means stdout, which is only safe when the command itself is quiet") unsafe := fs.Bool("unsafe-real-credentials", false, "allow the child to carry credentials that are not known fakes. The emulator never checks a signature, so this buys nothing and risks sending real keys to a third party") @@ -102,6 +103,16 @@ func runCommand(args []string, stdout io.Writer, stderr io.Writer) int { return exitError } + // Two flags that mean opposite things about a failed run: one erases the + // state, the other preserves it for inspection. Refused together rather + // than silently ordered, because either order is wrong for somebody and a + // reader cannot tell which they got. + // TestResetAfterAndKeepOnFailureAreRefusedTogether fails without this. + if *resetAfter && *keep { + fmt.Fprintln(stderr, "feint run: --reset-after erases the state --keep-on-failure keeps for you; pick one") + return exitError + } + // The runtime, asked of the host before anything starts, which is `up`'s // rule and for its reason: a refusal must fire with nothing running. The // same seam answers, so the refusal says what `up`'s says. @@ -181,6 +192,18 @@ func runCommand(args []string, stdout io.Writer, stderr io.Writer) int { stripped := hazardVariables() childEnv := childEnvironment{strip: stripped, set: exports} + // The session's initial state, named after the run's own directory so two + // runs against one shared emulator cannot reset each other's. + session := labSession(owned) + if session == "" { + session = "feint-run-session" + } + if *resetAfter { + if err := takeSessionCheckpoint(target, session, *addr); err != nil { + return fail(err) + } + } + // Counted before and after, so an ATTACHED lab's earlier traffic is not // reported as this run's. The owned case starts at zero and the delta is // the same number; the attached case is where a total would lie. @@ -196,6 +219,15 @@ func runCommand(args []string, stdout io.Writer, stderr io.Writer) int { // restart. Report what was last seen and say it is a window. calls, truncated = after, true } + // The reset, attempted on every way out of the child: success, failure, + // and a SIGTERM this command relayed and waited for. #853 asks for the + // three and concedes the fourth — a hard kill of this process cannot + // promise anything, and `feint doctor` is what finds that afterwards. + resetVerdict, unreconciled := "", []string(nil) + if *resetAfter { + resetVerdict, unreconciled = resetToSession(target, session, *addr, stderr) + } + runtimeName, capabilities, proof := labHealth(target) cleanup := "not owned" @@ -230,6 +262,7 @@ func runCommand(args []string, stdout io.Writer, stderr io.Writer) int { Providers: providers, Intercepted: intercepted, Command: command, Calls: calls, CallsTruncated: truncated, Stripped: stripped, Exit: code, Cleanup: cleanup, + Reset: resetVerdict, Unreconciled: unreconciled, RealCloud: "no known escape observed", } // stdout belongs to the CHILD, and to nothing else. Measured rather than @@ -245,6 +278,13 @@ func runCommand(args []string, stdout io.Writer, stderr io.Writer) int { return exitError } } + // A reset that did not converge is this command's failure even when the + // child passed: the caller asked for the lab to be left as found, and it + // was not. Reported separately from the child's code so a reader can tell + // a failing test from a leaking lab. + if resetVerdict == "did not converge" && code == exitOK { + return exitError + } // The child's code, not this command's: a wrapper that swallowed a // failing test would be worse than no wrapper. return code diff --git a/internal/cli/run_lifecycle.go b/internal/cli/run_lifecycle.go index 660a1557..7f98e21c 100644 --- a/internal/cli/run_lifecycle.go +++ b/internal/cli/run_lifecycle.go @@ -300,8 +300,20 @@ type runReceipt struct { // window. The emulator publishes the window for exactly this reason. CallsTruncated bool `json:"calls_truncated"` Stripped []string `json:"stripped"` - Exit int `json:"exit"` - Cleanup string `json:"cleanup"` + // Exit is the child's code, and NEGATIVE means a signal killed it, which + // is what Go answers and what the prose above says in words. A reader of + // the JSON gets the number; a reader of the receipt gets the sentence. + // Reset is empty when --reset-after was not asked for, and otherwise + // says what happened: a run that reset and a run that did not are + // different facts, and an empty Unreconciled must not be read as + // "it converged". + Reset string `json:"reset,omitempty"` + // Unreconciled names what the reset could not bring back, which is + // #853's own wording: a reset reports exactly what it could not + // reconcile. + Unreconciled []string `json:"unreconciled,omitempty"` + Exit int `json:"exit"` + Cleanup string `json:"cleanup"` // RealCloud is a field and not a sentence render() prints, because the // prose and the JSON must not be able to disagree about the one claim a // reader of this receipt cares about most. @@ -342,7 +354,13 @@ func callsSoFar(endpoint string) (int, bool) { // render writes the compact receipt #848 describes. func (r runReceipt) render(w io.Writer) { verdict := "passed" - if r.Exit != 0 { + switch { + case r.Exit < 0: + // Go answers -1 for a process a signal killed, and -1 is a number + // nobody can act on. The word is what a reader needs; the field keeps + // the number for a machine, and the JSON documents it. + verdict = "failed (the command was killed by a signal)" + case r.Exit != 0: verdict = fmt.Sprintf("failed (%d)", r.Exit) } fmt.Fprintf(w, "feint run: %s\n", verdict) @@ -369,6 +387,12 @@ func (r runReceipt) render(w io.Writer) { if len(r.RuntimeProof) != 0 { row("runtime proof", strings.Join(r.RuntimeProof, ", ")) } + if r.Reset != "" { + row("reset", r.Reset) + for _, difference := range r.Unreconciled { + fmt.Fprintf(w, " %s\n", difference) + } + } row("cleanup", r.Cleanup) // Deliberately narrow wording, and a field rather than a literal here: // "no known escape observed" is what #848's own example prints and what diff --git a/internal/cli/testdata/frozen/cli.json b/internal/cli/testdata/frozen/cli.json index 43ea4b68..904f4214 100644 --- a/internal/cli/testdata/frozen/cli.json +++ b/internal/cli/testdata/frozen/cli.json @@ -5637,6 +5637,501 @@ ] } } + }, + { + "schema_version": 31, + "content": { + "exit_codes": { + "drift": 2, + "error": 1, + "ok": 0 + }, + "verbs": { + "catalog": [ + "--format" + ], + "checkpoint": [ + "--addr" + ], + "clean": [ + "--check", + "--closing", + "--doorstep", + "--force", + "--format", + "--vm" + ], + "corpus": [ + "--accepted", + "--against-cloud", + "--bind", + "--check", + "--credential", + "--dir", + "--dry-run", + "--endpoint", + "--file", + "--format", + "--mark-stale", + "--timeout" + ], + "coverage": [ + "--artefact", + "--axis", + "--baseline", + "--contract", + "--contract-only", + "--document", + "--evidence", + "--fail-on-unknown", + "--format", + "--gaps", + "--observed", + "--products", + "--provider", + "--sdk", + "--write-baseline" + ], + "docs": [ + "--check", + "--client-pins", + "--confidence", + "--contracts", + "--coverage", + "--file", + "--install", + "--limits", + "--proved", + "--routes", + "--screenshots", + "--ui-manifest", + "--workflow" + ], + "doctor": [ + "--addr", + "--s3", + "--vm" + ], + "down": [ + "--file", + "--keep-emulator" + ], + "env": [ + "--client", + "--endpoint", + "--s3", + "--s3-region", + "--shell", + "--unset" + ], + "evidence": [ + "--allow-narrowing", + "--contracts", + "--endpoint", + "--join", + "--out", + "--reshape", + "--shapes", + "--suites" + ], + "evidence baseline": [ + "--axes", + "--evidence", + "--out" + ], + "evidence verify": [ + "--accepted", + "--baseline", + "--evidence" + ], + "images": [ + "--check", + "--only", + "--vm" + ], + "logs": [ + "--addr", + "-f", + "-n" + ], + "probe": [ + "--contracts", + "--endpoint", + "--provider" + ], + "proxy": [ + "--addr", + "--expose-to-network", + "--forward", + "--intercept", + "--max-body", + "--provider", + "--queue", + "--record", + "--upstream" + ], + "replay": [ + "--endpoint", + "--format", + "--refusals-only", + "--timeout" + ], + "reset": [ + "--addr" + ], + "restart": [ + "--addr", + "--timeout" + ], + "run": [ + "--addr", + "--endpoint", + "--json", + "--keep-on-failure", + "--provider", + "--runtime", + "--unsafe-real-credentials" + ], + "serve": [ + "--addr", + "--cleanup", + "--consistency", + "--contracts", + "--coverage", + "--expose-to-network", + "--log-level", + "--objectstore", + "--objectstore-budget-mib", + "--projects", + "--resolver", + "--s3-doors", + "--s3-host-doors", + "--s3-object-limit-mib", + "--shapes", + "--state", + "--strict-catalog", + "--vm" + ], + "shapes": [ + "--check", + "--dir", + "--dry-run", + "--profile", + "--provider", + "--record" + ], + "snapshot list": [ + "--format" + ], + "snapshot load": [ + "--addr" + ], + "snapshot save": [ + "--addr", + "--force" + ], + "start": [ + "--addr", + "--cleanup", + "--contracts", + "--detach", + "--foreground", + "--log-level", + "--objectstore", + "--objectstore-budget-mib", + "--projects", + "--resolver", + "--s3-doors", + "--s3-host-doors", + "--s3-object-limit-mib", + "--state", + "--strict-catalog", + "--timeout", + "--vm" + ], + "status": [ + "--addr", + "--coverage", + "--format" + ], + "stop": [ + "--addr", + "--timeout" + ], + "transcript": [ + "--against", + "--contract", + "--format", + "--sanitise", + "--shape" + ], + "ui": [ + "--addr", + "--print" + ], + "up": [ + "--file", + "--no-iac", + "--runtime", + "--timeout" + ], + "version": [ + "--check" + ], + "wait": [ + "--addr", + "--timeout" + ] + } + } + }, + { + "schema_version": 32, + "content": { + "exit_codes": { + "drift": 2, + "error": 1, + "ok": 0 + }, + "verbs": { + "catalog": [ + "--format" + ], + "checkpoint": [ + "--addr" + ], + "clean": [ + "--check", + "--closing", + "--doorstep", + "--force", + "--format", + "--vm" + ], + "corpus": [ + "--accepted", + "--against-cloud", + "--bind", + "--check", + "--credential", + "--dir", + "--dry-run", + "--endpoint", + "--file", + "--format", + "--mark-stale", + "--timeout" + ], + "coverage": [ + "--artefact", + "--axis", + "--baseline", + "--contract", + "--contract-only", + "--document", + "--evidence", + "--fail-on-unknown", + "--format", + "--gaps", + "--observed", + "--products", + "--provider", + "--sdk", + "--write-baseline" + ], + "docs": [ + "--check", + "--client-pins", + "--confidence", + "--contracts", + "--coverage", + "--file", + "--install", + "--limits", + "--proved", + "--routes", + "--screenshots", + "--ui-manifest", + "--workflow" + ], + "doctor": [ + "--addr", + "--s3", + "--vm" + ], + "down": [ + "--file", + "--keep-emulator" + ], + "env": [ + "--client", + "--endpoint", + "--s3", + "--s3-region", + "--shell", + "--unset" + ], + "evidence": [ + "--allow-narrowing", + "--contracts", + "--endpoint", + "--join", + "--out", + "--reshape", + "--shapes", + "--suites" + ], + "evidence baseline": [ + "--axes", + "--evidence", + "--out" + ], + "evidence verify": [ + "--accepted", + "--baseline", + "--evidence" + ], + "images": [ + "--check", + "--only", + "--vm" + ], + "logs": [ + "--addr", + "-f", + "-n" + ], + "probe": [ + "--contracts", + "--endpoint", + "--provider" + ], + "proxy": [ + "--addr", + "--expose-to-network", + "--forward", + "--intercept", + "--max-body", + "--provider", + "--queue", + "--record", + "--upstream" + ], + "replay": [ + "--endpoint", + "--format", + "--refusals-only", + "--timeout" + ], + "reset": [ + "--addr" + ], + "restart": [ + "--addr", + "--timeout" + ], + "run": [ + "--addr", + "--endpoint", + "--json", + "--keep-on-failure", + "--provider", + "--reset-after", + "--runtime", + "--unsafe-real-credentials" + ], + "serve": [ + "--addr", + "--cleanup", + "--consistency", + "--contracts", + "--coverage", + "--expose-to-network", + "--log-level", + "--objectstore", + "--objectstore-budget-mib", + "--projects", + "--resolver", + "--s3-doors", + "--s3-host-doors", + "--s3-object-limit-mib", + "--shapes", + "--state", + "--strict-catalog", + "--vm" + ], + "shapes": [ + "--check", + "--dir", + "--dry-run", + "--profile", + "--provider", + "--record" + ], + "snapshot list": [ + "--format" + ], + "snapshot load": [ + "--addr" + ], + "snapshot save": [ + "--addr", + "--force" + ], + "start": [ + "--addr", + "--cleanup", + "--contracts", + "--detach", + "--foreground", + "--log-level", + "--objectstore", + "--objectstore-budget-mib", + "--projects", + "--resolver", + "--s3-doors", + "--s3-host-doors", + "--s3-object-limit-mib", + "--state", + "--strict-catalog", + "--timeout", + "--vm" + ], + "status": [ + "--addr", + "--coverage", + "--format" + ], + "stop": [ + "--addr", + "--timeout" + ], + "transcript": [ + "--against", + "--contract", + "--format", + "--sanitise", + "--shape" + ], + "ui": [ + "--addr", + "--print" + ], + "up": [ + "--file", + "--no-iac", + "--runtime", + "--timeout" + ], + "version": [ + "--check" + ], + "wait": [ + "--addr", + "--timeout" + ] + } + } } ] } diff --git a/internal/core/emulator/checkpoint.go b/internal/core/emulator/checkpoint.go new file mode 100644 index 00000000..b149039a --- /dev/null +++ b/internal/core/emulator/checkpoint.go @@ -0,0 +1,367 @@ +package emulator + +import ( + "bytes" + "context" + "net/http" + "slices" + "sort" + "sync" + + "github.com/stephrobert/feint/internal/core/machine" + "github.com/stephrobert/feint/internal/core/resource" +) + +// Named checkpoints of a whole lab, and the reset that returns to one (#853). +// +// `feint snapshot` already persists the control plane, and +// objectstore.SaveCheckpoint already persists bytes. Neither had a caller that +// put them together, and putting them together is the point: #853 asks for a +// LIFECYCLE guarantee rather than two storage primitives, and says what +// separates them in one line — "reset is not successful because JSON was +// restored". +// +// ## Why the reset happens here rather than in the CLI +// +// Restoring the store is the easy half. The half that matters is the host: a +// resource that disappears in a restore leaves its container running while the +// API no longer mentions it, and nobody destroys it. Only this process holds +// both the store and the packs' bindings, so only this process can act on the +// difference. +// +// A CLI-side reset would have to name each pack's delete route to take a +// machine down, which is provider knowledge in the one place this repository +// keeps free of it. +// +// ## Why the VERDICT is not here +// +// This answers what it did. Whether the lab converged is a comparison of what +// a client can observe, and that belongs to whoever asked for the reset — +// internal/cli's observable state. An operation that graded itself would be +// the verdict this repository refuses everywhere else. + +// Reconciler is the optional half of a pack that can bring the host back in +// line with its own stored resources. +// +// Optional in the shape the other halves use (EnvHazards, StackHazards, +// FirewallEnforcer): a pack that drives no machine does not implement it, and +// nothing here holds a list of which do. +// +// The input is the pack's OWN resources that no longer exist, so no pack ever +// sees another's, and the core stays a loop over whatever is mounted. +type Reconciler interface { + ReconcileGone(ctx context.Context, gone []*resource.Resource) machine.Reconciled +} + +// MissingReconciler is the other half, and a SEPARATE interface rather than a +// second method on Reconciler. +// +// That is not style. A single interface carrying both means a pack +// implementing one of them satisfies neither, the type assertion fails, and +// the reconciliation silently stops happening — no error, no log, a reset that +// reports a clean pass and reconciles nothing. It happened while this half was +// being written: adding the second method to Reconciler turned every pack into +// a non-implementer, and the only reason it was caught is that a test asserted +// which records a pack was HANDED rather than that the call answered 200. +// +// Two optional halves cannot do that: a pack gains one when it is ready, and +// the one it already had keeps working. +// +// internal/cli's TestAPackThatDrivesMachinesReconcilesBothWays fails without +// both halves being satisfied by every pack that drives machines. +type MissingReconciler interface { + // ReconcileMissing starts again the machines of this pack's records that + // the host no longer holds. + // + // It takes the records and the machines the host DOES hold, and the pack + // decides which of its records are missing. The core deliberately does not + // decide: a record's machine name lives under a key that is the pack's own + // vocabulary, and a first version of this guessed with a "feint-" prefix + // in the core. It was wrong twice over — a provider-shaped assumption in + // the neutral layer, and broad enough to match a test's own bookkeeping + // stamp, which it then tried to boot. + ReconcileMissing(ctx context.Context, records []*resource.Resource, held []string) machine.Reconciled +} + +// checkpoints holds the control-plane half of each named checkpoint. +// +// In memory, and that is the scope #853 describes: "returns to the session's +// initial state". Bytes are durable because objectstore's checkpoints are, and +// a reader who wants the control plane to outlive the process has +// `feint snapshot save`, which writes a file and is a different promise. +type checkpoints struct { + mu sync.Mutex + saved map[string][]byte +} + +func (c *checkpoints) put(name string, state []byte) { + c.mu.Lock() + defer c.mu.Unlock() + if c.saved == nil { + c.saved = map[string][]byte{} + } + c.saved[name] = state +} + +func (c *checkpoints) get(name string) ([]byte, bool) { + c.mu.Lock() + defer c.mu.Unlock() + state, ok := c.saved[name] + return state, ok +} + +func (c *checkpoints) drop(name string) bool { + c.mu.Lock() + defer c.mu.Unlock() + _, ok := c.saved[name] + delete(c.saved, name) + return ok +} + +// handleCheckpointSave records the control plane and the bytes under one name. +// +// Both halves or neither: a checkpoint whose store was taken and whose bytes +// were not is a checkpoint a reset cannot honour, and it would fail later, +// somewhere else, with no mention of this call. So the byte half goes first — +// it is the one that can fail on a disk — and the store half follows only +// when it succeeded. +// +// TestACheckpointTakesBothHalvesOrNeither fails without this. +func (s *Server) handleCheckpointSave(w http.ResponseWriter, r *http.Request) { + name := r.PathValue("name") + if !validCheckpointName(name) { + writeJSON(w, http.StatusBadRequest, map[string]any{ + "error": "a checkpoint name is 1 to 64 characters of letters, digits, dash, dot or underscore", + }) + return + } + if s.env.Objects != nil { + if err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]any{ + "error": "the bytes could not be checkpointed: " + err.Error(), + }) + return + } + } + var state bytes.Buffer + if err := s.env.Store.Snapshot(&state); err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]any{ + "error": "the control plane could not be checkpointed: " + err.Error(), + }) + return + } + s.checkpoints.put(name, state.Bytes()) + writeJSON(w, http.StatusOK, map[string]any{ + "schema_version": CheckpointSchemaVersion, + "name": name, + "resources": s.env.Store.Len(), + "bytes": s.env.Objects != nil, + }) +} + +// handleCheckpointDelete forgets a checkpoint, both halves. +func (s *Server) handleCheckpointDelete(w http.ResponseWriter, r *http.Request) { + name := r.PathValue("name") + held := s.checkpoints.drop(name) + if s.env.Objects != nil { + if err := s.env.Objects.DeleteCheckpoint(r.Context(), name); err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]any{ + "error": "the bytes could not be forgotten: " + err.Error(), + }) + return + } + } + if !held { + writeJSON(w, http.StatusNotFound, map[string]any{ + "error": "no checkpoint named " + name, + }) + return + } + writeJSON(w, http.StatusOK, map[string]any{"status": "ok", "name": name}) +} + +// handleReset returns the lab to a checkpoint and reconciles the host with it. +// +// The order is not interchangeable, and each step's reason is its own: +// +// 1. read what the store holds NOW, because after step 2 it is gone and the +// machines it named would be unreachable; +// 2. restore the control plane, so the target state is in place before +// anything is destroyed — a failure here leaves the host untouched rather +// than half swept; +// 3. restore the bytes, which objectstore does atomically; +// 4. destroy the machines of records that no longer exist, through each +// pack's own binding and therefore through the ownership checks. +// +// What it does NOT do is recreate a machine a checkpoint had and the host no +// longer does. Starting one needs a Boot — an image, a login, the cloud-init a +// pack renders — which is pack knowledge, and #853 bounds it in the same words: +// "where the runtime contract supports it". The answer says so rather than +// leaving a caller to infer it from a quiet success. +// +// TestAResetDestroysWhatTheCheckpointDoesNotHold fails without this. +func (s *Server) handleReset(w http.ResponseWriter, r *http.Request) { + name := r.PathValue("name") + state, ok := s.checkpoints.get(name) + if !ok { + writeJSON(w, http.StatusNotFound, map[string]any{ + "error": "no checkpoint named " + name + "; take one with POST /_feint/checkpoint/" + name, + }) + return + } + + before := s.env.Store.All() + if err := s.env.Store.Restore(bytes.NewReader(state)); err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]any{ + "error": "the control plane could not be restored: " + err.Error(), + }) + return + } + if s.env.Objects != nil { + if err := s.env.Objects.ResetCheckpoint(r.Context(), name); err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]any{ + "error": "the bytes could not be restored: " + err.Error(), + }) + return + } + } + + restored := s.env.Store.All() + gone := goneAfterRestore(before, restored) + // What the host actually holds, asked of the runtime rather than deduced + // from the store: the store is exactly what just changed, it says a server + // is running, and whether anything backs it is the question. A driver that + // cannot be surveyed answers asked=false, and then nothing is offered for + // recreation at all — claiming a machine is absent because nobody could be + // asked would start containers beside ones already running. + held, surveyed := heldOnHost(r.Context(), s.env) + reconciled := map[string]any{} + var destroyed, skipped, recreated, failed []string + for _, pack := range s.packs { + mine := ownedBy(gone, pack.Name()) + records := ownedBy(restored, pack.Name()) + if len(mine) == 0 && (!surveyed || len(records) == 0) { + continue + } + done := machine.Reconciled{} + if reconciler, ok := pack.(Reconciler); ok && len(mine) != 0 { + done = reconciler.ReconcileGone(r.Context(), mine) + } + if starter, ok := pack.(MissingReconciler); ok && surveyed && len(records) != 0 { + back := starter.ReconcileMissing(r.Context(), records, held) + done.Recreated = append(done.Recreated, back.Recreated...) + done.Failed = append(done.Failed, back.Failed...) + done.Skipped = append(done.Skipped, back.Skipped...) + // A recreation rewrites the record's Runtime and State, so the + // store is written back here rather than left holding the machine + // name of a container that is gone. Put rather than Commit: the + // resource came out of this store a moment ago and nothing else + // writes during a reset, which holds the whole store's lock. + for _, res := range records { + s.env.Store.Put(res) + } + } + destroyed = append(destroyed, done.Destroyed...) + skipped = append(skipped, done.Skipped...) + recreated = append(recreated, done.Recreated...) + failed = append(failed, done.Failed...) + reconciled[pack.Name()] = map[string]any{ + "destroyed": done.Destroyed, "skipped": done.Skipped, + "recreated": done.Recreated, "failed": done.Failed, + } + } + sort.Strings(destroyed) + sort.Strings(skipped) + sort.Strings(recreated) + sort.Strings(failed) + + writeJSON(w, http.StatusOK, map[string]any{ + "schema_version": CheckpointSchemaVersion, + "name": name, + "resources": s.env.Store.Len(), + "destroyed": destroyed, + "skipped": skipped, + "recreated": recreated, + // Named apart from skipped: a start that was attempted and refused and + // a record nobody could act on need different answers from whoever + // reads this. + "failed": failed, + "per_provider": reconciled, + "recreates": true, + }) +} + +// goneAfterRestore answers the resources the restore removed. +// +// By identity rather than by count: a restore that replaced one server with +// another of the same kind leaves the count equal and the host wrong, which is +// the comparison a length check would miss. +func goneAfterRestore(before, after []*resource.Resource) []*resource.Resource { + keys := make([]string, 0, len(after)) + for _, res := range after { + keys = append(keys, resourceKey(res)) + } + slices.Sort(keys) + var gone []*resource.Resource + for _, res := range before { + if _, found := slices.BinarySearch(keys, resourceKey(res)); !found { + gone = append(gone, res) + } + } + return gone +} + +func resourceKey(res *resource.Resource) string { + return res.Tenant.Provider + "\x00" + res.Kind + "\x00" + res.ID +} + +// validCheckpointName keeps a name out of a path and out of a shell. +// +// The byte store writes a directory per checkpoint, so a name is a path +// component before it is anything else. Refused at the door rather than +// escaped later: this repository's order of preference is a serialiser first, +// a refusal at intake second, escaping last. +func validCheckpointName(name string) bool { + if name == "" || len(name) > 64 { + return false + } + for _, r := range name { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': + case r == '-', r == '.', r == '_': + default: + return false + } + } + // A name of dots is a path, not a name. + return name != "." && name != ".." +} + +// ownedBy narrows a list of resources to one pack's own, so no pack ever sees +// another's. +func ownedBy(resources []*resource.Resource, provider string) []*resource.Resource { + out := make([]*resource.Resource, 0, len(resources)) + for _, res := range resources { + if res.Tenant.Provider == provider { + out = append(out, res) + } + } + return out +} + +// heldOnHost answers the machines the runtime actually holds, and whether it +// could be asked at all. +// +// Through Survey, which names only what the emulator labelled and issues no +// mutating command. The second return is not a convenience: a driver nobody +// can survey is not an empty host, and treating it as one would have a reset +// start a container beside every one already running. +// +// TestAResetAsksNothingOfAHostNobodyCanSurvey fails without this. +func heldOnHost(ctx context.Context, env *Env) ([]string, bool) { + left, asked, err := env.machines.Survey(ctx) + if !asked || err != nil { + return nil, false + } + return left.Machines, true +} diff --git a/internal/core/emulator/checkpoint_test.go b/internal/core/emulator/checkpoint_test.go new file mode 100644 index 00000000..8200ccba --- /dev/null +++ b/internal/core/emulator/checkpoint_test.go @@ -0,0 +1,271 @@ +package emulator_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "sort" + "strings" + "testing" + "time" + + "github.com/stephrobert/feint/internal/core/emulator" + "github.com/stephrobert/feint/internal/core/machine" + "github.com/stephrobert/feint/internal/core/objectstore" + "github.com/stephrobert/feint/internal/core/resource" + "github.com/stephrobert/feint/internal/core/store" +) + +// Named checkpoints and the reset that returns to one (#853). +// +// What these tests hold is the half the issue says cannot be skipped: a reset +// is not successful because JSON was restored. So the assertion is never "the +// call answered 200", it is which machines the pack was asked to take down. + +// reconcilingPack is a stubPack that can reconcile, and records what it was +// handed. A pack that drives no machine does not implement the interface at +// all, which is the other population below. +type reconcilingPack struct { + stubPack + got [][]string + done machine.Reconciled +} + +func (p *reconcilingPack) ReconcileGone(_ context.Context, gone []*resource.Resource) machine.Reconciled { + var names []string + for _, res := range gone { + names = append(names, res.ID) + } + p.got = append(p.got, names) + return p.done +} + +func labServer(t *testing.T, packs ...emulator.Pack) (*httptest.Server, *store.Store) { + t.Helper() + return labServerWithBytes(t, nil, packs...) +} + +// labServerWithBytes is the same lab with an optional byte store, because a +// checkpoint's two halves cannot both be asserted against a lab that has one +// of them. The falsification found that: skipping the byte half changed +// nothing in a test whose emulator served no objects. +func labServerWithBytes(t *testing.T, objects *objectstore.Store, packs ...emulator.Pack) (*httptest.Server, *store.Store) { + t.Helper() + return labServerWith(t, objects, nil, packs...) +} + +// labServerWithRuntime is the same lab with a runtime that can be surveyed, +// which is the only way to measure what a reset considers MISSING: the store +// is what just changed, so whether anything backs a record is a question only +// the host can answer. +func labServerWithRuntime(t *testing.T, rt machine.Runtime, packs ...emulator.Pack) (*httptest.Server, *store.Store) { + t.Helper() + return labServerWith(t, nil, &rt, packs...) +} + +func labServerWith(t *testing.T, objects *objectstore.Store, rt *machine.Runtime, packs ...emulator.Pack) (*httptest.Server, *store.Store) { + t.Helper() + held := store.New() + env := &emulator.Env{ + Store: held, + Objects: objects, + Now: func() time.Time { return time.Unix(1700000000, 0).UTC() }, + NewID: func() string { return "00000000-0000-4000-8000-000000000001" }, + } + if rt != nil { + env.UseMachines(*rt) + } + srv, err := emulator.NewServer(env, packs...) + if err != nil { + t.Fatalf("build the emulator: %v", err) + } + server := httptest.NewServer(srv.Handler()) + t.Cleanup(server.Close) + return server, held +} + +func checkpointPost(t *testing.T, url string) (int, map[string]any) { + t.Helper() + res, err := http.Post(url, "application/json", nil) //nolint:noctx // a loopback test server + if err != nil { + t.Fatalf("POST %s: %v", url, err) + } + defer func() { _ = res.Body.Close() }() + var body map[string]any + _ = json.NewDecoder(res.Body).Decode(&body) + return res.StatusCode, body +} + +func put(t *testing.T, held *store.Store, provider, kind, id string) { + t.Helper() + res := resource.New(id, kind, resource.Tenant{Provider: provider, Project: "p", Zone: "z"}, + "running", time.Unix(1700000000, 0).UTC()) + res.Runtime = map[string]string{"machine": "feint-" + provider + "-" + id} + held.Put(res) +} + +// TestACheckpointTakesBothHalvesOrNeither is cited by handleCheckpointSave. +// +// A checkpoint whose store was taken and whose bytes were not is a checkpoint +// a reset cannot honour, and it would fail later, somewhere else, with no +// mention of this call. With no byte store there is only one half to take, and +// the answer says which — a caller that reset expecting bytes back would +// otherwise find none and blame the reset. +func TestACheckpointTakesBothHalvesOrNeither(t *testing.T) { + server, held := labServer(t, stubPack{name: "acme"}) + put(t, held, "acme", "instance/server", "a") + + code, body := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline") + if code != http.StatusOK { + t.Fatalf("checkpoint: %d %v", code, body) + } + if body["name"] != "baseline" { + t.Errorf("name %v", body["name"]) + } + if count, _ := body["resources"].(float64); int(count) != 1 { + t.Errorf("resources %v", body["resources"]) + } + // Said rather than guessed: this lab has no byte store, so a reset will + // restore no bytes, and the caller is told here rather than discovering it + // after a reset that looked clean. + if carried, _ := body["bytes"].(bool); carried { + t.Error("a lab with no byte store reported carrying bytes") + } + if version, _ := body["schema_version"].(float64); int(version) != 1 { + t.Errorf("schema_version %v", body["schema_version"]) + } + + // With a byte store armed, both halves are taken and the answer says so. + // Asserted on a SECOND lab rather than by widening the first, because the + // absence of a byte store is the ordinary case and must keep its own + // assertion above. + root := t.TempDir() + if err := os.Chmod(root, 0o700); err != nil { + t.Fatal(err) + } + bytesStore, err := objectstore.Open(t.Context(), filepath.Join(root, "objects")) + if err != nil { + t.Fatalf("open the byte store: %v", err) + } + t.Cleanup(func() { + if err := bytesStore.Close(); err != nil { + t.Error(err) + } + }) + withBytes, _ := labServerWithBytes(t, bytesStore, stubPack{name: "acme"}) + code, body = checkpointPost(t, withBytes.URL+"/_feint/checkpoint/baseline") + if code != http.StatusOK { + t.Fatalf("checkpoint with bytes: %d %v", code, body) + } + if carried, _ := body["bytes"].(bool); !carried { + t.Error("a lab with a byte store reported carrying none") + } + // And the byte checkpoint really exists, which is the half a status code + // cannot show: resetting it must find something to restore. + if err := bytesStore.ResetCheckpoint(t.Context(), "baseline"); err != nil { + t.Errorf("the checkpoint named the store and did not take its bytes: %v", err) + } + + // A name that is a path component before it is a name. Refused at the + // door, because the byte store writes a directory per checkpoint and this + // repository's order is a refusal at intake rather than escaping later. + for _, bad := range []string{"..", ".", "a/b", "a b", ""} { + if code, _ := checkpointPost(t, server.URL+"/_feint/checkpoint/"+bad); code == http.StatusOK { + t.Errorf("the name %q was accepted", bad) + } + } +} + +// TestAResetDestroysWhatTheCheckpointDoesNotHold is cited by handleReset. +// +// The assertion is on what the PACK WAS ASKED to take down, not on the status +// code: a reset that restored the store and told nobody about the container +// still running is the exact gap #853 names. +func TestAResetDestroysWhatTheCheckpointDoesNotHold(t *testing.T) { + acme := &reconcilingPack{ + stubPack: stubPack{name: "acme"}, + done: machine.Reconciled{Destroyed: []string{"feint-acme-later"}}, + } + // A second pack that cannot reconcile, mounted on purpose: the loop must + // skip it rather than fail, because a pack that drives no machine is the + // ordinary case. + server, held := labServer(t, acme, stubPack{name: "quiet"}) + + put(t, held, "acme", "instance/server", "kept") + // A second record SHARING that identifier across kinds, which is the + // population a narrower comparison cannot see. The falsification found + // this gap: a difference computed on identifiers alone passed every + // assertion here until these two existed, because every id in the test + // was distinct and a narrowed key still told them apart. + put(t, held, "acme", "instance/ip", "kept") + if code, body := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline"); code != http.StatusOK { + t.Fatalf("checkpoint: %d %v", code, body) + } + + // The lab moves on: one more resource of each pack, and one that shares + // its identifier with a record the checkpoint DOES hold, on another kind. + put(t, held, "acme", "instance/server", "later") + put(t, held, "acme", "instance/volume", "kept") + put(t, held, "quiet", "instance/server", "alsolater") + + code, body := checkpointPost(t, server.URL+"/_feint/reset/baseline") + if code != http.StatusOK { + t.Fatalf("reset: %d %v", code, body) + } + + // The store is back to the two the checkpoint held. + if count, _ := body["resources"].(float64); int(count) != 2 { + t.Errorf("resources after the reset: %v", body["resources"]) + } + // And the pack was handed exactly the records the checkpoint does not + // hold — not the ones it does, which is the direction that would destroy a + // machine the checkpoint expects to keep. TWO of them, and the second + // shares its identifier with a kept record: a comparison that dropped the + // kind would have taken it for the one that survived and left its machine + // running. + if len(acme.got) != 1 || len(acme.got[0]) != 2 { + t.Fatalf("the pack was asked to reconcile %v", acme.got) + } + handed := append([]string(nil), acme.got[0]...) + sort.Strings(handed) + if handed[0] != "kept" || handed[1] != "later" { + t.Fatalf("the pack was handed %v, want the volume sharing an id and the extra server", handed) + } + if destroyed, _ := body["destroyed"].([]any); len(destroyed) != 1 || destroyed[0] != "feint-acme-later" { + t.Errorf("destroyed %v", body["destroyed"]) + } + // Recreation IS attempted now, and the answer says so. Nothing was + // recreated here because this lab has no runtime: the records the + // checkpoint holds name machines the survey cannot be asked about, and a + // reset that started containers because nobody could be asked would be + // the opposite defect. + if recreates, _ := body["recreates"].(bool); !recreates { + t.Error("the reset says it does not recreate, which stopped being true") + } + if back, _ := body["recreated"].([]any); len(back) != 0 { + t.Errorf("a lab with no runtime recreated %v", back) + } + + // Twice is a no-op, which is #853's own criterion: nothing is left to + // reconcile, so the pack is not asked again. + if code, body := checkpointPost(t, server.URL+"/_feint/reset/baseline"); code != http.StatusOK { + t.Fatalf("second reset: %d %v", code, body) + } + if len(acme.got) != 1 { + t.Errorf("a second reset asked for more reconciliation: %v", acme.got) + } + + // An unknown checkpoint is refused by name, and the refusal says how to + // make one: a 404 alone sends the reader to the source. + code, body = checkpointPost(t, server.URL+"/_feint/reset/nothing-here") + if code != http.StatusNotFound { + t.Fatalf("reset of an unknown checkpoint: %d %v", code, body) + } + if message, _ := body["error"].(string); message == "" || + !strings.Contains(message, "nothing-here") || !strings.Contains(message, "/_feint/checkpoint/") { + t.Errorf("the refusal does not say what to do: %v", body["error"]) + } +} diff --git a/internal/core/emulator/emulator.go b/internal/core/emulator/emulator.go index dd457954..8b24bb22 100644 --- a/internal/core/emulator/emulator.go +++ b/internal/core/emulator/emulator.go @@ -360,6 +360,11 @@ type Server struct { // self records the patterns of the endpoints the emulator serves about // itself, so they can be enumerated rather than remembered. self []string + // checkpoints holds the control-plane half of each named checkpoint + // (#853). In memory, which is the scope the issue describes: a reset + // returns to the session's initial state, and a checkpoint meant to + // outlive the process is what `feint snapshot save` writes to a file. + checkpoints checkpoints // shapeCovered is the set of operations an observed real-cloud shape // covers, handed in by SetShapeCovered. nil means no catalogue was given, // which the evidence record reports as "unknown" rather than "unobserved". @@ -456,6 +461,12 @@ func NewServer(env *Env, packs ...Pack) (*Server, error) { // conformance script or a CI job reads after the fact. Tying it to the page // would make it unavailable to exactly the callers it was asked for. s.mountSelf("GET /_feint/trace", s.handleTrace) + // Named checkpoints of a whole lab, and the reset that returns to one + // (#853). Mounted beside /_feint/state because they are the same + // subject seen as a lifecycle rather than as storage. + s.mountSelf("POST /_feint/checkpoint/{name}", s.handleCheckpointSave) + s.mountSelf("DELETE /_feint/checkpoint/{name}", s.handleCheckpointDelete) + s.mountSelf("POST /_feint/reset/{name}", s.handleReset) s.mountSelf("GET /_feint/state", s.handleStateRead) s.mountSelf("PUT /_feint/state", s.handleStateWrite) // The assertion spans behind the behaviour and negative evidence axes: a diff --git a/internal/core/emulator/recreate_test.go b/internal/core/emulator/recreate_test.go new file mode 100644 index 00000000..0070cd1f --- /dev/null +++ b/internal/core/emulator/recreate_test.go @@ -0,0 +1,128 @@ +package emulator_test + +import ( + "context" + "net/http" + "testing" + + "github.com/stephrobert/feint/internal/core/machine" + "github.com/stephrobert/feint/internal/core/resource" +) + +// restartingPack implements BOTH halves, and records what each was handed. +type restartingPack struct { + stubPack + gone [][]string + missing [][]string + back machine.Reconciled +} + +func (p *restartingPack) ReconcileGone(_ context.Context, gone []*resource.Resource) machine.Reconciled { + p.gone = append(p.gone, idsOf(gone)) + return machine.Reconciled{} +} + +func (p *restartingPack) ReconcileMissing(_ context.Context, records []*resource.Resource, + held []string, +) machine.Reconciled { + // The pack decides which of its records is missing, with its own key. + // Here the key is "machine", which is what put() writes. + var missing []*resource.Resource + for _, res := range records { + name := res.Runtime["machine"] + if name == "" { + continue + } + found := false + for _, candidate := range held { + if candidate == name { + found = true + } + } + if !found { + missing = append(missing, res) + } + } + p.missing = append(p.missing, idsOf(missing)) + return p.back +} + +func idsOf(resources []*resource.Resource) []string { + var out []string + for _, res := range resources { + out = append(out, res.ID) + } + return out +} + +// surveyingDriver answers what the host holds, and nothing else. +type surveyingDriver struct { + machine.Noop + held []string +} + +func (d surveyingDriver) Survey(context.Context) (machine.Leftovers, error) { + return machine.Leftovers{Machines: d.held}, nil +} + +// TestAResetStartsAgainWhatTheHostNoLongerHolds is cited by missingOnHost and +// by the three packs' ReconcileMissing. +// +// The question a reset has to ask is not what the store says — the store is +// exactly what just changed, and it says the server is running. It is whether +// anything backs it, and only the runtime can answer. +func TestAResetStartsAgainWhatTheHostNoLongerHolds(t *testing.T) { + acme := &restartingPack{ + stubPack: stubPack{name: "acme"}, + back: machine.Reconciled{Recreated: []string{"feint-acme-alive"}}, + } + server, held := labServerWithRuntime(t, machine.Use(surveyingDriver{held: []string{"feint-acme-alive"}}), acme) + + // Two records: one whose machine the host holds, one whose machine it does + // not. Only the second is missing. + put(t, held, "acme", "instance/server", "alive") + put(t, held, "acme", "instance/server", "vanished") + if code, body := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline"); code != http.StatusOK { + t.Fatalf("checkpoint: %d %v", code, body) + } + + code, body := checkpointPost(t, server.URL+"/_feint/reset/baseline") + if code != http.StatusOK { + t.Fatalf("reset: %d %v", code, body) + } + if len(acme.missing) != 1 || len(acme.missing[0]) != 1 || acme.missing[0][0] != "vanished" { + t.Fatalf("the pack was asked to start %v; only the record the host does not back is missing", + acme.missing) + } + if back, _ := body["recreated"].([]any); len(back) != 1 { + t.Errorf("recreated %v", body["recreated"]) + } + // And the record it DOES back was not touched, which is the direction + // that would start a second container beside one already running. + if len(acme.gone) != 0 { + t.Errorf("a reset that changed nothing asked for destructions: %v", acme.gone) + } +} + +// TestAResetAsksNothingOfAHostNobodyCanSurvey is the refusal half. +// +// A driver that cannot be surveyed answers asked=false, and then nothing is +// reported missing. Claiming a machine is absent because nobody could be asked +// would have a reset start containers on a host that already holds them — +// which is the expensive direction, and the one an undeclared capability is +// supposed to prevent everywhere else in this repository. +func TestAResetAsksNothingOfAHostNobodyCanSurvey(t *testing.T) { + acme := &restartingPack{stubPack: stubPack{name: "acme"}} + // The zero runtime: Noop, which is not a Surveyor. + server, held := labServer(t, acme) + put(t, held, "acme", "instance/server", "alive") + if code, _ := checkpointPost(t, server.URL+"/_feint/checkpoint/baseline"); code != http.StatusOK { + t.Fatal("checkpoint refused") + } + if code, _ := checkpointPost(t, server.URL+"/_feint/reset/baseline"); code != http.StatusOK { + t.Fatal("reset refused") + } + if len(acme.missing) != 0 { + t.Errorf("a host nobody can survey was taken for an empty one: %v", acme.missing) + } +} diff --git a/internal/core/emulator/schema.go b/internal/core/emulator/schema.go index 4dc1e93c..9a9879ce 100644 --- a/internal/core/emulator/schema.go +++ b/internal/core/emulator/schema.go @@ -126,4 +126,13 @@ const ( // because a suite that arms a fault from a committed file is a consumer on // day one — the same reasoning that put the other four here. FaultsSchemaVersion = 1 + // CheckpointSchemaVersion is the shape of POST /_feint/checkpoint/{name}, + // DELETE /_feint/checkpoint/{name} and POST /_feint/reset/{name}. + // + // 1 is the first: the name, the resource count and whether bytes were part + // of it; and for a reset, what was destroyed, what was skipped, the same + // split per provider, and the two fields that say recreation is not + // attempted. Frozen from the start like the four above, because a suite + // that resets between tests is a consumer on day one. + CheckpointSchemaVersion = 1 ) diff --git a/internal/core/machine/reconcile.go b/internal/core/machine/reconcile.go new file mode 100644 index 00000000..38049bec --- /dev/null +++ b/internal/core/machine/reconcile.go @@ -0,0 +1,185 @@ +package machine + +import ( + "context" + "slices" + + "github.com/stephrobert/feint/internal/core/resource" +) + +// Reconciling the host with a store that just changed under it (#853). +// +// `PUT /_feint/state` and `feint snapshot load` restore the control plane +// verbatim and touch nothing else, which is the gap #853 names: a resource +// that disappears from the store leaves its container running, and the API no +// longer mentions it. Nobody destroys it, and the next `incus list` is the +// first time anybody finds out. +// +// ## Why the destroying half lives here and the recreating half does not +// +// Destroying is the same gesture for every pack: take the machine the record +// names, check it is ours, remove it. Written three times it would be fixed +// once, which is the defect CLAUDE.md names and this repository has already +// paid for on the lifecycle. +// +// Recreating is NOT the same gesture. Starting a machine needs a Boot — the +// image, the login, the cloud-init a pack renders for its own product — and +// that is pack knowledge by construction. #853 hedges in the same place, with +// "missing checkpoint resources recreated where the runtime contract supports +// it", so the recreating half belongs to each pack and is bounded by what that +// pack can actually boot. +// +// ## Why it goes through Remove rather than the driver +// +// Remove answers the ownership question before the shape one: `Binding.ours` +// refuses a machine name outside this binding's prefix, and the driver's +// mustOwn reads the label the emulator itself wrote. A restored store is +// untrusted input — snapshot.go documents the format as meant to outlive its +// instance — so a reconciliation driven by names that came out of a file is +// exactly the path that must not shortcut either check. + +// Reconciled reports what a reconciliation did, so a caller can say it rather +// than assume it. +type Reconciled struct { + // Destroyed names the machines taken down because their record is gone. + Destroyed []string + // Recreated names the machines started again because a record the + // checkpoint holds had none. + Recreated []string + // Failed names the records whose machine could not be started again. + // Separate from Skipped, because a start that was attempted and refused + // and a record nobody could act on need different answers from the + // operator. + Failed []string + // Skipped names records whose machine this binding does not own, or which + // name no machine at all. Reported rather than silently passed over: a + // record nobody could act on is what an operator needs to hear about. + Skipped []string +} + +// ReconcileGone destroys the machines of records that no longer exist. +// +// The input is what the store held BEFORE the restore and no longer holds. The +// caller computes it, because only the caller saw both sides, and it is a +// plain list of resources so that nothing about a provider reaches this +// package. +// +// A binding with no runtime destroys nothing and says so through Skipped: the +// metadata-only mode is the default, and a reconciliation that claimed to have +// removed containers nobody started would be the lie this project exists to +// avoid. +// +// There is no ownership branch here, and that absence is deliberate: Remove +// already asks the question, and `ours` already decided what the answer does +// — fall back to the derived name rather than refuse. A second guard beside a +// working one is how two answers to one question start disagreeing. +// +// TestReconcileDestroysOnlyTheMachinesItOwns fails without this. +func (b Binding) ReconcileGone(ctx context.Context, gone []*resource.Resource) Reconciled { + var done Reconciled + for _, res := range gone { + if res == nil { + continue + } + name := res.Runtime[b.RuntimeKey] + if name == "" { + // Nothing was ever started for it. Not an error and not a skip + // worth reporting: a record with no machine is the ordinary case + // for an address, a volume or a security group. + continue + } + if b.driver == nil { + // Metadata only, which is the default: nothing was started, so + // nothing is destroyed and the caller is told rather than left to + // read an empty Destroyed as a success. + done.Skipped = append(done.Skipped, name) + continue + } + // Remove answers the ownership question itself, and answers it the way + // this package already decided: `ours` does not refuse a foreign name, + // it falls back to the one the emulator could have created, because a + // record may well have a machine and that is the only name it could + // ever have had. An ownership branch here would have been a second + // guard beside a working one, which is how two answers to one question + // start disagreeing. + b.Remove(ctx, res.ID, name) + done.Destroyed = append(done.Destroyed, name) + } + return done +} + +// ReconcileMissing starts again the machines of records that have none. +// +// It is the other half of #853's "runtime reconciled to the checkpoint", and +// the issue bounds it in the same breath: "where the runtime contract supports +// it". What that turned out to be here is better than I expected, and it was +// measured rather than assumed. +// +// ## Why a rebuilt boot is the boot the original had +// +// A pack builds its Boot from three places, and a reset has already put all +// three back by the time this runs: +// +// - the record's own attributes, which the restore brought back verbatim; +// - the store, for things like the project's SSH keys, which the restore +// brought back too — the keys are part of the checkpoint; +// - the image catalogue, which is compiled into the binary and depends on no +// state at all. +// +// So the fear that sequenced this half last — an image gone from the +// catalogue, a key deleted, a user data that no longer exists — does not +// survive reading where those values come from. What a label the catalogue +// does not know produces is an empty image reference, which is exactly what +// the ORIGINAL poweron produced for that same record. +// +// ## Why the Boot comes in as a function +// +// Building one is pack knowledge: the image table, the login, the cloud-init a +// pack renders for its own product. Taking a builder rather than a Boot keeps +// the loop, the ownership path and the honest reporting in one place while the +// one thing that genuinely varies stays with the pack. Each pack writes one +// line. +// +// ## What it refuses to call a success +// +// PowerOn already answers false when a runtime was configured and did not +// deliver, and sets FailedState rather than Running. This reports those as +// Failed instead of folding them into Recreated: a reset that said it brought +// three machines back when one of them is in FailedState would be the half- +// success this project exists to avoid, and the verdict above it compares the +// observable state anyway — so a lie here would be caught, loudly, in the +// wrong place. +// +// TestReconcileStartsOnlyTheMachinesThatAreMissing fails without this. +func (b Binding) ReconcileMissing(ctx context.Context, records []*resource.Resource, + held []string, boot func(*resource.Resource) Boot, +) Reconciled { + var done Reconciled + for _, res := range records { + if res == nil || boot == nil { + continue + } + // Which of these records is missing a machine is decided HERE, with + // this binding's own key, and not by the caller. The core tried to + // guess it with a "feint-" prefix and was wrong twice: a provider's + // vocabulary in the neutral layer, and a match broad enough to pick up + // a test's bookkeeping stamp and try to boot it. + name := res.Runtime[b.RuntimeKey] + if name == "" || slices.Contains(held, name) { + continue + } + if b.driver == nil { + // Metadata only: PowerOn would answer running without starting + // anything, which is the documented degraded mode and not + // something to report as a recreation. + done.Skipped = append(done.Skipped, b.Name(res.ID)) + continue + } + if !b.PowerOn(ctx, res, boot(res)) { + done.Failed = append(done.Failed, b.Name(res.ID)) + continue + } + done.Recreated = append(done.Recreated, res.Runtime[b.RuntimeKey]) + } + return done +} diff --git a/internal/core/machine/reconcile_test.go b/internal/core/machine/reconcile_test.go new file mode 100644 index 00000000..dba0152c --- /dev/null +++ b/internal/core/machine/reconcile_test.go @@ -0,0 +1,114 @@ +package machine + +import ( + "context" + "slices" + "testing" + "time" + + "github.com/stephrobert/feint/internal/core/resource" +) + +// removingDriver records the names a reconciliation asked to remove, which is +// the only thing worth asserting here: a reconciliation is judged by the +// commands it emits, not by what it returns about itself. +type removingDriver struct { + recordingDriver + removed []string +} + +func (d *removingDriver) Remove(_ context.Context, name string) error { + d.removed = append(d.removed, name) + return nil +} + +func record(id, machine string) *resource.Resource { + res := resource.New(id, "instance/server", + resource.Tenant{Provider: "acme", Project: "p", Zone: "z"}, "running", + time.Unix(1700000000, 0).UTC()) + if machine != "" { + res.Runtime = map[string]string{"machine": machine} + } + return res +} + +// TestReconcileDestroysOnlyTheMachinesItOwns is cited by ReconcileGone. +// +// #853's gap in one sentence: a restore brings the control plane back and +// touches nothing else, so a resource that disappeared leaves its container +// running and the API no longer mentions it. This is the gesture that closes +// it, and the four populations below are the ones that can go wrong. +func TestReconcileDestroysOnlyTheMachinesItOwns(t *testing.T) { + t.Run("a record with a machine of ours is destroyed", func(t *testing.T) { + d := &removingDriver{} + done := bootBinding(d).ReconcileGone(t.Context(), []*resource.Resource{ + record("a", "feint-acme-a"), + record("b", "feint-acme-b"), + }) + if !slices.Equal(d.removed, []string{"feint-acme-a", "feint-acme-b"}) { + t.Errorf("removed %v", d.removed) + } + if len(done.Destroyed) != 2 || len(done.Skipped) != 0 { + t.Errorf("reported %+v", done) + } + }) + + t.Run("a record with no machine is not an event", func(t *testing.T) { + // The ordinary case for an address, a volume or a security group. + // Reporting it would bury the machines under records that never had + // one. + d := &removingDriver{} + done := bootBinding(d).ReconcileGone(t.Context(), []*resource.Resource{record("a", "")}) + if len(d.removed) != 0 { + t.Errorf("removed %v for a record that named no machine", d.removed) + } + if len(done.Destroyed) != 0 || len(done.Skipped) != 0 { + t.Errorf("reported %+v", done) + } + }) + + t.Run("a foreign name reaches no command as itself", func(t *testing.T) { + // A restored store is untrusted input: snapshot.go documents the + // format as meant to outlive its instance, and an audit turned a + // crafted one into a forced removal of a name the emulator never + // created. The reconciliation must not be the path that reopens it. + d := &removingDriver{} + bootBinding(d).ReconcileGone(t.Context(), []*resource.Resource{ + record("a", "production-database"), + }) + if slices.Contains(d.removed, "production-database") { + t.Fatalf("a name outside this binding's prefix reached a removal: %v", d.removed) + } + // And it did not become a silent no-op either: the derived name is the + // only machine that record could ever have had, so that is what is + // removed. A reconciliation that skipped it would leave the real + // container of a tampered record running. + if !slices.Equal(d.removed, []string{"feint-acme-a"}) { + t.Errorf("removed %v, want the derived name", d.removed) + } + }) + + t.Run("no runtime destroys nothing and says so", func(t *testing.T) { + // Metadata only is the DEFAULT, so this is the common case rather + // than an edge one. An empty Destroyed read as success is how a + // caller comes to believe containers were removed on a station that + // never started any. + done := bootBinding(nil).ReconcileGone(t.Context(), []*resource.Resource{ + record("a", "feint-acme-a"), + }) + if len(done.Destroyed) != 0 { + t.Errorf("a binding with no runtime claims to have destroyed %v", done.Destroyed) + } + if !slices.Equal(done.Skipped, []string{"feint-acme-a"}) { + t.Errorf("skipped %v", done.Skipped) + } + }) + + t.Run("nothing to reconcile emits nothing", func(t *testing.T) { + d := &removingDriver{} + done := bootBinding(d).ReconcileGone(t.Context(), nil) + if len(d.removed) != 0 || len(done.Destroyed) != 0 || len(done.Skipped) != 0 { + t.Errorf("an empty reconciliation did something: %v %+v", d.removed, done) + } + }) +} diff --git a/internal/core/machine/recreate_test.go b/internal/core/machine/recreate_test.go new file mode 100644 index 00000000..55159041 --- /dev/null +++ b/internal/core/machine/recreate_test.go @@ -0,0 +1,149 @@ +package machine + +import ( + "context" + "errors" + "slices" + "testing" + "time" + + "github.com/stephrobert/feint/internal/core/resource" +) + +// startingDriver records what a reconciliation asked to start, and can refuse. +type startingDriver struct { + recordingDriver + started []string + refuse bool +} + +func (d *startingDriver) Start(_ context.Context, spec Spec) (Machine, error) { + d.started = append(d.started, spec.Name) + if d.refuse { + return Machine{}, errors.New("the runtime refused") + } + return Machine{Name: spec.Name, Addresses: []string{"10.42.0.9"}, Running: true}, nil +} + +func absent(id string) *resource.Resource { + res := resource.New(id, "instance/server", + resource.Tenant{Provider: "acme", Project: "p", Zone: "z"}, "running", + time.Unix(1700000000, 0).UTC()) + res.Runtime = map[string]string{"machine": "feint-acme-" + id} + return res +} + +// TestReconcileStartsOnlyTheMachinesThatAreMissing is cited by +// ReconcileMissing and by the three packs that delegate to it. +// +// The measurement that sequenced this half last is worth repeating here, +// because it turned out the other way: a Boot rebuilt from a restored record +// is the Boot the original had, since every input is the record itself, the +// store the reset has already restored, or the image catalogue compiled into +// the binary. So what is left to get wrong is the reporting, and that is what +// these four populations are about. +func TestReconcileStartsOnlyTheMachinesThatAreMissing(t *testing.T) { + boot := func(res *resource.Resource) Boot { + return Boot{Image: "img", User: "root", Hostname: res.ID} + } + + t.Run("a record with no machine on the host is started again", func(t *testing.T) { + d := &startingDriver{} + first, second := absent("a"), absent("b") + done := bootBinding(d).ReconcileMissing(t.Context(), + []*resource.Resource{first, second}, nil, boot) + if len(d.started) != 2 { + t.Fatalf("started %v", d.started) + } + if len(done.Recreated) != 2 || len(done.Failed) != 0 { + t.Errorf("reported %+v", done) + } + // The record is rewritten, which is the half that makes the next + // reset a no-op: a resource still naming the container that is gone + // would be reported missing again, forever. + if first.Runtime["machine"] == "" || first.State != "running" { + t.Errorf("the record was not brought up to date: %+v", first.Runtime) + } + }) + + t.Run("a runtime that refuses is Failed, not Recreated", func(t *testing.T) { + // PowerOn answers false and sets FailedState rather than Running. A + // reset that folded this into Recreated would say it brought a machine + // back that is in FailedState, and the verdict above it compares the + // observable state anyway — so the lie would surface loudly, in the + // wrong place. + d := &startingDriver{refuse: true} + res := absent("a") + done := bootBinding(d).ReconcileMissing(t.Context(), []*resource.Resource{res}, nil, boot) + if len(done.Recreated) != 0 { + t.Errorf("a refused start was reported as recreated: %+v", done) + } + if !slices.Equal(done.Failed, []string{"feint-acme-a"}) { + t.Errorf("failed %v", done.Failed) + } + if res.State != "failed" { + t.Errorf("the record says %q after a refused start", res.State) + } + }) + + t.Run("no runtime starts nothing and says so", func(t *testing.T) { + // Metadata only is the DEFAULT. PowerOn would answer running without + // starting anything, which is the documented degraded mode and not a + // recreation to report. + done := bootBinding(nil).ReconcileMissing(t.Context(), + []*resource.Resource{absent("a")}, nil, boot) + if len(done.Recreated) != 0 || len(done.Failed) != 0 { + t.Errorf("a binding with no runtime reported %+v", done) + } + if !slices.Equal(done.Skipped, []string{"feint-acme-a"}) { + t.Errorf("skipped %v", done.Skipped) + } + }) + + t.Run("nothing missing and no boot builder are both no-ops", func(t *testing.T) { + d := &startingDriver{} + if done := bootBinding(d).ReconcileMissing(t.Context(), nil, nil, boot); done.Recreated != nil { + t.Errorf("an empty reconciliation did something: %+v", done) + } + // A nil builder is a programming error upstream, and starting a + // machine from a zero Boot would be worse than doing nothing: it would + // boot an image nobody asked for. + if done := bootBinding(d).ReconcileMissing(t.Context(), + []*resource.Resource{absent("a")}, nil, nil); len(done.Recreated) != 0 { + t.Errorf("a nil boot builder started something: %+v", done) + } + if len(d.started) != 0 { + t.Errorf("commands were emitted: %v", d.started) + } + }) + t.Run("a machine the host still holds is left alone", func(t *testing.T) { + // This is where the decision moved to, and why. The core used to pick + // the missing records itself, with a "feint-" prefix: a provider's + // vocabulary in the neutral layer, and broad enough to match a test's + // own bookkeeping stamp, which it then tried to boot. The binding + // knows its RuntimeKey, so it decides. + d := &startingDriver{} + res := absent("a") + done := bootBinding(d).ReconcileMissing(t.Context(), + []*resource.Resource{res}, []string{"feint-acme-a"}, boot) + if len(d.started) != 0 { + t.Errorf("a machine the host holds was started again: %v", d.started) + } + if len(done.Recreated) != 0 || len(done.Failed) != 0 || len(done.Skipped) != 0 { + t.Errorf("reported %+v for a record that needed nothing", done) + } + }) + + t.Run("a record whose key names no machine is not a candidate", func(t *testing.T) { + d := &startingDriver{} + res := absent("a") + res.Runtime = map[string]string{"address": "10.0.0.5"} + if done := bootBinding(d).ReconcileMissing(t.Context(), + []*resource.Resource{res}, nil, boot); len(done.Recreated) != 0 { + t.Errorf("a record with no machine name was started: %+v", done) + } + if len(d.started) != 0 { + t.Errorf("commands were emitted: %v", d.started) + } + }) +} diff --git a/internal/core/machine/surface.go b/internal/core/machine/surface.go index 2d6ee666..3c09b261 100644 --- a/internal/core/machine/surface.go +++ b/internal/core/machine/surface.go @@ -125,6 +125,9 @@ func PackSurface() map[string]string { "Binding.Observe": "read a resource back after an effect, with the same write-back discipline", "Binding.PowerOff": "stop the machine behind a resource", "Binding.Destroy": "destroy the machine behind a resource", + "Binding.ReconcileGone": "take down the machines of records a reset removed (#853), in one shared gesture", + "Binding.ReconcileMissing": "start again the machines of records a reset brought back and the host no longer holds (#853), with the Boot this pack builds", + "Reconciled": "what a reconciliation did: what it destroyed, and what it could not act on", "Binding.RefreshIfRunning": "re-read what the host holds for a machine the store calls running", "Binding.RuntimeKey": "the pack's own Runtime key, to read the machine name it is about to hand to Unroute", "Boot": "what to boot: image, login, keys, user data", diff --git a/internal/providers/exoscale/machines.go b/internal/providers/exoscale/machines.go index 23fe0a3a..93e94a8a 100644 --- a/internal/providers/exoscale/machines.go +++ b/internal/providers/exoscale/machines.go @@ -279,3 +279,37 @@ func (p *Pack) refresh(ctx context.Context, res *resource.Resource) bool { func (p *Pack) addressOf(res *resource.Resource) string { return p.reconciler().PublicAddressOf(res) } + +// ReconcileGone takes down the machines of records a reset removed (#853). +// +// Two lines, and that is the point: the gesture is identical for every pack, +// so it lives on the shared binding where a defect is fixed once. What this +// pack adds is which binding — its prefix, its runtime key, its ownership +// mark — and nothing else. +// +// The recreating half is deliberately absent here too. Starting a machine +// needs a Boot this pack renders from its own catalogue, and #853 bounds that +// in the same words: "where the runtime contract supports it". +// emulator.TestAResetDestroysWhatTheCheckpointDoesNotHold drives this through +// the server; machine.TestReconcileDestroysOnlyTheMachinesItOwns holds what it +// delegates to. +func (p *Pack) ReconcileGone(ctx context.Context, gone []*resource.Resource) machine.Reconciled { + return p.binding().ReconcileGone(ctx, gone) +} + +// ReconcileMissing starts again the machines of records a reset brought back +// and the host no longer holds (#853). +// +// One line, for the reason ReconcileGone is two: the loop, the ownership path +// and the honest reporting are shared, and what varies is the Boot this pack +// builds from its own catalogue. Handing the builder rather than a Boot is +// what keeps it that way. +// +// emulator.TestAResetStartsAgainWhatTheHostNoLongerHolds drives this through +// the server; machine.TestReconcileStartsOnlyTheMachinesThatAreMissing holds +// what it delegates to. +func (p *Pack) ReconcileMissing(ctx context.Context, records []*resource.Resource, + held []string, +) machine.Reconciled { + return p.binding().ReconcileMissing(ctx, records, held, p.bootOf) +} diff --git a/internal/providers/outscale/machines.go b/internal/providers/outscale/machines.go index 90ceef88..e0c36092 100644 --- a/internal/providers/outscale/machines.go +++ b/internal/providers/outscale/machines.go @@ -272,3 +272,37 @@ func (p *Pack) attachmentOf(res *resource.Resource) []machine.Attachment { PrefixLen: prefix.Bits(), }} } + +// ReconcileGone takes down the machines of records a reset removed (#853). +// +// Two lines, and that is the point: the gesture is identical for every pack, +// so it lives on the shared binding where a defect is fixed once. What this +// pack adds is which binding — its prefix, its runtime key, its ownership +// mark — and nothing else. +// +// The recreating half is deliberately absent here too. Starting a machine +// needs a Boot this pack renders from its own catalogue, and #853 bounds that +// in the same words: "where the runtime contract supports it". +// emulator.TestAResetDestroysWhatTheCheckpointDoesNotHold drives this through +// the server; machine.TestReconcileDestroysOnlyTheMachinesItOwns holds what it +// delegates to. +func (p *Pack) ReconcileGone(ctx context.Context, gone []*resource.Resource) machine.Reconciled { + return p.binding().ReconcileGone(ctx, gone) +} + +// ReconcileMissing starts again the machines of records a reset brought back +// and the host no longer holds (#853). +// +// One line, for the reason ReconcileGone is two: the loop, the ownership path +// and the honest reporting are shared, and what varies is the Boot this pack +// builds from its own catalogue. Handing the builder rather than a Boot is +// what keeps it that way. +// +// emulator.TestAResetStartsAgainWhatTheHostNoLongerHolds drives this through +// the server; machine.TestReconcileStartsOnlyTheMachinesThatAreMissing holds +// what it delegates to. +func (p *Pack) ReconcileMissing(ctx context.Context, records []*resource.Resource, + held []string, +) machine.Reconciled { + return p.binding().ReconcileMissing(ctx, records, held, p.bootOf) +} diff --git a/internal/providers/scaleway/machines.go b/internal/providers/scaleway/machines.go index 91a54d9b..7a865205 100644 --- a/internal/providers/scaleway/machines.go +++ b/internal/providers/scaleway/machines.go @@ -333,3 +333,37 @@ func (p *Pack) gatewayPushesDefaultRoute(privateNetworkID string) bool { } return false } + +// ReconcileGone takes down the machines of records a reset removed (#853). +// +// Two lines, and that is the point: the gesture is identical for every pack, +// so it lives on the shared binding where a defect is fixed once. What this +// pack adds is which binding — its prefix, its runtime key, its ownership +// mark — and nothing else. +// +// The recreating half is deliberately absent here too. Starting a machine +// needs a Boot this pack renders from its own catalogue, and #853 bounds that +// in the same words: "where the runtime contract supports it". +// emulator.TestAResetDestroysWhatTheCheckpointDoesNotHold drives this through +// the server; machine.TestReconcileDestroysOnlyTheMachinesItOwns holds what it +// delegates to. +func (p *Pack) ReconcileGone(ctx context.Context, gone []*resource.Resource) machine.Reconciled { + return p.binding().ReconcileGone(ctx, gone) +} + +// ReconcileMissing starts again the machines of records a reset brought back +// and the host no longer holds (#853). +// +// One line, for the reason ReconcileGone is two: the loop, the ownership path +// and the honest reporting are shared, and what varies is the Boot this pack +// builds from its own catalogue. Handing the builder rather than a Boot is +// what keeps it that way. +// +// emulator.TestAResetStartsAgainWhatTheHostNoLongerHolds drives this through +// the server; machine.TestReconcileStartsOnlyTheMachinesThatAreMissing holds +// what it delegates to. +func (p *Pack) ReconcileMissing(ctx context.Context, records []*resource.Resource, + held []string, +) machine.Reconciled { + return p.binding().ReconcileMissing(ctx, records, held, p.bootOf) +} diff --git a/mise.toml b/mise.toml index 92b46559..2ca2a924 100644 --- a/mise.toml +++ b/mise.toml @@ -331,6 +331,7 @@ run = [ # itself under --vm incus and --vm incus-vm rather than failing there. "tools/conformance/outscale/balancer.sh http://$FEINT_ADDR", "tools/conformance/run-sweep.sh http://$FEINT_ADDR", + "tools/conformance/reset-host.sh http://$FEINT_ADDR", ] [tasks."conformance:exoscale-terraform"] diff --git a/tools/conformance/leg.sh b/tools/conformance/leg.sh index 8300aa35..b09dae31 100755 --- a/tools/conformance/leg.sh +++ b/tools/conformance/leg.sh @@ -177,6 +177,11 @@ case "$leg" in # command that starts and stops one, so the endpoint is read only to ask # whether a runtime is configured at all (#848). tools/conformance/run-sweep.sh "$endpoint" + # The host half of a reset (#853), which no unit test can reach: a + # recording driver proves which names the code emits, and this proves what + # a real runtime does with them — above all that the machine the + # checkpoint HOLDS is still there afterwards. + tools/conformance/reset-host.sh "$endpoint" ;; esac diff --git a/tools/conformance/reset-host.sh b/tools/conformance/reset-host.sh new file mode 100755 index 00000000..0482b0d8 --- /dev/null +++ b/tools/conformance/reset-host.sh @@ -0,0 +1,133 @@ +#!/usr/bin/env bash +# Conformance check: a reset reconciles the HOST, and touches nothing else. +# +# This is #853's closing criterion that no unit test can reach: "no foreign +# runtime object is touched". The reconciliation is exercised elsewhere through +# a recording driver, which proves which names the code emits; this proves what +# a real runtime does with them. +# +# Three assertions, and the middle one is the product: +# +# - the machine of a resource the checkpoint does NOT hold is gone; +# - the machine of a resource the checkpoint DOES hold is still running. A +# reset that swept both would return an operator to a checkpoint whose +# servers answer nothing, which is worse than no reset; +# - every container the emulator did not create is still there, by name. +# +# The third is why `incus list` is read before and after rather than counted: +# a count cannot tell a neighbour's container from one of ours, and this +# repository has already measured a forced removal reaching a name it did not +# own. +# +# Usage: tools/conformance/reset-host.sh [endpoint] +set -euo pipefail + +ENDPOINT="${1:-http://127.0.0.1:4599}" +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +FEINT="${FEINT_BIN:-./feint}" + +# shellcheck source=/dev/null +. "$SCRIPT_DIR/guard.sh" +guard_local "$ENDPOINT" + +fail() { echo "FAIL: $*" >&2; exit 1; } +ok() { echo " ok: $*"; } +skip() { echo " SKIP: $*" >&2; } + +echo "conformance: a reset reconciles the host and touches nothing else" + +mode="$(curl -fsS --max-time 20 "$ENDPOINT/_feint/health" 2>/dev/null \ + | python3 -I -c 'import sys,json; print(json.load(sys.stdin).get("machines",""))' 2>/dev/null || true)" +if [ -z "$mode" ] || [ "$mode" = "none" ] || [ "$mode" = "off" ]; then + skip "no machine runtime (start with FEINT_VM=incus); the host half cannot be measured" + exit 0 +fi +command -v incus >/dev/null 2>&1 || { skip "incus is not on PATH, so what survives cannot be read"; exit 0; } +command -v scw >/dev/null 2>&1 || { skip "the scw CLI is absent, and this suite drives a real client"; exit 0; } + +# This suite ENDS with a machine alive on purpose: the point of its third +# assertion is that the reset did not take it down. So it sweeps after itself, +# through `feint clean`, which removes what the emulator created by the labels +# it wrote and nothing else. Armed before the first server is created, so a +# failure in the middle does not leak either — the first version of this suite +# left a container running on the maintainer's station, found by reading +# `incus list` after the pass rather than by any gate. +sweep() { "$FEINT" clean --vm "$mode" >/dev/null 2>&1 || true; } +trap sweep EXIT + +ADDR="${ENDPOINT#http://}" +set -a +# shellcheck source=/dev/null +. "$SCRIPT_DIR/scaleway/fake-credentials.env" +set +a +export SCW_API_URL="$ENDPOINT" + +names() { incus list -f csv -c n 2>/dev/null | tr -d '\r'; } +ours() { names | grep '^feint-' || true; } +strangers(){ names | grep -v '^feint-' || true; } + +before_strangers="$(strangers)" +[ -z "$(ours)" ] || fail "feint-owned containers exist before this suite; nothing below could be attributed" + +echo "-- a machine the checkpoint will hold" +scw instance server create type=DEV1-S image=ubuntu_jammy name=reset-kept zone=fr-par-1 -o json >/dev/null +kept="$(ours | head -1)" +[ -n "$kept" ] || fail "no container was created for the first server, so this suite would measure nothing" +ok "the checkpoint's machine exists: $kept" + +"$FEINT" checkpoint reset-host --addr "$ADDR" >/dev/null || fail "the checkpoint was refused" + +echo "-- a machine it will not" +scw instance server create type=DEV1-S image=ubuntu_jammy name=reset-later zone=fr-par-1 -o json >/dev/null +after_second="$(ours | wc -l)" +[ "$after_second" -eq 2 ] || fail "expected two feint-owned containers, found $after_second" +extra="$(ours | grep -v "^$kept$" | head -1)" +ok "the second machine exists: $extra" + +echo "-- feint reset reset-host" +"$FEINT" reset reset-host --addr "$ADDR" || fail "the reset reported that the lab did not converge" +ok "the reset reports convergence" + +left="$(ours)" +printf '%s\n' "$left" | grep -qx "$extra" && fail "the machine the checkpoint does not hold outlived the reset: $extra" +ok "the machine the checkpoint does not hold is gone" + +# The half a sweep would get wrong in the expensive direction. +printf '%s\n' "$left" | grep -qx "$kept" || fail "the reset destroyed the machine the checkpoint HOLDS: $kept" +ok "the machine the checkpoint holds is still there" + +missing="" +while IFS= read -r stranger; do + [ -n "$stranger" ] || continue + printf '%s\n' "$(strangers)" | grep -qx "$stranger" || missing="$missing $stranger" +done </dev/null || fail "the second checkpoint was refused" +held="$(ours | head -1)" +[ -n "$held" ] || fail "no machine to lose, so this half would measure nothing" +incus delete --force "$held" >/dev/null 2>&1 || fail "could not take the machine away out of band" +[ -z "$(ours)" ] || fail "the machine is still there, so the reset below would have nothing to do" +ok "the machine is gone from the host while the record remains" + +"$FEINT" reset reset-back --addr "$ADDR" || fail "the reset reported that the lab did not converge" +printf '%s\n' "$(ours)" | grep -qx "$held" \ + || fail "the reset did not start the machine again: [$(ours | tr '\n' ' ')]" +ok "the machine the checkpoint holds is back, under the same name" + +# The name is the assertion, not the count: a reset that started SOME +# container would satisfy a count while leaving every record naming one that +# does not exist, and the next reset would report it missing again, forever. + +echo "conformance: the host half of a reset measured under $mode" diff --git a/tools/falsify/specs/a-reset-is-not-a-restore.json b/tools/falsify/specs/a-reset-is-not-a-restore.json new file mode 100644 index 00000000..69347c3b --- /dev/null +++ b/tools/falsify/specs/a-reset-is-not-a-restore.json @@ -0,0 +1,63 @@ +{ + "package": "./internal/core/emulator/", + "note": "The subject is #853's own line: \"reset is not successful because JSON was restored\". `feint snapshot load` and `PUT /_feint/state` restore the control plane verbatim and touch nothing else, so a resource that disappears leaves its container running while the API no longer mentions it. The issue's last closing criterion asks for exactly this spec: \"a falsification that restores only the store but not runtime makes the test fail\".\n\nThe mutations are therefore all of the same family: each one turns the reset back into a restore, by a different route. The route matters, because each is a shape that would look reasonable in review — skipping the reconciliation loop, computing the difference by count instead of identity, handing a pack the resources that SURVIVED, or taking the \"before\" picture after the restore, when it is already gone.\n\nThe last one is the one worth reading twice. Reading the store after the restore instead of before is a one-line reordering that compiles, runs, reports a clean reset, and reconciles nothing at all — because by then there is no difference left to see.", + "mutations": [ + { + "label": "the destroying half stops being called, so a reset restores the control plane and leaves every machine the checkpoint does not hold running. This is the gap #853 names, reproduced exactly: the API no longer mentions the container and nobody takes it down", + "file": "internal/core/emulator/checkpoint.go", + "find": "\t\tif reconciler, ok := pack.(Reconciler); ok && len(mine) != 0 {", + "replace": "\t\tif reconciler, ok := pack.(Reconciler); ok && len(mine) != 0 && false {", + "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" + }, + { + "label": "the before picture is taken AFTER the restore instead of before, so there is no difference left to compute and the reset reconciles nothing while reporting a clean pass. A one-line reordering that compiles, runs and lies", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tbefore := s.env.Store.All()\n\tif err := s.env.Store.Restore(bytes.NewReader(state)); err != nil {", + "replace": "\tbefore := []*resource.Resource(nil)\n\tif err := s.env.Store.Restore(bytes.NewReader(state)); err != nil {", + "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" + }, + { + "label": "each pack is handed ANOTHER pack's resources, so a reset destroys exactly the machines the checkpoint expects to keep and leaves the ones it does not. The opposite error of the first mutation and the expensive one: it takes down what the operator asked to come back", + "file": "internal/core/emulator/checkpoint.go", + "find": "\t\tif res.Tenant.Provider == provider {\n\t\t\tout = append(out, res)\n\t\t}", + "replace": "\t\tif res.Tenant.Provider != provider {\n\t\t\tout = append(out, res)\n\t\t}", + "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" + }, + { + "label": "the difference is computed by comparing identifiers only, dropping the kind and the provider, so two resources that share an id across products are taken for one another. Written as a narrowed key rather than a count because the count version does not compile, and the narrowed one is what a reviewer would wave through", + "file": "internal/core/emulator/checkpoint.go", + "find": "\treturn res.Tenant.Provider + \"\\x00\" + res.Kind + \"\\x00\" + res.ID", + "replace": "\treturn res.ID", + "test": "TestAResetDestroysWhatTheCheckpointDoesNotHold" + }, + { + "label": "the byte half of a checkpoint is skipped, so the store is named and the bytes are not. A later reset restores the control plane, finds no byte checkpoint, and an operator reads a clean reset over a store whose objects are whatever the last test left", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tif s.env.Objects != nil {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {", + "replace": "\tif s.env.Objects != nil && false {\n\t\tif err := s.env.Objects.SaveCheckpoint(r.Context(), name); err != nil {", + "test": "TestACheckpointTakesBothHalvesOrNeither" + }, + { + "label": "a checkpoint name stops being validated, so a name that is a path component before it is a name reaches the byte store, which writes a directory per checkpoint. This repository's order is a refusal at intake rather than escaping later, and `..` is the whole reason", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tif !validCheckpointName(name) {", + "replace": "\tif !validCheckpointName(name) && false {", + "test": "TestACheckpointTakesBothHalvesOrNeither" + }, + { + "label": "a host nobody can survey is taken for an empty one, so a reset offers every record for recreation and starts a second container beside each one already running. The defect is trusting the store about a question only the runtime can answer: the store is exactly what just changed", + "file": "internal/core/emulator/checkpoint.go", + "find": "\tleft, asked, err := env.machines.Survey(ctx)\n\tif !asked || err != nil {\n\t\treturn nil, false\n\t}", + "replace": "\tleft, asked, err := env.machines.Survey(ctx)\n\tif (!asked || err != nil) && false {\n\t\treturn nil, false\n\t}", + "test": "TestAResetAsksNothingOfAHostNobodyCanSurvey" + }, + { + "label": "the pack is handed the records and asked to start them without being told what the host holds, so every record looks missing. This is the version of the code that shipped for an hour: the core picked the missing records itself with a \"feint-\" prefix, which put a provider's vocabulary in the neutral layer and matched a test's own bookkeeping stamp, which it then tried to boot", + "file": "internal/core/machine/reconcile.go", + "find": "\t\tname := res.Runtime[b.RuntimeKey]\n\t\tif name == \"\" || slices.Contains(held, name) {\n\t\t\tcontinue\n\t\t}", + "replace": "\t\tname := res.Runtime[b.RuntimeKey]\n\t\tif name == \"\" || (slices.Contains(held, name) && false) {\n\t\t\tcontinue\n\t\t}", + "test": "TestReconcileStartsOnlyTheMachinesThatAreMissing", + "package": "./internal/core/machine/" + } + ] +} diff --git a/tools/falsify/specs/a-run-owns-its-lab.json b/tools/falsify/specs/a-run-owns-its-lab.json index f8081dfe..eff81685 100644 --- a/tools/falsify/specs/a-run-owns-its-lab.json +++ b/tools/falsify/specs/a-run-owns-its-lab.json @@ -85,6 +85,20 @@ "find": "\tif mode != \"off\" {\n\t\targs = append(args, \"--cleanup\")\n\t}", "replace": "\tif mode != \"off\" && false {\n\t\targs = append(args, \"--cleanup\")\n\t}", "test": "TestAnOwnedLabSweepsItsRuntimeObjects" + }, + { + "label": "--reset-after and --keep-on-failure are accepted together, so one of them silently loses: a failed run is either erased or kept, and whichever order the code happens to apply, half the users get the opposite of what they asked for and nothing says so", + "file": "internal/cli/run.go", + "find": "\tif *resetAfter && *keep {", + "replace": "\tif *resetAfter && *keep && false {", + "test": "TestResetAfterAndKeepOnFailureAreRefusedTogether" + }, + { + "label": "a reset that did not converge stops failing the command, so a shared emulator is left dirty by a run that reported success. The caller asked for the lab to be left as found; a quiet 0 is how the next test inherits the mess and blames itself", + "file": "internal/cli/run.go", + "find": "\tif resetVerdict == \"did not converge\" && code == exitOK {", + "replace": "\tif resetVerdict == \"did not converge\" && code == exitOK && false {", + "test": "TestARunWhoseResetDidNotConvergeFails" } ] } diff --git a/tools/testplan/rules.go b/tools/testplan/rules.go index 305a7542..7f6a1c7b 100644 --- a/tools/testplan/rules.go +++ b/tools/testplan/rules.go @@ -499,6 +499,20 @@ var rules = []rule{ Unproven: prepushIsTheWholeGate, }, + { + Path: "tools/conformance/reset-host.sh", + Why: "the host half of a reset, which only a real runtime can answer for", + Runs: []string{"FEINT_VM=incus-ovn mise run conformance:leg -- runtime"}, + Unproven: "the reset suite runs on the runtime leg, which refuses without a runtime, " + + "and measures one machine kept, one removed and one lost out of band and started " + + "again: a reset interrupted part-way, a machine whose image left the catalogue " + + "between checkpoint and reset, and more than two machines are not covered", + Cites: []claim{{ + About: "the reset suite runs on the runtime leg, which refuses without a runtime", + In: "tools/conformance/leg.sh", + Shows: []string{"tools/conformance/reset-host.sh"}, + }}, + }, { Path: "tools/conformance/run-sweep.sh", Why: "feint run's sweep, which only a real runtime can answer for",