-
Notifications
You must be signed in to change notification settings - Fork 35
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 Windows and more Linux platforms to the multi-arch image #110
Conversation
@HofmeisterAn I know you have Windows, could you please test @gesellix's WIndows images of Ryuk 🙏 ? |
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.
This LGTM, although I'm adding some comments that would like to discuss.
Let's wait for the final test of the Windows image before approving it.
Thank you for this!
This is awesome!! 😄 ❤️ |
Regarding the suggestions like actions/checkout@v3 -> v4 and using the go-version from go.mod: I'm totally fine with chore-on-the-go, but I'd like to remind ourselves how the previous pull request escalated and drifted away by putting too many aspects into it. I'd prefer to make it work, first. If anything can be changed in another pull request and independently from this one, please do so. I'm totally fine with another rebase on such stuff. Making the multi-arch image itself better, polishing, improving, and all the other useful stuff is certainly possible in a follow up. Please keep this in mind so that we're able to publish a working ryuk container as soon as possible. A working ryuk on WCOW is helpful to start working on the other libraries like testcontainers-java, where WCOW is currently disabled and volume mounts still hard-coded to /var/run/docker.sock. That won't work out of the box, and we should plan accordingly. I think I can add ltsc2022 in this pull request, but I won't make it in the most beautiful way. Please keep this in mind. I would also plan to improve and beautify the workflows in a follow-up, but not now. |
Thanks @gesellix for your work here. You're right. Let's address those tiny refactors (github actions and friends) in a follow-up PR. |
Some of your suggestions have been trivial, so they're now included here :) |
@HofmeisterAn if you want to check both ltsc2019 and ltsc2022, I would publish the test image at |
https://github.com/testcontainers/moby-ryuk/actions/runs/8018411466/job/21904562542?pr=110#step:2:8 The go.mod isn't found 🤔 I'll check later if a path is missing or so. Any hints are appreciated. |
I can take a look at the ltsc2019 version. It looks like this is the current published version, right? Otherwise, I can build it too. |
@HofmeisterAn yes, |
Should be fixed... using the go.mod file requires the source to be checked out, first. Duh! ;) |
I'm working on matrix builds for the Windows variants to reduce the copy & pasted jobs: gesellix#2 |
Do you want to include that here, or in a follow-up? I think this is fine (waiting for @HofmeisterAn confirming the Windows images work as expected) 🤞 |
It depends what happens first ;) I consider the published ryuk image as the interface and it doesn't matter from a user's perspective whether the image has been built with or without matrix. So yes, I would add a follow-up, if this pull request is merged first, but I'm confident that the matrix build can land with this pull request. |
The release workflow has a bug. I'm going to fix it along with the matrix build. |
The release workflow has been fixed (I'm waiting for its success in my own repo). |
A new image is available at https://github.com/gesellix/moby-ryuk/pkgs/container/moby-ryuk/182825956?tag=20240223.4 as |
Regarding the required checks, there is a edit: I've added a |
The image runs fine 👍. docker run -v \\.\pipe\docker_engine:\\.\pipe\docker_engine -e RYUK_PORT=8080 -p 8080:8080 ghcr.io/gesellix/moby-ryuk:20240223.4
2024/02/26 12:28:54 Pinging Docker...
2024/02/26 12:28:54 Docker daemon is available!
2024/02/26 12:28:54 Starting on port 8080...
2024/02/26 12:28:54 Started!
2024/02/26 12:28:55 New client connected: 172.23.128.1:54581
2024/02/26 12:28:55 Adding {"label":{"org.testcontainers.resource-reaper-session=37260511-0c8a-4205-984d-a5fa8b98dcda":true}} With some small changes, the image already works with Testcontainers for .NET. diff --git a/src/Testcontainers/Containers/ResourceReaper.cs b/src/Testcontainers/Containers/ResourceReaper.cs
index d3c9af5..f9d125c 100644
--- a/src/Testcontainers/Containers/ResourceReaper.cs
+++ b/src/Testcontainers/Containers/ResourceReaper.cs
@@ -104,9 +104,19 @@ namespace DotNet.Testcontainers.Containers
[PublicAPI]
public static async Task<ResourceReaper> GetAndStartDefaultAsync(IDockerEndpointAuthenticationConfiguration dockerEndpointAuthConfig, bool isWindowsEngineEnabled = false, CancellationToken ct = default)
{
+ IMount dockerSocket;
+
+ IImage resourceReaperImage;
+
if (isWindowsEngineEnabled)
{
- return null;
+ dockerSocket = new NPipeSocketMount();
+ resourceReaperImage = new DockerImage("ghcr.io/gesellix/moby-ryuk:20240223.4");
+ }
+ else
+ {
+ dockerSocket = new UnixSocketMount(dockerEndpointAuthConfig.Endpoint);
+ resourceReaperImage = TestcontainersSettings.ResourceReaperImage ?? RyukImage;
}
if (_defaultInstance != null && !_defaultInstance._disposed)
@@ -125,11 +135,9 @@ namespace DotNet.Testcontainers.Containers
try
{
- var resourceReaperImage = TestcontainersSettings.ResourceReaperImage ?? RyukImage;
-
var requiresPrivilegedMode = TestcontainersSettings.ResourceReaperPrivilegedModeEnabled;
- _defaultInstance = await GetAndStartNewAsync(DefaultSessionId, dockerEndpointAuthConfig, resourceReaperImage, new UnixSocketMount(dockerEndpointAuthConfig.Endpoint), requiresPrivilegedMode, ct: ct)
+ _defaultInstance = await GetAndStartNewAsync(DefaultSessionId, dockerEndpointAuthConfig, resourceReaperImage, dockerSocket, requiresPrivilegedMode, ct: ct)
.ConfigureAwait(false);
return _defaultInstance;
@@ -447,5 +455,32 @@ namespace DotNet.Testcontainers.Containers
return Task.CompletedTask;
}
}
+
+ private sealed class NPipeSocketMount : IMount
+ {
+ private const string DockerSocketFilePath = "\\\\.\\pipe\\docker_engine";
+
+ public MountType Type
+ => MountType.NamedPipe;
+
+ public AccessMode AccessMode
+ => AccessMode.ReadWrite;
+
+ public string Source
+ => DockerSocketFilePath;
+
+ public string Target
+ => DockerSocketFilePath;
+
+ public Task CreateAsync(CancellationToken ct = default)
+ {
+ return Task.CompletedTask;
+ }
+
+ public Task DeleteAsync(CancellationToken ct = default)
+ {
+ return Task.CompletedTask;
+ }
+ }
}
} |
That's great @HofmeisterAn, thanks for the quick feedback! |
@mdelapenya I think this is ready for another review. I'm looking into some kind of checks to ensure that a published manifest has expected platforms/architectures. A proof of concept is already working locally. I'd prefer to add those checks in a follow-up, combined with the publishing on each build (using Git hashes like you already mentioned). In case I'm quick enough (most probably this week), we might add the checks here. I would need to know where you'd like to publish "preview" artifacts - to the Docker Hub or to the GitHub Container Registry? I guess you'd like to publish the preview artifacts during the "build" workflow, correct? |
We want to probably publish to Docker Hub, and yeah the "preview" artifacts should be published in the build pipeline. Regarding the checks, yes, better in a follow-up 🙏 |
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.
LGTM, thanks for this @gesellix! ANd thanks @HofmeisterAn for the review 🙇
The leftover |
Following improvements are work in progress for a follow-up:
Additional thoughts:
|
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.
LGTM, thank you very much for your work and commitment to the project 👏
Adds Windows and more Linux platforms to the multi-arch image.
This is a kind of minimal approach, optimizations and refactoring may be added later.
Follow-up of #40
Clean version of gesellix#2
Relates to #35
Relates to testcontainers/testcontainers-java#5621
See https://github.com/gesellix/moby-ryuk/pkgs/container/moby-ryuk/182825956?tag=20240223.4 with a multi-arch image available for the proof of concept.
Docker image name:
ghcr.io/gesellix/moby-ryuk:20240223.4