dispatcher: Parse `--platform` via `FROM --platform=` and assign to field in stage's Builder #212

flouthoc · 2022-01-31T11:30:21Z

Allows imagebuilder to read --platform if specified in FROM and
relay it to implementations using stage.Builder.Platform so implementation can
actually use platform while pulling the image.

Example Demonstration/Implementation here: containers/buildah#3757
Reference: https://docs.docker.com/engine/reference/builder/#from

flouthoc · 2022-01-31T11:36:53Z

@nalind PTAL. This is the approach which i am thinking about. Following approach delegates whole Platform string to implementation since they are responsible for deciding about how are they going to use it.

Also platform string is unqiue for stages so each stage can have its own platform example

FROM --platform=linux/amd64 alpine
RUN uname -m

FROM --platform=linux/arm64 alpine
RUN uname -m

TomSweeneyRedHat · 2022-01-31T12:23:36Z

@flouthoc, does Docker allow for different platforms per FROM statement? The documentation you provided wasn't clear about that.

TomSweeneyRedHat · 2022-01-31T12:33:08Z

Would be good to have some tests added. I think, but I'm not sure that this handles an "AS" clause. Have you confirmed?

flouthoc · 2022-01-31T12:44:16Z

@flouthoc, does Docker allow for different platforms per FROM statement? The documentation you provided wasn't clear about that.

@TomSweeneyRedHat Yes I tried it last time with buildkit it supports that but only makes lot of sense when user has a qemu-user-static and bindfmt_misc setup otherwise it would not make lot of sense to user.

But irrespective of the even legacy docker build supports --platform with FROM statements.

Would be good to have some tests added. I think, but I'm not sure that this handles an "AS" clause. Have you confirmed?

Yes AS clause is still handled it is exclusive of --platform and part of generic FROM. I'll add a test and implementation to docker client once everybody agrees on this approach.

nalind · 2022-01-31T14:31:15Z

How complete is this intended to be? This needs to update the various TARGET... built-in args introduced in #142 and sort out how we want to deal with "ARG TARGETPLATFORM ...", which overlaps with it. The dockerclient package is using a version of go-dockerclient that can accept a platform value when it's pulling an image, so this PR may want to wire that up as well.

flouthoc · 2022-01-31T14:45:03Z

@nalind hmm i'm still thinking if we should override the variables I'll check with buildkit once if they are doing it then we can do the same ? But go-dockerclient for sure I'll wire it up. Just wanted to make sure if approach seemed right.

flouthoc · 2022-01-31T16:09:19Z

@nalind I think --platform does not changes or alters any of the TARGETPLATFORM or BUILDPLATFORM. I tried this using buildkit

FROM --platform=linux/arm64 alpine
ARG TARGETPLATFORM
ARG BUILDPLATFORM
RUN echo "I am running on $BUILDPLATFORM, building for $TARGETPLATFORM"

still shows I am running on linux/amd64, building for linux/amd64 even though my --platform was linux/arm64 but actual image pulled was for linux/arm64.

Command sudo docker buildx build -t test .

--platform can use TARGETPLATFORM or BUILDPLATFORM but cannot alter them.

nalind · 2022-01-31T16:53:59Z

@nalind I think --platform does not changes or alters any of the TARGETPLATFORM or BUILDPLATFORM. I tried this using buildkit
FROM --platform=linux/arm64 alpine
ARG TARGETPLATFORM
ARG BUILDPLATFORM
RUN echo "I am running on $BUILDPLATFORM, building for $TARGETPLATFORM"
still shows I am running on linux/amd64, building for linux/amd64 even though my --platform was linux/arm64 but actual image pulled was for linux/arm64.

Command sudo docker buildx build -t test .

--platform can use TARGETPLATFORM or BUILDPLATFORM but cannot alter them.

Apparently docker build doesn't set the TARGETPLATFORM args based on FROM --platform, but does set it to match a --platform flag passed on the command line, and there's no correspondence enforced between the two. The output image is marked as being for TARGETPLATFORM. That suggests that TARGETPLATFORM is more about supporting cross-compiling, and FROM --platform isn't really tied to it at all. So I guess we don't have to worry about setting TARGETPLATFORM based on FROM after all.

flouthoc · 2022-01-31T17:20:20Z

@nalind I think --platform does not changes or alters any of the TARGETPLATFORM or BUILDPLATFORM. I tried this using buildkit
FROM --platform=linux/arm64 alpine
ARG TARGETPLATFORM
ARG BUILDPLATFORM
RUN echo "I am running on $BUILDPLATFORM, building for $TARGETPLATFORM"
still shows I am running on linux/amd64, building for linux/amd64 even though my --platform was linux/arm64 but actual image pulled was for linux/arm64.
Command sudo docker buildx build -t test .
--platform can use TARGETPLATFORM or BUILDPLATFORM but cannot alter them.
Apparently docker build doesn't set the TARGETPLATFORM args based on FROM --platform, but does set it to match a --platform flag passed on the command line, and there's no correspondence enforced between the two. The output image is marked as being for TARGETPLATFORM. That suggests that TARGETPLATFORM is more about supporting cross-compiling, and FROM --platform isn't really tied to it at all. So I guess we don't have to worry about setting TARGETPLATFORM based on FROM after all.

@nalind Cool then i guess just instrumenting docker-client is enough right, i'll also add a test.

dockerclient/client.go

flouthoc · 2022-02-01T10:02:38Z

/assign @nalind

nalind

A couple of formatting nits, otherwise LGTM.

nalind · 2022-02-02T14:16:18Z

builder.go


 	Warnings []string
+	// Raw platform string specified with `FROM --platform` of the stage
+	// Its upto the implementation or client to parse and use this field


Nit: typo "upto"

nalind · 2022-02-02T14:16:51Z

dispatchers_test.go

+	if mybuilder.Platform != expectedPlatform {
+		t.Errorf("Expected %v, to match %v\n", expectedPlatform, mybuilder.Platform)
+	}
+


Nit: extra blank line

…builder Allows imagebuilder to read and `--platform` if specified in `FROM` and relay it to implementations using `stage.Builder.Platform` Signed-off-by: Aditya R <[email protected]>

Add support to dockerclient to actually use platform while pulling image if specified in `FROM --platform=<>` before this commit platform was `no-op` for image pulls. Signed-off-by: Aditya R <[email protected]>

openshift-ci · 2022-02-02T14:46:44Z

@flouthoc: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

flouthoc · 2022-02-02T15:24:12Z

@rhatdan @nalind Fixed the nits. Does it look better now.

rhatdan · 2022-02-02T15:26:05Z

/approve
/lgtm

openshift-ci · 2022-02-02T15:26:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from rhatdan and smarterclayton January 31, 2022 11:30

flouthoc mentioned this pull request Jan 31, 2022

stage_executor: Add support for inline FROM --platform= within Containerfile/Dockerfile containers/buildah#3757

Merged

flouthoc force-pushed the from-platform branch from d498d9b to 92bd705 Compare February 1, 2022 09:11

flouthoc commented Feb 1, 2022

View reviewed changes

dockerclient/client.go Outdated Show resolved Hide resolved

flouthoc commented Feb 1, 2022

View reviewed changes

dockerclient/client.go Outdated Show resolved Hide resolved

openshift-ci bot assigned nalind Feb 1, 2022

flouthoc force-pushed the from-platform branch from 92bd705 to 2159e4b Compare February 2, 2022 06:16

flouthoc requested a review from nalind February 2, 2022 06:18

nalind reviewed Feb 2, 2022

View reviewed changes

flouthoc added 2 commits February 2, 2022 20:14

dispatcher: Parse --platform via FROM and assign to field in stage's …

264f8c7

…builder Allows imagebuilder to read and `--platform` if specified in `FROM` and relay it to implementations using `stage.Builder.Platform` Signed-off-by: Aditya R <[email protected]>

dockerclient: Add support for platform when pulling image

d77c933

Add support to dockerclient to actually use platform while pulling image if specified in `FROM --platform=<>` before this commit platform was `no-op` for image pulls. Signed-off-by: Aditya R <[email protected]>

flouthoc force-pushed the from-platform branch from 2159e4b to d77c933 Compare February 2, 2022 14:45

flouthoc requested a review from nalind February 2, 2022 14:45

openshift-ci bot assigned rhatdan Feb 2, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2022

openshift-merge-robot merged commit 28a05a4 into openshift:master Feb 2, 2022

flouthoc mentioned this pull request Apr 8, 2022

dispatcher: honor default builtin-args while processing FROM --platform= #224

Merged

dispatcher: Parse --platform via FROM --platform= and assign to field in stage's Builder #212

dispatcher: Parse --platform via FROM --platform= and assign to field in stage's Builder #212

Uh oh!

Conversation

flouthoc commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flouthoc commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomSweeneyRedHat commented Jan 31, 2022

Uh oh!

TomSweeneyRedHat commented Jan 31, 2022

Uh oh!

flouthoc commented Jan 31, 2022

Uh oh!

nalind commented Jan 31, 2022

Uh oh!

flouthoc commented Jan 31, 2022

Uh oh!

flouthoc commented Jan 31, 2022

Uh oh!

nalind commented Jan 31, 2022

Uh oh!

flouthoc commented Jan 31, 2022

Uh oh!

Uh oh!

Uh oh!

flouthoc commented Feb 1, 2022

Uh oh!

nalind left a comment

Choose a reason for hiding this comment

Uh oh!

nalind Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

nalind Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Feb 2, 2022

Uh oh!

flouthoc commented Feb 2, 2022

Uh oh!

rhatdan commented Feb 2, 2022

Uh oh!

openshift-ci bot commented Feb 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dispatcher: Parse `--platform` via `FROM --platform=` and assign to field in stage's Builder #212

dispatcher: Parse `--platform` via `FROM --platform=` and assign to field in stage's Builder #212

flouthoc commented Jan 31, 2022 •

edited

Loading

flouthoc commented Jan 31, 2022 •

edited

Loading