Support for specifying image platform #342

amitsaha · 2021-08-07T10:22:22Z

This pull request adds support for specifying a platform of the image to fetch. Equivalent to docker run --platform ...

mdelapenya · 2021-08-07T16:58:30Z

Hi @amitsaha, thanks for this contribution. It LGTM, although I'd like to see a test if possible.

I'm currently checking if github actions provide support for ARM so that we are able to run tests on ARM.

Could you also check it please?

amitsaha · 2021-08-08T00:57:37Z

Hi @amitsaha, thanks for this contribution. It LGTM, although I'd like to see a test if possible.

I'm currently checking if github actions provide support for ARM so that we are able to run tests on ARM.

Could you also check it please?

Sure, so it looks like github actions can run tests on arm64 as explained here.

Regarding the test, sure i am happy to. I am thinking around the testing strategy though.

My current use case is i specified a x86-64 image for my Macbook M1 - and it works because of the emulation that M1 has.

If we test x86-64 as the image platform, it can only succeed on a x86-64 build host. Similarly for the arm64 image.

Here we really want to ensure that the ImagePlatform is propagated to the code that calls out to the docker API. So perhaps we can test that instead of creating a real container which i am not sure is possible given the above explanation.

amitsaha · 2021-08-08T01:17:12Z

I also realised, this only works for images which are to be pulled. For existing images, the platform option is not accounted for. I will push an update once i have got it working. In fact, that may mean we can test this in a nice way as well.

mdelapenya · 2021-08-09T14:12:33Z

docker.go

 	}

 	var tag string
+	customPlatform := req.ImagePlatform


Super NIT: wdyt about saving this variable and directly use req.ImagePlatform elsewhere? Therefore the code itself will be self-contained: I had to double check that the customPlatform variable was in fact initialised with the requested one.

mdelapenya · 2021-08-12T11:11:56Z

@amitsaha could you take a look at the test failures? 🙏 I think we are close to merge :)

amitsaha · 2021-08-15T04:23:17Z

Hi @mdelapenya thanks. Pushed a fix.

amitsaha · 2021-08-17T02:32:18Z

looking at the test failures:

2021/08/16 22:55:00 Waiting for container id acae21f20ba8 image: rabbitmq:management-alpine
    network_test.go:99: failed to start container: /bin/sh command not executable

Is this something you folks have seen before?

mdelapenya · 2021-08-17T08:51:38Z

No, I haven't that error before in this test suite. Although the error is familiar to me when dealing with multi-platform support: can we ensure that the container uses the same platform (AMD Vs ARM) as the underlying CI machine? 🤷‍♀️

amitsaha · 2021-08-18T02:18:27Z

No, I haven't that error before in this test suite. Although the error is familiar to me when dealing with multi-platform support: can we ensure that the container uses the same platform (AMD Vs ARM) as the underlying CI machine? 🤷‍♀️

I thought that was the default behavior? I will have a look.

amitsaha · 2021-08-18T03:37:40Z

@mdelapenya Fetching upstream master and then rebased fixed the test run. 🤞 let's see how the CI run goes.

mdelapenya

I made a few comments, but other than that, this LGTM. Waiting for last minute feedback to approve it.

Thank you for your work here!!

mdelapenya · 2021-08-18T05:39:45Z

docker_test.go

 }

+func TestContainerCustomPlatformImage(t *testing.T) {
+	t.Run("Use a custom image platform", func(t *testing.T) {


Wdyt about renaming the test case? Because windows/arm12 is not a valid platform, I guess that even if running the tests on a Windows ARM machine, this will fail, right?

Suggested change

t.Run("Use a custom image platform", func(t *testing.T) {

t.Run("Use an invalid custom image platform", func(t *testing.T) {

Hi @mdelapenya this was my not so optimal attempt to test the functionality i am adding. I used windows/arm12 since that's a non-existent image platform so I expect the test to errror out.

wdyt about Use a non-existent custom image platform then?

My intention here is to make the test super explicit to casual readers, specially on failures

mdelapenya · 2021-08-18T05:41:20Z

docker_test.go

+			defer c.Terminate(context.Background())
+		}
+		if err == nil {
+			t.Fatalf("Expected non-nil error")


Because the test expects to raise an error, I would log it with more information:

Suggested change

t.Fatalf("Expected non-nil error")

t.Fatalf("Expected non-nil error when using an invalid platform: 'windows/arm12'")

Maybe extracting the platform to a variable to be reused in this message? wdyt?

codecov · 2021-08-18T05:47:53Z

Codecov Report

Merging #342 (dcd0372) into master (5963181) will increase coverage by 0.57%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
+ Coverage   61.32%   61.90%   +0.57%     
==========================================
  Files          15       15              
  Lines         993     1008      +15     
==========================================
+ Hits          609      624      +15     
- Misses        292      293       +1     
+ Partials       92       91       -1

Impacted Files	Coverage Δ
container.go	`85.29% <ø> (ø)`
docker.go	`64.70% <100.00%> (+1.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5963181...dcd0372. Read the comment docs.

mdelapenya · 2021-08-18T08:10:48Z

docker_test.go

-			t.Fatalf("Expected non-nil error")
+			t.Fatalf("Expected an error containing failed to create container: Error response from daemon")
+		}
+		expectedErrorMsgSubstring := "image with reference redis:latest was found but does not match the specified platform"


Mmm, not sure about this new verification. Will that message change between Docker versions? If you see it convenient, I'm OK with it too, but I'd like to make sure we are not adding flakiness because of specific message formats.

if err == nil { t.Fatalf("Expected non-nil error with a non-existent platform: %s", nonExistentPlatform) }

Sounds like a good idea. done.

docker_test.go

mdelapenya

LGTM! Thanks for your patience while reviewing it!

amitsaha · 2021-08-26T23:54:52Z

Just a gentle reminder on this one - thanks.

mdelapenya · 2021-10-02T06:00:16Z

@gianarb what are your thoughts on this one? Testing in ARM is indeed a thing nowadays. Could you take a look? 🙏

gianarb · 2021-10-22T08:00:50Z

I feel like I would like to know how java handles that! @bsideup any tip for us?! How do you do multi arch in java?
Thanks

bsideup · 2021-10-22T10:52:43Z

Hey @gianarb and @mdelapenya,

Check testcontainers/testcontainers-java#4290

amitsaha · 2021-10-25T21:34:14Z

It seems to me that the Java client by default will aim to pull the image of the same platform as the docker daemon and then fallback to x86 if that fails? Essentially, you are not exposing the image platform spec to the consumer of the library?

Would it possible to get a resolution on this soon please?

mdelapenya · 2021-12-03T08:59:52Z

docker.go

+			customOS = strings.Split(req.ImagePlatform, "/")[0]
+			customArch = strings.Split(req.ImagePlatform, "/")[1]


This is correct, as all platforms are formed by a string in this format: "os/arch"

👍

A platform may also have 3 parts (os/arch/variant) in some cases. It's probably safest to use containerd's platforms.Parse func for this.

hairyhenderson · 2022-01-07T15:27:01Z

Any possibility to get this merged soon? This is necessary in some common scenarios (i.e. running the mysql:5.7 image, which isn't available for arm64 yet)

hairyhenderson · 2022-01-08T17:22:08Z

I've issued #395 to try and help things move along here - I'm easy with either PR being merged (I needed the changes regardless, so I figured I might as well issue a PR if the commit's in my fork anyway!)

mdelapenya · 2022-07-01T05:49:16Z

I think we can close this one, given #395 was merged

Thanks @amitsaha for bringing the seed for this feature!

amitsaha changed the title ~~Support for specifying image platform~~ WIP: Support for specifying image platform Aug 8, 2021

mdelapenya reviewed Aug 9, 2021

View reviewed changes

amitsaha changed the title ~~WIP: Support for specifying image platform~~ Support for specifying image platform Aug 15, 2021

amitsaha added 3 commits August 18, 2021 03:35

Support for specifying image platform

90dbd2d

Fixes

689d84b

Fixes

826e159

amitsaha force-pushed the image_platform branch from 19868c4 to 826e159 Compare August 18, 2021 03:36

mdelapenya reviewed Aug 18, 2021

View reviewed changes

update test

446cdc3

mdelapenya reviewed Aug 18, 2021

View reviewed changes

fix

6dfc01f

mdelapenya reviewed Aug 19, 2021

View reviewed changes

docker_test.go Outdated Show resolved Hide resolved

Remove additional check

dcd0372

mdelapenya reviewed Aug 19, 2021

View reviewed changes

mdelapenya requested a review from gianarb August 19, 2021 10:34

mdelapenya mentioned this pull request Aug 30, 2021

Support arm64 architecture #352

Closed

mdelapenya approved these changes Oct 21, 2021

View reviewed changes

mdelapenya reviewed Dec 3, 2021

View reviewed changes

hairyhenderson mentioned this pull request Jan 8, 2022

Send optional platform spec when creating container #395

Merged

mdelapenya closed this Jul 1, 2022

	t.Run("Use a custom image platform", func(t *testing.T) {
	t.Run("Use an invalid custom image platform", func(t *testing.T) {

	t.Fatalf("Expected non-nil error")
	t.Fatalf("Expected non-nil error when using an invalid platform: 'windows/arm12'")

		customOS = strings.Split(req.ImagePlatform, "/")[0]
		customArch = strings.Split(req.ImagePlatform, "/")[1]

Uh oh!

Support for specifying image platform #342

Support for specifying image platform #342

Uh oh!

Conversation

amitsaha commented Aug 7, 2021

Uh oh!

mdelapenya commented Aug 7, 2021

Uh oh!

amitsaha commented Aug 8, 2021

Uh oh!

amitsaha commented Aug 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdelapenya commented Aug 12, 2021

Uh oh!

amitsaha commented Aug 15, 2021

Uh oh!

amitsaha commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdelapenya commented Aug 17, 2021

Uh oh!

amitsaha commented Aug 18, 2021

Uh oh!

amitsaha commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdelapenya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdelapenya left a comment

Choose a reason for hiding this comment

Uh oh!

amitsaha commented Aug 26, 2021

Uh oh!

mdelapenya commented Oct 2, 2021

Uh oh!

gianarb commented Oct 22, 2021

Uh oh!

bsideup commented Oct 22, 2021

Uh oh!

amitsaha commented Oct 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hairyhenderson commented Jan 7, 2022

Uh oh!

hairyhenderson commented Jan 8, 2022

Uh oh!

mdelapenya commented Jul 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

amitsaha commented Aug 17, 2021 •

edited

Loading

amitsaha commented Aug 18, 2021 •

edited

Loading

codecov bot commented Aug 18, 2021 •

edited

Loading