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 Windows and more Linux platforms to the multi-arch image #110

Merged
merged 8 commits into from
Mar 1, 2024

Conversation

gesellix
Copy link
Contributor

@gesellix gesellix commented Feb 23, 2024

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

@gesellix gesellix requested a review from a team as a code owner February 23, 2024 06:56
This was referenced Feb 23, 2024
@mdelapenya
Copy link
Member

@HofmeisterAn I know you have Windows, could you please test @gesellix's WIndows images of Ryuk 🙏 ?

Copy link
Member

@mdelapenya mdelapenya left a 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!

.github/workflows/build-docker-image.yml Outdated Show resolved Hide resolved
.github/workflows/build-docker-image.yml Outdated Show resolved Hide resolved
.github/workflows/build-docker-image.yml Outdated Show resolved Hide resolved
.github/workflows/build-docker-image.yml Outdated Show resolved Hide resolved
.github/workflows/build-docker-image.yml Outdated Show resolved Hide resolved
.github/workflows/build-docker-image.yml Outdated Show resolved Hide resolved
.github/workflows/publish-docker-image.yml Outdated Show resolved Hide resolved
.github/workflows/build-docker-image.yml Outdated Show resolved Hide resolved
.github/workflows/build-docker-image.yml Outdated Show resolved Hide resolved
.github/workflows/publish-docker-image.yml Outdated Show resolved Hide resolved
.github/workflows/publish-docker-image.yml Outdated Show resolved Hide resolved
.github/workflows/build-docker-image.yml Outdated Show resolved Hide resolved
@narcis-ro
Copy link

This is awesome!! 😄 ❤️

@gesellix
Copy link
Contributor Author

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.

@mdelapenya
Copy link
Member

Thanks @gesellix for your work here. You're right. Let's address those tiny refactors (github actions and friends) in a follow-up PR.

@gesellix
Copy link
Contributor Author

Some of your suggestions have been trivial, so they're now included here :)

@gesellix
Copy link
Contributor Author

@HofmeisterAn I know you have Windows, could you please test @gesellix's WIndows images of Ryuk 🙏 ?

@HofmeisterAn if you want to check both ltsc2019 and ltsc2022, I would publish the test image at ghcr.io/gesellix/moby-ryuk:.... Please ping me, if you need it.

@gesellix
Copy link
Contributor Author

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.

@HofmeisterAn
Copy link
Contributor

@HofmeisterAn I know you have Windows, could you please test @gesellix's WIndows images of Ryuk 🙏 ?

@HofmeisterAn if you want to check both ltsc2019 and ltsc2022, I would publish the test image at ghcr.io/gesellix/moby-ryuk:.... Please ping me, if you need it.

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.

@gesellix
Copy link
Contributor Author

@HofmeisterAn yes, ltsc2019 ist the currently published version. I'll probably publish a new version including both ltsc2019 and ltsc2022 in the next days.

@gesellix
Copy link
Contributor Author

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.

Should be fixed... using the go.mod file requires the source to be checked out, first. Duh! ;)

@gesellix
Copy link
Contributor Author

I'm working on matrix builds for the Windows variants to reduce the copy & pasted jobs: gesellix#2

@mdelapenya
Copy link
Member

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

@gesellix
Copy link
Contributor Author

Do you want to include that here, or in a follow-up?

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.

@gesellix
Copy link
Contributor Author

The release workflow has a bug. I'm going to fix it along with the matrix build.

@gesellix
Copy link
Contributor Author

The release workflow has been fixed (I'm waiting for its success in my own repo).
Matrix builds are included, too. Only the last step where the manifest is merged is still not perfect and doesn't use the matrix.

@gesellix
Copy link
Contributor Author

A new image is available at https://github.com/gesellix/moby-ryuk/pkgs/container/moby-ryuk/182825956?tag=20240223.4 as ghcr.io/gesellix/moby-ryuk:20240223.4 with Windows os versions ltsc2019 and ltsc2022 (10.0.17763.5458, 10.0.20348.2322).

@gesellix
Copy link
Contributor Author

gesellix commented Feb 23, 2024

Regarding the required checks, there is a build expected to run. I've renamed the job, so we should decide if a build job with a needs: [] property should be introduced or if the branch protection should be changed to require the new job names.

edit: I've added a build job to join the other jobs in the build workflow.

@HofmeisterAn
Copy link
Contributor

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

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;
+      }
+    }
   }
 }

@gesellix
Copy link
Contributor Author

That's great @HofmeisterAn, thanks for the quick feedback!

@gesellix
Copy link
Contributor Author

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

@mdelapenya
Copy link
Member

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 🙏

@mdelapenya mdelapenya self-assigned this Feb 27, 2024
@mdelapenya mdelapenya added the enhancement New feature or request label Feb 27, 2024
mdelapenya
mdelapenya previously approved these changes Feb 28, 2024
Copy link
Member

@mdelapenya mdelapenya left a 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 🙇

@gesellix
Copy link
Contributor Author

The leftover inputs.tag_name is fixed, ready to give it another run and another (final? 😉) review.

@gesellix
Copy link
Contributor Author

Following improvements are work in progress for a follow-up:

  • publish during build (not only at release)
  • check to ensure the multi-arch image to contain all archs/versions

Additional thoughts:

  • consolidating the build, and release workflows by extracting shared actions

@gesellix
Copy link
Contributor Author

gesellix commented Feb 29, 2024

A follow-up is prepared at #111 (it contains the commits of this pr #110, so I"m going to rebase as soon as this one is merged).

Copy link
Member

@mdelapenya mdelapenya left a 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 👏

@mdelapenya mdelapenya merged commit 19e6e7b into testcontainers:main Mar 1, 2024
7 checks passed
@gesellix gesellix deleted the wcow2024 branch March 1, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants