diff --git a/cmd/configcheck/main.go b/cmd/configcheck/main.go index 45f4b68..2b59e88 100644 --- a/cmd/configcheck/main.go +++ b/cmd/configcheck/main.go @@ -3,9 +3,13 @@ // writes or cloud calls. It is intended to run in GitOps CI so a bad runtime // configuration is rejected before expensive image builds. // +// With --node-config it instead validates a hand-authored node config, the +// direct-Git alternative to control-plane-managed placement. +// // Usage: // // configcheck --input-dir [--require-remote-routing] +// configcheck --node-config // // Exit status is non-zero on any validation error, and on any promoted warning // when --require-remote-routing is set. @@ -16,20 +20,32 @@ import ( "fmt" "os" + "github.com/artemnikitin/firework/internal/config" "github.com/artemnikitin/firework/internal/enricher" + "github.com/artemnikitin/firework/internal/volume" ) func main() { inputDir := flag.String("input-dir", "", "path to the GitOps input directory to validate") + nodeConfig := flag.String("node-config", "", "path to a hand-authored node config to validate (direct-Git mode)") requireRemoteRouting := flag.Bool("require-remote-routing", false, "treat a routed service without a valid first port_forwards host port as a validation failure") flag.Parse() - if *inputDir == "" { - fmt.Fprintln(os.Stderr, "configcheck: --input-dir is required") + if (*inputDir == "") == (*nodeConfig == "") { + fmt.Fprintln(os.Stderr, "configcheck: exactly one of --input-dir or --node-config is required") os.Exit(2) } + if *nodeConfig != "" { + if err := runNodeConfig(*nodeConfig); err != nil { + fmt.Fprintln(os.Stderr, "configcheck: "+err.Error()) + os.Exit(1) + } + fmt.Println("configcheck: OK") + return + } + if err := run(*inputDir, *requireRemoteRouting); err != nil { fmt.Fprintln(os.Stderr, "configcheck: "+err.Error()) os.Exit(1) @@ -60,3 +76,37 @@ func run(inputDir string, requireRemoteRouting bool) error { fmt.Printf("validated %d node config(s)\n", len(result.NodeConfigs)) return nil } + +// runNodeConfig validates a hand-authored node config. Its warnings are +// advisory: nothing can verify a resize_generation without history, so the +// check reports the shape that is almost always a mistake rather than failing. +func runNodeConfig(path string) error { + data, err := os.ReadFile(path) + if err != nil { + return fmt.Errorf("reading node config: %w", err) + } + nc, err := config.ParseNodeConfig(data) + if err != nil { + return err + } + // Parsing only proves the YAML is well formed. A hand-authored node config + // is the direct-Git equivalent of an enriched one, so it gets the same + // semantic validation the control plane applies before rendering — + // otherwise this command reports OK for a config with no node name, no + // image or kernel, zero compute, or a negative volume size, which defeats + // the point of running it in CI. + if err := enricher.ValidateOutput(nc); err != nil { + return fmt.Errorf("validation failed:\n%v", err) + } + // ValidateOutput covers generic service fields. The volume contract is + // enforced separately, by the agent's own rules, so a config cannot pass + // here and then fail to start on the node. + if err := volume.ValidateNodeVolumes(nc); err != nil { + return fmt.Errorf("validation failed:\n%v", err) + } + for _, warning := range config.NodeConfigWarnings(nc) { + fmt.Fprintf(os.Stderr, "warning [volume_size_without_generation]: %s\n", warning) + } + fmt.Printf("validated node config %s with %d service(s)\n", nc.Node, len(nc.Services)) + return nil +} diff --git a/cmd/configcheck/main_test.go b/cmd/configcheck/main_test.go index 0432040..11ca806 100644 --- a/cmd/configcheck/main_test.go +++ b/cmd/configcheck/main_test.go @@ -1,9 +1,13 @@ package main import ( + "fmt" "os" "path/filepath" + "strings" "testing" + + "github.com/artemnikitin/firework/internal/config" ) func writeTenant(t *testing.T, root, tenant, body string) { @@ -79,3 +83,148 @@ metadata: t.Fatal("expected failure with --require-remote-routing") } } + +func TestRunNodeConfig_WarnsOnVolumeSizeWithoutGeneration(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "node.yaml") + if err := os.WriteFile(path, []byte(`node: node-1 +services: + - name: db + image: /images/db.ext4 + kernel: /images/vmlinux + vcpus: 1 + memory_mb: 512 + volumes: + - name: data + type: local + mount_path: /var/lib/db + size_bytes: 10737418240 + bound_node: node-1 +`), 0o644); err != nil { + t.Fatal(err) + } + if err := runNodeConfig(path); err != nil { + t.Fatalf("an absent generation is advisory, not a failure: %v", err) + } + + nc, err := config.ParseNodeConfig([]byte(`node: node-1 +services: + - name: db + volumes: + - name: data + type: local + mount_path: /var/lib/db + size_bytes: 10737418240 +`)) + if err != nil { + t.Fatal(err) + } + warnings := config.NodeConfigWarnings(nc) + if len(warnings) != 1 || !strings.Contains(warnings[0], "resize_generation") { + t.Fatalf("expected a resize_generation warning, got %#v", warnings) + } + + nc.Services[0].Volumes[0].ResizeGeneration = 1 + if got := config.NodeConfigWarnings(nc); len(got) != 0 { + t.Fatalf("a declared generation must not warn, got %#v", got) + } +} + +// --node-config must validate, not just parse: a plainly invalid node config +// reporting OK defeats the point of running this in CI. +func TestNodeConfigIsSemanticallyValidated(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "node.yaml") + if err := os.WriteFile(path, []byte(`node: "" +services: + - name: broken + vcpus: 0 + memory_mb: 0 + volumes: + - name: data + type: local + mount_path: /var/lib/db + size_bytes: -1 +`), 0o644); err != nil { + t.Fatal(err) + } + if err := runNodeConfig(path); err == nil { + t.Fatal("a node config with no name, no image/kernel, zero compute and a negative volume size must not validate") + } +} + +// ValidateOutput covers generic service fields, not the volume invariants the +// agent enforces. A local volume with no bound_node is unusable, and +// configcheck must say so rather than printing OK. +func TestLocalVolumeWithoutBoundNodeIsRejected(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "node.yaml") + if err := os.WriteFile(path, []byte(`node: node-1 +services: + - name: db + image: /images/db.ext4 + kernel: /images/vmlinux + vcpus: 1 + memory_mb: 512 + volumes: + - name: data + type: local + mount_path: /var/lib/db + size_bytes: 10737418240 + resize_generation: 1 +`), 0o644); err != nil { + t.Fatal(err) + } + if err := runNodeConfig(path); err == nil { + t.Fatal("a local volume with no bound_node is unusable and must not validate") + } +} + +// The volume contract is checked through the agent's own rules, so the two +// cannot drift into a config that passes CI and then fails to start. +func TestNodeConfigVolumeContractMatchesTheAgent(t *testing.T) { + valid := `node: node-1 +services: + - name: db + image: /images/db.ext4 + kernel: /images/vmlinux + vcpus: 1 + memory_mb: 512 + volumes: + - name: data + type: local + mount_path: %s + size_bytes: 10737418240 + bound_node: %s + resize_generation: 1 +` + tests := []struct { + name string + mountPath string + boundNode string + wantErr bool + }{ + {name: "valid", mountPath: "/var/lib/db", boundNode: "node-1"}, + {name: "reserved mount path", mountPath: "/proc/db", boundNode: "node-1", wantErr: true}, + {name: "relative mount path", mountPath: "var/lib/db", boundNode: "node-1", wantErr: true}, + // A bound_node naming another node is only probably wrong — the agent + // matches its stable node_id, which need not equal the config key — so + // it warns rather than failing. + {name: "bound elsewhere warns only", mountPath: "/var/lib/db", boundNode: "node-2"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "node.yaml") + if err := os.WriteFile(path, []byte(fmt.Sprintf(valid, test.mountPath, test.boundNode)), 0o644); err != nil { + t.Fatal(err) + } + err := runNodeConfig(path) + if test.wantErr && err == nil { + t.Fatal("expected the volume contract to reject this config") + } + if !test.wantErr && err != nil { + t.Fatalf("expected the config to validate, got %v", err) + } + }) + } +} diff --git a/cmd/fc-init/main.go b/cmd/fc-init/main.go index 8ee47eb..810e392 100644 --- a/cmd/fc-init/main.go +++ b/cmd/fc-init/main.go @@ -380,7 +380,7 @@ func ensureWritablePaths(paths, volumePaths []string, uid, gid int) error { if overlapsVolumePath(path, volumePaths) { continue } - if err := chownPathRecursive(path, uid, gid); err != nil { + if err := chownPathRecursive(path, volumePaths, uid, gid); err != nil { if os.IsNotExist(err) { continue } @@ -393,29 +393,50 @@ func ensureWritablePaths(paths, volumePaths []string, uid, gid int) error { return nil } +// overlapsVolumePath reports whether a declared writable path is at or below a +// volume root. A path *above* a volume root does not overlap: the volume is a +// separate filesystem mounted inside it, and skipping the parent would leave a +// non-root guest process unable to write to its own directory. func overlapsVolumePath(path string, volumePaths []string) bool { for _, volumePath := range volumePaths { - if path == volumePath || strings.HasPrefix(path, volumePath+"/") || strings.HasPrefix(volumePath, path+"/") { + if path == volumePath || strings.HasPrefix(path, volumePath+"/") { return true } } return false } -func chownPathRecursive(path string, uid, gid int) error { +// chownFn is a seam so the walk can be observed in tests without running as +// root. Production always uses os.Lchown. +var chownFn = os.Lchown + +// chownPathRecursive walks a writable path, pruning any volume mount point it +// reaches. A mounted volume carries its own ownership on its own ext4 +// filesystem; descending into it is both wrong and potentially expensive. +func chownPathRecursive(path string, volumePaths []string, uid, gid int) error { info, err := os.Lstat(path) if err != nil { return err } if !info.IsDir() || info.Mode()&os.ModeSymlink != 0 { - return os.Lchown(path, uid, gid) + return chownFn(path, uid, gid) } + pruned := make(map[string]struct{}, len(volumePaths)) + for _, volumePath := range volumePaths { + pruned[volumePath] = struct{}{} + } return filepath.WalkDir(path, func(p string, d fs.DirEntry, walkErr error) error { if walkErr != nil { return walkErr } - return os.Lchown(p, uid, gid) + if _, isVolume := pruned[p]; isVolume { + if d != nil && d.IsDir() { + return fs.SkipDir + } + return nil + } + return chownFn(p, uid, gid) }) } diff --git a/cmd/fc-init/main_test.go b/cmd/fc-init/main_test.go index 2f87bf8..f79c40a 100644 --- a/cmd/fc-init/main_test.go +++ b/cmd/fc-init/main_test.go @@ -5,6 +5,8 @@ package main import ( "encoding/base64" "encoding/json" + "os" + "path/filepath" "reflect" "testing" ) @@ -111,3 +113,77 @@ func TestParseFireworkEnvArg_IgnoresNonEnvArg(t *testing.T) { t.Fatal("expected non-env arg to be ignored") } } + +// A writable path that is a strict parent of a volume mount must still be +// chowned. Skipping it left a non-root guest process unable to write to its +// own directory whenever a volume was declared beneath it. +func TestEnsureWritablePathsChownsParentOfVolumeMount(t *testing.T) { + root := t.TempDir() + appDir := filepath.Join(root, "var", "lib", "app") + volumeDir := filepath.Join(appDir, "data") + inVolume := filepath.Join(volumeDir, "payload") + sibling := filepath.Join(appDir, "cache") + unrelated := filepath.Join(root, "srv") + for _, dir := range []string{volumeDir, sibling, unrelated} { + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(inVolume, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + + // The chown itself is stubbed so the walk can be observed without needing + // a second uid to chown to. + var walked []string + original := chownFn + chownFn = func(path string, uid, gid int) error { + walked = append(walked, path) + return nil + } + t.Cleanup(func() { chownFn = original }) + + if err := ensureWritablePaths([]string{appDir, volumeDir, unrelated}, []string{volumeDir}, os.Getuid(), os.Getgid()); err != nil { + t.Fatalf("ensureWritablePaths failed: %v", err) + } + + contains := func(want string) bool { + for _, got := range walked { + if got == want { + return true + } + } + return false + } + if !contains(appDir) { + t.Fatalf("expected the parent of a volume mount to be chowned, walked %v", walked) + } + if !contains(sibling) { + t.Fatalf("expected a sibling of a volume mount to be chowned, walked %v", walked) + } + if !contains(unrelated) { + t.Fatalf("expected an unrelated writable path to be chowned, walked %v", walked) + } + if contains(volumeDir) || contains(inVolume) { + t.Fatalf("expected the volume subtree to be pruned, walked %v", walked) + } +} + +func TestOverlapsVolumePathMatchesOnlyAtOrBelowAVolumeRoot(t *testing.T) { + volumes := []string{"/var/lib/app/data"} + tests := []struct { + path string + want bool + }{ + {"/var/lib/app/data", true}, + {"/var/lib/app/data/sub", true}, + {"/var/lib/app", false}, + {"/var/lib/app/cache", false}, + {"/srv", false}, + } + for _, test := range tests { + if got := overlapsVolumePath(test.path, volumes); got != test.want { + t.Fatalf("overlapsVolumePath(%q) = %v, want %v", test.path, got, test.want) + } + } +} diff --git a/cmd/fireworkctl/main.go b/cmd/fireworkctl/main.go index 1a2d2e3..f68bd43 100644 --- a/cmd/fireworkctl/main.go +++ b/cmd/fireworkctl/main.go @@ -335,12 +335,26 @@ func runService(cfg cliConfig, args []string, out io.Writer) error { } } if len(response.Volumes) > 0 { - fmt.Fprintln(w, "\nVOLUME\tTYPE\tMOUNT PATH\tBOUND NODE\tBACKEND\tDESIRED BYTES\tAPPLIED BYTES\tGENERATION\tSTATE\tLAST ERROR") + // REQUESTED is what the GitOps repo asked for, EFFECTIVE what the + // control plane accepted and rendered, APPLIED what exists on + // disk. They differ only when a request was refused — which is + // exactly the case where an operator edits size:, sees nothing + // change, and needs to be told why rather than left to diff two + // revisions. + fmt.Fprintln(w, "\nVOLUME\tTYPE\tMOUNT PATH\tBOUND NODE\tBACKEND\tREQUESTED BYTES\tEFFECTIVE BYTES\tAPPLIED BYTES\tGENERATION\tSTATE\tLAST ERROR") for _, volume := range response.Volumes { - fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%d\t%d\t%d\t%s\t%s\n", + requested := volume.RequestedSizeBytes + if requested == 0 { + requested = volume.DesiredSizeBytes + } + state := volume.State + if volume.Rejected { + state = fmt.Sprintf("%s (rejected: %s)", state, valueOrDash(volume.RejectedReason)) + } + fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%d\t%d\t%d\t%d\t%s\t%s\n", volume.LogicalID, volume.Type, volume.MountPath, valueOrDash(volume.BoundNode), - valueOrDash(volume.SharedBackendID), volume.DesiredSizeBytes, volume.AppliedSizeBytes, - volume.ResizeGeneration, volume.State, valueOrDash(volume.LastError)) + valueOrDash(volume.SharedBackendID), requested, volume.DesiredSizeBytes, volume.AppliedSizeBytes, + volume.ResizeGeneration, state, valueOrDash(volume.LastError)) } } return w.Flush() diff --git a/docs/configs/README.md b/docs/configs/README.md index 6bb9b35..b6af83a 100644 --- a/docs/configs/README.md +++ b/docs/configs/README.md @@ -309,7 +309,12 @@ Notes: acknowledge a stable rendered revision. Direct-Git configs may omit them. - `size_bytes`, `bound_node`, `shared_backend_id`, and `resize_generation` are resolved/system-owned volume fields. Direct-Git local configs must contain a - `bound_node` matching the agent's stable `node_id`. + `bound_node` matching the agent's stable `node_id`, and must **bump + `resize_generation` whenever `size_bytes` changes** — the control plane mints + one automatically, but a hand-authored file carries its own, and a size change + at an unchanged generation is not recognized as a resize. Run + `configcheck --node-config ` to catch a declared size with no + generation. See [persistent volumes](../persistent-volumes.md). - `metadata.subdomain` is the portable, deployment-neutral form: it is exactly one DNS label and the agent forms the final hostname as `.`. It requires the agent's `ingress_domain` to be set and `traefik_config_dir` to be enabled; diff --git a/docs/deployment-visibility.md b/docs/deployment-visibility.md index 6700db7..a51a4b5 100644 --- a/docs/deployment-visibility.md +++ b/docs/deployment-visibility.md @@ -32,7 +32,7 @@ filters are `state` for nodes and `state`, `health`, and `node` for services. Node capacity is requested capacity, not measured utilization. CPU and memory `allocated` values are the sum of desired services assigned to the node. Storage allocation is the durable persistent-volume reservation used by the -scheduler, including retained volumes and the larger of desired/applied size +scheduler, including retained volumes and the larger of effective/applied size during a shrink. `available` is capacity minus allocated with a floor of zero; these values do not represent filesystem I/O or blocks written by guests. Service list/detail responses aggregate local and shared volume counts plus @@ -45,8 +45,10 @@ Missing data fails closed: - expired node leases become `stale`; - unplaced desired services are `pending`, carrying the scheduler's reason code (for example `insufficient_compute_capacity`, `volume_capacity_unavailable`, + `node_storage_exhausted`, `storage_capacity_unknown`, `volume_record_invalid`, or `host_port_conflict`, which names the contested port and the service - already holding it); + already holding it). See [fireworkctl](fireworkctl.md) for what each storage + reason means and how to resolve it; - placed services with missing, stale, or unsupported agent status are `unknown`; - VM state and health remain separate, so a service can be `running` and @@ -118,8 +120,17 @@ second state machine: - `progressing`: at least one relevant agent is applying the current revision; - `converged`: every relevant node is fresh and has applied it with no false or unknown blocking condition; -- `degraded`: convergence criteria are met, but at least one node reports the - non-blocking peer-route condition false; +- `degraded`: convergence criteria are met, but at least one node reports a + non-blocking condition false — the peer-route condition + (`peer_routes_degraded`), or `VolumeSizesApplied` + (`volume_size_rejected`), meaning a volume is running at an effective size + because the requested one was refused. The revision itself also reports + `volume_size_rejected` while any retained record carries a standing refusal: + once the refusal is acknowledged the rendered config carries the effective + size, so the record is the only thing that still knows the operator's request + stands. A service whose retained volume record could not be read is likewise + degraded rather than converged: it keeps running its last applied + configuration, but the desired revision was not applied to it; - `failed`: scheduling left a service pending, or a relevant agent reports a blocking failure for the current revision; - `unknown`: required node status is missing, unsupported, truncated, stale, @@ -194,7 +205,9 @@ memory, and local-volume storage are shown with allocated/available values and capacity bars. Shared storage is a backend-level resource, so it is not duplicated on every node. Service lists show a prominent disk summary, and details show local/shared reservation and applied size plus the per-volume -table. Service details also include a clickable HTTPS public URL when routing +table, which carries requested, effective, and applied sizes so a refused +request is visible rather than left to a revision diff. Service details also +include a clickable HTTPS public URL when routing metadata resolves through the API role's `ingress_domain` (or uses an exact `metadata.host`). All views refresh automatically. diff --git a/docs/fireworkctl.md b/docs/fireworkctl.md index 3f683e2..c067351 100644 --- a/docs/fireworkctl.md +++ b/docs/fireworkctl.md @@ -89,10 +89,47 @@ fireworkctl services --help `fireworkctl service SERVICE_NAME` also prints a persistent-volume table when the service declares volumes. It shows the logical ID, type, guest mount path, -local node binding or shared backend, desired/applied bytes, resize generation, -and preparation state. `local_volume_node_unavailable` means retained data is -still bound to a node that is not currently schedulable; Firework does not -replace it with an empty volume elsewhere. +local node binding or shared backend, requested/effective/applied bytes, resize +generation, and preparation state. + +Requested is what the repo asked for, effective is what the control plane +accepted and rendered, and applied is what exists on disk. They differ only +when a size request was refused, and in that case the state column names the +refusal reason. See [persistent volumes](persistent-volumes.md). + +Storage-related pending and refusal reasons: + +- `local_volume_node_unavailable`: retained data is still bound to a node that + is not currently schedulable. Firework does not replace it with an empty + volume elsewhere; +- `volume_capacity_unavailable`: the volume cannot bind to any candidate node + at all — no local pool there, or its binding names somewhere else. A + placement problem; +- `node_storage_exhausted`: the volume could bind, but no node's pool has room + for the new reservation. A capacity problem, resolved by freeing retained + volumes or growing the pool; +- `storage_capacity_unknown`: remaining capacity cannot be verified, because a + retained record could not be fully read or its bound node is not active. + New volume-bearing placement waits rather than being allocated against + capacity that may already be occupied; +- `volume_record_invalid`: the service's own retained record could not be + parsed. An already-running service keeps running at its last applied + configuration; one that was never placed stays pending until the record is + repaired; +- `shrink_below_minimum`: the requested shrink is smaller than the filesystem's + current contents allow. + +`configcheck --node-config ` validates a hand-authored node config with +the same semantic checks the control plane applies before rendering, plus the +volume contract the agent itself enforces — a local volume must declare +`bound_node`, mount paths must be absolute and outside the reserved set, and +shared volumes are not yet runnable. It warns about a volume size declared with +no `resize_generation`, and about a `bound_node` naming a different node than +the config is for. + +Host-port claims have their own reasons — `host_port_conflict` and +`duplicate_host_port_claims` — described in +[docs/configs](configs/README.md#host-port-claims). `unknown` is intentional: it means the control plane cannot safely confirm the current state. For example, a stale node or an agent that has not converged to diff --git a/docs/persistent-volumes.md b/docs/persistent-volumes.md index 8b1f4b6..ac2f250 100644 --- a/docs/persistent-volumes.md +++ b/docs/persistent-volumes.md @@ -66,6 +66,93 @@ identity, binding, capacity, filesystem health, and shrink feasibility. It then records a durable resize transaction, grows the file before ext4, or shrinks ext4 before truncating the file. Ambiguous states fail closed. +### The agent's service unit must not kill filesystem utilities + +Creating, checking, and resizing a volume runs `mkfs.ext4`, `e2fsck`, and +`resize2fs` as child processes. Interrupting one mid-operation is what the +resize transaction and `e2fsck -f -y` exist to recover from, and the agent +therefore detaches those commands from its own shutdown: they keep running when +the agent's context is cancelled, with their own timeout and a SIGTERM-then-wait +cancellation rather than an immediate kill. + +**That protection is defeated by the default systemd supervision.** With +`KillMode=control-group`, systemd's default, stopping or restarting the unit +signals *every* process in its control group, including the detached utility, +and escalates to SIGKILL for the whole group once `TimeoutStopSec` expires. A +node drain during a shrink then kills `resize2fs` exactly as if the agent had +never detached it. + +A unit running `firework-agent` must therefore set: + +```ini +[Service] +# SIGTERM reaches only the agent, so a detached utility keeps running. +KillMode=mixed +# Must exceed the agent's destructive-command deadline (30m) plus its grace +# period, or systemd force-kills the utility at the timeout. +TimeoutStopSec=2100 +``` + +The two numbers are a contract: `destructiveCommandTimeout` in +`internal/volume` bounds the command, and `TimeoutStopSec` must stay above it. +Raising one without the other reopens the gap. + +### Guest data is not flushed before a microVM is stopped + +Stopping a microVM — including every service update and every volume resize, +which are implemented as stop-then-start — terminates the Firecracker process +without a guest-side shutdown. The ext4 journal keeps the filesystem +consistent, but application data still in the guest page cache is lost. +Workloads with persistent volumes should be configured to fsync data they +cannot afford to lose. A guest quiesce channel is tracked in +[#49](https://github.com/artemnikitin/firework/issues/49). + +### Refused size requests + +A size request the cluster cannot serve is refused rather than applied, and the +service keeps running at the size it already has. Two cases produce this: + +- the bound node's pool has no room for the larger reservation, or its capacity + cannot be verified because the node is not currently active; +- the requested shrink is below the safe minimum for the filesystem's current + contents. + +A refusal is terminal for that request: it is recorded durably, no further +resize is attempted, and the service is neither stopped nor restarted. Status +therefore carries three sizes rather than two — **requested** (what the repo +asked for), **effective** (what the control plane accepted and rendered), and +**applied** (what exists on disk) — and the API, `fireworkctl service `, +and the UI show requested next to effective. When they differ, the volume is +also flagged with the reason it was refused. + +To recover, change the requested size to something feasible. That is a new +request, so it is measured and admitted from scratch. Reverting the request to +the effective size withdraws it: the refusal stops being reported and no resize +is performed. + +While a refusal stands for a volume the desired revision still declares, the +deployment status reports the revision `degraded` with reason +`volume_size_rejected` rather than converged — a cluster running a size nobody +asked for is not converged, even though every workload is healthy. Deleting the +service ends that: its record is retained, but nothing is asking for the size +any more. + +### Direct-Git node configs must bump `resize_generation` + +In control-plane-managed mode the controller mints a new `resize_generation` +whenever a volume's requested size changes, so a corrected request is always +distinguishable from a repeat of a refused one. + +Hand-authored node configs (see [docs/configs](configs/README.md)) carry +`resize_generation` themselves, and Firework cannot mint one for them. +**Bump `resize_generation` whenever you change a volume's `size_bytes`.** A +corrected size at an unchanged generation is still re-measured rather than +clamped, and reverting to the size already running withdraws the request at any +generation — but leaving the generation at `0` or omitting it means no resize +is ever recognized at all. `configcheck --node-config ` validates the +config and warns when it declares a volume size with an absent or zero +generation. + Deleting application YAML never deletes a volume. Permanent deletion is a manual operator action: first stop/remove the service placement, back up the volume if required, then remove both its host directory and @@ -73,5 +160,5 @@ volume if required, then remove both its host directory and automatic adoption. Service detail in the UI, API, and `fireworkctl service ` includes the -logical ID, binding/backend, desired and applied quota, resize generation, and -preparation state. +logical ID, binding/backend, requested/effective/applied quota, resize +generation, and preparation state. diff --git a/internal/agent/agent.go b/internal/agent/agent.go index fafedfc..f68b896 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -46,6 +46,7 @@ type Agent struct { cfg config.AgentConfig store store.Store vmManager *vm.Manager + volumeManager *volume.Manager reconciler *reconciler.Reconciler healthMon *healthcheck.Monitor networkMgr *network.Manager @@ -165,6 +166,7 @@ func New(cfg config.AgentConfig, s store.Store, logger *slog.Logger) *Agent { cfg: cfg, store: s, vmManager: vmMgr, + volumeManager: volumeMgr, reconciler: rec, healthMon: healthMon, networkMgr: networkMgr, @@ -345,6 +347,14 @@ func (a *Agent) tick(ctx context.Context) { a.setStatusCondition("ConfigFetched", statusmodel.ConditionTrue, "", "") a.setStatusCondition("ConfigParsed", statusmodel.ConditionTrue, "", "") + // Clamp any volume size this node has already refused, before anything + // else in the tick reads the merged configuration. Normalizing later — say, + // just before Plan — would leave the status snapshot describing the raw + // requested size while Plan, the instance, and the Firecracker config all + // use the effective one, and the two surfaces would disagree permanently. + a.vmManager.NormalizeVolumes(merged.Services) + a.reportVolumeSizeConvergence() + // Check revision only after fetch, so stores that update revision state // during Fetch (Git pull, object write token) are evaluated against fresh data. // For multi-label nodes we skip this optimization because revision is @@ -485,6 +495,22 @@ func (a *Agent) tick(ctx context.Context) { reconcileStart := time.Now() err := a.reconciler.Reconcile(ctx, *merged) a.metrics.observeReconcile(time.Since(reconcileStart), err != nil) + if err != nil && reconciler.IsIncomplete(err) { + // Every collected error was a benign start race: a stop or remove + // arrived while a start was preparing volumes, or two starts + // collided. Nothing failed, but nothing converged either, so the + // revision must not advance and the node must not be reported as + // having reconciled. Returning here leaves lastRevision unchanged, so + // the next tick re-plans from real state instead of taking the + // unchanged-revision shortcut. + a.logger.Info("reconciliation incomplete; retrying on the next tick", "error", err) + a.setStatusCondition("NetworkReady", statusmodel.ConditionTrue, "", "") + a.setStatusCondition("VMsReconciled", statusmodel.ConditionUnknown, "start_aborted", err.Error()) + a.incompleteAgentStatus("Reconciled", "reconcile_incomplete", err.Error()) + a.refreshRuntimeMetrics() + a.syncRegistryAfterTick(ctx, nodeCap, used) + return + } if err != nil { a.logger.Error("reconciliation failed", "error", err) networkFailed, vmFailed, code := classifyReconcileFailure(err) @@ -1004,6 +1030,10 @@ func (a *Agent) MetricsText() string { func (a *Agent) refreshRuntimeMetrics() { a.finishTickStatus() + // Pool gauges are published from retained state on every tick, not from + // the admission path, so a node holding retained-but-unplaced volumes + // keeps reporting them. + a.volumeManager.ObservePool() results := make(map[string]healthcheck.Result) if a.healthMon != nil { results = a.healthMon.Results() diff --git a/internal/agent/start_abort_test.go b/internal/agent/start_abort_test.go new file mode 100644 index 0000000..dda5ac4 --- /dev/null +++ b/internal/agent/start_abort_test.go @@ -0,0 +1,168 @@ +package agent + +import ( + "context" + "errors" + "fmt" + "log/slog" + "strings" + "testing" + + "github.com/artemnikitin/firework/internal/config" + "github.com/artemnikitin/firework/internal/reconciler" + "github.com/artemnikitin/firework/internal/statusmodel" + "github.com/artemnikitin/firework/internal/vm" + "github.com/artemnikitin/firework/internal/volume" +) + +// abortingVMManager fails every start the way a start aborted by a concurrent +// stop fails, and can be switched to a genuine failure or to success. +type abortingVMManager struct { + starts int + err error +} + +func (m *abortingVMManager) List() map[string]*vm.Instance { return map[string]*vm.Instance{} } +func (m *abortingVMManager) Start(context.Context, config.ServiceConfig) error { + m.starts++ + return m.err +} +func (m *abortingVMManager) Remove(string) error { return nil } + +func abortStore() *fakeStore { + return &fakeStore{ + data: map[string][]byte{"web": []byte(`node: web +services: + - name: app + image: /image + kernel: /kernel + vcpus: 1 + memory_mb: 128 +`)}, + revision: "rev-1", + } +} + +func agentWithVMManager(t *testing.T, s *fakeStore, manager reconciler.VMManager, strategy string) *Agent { + t.Helper() + cfg := testAgentConfig(t) + cfg.UpdateStrategy = strategy + a := New(cfg, s, testLogger()) + a.reconciler = reconciler.New(manager, slog.New(slog.NewTextHandler(discardWriter{}, nil)), nil, nil, strategy, 0). + WithStateDir(cfg.StateDir) + return a +} + +type discardWriter struct{} + +func (discardWriter) Write(p []byte) (int, error) { return len(p), nil } + +// An aborted start is a benign race, not a failure — but it is also not +// success. If the tick returned as though it had converged, lastRevision would +// advance and the next tick would take the unchanged-revision shortcut, leaving +// the service down until the revision itself changed. +func TestTick_AbortedStartDoesNotAdvanceRevisionAndReplans(t *testing.T) { + for _, strategy := range []string{"", "rolling"} { + name := strategy + if name == "" { + name = "all-at-once" + } + t.Run(name, func(t *testing.T) { + manager := &abortingVMManager{err: fmt.Errorf("service app: %w", vm.ErrStartAborted)} + a := agentWithVMManager(t, abortStore(), manager, strategy) + + a.tick(context.Background()) + if a.lastRevision != "" { + t.Fatalf("aborted start advanced lastRevision to %q", a.lastRevision) + } + status := a.agentStatusSnapshot() + if status.AppliedRevision != "" { + t.Fatalf("aborted start claimed applied revision %q", status.AppliedRevision) + } + if status.Phase == statusmodel.PhaseFailed { + t.Fatal("a benign start race must not publish the node as failed") + } + condition, ok := agentCondition(status, "Reconciled") + if !ok || condition.Status != statusmodel.ConditionUnknown || condition.ReasonCode != "reconcile_incomplete" { + t.Fatalf("expected an incomplete Reconciled condition, got %#v", condition) + } + + // The next tick must re-plan rather than short-circuit on the + // unchanged revision. + a.tick(context.Background()) + if manager.starts != 2 { + t.Fatalf("expected the next tick to retry the start, got %d starts", manager.starts) + } + + // Once the race clears, the tick converges normally. + manager.err = nil + a.tick(context.Background()) + if a.lastRevision != "rev-1" { + t.Fatalf("expected the revision to advance after a clean tick, got %q", a.lastRevision) + } + }) + } +} + +// A batch that mixes an abort with a genuine failure is a failure. Both leave +// the revision unchanged; the difference is what the node reports. +func TestTick_GenuineStartFailureIsStillReportedAsFailed(t *testing.T) { + manager := &abortingVMManager{err: errors.New("firecracker exited immediately")} + a := agentWithVMManager(t, abortStore(), manager, "") + + a.tick(context.Background()) + + status := a.agentStatusSnapshot() + if status.Phase != statusmodel.PhaseFailed { + t.Fatalf("expected a genuine start failure to publish a failed node, got %v", status.Phase) + } + condition, ok := agentCondition(status, "Reconciled") + if !ok || condition.Status != statusmodel.ConditionFalse { + t.Fatalf("expected a false Reconciled condition, got %#v", condition) + } + if a.lastRevision != "" { + t.Fatalf("a failed tick must not advance lastRevision, got %q", a.lastRevision) + } +} + +// A standing rejection must not read as ordinary convergence. The tick runs +// clean to the end — nothing failed — so without a distinct signal the node +// reports ready while running a size nobody asked for. +func TestTick_StandingVolumeRejectionDegradesRatherThanConverges(t *testing.T) { + a := agentWithVMManager(t, abortStore(), &abortingVMManager{}, "") + + // No rejection: the condition is true and the node converges. + a.tick(context.Background()) + clean := a.agentStatusSnapshot() + if condition, ok := agentCondition(clean, "VolumeSizesApplied"); !ok || condition.Status != statusmodel.ConditionTrue { + t.Fatalf("expected a satisfied volume-size condition, got %#v", condition) + } + if clean.Phase != statusmodel.PhaseReady { + t.Fatalf("expected a clean tick to be ready, got %v", clean.Phase) + } + + // A standing rejection degrades the node without failing it. + a.vmManager.SeedVolumeRejectionsForTest(map[string]volume.Rejection{"app/data": { + LogicalID: "app/data", ResizeGeneration: 2, AppliedGeneration: 1, + RequestedSizeBytes: 2 << 20, AppliedSizeBytes: 16 << 20, MinimumSizeBytes: 4 << 20, + }}) + a.lastRevision = "" + a.tick(context.Background()) + + status := a.agentStatusSnapshot() + condition, ok := agentCondition(status, "VolumeSizesApplied") + if !ok || condition.Status != statusmodel.ConditionFalse || condition.ReasonCode != "volume_size_rejected" { + t.Fatalf("expected a degrading volume-size condition, got %#v", condition) + } + if !strings.Contains(condition.Message, "app/data") { + t.Fatalf("expected the refused volume to be named, got %q", condition.Message) + } + // The workload is healthy, so this must degrade rather than fail: failing + // the node over a wrong quota would be the worse outcome. + if statusmodel.IsBlockingCondition("VolumeSizesApplied") { + t.Fatal("a standing rejection must not be a blocking failure") + } + if status.Phase == statusmodel.PhaseFailed { + t.Fatal("a standing rejection must not publish the node as failed") + } +} diff --git a/internal/agent/status.go b/internal/agent/status.go index 74c33c1..85b8901 100644 --- a/internal/agent/status.go +++ b/internal/agent/status.go @@ -205,6 +205,39 @@ func (a *Agent) failAgentStatus(condition, code, message string) { a.metrics.setAgentStatusSnapshot(a.agentStatusSnapshot()) } +// reportVolumeSizeConvergence publishes whether this node is running every +// volume at the size the desired revision asked for. +// +// A standing rejection is deliberately not a failure: the service is running +// and healthy, and failing the node would be a worse outcome than the wrong +// quota. But the tick that follows one runs clean to the end — advancing the +// revision and marking it applied — so without this the node reports ordinary +// convergence while running a size nobody requested. The non-blocking +// condition degrades it instead. +func (a *Agent) reportVolumeSizeConvergence() { + rejections := a.vmManager.VolumeRejections() + if len(rejections) == 0 { + a.setStatusCondition("VolumeSizesApplied", statusmodel.ConditionTrue, "", "") + return + } + names := make([]string, 0, len(rejections)) + for logicalID := range rejections { + names = append(names, logicalID) + } + sort.Strings(names) + a.setStatusCondition("VolumeSizesApplied", statusmodel.ConditionFalse, "volume_size_rejected", + fmt.Sprintf("running at an effective size for: %s", strings.Join(names, ", "))) +} + +// incompleteAgentStatus records a stage that neither succeeded nor failed. The +// node stays in the reconciling phase and retries, rather than being published +// as failed for what is a benign race. +func (a *Agent) incompleteAgentStatus(condition, code, message string) { + a.setStatusCondition(condition, statusmodel.ConditionUnknown, code, message) + a.refreshAgentStatus(statusmodel.PhaseReconciling, code, message) + a.metrics.setAgentStatusSnapshot(a.agentStatusSnapshot()) +} + func (a *Agent) markAgentStatusApplied(revision string) { a.statusMu.Lock() if observed := a.currentStatus.ObservedRevision; observed != "" { @@ -232,6 +265,13 @@ func (a *Agent) refreshAgentStatus(phase statusmodel.Phase, code, message string previous[service.Name] = service } + // The refusal snapshot is read from the volume manager directly rather + // than inferred from a running instance's prepared volumes. A preflight + // rejection produces no fresh preparation to read — it refuses the update + // before anything is stopped, so the instance still describes the previous + // one — and that coupling is what used to break the preflight path. + rejections := a.vmManager.VolumeRejections() + services := make([]statusmodel.ServiceStatus, 0, len(a.statusServices)) ready := 0 for _, desired := range a.statusServices { @@ -261,9 +301,9 @@ func (a *Agent) refreshAgentStatus(phase statusmodel.Phase, code, message string for _, prepared := range instance.Volumes { preparedByID[prepared.LogicalID] = prepared } - service.Volumes = BuildVolumeStatuses(desired, preparedByID) + service.Volumes = BuildVolumeStatusesWithRejections(desired, preparedByID, rejections) } else { - service.Volumes = BuildVolumeStatuses(desired, nil) + service.Volumes = BuildVolumeStatusesWithRejections(desired, nil, rejections) } if volumeError := a.vmManager.VolumeError(desired.Name); volumeError != "" { service.ReasonCode = "volume_failed" @@ -273,6 +313,11 @@ func (a *Agent) refreshAgentStatus(phase statusmodel.Phase, code, message string desiredGeneration[desired.Name+"/"+desiredVolume.Name] = desiredVolume.ResizeGeneration } for i := range service.Volumes { + // A refused size is not a failure, and a genuine failure on + // one volume must not relabel a rejection on another. + if service.Volumes[i].State == "rejected" { + continue + } service.Volumes[i].State = "error" service.Volumes[i].LastError = statusmodel.BoundedMessage(volumeError) service.Volumes[i].ResizeGeneration = desiredGeneration[service.Volumes[i].LogicalID] @@ -324,6 +369,19 @@ func (a *Agent) refreshAgentStatus(phase statusmodel.Phase, code, message string // real agent uses, rather than hand-building a statusmodel.VolumeStatus and // only coincidentally matching what this function actually sends. func BuildVolumeStatuses(service config.ServiceConfig, prepared map[string]volume.PreparedVolume) []statusmodel.VolumeStatus { + return BuildVolumeStatusesWithRejections(service, prepared, nil) +} + +// BuildVolumeStatusesWithRejections additionally reports a refused size +// request as its own state. +// +// A rejection cannot ride on the "prepared" branch: after a refusal Prepare +// succeeds at the old size, so the volume looks prepared, but the control +// plane's acknowledgement requires the observed applied size to equal the +// record's desired size — which is exactly what a rejection makes false. It +// therefore gets a third state alongside pending and prepared, carrying the +// *requested* generation so it matches the record it has to converge. +func BuildVolumeStatusesWithRejections(service config.ServiceConfig, prepared map[string]volume.PreparedVolume, rejections map[string]volume.Rejection) []statusmodel.VolumeStatus { statuses := make([]statusmodel.VolumeStatus, 0, len(service.Volumes)) for _, desired := range service.Volumes { // logicalID (untruncated) is the map key shared with volume.Manager's @@ -341,6 +399,24 @@ func BuildVolumeStatuses(service config.ServiceConfig, prepared map[string]volum status.ResizeGeneration = applied.ResizeGeneration status.State = "prepared" } + // Matched by logical ID alone. The desired config reaching status has + // already been normalized to the effective generation, so comparing + // generations here would never match — and the snapshot is the durable + // truth either way: it is rebuilt from the manifests, and a resize that + // actually applies clears it. + if rejection, ok := rejections[logicalID]; ok { + status.State = "rejected" + status.AppliedSizeBytes = rejection.AppliedSizeBytes + status.RequestedSizeBytes = rejection.RequestedSizeBytes + status.Rejected = true + status.RejectedReason = "shrink_below_minimum" + // The requested generation, not the manifest's applied one: the + // usual override would report a generation the record cannot match. + status.ResizeGeneration = rejection.ResizeGeneration + status.LastError = statusmodel.BoundedMessage(fmt.Sprintf( + "requested %d bytes is below the safe minimum %d for the current contents", + rejection.RequestedSizeBytes, rejection.MinimumSizeBytes)) + } statuses = append(statuses, status) } sort.Slice(statuses, func(i, j int) bool { return statuses[i].LogicalID < statuses[j].LogicalID }) diff --git a/internal/agent/status_test.go b/internal/agent/status_test.go index e5fd814..13aa5c6 100644 --- a/internal/agent/status_test.go +++ b/internal/agent/status_test.go @@ -122,3 +122,50 @@ func TestFailedVMNamesFrom(t *testing.T) { t.Fatalf("expected the failed VMs in sorted order, got %#v", got) } } + +// A refused size is reported as its own state carrying the requested +// generation. Reporting it as "prepared" would be dropped by the control plane +// twice over — the generation guard rejects it, and the prepared arm requires +// the applied size to equal the record's desired size. +func TestBuildVolumeStatusesReportsRejectionsWithTheRequestedGeneration(t *testing.T) { + // The desired config reaching status has already been normalized to the + // effective generation, which is what the running instance carries. + service := config.ServiceConfig{Name: "db", Volumes: []config.VolumeConfig{ + {Name: "data", Type: config.VolumeTypeLocal, MountPath: "/data", SizeBytes: 10 * config.GiB, ResizeGeneration: 1}, + {Name: "cache", Type: config.VolumeTypeLocal, MountPath: "/cache", SizeBytes: 4 * config.GiB, ResizeGeneration: 1}, + }} + prepared := map[string]volume.PreparedVolume{ + "db/data": {LogicalID: "db/data", SizeBytes: 10 * config.GiB, ResizeGeneration: 1}, + "db/cache": {LogicalID: "db/cache", SizeBytes: 4 * config.GiB, ResizeGeneration: 1}, + } + rejections := map[string]volume.Rejection{"db/data": { + LogicalID: "db/data", ResizeGeneration: 2, AppliedGeneration: 1, + RequestedSizeBytes: 2 * config.GiB, AppliedSizeBytes: 10 * config.GiB, + MinimumSizeBytes: 5 * config.GiB, + }} + + statuses := BuildVolumeStatusesWithRejections(service, prepared, rejections) + byID := make(map[string]statusmodel.VolumeStatus, len(statuses)) + for _, status := range statuses { + byID[status.LogicalID] = status + } + + rejected := byID["db/data"] + if rejected.State != "rejected" || !rejected.Rejected { + t.Fatalf("expected a rejected state, got %#v", rejected) + } + if rejected.ResizeGeneration != 2 { + t.Fatalf("expected the requested generation to be reported, got %d", rejected.ResizeGeneration) + } + if rejected.AppliedSizeBytes != 10*config.GiB || rejected.RequestedSizeBytes != 2*config.GiB { + t.Fatalf("unexpected sizes on the rejected volume: %#v", rejected) + } + if rejected.LastError == "" { + t.Fatal("expected the measured minimum to be reported") + } + + // A volume with no rejection is untouched. + if other := byID["db/cache"]; other.State != "prepared" || other.Rejected { + t.Fatalf("an unrejected volume was relabelled: %#v", other) + } +} diff --git a/internal/config/loader.go b/internal/config/loader.go index ec8c2f2..ab43c06 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -187,6 +187,36 @@ func ParseNodeConfig(data []byte) (NodeConfig, error) { return nc, nil } +// NodeConfigWarnings reports non-fatal problems in a hand-authored node config. +// +// It exists for direct-Git mode. A control-plane-managed cluster mints a new +// resize_generation whenever a volume's requested size changes; a hand-authored +// file carries the generation itself, and Firework cannot mint one for it. A +// declared size with an absent or zero generation is the shape such a file most +// often takes, and it means no resize is ever recognized — so it is worth +// saying out loud even though nothing can verify the generation without history. +func NodeConfigWarnings(nc NodeConfig) []string { + var warnings []string + for _, service := range nc.Services { + for _, volume := range service.Volumes { + if volume.SizeBytes > 0 && volume.ResizeGeneration <= 0 { + warnings = append(warnings, fmt.Sprintf( + "service %s volume %s declares a size with no resize_generation; bump resize_generation whenever you change size_bytes", + service.Name, volume.Name)) + } + // The agent matches bound_node against its stable node_id, which + // need not equal the config's node key — so a mismatch is only + // probably wrong, and warns rather than failing. + if volume.Type == VolumeTypeLocal && volume.BoundNode != "" && volume.BoundNode != nc.Node { + warnings = append(warnings, fmt.Sprintf( + "service %s volume %s is bound to %q but this config is for node %q; bound_node must match the agent's node_id", + service.Name, volume.Name, volume.BoundNode, nc.Node)) + } + } + } + return warnings +} + func resolveRegistryBootstrapToken(token, tokenFile string) (string, error) { t := strings.TrimSpace(os.ExpandEnv(token)) f := strings.TrimSpace(os.ExpandEnv(tokenFile)) diff --git a/internal/controlplane/admission_safety_test.go b/internal/controlplane/admission_safety_test.go new file mode 100644 index 0000000..6644c62 --- /dev/null +++ b/internal/controlplane/admission_safety_test.go @@ -0,0 +1,455 @@ +package controlplane + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/artemnikitin/firework/internal/config" + "github.com/artemnikitin/firework/internal/scheduler" + "github.com/artemnikitin/firework/internal/statusmodel" +) + +// when the previous placement cannot be read, a held service has no +// recoverable node — it would be classified as never-placed, left pending, and +// dropped from the rendered node configs, which the agent turns into a delete. +// Publishing anything in that state evicts a healthy workload over a transient +// read failure. +func TestUnreadablePlacementHoldsPublicationWhenServicesAreHeld(t *testing.T) { + held := map[string]string{"running": scheduler.ReasonVolumeRecordInvalid} + + tests := []struct { + name string + placementFound bool + held map[string]string + wantHold bool + }{ + {name: "unrecoverable placement with a held service", held: held, wantHold: true}, + {name: "unrecoverable placement with nothing held", wantHold: false}, + {name: "recovered placement with a held service", placementFound: true, held: held, wantHold: false}, + {name: "recovered placement with nothing held", placementFound: true, wantHold: false}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if got := heldPlacementUnrecoverable(test.placementFound, test.held); got != test.wantHold { + t.Fatalf("heldPlacementUnrecoverable = %v, want %v", got, test.wantHold) + } + }) + } +} + +// An unreadable placement revision must actually +// surface as an error, or the guard above is never reached. +func TestCorruptPlacementRevisionIsReportedAsAnError(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + + if _, err := store.PutJSON(ctx, placementCurrentKey("cp/v1/"), RevisionPointer{Revision: "placement-1"}); err != nil { + t.Fatal(err) + } + if _, err := store.PutRaw(ctx, placementRevisionKey("cp/v1/", "placement-1"), []byte("{not json"), "application/json"); err != nil { + t.Fatal(err) + } + + placement, found, err := controller.readExistingPlacement(ctx) + if err == nil { + t.Fatalf("expected a corrupt placement revision to be an error, got placement %#v", placement) + } + if found { + t.Fatal("a failed read must not report the placement as found") + } + if placement != nil { + t.Fatalf("a failed read must not return a partial placement: %#v", placement) + } +} + +// a shrink does not raise the reservation, so it must not be +// subject to capacity admission. Otherwise a pool reconfigured smaller than +// its applied volumes refuses the very shrink that would restore it. +func TestShrinkRecoversAnOverCapacityPool(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + // 20 GiB applied, in a pool now configured for only 10 GiB. + putRecord(t, store, "db", "data", appliedRecord("db/data", 20*config.GiB)) + + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + services := []config.ServiceConfig{serviceWithVolume("db", 5*config.GiB)} + if _, err := controller.applyExistingVolumeRecords(ctx, services, set, poolNodes(10*config.GiB)); err != nil { + t.Fatal(err) + } + stored := set.Records["db/data"].Record + if stored.rejectionStands() { + t.Fatalf("a shrink cannot over-commit a pool and must be admitted, got %#v", stored) + } + if stored.DesiredSizeBytes != 5*config.GiB { + t.Fatalf("expected the shrink to be accepted, got desired %d", stored.DesiredSizeBytes) + } +} + +// an unrecognized resize_state is tolerated for forward +// compatibility, but the admission path still rewrites the record — so an +// older controller destroys state owned by a newer resize protocol. +func TestUnknownResizeStateIsNotOverwrittenByAdmission(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + record := appliedRecord("db/data", 10*config.GiB) + record.ResizeState = VolumeResizeState("quiescing") + record.UpdatedAt = time.Now().UTC() + putRecord(t, store, "db", "data", record) + + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + services := []config.ServiceConfig{serviceWithVolume("db", 20*config.GiB)} + if _, err := controller.applyExistingVolumeRecords(ctx, services, set, poolNodes(100*config.GiB)); err != nil { + t.Fatal(err) + } + stored := set.Records["db/data"].Record + if stored.ResizeState != VolumeResizeState("quiescing") { + t.Fatalf("an older controller overwrote a newer protocol's state: %q", stored.ResizeState) + } + if stored.ResizeGeneration != 1 { + t.Fatalf("an older controller advanced a newer protocol's generation to %d", stored.ResizeGeneration) + } + // The rendered configuration must follow the record, not the request. + if got := services[0].Volumes[0].SizeBytes; got != 10*config.GiB { + t.Fatalf("expected the record's size to render, got %d", got) + } +} + +// The agent stops reporting a refusal once the rendered config carries the +// effective size, because those bytes are ambiguous to it. The record is not +// ambiguous, so the revision status has to carry that half — otherwise a +// cluster running a size nobody asked for reports plain convergence. +func TestStandingRecordRefusalDegradesTheRevision(t *testing.T) { + refused := VolumeRecord{ + LogicalID: "db/data", Type: config.VolumeTypeLocal, BoundNode: "node-1", + DesiredSizeBytes: 10 * config.GiB, AppliedSizeBytes: 10 * config.GiB, + RequestedSizeBytes: 2 * config.GiB, ResizeGeneration: 2, + ResizeState: VolumeResizeRejected, RejectedReason: "shrink_below_minimum", + } + // The desired revision still declares the volume — a refusal only matters + // while something is asking for the size. + snapshot := visibilitySnapshot{ + desired: DesiredRevision{ + Revision: "rev-1", + Services: []config.ServiceConfig{serviceWithVolume("db", 2*config.GiB)}, + }, + placementCurrent: true, + placement: PlacementRevision{Revision: "placement-1"}, + volumeByID: map[string]VolumeRecord{"db/data": refused}, + } + + status := snapshot.revisionStatus() + if status.Phase != "degraded" { + t.Fatalf("a standing refusal must not read as convergence, got %q", status.Phase) + } + if status.ReasonCode != "volume_size_rejected" { + t.Fatalf("unexpected reason: %q", status.ReasonCode) + } + if !strings.Contains(status.Message, "db/data") { + t.Fatalf("expected the refused volume to be named, got %q", status.Message) + } + + // Once the refusal is cleared the revision converges again. + cleared := refused + cleared.clearRejection() + cleared.ResizeState = VolumeResizeApplied + snapshot.volumeByID = map[string]VolumeRecord{"db/data": cleared} + if got := snapshot.revisionStatus(); got.Phase == "degraded" { + t.Fatalf("a cleared refusal must stop degrading the revision: %#v", got) + } +} + +// A missing placement object is not a read error, so a guard keyed on the +// error alone never fires and a held running service is dropped. +func TestMissingPlacementObjectCountsAsUnrecoverable(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + + // The pointer exists and names a revision whose object is gone. + if _, err := store.PutJSON(ctx, placementCurrentKey("cp/v1/"), RevisionPointer{Revision: "placement-1"}); err != nil { + t.Fatal(err) + } + + placement, found, err := controller.readExistingPlacement(ctx) + if err != nil { + t.Fatalf("a missing object is not a read error, got %v", err) + } + if placement != nil { + t.Fatalf("expected no placement, got %#v", placement) + } + // The guard keys on found, not on err: a pointer naming a revision whose + // object is gone is not "nothing has been placed yet". + if found { + t.Fatal("a missing placement object must not report as found") + } + if !heldPlacementUnrecoverable(found, map[string]string{"running": "volume_record_invalid"}) { + t.Fatal("a missing placement object must count as unrecoverable for a held service") + } +} + +// An absent resize_state is an empty string, which is malformed rather than a +// state owned by a newer protocol. +func TestAbsentResizeStateIsQuarantinedAsMalformed(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + record := appliedRecord("db/data", 10*config.GiB) + record.ResizeState = "" + putRecord(t, store, "db", "data", record) + + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + if _, quarantined := set.Quarantined["db/data"]; !quarantined { + t.Fatalf("an absent resize_state must be quarantined as malformed, got record %#v", set.Records["db/data"].Record) + } +} + +// The withdrawn-request shape is not direct-Git-only. In controller-managed mode, reverting the +// GitOps size: to the effective size clears the *record's* rejection — but the +// controller still renders that same (effective size, refused generation) +// shape, so the agent-side refusal survives on the identical bytes. +func TestControllerModeAlsoRendersTheWithdrawnShape(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + putRecord(t, store, "db", "data", VolumeRecord{ + LogicalID: "db/data", Type: config.VolumeTypeLocal, BoundNode: "node-1", + DesiredSizeBytes: 10 * config.GiB, AppliedSizeBytes: 10 * config.GiB, + RequestedSizeBytes: 2 * config.GiB, ResizeGeneration: 2, + ResizeState: VolumeResizeRejected, RejectedReason: "shrink_below_minimum", + }) + + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + // The operator reverts size: to the effective size. + services := []config.ServiceConfig{serviceWithVolume("db", 10*config.GiB)} + if _, err := controller.applyExistingVolumeRecords(ctx, services, set, poolNodes(100*config.GiB)); err != nil { + t.Fatal(err) + } + if set.Records["db/data"].Record.rejectionStands() { + t.Fatal("reverting to the effective size must clear the record's rejection") + } + // And the rendered shape is exactly the one the agent must not keep + // refusing: the effective size at the refused generation. + got := services[0].Volumes[0] + if got.SizeBytes != 10*config.GiB || got.ResizeGeneration != 2 { + t.Fatalf("expected the effective size at the refused generation, got (%d, %d)", + got.SizeBytes, got.ResizeGeneration) + } +} + +// Retained records outlive their service by design. A refusal on a record +// whose service is no longer desired must not degrade every later revision. +func TestRefusalOnADeletedServiceDoesNotDegradeTheRevision(t *testing.T) { + refused := VolumeRecord{ + LogicalID: "gone/data", Type: config.VolumeTypeLocal, BoundNode: "node-1", + DesiredSizeBytes: 10 * config.GiB, AppliedSizeBytes: 10 * config.GiB, + RequestedSizeBytes: 2 * config.GiB, ResizeGeneration: 2, + ResizeState: VolumeResizeRejected, RejectedReason: "shrink_below_minimum", + } + // An empty desired revision: the service was deleted, its record retained. + snapshot := visibilitySnapshot{ + desired: DesiredRevision{Revision: "rev-2"}, + placementCurrent: true, + placement: PlacementRevision{Revision: "placement-2"}, + volumeByID: map[string]VolumeRecord{"gone/data": refused}, + } + if got := snapshot.revisionStatus(); got.Phase == "degraded" { + t.Fatalf("a refusal for a service no longer desired must not degrade the revision: %#v", got) + } +} + +// Withdrawing a request must leave the record self-consistent. Clearing the +// rejection fields while ResizeState stays "rejected" makes status report +// state: rejected with rejected: false, and nothing later repairs it. +func TestWithdrawnRefusalLeavesNoContradictoryState(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + putRecord(t, store, "db", "data", VolumeRecord{ + LogicalID: "db/data", Type: config.VolumeTypeLocal, BoundNode: "node-1", + DesiredSizeBytes: 10 * config.GiB, AppliedSizeBytes: 10 * config.GiB, + RequestedSizeBytes: 2 * config.GiB, ResizeGeneration: 2, + ResizeState: VolumeResizeRejected, RejectedReason: "shrink_below_minimum", + LastError: "below the safe minimum", + }) + + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + services := []config.ServiceConfig{serviceWithVolume("db", 10*config.GiB)} + if _, err := controller.applyExistingVolumeRecords(ctx, services, set, poolNodes(100*config.GiB)); err != nil { + t.Fatal(err) + } + got := set.Records["db/data"].Record + if got.rejectionStands() { + t.Fatalf("precondition: the rejection fields must clear, got %#v", got) + } + if got.ResizeState == VolumeResizeRejected { + t.Fatalf("state stayed rejected while the rejection was cleared: %#v", got) + } + if got.LastError != "" { + t.Fatalf("the refusal message outlived the refusal: %q", got.LastError) + } +} + +// A stale heartbeat at the same generation must not reopen a refusal the +// desired state has already withdrawn — that produces two durable writes per +// tick, and a crash between them leaves the degraded state behind. +func TestStaleRejectionDoesNotReopenAWithdrawnRefusal(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + recordKey := mustVolumeRecordKey("cp/v1/", "db", "data") + // The refusal was withdrawn: fields cleared, state settled back to applied. + putRecord(t, store, "db", "data", VolumeRecord{ + LogicalID: "db/data", Type: config.VolumeTypeLocal, BoundNode: "node-1", + DesiredSizeBytes: 10 * config.GiB, AppliedSizeBytes: 10 * config.GiB, + ResizeGeneration: 2, ResizeState: VolumeResizeApplied, + }) + + // An agent heartbeat still carrying the old refusal at the same generation. + nodeKey, err := nodeRecordKey("cp/v1/", "node-1") + if err != nil { + t.Fatal(err) + } + node := NodeRecord{NodeID: "node-1", AgentStatus: &statusmodel.AgentStatus{Services: []statusmodel.ServiceStatus{{ + Name: "db", Volumes: []statusmodel.VolumeStatus{{ + LogicalID: "db/data", Type: "local", BoundNode: "node-1", + AppliedSizeBytes: 10 * config.GiB, RequestedSizeBytes: 2 * config.GiB, + ResizeGeneration: 2, State: "rejected", Rejected: true, + RejectedReason: "shrink_below_minimum", + }}, + }}}} + if _, err := store.PutJSON(ctx, nodeKey, node); err != nil { + t.Fatal(err) + } + + if err := controller.acknowledgeVolumeRecords(ctx); err != nil { + t.Fatal(err) + } + var got VolumeRecord + if _, exists, err := store.GetJSON(ctx, recordKey, &got); err != nil || !exists { + t.Fatalf("read record: %v", err) + } + if got.rejectionStands() || got.ResizeState == VolumeResizeRejected { + t.Fatalf("a stale heartbeat reopened a withdrawn refusal: %#v", got) + } +} + +// Zero volumes is valid prior state, not a missing snapshot. Substituting the +// desired configuration there reintroduces exactly the unvalidated volume +// config the hold exists to gate. +func TestZeroVolumePriorSnapshotIsNotAMissingOne(t *testing.T) { + // The prior render legitimately had no volumes. + prior := config.ServiceConfig{Name: "svc", VCPUs: 1, MemoryMB: 512} + // The desired revision now adds one, which is what is being gated. + desired := []config.ServiceConfig{serviceWithVolume("svc", 90*config.GiB)} + admission := volumeAdmission{Held: map[string]string{"svc": scheduler.ReasonVolumeRecordInvalid}} + placement := map[string]renderedPlacement{"svc": {Node: "node-1", Service: prior}} + + _, held, _ := splitHeldServices(desired, admission, placement, map[string]struct{}{"node-1": {}}) + + rendered := held["node-1"] + if len(rendered) != 1 { + t.Fatalf("the held service must still be rendered: %#v", held) + } + if len(rendered[0].Volumes) != 0 { + t.Fatalf("the unvalidated desired volume config was rendered for a held service: %#v", rendered[0].Volumes) + } +} + +// A held service is re-rendered at its previous placement. If that node is no +// longer active, rendering it there produces a node config nothing reads while +// the service is, in reality, running nowhere. +func TestHeldServiceOnADepartedNodeIsPendingNotRendered(t *testing.T) { + prior := config.ServiceConfig{Name: "svc", VCPUs: 1, MemoryMB: 512, Volumes: []config.VolumeConfig{{ + Name: "data", Type: config.VolumeTypeLocal, MountPath: "/data", + SizeBytes: 10 * config.GiB, BoundNode: "node-gone", ResizeGeneration: 1, + }}} + desired := []config.ServiceConfig{serviceWithVolume("svc", 10*config.GiB)} + admission := volumeAdmission{Held: map[string]string{"svc": scheduler.ReasonVolumeRecordInvalid}} + placement := map[string]renderedPlacement{"svc": {Node: "node-gone", Service: prior}} + + // Only node-1 is active now; node-gone has left the fleet. + active := map[string]struct{}{"node-1": {}} + _, held, pending := splitHeldServices(desired, admission, placement, active) + + if len(held) != 0 { + t.Fatalf("a held service must not be rendered onto a departed node: %#v", held) + } + if len(pending) != 1 || pending[0].Service != "svc" { + t.Fatalf("a service running nowhere must be reported pending, got %#v", pending) + } +} + +// A tier-3 quarantine blocks new volume-bearing placement for a whole storage +// class. If that block survives the deletion of the service the record belongs +// to, one corrupt object permanently prevents every future volume placement in +// the cluster, with no service left to repair. +func TestQuarantineBlocksCapacityEvenForADeletedService(t *testing.T) { + set := volumeRecordSet{ + Records: map[string]storedVolumeRecord{}, + Quarantined: map[string]volumeQuarantine{ + // Unreadable object: no binding, no class -> blocks both classes. + "deleted-service/data": {Key: "k", LogicalID: "deleted-service/data", Tier: quarantineTierUnattributable}, + }, + } + reservations := storageReservations(set) + if !reservations.LocalClassUnknown { + t.Skip("precondition changed") + } + + // A brand-new, unrelated service wants a volume on a healthy node. + nodes := []scheduler.Node{{ + InstanceID: "node-1", CapacityVCPUs: 8, CapacityMemMB: 8192, + LocalCapacityBytes: 100 * config.GiB, + }} + fresh := []config.ServiceConfig{serviceWithVolume("fresh", 10*config.GiB)} + assignments, pending := scheduler.ScheduleWithStorage(fresh, nodes, nil, reservations, nil) + + // This is correct, not a defect: a retained volume's bytes are still on + // disk after its service is deleted, so an unreadable record still hides + // real capacity. It is the opposite of a *refusal*, which ends with the + // service because nobody is asking for the size any more. + if len(assignments["node-1"]) != 0 { + t.Fatalf("unverifiable capacity must not be handed out: %#v", assignments) + } + if len(pending) != 1 || pending[0].ReasonCode != scheduler.ReasonStorageCapacityUnknown { + t.Fatalf("expected an unknown-capacity block, got %#v", pending) + } +} + +// §4.4 requires every tier to name the offending key so the repair target is +// unambiguous. A cluster-wide block whose cause is only in the controller log +// leaves an operator grepping for an object they cannot name. +func TestCapacityBlockNamesItsRepairTarget(t *testing.T) { + set := volumeRecordSet{ + Records: map[string]storedVolumeRecord{}, + Quarantined: map[string]volumeQuarantine{ + "svc/data": {Key: "k", LogicalID: "svc/data", Tier: quarantineTierUnattributable}, + }, + } + reservations := storageReservations(set) + nodes := []scheduler.Node{{ + InstanceID: "node-1", CapacityVCPUs: 8, CapacityMemMB: 8192, + LocalCapacityBytes: 100 * config.GiB, + }} + _, pending := scheduler.ScheduleWithStorage( + []config.ServiceConfig{serviceWithVolume("fresh", 10*config.GiB)}, nodes, nil, reservations, nil) + + if len(pending) != 1 || pending[0].ReasonCode != scheduler.ReasonStorageCapacityUnknown { + t.Fatalf("expected an unknown-capacity block, got %#v", pending) + } + if !strings.Contains(pending[0].Message, "svc/data") { + t.Fatalf("the block must name the record to repair, got %q", pending[0].Message) + } +} diff --git a/internal/controlplane/controller.go b/internal/controlplane/controller.go index e8421e4..c678e46 100644 --- a/internal/controlplane/controller.go +++ b/internal/controlplane/controller.go @@ -166,6 +166,16 @@ func (c *Controller) runReconcile(ctx context.Context) { c.logger.Warn("desired revision pointer targets missing object", "revision", desiredPtr.Revision) return } + // Node discovery is hoisted above the record work because admission needs + // node capacity in scope: a size request is only feasible relative to the + // pool it would land in. discoverActiveNodes does not read volume records, + // so the move is safe, and schedulingInputSignature is computed after both + // either way. + activeNodes, hostIPByNode, err := c.discoverActiveNodes(ctx) + if err != nil { + c.logger.Error("discovering active nodes failed", "error", err) + return + } if err := c.acknowledgeVolumeRecords(ctx); err != nil { c.logger.Warn("acknowledging volume records failed", "error", err) } @@ -178,16 +188,12 @@ func (c *Controller) runReconcile(ctx context.Context) { for i := range services { services[i].Volumes = append([]config.VolumeConfig(nil), desired.Services[i].Volumes...) } - if err := c.applyExistingVolumeRecords(ctx, services, volumeRecords); err != nil { + admission, err := c.applyExistingVolumeRecords(ctx, services, volumeRecords, activeNodes) + if err != nil { c.logger.Error("reconciling volume records failed", "error", err) return } - activeNodes, hostIPByNode, err := c.discoverActiveNodes(ctx) - if err != nil { - c.logger.Error("discovering active nodes failed", "error", err) - return - } inputSig, err := schedulingInputSignature(desired.Revision, activeNodes, hostIPByNode, volumeRecordsDigest(volumeRecords)) if err != nil { c.logger.Error("failed to compute scheduling input signature; skipping signature cache optimization", "error", err) @@ -200,13 +206,45 @@ func (c *Controller) runReconcile(ctx context.Context) { return } - existingAssignment, err := c.readExistingAssignment(ctx) + existingPlacement, placementFound, err := c.readExistingPlacement(ctx) if err != nil { c.logger.Warn("reading existing placement failed; will re-place all", "error", err) - existingAssignment = nil + existingPlacement = nil + } + // A held service is one already running that must keep running, and the + // prior placement is the only source for where. Without it the service + // would be classified as never-placed and left pending, which drops it + // from the rendered node configs — and the agent turns an absent service + // into a delete. Publishing anything here would evict a healthy workload + // over a transient read failure, so nothing is published and the next tick + // retries. Omission is eviction; there is no partial answer to give. + if heldPlacementUnrecoverable(placementFound, admission.Held) { + c.logger.Error("holding services but the previous placement is unreadable; not publishing", + "held", len(admission.Held)) + return + } + existingAssignment := make(map[string]string, len(existingPlacement)) + for name, placed := range existingPlacement { + existingAssignment[name] = placed.Node } - assignments, pending := scheduler.ScheduleWithStorage(services, activeNodes, existingAssignment, storageReservations(volumeRecords)) + // A service whose own volume record cannot be used is held, not dropped. + // Omitting it from the rendered node configs is what the agent turns into + // a delete, so a malformed *record* would stop a healthy *workload*. + activeNodeIDs := make(map[string]struct{}, len(activeNodes)) + for _, node := range activeNodes { + activeNodeIDs[node.InstanceID] = struct{}{} + } + schedulable, held, heldPending := splitHeldServices(services, admission, existingPlacement, activeNodeIDs) + schedulingNodes := reserveHeldCapacity(activeNodes, held) + + assignments, pending := scheduler.ScheduleWithStorage( + schedulable, schedulingNodes, existingAssignment, storageReservations(volumeRecords), heldPortClaims(held)) + for node, services := range held { + assignments[node] = append(assignments[node], services...) + } + pending = append(pending, heldPending...) + sort.Slice(pending, func(i, j int) bool { return pending[i].Service < pending[j].Service }) nodeConfigs := scheduler.BuildNodeConfigs(assignments) nodeConfigs = appendRetiredNodeConfigs(nodeConfigs, activeNodes) @@ -235,6 +273,17 @@ func (c *Controller) runReconcile(ctx context.Context) { Service: item.Service, ReasonCode: item.ReasonCode, Message: item.Message, }) } + for _, services := range held { + for _, service := range services { + placementRev.HeldServices = append(placementRev.HeldServices, PendingPlacement{ + Service: service.Name, ReasonCode: admission.Held[service.Name], + Message: "running the last applied configuration; the desired one could not be resolved", + }) + } + } + sort.Slice(placementRev.HeldServices, func(i, j int) bool { + return placementRev.HeldServices[i].Service < placementRev.HeldServices[j].Service + }) if err := c.publishPlacement(ctx, placementRev); err != nil { c.logger.Error("publishing placement failed", "error", err) return @@ -302,26 +351,182 @@ func (c *Controller) discoverActiveNodes(ctx context.Context) ([]scheduler.Node, return nodes, hostIPByNode, nil } -func (c *Controller) readExistingAssignment(ctx context.Context) (map[string]string, error) { +// renderedPlacement is one service as the previous cycle actually rendered it: +// its node together with its complete configuration at the effective volume +// sizes the cluster accepted. +// +// The placement map alone (service to node) cannot supply that configuration, +// and the malformed record is by definition not a source either. Without this +// third source, "re-render its last effective configuration" is unimplementable +// and the obvious shortcut — re-rendering the desired revision's volume config +// — silently reintroduces the very size the record was supposed to gate. +type renderedPlacement struct { + Node string + Service config.ServiceConfig +} + +// readExistingPlacement returns the previous placement and whether it could be +// established at all. +// +// found is false both for a read error and for missing state, and the +// difference matters to exactly one caller. A pointer that names a revision +// whose object is gone is *not* the same as "nothing has been placed yet": +// services may well be running under it. Treating a missing object as an empty +// placement is what lets a held service be classified as never-placed and +// dropped from the rendered configs, which the agent turns into a delete. +// +// An absent pointer is genuinely a cluster that has never placed anything, but +// it is reported the same way because the only caller that consults found also +// requires a held service — which requires a retained volume record, which a +// cluster that has never placed anything does not have. +func (c *Controller) readExistingPlacement(ctx context.Context) (placement map[string]renderedPlacement, found bool, err error) { var ptr RevisionPointer _, exists, err := c.store.GetJSON(ctx, placementCurrentKey(c.cfg.State.Prefix), &ptr) if err != nil || !exists || ptr.Revision == "" { - return nil, err + return nil, false, err } var rev PlacementRevision _, exists, err = c.store.GetJSON(ctx, placementRevisionKey(c.cfg.State.Prefix, ptr.Revision), &rev) if err != nil || !exists { - return nil, err + return nil, false, err } - assignment := make(map[string]string) + placement = make(map[string]renderedPlacement) for _, nc := range rev.NodeConfigs { for _, svc := range nc.Services { - assignment[svc.Name] = nc.Node + placement[svc.Name] = renderedPlacement{Node: nc.Node, Service: svc} + } + } + return placement, true, nil +} + +// splitHeldServices separates the services that can be scheduled from their +// desired configuration from those whose volume records cannot be used. +// +// Blocking is scoped by whether the owner is already running: +// +// - already placed: hold the last placement and re-render it at its last +// known effective volume configuration. The service keeps running, and no +// resize, rebinding, or capacity change is applied while the record is +// unreadable. +// - not yet placed: withhold placement. There is nothing running to disturb, +// and admitting it would allocate against capacity that cannot be verified. +// +// When the prior rendered snapshot is unavailable — a first reconcile after a +// prefix change, or an unreadable placement revision — the service is *still* +// never omitted, because omission is eviction. It is re-rendered from the +// desired revision instead, and charged no reservation; the scope block from +// storageReservations is what keeps anything new from being admitted against +// capacity that cannot be proved. +func splitHeldServices(services []config.ServiceConfig, admission volumeAdmission, placement map[string]renderedPlacement, activeNodes map[string]struct{}) ([]config.ServiceConfig, map[string][]config.ServiceConfig, []scheduler.Pending) { + if len(admission.Held) == 0 { + return services, nil, nil + } + schedulable := make([]config.ServiceConfig, 0, len(services)) + held := make(map[string][]config.ServiceConfig) + var pending []scheduler.Pending + for _, service := range services { + reason, blocked := admission.Held[service.Name] + if !blocked { + schedulable = append(schedulable, service) + continue + } + placed, wasPlaced := placement[service.Name] + if wasPlaced { + // Holding a placement is only meaningful while that node is still + // there to honour it. Once it is gone the service is running + // nowhere, so re-rendering it onto the departed node produces a + // config no agent reads and reports the service as neither running + // nor pending. There is nothing left to disturb, which is exactly + // the condition under which the rule below withholds placement. + if _, active := activeNodes[placed.Node]; !active { + wasPlaced = false + } + } + if !wasPlaced { + pending = append(pending, scheduler.Pending{ + Service: service.Name, ReasonCode: reason, + Message: "volume record cannot be read; placement withheld until it is repaired", + }) + continue + } + // The prior render is used exactly as it was, including when it + // declared no volumes at all — that is valid prior state, not a + // missing snapshot, and substituting the desired configuration there + // renders precisely the unvalidated volume config the hold exists to + // gate. A genuinely missing snapshot cannot reach here: it is the + // unrecoverable case, and heldPlacementUnrecoverable stops the cycle + // before anything is published. + held[placed.Node] = append(held[placed.Node], placed.Service) + } + return schedulable, held, pending +} + +// heldPlacementUnrecoverable reports whether publishing this cycle could evict +// a running service. +// +// A held service is one already running that must keep running, and the prior +// placement is the only source for where it runs. When that read fails, a held +// service would be classified as never-placed and left pending, which drops it +// from the rendered node configs — and the agent turns an absent service into a +// delete. Omission is eviction, and there is no partial answer to give, so the +// cycle publishes nothing and the next tick retries. +// +// With nothing held, a failed placement read is harmless: the scheduler simply +// re-places everything from the desired revision, which is the pre-existing +// behavior. +func heldPlacementUnrecoverable(placementFound bool, held map[string]string) bool { + return !placementFound && len(held) > 0 +} + +// heldPortClaims collects the node-exclusive host-port claims a held service is +// still holding. +// +// A held service is re-rendered outside the scheduler, so the scheduler cannot +// see its claims and would otherwise place a new service claiming the same +// (tcp, host_port) on the same node. The agent rejects a node config with a +// duplicate claim outright, so that would take down every service on the node — +// a strictly worse outcome than the unreadable record that caused the hold. +func heldPortClaims(held map[string][]config.ServiceConfig) map[string]map[config.PortClaim]string { + if len(held) == 0 { + return nil + } + claims := make(map[string]map[config.PortClaim]string, len(held)) + for node, services := range held { + for _, service := range services { + for _, claim := range service.PortClaims() { + if claims[node] == nil { + claims[node] = make(map[config.PortClaim]string) + } + claims[node][claim] = service.Name + } + } + } + return claims +} + +// reserveHeldCapacity removes the compute a held service is still using from +// the capacity offered to the scheduler, so re-rendering it outside the +// scheduler cannot over-commit the node it runs on. +func reserveHeldCapacity(nodes []scheduler.Node, held map[string][]config.ServiceConfig) []scheduler.Node { + if len(held) == 0 { + return nodes + } + adjusted := append([]scheduler.Node(nil), nodes...) + for i := range adjusted { + for _, service := range held[adjusted[i].InstanceID] { + adjusted[i].CapacityVCPUs -= service.VCPUs + adjusted[i].CapacityMemMB -= service.MemoryMB + } + if adjusted[i].CapacityVCPUs < 0 { + adjusted[i].CapacityVCPUs = 0 + } + if adjusted[i].CapacityMemMB < 0 { + adjusted[i].CapacityMemMB = 0 } } - return assignment, nil + return adjusted } func (c *Controller) stillLeader(ctx context.Context) bool { diff --git a/internal/controlplane/heartbeat_size_test.go b/internal/controlplane/heartbeat_size_test.go index b29c5da..e8f636f 100644 --- a/internal/controlplane/heartbeat_size_test.go +++ b/internal/controlplane/heartbeat_size_test.go @@ -75,6 +75,15 @@ func maximalAgentStatus(nodeID string) *statusmodel.AgentStatus { SharedBackendID: strings.Repeat("d", statusmodel.MaxVolumeIDLen), State: strings.Repeat("e", statusmodel.MaxEnumLen), LastError: wide, + // The rejection fields are additive but still per-volume, so + // they have to be at their bound here or the measured worst + // case understates what an agent can actually send. + DesiredSizeBytes: 1<<63 - 1, + AppliedSizeBytes: 1<<63 - 1, + ResizeGeneration: 1<<63 - 1, + RequestedSizeBytes: 1<<63 - 1, + Rejected: true, + RejectedReason: strings.Repeat("r", statusmodel.MaxReasonCodeLen), }) } status.Services = append(status.Services, service) diff --git a/internal/controlplane/registry.go b/internal/controlplane/registry.go index 43a851c..9746dc4 100644 --- a/internal/controlplane/registry.go +++ b/internal/controlplane/registry.go @@ -35,7 +35,7 @@ import ( // before marshaling, so statusmodel.MaxMessageLen is an actual limit on what // gets sent, not just an assumption; only a non-standard client bypassing // BoundedMessage could exceed it. Measured worst case at that limit is about -// 12.2 MiB; see TestMaxRegistryRequestBytesExceedsLargestValidHeartbeat, which +// 13.6 MiB; see TestMaxRegistryRequestBytesExceedsLargestValidHeartbeat, which // fails if the bounds grow past this cap. const maxRegistryRequestBytes = 16 << 20 @@ -497,6 +497,9 @@ func validateVolumeStatus(serviceName string, volume statusmodel.VolumeStatus) e if len(volume.State) > statusmodel.MaxEnumLen { return fmt.Errorf("agent_status service %q volume state exceeds %d bytes", serviceName, statusmodel.MaxEnumLen) } + if len(volume.RejectedReason) > statusmodel.MaxReasonCodeLen { + return fmt.Errorf("agent_status service %q volume rejected_reason exceeds %d bytes", serviceName, statusmodel.MaxReasonCodeLen) + } // LastError, like Message elsewhere in AgentStatus, is accept-then- // truncate rather than rejected: applyHeartbeatAgentStatus runs it // through BoundedMessage after validation succeeds. diff --git a/internal/controlplane/state.go b/internal/controlplane/state.go index e815a4e..9d2e644 100644 --- a/internal/controlplane/state.go +++ b/internal/controlplane/state.go @@ -92,6 +92,13 @@ type PlacementRevision struct { CreatedAt time.Time `json:"created_at"` NodeConfigs []config.NodeConfig `json:"node_configs"` PendingServices []PendingPlacement `json:"pending_services,omitempty"` + // HeldServices are running services whose desired configuration could not + // be applied — their own volume record could not be read, so the last + // rendered configuration was re-used instead. They are deliberately not + // pending: pending drops a service from the rendered node configs and the + // agent turns that into a delete. But they are not converged either, so + // they are reported here rather than being invisible. + HeldServices []PendingPlacement `json:"held_services,omitempty"` } type PendingPlacement struct { @@ -106,8 +113,26 @@ const ( VolumeResizePending VolumeResizeState = "pending" VolumeResizeApplied VolumeResizeState = "applied" VolumeResizeFailed VolumeResizeState = "failed" + // VolumeResizeRejected is a terminal sibling of VolumeResizeFailed. Failed + // means an operation that failed and may succeed on retry; rejected means + // the requested size is impossible for the current contents and will not + // be retried without a new generation. + VolumeResizeRejected VolumeResizeState = "rejected" ) +// knownVolumeResizeState reports whether a state is one this controller +// understands. An unrecognized value is carried through untouched rather than +// treated as invalid, so a record written by a newer control plane does not +// brick scheduling on an older one during a rollback. +func knownVolumeResizeState(state VolumeResizeState) bool { + switch state { + case VolumeResizePending, VolumeResizeApplied, VolumeResizeFailed, VolumeResizeRejected: + return true + default: + return false + } +} + // VolumeRecord retains volume identity and placement after workload removal. type VolumeRecord struct { LogicalID string `json:"logical_id"` @@ -119,8 +144,64 @@ type VolumeRecord struct { ResizeGeneration int64 `json:"resize_generation"` ResizeState VolumeResizeState `json:"resize_state"` LastError string `json:"last_error,omitempty"` - CreatedAt time.Time `json:"created_at"` - UpdatedAt time.Time `json:"updated_at"` + // RequestedSizeBytes preserves a size the cluster refused, so status can + // show the operator's request next to the effective size actually running. + // + // The invariant that makes one field enough is that DesiredSizeBytes never + // renders a size the cluster is not running: a capacity rejection is + // refused before it is written, and a shrink rejection is acknowledged by + // resetting DesiredSizeBytes to AppliedSizeBytes in the same write that + // records RequestedSizeBytes. + RequestedSizeBytes int64 `json:"requested_size_bytes,omitempty"` + // RejectedReason is non-empty exactly while a rejection stands. It is the + // signal the idempotence guard keys on, alongside RequestedSizeBytes. + RejectedReason string `json:"rejected_reason,omitempty"` + // RejectedAvailableBytes is the capacity figure the refusal was measured + // against, for the operator who has to decide what to free. + RejectedAvailableBytes int64 `json:"rejected_available_bytes,omitempty"` + // RejectedAt records when the request was *first* refused. It is preserved + // across ticks rather than restamped: restamping makes every comparison + // unequal and turns a standing rejection into a write loop. + RejectedAt time.Time `json:"rejected_at,omitempty"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` +} + +// rejectionStands reports whether the record currently carries a refused +// request. ResizeState deliberately does not carry a capacity rejection: it +// describes the last resize the agent actually attempted, and a request +// refused before it became a resize never entered that state machine. +func (r VolumeRecord) rejectionStands() bool { return r.RejectedReason != "" } + +// clearRejection ends a refusal and reports whether anything changed, so a +// caller can avoid a no-op write. +// +// It resets the resize state as well as the rejection fields, because the two +// describe one condition. Clearing only the fields publishes state "rejected" +// alongside rejected:false, and that contradiction does not self-heal: the +// agent reports the *applied* generation once the config is normalized, while +// acknowledgement only matches an observation at the *refused* generation, so +// nothing ever revisits the record. +func (r *VolumeRecord) clearRejection() bool { + changed := false + if r.RequestedSizeBytes != 0 || r.RejectedReason != "" || r.RejectedAvailableBytes != 0 || !r.RejectedAt.IsZero() { + r.RequestedSizeBytes = 0 + r.RejectedReason = "" + r.RejectedAvailableBytes = 0 + r.RejectedAt = time.Time{} + changed = true + } + if r.ResizeState == VolumeResizeRejected { + // The refusal is over, so the state describes what is on disk again. + r.ResizeState = VolumeResizePending + if r.AppliedSizeBytes == r.DesiredSizeBytes { + r.ResizeState = VolumeResizeApplied + } + // LastError held the measured minimum for the refused request. + r.LastError = "" + changed = true + } + return changed } // RevisionPointer points to the current immutable revision. diff --git a/internal/controlplane/visibility.go b/internal/controlplane/visibility.go index 38dab1b..5ed1ec9 100644 --- a/internal/controlplane/visibility.go +++ b/internal/controlplane/visibility.go @@ -341,11 +341,20 @@ func desiredVolumeStatuses(service config.ServiceConfig, records map[string]Volu if record, ok := records[status.LogicalID]; ok { status.BoundNode = record.BoundNode status.SharedBackendID = record.SharedBackendID + // DesiredSizeBytes is the *effective* size: what the control plane + // accepted and rendered. When a request was refused, the operator + // edited size: and would otherwise see nothing change with no + // explanation, so the refused request is surfaced beside it. status.DesiredSizeBytes = record.DesiredSizeBytes status.AppliedSizeBytes = record.AppliedSizeBytes status.ResizeGeneration = record.ResizeGeneration status.State = string(record.ResizeState) status.LastError = record.LastError + if record.rejectionStands() { + status.RequestedSizeBytes = record.RequestedSizeBytes + status.Rejected = true + status.RejectedReason = record.RejectedReason + } } volumes = append(volumes, status) } @@ -353,6 +362,36 @@ func desiredVolumeStatuses(service config.ServiceConfig, records map[string]Volu return volumes } +// refusedVolumes lists the logical IDs whose record carries a standing refusal +// *and* whose volume the desired revision still declares, in deterministic +// order. +// +// The desired-revision filter is the whole point. Records outlive their +// service by design — deleting application YAML never deletes a volume or its +// record — so scanning every retained record means one refused resize followed +// by a service deletion degrades that revision and every revision after it, +// forever, with no service left to repair. A refusal is only a convergence +// problem while something is still asking for the size. +func (s visibilitySnapshot) refusedVolumes() []string { + desired := make(map[string]struct{}) + for _, service := range s.desired.Services { + for _, volume := range service.Volumes { + desired[service.Name+"/"+volume.Name] = struct{}{} + } + } + var refused []string + for id, record := range s.volumeByID { + if _, wanted := desired[id]; !wanted { + continue + } + if record.rejectionStands() { + refused = append(refused, id) + } + } + sort.Strings(refused) + return refused +} + func mergeVolumeStatuses(base, observed []statusmodel.VolumeStatus) []statusmodel.VolumeStatus { merged := append([]statusmodel.VolumeStatus(nil), base...) indexByID := make(map[string]int, len(merged)) @@ -377,9 +416,35 @@ func mergeVolumeStatuses(base, observed []statusmodel.VolumeStatus) []statusmode if status.DesiredSizeBytes <= 0 { status.DesiredSizeBytes = fallback.DesiredSizeBytes } + // An image that exists on disk has a durable applied size even + // when its VM is not running and the agent reports zero. Without + // this guard the whole-struct replacement below displays applied + // 0 for every stopped service. (The merge still cannot tell + // "reported zero" from "did not report"; that distinction is the + // volume-status freshness work in #38, which supersedes this + // guard rather than fighting it.) + if status.AppliedSizeBytes <= 0 { + status.AppliedSizeBytes = fallback.AppliedSizeBytes + } if status.ResizeGeneration <= 0 { status.ResizeGeneration = fallback.ResizeGeneration } + // The capacity rejection in applyExistingVolumeRecords is entirely + // control-plane-sourced: the agent is handed the clamped config + // and does not know a rejection happened, so its observation + // carries none of these. Whole-struct replacement would clear them + // on every cycle and make the rejection invisible. A shrink + // rejection is the other way round — the agent reports it, so the + // observed values are non-empty and authoritative. + if status.RequestedSizeBytes <= 0 { + status.RequestedSizeBytes = fallback.RequestedSizeBytes + } + if !status.Rejected { + status.Rejected = fallback.Rejected + } + if status.RejectedReason == "" { + status.RejectedReason = fallback.RejectedReason + } if status.State == "" { status.State = fallback.State } @@ -652,6 +717,28 @@ func (s visibilitySnapshot) revisionStatus() RevisionStatus { status.Message = statusmodel.BoundedMessage(s.placement.PendingServices[0].Message) return status } + // A volume running at an effective size because its request was refused is + // not converged, and after the agent's refusal is acknowledged the record + // is the only place that still knows the operator's request stands — the + // rendered config carries the effective size by then, so the agent cannot + // tell a standing request from a withdrawn one. That half of the + // visibility therefore lives here. + if refused := s.refusedVolumes(); len(refused) > 0 { + status.Phase = "degraded" + status.ReasonCode = "volume_size_rejected" + status.Message = statusmodel.BoundedMessage(fmt.Sprintf( + "running an effective size for: %s", strings.Join(refused, ", "))) + return status + } + if len(s.placement.HeldServices) > 0 { + // The workloads are running, so this is not a failure — but the + // desired revision was not applied to them, so it is not convergence + // either. + status.Phase = "degraded" + status.ReasonCode = s.placement.HeldServices[0].ReasonCode + status.Message = statusmodel.BoundedMessage(s.placement.HeldServices[0].Message) + return status + } expectedRendered := "" for _, node := range s.placement.NodeConfigs { if node.RenderedRevision == "" { @@ -710,6 +797,7 @@ func (s visibilitySnapshot) revisionStatus() RevisionStatus { } observedCurrent := false + degradedTypes := make(map[string]struct{}) for nodeID := range relevant { record, exists := s.nodeByID[nodeID] if !exists { @@ -764,6 +852,11 @@ func (s visibilitySnapshot) revisionStatus() RevisionStatus { status.UnknownNodes = append(status.UnknownNodes, nodeID) case degraded: status.DegradedNodes = append(status.DegradedNodes, nodeID) + for _, condition := range agentStatus.Conditions { + if statusmodel.IsNonBlockingCondition(condition.Type) && condition.Status == statusmodel.ConditionFalse { + degradedTypes[condition.Type] = struct{}{} + } + } default: status.ConvergedNodes = append(status.ConvergedNodes, nodeID) } @@ -786,7 +879,7 @@ func (s visibilitySnapshot) revisionStatus() RevisionStatus { } case len(status.DegradedNodes) > 0: status.Phase = "degraded" - status.ReasonCode = "peer_routes_degraded" + status.ReasonCode = degradedFleetReason(degradedTypes) default: status.Phase = "converged" } @@ -832,6 +925,23 @@ func reportsAllServices(status statusmodel.AgentStatus, expected []string) bool return true } +// degradedFleetReason names the fleet-level cause of a degraded phase. The +// precedence is fixed rather than derived from map order so the reported reason +// is stable, and a volume running at the wrong size outranks peer-route +// telemetry because it describes the workload rather than the observability of +// it. +func degradedFleetReason(types map[string]struct{}) string { + for _, candidate := range []struct{ conditionType, reason string }{ + {"VolumeSizesApplied", "volume_size_rejected"}, + {"PeerRoutesReady", "peer_routes_degraded"}, + } { + if _, ok := types[candidate.conditionType]; ok { + return candidate.reason + } + } + return "peer_routes_degraded" +} + func assessConditions(conditions []statusmodel.Condition) (blockingFailure, unknown, degraded bool) { for _, condition := range conditions { switch { diff --git a/internal/controlplane/volume_admission_test.go b/internal/controlplane/volume_admission_test.go new file mode 100644 index 0000000..3453ec1 --- /dev/null +++ b/internal/controlplane/volume_admission_test.go @@ -0,0 +1,363 @@ +package controlplane + +import ( + "context" + "io" + "log/slog" + "testing" + "time" + + "github.com/artemnikitin/firework/internal/config" + "github.com/artemnikitin/firework/internal/scheduler" + "github.com/artemnikitin/firework/internal/statusmodel" +) + +func admissionController(t *testing.T) (*Controller, StateStore) { + t.Helper() + store := newBlobStateStore(newMemBlob()) + return NewController(Config{State: StateConfig{Prefix: "cp/v1/"}}, store, slog.New(slog.NewTextHandler(io.Discard, nil))), store +} + +func putRecord(t *testing.T, store StateStore, service, volume string, record VolumeRecord) { + t.Helper() + if _, err := store.PutJSON(context.Background(), mustVolumeRecordKey("cp/v1/", service, volume), record); err != nil { + t.Fatal(err) + } +} + +func appliedRecord(logicalID string, size int64) VolumeRecord { + now := time.Now().UTC() + return VolumeRecord{ + LogicalID: logicalID, Type: config.VolumeTypeLocal, BoundNode: "node-1", + DesiredSizeBytes: size, AppliedSizeBytes: size, ResizeGeneration: 1, + ResizeState: VolumeResizeApplied, CreatedAt: now, UpdatedAt: now, + } +} + +func serviceWithVolume(name string, size int64) config.ServiceConfig { + return config.ServiceConfig{Name: name, VCPUs: 1, MemoryMB: 512, Volumes: []config.VolumeConfig{{ + Name: "data", Type: config.VolumeTypeLocal, MountPath: "/data", SizeBytes: size, + }}} +} + +func poolNodes(capacity int64) []scheduler.Node { + return []scheduler.Node{{ + InstanceID: "node-1", CapacityVCPUs: 8, CapacityMemMB: 8192, LocalCapacityBytes: capacity, + }} +} + +// A size request the pool cannot satisfy is not adopted, and — the part that +// does the real work — the rendered configuration is clamped to the last +// accepted size. Declining the record write alone would leave the infeasible +// size in the scheduling copy and render it to the agent regardless. +func TestInfeasibleRaiseIsRefusedAndClampedToTheEffectiveSize(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + putRecord(t, store, "db", "data", appliedRecord("db/data", 10*config.GiB)) + + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + services := []config.ServiceConfig{serviceWithVolume("db", 90*config.GiB)} + admission, err := controller.applyExistingVolumeRecords(ctx, services, set, poolNodes(20*config.GiB)) + if err != nil { + t.Fatal(err) + } + if len(admission.Held) != 0 { + t.Fatalf("a refused resize must not hold the service: %#v", admission.Held) + } + volume := services[0].Volumes[0] + if volume.SizeBytes != 10*config.GiB || volume.ResizeGeneration != 1 { + t.Fatalf("expected the effective size and generation to render, got %#v", volume) + } + stored := set.Records["db/data"].Record + if stored.DesiredSizeBytes != 10*config.GiB { + t.Fatalf("the refused size must not become the effective size: %#v", stored) + } + if !stored.rejectionStands() || stored.RequestedSizeBytes != 90*config.GiB || + stored.RejectedReason != scheduler.ReasonNodeStorageExhausted { + t.Fatalf("rejection was not recorded durably: %#v", stored) + } + if stored.ResizeGeneration != 1 { + t.Fatalf("a refused request must not mint a generation, got %d", stored.ResizeGeneration) + } +} + +// A standing rejection must be idempotent. Comparing the incoming request +// against DesiredSizeBytes — which now holds the effective size — makes the +// unchanged request look new on every tick and mints a generation forever. +func TestStandingRejectionProducesNoFurtherWritesOrGenerations(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + putRecord(t, store, "db", "data", appliedRecord("db/data", 10*config.GiB)) + + var firstRejectedAt time.Time + var digests []string + for tick := 0; tick < 3; tick++ { + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + services := []config.ServiceConfig{serviceWithVolume("db", 90*config.GiB)} + if _, err := controller.applyExistingVolumeRecords(ctx, services, set, poolNodes(20*config.GiB)); err != nil { + t.Fatal(err) + } + stored := set.Records["db/data"].Record + if stored.ResizeGeneration != 1 { + t.Fatalf("tick %d minted a generation for a standing rejection: %d", tick, stored.ResizeGeneration) + } + if tick == 0 { + firstRejectedAt = stored.RejectedAt + } else if !stored.RejectedAt.Equal(firstRejectedAt) { + t.Fatalf("tick %d restamped RejectedAt: %v vs %v", tick, stored.RejectedAt, firstRejectedAt) + } + digests = append(digests, volumeRecordsDigest(set)) + } + if digests[1] != digests[2] { + t.Fatal("a steady rejected request must leave the records digest stable") + } +} + +// Reverting the request to a feasible size clears the rejection and mints +// exactly one generation. Reverting it to the effective size clears the +// rejection and mints none, because there is no resize to perform. +func TestRejectionRecovery(t *testing.T) { + tests := []struct { + name string + requested int64 + wantGeneration int64 + }{ + {name: "feasible new size", requested: 15 * config.GiB, wantGeneration: 2}, + {name: "back to the effective size", requested: 10 * config.GiB, wantGeneration: 1}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + putRecord(t, store, "db", "data", appliedRecord("db/data", 10*config.GiB)) + + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + refused := []config.ServiceConfig{serviceWithVolume("db", 90*config.GiB)} + if _, err := controller.applyExistingVolumeRecords(ctx, refused, set, poolNodes(20*config.GiB)); err != nil { + t.Fatal(err) + } + + set, err = controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + corrected := []config.ServiceConfig{serviceWithVolume("db", test.requested)} + if _, err := controller.applyExistingVolumeRecords(ctx, corrected, set, poolNodes(20*config.GiB)); err != nil { + t.Fatal(err) + } + stored := set.Records["db/data"].Record + if stored.rejectionStands() { + t.Fatalf("expected the rejection to clear, got %#v", stored) + } + if stored.ResizeGeneration != test.wantGeneration { + t.Fatalf("expected generation %d, got %d", test.wantGeneration, stored.ResizeGeneration) + } + if stored.DesiredSizeBytes != test.requested { + t.Fatalf("expected effective size %d, got %d", test.requested, stored.DesiredSizeBytes) + } + }) + } +} + +// Two raises that are each feasible on their own must not both be admitted +// when their combined reservation exceeds the pool, and the same one must win +// on every controller and after every leader change. +func TestBatchedRaisesAdmitOnlyWhatThePoolHolds(t *testing.T) { + ctx := context.Background() + for run := 0; run < 3; run++ { + controller, store := admissionController(t) + putRecord(t, store, "alpha", "data", appliedRecord("alpha/data", 10*config.GiB)) + putRecord(t, store, "beta", "data", appliedRecord("beta/data", 10*config.GiB)) + + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + // Declared out of name order to prove the evaluation order is by name. + services := []config.ServiceConfig{ + serviceWithVolume("beta", 30*config.GiB), + serviceWithVolume("alpha", 30*config.GiB), + } + if _, err := controller.applyExistingVolumeRecords(ctx, services, set, poolNodes(45*config.GiB)); err != nil { + t.Fatal(err) + } + alpha := set.Records["alpha/data"].Record + beta := set.Records["beta/data"].Record + if alpha.DesiredSizeBytes != 30*config.GiB { + t.Fatalf("run %d: the first raise by name must be admitted, got %#v", run, alpha) + } + if beta.DesiredSizeBytes != 10*config.GiB || !beta.rejectionStands() { + t.Fatalf("run %d: the second raise must be refused, got %#v", run, beta) + } + } +} + +// A bound node that is not observable fails closed: the raise waits rather +// than being admitted against capacity nobody can verify, and the existing +// effective size keeps rendering so the service is not disturbed. +func TestRaiseOnAnAbsentNodeFailsClosed(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + putRecord(t, store, "db", "data", appliedRecord("db/data", 10*config.GiB)) + + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + services := []config.ServiceConfig{serviceWithVolume("db", 12*config.GiB)} + if _, err := controller.applyExistingVolumeRecords(ctx, services, set, nil); err != nil { + t.Fatal(err) + } + if got := services[0].Volumes[0].SizeBytes; got != 10*config.GiB { + t.Fatalf("expected the effective size to keep rendering, got %d", got) + } + stored := set.Records["db/data"].Record + if stored.RejectedReason != scheduler.ReasonStorageCapacityUnknown { + t.Fatalf("expected an unknown-capacity refusal, got %#v", stored) + } +} + +// A capacity rejection is entirely control-plane-sourced: the agent is handed +// the clamped config and does not know one happened, so its observation +// carries none of the rejection fields. Whole-struct replacement in the merge +// would clear them on every cycle and make the rejection invisible in exactly +// the case it exists to make visible. +func TestCapacityRejectionSurvivesTheMergeAgainstAnUnawareAgent(t *testing.T) { + base := []statusmodel.VolumeStatus{{ + LogicalID: "db/data", Type: "local", MountPath: "/data", BoundNode: "node-1", + DesiredSizeBytes: 10 * config.GiB, AppliedSizeBytes: 10 * config.GiB, ResizeGeneration: 1, + State: "applied", RequestedSizeBytes: 90 * config.GiB, Rejected: true, + RejectedReason: scheduler.ReasonNodeStorageExhausted, + }} + observed := []statusmodel.VolumeStatus{{ + LogicalID: "db/data", Type: "local", MountPath: "/data", BoundNode: "node-1", + DesiredSizeBytes: 10 * config.GiB, ResizeGeneration: 1, State: "prepared", + }} + + merged := mergeVolumeStatuses(base, observed) + if len(merged) != 1 { + t.Fatalf("unexpected merge result: %#v", merged) + } + got := merged[0] + if got.RequestedSizeBytes != 90*config.GiB || !got.Rejected || got.RejectedReason != scheduler.ReasonNodeStorageExhausted { + t.Fatalf("the rejection was cleared by the merge: %#v", got) + } + // The agent reported no applied size because its VM is not running; the + // durable value must not be erased. + if got.AppliedSizeBytes != 10*config.GiB { + t.Fatalf("the durable applied size was erased: %#v", got) + } + // The agent's own observation still wins where it has one. + if got.State != "prepared" { + t.Fatalf("the agent observation must still be authoritative for state: %#v", got) + } +} + +// A rejection cannot ride on the "prepared" branch: after a refusal Prepare +// succeeds at the old size, but the prepared arm requires the observed applied +// size to equal the record's desired size — which is exactly what a rejection +// makes false. It needs its own state and its own acknowledgement arm. +func TestRejectedObservationConvergesTheRecordOnTheEffectiveSize(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + recordKey := mustVolumeRecordKey("cp/v1/", "db", "data") + // The raise to 2 GiB was accepted by the control plane; the agent then + // refused it as below the safe minimum. + putRecord(t, store, "db", "data", VolumeRecord{ + LogicalID: "db/data", Type: config.VolumeTypeLocal, BoundNode: "node-1", + DesiredSizeBytes: 2 * config.GiB, AppliedSizeBytes: 10 * config.GiB, + ResizeGeneration: 2, ResizeState: VolumeResizePending, + }) + + nodeKey, err := nodeRecordKey("cp/v1/", "node-1") + if err != nil { + t.Fatal(err) + } + node := NodeRecord{NodeID: "node-1", AgentStatus: &statusmodel.AgentStatus{Services: []statusmodel.ServiceStatus{{ + Name: "db", Volumes: []statusmodel.VolumeStatus{{ + LogicalID: "db/data", Type: "local", BoundNode: "node-1", + AppliedSizeBytes: 10 * config.GiB, RequestedSizeBytes: 2 * config.GiB, + ResizeGeneration: 2, State: "rejected", Rejected: true, + RejectedReason: "shrink_below_minimum", LastError: "below the safe minimum", + }}, + }}}} + if _, err := store.PutJSON(ctx, nodeKey, node); err != nil { + t.Fatal(err) + } + + if err := controller.acknowledgeVolumeRecords(ctx); err != nil { + t.Fatal(err) + } + readRecord := func() VolumeRecord { + t.Helper() + var got VolumeRecord + if _, exists, err := store.GetJSON(ctx, recordKey, &got); err != nil || !exists { + t.Fatalf("read record: exists=%v err=%v", exists, err) + } + return got + } + got := readRecord() + if got.ResizeState != VolumeResizeRejected { + t.Fatalf("expected the rejected state, got %#v", got) + } + // One write establishes the invariant that the record never renders a size + // the cluster is not running. + if got.DesiredSizeBytes != 10*config.GiB || got.RequestedSizeBytes != 2*config.GiB { + t.Fatalf("the record did not converge on the effective size: %#v", got) + } + if got.ResizeGeneration != 2 { + t.Fatalf("the requested generation must be retained, got %d", got.ResizeGeneration) + } + first := got.RejectedAt + + // Re-running against an unchanged observation must perform no second write. + if err := controller.acknowledgeVolumeRecords(ctx); err != nil { + t.Fatal(err) + } + if again := readRecord(); !again.RejectedAt.Equal(first) || again.UpdatedAt != got.UpdatedAt { + t.Fatalf("an unchanged rejected observation was written again: %#v vs %#v", again, got) + } +} + +// After the acknowledgement, the effective size is what renders — so no +// further update is planned, and the requested size is still visible. +func TestAcknowledgedRejectionRendersTheEffectiveSize(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + putRecord(t, store, "db", "data", VolumeRecord{ + LogicalID: "db/data", Type: config.VolumeTypeLocal, BoundNode: "node-1", + DesiredSizeBytes: 10 * config.GiB, AppliedSizeBytes: 10 * config.GiB, + RequestedSizeBytes: 2 * config.GiB, ResizeGeneration: 2, + ResizeState: VolumeResizeRejected, RejectedReason: "shrink_below_minimum", + RejectedAt: time.Now().UTC(), + }) + + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + services := []config.ServiceConfig{serviceWithVolume("db", 2*config.GiB)} + if _, err := controller.applyExistingVolumeRecords(ctx, services, set, poolNodes(100*config.GiB)); err != nil { + t.Fatal(err) + } + volume := services[0].Volumes[0] + if volume.SizeBytes != 10*config.GiB || volume.ResizeGeneration != 2 { + t.Fatalf("expected the effective configuration to render, got %#v", volume) + } + stored := set.Records["db/data"].Record + if stored.ResizeGeneration != 2 { + t.Fatalf("a standing rejection must not mint a generation, got %d", stored.ResizeGeneration) + } + if stored.RequestedSizeBytes != 2*config.GiB { + t.Fatalf("the refused size must stay visible, got %d", stored.RequestedSizeBytes) + } +} diff --git a/internal/controlplane/volume_quarantine.go b/internal/controlplane/volume_quarantine.go new file mode 100644 index 0000000..f70956a --- /dev/null +++ b/internal/controlplane/volume_quarantine.go @@ -0,0 +1,228 @@ +package controlplane + +import ( + "context" + "fmt" + "path" + "strings" + + "github.com/artemnikitin/firework/internal/config" + "github.com/artemnikitin/firework/internal/objectstorage" + "github.com/artemnikitin/firework/internal/statusmodel" +) + +// Quarantine tiers, ordered by how much of a record the parse could recover. +// The tier is what the caller uses to decide how far a block has to widen; the +// helper itself never decides what to do about a bad record. +const ( + // quarantineTierExact means both sizes and a binding parsed, so the + // reservation is exact and only the owning service is affected. + quarantineTierExact = 1 + // quarantineTierPartial means only some of that parsed. Whatever lower + // bound is attributable is charged, *and* the scope it is attributable to + // is marked unknown — a lower bound without the flag is the over-commit + // this tier exists to prevent. + quarantineTierPartial = 2 + // quarantineTierUnattributable means no binding could be determined, so + // there is no account to charge. The block widens to a whole storage + // class, or to both when even the type is unreadable. + quarantineTierUnattributable = 3 +) + +// volumeQuarantine describes a retained record that failed validation, in the +// terms scheduling actually consumes. +// +// Class is populated only from a parsed `type` field, never from the key: +// volumeRecordKey encodes service and volume names and nothing else, so an +// unreadable object gives no storage class and its block covers both. +type volumeQuarantine struct { + Key string + LogicalID string + Reason string + Tier int + ReservedBytes int64 + BoundNode string + SharedBackendID string + Class config.VolumeType +} + +// volumeRecordSet is the outcome of loading every retained record: the ones +// that can be used normally, and the ones that cannot but still hold capacity. +type volumeRecordSet struct { + Records map[string]storedVolumeRecord + Quarantined map[string]volumeQuarantine +} + +func (s volumeRecordSet) quarantineFor(logicalID string) (volumeQuarantine, bool) { + quarantine, ok := s.Quarantined[logicalID] + return quarantine, ok +} + +// classifyVolumeRecord is the single canonical parse of a retained volume +// record. What is canonical here is the *parse*, not the response to it: the +// scheduling loader and the status projection have different jobs and cannot +// share a policy, so this reports how much it recovered and each caller +// decides. +// +// readErr is the object-read failure, if any. Everything else is derived from +// the decoded record. +func classifyVolumeRecord(prefix, key string, record VolumeRecord, readErr error) (VolumeRecord, *volumeQuarantine) { + logicalID := logicalIDFromRecordKey(prefix, key) + quarantine := func(reason string) *volumeQuarantine { + return buildVolumeQuarantine(key, logicalID, reason, record) + } + if readErr != nil { + // Nothing was decoded, so nothing is attributable: no binding, and no + // class either, because the class lives inside the object. + return VolumeRecord{}, &volumeQuarantine{ + Key: key, LogicalID: logicalID, Tier: quarantineTierUnattributable, + Reason: statusmodel.BoundedMessage(fmt.Sprintf("read volume record: %v", readErr)), + } + } + if record.LogicalID == "" { + return VolumeRecord{}, quarantine("record has no logical_id") + } + if logicalID == "" || record.LogicalID != logicalID { + return VolumeRecord{}, quarantine("logical_id does not match its key") + } + if record.Type != config.VolumeTypeLocal && record.Type != config.VolumeTypeShared { + return VolumeRecord{}, quarantine(fmt.Sprintf("invalid type %q", record.Type)) + } + if record.Type == config.VolumeTypeLocal && record.BoundNode == "" { + return VolumeRecord{}, quarantine("missing bound_node") + } + if record.Type == config.VolumeTypeShared && record.SharedBackendID == "" { + return VolumeRecord{}, quarantine("missing shared_backend_id") + } + if record.DesiredSizeBytes <= 0 || record.ResizeGeneration <= 0 { + return VolumeRecord{}, quarantine("invalid size or generation") + } + if record.AppliedSizeBytes < 0 { + return VolumeRecord{}, quarantine("negative applied size") + } + if record.ResizeState == VolumeResizeApplied && record.AppliedSizeBytes != record.DesiredSizeBytes { + return VolumeRecord{}, quarantine("applied state with mismatched size") + } + if record.ResizeState == "" { + // An *absent* state is malformed, not a state owned by a newer + // protocol. Forward compatibility exists to protect a value a newer + // controller deliberately wrote; the empty string is what a truncated + // or hand-edited object produces, and treating it as untouchable would + // freeze the record forever — every later size request ignored, with + // nothing to name as the owner. + return VolumeRecord{}, quarantine("missing resize_state") + } + // A non-empty unrecognized resize_state is deliberately *not* a + // quarantine. A record written by a newer control plane must not brick + // scheduling on an older one during a rollback, so unknown states are + // carried through unchanged and never acknowledged or advanced here. + return record, nil +} + +// buildVolumeQuarantine recovers as much accounting as the decoded record +// allows. It is deliberately conservative: one parsed size is not an upper +// bound on the other (a pending shrink has applied > desired, a pending grow +// the reverse), so a record with only one readable size yields a lower bound, +// and a size with no readable binding is a number with no account. +func buildVolumeQuarantine(key, logicalID, reason string, record VolumeRecord) *volumeQuarantine { + quarantine := &volumeQuarantine{ + Key: key, LogicalID: logicalID, Reason: statusmodel.BoundedMessage(reason), + } + bothSizes := record.DesiredSizeBytes > 0 && record.AppliedSizeBytes > 0 + if record.DesiredSizeBytes > 0 { + quarantine.ReservedBytes = record.DesiredSizeBytes + } + if record.AppliedSizeBytes > quarantine.ReservedBytes { + quarantine.ReservedBytes = record.AppliedSizeBytes + } + + bound := false + switch record.Type { + case config.VolumeTypeLocal: + quarantine.Class = config.VolumeTypeLocal + if record.BoundNode != "" { + quarantine.BoundNode = record.BoundNode + bound = true + } + case config.VolumeTypeShared: + quarantine.Class = config.VolumeTypeShared + if record.SharedBackendID != "" { + quarantine.SharedBackendID = record.SharedBackendID + bound = true + } + } + + switch { + case bound && bothSizes: + quarantine.Tier = quarantineTierExact + case bound: + quarantine.Tier = quarantineTierPartial + default: + // No binding: the reservation cannot be charged to any node or + // backend. If the class parsed, the block covers that class; if it did + // not, it covers both. + quarantine.Tier = quarantineTierUnattributable + quarantine.ReservedBytes = 0 + } + return quarantine +} + +// logicalIDFromRecordKey recovers "service/volume" from a record key. The key +// is the only identity available for an object that cannot be decoded, and it +// is what lets a tier-3 quarantine still name the repair target. +func logicalIDFromRecordKey(prefix, key string) string { + root := volumeRecordsPrefix(prefix) + if !strings.HasPrefix(key, root) || !strings.HasSuffix(key, ".json") { + return "" + } + rest := strings.TrimSuffix(strings.TrimPrefix(key, root), ".json") + parts := strings.Split(rest, "/") + if len(parts) != 2 || parts[0] == "" || parts[1] == "" { + return "" + } + return path.Join(parts[0], parts[1]) +} + +// loadVolumeRecords partitions retained records instead of failing on the first +// bad one. Returning an error here stopped cluster-wide scheduling for a single +// malformed key. +// +// Partitioning alone would be unsafe: dropping a bad record silently releases +// its reservation and converts a hard failure into over-commit. So every +// quarantine is retained with whatever accounting can be proved, and the scope +// it cannot prove is flagged rather than handed out again. +func (c *Controller) loadVolumeRecords(ctx context.Context) (volumeRecordSet, error) { + keys, err := c.store.ListKeys(ctx, volumeRecordsPrefix(c.cfg.State.Prefix)) + if err != nil { + return volumeRecordSet{}, err + } + set := volumeRecordSet{ + Records: make(map[string]storedVolumeRecord, len(keys)), + Quarantined: make(map[string]volumeQuarantine), + } + for _, key := range keys { + if !strings.HasSuffix(key, ".json") { + continue + } + var record VolumeRecord + var token objectstorage.WriteToken + token, exists, readErr := c.store.GetJSON(ctx, key, &record) + if readErr == nil && !exists { + continue + } + valid, quarantine := classifyVolumeRecord(c.cfg.State.Prefix, key, record, readErr) + if quarantine != nil { + id := quarantine.LogicalID + if id == "" { + id = key + quarantine.LogicalID = key + } + c.logger.Warn("quarantined volume record", + "key", key, "tier", quarantine.Tier, "reason", quarantine.Reason) + set.Quarantined[id] = *quarantine + continue + } + set.Records[valid.LogicalID] = storedVolumeRecord{Record: valid, Token: token} + } + return set, nil +} diff --git a/internal/controlplane/volume_quarantine_test.go b/internal/controlplane/volume_quarantine_test.go new file mode 100644 index 0000000..4d68859 --- /dev/null +++ b/internal/controlplane/volume_quarantine_test.go @@ -0,0 +1,339 @@ +package controlplane + +import ( + "context" + "testing" + "time" + + "github.com/artemnikitin/firework/internal/config" + "github.com/artemnikitin/firework/internal/scheduler" +) + +func putRaw(t *testing.T, store StateStore, service, volume string, body []byte) { + t.Helper() + if _, err := store.PutRaw(context.Background(), mustVolumeRecordKey("cp/v1/", service, volume), body, "application/json"); err != nil { + t.Fatal(err) + } +} + +// One malformed record used to stop cluster-wide scheduling. It must now +// isolate to its own key while every other record stays usable. +func TestOneBadRecordDoesNotStopTheOthers(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + putRecord(t, store, "good", "data", appliedRecord("good/data", 10*config.GiB)) + putRaw(t, store, "bad", "data", []byte("{not json")) + + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatalf("a malformed record must not fail the load: %v", err) + } + if _, ok := set.Records["good/data"]; !ok { + t.Fatalf("the valid record was lost: %#v", set.Records) + } + if _, ok := set.Quarantined["bad/data"]; !ok { + t.Fatalf("the malformed record was not quarantined: %#v", set.Quarantined) + } +} + +// Every tier must hold onto whatever capacity it can prove. Dropping a bad +// record silently releases its reservation and turns a hard failure into +// over-commit, which is the failure mode the admission check exists for. +func TestQuarantineTiersChargeWhatTheyCanProve(t *testing.T) { + now := time.Now().UTC() + tests := []struct { + name string + record VolumeRecord + wantTier int + wantReserved int64 + wantNodeUnknown bool + wantLocalClassNew bool + }{ + { + name: "both sizes and a binding parse", + record: VolumeRecord{ + LogicalID: "svc/data", Type: config.VolumeTypeLocal, BoundNode: "node-1", + DesiredSizeBytes: 8 * config.GiB, AppliedSizeBytes: 10 * config.GiB, + ResizeGeneration: 1, ResizeState: VolumeResizeApplied, UpdatedAt: now, + }, + wantTier: quarantineTierExact, wantReserved: 10 * config.GiB, + }, + { + name: "binding parses but a size does not", + record: VolumeRecord{ + LogicalID: "svc/data", Type: config.VolumeTypeLocal, BoundNode: "node-1", + DesiredSizeBytes: 8 * config.GiB, ResizeGeneration: 0, UpdatedAt: now, + }, + wantTier: quarantineTierPartial, wantReserved: 8 * config.GiB, wantNodeUnknown: true, + }, + { + name: "type parses but no binding does", + record: VolumeRecord{ + LogicalID: "svc/data", Type: config.VolumeTypeLocal, + DesiredSizeBytes: 8 * config.GiB, AppliedSizeBytes: 8 * config.GiB, + ResizeGeneration: 1, UpdatedAt: now, + }, + wantTier: quarantineTierUnattributable, wantLocalClassNew: true, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + putRecord(t, store, "svc", "data", test.record) + + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + quarantine, ok := set.Quarantined["svc/data"] + if !ok { + t.Fatalf("expected a quarantine, got records %#v", set.Records) + } + if quarantine.Tier != test.wantTier { + t.Fatalf("tier = %d, want %d (%#v)", quarantine.Tier, test.wantTier, quarantine) + } + if quarantine.ReservedBytes != test.wantReserved { + t.Fatalf("reserved = %d, want %d", quarantine.ReservedBytes, test.wantReserved) + } + reservations := storageReservations(set) + if reservations.LocalByNode["node-1"] != test.wantReserved { + t.Fatalf("charged %d to the node, want %d", reservations.LocalByNode["node-1"], test.wantReserved) + } + if reservations.LocalUnknownByNode["node-1"] != test.wantNodeUnknown { + t.Fatalf("node unknown flag = %v, want %v", reservations.LocalUnknownByNode["node-1"], test.wantNodeUnknown) + } + if reservations.LocalClassUnknown != test.wantLocalClassNew { + t.Fatalf("class unknown flag = %v, want %v", reservations.LocalClassUnknown, test.wantLocalClassNew) + } + }) + } +} + +// An unreadable object gives no class, because the key encodes only service +// and volume names. The block therefore covers both storage classes. +func TestUnreadableRecordBlocksBothStorageClasses(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + putRaw(t, store, "svc", "data", []byte("{not json")) + + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + reservations := storageReservations(set) + if !reservations.LocalClassUnknown || !reservations.SharedClassUnknown { + t.Fatalf("expected both classes blocked, got %#v", reservations) + } +} + +// A record written by a newer control plane must not brick scheduling on an +// older one during a rollback. +func TestUnknownResizeStateIsToleratedNotQuarantined(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + record := appliedRecord("db/data", 10*config.GiB) + record.ResizeState = VolumeResizeState("quiescing") + putRecord(t, store, "db", "data", record) + + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + if _, quarantined := set.Quarantined["db/data"]; quarantined { + t.Fatalf("an unknown resize state must be carried through, not quarantined: %#v", set.Quarantined) + } + if got := set.Records["db/data"].Record.ResizeState; got != VolumeResizeState("quiescing") { + t.Fatalf("the unknown state was not carried through: %q", got) + } +} + +// A quarantined outcome must move the scheduling signature. Otherwise a +// partial repair, a changed binding, or the full repair an operator is waiting +// on all leave the cached signature unchanged and suppress the reconcile that +// would act on them. +func TestQuarantinedOutcomesEnterTheSchedulingDigest(t *testing.T) { + ctx := context.Background() + controller, store := admissionController(t) + + digest := func() string { + set, err := controller.loadVolumeRecords(ctx) + if err != nil { + t.Fatal(err) + } + return volumeRecordsDigest(set) + } + + // Tier 3: unreadable. + putRaw(t, store, "db", "data", []byte("{not json")) + tier3 := digest() + + // Tier 2: partially repaired — a binding now parses. + partial := VolumeRecord{ + LogicalID: "db/data", Type: config.VolumeTypeLocal, BoundNode: "node-1", + DesiredSizeBytes: 8 * config.GiB, ResizeGeneration: 0, + } + putRecord(t, store, "db", "data", partial) + tier2 := digest() + if tier2 == tier3 { + t.Fatal("a tier transition must change the records digest") + } + + // A changed binding moves which node is blocked. + partial.BoundNode = "node-2" + putRecord(t, store, "db", "data", partial) + rebound := digest() + if rebound == tier2 { + t.Fatal("a changed binding on a quarantined record must change the digest") + } + + // An unchanged invalid record must leave it stable. + if digest() != rebound { + t.Fatal("an unchanged quarantined record must leave the digest stable") + } + + // The full repair. + putRecord(t, store, "db", "data", appliedRecord("db/data", 10*config.GiB)) + if digest() == rebound { + t.Fatal("repairing a record must change the digest") + } +} + +// Blocking an owner must not evict it. A service that is already placed keeps +// running at its last effective configuration; one that was never placed is +// withheld, because there is nothing running to disturb. +func TestBlockedOwnersAreHeldOrPendingButNeverDropped(t *testing.T) { + held := config.ServiceConfig{Name: "running", VCPUs: 1, MemoryMB: 512, Volumes: []config.VolumeConfig{{ + Name: "data", Type: config.VolumeTypeLocal, MountPath: "/data", + SizeBytes: 10 * config.GiB, BoundNode: "node-1", ResizeGeneration: 3, + }}} + desired := []config.ServiceConfig{ + // The desired revision asks for a size the unreadable record cannot gate. + {Name: "running", VCPUs: 1, MemoryMB: 512, Volumes: []config.VolumeConfig{{ + Name: "data", Type: config.VolumeTypeLocal, MountPath: "/data", SizeBytes: 90 * config.GiB, + }}}, + serviceWithVolume("fresh", 10*config.GiB), + } + admission := volumeAdmission{Held: map[string]string{ + "running": scheduler.ReasonVolumeRecordInvalid, + "fresh": scheduler.ReasonVolumeRecordInvalid, + }} + placement := map[string]renderedPlacement{"running": {Node: "node-1", Service: held}} + + schedulable, heldByNode, pending := splitHeldServices(desired, admission, placement, map[string]struct{}{"node-1": {}}) + + if len(schedulable) != 0 { + t.Fatalf("both services are blocked, got %#v", schedulable) + } + rendered := heldByNode["node-1"] + if len(rendered) != 1 || rendered[0].Name != "running" { + t.Fatalf("the placed service must keep being rendered, got %#v", heldByNode) + } + if got := rendered[0].Volumes[0].SizeBytes; got != 10*config.GiB { + t.Fatalf("the held service must re-render its last effective size, got %d", got) + } + if len(pending) != 1 || pending[0].Service != "fresh" || pending[0].ReasonCode != scheduler.ReasonVolumeRecordInvalid { + t.Fatalf("the unplaced service must be pending, got %#v", pending) + } +} + +// Re-rendering a held service outside the scheduler must not let the scheduler +// hand its compute to something else. +func TestHeldServicesKeepTheirComputeReserved(t *testing.T) { + nodes := []scheduler.Node{{InstanceID: "node-1", CapacityVCPUs: 4, CapacityMemMB: 4096}} + held := map[string][]config.ServiceConfig{"node-1": {{Name: "running", VCPUs: 3, MemoryMB: 3072}}} + + adjusted := reserveHeldCapacity(nodes, held) + if adjusted[0].CapacityVCPUs != 1 || adjusted[0].CapacityMemMB != 1024 { + t.Fatalf("held compute was not reserved: %#v", adjusted[0]) + } + if nodes[0].CapacityVCPUs != 4 { + t.Fatal("reserveHeldCapacity must not mutate its input") + } +} + +// A held service is running, so it is not a failure — but the desired revision +// was never applied to it, so it must not read as convergence either. +func TestHeldServicesDegradeTheDeploymentStatus(t *testing.T) { + snapshot := visibilitySnapshot{ + desired: DesiredRevision{Revision: "rev-1"}, + placementCurrent: true, + placement: PlacementRevision{ + Revision: "placement-1", + HeldServices: []PendingPlacement{{ + Service: "db", ReasonCode: scheduler.ReasonVolumeRecordInvalid, + Message: "running the last applied configuration; the desired one could not be resolved", + }}, + }, + } + status := snapshot.revisionStatus() + if status.Phase != "degraded" { + t.Fatalf("expected a degraded deployment, got %q", status.Phase) + } + if status.ReasonCode != scheduler.ReasonVolumeRecordInvalid { + t.Fatalf("expected the hold reason to surface, got %q", status.ReasonCode) + } +} + +// The degraded fleet reason must name the actual cause. A volume running at +// the wrong size outranks peer-route telemetry, because it describes the +// workload rather than the observability of it. +func TestDegradedFleetReasonNamesTheCause(t *testing.T) { + tests := []struct { + name string + types map[string]struct{} + want string + }{ + {name: "volume rejection", types: map[string]struct{}{"VolumeSizesApplied": {}}, want: "volume_size_rejected"}, + {name: "peer routes", types: map[string]struct{}{"PeerRoutesReady": {}}, want: "peer_routes_degraded"}, + { + name: "both", + types: map[string]struct{}{"PeerRoutesReady": {}, "VolumeSizesApplied": {}}, + want: "volume_size_rejected", + }, + {name: "none recognized", types: map[string]struct{}{}, want: "peer_routes_degraded"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if got := degradedFleetReason(test.types); got != test.want { + t.Fatalf("degradedFleetReason = %q, want %q", got, test.want) + } + }) + } +} + +// A held service is re-rendered outside the scheduler, so its node-exclusive +// host-port claims have to be handed to the scheduler explicitly. Without this +// the scheduler places a new service on the same (tcp, host_port), and the +// agent then rejects the whole node config — taking down every service on the +// node over an unreadable record for one of them. +func TestHeldServicesKeepTheirHostPortClaims(t *testing.T) { + held := map[string][]config.ServiceConfig{"node-1": {{ + Name: "running", VCPUs: 1, MemoryMB: 512, + PortForwards: []config.PortForward{{HostPort: 8080, VMPort: 80}}, + }}} + + claims := heldPortClaims(held) + if holder := claims["node-1"][config.PortClaim{Protocol: "tcp", HostPort: 8080}]; holder != "running" { + t.Fatalf("expected the held service to hold its claim, got %#v", claims) + } + if heldPortClaims(nil) != nil { + t.Fatal("no held services means nothing is pinned") + } + + // End to end: the scheduler must not hand the same port to a new service. + nodes := []scheduler.Node{{InstanceID: "node-1", CapacityVCPUs: 8, CapacityMemMB: 8192}} + newcomer := config.ServiceConfig{ + Name: "newcomer", VCPUs: 1, MemoryMB: 512, + PortForwards: []config.PortForward{{HostPort: 8080, VMPort: 80}}, + } + assignments, pending := scheduler.ScheduleWithStorage( + []config.ServiceConfig{newcomer}, nodes, nil, scheduler.StorageReservations{}, claims) + + if len(assignments["node-1"]) != 0 { + t.Fatalf("the newcomer took a port a held service still holds: %#v", assignments) + } + if len(pending) != 1 || pending[0].ReasonCode != scheduler.ReasonHostPortConflict { + t.Fatalf("expected a host-port conflict, got %#v", pending) + } +} diff --git a/internal/controlplane/volume_records.go b/internal/controlplane/volume_records.go index e68483f..791c412 100644 --- a/internal/controlplane/volume_records.go +++ b/internal/controlplane/volume_records.go @@ -21,108 +21,302 @@ type storedVolumeRecord struct { Token objectstorage.WriteToken } -func (c *Controller) loadVolumeRecords(ctx context.Context) (map[string]storedVolumeRecord, error) { - keys, err := c.store.ListKeys(ctx, volumeRecordsPrefix(c.cfg.State.Prefix)) - if err != nil { - return nil, err +// volumeAdmission is the outcome of reconciling one desired revision against +// the retained records: which services cannot be scheduled from their desired +// configuration, and why. +type volumeAdmission struct { + // Held names the services whose own volume records could not be used. They + // are never simply dropped — omission from the rendered node configs is + // what the agent turns into a delete — so the caller either re-renders the + // last placement or leaves the service pending. + Held map[string]string +} + +// applyExistingVolumeRecords folds retained records into the desired revision +// and decides which size requests the cluster can accept. +// +// The key property is that a rejection clamps the *rendered* configuration +// rather than merely declining to write a record. Skipping the write alone +// changes nothing the agent sees: the scheduling copy still carries the +// requested SizeBytes, the logical ID is already recorded so the placement +// check computes a zero delta, and BuildNodeConfigs renders the infeasible +// size regardless of what the record says. +// +// Rejections never mark a service pending. Pending drops the service from +// BuildNodeConfigs, the agent turns the absent service into a delete, and a +// refused resize would stop a healthy workload. +func (c *Controller) applyExistingVolumeRecords(ctx context.Context, services []config.ServiceConfig, set volumeRecordSet, nodes []scheduler.Node) (volumeAdmission, error) { + admission := volumeAdmission{Held: make(map[string]string)} + nodeByID := make(map[string]scheduler.Node, len(nodes)) + for _, node := range nodes { + nodeByID[node.InstanceID] = node } - records := make(map[string]storedVolumeRecord, len(keys)) - for _, key := range keys { - if !strings.HasSuffix(key, ".json") { - continue - } - var record VolumeRecord - token, exists, err := c.store.GetJSON(ctx, key, &record) - if err != nil { - return nil, fmt.Errorf("read volume record %s: %w", key, err) - } - if !exists || record.LogicalID == "" { - continue - } - parts := strings.Split(record.LogicalID, "/") - expectedKey := "" - if len(parts) == 2 { - expectedKey, _ = volumeRecordKey(c.cfg.State.Prefix, parts[0], parts[1]) - } - if expectedKey == "" || key != expectedKey { - return nil, fmt.Errorf("volume record %s logical_id does not match its key", key) - } - if record.DesiredSizeBytes <= 0 || record.ResizeGeneration <= 0 { - return nil, fmt.Errorf("volume record %s has invalid size or generation", key) - } - if record.AppliedSizeBytes < 0 { - return nil, fmt.Errorf("volume record %s has negative applied size", key) - } - if record.ResizeState != VolumeResizePending && record.ResizeState != VolumeResizeApplied && record.ResizeState != VolumeResizeFailed { - return nil, fmt.Errorf("volume record %s has invalid resize state %q", key, record.ResizeState) - } - if record.ResizeState == VolumeResizeApplied && record.AppliedSizeBytes != record.DesiredSizeBytes { - return nil, fmt.Errorf("volume record %s has applied state with mismatched size", key) - } - if record.Type == config.VolumeTypeLocal && record.BoundNode == "" { - return nil, fmt.Errorf("volume record %s is missing bound_node", key) - } else if record.Type == config.VolumeTypeShared && record.SharedBackendID == "" { - return nil, fmt.Errorf("volume record %s is missing shared_backend_id", key) - } else if record.Type != config.VolumeTypeLocal && record.Type != config.VolumeTypeShared { - return nil, fmt.Errorf("volume record %s has invalid type %q", key, record.Type) - } - records[record.LogicalID] = storedVolumeRecord{Record: record, Token: token} + // The evolving reservation total. Checking each raise against a total + // computed once before the loop is wrong: two individually feasible raises + // can both be admitted while their combined reservation exceeds the pool. + localByNode := make(map[string]int64) + for node, size := range storageReservations(set).LocalByNode { + localByNode[node] = size } - return records, nil -} -func (c *Controller) applyExistingVolumeRecords(ctx context.Context, services []config.ServiceConfig, records map[string]storedVolumeRecord) error { - for si := range services { - for vi := range services[si].Volumes { - volume := &services[si].Volumes[vi] - logicalID := services[si].Name + "/" + volume.Name - stored, exists := records[logicalID] + // Deterministic evaluation order — services then volumes, by name — so the + // same desired revision admits the same subset on every controller and + // after every leader change. + for _, si := range orderedServiceIndexes(services) { + service := &services[si] + for _, vi := range orderedVolumeIndexes(service.Volumes) { + volume := &service.Volumes[vi] + logicalID := service.Name + "/" + volume.Name + if quarantine, blocked := set.quarantineFor(logicalID); blocked { + admission.Held[service.Name] = scheduler.ReasonVolumeRecordInvalid + c.logger.Warn("holding service with an unreadable volume record", + "service", service.Name, "volume", volume.Name, + "tier", quarantine.Tier, "reason", quarantine.Reason) + continue + } + stored, exists := set.Records[logicalID] if !exists { volume.ResizeGeneration = 1 continue } if stored.Record.Type != volume.Type { - return fmt.Errorf("volume %s: type is immutable (stored %s, desired %s)", logicalID, stored.Record.Type, volume.Type) - } - if stored.Record.Type == config.VolumeTypeLocal && stored.Record.BoundNode == "" { - return fmt.Errorf("volume %s: retained local record is missing bound_node", logicalID) - } - if stored.Record.Type == config.VolumeTypeShared && stored.Record.SharedBackendID == "" { - return fmt.Errorf("volume %s: retained shared record is missing shared_backend_id", logicalID) + // A desired-configuration conflict, not a record fault. It is + // held rather than returned as an error, because failing the + // whole reconcile would stop scheduling for the cluster. + admission.Held[service.Name] = "volume_type_immutable" + c.logger.Warn("holding service whose volume type changed", + "service", service.Name, "volume", volume.Name, + "stored", stored.Record.Type, "desired", volume.Type) + continue } volume.BoundNode = stored.Record.BoundNode volume.SharedBackendID = stored.Record.SharedBackendID volume.ResizeGeneration = stored.Record.ResizeGeneration - if stored.Record.DesiredSizeBytes == volume.SizeBytes { - continue - } - updated := stored.Record - updated.DesiredSizeBytes = volume.SizeBytes - updated.ResizeGeneration++ - updated.ResizeState = VolumeResizePending - updated.LastError = "" - updated.UpdatedAt = time.Now().UTC() - ok, token, err := c.store.PutJSONIfMatch(ctx, mustVolumeRecordKey(c.cfg.State.Prefix, services[si].Name, volume.Name), stored.Token, updated) + + updated, changed, err := c.admitVolumeSize(ctx, service.Name, volume, stored, nodeByID, localByNode) if err != nil { - return err + return admission, err } - if !ok { - return fmt.Errorf("volume %s changed concurrently; retry reconciliation", logicalID) + if changed { + set.Records[logicalID] = updated } - volume.ResizeGeneration = updated.ResizeGeneration - records[logicalID] = storedVolumeRecord{Record: updated, Token: token} } } - return nil + return admission, nil +} + +// admitVolumeSize decides what the agent is told to run for one volume, and +// records the decision durably. It returns the record as it now stands and +// whether it was rewritten. +func (c *Controller) admitVolumeSize( + ctx context.Context, + serviceName string, + volume *config.VolumeConfig, + stored storedVolumeRecord, + nodeByID map[string]scheduler.Node, + localByNode map[string]int64, +) (storedVolumeRecord, bool, error) { + record := stored.Record + requested := volume.SizeBytes + + // A state this controller does not recognize belongs to a newer resize + // protocol that is presumably mid-flight. Tolerating it in the parse + // (so it does not brick scheduling) is only half the rule: advancing the + // generation and resetting the state to pending here would destroy the + // very state the tolerance exists to preserve. Render what the record + // says and write nothing. + if !knownVolumeResizeState(record.ResizeState) { + volume.SizeBytes = record.DesiredSizeBytes + volume.ResizeGeneration = record.ResizeGeneration + return stored, false, nil + } + + // A standing rejection for exactly this request. The match key is + // RequestedSizeBytes, not DesiredSizeBytes: after a rejection + // DesiredSizeBytes holds the *effective* size, so comparing against it + // makes the unchanged request look new on every tick and mints a + // generation forever. + if record.rejectionStands() && requested == record.RequestedSizeBytes { + volume.SizeBytes = record.DesiredSizeBytes + volume.ResizeGeneration = record.ResizeGeneration + available := c.availableLocalBytes(record, nodeByID, localByNode) + if record.RejectedAvailableBytes == available { + // Nothing about the rejection changed, so nothing is written and + // the records digest stays stable. + return stored, false, nil + } + record.RejectedAvailableBytes = available + return c.putVolumeRecord(ctx, serviceName, volume.Name, stored.Token, record) + } + + if record.DesiredSizeBytes == requested { + // The request matches the effective size. Any standing rejection was + // for a different size and is cleared — without minting a generation, + // because there is no resize to perform. + if !record.clearRejection() { + return stored, false, nil + } + return c.putVolumeRecord(ctx, serviceName, volume.Name, stored.Token, record) + } + + reason, available := c.admitLocalRaise(record, requested, nodeByID, localByNode) + if reason != "" { + // Clamp the rendered configuration to the last accepted size so the + // service keeps running at a size the cluster is actually able to + // serve, and record the refusal for status. + volume.SizeBytes = record.DesiredSizeBytes + volume.ResizeGeneration = record.ResizeGeneration + if record.RequestedSizeBytes == requested && record.RejectedReason == reason && record.RejectedAvailableBytes == available { + return stored, false, nil + } + if !record.rejectionStands() || record.RequestedSizeBytes != requested { + // First refusal of this request: stamp the time once. It is + // preserved on later ticks so a standing rejection produces no + // further writes. + record.RejectedAt = time.Now().UTC() + } + record.RequestedSizeBytes = requested + record.RejectedReason = reason + record.RejectedAvailableBytes = available + c.logger.Warn("refused a volume size request", + "service", serviceName, "volume", volume.Name, + "requested_bytes", requested, "effective_bytes", record.DesiredSizeBytes, "reason", reason) + return c.putVolumeRecord(ctx, serviceName, volume.Name, stored.Token, record) + } + + // Accepted. Fold the new contribution into the evolving total before the + // next raise is evaluated. + if record.Type == config.VolumeTypeLocal && record.BoundNode != "" { + localByNode[record.BoundNode] += recordContribution(requested, record.AppliedSizeBytes) - + recordContribution(record.DesiredSizeBytes, record.AppliedSizeBytes) + } + record.clearRejection() + record.DesiredSizeBytes = requested + record.ResizeGeneration++ + record.ResizeState = VolumeResizePending + record.LastError = "" + updated, _, err := c.putVolumeRecord(ctx, serviceName, volume.Name, stored.Token, record) + if err != nil { + return stored, false, err + } + volume.ResizeGeneration = updated.Record.ResizeGeneration + return updated, true, nil +} + +// admitLocalRaise applies the replacement arithmetic that keeps a batch of +// raises inside one pool. It returns an empty reason when the raise is +// accepted. +// +// Shared volumes are admitted here because shared execution is gated off +// upstream (`shared_volume_runtime_unavailable`), so no shared reservation is +// ever handed to a running workload. +func (c *Controller) admitLocalRaise(record VolumeRecord, requested int64, nodeByID map[string]scheduler.Node, localByNode map[string]int64) (string, int64) { + if record.Type != config.VolumeTypeLocal || record.BoundNode == "" { + return "", 0 + } + // A request that does not increase the record's contribution cannot + // over-commit the pool, so it is not subject to admission at all. This is + // not just an optimization: because the contribution is + // max(size, applied), a shrink leaves it unchanged until the shrink + // actually applies — so checking it against a pool that is *already* over + // capacity refuses the one operation that would restore the pool. The same + // reasoning covers a node that is not currently observable: there is + // nothing to verify when nothing new is being claimed. + if recordContribution(requested, record.AppliedSizeBytes) <= + recordContribution(record.DesiredSizeBytes, record.AppliedSizeBytes) { + return "", 0 + } + node, active := nodeByID[record.BoundNode] + if !active { + // The node's capacity is not observable, and a node being absent is + // correlated with the node being in trouble — exactly when adopting an + // unverifiable larger reservation is worst. Only the raise waits; the + // existing effective size keeps rendering. + return scheduler.ReasonStorageCapacityUnknown, 0 + } + total := localByNode[record.BoundNode] + old := recordContribution(record.DesiredSizeBytes, record.AppliedSizeBytes) + fresh := recordContribution(requested, record.AppliedSizeBytes) + // Subtract before adding so a large existing contribution cannot make the + // running total transiently overflow. + candidate := total - old + if fresh > 0 && candidate > (1<<63-1)-fresh { + return scheduler.ReasonNodeStorageExhausted, node.LocalCapacityBytes + } + candidate += fresh + if candidate > node.LocalCapacityBytes { + // Headroom, floored at zero: a pool that is already over-committed + // would otherwise report a negative "available", which reads as a + // bug rather than as "there is none". + available := node.LocalCapacityBytes - (total - old) + if available < 0 { + available = 0 + } + return scheduler.ReasonNodeStorageExhausted, available + } + return "", 0 +} + +// availableLocalBytes recomputes the capacity figure a standing rejection was +// measured against, so a changed pool is reflected without restamping the time. +func (c *Controller) availableLocalBytes(record VolumeRecord, nodeByID map[string]scheduler.Node, localByNode map[string]int64) int64 { + if record.Type != config.VolumeTypeLocal || record.BoundNode == "" { + return 0 + } + node, active := nodeByID[record.BoundNode] + if !active { + return 0 + } + return node.LocalCapacityBytes - (localByNode[record.BoundNode] - recordContribution(record.DesiredSizeBytes, record.AppliedSizeBytes)) +} + +// recordContribution is what storageReservations charges for one record. +func recordContribution(desired, applied int64) int64 { + return max64(max64(desired, 0), max64(applied, 0)) +} + +func (c *Controller) putVolumeRecord(ctx context.Context, service, volume string, token objectstorage.WriteToken, record VolumeRecord) (storedVolumeRecord, bool, error) { + record.UpdatedAt = time.Now().UTC() + ok, newToken, err := c.store.PutJSONIfMatch(ctx, mustVolumeRecordKey(c.cfg.State.Prefix, service, volume), token, record) + if err != nil { + return storedVolumeRecord{}, false, err + } + if !ok { + return storedVolumeRecord{}, false, fmt.Errorf("volume %s/%s changed concurrently; retry reconciliation", service, volume) + } + return storedVolumeRecord{Record: record, Token: newToken}, true, nil +} + +func orderedServiceIndexes(services []config.ServiceConfig) []int { + indexes := make([]int, len(services)) + for i := range services { + indexes[i] = i + } + sort.Slice(indexes, func(i, j int) bool { return services[indexes[i]].Name < services[indexes[j]].Name }) + return indexes } -func (c *Controller) createAssignedVolumeRecords(ctx context.Context, nodeConfigs []config.NodeConfig, records map[string]storedVolumeRecord) error { +func orderedVolumeIndexes(volumes []config.VolumeConfig) []int { + indexes := make([]int, len(volumes)) + for i := range volumes { + indexes[i] = i + } + sort.Slice(indexes, func(i, j int) bool { return volumes[indexes[i]].Name < volumes[indexes[j]].Name }) + return indexes +} + +func (c *Controller) createAssignedVolumeRecords(ctx context.Context, nodeConfigs []config.NodeConfig, set volumeRecordSet) error { now := time.Now().UTC() for _, node := range nodeConfigs { for _, service := range node.Services { for _, volume := range service.Volumes { logicalID := service.Name + "/" + volume.Name - if _, exists := records[logicalID]; exists { + if _, exists := set.Records[logicalID]; exists { + continue + } + // A quarantined key already has an object. Creating a fresh + // record over it would overwrite state that has not been read. + if _, quarantined := set.Quarantined[logicalID]; quarantined { continue } record := VolumeRecord{ @@ -139,24 +333,35 @@ func (c *Controller) createAssignedVolumeRecords(ctx context.Context, nodeConfig if !ok { return fmt.Errorf("volume %s was created concurrently; retry reconciliation", logicalID) } - records[logicalID] = storedVolumeRecord{Record: record, Token: token} + set.Records[logicalID] = storedVolumeRecord{Record: record, Token: token} } } } return nil } -func storageReservations(records map[string]storedVolumeRecord) scheduler.StorageReservations { +func storageReservations(set volumeRecordSet) scheduler.StorageReservations { reservations := scheduler.StorageReservations{ LocalByNode: make(map[string]int64), SharedByBackend: make(map[string]int64), - RecordedLogicalIDs: make(map[string]bool, len(records)), SharedEnabled: false, + RecordedLogicalIDs: make(map[string]bool, len(set.Records)+len(set.Quarantined)), + LocalUnknownByNode: make(map[string]bool), + SharedUnknownByBackend: make(map[string]bool), + UnknownCapacityKeys: make(map[string]string), + SharedEnabled: false, } - for id, stored := range records { - record := stored.Record - size := record.DesiredSizeBytes - if record.AppliedSizeBytes > size { - size = record.AppliedSizeBytes + // One quarantined key per blocked scope, chosen deterministically so the + // reported repair target does not change between controllers or leaders. + nameTarget := func(scope, key string) { + if scope == "" { + return } + if existing, ok := reservations.UnknownCapacityKeys[scope]; !ok || key < existing { + reservations.UnknownCapacityKeys[scope] = key + } + } + for id, stored := range set.Records { + record := stored.Record + size := recordContribution(record.DesiredSizeBytes, record.AppliedSizeBytes) reservations.RecordedLogicalIDs[id] = true if record.Type == config.VolumeTypeLocal { reservations.LocalByNode[record.BoundNode] += size @@ -164,15 +369,86 @@ func storageReservations(records map[string]storedVolumeRecord) scheduler.Storag reservations.SharedByBackend[record.SharedBackendID] += size } } + // A quarantined record still holds capacity. Dropping it would silently + // release the reservation and turn a hard failure into over-commit, which + // is the failure mode the admission check exists to prevent. So each tier + // charges what it can prove and flags the scope it cannot. + for id, quarantine := range set.Quarantined { + // Marked recorded so the owner's volume never contributes a *second* + // delta on top of the reservation charged here. + reservations.RecordedLogicalIDs[id] = true + switch quarantine.Tier { + case quarantineTierExact, quarantineTierPartial: + switch quarantine.Class { + case config.VolumeTypeLocal: + reservations.LocalByNode[quarantine.BoundNode] += quarantine.ReservedBytes + if quarantine.Tier == quarantineTierPartial { + reservations.LocalUnknownByNode[quarantine.BoundNode] = true + nameTarget(quarantine.BoundNode, id) + } + case config.VolumeTypeShared: + reservations.SharedByBackend[quarantine.SharedBackendID] += quarantine.ReservedBytes + if quarantine.Tier == quarantineTierPartial { + reservations.SharedUnknownByBackend[quarantine.SharedBackendID] = true + nameTarget(quarantine.SharedBackendID, id) + } + } + default: + // No binding, so there is no account to charge. The block widens to + // the class if the type parsed, and to both classes if it did not — + // the key encodes only service and volume names, never the class. + switch quarantine.Class { + case config.VolumeTypeLocal: + reservations.LocalClassUnknown = true + nameTarget(string(config.VolumeTypeLocal), id) + case config.VolumeTypeShared: + reservations.SharedClassUnknown = true + nameTarget(string(config.VolumeTypeShared), id) + default: + reservations.LocalClassUnknown = true + reservations.SharedClassUnknown = true + nameTarget(string(config.VolumeTypeLocal), id) + nameTarget(string(config.VolumeTypeShared), id) + } + } + } return reservations } -func volumeRecordsDigest(records map[string]storedVolumeRecord) string { - ordered := make([]VolumeRecord, 0, len(records)) - for _, stored := range records { - ordered = append(ordered, stored.Record) +// digestEntry is the normalized scheduling-visible outcome for one record key. +// +// Quarantined objects have no valid VolumeRecord, so hashing only the valid +// ones would leave the scheduling signature unchanged across transitions that +// must trigger re-placement — a partial repair that narrows a block, a changed +// binding that moves which node is blocked, or the full repair an operator is +// actively waiting on. +// +// Reason is deliberately excluded: it is display text, and hashing it would +// make a reworded error message invalidate the signature cache. +type digestEntry struct { + Key string `json:"key"` + Record *VolumeRecord `json:"record,omitempty"` + Tier int `json:"tier,omitempty"` + BoundNode string `json:"bound_node,omitempty"` + SharedBackendID string `json:"shared_backend_id,omitempty"` + Class config.VolumeType `json:"class,omitempty"` + ReservedBytes int64 `json:"reserved_bytes,omitempty"` +} + +func volumeRecordsDigest(set volumeRecordSet) string { + ordered := make([]digestEntry, 0, len(set.Records)+len(set.Quarantined)) + for id, stored := range set.Records { + record := stored.Record + ordered = append(ordered, digestEntry{Key: id, Record: &record}) + } + for id, quarantine := range set.Quarantined { + ordered = append(ordered, digestEntry{ + Key: id, Tier: quarantine.Tier, BoundNode: quarantine.BoundNode, + SharedBackendID: quarantine.SharedBackendID, Class: quarantine.Class, + ReservedBytes: quarantine.ReservedBytes, + }) } - sort.Slice(ordered, func(i, j int) bool { return ordered[i].LogicalID < ordered[j].LogicalID }) + sort.Slice(ordered, func(i, j int) bool { return ordered[i].Key < ordered[j].Key }) data, _ := json.Marshal(ordered) sum := sha256.Sum256(data) return hex.EncodeToString(sum[:]) @@ -191,7 +467,7 @@ func (c *Controller) acknowledgeVolumeRecords(ctx context.Context) error { } for _, service := range node.AgentStatus.Services { for _, observed := range service.Volumes { - if observed.State != "prepared" && observed.State != "error" { + if observed.State != "prepared" && observed.State != "error" && observed.State != "rejected" { continue } parts := strings.Split(observed.LogicalID, "/") @@ -204,6 +480,12 @@ func (c *Controller) acknowledgeVolumeRecords(ctx context.Context) error { if err != nil || !exists || observed.ResizeGeneration != record.ResizeGeneration { continue } + // A state written by a newer control plane is carried through + // untouched: an older controller must not advance a state + // machine it does not understand. + if !knownVolumeResizeState(record.ResizeState) { + continue + } if observed.Type != string(record.Type) { continue } @@ -230,6 +512,48 @@ func (c *Controller) acknowledgeVolumeRecords(ctx context.Context) error { record.AppliedSizeBytes = observed.AppliedSizeBytes record.ResizeState = VolumeResizeApplied record.LastError = "" + case "rejected": + // The generation alone does not say the refusal is still + // outstanding. A record whose refusal was withdrawn sits at + // the same generation, so a stale heartbeat would reopen it + // — and the next desired-state pass clears it again, two + // durable writes every tick, with a crash between them + // leaving the degraded state behind. Accept the + // observation only while the record is still refusing, or + // while it already records this same refusal. + if !record.rejectionStands() && record.ResizeState != VolumeResizeRejected { + if record.DesiredSizeBytes != observed.RequestedSizeBytes { + continue + } + } + // The agent refused this size. Converge the record on the + // effective size in one write: RequestedSizeBytes keeps the + // refused size for display, DesiredSizeBytes becomes what + // is actually running, and the generation is left alone so + // the rejection stays keyed to this one request. + // + // After this write DesiredSizeBytes means "effective" for + // both rejection kinds, which is what lets one clamp serve + // both. The invariant is that the record never renders a + // size the cluster is not running. + if observed.AppliedSizeBytes <= 0 || observed.RequestedSizeBytes <= 0 { + continue + } + if record.ResizeState == VolumeResizeRejected && + record.DesiredSizeBytes == observed.AppliedSizeBytes && + record.AppliedSizeBytes == observed.AppliedSizeBytes && + record.RequestedSizeBytes == observed.RequestedSizeBytes { + continue + } + if record.RejectedAt.IsZero() || record.RequestedSizeBytes != observed.RequestedSizeBytes { + record.RejectedAt = time.Now().UTC() + } + record.RequestedSizeBytes = observed.RequestedSizeBytes + record.DesiredSizeBytes = observed.AppliedSizeBytes + record.AppliedSizeBytes = observed.AppliedSizeBytes + record.ResizeState = VolumeResizeRejected + record.RejectedReason = observed.RejectedReason + record.LastError = statusmodel.BoundedMessage(observed.LastError) case "error": if record.ResizeState == VolumeResizeFailed && record.LastError == statusmodel.BoundedMessage(observed.LastError) { continue diff --git a/internal/controlplane/volume_records_test.go b/internal/controlplane/volume_records_test.go index fb96656..210a956 100644 --- a/internal/controlplane/volume_records_test.go +++ b/internal/controlplane/volume_records_test.go @@ -8,6 +8,7 @@ import ( "time" "github.com/artemnikitin/firework/internal/config" + "github.com/artemnikitin/firework/internal/scheduler" "github.com/artemnikitin/firework/internal/statusmodel" ) @@ -31,14 +32,15 @@ func TestVolumeRecordsRetainBindingAndAdvanceResizeGeneration(t *testing.T) { services := []config.ServiceConfig{{Name: "db", Volumes: []config.VolumeConfig{{ Name: "data", Type: config.VolumeTypeLocal, MountPath: "/data", SizeBytes: 20 * config.GiB, }}}} - if err := controller.applyExistingVolumeRecords(ctx, services, records); err != nil { + nodes := []scheduler.Node{{InstanceID: "node-1", CapacityVCPUs: 8, CapacityMemMB: 8192, LocalCapacityBytes: 100 * config.GiB}} + if _, err := controller.applyExistingVolumeRecords(ctx, services, records, nodes); err != nil { t.Fatal(err) } volume := services[0].Volumes[0] if volume.BoundNode != "node-1" || volume.ResizeGeneration != 2 { t.Fatalf("resolved volume = %#v", volume) } - stored := records["db/data"].Record + stored := records.Records["db/data"].Record if stored.DesiredSizeBytes != 20*config.GiB || stored.AppliedSizeBytes != 10*config.GiB || stored.ResizeState != VolumeResizePending { t.Fatalf("stored volume = %#v", stored) } @@ -52,14 +54,14 @@ func TestCreateAssignedVolumeRecordUsesSchedulerBinding(t *testing.T) { ctx := context.Background() store := newBlobStateStore(newMemBlob()) controller := NewController(Config{State: StateConfig{Prefix: "cp/v1/"}}, store, slog.New(slog.NewTextHandler(io.Discard, nil))) - records := make(map[string]storedVolumeRecord) + records := volumeRecordSet{Records: make(map[string]storedVolumeRecord), Quarantined: map[string]volumeQuarantine{}} nodes := []config.NodeConfig{{Node: "node-1", Services: []config.ServiceConfig{{Name: "db", Volumes: []config.VolumeConfig{{ Name: "data", Type: config.VolumeTypeLocal, MountPath: "/data", SizeBytes: config.GiB, BoundNode: "node-1", ResizeGeneration: 1, }}}}}} if err := controller.createAssignedVolumeRecords(ctx, nodes, records); err != nil { t.Fatal(err) } - if got := records["db/data"].Record; got.BoundNode != "node-1" || got.ResizeState != VolumeResizePending { + if got := records.Records["db/data"].Record; got.BoundNode != "node-1" || got.ResizeState != VolumeResizePending { t.Fatalf("created record = %#v", got) } } diff --git a/internal/controlplane/web/app.js b/internal/controlplane/web/app.js index ef8683c..e226c69 100644 --- a/internal/controlplane/web/app.js +++ b/internal/controlplane/web/app.js @@ -337,15 +337,20 @@ function renderServiceDetail(service) { ${esc(port.VMPort ?? port.vm_port)} `); + // Requested is what the repo asked for and effective is what the cluster + // accepted. They differ only when a size request was refused, and that is + // precisely the case an operator cannot diagnose from the effective size + // alone — so the refusal is shown inline rather than left to a revision diff. const volumeRows = (service.volumes || []).map(volume => ` ${esc(volume.logical_id)} ${badge(volume.type)} ${esc(volume.mount_path)} ${nodeLink(volume.bound_node)} ${display(volume.shared_backend_id)} + ${esc(formatBytes(volume.requested_size_bytes || volume.desired_size_bytes))} ${esc(formatBytes(volume.desired_size_bytes))} ${esc(formatBytes(volume.applied_size_bytes))} - ${badge(volume.state)} + ${badge(volume.state)}${volume.rejected ? ` ${badge(volume.rejected_reason || 'rejected')}` : ''} ${display(volume.last_error)} `); @@ -357,7 +362,7 @@ function renderServiceDetail(service) { ${section('Revisions', revisions)} ${(service.port_forwards || []).length ? section('Port forwards', table(['Host port', 'VM port'], portRows, '')) : ''} - ${(service.volumes || []).length ? section('Persistent volumes', table(['Volume', 'Type', 'Mount path', 'Bound node', 'Backend', 'Desired', 'Applied', 'State', 'Last error'], volumeRows, '')) : ''}`; + ${(service.volumes || []).length ? section('Persistent volumes', table(['Volume', 'Type', 'Mount path', 'Bound node', 'Backend', 'Requested', 'Effective', 'Applied', 'State', 'Last error'], volumeRows, '')) : ''}`; } async function detail(kind, id) { diff --git a/internal/reconciler/errors.go b/internal/reconciler/errors.go index 9e23377..0f78984 100644 --- a/internal/reconciler/errors.go +++ b/internal/reconciler/errors.go @@ -1,6 +1,10 @@ package reconciler -import "fmt" +import ( + "fmt" + + "github.com/artemnikitin/firework/internal/vm" +) // FailureStage identifies the blocking host stage that prevented convergence. // It is intentionally a small, stable set so agents can publish bounded @@ -53,3 +57,51 @@ func HasFailureStage(err error, stage FailureStage) bool { } return false } + +// IsIncomplete reports whether a reconciliation error consists *only* of benign +// start-barrier races — a start that was aborted by a concurrent stop or +// remove, or one that collided with another start still preparing volumes. +// +// The distinction matters because of what an ordinary nil return would do. The +// agent advances lastRevision at the end of a successful tick, and the next +// tick then takes the unchanged-revision shortcut and never re-plans, leaving +// an aborted service down until the revision itself changes. So an aborted +// start must neither succeed nor be reported as a hard failure: it is +// incomplete, and the caller retries on the next tick without claiming the +// revision or raising a reconcile_failed condition. +// +// A batch that mixes an abort with a genuine failure is a failure. Both +// classifications leave the revision unchanged; the difference is what the node +// reports. +func IsIncomplete(err error) bool { + leaves := reconcileLeaves(err, nil) + if len(leaves) == 0 { + return false + } + for _, leaf := range leaves { + if !vm.IsStartRace(leaf) { + return false + } + } + return true +} + +// reconcileLeaves flattens an aggregate error into the individual errors it was +// built from. Joined branches are walked; a plain wrapped chain is followed to +// its innermost error, which is where a sentinel lives. +func reconcileLeaves(err error, out []error) []error { + for err != nil { + if joined, ok := err.(interface{ Unwrap() []error }); ok { + for _, child := range joined.Unwrap() { + out = reconcileLeaves(child, out) + } + return out + } + wrapped, ok := err.(interface{ Unwrap() error }) + if !ok || wrapped.Unwrap() == nil { + return append(out, err) + } + err = wrapped.Unwrap() + } + return out +} diff --git a/internal/reconciler/errors_test.go b/internal/reconciler/errors_test.go index 6eb572c..4a2ebd4 100644 --- a/internal/reconciler/errors_test.go +++ b/internal/reconciler/errors_test.go @@ -4,6 +4,8 @@ import ( "errors" "fmt" "testing" + + "github.com/artemnikitin/firework/internal/vm" ) func TestHasFailureStageFindsWrappedAndJoinedStages(t *testing.T) { @@ -18,3 +20,37 @@ func TestHasFailureStageFindsWrappedAndJoinedStages(t *testing.T) { t.Fatal("VM stage was lost through aggregate error") } } + +// The abort must survive the exact wrapping the apply path performs. A +// hand-built error proves nothing: the defect this guards against is a link in +// that chain flattening the error with %v. +func TestIsIncompleteSeesThroughTheProductionErrorShape(t *testing.T) { + abort := stageError(FailureStageVM, + fmt.Errorf("starting VM: %w", fmt.Errorf("service app: %w", vm.ErrStartAborted))) + inProgress := stageError(FailureStageVM, + fmt.Errorf("starting VM: %w", fmt.Errorf("service api is in state starting: %w", vm.ErrStartInProgress))) + genuine := stageError(FailureStageNetwork, errors.New("tap creation failed")) + + wrapApply := func(errs ...error) error { + return combineErrors([]error{fmt.Errorf("reconciliation had %d error(s): %w", len(errs), errors.Join(errs...))}) + } + + tests := []struct { + name string + err error + want bool + }{ + {name: "nil is not incomplete", err: nil, want: false}, + {name: "single abort", err: wrapApply(abort), want: true}, + {name: "abort plus concurrent start", err: wrapApply(abort, inProgress), want: true}, + {name: "abort mixed with a genuine failure", err: wrapApply(abort, genuine), want: false}, + {name: "genuine failure alone", err: wrapApply(genuine), want: false}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if got := IsIncomplete(test.err); got != test.want { + t.Fatalf("IsIncomplete = %v, want %v (err: %v)", got, test.want, test.err) + } + }) + } +} diff --git a/internal/reconciler/reconciler.go b/internal/reconciler/reconciler.go index 1efc87f..8bf1e03 100644 --- a/internal/reconciler/reconciler.go +++ b/internal/reconciler/reconciler.go @@ -18,6 +18,7 @@ import ( "github.com/artemnikitin/firework/internal/healthcheck" "github.com/artemnikitin/firework/internal/network" "github.com/artemnikitin/firework/internal/vm" + "github.com/artemnikitin/firework/internal/volume" ) // Action represents a reconciliation action the agent needs to take. @@ -44,7 +45,7 @@ type VMManager interface { } type volumePreflighter interface { - Preflight(context.Context, config.ServiceConfig) error + Preflight(context.Context, config.ServiceConfig) ([]volume.Rejection, error) } type vmRecoverer interface { @@ -552,11 +553,15 @@ func (r *Reconciler) applyAllAtOnce(ctx context.Context, actions []Action) error case ActionUpdate: r.logger.Info("updating service (stop + start)", "service", action.Service.Name) - if err := r.preflight(ctx, action.Service); err != nil { + rejections, err := r.preflight(ctx, action.Service) + if err != nil { r.logger.Error("volume preflight failed; keeping current VM running", "service", action.Service.Name, "error", err) errs = append(errs, stageError(FailureStageVM, fmt.Errorf("preflight update %s: %w", action.Service.Name, err))) continue } + if r.settleRejections(&action, rejections) { + continue + } prev := action.Service if action.PreviousService != nil { prev = *action.PreviousService @@ -623,11 +628,15 @@ func (r *Reconciler) applyRolling(ctx context.Context, actions []Action) error { for i, action := range updates { r.logger.Info("updating service (stop + start)", "service", action.Service.Name) - if err := r.preflight(ctx, action.Service); err != nil { + rejections, err := r.preflight(ctx, action.Service) + if err != nil { r.logger.Error("volume preflight failed; keeping current VM running", "service", action.Service.Name, "error", err) errs = append(errs, stageError(FailureStageVM, fmt.Errorf("preflight update %s: %w", action.Service.Name, err))) break } + if r.settleRejections(&action, rejections) { + continue + } prev := action.Service if action.PreviousService != nil { prev = *action.PreviousService @@ -657,11 +666,71 @@ func (r *Reconciler) applyRolling(ctx context.Context, actions []Action) error { return nil } -func (r *Reconciler) preflight(ctx context.Context, svc config.ServiceConfig) error { +func (r *Reconciler) preflight(ctx context.Context, svc config.ServiceConfig) ([]volume.Rejection, error) { if manager, ok := r.vmManager.(volumePreflighter); ok { return manager.Preflight(ctx, svc) } - return nil + return nil, nil +} + +// settleRejections applies a preflight refusal to a planned update and reports +// whether the update is now a no-op. +// +// This is what makes an advisory preflight rejection *terminal*. The VM is +// still live at this point, and once the clamp lands the desired configuration +// no longer differs from the running one, so no further update is planned — +// not merely no further failure. Without it the measurement repeats on every +// tick forever. +func (r *Reconciler) settleRejections(action *Action, rejections []volume.Rejection) bool { + if !clampRejected(&action.Service, rejections) { + return false + } + instance := r.vmManager.List()[action.Service.Name] + if instance == nil || needsUpdate(instance, action.Service) { + // Something else about the service still differs — an image change, + // for instance — so the update proceeds, now carrying the effective + // volume size and performing no resize. + return false + } + r.logger.Info("volume size request refused; keeping the effective size and skipping the update", + "service", action.Service.Name) + return true +} + +// clampRejected substitutes the effective size for every volume the preflight +// refused, and reports whether anything changed. +// +// The refusal is applied by clamping rather than by failing the update, so an +// update that changes the image *and* requests a refused shrink still deploys +// the image. Failing the preflight would wedge every unrelated change behind a +// size the node will never accept. +func clampRejected(svc *config.ServiceConfig, rejections []volume.Rejection) bool { + if len(rejections) == 0 { + return false + } + byID := make(map[string]volume.Rejection, len(rejections)) + for _, rejection := range rejections { + byID[rejection.LogicalID] = rejection + } + clamped := false + for i := range svc.Volumes { + rejection, ok := byID[svc.Name+"/"+svc.Volumes[i].Name] + if !ok { + continue + } + if svc.Volumes[i].SizeBytes == rejection.AppliedSizeBytes && + svc.Volumes[i].ResizeGeneration == rejection.AppliedGeneration { + continue + } + // Substitute the whole effective configuration. Clamping only the size + // leaves the generation differing forever, and needsUpdate compares + // whole volume configs — so the update would be re-planned on every + // tick, which is worse than the single failure this replaces. + svc.Volumes[i].SizeBytes = rejection.AppliedSizeBytes + svc.Volumes[i].ResizeGeneration = rejection.AppliedGeneration + clamped = true + } + return clamped } // Reconcile is a convenience method that plans and applies in one step. diff --git a/internal/reconciler/shrink_rejection_test.go b/internal/reconciler/shrink_rejection_test.go new file mode 100644 index 0000000..aaeeb8d --- /dev/null +++ b/internal/reconciler/shrink_rejection_test.go @@ -0,0 +1,124 @@ +package reconciler + +import ( + "context" + "io" + "log/slog" + "testing" + + "github.com/artemnikitin/firework/internal/config" + "github.com/artemnikitin/firework/internal/vm" + "github.com/artemnikitin/firework/internal/volume" +) + +// rejectingVMManager refuses one volume's shrink at preflight, the way the +// advisory pre-stop measurement does. +type rejectingVMManager struct { + *fakeVMManager + rejections []volume.Rejection + preflightCalls int +} + +func (f *rejectingVMManager) Preflight(context.Context, config.ServiceConfig) ([]volume.Rejection, error) { + f.preflightCalls++ + return f.rejections, nil +} + +func volumeService(name string, size int64, generation int64) config.ServiceConfig { + return config.ServiceConfig{ + Name: name, Image: "/image", Kernel: "/kernel", VCPUs: 1, MemoryMB: 128, + Volumes: []config.VolumeConfig{{ + Name: "data", Type: config.VolumeTypeLocal, MountPath: "/data", + SizeBytes: size, BoundNode: "node-1", ResizeGeneration: generation, + }}, + } +} + +func rejectingReconciler(t *testing.T, running config.ServiceConfig, applied int64) (*Reconciler, *rejectingVMManager) { + t.Helper() + manager := &rejectingVMManager{ + fakeVMManager: newFakeVMManager(), + rejections: []volume.Rejection{{ + LogicalID: running.Name + "/data", ResizeGeneration: 2, AppliedGeneration: 1, + RequestedSizeBytes: 2 * config.MiB, AppliedSizeBytes: applied, MinimumSizeBytes: 4 * config.MiB, + }}, + } + manager.instances[running.Name] = &vm.Instance{Name: running.Name, State: vm.StateRunning, Config: running} + r := New(manager, slog.New(slog.NewTextHandler(io.Discard, nil)), nil, nil, "", 0).WithStateDir(t.TempDir()) + return r, manager +} + +// A preflight refusal is advisory and the VM is still live, so it must be made +// terminal by clamping rather than by failing: the desired configuration stops +// differing from the running one, so no further update is planned. Failing it +// instead would re-measure on every tick forever. +func TestPreflightRejectionIsTerminalAndLeavesTheVMRunning(t *testing.T) { + for _, strategy := range []string{"", "rolling"} { + name := strategy + if name == "" { + name = "all-at-once" + } + t.Run(name, func(t *testing.T) { + running := volumeService("app", 16*config.MiB, 1) + r, manager := rejectingReconciler(t, running, 16*config.MiB) + r.updateStrategy = strategy + + // The desired revision asks for a shrink the node will refuse. + desired := config.NodeConfig{Node: "node-1", Services: []config.ServiceConfig{ + volumeService("app", 2*config.MiB, 2), + }} + + if err := r.Reconcile(context.Background(), desired); err != nil { + t.Fatalf("a refused shrink must not fail reconciliation: %v", err) + } + if len(manager.removeCalls) != 0 { + t.Fatalf("the VM was stopped for a refused shrink: %v", manager.removeCalls) + } + if len(manager.startCalls) != 0 { + t.Fatalf("the VM was restarted for a refused shrink: %v", manager.startCalls) + } + if manager.instances["app"].State != vm.StateRunning { + t.Fatal("the VM must stay running through a refused shrink") + } + + // The following ticks must plan no further update. The agent-side + // normalization is what makes this hold in production; here the + // clamp inside the apply path already settles it. + for tick := 0; tick < 2; tick++ { + if err := r.Reconcile(context.Background(), desired); err != nil { + t.Fatalf("tick %d: %v", tick, err) + } + if len(manager.removeCalls) != 0 || len(manager.startCalls) != 0 { + t.Fatalf("tick %d stopped or restarted the service: removes=%v starts=%v", + tick, manager.removeCalls, manager.startCalls) + } + } + }) + } +} + +// An update that changes the image *and* requests a refused shrink must still +// deploy the image. This falls out of clamping rather than blocking: failing +// the preflight would wedge every unrelated change behind a refused size. +func TestMixedUpdateStillDeploysTheImageWithNoResize(t *testing.T) { + running := volumeService("app", 16*config.MiB, 1) + r, manager := rejectingReconciler(t, running, 16*config.MiB) + + updated := volumeService("app", 2*config.MiB, 2) + updated.Image = "/image-v2" + desired := config.NodeConfig{Node: "node-1", Services: []config.ServiceConfig{updated}} + + if err := r.Reconcile(context.Background(), desired); err != nil { + t.Fatal(err) + } + if len(manager.startCalls) != 1 { + t.Fatalf("expected the image change to be deployed, got starts %v", manager.startCalls) + } + started := manager.instances["app"].Config + if started.Image != "/image-v2" { + t.Fatalf("the new image was not deployed: %q", started.Image) + } + if started.Volumes[0].SizeBytes != 16*config.MiB { + t.Fatalf("the refused size reached the launch path: %d", started.Volumes[0].SizeBytes) + } +} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 9039370..f1a99f3 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -37,6 +37,95 @@ type StorageReservations struct { SharedByBackend map[string]int64 RecordedLogicalIDs map[string]bool SharedEnabled bool + // LocalUnknownByNode and SharedUnknownByBackend mark a scope whose + // remaining capacity cannot be proved because a retained record was only + // partially readable. Its lower bound is still charged above; the flag is + // what stops the unaccounted remainder from being handed out again. + LocalUnknownByNode map[string]bool + SharedUnknownByBackend map[string]bool + // LocalClassUnknown and SharedClassUnknown widen that block to a whole + // storage class, for a record so unreadable that no binding — and + // therefore no narrower scope — could be determined. + LocalClassUnknown bool + SharedClassUnknown bool + // UnknownCapacityKeys names one offending record per blocked scope, keyed + // by node ID, backend ID, or the storage class for a class-wide block. + // A block with no repair target named is one an operator cannot act on: + // the record is somewhere in the pool, and the message is all they have. + UnknownCapacityKeys map[string]string +} + +// UnknownCapacityTarget returns a record key an operator should repair to lift +// a scope's block, preferring the narrowest scope that names one. +func (r StorageReservations) UnknownCapacityTarget(node, backend string, class config.VolumeType) string { + for _, scope := range []string{node, backend, string(class)} { + if scope == "" { + continue + } + if key := r.UnknownCapacityKeys[scope]; key != "" { + return key + } + } + return "" +} + +// Pending reason codes. They are a bounded vocabulary because the status API, +// fireworkctl, and the web UI all render them. +const ( + // ReasonInsufficientCompute means vCPU or memory, and nothing else. + ReasonInsufficientCompute = "insufficient_compute_capacity" + // ReasonVolumeCapacityUnavailable means the volume cannot bind to any + // candidate at all: no pool is configured there, or its retained binding + // names somewhere else. A configuration or placement fact. + ReasonVolumeCapacityUnavailable = "volume_capacity_unavailable" + // ReasonNodeStorageExhausted means the volume could bind, but the pool has + // no room for the new reservation. A capacity fact, resolved by freeing + // retained volumes or growing the pool. + ReasonNodeStorageExhausted = "node_storage_exhausted" + // ReasonStorageCapacityUnknown means remaining capacity cannot be proved, + // so new volume-bearing placement is withheld rather than guessed. + ReasonStorageCapacityUnknown = "storage_capacity_unknown" + // ReasonVolumeRecordInvalid means the service's own retained record could + // not be parsed, so it is not placed for the first time. + ReasonVolumeRecordInvalid = "volume_record_invalid" + // ReasonHostPortConflict means every candidate node already holds one of + // the service's (tcp, host_port) claims. It outranks the storage reasons + // below because the port check runs first: a node rejected on ports is + // never evaluated for storage, so a storage reason recorded elsewhere + // describes a different node than the one the operator has to fix. + ReasonHostPortConflict = "host_port_conflict" +) + +// storageRank orders storage rejection causes from least to most actionable so +// the dominant one survives across candidate nodes. +func storageRank(reason string) int { + switch reason { + case ReasonVolumeCapacityUnavailable: + return 1 + case ReasonStorageCapacityUnknown: + return 2 + case ReasonNodeStorageExhausted: + return 3 + default: + return 0 + } +} + +func storageReasonMessage(rejected storageRejection) string { + switch rejected.Reason { + case ReasonNodeStorageExhausted: + return "no active node has room for the requested volume reservation" + case ReasonStorageCapacityUnknown: + // The offending key is the whole value of this message. A cluster-wide + // block whose cause is only in the controller log leaves an operator + // grepping for an object they cannot name. + if rejected.Target != "" { + return fmt.Sprintf("remaining volume capacity cannot be verified; repair volume record %s", rejected.Target) + } + return "remaining volume capacity cannot be verified; repair the quarantined volume record" + default: + return "no active node satisfies volume binding and capacity" + } } type Pending struct { @@ -205,7 +294,16 @@ func BuildNodeConfigs(assignment map[string][]config.ServiceConfig) []config.Nod // guest destinations, so traffic silently reaches only one of them. A service // is therefore placed only on a node where all of its claims are free, and its // claims are taken atomically. -func ScheduleWithStorage(services []config.ServiceConfig, nodes []Node, existing map[string]string, reservations StorageReservations) (map[string][]config.ServiceConfig, []Pending) { +// pinned describes services the caller will render itself, outside this +// scheduler, on a node it has already chosen. They still occupy node-exclusive +// resources, so the scheduler has to be told about them or it will hand the +// same host port to something else — the exact collision node-exclusive claims +// exist to prevent. Compute is reserved by the caller adjusting node capacity; +// ports cannot be expressed that way, so they are passed here. +// +// ScheduleWithStorage takes pinnedClaims as node -> claim -> holding service. +// A nil map means nothing is pinned, which is the ordinary case. +func ScheduleWithStorage(services []config.ServiceConfig, nodes []Node, existing map[string]string, reservations StorageReservations, pinnedClaims map[string]map[config.PortClaim]string) (map[string][]config.ServiceConfig, []Pending) { result := make(map[string][]config.ServiceConfig, len(nodes)) usedVCPU := make(map[string]int, len(nodes)) usedMem := make(map[string]int, len(nodes)) @@ -218,6 +316,9 @@ func ScheduleWithStorage(services []config.ServiceConfig, nodes []Node, existing result[node.InstanceID] = nil groups[node.InstanceID] = make(map[string]bool) claimedPorts[node.InstanceID] = make(map[config.PortClaim]string) + for claim, holder := range pinnedClaims[node.InstanceID] { + claimedPorts[node.InstanceID][claim] = holder + } nodeByID[node.InstanceID] = node } @@ -281,6 +382,7 @@ func ScheduleWithStorage(services []config.ServiceConfig, nodes []Node, existing claims := service.PortClaims() chosen := "" chosenService := service + var dominantStorage storageRejection portConflict := "" for _, node := range candidates { if boundNode != "" && node.InstanceID != boundNode { @@ -297,8 +399,14 @@ func ScheduleWithStorage(services []config.ServiceConfig, nodes []Node, existing } continue } - candidateService, localDelta, sharedDelta, ok := fitStorage(service, node, reservations, usedLocal, usedShared) - if !ok { + candidateService, localDelta, sharedDelta, rejected := fitStorage(service, node, reservations, usedLocal, usedShared) + if rejected.Reason != "" { + // Keep the most actionable cause seen across candidates. The + // dominant reason tells the operator whether the placement is + // wrong or the chosen node is simply full. + if storageRank(rejected.Reason) > storageRank(dominantStorage.Reason) { + dominantStorage = rejected + } continue } chosen = node.InstanceID @@ -310,14 +418,14 @@ func ScheduleWithStorage(services []config.ServiceConfig, nodes []Node, existing break } if chosen == "" { - reason := "insufficient_compute_capacity" + reason := ReasonInsufficientCompute message := "no active node satisfies compute capacity" - if len(service.Volumes) > 0 { - reason = "volume_capacity_unavailable" - message = "no active node satisfies volume binding and capacity" + if dominantStorage.Reason != "" { + reason = dominantStorage.Reason + message = storageReasonMessage(dominantStorage) } if portConflict != "" { - reason = "host_port_conflict" + reason = ReasonHostPortConflict message = portConflict } pending = append(pending, Pending{Service: service.Name, ReasonCode: reason, Message: message}) @@ -374,7 +482,19 @@ func hasSharedVolume(service config.ServiceConfig) bool { return false } -func fitStorage(service config.ServiceConfig, node Node, reservations StorageReservations, usedLocal, usedShared map[string]int64) (config.ServiceConfig, int64, int64, bool) { +// fitStorage reports whether a service's volumes can bind to a node, and why +// not when they cannot. The reason separates a placement fact (the volume +// cannot bind here at all) from a capacity fact (it could bind, but the pool +// has no room), because the two have opposite operator remedies. +// storageRejection is why a node was refused, and where to look to fix it. +type storageRejection struct { + Reason string + // Target names a quarantined record to repair, for the reasons where one + // exists. Empty otherwise. + Target string +} + +func fitStorage(service config.ServiceConfig, node Node, reservations StorageReservations, usedLocal, usedShared map[string]int64) (config.ServiceConfig, int64, int64, storageRejection) { candidate := service candidate.Volumes = append([]config.VolumeConfig(nil), service.Volumes...) var localDelta, sharedDelta int64 @@ -384,7 +504,7 @@ func fitStorage(service config.ServiceConfig, node Node, reservations StorageRes switch volume.Type { case config.VolumeTypeLocal: if node.LocalCapacityBytes <= 0 || (volume.BoundNode != "" && volume.BoundNode != node.InstanceID) { - return service, 0, 0, false + return service, 0, 0, storageRejection{Reason: ReasonVolumeCapacityUnavailable} } volume.BoundNode = node.InstanceID if !reservations.RecordedLogicalIDs[logicalID] { @@ -392,7 +512,7 @@ func fitStorage(service config.ServiceConfig, node Node, reservations StorageRes } case config.VolumeTypeShared: if node.SharedBackendID == "" || (volume.SharedBackendID != "" && volume.SharedBackendID != node.SharedBackendID) { - return service, 0, 0, false + return service, 0, 0, storageRejection{Reason: ReasonVolumeCapacityUnavailable} } volume.SharedBackendID = node.SharedBackendID if !reservations.RecordedLogicalIDs[logicalID] { @@ -400,11 +520,33 @@ func fitStorage(service config.ServiceConfig, node Node, reservations StorageRes } } } - if reservations.LocalByNode[node.InstanceID]+usedLocal[node.InstanceID]+localDelta > node.LocalCapacityBytes { - return service, 0, 0, false + // A service that adds no new local reservation cannot recover capacity by + // being rejected, it can only be evicted. Volumes already counted in + // LocalByNode contribute a zero delta, and a service with no volumes at + // all contributes nothing — so retained reservations above the pool must + // not make the node reject either of them. Only a genuinely new + // allocation is checked against the pool. + if localDelta > 0 && reservations.LocalByNode[node.InstanceID]+usedLocal[node.InstanceID]+localDelta > node.LocalCapacityBytes { + return service, 0, 0, storageRejection{Reason: ReasonNodeStorageExhausted} } if sharedDelta > 0 && node.SharedCapacityBytes > 0 && reservations.SharedByBackend[node.SharedBackendID]+usedShared[node.SharedBackendID]+sharedDelta > node.SharedCapacityBytes { - return service, 0, 0, false + return service, 0, 0, storageRejection{Reason: ReasonNodeStorageExhausted} + } + // A quarantined record whose reservation could not be read makes the + // node's remaining pool unknowable. New volume-bearing placement is + // withheld there rather than allocated against capacity that may already + // be occupied; an already-placed service is re-rendered untouched. + if localDelta > 0 && (reservations.LocalUnknownByNode[node.InstanceID] || reservations.LocalClassUnknown) { + return service, 0, 0, storageRejection{ + Reason: ReasonStorageCapacityUnknown, + Target: reservations.UnknownCapacityTarget(node.InstanceID, "", config.VolumeTypeLocal), + } + } + if sharedDelta > 0 && (reservations.SharedUnknownByBackend[node.SharedBackendID] || reservations.SharedClassUnknown) { + return service, 0, 0, storageRejection{ + Reason: ReasonStorageCapacityUnknown, + Target: reservations.UnknownCapacityTarget("", node.SharedBackendID, config.VolumeTypeShared), + } } - return candidate, localDelta, sharedDelta, true + return candidate, localDelta, sharedDelta, storageRejection{} } diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index ec41d9f..3fc2f6b 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -255,7 +255,7 @@ func TestScheduleWithStorageBindsLocalVolumeAndHonorsRetainedBinding(t *testing. {InstanceID: "small", CapacityVCPUs: 4, CapacityMemMB: 1024, LocalCapacityBytes: 5 * config.GiB}, {InstanceID: "large", CapacityVCPUs: 4, CapacityMemMB: 1024, LocalCapacityBytes: 20 * config.GiB}, } - result, pending := ScheduleWithStorage([]config.ServiceConfig{service}, nodes, nil, StorageReservations{}) + result, pending := ScheduleWithStorage([]config.ServiceConfig{service}, nodes, nil, StorageReservations{}, nil) if len(pending) != 0 || len(result["large"]) != 1 { t.Fatalf("unexpected placement result=%#v pending=%#v", result, pending) } @@ -264,7 +264,7 @@ func TestScheduleWithStorageBindsLocalVolumeAndHonorsRetainedBinding(t *testing. } service.Volumes[0].BoundNode = "lost" - _, pending = ScheduleWithStorage([]config.ServiceConfig{service}, nodes, nil, StorageReservations{}) + _, pending = ScheduleWithStorage([]config.ServiceConfig{service}, nodes, nil, StorageReservations{}, nil) if len(pending) != 1 || pending[0].ReasonCode != "local_volume_node_unavailable" { t.Fatalf("unexpected retained binding result: %#v", pending) } @@ -274,7 +274,7 @@ func TestScheduleWithStorageKeepsSharedPendingUntilSafetyGate(t *testing.T) { service := svc("db", 1, 256) service.Volumes = []config.VolumeConfig{{Name: "data", Type: config.VolumeTypeShared, MountPath: "/data", SizeBytes: config.GiB}} nodes := []Node{{InstanceID: "node", CapacityVCPUs: 4, CapacityMemMB: 1024, SharedBackendID: "primary"}} - _, pending := ScheduleWithStorage([]config.ServiceConfig{service}, nodes, nil, StorageReservations{}) + _, pending := ScheduleWithStorage([]config.ServiceConfig{service}, nodes, nil, StorageReservations{}, nil) if len(pending) != 1 || pending[0].ReasonCode != "shared_volume_runtime_unavailable" { t.Fatalf("unexpected pending result: %#v", pending) } @@ -309,7 +309,7 @@ func TestScheduleWithStorageSeparatesServicesSharingHostPort(t *testing.T) { } nodes := []Node{node("i-001", 32, 16384), node("i-002", 4, 2048)} - result, pending := ScheduleWithStorage(services, nodes, nil, StorageReservations{}) + result, pending := ScheduleWithStorage(services, nodes, nil, StorageReservations{}, nil) if len(pending) != 0 { t.Fatalf("unexpected pending services: %#v", pending) } @@ -327,7 +327,7 @@ func TestScheduleWithStorageKeepsRepeatedHostPortsOnDifferentNodes(t *testing.T) nodes := []Node{node("i-001", 8, 4096), node("i-002", 8, 4096)} existing := map[string]string{"a": "i-001", "b": "i-002"} - result, pending := ScheduleWithStorage(services, nodes, existing, StorageReservations{}) + result, pending := ScheduleWithStorage(services, nodes, existing, StorageReservations{}, nil) if len(pending) != 0 { t.Fatalf("unexpected pending services: %#v", pending) } @@ -343,7 +343,7 @@ func TestScheduleWithStorageLeavesConflictingServicePending(t *testing.T) { } nodes := []Node{node("i-001", 8, 4096)} - result, pending := ScheduleWithStorage(services, nodes, nil, StorageReservations{}) + result, pending := ScheduleWithStorage(services, nodes, nil, StorageReservations{}, nil) if len(result["i-001"]) != 1 { t.Fatalf("expected exactly one service placed, got %#v", result) } @@ -367,7 +367,7 @@ func TestScheduleWithStorageRelocatesExistingPlacementOnNewConflict(t *testing.T nodes := []Node{node("i-001", 8, 4096), node("i-002", 8, 4096)} existing := map[string]string{"a": "i-001", "b": "i-001"} - result, pending := ScheduleWithStorage(services, nodes, existing, StorageReservations{}) + result, pending := ScheduleWithStorage(services, nodes, existing, StorageReservations{}, nil) if len(pending) != 0 { t.Fatalf("unexpected pending services: %#v", pending) } @@ -389,7 +389,7 @@ func TestScheduleWithStorageTreatsMultipleClaimsAtomically(t *testing.T) { nodes := []Node{node("i-001", 16, 8192), node("i-002", 8, 4096)} existing := map[string]string{"keeper": "i-001"} - result, pending := ScheduleWithStorage(services, nodes, existing, StorageReservations{}) + result, pending := ScheduleWithStorage(services, nodes, existing, StorageReservations{}, nil) if len(pending) != 0 { t.Fatalf("unexpected pending services: %#v", pending) } @@ -408,7 +408,7 @@ func TestScheduleWithStorageRejectsSelfConflictingService(t *testing.T) { service := withPorts("broken", 2, 512, 8080, 8080) nodes := []Node{node("i-001", 8, 4096), node("i-002", 8, 4096)} - result, pending := ScheduleWithStorage([]config.ServiceConfig{service}, nodes, nil, StorageReservations{}) + result, pending := ScheduleWithStorage([]config.ServiceConfig{service}, nodes, nil, StorageReservations{}, nil) if len(pending) != 1 || pending[0].ReasonCode != "duplicate_host_port_claims" { t.Fatalf("unexpected pending result: %#v", pending) } diff --git a/internal/scheduler/volume_hardening_test.go b/internal/scheduler/volume_hardening_test.go new file mode 100644 index 0000000..6509cb5 --- /dev/null +++ b/internal/scheduler/volume_hardening_test.go @@ -0,0 +1,174 @@ +package scheduler + +import ( + "testing" + + "github.com/artemnikitin/firework/internal/config" +) + +// exhaustedNode has a pool whose retained reservations already exceed its +// configured capacity — the state an oversized `size:` edit produces, and the +// one that used to make the node reject every workload on it. +func exhaustedNode() ([]Node, StorageReservations) { + nodes := []Node{{ + InstanceID: "i-1", CapacityVCPUs: 8, CapacityMemMB: 8192, + LocalCapacityBytes: 100 * config.MiB, + }} + reservations := StorageReservations{ + LocalByNode: map[string]int64{"i-1": 500 * config.MiB}, + SharedByBackend: map[string]int64{}, + RecordedLogicalIDs: map[string]bool{"kept/data": true}, + } + return nodes, reservations +} + +func localVolumeService(name string, size int64) config.ServiceConfig { + return config.ServiceConfig{ + Name: name, VCPUs: 1, MemoryMB: 512, + Volumes: []config.VolumeConfig{{ + Name: "data", Type: config.VolumeTypeLocal, MountPath: "/data", SizeBytes: size, + }}, + } +} + +func pendingReason(pending []Pending, service string) string { + for _, item := range pending { + if item.Service == service { + return item.ReasonCode + } + } + return "" +} + +func placedOn(assignments map[string][]config.ServiceConfig, service string) string { + for node, services := range assignments { + for _, placed := range services { + if placed.Name == service { + return node + } + } + } + return "" +} + +// A node whose retained reservations exceed its pool must still run the +// workloads that do not add to those reservations. Rejecting them cannot +// recover a single byte; it can only evict. +func TestOverReservedNodeStillAcceptsServicesThatAddNoReservation(t *testing.T) { + nodes, reservations := exhaustedNode() + services := []config.ServiceConfig{ + {Name: "stateless", VCPUs: 1, MemoryMB: 512}, + localVolumeService("kept", 16*config.MiB), + } + existing := map[string]string{"stateless": "i-1", "kept": "i-1"} + + assignments, pending := ScheduleWithStorage(services, nodes, existing, reservations, nil) + + if len(pending) != 0 { + t.Fatalf("expected no pending services, got %#v", pending) + } + if placedOn(assignments, "stateless") != "i-1" || placedOn(assignments, "kept") != "i-1" { + t.Fatalf("expected both services to stay on i-1, got %#v", assignments) + } +} + +// The capacity guard still holds for a genuinely new allocation, and now says +// which of the two very different storage causes applied. +func TestStorageRejectionReasonsAreDistinct(t *testing.T) { + nodes, reservations := exhaustedNode() + // An active node with no local pool at all: a volume bound there cannot + // bind, which is a placement fact rather than a capacity one. + nodes = append(nodes, Node{InstanceID: "i-poolless", CapacityVCPUs: 8, CapacityMemMB: 8192}) + + tests := []struct { + name string + service config.ServiceConfig + want string + }{ + { + name: "new allocation on a full pool", + service: localVolumeService("fresh", 16*config.MiB), + want: ReasonNodeStorageExhausted, + }, + { + name: "bound node has no local pool configured", + service: func() config.ServiceConfig { + svc := localVolumeService("elsewhere", 16*config.MiB) + svc.Volumes[0].BoundNode = "i-poolless" + return svc + }(), + want: ReasonVolumeCapacityUnavailable, + }, + { + name: "compute only", + service: config.ServiceConfig{Name: "huge", VCPUs: 64, MemoryMB: 65536}, + want: ReasonInsufficientCompute, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + _, pending := ScheduleWithStorage([]config.ServiceConfig{test.service}, nodes, nil, reservations, nil) + if got := pendingReason(pending, test.service.Name); got != test.want { + t.Fatalf("expected reason %q, got %q (%#v)", test.want, got, pending) + } + }) + } +} + +// A quarantined record whose reservation could not be read makes the node's +// remaining pool unknowable, so new volume-bearing placement waits for the +// repair rather than being allocated against capacity that may be occupied. +// Existing placements and stateless services are untouched. +func TestUnknownCapacityBlocksOnlyNewVolumeBearingPlacement(t *testing.T) { + nodes, reservations := exhaustedNode() + reservations.LocalByNode = map[string]int64{"i-1": 16 * config.MiB} + reservations.LocalUnknownByNode = map[string]bool{"i-1": true} + + services := []config.ServiceConfig{ + {Name: "stateless", VCPUs: 1, MemoryMB: 512}, + localVolumeService("kept", 16*config.MiB), + localVolumeService("fresh", 16*config.MiB), + } + assignments, pending := ScheduleWithStorage(services, nodes, map[string]string{"kept": "i-1"}, reservations, nil) + + if placedOn(assignments, "stateless") == "" || placedOn(assignments, "kept") == "" { + t.Fatalf("existing and stateless workloads must keep running: %#v (%#v)", assignments, pending) + } + if got := pendingReason(pending, "fresh"); got != ReasonStorageCapacityUnknown { + t.Fatalf("expected %q for a new volume-bearing service, got %q", ReasonStorageCapacityUnknown, got) + } +} + +// The eviction path end to end: an oversized size: edit raises the node's +// reservations above its pool, and every service on it must still be rendered. +// BuildNodeConfigs dropping a node's services is what the agent turns into a +// delete for each one, so an empty render here is the eviction itself. +func TestOversizedReservationDoesNotEmptyTheRenderedNodeConfig(t *testing.T) { + nodes := []Node{{ + InstanceID: "i-1", CapacityVCPUs: 8, CapacityMemMB: 8192, + LocalCapacityBytes: 100 * config.MiB, + }} + // The operator edited size: to something far above the pool; the record + // was already retained, so it contributes its inflated reservation. + reservations := StorageReservations{ + LocalByNode: map[string]int64{"i-1": 900 * config.MiB}, + SharedByBackend: map[string]int64{}, + RecordedLogicalIDs: map[string]bool{"db/data": true}, + } + services := []config.ServiceConfig{ + {Name: "web", VCPUs: 1, MemoryMB: 512}, + {Name: "api", VCPUs: 1, MemoryMB: 512}, + localVolumeService("db", 900*config.MiB), + } + existing := map[string]string{"web": "i-1", "api": "i-1", "db": "i-1"} + + assignments, pending := ScheduleWithStorage(services, nodes, existing, reservations, nil) + if len(pending) != 0 { + t.Fatalf("expected no service to be evicted, got %#v", pending) + } + rendered := BuildNodeConfigs(assignments) + if len(rendered) != 1 || len(rendered[0].Services) != 3 { + t.Fatalf("expected all three services rendered for i-1, got %#v", rendered) + } +} diff --git a/internal/statusmodel/status.go b/internal/statusmodel/status.go index 234b704..4d6be0b 100644 --- a/internal/statusmodel/status.go +++ b/internal/statusmodel/status.go @@ -63,7 +63,13 @@ var ( "ConfigFetched", "ConfigParsed", "NetworkReady", "CapacityReady", "ImagesReady", "VMsReconciled", "Reconciled", "LocalRoutesReady", } - nonBlockingConditionTypes = []string{"PeerRoutesReady"} + // VolumeSizesApplied is false while this node is running a volume at a + // size other than the one the desired revision asked for. It is + // non-blocking because the workload is healthy — it is running, just not + // at the requested quota — but it must not read as ordinary convergence, + // or the operator sees a service quietly running at the wrong size with no + // explanation. + nonBlockingConditionTypes = []string{"PeerRoutesReady", "VolumeSizesApplied"} ) // BlockingConditionTypes returns the conditions whose failure is fatal. @@ -152,6 +158,13 @@ type VolumeStatus struct { ResizeGeneration int64 `json:"resize_generation,omitempty"` State string `json:"state"` LastError string `json:"last_error,omitempty"` + // RequestedSizeBytes is what the desired revision asked for, when that + // differs from the effective DesiredSizeBytes the cluster accepted and + // rendered. Equal sizes are reported only through DesiredSizeBytes, so an + // unrejected volume's surface is unchanged. + RequestedSizeBytes int64 `json:"requested_size_bytes,omitempty"` + Rejected bool `json:"rejected,omitempty"` + RejectedReason string `json:"rejected_reason,omitempty"` } type AgentStatus struct { diff --git a/internal/vm/manager.go b/internal/vm/manager.go index 5f15078..5ff4d68 100644 --- a/internal/vm/manager.go +++ b/internal/vm/manager.go @@ -51,8 +51,34 @@ const ( // StateRecoveryPending means durable state exists but ownership could not // be proved. Firework preserves the process and files and blocks duplicates. StateRecoveryPending State = "recovery_pending" + // StateStarting is published while a start has released the manager lock + // to prepare volumes. It exists so List reports something truthful during + // a multi-minute mkfs or resize rather than reporting nothing at all. + StateStarting State = "starting" + // StateStartAborting means a Stop or Remove arrived while a start was in + // its unlocked preparation phase. The start's own final phase observes it + // and cleans up without launching anything. + StateStartAborting State = "start_aborting" ) +var ( + // ErrStartAborted reports that a start was cancelled by a concurrent Stop + // or Remove before anything was launched. It is a benign race, not a + // fault: the caller must retry rather than record a reconcile failure. + ErrStartAborted = errors.New("start aborted by a concurrent stop or remove") + // ErrStartInProgress reports that another start for the same service is + // still in its preparation phase. Like ErrStartAborted this is a retry + // signal, not a failure. + ErrStartInProgress = errors.New("start already in progress") +) + +// IsStartRace reports whether an error is one of the benign start-barrier +// races. Callers use it to classify a reconciliation as incomplete — retry on +// the next tick without advancing the applied revision — rather than failed. +func IsStartRace(err error) bool { + return errors.Is(err, ErrStartAborted) || errors.Is(err, ErrStartInProgress) +} + // Instance represents a running Firecracker microVM. type Instance struct { // Name is the service name from the config. @@ -72,6 +98,10 @@ type Instance struct { instanceID string manifest *instanceManifest + // startID identifies one Start attempt. Phase 3 validates against it + // rather than against the service name, so a placeholder that was cleared + // and replaced by a later attempt is never mistaken for one's own. + startID string } // Manager manages the lifecycle of Firecracker microVMs on the local host. @@ -119,23 +149,54 @@ func NewManagerWithVolumes(firecrackerBin, stateDir string, logger *slog.Logger, // Preflight validates persistent volumes without changing them. Reconciliation // calls this before stopping an existing VM so a failed resize leaves it live. -func (m *Manager) Preflight(ctx context.Context, svc config.ServiceConfig) error { +// It also returns any size requests it refused. A rejection never goes through +// setVolumeError: it is a decision rather than a fault, and recording it there +// would set a volume_failed reason code and trigger the blanket overwrite that +// relabels every one of the service's volumes as errored. +func (m *Manager) Preflight(ctx context.Context, svc config.ServiceConfig) ([]volume.Rejection, error) { if len(svc.Volumes) == 0 { m.setVolumeError(svc.Name, nil) - return nil + return nil, nil } if err := validateVolumeKernelArgs(svc); err != nil { m.setVolumeError(svc.Name, err) - return err + return nil, err } if m.volumeManager == nil { err := fmt.Errorf("service %s declares volumes but agent storage is not configured", svc.Name) m.setVolumeError(svc.Name, err) - return err + return nil, err } - err := m.volumeManager.Preflight(ctx, svc) + rejections, err := m.volumeManager.Preflight(ctx, svc) m.setVolumeError(svc.Name, err) - return err + return rejections, err +} + +// VolumeRejections returns the agent's current per-volume refusal snapshot. +func (m *Manager) VolumeRejections() map[string]volume.Rejection { + if m.volumeManager == nil { + return nil + } + return m.volumeManager.Rejections() +} + +// SeedVolumeRejectionsForTest installs a refusal snapshot without running a +// real filesystem operation, so the status and convergence paths can be +// exercised without a live pool. +func (m *Manager) SeedVolumeRejectionsForTest(rejections map[string]volume.Rejection) { + if m.volumeManager == nil { + return + } + m.volumeManager.SeedRejectionsForTest(rejections) +} + +// NormalizeVolumes clamps a desired node configuration to the sizes the node +// is actually able to serve, before anything else in the tick reads it. +func (m *Manager) NormalizeVolumes(services []config.ServiceConfig) { + if m.volumeManager == nil { + return + } + m.volumeManager.NormalizeVolumes(services) } // VolumeError returns the latest persistent-volume preparation failure for a @@ -146,6 +207,32 @@ func (m *Manager) VolumeError(service string) string { return m.volumeErrors[service] } +// clampToPrepared substitutes the effective configuration — size and +// generation — for every volume whose request was refused, so the instance the +// next tick compares against describes what is actually running. The refused +// request is reported from the volume manager's rejection snapshot instead, +// which is what the control-plane acknowledgement matches on. +func clampToPrepared(svc config.ServiceConfig, prepared []volume.PreparedVolume) config.ServiceConfig { + effective := make(map[string]volume.PreparedVolume, len(prepared)) + for _, preparedVolume := range prepared { + if preparedVolume.Rejected { + effective[preparedVolume.LogicalID] = preparedVolume + } + } + if len(effective) == 0 { + return svc + } + clamped := svc + clamped.Volumes = append([]config.VolumeConfig(nil), svc.Volumes...) + for i := range clamped.Volumes { + if preparedVolume, ok := effective[svc.Name+"/"+clamped.Volumes[i].Name]; ok { + clamped.Volumes[i].SizeBytes = preparedVolume.SizeBytes + clamped.Volumes[i].ResizeGeneration = preparedVolume.ResizeGeneration + } + } + return clamped +} + func (m *Manager) setVolumeError(service string, err error) { m.mu.Lock() defer m.mu.Unlock() @@ -156,32 +243,64 @@ func (m *Manager) setVolumeError(service string, err error) { m.volumeErrors[service] = err.Error() } -func validateVolumeKernelArgs(svc config.ServiceConfig) error { - volumes := append([]config.VolumeConfig(nil), svc.Volumes...) - sort.Slice(volumes, func(i, j int) bool { return volumes[i].Name < volumes[j].Name }) - guestVolumes := make([]guestVolume, 0, len(volumes)) - for i, volume := range volumes { +// defaultKernelArgs is the boot command line used when a service declares none. +const defaultKernelArgs = "console=ttyS0 reboot=k panic=1 pci=off" + +// buildBootArgs composes a service's kernel command line and enforces the +// command-line length limit. +// +// It is the single place boot args are built. Preflight's early check and the +// launch path previously constructed the payload separately and drifted: the +// length check lived only in the update path, so ActionCreate could boot a VM +// with an over-long command line instead of failing with a clear error. +func buildBootArgs(svc config.ServiceConfig, guestVolumes []guestVolume) (string, error) { + kernelArgs := svc.KernelArgs + if kernelArgs == "" { + kernelArgs = defaultKernelArgs + } + if len(guestVolumes) > 0 { + payload, err := json.Marshal(guestVolumePayload{Version: 1, Volumes: guestVolumes}) + if err != nil { + return "", fmt.Errorf("marshal guest volume payload: %w", err) + } + arg := "firework.volumes64=" + base64.RawURLEncoding.EncodeToString(payload) + kernelArgs = insertBeforeApplicationSeparator(kernelArgs, arg) + } + if len(kernelArgs) > maxKernelCommandLineBytes { + what := "kernel command line" + if len(guestVolumes) > 0 { + what = "kernel command line with volume payload" + } + return "", fmt.Errorf("service %s: %s is %d bytes; maximum is %d", svc.Name, what, len(kernelArgs), maxKernelCommandLineBytes) + } + return kernelArgs, nil +} + +// guestVolumesFromConfig builds the guest payload entries from declared +// volumes, for the preflight that runs before anything has been prepared. +func guestVolumesFromConfig(volumes []config.VolumeConfig) ([]guestVolume, error) { + ordered := append([]config.VolumeConfig(nil), volumes...) + sort.Slice(ordered, func(i, j int) bool { return ordered[i].Name < ordered[j].Name }) + guestVolumes := make([]guestVolume, 0, len(ordered)) + for i, declared := range ordered { device, err := guestBlockDevice(i) if err != nil { - return err + return nil, err } guestVolumes = append(guestVolumes, guestVolume{ - Name: volume.Name, Device: device, MountPath: volume.MountPath, Type: volume.Type, + Name: declared.Name, Device: device, MountPath: declared.MountPath, Type: declared.Type, }) } - payload, err := json.Marshal(guestVolumePayload{Version: 1, Volumes: guestVolumes}) + return guestVolumes, nil +} + +func validateVolumeKernelArgs(svc config.ServiceConfig) error { + guestVolumes, err := guestVolumesFromConfig(svc.Volumes) if err != nil { - return fmt.Errorf("marshal guest volume payload: %w", err) - } - kernelArgs := svc.KernelArgs - if kernelArgs == "" { - kernelArgs = "console=ttyS0 reboot=k panic=1 pci=off" - } - kernelArgs = insertBeforeApplicationSeparator(kernelArgs, "firework.volumes64="+base64.RawURLEncoding.EncodeToString(payload)) - if len(kernelArgs) > maxKernelCommandLineBytes { - return fmt.Errorf("service %s: kernel command line with volume payload is %d bytes; maximum is %d", svc.Name, len(kernelArgs), maxKernelCommandLineBytes) + return err } - return nil + _, err = buildBootArgs(svc, guestVolumes) + return err } // List returns a snapshot of all known VM instances. @@ -198,59 +317,155 @@ func (m *Manager) List() map[string]*Instance { } // Start launches a new Firecracker microVM for the given service config. +// Start launches a microVM for a service. +// +// It runs in three phases so the manager lock is not held across volume +// preparation, which can spend minutes in mkfs.ext4, e2fsck, or resize2fs. +// Holding the lock there stalled every reader of it — including the heartbeat +// goroutine, which reaches it through List — so a node went stale precisely +// while it was busy resizing its own services' volumes. +// +// Releasing the lock opens a window in which a Stop or Remove can arrive, so +// the phases are governed by a barrier rather than by extra branches: +// +// absent -> StateStarting phase 1 publishes the placeholder +// StateStarting -> StateRunning phase 3, own startID still present +// StateStarting -> StateStartAborting Stop or Remove during phase 2 +// StateStartAborting -> absent phase 3 observes the abort +// StateStarting -> absent phase 2 failed +// +// Phase 3 confirms ownership before any side effect: no manifest is written +// and nothing is launched unless the placeholder is still this attempt's own. func (m *Manager) Start(ctx context.Context, svc config.ServiceConfig) error { + startID, vmDir, socketPath, err := m.beginStart(svc) + if err != nil { + return err + } + + // Phase 2 runs without the manager lock. Its side effects — a created or + // resized volume image — are durable, retained state by design, so they + // are deliberately not rolled back when the start is aborted: the next + // start reuses them. + prepared, err := m.prepareVolumes(ctx, svc) + if err != nil { + m.discardStart(svc.Name, startID) + return err + } + + return m.finishStart(ctx, svc, startID, vmDir, socketPath, prepared) +} + +// beginStart is phase 1: it takes the entry checks and publishes the starting +// placeholder under the manager lock. +func (m *Manager) beginStart(svc config.ServiceConfig) (startID, vmDir, socketPath string, err error) { m.mu.Lock() defer m.mu.Unlock() if inst, exists := m.instances[svc.Name]; exists { - if inst.State == StateRecoveryPending { - return fmt.Errorf("service %s has ambiguous surviving state: %s", svc.Name, inst.LastError) - } - if inst.State == StateRunning || inst.State == StateStopping { - return fmt.Errorf("service %s is already active (pid %d, state %s)", svc.Name, inst.PID, inst.State) + switch inst.State { + case StateRecoveryPending: + return "", "", "", fmt.Errorf("service %s has ambiguous surviving state: %s", svc.Name, inst.LastError) + case StateRunning, StateStopping: + return "", "", "", fmt.Errorf("service %s is already active (pid %d, state %s)", svc.Name, inst.PID, inst.State) + case StateStarting, StateStartAborting: + // A start already holds this name. Rejecting here is what keeps + // the agent API and shutdown paths from racing the reconcile loop. + return "", "", "", fmt.Errorf("service %s is in state %s: %w", svc.Name, inst.State, ErrStartInProgress) } } m.logger.Info("starting microVM", "service", svc.Name, "vcpus", svc.VCPUs, "memory_mb", svc.MemoryMB) - vmDir := filepath.Join(m.stateDir, "vms", svc.Name) + vmDir = filepath.Join(m.stateDir, "vms", svc.Name) if err := m.reclaimUnownedState(svc.Name, vmDir); err != nil { - return err + return "", "", "", err } if err := os.MkdirAll(vmDir, 0o755); err != nil { - return fmt.Errorf("creating vm dir: %w", err) + return "", "", "", fmt.Errorf("creating vm dir: %w", err) } - socketPath := filepath.Join(vmDir, "firecracker.sock") + socketPath = filepath.Join(vmDir, "firecracker.sock") // Remove stale socket if it exists. _ = os.Remove(socketPath) - var prepared []volume.PreparedVolume - var err error - if len(svc.Volumes) > 0 { - if m.volumeManager == nil { - err := fmt.Errorf("service %s declares volumes but agent storage is not configured", svc.Name) - m.volumeErrors[svc.Name] = err.Error() - return err - } - prepared, err = m.volumeManager.Prepare(ctx, svc) - if err != nil { - m.volumeErrors[svc.Name] = err.Error() - return fmt.Errorf("preparing volumes: %w", err) + startID, err = newInstanceID() + if err != nil { + return "", "", "", err + } + m.instances[svc.Name] = &Instance{ + Name: svc.Name, Config: svc, State: StateStarting, + SocketPath: socketPath, startID: startID, + } + return startID, vmDir, socketPath, nil +} + +// prepareVolumes is phase 2. It runs without the manager lock, so it records +// volume errors through setVolumeError rather than writing the map directly. +func (m *Manager) prepareVolumes(ctx context.Context, svc config.ServiceConfig) ([]volume.PreparedVolume, error) { + if len(svc.Volumes) == 0 { + return nil, nil + } + if m.volumeManager == nil { + err := fmt.Errorf("service %s declares volumes but agent storage is not configured", svc.Name) + m.setVolumeError(svc.Name, err) + return nil, err + } + prepared, err := m.volumeManager.Prepare(ctx, svc) + if err != nil { + m.setVolumeError(svc.Name, err) + return nil, fmt.Errorf("preparing volumes: %w", err) + } + return prepared, nil +} + +// discardStart removes this attempt's placeholder after a phase-2 failure. It +// leaves a placeholder belonging to some later attempt alone. +func (m *Manager) discardStart(name, startID string) { + m.mu.Lock() + defer m.mu.Unlock() + if inst, exists := m.instances[name]; exists && inst.startID == startID { + delete(m.instances, name) + } +} + +// finishStart is phase 3: it re-takes the manager lock, confirms this attempt +// still owns the placeholder, and only then writes durable state or launches. +func (m *Manager) finishStart(ctx context.Context, svc config.ServiceConfig, startID, vmDir, socketPath string, prepared []volume.PreparedVolume) error { + m.mu.Lock() + defer m.mu.Unlock() + + placeholder, exists := m.instances[svc.Name] + if !exists || placeholder.startID != startID || placeholder.State != StateStarting { + if exists && placeholder.startID == startID { + delete(m.instances, svc.Name) } + m.logger.Info("start aborted before launch", "service", svc.Name) + return fmt.Errorf("service %s: %w", svc.Name, ErrStartAborted) } + // Clamp the service configuration to what was actually prepared. A refused + // shrink prepares successfully at the applied size, and everything + // downstream — the config hash, the Firecracker config, the ownership + // manifest, and the instance the next tick compares against — must describe + // that effective configuration. Storing it here is what makes needsUpdate + // compare equal on the following tick rather than one convergence cycle + // later. + svc = clampToPrepared(svc, prepared) + configPath, err := m.writeVMConfig(vmDir, svc, prepared) if err != nil { + delete(m.instances, svc.Name) return fmt.Errorf("writing vm config: %w", err) } configHash, err := serviceConfigHash(svc) if err != nil { + delete(m.instances, svc.Name) return err } instanceID, err := newInstanceID() if err != nil { + delete(m.instances, svc.Name) return err } launcherKind, launcherUnit := startingLauncherMetadata(m.launcher, instanceID) @@ -262,6 +477,7 @@ func (m *Manager) Start(ctx context.Context, svc config.ServiceConfig) error { StartedAt: time.Now().UTC(), Volumes: append([]volume.PreparedVolume(nil), prepared...), } if err := writeManifest(manifestPath(vmDir), manifest); err != nil { + delete(m.instances, svc.Name) return err } launched, err := m.launcher.Launch(ctx, launchSpec{ @@ -272,6 +488,7 @@ func (m *Manager) Start(ctx context.Context, svc config.ServiceConfig) error { manifest.Lifecycle = lifecycleFailed manifest.LastError = err.Error() _ = writeManifest(manifestPath(vmDir), manifest) + delete(m.instances, svc.Name) return fmt.Errorf("starting firecracker: %w", err) } manifest.PID = launched.PID @@ -280,12 +497,15 @@ func (m *Manager) Start(ctx context.Context, svc config.ServiceConfig) error { if identityErr := m.recordLaunchedIdentity(manifest, launched); identityErr != nil { if m.abandonLaunch(svc.Name, manifest, launched, identityErr) { m.instances[svc.Name] = instanceFromManifest(manifest, StateRecoveryPending, manifest.LastError) + } else { + delete(m.instances, svc.Name) } return fmt.Errorf("confirming launched process identity: %w", identityErr) } manifest.Lifecycle = lifecycleRunning if err := writeManifest(manifestPath(vmDir), manifest); err != nil { _ = m.launcher.Stop(manifest, syscall.SIGKILL) + delete(m.instances, svc.Name) return err } @@ -298,6 +518,7 @@ func (m *Manager) Start(ctx context.Context, svc config.ServiceConfig) error { Volumes: append([]volume.PreparedVolume(nil), prepared...), instanceID: instanceID, manifest: manifest, + startID: startID, } delete(m.volumeErrors, svc.Name) @@ -475,6 +696,18 @@ func (m *Manager) Stop(name string) error { m.mu.Unlock() return err } + // A start that is still preparing volumes has launched nothing, so there + // is no process to signal. Mark the attempt aborted and return + // immediately rather than waiting for a possibly multi-minute mkfs — not + // stalling shutdown behind volume work is the point of the barrier. + // Marking is idempotent so shutdown and the agent API can both issue a + // stop without the loser seeing an error for work the winner already did. + if inst.State == StateStarting || inst.State == StateStartAborting { + inst.State = StateStartAborting + m.mu.Unlock() + m.logger.Info("aborting in-flight start instead of stopping", "service", name) + return nil + } manifest := inst.manifest if manifest == nil { m.mu.Unlock() @@ -547,8 +780,25 @@ func (m *Manager) Stop(name string) error { func (m *Manager) Remove(name string) error { m.mu.Lock() inst, exists := m.instances[name] + aborting := exists && (inst.State == StateStarting || inst.State == StateStartAborting) + if aborting { + inst.State = StateStartAborting + } m.mu.Unlock() + // Removing the VM state directory while phase 2 runs is safe: volume + // preparation writes only under the storage pool, and writeVMConfig — the + // only writer of this directory — lives in phase 3, which will abort. The + // placeholder is left for phase 3 to clear, so a second Remove before then + // takes this same branch and also succeeds. + if aborting { + m.logger.Info("aborting in-flight start instead of removing", "service", name) + if err := os.RemoveAll(filepath.Join(m.stateDir, "vms", name)); err != nil { + return fmt.Errorf("removing vm dir: %w", err) + } + return nil + } + if exists && (inst.State == StateRunning || inst.State == StateStopping) { if err := m.Stop(name); err != nil { return fmt.Errorf("stopping VM during remove: %w", err) @@ -664,11 +914,6 @@ func (m *Manager) quarantine(name string, manifest *instanceManifest, err error) // writeVMConfig writes a Firecracker JSON config file for the given service. func (m *Manager) writeVMConfig(vmDir string, svc config.ServiceConfig, prepared []volume.PreparedVolume) (string, error) { - kernelArgs := svc.KernelArgs - if kernelArgs == "" { - kernelArgs = "console=ttyS0 reboot=k panic=1 pci=off" - } - sort.Slice(prepared, func(i, j int) bool { return prepared[i].LogicalID < prepared[j].LogicalID }) drives := []firecrackerDrive{{DriveID: "rootfs", PathOnHost: svc.Image, IsRootDevice: true, IsReadOnly: false}} guestVolumes := make([]guestVolume, 0, len(prepared)) @@ -686,13 +931,11 @@ func (m *Manager) writeVMConfig(vmDir string, svc config.ServiceConfig, prepared MountPath: preparedVolume.MountPath, Type: preparedVolume.Type, }) } - if len(guestVolumes) > 0 { - payload, err := json.Marshal(guestVolumePayload{Version: 1, Volumes: guestVolumes}) - if err != nil { - return "", fmt.Errorf("marshal guest volume payload: %w", err) - } - arg := "firework.volumes64=" + base64.RawURLEncoding.EncodeToString(payload) - kernelArgs = insertBeforeApplicationSeparator(kernelArgs, arg) + // The same builder Preflight uses, so an over-long command line now fails + // on create too rather than only on update. + kernelArgs, err := buildBootArgs(svc, guestVolumes) + if err != nil { + return "", err } var networkInterfaces []firecrackerNetworkInterface diff --git a/internal/vm/manager_test.go b/internal/vm/manager_test.go index 082241c..1197809 100644 --- a/internal/vm/manager_test.go +++ b/internal/vm/manager_test.go @@ -411,7 +411,7 @@ func TestPreflightRetainsVisibleVolumeError(t *testing.T) { service := config.ServiceConfig{Name: "app", Volumes: []config.VolumeConfig{{ Name: "data", Type: config.VolumeTypeLocal, MountPath: "/data", }}} - if err := manager.Preflight(context.Background(), service); err == nil { + if _, err := manager.Preflight(context.Background(), service); err == nil { t.Fatal("expected missing storage error") } if got := manager.VolumeError("app"); !strings.Contains(got, "storage is not configured") { diff --git a/internal/vm/start_barrier_test.go b/internal/vm/start_barrier_test.go new file mode 100644 index 0000000..a79a979 --- /dev/null +++ b/internal/vm/start_barrier_test.go @@ -0,0 +1,412 @@ +package vm + +import ( + "context" + "errors" + "io" + "log/slog" + "os" + "path/filepath" + "strings" + "sync" + "syscall" + "testing" + "time" + + "github.com/artemnikitin/firework/internal/config" + "github.com/artemnikitin/firework/internal/volume" +) + +// blockingRunner parks inside the first filesystem-mutating command, which is +// what a multi-minute mkfs.ext4 or resize2fs looks like from the manager's +// point of view. +type blockingRunner struct { + entered chan struct{} + release chan struct{} + once sync.Once + fail error +} + +func (r *blockingRunner) Run(_ context.Context, name string, args ...string) ([]byte, error) { + if name == "resize2fs" && len(args) > 0 && args[0] == "-P" { + return []byte("Estimated minimum size of the filesystem: 1024\n"), nil + } + if name == "tune2fs" { + return []byte("Block size: 4096\n"), nil + } + return nil, nil +} + +func (r *blockingRunner) RunDestructive(ctx context.Context, name string, args ...string) ([]byte, error) { + r.once.Do(func() { + close(r.entered) + <-r.release + }) + if r.fail != nil { + return nil, r.fail + } + return r.Run(ctx, name, args...) +} + +type acceptingMounts struct{} + +func (acceptingMounts) Verify(string) error { return nil } + +// fakeRunner reports a fixed filesystem minimum, so a small shrink target is +// refused and a large one is accepted. +type fakeRunner struct{} + +func (fakeRunner) Run(_ context.Context, name string, args ...string) ([]byte, error) { + if name == "resize2fs" && len(args) > 0 && args[0] == "-P" { + return []byte("Estimated minimum size of the filesystem: 1024\n"), nil + } + if name == "tune2fs" { + return []byte("Block size: 4096\n"), nil + } + return nil, nil +} + +func (r fakeRunner) RunDestructive(ctx context.Context, name string, args ...string) ([]byte, error) { + return r.Run(ctx, name, args...) +} + +// countingLauncher fails the test's purpose loudly: phase 3 must not launch +// anything after an abort, so any launch at all is the defect. +type countingLauncher struct { + mu sync.Mutex + launches int +} + +func (l *countingLauncher) Launch(context.Context, launchSpec) (*launchedProcess, error) { + l.mu.Lock() + l.launches++ + l.mu.Unlock() + return nil, errors.New("launch should not have been reached") +} + +func (l *countingLauncher) Stop(*instanceManifest, syscall.Signal) error { return nil } + +func (l *countingLauncher) count() int { + l.mu.Lock() + defer l.mu.Unlock() + return l.launches +} + +func barrierManager(t *testing.T, runner volume.CommandRunner) (*Manager, *countingLauncher) { + t.Helper() + stateDir := t.TempDir() + pool := t.TempDir() + volumeMgr := volume.NewManagerWithDependencies("node-1", config.StorageConfig{ + Local: &config.LocalStorageConfig{Path: pool, CapacityBytes: 1 << 30}, + }, runner, acceptingMounts{}) + manager := NewManagerWithVolumes("/bin/true", stateDir, slog.New(slog.NewTextHandler(io.Discard, nil)), volumeMgr) + launcher := &countingLauncher{} + manager.launcher = launcher + return manager, launcher +} + +func barrierService() config.ServiceConfig { + return config.ServiceConfig{ + Name: "app", Image: "/image", Kernel: "/kernel", VCPUs: 1, MemoryMB: 128, + Volumes: []config.VolumeConfig{{ + Name: "data", Type: config.VolumeTypeLocal, MountPath: "/var/lib/app", + SizeBytes: 16 * config.MiB, BoundNode: "node-1", ResizeGeneration: 1, + }}, + } +} + +// The whole point of releasing the lock: a reader must not block behind a +// multi-minute volume operation, and what it reads must be truthful. +func TestListReportsStartingWhilePrepareRuns(t *testing.T) { + runner := &blockingRunner{entered: make(chan struct{}), release: make(chan struct{})} + manager, launcher := barrierManager(t, runner) + + done := make(chan error, 1) + go func() { done <- manager.Start(context.Background(), barrierService()) }() + <-runner.entered + + instance := manager.List()["app"] + if instance == nil || instance.State != StateStarting { + t.Fatalf("expected a starting placeholder during Prepare, got %#v", instance) + } + + close(runner.release) + <-done + if launcher.count() == 0 { + t.Fatal("expected the start to proceed to launch after Prepare returned") + } +} + +// A stop that arrives while volumes are being prepared must return at once — +// not wait out the mkfs — and the start must then launch nothing. +func TestStopDuringPrepareAbortsTheStart(t *testing.T) { + runner := &blockingRunner{entered: make(chan struct{}), release: make(chan struct{})} + manager, launcher := barrierManager(t, runner) + + done := make(chan error, 1) + go func() { done <- manager.Start(context.Background(), barrierService()) }() + <-runner.entered + + stopped := make(chan error, 1) + go func() { stopped <- manager.Stop("app") }() + select { + case err := <-stopped: + if err != nil { + t.Fatalf("stopping a starting service should succeed, got %v", err) + } + case <-time.After(5 * time.Second): + t.Fatal("Stop blocked behind volume preparation") + } + // A second stop must not see an error for work the first one already did. + if err := manager.Stop("app"); err != nil { + t.Fatalf("repeated stop while aborting should succeed, got %v", err) + } + + close(runner.release) + err := <-done + if !errors.Is(err, ErrStartAborted) { + t.Fatalf("expected ErrStartAborted, got %v", err) + } + if launcher.count() != 0 { + t.Fatalf("aborted start launched %d process(es)", launcher.count()) + } + if instance := manager.List()["app"]; instance != nil { + t.Fatalf("expected the placeholder to be cleaned up, got %#v", instance) + } +} + +func TestRemoveDuringPrepareAbortsTheStartAndClearsState(t *testing.T) { + runner := &blockingRunner{entered: make(chan struct{}), release: make(chan struct{})} + manager, launcher := barrierManager(t, runner) + vmDir := filepath.Join(manager.stateDir, "vms", "app") + + done := make(chan error, 1) + go func() { done <- manager.Start(context.Background(), barrierService()) }() + <-runner.entered + + removed := make(chan error, 1) + go func() { removed <- manager.Remove("app") }() + select { + case err := <-removed: + if err != nil { + t.Fatalf("removing a starting service should succeed, got %v", err) + } + case <-time.After(5 * time.Second): + t.Fatal("Remove blocked behind volume preparation") + } + if err := manager.Remove("app"); err != nil { + t.Fatalf("repeated remove while aborting should succeed, got %v", err) + } + if _, err := os.Stat(vmDir); !os.IsNotExist(err) { + t.Fatalf("expected the VM state directory to be removed, got %v", err) + } + + close(runner.release) + if err := <-done; !errors.Is(err, ErrStartAborted) { + t.Fatalf("expected ErrStartAborted, got %v", err) + } + if launcher.count() != 0 { + t.Fatalf("aborted start launched %d process(es)", launcher.count()) + } + if instance := manager.List()["app"]; instance != nil { + t.Fatalf("expected the placeholder to be cleaned up, got %#v", instance) + } +} + +// Phase 3 validates the startID, not the service name, so a placeholder that +// was cleared and replaced by a later attempt is never mistaken for its own. +func TestPhaseThreeIgnoresAPlaceholderFromAnotherAttempt(t *testing.T) { + runner := &blockingRunner{entered: make(chan struct{}), release: make(chan struct{})} + manager, launcher := barrierManager(t, runner) + + done := make(chan error, 1) + go func() { done <- manager.Start(context.Background(), barrierService()) }() + <-runner.entered + + // Simulate the placeholder being taken over by a later start. + manager.mu.Lock() + manager.instances["app"] = &Instance{Name: "app", State: StateStarting, startID: "someone-else"} + manager.mu.Unlock() + + close(runner.release) + if err := <-done; !errors.Is(err, ErrStartAborted) { + t.Fatalf("expected ErrStartAborted, got %v", err) + } + if launcher.count() != 0 { + t.Fatalf("start touched the launcher despite losing its placeholder (%d launches)", launcher.count()) + } + if instance := manager.List()["app"]; instance == nil || instance.startID != "someone-else" { + t.Fatalf("expected the other attempt's placeholder to survive, got %#v", instance) + } +} + +func TestFailedPrepareLeavesNoPlaceholder(t *testing.T) { + runner := &blockingRunner{entered: make(chan struct{}), release: make(chan struct{}), fail: errors.New("mkfs failed")} + manager, launcher := barrierManager(t, runner) + + done := make(chan error, 1) + go func() { done <- manager.Start(context.Background(), barrierService()) }() + <-runner.entered + close(runner.release) + + err := <-done + if err == nil || !strings.Contains(err.Error(), "preparing volumes") { + t.Fatalf("expected a volume preparation failure, got %v", err) + } + if instance := manager.List()["app"]; instance != nil { + t.Fatalf("expected no placeholder after a failed Prepare, got %#v", instance) + } + if launcher.count() != 0 { + t.Fatalf("failed Prepare still launched %d process(es)", launcher.count()) + } + if manager.VolumeError("app") == "" { + t.Fatal("expected the preparation failure to stay visible as a volume error") + } +} + +// The phase-1 entry check treats both starting states as active, so the agent +// API and the shutdown path cannot start a second VM for the same service. +func TestConcurrentStartIsRejectedWhileStarting(t *testing.T) { + runner := &blockingRunner{entered: make(chan struct{}), release: make(chan struct{})} + manager, _ := barrierManager(t, runner) + + done := make(chan error, 1) + go func() { done <- manager.Start(context.Background(), barrierService()) }() + <-runner.entered + + second := manager.Start(context.Background(), barrierService()) + if !errors.Is(second, ErrStartInProgress) { + t.Fatalf("expected ErrStartInProgress, got %v", second) + } + if !IsStartRace(second) { + t.Fatal("a rejected concurrent start must classify as a benign race") + } + + close(runner.release) + <-done +} + +// An over-long command line must fail on create, not only on update: the +// launch path and Preflight now build the args through the same function. +func TestOverlongKernelCommandLineFailsOnCreate(t *testing.T) { + manager, _ := barrierManager(t, &fakeVolumeRunner{}) + svc := barrierService() + svc.KernelArgs = strings.Repeat("x", maxKernelCommandLineBytes) + + _, err := manager.writeVMConfig(t.TempDir(), svc, []volume.PreparedVolume{{ + LogicalID: "app/data", PathOnHost: "/pool/app/data/volume.ext4", + MountPath: "/var/lib/app", Type: config.VolumeTypeLocal, SizeBytes: 16 * config.MiB, + }}) + if err == nil || !strings.Contains(err.Error(), "kernel command line") { + t.Fatalf("expected the create path to enforce the command-line limit, got %v", err) + } +} + +// Preflight rejects before a running VM is touched, and the launch path +// rejects before it boots something unbootable. They must agree exactly. +func TestPreflightAndWriteVMConfigBuildIdenticalBootArgs(t *testing.T) { + svc := barrierService() + svc.Volumes = append(svc.Volumes, config.VolumeConfig{ + Name: "cache", Type: config.VolumeTypeLocal, MountPath: "/var/cache/app", + SizeBytes: 8 * config.MiB, BoundNode: "node-1", ResizeGeneration: 1, + }) + + declared, err := guestVolumesFromConfig(svc.Volumes) + if err != nil { + t.Fatal(err) + } + fromConfig, err := buildBootArgs(svc, declared) + if err != nil { + t.Fatal(err) + } + + manager, _ := barrierManager(t, &fakeVolumeRunner{}) + vmDir := t.TempDir() + prepared := []volume.PreparedVolume{ + {LogicalID: "app/data", PathOnHost: "/pool/app/data/volume.ext4", MountPath: "/var/lib/app", Type: config.VolumeTypeLocal}, + {LogicalID: "app/cache", PathOnHost: "/pool/app/cache/volume.ext4", MountPath: "/var/cache/app", Type: config.VolumeTypeLocal}, + } + if _, err := manager.writeVMConfig(vmDir, svc, prepared); err != nil { + t.Fatal(err) + } + written, err := os.ReadFile(filepath.Join(vmDir, "vm-config.json")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(written), jsonEscape(fromConfig)) { + t.Fatalf("preflight and launch boot args differ.\npreflight: %s\nwritten: %s", fromConfig, written) + } +} + +// jsonEscape renders a boot-args string the way it appears inside the +// Firecracker config document. +func jsonEscape(value string) string { + return strings.ReplaceAll(value, `"`, `\"`) +} + +// fakeVolumeRunner satisfies the runner interface for tests that never reach a +// real filesystem operation. +type fakeVolumeRunner struct{} + +func (fakeVolumeRunner) Run(context.Context, string, ...string) ([]byte, error) { return nil, nil } +func (fakeVolumeRunner) RunDestructive(context.Context, string, ...string) ([]byte, error) { + return nil, nil +} + +// The acknowledged form of a refused shrink must converge. +// +// After the control plane acknowledges a rejection it renders the *effective* +// size with the *refused* generation — it keeps that generation so the +// acknowledgement can match its own record. The running instance, meanwhile, +// carries the applied generation. needsUpdate compares whole VolumeConfig +// structs, so unless normalization reconciles the two the reconciler plans an +// update, stops the VM, and restarts it — every tick that reaches Plan. +func TestAcknowledgedRejectionConvergesWithTheRunningConfig(t *testing.T) { + manager, _ := barrierManager(t, &fakeRunner{}) + svc := barrierService() + + if _, err := manager.volumeManager.Prepare(context.Background(), svc); err != nil { + t.Fatal(err) + } + // A shrink the fake measurement refuses, at generation 2. + refused := svc + refused.Volumes = append([]config.VolumeConfig(nil), svc.Volumes...) + refused.Volumes[0].SizeBytes = 2 * config.MiB + refused.Volumes[0].ResizeGeneration = 2 + prepared, err := manager.volumeManager.Prepare(context.Background(), refused) + if err != nil { + t.Fatal(err) + } + if !prepared[0].Rejected { + t.Fatalf("precondition: expected the shrink to be refused, got %#v", prepared[0]) + } + + // What Start stores on the instance: the effective configuration. + running := clampToPrepared(refused, prepared) + + // What the control plane renders once it has acknowledged the rejection. + acknowledged := svc + acknowledged.Volumes = append([]config.VolumeConfig(nil), svc.Volumes...) + acknowledged.Volumes[0].SizeBytes = 16 * config.MiB + acknowledged.Volumes[0].ResizeGeneration = 2 + + services := []config.ServiceConfig{acknowledged} + manager.NormalizeVolumes(services) + + // volumesEqual compares whole VolumeConfig structs, so this equality is + // exactly the condition under which no ActionUpdate is planned. + if services[0].Volumes[0] != running.Volumes[0] { + t.Fatalf("the rendered config does not match the running one, so an update would be re-planned:\nrendered %#v\nrunning %#v", + services[0].Volumes[0], running.Volumes[0]) + } + + // The agent stops reporting a refusal here, and that is deliberate. These + // bytes are exactly what a direct-Git operator writes to *withdraw* the + // request, so the agent cannot tell a standing request from a withdrawn + // one and must not degrade the node forever on the ambiguity. Only the + // record still knows the operator's request, so that half of the + // visibility is the control plane's — see refusedVolumes there. + if got := manager.VolumeRejections(); len(got) != 0 { + t.Fatalf("the acknowledged shape must not keep reporting a refusal: %#v", got) + } +} diff --git a/internal/volume/hardening_test.go b/internal/volume/hardening_test.go new file mode 100644 index 0000000..56c1787 --- /dev/null +++ b/internal/volume/hardening_test.go @@ -0,0 +1,274 @@ +package volume + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/artemnikitin/firework/internal/config" +) + +func hardeningManager(t *testing.T, runner CommandRunner) (*Manager, string) { + t.Helper() + root := t.TempDir() + return NewManagerWithDependencies("node-1", config.StorageConfig{Local: &config.LocalStorageConfig{ + Path: root, CapacityBytes: 100 * config.MiB, + }}, runner, acceptingMounts{}), root +} + +func volumePaths(root string) (dir, image, manifest, marker string) { + dir = filepath.Join(root, "app", "data") + return dir, filepath.Join(dir, imageFilename), filepath.Join(dir, manifestFilename), filepath.Join(dir, creationMarkerFilename) +} + +// crashingRunner stops the create sequence at a chosen command, standing in for +// a process that died partway through a first creation. +type crashingRunner struct { + fakeRunner + failOn string +} + +func (r *crashingRunner) RunDestructive(ctx context.Context, name string, args ...string) ([]byte, error) { + if name == r.failOn { + return nil, errors.New("simulated crash") + } + return r.fakeRunner.RunDestructive(ctx, name, args...) +} + +// A crash after the image is created but before the manifest is written leaves +// an empty volume. Nothing is protected by failing closed there, so the marker +// makes it recoverable without an operator rm. +func TestInterruptedCreationRecoversWithoutOperatorIntervention(t *testing.T) { + runner := &crashingRunner{failOn: "mkfs.ext4"} + manager, root := hardeningManager(t, runner) + dir, image, manifest, marker := volumePaths(root) + + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err == nil { + t.Fatal("expected the simulated crash to fail the first creation") + } + if _, err := os.Stat(image); err != nil { + t.Fatalf("expected the partially created image to remain: %v", err) + } + if _, err := os.Stat(marker); err != nil { + t.Fatalf("expected a creation marker to record the interrupted attempt: %v", err) + } + + manager, _ = hardeningManager(t, &fakeRunner{}) + manager.storage.Local.Path = root + prepared, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)) + if err != nil { + t.Fatalf("expected the interrupted creation to be recoverable: %v", err) + } + if len(prepared) != 1 || prepared[0].SizeBytes != 16*config.MiB { + t.Fatalf("unexpected recovered volume: %#v", prepared) + } + if _, err := os.Stat(manifest); err != nil { + t.Fatalf("expected a manifest after recovery: %v", err) + } + if _, err := os.Stat(marker); !os.IsNotExist(err) { + t.Fatalf("expected the marker to be removed after a successful creation: %v", err) + } + _ = dir +} + +// A crash between the image being sized and mkfs running is the same case one +// step earlier, and must recover the same way. +func TestInterruptedCreationBeforeMkfsRecovers(t *testing.T) { + manager, root := hardeningManager(t, &fakeRunner{}) + dir, image, _, _ := volumePaths(root) + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatal(err) + } + if err := writeCreationMarker(dir, "app", localService(16*config.MiB, 1).Volumes[0], "node-1"); err != nil { + t.Fatal(err) + } + if err := createSparseImage(image, 16*config.MiB); err != nil { + t.Fatal(err) + } + + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatalf("expected recovery from a sized-but-unformatted image: %v", err) + } +} + +// The manifest was written but the process died before the marker was removed. +// This is ordinary reuse — and the marker must not survive it, because a later +// manifest loss would otherwise make a populated image look like an +// interrupted empty creation and authorize deleting it. +func TestSurvivingMarkerIsClearedOnReuse(t *testing.T) { + manager, root := hardeningManager(t, &fakeRunner{}) + dir, _, _, marker := volumePaths(root) + + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + // Recreate the state a crash between the manifest write and the marker + // removal leaves behind. + if err := writeCreationMarker(dir, "app", localService(16*config.MiB, 1).Volumes[0], "node-1"); err != nil { + t.Fatal(err) + } + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatalf("reuse failed: %v", err) + } + if _, err := os.Stat(marker); !os.IsNotExist(err) { + t.Fatalf("expected reuse to clear a marker that outlived its condition: %v", err) + } +} + +// An image Firework did not create is exactly what failing closed is for, and +// a marker that names a different node or volume proves nothing about this one. +func TestUnprovenImageStaysQuarantined(t *testing.T) { + tests := []struct { + name string + marker *creationMarker + }{ + {name: "no marker"}, + {name: "marker names another node", marker: &creationMarker{LogicalID: "app/data", NodeID: "node-2"}}, + {name: "marker names another volume", marker: &creationMarker{LogicalID: "other/data", NodeID: "node-1"}}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + manager, root := hardeningManager(t, &fakeRunner{}) + dir, image, _, marker := volumePaths(root) + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatal(err) + } + if err := createSparseImage(image, 16*config.MiB); err != nil { + t.Fatal(err) + } + if test.marker != nil { + test.marker.CreatedAt = time.Now().UTC() + if err := writeJSONAtomic(marker, *test.marker); err != nil { + t.Fatal(err) + } + } + _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)) + if err == nil || !strings.Contains(err.Error(), "quarantined") { + t.Fatalf("expected the volume to stay quarantined, got %v", err) + } + }) + } +} + +// Filesystem-mutating commands must not run on a context the agent's signal +// handler cancels, because exec.CommandContext cancellation is SIGKILL. +func TestDestructiveCommandsDoNotTakeTheCancellablePath(t *testing.T) { + runner := &fakeRunner{} + manager, _ := hardeningManager(t, runner) + + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + if _, err := manager.Prepare(context.Background(), localService(24*config.MiB, 2)); err != nil { + t.Fatal(err) + } + if _, err := manager.Prepare(context.Background(), localService(20*config.MiB, 3)); err != nil { + t.Fatal(err) + } + + destructive := strings.Join(runner.destructive, "\n") + for _, want := range []string{"mkfs.ext4", "e2fsck", "resize2fs"} { + if !strings.Contains(destructive, want) { + t.Fatalf("expected %s to run on the uncancellable path, got:\n%s", want, destructive) + } + } + // The shrink measurement is read-only and must stay promptly cancellable. + for _, call := range runner.destructive { + if strings.HasPrefix(call, "resize2fs -P") || strings.HasPrefix(call, "tune2fs") { + t.Fatalf("measurement command %q must not be detached from the caller's context", call) + } + } +} + +// A cancelled parent context must not stop a destructive command, which is the +// whole reason RunDestructive exists. +func TestRunDestructiveSurvivesParentCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + if _, err := (execRunner{}).RunDestructive(ctx, "true"); err != nil { + t.Fatalf("destructive command was killed by the cancelled parent context: %v", err) + } + if _, err := (execRunner{}).Run(ctx, "true"); err == nil { + t.Fatal("expected a read-only command to remain promptly cancellable") + } +} + +// A retained manifest carrying a non-positive applied size subtracts from the +// pool's reserved total, admitting a volume the pool cannot hold. +func TestMalformedRetainedSizeCannotBypassPoolCapacity(t *testing.T) { + manager, root := hardeningManager(t, &fakeRunner{}) + // A neighbouring service's retained manifest with a negative applied size. + dir := filepath.Join(root, "other", "data") + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatal(err) + } + if err := writeJSONAtomic(filepath.Join(dir, manifestFilename), manifest{ + LogicalID: "other/data", Type: config.VolumeTypeLocal, BoundNode: "node-1", + Filesystem: "ext4", AppliedSizeBytes: -100 * config.MiB, ResizeGeneration: 1, + }); err != nil { + t.Fatal(err) + } + + // The pool is 100 MiB; 150 MiB must not fit regardless of the bad record. + _, err := manager.Preflight(context.Background(), localService(150*config.MiB, 1)) + if err == nil { + t.Fatal("a negative retained size let an oversized volume into the pool") + } + if !strings.Contains(err.Error(), "capacity") && !strings.Contains(err.Error(), "quarantined") { + t.Fatalf("expected a capacity or quarantine failure, got %v", err) + } +} + +// The agent derives a filesystem path from the service name, so a name that is +// not a safe path component fails at preflight. configcheck must reject it too. +func TestServiceNameIsValidatedByTheExportedValidator(t *testing.T) { + nc := config.NodeConfig{Node: "node-1", Services: []config.ServiceConfig{{ + Name: "bad/name", Image: "/i", Kernel: "/k", VCPUs: 1, MemoryMB: 128, + Volumes: []config.VolumeConfig{{ + Name: "data", Type: config.VolumeTypeLocal, MountPath: "/var/lib/app", + SizeBytes: config.MiB, BoundNode: "node-1", ResizeGeneration: 1, + }}, + }}} + if err := ValidateNodeVolumes(nc); err == nil { + t.Fatal("a service name that is not a safe path component must be rejected") + } +} + +// readRetained keys the pool's reservation map by the manifest's *declared* +// LogicalID rather than by its path. Two manifests claiming the same logical ID +// therefore collapse to one entry, and the other volume's bytes vanish from the +// reserved total — a capacity bypass from state the node already holds. +func TestRetainedManifestCannotMaskAnotherReservation(t *testing.T) { + manager, root := hardeningManager(t, &fakeRunner{}) + + // Two real, separate retained volumes, each 40 MiB in a 100 MiB pool. + for _, svc := range []string{"alpha", "beta"} { + dir := filepath.Join(root, svc, "data") + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatal(err) + } + // beta's manifest lies about its identity and claims alpha's. + logicalID := svc + "/data" + if svc == "beta" { + logicalID = "alpha/data" + } + if err := writeJSONAtomic(filepath.Join(dir, manifestFilename), manifest{ + LogicalID: logicalID, Type: config.VolumeTypeLocal, BoundNode: "node-1", + Filesystem: "ext4", AppliedSizeBytes: 40 * config.MiB, ResizeGeneration: 1, + }); err != nil { + t.Fatal(err) + } + } + + // 80 MiB is genuinely retained. A new 40 MiB volume would need 120 MiB of a + // 100 MiB pool and must be refused. + _, err := manager.Preflight(context.Background(), localService(40*config.MiB, 1)) + if err == nil { + t.Fatal("a mislabelled retained manifest masked another volume's reservation") + } +} diff --git a/internal/volume/manager.go b/internal/volume/manager.go index f140a11..c136485 100644 --- a/internal/volume/manager.go +++ b/internal/volume/manager.go @@ -15,6 +15,7 @@ import ( "sort" "strconv" "strings" + "sync" "syscall" "time" @@ -25,6 +26,10 @@ const ( manifestFilename = "manifest.json" transactionFilename = "resize-transaction.json" imageFilename = "volume.ext4" + // creationMarkerFilename records that a first creation is in flight. It is + // what separates "we crashed while making an empty image" from "an image + // Firework did not create", which the manifest's absence alone cannot. + creationMarkerFilename = "creating.json" ) var ( @@ -34,14 +39,68 @@ var ( ErrSharedUnsupported = errors.New("shared volumes require the durable per-VM supervisor and fencing validation") ) +// ErrShrinkRejected reports that a requested shrink is below the safe minimum +// for the filesystem's current contents. It is a *decision*, not a fault: the +// distinction is what lets the caller keep the workload running instead of +// treating a refusal like a failed operation. +// +// LogicalID is carried because two volumes sharing a size and generation are +// otherwise indistinguishable, and the clamp cannot tell which one it applies +// to. +type ErrShrinkRejected struct { + LogicalID string + Requested int64 + Minimum int64 + Generation int64 +} + +func (e *ErrShrinkRejected) Error() string { + return fmt.Sprintf("volume %s: shrink target %d is below safe minimum %d", e.LogicalID, e.Requested, e.Minimum) +} + +// Rejection is a durable refusal of one volume's size request, as reported to +// status and consumed by the agent-side clamp. +type Rejection struct { + LogicalID string + // ResizeGeneration is the *requested* generation — the one that was + // refused. It is what a reported rejection must carry so the control + // plane's acknowledgement can match it to the record it has to converge. + ResizeGeneration int64 + // AppliedGeneration is the generation actually applied to the filesystem. + // Together with AppliedSizeBytes it is the *effective* configuration: what + // the node is running, what Plan compares against, and what the clamp + // substitutes. Keeping the two apart is what lets one rejection be both + // terminal locally and matchable remotely. + AppliedGeneration int64 + RequestedSizeBytes int64 + AppliedSizeBytes int64 + MinimumSizeBytes int64 + At time.Time +} + // PreparedVolume is safe to attach to a stopped/new Firecracker process. type PreparedVolume struct { - LogicalID string - PathOnHost string - MountPath string - Type config.VolumeType - SizeBytes int64 + LogicalID string + PathOnHost string + MountPath string + Type config.VolumeType + // SizeBytes is the *effective* size: what the image actually is. For a + // rejected shrink this is the applied size, not the refused request. + SizeBytes int64 + // ResizeGeneration is always the generation actually applied to the + // filesystem. Together with SizeBytes it is the effective configuration + // the caller stores on the instance, which is what makes the next tick + // compare equal instead of re-planning the same update. ResizeGeneration int64 + // Rejected marks a preparation that succeeded at a size other than the one + // requested. It is not an error, so Prepare continues to the next volume + // and one pass collects every rejection. + Rejected bool + // RequestedGeneration and RequestedSizeBytes describe the refused request. + // They are reported rather than run. + RequestedGeneration int64 + RequestedSizeBytes int64 + MinimumSizeBytes int64 } // Status is the agent-observed state of one logical volume. @@ -64,7 +123,102 @@ type manifest struct { Filesystem string `json:"filesystem"` AppliedSizeBytes int64 `json:"applied_size_bytes"` ResizeGeneration int64 `json:"resize_generation"` - UpdatedAt time.Time `json:"updated_at"` + // The rejection is keyed to one (generation, size) request. Recording it + // durably is what makes the refusal terminal without depending on the + // control plane: the agent-side clamp reads it locally, so the stop/restart + // loop is broken even if the acknowledgement never lands. + // + // ResizeGeneration deliberately continues to describe the last generation + // actually applied to the filesystem. Advancing it here would erase the + // evidence that this generation was refused. + RejectedGeneration int64 `json:"rejected_generation,omitempty"` + RejectedSizeBytes int64 `json:"rejected_size_bytes,omitempty"` + RejectedMinimumBytes int64 `json:"rejected_minimum_bytes,omitempty"` + RejectedAt time.Time `json:"rejected_at,omitempty"` + UpdatedAt time.Time `json:"updated_at"` +} + +// rejectionFor builds the reported rejection for a manifest that carries one. +func (m manifest) rejectionFor(generation int64) Rejection { + return Rejection{ + LogicalID: m.LogicalID, ResizeGeneration: generation, AppliedGeneration: m.ResizeGeneration, + RequestedSizeBytes: m.RejectedSizeBytes, AppliedSizeBytes: m.AppliedSizeBytes, + MinimumSizeBytes: m.RejectedMinimumBytes, At: m.RejectedAt, + } +} + +// matchesRejection reports whether a desired volume config is a request this +// manifest already refused. +// +// The generation must always match. A generation-only match is not enough: +// direct-Git node configs are hand-authored and carry their own +// resize_generation, so an operator correcting a refused shrink by editing +// size_bytes alone presents a different request under the same generation, and +// a generation-only match would clamp that forever. +// +// Two sizes carry the same refused request, and both must be recognized: +// +// - the refused size itself, which is what a direct-Git config renders and +// what the control plane renders until it has acknowledged the refusal; +// - the applied size, which is what the control plane renders *after* +// acknowledging it — the clamp there substitutes the effective size but +// keeps the refused generation, because the acknowledgement has to be able +// to match that generation to its record. +// +// Recognizing only the first leaves the second carrying the refused generation +// while the running instance carries the applied one. needsUpdate compares +// whole volume configs, so the service is stopped and restarted on every +// reconcile that reaches Plan — the loop this whole mechanism exists to end. +func (m manifest) matchesRejection(volume config.VolumeConfig) bool { + if m.RejectedGeneration == 0 || m.RejectedGeneration != volume.ResizeGeneration { + return false + } + return volume.SizeBytes == m.RejectedSizeBytes || volume.SizeBytes == m.AppliedSizeBytes +} + +// refusesRequest reports whether the config in front of the agent is still +// asking for the size that was refused. +// +// This is deliberately narrower than matchesRejection, and the two must not be +// conflated. Clamping has to keep applying to both shapes for as long as the +// refused generation stands, or the generation diverges from the running +// instance and the service restarts on every reconcile. But a *report* of a +// standing refusal is only true while the refused size is actually being +// requested: once the config asks for the size already running — because a +// direct-Git operator withdrew the request, or the control plane acknowledged +// the refusal and now renders the effective size — nothing is being refused +// here any more. Reporting one anyway leaves the node degraded forever with no +// exit but a generation bump. +// +// After the control plane acknowledges, the two shapes become identical bytes +// and the agent genuinely cannot tell whether the operator still wants the +// refused size. Only the record knows, so that half of the visibility belongs +// to the control plane; see §7.3.2 of the hardening plan. +func (m manifest) refusesRequest(volume config.VolumeConfig) bool { + return m.RejectedGeneration != 0 && + m.RejectedGeneration == volume.ResizeGeneration && + m.RejectedSizeBytes == volume.SizeBytes +} + +func (m *manifest) clearRejection() { + m.RejectedGeneration = 0 + m.RejectedSizeBytes = 0 + m.RejectedMinimumBytes = 0 + m.RejectedAt = time.Time{} +} + +// creationMarker is written before the backing image and removed after the +// manifest. Its presence authorizes deleting an image that has no manifest, so +// its lifetime is deliberately bounded by the condition it describes: every +// path that reads a valid manifest removes a matching marker (see +// clearStaleCreationMarker). A marker that outlived a successful creation would +// otherwise authorize destroying populated data if the manifest were later lost. +type creationMarker struct { + LogicalID string `json:"logical_id"` + NodeID string `json:"node_id"` + TargetSizeBytes int64 `json:"target_size_bytes"` + ResizeGeneration int64 `json:"resize_generation"` + CreatedAt time.Time `json:"created_at"` } type resizeTransaction struct { @@ -76,15 +230,62 @@ type resizeTransaction struct { UpdatedAt time.Time `json:"updated_at"` } +// destructiveCommandTimeout bounds a filesystem-mutating command that has been +// detached from the caller's context. It has to accommodate mkfs, e2fsck, and +// resize2fs on a pool-sized image, so it is generous: the point is that the +// operation is not killed by an agent restart, not that it is killed promptly. +// +// Detaching from the Go context is only half of the protection, and the other +// half is not in this repository. Under systemd's default +// KillMode=control-group, stopping the agent's unit signals every process in +// its cgroup — including this child — and force-kills the group at +// TimeoutStopSec. The unit must set KillMode=mixed and a TimeoutStopSec above +// this value; see docs/persistent-volumes.md. Raising this constant without +// raising that one reopens the gap it exists to close. +const destructiveCommandTimeout = 30 * time.Minute + +// destructiveCommandGrace is how long a timed-out destructive command is given +// to handle SIGTERM before the process group is killed. +const destructiveCommandGrace = 10 * time.Second + // CommandRunner isolates filesystem utilities for unit tests. +// +// The split between Run and RunDestructive is the interface's whole point, and +// it lives here rather than in a name match inside the runner so a new +// filesystem-mutating command cannot inherit the cancellable path by omission. type CommandRunner interface { + // Run executes a read-only measurement command. It keeps the caller's + // context and stays promptly cancellable. Run(context.Context, string, ...string) ([]byte, error) + // RunDestructive executes a command that mutates a filesystem. It must not + // be killed when the caller's context is cancelled: the agent's context is + // cancelled on SIGINT/SIGTERM, and exec.CommandContext cancellation is + // SIGKILL, so a systemd restart or node drain during a shrink would + // SIGKILL resize2fs mid-operation. + RunDestructive(context.Context, string, ...string) ([]byte, error) } type execRunner struct{} func (execRunner) Run(ctx context.Context, name string, args ...string) ([]byte, error) { - output, err := exec.CommandContext(ctx, name, args...).CombinedOutput() + return runCommand(exec.CommandContext(ctx, name, args...), name, args) +} + +func (execRunner) RunDestructive(ctx context.Context, name string, args ...string) ([]byte, error) { + // WithoutCancel keeps the values (and therefore any tracing) from the + // caller's context while detaching it from the SIGTERM cancellation chain. + // The command then gets its own absolute deadline, and that deadline is a + // SIGTERM with a grace period rather than an unconditional SIGKILL. + detached, cancel := context.WithTimeout(context.WithoutCancel(ctx), destructiveCommandTimeout) + defer cancel() + cmd := exec.CommandContext(detached, name, args...) + cmd.Cancel = func() error { return cmd.Process.Signal(syscall.SIGTERM) } + cmd.WaitDelay = destructiveCommandGrace + return runCommand(cmd, name, args) +} + +func runCommand(cmd *exec.Cmd, name string, args []string) ([]byte, error) { + output, err := cmd.CombinedOutput() if err != nil { return output, fmt.Errorf("%s %s: %w: %s", name, strings.Join(args, " "), err, strings.TrimSpace(string(output))) } @@ -139,10 +340,19 @@ type Manager struct { runner CommandRunner mounts MountVerifier observer Observer + + // rejections is the synchronized per-volume refusal snapshot, updated + // wherever a rejection is recorded — preflight or post-stop. Status reads + // it directly rather than inferring state from a running instance's + // prepared volumes, because a preflight rejection produces no fresh + // preparation to read: it fails the update before anything is stopped, so + // the instance still describes the *previous* preparation. + rejectionMu sync.RWMutex + rejections map[string]Rejection } func NewManager(nodeID string, storage config.StorageConfig) *Manager { - return &Manager{nodeID: nodeID, storage: storage, runner: execRunner{}, mounts: procMountVerifier{}} + return &Manager{nodeID: nodeID, storage: storage, runner: execRunner{}, mounts: procMountVerifier{}, rejections: make(map[string]Rejection)} } func NewManagerWithObserver(nodeID string, storage config.StorageConfig, observer Observer) *Manager { @@ -152,73 +362,145 @@ func NewManagerWithObserver(nodeID string, storage config.StorageConfig, observe } func NewManagerWithDependencies(nodeID string, storage config.StorageConfig, runner CommandRunner, mounts MountVerifier) *Manager { - return &Manager{nodeID: nodeID, storage: storage, runner: runner, mounts: mounts} + return &Manager{nodeID: nodeID, storage: storage, runner: runner, mounts: mounts, rejections: make(map[string]Rejection)} } -// Preflight validates every declaration and retained image without mutating it. -func (m *Manager) Preflight(ctx context.Context, svc config.ServiceConfig) error { - _ = ctx +// Preflight validates every declaration and retained image without mutating it, +// apart from recording a refusal. +// +// It returns the rejections it found alongside its error rather than returning +// on the first one. A rejection is a decision and a failure is a fault: only +// the latter aborts the batch, so one pass collects every refusal and the +// caller never has to retry per volume. +func (m *Manager) Preflight(ctx context.Context, svc config.ServiceConfig) ([]Rejection, error) { if len(svc.Volumes) == 0 { - return nil + return nil, nil } if err := validateServiceVolumes(svc.Volumes); err != nil { - return fmt.Errorf("service %s: %w", svc.Name, err) + return nil, fmt.Errorf("service %s: %w", svc.Name, err) } + var rejections []Rejection desiredLocal := make(map[string]int64) for _, volume := range svc.Volumes { logicalID := svc.Name + "/" + volume.Name switch volume.Type { case config.VolumeTypeLocal: if m.storage.Local == nil { - return fmt.Errorf("volume %s: storage.local is not configured", logicalID) + return rejections, fmt.Errorf("volume %s: storage.local is not configured", logicalID) } if volume.BoundNode == "" || volume.BoundNode != m.nodeID { - return fmt.Errorf("volume %s: bound_node %q does not match node %q", logicalID, volume.BoundNode, m.nodeID) + return rejections, fmt.Errorf("volume %s: bound_node %q does not match node %q", logicalID, volume.BoundNode, m.nodeID) } if m.mounts != nil { if err := m.mounts.Verify(m.storage.Local.Path); err != nil { - return fmt.Errorf("volume %s: verify local storage: %w", logicalID, err) + return rejections, fmt.Errorf("volume %s: verify local storage: %w", logicalID, err) } } desiredLocal[logicalID] = volume.SizeBytes if err := m.validateExisting(svc.Name, volume, m.storage.Local.Path); err != nil { - return err + return rejections, err + } + rejection, err := m.preflightResize(ctx, svc.Name, volume, m.storage.Local.Path) + if err != nil { + return rejections, err } - if err := m.preflightResize(ctx, svc.Name, volume, m.storage.Local.Path); err != nil { - return err + if rejection != nil { + // The effective size is what capacity should be checked + // against; charging the refused request would reject a + // configuration the node is already running. + desiredLocal[logicalID] = rejection.AppliedSizeBytes + rejections = append(rejections, *rejection) } case config.VolumeTypeShared: - return fmt.Errorf("volume %s: %w", logicalID, ErrSharedUnsupported) + return rejections, fmt.Errorf("volume %s: %w", logicalID, ErrSharedUnsupported) default: - return fmt.Errorf("volume %s: unsupported type %q", logicalID, volume.Type) + return rejections, fmt.Errorf("volume %s: unsupported type %q", logicalID, volume.Type) } } if len(desiredLocal) > 0 { if err := m.checkCapacity(m.storage.Local, desiredLocal); err != nil { - return err + return rejections, err } } - return nil + m.refreshRejections(svc) + return rejections, nil } -func (m *Manager) preflightResize(ctx context.Context, service string, volume config.VolumeConfig, root string) error { +// preflightResize measures a requested shrink before anything is stopped, and +// records a refusal durably so the refusal is terminal rather than re-measured +// on every tick forever. +// +// The measurement is advisory: a live resize2fs -P errs in both directions, +// because guest deletions whose bitmap updates are still in the page cache read +// too large and guest writes not yet flushed read too small. Terminality is +// still the right call, because the costs are asymmetric — a false refusal +// costs the operator one re-request, which mints a new generation and +// re-measures from scratch, while a non-terminal preflight costs an unbounded +// measurement loop on every tick. +// +// The whole read → measure → write sequence runs under the volume's lifecycle +// lock, the same lock prepareOne takes. Preflight used to be a pure reader; now +// that it writes the manifest it can interleave with a concurrent Prepare, and +// a measurement taken under one lock and written under another is the same lost +// update with extra steps. +func (m *Manager) preflightResize(ctx context.Context, service string, volume config.VolumeConfig, root string) (*Rejection, error) { dir, err := volumeDir(root, service, volume.Name) if err != nil { - return err + return nil, err + } + manifestPath := filepath.Join(dir, manifestFilename) + if _, statErr := os.Stat(manifestPath); statErr != nil { + if os.IsNotExist(statErr) { + return nil, nil + } + return nil, statErr + } + lock, err := lockFile(filepath.Join(dir, "lifecycle.lock")) + if err != nil { + return nil, err } + defer unlockFile(lock) + var current manifest - if err := readJSON(filepath.Join(dir, manifestFilename), ¤t); err != nil { + if err := readJSON(manifestPath, ¤t); err != nil { if os.IsNotExist(err) { - return nil + return nil, nil } - return err + return nil, err + } + if current.refusesRequest(volume) { + // Already refused, for exactly this request. Re-measuring would be the + // unbounded loop this record exists to stop. A config that merely + // carries the refused *generation* at the effective size is not a + // refusal — nothing is being asked for that was denied — and falls + // through to the size comparison below, which finds nothing to do. + rejection := current.rejectionFor(volume.ResizeGeneration) + return &rejection, nil } if volume.SizeBytes >= current.AppliedSizeBytes { - return nil + return nil, nil + } + err = m.inspectShrinkMinimum(ctx, service, volume, filepath.Join(dir, imageFilename)) + var rejected *ErrShrinkRejected + if errors.As(err, &rejected) { + // Nothing has been stopped and no resize has begun, so there is no + // transaction to clean up here — only the manifest write applies. + current.RejectedGeneration = volume.ResizeGeneration + current.RejectedSizeBytes = volume.SizeBytes + current.RejectedMinimumBytes = rejected.Minimum + current.RejectedAt = time.Now().UTC() + current.UpdatedAt = current.RejectedAt + if writeErr := writeJSONAtomic(manifestPath, current); writeErr != nil { + return nil, writeErr + } + if syncErr := syncDir(dir); syncErr != nil { + return nil, syncErr + } + rejection := current.rejectionFor(volume.ResizeGeneration) + return &rejection, nil } - imagePath := filepath.Join(dir, imageFilename) - return m.inspectShrinkMinimum(ctx, service, volume, imagePath) + return nil, err } func (m *Manager) inspectShrinkMinimum(ctx context.Context, service string, volume config.VolumeConfig, imagePath string) error { @@ -243,7 +525,10 @@ func (m *Manager) inspectShrinkMinimum(ctx context.Context, service string, volu // guarantee and can change after a final fsck. minimumWithHeadroom := minimumBytes + minimumBytes/20 if volume.SizeBytes < minimumWithHeadroom { - return fmt.Errorf("volume %s/%s: shrink target %d is below safe minimum %d", service, volume.Name, volume.SizeBytes, minimumWithHeadroom) + return &ErrShrinkRejected{ + LogicalID: service + "/" + volume.Name, Requested: volume.SizeBytes, + Minimum: minimumWithHeadroom, Generation: volume.ResizeGeneration, + } } return nil } @@ -251,7 +536,7 @@ func (m *Manager) inspectShrinkMinimum(ctx context.Context, service string, volu // Prepare creates/reuses/resizes all service images in deterministic order. // Callers must invoke Preflight before stopping a running VM. func (m *Manager) Prepare(ctx context.Context, svc config.ServiceConfig) ([]PreparedVolume, error) { - if err := m.Preflight(ctx, svc); err != nil { + if _, err := m.Preflight(ctx, svc); err != nil { if m.observer != nil { outcome := "failure" if strings.Contains(err.Error(), "quarantined") { @@ -270,10 +555,14 @@ func (m *Manager) Prepare(ctx context.Context, svc config.ServiceConfig) ([]Prep root := m.storage.Local.Path p, err := m.prepareOne(ctx, svc.Name, volume, root) if err != nil { + // A genuine failure still aborts the batch: a rejection is a + // decision, a failure is a fault, and only the latter means the + // remaining volumes cannot be trusted. return nil, err } prepared = append(prepared, p) } + m.refreshRejections(svc) return prepared, nil } @@ -288,6 +577,16 @@ func (m *Manager) validateExisting(service string, volume config.VolumeConfig, r if err := readJSON(manifestPath, &found); err != nil { if os.IsNotExist(err) { if _, statErr := os.Stat(imagePath); statErr == nil { + // An image with no manifest is either a creation this node + // crashed partway through — recoverable, because the image is + // empty and nothing is protected by failing closed — or an + // image Firework did not create, which is exactly what + // fail-closed exists for. Only a matching marker tells them + // apart, so an absent, unreadable, or mismatched marker still + // quarantines. + if matchingCreationMarker(dir, service, volume, m.nodeID) { + return nil + } return fmt.Errorf("volume %s/%s: image exists without manifest; quarantined", service, volume.Name) } return nil @@ -327,6 +626,7 @@ func (m *Manager) prepareOne(ctx context.Context, service string, volume config. } m.observer.ObserveVolumeOperation(string(volume.Type), operation, outcome, time.Since(started)) }() + var rejection *Rejection dir, err := volumeDir(root, service, volume.Name) if err != nil { return PreparedVolume{}, err @@ -346,16 +646,25 @@ func (m *Manager) prepareOne(ctx context.Context, service string, volume config. err = readJSON(manifestPath, ¤t) if os.IsNotExist(err) { operation = "create" + if err := m.clearInterruptedCreation(dir, imagePath, service, volume); err != nil { + return PreparedVolume{}, err + } + if err := writeCreationMarker(dir, service, volume, m.nodeID); err != nil { + return PreparedVolume{}, err + } if err := createSparseImage(imagePath, volume.SizeBytes); err != nil { return PreparedVolume{}, err } - if _, err := m.runner.Run(ctx, "mkfs.ext4", "-F", "-m", "0", imagePath); err != nil { + if _, err := m.runner.RunDestructive(ctx, "mkfs.ext4", "-F", "-m", "0", imagePath); err != nil { return PreparedVolume{}, err } current = manifestFor(service, volume, m.nodeID) if err := writeJSONAtomic(manifestPath, current); err != nil { return PreparedVolume{}, err } + if err := removeCreationMarker(dir); err != nil { + return PreparedVolume{}, err + } } else if err != nil { return PreparedVolume{}, fmt.Errorf("read manifest: %w", err) } else { @@ -363,6 +672,13 @@ func (m *Manager) prepareOne(ctx context.Context, service string, volume config. if err := verifyManifest(current, service, volume, m.nodeID); err != nil { return PreparedVolume{}, err } + // The manifest is valid, so any surviving marker describes a condition + // that has already ended — a crash between the manifest write and the + // marker removal. Clearing it here is what stops it from authorizing a + // delete later, if the manifest is ever lost. + if err := removeCreationMarker(dir); err != nil { + return PreparedVolume{}, err + } transactionPath := filepath.Join(dir, transactionFilename) var stale resizeTransaction transactionErr := readJSON(transactionPath, &stale) @@ -380,7 +696,27 @@ func (m *Manager) prepareOne(ctx context.Context, service string, volume config. return PreparedVolume{}, err } } - if current.AppliedSizeBytes != volume.SizeBytes || current.ResizeGeneration != volume.ResizeGeneration { + // The clamped configuration needs its own branch, evaluated before the + // resize condition. The clamp substitutes the applied size but keeps + // the *requested* generation — which is what the acknowledgement has + // to match — so it still satisfies the generation arm below and would + // re-enter resize forever. A short-circuit keyed on the requested + // (generation, size) pair cannot help either: the manifest records the + // rejection at the refused size while the clamped input presents the + // applied one, so the two never match by construction. + // + // The applied-size equality is what keeps a genuinely new request from + // being clamped: a raw config arriving at a matching generation but a + // non-applied size falls through to resize and re-measures. + if current.RejectedGeneration != 0 && current.RejectedGeneration == volume.ResizeGeneration && + volume.SizeBytes == current.AppliedSizeBytes { + operation = "rejected" + rejection = &Rejection{ + LogicalID: current.LogicalID, ResizeGeneration: volume.ResizeGeneration, + RequestedSizeBytes: current.RejectedSizeBytes, AppliedSizeBytes: current.AppliedSizeBytes, + MinimumSizeBytes: current.RejectedMinimumBytes, At: current.RejectedAt, + } + } else if current.AppliedSizeBytes != volume.SizeBytes || current.ResizeGeneration != volume.ResizeGeneration { operation = "grow" if volume.SizeBytes < current.AppliedSizeBytes { operation = "shrink" @@ -394,20 +730,39 @@ func (m *Manager) prepareOne(ctx context.Context, service string, volume config. return PreparedVolume{}, fmt.Errorf("volume %s/%s: resize transaction does not match desired generation; quarantined", service, volume.Name) } } - if err := m.resize(ctx, dir, imagePath, ¤t, volume); err != nil { + resized, err := m.resize(ctx, dir, imagePath, ¤t, volume) + if err != nil { return PreparedVolume{}, err } + rejection = resized } } - return PreparedVolume{ + prepared = PreparedVolume{ LogicalID: service + "/" + volume.Name, PathOnHost: imagePath, MountPath: volume.MountPath, Type: volume.Type, SizeBytes: current.AppliedSizeBytes, ResizeGeneration: current.ResizeGeneration, - }, nil + } + if rejection != nil { + // A rejection is a non-fatal outcome of a *successful* preparation, so + // no error is returned and Prepare continues to the next volume. One + // pass therefore collects every rejection, without a retry budget that + // a second rejected volume would exhaust. + prepared.Rejected = true + prepared.RequestedSizeBytes = rejection.RequestedSizeBytes + prepared.RequestedGeneration = rejection.ResizeGeneration + prepared.MinimumSizeBytes = rejection.MinimumSizeBytes + } + return prepared, nil } -func (m *Manager) resize(ctx context.Context, dir, imagePath string, current *manifest, desired config.VolumeConfig) error { +// resize applies a size change, or refuses one. +// +// A refusal returns a Rejection and no error, because by this point +// deleteService has already stopped the VM: treating the refusal as a failure +// would leave the workload down, which is precisely what the caller must be +// able to avoid. +func (m *Manager) resize(ctx context.Context, dir, imagePath string, current *manifest, desired config.VolumeConfig) (*Rejection, error) { transactionPath := filepath.Join(dir, transactionFilename) direction := "grow" if desired.SizeBytes < current.AppliedSizeBytes { @@ -418,63 +773,112 @@ func (m *Manager) resize(ctx context.Context, dir, imagePath string, current *ma Generation: desired.ResizeGeneration, Direction: direction, Phase: "checking", UpdatedAt: time.Now().UTC(), } if err := writeJSONAtomic(transactionPath, tx); err != nil { - return fmt.Errorf("write resize transaction: %w", err) + return nil, fmt.Errorf("write resize transaction: %w", err) } - if _, err := m.runner.Run(ctx, "e2fsck", "-f", "-y", imagePath); err != nil { - return err + if _, err := m.runner.RunDestructive(ctx, "e2fsck", "-f", "-y", imagePath); err != nil { + return nil, err } if direction == "shrink" { parts := strings.SplitN(current.LogicalID, "/", 2) service := parts[0] - if err := m.inspectShrinkMinimum(ctx, service, desired, imagePath); err != nil { - return err + err := m.inspectShrinkMinimum(ctx, service, desired, imagePath) + var rejected *ErrShrinkRejected + if errors.As(err, &rejected) { + rejection, cleanupErr := m.recordShrinkRejection(dir, current, desired, rejected) + if cleanupErr != nil { + return nil, cleanupErr + } + return rejection, nil + } + if err != nil { + return nil, err } } if direction == "grow" { tx.Phase = "file_extended" if err := writeJSONAtomic(transactionPath, tx); err != nil { - return err + return nil, err } if err := os.Truncate(imagePath, desired.SizeBytes); err != nil { - return fmt.Errorf("extend backing image: %w", err) + return nil, fmt.Errorf("extend backing image: %w", err) } - if _, err := m.runner.Run(ctx, "resize2fs", imagePath); err != nil { - return err + if _, err := m.runner.RunDestructive(ctx, "resize2fs", imagePath); err != nil { + return nil, err } } else { tx.Phase = "filesystem_shrinking" if err := writeJSONAtomic(transactionPath, tx); err != nil { - return err + return nil, err } - if _, err := m.runner.Run(ctx, "resize2fs", imagePath, strconv.FormatInt(desired.SizeBytes/1024, 10)+"K"); err != nil { - return err + if _, err := m.runner.RunDestructive(ctx, "resize2fs", imagePath, strconv.FormatInt(desired.SizeBytes/1024, 10)+"K"); err != nil { + return nil, err } tx.Phase = "filesystem_shrunk" if err := writeJSONAtomic(transactionPath, tx); err != nil { - return err + return nil, err } if err := os.Truncate(imagePath, desired.SizeBytes); err != nil { - return fmt.Errorf("truncate backing image after filesystem shrink: %w", err) + return nil, fmt.Errorf("truncate backing image after filesystem shrink: %w", err) } } - if _, err := m.runner.Run(ctx, "e2fsck", "-f", "-y", imagePath); err != nil { - return err + if _, err := m.runner.RunDestructive(ctx, "e2fsck", "-f", "-y", imagePath); err != nil { + return nil, err } current.AppliedSizeBytes = desired.SizeBytes current.ResizeGeneration = desired.ResizeGeneration + // A size actually applied supersedes any earlier refusal. + current.clearRejection() current.UpdatedAt = time.Now().UTC() if err := writeJSONAtomic(filepath.Join(dir, manifestFilename), current); err != nil { - return err + return nil, err } if err := os.Remove(transactionPath); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("remove resize transaction: %w", err) + return nil, fmt.Errorf("remove resize transaction: %w", err) } if err := syncDir(dir); err != nil { - return fmt.Errorf("sync volume directory: %w", err) + return nil, fmt.Errorf("sync volume directory: %w", err) } - return nil + return nil, nil +} + +// recordShrinkRejection cleans up the checking transaction and records the +// refusal, in that order. +// +// The order is fixed and crash-consistent. Writing the rejection first risks +// "rejection recorded plus stale checking transaction", which is exactly the +// state that quarantines the corrected retry: prepareOne compares the stale +// transaction's generation against the new one and refuses to proceed. Crashing +// after the removal instead loses only the rejection record — the request is +// re-measured, refused again, and recorded on the next pass, which is +// idempotent and self-healing. +// +// Removing the transaction is safe here not because nothing has touched the +// image (e2fsck ran, and may have replayed a journal) but because the checking +// phase completes without changing the filesystem's *geometry*. No +// partially-applied resize exists for the transaction to describe. Every later +// phase has moved geometry, and its transaction must survive for recovery. +func (m *Manager) recordShrinkRejection(dir string, current *manifest, desired config.VolumeConfig, rejected *ErrShrinkRejected) (*Rejection, error) { + if err := os.Remove(filepath.Join(dir, transactionFilename)); err != nil && !os.IsNotExist(err) { + return nil, fmt.Errorf("remove checking transaction after rejection: %w", err) + } + if err := syncDir(dir); err != nil { + return nil, err + } + current.RejectedGeneration = desired.ResizeGeneration + current.RejectedSizeBytes = desired.SizeBytes + current.RejectedMinimumBytes = rejected.Minimum + current.RejectedAt = time.Now().UTC() + current.UpdatedAt = current.RejectedAt + if err := writeJSONAtomic(filepath.Join(dir, manifestFilename), current); err != nil { + return nil, err + } + if err := syncDir(dir); err != nil { + return nil, err + } + rejection := current.rejectionFor(desired.ResizeGeneration) + return &rejection, nil } func (m *Manager) checkCapacity(pool *config.LocalStorageConfig, desired map[string]int64) error { @@ -486,8 +890,14 @@ func (m *Manager) checkCapacity(pool *config.LocalStorageConfig, desired map[str retained[id] = size } var reserved int64 - for _, size := range retained { - if size > 0 && reserved > (1<<63-1)-size { + for id, size := range retained { + // Defence in depth: readRetained rejects a non-positive retained size, + // but the desired sizes merged in above come from a node config, and a + // reservation total that can be driven downwards is a capacity bypass. + if size <= 0 { + return fmt.Errorf("volume %s has non-positive size %d", id, size) + } + if reserved > (1<<63-1)-size { return fmt.Errorf("local volume reservations overflow") } reserved += size @@ -497,9 +907,11 @@ func (m *Manager) checkCapacity(pool *config.LocalStorageConfig, desired map[str return fmt.Errorf("read local storage free space: %w", err) } available := int64(stat.Bavail) * int64(stat.Bsize) - if m.observer != nil { - m.observer.ObserveVolumePool(string(config.VolumeTypeLocal), reserved, pool.CapacityBytes, available) - } + // Pool observation deliberately does not happen here. checkCapacity runs + // only when a service declares local volumes, so reporting from it made + // the gauges vanish on a node holding retained-but-unplaced volumes — + // exactly the state an operator needs them for. ObservePool now publishes + // them once per tick from the agent loop, independent of desired state. if reserved > pool.CapacityBytes { return fmt.Errorf("local volume capacity exceeded: reserved %d bytes, configured %d bytes", reserved, pool.CapacityBytes) } @@ -519,6 +931,214 @@ func (m *Manager) checkCapacity(pool *config.LocalStorageConfig, desired map[str return nil } +// rebuildRejections replaces the whole refusal snapshot from the durable +// manifests of the currently desired services. It is the complete +// reconciliation; refreshRejections keeps one service fresh within a tick. +func (m *Manager) rebuildRejections(services []config.ServiceConfig) { + if m == nil || m.storage.Local == nil { + return + } + rebuilt := make(map[string]Rejection) + for _, svc := range services { + for _, declared := range svc.Volumes { + if declared.Type != config.VolumeTypeLocal { + continue + } + // Evaluated against the *raw* desired config, before the clamp + // below rewrites it. After clamping, the size is the applied one + // and the request that was refused is no longer visible. + if rejection, refusing, _ := m.storedRejection(svc.Name, declared); refusing { + rebuilt[svc.Name+"/"+declared.Name] = rejection + } + } + } + m.rejectionMu.Lock() + defer m.rejectionMu.Unlock() + m.rejections = rebuilt +} + +// storedRejection reads one volume's durable refusal and reports whether it +// still describes the request being made. hasRecord distinguishes "no refusal +// recorded at all" from "recorded, but no longer being requested", which the +// callers need in order to prune correctly. +func (m *Manager) storedRejection(service string, declared config.VolumeConfig) (rejection Rejection, refusing, hasRecord bool) { + dir, err := volumeDir(m.storage.Local.Path, service, declared.Name) + if err != nil { + return Rejection{}, false, false + } + var current manifest + if err := readJSON(filepath.Join(dir, manifestFilename), ¤t); err != nil || current.RejectedGeneration == 0 { + return Rejection{}, false, false + } + return current.rejectionFor(current.RejectedGeneration), current.refusesRequest(declared), true +} + +// refreshRejections updates the refusal snapshot for one service from the +// durable manifests. +// +// It reads the manifests rather than only the outcomes of this pass because +// the clamp erases the evidence from the desired configuration: once the +// effective size and generation are substituted, neither the preflight nor +// prepareOne has anything left to refuse, and a snapshot built from outcomes +// alone would clear itself on the very tick that proves the rejection is +// working. The manifest is where the rejection actually lives, and a resize +// that succeeds clears it there. +func (m *Manager) refreshRejections(svc config.ServiceConfig) { + if m == nil || m.storage.Local == nil { + return + } + type outcome struct { + rejection Rejection + refusing bool + hasRecord bool + } + current := make(map[string]outcome, len(svc.Volumes)) + for _, declared := range svc.Volumes { + if declared.Type != config.VolumeTypeLocal { + continue + } + rejection, refusing, hasRecord := m.storedRejection(svc.Name, declared) + current[svc.Name+"/"+declared.Name] = outcome{rejection, refusing, hasRecord} + } + m.rejectionMu.Lock() + defer m.rejectionMu.Unlock() + for logicalID, got := range current { + switch { + case !got.hasRecord: + // The refusal is gone from the manifest — a resize applied — so + // it stops being reported immediately rather than a tick later. + delete(m.rejections, logicalID) + case got.refusing: + m.rejections[logicalID] = got.rejection + } + // Otherwise leave the entry alone. By this point the config has + // already been normalized, so the refused size is no longer visible in + // it and this function cannot tell a withdrawn request from a standing + // one. rebuildRejections makes that call once per tick against the raw + // config; this pass only ever adds a refusal it has just discovered. + } +} + +// Rejections returns the current refusal snapshot, keyed by logical ID. +func (m *Manager) Rejections() map[string]Rejection { + if m == nil { + return nil + } + m.rejectionMu.RLock() + defer m.rejectionMu.RUnlock() + out := make(map[string]Rejection, len(m.rejections)) + for id, rejection := range m.rejections { + out[id] = rejection + } + return out +} + +// SeedRejectionsForTest installs a refusal snapshot directly. Production only +// ever populates it from the durable manifests, through refreshRejections. +func (m *Manager) SeedRejectionsForTest(rejections map[string]Rejection) { + if m == nil { + return + } + m.rejectionMu.Lock() + defer m.rejectionMu.Unlock() + m.rejections = make(map[string]Rejection, len(rejections)) + for id, rejection := range rejections { + m.rejections[id] = rejection + } +} + +// NormalizeVolumes rewrites a desired node configuration so every volume whose +// exact request has already been refused renders its effective size instead. +// +// This closes the window before the control plane's own clamp catches up: +// acknowledging a rejection and re-rendering takes at least one control-plane +// cycle, and during that window the node config still carries the refused size. +// Running the clamp here means needsUpdate, Prepare, and writeVMConfig all see +// one configuration, and the instance stores that same configuration — so it +// compares equal on the very next tick rather than one convergence cycle later. +// +// It reads the manifests rather than the in-memory snapshot so it is correct on +// the first tick after an agent restart, when nothing has been measured yet. +// +// The match is on both generation and requested size (see manifest.matchesRejection), +// which is what lets a hand-authored direct-Git config correct a refused shrink +// by editing size_bytes alone. +func (m *Manager) NormalizeVolumes(services []config.ServiceConfig) { + if m == nil || m.storage.Local == nil { + return + } + // Reconcile the refusal snapshot against the durable manifests for the + // whole desired set, not just the volumes some later Prepare happens to + // touch. Two things depend on it being complete: + // + // - after an agent restart the snapshot is empty, and if normalization + // clamps the config so that no action is planned, nothing else would + // ever repopulate it — the node would report every size applied while + // running an effective one; + // - a volume that is no longer declared has to drop out, or its stale + // entry keeps VolumeSizesApplied false forever. + m.rebuildRejections(services) + for si := range services { + for vi := range services[si].Volumes { + volume := &services[si].Volumes[vi] + if volume.Type != config.VolumeTypeLocal { + continue + } + dir, err := volumeDir(m.storage.Local.Path, services[si].Name, volume.Name) + if err != nil { + continue + } + var current manifest + if err := readJSON(filepath.Join(dir, manifestFilename), ¤t); err != nil { + continue + } + if !current.matchesRejection(*volume) { + continue + } + // Substitute the whole effective configuration — size *and* + // generation. Clamping only the size leaves the generation + // differing forever, and needsUpdate compares whole volume + // configs: the service would be re-planned on every tick, which is + // exactly the loop this is here to end. The refused request is not + // lost: it is reported from the rejection snapshot, which is where + // the acknowledgement reads it. + volume.SizeBytes = current.AppliedSizeBytes + volume.ResizeGeneration = current.ResizeGeneration + } + } +} + +// ObservePool publishes the local pool gauges from retained state alone. It is +// called once per agent tick, independent of any desired configuration, so a +// node with retained but unplaced volumes — or with no desired local volumes at +// all — keeps reporting reserved, capacity, and available bytes. +// +// It never fails a tick: a pool that is not configured or not readable is +// simply not reported, because a metrics side effect must not be able to block +// reconciliation. +func (m *Manager) ObservePool() { + if m == nil || m.observer == nil || m.storage.Local == nil { + return + } + pool := m.storage.Local + retained, err := readRetained(pool.Path) + if err != nil { + return + } + var reserved int64 + for _, size := range retained { + if size > 0 && reserved > (1<<63-1)-size { + return + } + reserved += size + } + var stat syscall.Statfs_t + if err := syscall.Statfs(pool.Path, &stat); err != nil { + return + } + m.observer.ObserveVolumePool(string(config.VolumeTypeLocal), reserved, pool.CapacityBytes, int64(stat.Bavail)*int64(stat.Bsize)) +} + func readRetained(root string) (map[string]int64, error) { retained := make(map[string]int64) err := filepath.WalkDir(root, func(path string, entry os.DirEntry, walkErr error) error { @@ -535,7 +1155,28 @@ func readRetained(root string) (map[string]int64, error) { if err := readJSON(path, &m); err != nil { return fmt.Errorf("read retained manifest %s: %w", path, err) } - retained[m.LogicalID] = m.AppliedSizeBytes + // A retained size feeds pool arithmetic directly, and a non-positive + // one *subtracts* from the reserved total — so a single corrupt or + // hand-edited manifest can admit a volume the pool cannot hold. There + // is no safe number to assume for it, so it fails closed. + if m.AppliedSizeBytes <= 0 { + return fmt.Errorf("retained manifest %s has non-positive applied size %d; quarantined", path, m.AppliedSizeBytes) + } + // The map key must come from the volume's location, not from what its + // manifest claims to be. Keying by the declared logical ID lets two + // manifests collapse onto one entry, and the second volume's bytes + // disappear from the reserved total — the same capacity bypass a + // negative size produces, by a different route. A disagreement between + // the two is itself the corruption, so it fails closed. + located, err := logicalIDFromManifestPath(root, path) + if err != nil { + return err + } + if m.LogicalID != located { + return fmt.Errorf("retained manifest %s declares logical id %q but is stored at %q; quarantined", + path, m.LogicalID, located) + } + retained[located] = m.AppliedSizeBytes return nil }) if os.IsNotExist(err) { @@ -544,6 +1185,74 @@ func readRetained(root string) (map[string]int64, error) { return retained, err } +// ValidateNodeVolumes checks every volume declaration in a node config against +// the invariants the agent enforces before it will run them. +// +// It is exported for `configcheck --node-config`, which validates hand-authored +// direct-Git configs. It deliberately reuses the agent's own rules rather than +// restating them: a second copy would drift, and the failure mode of drift here +// is a config that validates in CI and then cannot start on the node. +// +// It checks declarations only. Anything requiring the host — a retained +// manifest, pool capacity, free space, the node's own identity — is not +// knowable from a config file and is left to the agent's Preflight. +func ValidateNodeVolumes(nc config.NodeConfig) error { + var problems []string + for _, svc := range nc.Services { + if len(svc.Volumes) == 0 { + continue + } + // The agent derives a filesystem path from the service name as well as + // the volume name, so a name that is not a safe path component fails at + // preflight. It shares volumeDir's predicate rather than restating it, + // so the two cannot drift. + if err := validatePathComponent("service", svc.Name); err != nil { + problems = append(problems, err.Error()) + continue + } + if err := validateServiceVolumes(svc.Volumes); err != nil { + problems = append(problems, fmt.Sprintf("service %s: %v", svc.Name, err)) + continue + } + for _, declared := range svc.Volumes { + logicalID := svc.Name + "/" + declared.Name + switch declared.Type { + case config.VolumeTypeLocal: + // A local volume is durably bound to one physical node, and + // the agent refuses any volume whose bound_node does not match + // its own stable node_id. A config that omits it can never + // start. + if declared.BoundNode == "" { + problems = append(problems, fmt.Sprintf( + "volume %s: local volumes must declare bound_node matching the agent's node_id", logicalID)) + } + case config.VolumeTypeShared: + problems = append(problems, fmt.Sprintf("volume %s: %v", logicalID, ErrSharedUnsupported)) + default: + problems = append(problems, fmt.Sprintf("volume %s: unsupported type %q", logicalID, declared.Type)) + } + } + } + if len(problems) > 0 { + return fmt.Errorf("%s", strings.Join(problems, "\n")) + } + return nil +} + +// logicalIDFromManifestPath recovers "service/volume" from a manifest's +// location under the pool root, which is the volume's real identity. +func logicalIDFromManifestPath(root, path string) (string, error) { + rel, err := filepath.Rel(root, path) + if err != nil { + return "", fmt.Errorf("locate retained manifest %s: %w", path, err) + } + parts := strings.Split(filepath.ToSlash(rel), "/") + if len(parts) != 3 || parts[0] == "" || parts[1] == "" { + return "", fmt.Errorf("retained manifest %s is not at //%s; quarantined", path, manifestFilename) + } + return parts[0] + "/" + parts[1], nil +} + func validateServiceVolumes(volumes []config.VolumeConfig) error { if len(volumes) > config.MaxServiceVolumes { return fmt.Errorf("at most %d volumes are supported", config.MaxServiceVolumes) @@ -579,12 +1288,21 @@ func validateServiceVolumes(volumes []config.VolumeConfig) error { return nil } +// validatePathComponent is the single predicate for anything that becomes a +// directory name under the volume pool. +func validatePathComponent(kind, name string) error { + if !componentPattern.MatchString(name) || strings.Contains(name, "..") { + return fmt.Errorf("invalid %s name %q for volume path", kind, name) + } + return nil +} + func volumeDir(root, service, volume string) (string, error) { - if !componentPattern.MatchString(service) || strings.Contains(service, "..") { - return "", fmt.Errorf("invalid service name %q for volume path", service) + if err := validatePathComponent("service", service); err != nil { + return "", err } - if !componentPattern.MatchString(volume) || strings.Contains(volume, "..") { - return "", fmt.Errorf("invalid volume name %q", volume) + if err := validatePathComponent("volume", volume); err != nil { + return "", err } return filepath.Join(root, service, volume), nil } @@ -616,6 +1334,61 @@ func manifestFor(service string, volume config.VolumeConfig, nodeID string) mani return m } +// clearInterruptedCreation removes an image left behind by a crashed first +// creation. It refuses — leaving the volume quarantined — for any image whose +// creation this node cannot prove it started. +func (m *Manager) clearInterruptedCreation(dir, imagePath, service string, volume config.VolumeConfig) error { + if _, err := os.Stat(imagePath); err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("volume %s/%s: stat image: %w", service, volume.Name, err) + } + if !matchingCreationMarker(dir, service, volume, m.nodeID) { + return fmt.Errorf("volume %s/%s: image exists without manifest; quarantined", service, volume.Name) + } + if err := os.Remove(imagePath); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("volume %s/%s: remove interrupted image: %w", service, volume.Name, err) + } + if err := syncDir(dir); err != nil { + return err + } + return nil +} + +func matchingCreationMarker(dir, service string, volume config.VolumeConfig, nodeID string) bool { + var marker creationMarker + if err := readJSON(filepath.Join(dir, creationMarkerFilename), &marker); err != nil { + return false + } + return marker.LogicalID == service+"/"+volume.Name && marker.NodeID == nodeID +} + +func writeCreationMarker(dir, service string, volume config.VolumeConfig, nodeID string) error { + marker := creationMarker{ + LogicalID: service + "/" + volume.Name, NodeID: nodeID, + TargetSizeBytes: volume.SizeBytes, ResizeGeneration: volume.ResizeGeneration, + CreatedAt: time.Now().UTC(), + } + if err := writeJSONAtomic(filepath.Join(dir, creationMarkerFilename), marker); err != nil { + return fmt.Errorf("write creation marker: %w", err) + } + return nil +} + +// removeCreationMarker is idempotent: the common case is that there is no +// marker to remove, and it must stay cheap enough to call on every reuse. +func removeCreationMarker(dir string) error { + err := os.Remove(filepath.Join(dir, creationMarkerFilename)) + if os.IsNotExist(err) { + return nil + } + if err != nil { + return fmt.Errorf("remove creation marker: %w", err) + } + return syncDir(dir) +} + func createSparseImage(path string, size int64) error { f, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_RDWR, 0o640) if err != nil { diff --git a/internal/volume/manager_test.go b/internal/volume/manager_test.go index e78979a..857baf2 100644 --- a/internal/volume/manager_test.go +++ b/internal/volume/manager_test.go @@ -11,7 +11,13 @@ import ( "github.com/artemnikitin/firework/internal/config" ) -type fakeRunner struct{ calls []string } +type fakeRunner struct { + calls []string + // destructive records the commands that were routed through + // RunDestructive, so a test can assert a filesystem-mutating command did + // not take the promptly-cancellable path. + destructive []string +} func (r *fakeRunner) Run(_ context.Context, name string, args ...string) ([]byte, error) { r.calls = append(r.calls, name+" "+strings.Join(args, " ")) @@ -24,6 +30,11 @@ func (r *fakeRunner) Run(_ context.Context, name string, args ...string) ([]byte return nil, nil } +func (r *fakeRunner) RunDestructive(ctx context.Context, name string, args ...string) ([]byte, error) { + r.destructive = append(r.destructive, name+" "+strings.Join(args, " ")) + return r.Run(ctx, name, args...) +} + type acceptingMounts struct{} func (acceptingMounts) Verify(string) error { return nil } @@ -78,16 +89,16 @@ func TestManagerCreatesReusesGrowsAndShrinksLocalVolume(t *testing.T) { func TestManagerRejectsBindingCapacityAndSharedRuntime(t *testing.T) { root := t.TempDir() manager := NewManagerWithDependencies("node-1", config.StorageConfig{Local: &config.LocalStorageConfig{Path: root, CapacityBytes: 8 * config.MiB}}, &fakeRunner{}, acceptingMounts{}) - if err := manager.Preflight(context.Background(), localService(16*config.MiB, 1)); err == nil || !strings.Contains(err.Error(), "capacity exceeded") { + if _, err := manager.Preflight(context.Background(), localService(16*config.MiB, 1)); err == nil || !strings.Contains(err.Error(), "capacity exceeded") { t.Fatalf("expected capacity error, got %v", err) } wrong := localService(config.MiB, 1) wrong.Volumes[0].BoundNode = "node-2" - if err := manager.Preflight(context.Background(), wrong); err == nil || !strings.Contains(err.Error(), "does not match") { + if _, err := manager.Preflight(context.Background(), wrong); err == nil || !strings.Contains(err.Error(), "does not match") { t.Fatalf("expected binding error, got %v", err) } shared := config.ServiceConfig{Name: "app", Volumes: []config.VolumeConfig{{Name: "data", Type: config.VolumeTypeShared, MountPath: "/data", SizeBytes: config.GiB}}} - if err := manager.Preflight(context.Background(), shared); !errors.Is(err, ErrSharedUnsupported) && (err == nil || !strings.Contains(err.Error(), ErrSharedUnsupported.Error())) { + if _, err := manager.Preflight(context.Background(), shared); !errors.Is(err, ErrSharedUnsupported) && (err == nil || !strings.Contains(err.Error(), ErrSharedUnsupported.Error())) { t.Fatalf("expected shared safety gate, got %v", err) } } @@ -169,7 +180,7 @@ func TestManagerQuarantinesAmbiguousRetainedState(t *testing.T) { t.Fatal(err) } manager := NewManagerWithDependencies("node-1", config.StorageConfig{Local: &config.LocalStorageConfig{Path: root, CapacityBytes: 100 * config.MiB}}, &fakeRunner{}, acceptingMounts{}) - if err := manager.Preflight(context.Background(), localService(16*config.MiB, 1)); err == nil || !strings.Contains(err.Error(), "quarantined") { + if _, err := manager.Preflight(context.Background(), localService(16*config.MiB, 1)); err == nil || !strings.Contains(err.Error(), "quarantined") { t.Fatalf("expected quarantine error, got %v", err) } }) @@ -184,7 +195,7 @@ func TestManagerQuarantinesAmbiguousRetainedState(t *testing.T) { if err := os.Truncate(prepared[0].PathOnHost, 16*config.MiB); err != nil { t.Fatal(err) } - if err := manager.Preflight(context.Background(), localService(16*config.MiB, 2)); err == nil || !strings.Contains(err.Error(), "quarantined") { + if _, err := manager.Preflight(context.Background(), localService(16*config.MiB, 2)); err == nil || !strings.Contains(err.Error(), "quarantined") { t.Fatalf("expected quarantine error, got %v", err) } }) @@ -198,7 +209,15 @@ func TestManagerRejectsShrinkBelowSafeMinimum(t *testing.T) { if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { t.Fatal(err) } - if err := manager.Preflight(context.Background(), localService(4*config.MiB, 2)); err == nil || !strings.Contains(err.Error(), "below safe minimum") { - t.Fatalf("expected safe-minimum error, got %v", err) + // A shrink below the safe minimum is refused, and the refusal is a + // decision rather than a fault: it is reported as a rejection so the + // caller can keep the workload running at its effective size. + rejections, err := manager.Preflight(context.Background(), localService(4*config.MiB, 2)) + if err != nil { + t.Fatalf("a refused shrink must not be an error: %v", err) + } + if len(rejections) != 1 || rejections[0].RequestedSizeBytes != 4*config.MiB || + rejections[0].AppliedSizeBytes != 16*config.MiB || rejections[0].MinimumSizeBytes == 0 { + t.Fatalf("unexpected rejection: %#v", rejections) } } diff --git a/internal/volume/rejection_snapshot_test.go b/internal/volume/rejection_snapshot_test.go new file mode 100644 index 0000000..3f7e231 --- /dev/null +++ b/internal/volume/rejection_snapshot_test.go @@ -0,0 +1,141 @@ +package volume + +import ( + "context" + "testing" + + "github.com/artemnikitin/firework/internal/config" +) + +// The control plane renders the applied size with the *refused* +// generation. Normalization must recognize that form, or the generation stays +// different from the running instance and the reconciler re-plans an update. +func TestNormalizeRecognizesTheControlPlaneClampedForm(t *testing.T) { + manager, _ := hardeningManager(t, &fakeRunner{}) + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + if _, err := manager.Prepare(context.Background(), localService(2*config.MiB, 2)); err != nil { + t.Fatal(err) + } + + // What the control plane renders after acknowledging the rejection: + // effective size, refused generation. + rendered := localService(16*config.MiB, 2) + services := []config.ServiceConfig{rendered} + manager.NormalizeVolumes(services) + + got := services[0].Volumes[0] + if got.SizeBytes != 16*config.MiB || got.ResizeGeneration != 1 { + t.Fatalf("expected normalization to the effective (size, generation) = (16MiB, 1), got (%d, %d)", + got.SizeBytes, got.ResizeGeneration) + } +} + +// After an agent restart the snapshot is empty. Normalization can +// clamp from the durable manifest and plan no action, so nothing ever +// repopulates it and the node falsely reports every size applied. +func TestRejectionSnapshotSurvivesAnAgentRestart(t *testing.T) { + manager, root := hardeningManager(t, &fakeRunner{}) + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + if _, err := manager.Prepare(context.Background(), localService(2*config.MiB, 2)); err != nil { + t.Fatal(err) + } + + // A fresh manager over the same pool is exactly an agent restart. + restarted, _ := hardeningManager(t, &fakeRunner{}) + restarted.storage.Local.Path = root + if got := restarted.Rejections(); len(got) != 0 { + t.Fatalf("precondition: a fresh manager starts empty, got %#v", got) + } + + services := []config.ServiceConfig{localService(2*config.MiB, 2)} + restarted.NormalizeVolumes(services) + + if got := restarted.Rejections(); len(got) != 1 { + t.Fatalf("expected normalization to restore the durable rejection, got %#v", got) + } +} + +// A volume that is no longer declared must drop out of the +// snapshot, or VolumeSizesApplied stays false forever. +func TestRemovedVolumeDropsOutOfTheRejectionSnapshot(t *testing.T) { + manager, _ := hardeningManager(t, &fakeRunner{}) + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + if _, err := manager.Prepare(context.Background(), localService(2*config.MiB, 2)); err != nil { + t.Fatal(err) + } + if got := manager.Rejections(); len(got) != 1 { + t.Fatalf("precondition: expected one rejection, got %#v", got) + } + + // The service no longer declares any volume. + manager.NormalizeVolumes([]config.ServiceConfig{{Name: "app"}}) + + if got := manager.Rejections(); len(got) != 0 { + t.Fatalf("expected the stale rejection to be pruned, got %#v", got) + } +} + +// After a refusal, asking for the size already running withdraws the request. The refusal must stop being reported — otherwise the node is +// degraded forever with no way out but a generation bump. +func TestWithdrawnRequestStopsBeingReported(t *testing.T) { + manager, _ := hardeningManager(t, &fakeRunner{}) + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + if _, err := manager.Prepare(context.Background(), localService(2*config.MiB, 2)); err != nil { + t.Fatal(err) + } + if len(manager.Rejections()) != 1 { + t.Fatal("precondition: expected a standing rejection") + } + + // The request is now the size already running, at the same generation. + withdrawn := []config.ServiceConfig{localService(16*config.MiB, 2)} + manager.NormalizeVolumes(withdrawn) + + // The clamp must still hold, so no restart is planned... + got := withdrawn[0].Volumes[0] + if got.SizeBytes != 16*config.MiB || got.ResizeGeneration != 1 { + t.Fatalf("expected the effective configuration, got (%d, %d)", got.SizeBytes, got.ResizeGeneration) + } + // ...but nothing is being refused any more. + if len(manager.Rejections()) != 0 { + t.Fatalf("a withdrawn request is not a standing refusal: %#v", manager.Rejections()) + } +} + +// The clamp/report split is only unambiguous because a refused size is always +// strictly below the applied size — both refusal sites reach the measurement +// only on a shrink. If that ever stopped holding, matchesRejection's two arms +// could match the same value and a withdrawn request would be indistinguishable +// from a standing one, silently degrading the node forever. +func TestARefusedSizeIsAlwaysBelowTheAppliedSize(t *testing.T) { + manager, root := hardeningManager(t, &fakeRunner{}) + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + + // A grow is never measured for a minimum, so it can never be refused. + if rejections, err := manager.Preflight(context.Background(), localService(32*config.MiB, 2)); err != nil || len(rejections) != 0 { + t.Fatalf("a grow must not be refusable: %#v, %v", rejections, err) + } + // Nor is a request for the size already applied. + if rejections, err := manager.Preflight(context.Background(), localService(16*config.MiB, 2)); err != nil || len(rejections) != 0 { + t.Fatalf("an unchanged size must not be refusable: %#v, %v", rejections, err) + } + + if _, err := manager.Prepare(context.Background(), localService(2*config.MiB, 3)); err != nil { + t.Fatal(err) + } + found := readManifest(t, root, "app", "data") + if found.RejectedSizeBytes >= found.AppliedSizeBytes { + t.Fatalf("a refused size must be strictly below the applied size: refused %d, applied %d", + found.RejectedSizeBytes, found.AppliedSizeBytes) + } +} diff --git a/internal/volume/shrink_rejection_test.go b/internal/volume/shrink_rejection_test.go new file mode 100644 index 0000000..8267587 --- /dev/null +++ b/internal/volume/shrink_rejection_test.go @@ -0,0 +1,264 @@ +package volume + +import ( + "context" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/artemnikitin/firework/internal/config" +) + +func twoVolumeService(dataSize, cacheSize, generation int64) config.ServiceConfig { + return config.ServiceConfig{Name: "app", Volumes: []config.VolumeConfig{ + {Name: "data", Type: config.VolumeTypeLocal, MountPath: "/var/lib/app", + SizeBytes: dataSize, BoundNode: "node-1", ResizeGeneration: generation}, + {Name: "cache", Type: config.VolumeTypeLocal, MountPath: "/var/cache/app", + SizeBytes: cacheSize, BoundNode: "node-1", ResizeGeneration: generation}, + }} +} + +func readManifest(t *testing.T, root, service, volume string) manifest { + t.Helper() + var found manifest + if err := readJSON(filepath.Join(root, service, volume, manifestFilename), &found); err != nil { + t.Fatal(err) + } + return found +} + +// A refused shrink after the VM has already been stopped must still bring the +// service back. It is a non-fatal outcome of a successful preparation, not an +// error, so the volume is prepared at its applied size and the caller gets +// enough to clamp with. +func TestPostStopShrinkRejectionPreparesAtTheAppliedSize(t *testing.T) { + manager, root := hardeningManager(t, &fakeRunner{}) + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + + prepared, err := manager.Prepare(context.Background(), localService(2*config.MiB, 2)) + if err != nil { + t.Fatalf("a refused shrink must not fail the preparation: %v", err) + } + if len(prepared) != 1 { + t.Fatalf("unexpected prepared set: %#v", prepared) + } + got := prepared[0] + if !got.Rejected || got.SizeBytes != 16*config.MiB || got.RequestedSizeBytes != 2*config.MiB { + t.Fatalf("unexpected prepared volume: %#v", got) + } + // The prepared volume describes the *effective* configuration, so the + // caller can store it on the instance and have the next tick compare + // equal. The refused request travels separately. + if got.ResizeGeneration != 1 { + t.Fatalf("expected the applied generation on the prepared volume, got %d", got.ResizeGeneration) + } + if got.RequestedGeneration != 2 { + t.Fatalf("expected the refused generation to be reported, got %d", got.RequestedGeneration) + } + // The rejection snapshot status reads carries both, so the control plane + // can match the observation to the record it has to converge. + snapshot := manager.Rejections()["app/data"] + if snapshot.ResizeGeneration != 2 || snapshot.AppliedGeneration != 1 { + t.Fatalf("unexpected rejection snapshot: %#v", snapshot) + } + + found := readManifest(t, root, "app", "data") + if found.ResizeGeneration != 1 { + t.Fatalf("the manifest's applied generation must keep describing what was applied, got %d", found.ResizeGeneration) + } + if found.RejectedGeneration != 2 || found.RejectedSizeBytes != 2*config.MiB || found.RejectedMinimumBytes == 0 { + t.Fatalf("the rejection was not recorded durably: %#v", found) + } + // The checking transaction must be gone, or it quarantines the corrected + // retry. + if _, err := os.Stat(filepath.Join(root, "app", "data", transactionFilename)); !os.IsNotExist(err) { + t.Fatalf("the checking transaction survived the rejection: %v", err) + } +} + +// The clamped configuration re-enters prepareOne carrying the applied size with +// the *requested* generation. That combination still satisfies the resize +// condition's generation arm, so it needs its own branch — and that branch must +// run no tools and must not advance the manifest's applied generation. +func TestClampedConfigDoesNotReenterResize(t *testing.T) { + runner := &fakeRunner{} + manager, root := hardeningManager(t, runner) + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + if _, err := manager.Prepare(context.Background(), localService(2*config.MiB, 2)); err != nil { + t.Fatal(err) + } + + before := len(runner.destructive) + // The clamped config: applied size, requested generation. + prepared, err := manager.Prepare(context.Background(), localService(16*config.MiB, 2)) + if err != nil { + t.Fatal(err) + } + if len(runner.destructive) != before { + t.Fatalf("the clamped config ran filesystem tools: %v", runner.destructive[before:]) + } + if !prepared[0].Rejected || prepared[0].SizeBytes != 16*config.MiB { + t.Fatalf("the clamped config lost the rejection: %#v", prepared[0]) + } + if found := readManifest(t, root, "app", "data"); found.ResizeGeneration != 1 { + t.Fatalf("the clamped config advanced the applied generation to %d", found.ResizeGeneration) + } +} + +// A new generation is a new request. It must re-measure rather than inherit the +// old refusal, and the removed checking transaction must not quarantine it. +func TestCorrectedGenerationRetriesAndIsNotQuarantined(t *testing.T) { + manager, _ := hardeningManager(t, &fakeRunner{}) + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + if _, err := manager.Prepare(context.Background(), localService(2*config.MiB, 2)); err != nil { + t.Fatal(err) + } + + // 8 MiB is above the fake minimum, so the corrected request succeeds. + prepared, err := manager.Prepare(context.Background(), localService(8*config.MiB, 3)) + if err != nil { + t.Fatalf("the corrected request was not retried cleanly: %v", err) + } + if prepared[0].Rejected || prepared[0].SizeBytes != 8*config.MiB { + t.Fatalf("the corrected shrink did not apply: %#v", prepared[0]) + } +} + +// Direct-Git node configs are hand authored and carry their own +// resize_generation, so an operator correcting a refused shrink by editing +// size_bytes alone presents a different request under the same generation. +// Matching on the generation alone would clamp that forever. +func TestDirectGitSizeEditReMeasuresAtTheSameGeneration(t *testing.T) { + manager, _ := hardeningManager(t, &fakeRunner{}) + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + if _, err := manager.Prepare(context.Background(), localService(2*config.MiB, 2)); err != nil { + t.Fatal(err) + } + + // Same generation, corrected size: a genuinely new request. + corrected := localService(8*config.MiB, 2) + rejections, err := manager.Preflight(context.Background(), corrected) + if err != nil { + t.Fatal(err) + } + if len(rejections) != 0 { + t.Fatalf("a corrected size at the same generation must re-measure, got %#v", rejections) + } + + // An unchanged request at the same generation still clamps. + repeat := localService(2*config.MiB, 2) + rejections, err = manager.Preflight(context.Background(), repeat) + if err != nil { + t.Fatal(err) + } + if len(rejections) != 1 { + t.Fatalf("an unchanged refused request must stay refused, got %#v", rejections) + } +} + +// A retry budget cannot survive more than one rejected volume, so a rejection +// is not an error at all: one pass collects every refusal. +func TestTwoRejectedShrinksBothStartInOnePass(t *testing.T) { + manager, root := hardeningManager(t, &fakeRunner{}) + if _, err := manager.Prepare(context.Background(), twoVolumeService(16*config.MiB, 16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + + prepared, err := manager.Prepare(context.Background(), twoVolumeService(2*config.MiB, 2*config.MiB, 2)) + if err != nil { + t.Fatalf("two rejections must not fail the batch: %v", err) + } + if len(prepared) != 2 { + t.Fatalf("expected both volumes prepared, got %#v", prepared) + } + for _, got := range prepared { + if !got.Rejected || got.SizeBytes != 16*config.MiB { + t.Fatalf("volume %s was not prepared at its applied size: %#v", got.LogicalID, got) + } + } + for _, name := range []string{"data", "cache"} { + if found := readManifest(t, root, "app", name); found.RejectedGeneration != 2 { + t.Fatalf("volume %s did not record its rejection: %#v", name, found) + } + } + // Both rejections are visible in the snapshot status reads. + if got := manager.Rejections(); len(got) != 2 { + t.Fatalf("expected two rejections in the snapshot, got %#v", got) + } +} + +// The transaction is removed before the rejection is written, so a crash +// between them loses only the record — idempotent and self-healing. The +// reverse order would leave the state that quarantines the corrected retry. +func TestRejectionRemovesOnlyTheGeometryPreservingTransaction(t *testing.T) { + manager, root := hardeningManager(t, &fakeRunner{}) + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + dir := filepath.Join(root, "app", "data") + + // A transaction from a later phase describes moved geometry and must + // survive; the recovery path depends on it. + later := resizeTransaction{ + OldSizeBytes: 16 * config.MiB, DesiredSizeBytes: 8 * config.MiB, Generation: 5, + Direction: "shrink", Phase: "filesystem_shrunk", UpdatedAt: time.Now().UTC(), + } + if err := writeJSONAtomic(filepath.Join(dir, transactionFilename), later); err != nil { + t.Fatal(err) + } + var current manifest + if err := readJSON(filepath.Join(dir, manifestFilename), ¤t); err != nil { + t.Fatal(err) + } + if _, err := manager.recordShrinkRejection(dir, ¤t, config.VolumeConfig{ + Name: "data", SizeBytes: 2 * config.MiB, ResizeGeneration: 2, + }, &ErrShrinkRejected{LogicalID: "app/data", Requested: 2 * config.MiB, Minimum: 4 * config.MiB}); err != nil { + t.Fatal(err) + } + // recordShrinkRejection is only ever reached from the checking phase, so + // what it removes is by construction a checking transaction. The assertion + // that matters is the ordering: after it returns, the rejection is durable + // and no checking transaction is left to poison the retry. + found := readManifest(t, root, "app", "data") + if found.RejectedGeneration != 2 || found.RejectedSizeBytes != 2*config.MiB { + t.Fatalf("the rejection was not written: %#v", found) + } +} + +// Preflight became a writer of the same manifest prepareOne rewrites. Without +// the shared lifecycle lock the two interleave and one update is lost. +func TestConcurrentPreflightAndPrepareDoNotLoseTheManifest(t *testing.T) { + manager, root := hardeningManager(t, &fakeRunner{}) + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + if i%2 == 0 { + _, _ = manager.Preflight(context.Background(), localService(2*config.MiB, 2)) + return + } + _, _ = manager.Prepare(context.Background(), localService(16*config.MiB, 1)) + }(i) + } + wg.Wait() + + found := readManifest(t, root, "app", "data") + if found.AppliedSizeBytes != 16*config.MiB { + t.Fatalf("the applied size was lost to a concurrent update: %#v", found) + } +} diff --git a/internal/volume/snapshot_paths_test.go b/internal/volume/snapshot_paths_test.go new file mode 100644 index 0000000..de3a7a5 --- /dev/null +++ b/internal/volume/snapshot_paths_test.go @@ -0,0 +1,51 @@ +package volume + +import ( + "context" + "testing" + + "github.com/artemnikitin/firework/internal/config" +) + +// The snapshot is maintained by two passes with different jobs, and a standing +// refusal must survive every path through them: rebuildRejections decides +// against the raw config once per tick, refreshRejections only ever adds a +// refusal it just discovered or drops one the manifest no longer has. +func TestSnapshotSurvivesEveryUpdatePath(t *testing.T) { + manager, _ := hardeningManager(t, &fakeRunner{}) + if _, err := manager.Prepare(context.Background(), localService(16*config.MiB, 1)); err != nil { + t.Fatal(err) + } + + // A brand-new refusal must be reported in the very tick it happens, from + // the pre-clamp config the preflight sees. + raw := localService(2*config.MiB, 2) + if _, err := manager.Preflight(context.Background(), raw); err != nil { + t.Fatal(err) + } + if len(manager.Rejections()) != 1 { + t.Fatalf("a fresh refusal must be reported in the same tick: %#v", manager.Rejections()) + } + + // A later tick: rebuild sees the raw config and keeps it, then Prepare + // runs against the clamped config and must not drop it. + services := []config.ServiceConfig{localService(2*config.MiB, 2)} + manager.NormalizeVolumes(services) + if len(manager.Rejections()) != 1 { + t.Fatalf("rebuild dropped a standing refusal: %#v", manager.Rejections()) + } + if _, err := manager.Prepare(context.Background(), services[0]); err != nil { + t.Fatal(err) + } + if len(manager.Rejections()) != 1 { + t.Fatalf("the clamped-config pass dropped a standing refusal: %#v", manager.Rejections()) + } + + // A resize that actually applies clears it in the same tick, not the next. + if _, err := manager.Prepare(context.Background(), localService(8*config.MiB, 3)); err != nil { + t.Fatal(err) + } + if len(manager.Rejections()) != 0 { + t.Fatalf("an applied resize must clear the refusal immediately: %#v", manager.Rejections()) + } +}