dispatcher: honor default builtin-args while processing `FROM --platform=` by flouthoc · Pull Request #224 · openshift/imagebuilder

flouthoc · 2022-04-04T10:14:16Z

While processing inline --platform= in FROM , imagebuilder ignores
default builtinArgs so for example if user wants to use inbuilt default arg
FROM --platform=$BUILDPLATFORM it should automatically replace it with
default value of BUILDPLATFORM.

Fixes

FROM --platform=$BUILDPLATFORM busybox
RUN echo "do something"

While processing inline `--platform=` in FROM , imagebuilder ignores default builtinArgs so for example if user wants to use inbuilt default arg `FROM --platform=$BUILDPLATFORM` it should automatically replace it with default value of BUILDPLATFORM. Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc · 2022-04-04T10:15:29Z

@nalind @rhatdan @TomSweeneyRedHat PTAL

openshift-ci · 2022-04-04T10:17:18Z

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

rhatdan · 2022-04-04T13:44:39Z

LGTM
@nalind PTAL

nalind · 2022-04-04T21:45:36Z

dispatchers.go

 			return fmt.Errorf("Windows does not support FROM scratch")
 		}
 	}
+	defaultArgs := envMapAsSlice(builtinBuildArgs)


Are the default values all correct when --platform is specified differently for different stages?

@nalind mostly we only use $BUILDPLATFORM or $TARGETPLATFORM in inline platform syntax so yes default values are correct for different stages. However there is still a small issue with the way buildah populates $TARGETPLATFORM but its a small fix on buildah side not on imagebuilder side and might not be relevant to this PR.

Created a PR in buildah which exercises this and fixes $TARGETPLATFORM issue: containers/buildah#3899

What about the dockerclient client?

@nalind For dockerclient we are already setting args correctly so $BUILDPLATFORM will be set correctly but other options like $TARGETPLATFORM will be only exercised by docker deamon if "features": { "buildkit": true } is enabled on the server side. So if docker daemon is running without buildkit certain things will be no-op but I think imagebuilder can not help a lot in such cases.

@nalind For dockerclient we are already setting args correctly so $BUILDPLATFORM will be set correctly but other options like $TARGETPLATFORM will be only exercised by docker deamon if "features": { "buildkit": true } is enabled on the server side. So if docker daemon is running without buildkit certain things will be no-op but I think imagebuilder can not help a lot in such cases.

Its pretty much like above everything works fine except its just that $TARGETPLATFORM might be no-op depending upon the fact if dockerserver supports buildkit or not but I also think this part might be out of the scope of this PR itself.

But PR only talks about implementation of FROM --platform= which works fine for dockerclient and we added implementation for dockerclient here: #212

The docker daemon doesn't evaluate the Dockerfile, we do.

The docker daemon doesn't evaluate the Dockerfile, we do.

Oh my bad i mixed things, if we use TARGETPLATFORM as -build-arg with imagebuilder it will work but we can't expect $TARGETPLATFORM to get correctly populated cause we don't have cli arg like buildah build --platform or docker build --platform in imagebuilder to correctly set the value of TARGETPLATFORM correct me if i got this wrong.

So its just that with imagebuilder it is expected from user to set the value of TARGETGETPLATFORM using -build-arg otherwise there is no way set the correct value of TARGETPLATFORM and default value is always https://github.com/openshift/imagebuilder/blob/master/dispatchers.go#L35

I have a feeling that this is just expected behavior and I'm overthinking this.

Wouldn't TAGETGETPLATFORM just default to the local platform then?

@rhatdan Yes It defaults to local then.

TomSweeneyRedHat · 2022-04-05T18:03:25Z

LGTM
if @nalind is happy with this.

flouthoc · 2022-04-07T19:14:19Z

@nalind Could PTAL again.

rhatdan · 2022-04-12T10:26:41Z

/approve
LGTM

flouthoc · 2022-04-29T11:16:13Z

@nalind @rhatdan Performed conformance test on this PR with buildkit enabled.

with and without below diff I got same output as: https://pastebin.com/raw/mrb63TWE

diff --git a/dockerclient/conformance_test.go b/dockerclient/conformance_test.go
index 66508f3..5585c9c 100644
--- a/dockerclient/conformance_test.go
+++ b/dockerclient/conformance_test.go
@@ -142,7 +142,7 @@ func TestCopyFrom(t *testing.T) {
                        e.Out, e.ErrOut = out, out
                        b := imagebuilder.NewBuilder(nil)
                        dockerfile := fmt.Sprintf(`
-                               FROM busybox AS base
+                               FROM --platform=$BUILDPLATFORM busybox AS base
                                RUN %s
                                FROM busybox
                                %s

Two tests always fail for me even on the master branch without this PR and with or without buildkit enabled ( also both tests seems unrelated to this PR )

TestMount
TestConformanceInternal/novolume

nalind · 2022-04-29T14:55:21Z

A chmod -R go-w ./dockerclient/testdata is generally needed before running the conformance tests, as they make assumptions about permissions of files in the source tree which are not always correct.

flouthoc · 2022-04-29T17:26:20Z

Thanks @nalind after chmod -R go-w ./dockerclient/testdata TestMount passes now just TestConformanceInternal/novolume fails for me both on this PR and master branch as well.

Output: https://pastebin.com/raw/HVGGGAya

Independent run for TestConformanceInternal/novolume

go test -v -tags conformance -timeout 3h ./dockerclient -run=TestConformanceInternal/novolume
=== RUN   TestConformanceInternal
=== RUN   TestConformanceInternal/novolume
    conformance_test.go:1068: conformance-dockerbuild-28-docker-0-5 and conformance-dockerbuild-28-direct-0-5 differ at var/lib/not-in-this-image/:
        &tar.Header{Typeflag:0x35, Name:"var/lib/not-in-this-image/", Linkname:"", Size:0, Mode:17389, Uid:0, Gid:0, Uname:"", Gname:"", ModTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), AccessTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), ChangeTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), Devmajor:0, Devminor:0, Xattrs:map[string]string(nil), PAXRecords:map[string]string(nil), Format:2}
        &tar.Header{Typeflag:0x35, Name:"var/lib/not-in-this-image/", Linkname:"", Size:0, Mode:16877, Uid:0, Gid:0, Uname:"", Gname:"", ModTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), AccessTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), ChangeTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), Devmajor:0, Devminor:0, Xattrs:map[string]string(nil), PAXRecords:map[string]string(nil), Format:2}
    conformance_test.go:1068: conformance-dockerbuild-28-docker-0-5 and conformance-dockerbuild-28-direct-0-5 differ at var/lib/:
        &tar.Header{Typeflag:0x35, Name:"var/lib/", Linkname:"", Size:0, Mode:17389, Uid:0, Gid:0, Uname:"", Gname:"", ModTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), AccessTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), ChangeTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), Devmajor:0, Devminor:0, Xattrs:map[string]string(nil), PAXRecords:map[string]string(nil), Format:2}
        &tar.Header{Typeflag:0x35, Name:"var/lib/", Linkname:"", Size:0, Mode:16877, Uid:0, Gid:0, Uname:"", Gname:"", ModTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), AccessTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), ChangeTime:time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), Devmajor:0, Devminor:0, Xattrs:map[string]string(nil), PAXRecords:map[string]string(nil), Format:2}
    conformance_test.go:1083: a(conformance-dockerbuild-28-docker-0-5)=map[] b(conformance-dockerbuild-28-direct-0-5)=map[] diff=map[var/lib/:[<0 dtrwxr-xr-x> <0 drwxr-xr-x>] var/lib/not-in-this-image/:[<0 dtrwxr-xr-x> <0 drwxr-xr-x>]]
    conformance_test.go:946: 28: full Docker build was not equivalent to squashed image metadata /tmp/dockerbuild-conformance-1936807101/Dockerfile
=== RUN   TestConformanceInternal/novolumenorun
--- FAIL: TestConformanceInternal (38.38s)
    --- FAIL: TestConformanceInternal/novolume (23.33s)
    --- PASS: TestConformanceInternal/novolumenorun (15.05s)
FAIL
FAIL	github.com/openshift/imagebuilder/dockerclient	38.386s
FAIL
make: *** [Makefile:10: test-conformance] Error 1

nalind · 2022-04-29T17:42:10Z

That's... is docker setting the sticky bit on those directories? Is this a difference in behavior between docker-with-buildkit and docker-without-buildkit?

flouthoc · 2022-04-29T17:51:02Z

Was failing for me irrespective of the buildkit but i can confirm again.

flouthoc · 2022-04-29T18:23:26Z

It was my docker version updating it fixed for both buildkit and regular docker.

flouthoc · 2022-04-29T18:28:46Z

Passes now

$ sudo go test -v -tags conformance -timeout 3h ./dockerclient -run=TestConformanceInternal/novolume
=== RUN   TestConformanceInternal
=== RUN   TestConformanceInternal/novolume
=== RUN   TestConformanceInternal/novolumenorun
--- PASS: TestConformanceInternal (9.63s)
    --- PASS: TestConformanceInternal/novolume (6.67s)
    --- PASS: TestConformanceInternal/novolumenorun (2.96s)
PASS
ok  	github.com/openshift/imagebuilder/dockerclient	9.630s

rhatdan · 2022-05-02T17:25:58Z

/lgtm

openshift-ci · 2022-05-02T17:26:31Z

[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

Use imagebuilder which sets correct behaviour for default builtinargs while processing inline `FROM --platform=` after openshift/imagebuilder#224 Signed-off-by: Aditya R <arajan@redhat.com>

siretart · 2023-09-03T01:02:13Z

This patch introduces tests that fail on arm64, armhf and armel with:

On arm64:

=== RUN   TestDispatchFromFlagsAndUseBuiltInArgs
    dispatchers_test.go:798: Expected linux/arm64, to match linux/arm64/v8
--- FAIL: TestDispatchFromFlagsAndUseBuiltInArgs (0.00s)

on armel:

=== RUN   TestDispatchFromFlagsAndUseBuiltInArgs
    dispatchers_test.go:798: Expected linux/arm, to match linux/arm/v8
--- FAIL: TestDispatchFromFlagsAndUseBuiltInArgs (0.00s)

On armhf:

=== RUN   TestDispatchFromFlagsAndUseBuiltInArgs
    dispatchers_test.go:798: Expected linux/arm, to match linux/arm/v7
--- FAIL: TestDispatchFromFlagsAndUseBuiltInArgs (0.00s)

openshift-ci bot requested review from TomSweeneyRedHat and mrunalp April 4, 2022 10:14

nalind reviewed Apr 4, 2022

View reviewed changes

flouthoc requested a review from nalind April 5, 2022 07:44

flouthoc mentioned this pull request Apr 5, 2022

build: automatically set correct $TARGETPLATFORM where expected. containers/buildah#3899

Merged

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

openshift-ci bot assigned rhatdan May 2, 2022

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

openshift-merge-robot merged commit 009dbc6 into openshift:master May 2, 2022

siretart mentioned this pull request Sep 4, 2023

TestDispatchFromFlagsAndUseBuiltInArgs fails on arm #263

Closed

Conversation

flouthoc commented Apr 4, 2022

Uh oh!

flouthoc commented Apr 4, 2022

Uh oh!

openshift-ci bot commented Apr 4, 2022

Uh oh!

rhatdan commented Apr 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat commented Apr 5, 2022

Uh oh!

flouthoc commented Apr 7, 2022

Uh oh!

rhatdan commented Apr 12, 2022

Uh oh!

flouthoc commented Apr 29, 2022

Uh oh!

nalind commented Apr 29, 2022

Uh oh!

flouthoc commented Apr 29, 2022

Uh oh!

nalind commented Apr 29, 2022

Uh oh!

flouthoc commented Apr 29, 2022

Uh oh!

flouthoc commented Apr 29, 2022

Uh oh!

flouthoc commented Apr 29, 2022

Uh oh!

rhatdan commented May 2, 2022

Uh oh!

openshift-ci bot commented May 2, 2022

Uh oh!

siretart commented Sep 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

flouthoc Apr 5, 2022 •

edited

Loading

flouthoc Apr 12, 2022 •

edited

Loading

siretart commented Sep 3, 2023 •

edited

Loading