Snapshots existing already are ok! (#1759)

Previously, the Downstairs would consider a snapshot existing already as
an error, and unfortunately the Upstairs would continuously retry in the
face of these errors, leading to an indefinite retry loop if any
previous flush (with snapshot) succeeded.

This commit removes the SnapshotExistsAlready error variant, and returns
Ok if the snapshot exists already. This is ok as long as:

- job dependency lists are honoured, meaning no writes can land that
would make each downstairs inconsistent

- snapshot names are random enough that collisions are rare, and
therefore an existing snapshot can be treated as the previous attempt
succeeding

Fixes #1758.

Author: jmpesp

common/src/lib.rs

@@ -115,9 +115,6 @@ pub enum CrucibleError {
     #[error("Snapshot failed! {0}")]
     SnapshotFailed(String),
 
-    #[error("Snapshot {0} exists already")]
-    SnapshotExistsAlready(String),
-
     #[error("Attempting to modify read-only region!")]
     ModifyingReadOnlyRegion,
 
@@ -426,7 +423,6 @@ impl From<CrucibleError> for dropshot::HttpError {
             | CrucibleError::OffsetUnaligned
             | CrucibleError::RegionIncompatible(_)
             | CrucibleError::ReplaceRequestInvalid(_)
-            | CrucibleError::SnapshotExistsAlready(_)
             | CrucibleError::Unsupported(_) => {
                 dropshot::HttpError::for_bad_request(None, e.to_string())
             }

downstairs/src/region.rs

@@ -953,19 +953,34 @@ impl Region {
         if cfg!(feature = "zfs_snapshot") {
             if let Some(snapshot_details) = snapshot_details {
                 info!(self.log, "Flush and snap request received");
-                // Check if the path exists, return an error if it does
+
+                // Check if the path exists, return Ok if it does
                 let test_path = format!(
                     "{}/.zfs/snapshot/{}",
                     self.dir.clone().into_os_string().into_string().unwrap(),
                     snapshot_details.snapshot_name,
                 );
 
                 if std::path::Path::new(&test_path).is_dir() {
-                    crucible_bail!(
-                        SnapshotExistsAlready,
-                        "{}",
+                    // If the snapshot exists already, then a previous call to
+                    // `flush` succeeded, and this function can return Ok. As
+                    // long as job dependency lists are honoured, then no
+                    // concurrent writes can come in, and even if the `flush`
+                    // was called multiple times (say from a saga that is
+                    // retrying a call) all downstairs should be consistent.
+                    //
+                    // This assumes that the snapshot name is random and name
+                    // collisions are either not possible or so rare that this
+                    // check should be considered the same as a previous flush
+                    // succeeding.
+
+                    info!(
+                        self.log,
+                        "snapshot {} exists already, returning Ok",
                         snapshot_details.snapshot_name,
                     );
+
+                    return Ok(());
                 }
 
                 // Look up dataset name for path (this works with any path, and
@@ -999,6 +1014,13 @@ impl Region {
                     })?;
 
                 if !output.status.success() {
+                    error!(
+                        self.log,
+                        "snapshot failed {} {}",
+                        String::from_utf8_lossy(&output.stdout),
+                        String::from_utf8_lossy(&output.stderr),
+                    );
+
                     crucible_bail!(
                         SnapshotFailed,
                         "{}",
@@ -1011,6 +1033,7 @@ impl Region {
         } else if snapshot_details.is_some() {
             error!(self.log, "Snapshot request received on unsupported binary");
         }
+
         Ok(())
     }
 

integration_tests/Cargo.toml

@@ -14,9 +14,7 @@ anyhow.workspace = true
 base64.workspace = true
 bytes.workspace = true
 crucible-client-types.workspace = true
-# importantly, don't use features = ["zfs_snapshot"] here, this will cause
-# cleanup issues!
-crucible-downstairs = { workspace = true, features = ["integration-tests"] }
+crucible-downstairs = { workspace = true, features = ["integration-tests", "zfs_snapshot"] }
 crucible-pantry-client.workspace = true
 crucible-pantry.workspace = true
 crucible = { workspace = true, features = ["integration-tests"] }