diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f47818d6..2ed26e15 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 40820a88..6900b8e5 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/libcnb-test/src/docker.rs b/libcnb-test/src/docker.rs index f593405e..0cef7e39 100644 --- a/libcnb-test/src/docker.rs +++ b/libcnb-test/src/docker.rs @@ -254,6 +254,37 @@ impl From for Command { } } +/// Represents a `docker volume remove` command. +#[derive(Clone, Debug)] +pub(crate) struct DockerRemoveVolumeCommand { + force: bool, + volume_names: Vec, +} + +impl DockerRemoveVolumeCommand { + pub fn new, S: Into>(volume_names: I) -> Self { + Self { + force: true, + volume_names: volume_names.into_iter().map(S::into).collect(), + } + } +} + +impl From 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::>(), + ["volume", "remove", "volume1", "volume2", "--force"] + ); + } } diff --git a/libcnb-test/src/pack.rs b/libcnb-test/src/pack.rs index 2fc5e966..d5201ef3 100644 --- a/libcnb-test/src/pack.rs +++ b/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, env: BTreeMap, 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, path: impl Into, image_name: impl Into, + build_cache_volume_name: impl Into, + launch_cache_volume_name: impl Into, ) -> 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 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", diff --git a/libcnb-test/src/test_context.rs b/libcnb-test/src/test_context.rs index 35090501..78ccef59 100644 --- a/libcnb-test/src/test_context.rs +++ b/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) -> 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>(&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, 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); } } diff --git a/libcnb-test/src/test_runner.rs b/libcnb-test/src/test_runner.rs index df4d1f3c..72edc3a5 100644 --- a/libcnb-test/src/test_runner.rs +++ b/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, 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, 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,7 +159,7 @@ impl TestRunner { let test_context = TestContext { pack_stdout: output.stdout, pack_stderr: output.stderr, - image_name, + docker_resources, config: config.clone(), runner: self, }; @@ -168,3 +167,34 @@ impl TestRunner { 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, + ])); + } +}