Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libcnb-test: Images not cleaned up when pack build unexpectedly suceeeds #586

Closed
edmorley opened this issue Jul 4, 2023 · 0 comments · Fixed by #625
Closed

libcnb-test: Images not cleaned up when pack build unexpectedly suceeeds #586

edmorley opened this issue Jul 4, 2023 · 0 comments · Fixed by #625
Assignees
Labels
bug Something isn't working libcnb-test

Comments

@edmorley
Copy link
Member

edmorley commented Jul 4, 2023

When libcnb-test invokes pack build, it passes an explicit Docker image name that is then docker rmi'd once the associated TestContext struct is dropped:

impl<'a> Drop for TestContext<'a> {
fn drop(&mut self) {
// We do not care if image removal succeeded or not. Panicking here would result in
// SIGILL since this function might be called in a Tokio runtime.
let _image_delete_result =
self.runner
.tokio_runtime
.block_on(self.runner.docker.remove_image(
&self.image_name,
Some(RemoveImageOptions {
force: true,
..RemoveImageOptions::default()
}),
None,
));
}
}

However, there is one scenario where this clean-up is not taking place: When .expected_pack_result(PackResult::Failure) has been used, and then the pack build unexpectedly succeeds.

For example, during this the unexpected_pack_success testcase:

#[test]
#[ignore = "integration test"]
#[should_panic(expected = "pack command unexpectedly succeeded with exit-code 0!
pack stdout:
")]
fn unexpected_pack_success() {
TestRunner::default().build(
BuildConfig::new("heroku/builder:22", "test-fixtures/procfile")
.buildpacks(vec![BuildpackReference::Other(String::from(PROCFILE_URL))])
.expected_pack_result(PackResult::Failure),
|_| {},
);
}

In this scenario, the panic occurs in run_pack_command before the TestContext has been created:

/// Runs the given pack command, panicking with user-friendly error messages when errors occur.
pub(crate) fn run_pack_command<C: Into<Command>>(
command: C,
expected_result: &PackResult,
) -> Output {
let output = command.into()
.output()
.unwrap_or_else(|io_error| {
if io_error.kind() == std::io::ErrorKind::NotFound {
panic!("External `pack` command not found. Install Pack CLI and ensure it is on PATH: https://buildpacks.io/docs/install-pack");
} else {
panic!("Could not spawn external `pack` process: {io_error}");
};
});
if (expected_result == &PackResult::Success && !output.status.success())
|| (expected_result == &PackResult::Failure && output.status.success())
{
panic!(
"pack command unexpectedly {} with exit-code {}!\n\npack stdout:\n{}\n\npack stderr:\n{}",
if output.status.success() {
"succeeded"
} else {
"failed"
},
output
.status
.code()
.map_or(String::from("<unknown>"), |exit_code| exit_code.to_string()),
String::from_utf8_lossy(&output.stdout),
String::from_utf8_lossy(&output.stderr)
);
}
output
}

This means there is no TestContext to be dropped, so no remove image command is invoked.

This causes a leak of one image each time the libcnb.rs repo's integration tests are run, eg:

$ docker images
REPOSITORY                                  TAG       IMAGE ID       CREATED        SIZE
...
libcnbtest_bwgvfgtzszywyveaqjfnhhknfycclp   latest    d35f71755225   43 years ago   650MB

GUS-W-13903181.

@edmorley edmorley added bug Something isn't working libcnb-test labels Jul 4, 2023
colincasey added a commit that referenced this issue Jul 5, 2023
Moves the creation of the `TestContext` to before the invocation of `run_pack_command` so that the context will be cleaned up regardless of the outcome.

@fixes #586
colincasey added a commit that referenced this issue Jul 5, 2023
Moves the creation of the `TestContext` to before the invocation of `run_pack_command` so that the context will be cleaned up regardless of the outcome.

@fixes #586
@edmorley edmorley self-assigned this Aug 8, 2023
edmorley added a commit that referenced this issue Aug 8, 2023
#625)

Previously, if a test with an expected result of `PackResult::Failure`
unexpectedly succeeded, the built app image was left behind since
cleanup was only performed in `TestContext::Drop` (and in this
scenario, a `TestContext` is never created in the first place).

Now the app image is correctly cleaned up.

Fixes #586.
Closes #588.
GUS-W-13903181.
edmorley added a commit that referenced this issue Nov 16, 2023
At the end of each test libcnb-test is meant to clean up any Docker
resources that were created during the test. However, this functionality
currently isn't tested, and we've found several cases where cleanup
wasn't occurring as expected:
- #586
- #570
- #737

We could try and add a Rust integration test or two to check that
cleanup works in the common case, however, as seen by the issues
above, it's often in the edge cases where resource cleanup wasn't
working. In addition, testing cleanup during an individual test is hard,
since other tests will be running at the same time, so we'd have to
check for individual image or container names, which would miss
cases like #570.

As such, it's much easier for us to instead just check that there are
zero unexpected resources left behind at the end of the existing entire
libcnb-test test suite.

For now, two of the checks don't cause CI to fail, since the fixes for
the cleanup implementations have not yet landed.

GUS-W-14502524.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libcnb-test
Projects
None yet
1 participant