libcnb-test: Clean up Docker volumes after each test

When using `pack build`, by default Pack will create two Docker
volumes for caching - one for the build layer cache, and the other
for the launch layer cache. Pack bases the names for these volumes
on the app image name, so that the next time a build is performed
with the same app image name, the associated cache is used for
that rebuild.

When libcnb-test runs tests, it intentionally uses a random Docker
image name for each new test, and then tears down the image and
any associated containers after the tests.

However, until now the Docker volumes automatically created by
Pack were not being removed, which can cause issues when
running the tests locally, since the Docker VM can run out of disk
space - as seen in #570. The leftover volumes can also be seen
in the tests log output added in #740.

Pack doesn't currently offer a "clean up the volumes associated
with this app name" feature, so we have to remove these volumes
manually. To do so we need the volume name - which we achieve
by generating a name ourselves, and passing it to `pack build`
using the `--cache` options, so Pack uses our volume name instead
of generating its own automatic volume name.

Since we're now cleaning up not only images but volumes too, I've
refactored the way resources are cleaned up, to reduce the amount
of duplication (since otherwise we'd need to handle the "still clean up
for expected failures" case for volumes too).

This new implementation also solves #735 and #736.

Fixes #570.
Fixes #735.
Fixes #736.
GUS-W-13195631.
GUS-W-14503186.
GUS-W-14503192.

.github/workflows/ci.yml

@@ -79,9 +79,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))
 
 ## [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());
 
@@ -166,8 +170,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())
@@ -225,7 +231,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)
@@ -266,15 +272,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,12 @@ 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);
+        self.build_internal(TemporaryDockerResources::new(), 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 +94,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 +149,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 +159,42 @@ 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 TemporaryDockerResources {
+    pub fn new() -> Self {
+        let image_name = util::random_docker_identifier();
+        Self {
+            build_cache_volume_name: format!("{image_name}.build-cache"),
+            launch_cache_volume_name: format!("{image_name}.launch-cache"),
+            image_name,
+        }
+    }
+}
+
+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,
+        ]));
+    }
+}