Merge pull request #741 from cgwalters/init-container-storage-prep3

A few prep commits for #724

Author: cgwalters

cli/Cargo.toml

@@ -6,7 +6,8 @@ license = "MIT OR Apache-2.0"
 repository = "https://github.com/containers/bootc"
 readme = "README.md"
 publish = false
-rust-version = "1.63.0"
+# For now don't bump this above what is currently shipped in RHEL9.
+rust-version = "1.75.0"
 default-run = "bootc"
 
 # See https://github.com/coreos/cargo-vendor-filterer

lib/Cargo.toml

@@ -6,6 +6,8 @@ name = "bootc-lib"
 readme = "README.md"
 repository = "https://github.com/containers/bootc"
 version = "0.1.14"
+# For now don't bump this above what is currently shipped in RHEL9;
+# also keep in sync with the version in cli.
 rust-version = "1.75.0"
 build = "build.rs"
 
@@ -30,7 +32,7 @@ liboverdrop = "0.1.0"
 libsystemd = "0.7"
 openssl = "^0.10.64"
 regex = "1.10.4"
-rustix = { "version" = "0.38.34", features = ["thread", "fs", "system", "process"] }
+rustix = { "version" = "0.38.34", features = ["thread", "fs", "system", "process", "mount"] }
 schemars = { version = "0.8.17", features = ["chrono"] }
 serde = { workspace = true, features = ["derive"] }
 serde_ignored = "0.1.10"

lib/src/utils.rs

@@ -1,5 +1,5 @@
 use std::future::Future;
-use std::io::Write;
+use std::io::{Read, Seek, Write};
 use std::os::fd::BorrowedFd;
 use std::process::Command;
 use std::time::Duration;
@@ -15,17 +15,79 @@ pub(crate) trait CommandRunExt {
     fn run(&mut self) -> Result<()>;
 }
 
+/// Helpers intended for [`std::process::ExitStatus`].
+pub(crate) trait ExitStatusExt {
+    /// If the exit status signals it was not successful, return an error.
+    /// Note that we intentionally *don't* include the command string
+    /// in the output; we leave it to the caller to add that if they want,
+    /// as it may be verbose.
+    fn check_status(&mut self, stderr: std::fs::File) -> Result<()>;
+}
+
+/// Parse the last chunk (e.g. 1024 bytes) from the provided file,
+/// ensure it's UTF-8, and return that value. This function is infallible;
+/// if the file cannot be read for some reason, a copy of a static string
+/// is returned.
+fn last_utf8_content_from_file(mut f: std::fs::File) -> String {
+    // u16 since we truncate to just the trailing bytes here
+    // to avoid pathological error messages
+    const MAX_STDERR_BYTES: u16 = 1024;
+    let size = f
+        .metadata()
+        .map_err(|e| {
+            tracing::warn!("failed to fstat: {e}");
+        })
+        .map(|m| m.len().try_into().unwrap_or(u16::MAX))
+        .unwrap_or(0);
+    let size = size.min(MAX_STDERR_BYTES);
+    let seek_offset = -(size as i32);
+    let mut stderr_buf = Vec::with_capacity(size.into());
+    // We should never fail to seek()+read() really, but let's be conservative
+    let r = match f
+        .seek(std::io::SeekFrom::End(seek_offset.into()))
+        .and_then(|_| f.read_to_end(&mut stderr_buf))
+    {
+        Ok(_) => String::from_utf8_lossy(&stderr_buf),
+        Err(e) => {
+            tracing::warn!("failed seek+read: {e}");
+            "<failed to read stderr>".into()
+        }
+    };
+    (&*r).to_owned()
+}
+
+impl ExitStatusExt for std::process::ExitStatus {
+    fn check_status(&mut self, stderr: std::fs::File) -> Result<()> {
+        let stderr_buf = last_utf8_content_from_file(stderr);
+        if self.success() {
+            return Ok(());
+        }
+        anyhow::bail!(format!("Subprocess failed: {self:?}\n{stderr_buf}"))
+    }
+}
+
 impl CommandRunExt for Command {
     /// Synchronously execute the child, and return an error if the child exited unsuccessfully.
     fn run(&mut self) -> Result<()> {
-        let st = self.status()?;
-        if !st.success() {
-            // Note that we intentionally *don't* include the command string
-            // in the output; we leave it to the caller to add that if they want,
-            // as it may be verbose.
-            anyhow::bail!(format!("Subprocess failed: {st:?}"))
-        }
-        Ok(())
+        let stderr = tempfile::tempfile()?;
+        self.stderr(stderr.try_clone()?);
+        self.status()?.check_status(stderr)
+    }
+}
+
+/// Helpers intended for [`tokio::process::Command`].
+#[allow(dead_code)]
+pub(crate) trait AsyncCommandRunExt {
+    async fn run(&mut self) -> Result<()>;
+}
+
+impl AsyncCommandRunExt for tokio::process::Command {
+    /// Asynchronously execute the child, and return an error if the child exited unsuccessfully.
+    ///
+    async fn run(&mut self) -> Result<()> {
+        let stderr = tempfile::tempfile()?;
+        self.stderr(stderr.try_clone()?);
+        self.status().await?.check_status(stderr)
     }
 }
 
@@ -132,14 +194,14 @@ pub(crate) fn medium_visibility_warning(s: &str) {
 /// with an automatic spinner to show that we're not blocked.
 /// Note that generally the called function should not output
 /// anything to stdout as this will interfere with the spinner.
-pub(crate) async fn async_task_with_spinner<F, T>(msg: &'static str, f: F) -> T
+pub(crate) async fn async_task_with_spinner<F, T>(msg: &str, f: F) -> T
 where
     F: Future<Output = T>,
 {
     let pb = indicatif::ProgressBar::new_spinner();
     let style = indicatif::ProgressStyle::default_bar();
     pb.set_style(style.template("{spinner} {msg}").unwrap());
-    pb.set_message(msg);
+    pb.set_message(msg.to_string());
     pb.enable_steady_tick(Duration::from_millis(150));
     // We need to handle the case where we aren't connected to
     // a tty, so indicatif would show nothing by default.
@@ -212,6 +274,43 @@ fn test_sigpolicy_from_opts() {
 
 #[test]
 fn command_run_ext() {
+    // The basics
     Command::new("true").run().unwrap();
     assert!(Command::new("false").run().is_err());
+
+    // Verify we capture stderr
+    let e = Command::new("/bin/sh")
+        .args(["-c", "echo expected-this-oops-message 1>&2; exit 1"])
+        .run()
+        .err()
+        .unwrap();
+    similar_asserts::assert_eq!(
+        e.to_string(),
+        "Subprocess failed: ExitStatus(unix_wait_status(256))\nexpected-this-oops-message\n"
+    );
+
+    // Ignoring invalid UTF-8
+    let e = Command::new("/bin/sh")
+        .args([
+            "-c",
+            r"echo -e 'expected\xf5\x80\x80\x80\x80-foo\xc0bar\xc0\xc0' 1>&2; exit 1",
+        ])
+        .run()
+        .err()
+        .unwrap();
+    similar_asserts::assert_eq!(
+        e.to_string(),
+        "Subprocess failed: ExitStatus(unix_wait_status(256))\nexpected�����-foo�bar��\n"
+    );
+}
+
+#[tokio::test]
+async fn async_command_run_ext() {
+    use tokio::process::Command as AsyncCommand;
+    let mut success = AsyncCommand::new("true");
+    let mut fail = AsyncCommand::new("false");
+    // Run these in parallel just because we can
+    let (success, fail) = tokio::join!(success.run(), fail.run(),);
+    success.unwrap();
+    assert!(fail.is_err());
 }

tests-integration/src/install.rs

@@ -2,6 +2,7 @@ use std::path::Path;
 use std::{os::fd::AsRawFd, path::PathBuf};
 
 use anyhow::Result;
+use camino::Utf8Path;
 use cap_std_ext::cap_std;
 use cap_std_ext::cap_std::fs::Dir;
 use fn_error_context::context;
@@ -53,6 +54,12 @@ fn find_deployment_root() -> Result<Dir> {
     anyhow::bail!("Failed to find deployment root")
 }
 
+// Hook relatively cheap post-install tests here
+fn generic_post_install_verification() -> Result<()> {
+    assert!(Utf8Path::new("/ostree/repo").try_exists()?);
+    Ok(())
+}
+
 #[context("Install tests")]
 pub(crate) fn run_alongside(image: &str, mut testargs: libtest_mimic::Arguments) -> Result<()> {
     // Force all of these tests to be serial because they mutate global state
@@ -88,6 +95,8 @@ pub(crate) fn run_alongside(image: &str, mut testargs: libtest_mimic::Arguments)
                 std::fs::write(&tmp_keys, b"ssh-ed25519 ABC0123 [email protected]")?;
                 cmd!(sh, "sudo {BASE_ARGS...} {target_args...} -v {tmp_keys}:/test_authorized_keys {image} bootc install to-filesystem {generic_inst_args...} --acknowledge-destructive --karg=foo=bar --replace=alongside --root-ssh-authorized-keys=/test_authorized_keys /target").run()?;
 
+                generic_post_install_verification()?;
+
                 // Test kargs injected via CLI
                 cmd!(
                     sh,
@@ -120,6 +129,7 @@ pub(crate) fn run_alongside(image: &str, mut testargs: libtest_mimic::Arguments)
             let sh = &xshell::Shell::new()?;
             reset_root(sh)?;
             cmd!(sh, "sudo {BASE_ARGS...} {target_args...} {image} bootc install to-existing-root --acknowledge-destructive {generic_inst_args...}").run()?;
+            generic_post_install_verification()?;
             let root = &Dir::open_ambient_dir("/ostree", cap_std::ambient_authority()).unwrap();
             let mut path = PathBuf::from(".");
             crate::selinux::verify_selinux_recurse(root, &mut path, false)?;
@@ -131,6 +141,7 @@ pub(crate) fn run_alongside(image: &str, mut testargs: libtest_mimic::Arguments)
             let empty = sh.create_temp_dir()?;
             let empty = empty.path().to_str().unwrap();
             cmd!(sh, "sudo {BASE_ARGS...} {target_args...} -v {empty}:/usr/lib/bootc/install {image} bootc install to-existing-root {generic_inst_args...}").run()?;
+            generic_post_install_verification()?;
             Ok(())
         }),
     ];