libcnb-test: Clean up Docker volumes after each test

.github/workflows/ci.yml

@@ -77,9 +77,7 @@ jobs:
           if [[ -n "$(docker volume ls --quiet)" ]]; then
             docker volume ls
             echo "Unexpected Docker volumes left behind!"
-            # TODO: Fix volume cleanup and make this fail
-            # https://github.com/heroku/libcnb.rs/issues/570
-            # exit 1
+            exit 1
           fi
 
           if [[ -n "$(docker images --all --quiet '*libcnb*')" ]]; then

CHANGELOG.md

@@ -18,6 +18,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - `libcnb-test`:
   - Fixed incorrect error messages being shown for buildpack compilation/packaging failures. ([#720](https://github.com/heroku/libcnb.rs/pull/720))
+  - The Docker volumes created by Pack for the build and launch layer caches are now cleaned up after each test. ([#741](https://github.com/heroku/libcnb.rs/pull/741))
+  - The Docker image cleanup process no longer makes duplicate attempts to remove images when using `TestContext::rebuild`. ([#741](https://github.com/heroku/libcnb.rs/pull/741))
+  - Test failures due to the Docker daemon not being installed or started no longer cause a non-unwinding panic abort with noisy traceback. ([#741](https://github.com/heroku/libcnb.rs/pull/741))
   - Containers created by `TestContext::start_container` are now correctly cleaned up if the container failed to start. ([#742](https://github.com/heroku/libcnb.rs/pull/742))
 
 ## [0.15.0] - 2023-09-25

libcnb-test/src/docker.rs

@@ -254,6 +254,37 @@ impl From<DockerRemoveImageCommand> for Command {
     }
 }
 
+/// Represents a `docker volume remove` command.
+#[derive(Clone, Debug)]
+pub(crate) struct DockerRemoveVolumeCommand {
+    force: bool,
+    volume_names: Vec<String>,
+}
+
+impl DockerRemoveVolumeCommand {
+    pub fn new<I: IntoIterator<Item = S>, S: Into<String>>(volume_names: I) -> Self {
+        Self {
+            force: true,
+            volume_names: volume_names.into_iter().map(S::into).collect(),
+        }
+    }
+}
+
+impl From<DockerRemoveVolumeCommand> for Command {
+    fn from(docker_remove_volume_command: DockerRemoveVolumeCommand) -> Self {
+        let mut command = Command::new("docker");
+        command
+            .args(["volume", "remove"])
+            .args(&docker_remove_volume_command.volume_names);
+
+        if docker_remove_volume_command.force {
+            command.arg("--force");
+        }
+
+        command
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -375,4 +406,15 @@ mod tests {
             ["rmi", "my-image", "--force"]
         );
     }
+
+    #[test]
+    fn from_docker_remove_volume_command_to_command() {
+        let docker_remove_volume_command = DockerRemoveVolumeCommand::new(["volume1", "volume2"]);
+        let command: Command = docker_remove_volume_command.into();
+        assert_eq!(command.get_program(), "docker");
+        assert_eq!(
+            command.get_args().collect::<Vec<&OsStr>>(),
+            ["volume", "remove", "volume1", "volume2", "--force"]
+        );
+    }
 }

libcnb-test/src/pack.rs

@@ -5,10 +5,12 @@ use std::process::Command;
 /// Represents a `pack build` command.
 #[derive(Clone, Debug)]
 pub(crate) struct PackBuildCommand {
+    build_cache_volume_name: String,
     builder: String,
     buildpacks: Vec<BuildpackReference>,
     env: BTreeMap<String, String>,
     image_name: String,
+    launch_cache_volume_name: String,
     path: PathBuf,
     pull_policy: PullPolicy,
     trust_builder: bool,
@@ -49,12 +51,16 @@ impl PackBuildCommand {
         builder: impl Into<String>,
         path: impl Into<PathBuf>,
         image_name: impl Into<String>,
+        build_cache_volume_name: impl Into<String>,
+        launch_cache_volume_name: impl Into<String>,
     ) -> Self {
         Self {
+            build_cache_volume_name: build_cache_volume_name.into(),
             builder: builder.into(),
             buildpacks: Vec::new(),
             env: BTreeMap::new(),
             image_name: image_name.into(),
+            launch_cache_volume_name: launch_cache_volume_name.into(),
             path: path.into(),
             // Prevent redundant image-pulling, which slows tests and risks hitting registry rate limits.
             pull_policy: PullPolicy::IfNotPresent,
@@ -82,6 +88,16 @@ impl From<PackBuildCommand> for Command {
             &pack_build_command.image_name,
             "--builder",
             &pack_build_command.builder,
+            "--cache",
+            &format!(
+                "type=build;format=volume;name={}",
+                pack_build_command.build_cache_volume_name
+            ),
+            "--cache",
+            &format!(
+                "type=launch;format=volume;name={}",
+                pack_build_command.launch_cache_volume_name
+            ),
             "--path",
             &pack_build_command.path.to_string_lossy(),
             "--pull-policy",
@@ -157,6 +173,7 @@ mod tests {
     #[test]
     fn from_pack_build_command_to_command() {
         let mut input = PackBuildCommand {
+            build_cache_volume_name: String::from("build-cache-volume"),
             builder: String::from("builder:20"),
             buildpacks: vec![
                 BuildpackReference::Id(String::from("libcnb/buildpack1")),
@@ -167,6 +184,7 @@ mod tests {
                 (String::from("ENV_BAR"), String::from("WHITESPACE VALUE")),
             ]),
             image_name: String::from("my-image"),
+            launch_cache_volume_name: String::from("launch-cache-volume"),
             path: PathBuf::from("/tmp/foo/bar"),
             pull_policy: PullPolicy::IfNotPresent,
             trust_builder: true,
@@ -183,6 +201,10 @@ mod tests {
                 "my-image",
                 "--builder",
                 "builder:20",
+                "--cache",
+                "type=build;format=volume;name=build-cache-volume",
+                "--cache",
+                "type=launch;format=volume;name=launch-cache-volume",
                 "--path",
                 "/tmp/foo/bar",
                 "--pull-policy",

libcnb-test/src/test_context.rs

@@ -1,6 +1,9 @@
-use crate::docker::{DockerRemoveImageCommand, DockerRunCommand};
+use crate::docker::DockerRunCommand;
 use crate::pack::PackSbomDownloadCommand;
-use crate::{util, BuildConfig, ContainerConfig, ContainerContext, LogOutput, TestRunner};
+use crate::{
+    util, BuildConfig, ContainerConfig, ContainerContext, LogOutput, TemporaryDockerResources,
+    TestRunner,
+};
 use libcnb_data::buildpack::BuildpackId;
 use libcnb_data::layer::LayerName;
 use libcnb_data::sbom::SbomFormat;
@@ -17,7 +20,7 @@ pub struct TestContext<'a> {
     /// The configuration used for this integration test.
     pub config: BuildConfig,
 
-    pub(crate) image_name: String,
+    pub(crate) docker_resources: TemporaryDockerResources,
     pub(crate) runner: &'a TestRunner,
 }
 
@@ -86,7 +89,8 @@ impl<'a> TestContext<'a> {
         let config = config.borrow();
         let container_name = util::random_docker_identifier();
 
-        let mut docker_run_command = DockerRunCommand::new(&self.image_name, &container_name);
+        let mut docker_run_command =
+            DockerRunCommand::new(&self.docker_resources.image_name, &container_name);
         docker_run_command.detach(true);
         docker_run_command.platform(self.determine_container_platform());
 
@@ -170,8 +174,10 @@ impl<'a> TestContext<'a> {
     /// Panics if there was an error starting the container, or the command exited with a non-zero
     /// exit code.
     pub fn run_shell_command(&self, command: impl Into<String>) -> LogOutput {
-        let mut docker_run_command =
-            DockerRunCommand::new(&self.image_name, util::random_docker_identifier());
+        let mut docker_run_command = DockerRunCommand::new(
+            &self.docker_resources.image_name,
+            util::random_docker_identifier(),
+        );
         docker_run_command
             .remove(true)
             .platform(self.determine_container_platform())
@@ -229,7 +235,7 @@ impl<'a> TestContext<'a> {
     pub fn download_sbom_files<R, F: Fn(SbomFiles) -> R>(&self, f: F) -> R {
         let temp_dir = tempdir().expect("Couldn't create temporary directory for SBOM files");
 
-        let mut command = PackSbomDownloadCommand::new(&self.image_name);
+        let mut command = PackSbomDownloadCommand::new(&self.docker_resources.image_name);
         command.output_dir(temp_dir.path());
 
         util::run_command(command)
@@ -270,15 +276,7 @@ impl<'a> TestContext<'a> {
     /// );
     /// ```
     pub fn rebuild<C: Borrow<BuildConfig>, F: FnOnce(TestContext)>(self, config: C, f: F) {
-        self.runner
-            .build_internal(self.image_name.clone(), config, f);
-    }
-}
-
-impl<'a> Drop for TestContext<'a> {
-    fn drop(&mut self) {
-        util::run_command(DockerRemoveImageCommand::new(&self.image_name))
-            .unwrap_or_else(|command_err| panic!("Error removing Docker image:\n\n{command_err}"));
+        self.runner.build_internal(self.docker_resources, config, f);
     }
 }
 

libcnb-test/src/test_runner.rs

@@ -1,4 +1,4 @@
-use crate::docker::DockerRemoveImageCommand;
+use crate::docker::{DockerRemoveImageCommand, DockerRemoveVolumeCommand};
 use crate::pack::PackBuildCommand;
 use crate::util::CommandError;
 use crate::{app, build, util, BuildConfig, BuildpackReference, PackResult, TestContext};
@@ -48,12 +48,18 @@ impl TestRunner {
     /// )
     /// ```
     pub fn build<C: Borrow<BuildConfig>, F: FnOnce(TestContext)>(&self, config: C, f: F) {
-        self.build_internal(util::random_docker_identifier(), config, f);
+        let image_name = util::random_docker_identifier();
+        let docker_resources = TemporaryDockerResources {
+            build_cache_volume_name: format!("{image_name}.build-cache"),
+            launch_cache_volume_name: format!("{image_name}.launch-cache"),
+            image_name,
+        };
+        self.build_internal(docker_resources, config, f);
     }
 
     pub(crate) fn build_internal<C: Borrow<BuildConfig>, F: FnOnce(TestContext)>(
         &self,
-        image_name: String,
+        docker_resources: TemporaryDockerResources,
         config: C,
         f: F,
     ) {
@@ -94,7 +100,13 @@ impl TestRunner {
         let buildpacks_target_dir =
             tempdir().expect("Error creating temporary directory for compiled buildpacks");
 
-        let mut pack_command = PackBuildCommand::new(&config.builder_name, &app_dir, &image_name);
+        let mut pack_command = PackBuildCommand::new(
+            &config.builder_name,
+            &app_dir,
+            &docker_resources.image_name,
+            &docker_resources.build_cache_volume_name,
+            &docker_resources.launch_cache_volume_name,
+        );
 
         config.env.iter().for_each(|(key, value)| {
             pack_command.env(key, value);
@@ -143,13 +155,6 @@ impl TestRunner {
                 log_output
             }
             (PackResult::Failure, Ok(log_output)) => {
-                // Ordinarily the Docker image created by `pack build` will either be cleaned up by
-                // `TestContext::Drop` later on, or will not have been created in the first place,
-                // if the `pack build` was not successful. However, in the case of an unexpectedly
-                // successful `pack build` we have to clean this image up manually before `panic`ing.
-                util::run_command(DockerRemoveImageCommand::new(image_name)).unwrap_or_else(
-                    |command_err| panic!("Error removing Docker image:\n\n{command_err}"),
-                );
                 panic!("The pack build was expected to fail, but did not:\n\n{log_output}");
             }
             (_, Err(command_err)) => {
@@ -160,11 +165,31 @@ impl TestRunner {
         let test_context = TestContext {
             pack_stdout: output.stdout,
             pack_stderr: output.stderr,
-            image_name,
+            docker_resources,
             config: config.clone(),
             runner: self,
         };
 
         f(test_context);
     }
 }
+
+pub(crate) struct TemporaryDockerResources {
+    pub(crate) build_cache_volume_name: String,
+    pub(crate) image_name: String,
+    pub(crate) launch_cache_volume_name: String,
+}
+
+impl Drop for TemporaryDockerResources {
+    fn drop(&mut self) {
+        // Ignoring errors here since we don't want to panic inside Drop.
+        // We don't emit a warning to stderr since that gets too noisy in some common
+        // cases (such as running a test suite when Docker isn't started) where the tests
+        // themselves will also report the same error message.
+        let _ = util::run_command(DockerRemoveImageCommand::new(&self.image_name));
+        let _ = util::run_command(DockerRemoveVolumeCommand::new([
+            &self.build_cache_volume_name,
+            &self.launch_cache_volume_name,
+        ]));
+    }
+}