Repository navigation
fix(vdev): ensure integration test containers despawn - #26838
Captain1653 wants to merge 3 commits into
Conversation
| pub fn new(test: &'a ComposeTest) -> Self { | ||
| let auto_despawn = std::env::var("AUTODESPAWN") | ||
| .or_else(|_| std::env::var("AUTOSPAWN")) | ||
| .map_or(true, |v| v.eq_ignore_ascii_case("true")); |
There was a problem hiding this comment.
Kept the env parsing straightforward. If the team prefers stricter parsing (e.g., explicitly matching "1" / "0", or treating invalid strings as errors), I'm happy to adjust it to project conventions.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadoghq-integration[bot]](https://avatars.githubusercontent.com/u/16866914?s=60&v=4){kind=small}
There was a problem hiding this comment.
The cleanup guard removes caller-owned environments, breaking repeated local testing and deleting containers before CI collects failure diagnostics. Restricting the guard to environments started by the test invocation preserves both workflows.
🤖 Bits Code Review · Commit 2aa43a9
Captain1653
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a50168aa6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 497e985167
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| impl Drop for DespawnGuard<'_> { | ||
| fn drop(&mut self) { | ||
| if self.should_despawn { | ||
| let _ = self.test.stop(); |
There was a problem hiding this comment.
When a failing integration loses all of its compose services, is_running() returns false because it queries only --status running; this Drop path then calls ComposeTest::stop(), which bails before both compose.stop() and runner.remove(), and the error is discarded here. Consequently, service-crash failures still leave the exited compose project and the live test-runner container behind, so the guard's cleanup path needs to handle an existing stopped project rather than requiring a running service.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch regarding crashed services and is_running(), but that is pre-existing behavior of ComposeTest::stop(). Modifying how stop() handles non-running environments affects other workflows and is outside the scope of this PR.
Let's wait for the core team's review.
Summary
Droptrait andComposeTest.stop(), reliably tearing down test containers even on test failures or panics.AUTODESPAWNandAUTOSPAWNenvironment variables (defaults totrue). This implementation readswas_runningflag and honors it.test-integration-%-cleanupintact for debugging workflows when autodespawn is disabled.Although the original issue was reported specifically for AWS integration tests, this change fixes the cleanup behavior at the core runner level (
ComposeTest). Consequently, all integration test suites now benefit from reliable automatic teardown on failure.References
Closes: #13017
Vector configuration
Not Applicable
How did you test this PR?
Reproduction on
master:make test-integration-amqp.rabbitmq, test runner) remained running indocker ps.Verification on this branch:
docker ps- no containers.Implementation Notes & Considerations
Bumped
vdevpatch version: Bumped the patch version ofvdevdirectly in this PR, as this change fixes a defect in the integration test orchestration workflow.AUTOSPAWNvariable usage: Noticed thatAUTOSPAWNwas defined in theMakefile, but never actually referenced anywhere else in the codebase (only in MD-files) :Is this expected legacy behavior or an intentional flag meant to be supported alongside
AUTODESPAWN?Direct call to domain ComposeTest::stop(): The RAII guard invokes
self.test.stop()directly rather than re-exporting or routing through the CLI command handler (commands::compose_tests::stop). Calling the high-level CLI command would require changing visibility for internal CLI handlers, whereasComposeTest::stop()cleanly handles container teardown at the domain level.Preserved AUTODESPAWN in Makefile: Left the environment variable definitions in the Makefile intact to ensure zero impact on existing CI workflows, scripts, or external developer habits.
Retained manual cleanup Makefile target: Kept the explicit clean
test-integration-%-cleanuptarget in the Makefile. If a developer explicitly runs withAUTODESPAWN=falseto inspect containers after a failure, they still need a convenient way to tear down the environment afterwards.Does this PR include user facing changes?
no-changeloglabel to this PR.Contributor Guidelines
@vectordotdev/vectorto reach out to us regarding this PR.git merge origin masterandgit push.