stage_executor: set `avoidLookingCache` only if mounting stage and not additional build context

[flouthoc]

trafficstars

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

Newly added integration tests.

Which issue(s) this PR fixes:

Closes: https://github.com/containers/buildah/issues/5692

Special notes for your reviewer:

Does this PR introduce a user-facing change?

build: cache now works with when additional build context is used as source of --mount

[flouthoc]

@containers/buildah-maintainers PTAL

[rhatdan]

LGTM @nalind PTAL

[flouthoc]

Rebased.

[nalind]

Hang on, if the contents of the additional build context are the same as, or changed from, on a previous run, do we, or do we not want to reuse the cache candidate? What does "build: cache now works with additional build context and inline --mount" mean? How did it break before?

[flouthoc]

Hang on, if the contents of the additional build context are the same as, or changed from, on a previous run, do we, or do we not want to reuse the cache candidate?

Following concern will be fixed with this https://github.com/containers/buildah/pull/5691, where we will put the hash of the files in the context and make it part of the history.

What does "build: cache now works with additional build context and inline --mount" mean? How did it break before?

I think cache stopped working for --mount when additional build context is used as source after PR https://github.com/containers/buildah/pull/4526

Also I fixed the release note it should be

build: cache now works with when additional build context is used as source of --mount

[flouthoc]

@nalind PTAL

[nalind]

I think cache stopped working for --mount when additional build context is used as source after PR #4526

Stopped working how? Did it fail to find a suitable image to use as a cache image? Did it choose an unsuitable image?

Also I fixed the release note it should be

build: cache now works with when additional build context is used as source of --mount

What behavior would someone have seen before when things went wrong?

[rhatdan]

@flouthoc waiting on you?

[flouthoc]

@flouthoc waiting on you?

I don't know how I missed GH notification of this PR.

@nalind

Stopped working how? Did it fail to find a suitable image to use as a cache image? Did it choose an unsuitable image?

For this part the code which was introduced in PR https://github.com/containers/buildah/pull/4526 accounted avoidLookingCache incorrectly for additionalBuildContext when it is used as source for RUN --mount and whenever additionalBuildContext was selected it used to set avoidLookingCache = true which should have only been done for the newly built stages and not for everything else so caching never worked. It was fault in the logic.

What behavior would someone have seen before when things went wrong?

Previously the layer based caching was simply not working for users who were using additional-build-context as a source for RUN --mount command. No one ever reported it directly because I think no-one has spotted this till now.

[flouthoc]

@nalind Could you PTAL again :)

[nalind]

The "now works" in the release note still doesn't describe what was fixed. Are cached images used where they weren't before? Are they not when they shouldn't have been? Does cache evaluation now take into consideration something that it didn't before that it should have? If #5691 starts checking the contents of the bound directory, are we going to end up revisiting this change?

[flouthoc]

I am thinking if this explains it more correctly, could you read this and see if is answering all questions correctly.

Previously, Buildah would ignore cached layers for step RUN --mount steps which contained source as an additional build context, causing the step to rebuild every time. This occurred because the internal stage_executor incorrectly treated source as an internal stage, bypassing the entire cache searching mechanism and not looking for cached layer at all. With this update, Buildah now checks storage for an existing layer to use as a cache when RUN --mount includes source as an additional build context.

If https://github.com/containers/buildah/pull/5691 starts checking the contents of the bound directory, are we going to end up revisiting this change?

No, actually the reason of adding this change was that I found this bug while implementing #5691 since buildah was bypassing cache searching mechanism so my changes in #5691 were not working correctly, so I think this can be merged before #5691 so it actually use this change.

[flouthoc]

@nalind Any thoughts on above comment.

[flouthoc]

Just waiting for review on my last comment https://github.com/containers/buildah/pull/5693#issuecomment-2356689254

[github-actions[bot]]

A friendly reminder that this PR had no activity for 30 days.

[TomSweeneyRedHat]

@nalind PTAL again @flouthoc needs a rebase

[TomSweeneyRedHat]

LGTM

[rhatdan]

/approve /lgtm

[openshift-ci[bot]]

[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:

• ~~OWNERS~~ [rhatdan]

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