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.

Sign up for GitHub

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

Jump to bottom

Stop container that doesn't match wait strategy #9474

Merged
merged 2 commits into from
Nov 6, 2024
Merged

Stop container that doesn't match wait strategy #9474

merged 2 commits into from
Nov 6, 2024

Conversation

ssheikin
Copy link
Contributor

@ssheikin ssheikin commented Oct 28, 2024 •
edited
Loading 

new GenericContainer<>(IMAGE_NAME)
    .withStartupAttempts(3)
    .waitingFor(forLogMessage("this text does not exist in logs", 1)
        .withStartupTimeout(Duration.ofSeconds(5)))

Leaves not closed containers in case of timeout and retry.

For more extensive description see #2877 which this PR fixes.

reportLeakedContainers adapted from
trinodb/trino#20297
trinodb/trino#21280

@ssheikin ssheikin requested a review from a team as a code owner October 28, 2024 23:31
@ssheikin ssheikin force-pushed the ssheikin/Cleanup-resources-of-container-failed-to-start branch from bad07d1 to 1b7041c Compare October 28, 2024 23:33
@ssheikin ssheikin changed the title Cleanup resources of container failed to start Prevent ghost containers after retries Oct 28, 2024
@eddumelendez
Copy link
Member

Can you please elaborate about the change?

@ssheikin ssheikin force-pushed the ssheikin/Cleanup-resources-of-container-failed-to-start branch from 1b7041c to 3afa556 Compare October 29, 2024 15:43
@ssheikin
Copy link
Contributor Author

cc @nineinchnick

@nineinchnick
Copy link
Contributor

Would this solve #2877 ?

@ssheikin ssheikin force-pushed the ssheikin/Cleanup-resources-of-container-failed-to-start branch 3 times, most recently from d48eaf0 to 8210e08 Compare October 30, 2024 13:17
@ssheikin
Copy link
Contributor Author

@nineinchnick thanks for pointing out to the existing issue.
I've added tests and updated the PR description
@eddumelendez please review

@ssheikin

This comment was marked as off-topic.

Sign in to view
@ssheikin ssheikin force-pushed the ssheikin/Cleanup-resources-of-container-failed-to-start branch from 8210e08 to 630c150 Compare October 31, 2024 21:42
@ssheikin

This comment was marked as off-topic.

Sign in to view
@eddumelendez
Copy link
Member

Hi @ssheikin, with all due respect, please stop tagging people in this PR. It is only creating noise in emails, slack and notifications to members who are taking a break from the project. As I mentioned via DM, when a PR is raised the team is notified. Please also be aware that there are other PRs, tasks that we are currently working on.

We really appreciate your contribution and it is going to be reviewed but give some time meanwhile it is triaged and discussed.

@ssheikin @findepi @nineinchnick
Prevent ghost containers after retries
902cb03 
reportLeakedContainers adapted from
trinodb/trino#20297
trinodb/trino#21280

Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com>
Co-authored-by: Jan Waś <jan.was@starburstdata.com>
@ssheikin ssheikin force-pushed the ssheikin/Cleanup-resources-of-container-failed-to-start branch from 630c150 to 902cb03 Compare November 5, 2024 23:38
@eddumelendez eddumelendez added this to the next milestone Nov 6, 2024
@eddumelendez eddumelendez changed the title Prevent ghost containers after retries Remove containers that doesn't match wait strategy when startup attempts is defined Nov 6, 2024
@eddumelendez eddumelendez changed the title Remove containers that doesn't match wait strategy when startup attempts is defined Stop container that doesn't match wait strategy Nov 6, 2024
@eddumelendez eddumelendez merged commit 10f32af into testcontainers:main Nov 6, 2024
107 checks passed
@eddumelendez
Copy link
Member

Thanks for your contribution, @ssheikin.

@ssheikin ssheikin deleted the ssheikin/Cleanup-resources-of-container-failed-to-start branch November 6, 2024 06:20
@ssheikin
Copy link
Contributor Author

ssheikin commented Nov 7, 2024

Thanks for getting this across the finish line!
Could you please let us know when the next release is scheduled?
We’re excited to use withStartupAttempts for containers that occasionally fail to start.

@kokosing kokosing mentioned this pull request Nov 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
type/bug
Projects
None yet
Milestone
1.20.4
Development

Successfully merging this pull request may close these issues.
3 participants
@ssheikin @eddumelendez @nineinchnick