feat: add persistent volumes - #214
Conversation
yingdi-shan
changed the title
yingdi-shan
changed the title
yingdi-shan
changed the title
yingdi-shan
changed the title
yingdi-shan
changed the title
yingdi-shan
changed the title
yingdi-shan
changed the title
yingdi-shan
changed the title
yingdi-shan
changed the title
2fdc518 to
45c36e0
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
45c36e0 to
d1530f9
Compare
f93fcdb to
61dac2e
Compare
LSX-s-Software
left a comment
LSX-s-Software
left a comment
There was a problem hiding this comment.
This PR seems completely reverted #194 (See the diffs in src/orchestrator). Please restore those changes.
|
🔍 OpenCodeReview found 42 issue(s) in this PR.
|
| } | ||
|
|
||
| #[test] | ||
| fn create_volume_serializes_issue_fields() { |
There was a problem hiding this comment.
issue_fields appears to be a typo and does not describe what this serialization test covers. Rename it to something like create_volume_serializes_request_fields so the test name remains searchable and clear.
Suggestion:
| fn create_volume_serializes_issue_fields() { | |
| fn create_volume_serializes_request_fields() { |
| #[arg( | ||
| long = "size-mb", | ||
| default_value_t = DEFAULT_VOLUME_SIZE_MB, | ||
| value_parser = parse_volume_size_mb | ||
| )] | ||
| size_mb: u64, |
There was a problem hiding this comment.
--size-mb is always populated with 65536, including for --from-volume. The server requires a child volume's size to exactly match its source, so aenv volume create child --from-volume parent fails for every source whose size is not 65536 MiB unless the user already knows and repeats the exact size. Preserve whether --size-mb was omitted and, for a COW create, resolve the source volume's size (or otherwise make inheritance explicit) instead of applying the empty-volume default.
Suggestion:
| #[arg( | |
| long = "size-mb", | |
| default_value_t = DEFAULT_VOLUME_SIZE_MB, | |
| value_parser = parse_volume_size_mb | |
| )] | |
| size_mb: u64, | |
| #[arg(long = "size-mb", value_parser = parse_volume_size_mb)] | |
| size_mb: Option<u64>, |
| exit 1 | ||
| fi | ||
|
|
||
| RANDOM=$((VOLUME_RANDOM_SEED & 32767)) |
There was a problem hiding this comment.
Masking the accepted seed to 15 bits means seeds that differ by 32768 initialize Bash's PRNG identically, so the logged/advertised seed space of 0..2147483647 collapses to only 32768 initial states. This reduces coverage and makes distinct reported seeds replay the same random choices. Assign the validated seed directly (RANDOM=$VOLUME_RANDOM_SEED); Bash handles seeding while each subsequent read still returns a 15-bit random value.
Suggestion:
| RANDOM=$((VOLUME_RANDOM_SEED & 32767)) | |
| RANDOM=${VOLUME_RANDOM_SEED} |
| assert_not_empty "${LAST_SANDBOX_ID}" "${CURRENT_STEP}: sandbox ID is present" | ||
| track_sandbox "${LAST_SANDBOX_ID}" |
There was a problem hiding this comment.
After a 201 response this helper immediately returns, but sandbox startup is asynchronous elsewhere in the E2E suite (existing tests call wait_for_sandbox_state ... running before proxying to envd). The next content read/write can therefore race startup and intermittently fail with a routing/connection status unrelated to volume behavior. Wait for running here before exercising the mounted filesystem, and report a focused timeout failure.
Suggestion:
| assert_not_empty "${LAST_SANDBOX_ID}" "${CURRENT_STEP}: sandbox ID is present" | |
| track_sandbox "${LAST_SANDBOX_ID}" | |
| assert_not_empty "${LAST_SANDBOX_ID}" "${CURRENT_STEP}: sandbox ID is present" | |
| track_sandbox "${LAST_SANDBOX_ID}" | |
| wait_for_sandbox_state "${LAST_SANDBOX_ID}" "running" 30 || | |
| _fail "${CURRENT_STEP}: volume sandbox reaches running state" "running" "timeout" |
| assert_contains "${HTTP_BODY}" "${expected}-file-${file_index}" \ | ||
| "${CURRENT_STEP}: guest file ${file_index} matches the model" |
There was a problem hiding this comment.
The modeled file check only requires the expected token to occur somewhere in the response. An implementation that appends instead of overwriting, returns stale data plus the new data, or otherwise adds corruption would still pass while the suite updates its model as though the file exactly matched. Compare HTTP_BODY to the complete expected value (accounting for the stripped trailing newline) so the persistence invariant detects extra/stale bytes.
Suggestion:
| assert_contains "${HTTP_BODY}" "${expected}-file-${file_index}" \ | |
| "${CURRENT_STEP}: guest file ${file_index} matches the model" | |
| assert_eq "${HTTP_BODY}" "${expected}-file-${file_index}" \ | |
| "${CURRENT_STEP}: guest file ${file_index} matches the model" |
| #[serde(default)] | ||
| pub volume_snapshots: Vec<SnapshotVolume>, |
There was a problem hiding this comment.
Validate this collection before committing or restoring it so mount_path values are absolute, normalized, and unique after normalization. As modeled, a persisted record can contain entries such as /mnt/data and /mnt//data; restore creates a volume for each entry and then inserts them into a mount-path map, so the later entry replaces the earlier one and leaves the first restored volume orphaned. A snapshot-level constructor/validator (also checking nonzero/bounded size_mb and nonempty layers) would prevent malformed repository data from creating resources before the conflict is detected.
| runtime_versions: build_execution.runtime_versions, | ||
| virtualization_mode: context.virtualization_mode, | ||
| image_configs: build_execution.image_configs, | ||
| volume_snapshots: Vec::new(), |
There was a problem hiding this comment.
Snapshot-based builds can receive a base whose committed().volume_snapshots is non-empty, but the runner resumes that VM snapshot without restoring those volumes and this line then silently removes their metadata. The resulting template can retain guest mount/device state for the reserved volume slots while publishing no volume layers to restore, so build steps may access placeholder-backed mounts and later launches cannot reconstruct the original volumes. Either reject volume-bearing base snapshots before running the build, or restore the base volumes and propagate freshly captured volume_snapshots into the derived snapshot.
| if let Some(owner) = parent.reserved_by_sandbox_id.as_deref() { | ||
| if source_owner != Some(owner) { | ||
| return Err(VolumeError::Reserved(owner.to_owned())); | ||
| } | ||
| } | ||
| if parent.size_mb != size_mb { |
There was a problem hiding this comment.
This is a check-then-act race for public fromVolume clones. An exclusive source can be reserved and mounted immediately after this check but before materialize_backing/create_child_backing reads its local config, so the clone may proceed from a source whose state is concurrently changing despite the API contract requiring it to be unmounted. Acquire a temporary exclusive reservation (or add an atomic repository clone/snapshot operation) for the full read/copy interval, and release it with cancellation-safe cleanup.
| self.repository | ||
| .delete_volume(&record.id) | ||
| .await | ||
| .map_err(repository_error)?; |
There was a problem hiding this comment.
A reservation can be acquired after the preceding get checks. The repository correctly rejects deletion in that race, but both added backends return RepositoryError::InvalidRequest, which repository_error converts to VolumeError::Storage; DELETE then reports a 500 instead of the documented 409 reservation conflict. Preserve a typed reservation-conflict error from the atomic repository delete (or re-read and map the reserved state) so this normal race is surfaced as VolumeError::Reserved.
| for volume_id in volume_ids { | ||
| self.repository | ||
| .replace_volume_owner_for(volume_id, owner, new_owner) | ||
| .await | ||
| .map_err(repository_error)?; | ||
| if let Some(record) = self.records.write().await.get_mut(volume_id) { | ||
| record.replace_owner(owner, new_owner); | ||
| } | ||
| } |
There was a problem hiding this comment.
This multi-volume ownership transition can fail after updating only a prefix. Callers treat an error as though the reservation set still has one owner (and attempt cleanup using that owner), so a transient backend failure can leave volumes split between the pending and sandbox owners and make later cleanup miss some of them. Provide an atomic repository batch operation, or roll back already-updated IDs before returning; release-only cleanup should at least continue through all IDs and aggregate errors.
| next_token = response | ||
| .header("x-next-token") | ||
| .map(str::trim) | ||
| .filter(|token| !token.is_empty()) | ||
| .map(str::to_string); | ||
| volumes.append(&mut response.into_json()?); | ||
| if next_token.is_none() { | ||
| return Ok(volumes); | ||
| } |
There was a problem hiding this comment.
Detect a non-progressing pagination cursor before issuing the next request. If the server returns the same nonempty x-next-token repeatedly (for example, due to a gateway/backend bug), this loop never terminates and keeps making requests indefinitely. Compare the returned token with the token used for the current page (or maintain a set of seen tokens) and return an error when it repeats.
Suggestion:
| next_token = response | |
| .header("x-next-token") | |
| .map(str::trim) | |
| .filter(|token| !token.is_empty()) | |
| .map(str::to_string); | |
| volumes.append(&mut response.into_json()?); | |
| if next_token.is_none() { | |
| return Ok(volumes); | |
| } | |
| let returned_token = response | |
| .header("x-next-token") | |
| .map(str::trim) | |
| .filter(|token| !token.is_empty()) | |
| .map(str::to_string); | |
| if returned_token.is_some() && returned_token == next_token { | |
| anyhow::bail!("volume pagination cursor did not advance"); | |
| } | |
| next_token = returned_token; | |
| volumes.append(&mut response.into_json()?); | |
| if next_token.is_none() { | |
| return Ok(volumes); | |
| } |
| } | ||
|
|
||
| #[test] | ||
| fn create_volume_serializes_issue_fields() { |
There was a problem hiding this comment.
The test name appears to contain a typo: issue_fields does not describe the create request being serialized. Rename it to create_volume_serializes_request_fields (or similar) so test output is readable.
Suggestion:
| fn create_volume_serializes_issue_fields() { | |
| fn create_volume_serializes_request_fields() { |
| if value.contains('\\') | ||
| || value.chars().any(char::is_whitespace) | ||
| || value.contains(',') | ||
| || value.contains(':') | ||
| { |
There was a problem hiding this comment.
This validation still accepts non-whitespace control characters such as \x01 and DEL in mount-path components. Those values are forwarded to the API and eventually used as guest filesystem/process arguments, producing malformed or terminal-control paths. Reject char::is_control() (in addition to whitespace and delimiters), and add a regression test for a control-character path. The backend should enforce the same rule as the authoritative boundary.
Suggestion:
| if value.contains('\\') | |
| || value.chars().any(char::is_whitespace) | |
| || value.contains(',') | |
| || value.contains(':') | |
| { | |
| if value.contains('\\') | |
| || value | |
| .chars() | |
| .any(|character| character.is_whitespace() || character.is_control()) | |
| || value.contains(',') | |
| || value.contains(':') | |
| { |
| #[arg( | ||
| long = "size-mb", | ||
| default_value_t = DEFAULT_VOLUME_SIZE_MB, | ||
| value_parser = parse_volume_size_mb | ||
| )] | ||
| size_mb: u64, |
There was a problem hiding this comment.
For --from-volume, this unconditional default makes a COW create fail whenever the source is not 65536 MiB: the CLI always sends 65536, while the backend requires a child size to exactly match its source. For example, a 2048 MiB parent created by this CLI cannot be copied with aenv volume create child --from-volume parent unless the user redundantly supplies --size-mb 2048. Preserve whether the option was omitted (e.g. use Option<u64>) and, for a source volume, resolve its size before constructing the request; apply the 64 GiB default only to non-COW creates.
| if [[ "$HTTP_STATUS" == "200" ]] \ | ||
| && [[ "$(echo "$HTTP_BODY" | jq -r '.status // empty')" == "$target_status" ]]; then | ||
| return 0 | ||
| fi | ||
| sleep 0.5 |
There was a problem hiding this comment.
failed is a terminal volume status, but this helper keeps polling it until timeout and then omits the final response details. Return failure immediately when .status == "failed" and log the status/body (also include the last HTTP status/body on timeout) so publication failures are fast and diagnosable.
| let mut record = self.load_volume_by_id_unlocked(volume_id)?.ok_or_else(|| { | ||
| RepositoryError::VolumeNotFound { | ||
| lookup: volume_id.to_string(), | ||
| } | ||
| })?; | ||
| if record.mode == VolumeMode::ReadOnly { |
There was a problem hiding this comment.
The reservation decision does not revalidate deleting or status while holding the catalog lock. VolumeManager checks a previously loaded record, but a concurrent publisher can change it to Uploading before this lock is acquired; this method will then reserve it anyway, allowing a sandbox to materialize/mount backing data while it is being replaced. The OSS implementation rejects deleting or non-Ready records inside its atomic reservation operation. Apply the same validation here (and in reserve_read_only_volume) under this lock.
| if record.mode != VolumeMode::ReadOnly { | ||
| return Err(RepositoryError::InvalidRequest { | ||
| reason: format!("volume '{volume_id}' is not read-only"), | ||
| }); | ||
| } | ||
| if record.read_only_mounts.iter().any(|entry| entry == owner) { |
There was a problem hiding this comment.
The read-only reservation path does not reject a volume whose deleting flag is set or whose status is Uploading/Failed. The pre-check in VolumeManager is based on an earlier read, so the state can change before this catalog lock is acquired; this then records a new mount on an unusable volume. Revalidate deleting and status == Ready under this lock, as the OSS implementation does.
Suggestion:
| if record.mode != VolumeMode::ReadOnly { | |
| return Err(RepositoryError::InvalidRequest { | |
| reason: format!("volume '{volume_id}' is not read-only"), | |
| }); | |
| } | |
| if record.read_only_mounts.iter().any(|entry| entry == owner) { | |
| if record.deleting || record.status != crate::volume::VolumeStatus::Ready { | |
| return Err(RepositoryError::InvalidRequest { | |
| reason: format!("volume '{volume_id}' is not usable"), | |
| }); | |
| } | |
| if record.mode != VolumeMode::ReadOnly { | |
| return Err(RepositoryError::InvalidRequest { | |
| reason: format!("volume '{volume_id}' is not read-only"), | |
| }); | |
| } | |
| if record.read_only_mounts.iter().any(|entry| entry == owner) { |
| match Flock::lock(file, FlockArg::LockExclusiveNonblock) { | ||
| Ok(mut file) => { |
There was a problem hiding this comment.
This changes the alias/record lock protocol incompatibly with older workers sharing the same repository. Existing versions treat the lock file's existence as ownership and, after its stale-age threshold, unlink it and create a new inode; this version locks the original inode with flock. During a rolling upgrade, an old worker can therefore remove a lock held here and enter the same critical section using a new file while this guard is still live. That can race alias binding and snapshot-record state transitions. Preserve a protocol understood by both versions or introduce a repository-format/version gate that prevents mixed-version access before switching to advisory locks.
| async fn publish_volume_backing( | ||
| &self, | ||
| _volume_id: &str, | ||
| _image_config_path: &Path, | ||
| ) -> RepositoryResult<Vec<crate::snapshot::OverlaybdLayerRef>> { | ||
| unsupported("volume backing publication") | ||
| } |
There was a problem hiding this comment.
Backing publication is exposed separately from create_volume/put_volume, with no commit or rollback operation. Current callers upload first and persist the returned layers afterward; cancellation or a catalog-write failure can therefore leave unreferenced durable layers, and updates can leave the record in Uploading indefinitely. Provide a repository-level transactional publication/commit operation (or an explicit abort/recovery contract) so publication and catalog visibility cannot diverge.
| /// Acquires an exclusive volume reservation atomically where the backend | ||
| /// supports it. `Ok(Some(owner))` reports an existing conflicting owner. | ||
| async fn reserve_volume( |
There was a problem hiding this comment.
Atomicity needs to be a mandatory contract for writable reservations, not conditional on backend support. A non-atomic implementation can return success to two callers and attach the same writable backing concurrently. Require implementations to perform the claim atomically and return Unsupported when they cannot; ideally add a repository conformance test for this contract.
| async fn replace_volume_owner_for( | ||
| &self, | ||
| _volume_id: &str, | ||
| _from: &str, | ||
| _to: Option<&str>, | ||
| ) -> RepositoryResult<()> { | ||
| unsupported("volume reservations") | ||
| } |
There was a problem hiding this comment.
This per-volume ownership API cannot atomically rebind a sandbox's full volume set. VolumeManager::replace_owner_for invokes it in a loop; if a later call fails, earlier volumes remain rebound while later ones retain from. In the create path, cleanup then releases only the pending owner, so volumes already rebound to the newly deleted sandbox can remain reserved indefinitely. Provide a repository operation that conditionally rebinds the complete volume-ID set atomically, or return enough rollback state and have callers explicitly reverse completed changes on failure.
| async fn replace_volume_owner_for( | ||
| &self, | ||
| _volume_id: &str, | ||
| _from: &str, | ||
| _to: Option<&str>, | ||
| ) -> RepositoryResult<()> { | ||
| unsupported("volume reservations") | ||
| } |
There was a problem hiding this comment.
The conditional operation returns Ok(()) both when ownership was changed and when from did not own the volume. Callers use success to finalize a pending reservation; a stale/missing from can therefore let sandbox creation continue without any durable lease, allowing the volume to be reserved elsewhere. Return an outcome such as bool/the current owner (or a conflict error), and require callers to treat a non-match as failure.
| pub struct SnapshotVolume { | ||
| pub mount_path: String, | ||
| pub size_mb: u64, | ||
| pub layers: Vec<OverlaybdLayerRef>, | ||
| } |
There was a problem hiding this comment.
SnapshotVolume drops the source volume's access mode. The capture path accepts read-only volumes, but restore creates every entry as VolumeMode::Exclusive, so a volume snapshotted as read-only becomes writable after launch from the snapshot. Persist the mode here (with a backward-compatible serde default) and use it when recreating the volume.
LSX-s-Software
left a comment
LSX-s-Software
left a comment
There was a problem hiding this comment.
PR #194 has been restored in the latest commits, but this PR still reverts several changes introduced by PR #209 in src/api/impls/sandbox.rs:
connect:SandboxOperationConflictshould return HTTP 409, but now falls through to 500.resume:InvalidTimeoutshould return HTTP 400, but now falls through to 500.resume:SandboxOperationConflictshould return HTTP 409, but now falls through to 500.- The
list_sandboxes_filtereddocumentation for thestarted_afterandtemplatefilters was removed. - Several
..SandboxListFilter::matches_all()usages were replaced with explicitNoneinitialization in production code and tests. The helper still exists and should remain the canonical default, so future filter fields do not require updating every call site. PaginationCursor::new_ascendingwas incorrectly marked by#[allow(dead_code)].
These changes were introduced by b3d376f and were not fully restored by the latest commit. Please restore them.
| #[allow(dead_code)] | ||
| pub fn new_ascending(time: SystemTime, value: T) -> Self { | ||
| Self::new(time, value, false) | ||
| } |
There was a problem hiding this comment.
This function is no longer dead code after restoring the changes from #209. Please remove the #[allow(dead_code)] attribute.
| next_token = response | ||
| .header("x-next-token") | ||
| .map(str::trim) | ||
| .filter(|token| !token.is_empty()) | ||
| .map(str::to_string); | ||
| volumes.append(&mut response.into_json()?); | ||
| if next_token.is_none() { | ||
| return Ok(volumes); | ||
| } |
There was a problem hiding this comment.
Detect repeated pagination tokens before issuing the next request. If a server/proxy returns the same nonempty x-next-token, this loop repeatedly downloads and appends the same page forever. Track seen tokens (or at least compare the new token with the token used for the current request) and return an error when pagination does not advance.
| } | ||
|
|
||
| #[test] | ||
| fn create_volume_serializes_issue_fields() { |
There was a problem hiding this comment.
The test name appears to contain a typo: “issue fields” does not describe what the test verifies. Rename it to something like create_volume_serializes_request_fields so test output is readable.
Suggestion:
| fn create_volume_serializes_issue_fields() { | |
| fn create_volume_serializes_request_fields() { |
| if value.contains('\\') | ||
| || value.chars().any(char::is_whitespace) | ||
| || value.contains(',') | ||
| || value.contains(':') | ||
| { | ||
| anyhow::bail!("volume mount path contains invalid characters or '..': {value}"); | ||
| } |
There was a problem hiding this comment.
This validation still accepts non-whitespace control characters such as NUL, BEL, and DEL. Those values can reach downstream filesystem/guest-command handling and fail only after sandbox creation has begun. Reject char::is_control() here (and ideally apply the same rule in the shared server-side mount-path validator) so invalid mount paths fail at the CLI boundary.
Suggestion:
| if value.contains('\\') | |
| || value.chars().any(char::is_whitespace) | |
| || value.contains(',') | |
| || value.contains(':') | |
| { | |
| anyhow::bail!("volume mount path contains invalid characters or '..': {value}"); | |
| } | |
| if value.contains('\\') | |
| || value.chars().any(|character| character.is_whitespace() || character.is_control()) | |
| || value.contains(',') | |
| || value.contains(':') | |
| { | |
| anyhow::bail!("volume mount path contains invalid characters or '..': {value}"); | |
| } |
| exit 1 | ||
| fi | ||
|
|
||
| RANDOM=$((VOLUME_RANDOM_SEED & 32767)) |
There was a problem hiding this comment.
This collapses the documented 0..2147483647 seed range to 15 bits, so seeds that differ by 32768 generate the same operation sequence. That substantially limits variation when CI rotates seeds and makes the accepted range misleading. Either constrain the accepted seed to Bash RANDOM's effective domain or use a deterministic PRNG that preserves the full validated seed.
| assert_not_empty "${LAST_VOLUME_ID}" "${CURRENT_STEP}: cloned volume ID is present" | ||
| assert_json_field "${HTTP_BODY}" '.status' "ready" \ | ||
| "${CURRENT_STEP}: cloned volume is ready" | ||
| register_volume "${LAST_VOLUME_ID}" "${mode}" "${VOLUME_CONTENT[${source_id}]}" |
There was a problem hiding this comment.
Assert that the clone ID differs from source_id. If the create endpoint incorrectly returns/reuses the source volume, an exclusive clone is registered over the same associative-array entry and the subsequent lifecycle can still pass, so this test would not enforce clone identity or independence. A deterministic follow-up that mutates the clone and verifies the source remains unchanged would also cover backing aliasing.
Suggestion:
| assert_not_empty "${LAST_VOLUME_ID}" "${CURRENT_STEP}: cloned volume ID is present" | |
| assert_json_field "${HTTP_BODY}" '.status' "ready" \ | |
| "${CURRENT_STEP}: cloned volume is ready" | |
| register_volume "${LAST_VOLUME_ID}" "${mode}" "${VOLUME_CONTENT[${source_id}]}" | |
| assert_not_empty "${LAST_VOLUME_ID}" "${CURRENT_STEP}: cloned volume ID is present" | |
| assert_not_eq "${LAST_VOLUME_ID}" "${source_id}" \ | |
| "${CURRENT_STEP}: clone receives an independent volume ID" | |
| assert_json_field "${HTTP_BODY}" '.status' "ready" \ | |
| "${CURRENT_STEP}: cloned volume is ready" | |
| register_volume "${LAST_VOLUME_ID}" "${mode}" "${VOLUME_CONTENT[${source_id}]}" |
| match parent.backing_image_config.as_ref() { | ||
| Some(path) => Some(self.create_child_backing(&id, path).await?), | ||
| None => None, | ||
| } |
There was a problem hiding this comment.
A source volume with backing_layers.is_empty() and no node-local config reaches None here, so the child is created successfully with neither a local backing nor repository layers. This can occur for a reserved child whose local backing was lost after restart (or a malformed/legacy catalog record), and the resulting volume later fails mounting with "has no backing image." Reject this state as Storage/Failed instead of creating an unusable child.
| } else { | ||
| self.publish_backing(&mut parent).await?; | ||
| self.persist_catalog(&parent).await?; | ||
| self.cache_record(parent.clone()).await; | ||
| } |
There was a problem hiding this comment.
This branch republishes a backing while leaving the record in Ready. If publish_backing changes backing_layers, persist_catalog is rejected by validate_catalog_update, which requires the existing record to be Uploading before replacing its backing. It also leaves the volume visible as ready during the upload. Use the same Uploading -> publish -> Ready/Failed transition as publish_records (ideally by routing this branch through that helper).
| match record.status { | ||
| VolumeStatus::Uploading => return Err(VolumeError::Uploading(record.id.clone())), | ||
| VolumeStatus::Failed => return Err(VolumeError::Failed(record.id.clone())), | ||
| VolumeStatus::Ready => {} | ||
| } | ||
| if record.mode == VolumeMode::ReadOnly { |
There was a problem hiding this comment.
This status check is not atomic with the subsequent repository reservation. A publisher can change Ready to Uploading after get() and before reserve_volume/reserve_read_only_volume; the POSIX repository implementation currently reserves without rechecking status, so a sandbox can mount a backing while it is being replaced. Make reservation atomically validate Ready in every repository backend (and return a typed unusable-status result) rather than relying on this preflight read.
| for volume_id in volume_ids { | ||
| self.repository | ||
| .replace_volume_owner_for(volume_id, owner, new_owner) | ||
| .await | ||
| .map_err(repository_error)?; | ||
| if let Some(record) = self.records.write().await.get_mut(volume_id) { | ||
| record.replace_owner(owner, new_owner); | ||
| } | ||
| } |
There was a problem hiding this comment.
This batch can be partially applied: if updating a later volume fails, earlier volumes have already moved or released ownership, while callers treat the method as one failed sandbox-level operation and retain/roll back metadata for the full mount set. Those earlier volumes can then be reserved elsewhere even though the sandbox still records them as mounted. Provide an atomic repository batch operation, or implement compensating rollback and surface an explicit partial-failure state.
| fn repository_error(error: RepositoryError) -> VolumeError { | ||
| match error { | ||
| RepositoryError::VolumeNotFound { lookup } => VolumeError::NotFound(lookup), | ||
| RepositoryError::VolumeNameConflict { name } => VolumeError::NameConflict(name), | ||
| error => VolumeError::Storage(error.to_string()), | ||
| } | ||
| } |
There was a problem hiding this comment.
Invalid user-supplied volume references are validated by the repository as RepositoryError::InvalidRequest, but this mapping turns them into Storage, so endpoints such as volume delete/mount return HTTP 500 for an ordinary malformed ID/name (for example, a value containing /). Validate reference in get before calling the repository, or map the repository's invalid-component error to a dedicated client-input VolumeError.
| } | ||
|
|
||
| #[test] | ||
| fn create_volume_serializes_issue_fields() { |
There was a problem hiding this comment.
The test name appears to contain a typo: “issue fields” does not describe what is being tested. Rename it to indicate that the create-volume request fields are serialized.
Suggestion:
| fn create_volume_serializes_issue_fields() { | |
| fn create_volume_serializes_request_fields() { |
| exit 1 | ||
| fi | ||
|
|
||
| RANDOM=$((VOLUME_RANDOM_SEED & 32767)) |
There was a problem hiding this comment.
This truncates every accepted seed to 15 bits, so distinct advertised seeds (for example, 1 and 32769) execute the same pseudo-random sequence even though logs report different seeds. That reduces coverage diversity and makes reproducing a failure from the logged seed misleading. Either restrict validation/logging to 0..32767, or use a deterministic PRNG that consumes the full accepted 31-bit seed.
| assert_contains "${HTTP_BODY}" "${expected}-file-${file_index}" \ | ||
| "${CURRENT_STEP}: guest file ${file_index} matches the model" |
There was a problem hiding this comment.
A substring check allows corrupted content with an added prefix/suffix (or duplicated payload) to pass, so this does not enforce the modeled exact state across clone, fork, pause, and remount. _curl_do loads the response through command substitution and therefore strips the uploaded trailing newline, as demonstrated by suite 15; use assert_eq here to detect any byte-content change.
Suggestion:
| assert_contains "${HTTP_BODY}" "${expected}-file-${file_index}" \ | |
| "${CURRENT_STEP}: guest file ${file_index} matches the model" | |
| assert_eq "${HTTP_BODY}" "${expected}-file-${file_index}" \ | |
| "${CURRENT_STEP}: guest file ${file_index} matches the model" |
| CURRENT_STEP="cross-node-remount" | ||
| exercise_volume_cycle "${base_volume_id}" 1 | ||
| assert_catalog_matches_model |
There was a problem hiding this comment.
The multi-node test never exercises concurrent ownership: every cycle creates one sandbox and deletes it before another sandbox mounts the volume. As a result, it cannot detect split-brain reservation bugs where the same exclusive volume is mounted simultaneously on both nodes, nor verify that ro volumes permit concurrent mounts. Add a cross-node case that keeps a node-A sandbox alive while attempting the same exclusive mount on node B (expecting rejection), plus a read-only case mounted on both nodes concurrently.
| children.entry(child_id).or_default().push(child.id.clone()); | ||
| child_mounts.insert(mount_path.clone(), child.id.clone()); | ||
| replace_drive_ids.push((volume.id.clone(), child.id.clone())); | ||
| volume_mounts.insert(mount_path.clone(), child.id); | ||
| } else { | ||
| volume_mounts.insert(mount_path.clone(), volume.id.clone()); | ||
| child_mounts.insert(mount_path.clone(), volume.id.clone()); | ||
| } |
There was a problem hiding this comment.
The cleanup map records only newly created exclusive volumes, but resolve_volume_mounts below also reserves every read-only source volume for owner. If preparation later fails, or this child is one of the failed fork outcomes, cleanup_fork_volume_children releases only this exclusive-ID list, leaving the read-only volume's read_only_mounts entry owned by a sandbox ID that was never created. Track all reserved IDs separately from the exclusive child IDs that should be deleted, and release all reservations for failed children.
| volume_ids.push(child.id.clone()); | ||
| mounts.insert(mount_path.to_string_lossy().into_owned(), child.id); |
There was a problem hiding this comment.
A snapshot can contain duplicate mount paths that normalize to the same key. The second insert silently replaces the first restored volume ID, while both IDs remain in volume_ids; on a successful launch only the replacement is reserved, leaving the first randomly named restored volume as an unintended orphan. Reject duplicate normalized paths before creating/inserting a child (or clean up the displaced child).
| let mut extra_drives: Vec<_> = resolved_attached.into_iter().map(|r| r.drive).collect(); | ||
| extra_drives.extend(volume_drives); |
There was a problem hiding this comment.
Attached drives and volume drives are validated independently, but the combined set is not checked before launch. A client can choose an attached-drive mount path that overlaps a volumeMounts path (or a drive ID equal to the resolved volume ID); the Firecracker config then rejects the combined set as duplicate/overlapping during sandbox construction. That factory error is not an InvalidRequestError, so this validatable client conflict is returned as HTTP 500 after doing image/materialization work. Validate IDs and normalized mount paths across both sets here and return 400 before reserving/launching.
| let _ = self.orchestrator.delete_sandbox(metadata.id).await; | ||
| if let Some(owner) = pending_volume_owner.as_deref() { | ||
| let _ = self | ||
| .volume_manager | ||
| .replace_owner_for(owner, None, &reserved_volume_ids) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
Do not release the pending reservations unless sandbox teardown is confirmed. delete_sandbox can fail while the sandbox remains in the store/runtime (for example, volume publication or owner replacement can fail), but this path ignores that error and then makes any still-pending volumes available for reuse by another sandbox. The warm-create equivalent also deletes restored_volume_ids, potentially removing backing storage still attached to the running sandbox. Handle the delete result explicitly and preserve reservations/backings when teardown fails; cleanup failures should also be surfaced or durably retried.
Keep pause and fork volume handling node-local, publish durable state at snapshot or final release boundaries, and patch reserved Firecracker drives only on first attachment. Harden distributed catalog updates, restore atomic orchestrator behavior, and expand API-only lifecycle and randomized Compose coverage.
| next_token = response | ||
| .header("x-next-token") | ||
| .map(str::trim) | ||
| .filter(|token| !token.is_empty()) | ||
| .map(str::to_string); | ||
| volumes.append(&mut response.into_json()?); | ||
| if next_token.is_none() { | ||
| return Ok(volumes); | ||
| } |
There was a problem hiding this comment.
Track previously seen continuation tokens and return an error if one repeats. As written, a stale token (or a cycle of malformed tokens) makes this synchronous CLI loop issue requests indefinitely while continuing to accumulate response data. A HashSet<String> of seen tokens would bound this failure and provide an actionable protocol error.
| } | ||
|
|
||
| #[test] | ||
| fn create_volume_serializes_issue_fields() { |
There was a problem hiding this comment.
“issue” appears to be a typo for “request”; rename this test to create_volume_serializes_request_fields so its purpose is clear in test output.
| local expected="${VOLUME_CONTENT[${volume_id}]}" | ||
| [[ -n "${expected}" ]] || return 0 |
There was a problem hiding this comment.
Do not skip verification for modeled-empty volumes. This makes a newly created volume—or an ro clone of one—pass even if it contains stale state-*.txt data from another volume. For the empty model, attempt to download the modeled paths and assert 404; for non-empty content, continue with exact body checks.
| assert_contains "${HTTP_BODY}" "${expected}-file-${file_index}" \ | ||
| "${CURRENT_STEP}: guest file ${file_index} matches the model" |
There was a problem hiding this comment.
Use exact equality for this file-integrity oracle. A response such as <expected>-file-N-corrupt currently passes, so appended or otherwise corrupted volume data can go undetected. Since command substitution already strips the newline written by printf, assert_eq "${HTTP_BODY}" "${expected}-file-${file_index}" matches the intended payload exactly.
| while :; do | ||
| local path="/volumes?limit=100" |
There was a problem hiding this comment.
Bound this pagination loop and detect a repeated x-next-token. If the gateway regresses and returns the same non-empty token repeatedly, this suite hangs indefinitely rather than reporting a test failure. The existing pagination suite uses a maximum page count; a seen-token set would additionally produce a precise failure.
| runtime_versions: build_execution.runtime_versions, | ||
| virtualization_mode: context.virtualization_mode, | ||
| image_configs: build_execution.image_configs, | ||
| volume_snapshots: Vec::new(), |
There was a problem hiding this comment.
This unconditionally drops volume snapshot metadata when deriving a template from an existing snapshot. prepare_snapshot_base_context accepts any RunnableSnapshot, including one with committed().volume_snapshots, so the resulting template no longer restores those volumes even when the build did not override them. In addition, the runner currently resumes that base without supplying restored volume drives, so build steps cannot reliably observe or modify the captured volume contents. Please either restore/capture and publish the base volumes as part of snapshot-based builds, or explicitly reject volume-bearing base snapshots until that flow is supported; using an empty vector silently loses the state.
| let create_result = async { | ||
| if reserved_owner.is_none() { | ||
| self.publish_backing(&mut record).await?; | ||
| } | ||
| self.repository | ||
| .create_volume(record.clone()) |
There was a problem hiding this comment.
The unique name is claimed only by create_volume, after the backing has already been published. Two concurrent requests for the same name can therefore both perform the expensive publication; the loser removes only its node-local directory, leaving any repository layers it uploaded unreachable. Claim/create the catalog record before publication (for example in an Uploading state and finalize it afterward), or explicitly remove publication artifacts when catalog creation loses the race.
| if size_mb == 0 || backing_layers.is_empty() { | ||
| return Err(VolumeError::InvalidSize); | ||
| } |
There was a problem hiding this comment.
An empty snapshot backing is reported as “volume size must be greater than zero” even when size_mb is valid. This error is propagated through snapshot restore to the API, so it gives clients the wrong corrective action. Add a distinct missing/invalid-backing error (or at least a storage/snapshot validation error) and validate the two conditions separately.
| for volume_id in volume_ids { | ||
| self.repository | ||
| .replace_volume_owner_for(volume_id, owner, new_owner) | ||
| .await | ||
| .map_err(repository_error)?; | ||
| if let Some(record) = self.records.write().await.get_mut(volume_id) { | ||
| record.replace_owner(owner, new_owner); | ||
| } | ||
| } |
There was a problem hiding this comment.
This multi-volume ownership transition can fail after some records have already moved. Callers treat the operation as one reservation/finalization step, but retries using owner will skip records already moved to new_owner, leaving a split reservation set. Use a repository batch/transaction where available, or track completed updates and roll them back to owner before returning the error.
| fn repository_error(error: RepositoryError) -> VolumeError { | ||
| match error { | ||
| RepositoryError::VolumeNotFound { lookup } => VolumeError::NotFound(lookup), | ||
| RepositoryError::VolumeNameConflict { name } => VolumeError::NameConflict(name), | ||
| error => VolumeError::Storage(error.to_string()), | ||
| } | ||
| } |
There was a problem hiding this comment.
Repository-side state conflicts are collapsed into Storage, so normal races become HTTP 500 responses. For example, after reserve() reads a Ready record, a concurrent publisher can transition it to Uploading; reserve_volume then returns InvalidRequest("not usable"), which is reported as a storage failure instead of Uploading/409. A concurrent reservation during delete() is similarly surfaced as 500 rather than Reserved. Add typed repository errors for these volume states and map them to the corresponding VolumeError variants (rather than parsing diagnostic strings).
2 participants
What
Add E2B-compatible persistent volumes backed by the configured snapshot repository, with volume CRUD, sandbox mounts, lifecycle persistence, and native CLI commands.
Volumes are visible from every AgentENV node that shares the POSIX or OSS repository. New volumes default to 64 GiB, can be created with an explicit size, expose
uploadingwhile publication is incomplete, and support exclusive writable or shared read-only mounts.Why
Closes the persistent-volume gap tracked in issue #211. Previously, attached storage was node-local and could not be listed or reused reliably from another runtime node. Users also lacked an E2B-aligned API and CLI flow for creating, sizing, mounting, and deleting durable volumes.
Related issue
Closes #211
Scope and non-goals
Included:
volumeMounts.readyanduploadingstatus semantics.aenv volumecommands andaenv start --volumemounting.Not included:
Design and behavior changes
The configured snapshot repository owns the authoritative volume catalog and backing layers. Each node reloads this shared catalog, so creating a volume through node A makes it listable and mountable through node B without scheduler state.
Creating an empty volume provisions an OverlayBD-compatible filesystem image. Creating from another volume publishes an independent child backing. A volume is
uploadingwhile its backing is being published and cannot be mounted until it becomesready.Writable volumes use an exclusive repository reservation. Read-only volumes can be mounted by multiple sandboxes and are exposed read-only to the guest, so guest writes fail at the block device/filesystem boundary. Sandbox mount paths are supplied as the E2B-compatible path-to-volume map.
Mounted drives are remounted through envd's
/patch/drivesflow after resume. Pause and deletion seal and publish recent writable data before releasing ownership. Sandbox fork and sandbox snapshot operations create logical volume snapshots with independent backing references. Expired sandboxes with mounted writable volumes are atomically claimed, paused to flush their drives, and then deleted.After the durable catalog entry is deleted, node-local backing cleanup is best-effort. A local cleanup failure is logged and does not turn a successful durable deletion into a misleading HTTP 500.
Compatibility and operations
/volumesCRUD operations, volume models/status fields, andvolumeMountson sandbox creation. Generated Rust server bindings are updated fromsrc/api/openapi.yml.mkfs.ext4runtime package.Validation
make fmtmake clippymake test-unitmake -C services test(required whenservices/changes)maketargetCommands and results:
Skipped checks and reasons:
Checklist