diff --git a/docs/manifest.md b/docs/manifest.md index 4e9350e..0f8d9ad 100644 --- a/docs/manifest.md +++ b/docs/manifest.md @@ -32,6 +32,7 @@ Odorobo should provision. It contains: Cloud Hypervisor has no per-disk boot index, so providers must preserve this order when translating the manifest. The control plane owns attachment identity and ordering; the provider driver/storage transform resolves the URI or volume ID to a node-local device path. + Current URI support and attachment lifecycle are documented in [the storage contract](storage.md). - `networks`: stable network IDs and optional guest MAC addresses. The control plane owns the attachment identity and requested MAC; the provider networking transform resolves the network to a host interface or tap device. diff --git a/docs/provisioning.md b/docs/provisioning.md index f56a078..972c339 100644 --- a/docs/provisioning.md +++ b/docs/provisioning.md @@ -52,8 +52,25 @@ reservations from failed startup attempts are rolled back only after confirmed process exit. An active lease cannot be reused—even for the same VM ID—until exit is confirmed. Every reservation has a distinct persisted lease token; startup rollback, exit recording, and deletion retries use that token so one -incarnation cannot release a newer incarnation's lease. After an unclean agent/host failure, stale active leases may -need manual recovery rather than assuming the old VMM is gone. A requested CID is +incarnation cannot release a newer incarnation's lease. + +On SIGTERM/SIGINT the agent stops its VMs before exiting (waiting up to 60 +seconds), so each VMM is reaped and its lease is marked stopped. A second +signal exits immediately. The shipped systemd unit uses `KillMode=mixed` so +only the agent gets the SIGTERM (the VMMs run in its cgroup), and a 90 second +`TimeoutStopSec` so systemd doesn't SIGKILL it mid-shutdown. If you write your +own unit, keep both. The single-instance lockfile (`/var/lock/odorobo.lock`) is +an flock, so an agent that was killed outright doesn't block the next start. + +The registry also records each lease's VMM process by PID, kernel start time, +and boot ID, so a process from before a reboot is never mistaken for a new one. +After an unclean agent exit, a restarted agent treats a lease as stopped once +that exact process is gone, meaning every one of its threads has exited. An explicit delete +of a VM whose VMM is still running with no actor left (for example after the +agent was killed) SIGKILLs that exact process before releasing its lease and +runtime files. A lease that never had its process recorded (the agent died +between reserving the CID and spawning the VMM, or recording failed) still +needs manual recovery. A requested CID is reserved if free; a conflict fails VM creation rather than silently assigning a different CID. `GetVMInfo` returns the effective VM manifest, including the allocated CID in `observed.vsock_guest_cid` and the desired socket path; @@ -69,9 +86,27 @@ a host need distinct key paths. Deletion is acknowledged only after the known VM-owning agents confirm process exit and node-local cleanup. Missing owners keep deletion pending; retry after -those agents become reachable. Cleanup ownership survives discovery-cache -eviction in scheduler memory, but is not a durable cluster inventory across -scheduler restarts. Keep node identities stable when recovering failed agents. +those agents become reachable, or delete with `?force=true` to drop owners whose +agent can't be reached at all (an agent that answers with an error, or that +might have gotten the request but timed out, still fails the delete). Every +create and delete the scheduler sends carries a time-ordered stamp, so a delete +that arrives after its VM ID was recreated is acked without touching the new +VM. Cleanup ownership survives discovery-cache eviction in scheduler +memory, and after a scheduler restart it is rebuilt from agent status: each +agent reports running VMs and `retained_vms` (IDs that still hold node-local +state, like a stopped CID lease). An agent that was unreachable since the +restart can't be known as an owner, so keep node identities stable when +recovering failed agents. + +If an agent reports a VM the scheduler already finished deleting (like a node +skipped by a forced delete coming back), the scheduler deletes it there in the +background. Stopped VMs are never cleaned up this way. Until that node acks a +delete or reports a full status without the VM, the ID can't be recreated, since +the old copy could still be running on the same storage. Use a new VM ID if the +node isn't coming back. Stop/delete tombstones are kept in scheduler memory +only: stopped ones for an hour, deleted ones for a day (longer while a node +might still hold the VM), and pending deletes until they finish. A scheduler +restart forgets all of this. Storage teardown releases only resources acquired by Odorobo. Shared mappings are reference-counted within the agent process; pre-existing external mappings diff --git a/docs/scheduler_routes.md b/docs/scheduler_routes.md index a1e62f9..2ba16f9 100644 --- a/docs/scheduler_routes.md +++ b/docs/scheduler_routes.md @@ -8,6 +8,14 @@ we need CRUD. we keep track of if its active. MACs for VMs need to be stored somewhere. it can be either in the DHT or in the Dashboard. doesnt matter. +### deleting VMs + +`POST /vms` refuses an ID that some agent still reports running when the scheduler has no record of it, like right after a scheduler restart before discovery catches up, or a VM whose exit couldn't be confirmed. starting it somewhere else would run two copies on the same storage. retry once discovery finds it (the retry then returns the existing VM), or delete it first. an agent that only holds a stopped VM's lease doesn't block this, since recreating is how a stopped VM gets started again. + +`DELETE /vms/{vmid}` only succeeds once every node that might still hold something for the VM (a running process, a stopped vsock CID lease, storage mappings) confirms cleanup. if one of those nodes is unreachable, the delete fails and the ID can't be recreated until a retry works. + +`DELETE /vms/{vmid}?force=true` is for when a node is gone for good. owners whose agent can't be reached at all (no route, the dial fails, or no agent is running there) are dropped with a warning instead of failing the delete. an agent that answers and says cleanup failed still fails the delete, force or not, since it's alive and something is actually wrong. so does one that might have gotten the request but didn't answer (a timeout or a connection dropped mid-request), since a slow teardown looks exactly like that. retry, and an agent that's really gone shows up as unreachable. whatever a dropped node still holds is leaked, but if it ever comes back and reports the VM, the scheduler deletes it there in the background. until then `POST /vms` with the same ID is refused, since the old copy might still be running on the same storage. use a new VM ID if the node is gone for good. + ## volumes we again need CRUD, and we keep track of the volumes cause we want to just check what is in the SAN diff --git a/docs/storage.md b/docs/storage.md index 9305377..ac77a10 100644 --- a/docs/storage.md +++ b/docs/storage.md @@ -1,52 +1,174 @@ -# Storage Integration +# VM storage contract -Orodobo adds a storage integration layer on top of Cloud Hypervisor's block device support, using config transformers to convert custom URI schemes into valid block device paths for Cloud Hypervisor. -This allows users to specify storage devices in a more flexible way, and enables simpler integration with networked storage backends, by having Odorobo abstract and attach/detach storage devices on demand based on the VM's lifecycle and provisioning hooks. +Odorobo accepts storage attachments through the VM manifest's ordered +`desired.storage` list. The Cloud Hypervisor driver resolves each supported URI +to a host-local file or block-device path when creating a VM. It does not copy +an RBD image onto each compute node. -Currently, Odorobo's Storage transformer supports the following backends: +The initial storage contract supports local files and kernel-mapped Ceph RBD +images. iSCSI remains available through the existing transform, but is outside +the Ceph lifecycle contract described here. iSCSI URIs must be exactly +`iscsi://[:]//` (port defaults to 3260), where `` +is plain decimal digits that fit in 32 bits, optionally written `lun`. The +LUN is required. URIs with a malformed or out-of-range LUN, extra path segments, +a query or fragment, or a percent-encoded portal or IQN are rejected before any +login rather than falling back to LUN 0. This defines behavior for new +manifest attachments; it does not migrate existing Odorobo deployments or +rewrite existing local images. A manifest attachment has a stable +attachment ID, one source (`uri` or `volume_id`), and an optional `read_only` +flag. `volume_id` resolution is not yet defined; use a supported URI. -- **Local Disk Images**: Specify a local disk image with the `file://` URI scheme, e.g. `file:///var/lib/odorobo/images/my-disk.qcow2`. This is simply passed through to Cloud Hypervisor as if the `file://` prefix was not there, implemented as a dummy transformer for validating the storage transformer API. -- **Ceph RBD**: Specify a Ceph RBD image with the `rbd://` URI scheme, e.g. `rbd://my-pool/my-image`. Odorobo will use the `rbd` command line tool to map the RBD image to a local block device (e.g. `/dev/rbd0`), and then pass this block device path to Cloud Hypervisor. -- **iSCSI**: Specify an iSCSI target with the `iscsi://` URI scheme, e.g. `iscsi://://`. Odorobo will use the `iscsiadm` command line tool to log in to the iSCSI target and map the specified LUN to a local block device (e.g. `/dev/disk/by-path/ip-:-iscsi--lun-`), and then pass this block device path to Cloud Hypervisor. +## Local file images -To use the storage transformers, simply specify the desired URI in place of the `path` field for a block device in your VM config. For example: +Use an absolute `file://` URI, for example: ```json -{ - ... - "disks": [ - { - "path": "rdb://my-pool/my-image", - "id": "disk0", - } - ], -} +{ "id": "root", "uri": "file:///var/lib/odorobo/images/root.raw" } ``` -Odorobo will automatically detect the `rbd://` scheme, map the RBD image to a local block device, and pass the block device path to Cloud Hypervisor when creating the VM, transforming them to: +The path refers to a file already present on the node running the VM. Odorobo +passes the path to Cloud Hypervisor; it does not copy, create, or delete the +image. The file must be readable (and writable unless the attachment is +read-only) by the service account and accessible inside the Cloud Hypervisor +process's mount namespace. Local-file attachments have no external resource +to unmap during teardown. + +## Ceph RBD images + +Specify an existing RBD image as `rbd:///`: ```json { - ... - "disks": [ - { - "path": "/dev/rbd/my-pool/my-image", - "id": "rdb://my-pool/my-image?id=disk0", - } - ], + "id": "root", + "uri": "rbd://odorobo-vms/root-01", + "read_only": false } ``` -The `id` field is transformed to include the original URI for reference, and can be used in provisioning hooks to identify which disk is which when attaching/detaching storage devices on demand. +The pool and image are part of the VM attachment URI, not a local cache path. +For the initial contract, pool and image names use ASCII letters, digits, `.`, +`_`, or `-`; names cannot be empty or start with `-`. Pool names must be +lowercase; the transformer validates the original URI and rejects case variants +rather than normalizing them. The URI must have exactly one image path segment. +Credentials, ports, query parameters, fragments, namespaces, and snapshot +selectors are not accepted. In particular, +`rbd://pool/ns/image` and `rbd://pool/image@snapshot` are not supported by this +contract. These restrictions prevent an ambiguous URI from selecting a +resource different from the one Odorobo tracks. + +The referenced pool and image must already exist and be reachable by the +configured Ceph client. Odorobo maps the image with the `rbd` CLI and passes the +resulting kernel block device to Cloud Hypervisor. It does not create pools or +images, clone/import images, resize them, or delete them. Deleting a VM removes +its attachment and may unmap a mapping owned by Odorobo; it never deletes the +RBD image or pool. Provisioning, snapshotting, importing, and image deletion +remain operator-managed operations. + +`read_only: true` makes the Cloud Hypervisor disk read-only. Odorobo's host-side +attachment reference counting prevents a mapping from being unmapped while +another VM still references it; it does not make concurrent writable use of a +filesystem safe. Share writable images only when the guest workload and +filesystem are explicitly designed for multi-writer access. Read-only sharing +is the usual safe shared-image pattern. + +### Node prerequisites and Ceph client configuration + +Each VM node needs the `rbd` executable (typically from `ceph-common`), the +kernel RBD module, permission to map block devices, and access to the Ceph +cluster. Odorobo passes these optional environment variables through to `rbd`: + +| Environment variable | `rbd` option | Purpose | +| --- | --- | --- | +| `CEPH_CONFIG` | `--conf` | Ceph configuration file path | +| `CEPH_ID` | `--id` | Ceph client identity, such as `odorobo` | +| `CEPH_KEYFILE` | `--keyfile` | Path to the client key file; do not put key contents in a manifest | +| `CEPH_CLUSTER` | `--cluster` | Ceph cluster name | + +When omitted, the Ceph CLI uses its normal defaults. These values are node +configuration: all RBD references on a node use the same configured client +context. Keep key files readable only by the Odorobo service account and grant +only the pool capabilities needed by the node. Pool and image selection is +configured per attachment in its `rbd://` URI. + +Odorobo asks `rbd device map` not to wait on a udev event in its own namespace. +When the host's Ceph udev rule is present, the driver prefers the stable path +`/dev/rbd//`. Otherwise, it falls back to the device reported by +`rbd device list` (for example `/dev/rbd0`). The udev rule commonly provided by +Ceph packages is: + +```udev +KERNEL=="rbd[0-9]*", ENV{DEVTYPE}=="disk", PROGRAM="/usr/bin/ceph-rbdnamer %k", SYMLINK+="rbd/%c" +KERNEL=="rbd[0-9]*", ENV{DEVTYPE}=="partition", PROGRAM="/usr/bin/ceph-rbdnamer %k", SYMLINK+="rbd/%c-part%n" +``` + +Install it as `/etc/udev/rules.d/50-rbd.rules` only if the distribution's Ceph +packages do not already provide an equivalent rule. + +## Attachment and mapping lifecycle + +The control plane owns the desired attachment ID, source, read-only setting, +and disk order. On a node, the storage provider owns mapping and cleanup +bookkeeping; Cloud Hypervisor only receives the resolved path. The lifecycle +is: + +1. **Requested** — the manifest names a URI-backed attachment. +2. **Acquiring** — the storage provider records a VM lease before starting map + or path-resolution work. A failed or cancelled create therefore cannot + silently lose track of a partial mapping. +3. **Attached** — the resolved path is passed to Cloud Hypervisor. Multiple + Odorobo VMs using the same canonical RBD resource share one provider + acquisition and increment its reference count. +4. **Releasing** — after the VM process has exited, teardown releases that + VM's lease. An Odorobo-owned mapping is unmapped only when its final active + lease is released. A mapping that existed before Odorobo acquired it is + borrowed and is never automatically unmapped by Odorobo. +5. **Released** — the lease is removed. Repeated cleanup of an already-released + lease is a no-op. + +A confirmed failure to map can be retried. If a command's result is ambiguous +(for example, the command timed out but the mapping may have been created), +Odorobo quarantines the resource rather than guessing who owns it: it blocks +reuse and automatic unmapping and reports that operator reconciliation is +required. Do not manually unmap a device while an affected VM may still be +running. The in-memory lease ledger is not persisted across agent restarts, so +inventory and reconcile outstanding RBD mappings before restarting an agent +that has active or uncertain storage operations. + +### Operation deadlines + +Storage work runs in background tasks; callers (VM create and delete) only +wait for it up to a deadline. The deadline is derived from the storage command +limits rather than picked by hand: each `rbd`/`iscsiadm` invocation gets 30 +seconds plus a 2-second reap grace, and one disk resolution can run up to four +of them (RBD `device list`, `device map`, `device list`, then another +`device list` after up to 10 seconds of waiting for the udev path), plus up to +four 5-second iSCSI DNS lookups. That gives a per-operation budget of about +158 seconds. VM create applies it to each disk separately, and Cloud +Hypervisor's create timeout is 60 seconds on top of every disk's budget, so a +slow but working map isn't torn down as a failed create. The scheduler keeps a +pending placement for the VM's whole worst-case startup plus two minutes +before rescheduling it, since the create request itself times out after 120 +seconds while the agent is still starting the VM. VM delete releases +disks one after another, so its deadline is one budget per lease (and two per +still-pending resolution, which must finish and may add a lease). + +A "Storage operation deadline exceeded" error does not mean the operation +failed or was cancelled. The work is still running and the agent keeps the VM's +leases and any quarantine while it does. Retrying the delete (or the teardown +after a failed create) joins the in-flight cleanup instead of starting a second +one, and a new create for the same VM is refused until the earlier work +finishes. Do not reuse or manually unmap/log out the storage while it is still +in flight. + +VM shutdown/deletion releases VM attachments according to the VM lifecycle, +but does not delete backing images. A VM's configured disk order is preserved; +the first manifest storage entry is the preferred boot disk. +## Mocking and tests -> [!NOTE] -> Ceph RBD integration requires the following udev rule to allow Odorobo to easily find the mapped block device for a given RBD image: -> -> ``` -> # /etc/udev/rules.d/50-rbd.rules -> KERNEL=="rbd[0-9]*", ENV{DEVTYPE}=="disk", PROGRAM="/usr/bin/ceph-rbdnamer %k", SYMLINK+="rbd/%c" -> KERNEL=="rbd[0-9]*", ENV{DEVTYPE}=="partition", PROGRAM="/usr/bin/ceph-rbdnamer %k", SYMLINK+="rbd/%c-part%n" -> ``` -> -> This rule is commonly included in Ceph packages (`ceph-common` in Fedora), but if your distribution does not include it by default, you will need to add it manually for RBD support to work properly. This allows Odorobo to find the mapped block device for a given RBD image under `/dev/rbd/`, which is necessary for passing the correct block device path to Cloud Hypervisor. +Storage backends implement a mockable provider interface for acquisition, +resolution, and release. Lifecycle tests use fake providers to cover shared +leases, borrowed mappings, partial failures, ambiguous command outcomes, +retries, cancellation, and idempotent cleanup; they do not require a live Ceph +cluster. The local Ceph development environment is documented in +[`.local/dev/README.md`](../.local/dev/README.md). diff --git a/odorobo/benches/agent_status.rs b/odorobo/benches/agent_status.rs index 56c133f..6373258 100644 --- a/odorobo/benches/agent_status.rs +++ b/odorobo/benches/agent_status.rs @@ -18,6 +18,7 @@ fn status(vm_count: usize) -> AgentStatus { vms.sort_unstable(); vms }, + retained_vms: Vec::new(), metadata: ObjectMetadata::default(), } } @@ -44,6 +45,7 @@ fn main() { revision: 1, added, removed, + retained_vms: Vec::new(), used_vcpus: base.used_vcpus, used_ram: base.used_ram, }; diff --git a/odorobo/src/actors/agent_actor.rs b/odorobo/src/actors/agent_actor.rs index 99b14fe..caf6f5b 100644 --- a/odorobo/src/actors/agent_actor.rs +++ b/odorobo/src/actors/agent_actor.rs @@ -27,19 +27,27 @@ use ahash::AHashMap; use bytesize::ByteSize; use kameo::prelude::*; use stable_eyre::{Report, Result}; -use std::{ops::ControlFlow, sync::Arc}; +use std::{ops::ControlFlow, sync::Arc, time::Duration}; use sysinfo::System; use tracing::{error, info, trace, warn}; use ulid::Ulid; use kameo::error::PanicError; +/// how long the agent waits for its VMs to shut down and reap before exiting +const SHUTDOWN_VM_TIMEOUT: Duration = Duration::from_secs(60); +/// how long a delete waits for a leftover VMM to die after SIGKILL +const LEFTOVER_VMM_KILL_TIMEOUT: Duration = Duration::from_secs(10); + pub struct VMCacheData { actor_ref: ActorRef, config: VmManifest, vcpus: u32, memory_bytes: u64, cleanup_proof: Arc, + /// the scheduler's stamp for this incarnation, if it sent one. a delete + /// stamped before it was meant for an older VM with the same ID + generation: Option, } #[derive(RemoteActor)] @@ -52,7 +60,7 @@ pub struct AgentActor { status_history: StatusChangeHistory, pub config: Config, pub vms: AHashMap, - /// VM IDs whose actor teardown failed. We don't let these get recreated until + /// VM IDs whose actor teardown failed. we don't let these get recreated until /// an explicit delete confirms any leftover CID lease is safe to release blocked_vm_ids: AHashMap>, // pub network_actor: ActorRef, @@ -68,8 +76,49 @@ fn startup_cleanup_failed(result: &Result<(), kameo::error::HookError<&Report>>) } } +/// whether a VM actor that already stopped has to block its ID. a startup that +/// failed cleanly with a confirmed exit leaves nothing behind, so it doesn't +fn needs_quarantine( + actor_ref: &ActorRef, + shutdown_failed: bool, + proof: &VMCleanupProof, +) -> bool { + let startup_cleanup_failed = actor_ref + .with_startup_result(|result| startup_cleanup_failed(&result)) + .unwrap_or(false); + let started = actor_ref + .with_startup_result(|result| result.is_ok()) + .unwrap_or(false); + startup_cleanup_failed || !proof.process_exit_confirmed() || shutdown_failed && started +} + +/// make sure a VMM we lost track of is really gone, killing it if it's still +/// running. returns whether we found one and confirmed its exit +async fn stop_leftover_vmm( + allocator: &VsockCidAllocator, + vmid: Ulid, + proof: Option<&VMCleanupProof>, +) -> Result { + let process = match proof.and_then(VMCleanupProof::process) { + Some(process) => Some(process), + None => allocator.recorded_process(vmid)?, + }; + let Some(process) = process else { + return Ok(false); + }; + warn!(%vmid, pid = process.pid, "stopping leftover VMM before cleaning up after it"); + process.terminate(LEFTOVER_VMM_KILL_TIMEOUT).await?; + if let Some(proof) = proof { + proof.set_process_exit_confirmed(true); + } + // a lease we don't hold a token for (like after an agent restart) gets + // marked stopped through the VMM recorded next to it + allocator.reconcile_exited(vmid)?; + Ok(true) +} + /// a VM without vsock exiting doesn't tell us anything about a lease someone -/// else grabbed. We only release it once there's a stopped record on disk +/// else grabbed. we only release it once there's a stopped record on disk fn persist_cleanup_exit( allocator: &VsockCidAllocator, vmid: Ulid, @@ -103,7 +152,12 @@ fn release_cleanup_lease( impl AgentActor { fn verify_no_active_lease(&mut self, vmid: Ulid) -> Result<()> { - let result = VsockCidAllocator::from_environment().ensure_no_active_lease(vmid); + let allocator = VsockCidAllocator::from_environment(); + // a lease left over from before an agent restart is fine once its VMM is gone + if let Err(error) = allocator.reconcile_exited(vmid) { + warn!(%vmid, ?error, "failed to check the VMM behind an old vsock lease"); + } + let result = allocator.ensure_no_active_lease(vmid); if result .as_ref() .is_err_and(|error| error.downcast_ref::().is_some()) @@ -148,6 +202,102 @@ impl AgentActor { Some(removed) } + /// block a VM ID after its teardown failed. if we can't confirm the VMM + /// exited we keep it listed (and its resources counted), so the scheduler + /// doesn't fail it over while it might still be running + fn quarantine(&mut self, vmid: Ulid, proof: Arc) { + let exit_confirmed = proof.process_exit_confirmed(); + self.blocked_vm_ids.insert(vmid, proof); + if exit_confirmed { + self.remove_vm(vmid); + } + } + + fn retained_vms(&self) -> Vec { + let mut retained = VsockCidAllocator::from_environment() + .leased_vms() + .unwrap_or_else(|error| { + warn!(?error, "failed to read vsock CID leases for agent status"); + Vec::new() + }); + retained.extend(self.blocked_vm_ids.keys().copied()); + retained.retain(|vmid| !self.vms.contains_key(vmid)); + retained.sort_unstable(); + retained.dedup(); + retained + } + + /// cleanup for a VM whose actor is already gone, like after a failed + /// teardown, a shutdown, or an agent restart + async fn retry_detached_cleanup(&mut self, vmid: Ulid) -> DeleteVMReply { + let proof = self.blocked_vm_ids.get(&vmid).cloned(); + let allocator = VsockCidAllocator::from_environment(); + let mut exit_confirmed = proof + .as_ref() + .is_some_and(|proof| proof.process_exit_confirmed()); + if !exit_confirmed { + match stop_leftover_vmm(&allocator, vmid, proof.as_deref()).await { + Ok(stopped) => exit_confirmed = stopped, + Err(error) => { + return DeleteVMReply { + error: Some(format!("failed to stop leftover VMM: {error}")), + }; + } + } + } + if exit_confirmed { + if let Some(proof) = &proof + && let Err(error) = persist_cleanup_exit(&allocator, vmid, proof) + { + return DeleteVMReply { + error: Some(format!( + "failed to persist confirmed VM process exit: {error}" + )), + }; + } + // leftover storage leases are process-global, so we can still retry + // after the failed VM actor/transformer is gone. don't detach + // anything until we know for sure the VMM exited + if let Err(error) = StorageDriverTransformer::default().teardown( + &vmid.to_string(), + &mut cloud_hypervisor_client::models::VmConfig::default(), + ) { + return DeleteVMReply { + error: Some(format!("failed to retry VM storage cleanup: {error}")), + }; + } + // the old process exited but runtime cleanup failed. retry it before + // we ack the delete and let the paths get reused + let runtime_dir = crate::ch_driver::VMInstance::runtime_dir_for(&vmid.to_string()); + if let Err(error) = std::fs::remove_dir_all(runtime_dir) + && error.kind() != std::io::ErrorKind::NotFound + { + return DeleteVMReply { + error: Some(format!("failed to retry VM runtime cleanup: {error}")), + }; + } + } + // don't fall back to release_stopped when we have a token. someone else + // might've grabbed a new lease since we marked it (or since an earlier + // delete failed), so removal has to check this exact token under the lock + match release_cleanup_lease(&allocator, vmid, proof.as_deref()) { + Ok(had_cid_lease) if proof.is_some() && !exit_confirmed && !had_cid_lease => DeleteVMReply { + error: Some("VM teardown failed and its VMM exit can't be confirmed; refusing to unblock its ID".to_owned()), + }, + Ok(_) => { + self.blocked_vm_ids.remove(&vmid); + self.remove_vm(vmid); + DeleteVMReply { error: None } + } + Err(error) => { + warn!(vm_id = %vmid, ?error, "failed to release stopped VM CID"); + DeleteVMReply { + error: Some(format!("failed to release stopped VM CID: {error}")), + } + } + } + } + async fn lookup_vm_actor(vmid: Ulid) -> Option> { ActorRef::::lookup(format!("vm:{vmid}")) .await @@ -199,6 +349,43 @@ impl Actor for AgentActor { Ok(ControlFlow::Break(ActorStopReason::Panicked(err))) } + async fn on_stop( + &mut self, + _actor_ref: WeakActorRef, + reason: ActorStopReason, + ) -> Result<()> { + // stop the VMs ourselves so each one reaps its VMM and marks its lease + // stopped. otherwise a restart leaves every lease looking active + let actors: Vec<_> = self.vms.values().map(|vm| vm.actor_ref.clone()).collect(); + if actors.is_empty() { + return Ok(()); + } + info!( + count = actors.len(), + ?reason, + "stopping VMs before the agent exits" + ); + let mut stops = tokio::task::JoinSet::new(); + for actor in actors { + stops.spawn(async move { + if actor.is_alive() + && let Err(error) = actor.stop_gracefully().await + { + warn!(?error, "failed to ask VM actor to stop"); + } + actor.wait_for_shutdown().await; + }); + } + let all_stopped = async { while stops.join_next().await.is_some() {} }; + if tokio::time::timeout(SHUTDOWN_VM_TIMEOUT, all_stopped) + .await + .is_err() + { + warn!("timed out stopping VMs, their vsock leases stay active until they're deleted"); + } + Ok(()) + } + async fn on_link_died( &mut self, _actor_ref: WeakActorRef, @@ -215,7 +402,7 @@ impl Actor for AgentActor { .collect(); for vmid in removed { // link death shows up before the child's on_stop finishes. - // Keep the ID taken until teardown is done, or a new VM could race us + // keep the ID taken until teardown is done, or a new VM could race us // while we're still deleting its runtime files and CID lease let Some(actor_ref) = self.vms.get(&vmid).map(|vm| vm.actor_ref.clone()) else { continue; @@ -225,12 +412,19 @@ impl Actor for AgentActor { result.map(|_| ()).map_err(|error| error.to_string()) }) .await; + let proof = Arc::clone(&self.vms[&vmid].cleanup_proof); if let Err(error) = shutdown_result { warn!(?vmid, %error, "VM teardown failed; blocking ID reuse until explicit deletion"); - let proof = Arc::clone(&self.vms[&vmid].cleanup_proof); - self.blocked_vm_ids.insert(vmid, proof); + self.quarantine(vmid, proof); + } else if !proof.process_exit_confirmed() { + warn!( + ?vmid, + "VM stopped without a confirmed exit; blocking ID reuse until explicit deletion" + ); + self.quarantine(vmid, proof); + } else { + self.remove_vm(vmid); } - self.remove_vm(vmid); } Ok(ControlFlow::Continue(())) @@ -263,7 +457,7 @@ impl Message for AgentActor { error: Some("conflicting create request for existing VM ID".to_owned()), }; } - // a failed teardown can leave a dead actor in the cache. Make sure + // a failed teardown can leave a dead actor in the cache. make sure // the VM is actually alive before treating the retry as idempotent, // so we don't claim a stopped VM still exists if let Err(error) = existing.actor_ref.ask(GetVMInfo { vmid: Some(vmid) }).await { @@ -291,7 +485,7 @@ impl Message for AgentActor { }; } - // spawn and link at the same time. Hold onto the exit and lease info + // spawn and link at the same time. hold onto the exit and lease info // even if startup never gives us a usable actor let cleanup_proof = Arc::new(VMCleanupProof::new(true)); let actor_ref = VMActor::spawn_link( @@ -300,7 +494,7 @@ impl Message for AgentActor { ) .await; - // wait for on_start to finish before we say the VM was created. That way + // wait for on_start to finish before we say the VM was created. that way // conversion failures (like vsock CID collisions) go back to the // scheduler instead of us caching a dead VM actor as a success let effective_config = match actor_ref.ask(GetVMInfo { vmid: Some(vmid) }).await { @@ -312,16 +506,7 @@ impl Message for AgentActor { result.map(|_| ()).map_err(|error| error.to_string()) }) .await; - let startup_cleanup_failed = actor_ref - .with_startup_result(|result| startup_cleanup_failed(&result)) - .unwrap_or(false); - if startup_cleanup_failed - || !cleanup_proof.process_exit_confirmed() - || shutdown_result.is_err() - && actor_ref - .with_startup_result(|result| result.is_ok()) - .unwrap_or(false) - { + if needs_quarantine(&actor_ref, shutdown_result.is_err(), &cleanup_proof) { self.blocked_vm_ids.insert(vmid, Arc::clone(&cleanup_proof)); } return CreateVMReply { @@ -344,6 +529,7 @@ impl Message for AgentActor { vcpus: msg.config.desired.compute.vcpus, memory_bytes: msg.config.desired.compute.memory_bytes, cleanup_proof: Arc::clone(&cleanup_proof), + generation: msg.generation, }, ); @@ -394,6 +580,8 @@ impl Message for AgentActor { vcpus: msg.config.desired.compute.vcpus, memory_bytes: msg.config.desired.compute.memory_bytes, cleanup_proof: Arc::clone(&cleanup_proof), + // a migrated VM has no create stamp, so any delete still applies + generation: None, }, ); @@ -410,10 +598,11 @@ impl Message for AgentActor { let failed = actor_ref .wait_for_shutdown_with_result(|result| result.is_err()) .await; - if failed || !cleanup_proof.process_exit_confirmed() { - self.blocked_vm_ids.insert(vmid, Arc::clone(&cleanup_proof)); + if needs_quarantine(&actor_ref, failed, &cleanup_proof) { + self.quarantine(vmid, Arc::clone(&cleanup_proof)); + } else { + self.remove_vm(vmid); } - self.remove_vm(vmid); } reply @@ -429,99 +618,61 @@ impl Message for AgentActor { msg: DeleteVM, _ctx: &mut Context, ) -> Self::Reply { - let cached_actor = self - .vms - .get(&msg.vmid) - .map(|cache_data| cache_data.actor_ref.clone()); - let actor = match cached_actor { - Some(actor) => Some(actor), - None => Self::lookup_vm_actor(msg.vmid).await, - }; - - if let Some(actor) = actor { - if let Err(error) = actor.ask(msg.clone()).await { - warn!(vm_id = %msg.vmid, ?error, "failed to stop VM actor for delete"); - return DeleteVMReply { - error: Some(format!("failed to stop VM actor: {error}")), - }; - } - // don't ack the delete (or let it get recreated) until on_stop has - // reaped the old process and finished releasing its CID - let shutdown_result = actor - .wait_for_shutdown_with_result(|result| { - result.map(|_| ()).map_err(|error| error.to_string()) - }) - .await; - if let Err(error) = shutdown_result { - let proof = self.vms.get(&msg.vmid).map_or_else( - || Arc::new(VMCleanupProof::new(false)), - |vm| Arc::clone(&vm.cleanup_proof), - ); - self.blocked_vm_ids.insert(msg.vmid, proof); - self.remove_vm(msg.vmid); - return DeleteVMReply { - error: Some(format!("VM teardown failed: {error}")), - }; - } - self.remove_vm(msg.vmid); - self.blocked_vm_ids.remove(&msg.vmid); + let vmid = msg.vmid; + // the ID got recreated after this delete was sent, so whatever it was + // meant for is already gone. a VM only exists once per agent + if let Some(vm) = self.vms.get(&vmid) + && msg.predates(vm.generation) + { + info!(%vmid, ?msg.issued_at, generation = ?vm.generation, "ignoring a delete sent before this VM was created"); return DeleteVMReply { error: None }; } + // a blocked VM's actor is already gone, even if we kept it listed + // because its exit couldn't be confirmed + if !self.blocked_vm_ids.contains_key(&vmid) { + let cached_actor = self + .vms + .get(&vmid) + .map(|cache_data| cache_data.actor_ref.clone()); + let actor = match cached_actor { + Some(actor) => Some(actor), + None => Self::lookup_vm_actor(vmid).await, + }; - // shutdown removes the VM actor on purpose but keeps its node-local CID. - // Only release it if the VM actor saw the process actually exit before - // it went away - let proof = self.blocked_vm_ids.get(&msg.vmid); - let exit_unconfirmed = proof.is_some_and(|proof| !proof.process_exit_confirmed()); - let allocator = VsockCidAllocator::from_environment(); - if let Some(proof) = proof.filter(|proof| proof.process_exit_confirmed()) { - if let Err(error) = persist_cleanup_exit(&allocator, msg.vmid, proof) { - return DeleteVMReply { - error: Some(format!( - "failed to persist confirmed VM process exit: {error}" - )), - }; - } - // leftover storage leases are process-global, so we can still retry - // after the failed VM actor/transformer is gone. Don't detach - // anything until we know for sure the VMM exited - if let Err(error) = StorageDriverTransformer::default().teardown( - &msg.vmid.to_string(), - &mut cloud_hypervisor_client::models::VmConfig::default(), - ) { - return DeleteVMReply { - error: Some(format!("failed to retry VM storage cleanup: {error}")), - }; - } - // the old process exited but runtime cleanup failed. Retry it before - // we ack the delete and let the paths get reused - let runtime_dir = crate::ch_driver::VMInstance::runtime_dir_for(&msg.vmid.to_string()); - if let Err(error) = std::fs::remove_dir_all(runtime_dir) - && error.kind() != std::io::ErrorKind::NotFound - { - return DeleteVMReply { - error: Some(format!("failed to retry VM runtime cleanup: {error}")), + if let Some(actor) = actor { + if let Err(error) = actor.ask(msg.clone()).await { + warn!(vm_id = %vmid, ?error, "failed to stop VM actor for delete"); + return DeleteVMReply { + error: Some(format!("failed to stop VM actor: {error}")), + }; + } + // don't ack the delete (or let it get recreated) until on_stop has + // reaped the old process and finished releasing its CID + let shutdown_result = actor + .wait_for_shutdown_with_result(|result| { + result.map(|_| ()).map_err(|error| error.to_string()) + }) + .await; + let proof = self.vms.get(&vmid).map(|vm| Arc::clone(&vm.cleanup_proof)); + let error = match (&shutdown_result, &proof) { + (Err(error), _) => Some(format!("VM teardown failed: {error}")), + (Ok(()), Some(proof)) if !proof.process_exit_confirmed() => { + Some("VM stopped but its VMM exit wasn't confirmed".to_owned()) + } + (Ok(()), _) => None, }; - } - } - // don't fall back to release_stopped when we have a token. Someone else - // might've grabbed a new lease since we marked it (or since an earlier - // delete failed), so removal has to check this exact token under the lock - match release_cleanup_lease(&allocator, msg.vmid, proof.map(Arc::as_ref)) { - Ok(had_cid_lease) if exit_unconfirmed && !had_cid_lease => DeleteVMReply { - error: Some("VM teardown failed and no confirmed VMM-exit record exists; refusing to unblock its ID".to_owned()), - }, - Ok(_) => { - self.blocked_vm_ids.remove(&msg.vmid); - DeleteVMReply { error: None } - } - Err(error) => { - warn!(vm_id = %msg.vmid, ?error, "failed to release stopped VM CID"); - DeleteVMReply { - error: Some(format!("failed to release stopped VM CID: {error}")), + if let Some(error) = error { + let proof = proof.unwrap_or_else(|| Arc::new(VMCleanupProof::new(false))); + self.quarantine(vmid, proof); + return DeleteVMReply { error: Some(error) }; } + self.remove_vm(vmid); + self.blocked_vm_ids.remove(&vmid); + return DeleteVMReply { error: None }; } } + + self.retry_detached_cleanup(vmid).await } } @@ -648,6 +799,7 @@ impl Message for AgentActor { vms.sort_unstable(); vms }, + retained_vms: self.retained_vms(), used_vcpus, used_ram, metadata: self.metadata.clone(), @@ -689,6 +841,7 @@ impl Message for AgentActor { revision: self.membership_revision, added, removed, + retained_vms: self.retained_vms(), used_vcpus, used_ram, } @@ -697,7 +850,9 @@ impl Message for AgentActor { #[cfg(test)] mod startup_cleanup_tests { - use super::{persist_cleanup_exit, release_cleanup_lease, startup_cleanup_failed}; + use super::{ + persist_cleanup_exit, release_cleanup_lease, startup_cleanup_failed, stop_leftover_vmm, + }; use crate::ch_driver::actor::FailedStartupCleanup; use kameo::error::HookError; use stable_eyre::Report; @@ -770,4 +925,33 @@ mod startup_cleanup_tests { )))); assert!(!startup_cleanup_failed(&Ok(()))); } + + #[tokio::test] + async fn delete_after_restart_kills_the_orphaned_vmm_and_frees_its_lease() { + use crate::{vmm_process::ProcessIdentity, vsock_cid::VsockCidAllocator}; + let directory = + std::env::temp_dir().join(format!("odorobo-orphan-vmm-{}", ulid::Ulid::generate())); + let allocator = VsockCidAllocator::new(directory.join("registry.json")); + let vmid = ulid::Ulid::generate(); + let reservation = allocator.reserve_with_previous(vmid, Some(42)).unwrap(); + let mut vmm = tokio::process::Command::new("sleep") + .arg("30") + .kill_on_drop(true) + .spawn() + .unwrap(); + let process = ProcessIdentity::of(vmm.id().unwrap()).unwrap(); + allocator + .record_process_owned(vmid, reservation.token, process) + .unwrap(); + + // no proof, like an agent that restarted while the VMM kept running + assert!(stop_leftover_vmm(&allocator, vmid, None).await.unwrap()); + assert!(!process.is_running().unwrap()); + assert!(release_cleanup_lease(&allocator, vmid, None).unwrap()); + drop(vmm.wait().await); + + // nothing recorded means nothing to kill + assert!(!stop_leftover_vmm(&allocator, vmid, None).await.unwrap()); + std::fs::remove_dir_all(directory).unwrap(); + } } diff --git a/odorobo/src/actors/scheduler_actor.rs b/odorobo/src/actors/scheduler_actor.rs index 18726f7..79eee8f 100644 --- a/odorobo/src/actors/scheduler_actor.rs +++ b/odorobo/src/actors/scheduler_actor.rs @@ -16,6 +16,7 @@ //! discovery/polling, scheduling policy, and actor message handlers. mod cache; +mod deletion; mod discovery; mod handlers; mod scheduling; @@ -86,6 +87,14 @@ struct AgentUpdaterStopped { #[derive(Debug)] struct ReconcileVmPlacements; +/// a background delete for a VM we already deleted came back from an agent +#[derive(Debug)] +struct AutoDeleteFinished { + vmid: Ulid, + agent_id: ActorId, + error: Option, +} + /// The latest scheduler-approved status snapshot for an agent. /// /// The first accepted update must be a full snapshot. Later updates are @@ -129,9 +138,9 @@ pub struct CachedVMActor { pub actor_ref: Option>, } -/// who's on the hook for cleaning up a VM on a node. This sticks around through -/// actor replacement and cache eviction. A peer tells us which agent handles both -/// VM teardown and CID leases. Without a peer we keep the exact ID instead of +/// who's on the hook for cleaning up a VM on a node. this sticks around through +/// actor replacement and cache eviction. a peer tells us which agent handles both +/// VM teardown and CID leases. without a peer we keep the exact ID instead of /// guessing some other owner #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] enum VmDeleteOwner { @@ -166,6 +175,44 @@ struct VmDeleteTarget { confirmed: bool, } +/// what an explicit stop or delete left behind for a VM ID +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum VmTombstoneState { + /// shut down on purpose. late discovery is ignored, but the node can keep + /// its stopped CID lease since the VM might come back + Stopped, + /// delete was asked for but some owner hasn't acked yet. blocks recreating + /// the ID until a retry works + DeletePending, + /// every owner acked (or got force-dropped). anything an agent still reports + /// for this ID is a leftover and gets deleted in the background + Deleted, +} + +#[derive(Debug, Clone, Copy)] +struct VmTombstone { + state: VmTombstoneState, + /// when the tombstone last changed state, used for pruning + since: Instant, +} + +impl VmTombstone { + fn new(state: VmTombstoneState) -> Self { + Self { + state, + since: Instant::now(), + } + } +} + +/// rate limit for background deletes, one per (VM, agent) +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum AutoDeleteState { + InFlight, + /// the last attempt failed at this time, so wait before trying again + Failed(Instant), +} + /// An eventually consistent, in-memory VM scheduler. /// /// Public caches are exposed for inspection, but correlated maps must be @@ -183,16 +230,28 @@ pub struct SchedulerActor { pub vm_manifests: AHashMap, /// last effective manifest a VM actor reported, including node-local values pub vm_effective_manifests: AHashMap, - /// ignores late discovery after an explicit stop/delete. `true` means delete - /// cleanup is still pending, so recreating has to wait for a retry to succeed - vm_tombstones: AHashMap, + /// ignores late discovery after an explicit stop/delete. `DeletePending` + /// blocks recreating until a retry succeeds. `Stopped` and `Deleted` get + /// pruned after a while during placement maintenance + vm_tombstones: AHashMap, + /// background deletes for `Deleted` VMs that an agent still reports + vm_auto_deletes: AHashMap<(Ulid, ActorId), AutoDeleteState>, + /// nodes that might still run a deleted VM: owners a force delete gave up + /// on, and agents caught still reporting one. while any are left the ID + /// can't be recreated and its `Deleted` tombstone isn't pruned, so when the + /// node comes back its copy gets deleted instead of adopted as the new VM. + /// an entry goes away once that node acks a delete or reports a full status + /// without the VM + vm_leftover_owners: AHashMap>, + /// last stamp handed out by `next_stamp` + last_stamp: Ulid, /// last discovered agent per owner, kept even after reachability-cache eviction. - /// These are just for finding known VM owners, not a list to broadcast to + /// these are just for finding known VM owners, not a list to broadcast to known_agent_refs: AHashMap>, /// nodes that might need to clean up a VM, recorded at create/discovery time - /// and kept through shutdown, migration, and eviction. Confirmed entries stay + /// and kept through shutdown, migration, and eviction. confirmed entries stay /// until every owner acks the delete, so stale discovery can't re-add them. - /// If we don't have a ref yet, the owner stays pending instead of getting lost + /// if we don't have a ref yet, the owner stays pending instead of getting lost vm_delete_targets: AHashMap>, /// Desired and observed VM placements; multiple entries allow migration. pub vm_placements: AHashMap>, diff --git a/odorobo/src/actors/scheduler_actor/cache.rs b/odorobo/src/actors/scheduler_actor/cache.rs index a5363eb..ad257ac 100644 --- a/odorobo/src/actors/scheduler_actor/cache.rs +++ b/odorobo/src/actors/scheduler_actor/cache.rs @@ -13,10 +13,25 @@ use crate::manifest::VmManifest; use super::{ CachedVMActor, SchedulerActor, VmDeleteOwner, VmDeleteTarget, VmLifecycle, VmPlacement, + VmTombstone, VmTombstoneState, }; -// Pending creates are given time to appear in an agent status update. +// Pending creates are given time to appear in an agent status update once +// the agent is done starting them. see `pending_placement_timeout` const UNRESOLVED_VM_CACHE_TIMEOUT: Duration = Duration::from_secs(120); + +/// how long a pending placement waits to show up before it's given up on and +/// the VM gets rescheduled. a create whose reply got lost (the libp2p request +/// times out well before a slow create finishes) is still starting on that +/// agent, so this has to cover its whole startup or the VM ends up running twice +pub(super) fn pending_placement_timeout(manifest: Option<&VmManifest>) -> Duration { + // +1 for the cloud-init seed disk, which the manifest doesn't list + let disks = manifest.map_or(0, |manifest| { + manifest.desired.storage.len().saturating_add(1) + }); + crate::ch_driver::instance::startup_timeout(disks).saturating_add(UNRESOLVED_VM_CACHE_TIMEOUT) +} + // Previously observed VMs are expired promptly when repeated status updates omit them. const UNCONFIRMED_RUNNING_PLACEMENT_TIMEOUT: Duration = Duration::from_secs(30); @@ -60,6 +75,7 @@ impl SchedulerActor { /// the manifest remains scheduler intent, allowing periodic reconciliation to /// dispatch a replacement create request. pub(super) fn cleanup_unresolved_vm_cache( + manifests: &AHashMap, placements: &mut AHashMap>, data_cache: &mut AHashMap>, ) { @@ -76,10 +92,11 @@ impl SchedulerActor { let empty_vmids: Vec<_> = placements .iter_mut() .filter_map(|(vmid, entries)| { + let timeout = pending_placement_timeout(manifests.get(vmid)); entries.retain(|entry| { entry.lifecycle != VmLifecycle::Pending || discovered_vmids.contains(vmid) - || now.duration_since(entry.created_at) < UNRESOLVED_VM_CACHE_TIMEOUT + || now.duration_since(entry.created_at) < timeout }); Self::shrink_non_migrating_entries(entries); entries.is_empty().then_some(*vmid) @@ -155,7 +172,7 @@ impl SchedulerActor { } /// remembers a node that might still have a VM process or stopped CID lease. - /// A VM dropping out of status or a placement expiring doesn't mean the CID + /// a VM dropping out of status or a placement expiring doesn't mean the CID /// was released pub(super) fn remember_vm_delete_owner(&mut self, vmid: Ulid, owner: VmDeleteOwner) { let agent = self.known_agent_refs.get(&owner).cloned(); @@ -192,10 +209,10 @@ impl SchedulerActor { } /// grabs every possible owner before stop/delete or eviction wipes that info. - /// Old per-VM owners stick around even once status stops reporting the VM + /// old per-VM owners stick around even once status stops reporting the VM pub(super) fn capture_vm_delete_owners(&mut self, vmid: Ulid) { if self.vm_tombstones.contains_key(&vmid) { - // we already captured everything. Only explicit late discovery can + // we already captured everything. only explicit late discovery can // add owners now, not stale cache/index data return; } @@ -243,7 +260,7 @@ impl SchedulerActor { } /// only counts a create retry as confirmed if we recorded the agent accepting - /// it or discovered the actor. A pending placeholder on its own could be a + /// it or discovered the actor. a pending placeholder on its own could be a /// request where we don't know if it ever made it pub(super) fn create_is_confirmed(&self, vmid: Ulid) -> bool { self.vm_effective_manifests.contains_key(&vmid) @@ -263,7 +280,7 @@ impl SchedulerActor { } /// sorts out cleanup owners after the agent explicitly rejected a create and - /// a lookup found no VM actor. If cleanup wasn't acked, it stays a delete + /// a lookup found no VM actor. if cleanup wasn't acked, it stays a delete /// target and blocks recreating the VM until a later delete retry works pub(super) fn settle_failed_create_cleanup( &mut self, @@ -273,7 +290,7 @@ impl SchedulerActor { cleanup_confirmed: bool, ) { if !cleanup_confirmed { - self.vm_tombstones.insert(vmid, true); + self.set_vm_tombstone(vmid, VmTombstoneState::DeletePending); return; } @@ -289,11 +306,11 @@ impl SchedulerActor { } if !self.finish_vm_delete(vmid) && self.vm_delete_targets.contains_key(&vmid) { - self.vm_tombstones.insert(vmid, true); + self.set_vm_tombstone(vmid, VmTombstoneState::DeletePending); } } - /// only marks an owner that actually acked. Failures and missing routes stay + /// only marks an owner that actually acked. failures and missing routes stay /// pending across retries, no matter what other agents reply pub(super) fn acknowledge_vm_delete_owner(&mut self, vmid: Ulid, owner: VmDeleteOwner) { if let Some(target) = self @@ -311,7 +328,7 @@ impl SchedulerActor { !targets.is_empty() && targets.values().all(|target| target.confirmed) }) { self.vm_delete_targets.remove(&vmid); - self.vm_tombstones.insert(vmid, false); + self.set_vm_tombstone(vmid, VmTombstoneState::Deleted); true } else { false @@ -319,11 +336,15 @@ impl SchedulerActor { } /// Removes VM intent and every actor-ID association so reconciliation cannot - /// recreate a VM after an explicit stop or delete request. We still keep - /// the cleanup owners around on purpose, including for shutdown's stopped CID leases + /// recreate a VM after an explicit stop or delete request. we still keep + /// the cleanup owners around on purpose, including for shutdown's stopped CID leases. + /// a fresh tombstone is `Stopped`, never `Deleted`, so a stopped VM's lease + /// is never deleted in the background. delete upgrades it afterwards pub(super) fn remove_vm_intent(&mut self, vmid: Ulid) { self.capture_vm_delete_owners(vmid); - self.vm_tombstones.entry(vmid).or_insert(false); + self.vm_tombstones + .entry(vmid) + .or_insert_with(|| VmTombstone::new(VmTombstoneState::Stopped)); Self::remove_vm_state( vmid, &mut self.vm_manifests, @@ -575,6 +596,7 @@ impl SchedulerActor { let empty_vmids: Vec<_> = placements .iter_mut() .filter_map(|(vmid, entries)| { + let pending_timeout = pending_placement_timeout(manifests.get(vmid)); entries.retain_mut(|entry| { if entry.agent_id != agent_id { return true; @@ -588,7 +610,7 @@ impl SchedulerActor { match entry.lifecycle { VmLifecycle::Pending => { - now.duration_since(entry.created_at) < UNRESOLVED_VM_CACHE_TIMEOUT + now.duration_since(entry.created_at) < pending_timeout } VmLifecycle::Running => { entry.last_confirmed_at.is_some_and(|last_confirmed_at| { diff --git a/odorobo/src/actors/scheduler_actor/deletion.rs b/odorobo/src/actors/scheduler_actor/deletion.rs new file mode 100644 index 0000000..d0ce509 --- /dev/null +++ b/odorobo/src/actors/scheduler_actor/deletion.rs @@ -0,0 +1,497 @@ +//! delete bookkeeping that doesn't need the network: tombstones, deciding what +//! to do with each cleanup owner's reply, background deletes, and pruning + +use std::time::{Duration, Instant}; + +use ahash::AHashSet; + +use kameo::error::RemoteSendError; +use kameo::prelude::*; +use tracing::warn; +use ulid::Ulid; + +use crate::messages::agent::{AgentStatus, AgentStatusUpdate}; + +use super::{AutoDeleteState, SchedulerActor, VmDeleteOwner, VmTombstone, VmTombstoneState}; + +/// stopped VMs only need their tombstone long enough to ignore late discovery +pub(super) const STOPPED_TOMBSTONE_TTL: Duration = Duration::from_hours(1); +/// deleted VMs keep theirs longer so a node that comes back late still gets +/// its leftovers cleaned up in the background +pub(super) const DELETED_TOMBSTONE_TTL: Duration = Duration::from_hours(24); +/// how long to wait before retrying a background delete that failed +pub(super) const AUTO_DELETE_RETRY_INTERVAL: Duration = Duration::from_secs(30); +/// a background delete that takes longer than this counts as failed +pub(super) const AUTO_DELETE_TIMEOUT: Duration = Duration::from_mins(5); + +/// one route's answer when we asked an owner to delete a VM +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) enum DeleteAttempt { + Acked, + /// the agent is alive and explicitly said cleanup failed + AgentError(String), + /// the request never got to a running agent: no route, dial failure, no + /// such actor. the only kind of failure force is allowed to skip + Unreachable(String), + /// the agent might have gotten the request and might still be working on + /// it, like a timeout or a connection that closed mid-request. a slow + /// teardown looks exactly like this, so even force can't skip it + Unknown(String), +} + +impl DeleteAttempt { + /// sorts a failed ask by whether the agent could have seen the request + pub(super) fn from_send_error(error: &RemoteSendError) -> Self { + let message = format!("{error:?}"); + match error { + RemoteSendError::ActorNotRunning + | RemoteSendError::UnknownActor { .. } + | RemoteSendError::BadActorType + | RemoteSendError::SwarmNotBootstrapped + | RemoteSendError::DialFailure + | RemoteSendError::UnsupportedProtocols + | RemoteSendError::SerializeMessage(_) => Self::Unreachable(message), + // everything else either reached a live agent or might have + RemoteSendError::ActorStopped + | RemoteSendError::UnknownMessage { .. } + | RemoteSendError::MailboxFull + | RemoteSendError::ReplyTimeout + | RemoteSendError::HandlerError(_) + | RemoteSendError::DeserializeMessage(_) + | RemoteSendError::SerializeReply(_) + | RemoteSendError::SerializeHandlerError(_) + | RemoteSendError::DeserializeHandlerError(_) + | RemoteSendError::NetworkTimeout + | RemoteSendError::ConnectionClosed + | RemoteSendError::Io(_) => Self::Unknown(message), + } + } +} + +/// what all of an owner's routes added up to +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) enum DeleteOwnerOutcome { + Acked, + /// at least one live agent rejected the delete. even force can't skip this + Rejected(Vec), + /// no ack or rejection, but the agent might still be working on it. even + /// force can't skip this, a retry finds out how it went + Unknown(Vec), + /// nothing answered. empty means we didn't even have a route + Unreachable(Vec), +} + +/// what to do with every owner after one delete pass +#[derive(Debug, Default, PartialEq, Eq)] +pub(super) struct DeleteSettlement { + pub acked: Vec, + /// owners force let us give up on, with why + pub dropped: Vec<(VmDeleteOwner, String)>, + pub failures: Vec, +} + +/// folds an owner's per-route attempts into one outcome. any ack wins, then +/// any explicit agent error, then any route that might have gotten through, +/// since a stale route failing doesn't matter if a live replacement on the same +/// peer answered +pub(super) fn delete_owner_outcome(attempts: Vec) -> DeleteOwnerOutcome { + let mut rejected = Vec::new(); + let mut unknown = Vec::new(); + let mut unreachable = Vec::new(); + for attempt in attempts { + match attempt { + DeleteAttempt::Acked => return DeleteOwnerOutcome::Acked, + DeleteAttempt::AgentError(error) => rejected.push(error), + DeleteAttempt::Unknown(error) => unknown.push(error), + DeleteAttempt::Unreachable(error) => unreachable.push(error), + } + } + if !rejected.is_empty() { + DeleteOwnerOutcome::Rejected(rejected) + } else if !unknown.is_empty() { + DeleteOwnerOutcome::Unknown(unknown) + } else { + DeleteOwnerOutcome::Unreachable(unreachable) + } +} + +/// decides which owners count as done. without force, anything short of an +/// ack fails the delete. with force, owners we couldn't reach at all get +/// dropped, but an agent that answered with an error, or might still be +/// working on it, still fails it +pub(super) fn plan_delete_settlement( + outcomes: Vec<(VmDeleteOwner, DeleteOwnerOutcome)>, + force: bool, +) -> DeleteSettlement { + let mut settlement = DeleteSettlement::default(); + for (owner, outcome) in outcomes { + match outcome { + DeleteOwnerOutcome::Acked => settlement.acked.push(owner), + DeleteOwnerOutcome::Rejected(errors) => settlement + .failures + .push(format!("{owner:?}: {}", errors.join("; "))), + DeleteOwnerOutcome::Unknown(errors) => settlement.failures.push(format!( + "{owner:?}: no answer, it might still be deleting; retry: {}", + errors.join("; ") + )), + DeleteOwnerOutcome::Unreachable(errors) => { + let reason = if errors.is_empty() { + "missing agent for known VM owner".to_owned() + } else { + errors.join("; ") + }; + if force { + settlement.dropped.push((owner, reason)); + } else { + settlement.failures.push(format!("{owner:?}: {reason}")); + } + } + } + } + settlement +} + +/// whether a same-revision delta just refreshes usage and `retained_vms`. +/// those can change without a membership revision bump +pub(super) fn is_refresh_only_update( + update: &AgentStatusUpdate, + cached_revision: Option, +) -> bool { + matches!( + update, + AgentStatusUpdate::Delta { revision, added, removed, .. } + if cached_revision == Some(*revision) && added.is_empty() && removed.is_empty() + ) +} + +/// VM IDs an accepted status update says this agent holds something for, or +/// `None` if the update is stale and gets ignored. only snapshots, additions, +/// and `retained_vms` count. a removal doesn't tell us anything about a +/// stopped node-local CID lease +pub(super) fn status_update_held_vmids( + update: &AgentStatusUpdate, + cached_revision: Option, +) -> Option> { + match update { + AgentStatusUpdate::Full { revision, status } + if cached_revision.is_none_or(|cached| *revision > cached) => + { + Some( + status + .vms + .iter() + .chain(&status.retained_vms) + .copied() + .collect(), + ) + } + AgentStatusUpdate::Delta { + revision, + added, + retained_vms, + .. + } if is_refresh_only_update(update, cached_revision) + || cached_revision.is_some_and(|cached| *revision > cached) => + { + Some(added.iter().chain(retained_vms).copied().collect()) + } + AgentStatusUpdate::Full { .. } | AgentStatusUpdate::Delta { .. } => None, + } +} + +/// refuses a create for a VM ID some agent says it's running. that's a VM we +/// haven't discovered yet (right after a scheduler restart) or one whose exit +/// couldn't be confirmed. scheduling it somewhere else would run two copies on +/// the same storage. a retained stopped lease is fine, that's how a stopped VM +/// gets started again +pub(super) fn check_not_running_elsewhere<'a, I>(vmid: Ulid, agents: I) -> Result<(), String> +where + I: IntoIterator, +{ + let reporting: Vec<_> = agents + .into_iter() + .filter(|(_, status)| status.vms.contains(&vmid)) + .map(|(agent_id, _)| agent_id) + .collect(); + if reporting.is_empty() { + return Ok(()); + } + Err(format!( + "agent(s) {reporting:?} still report a running VM with this ID that the scheduler \ + hasn't discovered; retry shortly, or delete it before recreating" + )) +} + +impl SchedulerActor { + /// records the agent as a cleanup owner for every VM its status says it + /// holds. this is how owner history comes back after a scheduler restart + pub(super) fn record_status_delete_owners( + &mut self, + agent_id: ActorId, + vmids: impl IntoIterator, + ) { + for vmid in vmids { + if self.should_record_delete_owner(vmid) { + self.remember_vm_delete_owner(vmid, VmDeleteOwner::agent(agent_id)); + } + } + } + + pub(super) fn vm_tombstone_state(&self, vmid: Ulid) -> Option { + self.vm_tombstones + .get(&vmid) + .map(|tombstone| tombstone.state) + } + + pub(super) fn set_vm_tombstone(&mut self, vmid: Ulid, state: VmTombstoneState) { + self.vm_tombstones.insert(vmid, VmTombstone::new(state)); + } + + /// stopped or fully deleted, so late discovery shouldn't bring the VM back + /// or re-add cleanup owners + pub(super) fn vm_is_stopped_or_deleted(&self, vmid: Ulid) -> bool { + matches!( + self.vm_tombstone_state(vmid), + Some(VmTombstoneState::Stopped | VmTombstoneState::Deleted) + ) + } + + /// whether agent status or late VM discovery should count as a cleanup + /// owner. a live or delete-pending VM always does, a stopped/deleted one + /// only if we're still tracking its owners + pub(super) fn should_record_delete_owner(&self, vmid: Ulid) -> bool { + !self.vm_is_stopped_or_deleted(vmid) || self.vm_delete_targets.contains_key(&vmid) + } + + /// forgets an owner force let us give up on. the caller records it as a + /// leftover owner, so if it comes back and still reports the VM, the status + /// handler deletes it in the background + pub(super) fn drop_vm_delete_owner(&mut self, vmid: Ulid, owner: VmDeleteOwner) { + if let Some(targets) = self.vm_delete_targets.get_mut(&vmid) { + targets.remove(&owner); + } + } + + /// a time-ordered stamp for a create or delete we send to an agent. always + /// moves forward, even if two land in the same millisecond or the clock + /// steps back + pub(super) fn next_stamp(&mut self) -> Ulid { + let now = Ulid::generate(); + self.last_stamp = if now > self.last_stamp { + now + } else { + self.last_stamp.increment().unwrap_or(now) + }; + self.last_stamp + } + + /// like `finish_vm_delete`, but also finishes when force dropped every + /// owner or there never were any + pub(super) fn force_finish_vm_delete(&mut self, vmid: Ulid) -> bool { + if self + .vm_delete_targets + .get(&vmid) + .is_some_and(|targets| targets.values().any(|target| !target.confirmed)) + { + return false; + } + self.vm_delete_targets.remove(&vmid); + self.set_vm_tombstone(vmid, VmTombstoneState::Deleted); + true + } + + /// records acks and drops from one delete pass, then finishes the delete if + /// nothing failed. acks stick even when another owner failed + pub(super) fn apply_delete_settlement( + &mut self, + vmid: Ulid, + settlement: DeleteSettlement, + force: bool, + ) -> Result<(), String> { + for owner in settlement.acked { + self.acknowledge_vm_delete_owner(vmid, owner); + self.forget_leftover_owner(vmid, owner); + } + for (owner, reason) in settlement.dropped { + warn!( + %vmid, + ?owner, + %reason, + "force delete is dropping an unreachable cleanup owner, its node-local state might leak" + ); + self.drop_vm_delete_owner(vmid, owner); + // it might still be running the VM. remember it so recreating the + // ID waits for that copy to get cleaned up + self.vm_leftover_owners + .entry(vmid) + .or_default() + .insert(owner); + } + if !settlement.failures.is_empty() { + return Err(format!( + "failed to release VM CID on one or more agents: {}", + settlement.failures.join("; ") + )); + } + let finished = if force { + self.force_finish_vm_delete(vmid) + } else { + self.finish_vm_delete(vmid) + }; + if finished { + Ok(()) + } else { + Err("no agents can confirm VM teardown and CID release".to_owned()) + } + } + + /// picks VMs an agent still reports that we already finished deleting, and + /// marks each one in flight. at most one per (VM, agent) at a time, and a + /// failed one waits `AUTO_DELETE_RETRY_INTERVAL` before going again + pub(super) fn take_auto_delete_vmids( + &mut self, + agent_id: ActorId, + reported: impl IntoIterator, + now: Instant, + ) -> Vec { + let mut vmids = Vec::new(); + for vmid in reported { + if self.vm_tombstone_state(vmid) != Some(VmTombstoneState::Deleted) { + continue; + } + // it still has a copy. block recreating the ID until that's gone, + // even if this background delete fails or times out + self.vm_leftover_owners + .entry(vmid) + .or_default() + .insert(VmDeleteOwner::agent(agent_id)); + let ready = match self.vm_auto_deletes.get(&(vmid, agent_id)) { + None => true, + Some(AutoDeleteState::InFlight) => false, + Some(AutoDeleteState::Failed(at)) => { + now.saturating_duration_since(*at) >= AUTO_DELETE_RETRY_INTERVAL + } + }; + if ready { + self.vm_auto_deletes + .insert((vmid, agent_id), AutoDeleteState::InFlight); + vmids.push(vmid); + } + } + vmids + } + + /// clears the in-flight marker. failures stay around so the next status + /// report after the retry interval tries again + pub(super) fn settle_auto_delete( + &mut self, + vmid: Ulid, + agent_id: ActorId, + error: Option, + now: Instant, + ) { + if let Some(error) = error { + warn!(%vmid, ?agent_id, %error, "background delete of a deleted VM failed, will retry"); + self.vm_auto_deletes + .insert((vmid, agent_id), AutoDeleteState::Failed(now)); + } else { + self.vm_auto_deletes.remove(&(vmid, agent_id)); + self.forget_leftover_owner(vmid, VmDeleteOwner::agent(agent_id)); + } + } + + /// clears a leftover owner once it's known not to hold the VM anymore + pub(super) fn forget_leftover_owner(&mut self, vmid: Ulid, owner: VmDeleteOwner) { + if let Some(owners) = self.vm_leftover_owners.get_mut(&vmid) { + owners.remove(&owner); + if owners.is_empty() { + self.vm_leftover_owners.remove(&vmid); + } + } + } + + /// clears this agent as a leftover owner of every VM its full status no + /// longer lists as running or retained. that's how a node that came back + /// without the VM (say its VMMs died with it) stops blocking the ID + pub(super) fn forget_unreported_leftovers( + &mut self, + agent_id: ActorId, + reported: &AHashSet, + ) { + self.vm_leftover_owners.retain(|vmid, owners| { + if !reported.contains(vmid) { + owners.retain(|owner| !owner.matches_agent(agent_id)); + } + !owners.is_empty() + }); + } + + /// refuses a create for a VM ID some agent says it's running when we have + /// no intent for it. see [`check_not_running_elsewhere`] + pub(super) fn check_not_running_elsewhere(&self, vmid: Ulid) -> Result<(), String> { + check_not_running_elsewhere( + vmid, + self.agent_data_cache + .iter() + .map(|(agent_id, agent)| (*agent_id, &agent.data)), + ) + } + + /// refuses to recreate a VM ID while something from its last life could + /// still be around + pub(super) fn check_create_allowed(&self, vmid: Ulid) -> Result<(), String> { + if self.vm_tombstone_state(vmid) == Some(VmTombstoneState::DeletePending) { + return Err( + "VM deletion cleanup is pending; retry deletion before recreating it".to_owned(), + ); + } + // a background delete of the old VM could land after the new one starts + if self.auto_delete_in_flight(vmid) { + return Err( + "leftover cleanup for a deleted VM with this ID is still running; retry shortly" + .to_owned(), + ); + } + // a node force delete gave up on might still run the old VM on the same + // storage. recreating now would run two copies + if let Some(owners) = self.vm_leftover_owners.get(&vmid) { + return Err(format!( + "{} node(s) may still be running a deleted VM with this ID ({owners:?}); \ + it can be recreated once they come back and get cleaned up, or use a new VM ID", + owners.len() + )); + } + Ok(()) + } + + /// whether a background delete for this VM could still be running on some agent + pub(super) fn auto_delete_in_flight(&self, vmid: Ulid) -> bool { + self.vm_auto_deletes + .iter() + .any(|((id, _), state)| *id == vmid && *state == AutoDeleteState::InFlight) + } + + /// drops old `Stopped`/`Deleted` tombstones and expired retry markers. + /// `DeletePending`, anything we still track owners for, and deleted VMs a + /// node might still be running are kept no matter how old + pub(super) fn prune_vm_tombstones(&mut self, now: Instant) { + let delete_targets = &self.vm_delete_targets; + let leftover_owners = &self.vm_leftover_owners; + self.vm_tombstones.retain(|vmid, tombstone| { + let ttl = match tombstone.state { + VmTombstoneState::DeletePending => return true, + VmTombstoneState::Stopped => STOPPED_TOMBSTONE_TTL, + VmTombstoneState::Deleted => DELETED_TOMBSTONE_TTL, + }; + delete_targets.contains_key(vmid) + || leftover_owners.contains_key(vmid) + || now.saturating_duration_since(tombstone.since) < ttl + }); + self.vm_auto_deletes.retain(|_, state| match state { + AutoDeleteState::InFlight => true, + AutoDeleteState::Failed(at) => { + now.saturating_duration_since(*at) < AUTO_DELETE_RETRY_INTERVAL + } + }); + } +} diff --git a/odorobo/src/actors/scheduler_actor/discovery.rs b/odorobo/src/actors/scheduler_actor/discovery.rs index ade9c9c..80439c9 100644 --- a/odorobo/src/actors/scheduler_actor/discovery.rs +++ b/odorobo/src/actors/scheduler_actor/discovery.rs @@ -1,22 +1,23 @@ //! Remote actor discovery, polling tasks, and their internal messages. -use std::time::Duration; +use std::time::{Duration, Instant}; use kameo::prelude::*; use libp2p::futures::TryStreamExt; use stable_eyre::{Report, eyre::eyre}; -use tracing::{trace, warn}; +use tracing::{info, trace, warn}; use crate::actors::agent_actor::AgentActor; use crate::ch_driver::actor::VMActor; use crate::messages::agent::{AgentStatusUpdate, GetAgentStatus, apply_status_update}; -use crate::messages::vm::{GetVMHeartbeat, GetVMInfo}; +use crate::messages::vm::{DeleteVM, DeleteVMReply, GetVMHeartbeat, GetVMInfo}; use crate::utils::actor_names::{AGENT, VM}; +use super::deletion::{AUTO_DELETE_TIMEOUT, status_update_held_vmids}; use super::{ - AgentActorDiscovered, AgentUpdated, AgentUpdaterStopped, CachedActorKind, CachedAgentActor, - CachedVMActor, ReconcileVmPlacements, SchedulerActor, VmActorDiscovered, VmDeleteOwner, - VmUpdated, VmUpdaterStopped, + AgentActorDiscovered, AgentUpdated, AgentUpdaterStopped, AutoDeleteFinished, CachedActorKind, + CachedAgentActor, CachedVMActor, ReconcileVmPlacements, SchedulerActor, VmActorDiscovered, + VmDeleteOwner, VmUpdated, VmUpdaterStopped, }; impl SchedulerActor { @@ -263,9 +264,7 @@ impl Message for SchedulerActor { let vmid = msg.data.vmid; // if a delete is still pending, late discovery should still tell us about a // cleanup owner we didn't know about, but shouldn't bring back placement intent - if self.vm_tombstones.get(&vmid) != Some(&false) - || self.vm_delete_targets.contains_key(&vmid) - { + if self.should_record_delete_owner(vmid) { self.remember_vm_delete_owner(vmid, VmDeleteOwner::vm(msg.actor_ref.id())); } if self.vm_tombstones.contains_key(&vmid) { @@ -276,7 +275,7 @@ impl Message for SchedulerActor { if let Some(manifest) = msg.data.config { self.vm_effective_manifests.insert(vmid, manifest.clone()); // if we only found this manifest after the scheduler started, it's the - // only intent we've got. Otherwise keep the original request. Effective + // only intent we've got. otherwise keep the original request. effective // values like an agent-assigned vsock CID are node-local and shouldn't // turn into hard requirements during failover self.vm_manifests.entry(vmid).or_insert(manifest); @@ -306,43 +305,21 @@ impl Message for SchedulerActor { } } -/// Applies a newer agent update, requiring a full snapshot before accepting deltas. -impl Message for SchedulerActor { - type Reply = (); - - async fn handle(&mut self, msg: AgentUpdated, _ctx: &mut Context) { - // only accepted snapshots/additions count as owning the VM. A removal - // doesn't tell us anything about a stopped node-local CID lease - let observed = match &msg.update { - AgentStatusUpdate::Full { revision, status } - if self - .agent_data_cache - .get(&msg.actor_id) - .is_none_or(|cached| *revision > cached.status_revision) => - { - Some(status.vms.as_slice()) - } - AgentStatusUpdate::Delta { - revision, added, .. - } if self - .agent_data_cache - .get(&msg.actor_id) - .is_some_and(|cached| *revision > cached.status_revision) => - { - Some(added.as_slice()) - } - AgentStatusUpdate::Full { .. } | AgentStatusUpdate::Delta { .. } => None, +impl SchedulerActor { + /// applies an agent status update to the caches. returns whether it was + /// accepted, so callers know the cached snapshot is fresh. a same-revision + /// delta with no membership changes is accepted too, since it carries the + /// latest usage and `retained_vms` and background deletes need those to retry + fn apply_agent_update(&mut self, msg: AgentUpdated) -> bool { + let cached_revision = self + .agent_data_cache + .get(&msg.actor_id) + .map(|cached| cached.status_revision); + let Some(held) = status_update_held_vmids(&msg.update, cached_revision) else { + return false; }; - if let Some(observed) = observed { - self.remember_agent_ref(&msg.actor_ref); - for vmid in observed { - if self.vm_tombstones.get(vmid) != Some(&false) - || self.vm_delete_targets.contains_key(vmid) - { - self.remember_vm_delete_owner(*vmid, VmDeleteOwner::agent(msg.actor_id)); - } - } - } + self.remember_agent_ref(&msg.actor_ref); + self.record_status_delete_owners(msg.actor_id, held); let Some(cached) = self.agent_data_cache.get_mut(&msg.actor_id) else { if let AgentStatusUpdate::Full { revision, status } = msg.update { self.agent_vm_index @@ -363,17 +340,11 @@ impl Message for SchedulerActor { status_revision: revision, }, ); + return true; } - return; + return false; }; - let revision = match &msg.update { - AgentStatusUpdate::Full { revision, .. } - | AgentStatusUpdate::Delta { revision, .. } => *revision, - }; - if revision <= cached.status_revision { - return; - } if let AgentStatusUpdate::Delta { added, removed, .. } = &msg.update { let added = added.clone(); let removed = removed.clone(); @@ -409,6 +380,78 @@ impl Message for SchedulerActor { ); self.invalidate_pending_resources(); } + true + } + + /// sends a background delete for every VM this agent still reports (running + /// or retained) that we already finished deleting, like after a force delete + /// skipped a node that later came back. stopped VMs are never touched. the + /// status handler doesn't wait on these, results come back as `AutoDeleteFinished` + fn spawn_auto_deletes(&mut self, scheduler: &ActorRef, agent_id: ActorId) { + let Some(cached) = self.agent_data_cache.get(&agent_id) else { + return; + }; + let agent = cached.actor_ref.clone(); + let reported: Vec<_> = cached + .data + .vms + .iter() + .chain(&cached.data.retained_vms) + .copied() + .collect(); + // the cached snapshot is complete even after a delta, so anything it + // doesn't list isn't on this node anymore + self.forget_unreported_leftovers(agent_id, &reported.iter().copied().collect()); + for vmid in self.take_auto_delete_vmids(agent_id, reported, Instant::now()) { + info!(%vmid, ?agent_id, "agent still holds a deleted VM, deleting it in the background"); + let scheduler = scheduler.clone(); + let agent = agent.clone(); + let request = DeleteVM { + vmid, + force: false, + issued_at: Some(self.next_stamp()), + }; + tokio::spawn(async move { + let error = + match tokio::time::timeout(AUTO_DELETE_TIMEOUT, agent.ask(&request)).await { + Ok(Ok(DeleteVMReply { error })) => error, + Ok(Err(error)) => Some(error.to_string()), + Err(_) => Some("timed out waiting for the agent".to_owned()), + }; + let send_result = scheduler + .tell(AutoDeleteFinished { + vmid, + agent_id, + error, + }) + .send() + .await; + if let Err(error) = send_result { + warn!(?error, %vmid, "background delete could not notify scheduler"); + } + }); + } + } +} + +/// Applies a newer agent update, requiring a full snapshot before accepting deltas. +impl Message for SchedulerActor { + type Reply = (); + + async fn handle(&mut self, msg: AgentUpdated, ctx: &mut Context) { + let agent_id = msg.actor_id; + if self.apply_agent_update(msg) { + self.spawn_auto_deletes(ctx.actor_ref(), agent_id); + } + } +} + +/// clears the in-flight marker for a background delete +impl Message for SchedulerActor { + type Reply = (); + + async fn handle(&mut self, msg: AutoDeleteFinished, _ctx: &mut Context) { + self.settle_auto_delete(msg.vmid, msg.agent_id, msg.error, Instant::now()); } } @@ -422,12 +465,18 @@ impl Message for SchedulerActor { } } -/// Performs periodic pending-placement expiry and refreshes resource accounting. +/// Performs periodic pending-placement expiry, prunes old tombstones, and +/// refreshes resource accounting. impl Message for SchedulerActor { type Reply = (); async fn handle(&mut self, _msg: ReconcileVmPlacements, _ctx: &mut Context) { - Self::cleanup_unresolved_vm_cache(&mut self.vm_placements, &mut self.vm_data_cache); + Self::cleanup_unresolved_vm_cache( + &self.vm_manifests, + &mut self.vm_placements, + &mut self.vm_data_cache, + ); + self.prune_vm_tombstones(Instant::now()); self.invalidate_pending_resources(); let unplaced_vms: Vec<_> = self @@ -442,7 +491,11 @@ impl Message for SchedulerActor { .collect(); for (vmid, config) in unplaced_vms { - let request = crate::messages::vm::CreateVM { vmid, config }; + let request = crate::messages::vm::CreateVM { + vmid, + config, + generation: Some(self.next_stamp()), + }; match self.schedule_agent(&request) { Ok(agent) => { self.remember_agent_ref(&agent); diff --git a/odorobo/src/actors/scheduler_actor/handlers.rs b/odorobo/src/actors/scheduler_actor/handlers.rs index 5d5bff7..7e365f7 100644 --- a/odorobo/src/actors/scheduler_actor/handlers.rs +++ b/odorobo/src/actors/scheduler_actor/handlers.rs @@ -20,8 +20,10 @@ use crate::messages::vm::{ use crate::messages::{Ping, Pong}; use crate::utils::actor_names::{AGENT, vm_actor_id}; +use super::deletion::{DeleteAttempt, delete_owner_outcome, plan_delete_settlement}; use super::{ CachedActorKind, CachedVMActor, SchedulerActor, VmDeleteOwner, VmLifecycle, VmPlacement, + VmTombstoneState, }; /// Owns scheduler initialization and cleanup for linked remote actors. @@ -44,6 +46,9 @@ impl Actor for SchedulerActor { vm_manifests: AHashMap::new(), vm_effective_manifests: AHashMap::new(), vm_tombstones: AHashMap::new(), + vm_auto_deletes: AHashMap::new(), + vm_leftover_owners: AHashMap::new(), + last_stamp: ulid::Ulid::nil(), known_agent_refs: AHashMap::new(), vm_delete_targets: AHashMap::new(), vm_placements: AHashMap::new(), @@ -99,11 +104,12 @@ impl Message for SchedulerActor { msg: CreateVM, _ctx: &mut Context, ) -> Self::Reply { - if self.vm_tombstones.get(&msg.vmid) == Some(&true) { - return Err(eyre!( - "VM deletion cleanup is pending; retry deletion before recreating it" - )); - } + self.check_create_allowed(msg.vmid) + .map_err(|error| eyre!(error))?; + let msg = CreateVM { + generation: Some(self.next_stamp()), + ..msg + }; if let Some(existing) = self.vm_manifests.get(&msg.vmid) { let matches_intent = same_create_intent(existing, &msg.config) || self @@ -136,6 +142,10 @@ impl Message for SchedulerActor { }); } + // only for IDs we have no intent for. a known VM showing up in agent + // status is just that VM, and the retry handling above covers it + self.check_not_running_elsewhere(msg.vmid) + .map_err(|error| eyre!(error))?; let target_agent = self.schedule_agent(&msg)?; self.remember_agent_ref(&target_agent); @@ -200,7 +210,7 @@ impl Message for SchedulerActor { } // a transport or discovery failure doesn't mean startup actually failed. - // Only roll back if the agent explicitly rejected it and a lookup worked + // only roll back if the agent explicitly rejected it and a lookup worked // and found no VM actor if reply.is_err() && !startup_outcome_unknown @@ -221,7 +231,13 @@ impl Message for SchedulerActor { ); let cleanup_confirmed = matches!( - target_agent.ask(&DeleteVM { vmid: msg.vmid }).await, + target_agent + .ask(&DeleteVM { + vmid: msg.vmid, + force: false, + issued_at: Some(self.next_stamp()), + }) + .await, Ok(DeleteVMReply { error: None }) ); self.settle_failed_create_cleanup( @@ -285,7 +301,9 @@ impl Message for SchedulerActor { } /// stops recreation right away, but keeps track of cleanup owners (evicted ones -/// too) until the agents confirm the process exited and the node-local CID was released +/// too) until the agents confirm the process exited and the node-local CID was released. +/// with `force`, owners we can't reach at all get dropped instead of failing the +/// delete. an agent that answers with an error still fails it impl Message for SchedulerActor { type Reply = Result; @@ -294,22 +312,28 @@ impl Message for SchedulerActor { msg: DeleteVM, _ctx: &mut Context, ) -> Self::Reply { - let already_deleted = self.vm_tombstones.get(&msg.vmid) == Some(&false) + // stamp it so a copy that's still in flight when the ID gets recreated + // can't take the new VM down with it + let msg = DeleteVM { + issued_at: Some(self.next_stamp()), + ..msg + }; + let already_deleted = self.vm_tombstone_state(msg.vmid) == Some(VmTombstoneState::Deleted) && !self.vm_delete_targets.contains_key(&msg.vmid); self.remove_vm_intent(msg.vmid); - self.vm_tombstones.insert(msg.vmid, true); + self.set_vm_tombstone(msg.vmid, VmTombstoneState::DeletePending); // the unique VM name can show us a migration owner before its agent status - // shows up. Don't ask the VM actors directly though, since they can't ack + // shows up. don't ask the VM actors directly though, since they can't ack // the agent's stopped lease cleanup let mut vms = RemoteActorRef::::lookup_all(vm_actor_id(msg.vmid)); while let Some(vm) = vms.try_next().await? { self.remember_vm_delete_owner(msg.vmid, VmDeleteOwner::vm(vm.id())); } if already_deleted && !self.vm_delete_targets.contains_key(&msg.vmid) { - self.vm_tombstones.insert(msg.vmid, false); + self.set_vm_tombstone(msg.vmid, VmTombstoneState::Deleted); return Ok(DeleteVMReply { error: None }); } - // we still need discovery even with cached targets. An agent that restarted + // we still need discovery even with cached targets. an agent that restarted // on the same peer has a new actor ID, and that has to replace the old route let mut discovered = RemoteActorRef::::lookup_all(AGENT); let mut available = AHashMap::new(); @@ -322,8 +346,10 @@ impl Message for SchedulerActor { .get(&msg.vmid) .is_none_or(|targets| targets.is_empty()) { - // no owner history (like a stopped VM after a scheduler restart), so - // just ask the agents we know about now, not every dead agent ever + // no owner history and no agent has reported holding anything for + // this VM, so just ask the agents we know about now, not every dead + // agent ever. agents report `retained_vms` in their status, which + // rebuilds owner history after a scheduler restart available.extend( self.agent_data_cache .values() @@ -341,18 +367,11 @@ impl Message for SchedulerActor { .filter(|(_, target)| !target.confirmed) .map(|(owner, target)| (*owner, target.actor_ref.clone())) .collect(); - if targets.is_empty() { - return if self.finish_vm_delete(msg.vmid) { - Ok(DeleteVMReply { error: None }) - } else { - Err(eyre!("no agents can confirm VM teardown and CID release")) - }; - } - let mut failures = Vec::new(); + let mut outcomes = Vec::with_capacity(targets.len()); for (owner, retained) in targets { // the registry can return both the old and the replacement actor on - // one peer. Don't let a stale route hide a working one forever + // one peer. don't let a stale route hide a working one forever let mut candidates: AHashMap<_, _> = available .iter() .filter(|(id, _)| owner.matches_agent(**id)) @@ -361,38 +380,26 @@ impl Message for SchedulerActor { if let Some(agent) = retained.filter(|agent| owner.matches_agent(agent.id())) { candidates.entry(agent.id()).or_insert(agent); } - let mut errors = Vec::new(); - let mut confirmed = false; + let mut attempts = Vec::with_capacity(candidates.len()); for agent in candidates.values() { - match agent.ask(&msg).await { - Ok(reply) if reply.error.is_none() => { - self.acknowledge_vm_delete_owner(msg.vmid, owner); - confirmed = true; - break; - } - Ok(reply) => errors.push(reply.error.unwrap_or_default()), - Err(error) => errors.push(error.to_string()), + let attempt = match agent.ask(&msg).await { + Ok(DeleteVMReply { error: None }) => DeleteAttempt::Acked, + Ok(DeleteVMReply { error: Some(error) }) => DeleteAttempt::AgentError(error), + Err(error) => DeleteAttempt::from_send_error(&error), + }; + let acked = attempt == DeleteAttempt::Acked; + attempts.push(attempt); + if acked { + break; } } - if !confirmed { - failures.push(format!( - "{owner:?}: {}", - if errors.is_empty() { - "missing agent for known VM owner".to_owned() - } else { - errors.join("; ") - } - )); - } - } - if failures.is_empty() && self.finish_vm_delete(msg.vmid) { - Ok(DeleteVMReply { error: None }) - } else { - Err(eyre!( - "failed to release VM CID on one or more agents: {}", - failures.join("; ") - )) + outcomes.push((owner, delete_owner_outcome(attempts))); } + + let settlement = plan_delete_settlement(outcomes, msg.force); + self.apply_delete_settlement(msg.vmid, settlement, msg.force) + .map(|()| DeleteVMReply { error: None }) + .map_err(|error| eyre!(error)) } } diff --git a/odorobo/src/actors/scheduler_actor/tests.rs b/odorobo/src/actors/scheduler_actor/tests.rs index 126716e..09813e8 100644 --- a/odorobo/src/actors/scheduler_actor/tests.rs +++ b/odorobo/src/actors/scheduler_actor/tests.rs @@ -1,5 +1,5 @@ use super::{ - CachedActorKind, CachedVMActor, SchedulerActor, VmLifecycle, VmPlacement, + CachedActorKind, CachedVMActor, SchedulerActor, VmLifecycle, VmPlacement, VmTombstoneState, scheduling::{ affinity_delta, evaluate_affinity_rule, evaluate_table_value, has_capacity, pending_resources_for_agent, @@ -58,16 +58,17 @@ fn expires_unresolved_vm_placeholder_but_retains_vm_intent() { vec![VmPlacement { agent_id, lifecycle: VmLifecycle::Pending, - created_at: Instant::now() - .checked_sub(Duration::from_secs(121)) - .expect("test timestamp should be representable"), + created_at: ago( + super::cache::pending_placement_timeout(Some(&test_manifest(1, 1))) + .saturating_add(Duration::from_secs(1)), + ), last_confirmed_at: None, }], ); let manifests = AHashMap::from([(vmid, test_manifest(1, 1))]); let mut data_cache: AHashMap> = AHashMap::new(); - SchedulerActor::cleanup_unresolved_vm_cache(&mut placements, &mut data_cache); + SchedulerActor::cleanup_unresolved_vm_cache(&manifests, &mut placements, &mut data_cache); assert!(manifests.contains_key(&vmid)); assert!(placements[&vmid].is_empty()); @@ -117,6 +118,7 @@ fn reconciling_source_agent_preserves_destination_migration_placement() { used_vcpus: 0, used_ram: ByteSize::b(0), vms: vec![vmid], + retained_vms: Vec::new(), metadata: ObjectMetadata::default(), }; SchedulerActor::reconcile_agent_placements( @@ -134,6 +136,7 @@ fn reconciling_source_agent_preserves_destination_migration_placement() { used_vcpus: 0, used_ram: ByteSize::b(0), vms: Vec::new(), + retained_vms: Vec::new(), metadata: ObjectMetadata::default(), }; @@ -272,6 +275,7 @@ fn pending_placement_survives_full_snapshot_until_pending_timeout() { used_vcpus: 0, used_ram: ByteSize::b(0), vms: Vec::new(), + retained_vms: Vec::new(), metadata: ObjectMetadata::default(), }; @@ -310,6 +314,7 @@ fn stale_running_placement_is_expired_by_full_snapshot() { used_vcpus: 0, used_ram: ByteSize::b(0), vms: Vec::new(), + retained_vms: Vec::new(), metadata: ObjectMetadata::default(), }; @@ -335,6 +340,9 @@ fn vm_cleanup_unplaces_vm_without_another_discovered_actor() { vm_manifests: AHashMap::from([(vmid, test_manifest(1, 1))]), vm_effective_manifests: AHashMap::new(), vm_tombstones: AHashMap::new(), + vm_auto_deletes: AHashMap::new(), + vm_leftover_owners: AHashMap::new(), + last_stamp: Ulid::nil(), known_agent_refs: AHashMap::new(), vm_delete_targets: AHashMap::new(), vm_placements: AHashMap::from([( @@ -404,7 +412,10 @@ fn settled_failed_create_cleanup_allows_corrected_recreate() { scheduler.settle_failed_create_cleanup(vmid, owner, true, true); assert!(!scheduler.vm_delete_targets.contains_key(&vmid)); - assert_ne!(scheduler.vm_tombstones.get(&vmid), Some(&true)); + assert_ne!( + scheduler.vm_tombstone_state(vmid), + Some(VmTombstoneState::DeletePending) + ); assert!(!scheduler.create_is_confirmed(vmid)); } @@ -418,7 +429,10 @@ fn uncertain_failed_create_cleanup_retains_owner_and_blocks_recreate() { scheduler.settle_failed_create_cleanup(vmid, owner, true, false); assert!(scheduler.vm_delete_targets[&vmid].contains_key(&owner)); - assert_eq!(scheduler.vm_tombstones.get(&vmid), Some(&true)); + assert_eq!( + scheduler.vm_tombstone_state(vmid), + Some(VmTombstoneState::DeletePending) + ); } #[test] @@ -495,6 +509,9 @@ fn explicit_stop_removes_vm_intent_and_actor_mapping() { vm_manifests: AHashMap::from([(vmid, test_manifest(1, 1))]), vm_effective_manifests: AHashMap::new(), vm_tombstones: AHashMap::new(), + vm_auto_deletes: AHashMap::new(), + vm_leftover_owners: AHashMap::new(), + last_stamp: Ulid::nil(), known_agent_refs: AHashMap::new(), vm_delete_targets: AHashMap::new(), vm_placements: AHashMap::from([(vmid, Vec::new())]), @@ -529,6 +546,9 @@ fn agent_cleanup_does_not_remove_unrelated_vm_state() { vm_manifests: AHashMap::from([(vmid, test_manifest(1, 1))]), vm_effective_manifests: AHashMap::new(), vm_tombstones: AHashMap::new(), + vm_auto_deletes: AHashMap::new(), + vm_leftover_owners: AHashMap::new(), + last_stamp: Ulid::nil(), known_agent_refs: AHashMap::new(), vm_delete_targets: AHashMap::new(), vm_placements: AHashMap::from([( @@ -568,6 +588,9 @@ fn vm_cleanup_does_not_remove_unrelated_agent_state() { vm_manifests: AHashMap::from([(vmid, test_manifest(1, 1))]), vm_effective_manifests: AHashMap::new(), vm_tombstones: AHashMap::new(), + vm_auto_deletes: AHashMap::new(), + vm_leftover_owners: AHashMap::new(), + last_stamp: Ulid::nil(), known_agent_refs: AHashMap::new(), vm_delete_targets: AHashMap::new(), vm_placements: AHashMap::new(), @@ -594,6 +617,9 @@ fn empty_scheduler() -> SchedulerActor { vm_manifests: AHashMap::new(), vm_effective_manifests: AHashMap::new(), vm_tombstones: AHashMap::new(), + vm_auto_deletes: AHashMap::new(), + vm_leftover_owners: AHashMap::new(), + last_stamp: Ulid::nil(), known_agent_refs: AHashMap::new(), vm_delete_targets: AHashMap::new(), vm_placements: AHashMap::new(), @@ -683,7 +709,7 @@ fn unrelated_success_cannot_complete_delete_and_failures_survive_retries() { scheduler.remember_vm_delete_owner(vmid, source); scheduler.remember_vm_delete_owner(vmid, destination); scheduler.remove_vm_intent(vmid); - scheduler.vm_tombstones.insert(vmid, true); + scheduler.set_vm_tombstone(vmid, VmTombstoneState::DeletePending); scheduler.acknowledge_vm_delete_owner(vmid, unrelated); assert!(!scheduler.finish_vm_delete(vmid)); assert_eq!(scheduler.vm_delete_targets[&vmid].len(), 2); @@ -695,11 +721,17 @@ fn unrelated_success_cannot_complete_delete_and_failures_survive_retries() { assert!(scheduler.vm_delete_targets[&vmid][&source].confirmed); assert!(!scheduler.vm_delete_targets[&vmid][&destination].confirmed); assert!(!scheduler.finish_vm_delete(vmid)); - assert_eq!(scheduler.vm_tombstones.get(&vmid), Some(&true)); + assert_eq!( + scheduler.vm_tombstone_state(vmid), + Some(VmTombstoneState::DeletePending) + ); scheduler.acknowledge_vm_delete_owner(vmid, destination); assert!(scheduler.finish_vm_delete(vmid)); assert!(!scheduler.vm_delete_targets.contains_key(&vmid)); - assert_eq!(scheduler.vm_tombstones.get(&vmid), Some(&false)); + assert_eq!( + scheduler.vm_tombstone_state(vmid), + Some(VmTombstoneState::Deleted) + ); } #[test] @@ -877,3 +909,767 @@ fn evaluates_required_preferred_and_capacity_rules() { assert!(!has_capacity(8, 7, 2, 16, 4, 4)); assert!(!has_capacity(8, 2, 2, 16, 13, 4)); } + +fn ago(duration: Duration) -> Instant { + Instant::now() + .checked_sub(duration) + .expect("test timestamp should be representable") +} + +fn status_with(vms: Vec, retained_vms: Vec) -> AgentStatus { + AgentStatus { + hostname: "agent".to_owned(), + vcpus: 1, + ram: ByteSize::b(1), + used_vcpus: 0, + used_ram: ByteSize::b(0), + vms, + retained_vms, + metadata: ObjectMetadata::default(), + } +} + +#[test] +fn any_ack_wins_and_explicit_agent_errors_beat_unreachable_routes() { + use super::deletion::{DeleteAttempt, DeleteOwnerOutcome, delete_owner_outcome}; + + assert_eq!( + delete_owner_outcome(vec![ + DeleteAttempt::Unreachable("stale route".to_owned()), + DeleteAttempt::Acked, + ]), + DeleteOwnerOutcome::Acked + ); + // a live replacement saying no matters more than a dead old route + assert_eq!( + delete_owner_outcome(vec![ + DeleteAttempt::Unreachable("stale route".to_owned()), + DeleteAttempt::AgentError("teardown failed".to_owned()), + ]), + DeleteOwnerOutcome::Rejected(vec!["teardown failed".to_owned()]) + ); + assert_eq!( + delete_owner_outcome(vec![DeleteAttempt::Unreachable("timeout".to_owned())]), + DeleteOwnerOutcome::Unreachable(vec!["timeout".to_owned()]) + ); + assert_eq!( + delete_owner_outcome(Vec::new()), + DeleteOwnerOutcome::Unreachable(Vec::new()) + ); +} + +#[test] +fn force_drops_unreachable_owners_but_not_explicit_agent_errors() { + use super::deletion::{DeleteOwnerOutcome, plan_delete_settlement}; + + let acked = super::VmDeleteOwner::agent(super::ActorId::new(1)); + let no_route = super::VmDeleteOwner::agent(super::ActorId::new(2)); + let transport = super::VmDeleteOwner::agent(super::ActorId::new(3)); + let rejected = super::VmDeleteOwner::agent(super::ActorId::new(4)); + let outcomes = || { + vec![ + (acked, DeleteOwnerOutcome::Acked), + (no_route, DeleteOwnerOutcome::Unreachable(Vec::new())), + ( + transport, + DeleteOwnerOutcome::Unreachable(vec!["timeout".to_owned()]), + ), + ( + rejected, + DeleteOwnerOutcome::Rejected(vec!["teardown failed".to_owned()]), + ), + ] + }; + + let forced = plan_delete_settlement(outcomes(), true); + assert_eq!(forced.acked, vec![acked]); + assert_eq!( + forced + .dropped + .iter() + .map(|(owner, _)| *owner) + .collect::>(), + vec![no_route, transport] + ); + assert_eq!(forced.failures.len(), 1); + assert!(forced.failures[0].contains("teardown failed")); + + let unforced = plan_delete_settlement(outcomes(), false); + assert_eq!(unforced.acked, vec![acked]); + assert_eq!(unforced.dropped, Vec::new()); + assert_eq!(unforced.failures.len(), 3); + assert!( + unforced + .failures + .iter() + .any(|failure| failure.contains("missing agent for known VM owner")) + ); +} + +#[test] +fn forced_delete_finishes_once_unreachable_owners_are_dropped() { + use super::deletion::{DeleteOwnerOutcome, plan_delete_settlement}; + + let vmid = Ulid::generate(); + let alive = super::VmDeleteOwner::agent(super::ActorId::new(1)); + let dead = super::VmDeleteOwner::agent(super::ActorId::new(2)); + let mut scheduler = empty_scheduler(); + scheduler.remember_vm_delete_owner(vmid, alive); + scheduler.remember_vm_delete_owner(vmid, dead); + scheduler.remove_vm_intent(vmid); + scheduler.set_vm_tombstone(vmid, VmTombstoneState::DeletePending); + let outcomes = || { + vec![ + (alive, DeleteOwnerOutcome::Acked), + (dead, DeleteOwnerOutcome::Unreachable(Vec::new())), + ] + }; + + // without force the dead node keeps the delete pending, but the ack sticks + let settlement = plan_delete_settlement(outcomes(), false); + assert!( + scheduler + .apply_delete_settlement(vmid, settlement, false) + .is_err() + ); + assert!(scheduler.vm_delete_targets[&vmid][&alive].confirmed); + assert!(scheduler.vm_delete_targets[&vmid].contains_key(&dead)); + assert_eq!( + scheduler.vm_tombstone_state(vmid), + Some(VmTombstoneState::DeletePending) + ); + + let settlement = plan_delete_settlement(outcomes(), true); + assert_eq!( + scheduler.apply_delete_settlement(vmid, settlement, true), + Ok(()) + ); + assert!(!scheduler.vm_delete_targets.contains_key(&vmid)); + assert_eq!( + scheduler.vm_tombstone_state(vmid), + Some(VmTombstoneState::Deleted) + ); +} + +#[test] +fn forced_delete_still_fails_when_a_live_agent_rejects_it() { + use super::deletion::{DeleteOwnerOutcome, plan_delete_settlement}; + + let vmid = Ulid::generate(); + let dead = super::VmDeleteOwner::agent(super::ActorId::new(1)); + let rejecting = super::VmDeleteOwner::agent(super::ActorId::new(2)); + let mut scheduler = empty_scheduler(); + scheduler.remember_vm_delete_owner(vmid, dead); + scheduler.remember_vm_delete_owner(vmid, rejecting); + scheduler.remove_vm_intent(vmid); + scheduler.set_vm_tombstone(vmid, VmTombstoneState::DeletePending); + + let settlement = plan_delete_settlement( + vec![ + (dead, DeleteOwnerOutcome::Unreachable(Vec::new())), + ( + rejecting, + DeleteOwnerOutcome::Rejected(vec!["teardown failed".to_owned()]), + ), + ], + true, + ); + let error = scheduler + .apply_delete_settlement(vmid, settlement, true) + .expect_err("a live rejection fails even a forced delete"); + assert!(error.contains("teardown failed")); + // the dead owner is gone, but the rejecting one still blocks recreate + assert!(!scheduler.vm_delete_targets[&vmid].contains_key(&dead)); + assert!(!scheduler.vm_delete_targets[&vmid][&rejecting].confirmed); + assert_eq!( + scheduler.vm_tombstone_state(vmid), + Some(VmTombstoneState::DeletePending) + ); +} + +#[test] +fn forced_delete_without_any_owners_finishes_but_unforced_does_not() { + use super::deletion::DeleteSettlement; + + let vmid = Ulid::generate(); + let mut scheduler = empty_scheduler(); + scheduler.remove_vm_intent(vmid); + scheduler.set_vm_tombstone(vmid, VmTombstoneState::DeletePending); + + assert!( + scheduler + .apply_delete_settlement(vmid, DeleteSettlement::default(), false) + .is_err() + ); + assert_eq!( + scheduler.vm_tombstone_state(vmid), + Some(VmTombstoneState::DeletePending) + ); + assert_eq!( + scheduler.apply_delete_settlement(vmid, DeleteSettlement::default(), true), + Ok(()) + ); + assert_eq!( + scheduler.vm_tombstone_state(vmid), + Some(VmTombstoneState::Deleted) + ); +} + +#[test] +fn shutdown_tombstones_as_stopped_and_delete_goes_pending_then_deleted() { + let vmid = Ulid::generate(); + let owner = super::VmDeleteOwner::agent(super::ActorId::new(1)); + let mut scheduler = empty_scheduler(); + scheduler.vm_manifests.insert(vmid, test_manifest(1, 1)); + scheduler.remember_vm_delete_owner(vmid, owner); + + // shutdown only removes intent + scheduler.remove_vm_intent(vmid); + assert_eq!( + scheduler.vm_tombstone_state(vmid), + Some(VmTombstoneState::Stopped) + ); + assert!(scheduler.vm_is_stopped_or_deleted(vmid)); + + // delete does the same, then marks cleanup pending until owners ack + scheduler.remove_vm_intent(vmid); + scheduler.set_vm_tombstone(vmid, VmTombstoneState::DeletePending); + assert!(!scheduler.vm_is_stopped_or_deleted(vmid)); + scheduler.acknowledge_vm_delete_owner(vmid, owner); + assert!(scheduler.finish_vm_delete(vmid)); + assert_eq!( + scheduler.vm_tombstone_state(vmid), + Some(VmTombstoneState::Deleted) + ); + + // shutting down again doesn't downgrade a finished delete + scheduler.remove_vm_intent(vmid); + assert_eq!( + scheduler.vm_tombstone_state(vmid), + Some(VmTombstoneState::Deleted) + ); +} + +#[test] +fn auto_delete_only_fires_for_deleted_vms() { + let agent = super::ActorId::new(1); + let stopped = Ulid::generate(); + let pending = Ulid::generate(); + let deleted = Ulid::generate(); + let live = Ulid::generate(); + let mut scheduler = empty_scheduler(); + scheduler.set_vm_tombstone(stopped, VmTombstoneState::Stopped); + scheduler.set_vm_tombstone(pending, VmTombstoneState::DeletePending); + scheduler.set_vm_tombstone(deleted, VmTombstoneState::Deleted); + + let picked = + scheduler.take_auto_delete_vmids(agent, [stopped, pending, deleted, live], Instant::now()); + assert_eq!(picked, vec![deleted]); + assert!(scheduler.auto_delete_in_flight(deleted)); + assert!(!scheduler.auto_delete_in_flight(stopped)); +} + +#[test] +fn auto_delete_is_rate_limited_per_vm_and_agent() { + use super::deletion::AUTO_DELETE_RETRY_INTERVAL; + + let agent = super::ActorId::new(1); + let other_agent = super::ActorId::new(2); + let vmid = Ulid::generate(); + let mut scheduler = empty_scheduler(); + scheduler.set_vm_tombstone(vmid, VmTombstoneState::Deleted); + let now = Instant::now(); + + assert_eq!( + scheduler.take_auto_delete_vmids(agent, [vmid], now), + vec![vmid] + ); + // still in flight, so the next status poll doesn't send another + assert_eq!( + scheduler.take_auto_delete_vmids(agent, [vmid], now), + Vec::new() + ); + // a different agent holding the same VM gets its own + assert_eq!( + scheduler.take_auto_delete_vmids(other_agent, [vmid], now), + vec![vmid] + ); + + scheduler.settle_auto_delete(vmid, agent, Some("busy".to_owned()), now); + assert_eq!( + scheduler.take_auto_delete_vmids(agent, [vmid], now), + Vec::new() + ); + let later = now + .checked_add(AUTO_DELETE_RETRY_INTERVAL) + .expect("test timestamp should be representable"); + assert_eq!( + scheduler.take_auto_delete_vmids(agent, [vmid], later), + vec![vmid] + ); + + scheduler.settle_auto_delete(vmid, agent, None, later); + scheduler.settle_auto_delete(vmid, other_agent, None, later); + assert!(!scheduler.auto_delete_in_flight(vmid)); + assert_eq!(scheduler.vm_auto_deletes.len(), 0); +} + +#[test] +fn retained_vms_rebuild_delete_owners_after_scheduler_restart() { + use super::deletion::status_update_held_vmids; + use crate::messages::agent::AgentStatusUpdate; + + let agent = super::ActorId::new_with_peer_id(1, libp2p::PeerId::random()); + let running = Ulid::generate(); + let stopped_lease = Ulid::generate(); + let full = AgentStatusUpdate::Full { + revision: 1, + status: status_with(vec![running], vec![stopped_lease]), + }; + let held = status_update_held_vmids(&full, None).expect("first full snapshot is accepted"); + + // a fresh scheduler has no tombstones or owners for either VM + let mut scheduler = empty_scheduler(); + scheduler.record_status_delete_owners(agent, held); + let owner = super::VmDeleteOwner::agent(agent); + assert!(scheduler.vm_delete_targets[&running].contains_key(&owner)); + assert!(scheduler.vm_delete_targets[&stopped_lease].contains_key(&owner)); + + // so a later delete has to wait for this agent instead of acking without it + scheduler.remove_vm_intent(stopped_lease); + scheduler.set_vm_tombstone(stopped_lease, VmTombstoneState::DeletePending); + assert!(!scheduler.finish_vm_delete(stopped_lease)); +} + +#[test] +fn status_deltas_carry_retained_vms_even_without_a_revision_bump() { + use super::deletion::status_update_held_vmids; + use crate::messages::agent::AgentStatusUpdate; + + let added = Ulid::generate(); + let retained = Ulid::generate(); + let delta = |revision, added: Vec| AgentStatusUpdate::Delta { + revision, + added, + removed: Vec::new(), + retained_vms: vec![retained], + used_vcpus: 0, + used_ram: ByteSize::b(0), + }; + + assert_eq!( + status_update_held_vmids(&delta(3, vec![added]), Some(2)), + Some(vec![added, retained]) + ); + // same revision, nothing added, still refreshes retained state + assert_eq!( + status_update_held_vmids(&delta(2, Vec::new()), Some(2)), + Some(vec![retained]) + ); + // stale ones and deltas before any full snapshot are ignored + assert_eq!( + status_update_held_vmids(&delta(1, Vec::new()), Some(2)), + None + ); + assert_eq!(status_update_held_vmids(&delta(3, Vec::new()), None), None); + let stale_full = AgentStatusUpdate::Full { + revision: 2, + status: status_with(vec![added], vec![retained]), + }; + assert_eq!(status_update_held_vmids(&stale_full, Some(2)), None); +} + +#[test] +fn status_does_not_re_add_owners_for_finished_deletes_or_untracked_stops() { + let agent = super::ActorId::new(1); + let owner = super::VmDeleteOwner::agent(agent); + let deleted = Ulid::generate(); + let stopped_tracked = Ulid::generate(); + let pending = Ulid::generate(); + let mut scheduler = empty_scheduler(); + scheduler.set_vm_tombstone(deleted, VmTombstoneState::Deleted); + scheduler.set_vm_tombstone(pending, VmTombstoneState::DeletePending); + scheduler.remember_vm_delete_owner( + stopped_tracked, + super::VmDeleteOwner::agent(super::ActorId::new(9)), + ); + scheduler.set_vm_tombstone(stopped_tracked, VmTombstoneState::Stopped); + + scheduler.record_status_delete_owners(agent, [deleted, stopped_tracked, pending]); + + // finished deletes get cleaned up in the background instead + assert!(!scheduler.vm_delete_targets.contains_key(&deleted)); + assert!(scheduler.vm_delete_targets[&stopped_tracked].contains_key(&owner)); + assert!(scheduler.vm_delete_targets[&pending].contains_key(&owner)); +} + +#[test] +fn prunes_old_stopped_and_deleted_tombstones_but_never_pending_or_tracked_ones() { + use super::deletion::{ + AUTO_DELETE_RETRY_INTERVAL, DELETED_TOMBSTONE_TTL, STOPPED_TOMBSTONE_TTL, + }; + use super::{AutoDeleteState, VmTombstone}; + + let tombstone = |state, since| VmTombstone { state, since }; + let old_stopped = Ulid::generate(); + let fresh_stopped = Ulid::generate(); + let old_deleted = Ulid::generate(); + let fresh_deleted = Ulid::generate(); + let ancient_pending = Ulid::generate(); + let old_stopped_with_owner = Ulid::generate(); + let mut scheduler = empty_scheduler(); + let past_stopped_ttl = ago(STOPPED_TOMBSTONE_TTL.saturating_add(Duration::from_secs(1))); + let past_deleted_ttl = ago(DELETED_TOMBSTONE_TTL.saturating_add(Duration::from_secs(1))); + scheduler.vm_tombstones.extend([ + ( + old_stopped, + tombstone(VmTombstoneState::Stopped, past_stopped_ttl), + ), + ( + fresh_stopped, + tombstone(VmTombstoneState::Stopped, Instant::now()), + ), + ( + old_deleted, + tombstone(VmTombstoneState::Deleted, past_deleted_ttl), + ), + // older than the stopped TTL, but deleted ones live longer + ( + fresh_deleted, + tombstone(VmTombstoneState::Deleted, past_stopped_ttl), + ), + ( + ancient_pending, + tombstone(VmTombstoneState::DeletePending, past_deleted_ttl), + ), + ( + old_stopped_with_owner, + tombstone(VmTombstoneState::Stopped, past_deleted_ttl), + ), + ]); + scheduler.remember_vm_delete_owner( + old_stopped_with_owner, + super::VmDeleteOwner::agent(super::ActorId::new(1)), + ); + let agent = super::ActorId::new(2); + scheduler + .vm_auto_deletes + .insert((old_deleted, agent), AutoDeleteState::InFlight); + scheduler.vm_auto_deletes.insert( + (fresh_deleted, agent), + AutoDeleteState::Failed(ago(AUTO_DELETE_RETRY_INTERVAL)), + ); + + scheduler.prune_vm_tombstones(Instant::now()); + + assert!(!scheduler.vm_tombstones.contains_key(&old_stopped)); + assert!(scheduler.vm_tombstones.contains_key(&fresh_stopped)); + assert!(!scheduler.vm_tombstones.contains_key(&old_deleted)); + assert!(scheduler.vm_tombstones.contains_key(&fresh_deleted)); + assert!(scheduler.vm_tombstones.contains_key(&ancient_pending)); + assert!( + scheduler + .vm_tombstones + .contains_key(&old_stopped_with_owner) + ); + // in-flight deletes stay until they report back, expired failures go + assert!( + scheduler + .vm_auto_deletes + .contains_key(&(old_deleted, agent)) + ); + assert!( + !scheduler + .vm_auto_deletes + .contains_key(&(fresh_deleted, agent)) + ); +} + +#[test] +fn delete_of_an_unknown_vm_ends_up_prunable() { + use super::deletion::DELETED_TOMBSTONE_TTL; + + let vmid = Ulid::generate(); + let agent = super::VmDeleteOwner::agent(super::ActorId::new(1)); + let mut scheduler = empty_scheduler(); + // the handler falls back to asking every reachable agent, which all ack + scheduler.remove_vm_intent(vmid); + scheduler.set_vm_tombstone(vmid, VmTombstoneState::DeletePending); + scheduler.remember_vm_delete_owner(vmid, agent); + scheduler.acknowledge_vm_delete_owner(vmid, agent); + assert!(scheduler.finish_vm_delete(vmid)); + + scheduler.prune_vm_tombstones(Instant::now()); + assert!(scheduler.vm_tombstones.contains_key(&vmid)); + scheduler.prune_vm_tombstones( + Instant::now() + .checked_add(DELETED_TOMBSTONE_TTL) + .expect("test timestamp should be representable"), + ); + assert!(!scheduler.vm_tombstones.contains_key(&vmid)); + assert!(!scheduler.vm_delete_targets.contains_key(&vmid)); +} + +/// force-deletes `vmid` with `dead` unreachable, leaving it a leftover owner +fn force_delete_with_dropped_owner( + scheduler: &mut SchedulerActor, + vmid: Ulid, + dead: super::VmDeleteOwner, +) { + use super::deletion::{DeleteOwnerOutcome, plan_delete_settlement}; + + scheduler.remember_vm_delete_owner(vmid, dead); + scheduler.remove_vm_intent(vmid); + scheduler.set_vm_tombstone(vmid, VmTombstoneState::DeletePending); + let settlement = plan_delete_settlement( + vec![(dead, DeleteOwnerOutcome::Unreachable(Vec::new()))], + true, + ); + assert_eq!( + scheduler.apply_delete_settlement(vmid, settlement, true), + Ok(()) + ); + assert_eq!( + scheduler.vm_tombstone_state(vmid), + Some(VmTombstoneState::Deleted) + ); +} + +#[test] +fn force_dropped_owner_blocks_recreate_until_its_copy_is_deleted() { + use super::deletion::DELETED_TOMBSTONE_TTL; + + let agent = super::ActorId::new_with_peer_id(1, libp2p::PeerId::random()); + let dead = super::VmDeleteOwner::agent(agent); + let vmid = Ulid::generate(); + let mut scheduler = empty_scheduler(); + force_delete_with_dropped_owner(&mut scheduler, vmid, dead); + + // the partitioned node might still run it on the same storage + let error = scheduler.check_create_allowed(vmid).unwrap_err(); + assert!(error.contains("may still be running"), "{error}"); + + // the tombstone outlives its TTL while that node could come back + let past_ttl = ago(DELETED_TOMBSTONE_TTL.saturating_add(Duration::from_secs(1))); + scheduler.vm_tombstones.get_mut(&vmid).unwrap().since = past_ttl; + scheduler.prune_vm_tombstones(Instant::now()); + assert_eq!( + scheduler.vm_tombstone_state(vmid), + Some(VmTombstoneState::Deleted) + ); + + // it comes back (a restarted agent on the same peer) still holding the VM + let restarted = super::ActorId::new_with_peer_id(7, *agent.peer_id().unwrap()); + let now = Instant::now(); + assert_eq!( + scheduler.take_auto_delete_vmids(restarted, [vmid], now), + vec![vmid] + ); + assert!(scheduler.check_create_allowed(vmid).is_err()); + + scheduler.settle_auto_delete(vmid, restarted, None, now); + assert_eq!(scheduler.check_create_allowed(vmid), Ok(())); + assert!(!scheduler.vm_leftover_owners.contains_key(&vmid)); +} + +#[test] +fn failed_background_delete_keeps_blocking_recreate() { + use super::deletion::AUTO_DELETE_RETRY_INTERVAL; + + let agent = super::ActorId::new_with_peer_id(1, libp2p::PeerId::random()); + let vmid = Ulid::generate(); + let mut scheduler = empty_scheduler(); + // a plain delete finished, then some agent turns out to still hold the VM + scheduler.set_vm_tombstone(vmid, VmTombstoneState::Deleted); + let now = Instant::now(); + assert_eq!( + scheduler.take_auto_delete_vmids(agent, [vmid], now), + vec![vmid] + ); + + scheduler.settle_auto_delete(vmid, agent, Some("timed out".to_owned()), now); + assert!(!scheduler.auto_delete_in_flight(vmid)); + // the retry marker expiring doesn't make the leftover go away + let later = now + .checked_add(AUTO_DELETE_RETRY_INTERVAL) + .expect("test timestamp should be representable"); + scheduler.prune_vm_tombstones(later); + assert!(scheduler.check_create_allowed(vmid).is_err()); +} + +#[test] +fn leftover_owner_is_cleared_when_it_reports_without_the_vm_or_acks_a_delete() { + use super::deletion::{DeleteOwnerOutcome, plan_delete_settlement}; + + let agent = super::ActorId::new_with_peer_id(1, libp2p::PeerId::random()); + let other = super::ActorId::new_with_peer_id(2, libp2p::PeerId::random()); + let owner = super::VmDeleteOwner::agent(agent); + let reported_elsewhere = Ulid::generate(); + let gone = Ulid::generate(); + let acked = Ulid::generate(); + let mut scheduler = empty_scheduler(); + force_delete_with_dropped_owner(&mut scheduler, reported_elsewhere, owner); + force_delete_with_dropped_owner(&mut scheduler, gone, owner); + force_delete_with_dropped_owner(&mut scheduler, acked, owner); + + // another node's status says nothing about this one + scheduler.forget_unreported_leftovers(other, &AHashSet::new()); + assert!(scheduler.check_create_allowed(gone).is_err()); + + // it came back, and its VMMs died with it except one + scheduler.forget_unreported_leftovers(agent, &std::iter::once(reported_elsewhere).collect()); + assert_eq!(scheduler.check_create_allowed(gone), Ok(())); + assert!(scheduler.check_create_allowed(reported_elsewhere).is_err()); + + // an explicit delete that reaches it clears it too + scheduler.remember_vm_delete_owner(acked, owner); + let settlement = plan_delete_settlement(vec![(owner, DeleteOwnerOutcome::Acked)], false); + scheduler.set_vm_tombstone(acked, VmTombstoneState::DeletePending); + assert_eq!( + scheduler.apply_delete_settlement(acked, settlement, false), + Ok(()) + ); + assert_eq!(scheduler.check_create_allowed(acked), Ok(())); +} + +#[test] +fn only_undelivered_asks_count_as_unreachable() { + use super::deletion::DeleteAttempt; + use kameo::error::RemoteSendError; + + let classify = + |error: RemoteSendError| DeleteAttempt::from_send_error(&error); + for never_delivered in [ + RemoteSendError::DialFailure, + RemoteSendError::ActorNotRunning, + RemoteSendError::SwarmNotBootstrapped, + ] { + assert!( + matches!(classify(never_delivered), DeleteAttempt::Unreachable(_)), + "never delivered" + ); + } + // a slow teardown on a live agent looks like these, so force can't skip them + for maybe_delivered in [ + RemoteSendError::NetworkTimeout, + RemoteSendError::ConnectionClosed, + RemoteSendError::ReplyTimeout, + RemoteSendError::ActorStopped, + RemoteSendError::MailboxFull, + ] { + assert!( + matches!(classify(maybe_delivered), DeleteAttempt::Unknown(_)), + "might have been delivered" + ); + } +} + +#[test] +fn force_never_drops_an_owner_that_might_still_be_deleting() { + use super::deletion::{ + DeleteAttempt, DeleteOwnerOutcome, delete_owner_outcome, plan_delete_settlement, + }; + + // a dead old route doesn't hide that the live one timed out + let outcome = delete_owner_outcome(vec![ + DeleteAttempt::Unreachable("dial failure".to_owned()), + DeleteAttempt::Unknown("network timeout".to_owned()), + ]); + assert_eq!( + outcome, + DeleteOwnerOutcome::Unknown(vec!["network timeout".to_owned()]) + ); + // but an explicit rejection or an ack still wins + assert!(matches!( + delete_owner_outcome(vec![ + DeleteAttempt::Unknown("timeout".to_owned()), + DeleteAttempt::AgentError("teardown failed".to_owned()), + ]), + DeleteOwnerOutcome::Rejected(_) + )); + assert_eq!( + delete_owner_outcome(vec![ + DeleteAttempt::Unknown("timeout".to_owned()), + DeleteAttempt::Acked, + ]), + DeleteOwnerOutcome::Acked + ); + + let slow = super::VmDeleteOwner::agent(super::ActorId::new(1)); + let settlement = plan_delete_settlement(vec![(slow, outcome)], true); + assert_eq!(settlement.dropped, Vec::new()); + assert_eq!(settlement.failures.len(), 1); + assert!(settlement.failures[0].contains("might still be deleting")); +} + +#[test] +fn stamps_always_move_forward() { + let mut scheduler = empty_scheduler(); + let first = scheduler.next_stamp(); + let second = scheduler.next_stamp(); + assert!(second > first); + // even if the clock steps back behind what we already handed out + scheduler.last_stamp = Ulid::from_parts(u64::MAX >> 16, 0); + let after_clock_skew = scheduler.next_stamp(); + assert!(after_clock_skew > Ulid::from_parts(u64::MAX >> 16, 0)); +} + +#[test] +fn create_is_refused_while_an_agent_runs_an_undiscovered_copy() { + use super::deletion::check_not_running_elsewhere; + + let vmid = Ulid::generate(); + let running_here = status_with(vec![vmid], Vec::new()); + let stopped_here = status_with(Vec::new(), vec![vmid]); + let unrelated = status_with(vec![Ulid::generate()], Vec::new()); + + // say right after a scheduler restart, before VM discovery found it + let error = check_not_running_elsewhere( + vmid, + [ + (super::ActorId::new(1), &unrelated), + (super::ActorId::new(2), &running_here), + ], + ) + .unwrap_err(); + assert!(error.contains("still report a running VM"), "{error}"); + + // a stopped VM's retained lease doesn't block starting it again + assert_eq!( + check_not_running_elsewhere( + vmid, + [ + (super::ActorId::new(1), &unrelated), + (super::ActorId::new(2), &stopped_here), + ], + ), + Ok(()) + ); +} + +#[test] +fn pending_placement_outlives_a_slow_but_successful_startup() { + use super::cache::pending_placement_timeout; + use crate::ch_driver::instance::startup_timeout; + + let with_disks = |count: usize| { + let mut manifest = test_manifest(1, 1); + manifest.desired.storage = (0..count) + .map(|index| crate::manifest::Storage { + id: format!("disk{index}"), + uri: Some(format!("rbd://pool/image{index}")), + volume_id: None, + read_only: false, + }) + .collect(); + manifest + }; + let one_disk = with_disks(1); + let three_disks = with_disks(3); + let disks = |manifest: &crate::manifest::VmManifest| manifest.desired.storage.len() + 1; + + assert!(pending_placement_timeout(Some(&one_disk)) > startup_timeout(disks(&one_disk))); + assert!(pending_placement_timeout(Some(&three_disks)) > startup_timeout(disks(&three_disks))); + // the scheduler's request timeout can't be what decides this + assert!(pending_placement_timeout(Some(&one_disk)) > Duration::from_secs(120)); +} diff --git a/odorobo/src/ch_driver/actor.rs b/odorobo/src/ch_driver/actor.rs index b336246..ef343ad 100644 --- a/odorobo/src/ch_driver/actor.rs +++ b/odorobo/src/ch_driver/actor.rs @@ -15,6 +15,7 @@ use crate::messages::vm::{ use crate::{ ch_driver::{VMInstance, manifest::to_vm_config}, manifest::VmManifest, + vmm_process::ProcessIdentity, vsock_cid::{PublishedRegistryUpdate, VsockCidAllocator, VsockCidReservation, VsockLeaseToken}, }; use kameo::prelude::*; @@ -34,7 +35,7 @@ pub struct MigrationState { /// The task handle for the migration process. pub migration_task: Option>>, /// set by the receive operation itself, before it sends the completion notice. - /// Teardown has to commit a successful receive even if the mailbox is closed + /// teardown has to commit a successful receive even if the mailbox is closed receive_succeeded: Arc, completion_task: Option>, receive_abort: Option, @@ -60,11 +61,13 @@ impl MigrationState { pub struct FailedStartupCleanup(pub String); /// shared with the agent, even if startup/teardown fails and there's no actor left. -/// A process exiting doesn't let us touch a lease some other VMM grabbed +/// a process exiting doesn't let us touch a lease some other VMM grabbed #[derive(Debug)] pub struct VMCleanupProof { process_exit_confirmed: AtomicBool, lease_token: std::sync::Mutex>, + /// lets a delete retry kill or confirm the VMM after the watcher is gone + process: std::sync::Mutex>, } impl VMCleanupProof { @@ -72,6 +75,7 @@ impl VMCleanupProof { Self { process_exit_confirmed: AtomicBool::new(process_exit_confirmed), lease_token: std::sync::Mutex::new(None), + process: std::sync::Mutex::new(None), } } @@ -79,7 +83,7 @@ impl VMCleanupProof { self.process_exit_confirmed.load(Ordering::SeqCst) } - fn set_process_exit_confirmed(&self, confirmed: bool) { + pub(crate) fn set_process_exit_confirmed(&self, confirmed: bool) { self.process_exit_confirmed .store(confirmed, Ordering::SeqCst); } @@ -97,6 +101,44 @@ impl VMCleanupProof { .lock() .expect("cleanup proof lock poisoned") = Some(token); } + + pub fn process(&self) -> Option { + *self.process.lock().expect("cleanup proof lock poisoned") + } + + fn set_process(&self, process: ProcessIdentity) { + *self.process.lock().expect("cleanup proof lock poisoned") = Some(process); + } + + /// identifies a freshly spawned VMM and writes it down right away. startup + /// can take a minute or more after the spawn, and if the agent dies in that + /// window a lease with no recorded process can't be cleaned up, even by + /// force delete + fn record_spawned_process( + &self, + allocator: &VsockCidAllocator, + vmid: ulid::Ulid, + pid: Option, + ) { + match pid.map(ProcessIdentity::of) { + Some(Ok(process)) => { + self.set_process(process); + self.record_process(allocator, vmid); + } + Some(Err(error)) => warn!(%vmid, ?error, "failed to identify VMM process"), + None => warn!(%vmid, "VMM process exited before we could identify it"), + } + } + + /// write the VMM down next to its lease so a restarted agent can check on it. + /// this is best effort, without it the lease just needs a manual cleanup + fn record_process(&self, allocator: &VsockCidAllocator, vmid: ulid::Ulid) { + if let (Some(token), Some(process)) = (self.lease_token(), self.process()) + && let Err(error) = allocator.record_process_owned(vmid, token, process) + { + warn!(%vmid, ?error, "failed to record VMM process for its vsock lease"); + } + } } const CONSOLE_SPOOL_SIZE: usize = 1024 * 1024; @@ -305,15 +347,20 @@ fn allocate_vsock_cid( return Ok(Some(reservation)); } // turning off vsock doesn't let us skip an older run's active lease after an - // agent restart. Stopping this new VMM wouldn't tell us the old one exited + // agent restart. stopping this new VMM wouldn't tell us the old one exited VsockCidAllocator::from_environment().ensure_no_active_lease(vmid)?; + // a CID observed somewhere else isn't in use here, so don't report it + if let Some(observed) = manifest.observed.as_mut() { + observed.vsock_guest_cid = None; + } Ok(None) } fn classify_cid_reservation_error(error: Report, cleanup_proof: &VMCleanupProof) -> Report { if let Some(token) = error .downcast_ref::() - .and_then(|published| published.reservation_token) + .and_then(|published| published.reservation) + .map(|reservation| reservation.token) { cleanup_proof.own_lease(token); error.wrap_err(FailedStartupCleanup( @@ -321,7 +368,7 @@ fn classify_cid_reservation_error(error: Report, cleanup_proof: &VMCleanupProof) )) } else { // validation or pre-publish failures might be about another running - // instance's lease. Don't give the agent anything that'd let it release it + // instance's lease. don't give the agent anything that'd let it release it error } } @@ -451,6 +498,13 @@ impl Actor for VMActor { vm_config_for_ch, boot, None, + |pid| { + cleanup_proof.record_spawned_process( + &VsockCidAllocator::from_environment(), + vmid, + pid, + ); + }, ) .await { @@ -529,6 +583,14 @@ impl Actor for VMActor { // Take the child process out so we can watch for unexpected death. // destroy() handles a missing child_process gracefully. let process_watcher = if let Some(mut child_process) = vminstance.take_child_process() { + // normally already recorded at spawn. try again if that failed + if cleanup_proof.process().is_none() { + cleanup_proof.record_spawned_process( + &VsockCidAllocator::from_environment(), + vmid, + child_process.id(), + ); + } let (kill_sender, kill_receiver) = tokio::sync::oneshot::channel(); let actor_ref = actor_ref.clone(); let watcher = tokio::spawn(async move { @@ -610,9 +672,8 @@ impl Actor for VMActor { } // collect every failure instead of bailing early, so one error doesn't hide another let mut errors = Vec::new(); - if let Err(error) = self.vm_instance.stop().await { - errors.push(error); - } + // a failed graceful shutdown only matters if we can't confirm the kill below + let stop_error = self.vm_instance.stop().await.err(); let process_exited = if let Some((kill_sender, watcher)) = self.process_watcher.take() { let _send_result: std::result::Result<(), ()> = kill_sender.send(()); match watcher.await { @@ -632,10 +693,20 @@ impl Actor for VMActor { } else { // without a watcher we can't tell if the VMM process exited, so keep // the reservations instead of risking giving a live guest's CID away + errors.push(Report::msg( + "VM has no process watcher, so its exit can't be confirmed", + )); false }; + if let Some(error) = stop_error { + if process_exited { + warn!(vmid = %self.vmid, ?error, "graceful VM shutdown failed, but the VMM was killed and reaped"); + } else { + errors.push(error); + } + } - // reaping closes the receive connection. Let its task see the final + // reaping closes the receive connection. let its task see the final // response before we decide whether to roll back the reservation if let Some(state) = self.migration_state.as_mut() && let Some(mut task) = state.completion_task.take() @@ -802,8 +873,25 @@ impl Message for VMActor { Ok(reservation) => reservation, Err(error) => { // allocate_vsock_cid only keeps the token if it actually got - // published. Getting rejected over another instance's lease - // never makes it ours + // published. getting rejected over another instance's lease + // never makes it ours. a published one still needs its + // previous CID put back, so hand it to on_stop for rollback + if let Some(reservation) = error + .downcast_ref::() + .and_then(|published| published.reservation) + { + self.migration_state = Some(MigrationState { + migration_task: None, + receive_succeeded: Arc::new(AtomicBool::new(false)), + completion_task: None, + receive_abort: None, + console_attach_task: None, + listening_address: String::new(), + cid_reservation: Some(reservation), + previous_manifest: self.manifest.clone(), + }); + ctx.stop(); + } return MigrateVMReceiveReply { listening_address: String::new(), error: Some(error.to_string()), @@ -811,7 +899,10 @@ impl Message for VMActor { } }; - // the actor holds onto this reservation until the process exits. If + self.cleanup_proof + .record_process(&VsockCidAllocator::from_environment(), self.vmid); + + // the actor holds onto this reservation until the process exits. if // anything below fails, we tear down the receiver and on_stop rolls it // back once the process watcher confirms exit let receive_succeeded = Arc::new(AtomicBool::new(false)); @@ -841,7 +932,7 @@ impl Message for VMActor { } }; - // do everything that can fail before opening the receiver. If something + // do everything that can fail before opening the receiver. if something // fails, stop the actor so on_stop rolls back after the process exits if let Err(error) = self.vm_instance.prep_config(config.clone()).await { ctx.stop(); @@ -930,7 +1021,15 @@ impl Message for VMActor { msg: MigrationFinished, ctx: &mut Context, ) -> Self::Reply { - if msg.succeeded { + // only the receive operation's own flag commits the CID, not whoever sent this + let succeeded = self + .migration_state + .as_ref() + .is_some_and(|state| state.receive_succeeded.load(Ordering::SeqCst)); + if msg.succeeded != succeeded && self.migration_state.is_some() { + warn!(vmid = %self.vmid, reported = msg.succeeded, succeeded, "migration finished notice disagrees with the receive result"); + } + if succeeded { if let Some(mut state) = self.migration_state.take() { if let Some(task) = state.console_attach_task.take() { task.abort(); @@ -946,6 +1045,9 @@ impl Message for VMActor { } else { warn!(vmid = %self.vmid, "received migration finished notification with no active migration state"); } + } else if msg.succeeded { + // claims success the receive never recorded, so don't commit or tear anything down + warn!(vmid = %self.vmid, "ignoring migration success notice without a successful receive"); } else if let Some(state) = self.migration_state.as_ref() { self.manifest.clone_from(&state.previous_manifest); warn!(vmid = %self.vmid, "migration failed; stopping receiver before rolling back CID reservation"); @@ -977,7 +1079,7 @@ mod vsock_manifest_tests { let vmid = ulid::Ulid::generate(); let reservation = allocator.reserve_with_previous(vmid, Some(42)).unwrap(); let registry = std::fs::read(®istry_path).unwrap(); - // write garbage to the registry so rollback fails. Unlike permissions, + // write garbage to the registry so rollback fails. unlike permissions, // this works the same whether tests run as root or not std::fs::write(®istry_path, b"invalid registry").unwrap(); let error = rollback_vsock_cid(&allocator, vmid, Some(reservation)).unwrap_err(); @@ -1016,6 +1118,33 @@ mod vsock_manifest_tests { std::fs::remove_dir_all(directory).unwrap(); } + #[tokio::test] + async fn spawned_vmm_is_recorded_against_its_lease_right_away() { + let directory = + std::env::temp_dir().join(format!("odorobo-spawn-record-{}", ulid::Ulid::generate())); + let allocator = VsockCidAllocator::new(directory.join("registry.json")); + let vmid = ulid::Ulid::generate(); + let reservation = allocator.reserve_with_previous(vmid, Some(48)).unwrap(); + let proof = VMCleanupProof::new(true); + proof.own_lease(reservation.token); + let mut child = tokio::process::Command::new("sleep") + .arg("30") + .kill_on_drop(true) + .spawn() + .unwrap(); + + proof.record_spawned_process(&allocator, vmid, child.id()); + + // a restarted agent can now find and kill it, even if we die before + // startup finishes + let recorded = allocator.recorded_process(vmid).unwrap().unwrap(); + assert_eq!(Some(recorded), proof.process()); + assert!(recorded.is_running().unwrap()); + child.kill().await.unwrap(); + assert!(!recorded.is_running().unwrap()); + std::fs::remove_dir_all(directory).unwrap(); + } + #[test] fn ordinary_reservation_rejection_never_claims_cleanup_ownership() { let proof = VMCleanupProof::new(true); @@ -1038,7 +1167,7 @@ mod vsock_manifest_tests { let error = classify_cid_reservation_error( stable_eyre::Report::new(crate::vsock_cid::PublishedRegistryUpdate { source: std::io::Error::other("directory sync failed"), - reservation_token: Some(reservation.token), + reservation: Some(reservation), }) .wrap_err("reserve failed"), &proof, diff --git a/odorobo/src/ch_driver/cloud_init.rs b/odorobo/src/ch_driver/cloud_init.rs index eb5eb6b..7408c78 100644 --- a/odorobo/src/ch_driver/cloud_init.rs +++ b/odorobo/src/ch_driver/cloud_init.rs @@ -25,7 +25,7 @@ static NEXT_TEMP_FILE: AtomicU64 = AtomicU64::new(0); /// create a FAT seed disk (attachable read-only) that `NoCloud` picks up as CIDATA /// /// the image lives in the VM runtime directory, which gets removed on VM teardown. -/// The file is private on the host since user/vendor-data might have credentials +/// the file is private on the host since user/vendor-data might have credentials pub fn create_seed_image(runtime_dir: &Path, cloud_init: &CloudInit) -> Result { create_seed_image_with_io(runtime_dir, cloud_init, std::convert::identity) } diff --git a/odorobo/src/ch_driver/instance.rs b/odorobo/src/ch_driver/instance.rs index c4eeac8..d502aac 100644 --- a/odorobo/src/ch_driver/instance.rs +++ b/odorobo/src/ch_driver/instance.rs @@ -38,6 +38,27 @@ pub type ConsoleStream = std::fs::File; const DEFAULT_RUNTIME_ROOT_DIR: &str = "/run/odorobo"; const RUNTIME_ROOT_ENV_VAR: &str = "ODOROBO_RUNTIME_DIR"; +/// how many times we ping a fresh VMM's API socket before giving up on it +const SOCKET_ATTEMPTS: u32 = 31; +const SOCKET_PING_TIMEOUT: std::time::Duration = std::time::Duration::from_millis(500); +const SOCKET_RETRY_DELAY: std::time::Duration = std::time::Duration::from_secs(1); +/// how long CH gets to create and boot a VM, on top of resolving its disks +const CH_CREATE_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(60); + +/// how long creating a VM with this many disks can take once the VMM is up +pub fn create_timeout(disks: usize) -> std::time::Duration { + CH_CREATE_TIMEOUT.saturating_add(super::transform::storage::create_wait(disks)) +} + +/// the longest [`VMInstance::spawn`] can take for a VM with this many disks, +/// from spawning the VMM to a booted VM +pub fn startup_timeout(disks: usize) -> std::time::Duration { + SOCKET_PING_TIMEOUT + .saturating_add(SOCKET_RETRY_DELAY) + .saturating_mul(SOCKET_ATTEMPTS) + .saturating_add(create_timeout(disks)) +} + pub struct VMInstance { pub id: String, pub ch_socket_path: PathBuf, @@ -387,16 +408,21 @@ impl VMInstance { /// Spawn a Cloud Hypervisor process and optionally create/boot its VM. /// + /// see [`startup_timeout`] for how long this can take at most + /// /// `vm_config` is already translated by the CH manifest boundary. The /// separate `boot` flag preserves the manifest's desired start behavior; /// callers can create a stopped VM without changing its provider config. /// The socket is polled for up to roughly 30 seconds before failing. + /// `on_spawned` gets the VMM's PID as soon as the process exists, before + /// any of that waiting, so callers can record it in case we die mid-startup #[tracing::instrument(skip_all)] - pub async fn spawn( + pub async fn spawn)>( id: &str, vm_config: Option, boot: bool, transformer: Option, + on_spawned: F, ) -> Result { let ch_socket_path = Self::runtime_dir_for(id).join(SOCKET_FILE_NAME); info!(?ch_socket_path, "Spawning VM"); @@ -409,24 +435,24 @@ impl VMInstance { .arg(&ch_socket_path) .kill_on_drop(true) .spawn()?; + on_spawned(ch_process.id()); let mut instance = Self::new(id, ch_socket_path, transformer, Some(ch_process)); - const MAX_ATTEMPTS: u32 = 31; - for attempt in 0..MAX_ATTEMPTS { + for attempt in 0..SOCKET_ATTEMPTS { info!(vm_id = id, attempt, "Checking if CH socket is available"); if matches!( - tokio::time::timeout( - std::time::Duration::from_millis(500), - instance.conn().vmm_ping_get() - ) - .await, + tokio::time::timeout(SOCKET_PING_TIMEOUT, instance.conn().vmm_ping_get()).await, Ok(Ok(_)) ) { info!(vm_id = id, "CH socket available"); if let Some(vm_config) = vm_config { info!(boot, ?vm_config, "Creating VM config"); + // storage resolves inside create_config, so the timeout has + // to cover every disk's budget, or a slow but working map + // gets torn down as a failed create + let disks = vm_config.disks.as_ref().map_or(0, Vec::len); let create_result = tokio::time::timeout( - std::time::Duration::from_secs(60), + create_timeout(disks), instance.create_config(vm_config, boot), ) .await @@ -441,15 +467,15 @@ impl VMInstance { return Ok(instance); } - if attempt < MAX_ATTEMPTS - 1 { - tokio::time::sleep(tokio::time::Duration::from_secs(1)).await; + if attempt < SOCKET_ATTEMPTS - 1 { + tokio::time::sleep(SOCKET_RETRY_DELAY).await; } } Err(startup_failure_error( eyre!( "CH socket not available after {} attempts for VM {}", - MAX_ATTEMPTS, + SOCKET_ATTEMPTS, id ), instance.cleanup_failed_start().await, @@ -555,7 +581,7 @@ impl VMInstance { fn purge_instance_data_at(&mut self, runtime_dir: &Path) -> Result<()> { // keep the original config and runtime files until all cleanup succeeds. - // A failed teardown might still need its disk IDs/paths on a later retry + // a failed teardown might still need its disk IDs/paths on a later retry let mut vm_config = self.vm_config.clone().unwrap_or_default(); self.transformer.teardown(self.vm_id(), &mut vm_config)?; remove_runtime_directory(runtime_dir, self.vm_id())?; @@ -704,7 +730,7 @@ fn startup_failure_error(startup_error: Report, cleanup_result: Result<()>) -> R fn remove_runtime_directory(runtime_dir: &Path, vmid: &str) -> Result<()> { // don't use exists(), a metadata error there would turn failed credential - // cleanup into success. Only an already-missing directory is fine + // cleanup into success. only an already-missing directory is fine match fs::remove_dir_all(runtime_dir) { Ok(()) => Ok(()), Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()), diff --git a/odorobo/src/ch_driver/transform/storage/command.rs b/odorobo/src/ch_driver/transform/storage/command.rs index d3fed76..86c1acf 100644 --- a/odorobo/src/ch_driver/transform/storage/command.rs +++ b/odorobo/src/ch_driver/transform/storage/command.rs @@ -1,4 +1,4 @@ -//! runs storage CLIs with a time limit. A CLI we killed might've already handed +//! runs storage CLIs with a time limit. a CLI we killed might've already handed //! work off to a daemon/kernel, so any error after spawn is always uncertain use super::super::StorageAcquisition; use stable_eyre::{Report, Result, eyre::eyre}; @@ -8,8 +8,10 @@ use tokio::{ process::{Child, Command}, }; -const COMMAND_TIMEOUT: Duration = Duration::from_secs(30); -const REAP_TIMEOUT: Duration = Duration::from_secs(2); +pub(in crate::ch_driver::transform::storage) const COMMAND_TIMEOUT: Duration = + Duration::from_secs(30); +// a killed CLI gets this long to be reaped before we stop waiting on it +pub(in crate::ch_driver::transform::storage) const REAP_TIMEOUT: Duration = Duration::from_secs(2); const OUTPUT_LIMIT: usize = 64 * 1024; pub(in crate::ch_driver::transform::storage) enum CommandFailure { @@ -36,7 +38,7 @@ impl CommandFailure { ) -> Result { match self { Self::BeforeSpawn(error) => Err(error), - // checking that it's not there right away isn't enough. A daemon or + // checking that it's not there right away isn't enough. a daemon or // kernel request could still finish after we killed/reaped the CLI Self::Uncertain(error) => Ok(StorageAcquisition::uncertain(error)), } @@ -53,7 +55,7 @@ impl RunningCommand { // the CLI's helpers might inherit its output pipes, so kill the whole // process group, not just the leader, before we try to reap it. // SAFETY: spawn put the child in a new group whose id is its PID. - // That id is always positive, so this can't hit our own process group + // that id is always positive, so this can't hit our own process group unsafe { libc::kill( self.group.checked_neg().expect("positive child PID"), @@ -72,7 +74,7 @@ impl RunningCommand { self.child.take(); } else { // a process stuck in an uninterruptible kernel call might not get - // reaped quickly. Leave a waiter in the background instead of blocking + // reaped quickly. leave a waiter in the background instead of blocking // storage forever. kill_on_drop also handles runtime shutdown/cancellation let mut child = self.child.take().expect("running child"); tokio::spawn(async move { @@ -92,7 +94,7 @@ impl Drop for RunningCommand { } } -/// `strict` makes going over the limit an error. Only stdout gets parsed, so a +/// `strict` makes going over the limit an error. only stdout gets parsed, so a /// chatty stderr just gets cut off instead of making the outcome uncertain async fn capture(mut reader: impl AsyncRead + Unpin, strict: bool) -> std::io::Result> { let mut output = Vec::new(); @@ -149,7 +151,7 @@ async fn run_with_timeout( let stderr = child.stderr.take().expect("piped stderr"); let output = tokio::time::timeout(timeout, async { let (stdout, stderr) = tokio::try_join!(capture(stdout, true), capture(stderr, false))?; - // reap after draining the pipes. If a helper inherited them, we hold on to + // reap after draining the pipes. if a helper inherited them, we hold on to // the leader's PID until the group is killed so the PID/group id can't get reused let status = child.wait().await?; Ok::<_, std::io::Error>(Output { diff --git a/odorobo/src/ch_driver/transform/storage/iscsi.rs b/odorobo/src/ch_driver/transform/storage/iscsi.rs index d24f9c3..28db943 100644 --- a/odorobo/src/ch_driver/transform/storage/iscsi.rs +++ b/odorobo/src/ch_driver/transform/storage/iscsi.rs @@ -6,7 +6,7 @@ use async_trait::async_trait; use serde::Deserialize; use stable_eyre::{ Result, - eyre::{WrapErr, eyre}, + eyre::{WrapErr, ensure, eyre}, }; use std::{ net::IpAddr, @@ -54,7 +54,7 @@ impl ISCSITarget { } /// open-iscsi keeps the node record's hostname in `persistent_address` even - /// when login used an IP. We use that persistent portal as the resource key, + /// when login used an IP. we use that persistent portal as the resource key, /// and it's also what udev by-path lookup and node logout actually go by async fn session_target_at(&self, root: &Path) -> Result> { let mut sessions = match tokio::fs::read_dir(root.join("iscsi_session")).await { @@ -171,9 +171,9 @@ impl ISCSITarget { }; if output.status.success() { // iscsiadm can exit zero without actually logging in (like if the - // session showed up after our absence check). We only count it as + // session showed up after our absence check). we only count it as // ours if it says the login worked and there's exactly one matching - // session. Just seeing a session could be someone else racing us + // session. just seeing a session could be someone else racing us if !confirms_login(&output.stdout, &self.iqn) { return Ok(StorageAcquisition::uncertain(eyre!( "iscsiadm exited successfully without confirming a new login" @@ -195,9 +195,9 @@ impl ISCSITarget { let Some(session) = self.session_target_at(root).await? else { return Ok(None); }; - // canonicalization happens before the registry reserves its key. If a + // canonicalization happens before the registry reserves its key. if a // session shows up in that gap, don't borrow it under a different key. - // Retrying resolution will find its persistent key + // retrying resolution will find its persistent key self.ensure_session_identity(&session)?; Ok(Some(StorageAcquisition::borrowed())) } @@ -207,7 +207,7 @@ impl ISCSITarget { Ok(Some(session)) => match self.ensure_session_identity(&session) { Ok(()) => StorageAcquisition::owned(), // login might've used an existing hostname node record or - // raced an alias acquisition. Never release a session under a + // raced an alias acquisition. never release a session under a // different key than the one we reserved before login Err(error) => StorageAcquisition::uncertain(error), }, @@ -291,7 +291,7 @@ fn confirms_login(stdout: &[u8], iqn: &str) -> bool { == 1 } -const DNS_TIMEOUT: Duration = Duration::from_secs(5); +pub(super) const DNS_TIMEOUT: Duration = Duration::from_secs(5); fn split_portal(portal: &str) -> Result<(&str, u16)> { let (host, port) = portal @@ -353,7 +353,7 @@ async fn portal_matches( return Ok(true); } // a hostname node's current DNS doesn't tell us which peer it's actually - // connected to. Its exact name still works even if DNS changes or lookup fails + // connected to. its exact name still works even if DNS changes or lookup fails if let Some(runtime) = runtime.and_then(|value| value.trim().trim_matches(['[', ']']).parse::().ok()) { @@ -374,23 +374,86 @@ impl From for PathBuf { } } -impl From<&Url> for ISCSITarget { - fn from(uri: &Url) -> Self { - let host_ip = uri.host_str().unwrap_or_default().to_owned(); - let port = uri.port().unwrap_or(3260); - let host = format_portal(&host_ip, port); - let mut path_segments = uri.path_segments().into_iter().flatten(); - let iqn = path_segments.next().unwrap_or_default().to_owned(); - let lun_str = path_segments.next().unwrap_or_default(); - let lun = lun_str - .strip_prefix("lun") - .unwrap_or(lun_str) - .parse::() - .unwrap_or(0); - Self { host, iqn, lun } +impl ISCSITarget { + /// validate the original URI before URL parsing can normalize dot segments or + /// strip URL whitespace/control characters, same idea as the RBD raw check + pub(super) fn parse_uri(value: &str) -> Result { + let resource = value + .strip_prefix("iscsi://") + .ok_or_else(|| eyre!("iSCSI storage URI must use the iscsi:// scheme"))?; + let parsed = + Url::parse(value).map_err(|error| eyre!("Invalid iSCSI storage URI: {error}"))?; + let target = Self::try_from(&parsed)?; + // the raw path has to be exactly what the parser kept. otherwise something + // like `/iqn/../other/0` would quietly log into a different target + let raw_path = resource.find('/').map_or("", |start| &resource[start..]); + ensure!( + raw_path == parsed.path(), + "iSCSI URI path must not change during URL normalization" + ); + Ok(target) + } +} + +impl TryFrom<&Url> for ISCSITarget { + type Error = stable_eyre::Report; + + /// accepts exactly `iscsi://[:]//`. a bad LUN is an + /// error, never LUN 0, since guessing boots the VM off the wrong disk + fn try_from(uri: &Url) -> Result { + ensure!( + uri.scheme() == "iscsi", + "iSCSI storage URI must use the iscsi:// scheme" + ); + ensure!( + uri.query().is_none() && uri.fragment().is_none(), + "iSCSI storage URI must not include a query or fragment" + ); + let host_ip = uri + .host_str() + .filter(|host| !host.is_empty()) + .ok_or_else(|| eyre!("iSCSI URI must have a portal host"))?; + // non-special URL hosts stay percent-encoded, and iscsiadm would get + // the encoded name as-is + ensure!( + !host_ip.contains('%'), + "iSCSI portal host must not be percent-encoded" + ); + let host = format_portal(host_ip, uri.port().unwrap_or(3260)); + let segments: Vec<_> = uri.path_segments().into_iter().flatten().collect(); + let [iqn, lun] = segments.as_slice() else { + return Err(eyre!( + "iSCSI URI must be iscsi:////, got {} path segment(s)", + segments.len() + )); + }; + ensure!(!iqn.is_empty(), "iSCSI URI must have a target IQN"); + // path segments aren't decoded, so an encoded IQN would log into the + // wrong target name + ensure!( + !iqn.contains('%'), + "iSCSI target IQN must not be percent-encoded" + ); + Ok(Self { + host, + iqn: (*iqn).to_owned(), + lun: parse_lun(lun)?, + }) } } +/// plain decimal digits that fit in a u32. the old `lun` form still works +fn parse_lun(segment: &str) -> Result { + let digits = segment.strip_prefix("lun").unwrap_or(segment); + ensure!( + !digits.is_empty() && digits.bytes().all(|byte| byte.is_ascii_digit()), + "iSCSI LUN must be plain decimal digits, got {segment:?}" + ); + digits + .parse() + .map_err(|error| eyre!("iSCSI LUN {segment:?} is out of range: {error}")) +} + // fn list_iscsi_devices() -> Result> { // // simply just list /dev/disk/by-path/ip-* for now @@ -445,15 +508,10 @@ pub struct ISCSIStorage; impl ISCSIStorage { async fn canonical_uri_at(uri: &Url, root: &Path) -> Result { - let target = ISCSITarget::from(uri); - if target.iqn.is_empty() { - return Err(eyre!("iSCSI URI must have a target IQN")); - } - if uri.host_str().is_none() { - return Err(eyre!("iSCSI URI must have a portal host")); - } + // fails here, before any login, if the URI is malformed + let target = ISCSITarget::try_from(uri)?; // the persistent portal stays the same across redirects and reconnects, - // unlike runtime addresses or a hostname's current DNS. Before login we + // unlike runtime addresses or a hostname's current DNS. before login we // keep the requested portal, and acquisition has to check that this exact // key is still the session's identity before reporting owned/borrowed let session = target.session_target_at(root).await?; @@ -470,8 +528,6 @@ impl ISCSIStorage { canonical .set_port(Some(port)) .map_err(|()| eyre!("Invalid iSCSI port"))?; - canonical.set_query(None); - canonical.set_fragment(None); Ok(canonical) } } @@ -486,16 +542,16 @@ impl StorageDriver for ISCSIStorage { } fn resource_key(&self, uri: &Url) -> Result { - let target = ISCSITarget::from(uri); + let target = ISCSITarget::try_from(uri)?; Ok(format!("iscsi://{}/{}", target.host, target.iqn)) } async fn acquire(&self, uri: &Url) -> Result { - ISCSITarget::from(uri).attach().await + ISCSITarget::try_from(uri)?.attach().await } async fn resolve(&self, uri: &Url) -> Result { - let target = ISCSITarget::from(uri); + let target = ISCSITarget::try_from(uri)?; let session = target.session_target().await?.ok_or_else(|| { eyre!( "No iSCSI session found for portal {} and IQN {}", @@ -508,7 +564,7 @@ impl StorageDriver for ISCSIStorage { } async fn release(&self, uri: &Url) -> Result<()> { - let target = ISCSITarget::from(uri); + let target = ISCSITarget::try_from(uri)?; target.detach().await } } @@ -524,10 +580,10 @@ mod tests { use tokio::sync::Semaphore; #[test] - fn resource_identity_is_session_not_lun_or_disk_id() { + fn resource_identity_is_session_not_lun() { let backend = ISCSIStorage; - let first = Url::parse("iscsi://127.0.0.1/iqn.test/0?id=first").unwrap(); - let second = Url::parse("iscsi://127.0.0.1:3260/iqn.test/lun1?id=second").unwrap(); + let first = Url::parse("iscsi://127.0.0.1/iqn.test/0").unwrap(); + let second = Url::parse("iscsi://127.0.0.1:3260/iqn.test/lun1").unwrap(); assert_eq!( backend.resource_key(&first).unwrap(), backend.resource_key(&second).unwrap() @@ -556,7 +612,8 @@ mod tests { tokio::fs::write(connection.join("persistent_port"), "3260\n") .await .unwrap(); - let mut target = ISCSITarget::from(&Url::parse("iscsi://127.0.0.1/iqn.test/99").unwrap()); + let mut target = + ISCSITarget::try_from(&Url::parse("iscsi://127.0.0.1/iqn.test/99").unwrap()).unwrap(); assert!(target.session_exists_at(&root).await.unwrap()); target.host = "127.0.0.2:3260".to_owned(); assert!(!target.session_exists_at(&root).await.unwrap()); @@ -629,7 +686,7 @@ mod tests { } async fn acquire(&self, uri: &Url) -> Result { - let target = ISCSITarget::from(uri); + let target = ISCSITarget::try_from(uri)?; if let Some(acquisition) = target.existing_acquisition_at(&self.root).await? { return Ok(acquisition); } @@ -641,14 +698,14 @@ mod tests { } async fn resolve(&self, uri: &Url) -> Result { - let target = ISCSITarget::from(uri); + let target = ISCSITarget::try_from(uri)?; let session = target.session_target_at(&self.root).await?.unwrap(); target.ensure_session_identity(&session)?; Ok(session.to_device_path()) } async fn release(&self, uri: &Url) -> Result<()> { - let target = ISCSITarget::from(uri); + let target = ISCSITarget::try_from(uri)?; let session = target.session_target_at(&self.root).await?.unwrap(); target.ensure_session_identity(&session)?; self.logouts.fetch_add(1, Ordering::SeqCst); @@ -680,7 +737,7 @@ mod tests { async fn redirected_owner_and_alias_share_the_key_reserved_before_login() { let state = SyntheticStorage::new("192.0.2.17", "192.0.2.18", false); let transform = synthetic_transformer(&state); - let owner_uri = Url::parse("iscsi://192.0.2.17/iqn.test/0?id=owner").unwrap(); + let owner_uri = Url::parse("iscsi://192.0.2.17/iqn.test/0").unwrap(); let before_login = ISCSIStorage::canonical_uri_at(&owner_uri, &state.root) .await .unwrap(); @@ -688,7 +745,7 @@ mod tests { resolve_synthetic(&transform, "owner", owner_uri.as_str()) .await .unwrap(); - let alias = "iscsi://192.0.2.18:3260/iqn.test/lun1?id=borrower"; + let alias = "iscsi://192.0.2.18:3260/iqn.test/lun1"; let path = resolve_synthetic(&transform, "borrower", alias) .await .unwrap(); @@ -799,7 +856,8 @@ mod tests { assert_eq!(state.logins.load(Ordering::SeqCst), 0); assert_eq!(state.logouts.load(Ordering::SeqCst), 0); assert!( - ISCSITarget::from(&Url::parse("iscsi://192.0.2.17/iqn.test/0").unwrap()) + ISCSITarget::try_from(&Url::parse("iscsi://192.0.2.17/iqn.test/0").unwrap()) + .unwrap() .session_exists_at(&state.root) .await .unwrap() @@ -809,7 +867,7 @@ mod tests { #[tokio::test] async fn hostname_owner_and_connected_alias_keep_the_prelogin_hostname_key() { - // this name has no DNS record on purpose. Alias lookup and final cleanup + // this name has no DNS record on purpose. alias lookup and final cleanup // should go by the node identity in sysfs, not a fresh DNS lookup let state = SyntheticStorage::new("missing-odorobo-node.invalid", "192.0.2.18", false); let transform = synthetic_transformer(&state); @@ -841,7 +899,8 @@ mod tests { let root = std::env::temp_dir().join(format!("odorobo-iscsi-dns-{}", ulid::Ulid::generate())); synthetic_session(&root, 17, "localhost\n", Some("127.0.0.1\n")).await; - let target = ISCSITarget::from(&Url::parse("iscsi://127.0.0.1/iqn.test/99").unwrap()); + let target = + ISCSITarget::try_from(&Url::parse("iscsi://127.0.0.1/iqn.test/99").unwrap()).unwrap(); let session = target.session_target_at(&root).await.unwrap().unwrap(); assert_eq!(session.host, "localhost:3260"); assert_eq!( @@ -860,7 +919,7 @@ mod tests { .await .unwrap(); // an existing node can keep a hostname even though we asked to log in by - // IP. We don't track aliases, so this isn't the key we reserved + // IP. we don't track aliases, so this isn't the key we reserved synthetic_session( &root, 17, @@ -868,7 +927,7 @@ mod tests { Some("192.0.2.17"), ) .await; - let target = ISCSITarget::from(&before); + let target = ISCSITarget::try_from(&before).unwrap(); assert!( target .existing_acquisition_at(&root) @@ -890,7 +949,7 @@ mod tests { ISCSIStorage.resource_key(&before).unwrap(), ISCSIStorage.resource_key(&after).unwrap() ); - let canonical_target = ISCSITarget::from(&after); + let canonical_target = ISCSITarget::try_from(&after).unwrap(); let borrowed = canonical_target .existing_acquisition_at(&root) .await @@ -932,15 +991,17 @@ mod tests { // localhost resolves to loopback, not the runtime address, so it shouldn't // make us think the old session belongs to a new canonical IP synthetic_session(&root, 17, "localhost\n", Some("192.0.2.17\n")).await; - let old = ISCSITarget::from(&Url::parse("iscsi://192.0.2.17/iqn.test/0").unwrap()); + let old = + ISCSITarget::try_from(&Url::parse("iscsi://192.0.2.17/iqn.test/0").unwrap()).unwrap(); assert_eq!( old.session_target_at(&root).await.unwrap().unwrap().host, "localhost:3260" ); - let new = ISCSITarget::from(&Url::parse("iscsi://127.0.0.1/iqn.test/0").unwrap()); + let new = + ISCSITarget::try_from(&Url::parse("iscsi://127.0.0.1/iqn.test/0").unwrap()).unwrap(); assert!(!new.session_exists_at(&root).await.unwrap()); // even with the DNS record gone, we should still resolve and log out via - // the exact node portal. We don't need to look up this .invalid hostname + // the exact node portal. we don't need to look up this .invalid hostname tokio::fs::write( root.join("iscsi_connection/connection17:0/persistent_address"), "missing-odorobo-node.invalid\n", @@ -964,7 +1025,8 @@ mod tests { let root = std::env::temp_dir().join(format!("odorobo-iscsi-unknown-{}", ulid::Ulid::generate())); synthetic_session(&root, 17, "missing-odorobo-node.invalid\n", None).await; - let target = ISCSITarget::from(&Url::parse("iscsi://127.0.0.1/iqn.test/0").unwrap()); + let target = + ISCSITarget::try_from(&Url::parse("iscsi://127.0.0.1/iqn.test/0").unwrap()).unwrap(); drop(target.session_exists_at(&root).await.unwrap_err()); tokio::fs::remove_dir_all(root).await.unwrap(); } @@ -1006,7 +1068,8 @@ mod tests { std::env::temp_dir().join(format!("odorobo-iscsi-multiple-{}", ulid::Ulid::generate())); synthetic_session(&root, 17, "127.0.0.1\n", Some("127.0.0.1\n")).await; synthetic_session(&root, 18, "localhost\n", Some("127.0.0.1\n")).await; - let target = ISCSITarget::from(&Url::parse("iscsi://127.0.0.1/iqn.test/0").unwrap()); + let target = + ISCSITarget::try_from(&Url::parse("iscsi://127.0.0.1/iqn.test/0").unwrap()).unwrap(); assert!( target .session_target_at(&root) @@ -1034,7 +1097,8 @@ mod tests { #[test] fn ipv6_portals_preserve_brackets_for_device_and_cli() { - let target = ISCSITarget::from(&Url::parse("iscsi://[::1]/iqn.test/0").unwrap()); + let target = + ISCSITarget::try_from(&Url::parse("iscsi://[::1]/iqn.test/0").unwrap()).unwrap(); assert_eq!(target.host, "[::1]:3260"); assert_eq!(split_portal(&target.host).unwrap(), ("::1", 3260)); assert_eq!(format_portal("::1", 3260), "[::1]:3260"); @@ -1058,14 +1122,14 @@ mod tests { let uri = Url::parse("iscsi://target.example.com:3260/iqn.2024-01.com.example:target1/lun0") .unwrap(); - let target = ISCSITarget::from(&uri); + let target = ISCSITarget::try_from(&uri).unwrap(); println!("{target:?}"); assert_eq!(target.host, "target.example.com:3260"); assert_eq!(target.iqn, "iqn.2024-01.com.example:target1"); assert_eq!(target.lun, 0); let uri2 = Url::parse("iscsi://target.example.com/iqn.2024-01.com.example:target1/0").unwrap(); - let target2 = ISCSITarget::from(&uri2); + let target2 = ISCSITarget::try_from(&uri2).unwrap(); println!("{target2:?}"); assert_eq!(target2.host, "target.example.com:3260"); assert_eq!(target2.iqn, "iqn.2024-01.com.example:target1"); @@ -1076,7 +1140,7 @@ mod tests { fn test_iscsi_lossless_conversion() { let uri = Url::parse("iscsi://target.example.com:3260/iqn.2024-01.com.example:target1/0") .unwrap(); - let target = ISCSITarget::from(&uri); + let target = ISCSITarget::try_from(&uri).unwrap(); let reconstructed_uri = format!("iscsi://{}/{}/{}", target.host, target.iqn, target.lun); assert_eq!(uri.as_str(), reconstructed_uri); } @@ -1086,7 +1150,7 @@ mod tests { let uri = Url::parse("iscsi://target.example.com:3260/iqn.2026-03.com.fyrastack:test/0").unwrap(); let actual_device_path = "/dev/disk/by-path/ip-target.example.com:3260-iscsi-iqn.2026-03.com.fyrastack:test-lun-0"; - let target = ISCSITarget::from(&uri); + let target = ISCSITarget::try_from(&uri).unwrap(); let device_path = target.to_device_path().display().to_string(); println!("Device path: {device_path}"); assert_eq!(device_path, actual_device_path); @@ -1121,16 +1185,112 @@ mod tests { // both numeric (/1) and prefixed (/lun1) URI forms for LUN > 0 let uri_numeric = Url::parse("iscsi://target.example.com:3260/iqn.2026-03.com.fyrastack:boot/1").unwrap(); - let t1 = ISCSITarget::from(&uri_numeric); + let t1 = ISCSITarget::try_from(&uri_numeric).unwrap(); assert_eq!(t1.lun, 1); let uri_prefixed = Url::parse("iscsi://target.example.com:3260/iqn.2026-03.com.fyrastack:boot/lun1") .unwrap(); - let t2 = ISCSITarget::from(&uri_prefixed); + let t2 = ISCSITarget::try_from(&uri_prefixed).unwrap(); assert_eq!(t2.lun, 1); } + #[test] + fn accepts_well_formed_iscsi_uris() { + for (input, host, lun) in [ + ( + "iscsi://san/iqn.2026-03.com.fyrastack:boot/0", + "san:3260", + 0, + ), + ( + "iscsi://san:3261/iqn.2026-03.com.fyrastack:boot/7", + "san:3261", + 7, + ), + ( + "iscsi://[::1]/iqn.2026-03.com.fyrastack:boot/lun2", + "[::1]:3260", + 2, + ), + ( + "iscsi://san/iqn.2026-03.com.fyrastack:boot/4294967295", + "san:3260", + u32::MAX, + ), + ] { + let target = ISCSITarget::parse_uri(input).unwrap_or_else(|error| { + panic!("rejected {input}: {error:#}"); + }); + assert_eq!(target.host, host, "{input}"); + assert_eq!(target.iqn, "iqn.2026-03.com.fyrastack:boot", "{input}"); + assert_eq!(target.lun, lun, "{input}"); + } + } + + #[test] + fn rejects_malformed_iscsi_uris_instead_of_guessing_lun_0() { + for input in [ + // LUNs that used to quietly become LUN 0 + "iscsi://san/iqn.x:tgt/lun-1", + "iscsi://san/iqn.x:tgt/-1", + "iscsi://san/iqn.x:tgt/+1", + "iscsi://san/iqn.x:tgt/1a", + "iscsi://san/iqn.x:tgt/0x1", + "iscsi://san/iqn.x:tgt/4294967296", + "iscsi://san/iqn.x:tgt/lun", + "iscsi://san/iqn.x:tgt/", + "iscsi://san/iqn.x:tgt", + "iscsi://san/", + "iscsi://san", + // extra segments used to get ignored + "iscsi://san/iqn.x:tgt/1/garbage", + "iscsi://san/iqn.x:tgt/1/", + "iscsi://san//1", + "iscsi:///iqn.x:tgt/1", + "iscsi://san/iqn.x:tgt/1?id=disk", + "iscsi://san/iqn.x:tgt/1#frag", + // encoded names aren't decoded, so they'd log into the wrong target + "iscsi://san/iqn.x%3Atgt/1", + "iscsi://san/iqn.x:has space/1", + "iscsi://s%61n/iqn.x:tgt/1", + ] { + let error = ISCSITarget::parse_uri(input); + assert!(error.is_err(), "accepted {input}"); + if let Ok(uri) = Url::parse(input) { + assert!(ISCSITarget::try_from(&uri).is_err(), "accepted {input}"); + } + } + } + + #[test] + fn rejects_iscsi_uris_the_url_parser_would_rewrite() { + for input in [ + "iscsi://san/iqn.x:other/../iqn.x:tgt/1", + "iscsi://san/./iqn.x:tgt/1", + "iscsi://san/iqn.x:tgt/\t1", + " iscsi://san/iqn.x:tgt/1", + "ISCSI://san/iqn.x:tgt/1", + ] { + assert!(ISCSITarget::parse_uri(input).is_err(), "accepted {input:?}"); + } + } + + #[tokio::test] + async fn malformed_lun_fails_before_any_login() { + let uri = Url::parse("iscsi://192.0.2.17/iqn.test/1a").unwrap(); + let error = ISCSIStorage.canonical_uri(&uri).await.unwrap_err(); + assert!(error.to_string().contains("LUN"), "{error:#}"); + drop( + ISCSIStorage + .acquire(&uri) + .await + .err() + .expect("acquire must fail"), + ); + drop(ISCSIStorage.resource_key(&uri).unwrap_err()); + } + #[test] fn test_iscsi_device_path_roundtrip() { // device_path → TryFrom → same ISCSITarget fields diff --git a/odorobo/src/ch_driver/transform/storage/mod.rs b/odorobo/src/ch_driver/transform/storage/mod.rs index d90a2b4..d9e05b6 100644 --- a/odorobo/src/ch_driver/transform/storage/mod.rs +++ b/odorobo/src/ch_driver/transform/storage/mod.rs @@ -40,7 +40,7 @@ impl StorageOwnership { } /// keeps "we know we own this" separate from "the command did something weird". -/// We only auto-release stuff we know we own. Anything uncertain gets quarantined +/// we only auto-release stuff we know we own. anything uncertain gets quarantined /// along with its VM lease until someone fixes it up by hand pub struct StorageAcquisition { ownership: StorageOwnership, @@ -79,7 +79,7 @@ impl StorageAcquisition { } /// a command that failed (or that we couldn't wait on) might've still created the - /// resource, or raced someone else creating it. We only drop the lease if we know + /// resource, or raced someone else creating it. we only drop the lease if we know /// for sure it's not there fn after_failed_command(error: Report, exists: Result) -> Result { match exists { @@ -92,8 +92,8 @@ impl StorageAcquisition { } } -/// a release command we spawned but don't know if it finished. The daemon/kernel -/// side of it could still land after a retry thinks the resource is gone. This is +/// a release command we spawned but don't know if it finished. the daemon/kernel +/// side of it could still land after a retry thinks the resource is gone. this is /// on purpose separate from a command that exited nonzero (that one's retryable) #[derive(Debug, thiserror::Error)] #[error("Storage release outcome is uncertain: {0:#}")] @@ -105,17 +105,17 @@ pub(crate) fn uncertain_release(error: Report) -> Report { StorageReleaseUncertain(error).into() } -/// a storage backend. Acquiring is separate from resolving the path so we still -/// own the resource even if finding the device fails later. If acquire returns an -/// error, there's nothing to track. Use `StorageAcquisition::partial` when we know +/// a storage backend. acquiring is separate from resolving the path so we still +/// own the resource even if finding the device fails later. if acquire returns an +/// error, there's nothing to track. use `StorageAcquisition::partial` when we know /// we own something but setup failed, or `uncertain` when we can't tell #[async_trait] pub trait StorageDriver: Send + Sync { fn scheme(&self) -> &'static str; /// normalize the connection identity before acquiring and keep it around for - /// release. Every alias we accept has to map to the same key, including the - /// key we reserved before acquiring. If the identity we discover is different, + /// release. every alias we accept has to map to the same key, including the + /// key we reserved before acquiring. if the identity we discover is different, /// refuse to borrow before doing anything, or return uncertain if we already did async fn canonical_uri(&self, uri: &Url) -> Result { let mut uri = uri.clone(); @@ -124,7 +124,7 @@ pub trait StorageDriver: Send + Sync { Ok(uri) } - /// identity of the thing this backend releases. Not always a single disk, like + /// identity of the thing this backend releases. not always a single disk, like /// iSCSI logout drops the whole session along with all its LUNs fn resource_key(&self, uri: &Url) -> Result { Ok(uri.to_string()) @@ -135,16 +135,37 @@ pub trait StorageDriver: Send + Sync { /// finds a local path without acquiring anything else async fn resolve(&self, uri: &Url) -> Result; - /// a failure that actually finished can be retried. If we spawned a command + /// a failure that actually finished can be retried. if we spawned a command /// and don't know if it finished, return `uncertain_release(error)` so a later /// retry that sees nothing there can't hand it out again before a late detach async fn release(&self, uri: &Url) -> Result<()>; } // wait deadlines only limit how long the caller waits, not the backend future. -// So block_in_place/Handle::block_on callers get control back without cancelling -// a map/logout command whose subprocess or daemon might still be doing stuff -const OPERATION_WAIT: Duration = Duration::from_secs(30); +// so block_in_place/Handle::block_on callers get control back without cancelling +// a map/logout command whose subprocess or daemon might still be doing stuff. +// it still has to cover the slowest work that can actually succeed, or a slow +// but fine create gets reported as failed and torn down. worst cases per op: +// - RBD resolve: `device list` + `device map` + `device list` to acquire, then +// the udev wait and one more `device list`. that's the most commands, 4 +// - RBD release: `device list` + `device unmap` + `device list` +// - iSCSI: one login/logout, but each sysfs scan can do a DNS lookup (canonical +// lookup, pre-login check, post-login check, resolve/logout check) +// a stuck command gets its timeout plus reap grace before we give up on it. +// adding the RBD and iSCSI extras together over-counts a bit, which is fine +const MAX_COMMANDS_PER_OPERATION: u32 = 4; +const MAX_DNS_LOOKUPS_PER_OPERATION: u32 = 4; +const OPERATION_WAIT: Duration = iscsi::command::COMMAND_TIMEOUT + .saturating_add(iscsi::command::REAP_TIMEOUT) + .saturating_mul(MAX_COMMANDS_PER_OPERATION) + .saturating_add(rbd::UDEV_WAIT) + .saturating_add(iscsi::DNS_TIMEOUT.saturating_mul(MAX_DNS_LOOKUPS_PER_OPERATION)); + +/// the longest VM create can spend on storage while it still succeeds. each +/// disk gets its own operation budget, one after another +pub fn create_wait(disks: usize) -> Duration { + OPERATION_WAIT.saturating_mul(u32::try_from(disks).unwrap_or(u32::MAX)) +} struct Operation { result: watch::Sender>>, @@ -211,7 +232,7 @@ struct Resource { struct StorageRegistry { resources: HashMap, leases: HashMap>, - // claim the VM before we even do the canonical lookup. Otherwise cleanup could + // claim the VM before we even do the canonical lookup. otherwise cleanup could // say it's done while a detached lookup goes on to acquire something after resolving: HashMap>>>, cleaning: HashMap>>, @@ -230,17 +251,17 @@ impl StorageRegistry { } } -// each VM gets its own transform chain. The registry lock is only held for quick -// bookkeeping. The per-key operation states are what keep external work in order. -// Spawned tasks keep going even if the waiter gets cancelled or the transformer is -// dropped. Same as before, if the agent restarts someone has to reconcile all of +// each VM gets its own transform chain. the registry lock is only held for quick +// bookkeeping. the per-key operation states are what keep external work in order. +// spawned tasks keep going even if the waiter gets cancelled or the transformer is +// dropped. same as before, if the agent restarts someone has to reconcile all of // its storage by hand fn shared_registry() -> Arc> { static REGISTRY: OnceLock>> = OnceLock::new(); Arc::clone(REGISTRY.get_or_init(Arc::default)) } -/// resolves disk URIs and only tracks resources each VM actually acquired. The URIs +/// resolves disk URIs and only tracks resources each VM actually acquired. the URIs /// in the config just describe the disk, they don't mean we own anything pub struct StorageDriverTransformer { backends: Vec>, @@ -425,7 +446,7 @@ impl StorageDriverTransformer { .expect("in-flight acquisition retains its resource"); resource.ownership = acquisition.ownership; // uncertain ownership keeps its own manual recovery error. - // Partial ownership we know about can be cleaned up, not shared + // partial ownership we know about can be cleaned up, not shared resource.state = if matches!(resource.ownership, StorageOwnership::Uncertain(_)) { ResourceState::Ready @@ -438,7 +459,7 @@ impl StorageDriverTransformer { } Err(error) => { // backends only return Err when the resource definitely - // isn't there. If they can't tell, they return uncertain + // isn't there. if they can't tell, they return uncertain ledger.resources.remove(&key); ledger.remove_lease(&vmid, &key); Err(error) @@ -449,12 +470,13 @@ impl StorageDriverTransformer { }); } - /// releases only this VM's leases, once we know the process exited. If the + /// releases only this VM's leases, once we know the process exited. if the /// caller is cancelled or times out, cleanup keeps running and the VM can't be - /// reused yet. We never release pre-existing or uncertain mappings. Failures keep + /// reused yet. we never release pre-existing or uncertain mappings. failures keep /// their leases so they can be retried, even from a fresh default transformer pub async fn release_vm(&self, vmid: &str) -> Result<()> { let mut registry = self.registry.lock().await; + let deadline = self.cleanup_wait(®istry, vmid); let operation = if let Some(operation) = registry.cleaning.get(vmid) { Arc::clone(operation) } else { @@ -489,7 +511,19 @@ impl StorageDriverTransformer { operation }; drop(registry); - operation.wait_bounded(self.operation_wait).await + operation.wait_bounded(deadline).await + } + + /// cleanup goes through a VM's disks one at a time, so give it one operation + /// budget per step instead of squeezing every disk into one. a pending + /// resolve has to finish first and can add a lease of its own, so it counts + /// twice. joining an in-flight cleanup only has what's left to wait on + fn cleanup_wait(&self, registry: &StorageRegistry, vmid: &str) -> Duration { + let leases = registry.leases.get(vmid).map_or(0, Vec::len); + let pending = registry.resolving.get(vmid).map_or(0, Vec::len); + let steps = leases.saturating_add(pending.saturating_mul(2)).max(1); + self.operation_wait + .saturating_mul(u32::try_from(steps).unwrap_or(u32::MAX)) } async fn release_owned(registry: Arc>, vmid: &str) -> Result<()> { @@ -529,7 +563,7 @@ impl StorageDriverTransformer { let operation = Arc::clone(operation); drop(ledger); // the task that owns this updates the lease and refcount all at once. - // Since VM cleanup is serialized, this is usually just waiting on a release + // since VM cleanup is serialized, this is usually just waiting on a release return operation.wait().await; } if let Some(error) = resource.ownership.uncertainty_error(key) { @@ -589,6 +623,68 @@ impl Default for StorageDriverTransformer { } } +fn raw_uri_scheme(path: &str) -> Option { + // the URL parser ignores these characters in parts of its input and trims + // leading C0 controls and spaces. we only mirror that preprocessing so we can + // spot malformed supported URIs. validation still uses `path` as-is. + // we skip trailing trimming since it can never change the scheme + let normalized: String = path + .chars() + .filter(|&character| !matches!(character, '\t' | '\n' | '\r')) + .skip_while(|&character| character <= '\u{20}') + .collect(); + + let (scheme, _) = normalized.split_once(':')?; + let mut characters = scheme.chars(); + // the parser lowercases schemes, so match that to catch e.g. `RBD://` + (characters.next()?.is_ascii_alphabetic() + && characters.all(|character| { + character.is_ascii_alphanumeric() || matches!(character, '+' | '-' | '.') + })) + .then(|| scheme.to_ascii_lowercase()) +} + +fn parse_storage_uri(path: &str) -> Result> { + match Url::parse(path) { + Ok(uri) => { + // check every URL the parser thinks is RBD or iSCSI against the + // original input. this rejects casing and parser cleanup/normalization + // instead of quietly mapping a fixed-up URI + match uri.scheme() { + "rbd" => { + rbd::RbdImage::parse_uri(path)?; + } + "iscsi" => { + iscsi::ISCSITarget::parse_uri(path)?; + } + _ => {} + } + Ok(Some(uri)) + } + Err(error) + if raw_uri_scheme(path).as_deref() == Some("rbd") + || ["file://", "iscsi://"] + .iter() + .any(|prefix| path.starts_with(prefix)) => + { + Err(eyre!("Invalid supported storage URI: {error}")) + } + // plain local paths like `/var/lib/disk.img` are supposed to land here + // and get left alone. only warn when the input looks like a URI + Err(error) => { + if let Some(scheme) = raw_uri_scheme(path) { + warn!( + scheme, + path, + %error, + "Disk path looks like a URI but failed to parse, leaving disk path unchanged" + ); + } + Ok(None) + } + } +} + impl ConfigTransform for StorageDriverTransformer { fn teardown(&self, vmid: &str, _config: &mut VmConfig) -> Result<()> { tokio::task::block_in_place(|| { @@ -605,7 +701,7 @@ impl ConfigTransform for StorageDriverTransformer { let Some(path) = disk.path.as_deref() else { continue; }; - let Ok(uri) = Url::parse(path) else { + let Some(uri) = parse_storage_uri(path)? else { continue; }; let Some(backend) = self.find_backend(&uri) else { @@ -835,6 +931,169 @@ mod tests { .await } + #[test] + fn default_deadline_covers_worst_case_backend_work() { + let command = iscsi::command::COMMAND_TIMEOUT + iscsi::command::REAP_TIMEOUT; + // RBD acquire + resolve is the slowest path that can still succeed + assert!(OPERATION_WAIT >= command * 4 + rbd::UDEV_WAIT); + assert!(OPERATION_WAIT >= command + iscsi::DNS_TIMEOUT * 4); + } + + #[test] + fn cleanup_deadline_scales_with_serial_steps() { + let transform = StorageDriverTransformer { + registry: Arc::default(), + operation_wait: Duration::from_secs(10), + ..StorageDriverTransformer::new() + }; + let mut registry = StorageRegistry::default(); + assert_eq!( + transform.cleanup_wait(®istry, "vm"), + Duration::from_secs(10) + ); + registry + .leases + .insert("vm".to_owned(), vec!["a".to_owned(), "b".to_owned()]); + registry + .resolving + .insert("vm".to_owned(), vec![Operation::new()]); + assert_eq!( + transform.cleanup_wait(®istry, "vm"), + Duration::from_secs(40) + ); + assert_eq!( + transform.cleanup_wait(®istry, "other"), + Duration::from_secs(10) + ); + } + + #[test] + fn malformed_supported_storage_uri_syntax_is_not_passed_through() { + for path in [ + "rbd://[", + "RBD://[", + "file://[", + "rbd://pool/../other-image", + "rbd://pool/a/../image", + "rbd://pool/a/%2e%2e/image", + "RBD://pool/image", + "rBd://pool/image", + " rbd://pool/image", + "\trbd://pool/image", + "rbd://pool/\nimage", + "\u{1}rbd://pool/image", + "ISCSI://san/iqn.test/0", + "iscsi://san/iqn.other/../iqn.test/0", + "iscsi://san/iqn.test/1a", + "iscsi://san/iqn.test/0/extra", + ] { + assert!(parse_storage_uri(path).is_err(), "accepted {path:?}"); + } + assert!( + parse_storage_uri("/var/lib/odorobo/disk.raw") + .unwrap() + .is_none() + ); + assert!( + parse_storage_uri("custom://storage/disk") + .unwrap() + .is_some() + ); + assert!(parse_storage_uri("rbd://pool/image").unwrap().is_some()); + assert!( + parse_storage_uri("iscsi://san/iqn.test/lun1") + .unwrap() + .is_some() + ); + assert!( + parse_storage_uri("rbd://rbd-pool_1.ssd/image") + .unwrap() + .is_some() + ); + assert!( + parse_storage_uri("rbd://UPPER/image") + .unwrap_err() + .to_string() + .contains("must be lowercase") + ); + } + + struct RbdMockStorage(Arc>); + + #[async_trait] + impl StorageDriver for RbdMockStorage { + fn scheme(&self) -> &'static str { + "rbd" + } + + async fn acquire(&self, uri: &Url) -> Result { + self.0.lock().unwrap().acquisitions.push(uri.to_string()); + Ok(StorageAcquisition::owned()) + } + + async fn resolve(&self, _uri: &Url) -> Result { + Ok(PathBuf::from("/dev/fake-rbd")) + } + + async fn release(&self, uri: &Url) -> Result<()> { + self.0.lock().unwrap().releases.push(uri.to_string()); + Ok(()) + } + } + + #[tokio::test(flavor = "multi_thread")] + async fn malformed_rbd_uris_are_rejected_before_backend_or_lease_changes() { + let state = Arc::new(StdMutex::new(MockState::default())); + let mut transformer = + StorageDriverTransformer::new().with_backend(RbdMockStorage(Arc::clone(&state))); + transformer.registry = Arc::clone(&state.lock().unwrap().registry); + + for (index, path) in [ + "RBD://pool/image", + "rBd://pool/image", + " rbd://pool/image", + "\trbd://pool/image", + "rbd://pool/\nimage", + "\u{1}rbd://pool/image", + "rbd://pool/a/../image", + "rbd://pool/a/%2e%2e/image", + "RBD://[", + ] + .into_iter() + .enumerate() + { + let vmid = format!("malformed-rbd-{index}"); + let mut vm_config = config(&[path]); + let error = transformer + .transform(&vmid, &mut vm_config) + .expect_err("an invalid RBD source must fail closed"); + assert!(error.to_string().contains("RBD") || error.to_string().contains("URI")); + assert_eq!( + vm_config.disks.as_ref().unwrap()[0].path.as_deref(), + Some(path) + ); + assert_eq!( + vm_config.disks.as_ref().unwrap()[0].id.as_deref(), + Some("disk") + ); + + let registry = { + let backend_state = state.lock().unwrap(); + assert!( + backend_state.acquisitions.is_empty(), + "backend acquired for {path:?}" + ); + assert!(backend_state.releases.is_empty()); + Arc::clone(&backend_state.registry) + }; + let registry = registry.lock().await; + assert!(registry.resources.is_empty()); + assert!(registry.leases.is_empty()); + assert!(registry.resolving.is_empty()); + drop(registry); + } + } + fn config(paths: &[&str]) -> VmConfig { VmConfig { disks: Some( @@ -1421,7 +1680,7 @@ mod tests { registry.leases[&vmid].as_slice(), std::slice::from_ref(&uri) ); - // only the test resets this key. Production never clears uncertainty + // only the test resets this key. production never clears uncertainty registry.resources.remove(&uri); registry.leases.remove(&vmid); } diff --git a/odorobo/src/ch_driver/transform/storage/rbd.rs b/odorobo/src/ch_driver/transform/storage/rbd.rs index 693f8e8..ab8a55e 100644 --- a/odorobo/src/ch_driver/transform/storage/rbd.rs +++ b/odorobo/src/ch_driver/transform/storage/rbd.rs @@ -1,7 +1,10 @@ use super::{StorageAcquisition, StorageDriver, iscsi::command}; use async_trait::async_trait; -use stable_eyre::{Result, eyre::eyre}; -use std::path::PathBuf; +use stable_eyre::{ + Result, + eyre::{ensure, eyre}, +}; +use std::{path::PathBuf, time::Duration}; use tokio::process::Command; use tracing::{info, trace}; use url::Url; @@ -11,6 +14,10 @@ const CEPH_KEYFILE_ENV: &str = "CEPH_KEYFILE"; const CEPH_CLUSTER_ENV: &str = "CEPH_CLUSTER"; const CEPH_CONFIG_ENV: &str = "CEPH_CONFIG"; +const UDEV_POLL: Duration = Duration::from_millis(100); +/// how long resolve waits for the udev stable path before falling back +pub(super) const UDEV_WAIT: Duration = Duration::from_secs(10); + fn rbd_extra_args() -> Vec { let mut args = Vec::new(); if let Ok(ceph_config) = std::env::var(CEPH_CONFIG_ENV) { @@ -113,7 +120,7 @@ impl RbdImage { PathBuf::from(format!("/dev/rbd/{}/{}", self.pool, self.image)) } - /// maps the RBD image. If it's already mapped we borrow it instead of owning it + /// maps the RBD image. if it's already mapped we borrow it instead of owning it #[tracing::instrument(skip(self))] pub async fn map(&self) -> Result { let rbd_path = self.rbd_path(); @@ -148,7 +155,7 @@ impl RbdImage { String::from_utf8_lossy(&output.stderr) ); // it not being there before we ran the command doesn't mean a mapping now - // is ours. Something else could've mapped it while our command failed + // is ours. something else could've mapped it while our command failed let exists = rbd_map_list() .await .map(|mappings| mappings.iter().any(|(path, _)| path == &rbd_path)); @@ -193,21 +200,96 @@ impl RbdImage { } } +impl RbdImage { + /// validate the original URI before URL parsing can normalize dot segments or + /// strip URL whitespace/control characters. the raw lowercase rule gets + /// enforced here, since the `rbd` URL parser doesn't normalize pool-name casing + pub(super) fn parse_uri(value: &str) -> Result { + let resource = value + .strip_prefix("rbd://") + .ok_or_else(|| eyre!("RBD storage URI must use the rbd:// scheme"))?; + let (pool, image) = resource + .split_once('/') + .ok_or_else(|| eyre!("RBD URI must have a pool and one image path segment"))?; + ensure!( + !pool.chars().any(|character| character.is_ascii_uppercase()), + "RBD pool names in the URI must be lowercase" + ); + validate_rbd_component(pool, "pool")?; + ensure!( + !image.contains('/'), + "RBD URI supports only rbd:///; namespaces and nested image paths are not supported" + ); + validate_rbd_component(image, "image")?; + + let parsed = + Url::parse(value).map_err(|error| eyre!("Invalid RBD storage URI: {error}"))?; + let parsed_image = Self::try_from(&parsed)?; + // refusing any parser rewrite means we can't quietly pick a different + // pool or image than the one the original URI named + ensure!( + parsed_image.pool == pool && parsed_image.image == image, + "RBD URI components must not change during URL normalization" + ); + Ok(parsed_image) + } +} + impl TryFrom<&Url> for RbdImage { type Error = stable_eyre::Report; fn try_from(uri: &Url) -> Result { + ensure!( + uri.scheme() == "rbd", + "RBD storage URI must use the rbd:// scheme" + ); + ensure!( + uri.username().is_empty() + && uri.password().is_none() + && uri.port().is_none() + && uri.query().is_none() + && uri.fragment().is_none(), + "RBD storage URI must not include credentials, a port, query, or fragment" + ); + let pool = uri .host_str() .ok_or_else(|| eyre!("RBD URI must have a host (pool name)"))? .to_owned(); - let path = uri.path(); - if path.is_empty() || path == "/" { - return Err(eyre!("RBD URI must have a path (image name)")); - } - let image = path.trim_start_matches('/').to_owned(); - Ok(Self { pool, image }) + validate_rbd_component(&pool, "pool")?; + + let image = uri + .path() + .strip_prefix('/') + .ok_or_else(|| eyre!("RBD URI must have exactly one image path segment"))?; + ensure!( + !image.contains('/'), + "RBD URI supports only rbd:///; namespaces and nested image paths are not supported" + ); + validate_rbd_component(image, "image")?; + + Ok(Self { + pool, + image: image.to_owned(), + }) + } +} + +fn validate_rbd_component(value: &str, name: &str) -> Result<()> { + if value.is_empty() + || value == "." + || value == ".." + || value.starts_with('-') + || value.contains('%') + || value.chars().any(|character| { + !(character.is_ascii_alphanumeric() || matches!(character, '_' | '-' | '.')) + }) + { + return Err(eyre!( + "RBD {name} must be a non-empty name using only ASCII letters, digits, '.', '_' or '-'" + )); } + Ok(()) } pub struct RbdStorage; @@ -218,6 +300,14 @@ impl StorageDriver for RbdStorage { "rbd" } + async fn canonical_uri(&self, uri: &Url) -> Result { + // unlike generic URI options, RBD query/fragment data doesn't mean anything + // to the backend. reject it instead of quietly dropping it and mapping + // a different image than the manifest asked for + RbdImage::try_from(uri)?; + Ok(uri.clone()) + } + fn resource_key(&self, uri: &Url) -> Result { Ok(format!("rbd://{}", RbdImage::try_from(uri)?.rbd_path())) } @@ -228,19 +318,20 @@ impl StorageDriver for RbdStorage { async fn resolve(&self, uri: &Url) -> Result { let image = RbdImage::try_from(uri)?; - // map() uses `--options noudev`: with udev enabled, the rbd CLI waits + // map() uses `--options noudev`. with udev enabled, the rbd CLI waits // for a udev event in its own network namespace, but rbd devices are // created in the host's namespace, so that wait never completes in a - // container. The kernel uevent still reaches the host's udevd, so the - // stable path appears shortly after the map. Wait up to 10s (100 x - // 100ms steps) for it, and fall back to the kernel device name (e.g. - // /dev/rbd0) when the host has no ceph udev rule. + // container. the kernel uevent still reaches the host's udevd, so the + // stable path appears shortly after the map. wait up to UDEV_WAIT for + // it, and fall back to the kernel device name (e.g. /dev/rbd0) when the + // host has no ceph udev rule let udev_path = image.device_path(); - for _ in 0..100 { + let started = tokio::time::Instant::now(); + while started.elapsed() < UDEV_WAIT { if udev_path.exists() { return Ok(udev_path); } - tokio::time::sleep(std::time::Duration::from_millis(100)).await; + tokio::time::sleep(UDEV_POLL).await; } let device = rbd_map_list() .await? @@ -280,6 +371,27 @@ mod tests { ); } + #[test] + fn rejects_unsupported_rbd_uri_components() { + for input in [ + "file:///pool/image", + "rbd:///image", + "rbd://pool/", + "rbd://pool/image/", + "rbd://pool/ns/image", + "rbd://pool/image@snapshot", + "rbd://pool/image?read_only=true", + "rbd://pool/image#snapshot", + "rbd://user@pool/image", + "rbd://pool:6789/image", + "rbd://pool/has%20space", + "rbd://pool/-option", + ] { + let uri = Url::parse(input).expect("test URI parses"); + assert!(RbdImage::try_from(&uri).is_err(), "accepted {input}"); + } + } + #[test] fn mapping_identity_includes_namespace_and_snapshot() { let mappings = rbd_lines_list( diff --git a/odorobo/src/http_api/vms.rs b/odorobo/src/http_api/vms.rs index cbbb853..8120f09 100644 --- a/odorobo/src/http_api/vms.rs +++ b/odorobo/src/http_api/vms.rs @@ -6,17 +6,25 @@ use crate::{ types::{CreateVMRequest, VMListResponse, VmId}, utils::OdoroboError, }; -use aide::axum::{ - ApiRouter, IntoApiResponse, - routing::{delete, get, patch, post, put}, +use aide::{ + OperationInput, + axum::{ + ApiRouter, IntoApiResponse, + routing::{delete, get, patch, post, put}, + }, + generate::GenContext, + openapi::Operation, + operation::{ParamLocation, add_parameters, parameters_from_schema}, }; use axum::{ Json, - extract::{Path, State}, - http::header, + extract::{FromRequestParts, Path, Query, State}, + http::{header, request::Parts}, response::IntoResponse, }; use kameo::actor::ActorRef; +use schemars::JsonSchema; +use serde::Deserialize; pub fn router() -> ApiRouter> { ApiRouter::new() @@ -61,6 +69,7 @@ async fn create_vm( let message = CreateVM { vmid: request.vm.id, config: request.vm, + generation: None, }; let reply = state.ask(message).await?; @@ -70,11 +79,49 @@ async fn create_vm( Ok(Json(response)) } +/// query params for `DELETE /vms/{vmid}` +#[derive(Deserialize, JsonSchema, Debug, Default)] +struct DeleteVmQuery { + /// give up on cleanup owners whose agent can't be reached instead of + /// failing the delete. an agent that answers with an error still fails it. + /// whatever an unreachable node still holds is leaked until it comes back + #[serde(default)] + force: bool, +} + +/// `Query` with docs. aide only documents `Query` with its +/// `axum-query` feature, so this does the same thing by hand +struct DeleteVmParams(DeleteVmQuery); + +impl FromRequestParts for DeleteVmParams { + type Rejection = as FromRequestParts>::Rejection; + + async fn from_request_parts(parts: &mut Parts, state: &S) -> Result { + let Query(query) = Query::from_request_parts(parts, state).await?; + Ok(Self(query)) + } +} + +impl OperationInput for DeleteVmParams { + fn operation_input(ctx: &mut GenContext, operation: &mut Operation) { + let schema = ctx.schema.subschema_for::(); + let params = parameters_from_schema(ctx, schema, ParamLocation::Query); + add_parameters(ctx, operation, params); + } +} + async fn delete_vm( State(state): State>, Path(VmId(vmid)): Path, + DeleteVmParams(query): DeleteVmParams, ) -> Result { - let _reply = state.ask(DeleteVM { vmid }).await?; + let _reply = state + .ask(DeleteVM { + vmid, + force: query.force, + issued_at: None, + }) + .await?; Ok(Json(())) } diff --git a/odorobo/src/lib.rs b/odorobo/src/lib.rs index 90454de..2e313e9 100644 --- a/odorobo/src/lib.rs +++ b/odorobo/src/lib.rs @@ -1,4 +1,5 @@ pub mod manifest; pub mod messages; pub mod types; +pub mod vmm_process; pub mod vsock_cid; diff --git a/odorobo/src/main.rs b/odorobo/src/main.rs index ecde768..6ca9329 100644 --- a/odorobo/src/main.rs +++ b/odorobo/src/main.rs @@ -10,10 +10,12 @@ pub mod messages; pub mod networking; pub mod types; mod utils; +mod vmm_process; mod vsock_cid; use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; +use std::time::Duration; use kameo::actor::Spawn; use stable_eyre::{Result, eyre::eyre}; @@ -49,27 +51,34 @@ fn init_sentry(dsn: Option<&str>) -> Option { Some(guard) } +/// a little longer than the agent's own VM shutdown timeout, so it gets to finish. +/// the `TimeoutStopSec` in `systemd/odorobo.service` has to cover this plus +/// `RUNTIME_SHUTDOWN_GRACE`, or systemd SIGKILLs us mid-teardown +const SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(75); +/// how long stray tasks get once the actors are done +const RUNTIME_SHUTDOWN_GRACE: Duration = Duration::from_secs(5); + fn mainloop(term: &Arc, config: Config) -> Result<()> { let runtime = tokio::runtime::Builder::new_multi_thread() .enable_all() .build() .map_err(|err| eyre!("can't build tokio: {err}"))?; - let handle = runtime.spawn(inner_main(config)); + let result = runtime.block_on(inner_main(config, Arc::clone(term))); + // don't hang on stray tasks once the actors are done + runtime.shutdown_timeout(RUNTIME_SHUTDOWN_GRACE); + if term.load(Ordering::Relaxed) { + stable_eyre::eyre::bail!("Exit due to termination signal"); + } + result +} - loop { - if handle.is_finished() { - break runtime - .block_on(handle) - .map_err(|err| eyre!("cannot join main thread: {err}"))?; - } - if term.load(Ordering::Relaxed) { - handle.abort(); - stable_eyre::eyre::bail!("Exit due to termination signal"); - } +async fn wait_for_term(term: &AtomicBool) { + while !term.load(Ordering::Relaxed) { + tokio::time::sleep(Duration::from_millis(100)).await; } } -async fn inner_main(config: Config) -> Result<()> { +async fn inner_main(config: Config, term: Arc) -> Result<()> { tracing::info!("Starting odorobo"); let local_peer_id = connect_to_swarm(std::path::Path::new(config.get_peer_key_path()))?; @@ -79,18 +88,86 @@ async fn inner_main(config: Config) -> Result<()> { let agent_actor = AgentActor::spawn(config.clone()); agent_actor.register(AGENT).await?; - if config.get_manager_enabled() { + let manager = if config.get_manager_enabled() { let scheduler_actor = SchedulerActor::spawn(()); let http_actor = HTTPActor::spawn(scheduler_actor.clone()); scheduler_actor.register(SCHEDULER).await?; http_actor.register(HTTP_API_SERVER).await?; + Some((scheduler_actor, http_actor)) + } else { + None + }; + + let run = async { + if let Some((scheduler_actor, http_actor)) = &manager { + scheduler_actor.wait_for_shutdown().await; + http_actor.wait_for_shutdown().await; + } + agent_actor.wait_for_shutdown().await; + }; + tokio::select! { + () = run => return Ok(()), + () = wait_for_term(&term) => {} + } + + // stop gracefully so the agent reaps its VMMs and marks their vsock leases + // stopped. just dropping the runtime kills them without recording that + tracing::info!("termination signal received, shutting down"); + let shutdown = async { + if let Some((scheduler_actor, http_actor)) = &manager { + _ = http_actor.stop_gracefully().await; + _ = scheduler_actor.stop_gracefully().await; + } + if let Err(error) = agent_actor.stop_gracefully().await { + tracing::warn!(?error, "failed to stop agent actor"); + } + agent_actor.wait_for_shutdown().await; + }; + if tokio::time::timeout(SHUTDOWN_TIMEOUT, shutdown) + .await + .is_err() + { + tracing::warn!("timed out waiting for the agent to stop"); + } + Ok(()) +} - scheduler_actor.wait_for_shutdown().await; - http_actor.wait_for_shutdown().await; +#[cfg(test)] +mod tests { + use super::*; + + fn unit_stop_timeout() -> Duration { + let unit = include_str!("../../systemd/odorobo.service"); + let value = unit + .lines() + .find_map(|line| line.trim().strip_prefix("TimeoutStopSec=")) + .expect("the unit sets TimeoutStopSec"); + let secs = value + .strip_suffix('s') + .unwrap_or(value) + .parse() + .expect("TimeoutStopSec is a plain number of seconds"); + Duration::from_secs(secs) } - agent_actor.wait_for_shutdown().await; + #[test] + fn systemd_stop_timeout_covers_graceful_shutdown() { + // if systemd SIGKILLs us first, VMs are torn down without recording + // their leases stopped + assert!( + unit_stop_timeout() > SHUTDOWN_TIMEOUT + RUNTIME_SHUTDOWN_GRACE, + "TimeoutStopSec in systemd/odorobo.service must exceed {:?}", + SHUTDOWN_TIMEOUT + RUNTIME_SHUTDOWN_GRACE + ); + } - Ok(()) + #[test] + fn systemd_only_signals_the_agent_on_stop() { + let unit = include_str!("../../systemd/odorobo.service"); + assert!( + unit.lines().any(|line| line.trim() == "KillMode=mixed"), + "the VMMs share the agent's cgroup, so the unit needs KillMode=mixed" + ); + } } diff --git a/odorobo/src/manifest.rs b/odorobo/src/manifest.rs index 072f882..14adcc7 100644 --- a/odorobo/src/manifest.rs +++ b/odorobo/src/manifest.rs @@ -112,7 +112,7 @@ impl VmManifest { } } -/// compares create intent while ignoring node-local observed values. Leaving out +/// compares create intent while ignoring node-local observed values. leaving out /// the desired CID means "allocate one for me", which isn't the same as an explicit CID #[must_use] pub fn same_create_intent(existing: &VmManifest, requested: &VmManifest) -> bool { @@ -380,7 +380,7 @@ pub struct CloudInit { #[derive(Clone, Debug, Serialize, Deserialize, JsonSchema, PartialEq, Eq)] #[serde(deny_unknown_fields)] pub struct Vsock { - /// optional guest CID. If it's left out, the agent picks one and saves it + /// optional guest CID. if it's left out, the agent picks one and saves it #[serde(default, skip_serializing_if = "Option::is_none")] pub guest_cid: Option, pub socket: String, @@ -402,7 +402,7 @@ pub struct ObservedState { /// Actionable failure detail when the observed status is `error`. #[serde(default, skip_serializing_if = "Option::is_none")] pub error: Option, - /// the node-local guest CID vsock is actually using. It's reported for runtime + /// the node-local guest CID vsock is actually using. it's reported for runtime /// inspection and migration, but it isn't a hard placement requirement #[serde(default, skip_serializing_if = "Option::is_none")] pub vsock_guest_cid: Option, diff --git a/odorobo/src/messages/agent.rs b/odorobo/src/messages/agent.rs index d735221..294256b 100644 --- a/odorobo/src/messages/agent.rs +++ b/odorobo/src/messages/agent.rs @@ -25,6 +25,11 @@ pub struct AgentStatus { pub used_vcpus: u32, pub used_ram: ByteSize, pub vms: Vec, + /// VM IDs that aren't running here but still hold node-local state, like a + /// stopped vsock CID lease or a blocked ID from a failed teardown. the + /// scheduler uses this to rebuild delete owners it lost across a restart + #[serde(default)] + pub retained_vms: Vec, pub metadata: ObjectMetadata, } @@ -38,6 +43,9 @@ pub enum AgentStatusUpdate { revision: u64, added: Vec, removed: Vec, + /// always the full sorted set, since it's small and changes outside VM membership + #[serde(default)] + retained_vms: Vec, used_vcpus: u32, used_ram: ByteSize, }, @@ -67,6 +75,7 @@ pub fn apply_status_update(status: &mut AgentStatus, update: AgentStatusUpdate) revision, added, removed, + retained_vms, used_vcpus, used_ram, } => { @@ -81,6 +90,7 @@ pub fn apply_status_update(status: &mut AgentStatus, update: AgentStatusUpdate) Err(index) => status.vms.insert(index, vmid), } } + status.retained_vms = retained_vms; status.used_vcpus = used_vcpus; status.used_ram = used_ram; revision diff --git a/odorobo/src/messages/vm.rs b/odorobo/src/messages/vm.rs index 6feac58..b251022 100644 --- a/odorobo/src/messages/vm.rs +++ b/odorobo/src/messages/vm.rs @@ -18,6 +18,11 @@ pub struct CreateVM { /// Provider-neutral VM intent. Cloud Hypervisor conversion happens in the /// Cloud Hypervisor driver on the destination agent. pub config: VmManifest, + /// scheduler only. a time-ordered stamp for this incarnation of the VM, so + /// the agent can tell a delete meant for an older one apart from one meant + /// for this one. see [`DeleteVM::issued_at`] + #[serde(default, skip_serializing_if = "Option::is_none")] + pub generation: Option, } #[derive(Serialize, Deserialize, Reply, Debug)] @@ -69,6 +74,29 @@ pub struct MigrateVMReceiveReply { #[derive(Serialize, Deserialize, Debug, Clone)] pub struct DeleteVM { pub vmid: Ulid, + /// scheduler only, agents ignore this. drops cleanup owners whose agent + /// can't be reached at all (not ones that just timed out) instead of + /// failing the delete, so a node that's gone for + /// good doesn't make its VMs undeletable. anything that node still holds is + /// leaked until it comes back and gets cleaned up in the background + #[serde(default)] + pub force: bool, + /// scheduler only. stamped from the same clock as [`CreateVM::generation`] + /// when the scheduler sends the delete. a delete that sat in transit (or in + /// the agent's mailbox) while the ID got recreated is older than the new + /// VM's generation, and the agent acks it without touching the new VM + #[serde(default, skip_serializing_if = "Option::is_none")] + pub issued_at: Option, +} + +impl DeleteVM { + /// whether this delete was sent before a VM with this generation was + /// created, so it can only have been meant for an older incarnation. if + /// either side isn't stamped we can't tell, and the delete goes ahead + #[must_use] + pub fn predates(&self, generation: Option) -> bool { + matches!((self.issued_at, generation), (Some(issued_at), Some(generation)) if issued_at < generation) + } } #[derive(Serialize, Deserialize, Reply, Debug, Clone)] @@ -140,3 +168,26 @@ pub struct SendConsoleInputReply { pub written: usize, pub error: Option, } + +#[cfg(test)] +mod tests { + use super::DeleteVM; + use ulid::Ulid; + + #[test] + fn only_a_stamped_delete_older_than_a_stamped_vm_predates_it() { + let older = Ulid::from_parts(1, 0); + let newer = Ulid::from_parts(2, 0); + let delete = |issued_at| DeleteVM { + vmid: Ulid::nil(), + force: false, + issued_at, + }; + assert!(delete(Some(older)).predates(Some(newer))); + assert!(!delete(Some(newer)).predates(Some(older))); + assert!(!delete(Some(older)).predates(Some(older))); + // anything unstamped (old schedulers, migrated VMs) gets deleted as before + assert!(!delete(None).predates(Some(newer))); + assert!(!delete(Some(older)).predates(None)); + } +} diff --git a/odorobo/src/utils/lockfile.rs b/odorobo/src/utils/lockfile.rs index 92cb7dc..c54102e 100644 --- a/odorobo/src/utils/lockfile.rs +++ b/odorobo/src/utils/lockfile.rs @@ -1,19 +1,26 @@ -use std::io::Write; +use std::fs::{File, OpenOptions}; +use std::io::{Seek, SeekFrom, Write}; +use std::path::Path; + +use fs2::FileExt; const LOCKFILE: &str = "/var/lock/odorobo.lock"; -pub struct Lockfile(Option); +/// holds an exclusive flock on the lockfile for as long as it's alive. +/// +/// the kernel drops the lock when the process dies, however it dies (SIGKILL +/// from systemd's stop timeout, a second signal's `exit(1)`, a panic with +/// abort), so a leftover file never blocks the next start. the file itself is +/// never removed. unlinking it while another process has it open would let two +/// instances each lock a different inode +pub struct Lockfile(Option); impl Drop for Lockfile { fn drop(&mut self) { - let Some(lockfile) = self.0.take() else { - tracing::debug!("no lockfile noted, skipping drop"); - return; - }; - tracing::debug!("removing lockfile"); - drop(lockfile); - _ = std::fs::remove_file(LOCKFILE) - .inspect_err(|e| tracing::warn!(?LOCKFILE, ?e, "cannot remove lockfile")); + if self.0.take().is_some() { + // closing the fd releases the flock + tracing::debug!("released lockfile"); + } } } @@ -25,20 +32,49 @@ pub fn init_lockfile(cfg: &crate::config::Config) -> Result { tracing::info!("Skipping lockfile due to `no_lockfile`"); return Ok(Lockfile(None)); } - tracing::trace!("creating lockfile at {LOCKFILE}"); - let mut f = std::fs::File::create_new(LOCKFILE) - .map_err(|e| format!("Cannot create lockfile at {LOCKFILE}: {e:?}"))?; - f.write_all(&std::process::id().to_ne_bytes()) - .map_err(|e| format!("cannot write to {LOCKFILE}: {e:?}"))?; - f.flush() - .map_err(|e| format!("cannot flush {LOCKFILE}: {e:?}"))?; + acquire(Path::new(LOCKFILE)) +} + +fn acquire(path: &Path) -> Result { + tracing::trace!(?path, "taking lockfile"); + // std opens with O_CLOEXEC, so spawned VMMs don't inherit the fd and keep + // the lock alive after we exit + let mut f = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .open(path) + .map_err(|e| format!("Cannot open lockfile at {}: {e:?}", path.display()))?; + if let Err(e) = f.try_lock_exclusive() { + let holder = std::fs::read_to_string(path).unwrap_or_default(); + let holder = holder.trim(); + return Err(if holder.is_empty() { + format!( + "{} is locked by another odorobo instance: {e}", + path.display() + ) + } else { + format!( + "{} is locked by another odorobo instance (pid {holder}): {e}", + path.display() + ) + }); + } + // only touch the contents once we hold the lock + f.set_len(0) + .and_then(|()| f.seek(SeekFrom::Start(0)).map(drop)) + .and_then(|()| writeln!(f, "{}", std::process::id())) + .and_then(|()| f.flush()) + .map_err(|e| format!("cannot write to {}: {e:?}", path.display()))?; Ok(Lockfile(Some(f))) } /// Register termination signals to watch /// -/// We need to tidy up lockfiles before exiting. Watching these signals can give us a chance to -/// actually clean things up. +/// the first signal sets the flag so main can stop VMs gracefully. a second one +/// exits right away, which is fine for the lockfile since the kernel releases +/// its flock on exit pub fn register_termsigs() -> std::io::Result> { let term = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); for sig in signal_hook::consts::TERM_SIGNALS { @@ -47,3 +83,54 @@ pub fn register_termsigs() -> std::io::Result std::path::PathBuf { + std::env::temp_dir().join(format!( + "odorobo-lockfile-test-{}.lock", + ulid::Ulid::generate() + )) + } + + #[test] + fn second_instance_is_refused_while_the_lock_is_held() { + let path = temp_lock_path(); + + let held = acquire(&path).unwrap(); + let error = acquire(&path).err().unwrap(); + assert!( + error.contains("locked by another odorobo instance"), + "{error}" + ); + assert!( + error.contains(&format!("pid {}", std::process::id())), + "{error}" + ); + + drop(held); + assert!( + path.exists(), + "the lockfile is kept, only the lock is released" + ); + drop(acquire(&path).unwrap()); + _ = std::fs::remove_file(&path); + } + + #[test] + fn leftover_file_from_a_killed_instance_does_not_block_startup() { + let path = temp_lock_path(); + // what a SIGKILLed instance leaves behind: the file, but no lock + std::fs::write(&path, "4242\n").unwrap(); + + let lock = acquire(&path).unwrap(); + assert_eq!( + std::fs::read_to_string(&path).unwrap(), + format!("{}\n", std::process::id()) + ); + drop(lock); + _ = std::fs::remove_file(&path); + } +} diff --git a/odorobo/src/utils/mod.rs b/odorobo/src/utils/mod.rs index 51d70f3..7277075 100644 --- a/odorobo/src/utils/mod.rs +++ b/odorobo/src/utils/mod.rs @@ -163,11 +163,16 @@ pub struct ProductionBehaviour { // https://github.com/tqwewe/kameo/blob/main/examples/custom_swarm.rs // https://docs.page/tqwewe/kameo/distributed-actors/custom-swarm-configuration /// load this node's libp2p identity, or make and save one on first start. -/// The scheduler tracks which node has to clean up a VM by peer ID, so a new +/// the scheduler tracks which node has to clean up a VM by peer ID, so a new /// identity on every restart would leave it waiting on a peer that's gone fn load_or_create_identity(path: &Path) -> Result { match fs::read(path) { Ok(bytes) => { + if let Ok(metadata) = fs::metadata(path) + && std::os::unix::fs::PermissionsExt::mode(&metadata.permissions()) & 0o077 != 0 + { + warn!(path = %path.display(), "libp2p key is readable by other users, it should be 0600"); + } return Keypair::from_protobuf_encoding(&bytes) .wrap_err_with(|| format!("invalid libp2p key at {}", path.display())); } @@ -184,20 +189,27 @@ fn load_or_create_identity(path: &Path) -> Result { .parent() .ok_or_else(|| eyre!("libp2p key path {} has no parent", path.display()))?; fs::create_dir_all(parent).wrap_err("failed to create libp2p key directory")?; - // write to a temp file and rename it in, so a crash can't leave a half-written key - let temp_path = path.with_extension(format!("tmp-{}", std::process::id())); + // write to a temp file and rename it in, so a crash can't leave a half-written key. + // create_new won't follow a planted symlink or reuse a leftover file's permissions + let temp_path = path.with_extension(format!("tmp-{}", ulid::Ulid::generate())); let mut file = OpenOptions::new() - .create(true) - .truncate(true) + .create_new(true) .write(true) .mode(0o600) .open(&temp_path) .wrap_err("failed to create libp2p key")?; - file.write_all(&encoded) + let written = file + .write_all(&encoded) .and_then(|()| file.sync_all()) - .and_then(|()| fs::rename(&temp_path, path)) - .and_then(|()| File::open(parent)?.sync_all()) - .wrap_err_with(|| format!("failed to save libp2p key to {}", path.display()))?; + .and_then(|()| fs::rename(&temp_path, path)); + if let Err(error) = written { + drop(fs::remove_file(&temp_path)); + return Err(error) + .wrap_err_with(|| format!("failed to save libp2p key to {}", path.display())); + } + File::open(parent) + .and_then(|directory| directory.sync_all()) + .wrap_err_with(|| format!("failed to sync libp2p key directory {}", parent.display()))?; info!(path = %path.display(), "generated new libp2p identity"); Ok(keypair) } diff --git a/odorobo/src/vmm_process.rs b/odorobo/src/vmm_process.rs new file mode 100644 index 0000000..43612c6 --- /dev/null +++ b/odorobo/src/vmm_process.rs @@ -0,0 +1,377 @@ +//! identifies a VMM process across agent restarts, so a leftover lease can be +//! checked or cleaned up without trusting a bare PID that might've been reused + +use std::{ + fmt, io, + os::fd::{AsRawFd, FromRawFd, OwnedFd}, + sync::OnceLock, + time::Duration, +}; + +use serde::{Deserialize, Deserializer, Serialize, Serializer}; +use stable_eyre::{Result, eyre::eyre}; + +/// the kernel's random ID for the current boot. start times count from boot, +/// so a PID plus start time only means something within the boot it came from +#[derive(Clone, Copy, PartialEq, Eq)] +pub struct BootId(u128); + +impl BootId { + pub fn current() -> io::Result { + static CURRENT: OnceLock = OnceLock::new(); + if let Some(boot_id) = CURRENT.get() { + return Ok(*boot_id); + } + let contents = std::fs::read_to_string("/proc/sys/kernel/random/boot_id")?; + let boot_id = contents + .trim() + .parse() + .map_err(|()| io::Error::new(io::ErrorKind::InvalidData, "malformed kernel boot_id"))?; + Ok(*CURRENT.get_or_init(|| boot_id)) + } +} + +impl std::str::FromStr for BootId { + type Err = (); + + /// the kernel's UUID form, like `a15ea56c-ab53-44ab-b5b0-9f498b0e38a2` + fn from_str(value: &str) -> Result { + let hex: String = value.chars().filter(|c| *c != '-').collect(); + if hex.len() != 32 || value.len() != 36 { + return Err(()); + } + u128::from_str_radix(&hex, 16).map(Self).map_err(drop) + } +} + +impl fmt::Display for BootId { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let hex = format!("{:032x}", self.0); + write!( + f, + "{}-{}-{}-{}-{}", + &hex[0..8], + &hex[8..12], + &hex[12..16], + &hex[16..20], + &hex[20..32] + ) + } +} + +impl fmt::Debug for BootId { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(self, f) + } +} + +impl Serialize for BootId { + fn serialize(&self, serializer: S) -> Result { + serializer.collect_str(self) + } +} + +impl<'de> Deserialize<'de> for BootId { + fn deserialize>(deserializer: D) -> Result { + let value = String::deserialize(deserializer)?; + value + .parse() + .map_err(|()| serde::de::Error::custom(format!("invalid boot ID {value:?}"))) + } +} + +/// a PID plus its kernel start time and the boot it started in. PIDs get +/// reused and start times restart at every boot, but all three together can't +/// point at a different process +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct ProcessIdentity { + pub pid: u32, + pub start_time: u64, + pub boot_id: BootId, +} + +struct ProcessStat { + state: char, + start_time: u64, +} + +impl ProcessStat { + /// zombies have already let go of their fds and vsock CID + const fn is_dead(&self) -> bool { + matches!(self.state, 'Z' | 'X') + } +} + +fn read_stat(pid: u32) -> io::Result> { + read_stat_at(&format!("/proc/{pid}/stat")) +} + +/// whether any thread in the process is still alive. the leader can exit on +/// its own and show up as a zombie while other threads keep going, like CH's +/// disk threads stuck in D state on a hung device +fn has_live_thread(pid: u32) -> io::Result { + let tasks = match std::fs::read_dir(format!("/proc/{pid}/task")) { + Ok(tasks) => tasks, + Err(error) if is_gone(&error) => return Ok(false), + Err(error) => return Err(error), + }; + for task in tasks { + let task = match task { + Ok(task) => task, + Err(error) if is_gone(&error) => continue, + Err(error) => return Err(error), + }; + let path = task.path().join("stat"); + if read_stat_at(&path.to_string_lossy())?.is_some_and(|stat| !stat.is_dead()) { + return Ok(true); + } + } + Ok(false) +} + +fn is_gone(error: &io::Error) -> bool { + error.kind() == io::ErrorKind::NotFound || error.raw_os_error() == Some(libc::ESRCH) +} + +fn read_stat_at(path: &str) -> io::Result> { + let contents = match std::fs::read_to_string(path) { + Ok(contents) => contents, + Err(error) if is_gone(&error) => return Ok(None), + Err(error) => return Err(error), + }; + // comm can have spaces and parens in it, so skip to the last ')' + let invalid = || io::Error::new(io::ErrorKind::InvalidData, "malformed /proc stat"); + let (_, fields) = contents.rsplit_once(')').ok_or_else(invalid)?; + let mut fields = fields.split_whitespace(); + let state = fields + .next() + .and_then(|state| state.chars().next()) + .ok_or_else(invalid)?; + // starttime is field 22, and state was field 3 + let start_time = fields + .nth(18) + .and_then(|start_time| start_time.parse().ok()) + .ok_or_else(invalid)?; + Ok(Some(ProcessStat { state, start_time })) +} + +impl ProcessIdentity { + pub fn of(pid: u32) -> io::Result { + let stat = read_stat(pid)? + .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "process already exited"))?; + Ok(Self { + pid, + start_time: stat.start_time, + boot_id: BootId::current()?, + }) + } + + /// a process from an earlier boot is gone, whatever has its PID now. a + /// zombie only counts as exited once every one of its threads is dead + pub fn is_running(self) -> io::Result { + if self.boot_id != BootId::current()? { + return Ok(false); + } + let Some(stat) = read_stat(self.pid)? else { + return Ok(false); + }; + if stat.start_time != self.start_time { + return Ok(false); + } + if !stat.is_dead() { + return Ok(true); + } + // only trust the thread list if the PID still belongs to the same + // process after we read it + let live_thread = has_live_thread(self.pid)?; + Ok(live_thread + && read_stat(self.pid)?.is_some_and(|stat| stat.start_time == self.start_time)) + } + + /// SIGKILL this exact process (if it's still around) and wait for it to die + pub async fn terminate(self, timeout: Duration) -> Result<()> { + let Some(pidfd) = pidfd_open(self.pid) + .map_err(|error| eyre!("failed to open pidfd for VMM {}: {error}", self.pid))? + else { + return Ok(()); + }; + // the pidfd sticks to whichever process had the PID when we opened it. + // so if the identity still checks out now, the signal can't hit a different one + if !self.is_running()? { + return Ok(()); + } + pidfd_kill(&pidfd).map_err(|error| eyre!("failed to kill VMM {}: {error}", self.pid))?; + + let deadline = tokio::time::Instant::now() + .checked_add(timeout) + .ok_or_else(|| eyre!("VMM kill timeout overflowed"))?; + while self.is_running()? { + if tokio::time::Instant::now() >= deadline { + return Err(eyre!("VMM {} didn't exit after SIGKILL", self.pid)); + } + tokio::time::sleep(Duration::from_millis(50)).await; + } + drop(pidfd); + Ok(()) + } +} + +/// returns `None` if the process is already gone +fn pidfd_open(pid: u32) -> io::Result> { + let pid = libc::pid_t::try_from(pid) + .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "PID is out of range"))?; + // SAFETY: pidfd_open takes a pid and flags and returns a new fd or -1 + let raw = unsafe { libc::syscall(libc::SYS_pidfd_open, pid, 0) }; + if raw < 0 { + let error = io::Error::last_os_error(); + if error.raw_os_error() == Some(libc::ESRCH) { + return Ok(None); + } + return Err(error); + } + let raw = i32::try_from(raw).map_err(|_| { + io::Error::new( + io::ErrorKind::InvalidData, + "pidfd_open returned an invalid fd", + ) + })?; + // SAFETY: pidfd_open just handed us this fd and nothing else owns it + Ok(Some(unsafe { OwnedFd::from_raw_fd(raw) })) +} + +/// a process that already exited isn't an error +fn pidfd_kill(pidfd: &OwnedFd) -> io::Result<()> { + // SAFETY: the fd is a live pidfd, and a null siginfo is allowed + let sent = unsafe { + libc::syscall( + libc::SYS_pidfd_send_signal, + pidfd.as_raw_fd(), + libc::SIGKILL, + std::ptr::null::(), + 0, + ) + }; + if sent < 0 { + let error = io::Error::last_os_error(); + if error.raw_os_error() != Some(libc::ESRCH) { + return Err(error); + } + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::{BootId, ProcessIdentity}; + use std::time::Duration; + + #[test] + fn boot_id_round_trips_through_the_kernel_format() { + let raw = "a15ea56c-ab53-44ab-b5b0-9f498b0e38a2"; + let boot_id: BootId = raw.parse().unwrap(); + assert_eq!(boot_id.to_string(), raw); + assert_eq!( + serde_json::to_string(&boot_id).unwrap(), + format!("\"{raw}\"") + ); + "a15ea56cab5344abb5b09f498b0e38a2" + .parse::() + .unwrap_err(); + "not-a-boot-id".parse::().unwrap_err(); + BootId::current().expect("read this boot's ID"); + } + + #[tokio::test] + async fn identity_from_another_boot_is_never_running_or_signalled() { + let mut child = tokio::process::Command::new("sleep") + .arg("30") + .kill_on_drop(true) + .spawn() + .expect("spawn sleep"); + let identity = ProcessIdentity::of(child.id().expect("child pid")).expect("read identity"); + // same PID and start time, recorded before a reboot + let other_boot = ProcessIdentity { + boot_id: BootId(identity.boot_id.0 ^ 1), + ..identity + }; + assert!(!other_boot.is_running().expect("check old-boot identity")); + other_boot + .terminate(Duration::from_secs(1)) + .await + .expect("old-boot terminate is a no-op"); + assert!(identity.is_running().expect("still running")); + child.kill().await.expect("kill sleep"); + } + + #[tokio::test] + async fn zombie_leader_with_a_live_thread_still_counts_as_running() { + // the main thread exits on its own (SYS_exit, not exit_group) while a + // second thread sleeps. python is the easy way to get that shape + #[cfg(target_arch = "x86_64")] + const SYS_EXIT: u32 = 60; + #[cfg(target_arch = "aarch64")] + const SYS_EXIT: u32 = 93; + let script = format!( + "import threading, time, ctypes\n\ + threading.Thread(target=time.sleep, args=(30,)).start()\n\ + ctypes.CDLL(None).syscall({SYS_EXIT}, 0)\n" + ); + let Ok(mut child) = tokio::process::Command::new("python3") + .args(["-c", &script]) + .kill_on_drop(true) + .spawn() + else { + eprintln!("python3 isn't installed, skipping"); + return; + }; + let identity = ProcessIdentity::of(child.id().expect("child pid")).expect("read identity"); + let pid = identity.pid; + let deadline = std::time::Instant::now() + Duration::from_secs(10); + while !super::read_stat(pid) + .expect("read stat") + .is_some_and(|stat| stat.is_dead()) + { + assert!(std::time::Instant::now() < deadline, "leader never exited"); + tokio::time::sleep(Duration::from_millis(20)).await; + } + + assert!(identity.is_running().expect("check zombie leader")); + identity + .terminate(Duration::from_secs(5)) + .await + .expect("terminate kills the remaining thread"); + assert!(!identity.is_running().expect("whole group is dead")); + drop(child.wait().await); + } + + #[tokio::test] + async fn identity_tracks_and_kills_the_exact_process() { + let mut child = tokio::process::Command::new("sleep") + .arg("30") + .kill_on_drop(true) + .spawn() + .expect("spawn sleep"); + let identity = ProcessIdentity::of(child.id().expect("child pid")).expect("read identity"); + assert!(identity.is_running().expect("check identity")); + + let stale = ProcessIdentity { + start_time: identity.start_time.wrapping_add(1), + ..identity + }; + assert!(!stale.is_running().expect("check stale identity")); + // a stale identity never gets signalled, even though the PID is alive + stale + .terminate(Duration::from_secs(1)) + .await + .expect("stale terminate is a no-op"); + assert!(identity.is_running().expect("check identity again")); + + identity + .terminate(Duration::from_secs(5)) + .await + .expect("terminate sleep"); + assert!(!identity.is_running().expect("zombie counts as exited")); + drop(child.wait().await); + } +} diff --git a/odorobo/src/vsock_cid.rs b/odorobo/src/vsock_cid.rs index 1fca075..26b7d6a 100644 --- a/odorobo/src/vsock_cid.rs +++ b/odorobo/src/vsock_cid.rs @@ -17,9 +17,16 @@ use stable_eyre::{ }; use ulid::Ulid; +use crate::vmm_process::{BootId, ProcessIdentity}; + const FIRST_GUEST_CID: u32 = 3; const MAX_GUEST_CID: u32 = u32::MAX - 1; -const REGISTRY_VERSION: u32 = 1; +/// bump this for any change an older agent can't read. older agents refuse a +/// newer version instead of rewriting the file without the fields they don't +/// know. 2 added the boot ID to recorded processes +const REGISTRY_VERSION: u32 = 2; +/// oldest version we can still read and upgrade in place +const OLDEST_REGISTRY_VERSION: u32 = 1; const REGISTRY_ENV: &str = "ODOROBO_VSOCK_CID_REGISTRY"; const DEFAULT_REGISTRY: &str = "/var/lib/odorobo/vsock-cids.json"; static NEXT_TEMP_FILE: AtomicU64 = AtomicU64::new(0); @@ -34,9 +41,9 @@ pub struct VsockCidAllocator { /// a registry update went through, but we couldn't confirm it'll survive a crash /// /// the rename worked but syncing the directory failed, so callers can't treat -/// the registry as unchanged. For a failed reservation made before we spawn a +/// the registry as unchanged. for a failed reservation made before we spawn a /// VMM, the caller can safely hang on to its confirmed-exit cleanup state. -/// Normal validation or pre-publish errors don't give that guarantee, since +/// normal validation or pre-publish errors don't give that guarantee, since /// they might be about someone else's existing lease #[derive(Debug, thiserror::Error)] #[error("failed to sync vsock CID registry directory after publishing update: {source}")] @@ -44,8 +51,9 @@ pub struct PublishedRegistryUpdate { #[source] pub source: std::io::Error, /// set for a reservation even if the directory sync failed after the rename. - /// Only this token can show that reservation's process exited - pub reservation_token: Option, + /// only its token can show that reservation's process exited, and its + /// previous CID is what a rollback has to restore + pub reservation: Option, } #[derive(Debug, thiserror::Error)] @@ -79,9 +87,56 @@ struct Registry { #[serde(default)] process_exited: BTreeSet, /// old registries don't have tokens, so those leases only get released once - /// there's a stopped record on disk. Every new reservation gets a fresh token + /// there's a stopped record on disk. every new reservation gets a fresh token #[serde(default)] lease_tokens: BTreeMap, + /// the VMM holding each active lease, so a restarted agent can tell whether + /// it's still running instead of blocking the ID forever + #[serde(default, deserialize_with = "deserialize_processes")] + processes: BTreeMap, +} + +/// just enough to check the version before the strict parse, so a newer file +/// gets a clear error instead of a complaint about fields we don't know +#[derive(Deserialize)] +struct RegistryVersion { + version: u32, +} + +/// a recorded process as any version wrote it. version 1 had no boot ID +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct StoredProcess { + pid: u32, + start_time: u64, + #[serde(default)] + boot_id: Option, +} + +/// drops processes recorded without a boot ID. we can't tell which boot they +/// came from, so they can't safely be checked or killed. their leases go back +/// to needing a manual cleanup, like before processes were recorded at all +fn deserialize_processes<'de, D: serde::Deserializer<'de>>( + deserializer: D, +) -> Result, D::Error> { + let stored = BTreeMap::::deserialize(deserializer)?; + Ok(stored + .into_iter() + .filter_map(|(vmid, process)| { + let Some(boot_id) = process.boot_id else { + tracing::warn!(%vmid, pid = process.pid, "forgetting a VMM recorded without a boot ID"); + return None; + }; + Some(( + vmid, + ProcessIdentity { + pid: process.pid, + start_time: process.start_time, + boot_id, + }, + )) + }) + .collect()) } impl Registry { @@ -99,6 +154,7 @@ impl Registry { self.assignments.remove(vmid); self.process_exited.remove(vmid); self.lease_tokens.remove(vmid); + self.processes.remove(vmid); } } @@ -118,16 +174,17 @@ impl VsockCidAllocator { } /// reserve a CID and keep enough info to undo it if VM startup fails. CIDs - /// 0-2 and `u32::MAX` are reserved by the transport. If the error has a + /// 0-2 and `u32::MAX` are reserved by the transport. if the error has a /// [`PublishedRegistryUpdate`] in it, this reservation did get written, and - /// the error carries its token + /// the error carries it pub fn reserve_with_previous( &self, vmid: Ulid, requested: Option, ) -> Result { let token = VsockLeaseToken(Ulid::generate()); - self.with_registry_token(Some(token), |registry| { + let mut made = None; + let result = self.with_registry(|registry| { let lease_owner = vmid; let vmid = vmid.to_string(); let existing = registry.assignments.get(&vmid).copied(); @@ -166,13 +223,23 @@ impl VsockCidAllocator { registry.assignments.insert(vmid.clone(), cid); } registry.process_exited.remove(&vmid); + registry.processes.remove(&vmid); registry.lease_tokens.insert(vmid, token); - Ok(VsockCidReservation { + let reservation = VsockCidReservation { cid, token, previous_cid: existing, previous_process_exited, - }) + }; + made = Some(reservation); + Ok(reservation) + }); + result.map_err(|error| match error.downcast::() { + Ok(mut published) => { + published.reservation = made; + stable_eyre::Report::new(published) + } + Err(error) => error, }) } @@ -195,7 +262,9 @@ impl VsockCidAllocator { )); } registry.assignments.insert(vmid.clone(), previous_cid); - // keep this reservation's token. Restoring the stopped CID + // rollback only happens after our VMM exited, so its PID is stale + registry.processes.remove(&vmid); + // keep this reservation's token. restoring the stopped CID // shouldn't let an older token act on the lease again if reservation.previous_process_exited { registry.process_exited.insert(vmid); @@ -209,7 +278,7 @@ impl VsockCidAllocator { }) } - /// a new VM without vsock still can't skip past an older active lease. This + /// a new VM without vsock still can't skip past an older active lease. this /// only reads the registry, it never takes ownership or marks anything exited pub fn ensure_no_active_lease(&self, vmid: Ulid) -> Result<()> { let registry = self.read_registry()?; @@ -220,7 +289,70 @@ impl VsockCidAllocator { Ok(()) } - /// mark exit only for the exact reservation this VMM owns. A stale token + /// remember which VMM holds this reservation. a stale token can't attach a + /// process to a newer run of the same VM + pub fn record_process_owned( + &self, + vmid: Ulid, + token: VsockLeaseToken, + process: ProcessIdentity, + ) -> Result<()> { + self.with_registry(|registry| { + let vmid = vmid.to_string(); + if registry.owns_lease(&vmid, token)? { + registry.processes.insert(vmid, process); + } + Ok(()) + }) + } + + /// the VMM recorded for an active lease, if we know it + pub fn recorded_process(&self, vmid: Ulid) -> Result> { + let registry = self.read_registry()?; + let key = vmid.to_string(); + if registry.process_exited.contains(&key) { + return Ok(None); + } + Ok(registry.processes.get(&key).copied()) + } + + /// mark an active lease stopped if its recorded VMM is gone. this is how a + /// restarted agent gets past leases it lost track of. returns whether the + /// lease is now stopped (or there wasn't an active one) + pub fn reconcile_exited(&self, vmid: Ulid) -> Result { + let key = vmid.to_string(); + let registry = self.read_registry()?; + if !registry.assignments.contains_key(&key) || registry.process_exited.contains(&key) { + return Ok(true); + } + let Some(process) = registry.processes.get(&key).copied() else { + return Ok(false); + }; + if process.is_running()? { + return Ok(false); + } + self.with_registry(|registry| { + // only trust the check if nobody swapped the lease while we weren't holding the lock + if registry.assignments.contains_key(&key) + && registry.processes.get(&key) == Some(&process) + { + registry.process_exited.insert(key.clone()); + } + Ok(!registry.assignments.contains_key(&key) || registry.process_exited.contains(&key)) + }) + } + + /// every VM with a lease on this node, running or not + pub fn leased_vms(&self) -> Result> { + Ok(self + .read_registry()? + .assignments + .keys() + .filter_map(|vmid| vmid.parse().ok()) + .collect()) + } + + /// mark exit only for the exact reservation this VMM owns. a stale token /// can't mark a newer run of the same VM as exited pub fn mark_process_exited_owned(&self, vmid: Ulid, token: VsockLeaseToken) -> Result<()> { if !self.registry_path.try_exists()? { @@ -236,7 +368,7 @@ impl VsockCidAllocator { } /// release after a confirmed exit, under the same registry lock as removal. - /// Don't fall back to `release_stopped` if the token doesn't match + /// don't fall back to `release_stopped` if the token doesn't match pub fn release_owned(&self, vmid: Ulid, token: VsockLeaseToken) -> Result<()> { if !self.registry_path.try_exists()? { return Ok(()); @@ -250,7 +382,7 @@ impl VsockCidAllocator { }) } - /// unguarded helpers, only for the old allocator tests. Real callers need a + /// unguarded helpers, only for the old allocator tests. real callers need a /// token or a stopped record on disk #[cfg(test)] pub fn mark_process_exited(&self, vmid: Ulid) -> Result<()> { @@ -279,7 +411,7 @@ impl VsockCidAllocator { } /// release a lease on disk, but only if the VM actor already recorded a - /// confirmed process exit. Returns whether there was a lease, so callers can + /// confirmed process exit. returns whether there was a lease, so callers can /// use `false` to tell an untracked VM apart from one we actually released pub fn release_stopped(&self, vmid: Ulid) -> Result { if !self.registry_path.try_exists()? { @@ -299,14 +431,6 @@ impl VsockCidAllocator { } fn with_registry(&self, update: impl FnOnce(&mut Registry) -> Result) -> Result { - self.with_registry_token(None, update) - } - - fn with_registry_token( - &self, - reservation_token: Option, - update: impl FnOnce(&mut Registry) -> Result, - ) -> Result { let parent = self .registry_path .parent() @@ -327,31 +451,42 @@ impl VsockCidAllocator { let mut registry = self.read_registry()?; let result = update(&mut registry)?; - self.write_registry(®istry, reservation_token)?; - // just drop the file to unlock. An explicit unlock can fail, and then an + self.write_registry(®istry)?; + // just drop the file to unlock. an explicit unlock can fail, and then an // update that already went through would come back as an error drop(lock); Ok(result) } fn read_registry(&self) -> Result { - let registry = match fs::read(&self.registry_path) { - Ok(contents) => serde_json::from_slice::(&contents) - .wrap_err("vsock CID registry is invalid JSON")?, + let mut registry = match fs::read(&self.registry_path) { + Ok(contents) => { + let version = serde_json::from_slice::(&contents) + .wrap_err("vsock CID registry is invalid JSON")? + .version; + if version > REGISTRY_VERSION { + return Err(eyre!( + "vsock CID registry is version {version}, written by a newer odorobo. \ + this one only supports up to version {REGISTRY_VERSION}" + )); + } + if version < OLDEST_REGISTRY_VERSION { + return Err(eyre!("unsupported vsock CID registry version {version}")); + } + serde_json::from_slice::(&contents) + .wrap_err("vsock CID registry is invalid JSON")? + } Err(error) if error.kind() == std::io::ErrorKind::NotFound => Registry { version: REGISTRY_VERSION, assignments: BTreeMap::new(), process_exited: BTreeSet::new(), lease_tokens: BTreeMap::new(), + processes: BTreeMap::new(), }, Err(error) => return Err(error).wrap_err("failed to read vsock CID registry"), }; - if registry.version != REGISTRY_VERSION { - return Err(eyre!( - "unsupported vsock CID registry version {}", - registry.version - )); - } + // older versions upgrade in place. the next write saves the new version + registry.version = REGISTRY_VERSION; let mut seen = HashSet::new(); for (vmid, cid) in ®istry.assignments { if !(FIRST_GUEST_CID..=MAX_GUEST_CID).contains(cid) { @@ -366,11 +501,7 @@ impl VsockCidAllocator { Ok(registry) } - fn write_registry( - &self, - registry: &Registry, - reservation_token: Option, - ) -> Result<()> { + fn write_registry(&self, registry: &Registry) -> Result<()> { let parent = self .registry_path .parent() @@ -397,7 +528,7 @@ impl VsockCidAllocator { self.sync_registry_directory(parent) .map_err(|source| PublishedRegistryUpdate { source, - reservation_token, + reservation: None, })?; Ok(()) })(); @@ -552,11 +683,12 @@ mod tests { assert_eq!(registry.assignments.get(&vmid.to_string()), Some(&42)); assert!(!registry.process_exited.contains(&vmid.to_string())); let token = published - .reservation_token + .reservation + .map(|reservation| reservation.token) .expect("published reservation carries ownership"); assert_eq!(registry.lease_tokens.get(&vmid.to_string()), Some(&token)); - // the failed write let go of its lock. A fresh allocator sees an active + // the failed write let go of its lock. a fresh allocator sees an active // lease and can only clean it up once exit is confirmed (nothing spawned) let reopened = VsockCidAllocator::new(allocator.registry_path); let error = reopened.release_stopped(vmid).unwrap_err(); @@ -579,7 +711,8 @@ mod tests { let token = error .downcast_ref::() .unwrap() - .reservation_token + .reservation + .map(|reservation| reservation.token) .expect("published replacement carries ownership"); let registry = allocator.read_registry().unwrap(); assert_eq!(registry.assignments.get(&vmid.to_string()), Some(&43)); @@ -756,6 +889,72 @@ mod tests { std::fs::remove_dir_all(directory).unwrap(); } + #[test] + fn version_1_registries_upgrade_and_drop_processes_without_a_boot_id() { + let (allocator, directory) = allocator(); + fs::create_dir_all(&directory).unwrap(); + let vmid = Ulid::generate(); + let token = VsockLeaseToken(Ulid::generate()); + let legacy = serde_json::json!({ + "version": 1, + "assignments": { vmid.to_string(): 42 }, + "lease_tokens": { vmid.to_string(): token }, + "processes": { vmid.to_string(): { "pid": 1, "start_time": 1 } }, + }); + fs::write( + &allocator.registry_path, + serde_json::to_vec(&legacy).unwrap(), + ) + .unwrap(); + + // we can't tell which boot that process came from, so it's not used + assert_eq!(allocator.recorded_process(vmid).unwrap(), None); + assert!(!allocator.reconcile_exited(vmid).unwrap()); + + // the next write saves it as the current version with a boot ID + let process = ProcessIdentity::of(std::process::id()).unwrap(); + allocator + .record_process_owned(vmid, token, process) + .unwrap(); + let written: serde_json::Value = + serde_json::from_slice(&fs::read(&allocator.registry_path).unwrap()).unwrap(); + assert_eq!(written["version"], REGISTRY_VERSION); + assert_eq!( + written["processes"][vmid.to_string()]["boot_id"], + BootId::current().unwrap().to_string() + ); + assert_eq!(allocator.recorded_process(vmid).unwrap(), Some(process)); + std::fs::remove_dir_all(directory).unwrap(); + } + + #[test] + fn newer_registry_versions_are_refused_with_a_clear_error() { + let (allocator, directory) = allocator(); + fs::create_dir_all(&directory).unwrap(); + let newer = serde_json::json!({ + "version": REGISTRY_VERSION + 1, + "assignments": {}, + "something_new": true, + }); + fs::write( + &allocator.registry_path, + serde_json::to_vec(&newer).unwrap(), + ) + .unwrap(); + let error = allocator.leased_vms().unwrap_err().to_string(); + assert!(error.contains("written by a newer odorobo"), "{error}"); + // and nothing gets rewritten over it + drop( + allocator + .reserve_with_previous(Ulid::generate(), None) + .unwrap_err(), + ); + let on_disk: serde_json::Value = + serde_json::from_slice(&fs::read(&allocator.registry_path).unwrap()).unwrap(); + assert_eq!(on_disk, newer); + std::fs::remove_dir_all(directory).unwrap(); + } + #[test] fn rejects_wildcard_cid() { let (allocator, directory) = allocator(); @@ -766,4 +965,97 @@ mod tests { ); std::fs::remove_dir_all(directory).unwrap(); } + + #[tokio::test] + async fn restarted_agent_reconciles_a_lease_only_once_its_vmm_is_gone() { + let (allocator, directory) = allocator(); + let vmid = Ulid::generate(); + let reservation = allocator.reserve_with_previous(vmid, Some(42)).unwrap(); + let mut vmm = tokio::process::Command::new("sleep") + .arg("30") + .kill_on_drop(true) + .spawn() + .unwrap(); + let process = ProcessIdentity::of(vmm.id().unwrap()).unwrap(); + allocator + .record_process_owned(vmid, reservation.token, process) + .unwrap(); + assert_eq!(allocator.leased_vms().unwrap(), vec![vmid]); + + // a fresh allocator has no token, like an agent after a restart + let restarted = VsockCidAllocator::new(allocator.registry_path.clone()); + assert_eq!(restarted.recorded_process(vmid).unwrap(), Some(process)); + assert!(!restarted.reconcile_exited(vmid).unwrap()); + assert!(restarted.ensure_no_active_lease(vmid).is_err()); + + vmm.kill().await.unwrap(); + assert!(restarted.reconcile_exited(vmid).unwrap()); + restarted.ensure_no_active_lease(vmid).unwrap(); + assert_eq!(restarted.recorded_process(vmid).unwrap(), None); + assert!(restarted.release_stopped(vmid).unwrap()); + std::fs::remove_dir_all(directory).unwrap(); + } + + #[test] + fn lease_without_a_recorded_vmm_stays_active() { + let (allocator, directory) = allocator(); + let vmid = Ulid::generate(); + reserve(&allocator, vmid, Some(42)).unwrap(); + assert!(!allocator.reconcile_exited(vmid).unwrap()); + assert!(allocator.ensure_no_active_lease(vmid).is_err()); + // nothing to reconcile when there's no lease at all + assert!(allocator.reconcile_exited(Ulid::generate()).unwrap()); + std::fs::remove_dir_all(directory).unwrap(); + } + + #[test] + fn stale_token_cant_attach_a_process_and_new_reservations_forget_the_old_one() { + let (allocator, directory) = allocator(); + let vmid = Ulid::generate(); + let old = allocator.reserve_with_previous(vmid, Some(42)).unwrap(); + let process = ProcessIdentity { + pid: std::process::id(), + start_time: 1, + boot_id: BootId::current().unwrap(), + }; + allocator + .record_process_owned(vmid, old.token, process) + .unwrap(); + allocator + .mark_process_exited_owned(vmid, old.token) + .unwrap(); + + let new = allocator.reserve_with_previous(vmid, None).unwrap(); + assert_eq!(new.cid, 42); + assert_eq!(allocator.recorded_process(vmid).unwrap(), None); + assert!( + allocator + .record_process_owned(vmid, old.token, process) + .is_err() + ); + std::fs::remove_dir_all(directory).unwrap(); + } + + #[test] + fn published_reservation_error_carries_rollback_info() { + let (mut allocator, directory) = allocator(); + let vmid = Ulid::generate(); + reserve(&allocator, vmid, Some(42)).unwrap(); + allocator.mark_process_exited(vmid).unwrap(); + allocator.sync_fault = Some(RegistrySyncFault::Directory); + + let error = allocator.reserve_with_previous(vmid, Some(43)).unwrap_err(); + let reservation = error + .downcast_ref::() + .and_then(|published| published.reservation) + .expect("published reservation"); + assert_eq!(reservation.cid, 43); + + allocator.sync_fault = None; + allocator.rollback(vmid, reservation).unwrap(); + let registry = allocator.read_registry().unwrap(); + assert_eq!(registry.assignments.get(&vmid.to_string()), Some(&42)); + assert!(registry.process_exited.contains(&vmid.to_string())); + std::fs::remove_dir_all(directory).unwrap(); + } } diff --git a/systemd/odorobo.service b/systemd/odorobo.service index c4fcbce..964a379 100644 --- a/systemd/odorobo.service +++ b/systemd/odorobo.service @@ -16,7 +16,9 @@ ExecStart=/usr/bin/odorobo Restart=always RestartSec=3s RestartPreventExitStatus=0 -TimeoutStopSec=30s +KillMode=mixed +# longer than the agent's own shutdown time (SHUTDOWN_TIMEOUT plus the runtime grace in main.rs) +TimeoutStopSec=90s [Install] WantedBy=multi-user.target