diff --git a/Cargo.lock b/Cargo.lock index 4a2c4bfd..ba592a8c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -437,6 +437,7 @@ dependencies = [ "iana-time-zone", "js-sys", "num-traits", + "serde", "wasm-bindgen", "windows-link 0.2.1", ] diff --git a/Cargo.toml b/Cargo.toml index 8fe1f838..a33d3aa9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,7 +22,7 @@ clap = { version = "4.5.1", features = ["derive"] } # Generates the shell completion scripts emitted by `ring completions ` # from the same `Command` tree the CLI is parsed with, so the two can't drift. clap_complete = "4" -chrono = "0.4" +chrono = { version = "0.4", features = ["serde"] } uuid = { version = "1.4.1", features = ["v4"] } sqlx = { version = "0.8", features = ["runtime-tokio", "sqlite", "migrate"] } reqwest = { version = "0.12.11", features = ["json"] } diff --git a/documentation/concepts/deployment-status-lifecycle.md b/documentation/concepts/deployment-status-lifecycle.md index 7b9000c8..8594edd4 100644 --- a/documentation/concepts/deployment-status-lifecycle.md +++ b/documentation/concepts/deployment-status-lifecycle.md @@ -23,8 +23,8 @@ All values serialize as `snake_case`, identically on the wire (JSON), in the CLI | Status | Cause | Terminal? | |---|---|---| | `failed` | A job exited non-zero / crashed; **or** a readiness check never turned green before the deadline on a non-rolling deployment; **or** (Cloud Hypervisor) firmware not found. | **Terminal** | -| `crash_loop_back_off` | A worker's container/VM kept dying and `restart_count` reached `MAX_RESTART_COUNT` (5). The reconciler stops trying. | **Terminal** | -| `insufficient_resources` | The host doesn't have enough memory for the deployment's request. A retry can't conjure RAM, so Ring stops. | **Terminal** | +| `crash_loop_back_off` | A worker's instances keep dying. On Docker and Podman the worker is waiting for its next start (see [restart policy](#restart-policy)); on the other runtimes it reached 5 failed attempts. | Retried (Docker, Podman) | +| `insufficient_resources` | The host doesn't have enough free memory for the deployment's request right now. Retried, since memory gets freed. | Retried | | `image_pull_back_off` | The image couldn't be pulled (tag not found, registry auth, `image_pull_policy: Never` forbidding a pull, transient network). | Retried | | `create_container_error` | The runtime rejected container creation (invalid mount, unsupported option, a port conflict the daemon surfaces at create time). | Retried | | `network_error` | Creating the namespace network/bridge failed. | Retried | @@ -32,9 +32,9 @@ All values serialize as `snake_case`, identically on the wire (JSON), in the CLI | `file_system_error` | An IO error handling volumes or temp config files. | Retried | | `error` | Generic runtime fallback: a stats fetch, a JSON parse, a VM-start failure, or any error not classified above. | Retried | -**Terminal vs retried.** A *terminal* status is never reconciled again, because the deployment is done. A *retried* failure status stays in the reconcile loop: each tick re-attempts the apply, bumping `restart_count`, until either it succeeds (back to `creating` → `running`) or the counter hits `MAX_RESTART_COUNT` and a worker flips to `crash_loop_back_off`. The reconciler explicitly polls `pending`, `creating`, `running`, `deleted`, and the five *retried* error states; `completed`, `failed`, `crash_loop_back_off`, and `insufficient_resources` are left out on purpose. +**Terminal vs retried.** A *terminal* status is never reconciled again, because the deployment is done: only `completed` and `failed` are. Every other status stays in the reconcile loop, and a failed one is retried on the backoff curve described in [restart policy](#restart-policy) until it succeeds (back to `creating` → `running`). -> **Recover from a terminal or stuck state** by re-applying a fixed manifest: `ring apply` resets `restart_count` and re-enters the lifecycle from the top. The restart counter is cumulative over the deployment's lifetime, not a sliding window. +> **Recover from a terminal state** by re-applying a fixed manifest: `ring apply` resets `restart_count` and re-enters the lifecycle from the top. ## Worker lifecycle @@ -45,13 +45,12 @@ pending → creating ──────────────→ running ─ │ (gate: ready) │ │ ├──→ deleted (you delete it) │←── readiness not green │ - │ (held here) └──→ crash_loop_back_off (restart_count ≥ 5) + │ (held here) └──→ crash_loop_back_off (an instance died; + │ waiting for the next start) │ └──→ image_pull_back_off / create_container_error / network_error / - config_error / file_system_error / error (retried; → crash_loop_back_off - after MAX_RESTART_COUNT, or → running once resolved) - - insufficient_resources (terminal — host out of memory) + config_error / file_system_error / insufficient_resources / error + (waiting for the next start; → creating once it succeeds) ``` - **`creating → running`** happens as soon as the container/VM is up **unless** the deployment declares a `readiness: true` health check, in which case the [readiness gate](#the-readiness-gate) holds it in `creating` until ready. @@ -66,7 +65,7 @@ A `kind: job` runs one instance to completion (`replicas` is ignored). pending → creating → running ──→ completed (exit 0 / clean guest shutdown) │ ├──→ failed (non-zero exit, OOM, signal, host-side timeout) - └──→ failed (restart_count ≥ MAX_RESTART_COUNT) + └──→ failed (5 failed starts) ``` - On Cloud Hypervisor the host can't read the guest's exit code, so any clean VM shutdown is `completed`. Use a worker if you need precise exit-code semantics on CH. @@ -82,18 +81,15 @@ While in `creating`, only the readiness checks run (recorded for the gate to rea Without any readiness check, the legacy behaviour is preserved: `running` as soon as the container is up. See [Health checks (design) → the readiness gate](/documentation/concepts/health-checks-design#the-readiness-gate) for the full mechanics. -## Restart counter and `crash_loop_back_off` - -Ring tracks a cumulative `restart_count` per deployment. It is bumped when: +## Restart policy -- A worker's container dies unexpectedly (Docker `die`/`oom`/`kill` events, or a CH VM going unresponsive), unless the shutdown was intentional (a delete/scale-down). -- A *retried* error status re-attempts its apply and fails again. +`restart_count` counts failed attempts since the last reset: an instance that died unexpectedly (not one Ring stopped itself on a delete or scale-down), a start that failed, or an instance removed by a liveness check. -Once `restart_count` reaches `MAX_RESTART_COUNT` (5), the next tick flips a **worker** to `crash_loop_back_off` (terminal) and a **job** to `failed` (terminal): the reconciler stops retrying, protecting the host from a tight crash loop. The counter is **cumulative for the deployment's lifetime**, not a sliding window; `ring apply` with a fixed manifest resets it. +On **Docker and Podman**, a worker is never abandoned. Each failure pushes its next start out on a randomized exponential backoff, from up to 10 s after the first failure to at most 5 minutes, and the counter resets after 10 minutes of uninterrupted running. Failures that need an operator (a missing config, a rejected spec, an exit code `126`/`127`) wait up to 5 minutes from the first attempt on, and are still retried, so fixing the cause is picked up without a re-apply. The backoff is stored in the database and survives a `ring server` restart. See [Reconciliation → restart policy](/documentation/concepts/reconciliation#restart-policy) and [`[server.restart]`](/documentation/reference/config-toml#server-restart). -**Permanent failures skip the budget.** Some failures cannot fix themselves on a retry: the image genuinely doesn't exist, a referenced config or key is absent, the container/VM spec is rejected, the Firecracker kernel or Cloud Hypervisor firmware is missing at its configured path, or the host is out of memory. Rather than bumping the counter by one and burning five reconcile cycles to reach the same conclusion, Ring classifies these as terminal and lands on the matching status (`image_pull_back_off`, `config_error`, `create_container_error`, `failed`, `insufficient_resources`) on the next tick. Transient failures — a pull that died mid-flight, a busy port, a network setup race — still bump by one and retry within the budget. The classification is shared by every runtime, so Docker, Podman, containerd, Cloud Hypervisor and Firecracker converge identically. +On **containerd, Cloud Hypervisor and Firecracker**, and for **jobs** on every runtime, the previous budget still applies: once `restart_count` reaches 5, a worker lands in `crash_loop_back_off` and a job in `failed`, and the reconciler stops retrying until the manifest is re-applied. Failures that cannot fix themselves (a missing image, config or firmware, a rejected spec) exhaust the budget at once. -Counters live in memory only, so restarting `ring server` clears them, so each `(deployment, instance, check)` triple starts back at zero after a server restart. +Health-check failure counters live in memory only, so each `(deployment, instance, check)` triple starts back at zero after a server restart. ## Observing the status diff --git a/documentation/concepts/reconciliation.md b/documentation/concepts/reconciliation.md index 6fc31a58..ee6b361f 100644 --- a/documentation/concepts/reconciliation.md +++ b/documentation/concepts/reconciliation.md @@ -78,9 +78,17 @@ In both cases, the missing instance is recreated automatically on the next tick. ## Restart policy -Ring tracks restart attempts per deployment. Past `MAX_RESTART_COUNT` (currently 5) failed boots, the deployment lands in `crash_loop_back_off` and the reconciler stops trying. The counter is **cumulative for the lifetime of the deployment**, not a sliding window; fix the underlying issue and re-apply the manifest to reset. +On Docker and Podman, a worker is **never abandoned**. It is a service: whatever ended it (a crash, a clean exit, a failed start, a liveness check), Ring starts it again, and slows down instead of giving up. -This protects the host from a tight crash loop pegging Docker / Cloud Hypervisor. +- Each failure counts one attempt in `restart_count`, and the next start waits a random delay between zero and `min(cap, base × 2^(attempt − 1))`: up to 10 s after the first failure, 20 s after the second, and so on up to 5 minutes. The randomness ("full jitter") spreads retries out, so workloads that failed together, after a host reboot for instance, do not all come back in the same second. +- While a worker waits, it shows `crash_loop_back_off` (or the status of the start error, such as `image_pull_back_off`), and the reconciler keeps watching it: it is started again as soon as the delay elapses, without a re-apply. Creating a missing config, or pushing the missing image, is enough. +- Failures that need an operator (a missing config, a spec the runtime rejects, an exit code `126`/`127` meaning the program cannot run) wait up to `cap` from the first attempt on. +- Once the worker has run for `stable_after` (10 minutes) without a failure, `restart_count` resets to 0. +- The delay, the running time and the last exit (code, time and the last 80 lines or 2 KiB of output) are stored in the database, so a `ring server` restart does not reset anyone's backoff. + +All of this is tunable in [`[server.restart]`](/documentation/reference/config-toml#server-restart), including `on_exhaustion = "fail"` for an operator who prefers a worker to stop after `max_attempts` failures. + +Jobs, and workers on containerd, Cloud Hypervisor and Firecracker, still use the previous budget: past 5 failed attempts, a worker lands in `crash_loop_back_off` and a job in `failed`, and the reconciler stops trying until the manifest is re-applied. Their retries are spaced on the same backoff curve. ## What survives a `ring server` restart @@ -93,6 +101,7 @@ Every input to the loop lives in SQLite: Two things **don't** survive: +- **Restart state survives**: the next start time, the running time and the last exit are stored with the deployment. - **Health-check failure counters**: they live in memory. After a restart, each `(deployment, instance, check)` triple starts back at zero. A flapping service won't trigger `on_failure` immediately after a server restart. - **In-flight runtime operations**: if `ring server` crashes mid-`apply`, the partial state is detected on the next tick and the reconciler converges. diff --git a/documentation/design/restart-policy.md b/documentation/design/restart-policy.md index ecd4d0d9..a1678e87 100644 --- a/documentation/design/restart-policy.md +++ b/documentation/design/restart-policy.md @@ -261,9 +261,9 @@ Clients of the API (CLI, dashboard, webhook consumers) must be updated together Each step is a separate pull request that builds, passes the suite, and can ship on its own. 1. **Policy module.** `scheduler/restart.rs` with its tests: backoff curve and jitter bounds, reset after `stable_after`, worker vs job exhaustion, `on_exhaustion = "fail"`, start-error starting points, resolution of a manifest override over the server defaults. Not wired yet. -2. **Persisted restart state.** Migration for `next_attempt_at`, `running_since`, `last_termination`. The scheduler reads and writes them; `RetryBackoff` and `HealthyWindow` are removed. The `[restart]` server section and the manifest `restart` block are parsed and validated. -3. **Runtime interface.** `observe` / `start_instance` / `stop_instance`, implemented for Docker and Podman. Scaling, rolling updates and the readiness gate move into the scheduler. The policy module is wired in for these runtimes, and liveness kills are counted as restarts. -4. **Remaining runtimes.** containerd, then Firecracker and Cloud Hypervisor, each deleting its own `handle_*` and restart-count code. +2. **Docker and Podman workers.** Persisted restart state (`next_attempt_at`, `running_since`, `last_termination`), the `[restart]` server section, and the instance interface (`observe` / `start_instance` / `stop_instance` / `discard_instance`) implemented for Docker and Podman. The scheduler reconciles their workers with the policy module and counts liveness kills as restarts. `RetryBackoff` is removed; the other runtimes keep their `apply` path but get their retries spaced on the persisted backoff. Persisting the state alone would not have shipped on its own: the runtimes still decided when to give up, and a longer reset window without that change would have abandoned workers that are restarted indefinitely today. +3. **Jobs and manifest overrides.** `backoff_limit` and the `restart` block in the manifest, then Docker and Podman jobs on the policy module. +4. **Remaining runtimes.** containerd, then Firecracker and Cloud Hypervisor, each deleting its own `handle_*` and restart-count code. `HealthyWindow` goes with the last one. 5. **Phase and reason.** Migration of the status column, API, CLI, dashboard and webhook changes, documentation (`deployment-status-lifecycle.md`, `reconciliation.md`, troubleshooting). 6. **Image digest reuse and start rate limiting.** 7. **Metrics and events.** diff --git a/documentation/help/troubleshooting.md b/documentation/help/troubleshooting.md index 541c260c..72f86f4b 100644 --- a/documentation/help/troubleshooting.md +++ b/documentation/help/troubleshooting.md @@ -155,14 +155,14 @@ See [how-to: deploy with secrets → private registry credentials](/documentatio ### `crash_loop_back_off` -The container has crashed more than `MAX_RESTART_COUNT` times. Look at: +The worker's instances keep dying. On Docker and Podman, Ring keeps restarting it, waiting longer between attempts (up to 5 minutes); on the other runtimes it gave up after 5 attempts. Look at: ```bash -ring deployment events --level warning # crashes show up here +ring deployment events --level error # one container_crashed per exit, with its code ring deployment logs --tail 200 # what the app said ``` -After fixing the root cause, re-apply the manifest. Ring resets the counter on a fresh apply. +On Docker and Podman, fixing the cause (a config, a dependency) is enough: the next attempt picks it up. Re-apply the manifest to start again right away with a fresh counter, or scale the worker to 0 to stop the retries. ### `insufficient_resources` @@ -173,9 +173,9 @@ ring deployment events --level error --limit 5 # insufficient host memory for 'web': needs 4096 MiB but only 1800 MiB is available — … ``` -This status is **terminal**: Ring does not retry, because the memory isn't going to reappear on its own. Two ways out: +Ring retries on the backoff curve, so the deployment starts by itself once enough memory is free. To get it running sooner: -- Free memory on the host (stop other workloads), then re-apply the manifest. +- Free memory on the host (stop other workloads). - Lower the deployment's `resources.requests.memory` (or `resources.limits.memory` if no request is set) to fit, then re-apply. The check compares against memory available *at that moment*; it's a guard against gross over-asks, not a precise reservation system. CPU is not gated; CPU overcommit is allowed. diff --git a/documentation/reference/api.md b/documentation/reference/api.md index 8239db92..94e91a72 100644 --- a/documentation/reference/api.md +++ b/documentation/reference/api.md @@ -145,13 +145,13 @@ List deployments. | `completed` | Job exited `0` (jobs only) | Yes | | `failed` | Job failed, or readiness deadline exceeded, or firmware not found | Yes | | `deleted` | Marked for teardown; instances being removed, then purged | No | -| `crash_loop_back_off` | Worker hit `MAX_RESTART_COUNT` (5) restarts | Yes | +| `crash_loop_back_off` | Worker instances keep dying; waiting for the next start (Docker, Podman) or gave up after 5 attempts (other runtimes) | No on Docker/Podman | | `image_pull_back_off` | Image couldn't be pulled (tag, auth, policy, network) | No (retried) | | `create_container_error` | Runtime rejected container creation | No (retried) | | `network_error` | Namespace network/bridge creation failed | No (retried) | | `config_error` | A mounted config or key doesn't exist | No (retried) | | `file_system_error` | IO error on volumes or temp config files | No (retried) | -| `insufficient_resources` | Host out of memory for the deployment's request | Yes | +| `insufficient_resources` | Host out of memory for the deployment's request | No (retried) | | `error` | Generic runtime fallback (stats, JSON, VM start, unclassified) | No (retried) | **Examples:** diff --git a/documentation/reference/config-toml.md b/documentation/reference/config-toml.md index f42d1d24..1bf6d210 100644 --- a/documentation/reference/config-toml.md +++ b/documentation/reference/config-toml.md @@ -14,6 +14,7 @@ A context describes one client→server connection; it has no business deciding ```toml [server] # daemon config (shared) [server.scheduler] # optional +[server.restart] # optional [server.dashboard] # optional [server.exec] # opt-in: enabled = true [server.telemetry.traces] # opt-in: enabled = true @@ -72,6 +73,20 @@ The daemon's own configuration, shared by every context in the file. All subsect |---|---|---|---|---| | `interval` | int (seconds) | no | `10` | Reconciliation tick interval. Overridden by `RING_SCHEDULER_INTERVAL` if set | +### `[server.restart]` + +How the scheduler restarts a failed worker. See [Reconciliation → restart policy](/documentation/concepts/reconciliation#restart-policy). Durations accept `s`, `m`, `h` and `d` (`"10s"`, `"5m"`, `"1h30m"`). A value Ring cannot parse stops the server at startup. + +| Field | Type | Required | Default | Purpose | +|---|---|---|---|---| +| `base` | duration | no | `"10s"` | Upper bound of the delay before the first retry | +| `cap` | duration | no | `"5m"` | Upper bound of the delay between two retries, however many failed. Must not be lower than `base` | +| `stable_after` | duration | no | `"10m"` | Uninterrupted running time after which the attempt counter resets | +| `on_exhaustion` | string | no | `"backoff"` | `"backoff"` keeps retrying at `cap` forever; `"fail"` marks the worker `failed` once its failures exceed `max_attempts` | +| `max_attempts` | int | no | `5` | Only read when `on_exhaustion = "fail"` | + +Applies to workers on Docker and Podman. Other runtimes and jobs still give up after 5 failed attempts. + ### `[server.dashboard]` | Field | Type | Required | Default | Purpose | diff --git a/documentation/runtimes/podman.md b/documentation/runtimes/podman.md index 335c6cc8..44a8bcd8 100644 --- a/documentation/runtimes/podman.md +++ b/documentation/runtimes/podman.md @@ -46,7 +46,7 @@ ring apply -f web.yaml ## Good to know -- **Crash detection is reconcile-based.** Podman has no Docker-style event listener, so a crashed container is noticed on the next scheduler tick rather than sub-second. A crash loop still converges to `crash_loop_back_off` (bounded), it's just tick-paced. +- **Crash detection is reconcile-based.** Podman has no Docker-style event listener, so a crashed container is noticed on the next scheduler tick rather than sub-second. A crash loop backs off exactly as on Docker; it's just tick-paced. - **Rootless remaps UID/GID.** A file written inside the container has a different owner on the host. Bind-mount and named-volume ownership behave differently than under Docker-root, so mind permissions on mounts. - **Host networking** (`network.mode: host`) works the same as Docker: Podman shares the Docker-compatible lifecycle and honours `NetworkMode: host`, so the container binds the host's network stack directly (no `ports:`, one replica). See [use host network mode](/documentation/how-to/use-host-network). - **Registry auth from the host.** `podman login` writes to `containers/auth.json`. Authorize it with `use_host_registry_auth = true` (point `host_registry_config` at the auth file) and pull with `config.use_host_auth: true`, with no secret in the manifest. See [manifest `config`](/documentation/reference/manifest#use_host_auth-credentials-from-the-host). diff --git a/migrations/20220101000027_restart_state.sql b/migrations/20220101000027_restart_state.sql new file mode 100644 index 00000000..00282d6e --- /dev/null +++ b/migrations/20220101000027_restart_state.sql @@ -0,0 +1,14 @@ +-- Restart state, persisted so a `ring server` restart no longer resets every +-- failing deployment's backoff at once. +-- +-- `next_attempt_at` when the scheduler may start an instance again, NULL +-- when it may do so now. +-- `running_since` start of the current uninterrupted run, which resets +-- `restart_count` once it lasts `stable_after`. +-- `last_termination` the last instance exit (code, time, tail of its logs), +-- JSON, kept after the instance itself is removed. +-- +-- Timestamps are RFC 3339 UTC. +ALTER TABLE deployment ADD COLUMN next_attempt_at TEXT DEFAULT NULL; +ALTER TABLE deployment ADD COLUMN running_since TEXT DEFAULT NULL; +ALTER TABLE deployment ADD COLUMN last_termination JSON DEFAULT NULL; diff --git a/src/commands/server.rs b/src/commands/server.rs index 1939ca13..458a42f4 100644 --- a/src/commands/server.rs +++ b/src/commands/server.rs @@ -68,6 +68,13 @@ pub(crate) async fn execute( std::process::exit(1); } + // Same reasoning for the restart policy: a typo'd duration must stop the + // daemon now, not leave the scheduler without a policy later. + if let Err(e) = configuration.server.restart.policy() { + error!("Refusing to start: {}", e); + std::process::exit(1); + } + let pool = get_database_pool().await; migrate_from_refinery_if_needed(&pool).await; diff --git a/src/config/server.rs b/src/config/server.rs index 4df71d88..99440bd8 100644 --- a/src/config/server.rs +++ b/src/config/server.rs @@ -24,6 +24,95 @@ pub(crate) struct ServerConfig { pub(crate) telemetry: TelemetryConfig, #[serde(default)] pub(crate) exec: ExecConfig, + #[serde(default)] + pub(crate) restart: RestartConfig, +} + +/// `[server.restart]`: how the scheduler restarts failed workloads. See +/// `documentation/design/restart-policy.md`. Durations accept `s`, `m`, `h` +/// and `d` units (`"10s"`, `"5m"`, `"1h30m"`). +#[derive(Deserialize, Debug, Clone)] +pub(crate) struct RestartConfig { + /// Upper bound of the delay before the first retry. + #[serde(default = "default_restart_base")] + pub(crate) base: String, + /// Upper bound of the delay between two retries, however many failed. + #[serde(default = "default_restart_cap")] + pub(crate) cap: String, + /// Uninterrupted running time after which the attempt counter resets. + #[serde(default = "default_restart_stable_after")] + pub(crate) stable_after: String, + /// What a worker does once its failures would exceed `max_attempts`: + /// `"backoff"` keeps retrying at `cap`, `"fail"` marks it failed. + #[serde(default = "default_restart_on_exhaustion")] + pub(crate) on_exhaustion: String, + /// Only read when `on_exhaustion = "fail"`. + #[serde(default = "default_restart_max_attempts")] + pub(crate) max_attempts: u32, +} + +fn default_restart_base() -> String { + "10s".to_string() +} + +fn default_restart_cap() -> String { + "5m".to_string() +} + +fn default_restart_stable_after() -> String { + "10m".to_string() +} + +fn default_restart_on_exhaustion() -> String { + "backoff".to_string() +} + +fn default_restart_max_attempts() -> u32 { + crate::scheduler::restart::DEFAULT_MAX_ATTEMPTS +} + +impl Default for RestartConfig { + fn default() -> Self { + RestartConfig { + base: default_restart_base(), + cap: default_restart_cap(), + stable_after: default_restart_stable_after(), + on_exhaustion: default_restart_on_exhaustion(), + max_attempts: default_restart_max_attempts(), + } + } +} + +impl RestartConfig { + /// The validated policy this section describes. + pub(crate) fn policy(&self) -> Result { + use crate::models::deployments::parse_interval; + use crate::scheduler::restart::{OnExhaustion, RestartPolicy}; + + let duration = |key: &str, value: &str| { + parse_interval(value).map_err(|e| format!("[server.restart] {key}: {e}")) + }; + let on_exhaustion = match self.on_exhaustion.as_str() { + "backoff" => OnExhaustion::Backoff, + "fail" => OnExhaustion::Fail, + other => { + return Err(format!( + "[server.restart] on_exhaustion: '{other}' is not one of \"backoff\", \"fail\"" + )); + } + }; + let policy = RestartPolicy { + base: duration("base", &self.base)?, + cap: duration("cap", &self.cap)?, + stable_after: duration("stable_after", &self.stable_after)?, + on_exhaustion, + max_attempts: self.max_attempts, + }; + policy + .validate() + .map_err(|e| format!("[server.restart] {e}"))?; + Ok(policy) + } } /// Bounds on interactive `exec` sessions. @@ -648,3 +737,54 @@ mod telemetry_tests { assert_eq!(cfg.telemetry.traces.sampler, "ratio:0.25"); } } + +#[cfg(test)] +mod restart_tests { + use super::*; + use crate::scheduler::restart::{OnExhaustion, RestartPolicy}; + use std::time::Duration; + + #[test] + fn absent_section_yields_the_default_policy() { + let cfg: ServerConfig = toml::from_str("").unwrap(); + assert_eq!(cfg.restart.policy(), Ok(RestartPolicy::default())); + } + + #[test] + fn section_overrides_parse() { + let cfg: ServerConfig = toml::from_str( + r#" + [restart] + base = "5s" + cap = "1h" + stable_after = "30m" + on_exhaustion = "fail" + max_attempts = 3 + "#, + ) + .unwrap(); + assert_eq!( + cfg.restart.policy(), + Ok(RestartPolicy { + base: Duration::from_secs(5), + cap: Duration::from_secs(3600), + stable_after: Duration::from_secs(1800), + on_exhaustion: OnExhaustion::Fail, + max_attempts: 3, + }) + ); + } + + #[test] + fn invalid_values_are_rejected() { + for toml in [ + "[restart]\nbase = \"soon\"", + "[restart]\non_exhaustion = \"never\"", + "[restart]\nbase = \"10m\"\ncap = \"1m\"", + "[restart]\nmax_attempts = 0", + ] { + let cfg: ServerConfig = toml::from_str(toml).unwrap(); + assert!(cfg.restart.policy().is_err(), "accepted: {toml}"); + } + } +} diff --git a/src/hypervisor/classifier.rs b/src/hypervisor/classifier.rs index 199f3a44..660abaee 100644 --- a/src/hypervisor/classifier.rs +++ b/src/hypervisor/classifier.rs @@ -94,6 +94,101 @@ pub(crate) fn classify_create_error(err: &RuntimeError) -> Disposition { } } +/// The status, event reason and operator-facing message a create-boundary +/// [`RuntimeError`] maps to. Shared by the runtimes and the scheduler so the +/// same failure reads the same everywhere. +pub(crate) fn create_error_outcome( + err: &RuntimeError, + deployment: &crate::models::deployments::Deployment, +) -> (DeploymentStatus, &'static str, String) { + match err { + RuntimeError::ImageNotFound(detail) => ( + DeploymentStatus::ImagePullBackOff, + "image_pull_back_off", + // Preserve the inner detail — it carries operator-relevant + // context like `image_pull_policy=Never forbids pulling` that a + // generic "not found" would drop. + format!("Image '{}' not found: {}", deployment.image, detail), + ), + RuntimeError::ImagePullFailed(detail) => ( + DeploymentStatus::ImagePullBackOff, + "image_pull_back_off", + format!("Failed to pull image '{}': {}", deployment.image, detail), + ), + RuntimeError::InstanceCreationFailed(msg) => ( + DeploymentStatus::CreateContainerError, + "instance_creation_failed", + format!("Container creation failed: {}", msg), + ), + RuntimeError::NetworkCreationFailed(_) => ( + DeploymentStatus::NetworkError, + "network_creation_failed", + format!( + "Failed to create network for namespace '{}'", + deployment.namespace + ), + ), + RuntimeError::ConfigNotFound(_) => ( + DeploymentStatus::ConfigError, + "config_error", + format!("Config not found in namespace '{}'", deployment.namespace), + ), + RuntimeError::ConfigKeyNotFound(_) => ( + DeploymentStatus::ConfigError, + "config_error", + format!( + "Config key not found in namespace '{}'", + deployment.namespace + ), + ), + RuntimeError::StatsFetchFailed(msg) => ( + DeploymentStatus::Error, + "stats_fetch_failed", + format!("Stats fetch failed: {}", msg), + ), + RuntimeError::InsufficientResources(detail) => ( + DeploymentStatus::InsufficientResources, + "insufficient_resources", + detail.clone(), + ), + RuntimeError::Other(msg) => ( + DeploymentStatus::Error, + "runtime_error", + format!("Docker error: {}", msg), + ), + // CH-specific variants — Docker should never produce them, but the + // enum is shared so we map them to the closest Docker-side state. + RuntimeError::FirmwareNotFound(msg) => ( + DeploymentStatus::Failed, + "firmware_not_found", + format!("Firmware not found: {}", msg), + ), + RuntimeError::VmStartFailed(msg) => ( + DeploymentStatus::Error, + "vm_start_failed", + format!("VM start failed: {}", msg), + ), + // The CH runtime emits this; the Docker runtime doesn't (the daemon + // rejects port conflicts itself through InstanceCreationFailed). The + // arm exists to keep the match exhaustive over the shared enum. + RuntimeError::PortAlreadyInUse(port) => ( + DeploymentStatus::Error, + "port_allocation_failed", + format!("Port {} is already allocated", port), + ), + RuntimeError::Io(e) => ( + DeploymentStatus::FileSystemError, + "file_system_error", + format!("IO error: {}", e), + ), + RuntimeError::Json(e) => ( + DeploymentStatus::Error, + "runtime_error", + format!("JSON error: {}", e), + ), + } +} + /// Classify a crash-boundary exit code into terminal-or-retry. /// /// A worker that exits is normally restarted (transient): the process may have @@ -210,7 +305,13 @@ pub(crate) fn apply_vm_start_failure( // Concretely: a deployment refused by host-memory admission used to // report 5 restarts having never started a single process, sending // whoever read it looking for an instability that never existed. - if !scheduler_skips_by_status(&terminal) { + // + // A host short on memory is the exception: memory is freed all the + // time, so the refusal counts as one attempt and is retried on the + // backoff curve like a transient failure. + if terminal == DeploymentStatus::InsufficientResources { + deployment.restart_count += 1; + } else if !scheduler_skips_by_status(&terminal) { deployment.restart_count = MAX_RESTART_COUNT; } deployment.status = terminal; @@ -279,12 +380,11 @@ mod tests { ); } - /// A deployment refused before anything ran must not claim restarts it - /// never made. `restart_count` is displayed by the CLI, the API and the - /// dashboard as a count of actual restarts; reporting 5 for a workload that - /// never started a process sends an operator hunting a phantom instability. + /// A deployment refused for lack of memory is retried, since memory is + /// freed all the time: the refusal counts as one attempt, not as the whole + /// budget, which would claim restarts that never happened. #[test] - fn admission_refusal_does_not_invent_restarts() { + fn admission_refusal_counts_one_attempt() { let mut d = vm_deployment(); assert_eq!(d.restart_count, 0); @@ -296,10 +396,7 @@ mod tests { ); assert_eq!(d.status, DeploymentStatus::InsufficientResources); - assert_eq!( - d.restart_count, 0, - "no VM was ever spawned, so no restart may be reported" - ); + assert_eq!(d.restart_count, 1); } /// The counterpart: statuses the scheduler DOES keep reconciling still need diff --git a/src/hypervisor/instance_driver.rs b/src/hypervisor/instance_driver.rs new file mode 100644 index 00000000..3473940b --- /dev/null +++ b/src/hypervisor/instance_driver.rs @@ -0,0 +1,46 @@ +//! Instance-level runtime operations: report facts, start and stop instances, +//! make no decisions. +//! +//! A runtime that implements [`InstanceDriver`] leaves the restart policy, the +//! backoff and the scaling decisions to the scheduler (`scheduler::reconcile`), +//! which runs the same code for every such runtime. Runtimes that do not yet +//! implement it keep reconciling through [`RuntimeLifecycle::apply`]. +//! +//! [`RuntimeLifecycle::apply`]: crate::hypervisor::lifecycle_trait::RuntimeLifecycle::apply + +use crate::hypervisor::error::RuntimeError; +use crate::models::deployments::Deployment; +use crate::models::restart_state::Termination; +use crate::models::volume::ResolvedMount; +use async_trait::async_trait; + +/// The instances of one deployment, as the runtime sees them now. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub(crate) struct Observation { + /// Live instances. + pub(crate) running: Vec, + /// Instances that ended on their own, oldest first. Instances the + /// scheduler stopped itself are not reported. Each one keeps being + /// reported until it is [discarded](InstanceDriver::discard_instance). + pub(crate) terminated: Vec, +} + +#[async_trait] +pub(crate) trait InstanceDriver: Send + Sync { + async fn observe(&self, deployment: &Deployment) -> Observation; + + /// Create and start one instance, appending its id to + /// `deployment.instances`. Never touches the status or the counters. + async fn start_instance( + &self, + deployment: &mut Deployment, + resolved_mounts: &[ResolvedMount], + ) -> Result<(), RuntimeError>; + + /// Stop and remove a live instance. Its exit is not reported as a + /// termination. + async fn stop_instance(&self, instance_id: &str) -> bool; + + /// Remove a terminated instance once its termination has been handled. + async fn discard_instance(&self, instance_id: &str); +} diff --git a/src/hypervisor/lifecycle_trait.rs b/src/hypervisor/lifecycle_trait.rs index 4634660f..a9258502 100644 --- a/src/hypervisor/lifecycle_trait.rs +++ b/src/hypervisor/lifecycle_trait.rs @@ -239,6 +239,12 @@ pub(crate) trait RuntimeLifecycle: Send + Sync { resolved_mounts: Vec, ) -> Deployment; + /// Instance-level operations, for runtimes whose workers the scheduler + /// reconciles itself. `None` keeps the runtime on [`Self::apply`]. + fn instance_driver(&self) -> Option<&dyn crate::hypervisor::instance_driver::InstanceDriver> { + None + } + async fn list_instances(&self, deployment_id: String, status: &str) -> Vec; /// Resolve the running instance ids for many deployments at once, keyed by diff --git a/src/hypervisor/mod.rs b/src/hypervisor/mod.rs index edddfeef..1bbdb651 100644 --- a/src/hypervisor/mod.rs +++ b/src/hypervisor/mod.rs @@ -10,6 +10,7 @@ pub(crate) mod error; pub(crate) mod health_probes; pub(crate) mod host_nat; pub(crate) mod host_net; +pub(crate) mod instance_driver; pub(crate) mod lifecycle_trait; #[cfg(test)] pub(crate) mod mock; diff --git a/src/models/deployments.rs b/src/models/deployments.rs index 1b2c7dea..a8246335 100644 --- a/src/models/deployments.rs +++ b/src/models/deployments.rs @@ -15,16 +15,22 @@ pub(crate) const MAX_RESTART_COUNT: u32 = 5; /// them derived from one array is what stops the two drifting apart — a status /// added here while still reported as "skipped" would be retried forever with /// no restart budget to stop it. +/// +/// `crash_loop_back_off` and `insufficient_resources` are reconciled too: a +/// worker in either is backing off, not abandoned, and is started again once +/// its backoff elapses. Only `completed` and `failed` are left alone. pub(crate) const RECONCILED_STATUSES: &[DeploymentStatus] = &[ DeploymentStatus::Pending, DeploymentStatus::Creating, DeploymentStatus::Running, DeploymentStatus::Deleted, + DeploymentStatus::CrashLoopBackOff, DeploymentStatus::CreateContainerError, DeploymentStatus::ImagePullBackOff, DeploymentStatus::NetworkError, DeploymentStatus::ConfigError, DeploymentStatus::FileSystemError, + DeploymentStatus::InsufficientResources, DeploymentStatus::Error, ]; diff --git a/src/models/mod.rs b/src/models/mod.rs index 0706b10f..80c68a62 100644 --- a/src/models/mod.rs +++ b/src/models/mod.rs @@ -7,6 +7,7 @@ pub(crate) mod health_check; pub(crate) mod health_check_logs; pub(crate) mod namespace; pub(crate) mod query; +pub(crate) mod restart_state; pub(crate) mod secret; pub(crate) mod token; pub(crate) mod users; diff --git a/src/models/restart_state.rs b/src/models/restart_state.rs new file mode 100644 index 00000000..d5c6f7cf --- /dev/null +++ b/src/models/restart_state.rs @@ -0,0 +1,218 @@ +//! Persisted restart state of a deployment: when it may be started again, how +//! long its instances have been running, and how the last one ended. +//! +//! Kept apart from [`crate::models::deployments::Deployment`] and written with +//! targeted statements, like `restart_count`: the scheduler's full-row write +//! back must never carry these fields, or a stale copy would undo a backoff. + +use chrono::{DateTime, Utc}; +use serde::{Deserialize, Serialize}; +use sqlx::SqlitePool; +use std::collections::HashMap; + +/// How an instance ended, kept after the instance itself is removed. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub(crate) struct Termination { + pub(crate) instance_id: String, + /// `None` when the runtime could not tell. + pub(crate) exit_code: Option, + pub(crate) finished_at: DateTime, + /// Last lines of the instance's output, at most [`LOGS_TAIL_BYTES`]. + #[serde(skip_serializing_if = "Option::is_none", default)] + pub(crate) logs_tail: Option, +} + +/// Lines of output kept from a terminated instance. +pub(crate) const LOGS_TAIL_LINES: usize = 80; +/// Upper bound on the output kept from a terminated instance. +pub(crate) const LOGS_TAIL_BYTES: usize = 2048; + +/// Keep the end of `lines`: at most [`LOGS_TAIL_LINES`] lines and +/// [`LOGS_TAIL_BYTES`] bytes, whichever is smaller. `None` when there is +/// nothing to keep. +pub(crate) fn logs_tail(lines: &[String]) -> Option { + let start = lines.len().saturating_sub(LOGS_TAIL_LINES); + let joined = lines[start..].join("\n"); + if joined.is_empty() { + return None; + } + if joined.len() <= LOGS_TAIL_BYTES { + return Some(joined); + } + let mut cut = joined.len() - LOGS_TAIL_BYTES; + while !joined.is_char_boundary(cut) { + cut += 1; + } + Some(joined[cut..].to_string()) +} + +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub(crate) struct RestartState { + pub(crate) next_attempt_at: Option>, + pub(crate) running_since: Option>, + pub(crate) last_termination: Option, +} + +impl RestartState { + /// Whether the deployment must not be started before a later tick. + pub(crate) fn backing_off(&self, now: DateTime) -> bool { + self.next_attempt_at.is_some_and(|at| at > now) + } +} + +#[derive(sqlx::FromRow)] +struct Row { + id: String, + next_attempt_at: Option, + running_since: Option, + last_termination: Option, +} + +fn parse_time(raw: Option) -> Option> { + raw.and_then(|s| DateTime::parse_from_rfc3339(&s).ok()) + .map(|t| t.with_timezone(&Utc)) +} + +/// The restart state of every deployment that has one, by deployment id. +/// Deployments with nothing recorded are left out. +pub(crate) async fn find_all( + pool: &SqlitePool, +) -> Result, sqlx::Error> { + let rows: Vec = sqlx::query_as( + "SELECT id, next_attempt_at, running_since, last_termination FROM deployment \ + WHERE next_attempt_at IS NOT NULL OR running_since IS NOT NULL OR last_termination IS NOT NULL", + ) + .fetch_all(pool) + .await?; + + Ok(rows + .into_iter() + .map(|row| { + let state = RestartState { + next_attempt_at: parse_time(row.next_attempt_at), + running_since: parse_time(row.running_since), + last_termination: row + .last_termination + .and_then(|json| serde_json::from_str(&json).ok()), + }; + (row.id, state) + }) + .collect()) +} + +/// Store the restart state of one deployment. +pub(crate) async fn save( + pool: &SqlitePool, + deployment_id: &str, + state: &RestartState, +) -> Result<(), sqlx::Error> { + let last_termination = match &state.last_termination { + Some(t) => Some(serde_json::to_string(t).map_err(|e| sqlx::Error::Encode(Box::new(e)))?), + None => None, + }; + sqlx::query( + "UPDATE deployment SET next_attempt_at = ?, running_since = ?, last_termination = ? WHERE id = ?", + ) + .bind(state.next_attempt_at.map(|t| t.to_rfc3339())) + .bind(state.running_since.map(|t| t.to_rfc3339())) + .bind(last_termination) + .bind(deployment_id) + .execute(pool) + .await?; + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use sqlx::sqlite::SqlitePoolOptions; + + async fn test_pool() -> SqlitePool { + let pool = SqlitePoolOptions::new() + .max_connections(1) + .connect("sqlite::memory:") + .await + .unwrap(); + sqlx::migrate!("./migrations").run(&pool).await.unwrap(); + sqlx::query( + "INSERT INTO deployment (id, created_at, status, namespace, runtime, kind, name, restart_count) \ + VALUES ('d1', '2026-01-01', 'running', 'ns', 'docker', 'worker', 'w', 0), \ + ('d2', '2026-01-01', 'running', 'ns', 'docker', 'worker', 'w2', 0)", + ) + .execute(&pool) + .await + .unwrap(); + pool + } + + fn at(s: &str) -> DateTime { + DateTime::parse_from_rfc3339(s).unwrap().with_timezone(&Utc) + } + + #[tokio::test] + async fn saved_state_is_read_back() { + let pool = test_pool().await; + let state = RestartState { + next_attempt_at: Some(at("2026-01-01T00:05:00Z")), + running_since: None, + last_termination: Some(Termination { + instance_id: "c1".to_string(), + exit_code: Some(1), + finished_at: at("2026-01-01T00:00:00Z"), + logs_tail: Some("boom".to_string()), + }), + }; + save(&pool, "d1", &state).await.unwrap(); + + let all = find_all(&pool).await.unwrap(); + assert_eq!(all.get("d1"), Some(&state)); + assert!( + !all.contains_key("d2"), + "nothing recorded, nothing returned" + ); + } + + #[tokio::test] + async fn saving_an_empty_state_clears_it() { + let pool = test_pool().await; + let state = RestartState { + next_attempt_at: Some(at("2026-01-01T00:05:00Z")), + ..Default::default() + }; + save(&pool, "d1", &state).await.unwrap(); + save(&pool, "d1", &RestartState::default()).await.unwrap(); + assert!(find_all(&pool).await.unwrap().is_empty()); + } + + #[test] + fn backing_off_only_until_the_next_attempt() { + let state = RestartState { + next_attempt_at: Some(at("2026-01-01T00:05:00Z")), + ..Default::default() + }; + assert!(state.backing_off(at("2026-01-01T00:04:59Z"))); + assert!(!state.backing_off(at("2026-01-01T00:05:00Z"))); + assert!(!RestartState::default().backing_off(at("2026-01-01T00:00:00Z"))); + } + + #[test] + fn logs_tail_keeps_the_last_lines() { + let lines: Vec = (0..100).map(|i| format!("l{i}")).collect(); + let tail = logs_tail(&lines).unwrap(); + assert!(tail.starts_with("l20\n")); + assert!(tail.ends_with("l99")); + } + + #[test] + fn logs_tail_is_bounded_in_bytes() { + let lines = vec!["é".repeat(3000)]; + let tail = logs_tail(&lines).unwrap(); + assert!(tail.len() <= LOGS_TAIL_BYTES); + assert!(tail.chars().all(|c| c == 'é')); + } + + #[test] + fn logs_tail_of_nothing_is_none() { + assert_eq!(logs_tail(&[]), None); + } +} diff --git a/src/runtime/docker/docker_lifecycle.rs b/src/runtime/docker/docker_lifecycle.rs index 2039ecb9..c9d6e39f 100644 --- a/src/runtime/docker/docker_lifecycle.rs +++ b/src/runtime/docker/docker_lifecycle.rs @@ -1,10 +1,12 @@ use crate::api::dto::stats::InstanceStatsOutput; use crate::hypervisor::error::RuntimeError; +use crate::hypervisor::instance_driver::{InstanceDriver, Observation}; use crate::hypervisor::lifecycle_trait::{ ExecError, ExecRequest, ExecSession, Log, RuntimeLifecycle, classify_log, extract_date, }; use crate::models::deployments::Deployment; use crate::models::health_check::HealthCheckStatus; +use crate::models::restart_state::{LOGS_TAIL_LINES, Termination, logs_tail}; use crate::models::volume::{ReloadSignal, ResolvedMount}; use crate::runtime::registry_auth::HostAuthSettings; use crate::scheduler::intentional_shutdowns::IntentionalShutdowns; @@ -39,9 +41,9 @@ pub struct DockerLifecycle { } impl DockerLifecycle { - /// Crash counting is reconcile-driven for every runtime: the reconcile loop - /// owns `restart_count` and detects exited containers in-tick, so Docker and - /// Podman converge to `CrashLoopBackOff` the same way. The Docker event + /// Crash counting is reconcile-driven: the scheduler observes exited + /// containers through the instance driver and owns `restart_count`, so + /// Docker and Podman back off the same way. The Docker event /// listener is observational only (it logs crash causes, never the count). pub fn new( docker: Docker, @@ -66,8 +68,93 @@ impl DockerLifecycle { } } +#[async_trait] +impl InstanceDriver for DockerLifecycle { + async fn observe(&self, deployment: &Deployment) -> Observation { + let (running, ended) = + super::instances::list_live_and_ended(&self.docker, &deployment.id).await; + + let mut terminated = Vec::new(); + for instance_id in ended { + // Stopped by Ring itself (scale-down, rolling drain, health check): + // not a failure, nothing to report. + if self.intentional_shutdowns.take(&instance_id).await { + super::container::remove_container(self.docker.clone(), instance_id).await; + continue; + } + terminated.push(self.termination(instance_id).await); + } + terminated.sort_by_key(|t| t.finished_at); + + Observation { + running, + terminated, + } + } + + async fn start_instance( + &self, + deployment: &mut Deployment, + resolved_mounts: &[ResolvedMount], + ) -> Result<(), RuntimeError> { + super::container::create_container( + deployment, + &self.docker, + resolved_mounts, + &self.host_auth, + ) + .await + } + + async fn stop_instance(&self, instance_id: &str) -> bool { + self.intentional_shutdowns + .mark(instance_id.to_string()) + .await; + super::container::remove_container_by_id(&self.docker, instance_id.to_string()).await + } + + async fn discard_instance(&self, instance_id: &str) { + super::container::remove_container(self.docker.clone(), instance_id.to_string()).await; + } +} + +impl DockerLifecycle { + /// How an ended container finished: its exit code, when, and the end of + /// its output, read before the container is removed with them. + async fn termination(&self, instance_id: String) -> Termination { + let state = self + .docker + .inspect_container(&instance_id, None::) + .await + .ok() + .and_then(|details| details.state); + let exit_code = state.as_ref().and_then(|s| s.exit_code); + // A container that never ran reports the zero time; fall back to now. + let finished_at = state + .as_ref() + .and_then(|s| s.finished_at.as_deref()) + .and_then(|t| DateTime::parse_from_rfc3339(t).ok()) + .map(|t| t.with_timezone(&Utc)) + .filter(|t| t.timestamp() > 0) + .unwrap_or_else(Utc::now); + let tail = (LOGS_TAIL_LINES).to_string(); + let lines = super::logs::logs(&self.docker, instance_id.clone(), Some(&tail), None).await; + + Termination { + instance_id, + exit_code, + finished_at, + logs_tail: logs_tail(&lines), + } + } +} + #[async_trait] impl RuntimeLifecycle for DockerLifecycle { + fn instance_driver(&self) -> Option<&dyn InstanceDriver> { + Some(self) + } + async fn apply( &self, deployment: Deployment, @@ -281,6 +368,97 @@ impl RuntimeLifecycle for DockerLifecycle { mod tests { use super::*; + fn driver_test_worker(id: &str, command: &[&str]) -> Deployment { + use crate::models::deployments::DeploymentStatus; + use std::collections::HashMap; + + Deployment { + id: id.to_string(), + created_at: Utc::now().to_string(), + updated_at: None, + status: DeploymentStatus::Pending, + restart_count: 0, + namespace: "ring-driver-test".to_string(), + name: id.to_string(), + image: crate::runtime::docker::test_support::TEST_IMAGE.to_string(), + config: None, + runtime: "docker".to_string(), + kind: "worker".to_string(), + replicas: 1, + command: command.iter().map(|s| s.to_string()).collect(), + instances: vec![], + labels: HashMap::new(), + environment: HashMap::new(), + volumes: "[]".to_string(), + health_checks: vec![], + resources: None, + autoscale: None, + desired_replicas: None, + image_digest: None, + ports: vec![], + pending_events: vec![], + parent_id: None, + network: None, + } + } + + /// The driver reports an instance that exited with its code and output, + /// keeps reporting it until it is discarded, and does not report one it + /// stopped itself. + #[tokio::test] + async fn driver_reports_exits_but_not_its_own_stops() { + use crate::runtime::docker::test_support::{daemon, ensure_image}; + + let Some(docker) = daemon().await else { + eprintln!("skipping: no Docker daemon"); + return; + }; + if !ensure_image(&docker).await { + eprintln!("skipping: could not obtain the test image"); + return; + } + let driver = DockerLifecycle::new( + docker.clone(), + IntentionalShutdowns::new(), + HostAuthSettings::default(), + ); + + // An instance that exits on its own. + let mut crasher = driver_test_worker( + "ring-driver-crasher", + &["sh", "-c", "echo going down; exit 3"], + ); + driver.start_instance(&mut crasher, &[]).await.unwrap(); + let mut observed = Observation::default(); + for _ in 0..50 { + observed = driver.observe(&crasher).await; + if !observed.terminated.is_empty() { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + } + let ended = observed.terminated.first().cloned(); + if let Some(t) = &ended { + driver.discard_instance(&t.instance_id).await; + } + let after_discard = driver.observe(&crasher).await; + + // An instance Ring stops itself. + let mut sleeper = driver_test_worker("ring-driver-sleeper", &["sleep", "60"]); + driver.start_instance(&mut sleeper, &[]).await.unwrap(); + let running = driver.observe(&sleeper).await; + let stopped = driver.stop_instance(&sleeper.instances[0]).await; + let after_stop = driver.observe(&sleeper).await; + + let ended = ended.expect("the crashed instance is reported"); + assert_eq!(ended.exit_code, Some(3)); + assert_eq!(ended.logs_tail.as_deref(), Some("going down")); + assert_eq!(after_discard, Observation::default()); + assert_eq!(running.running.len(), 1); + assert!(stopped); + assert_eq!(after_stop, Observation::default()); + } + /// The start time comes from the daemon's own format, so only a real /// daemon can tell whether it parses. #[tokio::test] diff --git a/src/runtime/docker/instances.rs b/src/runtime/docker/instances.rs index f1086257..fb62f8e6 100644 --- a/src/runtime/docker/instances.rs +++ b/src/runtime/docker/instances.rs @@ -104,6 +104,53 @@ pub(crate) async fn list_running_instances_grouped( grouped } +/// Whether a container is live (`Some(true)`), has ended (`Some(false)`), or is +/// on its way out and belongs to neither (`None`). +/// +/// A container stuck in `created` has ended: Docker accepted the spec but it +/// never ran, and counting it as live would hide the failure. +fn liveness(state: Option<&ContainerSummaryStateEnum>) -> Option { + match state? { + ContainerSummaryStateEnum::RUNNING + | ContainerSummaryStateEnum::RESTARTING + | ContainerSummaryStateEnum::PAUSED => Some(true), + ContainerSummaryStateEnum::EXITED + | ContainerSummaryStateEnum::DEAD + | ContainerSummaryStateEnum::CREATED => Some(false), + ContainerSummaryStateEnum::REMOVING | ContainerSummaryStateEnum::EMPTY => None, + } +} + +/// The containers of a deployment split into live and ended ones, in a single +/// list call. +pub(crate) async fn list_live_and_ended(docker: &Docker, id: &str) -> (Vec, Vec) { + let mut live = Vec::new(); + let mut ended = Vec::new(); + + match docker + .list_containers(Some(build_list_options("all"))) + .await + { + Ok(containers) => { + for container in containers { + if let Some(labels) = &container.labels + && labels.get(super::RING_DEPLOYMENT_LABEL).map(String::as_str) == Some(id) + && let Some(container_id) = container.id + { + match liveness(container.state.as_ref()) { + Some(true) => live.push(container_id), + Some(false) => ended.push(container_id), + None => {} + } + } + } + } + Err(e) => debug!("Docker list instances error: {}", e), + } + + (live, ended) +} + pub(crate) async fn list_instances_with_names( docker: &Docker, id: String, @@ -141,6 +188,21 @@ pub(crate) async fn list_instances_with_names( mod tests { use super::*; + #[test] + fn liveness_splits_live_from_ended() { + use ContainerSummaryStateEnum::*; + for state in [RUNNING, RESTARTING, PAUSED] { + assert_eq!(liveness(Some(&state)), Some(true), "{state:?}"); + } + for state in [EXITED, DEAD, CREATED] { + assert_eq!(liveness(Some(&state)), Some(false), "{state:?}"); + } + for state in [REMOVING, EMPTY] { + assert_eq!(liveness(Some(&state)), None, "{state:?}"); + } + assert_eq!(liveness(None), None); + } + #[test] fn active_counts_running_and_restarting() { assert!(matches_status( diff --git a/src/runtime/docker/lifecycle.rs b/src/runtime/docker/lifecycle.rs index 6d0dd37e..fb86dbdb 100644 --- a/src/runtime/docker/lifecycle.rs +++ b/src/runtime/docker/lifecycle.rs @@ -1,6 +1,5 @@ use super::container::{create_container, remove_container}; use super::instances::list_instances; -use crate::hypervisor::classifier::Disposition; use crate::hypervisor::error::RuntimeError; use crate::hypervisor::types::InstanceStatus; use crate::models::deployments::{Deployment, DeploymentStatus, MAX_RESTART_COUNT}; @@ -9,9 +8,11 @@ use crate::runtime::registry_auth::HostAuthSettings; use crate::scheduler::intentional_shutdowns::IntentionalShutdowns; use bollard::Docker; use bollard::query_parameters::InspectContainerOptions; -use std::cmp::Ordering; -use std::convert::TryInto; +/// Reconcile a Docker/Podman deployment through the legacy `apply` path: jobs, +/// and the teardown of deleted deployments. Workers are reconciled by the +/// scheduler through [`super::docker_lifecycle::DockerLifecycle`]'s instance +/// driver, which owns their restart policy. pub(crate) async fn apply( mut deployment: Deployment, docker: Docker, @@ -26,131 +27,22 @@ pub(crate) async fn apply( }; deployment.instances = list_instances(&docker, deployment.id.to_string(), status_filter).await; - if deployment.kind == "job" { - handle_job_deployment( - deployment, - docker, - resolved_mounts, - intentional_shutdowns, - host_auth, - ) - .await - } else { - handle_worker_deployment( - deployment, - docker, - resolved_mounts, - intentional_shutdowns, - host_auth, - ) - .await - } -} - -/// Count each unexpectedly-exited container toward `restart_count`, record the -/// crash on the deployment's pending events, and flip to `CrashLoopBackOff` once -/// the count reaches `MAX_RESTART_COUNT`. Returns `true` when reconciling should -/// stop this tick — either because the bound was hit, or because an exit code is -/// non-retryable (the worker can never start its program) and we fail fast onto -/// the terminal status instead of burning the whole restart budget. -/// -/// Each entry is `(container_id, exit_code)`; the exit code drives the -/// fast-fail classification (`classify_exit_code`). Pure (no I/O) so the -/// crash-loop convergence and the fast-fail can be unit-tested without a Docker -/// daemon — see the tests in this module. -fn apply_unexpected_exits(deployment: &mut Deployment, exited: &[(String, Option)]) -> bool { - for (container_id, exit_code) in exited { - let disposition = crate::hypervisor::classifier::classify_exit_code(*exit_code); - - deployment.restart_count += 1; - deployment.emit_event( - "error", - format!( - "Container {} exited unexpectedly (restart {})", - &container_id[..container_id.len().min(12)], - deployment.restart_count - ), - "docker", - Some("container_crashed"), - ); - - // Fast-fail on a non-retryable exit (127 command-not-found, 126 - // not-executable): the container can never start its program, so - // retrying it up to MAX_RESTART_COUNT only delays the inevitable. Land - // on the terminal status now. - if let Disposition::Terminal(status) = disposition { - deployment.emit_event( - "error", - format!( - "Container {} exited with a non-retryable code ({:?}); failing fast", - &container_id[..container_id.len().min(12)], - exit_code - ), - "docker", - Some("non_retryable_exit"), - ); - deployment.status = status; - return true; - } - } - - if deployment.restart_count >= MAX_RESTART_COUNT { - deployment.status = DeploymentStatus::CrashLoopBackOff; - return true; + if deployment.status == DeploymentStatus::Deleted { + debug!("{} marked as deleted. Remove all instances", deployment.id); + let kind = deployment.kind.clone(); + remove_all_instances(&mut deployment, &docker, &kind, &intentional_shutdowns).await; + return deployment; } - false -} -/// Detect containers that started then exited unexpectedly since the last -/// reconcile, and bump `restart_count` for each so a crash loop eventually -/// reaches `CrashLoopBackOff`. -/// -/// Runs for every runtime, Docker included. The reconcile loop is the single -/// writer of `restart_count` for worker exits: it reads the row at the start of -/// a tick and writes it back at the end, so an async Docker-events listener that -/// also bumped the counter would lose its update to that full-row write (the -/// race that let a crash loop recreate forever). The events listener therefore -/// no longer mutates `restart_count`; this in-tick path owns it for both Docker -/// and Podman, which makes counting race-free by construction. -/// -/// Operator-initiated stops (scale-down, delete, rolling update, health-check -/// eviction) pre-mark the container in `IntentionalShutdowns`; those are -/// consumed and skipped here so a scale-down is never mistaken for a crash. Each -/// real crash is reaped (removed) so its `exited` state isn't re-counted on the -/// next tick. -async fn detect_and_count_crashes( - deployment: &mut Deployment, - docker: &Docker, - intentional_shutdowns: &IntentionalShutdowns, -) -> bool { - let exited = list_instances(docker, deployment.id.to_string(), "exited").await; - let mut crashed = Vec::new(); - for container_id in exited { - if intentional_shutdowns.take(&container_id).await { - // Operator-initiated stop — not a crash. Reap it quietly. - remove_container(docker.clone(), container_id).await; - continue; - } - // Capture the exit code before reaping so the crash boundary can decide - // retry-vs-fail-fast (e.g. 127 = bad entrypoint → never retryable). - let exit_code = inspect_exit_code(docker, &container_id).await; - // Reap the dead container so it is not counted again next tick. - remove_container(docker.clone(), container_id.clone()).await; - crashed.push((container_id, exit_code)); + if deployment.kind == "job" { + return handle_job_deployment(deployment, docker, resolved_mounts, host_auth).await; } - apply_unexpected_exits(deployment, &crashed) -} - -/// Read a container's last exit code via `inspect`, or `None` when it can't be -/// determined (inspect failed, or the daemon reports no code). `None` is treated -/// as retryable by the classifier — we never fail fast on a missing code. -async fn inspect_exit_code(docker: &Docker, container_id: &str) -> Option { - let info = docker - .inspect_container(container_id, None::) - .await - .ok()?; - info.state?.exit_code + warn!( + "Worker {} reached the Docker apply path; workers are reconciled by the scheduler", + deployment.id + ); + deployment } fn handle_create_error(deployment: &mut Deployment, err: RuntimeError, increment_restart: bool) { @@ -162,10 +54,9 @@ fn handle_create_error(deployment: &mut Deployment, err: RuntimeError, increment // next tick instead of five ticks from now. Transient errors still bump by // one and retry within the budget, exactly as before. // - // One exception: a few terminal statuses are already excluded from the - // scheduler's reconcile filter, so the budget marker buys nothing there and - // would report restarts that never happened — a host-memory refusal is - // checked before the image pull, so nothing was ever created. + // Exceptions: a terminal status the scheduler no longer reconciles needs + // no budget marker, and a host-memory refusal counts as one attempt only, + // since memory is freed all the time and the refusal is retried. let disposition = crate::hypervisor::classifier::classify_create_error(&err); let terminal = disposition.is_terminal(); let marker_needed = match &disposition { @@ -175,100 +66,17 @@ fn handle_create_error(deployment: &mut Deployment, err: RuntimeError, increment crate::hypervisor::classifier::Disposition::Retry => true, }; + let refused_for_memory = matches!(err, RuntimeError::InsufficientResources(_)); if increment_restart && marker_needed { - if terminal { + if terminal && !refused_for_memory { deployment.restart_count = MAX_RESTART_COUNT; } else { deployment.restart_count += 1; } } - let (status, reason, message) = match &err { - RuntimeError::ImageNotFound(detail) => ( - DeploymentStatus::ImagePullBackOff, - "image_pull_back_off", - // Preserve the inner detail — it carries operator-relevant - // context like `image_pull_policy=Never forbids pulling` that a - // generic "not found" would drop. - format!("Image '{}' not found: {}", deployment.image, detail), - ), - RuntimeError::ImagePullFailed(detail) => ( - DeploymentStatus::ImagePullBackOff, - "image_pull_back_off", - format!("Failed to pull image '{}': {}", deployment.image, detail), - ), - RuntimeError::InstanceCreationFailed(msg) => ( - DeploymentStatus::CreateContainerError, - "instance_creation_failed", - format!("Container creation failed: {}", msg), - ), - RuntimeError::NetworkCreationFailed(_) => ( - DeploymentStatus::NetworkError, - "network_creation_failed", - format!( - "Failed to create network for namespace '{}'", - deployment.namespace - ), - ), - RuntimeError::ConfigNotFound(_) => ( - DeploymentStatus::ConfigError, - "config_error", - format!("Config not found in namespace '{}'", deployment.namespace), - ), - RuntimeError::ConfigKeyNotFound(_) => ( - DeploymentStatus::ConfigError, - "config_error", - format!( - "Config key not found in namespace '{}'", - deployment.namespace - ), - ), - RuntimeError::StatsFetchFailed(msg) => ( - DeploymentStatus::Error, - "stats_fetch_failed", - format!("Stats fetch failed: {}", msg), - ), - RuntimeError::InsufficientResources(detail) => ( - DeploymentStatus::InsufficientResources, - "insufficient_resources", - detail.clone(), - ), - RuntimeError::Other(msg) => ( - DeploymentStatus::Error, - "runtime_error", - format!("Docker error: {}", msg), - ), - // CH-specific variants — Docker should never produce them, but the - // enum is shared so we map them to the closest Docker-side state. - RuntimeError::FirmwareNotFound(msg) => ( - DeploymentStatus::Failed, - "firmware_not_found", - format!("Firmware not found: {}", msg), - ), - RuntimeError::VmStartFailed(msg) => ( - DeploymentStatus::Error, - "vm_start_failed", - format!("VM start failed: {}", msg), - ), - // The CH runtime emits this; the Docker runtime doesn't (the daemon - // rejects port conflicts itself through InstanceCreationFailed). The - // arm exists to keep the match exhaustive over the shared enum. - RuntimeError::PortAlreadyInUse(port) => ( - DeploymentStatus::Error, - "port_allocation_failed", - format!("Port {} is already allocated", port), - ), - RuntimeError::Io(e) => ( - DeploymentStatus::FileSystemError, - "file_system_error", - format!("IO error: {}", e), - ), - RuntimeError::Json(e) => ( - DeploymentStatus::Error, - "runtime_error", - format!("JSON error: {}", e), - ), - }; + let (status, reason, message) = + crate::hypervisor::classifier::create_error_outcome(&err, deployment); error!("[{}] {}: {}", deployment.id, reason, err); deployment.status = status; @@ -329,15 +137,8 @@ async fn handle_job_deployment( mut deployment: Deployment, docker: Docker, resolved_mounts: Vec, - intentional_shutdowns: IntentionalShutdowns, host_auth: HostAuthSettings, ) -> Deployment { - if deployment.status == DeploymentStatus::Deleted { - debug!("{} marked as deleted. Remove all instances", deployment.id); - remove_all_instances(&mut deployment, &docker, "job", &intentional_shutdowns).await; - return deployment; - } - // Terminal states: a one-shot job is done either way. Stop reconciling. // `Failed` is the job equivalent of `CrashLoopBackOff` for workers — set // below when restart_count hits MAX_RESTART_COUNT. @@ -394,205 +195,6 @@ async fn handle_job_deployment( deployment } -async fn handle_worker_deployment( - mut deployment: Deployment, - docker: Docker, - resolved_mounts: Vec, - intentional_shutdowns: IntentionalShutdowns, - host_auth: HostAuthSettings, -) -> Deployment { - if deployment.status == DeploymentStatus::Deleted { - debug!("{} marked as deleted. Remove all instances", deployment.id); - remove_all_instances(&mut deployment, &docker, "worker", &intentional_shutdowns).await; - } else if deployment.restart_count >= MAX_RESTART_COUNT { - deployment.status = DeploymentStatus::CrashLoopBackOff; - return deployment; - } else if deployment.status == DeploymentStatus::CrashLoopBackOff { - return deployment; - } else { - // Count every container that started then exited unexpectedly toward - // restart_count before deciding how many to (re)create. This runs for - // Docker and Podman alike: the reconcile loop owns restart_count for - // worker exits (single writer per tick), so detecting crashes here is - // race-free. Without it the deployment recreates a crashing container - // forever and never reaches CrashLoopBackOff. When the bound is hit we - // converge to CrashLoopBackOff in the same tick. - if detect_and_count_crashes(&mut deployment, &docker, &intentional_shutdowns).await { - return deployment; - } - - let current_count: usize = deployment.instances.len(); - // `target_replicas()`, not `replicas`: when the deployment opted into - // autoscaling this is the autoscaler's current decision, otherwise it - // is the declared count unchanged. - let target_count: usize = match deployment.target_replicas().try_into() { - Ok(count) => count, - Err(_) => { - error!( - "Invalid replicas count for deployment {}: {}", - deployment.id, deployment.replicas - ); - deployment.status = DeploymentStatus::Failed; - return deployment; - } - }; - - debug!( - "Current instances: {}, Target instances: {}", - current_count, target_count - ); - - match current_count.cmp(&target_count) { - Ordering::Less => { - debug!( - "Scaling up: {} -> {} (creating 1 container)", - current_count, target_count - ); - - match create_container(&mut deployment, &docker, &resolved_mounts, &host_auth).await - { - Ok(_) => { - deployment.emit_event( - "info", - format!( - "Scaled up from {} to {} replicas", - current_count, - current_count + 1 - ), - "docker", - Some("scale_up"), - ); - - // Promote to Running only after confirming the container - // is still alive. Docker returns Ok from `start` the - // instant the daemon accepts it, before the entrypoint - // has had a chance to crash — so a container that exits - // immediately (bad command, missing file) was previously - // reported Running for a full tick. Re-inspect: if it - // already exited, leave the status in Creating/Pending so - // the next tick's crash detection counts it toward - // restart_count instead of flapping through Running. - // - // A *single* inspect right after `start` still races: the - // daemon may report `Running` in the few milliseconds - // before a fast `exit 1` propagates, so a doomed container - // can be briefly (and wrongly) promoted. Settle first, then - // inspect — by the time the window elapses an - // instantly-dying container has reliably flipped to - // exited, while a healthy one is unaffected beyond a sub- - // second delay on its *first* promotion. - // - // When a readiness HC is declared, the scheduler's - // readiness gate makes the final Running call anyway; this - // check only closes the gap for deployments with no - // readiness probe, and never overrides the gate (it can - // only withhold Running, not force it). - if deployment.status == DeploymentStatus::Pending - || deployment.status == DeploymentStatus::Creating - { - if let Some(container_id) = deployment.instances.last().cloned() { - tokio::time::sleep(LIVENESS_SETTLE).await; - let status = - check_container_status(docker.clone(), container_id).await; - if confirmed_alive(&status) { - deployment.status = DeploymentStatus::Running; - } else { - // The container exited inside the settle - // window. Drop its id from `instances` so it - // is not treated as a live replica: the - // scheduler's `handle_status_transitions` - // promotes Creating -> Running purely on - // "instances is non-empty", so leaving a dead - // id here would let it override this withheld - // Running. The container itself stays in Docker - // (still labelled), so the next tick's - // `detect_and_count_crashes` finds it via - // `list_instances("exited")`, counts it toward - // restart_count, and reaps it — the crash is - // never lost by this pop. - deployment.instances.pop(); - deployment.emit_event( - "warning", - "Container exited immediately after start; not reporting Running yet".to_string(), - "docker", - Some("liveness_unconfirmed"), - ); - } - } else { - deployment.status = DeploymentStatus::Running; - } - } - } - Err(err) => { - handle_create_error(&mut deployment, err, true); - } - } - } - Ordering::Greater => { - if target_count == 0 { - info!( - "Scaling deployment {} down to 0: removing container ({} remaining)", - deployment.name, - current_count - 1 - ); - } else { - debug!( - "Scaling down: {} -> {} (removing 1 container)", - current_count, target_count - ); - } - - if let Some(container_id) = deployment.instances.first().cloned() { - intentional_shutdowns.mark(container_id.clone()).await; - remove_container(docker.clone(), container_id.clone()).await; - deployment.instances.remove(0); - info!( - "Container {} removed from deployment {}", - container_id, deployment.id - ); - - deployment.emit_event( - "info", - format!( - "Scaled down from {} to {} replicas (removed container {})", - current_count, - current_count - 1, - container_id - ), - "docker", - Some("scale_down"), - ); - } - } - Ordering::Equal => { - debug!("Replicas count matches target: {} instances", current_count); - } - } - } - - debug!("Worker runtime apply {:?}", deployment.id); - deployment -} - -/// How long to let a just-started container settle before the liveness inspect -/// that gates the `Running` promotion. A single inspect issued the instant -/// `start_container` returns races the entrypoint: the daemon can still report -/// `Running` in the few milliseconds before a fast `exit 1` propagates, so a -/// doomed container gets briefly (and wrongly) promoted. 750ms reliably beats -/// that race (an instantly-dying container has flipped to exited by then) -/// without noticeably slowing healthy promotions — it costs at most this once, -/// on a worker's *first* promotion, and only when no readiness HC is declared -/// (the readiness gate owns the Running call otherwise). -const LIVENESS_SETTLE: std::time::Duration = std::time::Duration::from_millis(750); - -/// Whether a just-started container's inspected state confirms it is alive and -/// may be promoted to `Running`. Only `Running` qualifies: `Completed` (exit 0) -/// and `Failed` (non-zero / no state) both mean the entrypoint already exited, -/// so reporting `Running` would be a lie the next reconcile tick has to undo. -fn confirmed_alive(status: &InstanceStatus) -> bool { - matches!(status, InstanceStatus::Running) -} - async fn check_container_status(docker: Docker, container_id: String) -> InstanceStatus { let inspect_options = InspectContainerOptions { size: true }; match docker @@ -618,205 +220,3 @@ async fn check_container_status(docker: Docker, container_id: String) -> Instanc } } } - -#[cfg(test)] -mod tests { - use super::*; - use std::collections::HashMap; - - fn worker_running() -> Deployment { - Deployment { - id: "crash-loop".to_string(), - created_at: chrono::Utc::now().to_string(), - updated_at: None, - status: DeploymentStatus::Running, - restart_count: 0, - namespace: "test".to_string(), - name: "crasher".to_string(), - image: "busybox".to_string(), - config: None, - runtime: "docker".to_string(), - kind: "worker".to_string(), - replicas: 1, - command: vec![], - instances: vec![], - labels: HashMap::new(), - environment: HashMap::new(), - volumes: "[]".to_string(), - health_checks: vec![], - resources: None, - autoscale: None, - desired_replicas: None, - image_digest: None, - ports: vec![], - pending_events: vec![], - parent_id: None, - network: None, - } - } - - /// The bug: on the Docker (`events_driven == true`) path the in-tick crash - /// counter was gated off (`if !events_driven`), so a worker whose container - /// exited every tick was recreated forever and `restart_count` never climbed - /// to `MAX_RESTART_COUNT`. `apply_unexpected_exits` is exactly the per-tick - /// counting `detect_and_count_crashes` now runs for Docker too. Drive it once - /// per reconcile tick with one unexpected exit and assert the deployment - /// reaches `CrashLoopBackOff` within `MAX_RESTART_COUNT` ticks. - /// - /// Before the fix this convergence could not happen on Docker at all (the - /// branch was never entered), which is the regression this guards. - #[test] - fn worker_crash_loop_reaches_crashloopbackoff_within_bound() { - let mut deployment = worker_running(); - - let mut converged_at = None; - for tick in 1..=(MAX_RESTART_COUNT as usize) { - // One container started then exited unexpectedly this tick, with a - // retryable exit code (None) so convergence is driven by the count, - // not the fast-fail path. - let exited = vec![(format!("container-{tick:03}"), None)]; - let hit_bound = apply_unexpected_exits(&mut deployment, &exited); - - assert_eq!( - deployment.restart_count, tick as u32, - "each unexpected exit must bump restart_count exactly once" - ); - - if hit_bound { - converged_at = Some(tick); - break; - } - } - - assert_eq!( - converged_at, - Some(MAX_RESTART_COUNT as usize), - "must converge on the tick that reaches MAX_RESTART_COUNT, not before or never" - ); - assert_eq!(deployment.restart_count, MAX_RESTART_COUNT); - assert_eq!(deployment.status, DeploymentStatus::CrashLoopBackOff); - } - - /// A tick with no unexpected exits (e.g. only operator-initiated stops, which - /// `detect_and_count_crashes` filters out before calling this) must never - /// bump the counter or flip the status — otherwise a scale-down would be - /// mistaken for a crash. - #[test] - fn no_unexpected_exit_never_counts() { - let mut deployment = worker_running(); - let hit_bound = apply_unexpected_exits(&mut deployment, &[]); - assert!(!hit_bound); - assert_eq!(deployment.restart_count, 0); - assert_eq!(deployment.status, DeploymentStatus::Running); - } - - /// Counting must be idempotent across the bound: once `CrashLoopBackOff` is - /// reached the deployment is terminal for the worker path (the early return - /// in `handle_worker_deployment` stops calling this), but a single tick that - /// observes several exits at once must still land exactly on the count and - /// flip, never under-count. - #[test] - fn multiple_exits_in_one_tick_count_each() { - let mut deployment = worker_running(); - let exited: Vec<(String, Option)> = (0..MAX_RESTART_COUNT) - .map(|i| (format!("container-{i}"), None)) - .collect(); - let hit_bound = apply_unexpected_exits(&mut deployment, &exited); - assert!(hit_bound); - assert_eq!(deployment.restart_count, MAX_RESTART_COUNT); - assert_eq!(deployment.status, DeploymentStatus::CrashLoopBackOff); - } - - /// Fast-fail: a single exit with a non-retryable code (127 = command not - /// found) must land on the terminal CreateContainerError immediately, not - /// burn the whole restart budget. restart_count is bumped once (this was a - /// real exit) but convergence is driven by the classifier, not the bound. - #[test] - fn non_retryable_exit_code_fails_fast() { - let mut deployment = worker_running(); - let exited = vec![("container-bad".to_string(), Some(127))]; - let hit_bound = apply_unexpected_exits(&mut deployment, &exited); - assert!(hit_bound, "a non-retryable exit must stop reconciling"); - assert_eq!( - deployment.status, - DeploymentStatus::CreateContainerError, - "127 fails fast onto the terminal create-container status, not CrashLoopBackOff" - ); - assert_eq!( - deployment.restart_count, 1, - "the single real exit is counted, but we don't loop to MAX" - ); - } - - /// A retryable exit code (generic 1) on the first tick must NOT fail fast: - /// it counts toward the budget and keeps the deployment in a non-terminal - /// state so the next tick can retry. - #[test] - fn retryable_exit_code_keeps_retrying() { - let mut deployment = worker_running(); - let exited = vec![("container-1".to_string(), Some(1))]; - let hit_bound = apply_unexpected_exits(&mut deployment, &exited); - assert!(!hit_bound); - assert_eq!(deployment.restart_count, 1); - assert_eq!(deployment.status, DeploymentStatus::Running); - } - - /// A worker is never `Completed`: a clean exit (code 0) — typically a - /// graceful shutdown on SIGTERM when the host reboots — is recreated like - /// any other exit, so the service comes back. - #[test] - fn clean_exit_is_recreated() { - let mut deployment = worker_running(); - let exited = vec![("container-done".to_string(), Some(0))]; - let stop = apply_unexpected_exits(&mut deployment, &exited); - assert!(!stop, "a clean exit must not stop reconciling"); - assert_eq!(deployment.status, DeploymentStatus::Running); - assert_eq!(deployment.restart_count, 1); - } - - /// A one-shot program declared as a worker exits 0 on every start. It is - /// recreated, but its restarts stay bounded: it converges to - /// `CrashLoopBackOff` instead of looping forever. - #[test] - fn repeated_clean_exits_reach_crashloopbackoff() { - let mut deployment = worker_running(); - let mut stopped_at = None; - for tick in 1..=(MAX_RESTART_COUNT as usize) { - let exited = vec![(format!("container-{tick:03}"), Some(0))]; - if apply_unexpected_exits(&mut deployment, &exited) { - stopped_at = Some(tick); - break; - } - } - assert_eq!(stopped_at, Some(MAX_RESTART_COUNT as usize)); - assert_eq!(deployment.status, DeploymentStatus::CrashLoopBackOff); - } - - /// Liveness gate: a container still running right after start may be - /// promoted to Running... - #[test] - fn confirmed_alive_only_for_running() { - assert!(confirmed_alive(&InstanceStatus::Running)); - } - - /// ...but one that already exited (cleanly or not) must NOT be reported - /// Running — that's the immediate-exit window Phase 4 closes. The - /// `LIVENESS_SETTLE` delay before the inspect is what guarantees a - /// fast-exiting container is observed in one of these states (Completed for - /// exit 0, Failed for non-zero / no state) rather than a transient Running. - #[test] - fn not_alive_when_already_exited() { - assert!(!confirmed_alive(&InstanceStatus::Completed)); - assert!(!confirmed_alive(&InstanceStatus::Failed)); - } - - /// The settle window must be long enough to beat the start/exit race yet - /// short enough not to drag healthy first-promotions. Guards against an - /// accidental edit to 0 (no settle → race returns) or a multi-second value - /// that would visibly slow healthy workers. - #[test] - fn liveness_settle_is_within_a_sane_range() { - assert!(LIVENESS_SETTLE >= std::time::Duration::from_millis(500)); - assert!(LIVENESS_SETTLE <= std::time::Duration::from_millis(1000)); - } -} diff --git a/src/scheduler/autoscaler.rs b/src/scheduler/autoscaler.rs index 5792b28c..dbf89ef4 100644 --- a/src/scheduler/autoscaler.rs +++ b/src/scheduler/autoscaler.rs @@ -18,8 +18,8 @@ //! a slow scale-down is what stops a load that oscillates around the target //! from driving the instance count up and down with it. //! -//! State is in-memory and non-persistent, like [`super::healthy_window`] and -//! [`super::backoff`]. At process restart the cooldowns start over: the first +//! State is in-memory and non-persistent, like [`super::healthy_window`]. At +//! process restart the cooldowns start over: the first //! post-restart tick may act immediately, which is safe — the decision is still //! bounded by the policy and by one step. diff --git a/src/scheduler/backoff.rs b/src/scheduler/backoff.rs deleted file mode 100644 index 284f3967..00000000 --- a/src/scheduler/backoff.rs +++ /dev/null @@ -1,95 +0,0 @@ -//! Per-deployment retry backoff for the scheduler. -//! -//! When a runtime fails to apply a deployment (image pull, VM boot, etc.) the -//! scheduler should not retry on the next 1s tick — that burns CPU and clouds -//! logs. Instead, each failure schedules the next attempt with exponential -//! backoff (1, 2, 4, 8, 16, 32, capped at 60s). The state lives in the -//! scheduler so every runtime benefits without duplicating the logic. -//! -//! State is intentionally non-persistent: at process restart all deployments -//! get a fresh attempt, which is fine because the scheduler will requeue them -//! anyway and any in-flight failure will simply re-fail and re-arm. - -use std::collections::HashMap; -use std::time::{Duration, Instant}; - -const MAX_BACKOFF_SECS: u64 = 60; - -#[derive(Default)] -pub(crate) struct RetryBackoff { - next_attempt: HashMap, -} - -impl RetryBackoff { - pub(crate) fn new() -> Self { - Self::default() - } - - /// True if the deployment is still inside its backoff window and should - /// be skipped this cycle. - pub(crate) fn is_blocked(&self, deployment_id: &str) -> bool { - self.next_attempt - .get(deployment_id) - .map(|next| Instant::now() < *next) - .unwrap_or(false) - } - - /// Schedule the next retry. `attempt` is `restart_count` (already - /// incremented by the runtime). attempt=1 → 1s, 2 → 2s, 3 → 4s, …, - /// capped at MAX_BACKOFF_SECS. - pub(crate) fn arm(&mut self, deployment_id: &str, attempt: u32) { - let secs = 1u64 - .checked_shl(attempt.saturating_sub(1)) - .unwrap_or(MAX_BACKOFF_SECS) - .min(MAX_BACKOFF_SECS); - self.next_attempt.insert( - deployment_id.to_string(), - Instant::now() + Duration::from_secs(secs), - ); - } - - /// Drop any pending backoff for a deployment (success, terminal status, - /// or deletion). - pub(crate) fn clear(&mut self, deployment_id: &str) { - self.next_attempt.remove(deployment_id); - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn unknown_deployment_is_not_blocked() { - let b = RetryBackoff::new(); - assert!(!b.is_blocked("nope")); - } - - #[test] - fn arm_blocks_until_window_elapses() { - let mut b = RetryBackoff::new(); - b.arm("d1", 1); - assert!(b.is_blocked("d1")); - } - - #[test] - fn clear_unblocks() { - let mut b = RetryBackoff::new(); - b.arm("d1", 5); - b.clear("d1"); - assert!(!b.is_blocked("d1")); - } - - #[test] - fn backoff_doubles_then_caps() { - // We can't easily test the actual delay without sleeping, but we can - // check the arithmetic via the arm function's effect on stored time. - let mut b = RetryBackoff::new(); - let before = Instant::now(); - b.arm("d1", 10); // 2^9 = 512, capped to 60 - let next = *b.next_attempt.get("d1").unwrap(); - let delta = next.duration_since(before); - assert!(delta <= Duration::from_secs(MAX_BACKOFF_SECS + 1)); - assert!(delta >= Duration::from_secs(MAX_BACKOFF_SECS - 1)); - } -} diff --git a/src/scheduler/healthy_window.rs b/src/scheduler/healthy_window.rs index cf0f5ab3..5cd98c86 100644 --- a/src/scheduler/healthy_window.rs +++ b/src/scheduler/healthy_window.rs @@ -10,8 +10,11 @@ //! anti-flap window (`min_healthy_time`, same window the rollout readiness gate //! uses), the count is reset to 0 — the crash budget refills. //! -//! State is in-memory and non-persistent, like [`super::backoff::RetryBackoff`]: -//! at process restart the clock starts over, which is safe (a still-crashing +//! Only for runtimes still reconciling through `apply`: workers on an instance +//! driver reset their count with the restart policy's persisted `stable_after` +//! instead (see `super::reconcile`). +//! +//! State is in-memory and non-persistent: at process restart the clock starts over, which is safe (a still-crashing //! worker re-crashes and never accrues the window; a healthy one simply takes //! one window longer to have its old count forgiven). diff --git a/src/scheduler/intentional_shutdowns.rs b/src/scheduler/intentional_shutdowns.rs index f58b3e94..5a48e9dc 100644 --- a/src/scheduler/intentional_shutdowns.rs +++ b/src/scheduler/intentional_shutdowns.rs @@ -6,32 +6,32 @@ //! A container that stops shows up two ways: Docker emits a `die` event, and the //! reconcile pass sees it as an `exited` instance. Either can stem from a crash, //! an OOM kill, or a graceful `docker stop` we sent ourselves. The reconcile -//! pass (`detect_and_count_crashes`) counts each unexpected exit toward -//! `restart_count`; once it reaches `MAX_RESTART_COUNT` the deployment flips to -//! `CrashLoopBackOff`. Without this filter, every scale-down, delete, rolling -//! update step or health-check eviction would be counted as a crash and could -//! push a perfectly healthy deployment into `CrashLoopBackOff`. +//! pass (the Docker instance driver's `observe`) reports each unexpected exit, +//! and the scheduler counts it toward `restart_count` and backs off. Without +//! this filter, every scale-down, delete, rolling update step or health-check +//! eviction would be counted as a crash and delay the deployment's next start. //! //! # How it works //! //! Before the runtime stops a container on purpose, it calls //! [`IntentionalShutdowns::mark`] with the container id. Two consumers call -//! [`IntentionalShutdowns::take`]: `detect_and_count_crashes` skips the exited -//! container so it is reaped without bumping `restart_count`, and +//! [`IntentionalShutdowns::take`]: the instance driver's `observe` skips the +//! exited container so it is reaped without being reported, and //! `apply_docker_event` skips the matching `die` event so it is not logged as a //! crash. The first to observe the stop consumes the entry. //! //! # Where to mark //! //! Mark every Ring-initiated stop. Today that's: -//! - scale-down (`runtime/docker/lifecycle.rs`) +//! - scale-down, via the instance driver's `stop_instance` +//! (`runtime/docker/docker_lifecycle.rs`) //! - delete / `remove_all_instances` (`runtime/docker/lifecycle.rs`) //! - rolling update + health-check eviction, both via `remove_instance` //! (`runtime/docker/docker_lifecycle.rs`) //! //! Do NOT mark a container when the *container itself* failed (a real crash, -//! an OOM, an exit). Those must reach `detect_and_count_crashes` so they count -//! toward `restart_count`. +//! an OOM, an exit). Those must reach the instance driver's `observe` so they +//! count toward `restart_count`. //! //! # TTL //! diff --git a/src/scheduler/mod.rs b/src/scheduler/mod.rs index 1fe3654c..1111deaa 100644 --- a/src/scheduler/mod.rs +++ b/src/scheduler/mod.rs @@ -1,10 +1,10 @@ pub(crate) mod autoscaler; -pub(crate) mod backoff; pub(crate) mod docker_events; pub(crate) mod event_worker; pub(crate) mod health_checker; pub(crate) mod healthy_window; pub(crate) mod intentional_shutdowns; +pub(crate) mod reconcile; pub(crate) mod restart; pub(crate) mod scheduler; pub(crate) mod stats_cache; diff --git a/src/scheduler/reconcile.rs b/src/scheduler/reconcile.rs new file mode 100644 index 00000000..2d1cbb97 --- /dev/null +++ b/src/scheduler/reconcile.rs @@ -0,0 +1,733 @@ +//! Reconciliation of workers on runtimes that implement [`InstanceDriver`]. +//! +//! The runtime reports which instances run and which ended; this module decides +//! what to do about it with the restart policy, and asks the runtime to start +//! or stop instances accordingly. It is the same code for every such runtime, +//! so a runtime holds no restart counter, no backoff and no status logic. +//! +//! - An ended instance is a failure, whatever its exit code: a worker is a +//! service, and a service that exits must come back. Each failure is counted +//! and pushes the next start out on the backoff curve. +//! - While backing off, nothing is started. Scaling down still happens. +//! - Once the instances have run for `stable_after` without a failure, the +//! counter resets. +//! - A worker never gives up unless the policy says `on_exhaustion = "fail"`. + +use crate::hypervisor::classifier::{ + Disposition, classify_create_error, classify_exit_code, create_error_outcome, +}; +use crate::hypervisor::instance_driver::InstanceDriver; +use crate::models::deployments::{Deployment, DeploymentStatus}; +use crate::models::restart_state::RestartState; +use crate::models::volume::ResolvedMount; +use crate::scheduler::restart::{self, Decision, Event, RestartPolicy, StartFailure, WorkloadKind}; +use chrono::{DateTime, Utc}; +use rand::Rng; + +/// One reconciliation pass over a worker. Returns the deployment with its new +/// status, instances, `restart_count` and pending events, and updates `state`. +pub(crate) async fn reconcile_worker( + driver: &dyn InstanceDriver, + policy: &RestartPolicy, + mut deployment: Deployment, + resolved_mounts: &[ResolvedMount], + state: &mut RestartState, + now: DateTime, + rng: &mut (impl Rng + Send), +) -> Deployment { + let mut counter = restart::RestartState { + restart_count: deployment.restart_count, + running_since: state.running_since, + }; + + let observation = driver.observe(&deployment).await; + deployment.instances = observation.running; + + let failed_this_pass = !observation.terminated.is_empty(); + for termination in observation.terminated { + let decision = restart::decide( + policy, + WorkloadKind::Worker, + &mut counter, + Event::Exited { + exit_code: termination.exit_code, + }, + now, + rng, + ); + deployment.emit_event( + "error", + format!( + "Instance {} exited with code {} (restart {})", + short_id(&termination.instance_id), + termination + .exit_code + .map_or_else(|| "unknown".to_string(), |c| c.to_string()), + counter.restart_count + ), + &deployment.runtime.clone(), + Some("container_crashed"), + ); + if let Disposition::Terminal(status) = classify_exit_code(termination.exit_code) { + deployment.emit_event( + "error", + format!( + "Instance {} cannot run its program (exit code {:?}); fix the image or command", + short_id(&termination.instance_id), + termination.exit_code + ), + &deployment.runtime.clone(), + Some("non_retryable_exit"), + ); + deployment.status = status; + } + driver.discard_instance(&termination.instance_id).await; + state.last_termination = Some(termination); + apply_decision(&mut deployment, state, decision, counter.restart_count); + } + + if deployment.status == DeploymentStatus::Failed { + deployment.restart_count = counter.restart_count; + state.running_since = None; + return deployment; + } + if failed_this_pass + && deployment.instances.is_empty() + && deployment.status != DeploymentStatus::CreateContainerError + { + deployment.status = DeploymentStatus::CrashLoopBackOff; + } + + scale( + driver, + policy, + &mut deployment, + resolved_mounts, + state, + &mut counter, + now, + rng, + ) + .await; + + if deployment.instances.is_empty() { + counter.running_since = None; + } else if !failed_this_pass { + restart::on_running(&mut counter, now); + if restart::reset_if_stable(policy, &mut counter, now) { + deployment.emit_event( + "info", + format!( + "Restart count reset after {} of uninterrupted running", + humanize(policy.stable_after) + ), + "scheduler", + Some("restart_count_reset"), + ); + } + } + + deployment.restart_count = counter.restart_count; + state.running_since = counter.running_since; + deployment +} + +/// Count instances a liveness check removed as failures, exactly like +/// instances that exited on their own. +pub(crate) fn record_liveness_kills( + policy: &RestartPolicy, + deployment: &mut Deployment, + state: &mut RestartState, + kills: usize, + now: DateTime, + rng: &mut impl Rng, +) { + let mut counter = restart::RestartState { + restart_count: deployment.restart_count, + running_since: state.running_since, + }; + for _ in 0..kills { + let decision = restart::decide( + policy, + WorkloadKind::Worker, + &mut counter, + Event::Exited { exit_code: None }, + now, + rng, + ); + apply_decision(deployment, state, decision, counter.restart_count); + } + deployment.restart_count = counter.restart_count; + state.running_since = counter.running_since; +} + +#[allow(clippy::too_many_arguments)] +async fn scale( + driver: &dyn InstanceDriver, + policy: &RestartPolicy, + deployment: &mut Deployment, + resolved_mounts: &[ResolvedMount], + state: &mut RestartState, + counter: &mut restart::RestartState, + now: DateTime, + rng: &mut (impl Rng + Send), +) { + let current = deployment.instances.len(); + let target = usize::try_from(deployment.target_replicas()).unwrap_or(usize::MAX); + + if current > target { + let Some(instance_id) = deployment.instances.first().cloned() else { + return; + }; + if driver.stop_instance(&instance_id).await { + deployment.instances.remove(0); + deployment.emit_event( + "info", + format!( + "Scaled down from {} to {} replicas (removed instance {})", + current, + current - 1, + short_id(&instance_id) + ), + &deployment.runtime.clone(), + Some("scale_down"), + ); + } + return; + } + + if current == target { + return; + } + + if state.backing_off(now) { + debug!( + "Deployment {} backing off until {:?}", + deployment.id, state.next_attempt_at + ); + return; + } + + match driver.start_instance(deployment, resolved_mounts).await { + Ok(()) => { + state.next_attempt_at = None; + deployment.emit_event( + "info", + format!("Scaled up from {} to {} replicas", current, current + 1), + &deployment.runtime.clone(), + Some("scale_up"), + ); + // The new instance only counts once the next pass has seen it + // running. Counted now, a container that dies on start would be + // promoted to Running for a pass, and a worker restarted from a + // failure status would reach Running without going through the + // readiness gate, which only holds a deployment that was Creating. + deployment.instances.pop(); + if deployment.status != DeploymentStatus::Running { + deployment.status = DeploymentStatus::Creating; + } + } + Err(err) => { + let (status, reason, message) = create_error_outcome(&err, deployment); + error!("[{}] {}: {}", deployment.id, reason, err); + deployment.emit_event("error", message, &deployment.runtime.clone(), Some(reason)); + deployment.status = status; + + let failure = match classify_create_error(&err) { + Disposition::Terminal( + DeploymentStatus::ConfigError + | DeploymentStatus::CreateContainerError + | DeploymentStatus::Failed, + ) => StartFailure::NeedsOperator, + _ => StartFailure::Transient, + }; + let decision = restart::decide( + policy, + WorkloadKind::Worker, + counter, + Event::StartFailed(failure), + now, + rng, + ); + apply_decision(deployment, state, decision, counter.restart_count); + } + } +} + +fn apply_decision( + deployment: &mut Deployment, + state: &mut RestartState, + decision: Decision, + attempts: u32, +) { + match decision { + Decision::StartNow | Decision::Complete => state.next_attempt_at = None, + Decision::WaitUntil(at) => state.next_attempt_at = Some(at), + Decision::Fail => { + state.next_attempt_at = None; + if deployment.status != DeploymentStatus::Failed { + deployment.status = DeploymentStatus::Failed; + deployment.emit_event( + "error", + format!( + "Giving up after {} failed attempts (on_exhaustion = \"fail\")", + attempts + ), + "scheduler", + Some("restart_limit_reached"), + ); + } + } + } +} + +fn short_id(id: &str) -> &str { + &id[..id.len().min(12)] +} + +fn humanize(d: std::time::Duration) -> String { + let secs = d.as_secs(); + if secs.is_multiple_of(60) && secs >= 60 { + format!("{}m", secs / 60) + } else { + format!("{}s", secs) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::hypervisor::error::RuntimeError; + use crate::hypervisor::instance_driver::Observation; + use crate::models::restart_state::Termination; + use crate::scheduler::restart::OnExhaustion; + use async_trait::async_trait; + use rand::SeedableRng; + use rand::rngs::StdRng; + use std::collections::HashMap; + use std::sync::Mutex; + use std::time::Duration; + + /// A runtime whose instances the test drives by hand. + #[derive(Default)] + struct FakeDriver { + running: Mutex>, + terminated: Mutex>, + start_error: Mutex>, + started: Mutex, + discarded: Mutex>, + stopped: Mutex>, + } + + impl FakeDriver { + fn running(ids: &[&str]) -> Self { + let driver = Self::default(); + *driver.running.lock().unwrap() = ids.iter().map(|s| s.to_string()).collect(); + driver + } + + /// The instance `id` exits with `code`. + fn crash(&self, id: &str, code: Option) { + self.running.lock().unwrap().retain(|r| r != id); + self.terminated.lock().unwrap().push(Termination { + instance_id: id.to_string(), + exit_code: code, + finished_at: now(), + logs_tail: Some("panic".to_string()), + }); + } + + fn fail_starts_with(&self, err: RuntimeError) { + *self.start_error.lock().unwrap() = Some(err); + } + + fn starts(&self) -> u32 { + *self.started.lock().unwrap() + } + } + + #[async_trait] + impl InstanceDriver for FakeDriver { + async fn observe(&self, _deployment: &Deployment) -> Observation { + Observation { + running: self.running.lock().unwrap().clone(), + terminated: self.terminated.lock().unwrap().clone(), + } + } + + async fn start_instance( + &self, + deployment: &mut Deployment, + _resolved_mounts: &[ResolvedMount], + ) -> Result<(), RuntimeError> { + if let Some(err) = self.start_error.lock().unwrap().take() { + return Err(err); + } + let mut started = self.started.lock().unwrap(); + *started += 1; + let id = format!("new-{started}"); + self.running.lock().unwrap().push(id.clone()); + deployment.instances.push(id); + Ok(()) + } + + async fn stop_instance(&self, instance_id: &str) -> bool { + self.running.lock().unwrap().retain(|r| r != instance_id); + self.stopped.lock().unwrap().push(instance_id.to_string()); + true + } + + async fn discard_instance(&self, instance_id: &str) { + self.terminated + .lock() + .unwrap() + .retain(|t| t.instance_id != instance_id); + self.discarded.lock().unwrap().push(instance_id.to_string()); + } + } + + fn now() -> DateTime { + DateTime::parse_from_rfc3339("2026-01-01T00:00:00Z") + .unwrap() + .with_timezone(&Utc) + } + + fn worker(replicas: u32) -> Deployment { + Deployment { + id: "w1".to_string(), + created_at: now().to_string(), + updated_at: None, + status: DeploymentStatus::Running, + restart_count: 0, + namespace: "ns".to_string(), + name: "web".to_string(), + image: "nginx".to_string(), + config: None, + runtime: "docker".to_string(), + kind: "worker".to_string(), + replicas, + command: vec![], + instances: vec![], + labels: HashMap::new(), + environment: HashMap::new(), + volumes: "[]".to_string(), + health_checks: vec![], + resources: None, + autoscale: None, + desired_replicas: None, + image_digest: None, + ports: vec![], + pending_events: vec![], + parent_id: None, + network: None, + } + } + + async fn pass( + driver: &FakeDriver, + policy: &RestartPolicy, + deployment: Deployment, + state: &mut RestartState, + at: DateTime, + ) -> Deployment { + let mut rng = StdRng::seed_from_u64(7); + reconcile_worker(driver, policy, deployment, &[], state, at, &mut rng).await + } + + fn reasons(d: &Deployment) -> Vec { + d.pending_events + .iter() + .filter_map(|e| e.reason.clone()) + .collect() + } + + fn secs(s: i64) -> chrono::Duration { + chrono::Duration::seconds(s) + } + + #[tokio::test] + async fn a_crash_is_counted_recorded_and_discarded() { + let driver = FakeDriver::running(&["a"]); + driver.crash("a", Some(1)); + let mut state = RestartState::default(); + let policy = RestartPolicy::default(); + + let d = pass(&driver, &policy, worker(1), &mut state, now()).await; + + assert_eq!(d.restart_count, 1); + assert_eq!(*driver.discarded.lock().unwrap(), ["a"]); + let last = state.last_termination.as_ref().unwrap(); + assert_eq!(last.exit_code, Some(1)); + assert_eq!(last.logs_tail.as_deref(), Some("panic")); + assert!(reasons(&d).contains(&"container_crashed".to_string())); + // First retry within `base`. + if let Some(at) = state.next_attempt_at { + assert!(at <= now() + secs(10)); + } + } + + #[tokio::test] + async fn nothing_starts_while_backing_off() { + let driver = FakeDriver::default(); + let mut state = RestartState { + next_attempt_at: Some(now() + secs(60)), + ..Default::default() + }; + let mut d = worker(1); + d.status = DeploymentStatus::CrashLoopBackOff; + d.restart_count = 3; + + let d = pass(&driver, &RestartPolicy::default(), d, &mut state, now()).await; + + assert_eq!(driver.starts(), 0); + assert_eq!(d.status, DeploymentStatus::CrashLoopBackOff); + assert_eq!(d.restart_count, 3); + } + + #[tokio::test] + async fn the_backoff_elapsed_starts_an_instance() { + let driver = FakeDriver::default(); + let mut state = RestartState { + next_attempt_at: Some(now() + secs(60)), + ..Default::default() + }; + let mut d = worker(1); + d.status = DeploymentStatus::CrashLoopBackOff; + d.restart_count = 3; + + let d = pass( + &driver, + &RestartPolicy::default(), + d, + &mut state, + now() + secs(61), + ) + .await; + + assert_eq!(driver.starts(), 1); + assert_eq!(d.status, DeploymentStatus::Creating); + assert!( + d.instances.is_empty(), + "a new instance counts once it has been seen running" + ); + assert_eq!(state.next_attempt_at, None); + assert_eq!(d.restart_count, 3, "a start is not a reset"); + + // Seen running on the next pass: now it counts. + let d = pass( + &driver, + &RestartPolicy::default(), + d, + &mut state, + now() + secs(62), + ) + .await; + assert_eq!(d.instances, ["new-1"]); + assert_eq!(driver.starts(), 1); + } + + #[tokio::test] + async fn a_crash_looping_worker_is_never_abandoned() { + let driver = FakeDriver::default(); + let policy = RestartPolicy::default(); + let mut state = RestartState::default(); + let mut d = worker(1); + let mut t = now(); + for _ in 0..50 { + // Wait out whatever backoff is pending, then let the new instance crash. + t = state.next_attempt_at.unwrap_or(t).max(t) + secs(1); + d = pass(&driver, &policy, d, &mut state, t).await; + let id = driver.running.lock().unwrap().last().cloned(); + let id = id.expect("an instance was started"); + driver.crash(&id, Some(1)); + t += secs(15); + d = pass(&driver, &policy, d, &mut state, t).await; + assert_ne!(d.status, DeploymentStatus::Failed); + } + assert_eq!(d.restart_count, 50); + assert_eq!(d.status, DeploymentStatus::CrashLoopBackOff); + let wait = state.next_attempt_at.map_or(0, |at| (at - t).num_seconds()); + assert!(wait <= 300, "never waits past the cap, waited {wait}s"); + } + + #[tokio::test] + async fn on_exhaustion_fail_gives_up_after_max_attempts() { + let driver = FakeDriver::default(); + let policy = RestartPolicy { + on_exhaustion: OnExhaustion::Fail, + max_attempts: 2, + ..Default::default() + }; + let mut state = RestartState::default(); + let mut d = worker(1); + for (i, id) in ["a", "b", "c"].iter().enumerate() { + driver.crash(id, Some(1)); + d = pass( + &driver, + &policy, + d, + &mut state, + now() + secs(1000 * i as i64), + ) + .await; + } + assert_eq!(d.status, DeploymentStatus::Failed); + assert!(reasons(&d).contains(&"restart_limit_reached".to_string())); + } + + #[tokio::test] + async fn an_unrunnable_program_lands_on_create_container_error() { + let driver = FakeDriver::running(&["a"]); + driver.crash("a", Some(127)); + let mut state = RestartState::default(); + + let d = pass( + &driver, + &RestartPolicy::default(), + worker(1), + &mut state, + now(), + ) + .await; + + assert_eq!(d.status, DeploymentStatus::CreateContainerError); + assert!(reasons(&d).contains(&"non_retryable_exit".to_string())); + } + + #[tokio::test] + async fn a_start_error_sets_its_status_and_backs_off() { + let driver = FakeDriver::default(); + driver.fail_starts_with(RuntimeError::ConfigNotFound("app".to_string())); + let mut state = RestartState::default(); + let mut d = worker(1); + d.status = DeploymentStatus::Pending; + + let d = pass(&driver, &RestartPolicy::default(), d, &mut state, now()).await; + + assert_eq!(d.status, DeploymentStatus::ConfigError); + assert_eq!(d.restart_count, 1); + assert!(reasons(&d).contains(&"config_error".to_string())); + } + + #[tokio::test] + async fn a_missing_config_is_picked_up_without_a_reapply() { + let driver = FakeDriver::default(); + driver.fail_starts_with(RuntimeError::ConfigNotFound("app".to_string())); + let policy = RestartPolicy::default(); + let mut state = RestartState::default(); + let mut d = worker(1); + d.status = DeploymentStatus::Pending; + + d = pass(&driver, &policy, d, &mut state, now()).await; + // The operator creates the config; the next attempt succeeds. + let later = now() + secs(301); + let d = pass(&driver, &policy, d, &mut state, later).await; + + assert_eq!(driver.starts(), 1); + assert_eq!(d.status, DeploymentStatus::Creating); + } + + #[tokio::test] + async fn the_counter_resets_after_stable_after() { + let driver = FakeDriver::running(&["a"]); + let policy = RestartPolicy::default(); + let mut state = RestartState::default(); + let mut d = worker(1); + d.restart_count = 4; + + d = pass(&driver, &policy, d, &mut state, now()).await; + assert_eq!(state.running_since, Some(now())); + assert_eq!(d.restart_count, 4); + + d = pass(&driver, &policy, d, &mut state, now() + secs(9 * 60)).await; + assert_eq!(d.restart_count, 4, "not stable for long enough yet"); + + let d = pass(&driver, &policy, d, &mut state, now() + secs(10 * 60)).await; + assert_eq!(d.restart_count, 0); + assert!(reasons(&d).contains(&"restart_count_reset".to_string())); + } + + #[tokio::test] + async fn a_crash_restarts_the_stability_clock() { + let driver = FakeDriver::running(&["a", "b"]); + let policy = RestartPolicy::default(); + let mut state = RestartState::default(); + let mut d = worker(2); + d.restart_count = 1; + + d = pass(&driver, &policy, d, &mut state, now()).await; + driver.crash("a", Some(1)); + d = pass(&driver, &policy, d, &mut state, now() + secs(5 * 60)).await; + assert_eq!(state.running_since, None); + assert_eq!(d.status, DeploymentStatus::Running, "b is still serving"); + + d = pass(&driver, &policy, d, &mut state, now() + secs(6 * 60)).await; + let d = pass(&driver, &policy, d, &mut state, now() + secs(14 * 60)).await; + assert_eq!( + d.restart_count, 2, + "10 minutes have not passed since the crash" + ); + } + + #[tokio::test] + async fn extra_instances_are_stopped_one_per_pass() { + let driver = FakeDriver::running(&["a", "b", "c"]); + let mut state = RestartState::default(); + + let d = pass( + &driver, + &RestartPolicy::default(), + worker(1), + &mut state, + now(), + ) + .await; + + assert_eq!(*driver.stopped.lock().unwrap(), ["a"]); + assert_eq!(d.instances, ["b", "c"]); + assert_eq!(d.restart_count, 0, "a scale-down is not a failure"); + assert!(reasons(&d).contains(&"scale_down".to_string())); + } + + #[tokio::test] + async fn scaling_down_is_not_held_by_the_backoff() { + let driver = FakeDriver::running(&["a", "b"]); + let mut state = RestartState { + next_attempt_at: Some(now() + secs(60)), + ..Default::default() + }; + + pass( + &driver, + &RestartPolicy::default(), + worker(1), + &mut state, + now(), + ) + .await; + + assert_eq!(*driver.stopped.lock().unwrap(), ["a"]); + } + + #[test] + fn liveness_kills_count_as_failures() { + let policy = RestartPolicy::default(); + let mut state = RestartState { + running_since: Some(now()), + ..Default::default() + }; + let mut d = worker(1); + let mut rng = StdRng::seed_from_u64(1); + + record_liveness_kills(&policy, &mut d, &mut state, 2, now(), &mut rng); + + assert_eq!(d.restart_count, 2); + assert_eq!(state.running_since, None); + } + + #[test] + fn stable_after_reads_naturally() { + assert_eq!(humanize(Duration::from_secs(600)), "10m"); + assert_eq!(humanize(Duration::from_secs(45)), "45s"); + } +} diff --git a/src/scheduler/restart.rs b/src/scheduler/restart.rs index 8020bf58..0f0fd6d1 100644 --- a/src/scheduler/restart.rs +++ b/src/scheduler/restart.rs @@ -18,9 +18,6 @@ //! //! See `documentation/design/restart-policy.md`. -// Wired into the scheduler by the follow-up that persists the restart state. -#![allow(dead_code)] - use chrono::{DateTime, Utc}; use rand::Rng; use std::time::Duration; @@ -29,12 +26,18 @@ pub(crate) const DEFAULT_BASE: Duration = Duration::from_secs(10); pub(crate) const DEFAULT_CAP: Duration = Duration::from_secs(5 * 60); pub(crate) const DEFAULT_STABLE_AFTER: Duration = Duration::from_secs(10 * 60); pub(crate) const DEFAULT_MAX_ATTEMPTS: u32 = 5; +// Jobs and manifest overrides are wired in once the manifest carries +// `backoff_limit` and the `restart` block. +#[allow(dead_code)] pub(crate) const DEFAULT_BACKOFF_LIMIT: u32 = 6; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum WorkloadKind { Worker, - Job { backoff_limit: u32 }, + #[allow(dead_code)] + Job { + backoff_limit: u32, + }, } /// What a worker does once its failures would exceed `max_attempts`. @@ -70,6 +73,7 @@ impl Default for RestartPolicy { /// The `restart` block of a manifest. Every key is optional and falls back to /// the server policy. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +#[allow(dead_code)] pub(crate) struct RestartOverride { pub(crate) base: Option, pub(crate) cap: Option, @@ -113,6 +117,7 @@ impl RestartPolicy { /// The policy for one deployment: this one, with the manifest's overrides /// applied key by key. The result is validated as a whole, so an override /// of `cap` alone is checked against the server's `base`. + #[allow(dead_code)] pub(crate) fn with_override(&self, o: &RestartOverride) -> Result { let policy = Self { base: o.base.unwrap_or(self.base), @@ -166,6 +171,17 @@ pub(crate) enum Event { StartFailed(StartFailure), } +impl Event { + /// An exit code that says the program can never run (126 not executable, + /// 127 not found) needs an operator just like a rejected spec does. + fn needs_operator(&self) -> bool { + match self { + Event::StartFailed(failure) => *failure == StartFailure::NeedsOperator, + Event::Exited { exit_code } => matches!(exit_code, Some(126) | Some(127)), + } + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum Decision { StartNow, @@ -235,18 +251,38 @@ pub(crate) fn decide( return Decision::Fail; } - let ceiling = match event { - Event::StartFailed(StartFailure::NeedsOperator) => policy.cap, - _ => policy.delay_ceiling(state.restart_count), + match retry_at( + policy, + state.restart_count, + event.needs_operator(), + now, + rng, + ) { + Some(at) => Decision::WaitUntil(at), + None => Decision::StartNow, + } +} + +/// When attempt `attempt` (1-based) may run, or `None` to run it now. Failures +/// that need an operator draw from the whole `[0, cap]` range from the first +/// attempt on. +pub(crate) fn retry_at( + policy: &RestartPolicy, + attempt: u32, + needs_operator: bool, + now: DateTime, + rng: &mut impl Rng, +) -> Option> { + let ceiling = if needs_operator { + policy.cap + } else { + policy.delay_ceiling(attempt) }; let delay = jitter(ceiling, rng); if delay.is_zero() { - return Decision::StartNow; - } - match chrono::Duration::from_std(delay) { - Ok(delay) => Decision::WaitUntil(now + delay), - Err(_) => Decision::StartNow, + return None; } + chrono::Duration::from_std(delay).ok().map(|d| now + d) } /// Full jitter: uniform in `[0, ceiling]`, at millisecond resolution. @@ -523,6 +559,28 @@ mod tests { assert!(delays.iter().any(|d| *d > p.base)); } + #[test] + fn unrunnable_program_exits_start_at_the_cap() { + let p = RestartPolicy::default(); + let mut rng = rng(); + let delays: Vec = (0..50) + .map(|_| { + let mut state = RestartState::default(); + delay_of(decide( + &p, + WorkloadKind::Worker, + &mut state, + Event::Exited { + exit_code: Some(127), + }, + now(), + &mut rng, + )) + }) + .collect(); + assert!(delays.iter().any(|d| *d > p.base)); + } + #[test] fn counter_resets_after_stable_after_of_running() { let p = RestartPolicy::default(); diff --git a/src/scheduler/scheduler.rs b/src/scheduler/scheduler.rs index 49137fe6..99c68d5e 100644 --- a/src/scheduler/scheduler.rs +++ b/src/scheduler/scheduler.rs @@ -6,14 +6,17 @@ use crate::models::deployment_event; use crate::models::deployments::{self, Deployment, DeploymentStatus, EnvValue}; use crate::models::health_check::{HealthCheck, HealthCheckStatus}; use crate::models::health_check_logs; +use crate::models::restart_state; use crate::models::secret as SecretModel; use crate::models::volume::ResolvedMount; use crate::scheduler::autoscaler::{Autoscaler, Decision}; -use crate::scheduler::backoff::RetryBackoff; use crate::scheduler::docker_events::DockerEvent; use crate::scheduler::health_checker::HealthChecker; use crate::scheduler::healthy_window::HealthyWindow; use crate::scheduler::intentional_shutdowns::IntentionalShutdowns; +use crate::scheduler::reconcile; +use crate::scheduler::restart; +use rand::SeedableRng as _; use sqlx::SqlitePool; use std::collections::HashMap; use std::env; @@ -273,27 +276,104 @@ async fn apply_runtime( match tokio::time::timeout(apply_timeout, runtime.apply(resolved, resolved_mounts)).await { Ok(result) => Some(result), Err(_) => { - error!("runtime apply timed out for deployment {}", deployment.id); - if let Err(e) = deployment_event::log_event( - pool, - deployment.id.clone(), - "error", - format!( - "Scheduler apply timed out after {} seconds", - apply_timeout_secs - ), - "scheduler", - Some("apply_timeout"), - ) - .await - { - warn!("Failed to log apply timeout event: {}", e); - } + log_apply_timeout(pool, &deployment.id, apply_timeout_secs).await; + None + } + } +} + +async fn log_apply_timeout(pool: &SqlitePool, deployment_id: &str, apply_timeout_secs: u64) { + error!("runtime apply timed out for deployment {}", deployment_id); + if let Err(e) = deployment_event::log_event( + pool, + deployment_id.to_string(), + "error", + format!( + "Scheduler apply timed out after {} seconds", + apply_timeout_secs + ), + "scheduler", + Some("apply_timeout"), + ) + .await + { + warn!("Failed to log apply timeout event: {}", e); + } +} + +/// Reconcile a worker on a runtime that reports its instances, applying the +/// restart policy here rather than in the runtime. Bounded like +/// [`apply_runtime`]. +#[tracing::instrument( + name = "scheduler.reconcile_worker", + skip_all, + fields( + otel.kind = "internal", + deployment.id = %resolved.id, + deployment.namespace = %resolved.namespace, + deployment.name = %resolved.name, + deployment.status = %resolved.status, + ) +)] +#[allow(clippy::too_many_arguments)] +async fn reconcile_driven( + pool: &SqlitePool, + driver: &dyn crate::hypervisor::instance_driver::InstanceDriver, + policy: &restart::RestartPolicy, + resolved: Deployment, + resolved_mounts: Vec, + state: &mut restart_state::RestartState, + apply_timeout: Duration, + apply_timeout_secs: u64, + rng: &mut rand::rngs::StdRng, +) -> Option { + let id = resolved.id.clone(); + let pass = reconcile::reconcile_worker( + driver, + policy, + resolved, + &resolved_mounts, + state, + chrono::Utc::now(), + rng, + ); + match tokio::time::timeout(apply_timeout, pass).await { + Ok(result) => Some(result), + Err(_) => { + log_apply_timeout(pool, &id, apply_timeout_secs).await; None } } } +/// Persist the change a cycle made to `restart_count`, through targeted +/// statements rather than the blind full-row write back: two paths touching +/// the counter must never clobber each other. Returns the persisted value. +async fn persist_restart_count(pool: &SqlitePool, id: &str, before: u32, after: u32) -> u32 { + let result = if after < before { + // Reset, possibly followed by new failures in the same cycle. + match deployments::reset_restart_count(pool, id).await { + Ok(()) if after > 0 => deployments::increment_restart_count(pool, id, after).await, + Ok(()) => Ok(0), + Err(e) => Err(e), + } + } else if after > before { + deployments::increment_restart_count(pool, id, after - before).await + } else { + return after; + }; + match result { + Ok(persisted) => persisted, + Err(e) => { + error!( + "Failed to persist restart_count for deployment {}: {}", + id, e + ); + after + } + } +} + async fn persist_pending_events(pool: &SqlitePool, deployment: &mut Deployment) { for event in &deployment.pending_events { if let Err(e) = deployment_event::create_event(pool, event).await { @@ -433,13 +513,14 @@ async fn publish_status_change( } } +/// Returns how many instances a failed liveness check removed. async fn run_health_checks( pool: &SqlitePool, deployment: &mut Deployment, old_status: &DeploymentStatus, health_checker: &HealthChecker, runtime: &dyn RuntimeLifecycle, -) { +) -> usize { // Health checks run in `Running` (full set, drives `on_failure`) and, for // readiness gating, in `Creating` (readiness-only, record-only — see // `HealthChecker::execute_checks`). `execute_checks` enforces the @@ -452,7 +533,7 @@ async fn run_health_checks( _ => false, }; if !runnable || deployment.health_checks.is_empty() { - return; + return 0; } debug!("Executing health checks for deployment {}", deployment.id); @@ -498,6 +579,7 @@ async fn run_health_checks( runtime.remove_instance(instance_id.clone()).await; deployment.instances.retain(|id| id != instance_id); } + outcome.instances_to_remove.len() } /// Pick the instance `restart_interval` should replace this cycle: the oldest, @@ -1256,12 +1338,9 @@ async fn handle_rolling_update( /// the database. Non-blocking: returns as soon as the channel is empty so the /// scheduler can proceed to its reconciliation pass. /// -/// On every event that signals an instance has died (die / oom / kill), bump -/// `restart_count` for the deployment and log a deployment_event so the user -/// can see the crash trace. Once `restart_count` reaches `MAX_RESTART_COUNT`, -/// the existing logic in `lifecycle::handle_worker_deployment` flips the -/// status to `CrashLoopBackOff` and stops respawning — that's what bounds the -/// loop and prevents disk saturation. +/// Every event that signals an instance has died (die / oom / kill) is logged +/// as a deployment_event so the user can see the crash trace. Counting the +/// crash is the reconcile pass's job, not this one's. async fn drain_docker_events( pool: &SqlitePool, event_rx: &mut mpsc::Receiver, @@ -1324,8 +1403,8 @@ async fn apply_docker_event( // the deployment at the start of a tick and writes the full row back // at the end, so a bump issued here would be lost to that write (the // race that let crash loops recreate forever and never reach - // CrashLoopBackOff). `detect_and_count_crashes` now counts the same - // exited container in-tick for both Docker and Podman. This branch is + // CrashLoopBackOff). The reconcile pass counts the same exited + // container when it observes it, for Docker and Podman. This branch is // therefore log-only — it records the crash cause for traceability // without mutating the counter. if let Err(e) = deployment_event::log_event( @@ -1596,14 +1675,13 @@ pub(crate) async fn schedule( // no Docker event listener) is logged once, not every tick. let mut event_disconnect_logged = false; - // Per-deployment retry backoff. Shared across runtimes so any new runtime - // (Docker, Cloud Hypervisor, future Firecracker, ...) automatically gets - // exponential backoff on transient failures without duplicating the logic. - let mut backoff = RetryBackoff::new(); + // Validated at startup (`commands::server`), so the fallback is never used. + let policy = config.server.restart.policy().unwrap_or_default(); + let mut rng = rand::rngs::StdRng::from_os_rng(); - // Per-deployment "continuously healthy since" clock. Resets a worker's - // restart_count once it has run uninterrupted for the anti-flap window, so - // the crash budget is "N crashes within a window", not "N crashes for life". + // Runtimes still reconciling through `apply` refill a worker's crash budget + // after it runs for the anti-flap window. Workers on an instance driver use + // the restart policy's `stable_after` instead. let mut healthy_window = HealthyWindow::new(); // Per-deployment autoscaling cooldowns. Only ever consulted for deployments @@ -1669,6 +1747,15 @@ pub(crate) async fn schedule( debug!("Processing {} deployments", list_deployments.len()); + let mut restart_states = match restart_state::find_all(&pool).await { + Ok(states) => states, + Err(e) => { + error!("Failed to fetch restart states: {}", e); + sleep(duration).await; + continue; + } + }; + // Decide before reconciling, so a fresh decision takes effect on this // tick rather than waiting for the next one. Deployments without an // `autoscale` block are untouched. @@ -1730,7 +1817,6 @@ pub(crate) async fn schedule( // without reading env or mounts, so reconcile with the unresolved // deployment and go straight to cleanup. if deployment.status == DeploymentStatus::Deleted { - backoff.clear(&deployment.id); healthy_window.clear(&deployment.id); let mut result = match apply_runtime( &pool, @@ -1755,9 +1841,21 @@ pub(crate) async fn schedule( continue; } - // Honour the retry backoff. (Deletes are handled above and never - // reach this point, so they're never blocked by backoff.) - if backoff.is_blocked(&deployment.id) { + // Workers on an instance driver are reconciled by the scheduler, + // which applies the restart policy itself. Everything else still + // goes through the runtime's `apply`. + let driver = runtime + .instance_driver() + .filter(|_| deployment.kind == "worker"); + let mut state = restart_states.remove(&deployment.id).unwrap_or_default(); + let state_before = state.clone(); + let now = chrono::Utc::now(); + + // Honour the retry backoff. A driven worker is still reconciled + // while backing off (an extra instance is stopped, a crash is + // counted); only starting a new instance waits. (Deletes are + // handled above and never reach this point.) + if driver.is_none() && state.backing_off(now) { // Emit this at info as a structured event, not a bare debug line: // a deployment being skipped by backoff is the single most useful // signal when diagnosing "why is my rollout slow to converge?", @@ -1875,52 +1973,55 @@ pub(crate) async fn schedule( } let restart_count_before = deployment.restart_count; - let mut result = match apply_runtime( - &pool, - &deployment, - resolved, - resolved_mounts, - apply_timeout, - apply_timeout_secs, - runtime.as_ref(), - ) - .await - { + let applied = match driver { + Some(driver) => { + reconcile_driven( + &pool, + driver, + &policy, + resolved, + resolved_mounts, + &mut state, + apply_timeout, + apply_timeout_secs, + &mut rng, + ) + .await + } + None => { + apply_runtime( + &pool, + &deployment, + resolved, + resolved_mounts, + apply_timeout, + apply_timeout_secs, + runtime.as_ref(), + ) + .await + } + }; + let mut result = match applied { Some(d) => d, None => continue, }; - // Persist any restart_count bump the runtime made through a targeted - // atomic `+= delta` rather than the blind full-row `update` below. - // The blind write reads the row at tick start and rewrites it whole, - // so two paths touching the counter would clobber each other - // (last-writer-wins). The atomic statement folds the delta into the - // DB's current value, so no increment is ever lost. The in-memory - // count is then realigned to the persisted truth for the backoff and - // healthy-window decisions below. - if result.restart_count > restart_count_before { - let delta = result.restart_count - restart_count_before; - match deployments::increment_restart_count(&pool, &result.id, delta).await { - Ok(persisted) => result.restart_count = persisted, - Err(e) => error!( - "Failed to increment restart_count for deployment {}: {}", - result.id, e - ), - } - } - - // Translate the runtime's outcome into a backoff decision. - // A bumped restart_count means the runtime hit a transient - // failure — arm the next retry. Otherwise (success, terminal - // status, or deleted) drop any pending backoff so the next - // legitimate failure starts at 1s again. - if result.restart_count > restart_count_before - && result.status != DeploymentStatus::CrashLoopBackOff - && result.status != DeploymentStatus::Failed - { - backoff.arm(&result.id, result.restart_count); - } else { - backoff.clear(&result.id); + // A runtime that bumped restart_count hit a failure it will retry: + // schedule the retry on the policy's backoff curve. Otherwise + // (success, or a status the runtime gave up on) drop any pending + // backoff. + // Nor is there any wait once the budget is spent: the next apply + // only lands the deployment on its terminal status. + if driver.is_none() { + state.next_attempt_at = if result.restart_count > restart_count_before + && result.restart_count < deployments::MAX_RESTART_COUNT + && result.status != DeploymentStatus::CrashLoopBackOff + && result.status != DeploymentStatus::Failed + { + restart::retry_at(&policy, result.restart_count, false, now, &mut rng) + } else { + None + }; } persist_pending_events(&pool, &mut result).await; @@ -1944,7 +2045,7 @@ pub(crate) async fn schedule( // `publish_status_change` uses it to detect a real status change. let old_status = deployment.status.clone(); handle_status_transitions(&pool, &mut result, &mut deleted).await; - run_health_checks( + let liveness_kills = run_health_checks( &pool, &mut result, &old_status, @@ -1952,6 +2053,17 @@ pub(crate) async fn schedule( runtime.as_ref(), ) .await; + // An instance a liveness check removed failed, like one that exited. + if driver.is_some() && liveness_kills > 0 { + reconcile::record_liveness_kills( + &policy, + &mut result, + &mut state, + liveness_kills, + now, + &mut rng, + ); + } gate_running_on_readiness(&pool, &old_status, &mut result).await; // Taken before the rolling step, which clears `parent_id` on the // cycle a rollout completes: the deployment it just finished @@ -1981,7 +2093,8 @@ pub(crate) async fn schedule( // the count and converge (the window restarts on every fresh crash), // so this never masks a real crash loop. Scoped to workers — jobs // converge to Failed, not a windowed budget. - if result.kind == "worker" + if driver.is_none() + && result.kind == "worker" && healthy_window.observe( &result.id, has_live_container(&result.status, result.instances.is_empty()), @@ -1993,23 +2106,31 @@ pub(crate) async fn schedule( "Deployment {} healthy for the anti-flap window; resetting restart_count from {} to 0", result.id, result.restart_count ); - // Targeted reset, same reasoning as the atomic increment above: - // never let a stale full-row write resurrect the old count. - if let Err(e) = deployments::reset_restart_count(&pool, &result.id).await { - error!( - "Failed to reset restart_count for deployment {}: {}", - result.id, e - ); - } else { - result.restart_count = 0; - result.emit_event( - "info", - "restart_count reset to 0 after staying healthy".to_string(), - "scheduler", - Some("restart_count_reset"), - ); - persist_pending_events(&pool, &mut result).await; - } + // Persisted below with the cycle's other count changes. + result.restart_count = 0; + result.emit_event( + "info", + "restart_count reset to 0 after staying healthy".to_string(), + "scheduler", + Some("restart_count_reset"), + ); + } + + persist_pending_events(&pool, &mut result).await; + result.restart_count = persist_restart_count( + &pool, + &result.id, + restart_count_before, + result.restart_count, + ) + .await; + if state != state_before + && let Err(e) = restart_state::save(&pool, &result.id, &state).await + { + error!( + "Failed to save restart state for deployment {}: {}", + result.id, e + ); } write_back(&pool, &old_status, &result).await; diff --git a/tests/e2e/docker/t23_create_container_error_loop.sh b/tests/e2e/docker/t23_create_container_error_loop.sh index a028c0eb..ded412d2 100755 --- a/tests/e2e/docker/t23_create_container_error_loop.sh +++ b/tests/e2e/docker/t23_create_container_error_loop.sh @@ -1,14 +1,9 @@ #!/usr/bin/env bash # T23: a deployment whose container is accepted by Docker `create` but rejected # by `start` (OCI runtime cannot exec the binary, e.g. command points at a -# missing file) must converge to CrashLoopBackOff rather than spamming "Scaled -# up from 0 to 1 replicas" events forever. -# -# Today this test FAILS: the scheduler's `Scaled up` event is emitted on every -# reconciliation tick because Docker says `create` succeeded; `restart_count` -# never climbs to MAX_RESTART_COUNT for this failure mode; the events timeline -# fills with hundreds of identical entries. Captured live in the dashboard with -# ~49 duplicate events over a couple of minutes. +# missing file). The spec is rejected, which needs an operator: the worker lands +# on `create_container_error` and retries wait up to the backoff cap, instead of +# a "Scaled up from 0 to 1 replicas" event and a failed start on every tick. set -euo pipefail @@ -16,15 +11,14 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # shellcheck source=../lib.sh source "$SCRIPT_DIR/../lib.sh" -log "== T23: CreateContainerError must converge to CrashLoopBackOff ==" +log "== T23: a rejected start lands on create_container_error and backs off ==" start_ring ring_login "$RING_BIN" apply --file "$SCRIPT_DIR/../fixtures/oci-create-error.yaml" -# 60s is well past MAX_RESTART_COUNT (5) at a 1s scheduler interval, plus a -# safety margin for the Docker event listener to bump restart_count. +# 60s is 60 scheduler ticks: without a backoff, as many failed starts. log "waiting 60s for the scheduler to react to repeated start failures..." sleep 60 @@ -40,10 +34,8 @@ STATUS=$("$RING_BIN" deployment list --output json \ '.[] | select(.namespace==$ns and .name==$n) | .status' \ | head -n1) -# Count "Scaled up" events. The current bug emits one per reconciliation -# tick, so a 60-second window produces dozens. After the fix the scheduler -# should either stop emitting them or de-duplicate, plus reach -# CrashLoopBackOff and stop reconciling entirely. +# Count "Scaled up" events: a failed start emits none, and the backoff keeps +# the attempts rare. TOKEN=$(jq -r '.default.token' "$RING_TEST_DIR/auth.json") EVENTS_JSON=$(curl -fsS "$RING_URL/deployments/$DEPLOYMENT_ID/events" \ -H "Authorization: Bearer $TOKEN") @@ -52,25 +44,20 @@ SCALED_UP_COUNT=$(echo "$EVENTS_JSON" \ log "observed: restart_count=$RESTART_COUNT status=$STATUS scaled_up_events=$SCALED_UP_COUNT" -# 1) The deployment must converge to CrashLoopBackOff. Anything else means -# the scheduler keeps trying without a bound. -if [ "$STATUS" != "crash_loop_back_off" ]; then - fail "expected status crash_loop_back_off, got '$STATUS'" +# 1) The rejected spec is reported as such. +if [ "$STATUS" != "create_container_error" ]; then + fail "expected status create_container_error, got '$STATUS'" fi -# 2) restart_count must have reached at least MAX_RESTART_COUNT (5). If it -# doesn't, the failure mode isn't counted — that's the root cause of the -# event spam. -if [ "${RESTART_COUNT:-0}" -lt 5 ]; then - fail "expected restart_count >= 5, got $RESTART_COUNT — Docker start failures are not counted" +# 2) Each failed start is counted, but retries wait up to the 5-minute cap: +# a couple of attempts in 60s, not one per tick. +if [ "${RESTART_COUNT:-0}" -lt 1 ] || [ "$RESTART_COUNT" -gt 3 ]; then + fail "expected 1 to 3 attempts in 60s, got restart_count=$RESTART_COUNT" fi -# 3) "Scaled up" events must be bounded. Even a fix that only sets -# CrashLoopBackOff still leaves up to MAX_RESTART_COUNT scale-up attempts -# in the log. Anything beyond ~10 means the scheduler kept reconciling -# past the CrashLoopBackOff threshold. -if [ "$SCALED_UP_COUNT" -gt 10 ]; then - fail "too many 'Scaled up' events ($SCALED_UP_COUNT) — scheduler is re-emitting them past CrashLoopBackOff" +# 3) No "Scaled up" event for a start that failed. +if [ "$SCALED_UP_COUNT" -gt 0 ]; then + fail "$SCALED_UP_COUNT 'Scaled up' events for starts that all failed" fi # 4) Orphan containers must be cleaned up. Each failed `start_container` diff --git a/tests/e2e/docker/t31_insufficient_memory.sh b/tests/e2e/docker/t31_insufficient_memory.sh index 1a30a537..021f545a 100755 --- a/tests/e2e/docker/t31_insufficient_memory.sh +++ b/tests/e2e/docker/t31_insufficient_memory.sh @@ -5,8 +5,8 @@ # than the host has would start anyway and get OOM-killed at runtime (or, with # no limit, take the host down). Now `create_container` checks the requested # memory against the host's available memory *before* pulling/creating, and -# fails fast with a terminal `insufficient_resources` status and an actionable -# event. +# refuses with an `insufficient_resources` status and an actionable event. The +# refusal is retried on the backoff curve, since memory gets freed. # # Deterministic: 999Ti is more memory than any real host, so the check fails # every run regardless of the machine. No OOM is actually triggered. @@ -63,13 +63,14 @@ if docker ps -a --format '{{.Names}}' | grep -q "ring-e2e_oversized_"; then fi log "no container was created — admission control ran before create" -# Terminal: the status must not flap to crash_loop_back_off on later ticks. +# Retried, but still refused: the status stays insufficient_resources rather +# than flapping to another one. sleep 3 STATUS=$("$RING_BIN" deployment list --output json 2>/dev/null \ | jq -r --arg id "$DEP_ID" '.[] | select(.id == $id) | .status') [ "$STATUS" = "insufficient_resources" ] || \ - fail "status drifted from insufficient_resources to '$STATUS' (should be terminal)" -log "status stayed terminal at insufficient_resources" + fail "status drifted from insufficient_resources to '$STATUS'" +log "status stayed at insufficient_resources" # Cleanup "$RING_BIN" deployment delete "$DEP_ID" diff --git a/tests/e2e/docker/t39_git_auth_crashloop_converges.sh b/tests/e2e/docker/t39_git_auth_crashloop_converges.sh index f213a6e1..91c2aab3 100755 --- a/tests/e2e/docker/t39_git_auth_crashloop_converges.sh +++ b/tests/e2e/docker/t39_git_auth_crashloop_converges.sh @@ -1,14 +1,11 @@ #!/usr/bin/env bash -# T39: THE production case. A container whose entrypoint fails like a git clone -# auth failure (writes "fatal: Authentication failed" and exits 128) must -# converge to CrashLoopBackOff and STOP being recreated — not loop forever. +# T39: a container whose entrypoint fails like a git clone auth failure (writes +# "fatal: Authentication failed" and exits 128) keeps being retried, but slowly: +# the default restart policy backs off from 10s up to 5 minutes between +# attempts, instead of recreating it on every scheduler tick. # -# This is the exact shape of the bug that bit us in prod: exit 128 is the real -# git auth exit code, and per the classifier 128 is RETRYABLE, so this can only -# converge via the restart-count path (it is NOT short-circuited by the -# fast-fail terminal classification). If the in-tick crash counter regresses, -# restart_count stays low, the scheduler recreates the container every tick, and -# containers pile up without bound. This test would have caught that. +# With a 1s scheduler interval, a loop without backoff would create a container +# every tick (~90 over this window). The default backoff allows only a handful. set -euo pipefail @@ -16,17 +13,14 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # shellcheck source=../lib.sh source "$SCRIPT_DIR/../lib.sh" -log "== T39: git-auth-style crash loop must converge to CrashLoopBackOff ==" +log "== T39: git-auth-style crash loop is retried on the backoff curve ==" start_ring ring_login "$RING_BIN" apply --file "$SCRIPT_DIR/../fixtures/git-auth-crashloop.yaml" -# Give the scheduler time to react. With a 1s scheduler interval, 90s leaves -# comfortable headroom for restart_count to climb past MAX_RESTART_COUNT (5) -# through the restart-count path (128 is retryable, not fast-failed). -log "waiting 90s for the scheduler to react to repeated exit-128 crashes..." +log "waiting 90s for repeated exit-128 crashes..." sleep 90 DEPLOYMENT_ID=$(get_deployment_id "ring-e2e" "git-auth-crashloop") @@ -35,31 +29,31 @@ if [ -z "$DEPLOYMENT_ID" ]; then fi log "deployment id: $DEPLOYMENT_ID" -# Count every container ever created for this deployment (running + exited). TOTAL_CONTAINERS=$(docker ps -aq --filter "label=ring_deployment=$DEPLOYMENT_ID" | wc -l | tr -d ' ') RESTART_COUNT=$(get_restart_count "ring-e2e" "git-auth-crashloop") -STATUS=$("$RING_BIN" deployment list --output json \ - | jq -r --arg ns "ring-e2e" --arg n "git-auth-crashloop" \ - '.[] | select(.namespace==$ns and .name==$n) | .status' \ - | head -n1) +TOKEN=$(jq -r '.default.token' "$RING_TEST_DIR/auth.json") +CRASH_EVENTS=$(curl -fsS "$RING_URL/deployments/$DEPLOYMENT_ID/events" \ + -H "Authorization: Bearer $TOKEN" \ + | jq -r '[.[] | select(.reason == "container_crashed" and (.message | test("code 128")))] | length') -log "observed: total_containers=$TOTAL_CONTAINERS restart_count=$RESTART_COUNT status=$STATUS" +log "observed: total_containers=$TOTAL_CONTAINERS restart_count=$RESTART_COUNT crash_events=$CRASH_EVENTS" -# 1) restart_count must reach MAX_RESTART_COUNT (5) — it is counted via the -# retryable restart-count path, exactly like prod. -if [ "${RESTART_COUNT:-0}" -lt 5 ]; then - fail "expected restart_count >= 5, got $RESTART_COUNT (exit-128 crashes are not being counted)" +# 1) The crashes are counted, with their exit code. +if [ "${RESTART_COUNT:-0}" -lt 2 ]; then + fail "expected restart_count >= 2, got $RESTART_COUNT (exit-128 crashes are not being counted)" +fi +if [ "${CRASH_EVENTS:-0}" -lt 2 ]; then + fail "expected container_crashed events naming exit code 128, got $CRASH_EVENTS" fi -# 2) The deployment must converge to CrashLoopBackOff and STOP recreating. -if [ "$STATUS" != "crash_loop_back_off" ]; then - fail "expected status crash_loop_back_off, got '$STATUS'" +# 2) The backoff keeps the retries rare: delays of up to 10s, 20s, 40s, 80s. +if [ "$RESTART_COUNT" -gt 15 ]; then + fail "restart_count reached $RESTART_COUNT in 90s — retries are not backing off" fi -# 3) Total containers ever spawned must stay bounded. Without the cap this grows -# linearly with time (one new container per tick = ~90 over this window). -if [ "$TOTAL_CONTAINERS" -gt 10 ]; then - fail "too many containers spawned ($TOTAL_CONTAINERS) — restart loop is not bounded (prod bug)" +# 3) Dead containers are removed once recorded: they never pile up. +if [ "$TOTAL_CONTAINERS" -gt 1 ]; then + fail "$TOTAL_CONTAINERS containers left for the deployment — crashed containers are not removed" fi log "== T39: PASS ==" diff --git a/tests/e2e/docker/t40_windowed_restart_count_reset.sh b/tests/e2e/docker/t40_windowed_restart_count_reset.sh index 551b95e0..c8e12e24 100755 --- a/tests/e2e/docker/t40_windowed_restart_count_reset.sh +++ b/tests/e2e/docker/t40_windowed_restart_count_reset.sh @@ -1,8 +1,8 @@ #!/usr/bin/env bash -# T40: Phase 1 windowed restart_count reset. A worker that crashes a few times -# (N < MAX_RESTART_COUNT) and then runs healthy past the anti-flap window must -# have its restart_count forgiven (reset to 0), so an isolated later crash -# doesn't trip CrashLoopBackOff. +# T40: windowed restart_count reset. A worker that crashes a few times and then +# runs without a failure for the restart policy's `stable_after` must have its +# restart_count reset to 0, so a later crash starts back at the shortest delay. +# A short policy (retries at most 2s apart, reset after 15s) keeps it fast. # # Docker re-runs the SAME command on every (re)spawn, so the "crash then heal" # behaviour is driven by state on a host bind mount: each start increments a @@ -22,6 +22,11 @@ source "$SCRIPT_DIR/../lib.sh" log "== T40: restart_count is forgiven after a healthy window ==" +export RING_EXTRA_CONFIG='[server.restart] +base = "1s" +cap = "2s" +stable_after = "15s"' + # Host-side state directory the container increments a counter in. Start clean # so the counter begins at 0 even across re-runs of this test. STATE_DIR="/tmp/ring-e2e-t40" @@ -34,9 +39,7 @@ ring_login "$RING_BIN" apply --file "$SCRIPT_DIR/../fixtures/crash-then-heal.yaml" # Phase 1: the container crashes 3 times. Wait for restart_count to reach 3, -# which proves the crash phase registered. It must stop at 3 (the 4th start -# stays up), well short of MAX_RESTART_COUNT (5) — otherwise we couldn't test a -# *windowed* reset, only a CrashLoopBackOff. +# which proves the crash phase registered. It stops at 3: the 4th start stays up. log "waiting for the crash phase to accrue restart_count=3..." REACHED=0 for _ in $(seq 1 60); do @@ -52,14 +55,11 @@ if [ "$REACHED" -ne 1 ]; then fi log "crash phase registered: restart_count=$RC" -# It must not have blown past the CrashLoopBackOff bound; if it did, the windowed -# reset can't be exercised. -if [ "${RC:-0}" -ge 5 ]; then - fail "restart_count reached the CrashLoopBackOff bound ($RC); cannot test the windowed reset" +if [ "${RC:-0}" -gt 3 ]; then + fail "restart_count reached $RC; the fixture crashes exactly 3 times" fi -# Phase 2: the 4th container stays up (sleep 3600). Wait for it to be Running -# and STAY Running, then let the anti-flap window (DEFAULT_MIN_HEALTHY_TIME=10s) +# Phase 2: the 4th container stays up (sleep 3600). Let `stable_after` (15s) # elapse so the scheduler forgives the accrued count. 40s is generous headroom. log "waiting 40s for the worker to stay healthy past the window and forgive restart_count..." sleep 40 diff --git a/tests/e2e/docker/t41_fast_fail_exit_127.sh b/tests/e2e/docker/t41_fast_fail_exit_127.sh index 637dc9b6..9bbdd046 100755 --- a/tests/e2e/docker/t41_fast_fail_exit_127.sh +++ b/tests/e2e/docker/t41_fast_fail_exit_127.sh @@ -1,15 +1,13 @@ #!/usr/bin/env bash -# T41: Phase 3 fast-fail on a non-retryable exit code. A container that RUNS but -# exec's a binary that doesn't exist exits 127 (command not found). 127 is -# Terminal in the classifier, so the deployment must fail FAST — jump straight -# to a terminal status without burning all MAX_RESTART_COUNT (5) restart cycles. +# T41: a non-retryable exit code. A container that RUNS but exec's a binary +# that doesn't exist exits 127 (command not found). The program can never run +# until someone fixes the image or the command, so the worker lands on +# `create_container_error` at once and its next attempt waits up to the +# backoff cap (5 minutes by default) instead of climbing the curve from 10s. # # This is distinct from t23: there Docker `start` itself fails (a create/start # boundary error). Here `create`/`start` succeed and the container actually runs; -# it is the EXIT CODE at the crash boundary (127) that is non-retryable. We -# assert convergence happens in noticeably FEWER cycles than the full -# crash-loop path: very few containers spawned, and restart_count does not have -# to climb tick-by-tick to 5. +# it is the EXIT CODE at the crash boundary (127) that is non-retryable. set -euo pipefail @@ -17,18 +15,14 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # shellcheck source=../lib.sh source "$SCRIPT_DIR/../lib.sh" -log "== T41: exit 127 must fail fast, not burn the whole restart budget ==" +log "== T41: exit 127 lands on create_container_error and waits for an operator ==" start_ring ring_login "$RING_BIN" apply --file "$SCRIPT_DIR/../fixtures/command-not-found.yaml" -# Fast-fail should land on a terminal status within a handful of ticks. 30s is -# far short of the ~5+ ticks the slow restart-count path needs but is generous -# for the fast path; it also lets us assert "few containers spawned" before a -# slow loop could have created many. -log "waiting 30s for the fast-fail terminal convergence..." +log "waiting 30s..." sleep 30 DEPLOYMENT_ID=$(get_deployment_id "ring-e2e" "command-not-found") @@ -46,20 +40,19 @@ STATUS=$("$RING_BIN" deployment list --output json \ log "observed: total_containers=$TOTAL_CONTAINERS restart_count=$RESTART_COUNT status=$STATUS" -# 1) Must have reached a terminal status quickly. The classifier maps a 127 exit -# to CreateContainerError; CrashLoopBackOff is also acceptable as a terminal -# convergence, but the point is it must be terminal, not still trying. -case "$STATUS" in - create_container_error|crash_loop_back_off) ;; - *) fail "expected a terminal status (create_container_error / crash_loop_back_off) via fast-fail, got '$STATUS'" ;; -esac +# 1) The exit code is recognised as needing an operator. +if [ "$STATUS" != "create_container_error" ]; then + fail "expected create_container_error after an exit 127, got '$STATUS'" +fi + +# 2) Its retries start at the cap: in 30s, at most a couple of attempts. +if [ "${RESTART_COUNT:-0}" -lt 1 ] || [ "$RESTART_COUNT" -gt 3 ]; then + fail "expected 1 to 3 attempts in 30s, got restart_count=$RESTART_COUNT" +fi -# 2) Fast-fail means FEWER containers spawned than the slow path. The slow path -# (counting one crash per tick to 5) would spawn at least ~5 containers over -# several ticks; the fast path lands terminal almost immediately. Require the -# count to stay small. -if [ "$TOTAL_CONTAINERS" -gt 3 ]; then - fail "too many containers spawned ($TOTAL_CONTAINERS) — exit 127 is NOT failing fast (burning restart cycles)" +# 3) The dead container is removed once its exit is recorded. +if [ "$TOTAL_CONTAINERS" -gt 1 ]; then + fail "$TOTAL_CONTAINERS containers left for the deployment — crashed containers are not removed" fi log "== T41: PASS ==" diff --git a/tests/e2e/docker/t43_windowed_reset_stuck_creating.sh b/tests/e2e/docker/t43_windowed_reset_stuck_creating.sh index 6cfbbbfe..d531d7f4 100755 --- a/tests/e2e/docker/t43_windowed_reset_stuck_creating.sh +++ b/tests/e2e/docker/t43_windowed_reset_stuck_creating.sh @@ -19,12 +19,13 @@ # where the old gate left restart_count monotonic. # # Invariants: -# 1. The crash phase registers: restart_count climbs to 3 (< MAX = 5), so a -# *windowed* reset is exercised rather than CrashLoopBackOff. +# 1. The crash phase registers: restart_count climbs to 3. # 2. The worker then settles in `creating` (readiness can't pass on host net) # and STAYS there — it must never reach `running` or `failed`. -# 3. After the anti-flap window elapses the scheduler forgives the accrued -# count: restart_count is reset to 0 while the status is still `creating`. +# 3. After the restart policy's `stable_after` the scheduler forgives the +# accrued count: restart_count is reset to 0 while the status is still +# `creating`. A short policy (retries at most 2s apart, reset after 15s) +# keeps the test fast. set -euo pipefail @@ -43,15 +44,18 @@ STATE_DIR="/tmp/ring-e2e-t43" rm -rf "$STATE_DIR" mkdir -p "$STATE_DIR" +export RING_EXTRA_CONFIG='[server.restart] +base = "1s" +cap = "2s" +stable_after = "15s"' + start_ring ring_login "$RING_BIN" apply --file "$SCRIPT_DIR/../fixtures/crash-then-stuck-creating.yaml" # Invariant 1: the container crashes 3 times. Wait for restart_count to reach 3, -# which proves the crash phase registered. It must stop at 3 (the 4th start stays -# up), well short of MAX_RESTART_COUNT (5) — otherwise we couldn't test a -# *windowed* reset, only a CrashLoopBackOff. +# which proves the crash phase registered. It stops at 3: the 4th start stays up. log "waiting for the crash phase to accrue restart_count=3..." REACHED=0 for _ in $(seq 1 60); do @@ -67,13 +71,13 @@ if [ "$REACHED" -ne 1 ]; then fi log "crash phase registered: restart_count=$RC" -if [ "${RC:-0}" -ge 5 ]; then - fail "restart_count reached the CrashLoopBackOff bound ($RC); cannot test the windowed reset" +if [ "${RC:-0}" -gt 3 ]; then + fail "restart_count reached $RC; the fixture crashes exactly 3 times" fi # Invariant 2: the 4th container stays up but can never pass readiness on host # networking, so it settles in `creating`. Wait for that, then confirm it STAYS -# there past the anti-flap window (DEFAULT_MIN_HEALTHY_TIME = 10s) — it must +# there past `stable_after` (15s) — it must # never be promoted to `running` nor failed by the rollout deadline (deferred by # the 300s start_period). wait_deployment_status "$NS" "$NAME" "creating" 60 diff --git a/tests/e2e/docker/t6_crashloop.sh b/tests/e2e/docker/t6_crashloop.sh index d69888c1..b86533f5 100755 --- a/tests/e2e/docker/t6_crashloop.sh +++ b/tests/e2e/docker/t6_crashloop.sh @@ -1,8 +1,9 @@ #!/usr/bin/env bash -# T6: a worker that exits non-zero must hit CrashLoopBackOff after MAX_RESTART_COUNT -# instead of being respawned forever. Today (without the docker-events listener) -# this test FAILS: restart_count stays at 0 and the scheduler keeps creating new -# containers indefinitely, leaving stopped containers piling up on disk. +# T6: a worker that exits non-zero over and over is never abandoned. Each crash +# is counted, the dead container is removed, and the worker is started again +# after a backoff, well past the 5 attempts after which Ring used to give up. +# +# A short restart policy keeps the test fast: retries come at most 2s apart. set -euo pipefail @@ -10,17 +11,19 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # shellcheck source=../lib.sh source "$SCRIPT_DIR/../lib.sh" -log "== T6: crash loop must converge to CrashLoopBackOff ==" +log "== T6: a crash loop backs off and never gives up ==" + +export RING_EXTRA_CONFIG='[server.restart] +base = "1s" +cap = "2s"' start_ring ring_login "$RING_BIN" apply --file "$SCRIPT_DIR/../fixtures/crashloop.yaml" -# Give the scheduler time to react. With a 1s scheduler interval, 90s leaves -# comfortable headroom for restart_count to climb past MAX_RESTART_COUNT (5). -log "waiting 90s for the scheduler to react to repeated crashes..." -sleep 90 +log "waiting 45s for repeated crashes..." +sleep 45 DEPLOYMENT_ID=$(get_deployment_id "ring-e2e" "crashloop") if [ -z "$DEPLOYMENT_ID" ]; then @@ -28,7 +31,6 @@ if [ -z "$DEPLOYMENT_ID" ]; then fi log "deployment id: $DEPLOYMENT_ID" -# Count every container ever created for this deployment (running + exited). TOTAL_CONTAINERS=$(docker ps -aq --filter "label=ring_deployment=$DEPLOYMENT_ID" | wc -l | tr -d ' ') RESTART_COUNT=$(get_restart_count "ring-e2e" "crashloop") STATUS=$("$RING_BIN" deployment list --output json \ @@ -38,21 +40,21 @@ STATUS=$("$RING_BIN" deployment list --output json \ log "observed: total_containers=$TOTAL_CONTAINERS restart_count=$RESTART_COUNT status=$STATUS" -# Hard cap: restart_count must not exceed MAX_RESTART_COUNT (5 today). -if [ "${RESTART_COUNT:-0}" -lt 5 ]; then - fail "expected restart_count >= 5, got $RESTART_COUNT (the scheduler is not counting runtime crashes)" +# Still retrying past the old budget of 5: the worker was not abandoned. +if [ "${RESTART_COUNT:-0}" -le 5 ]; then + fail "expected restart_count > 5, got $RESTART_COUNT (the worker was abandoned or crashes are not counted)" fi -# The deployment must have transitioned to CrashLoopBackOff. -if [ "$STATUS" != "crash_loop_back_off" ]; then - fail "expected status crash_loop_back_off, got '$STATUS'" -fi +# Between two attempts the worker waits in crash_loop_back_off; caught right +# after a start, it is creating or running. +case "$STATUS" in + crash_loop_back_off|creating|running) ;; + *) fail "expected crash_loop_back_off (or a fresh start), got '$STATUS'" ;; +esac -# Sanity: the total number of containers ever spawned for this deployment must -# stay bounded. Without a cap on restarts, this number grows linearly with time. -# Allowing a small headroom over MAX_RESTART_COUNT for races. -if [ "$TOTAL_CONTAINERS" -gt 10 ]; then - fail "too many containers spawned ($TOTAL_CONTAINERS) — restart loop is not bounded" +# Every dead container is removed once its exit is recorded: they never pile up. +if [ "$TOTAL_CONTAINERS" -gt 1 ]; then + fail "$TOTAL_CONTAINERS containers left for the deployment — crashed containers are not removed" fi log "== T6: PASS ==" diff --git a/tests/e2e/fixtures/crash-then-heal.yaml b/tests/e2e/fixtures/crash-then-heal.yaml index 8af4bd5f..b7954dcc 100644 --- a/tests/e2e/fixtures/crash-then-heal.yaml +++ b/tests/e2e/fixtures/crash-then-heal.yaml @@ -4,14 +4,13 @@ deployments: namespace: ring-e2e runtime: docker image: alpine:3.19 - # Phase 1 windowed restart_count reset. Docker re-runs the SAME command on + # Windowed restart_count reset. Docker re-runs the SAME command on # every (re)spawn, so the "crash a few times then stay healthy" behaviour is # driven by state on a host bind mount: each start increments a counter in # /state/count. While count <= 3 the container exits 1 (a crash Ring counts # toward restart_count); on the 4th start it stops crashing and sleeps - # forever (stays Running). Once it has been continuously Running past the - # anti-flap window (DEFAULT_MIN_HEALTHY_TIME = 10s), the scheduler forgives - # the accrued restart_count and resets it to 0. + # forever (stays Running). Once it has been continuously Running for the + # restart policy's `stable_after`, the scheduler resets restart_count to 0. command: - "sh" - "-c" diff --git a/tests/e2e/fixtures/git-auth-crashloop.yaml b/tests/e2e/fixtures/git-auth-crashloop.yaml index 77812df7..72e0c100 100644 --- a/tests/e2e/fixtures/git-auth-crashloop.yaml +++ b/tests/e2e/fixtures/git-auth-crashloop.yaml @@ -6,9 +6,8 @@ deployments: image: alpine:3.19 # Reproduces the production case: an entrypoint that fails like a git clone # against a repo it can't authenticate to. `git` exits 128 on an auth - # failure, and 128 is RETRYABLE per the exit-code classifier, so the only - # way this converges is the restart-count path — exactly the prod bug: - # the container must reach CrashLoopBackOff after MAX_RESTART_COUNT and - # STOP being recreated, not loop forever. + # failure, and 128 is RETRYABLE per the exit-code classifier: the worker + # must keep being retried, spaced out by the restart backoff instead of + # recreated on every scheduler tick. command: ["sh", "-c", "echo 'fatal: Authentication failed for https://example.com/private.git' >&2; exit 128"] replicas: 1 diff --git a/tests/e2e/t3_ch_nginx.sh b/tests/e2e/t3_ch_nginx.sh new file mode 100755 index 00000000..099ebf98 --- /dev/null +++ b/tests/e2e/t3_ch_nginx.sh @@ -0,0 +1,139 @@ +#!/usr/bin/env bash +# T3-CH: Deploy nginx in a Cloud Hypervisor VM via ring-init, verify it +# responds to HTTP requests from the host, then delete and verify cleanup. +# +# Requires: CAP_NET_ADMIN on ring binary, cloud-hypervisor, kernel, initramfs, +# and a pre-built nginx.raw image. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=./lib.sh +source "$SCRIPT_DIR/lib.sh" +# shellcheck source=./setup-ch-ringinit.sh +source "$SCRIPT_DIR/setup-ch-ringinit.sh" + +NGINX_IMAGE="${RING_E2E_NGINX_IMAGE:-$HOME/.cache/ring-e2e/nginx.raw}" + +log "== T3-CH: nginx in VM ==" + +# Check nginx image exists +if [ ! -f "$NGINX_IMAGE" ]; then + fail "nginx image not found at $NGINX_IMAGE (build it with: ~/Workspace/kemeter/builder/build.sh)" +fi + +# Check CAP_NET_ADMIN +if ! getcap "$RING_BIN" 2>/dev/null | grep -q cap_net_admin; then + fail "ring binary needs CAP_NET_ADMIN (sudo setcap cap_net_admin+ep $RING_BIN)" +fi + +setup_ch_ringinit + +start_ring +ring_login + +FIXTURE="$RING_TEST_DIR/nginx-vm.yaml" +cat > "$FIXTURE" </dev/null | head -1) +if [ -z "$INIT_DISK" ]; then + fail "init disk not found" +fi + +VM_IP="" +if command -v fuse2fs > /dev/null; then + MNT=$(mktemp -d) + fuse2fs "$INIT_DISK" "$MNT" -o fakeroot,ro 2>/dev/null + VM_IP=$(cat "$MNT/config.json" 2>/dev/null | jq -r '.network.ip // empty') + fusermount -u "$MNT" 2>/dev/null + rmdir "$MNT" 2>/dev/null +fi + +if [ -z "$VM_IP" ]; then + fail "could not extract VM IP from init disk" +fi +log "VM IP: $VM_IP" + +# Wait for nginx to be ready +log "waiting for nginx to respond..." +NGINX_OK=false +for _ in $(seq 1 30); do + if curl -sf --connect-timeout 2 "http://$VM_IP" > /dev/null 2>&1; then + NGINX_OK=true + break + fi + sleep 1 +done + +if [ "$NGINX_OK" != "true" ]; then + fail "nginx did not respond at http://$VM_IP within 30s" +fi + +# Verify response content +RESPONSE=$(curl -sf --connect-timeout 5 "http://$VM_IP") +if echo "$RESPONSE" | grep -q "ring-init works"; then + log "nginx responded: OK" +else + fail "unexpected response from nginx: $RESPONSE" +fi + +# Verify network interfaces exist +BRIDGE=$(ip link show 2>/dev/null | grep -oP 'rbr\d+' | head -1) +TAP=$(ip link show 2>/dev/null | grep -oP 'rtap\d+' | head -1) +if [ -z "$BRIDGE" ] || [ -z "$TAP" ]; then + fail "network interfaces not found (bridge=$BRIDGE tap=$TAP)" +fi +log "network: bridge=$BRIDGE tap=$TAP" + +# Delete +"$RING_BIN" deployment delete "$DEPLOYMENT_ID" + +# Wait for cleanup +log "waiting for cleanup..." +for _ in $(seq 1 60); do + socket_count=$(find "$RING_E2E_CH_SOCKET_DIR" -maxdepth 1 -type s -name "ch-*.sock" 2>/dev/null | wc -l | tr -d ' ') + if [ "$socket_count" -eq 0 ]; then + break + fi + sleep 1 +done +if [ "$socket_count" -ne 0 ]; then + fail "CH socket still present after delete" +fi +log "VM cleaned up" + +# Verify network interfaces are gone +sleep 2 +if ip link show "$BRIDGE" > /dev/null 2>&1; then + fail "bridge $BRIDGE still exists after delete" +fi +log "network cleaned up" + +log "== T3-CH: PASS =="