Skip to content

buildkit: --mount-type=cache must support locking external cache store. #3820

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
Merged
openshift-merge-robot merged 2 commits into from
Mar 22, 2022
Merged

buildkit: --mount-type=cache must support locking external cache store. #3820

openshift-merge-robot merged 2 commits into from
Mar 22, 2022

Conversation

@flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Mar 14, 2022

A shared cache on host must support locking so other parallel/concurrent builds
will wait for current executing RUN statement to finish.

  • Locks the cache store as soon as RUN is triggered.
  • Locked target is added to cleanup list so it can be unlocked as soon
    as RUN step is completed.

Usage

RUN --mount=type=cache,target=/test,sharing=locked

Closes: #3815

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 14, 2022

[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

Details 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 force-pushed the mount-cache-sharing branch from e676815 to 6c763c1 Compare March 14, 2022 12:00
rhatdan
rhatdan reviewed Mar 15, 2022
View reviewed changes
@flouthoc flouthoc force-pushed the mount-cache-sharing branch from 6c763c1 to 09cc2b2 Compare March 15, 2022 10:47
nalind
nalind reviewed Mar 15, 2022
View reviewed changes
nalind
nalind reviewed Mar 15, 2022
View reviewed changes
nalind
nalind reviewed Mar 15, 2022
View reviewed changes
nalind
nalind reviewed Mar 15, 2022
View reviewed changes
nalind
nalind reviewed Mar 15, 2022
View reviewed changes
@flouthoc flouthoc force-pushed the mount-cache-sharing branch 2 times, most recently from 74234a8 to ae657ec Compare March 15, 2022 19:08
nalind
nalind reviewed Mar 15, 2022
View reviewed changes
@flouthoc flouthoc force-pushed the mount-cache-sharing branch from ae657ec to bc57835 Compare March 15, 2022 19:22
@flouthoc flouthoc requested a review from nalind March 15, 2022 19:25
@flouthoc flouthoc force-pushed the mount-cache-sharing branch from bc57835 to bba97f0 Compare March 15, 2022 19:27
@flouthoc
Copy link
Collaborator Author

flouthoc commented Mar 15, 2022

@nalind PTAL fixed requested issues.

nalind
nalind reviewed Mar 15, 2022
View reviewed changes
nalind
nalind reviewed Mar 15, 2022
View reviewed changes
pkg/parse/parse.go Outdated
// GetVolumes gets the volumes from --volume and --mount
func GetVolumes(ctx *types.SystemContext, store storage.Store, volumes []string, mounts []string, contextDir string) ([]specs.Mount, []string, error) {
unifiedMounts, mountedImages, err := getMounts(ctx, store, mounts, contextDir)
func GetVolumes(ctx *types.SystemContext, store storage.Store, volumes []string, mounts []string, contextDir string) ([]specs.Mount, []string, []string, error) {
Copy link
Member

@nalind nalind Mar 15, 2022

Choose a reason for hiding this comment

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

I'm not sure we want to keep GetVolumes() in the public API if we're going to have to change its signature when we add new volume features.

Copy link
Collaborator Author

@flouthoc flouthoc Mar 15, 2022

Choose a reason for hiding this comment

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

@nalind I'm also unsure about what to do here. Maybe we can add a new function which is not exposed and mark this as to-be deprecated ?

Copy link
Member

@nalind nalind Mar 15, 2022

Choose a reason for hiding this comment

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

I don't think this function gets called from outside of this repository, so no need to wait there.

Copy link
Collaborator Author

@flouthoc flouthoc Mar 16, 2022

Choose a reason for hiding this comment

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

Moved it to internal/parse/parse.go.

@flouthoc flouthoc force-pushed the mount-cache-sharing branch 2 times, most recently from 724cc80 to 66cd081 Compare March 16, 2022 07:16
TomSweeneyRedHat
TomSweeneyRedHat reviewed Mar 16, 2022
View reviewed changes
cmd/buildah/run.go Outdated
// since LockedTargets must contain list of all locked files
// don't break here since we need to unlock other files but
// log so user can take a look
logrus.Errorf("Lockfile %q was expected here, stat failed with %v", path, err)
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat Mar 16, 2022

Choose a reason for hiding this comment

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

Is there anything a user could do at this point to correct this? Or support for that matter?

Copy link
Collaborator Author

@flouthoc flouthoc Mar 17, 2022

Choose a reason for hiding this comment

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

@TomSweeneyRedHat No user cannot do anything but I think this is worth logging cause following case is never expected to happen and if it happens it could be a symptom of a bigger problem in our fs locking library or anywhere above that.

Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat Mar 17, 2022

Choose a reason for hiding this comment

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

That's what I thought @flouthoc . I was kind of waffling about whether this should be a debug or not, but probably better to keep it as an error.

Copy link
Member

@rhatdan rhatdan Mar 19, 2022

Choose a reason for hiding this comment

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

Errors should cause the app to exit, if this does not then this should be a warning.

@TomSweeneyRedHat
Copy link
Member

TomSweeneyRedHat commented Mar 16, 2022

LGTM, but would like another head nod.

@flouthoc
Copy link
Collaborator Author

flouthoc commented Mar 17, 2022

@rhatdan @nalind PTAL

rhatdan
rhatdan reviewed Mar 19, 2022
View reviewed changes
rhatdan
rhatdan reviewed Mar 19, 2022
View reviewed changes
@flouthoc flouthoc force-pushed the mount-cache-sharing branch from 66cd081 to efab259 Compare March 21, 2022 04:30
flouthoc added 2 commits March 21, 2022 10:02
@flouthoc
buildkit: mount=type=cache support locking external cache store
a4c5706 
A shared cache on host must support locking so other parallel/concurrent builds
will wait for current executing RUN statement to finish.

* Locks the cache store as soon as RUN is triggered.
* Locked target is added to cleanup list so it can be unlocked as soon
  as RUN step is completed.

Signed-off-by: Aditya R <arajan@redhat.com>
@flouthoc
parse: convert exposed GetVolumes to internal only
843cbe7 
Implementation of `GetVolumes` seems to be changing with addition of new
features hence moving it from exposed parse package to internal parse
package and move needed helpers with it.

Signed-off-by: Aditya R <arajan@redhat.com>
@flouthoc flouthoc force-pushed the mount-cache-sharing branch from efab259 to 843cbe7 Compare March 21, 2022 04:32
@flouthoc
Copy link
Collaborator Author

flouthoc commented Mar 21, 2022

@rhatdan @TomSweeneyRedHat @nalind PTAL. This should be good i guess, resolved all the feedback points.

@TomSweeneyRedHat
Copy link
Member

TomSweeneyRedHat commented Mar 22, 2022

@flouthoc, you might need a rebase

@TomSweeneyRedHat
Copy link
Member

TomSweeneyRedHat commented Mar 22, 2022

LGTM other than the possible rebase.

@rhatdan
Copy link
Member

rhatdan commented Mar 22, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 22, 2022
@openshift-merge-robot openshift-merge-robot merged commit 625c7a9 into containers:main Mar 22, 2022
@flouthoc flouthoc mentioned this pull request Mar 23, 2022
Closed
@mtrmac mtrmac mentioned this pull request Oct 19, 2022
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@nalind nalind nalind left review comments

@rhatdan rhatdan rhatdan left review comments

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

Assignees

@rhatdan rhatdan

Labels

approved buildkit lgtm locked - please file new issue/PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support for buildkit-style sharing option with RUN --mount=type=cache

5 participants

@flouthoc @TomSweeneyRedHat @rhatdan @nalind @openshift-merge-robot