Verify extent under repair is valid after copying files (#1159)

Verify extent under repair is valid after copying files

Added a call to the downstairs repair API that will verify that
an extent is closed (or the region is read only), meaning the extent
is okay to copy.

This prevents a rare but possible condition where an extent is closed
when a remote side contacts it for repair, but is re-opened before the
extent file copy starts and the resulting copy is invalid.

Keep a mutex for the repair task

---------

Co-authored-by: Alan Hanson <alan@oxide.computer>

Author: leftwo

Cargo.lock

@@ -47,6 +47,7 @@ mod stats;
 mod extent_inner_raw;
 mod extent_inner_sqlite;
 
+use extent::ExtentState;
 use region::Region;
 
 pub use admin::run_dropshot;
@@ -3211,7 +3212,7 @@ pub async fn start_downstairs(
     let repair_log = d.lock().await.log.new(o!("task" => "repair".to_string()));
 
     let repair_listener =
-        match repair::repair_main(&dss, repair_address, &repair_log).await {
+        match repair::repair_main(dss, repair_address, &repair_log).await {
             Err(e) => {
                 // TODO tear down other things if repair server can't be
                 // started?

downstairs/src/lib.rs

@@ -507,7 +507,9 @@ impl Region {
      *
      * Let us assume we are repairing extent 012
      *  1. Make new 012.copy dir  (extent name plus: .copy)
-     *  2. Get all extent files from source side, put in 012.copy directory
+     *  2. Get all extent files from source side, put in 012.copy directory.
+     *     Verify after the copy completes that the remote side still has
+     *     the extent closed (or the region is read only).
      *  3. fsync files we just downloaded
      *  4. Rename 012.copy dir to 012.replace dir
      *  5. fsync extent directory ( 00/000/ where the extent files live)
@@ -670,6 +672,29 @@ impl Region {
             count += 1;
         }
 
+        // We have copied over the extent.  Verify that the remote side
+        // still believes that it is valid for repair so we know we have
+        // received a valid copy.
+        info!(self.log, "Verify extent {eid} still ready for copy");
+        let rc = match repair_server.extent_repair_ready(eid as u32).await {
+            Ok(rc) => rc.into_inner(),
+            Err(e) => {
+                crucible_bail!(
+                    RepairRequestError,
+                    "Failed to verify extent is still valid for repair {:?}",
+                    e,
+                );
+            }
+        };
+
+        if !rc {
+            warn!(self.log, "The repair of {eid} is being aborted.");
+            crucible_bail!(
+                RepairRequestError,
+                "Extent {eid} on remote repair server is in incorrect state",
+            );
+        }
+
         // After we have all files: move the repair dir.
         info!(
             self.log,

downstairs/src/region.rs

@@ -25,6 +25,7 @@ pub struct FileServerContext {
     region_dir: PathBuf,
     read_only: bool,
     region_definition: RegionDefinition,
+    downstairs: Arc<Mutex<Downstairs>>,
 }
 
 pub fn write_openapi<W: Write>(f: &mut W) -> Result<()> {
@@ -39,13 +40,14 @@ fn build_api() -> ApiDescription<Arc<FileServerContext>> {
     api.register(get_files_for_extent).unwrap();
     api.register(get_region_info).unwrap();
     api.register(get_region_mode).unwrap();
+    api.register(extent_repair_ready).unwrap();
 
     api
 }
 
 /// Returns Ok(listen address) if everything launched ok, Err otherwise
 pub async fn repair_main(
-    ds: &Mutex<Downstairs>,
+    downstairs: Arc<Mutex<Downstairs>>,
     addr: SocketAddr,
     log: &Logger,
 ) -> Result<SocketAddr, String> {
@@ -67,7 +69,7 @@ pub async fn repair_main(
      * Record the region directory where all the extents and metadata
      * files live.
      */
-    let ds = ds.lock().await;
+    let ds = downstairs.lock().await;
     let region_dir = ds.region.dir.clone();
     let read_only = ds.read_only;
     let region_definition = ds.region.def();
@@ -78,6 +80,7 @@ pub async fn repair_main(
         region_dir,
         read_only,
         region_definition,
+        downstairs,
     };
 
     /*
@@ -186,6 +189,31 @@ async fn get_a_file(path: PathBuf) -> Result<Response<Body>, HttpError> {
     }
 }
 
+/// Return true if the provided extent is closed or the region is read only
+#[endpoint {
+    method = GET,
+    path = "/extent/{eid}/repair-ready",
+}]
+async fn extent_repair_ready(
+    rqctx: RequestContext<Arc<FileServerContext>>,
+    path: Path<Eid>,
+) -> Result<HttpResponseOk<bool>, HttpError> {
+    let eid: usize = path.into_inner().eid as usize;
+    let downstairs = &rqctx.context().downstairs;
+
+    // If the region is read only, the extent is always ready.
+    if rqctx.context().read_only {
+        return Ok(HttpResponseOk(true));
+    }
+
+    let res = {
+        let ds = downstairs.lock().await;
+        matches!(ds.region.extents[eid], ExtentState::Closed)
+    };
+
+    Ok(HttpResponseOk(res))
+}
+
 /**
  * Get the list of files related to an extent.
  *

downstairs/src/repair.rs

@@ -26,6 +26,7 @@ futures.workspace = true
 hex.workspace = true
 httptest.workspace = true
 rand.workspace = true
+repair-client.workspace = true
 reqwest.workspace = true
 serde.workspace = true
 serde_json.workspace = true

integration_tests/Cargo.toml

@@ -16,6 +16,7 @@ mod test {
     use futures::lock::Mutex;
     use httptest::{matchers::*, responders::*, Expectation, Server};
     use rand::Rng;
+    use repair_client::Client;
     use sha2::Digest;
     use slog::{info, o, warn, Drain, Logger};
     use tempfile::*;
@@ -2818,6 +2819,74 @@ mod test {
         Ok(())
     }
 
+    #[tokio::test]
+    async fn integration_test_repair_ready_not_closed() -> Result<()> {
+        // Create a new downstairs.
+        // Verify repair ready returns false when an extent is open
+
+        // Create a downstairs
+        let new_ds = TestDownstairs::new(
+            "127.0.0.1".parse().unwrap(),
+            true,  // encrypted
+            false, // read only
+            5,
+            2,
+            false,
+            Backend::RawFile,
+        )
+        .await
+        .unwrap();
+
+        let repair_addr = new_ds.repair_address().await;
+
+        let url = format!("http://{:?}", repair_addr);
+        let repair_server = Client::new(&url);
+
+        // Extent are open, so the repair ready request should return false.
+        let rc = repair_server.extent_repair_ready(0).await;
+        assert!(!rc.unwrap().into_inner());
+
+        let rc = repair_server.extent_repair_ready(1).await;
+        assert!(!rc.unwrap().into_inner());
+
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn integration_test_repair_ready_read_only() -> Result<()> {
+        // Create a new downstairs.
+        // Verify repair ready returns true for read only downstairs,
+        // even when extents are open.
+
+        // Create a downstairs
+        let new_ds = TestDownstairs::new(
+            "127.0.0.1".parse().unwrap(),
+            true, // encrypted
+            true, // read only
+            5,
+            2,
+            false,
+            Backend::RawFile,
+        )
+        .await
+        .unwrap();
+
+        let repair_addr = new_ds.repair_address().await;
+
+        let url = format!("http://{:?}", repair_addr);
+        let repair_server = Client::new(&url);
+
+        // Extent are not open, but the region is read only, so the requests
+        // should all return true.
+        let rc = repair_server.extent_repair_ready(0).await;
+        assert!(rc.unwrap().into_inner());
+
+        let rc = repair_server.extent_repair_ready(1).await;
+        assert!(rc.unwrap().into_inner());
+
+        Ok(())
+    }
+
     #[tokio::test]
     async fn integration_test_clone_diff_ec() -> Result<()> {
         // Test downstairs region clone.

integration_tests/src/lib.rs

@@ -46,6 +46,43 @@
         }
       }
     },
+    "/extent/{eid}/repair-ready": {
+      "get": {
+        "summary": "Return true if the provided extent is closed or the region is read only",
+        "operationId": "extent_repair_ready",
+        "parameters": [
+          {
+            "in": "path",
+            "name": "eid",
+            "required": true,
+            "schema": {
+              "type": "integer",
+              "format": "uint32",
+              "minimum": 0
+            }
+          }
+        ],
+        "responses": {
+          "200": {
+            "description": "successful operation",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "title": "Boolean",
+                  "type": "boolean"
+                }
+              }
+            }
+          },
+          "4XX": {
+            "$ref": "#/components/responses/Error"
+          },
+          "5XX": {
+            "$ref": "#/components/responses/Error"
+          }
+        }
+      }
+    },
     "/newextent/{eid}/{file_type}": {
       "get": {
         "operationId": "get_extent_file",

openapi/downstairs-repair.json

@@ -1271,8 +1271,8 @@ impl DownstairsClient {
         if old_state != IOState::InProgress {
             // This job is in an unexpected state.
             panic!(
-                "[{}] Job {} completed while not InProgress: {:?}",
-                self.client_id, ds_id, old_state
+                "[{}] Job {} completed while not InProgress: {:?} {:?}",
+                self.client_id, ds_id, old_state, job
             );
         }