-
-
Notifications
You must be signed in to change notification settings - Fork 296
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
Add support for ryuk resource reaper #413
Conversation
0cfdb4c
to
1fb9cfc
Compare
...DotNet.Testcontainers/Containers/Configurations/Misc/ResourceReaperContainerConfiguration.cs
Outdated
Show resolved
Hide resolved
1fb9cfc
to
7c18985
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work! I'm looking forward to merge this feature, this will help a lot.
- I suggest to change some parts of the
ResourceReaper
implementation. I thinkResourceReaperContainer
,ResourceReaperContainerConfiguration
andTestcontainersBuilderResourceReaperExtension
is not necessary. I attached a refactored class (not finished yet). - To access the logger in
ResourceReaper
, we can use the logger field inTestcontainersContainer
and add a getter. - Log messages are now defined in
Logging.cs
. - We need to keep an eye on Windows containers too (I think Windows containers and WSL will work, but Hyper-V not).
The PR is quite big to review. It would help a lot if you focus on the bare minimum and do not check-in unnessary changes like namespaces, etc.
public sealed class ResourceReaper : IAsyncDisposable
{
private const string RyukImage = "ghcr.io/psanetra/ryuk:2021.12.20";
private const int RyukPort = 3306;
private static readonly ResourceReaper DefaultResourceReaper = new ResourceReaper();
private readonly ITestcontainersContainer resourceReaperContainer;
private ResourceReaper()
: this(Guid.NewGuid())
{
}
private ResourceReaper(Guid sessionId)
{
this.resourceReaperContainer = new TestcontainersBuilder<ITestcontainersContainer>()
.WithName($"testcontainers-ryuk-{sessionId:D}")
.WithImage(RyukImage)
.WithAutoRemove(true)
.WithCleanUp(false)
.WithPortBinding(0, RyukPort)
.WithBindMount("/var/run/docker.sock", "/var/run/docker.sock", AccessMode.ReadOnly)
.WithWaitStrategy(Wait.ForUnixContainer().UntilPortIsAvailable(RyukPort))
.Build();
this.SessionId = sessionId;
}
/// <summary>
/// Gets the resource reaper session id.
/// </summary>
public Guid SessionId { get; }
/// <summary>
///
/// </summary>
/// <param name="ct">Cancellation token.</param>
/// <returns>Task that completes when the resource reaper has been started.</returns>
public static async Task<ResourceReaper> GetAndStartDefaultAsync(CancellationToken ct = default)
{
// TODO: Return `DefaultResourceReaper` if resource reaper is up and running.
await DefaultResourceReaper.StartAsync(ct)
.ConfigureAwait(false);
return DefaultResourceReaper;
}
/// <summary>
///
/// </summary>
/// <param name="ct">Cancellation token.</param>
/// <returns>Task that completes when the resource reaper has been started.</returns>
public static async Task<ResourceReaper> GetAndStartNewAsync(CancellationToken ct = default)
{
var resourceReaper = new ResourceReaper();
await resourceReaper.StartAsync(ct)
.ConfigureAwait(false);
return resourceReaper;
}
/// <inheritdoc />
public ValueTask DisposeAsync()
{
return this.resourceReaperContainer.DisposeAsync();
}
private Task StartAsync(CancellationToken ct = default)
{
return this.resourceReaperContainer.StartAsync(ct);
}
private Task StopAsync(CancellationToken ct = default)
{
return this.resourceReaperContainer.StopAsync(ct);
}
}
Thanks again.
6175275
to
fd968bb
Compare
@HofmeisterAn I have applied your suggestions and also did some additional improvements. I have added those changes in separate commits. Maybe this is easier to review and can be squashed before merging the PR. Unfortunately I am not able to test this under windows. |
Did you see my pr in your fork?
I can do that after the holidays. |
@HofmeisterAn Sorry I was not watching the fork and did not receive a notification. I have merged your branch into this PR and resolved the conflicts. I think you should be able to push directly to the branch in my fork as I have enabled the |
@PSanetra I'll start this week with the Windows tests, etc. |
@@ -3,6 +3,14 @@ namespace DotNet.Testcontainers.Tests.Fixtures | |||
public static class KeepTestcontainersUpAndRunning | |||
{ | |||
public static string[] Command { get; } | |||
= { "/bin/sh", "-c", "tail -f /dev/null" }; | |||
= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my tests your change does not work. E. g. alpine exits.
@PSanetra somehow I can not push on your branch. I created another pull request in your fork. It would be nice if you review and merge it again. After that I'll take a look at the Windows containers. Maybe we need to add some minor fixes too, I did not run all unit tests 😬. |
For some reason GitHub does not show my response to your comment Weird. I wanted to make the tests faster this way as Is the following experiment reproducible on your machine with alpine? $ docker run -d --rm --name test --entrypoint=/bin/sh alpine:3.15.0 '-c' 'trap "exit" TERM; while true; do sleep 1; done;'
59a9e9f2cf193e224a6c45b189fe181eaaacce6815709531dbc878b850fb3602
$ docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
59a9e9f2cf19 alpine:3.15.0 "/bin/sh -c 'trap \"e…" 6 seconds ago Up 5 seconds test
$ docker stop test
test
$ docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a few comments to your changes and todos. I don't know why pushing to my PR branch does not work. I have sent you an invitation as collaborator to my fork. I think as collaborator you should be able to push to any branch.
src/DotNet.Testcontainers/Containers/TestcontainersContainer.cs
Outdated
Show resolved
Hide resolved
Yes, you're right 👍 it works. I'm sorry for that. Probably I messed something else up. I'll add it again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A section in the README.md that explains the resource reaper / ryuk would be nice.
{ | ||
public const string ResourceReaperSessionLabel = TestcontainersClient.TestcontainersLabel + ".resource-reaper-session"; | ||
|
||
private const string RyukImage = "ghcr.io/psanetra/ryuk:2021.12.20"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update image to the official version.
@HofmeisterAn There were still some issues with the MaintainRyukConnection method and the way how the ResourceReaper was started, initialized and disposed. I have fixed those issues and added some tests for those cases. |
tests/DotNet.Testcontainers.Tests/Unit/Containers/Unix/ResourceReaperTest.cs
Outdated
Show resolved
Hide resolved
40fbfe7
to
174c725
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we are ready to go. What do you think? There are still a few small things to do, but nothing important. I suggest creating separate prs for them.
Squash the commits to a single one, add a proper commit message and merge (I rebased already on develop
, thats why I force pushed)?
Thanks again for your help.
@HofmeisterAn Yes, sounds good 👍 |
@HofmeisterAn I have created testcontainers/moby-ryuk#35 to see if it was possible to contribute Windows container support to the official Ryuk project or if it was needed to create a fork for that purpose. |
We don't need to merge the changes straight to master. We can update the image later, if the official release takes some more time. In the meantime, I can take a look at the minor improvements (SonarQube, etc.).
👍 Thanks. As far as I know Windows does not support to mount the Docker daemon like Linux. But maybe it will work with WSL or a socket. Perhaps I can do some tests end of this or next week. |
… function: ResourceReaper' {Add ResourceReaper.}
eea26a7
to
0d037ca
Compare
Kudos, SonarCloud Quality Gate passed! |
This PR adds support for the ryuk resource reaper.
Features
dotnet.testcontainers.resource-reaper-session
)ResourceReaper
instance). The TCP connection is not lost during halting on a break point.--rm
flag ondocker run
) on TestContainers. This is needed to remove the ryuk container itselfFixes #242