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

Prevent Ryuk from removing itself #53

Merged

Conversation

matthewmcnew
Copy link
Contributor

@bsideup
Copy link
Member

bsideup commented Jan 3, 2023

Actually, Ryuk shouldn't be labeled with the same label that it targets, that's a bug in Ryuk's consumer.

Here is the relevant piece in Testcontainers for Java:
https://github.com/testcontainers/testcontainers-java/blob/fa56f24dc77da80c3a77bb14f096c955c93e9e14/core/src/main/java/org/testcontainers/utility/RyukResourceReaper.java#L62-L70

@matthewmcnew
Copy link
Contributor Author

@bsideup Yes, It is a bug in testcontainers-go. In the meantime this PR to ensure that ryuk handles this incorrect consumer behavior as suggested by @HofmeisterAn.

 - Workaround for testcontainers/testcontainers-go#698
 - Add generic label to the ryuk image `org.testcontainers.ryuk` which enables the ryuk to detect itself and prevent deletions.
@HofmeisterAn
Copy link
Contributor

Yup, I agree it is in the first place an issue in Ryuk's consumer, but it sounds reasonable to prevent others from running into the same issue. My idea was to start a discussion in moby-ryuk first while we are fixing the Go implementation.

main.go Outdated Show resolved Hide resolved
@mdelapenya mdelapenya self-assigned this Mar 2, 2023
@mdelapenya mdelapenya added the bug An issue with the project label Mar 2, 2023
@mdelapenya mdelapenya merged commit 5fe8bc7 into testcontainers:main Mar 2, 2023
@mdelapenya
Copy link
Contributor

@matthewmcnew thanks for the issue report and your work here. Much appreciated!

mdelapenya added a commit to hhsnopek/moby-ryuk that referenced this pull request Mar 2, 2023
* main:
  Prevent Ryuk from removing itself (testcontainers#53)
  Bump github.com/stretchr/testify from 1.8.1 to 1.8.2 (testcontainers#61)
  chore: add dependabot updates (testcontainers#59)
  chore: sync governance files (testcontainers#58)
  chore: bump testcontainers-go to v0.18.0 (testcontainers#57)
  Bump github.com/containerd/containerd from 1.6.8 to 1.6.18 (testcontainers#55)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants