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.

Sign up for GitHub

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

Jump to bottom

pkg/imagebuilder: expose BuiltInBuildArgs for downstream #258

Closed
wants to merge 1 commit into from
Closed

pkg/imagebuilder: expose BuiltInBuildArgs for downstream #258

wants to merge 1 commit into from

Conversation

flouthoc
Copy link
Contributor

@flouthoc flouthoc commented Jun 2, 2023

Expose map BuiltInBuildArgs so downstream users like buildah can use it.

PR: containers/buildah#4839 directly uses this for info

@flouthoc flouthoc mentioned this pull request Jun 2, 2023
Open
@TomSweeneyRedHat
Copy link
Contributor

The change LGTM, but the conformance test is failing with an error code "2", which I'm not sure where it's popping from.

@flouthoc
Copy link
Contributor Author

flouthoc commented Jun 2, 2023

@TomSweeneyRedHat Conformance test failure looks unrealted, it needs to be restart but I dont have access.

@rhatdan
Copy link
Contributor

rhatdan commented Jun 5, 2023

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 5, 2023

[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

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2023
@flouthoc
Copy link
Contributor Author

flouthoc commented Jun 6, 2023

@nalind PTAL

@nalind
Copy link
Member

nalind commented Jun 14, 2023

Is there a way to prevent callers from overwriting these values?

@nalind
Copy link
Member

nalind commented Jun 15, 2023

At least one of the conformance test errors should be fixed by a rebase.

@flouthoc
Copy link
Contributor Author

Is there a way to prevent callers from overwriting these values?

@nalind Could we create a function which exposes a copy of the map and not the actual map. WDYT ?

@flouthoc flouthoc force-pushed the expose-builtinargs branch from 84015b2 to ac04851 Compare June 16, 2023 06:43
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 16, 2023

New changes are detected. LGTM label has been removed.

@flouthoc flouthoc force-pushed the expose-builtinargs branch from ac04851 to 36a0f49 Compare June 16, 2023 06:43
@nalind
Copy link
Member

nalind commented Jun 16, 2023

Is there a way to prevent callers from overwriting these values?

@nalind Could we create a function which exposes a copy of the map and not the actual map. WDYT ?

That would be fine.

@flouthoc flouthoc force-pushed the expose-builtinargs branch from 36a0f49 to ff5f1ae Compare June 16, 2023 15:40
@flouthoc
Copy link
Contributor Author

@nalind PTAL

@flouthoc flouthoc force-pushed the expose-builtinargs branch 2 times, most recently from 619a14d to 9bdb309 Compare June 16, 2023 15:59
flouthoc
flouthoc commented Jun 16, 2023
View reviewed changes
dispatchers.go Outdated
@@ -22,6 +22,7 @@ import (
"github.com/containers/storage/pkg/regexp"
"github.com/openshift/imagebuilder/signal"
"github.com/openshift/imagebuilder/strslice"
"golang.org/x/exp/maps"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this since this is easier to use, other projects such as containers/image is already using this since this is expected to become part of golang.

However this is totally optional, I can amend to do manual clone as well without any dep.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvm , this is changing to many deps.

@flouthoc
pkg/imagebuilder: expose BuiltInBuildArgs for downstream
0f679ca 
Expose map BuiltInBuildArgs so downstream users like buildah can use it.

Signed-off-by: Aditya R <[email protected]>
@flouthoc flouthoc force-pushed the expose-builtinargs branch from 9bdb309 to 0f679ca Compare June 16, 2023 16:11
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 16, 2023

@flouthoc: all tests passed!

Full PR test history. Your PR dashboard.

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
Copy link
Contributor Author

@nalind PTAL

@nalind
Copy link
Member

nalind commented Jun 21, 2023

The use case for this suggests that it expects these values to be updated when FROM uses a --platform flag, but I don't see those updates being reflected in the current iteration.

@flouthoc
Copy link
Contributor Author

Ah i see, I am not sure if values of defaultArgs are changed when baseimage uses a inline --platform but sure let me confirm this with buildkit and report back here.

@flouthoc
Copy link
Contributor Author

@nalind I don't think buildkit does this, I tried following Containerfile and values of defaultargs don't change for inline --platform change, I am sharing output of my experiment here.

FROM alpine
ARG TARGETOS TARGETARCH TARGETPLATFORM BUILDOS BUILDPLATFORM BUILDARCH
RUN echo "TARGETOS=$TARGETOS"
RUN echo "TARGETARCH=$TARETARCH"
RUN echo "TARGETPLATFORM=$TARGETPLATFORM"
RUN echo "BUILDOS=$BUILDOS"
RUN echo "BUILDARCH=$BUILDARCH"
RUN echo "BUILDPLATFORM=$BUILDPLATFORM"
RUN echo hey > hello

FROM --platform=linux/arm64 ubuntu
ARG TARGETOS TARGETARCH TARGETPLATFORM BUILDOS BUILDPLATFORM BUILDARCH
COPY --from=0 hello .
RUN echo "TARGETOS=$TARGETOS"
RUN echo "TARGETARCH=$TARETARCH"
RUN echo "TARGETPLATFORM=$TARGETPLATFORM"
RUN echo "BUILDOS=$BUILDOS"
RUN echo "BUILDARCH=$BUILDARCH"
RUN echo "BUILDPLATFORM=$BUILDPLATFORM"
[fl@fedora patform]$ sudo docker buildx build --no-cache --progress=plain -t test .
#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 32B done
#1 DONE 0.1s

#2 [internal] load .dockerignore
#2 transferring context: 2B done
#2 DONE 0.1s

#3 [internal] load metadata for docker.io/library/ubuntu:latest
#3 DONE 0.9s

#4 [internal] load metadata for docker.io/library/alpine:latest
#4 DONE 1.7s

#5 [stage-1 1/8] FROM docker.io/library/ubuntu@sha256:6120be6a2b7ce665d0cbddc3ce6eae60fe94637c6a66985312d1f02f63cc0bcd
#5 CACHED

#6 [stage-0 1/8] FROM docker.io/library/alpine@sha256:82d1e9d7ed48a7523bdebc18cf6290bdb97b82302a8a9c27d4fe885949ea94d1
#6 CACHED

#7 [stage-0 2/8] RUN echo "TARGETOS=linux"
#7 0.326 TARGETOS=linux
#7 DONE 0.4s

#8 [stage-0 3/8] RUN echo "TARGETARCH=$TARETARCH"
#8 0.363 TARGETARCH=
#8 DONE 0.4s

#9 [stage-0 4/8] RUN echo "TARGETPLATFORM=linux/amd64"
#9 0.349 TARGETPLATFORM=linux/amd64
#9 DONE 0.4s

#10 [stage-0 5/8] RUN echo "BUILDOS=linux"
#10 0.336 BUILDOS=linux
#10 DONE 0.4s

#11 [stage-0 6/8] RUN echo "BUILDARCH=amd64"
#11 0.339 BUILDARCH=amd64
#11 DONE 0.4s

#12 [stage-0 7/8] RUN echo "BUILDPLATFORM=linux/amd64"
#12 0.362 BUILDPLATFORM=linux/amd64
#12 DONE 0.4s

#13 [stage-0 8/8] RUN echo hey > hello
#13 DONE 0.4s

#14 [stage-1 2/8] COPY --from=0 hello .
#14 DONE 0.2s

#15 [stage-1 3/8] RUN echo "TARGETOS=linux"
#15 0.407 TARGETOS=linux
#15 DONE 0.5s

#16 [stage-1 4/8] RUN echo "TARGETARCH=$TARETARCH"
#16 0.382 TARGETARCH=
#16 DONE 0.4s

#17 [stage-1 5/8] RUN echo "TARGETPLATFORM=linux/amd64"
#17 0.419 TARGETPLATFORM=linux/amd64
#17 DONE 0.5s

#18 [stage-1 6/8] RUN echo "BUILDOS=linux"
#18 0.394 BUILDOS=linux
#18 DONE 0.4s

#19 [stage-1 7/8] RUN echo "BUILDARCH=amd64"
#19 0.387 BUILDARCH=amd64
#19 DONE 0.4s

#20 [stage-1 8/8] RUN echo "BUILDPLATFORM=linux/amd64"
#20 0.420 BUILDPLATFORM=linux/amd64
#20 DONE 0.5s

#21 exporting to image
#21 exporting layers
#21 exporting layers 0.5s done
#21 writing image sha256:db8f27a0fa0eaf3d57f1f2c93632f90e1f659b46898c650c4b8101d99b05ad63 done
#21 naming to docker.io/library/test
#21 naming to docker.io/library/test done
#21 DONE 0.6s
[fl@fedora patform]$

@rhatdan
Copy link
Contributor

rhatdan commented Jul 4, 2023

@nalind PTAL

@flouthoc flouthoc mentioned this pull request Jul 24, 2023
Open
@flouthoc
Copy link
Contributor Author

@nalind Could you PTAL

@nalind
Copy link
Member

nalind commented Jul 24, 2023

@nalind I don't think buildkit does this, I tried following Containerfile and values of defaultargs don't change for inline --platform change

Do they ever change? If not, what's the point of having two sets of variables whose values are always identical to each other?

@nalind
Copy link
Member

nalind commented Jul 24, 2023

Are they affected by the --platform command-line flag?

@flouthoc
Copy link
Contributor Author

@nalind Yes TARGETPLATFORM changes on cli --platform

[fl@fedora patform]$ sudo docker buildx build --platform linux/amd64 --progress=plain --no-cache -t test .
[sudo] password for fl: 
#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 691B done
#1 DONE 0.1s

#2 [internal] load .dockerignore
#2 transferring context: 2B done
#2 DONE 0.1s

#3 [internal] load metadata for docker.io/library/alpine:latest
#3 DONE 3.1s

#4 [1/8] FROM docker.io/library/alpine@sha256:82d1e9d7ed48a7523bdebc18cf6290bdb97b82302a8a9c27d4fe885949ea94d1
#4 CACHED

#5 [2/8] RUN echo "TARGETOS=linux"
#5 0.309 TARGETOS=linux
#5 DONE 0.4s

#6 [3/8] RUN echo "TARGETARCH=$TARETARCH"
#6 0.346 TARGETARCH=
#6 DONE 0.4s

#7 [4/8] RUN echo "TARGETPLATFORM=linux/amd64"
#7 0.326 TARGETPLATFORM=linux/amd64
#7 DONE 0.4s

#8 [5/8] RUN echo "BUILDOS=linux"
#8 0.331 BUILDOS=linux
#8 DONE 0.4s

#9 [6/8] RUN echo "BUILDARCH=amd64"
#9 0.334 BUILDARCH=amd64
#9 DONE 0.4s

#10 [7/8] RUN echo "BUILDPLATFORM=linux/amd64"
#10 0.325 BUILDPLATFORM=linux/amd64
#10 DONE 0.4s

#11 [8/8] RUN echo hey > hello
#11 DONE 0.4s

#12 exporting to image
#12 exporting layers
#12 exporting layers 0.6s done
#12 writing image sha256:db056b8eb8e45d89c42e58d84e67bb9ddc4ae235248fe2ba0c7511716ff3a70d done
#12 naming to docker.io/library/test done
#12 DONE 0.6s
[fl@fedora patform]$ sudo docker buildx build --platform linux/arm64 --progress=plain --no-cache -t test .
#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 32B done
#1 DONE 0.1s

#2 [internal] load .dockerignore
#2 transferring context: 2B done
#2 DONE 0.1s

#3 [internal] load metadata for docker.io/library/alpine:latest
#3 DONE 0.9s

#4 [1/8] FROM docker.io/library/alpine@sha256:82d1e9d7ed48a7523bdebc18cf6290bdb97b82302a8a9c27d4fe885949ea94d1
#4 CACHED

#5 [2/8] RUN echo "TARGETOS=linux"
#5 0.334 TARGETOS=linux
#5 DONE 0.4s

#6 [3/8] RUN echo "TARGETARCH=$TARETARCH"
#6 0.420 TARGETARCH=
#6 DONE 0.5s

#7 [4/8] RUN echo "TARGETPLATFORM=linux/arm64"
#7 0.354 TARGETPLATFORM=linux/arm64
#7 DONE 0.4s

#8 [5/8] RUN echo "BUILDOS=linux"
#8 0.401 BUILDOS=linux
#8 DONE 0.4s

#9 [6/8] RUN echo "BUILDARCH=amd64"
#9 0.343 BUILDARCH=amd64
#9 DONE 0.4s

#10 [7/8] RUN echo "BUILDPLATFORM=linux/amd64"
#10 0.348 BUILDPLATFORM=linux/amd64
#10 DONE 0.4s

#11 [8/8] RUN echo hey > hello
#11 DONE 0.4s

#12 exporting to image
#12 exporting layers
#12 exporting layers 1.1s done
#12 writing image sha256:6f64dac848d73a3a0560c82c05f4246b4b2be7f5e179927dfaf3fec6f7dcc858 done
#12 naming to docker.io/library/test done
#12 DONE 1.1s
[fl@fedora patform]$

@rhatdan
Copy link
Contributor

rhatdan commented Aug 5, 2023

@nalind PTAL

@nalind
Copy link
Member

nalind commented Aug 7, 2023

FROM --platform=linux/arm64 ubuntu
ARG TARGETOS TARGETARCH TARGETPLATFORM BUILDOS BUILDPLATFORM BUILDARCH
COPY --from=0 hello .
RUN echo "TARGETOS=$TARGETOS"
RUN echo "TARGETARCH=$TARETARCH"
(typo here)
RUN echo "TARGETPLATFORM=$TARGETPLATFORM"
RUN echo "BUILDOS=$BUILDOS"
RUN echo "BUILDARCH=$BUILDARCH"
RUN echo "BUILDPLATFORM=$BUILDPLATFORM"

[fl@fedora patform]$ sudo docker buildx build --no-cache --progress=plain -t test .
#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 32B done
#1 DONE 0.1s

#2 [internal] load .dockerignore
#2 transferring context: 2B done
#2 DONE 0.1s

#3 [internal] load metadata for docker.io/library/ubuntu:latest
#3 DONE 0.9s

#4 [internal] load metadata for docker.io/library/alpine:latest
#4 DONE 1.7s

#5 [stage-1 1/8] FROM docker.io/library/ubuntu@sha256:6120be6a2b7ce665d0cbddc3ce6eae60fe94637c6a66985312d1f02f63cc0bcd
#5 CACHED

#6 [stage-0 1/8] FROM docker.io/library/alpine@sha256:82d1e9d7ed48a7523bdebc18cf6290bdb97b82302a8a9c27d4fe885949ea94d1
#6 CACHED

#7 [stage-0 2/8] RUN echo "TARGETOS=linux"
#7 0.326 TARGETOS=linux
#7 DONE 0.4s

#8 [stage-0 3/8] RUN echo "TARGETARCH=$TARETARCH"
#8 0.363 TARGETARCH=
#8 DONE 0.4s

#9 [stage-0 4/8] RUN echo "TARGETPLATFORM=linux/amd64"
#9 0.349 TARGETPLATFORM=linux/amd64
#9 DONE 0.4s

#10 [stage-0 5/8] RUN echo "BUILDOS=linux"
#10 0.336 BUILDOS=linux
#10 DONE 0.4s

#11 [stage-0 6/8] RUN echo "BUILDARCH=amd64"
#11 0.339 BUILDARCH=amd64
#11 DONE 0.4s

#12 [stage-0 7/8] RUN echo "BUILDPLATFORM=linux/amd64"
#12 0.362 BUILDPLATFORM=linux/amd64
#12 DONE 0.4s

Is this an oversight in the FROM --platform implementation?

@flouthoc
Copy link
Contributor Author

flouthoc commented Aug 7, 2023

@nalind Do you mean if this could be a bug in buildkit implementation ? In a sense it could be an expected behavior since FROM --platform=someplatform does not necessarily confirms that we are building for someplatform it just means use the base image for the required platform which might be useful when building multi-platform image.

Consider a case where build-platform is amd64 while base image is of arm64 but contains a binary which can be executed on other build platforms as well without emulation ( This seems like a weird use-case but is still possible, I am sure there maybe many more such use-cases ).

Overall I'm not entirely sure if this is a bug but buildkit maintainers can confirm it better.

@rhatdan
Copy link
Contributor

rhatdan commented Sep 8, 2023

Still waiting on this one.

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 8, 2023
@openshift-bot
Copy link

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 7, 2024
@flouthoc
Copy link
Contributor Author

flouthoc commented Jan 7, 2024

@nalind Any opinion on this one ?

@openshift-bot
Copy link

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

@openshift-ci openshift-ci bot closed this Feb 7, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 7, 2024

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrunalp mrunalp Awaiting requested review from mrunalp

@TomSweeneyRedHat TomSweeneyRedHat Awaiting requested review from TomSweeneyRedHat

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@flouthoc @TomSweeneyRedHat @rhatdan @nalind @openshift-bot