mount,cache: internal lockfiles must not be part of user's cache content
#4349
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rhatdan
reviewed
Oct 18, 2022
internal/parse/parse.go
Outdated
|
|
||
| // create a subdirectory inside `cacheParent` just to store lockfiles | ||
| buildahLockFilesDir = filepath.Join(cacheParent, buildahLockFilesDir) | ||
| err = os.MkdirAll(buildahLockFilesDir, os.FileMode(0755)) |
There was a problem hiding this comment.
Does this lockdir need to be shared between different users? Should it be 0700? Does it matter?
There was a problem hiding this comment.
@rhatdan You are correct 0700 is better here.
flouthoc
force-pushed
the
fix-cache-locked
branch
from
9f87a20 to
f4b4d87
Compare
|
LGTM |
giuseppe
approved these changes
Oct 19, 2022
internal/parse/parse.go
Outdated
| @@ -33,6 +33,9 @@ const ( | |||
| BuildahCacheDir = "buildah-cache" | |||
| // mount=type=cache allows users to lock a cache store while its being used by another build | |||
| BuildahCacheLockfile = "buildah-cache-lockfile" | |||
| // All the lockfiles are stored in a seperate directory inside `BuildahCacheDir` | |||
`--mount=type=cache` must not add internal lockfiles to cache directory created by users instead store it in a different central directory with path as `/base/buildah-cache/buildah-lockfiles`. There are use-cases where users can wipe cache between the builds so lockfiles will be removed in unexpected manner and also its not okay to mix buildah's internal construct with user's cache content. Helps in: containers#4342 Signed-off-by: Aditya R <[email protected]>
Single `RUN` can contain multiple `--mount` commands so lets append into `lockedTargets` so we collect `lockfiles` from all the `--mount` instructions. Helps in: containers#4342 Signed-off-by: Aditya R <[email protected]>
flouthoc
force-pushed
the
fix-cache-locked
branch
from
f4b4d87 to
7ec6529
Compare
Use-cases as shown in below containerfile cleans cache between the
builds, in previous commits we have ensured that buildah lockfiles will
not be part of user's cache content hence following use-case must paas
```
FROM quay.io/centos/centos:7
ARG WIPE_CACHE
RUN --mount=type=cache,target=/cache1,sharing=locked \
--mount=type=cache,target=/cache2 \
set -ex; \
ls -l /cache1; \
if [[ -v WIPE_CACHE ]]; then \
>&2 echo "Wiping cache"; \
find /cache1 -mindepth 1 -delete; \
fi; \
echo "foo" > /cache1/foo.txt; \
ls -l /cache1; \
chmod --recursive g=u /cache1; \
: ;
RUN --mount=type=cache,target=/cache1,sharing=locked \
>&2 echo "Never get here"
```
Closes: containers#4342
Signed-off-by: Aditya R <[email protected]>
flouthoc
force-pushed
the
fix-cache-locked
branch
from
7ec6529 to
ffb2f27
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, giuseppe, 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 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following PR introduces two change
--mount=type=cachemust not add internal lockfiles to cache directorycreated by users instead store it in a different central directory with
path as
/base/buildah-cache/buildah-lockfiles.There are use-cases where users can wipe cache between the builds so
lockfiles will be removed in unexpected manner and also its not okay to
mix buildah's internal construct with user's cache content.
run: Single
RUNcan contain multiple--mountcommands so lets append intolockedTargetsso we collectlockfilesfrom all the--mountinstructions.
Tryout example
What type of PR is this?
What this PR does / why we need it:
How to verify it
Newly added
integrationtest and old tests must not fail.Which issue(s) this PR fixes:
Closes: #4342
Special notes for your reviewer:
Does this PR introduce a user-facing change?