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

Add support for ryuk resource reaper #413

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

PSanetra
Copy link
Contributor

@PSanetra PSanetra commented Dec 21, 2021

This PR adds support for the ryuk resource reaper.

Features

  • Starts a separate ryuk container and registers a session id label (dotnet.testcontainers.resource-reaper-session)
  • Ryuk deletes containers, networks, images and volumes with this label automatically after 10 seconds when the TCP connection to the test runner got lost or if the ryuk container is explicitly stopped (e.g. by disposing the ResourceReaper instance). The TCP connection is not lost during halting on a break point.
  • A default resource reaper is used on containers, images, volumes and networks if no separate resource reaper session id is specified.
  • Adds support for AutoRemove (equivalent to the --rm flag on docker run) on TestContainers. This is needed to remove the ryuk container itself
  • Adds support for privileged containers (not sure anymore why this is included in this PR. Was working already a while on this...)

Fixes #242

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a 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 think ResourceReaperContainer, ResourceReaperContainerConfiguration and TestcontainersBuilderResourceReaperExtension is not necessary. I attached a refactored class (not finished yet).
  • To access the logger in ResourceReaper, we can use the logger field in TestcontainersContainer 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.

@PSanetra
Copy link
Contributor Author

@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.

@HofmeisterAn
Copy link
Collaborator

@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.

Did you see my pr in your fork?

Unfortunately I am not able to test this under windows.

I can do that after the holidays.

@PSanetra
Copy link
Contributor Author

@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 Allow edits by maintainers option in this PR.

@HofmeisterAn
Copy link
Collaborator

@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" };
=
Copy link
Collaborator

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.

@HofmeisterAn
Copy link
Collaborator

@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 😬.

@PSanetra
Copy link
Contributor Author

For some reason GitHub does not show my response to your comment In my tests your change does not work. E. g. alpine exits. in this conversation overview, so I repost that here:

Weird. I wanted to make the tests faster this way as tail -f /dev/null does not stop on SIGTERM and there is always quite a delay until docker decides to use SIGKILL to stop the process.

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
$

Copy link
Contributor Author

@PSanetra PSanetra left a 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/ResourceReaper.cs Outdated Show resolved Hide resolved
src/DotNet.Testcontainers/Containers/ResourceReaper.cs Outdated Show resolved Hide resolved
src/DotNet.Testcontainers/Containers/ResourceReaper.cs Outdated Show resolved Hide resolved
src/DotNet.Testcontainers/Containers/ResourceReaper.cs Outdated Show resolved Hide resolved
src/DotNet.Testcontainers/Containers/ResourceReaper.cs Outdated Show resolved Hide resolved
@HofmeisterAn
Copy link
Collaborator

Is the following experiment reproducible on your machine with alpine?

Yes, you're right 👍 it works. I'm sorry for that. Probably I messed something else up. I'll add it again.

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a 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";
Copy link
Collaborator

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.

src/DotNet.Testcontainers/Containers/ResourceReaper.cs Outdated Show resolved Hide resolved
@PSanetra
Copy link
Contributor Author

@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.

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a 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.

@PSanetra
Copy link
Contributor Author

PSanetra commented Feb 1, 2022

@HofmeisterAn Yes, sounds good 👍
It would be nice to wait for a new Ryuk release and replace ghcr.io/psanetra/ryuk:2021.12.20 before a new dotnet-testcontainers release.

@PSanetra
Copy link
Contributor Author

PSanetra commented Feb 1, 2022

@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.

@HofmeisterAn
Copy link
Collaborator

@HofmeisterAn Yes, sounds good 👍
It would be nice to wait for a new Ryuk release and replace ghcr.io/psanetra/ryuk:2021.12.20 before a new dotnet-testcontainers release.

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.).

@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.

👍 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.}
@sonarcloud
Copy link

sonarcloud bot commented Feb 3, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 8 Code Smells

89.3% 89.3% Coverage
0.0% 0.0% Duplication

@HofmeisterAn HofmeisterAn merged commit 0d037ca into testcontainers:develop Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Force Clean Containers on build before start
2 participants