Fix caching when mounting a cached stage with COPY/ADD #5445
Conversation
|
Thanks @aaronlehmann |
There was a problem hiding this comment.
@aaronlehmann Thanks for PR, could you fill the section of issue in PR description with a small reproduce so its easier to verify. As of now I'm unable to exactly understand what was the problem before this PR.
|
@flouthoc: I added some notes in the "Issue" section. Please let me know if I can clarify further. |
There was a problem hiding this comment.
@aaronlehmann Thanks PR LGTM, could you please rebase it :)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aaronlehmann, 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 |
A comment states that avoidLookingCache is set when a previous stage
that executed as part of the build is referenced by --mount, to avoid
reusing content from an older build of the stage:
// Only attempt to find cache if its needed, this part
// so that if a step is using RUN --mount and mounts
// previous stages then it uses the freshly built stage
// of re-using the older stage from the store.
However, stages consisting of COPY/ADD seem to be flagged with
didExecute even if they were fetched from cache instead. I believe
this is an oversight, and these stages should not prevent subsequent
caching.
Also, avoidLookingCache would prevent a cache push, but I think it
should only prevent cache lookups, since populating the cache is still
useful in these caess.
It's very possible I'm misunderstanding something, but I believe the
RUN step in test case I've added wrongly skips cache, and I'd appreciate
some pointers in the right direction if what I've proposed here isn't
the right solution.
Signed-off-by: Aaron Lehmann <alehmann@netflix.com>
b413360 to
d9129b4
Compare
|
Thanks! Rebased. |
|
/lgtm |
|
/unhold |
What type of PR is this?
/kind bug
What this PR does / why we need it:
A comment states that
avoidLookingCacheis set when a previous stage that executed as part of the build is referenced by--mount, to avoid reusing content from an older build of the stage:However, stages consisting of
COPY/ADDseem to be flagged withdidExecuteeven if they were fetched from cache instead. I believe this is an oversight, and these stages should not prevent subsequent caching.Also,
avoidLookingCachewould prevent a cache push, but I think it should only prevent cache lookups, since populating the cache is still useful in these caess.How to verify it
Run the provided test case.
Which issue(s) this PR fixes:
Consider the test case added in this PR:
Without this fix, the
RUNstep skips cache completely when running with--cache-fromand--cache-to. Cache is neither pulled nor pushed for this step, even when theCOPYinstage1is cached. This seems to happen becausestage1has thedidExecuteflag set, regardless of whether the stage was cached or not.Special notes for your reviewer:
It's very possible I'm misunderstanding something, but I believe the
RUNstep in test case I've added wrongly skips cache, and I'd appreciate some pointers in the right direction if what I've proposed here isn't the right solution.Does this PR introduce a user-facing change?
None