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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 2 commits into from

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Aug 18, 2024 •
edited
Loading

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

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc

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

@flouthoc flouthoc changed the title stage_executor: set avoidLookingCache only if mounting stage stage_executor: set avoidLookingCache only if mounting stage and not additional build context Aug 18, 2024
@flouthoc flouthoc mentioned this pull request Aug 18, 2024
Draft
@flouthoc
Copy link
Collaborator Author

@containers/buildah-maintainers PTAL

@rhatdan
Copy link
Member

rhatdan commented Aug 19, 2024

LGTM
@nalind PTAL

@flouthoc
imagebuildah: additionalContext is not a local built stage
f1cd5ae 
This line looks like a typo in past commit as `additionalContext` which
is found is not a local built stage so set `IsStage` to `false`.

Signed-off-by: flouthoc <[email protected]>
@flouthoc
stage_executor: set avoidLookingCache only if mounting stage
d098893 
set `avoidLookingCache` to `true` if `--mount` is using a freshly built
stage and not for `additional-build-context`.

Signed-off-by: flouthoc <[email protected]>
@flouthoc flouthoc force-pushed the use-cache-with-build-context branch from 01ea275 to d098893 Compare August 20, 2024 15:28
@flouthoc
Copy link
Collaborator Author

Rebased.

@nalind
Copy link
Member

nalind commented Aug 21, 2024

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

flouthoc commented Aug 21, 2024 •
edited
Loading

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 #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 #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
Copy link
Collaborator Author

@nalind PTAL

@nalind
Copy link
Member

nalind commented Aug 29, 2024

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
Copy link
Member

rhatdan commented Sep 13, 2024

@flouthoc waiting on you?

@flouthoc
Copy link
Collaborator Author

@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 #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
Copy link
Collaborator Author

@nalind Could you PTAL again :)

nalind
nalind reviewed Sep 17, 2024
View reviewed changes
tests/bud.bats
_EOF

run_buildah build $WITH_POLICY_JSON --layers -t source2 -f $contextdir/Containerfile2
expect_output --substring "firstfile"
Copy link
Member

@nalind nalind Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This should probably also check that the string "Using cache" doesn't show up in the output for this build.

@nalind
Copy link
Member

nalind commented Sep 17, 2024

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

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 #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 flouthoc mentioned this pull request Sep 21, 2024
Open
@flouthoc
Copy link
Collaborator Author

@nalind Any thoughts on above comment.

@flouthoc
Copy link
Collaborator Author

Just waiting for review on my last comment #5693 (comment)

@github-actions GitHub Actions
Copy link

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

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

@rhatdan rhatdan Awaiting requested review from rhatdan

@nalind nalind Awaiting requested review from nalind

Assignees
No one assigned
Labels
approved stale-pr
Projects
None yet
Milestone
No milestone
3 participants
@flouthoc @rhatdan @nalind