stage_executor: reset platform in systemcontext for every stage. #5971

flouthoc · 2025-02-04T17:40:10Z

On processing of every stage platform spec in systemcontext must be correctly reset.

[NO NEW TESTS NEEDED]

Closes: #5968

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

stage_executor: reset platform in systemcontext for every stage.

openshift-ci · 2025-02-04T17:40:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: flouthoc
Once this PR has been reviewed and has the lgtm label, please assign lsm5 for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

flouthoc · 2025-02-04T17:41:01Z

Just wondering how to test this in CI since binfmt will correctly run every image type.

packit-as-a-service · 2025-02-04T17:43:49Z

Ephemeral COPR build failed. @containers/packit-build please check.

flouthoc · 2025-02-04T18:21:06Z

Temp adding No New Tests to see if CI passes, will remove the label when I add test.

nalind · 2025-02-04T18:58:29Z

The "header-builtin" test does something similar, so I'd suggest

ARG FOREIGNARCH
FROM --platform=linux/$FOREIGNARCH busybox AS foreign
FROM busybox
COPY --from=foreign /bin/busybox /bin/busybox.foreign
RUN arch
RUN ls -l /bin/busybox /bin/busybox.foreign
RUN ! cmp /bin/busybox /bin/busybox.foreign

Define the FOREIGNARCH build arg by checking if $(go env GOARCH) is amd64 or arm64 or ppc64le or s390x, having preselected one or them as a default, and choosing a different one if that default happens to match the local architecture.

imagebuildah/stage_executor.go

nalind · 2025-02-11T17:37:12Z

imagebuildah/stage_executor.go

+	// In a multi-stage build where `FROM --platform=<>` was used then we must
+	// reset context for new stages so that new stages don't inherit unexpected
+	// `--platform` from prior stages.
+	if stage.Builder.Platform != "" || (stage.Position != 0 && builderSystemContext.ArchitectureChoice == "" && builderSystemContext.VariantChoice == "" && builderSystemContext.OSChoice == "") {


Would this be simpler if the builderSystemContext had its ...Choice fields cleared as part of initializing it?

I don't think we can do that since in certain cases the fields are set beforehand for example when it's build with --platform or --all-platforms so clearing ...Choice would reset values in such case.

I think there's a weird corner case in here when jobs > 1, skipUnusedStages is false, and one or more later stages are using the same base image as the first stage.

I pushed a new patch and fixed this edge-case but I am scared that it's hard to reproduce the edge-case in CI since it happens rarely.

This also involved fixing libimage containers/common#2339

Is the intention here that the first stage should default to being built for the default platform when --platform is not specified? It's hard to tell.

There are also two cases where len(s.stages) > 1 is evaluated, and that makes this line longer and, for me, that much harder to wrap my head around.

I fixed it.

Anything that happens when jobs > 1 can also happen when there are multiple builds running, as podman allows. Given that, I don't think this recent addition actually fixes that third case.

Yes patch in this PR only covers the case when single buildah invoked and multiple jobs are sharing same context variables. I suspect there could be more cases in pull then parallel invocation of multiple buildah can also step into some new issues but to be honest I did not test that.

Okay did I two experiments one with --skip-unused-stages=false and one without with following containerfile

FROM --platform=linux/riscv64 quay.io/prometheus/busybox AS risc FROM --platform=linux/amd64 quay.io/prometheus/busybox AS amd FROM --platform=linux/arm64 quay.io/prometheus/busybox AS foreign FROM quay.io/prometheus/busybox COPY --from=foreign /bin/busybox /bin/busybox.foreign COPY --from=amd /bin/busybox /bin/busybox.amd RUN arch RUN ls -l /bin/busybox /bin/busybox.foreign RUN ! cmp /bin/busybox /bin/busybox.foreign RUN cmp /bin/busybox /bin/busybox.amd

Experiment One

for i in `seq 1 25`; do ./buildah build --iidfile id.$i . & done; wait; ls -al

Experiment Two

for i in `seq 1 25`; do ./buildah build --jobs=2 --skip-unused-stages=false --iidfile id.$i . & done; wait; ls -al

I ran both experiments a few times I did not see any failure.

@nalind Should I open a different github issue (to track) to find verifying to check if parallel race exists or not. As I able not able to find any issues till now but wondering if this PR can get blocked because of that.

Yes, if we don't know for sure we that don't still have problems in here, and we're not fixing them.

I created an issue to track this thing separately #6104 so it's easier to move with this PR.

tests/bud.bats

nalind · 2025-02-11T17:38:39Z

tests/bud.bats

+  if [[ "$otherarch" == "$myarch" ]]; then
+    otherarch="amd64"
+  fi
+  run_buildah build $WITH_POLICY_JSON --build-arg FOREIGNARCH=$otherarch -f $BUDFILES/multiarch/Containerfile.reset-platform $BUDFILES/multiarch


This should also test a case where the the TARGETPLATFORM build-arg is set, so that we don't regress on that.

I added a test which sets TARGETPLATFORM using --platform, should that cover what you meant by this comment ?

I mean literally setting --build-arg TARGETPLATFORM=, which has been known to be done instead of using the --platform flag. This had to be fixed before in #5731.

flouthoc · 2025-02-18T19:43:21Z

@nalind PTAL

Following commit fixes a `race` condition in `libimage` because in `Pull(` after performing `copy` from remote sources it agains attempts to resolve image via `LookupImage`, any operation between `copy` and `LookupImage` can remove `name` from the recently pulled image. Causing race in builds. This issue was discoverd while working on PR containers/buildah#5971 ``` buildah build -t test --jobs=2 --skip-unused-stages=false . ``` Containerfile ``` FROM quay.io/jitesoft/alpine RUN arch FROM --platform=linux/arm64 quay.io/jitesoft/alpine AS foreign ``` Following commit also addresses the commit containers@e2324dd by performing the neccessary refactor. No functional change in public exposed API, exisiting tests should pass as-is. [NO NEW TESTS NEEDED] Signed-off-by: flouthoc <[email protected]>

TomSweeneyRedHat · 2025-03-21T19:22:04Z

@flouthoc the branch needs updating. Also, did you address all of Nalin's comments?

flouthoc · 2025-03-21T21:37:45Z

I have to get back to @nalind 's comment since I was working on infra needed for this PR containers/common#2339

flouthoc · 2025-03-24T14:43:06Z

@nalind Could you PTAL again.

nalind · 2025-04-01T17:37:33Z

imagebuildah/stage_executor.go

-		if os != "" {
-			builderSystemContext.OSChoice = os
+		if imageOS != "" {
+			builderSystemContext.OSChoice = imageOS


Why are we creating builderSystemContext, which is a copy of the pointer value in s.systemContext, and modifying it through that pointer? It has the same effect as modifying s.systemContext directly, but is harder to follow. Is it intended to be a one-off copy that we just pass in the BuilderOptions?

Removed. This is not required since we are already copying context for each stage in executor.

Every stage now has its own copy of systemcontext. On processing of every stage platform spec in systemcontext must be correctly reset. Closes: containers#5968 Signed-off-by: flouthoc <[email protected]>

flouthoc mentioned this pull request Feb 4, 2025

--platform parameter spills out of its FROM clause #5968

Open

flouthoc force-pushed the reset-context-platform branch from d15172e to 8cef977 Compare February 4, 2025 18:04

flouthoc added the No New Tests Allow PR to proceed without adding regression tests label Feb 4, 2025

flouthoc force-pushed the reset-context-platform branch 4 times, most recently from cbc1002 to 4010772 Compare February 11, 2025 16:01

flouthoc removed the No New Tests Allow PR to proceed without adding regression tests label Feb 11, 2025

flouthoc requested a review from nalind February 11, 2025 16:42

nalind reviewed Feb 11, 2025

View reviewed changes

imagebuildah/stage_executor.go Outdated Show resolved Hide resolved

nalind reviewed Feb 11, 2025

View reviewed changes

tests/bud.bats Show resolved Hide resolved

nalind reviewed Feb 11, 2025

View reviewed changes

flouthoc mentioned this pull request Feb 12, 2025

Use tmpfs for integration tests #5959

Merged

flouthoc force-pushed the reset-context-platform branch from 4010772 to 66b0cac Compare February 18, 2025 19:39

flouthoc requested a review from nalind February 18, 2025 19:42

flouthoc force-pushed the reset-context-platform branch 2 times, most recently from f5d79f8 to c95236a Compare February 25, 2025 14:45

flouthoc mentioned this pull request Mar 1, 2025

pull,load: use *Image instead of re-resolving via name containers/common#2339

Merged

flouthoc force-pushed the reset-context-platform branch from c95236a to 1ba2b7a Compare March 24, 2025 14:39

flouthoc force-pushed the reset-context-platform branch 3 times, most recently from eb091a9 to 8ec6b2b Compare March 27, 2025 14:34

flouthoc mentioned this pull request Apr 1, 2025

Verify if --platform in FROM from parallel builds can affect other builds #6104

Open

nalind reviewed Apr 1, 2025

View reviewed changes

stage_executor: reset platform in systemcontext for stages

205de38

Every stage now has its own copy of systemcontext. On processing of every stage platform spec in systemcontext must be correctly reset. Closes: containers#5968 Signed-off-by: flouthoc <[email protected]>

flouthoc force-pushed the reset-context-platform branch from 8ec6b2b to 205de38 Compare April 1, 2025 21:57

flouthoc requested a review from nalind April 1, 2025 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stage_executor: reset platform in systemcontext for every stage. #5971

stage_executor: reset platform in systemcontext for every stage. #5971

flouthoc commented Feb 4, 2025

openshift-ci bot commented Feb 4, 2025

flouthoc commented Feb 4, 2025

packit-as-a-service bot commented Feb 4, 2025

flouthoc commented Feb 4, 2025

nalind commented Feb 4, 2025

nalind Feb 11, 2025

flouthoc Feb 18, 2025

nalind Feb 25, 2025

flouthoc Mar 24, 2025 •

edited

Loading

nalind Mar 26, 2025

flouthoc Mar 27, 2025

flouthoc Mar 27, 2025 •

edited

Loading

flouthoc Apr 1, 2025

nalind Apr 1, 2025

flouthoc Apr 1, 2025

nalind Feb 11, 2025

flouthoc Feb 18, 2025

nalind Feb 18, 2025

flouthoc commented Feb 18, 2025

TomSweeneyRedHat commented Mar 21, 2025

flouthoc commented Mar 21, 2025

flouthoc commented Mar 24, 2025

nalind Apr 1, 2025

flouthoc Apr 1, 2025

stage_executor: reset platform in systemcontext for every stage. #5971

Are you sure you want to change the base?

stage_executor: reset platform in systemcontext for every stage. #5971

Conversation

flouthoc commented Feb 4, 2025

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

openshift-ci bot commented Feb 4, 2025

flouthoc commented Feb 4, 2025

packit-as-a-service bot commented Feb 4, 2025

flouthoc commented Feb 4, 2025

nalind commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flouthoc Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flouthoc Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flouthoc commented Feb 18, 2025

TomSweeneyRedHat commented Mar 21, 2025

flouthoc commented Mar 21, 2025

flouthoc commented Mar 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flouthoc Mar 24, 2025 •

edited

Loading

flouthoc Mar 27, 2025 •

edited

Loading