fix(PodCreate): ensure pod cleanup on failure with outputFiles #210
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When checkContainerFailures throws an exception after detecting a failed container, the delete() call was being skipped, leaving pods running with only the sidecar container active. Wrapped the completion check and output handling in a try-finally block to ensure delete() is always called, even when the task fails. Added test failedWithOutputFilesDeletesPod() to verify pods are properly deleted despite container failures. Fixes pod cleanup issue reported by Maxime Yvart in PR kestra-io#208. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ests Refactored the handleEnd() method to better separate concerns between pods with and without outputFiles, and enhanced error logging. Changes: - Move failure checking logic into handleEnd() with hasOutputFiles parameter - Add logger parameter to PodService.checkContainerFailures() for better error visibility - Simplify main flow by removing duplicate error logging code Test Improvements: - Add log collection verification to failedWithOutputFiles test - Add pod deletion verification to failure tests (ensures cleanup works) - Add successWithOutputFiles test for success case coverage - Add multipleContainersOneFailsWithOutputFiles test (multi-container scenario) - Add logCollectionTimingWithOutputFiles test (verifies waitForLogInterval behavior) All failure tests now verify both: 1. Logs are collected before cleanup 2. Pods are properly deleted after failure (using Await.until) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Improvements to test coverage: - Add deletion verification to failed() test to ensure pod cleanup works in the non-outputFiles failure path (complements existing failedWithOutputFilesDeletesPod test) - Fix multipleContainersOneFailsWithOutputFiles() by increasing waitForLogInterval from 5s to 30s to reliably capture container logs before pod deletion (matches pattern used in other log verification tests) Both failure paths now have deletion verification: - Without outputFiles: failed() test - With outputFiles: failedWithOutputFilesDeletesPod() test 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@fdelbrayelle if this is accepted, the same may be done inside the Kubernetes task runner |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
After PR #208 fixed container exit code validation, a new issue emerged: when container failures are detected and an exception is thrown, pod deletion was skipped, leaving pods running indefinitely despite
delete: trueconfiguration.This is a resource leak issue - failed pods accumulate in the cluster and are never cleaned up, even though the task correctly reports the failure.
Solution
Main fix:
try-finallyblock to ensure pod deletion always runs, even when exceptions occurCode improvements:
handleEnd()to accepthasOutputFilesparameter for clearer separation of concernshandleEnd()to centralize log collection and failure validationPodService.checkContainerFailures()to ensure errors are logged before exceptions are thrownTesting
Enhanced and added comprehensive test cases:
Enhanced existing test:
failedWithOutputFiles()- Now verifies logs are collected AND pods are deletedNew test cases:
failedWithOutputFilesDeletesPod()- Explicitly tests pod deletion with async monitoringsuccessWithOutputFiles()- Verifies success path with outputFilesmultipleContainersOneFailsWithOutputFiles()- Tests multi-container failure detection with log and deletion verificationlogCollectionTimingWithOutputFiles()- Verifies waitForLogInterval is respected before failureAll failure tests now verify:
Note
Test execution time: ~8-9 minutes for full PodCreateTest suite. Duration slightly increased due to additional deletion verification and reliable log collection timing.