From b3c75c7e61d20c15fd8a14ff0669761074444154 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 22 Dec 2022 11:50:43 -0800 Subject: [PATCH 1/3] fail faster on errors reading CockroachDB listening-url file --- test-utils/src/dev/db.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/test-utils/src/dev/db.rs b/test-utils/src/dev/db.rs index 12b93fc14c2..183accbc800 100644 --- a/test-utils/src/dev/db.rs +++ b/test-utils/src/dev/db.rs @@ -377,8 +377,27 @@ impl CockroachStarter { ) }) } - - _ => Err(poll::CondCheckError::NotYet), + Ok(_) => { + // The file hasn't been fully written yet. + Err(poll::CondCheckError::NotYet) + } + Err(error) => { + let maybe_errno = error.raw_os_error(); + if maybe_errno.is_some() + && maybe_errno.unwrap() == libc::ENOENT + { + // The file doesn't exist yet. + Err(poll::CondCheckError::NotYet) + } else { + let source = anyhow!(error).context(format!( + "checking listen file {:?}", + listen_url_file + )); + Err(poll::CondCheckError::Failed( + CockroachStartError::Unknown { source }, + )) + } + } } } }, From 3d0cc4f6c71fa76cb15330eb1d592fc3f9fdd1c4 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 22 Dec 2022 14:09:07 -0800 Subject: [PATCH 2/3] add a test --- test-utils/src/dev/db.rs | 61 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 55 insertions(+), 6 deletions(-) diff --git a/test-utils/src/dev/db.rs b/test-utils/src/dev/db.rs index 183accbc800..ba8d58ff203 100644 --- a/test-utils/src/dev/db.rs +++ b/test-utils/src/dev/db.rs @@ -315,6 +315,12 @@ impl CockroachStarter { self.temp_dir.path() } + /// Returns the path to the listen-url file for this execution + #[cfg(test)] + pub fn listen_url_file(&self) -> &Path { + &self.listen_url_file + } + /// Returns the path to the storage directory created for this execution. pub fn store_dir(&self) -> &Path { self.store_dir.as_path() @@ -1015,7 +1021,7 @@ mod test { #[tokio::test] async fn test_bad_cmd() { let builder = CockroachStarterBuilder::new_with_cmd("/nonexistent"); - let _ = test_database_start_failure(builder).await; + let _ = test_database_start_failure(builder.build().unwrap()).await; } // Tests what happens if the "cockroach" command exits before writing the @@ -1025,7 +1031,8 @@ mod test { async fn test_cmd_fails() { let mut builder = new_builder(); builder.arg("not-a-valid-argument"); - let temp_dir = test_database_start_failure(builder).await; + let (temp_dir, _) = + test_database_start_failure(builder.build().unwrap()).await; fs::metadata(&temp_dir).await.expect("temporary directory was deleted"); // The temporary directory is preserved in this case so that we can // debug the failure. In this case, we injected the failure. Remove @@ -1051,9 +1058,8 @@ mod test { // caller can decide whether to check if it was cleaned up or not. The // expected behavior depends on the failure mode. async fn test_database_start_failure( - builder: CockroachStarterBuilder, - ) -> PathBuf { - let starter = builder.build().unwrap(); + starter: CockroachStarter, + ) -> (PathBuf, CockroachStartError) { let temp_dir = starter.temp_dir().to_owned(); eprintln!("will run: {}", starter.cmdline()); eprintln!("environment:"); @@ -1063,7 +1069,7 @@ mod test { let error = starter.start().await.expect_err("unexpectedly started database"); eprintln!("error: {:?}", error); - temp_dir + (temp_dir, error) } // Tests when CockroachDB hangs on startup by setting the start timeout @@ -1151,6 +1157,49 @@ mod test { }); } + // Test what happens if we can't read the listen-url file. This is a little + // obscure, but it has been a problem. + #[tokio::test] + async fn test_setup_database_bad_listen_url() { + // We don't need to actually run Cockroach for this test, and it's + // simpler (and faster) if we don't. But we do need something that + // won't exit before we get a chance to trigger an error and that can + // also accept the extra arguments that the builder will provide. + let mut builder = CockroachStarterBuilder::new_with_cmd("bash"); + builder.arg("-c").arg("sleep 60"); + let starter = builder.build().unwrap(); + + // We want to inject an error into the code path that reads the + // listen-url file. We do this by precreating that path as a directory. + // Then we'll get EISDIR when we try to read it. + let listen_url_file = starter.listen_url_file().to_owned(); + std::fs::create_dir(&listen_url_file) + .expect("pre-creating listen-URL path as directory"); + let (temp_dir, error) = test_database_start_failure(starter).await; + + if let CockroachStartError::Unknown { source } = error { + let message = format!("{:#}", source); + eprintln!("error message was: {}", message); + // Verify the error message refers to the listening file (since + // that's what we were operating on) and also reflects the EISDIR + // error. + assert!(message.starts_with("checking listen file \"")); + assert!(message.contains("Is a directory")); + } else { + panic!("unexpected error trying to start database: {:#}", error); + } + + // Clean up the temporary directory -- carefully. Since we know exactly + // what should be in it, we opt to remove these items individually + // rather than risk blowing away something else inadvertently. + fs::remove_dir(&listen_url_file) + .await + .expect("failed to remove listen-url directory"); + fs::remove_dir(temp_dir) + .await + .expect("failed to remove temporary directory"); + } + // Test the happy path using the default store directory. #[tokio::test] async fn test_setup_database_default_dir() { From 546b63c93ede93cf0a6f7c4c2c3605c79b8acbf2 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 22 Dec 2022 14:12:27 -0800 Subject: [PATCH 3/3] review feedback --- test-utils/src/dev/db.rs | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/test-utils/src/dev/db.rs b/test-utils/src/dev/db.rs index ba8d58ff203..23a1c2d1649 100644 --- a/test-utils/src/dev/db.rs +++ b/test-utils/src/dev/db.rs @@ -373,6 +373,8 @@ impl CockroachStarter { // memory. match tokio::fs::read_to_string(&listen_url_file).await { Ok(listen_url) if listen_url.contains('\n') => { + // The file is fully written. + // We're ready to move on. let listen_url = listen_url.trim_end(); make_pg_config(listen_url).map_err(|source| { poll::CondCheckError::Failed( @@ -383,26 +385,31 @@ impl CockroachStarter { ) }) } + Ok(_) => { // The file hasn't been fully written yet. + // Keep waiting. Err(poll::CondCheckError::NotYet) } + + Err(error) + if error.kind() == std::io::ErrorKind::NotFound => + { + // The file doesn't exist yet. + // Keep waiting. + Err(poll::CondCheckError::NotYet) + } + Err(error) => { - let maybe_errno = error.raw_os_error(); - if maybe_errno.is_some() - && maybe_errno.unwrap() == libc::ENOENT - { - // The file doesn't exist yet. - Err(poll::CondCheckError::NotYet) - } else { - let source = anyhow!(error).context(format!( - "checking listen file {:?}", - listen_url_file - )); - Err(poll::CondCheckError::Failed( - CockroachStartError::Unknown { source }, - )) - } + // Something else has gone wrong. Stop immediately + // and report the problem. + let source = anyhow!(error).context(format!( + "checking listen file {:?}", + listen_url_file + )); + Err(poll::CondCheckError::Failed( + CockroachStartError::Unknown { source }, + )) } } }