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

build: Add support for buildkit like --mount=type=bind #3548

Merged
merged 1 commit into from Oct 6, 2021
Merged

build: Add support for buildkit like --mount=type=bind #3548

merged 1 commit into from Oct 6, 2021

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Sep 28, 2021 •
edited

Following commit adds support for using buildkit like --mount=type=bind with RUN statements.
Mounts created by --mount are transient in nature and only scoped to current RUN statements just like buildkit.

Example

FROM alpine
RUN --mount=type=bind,source=.,target=/mnt cat /mnt/input_file

PS: docs already have entry for this but it was never implemented.

Closes: #3217

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 28, 2021

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

@containers/buildah-maintainers @giuseppe @rhatdan @nalind @vrothberg PTAL

nalind
nalind reviewed Sep 28, 2021
View reviewed changes
buildah.go Outdated Show resolved Hide resolved
TomSweeneyRedHat
TomSweeneyRedHat reviewed Sep 28, 2021
View reviewed changes
run_linux.go Outdated Show resolved Hide resolved
@flouthoc flouthoc marked this pull request as draft September 28, 2021 20:12
@flouthoc flouthoc force-pushed the buildkit-mount-bind branch 3 times, most recently from 8c78cd0 to aedfcb8 Compare September 29, 2021 07:06
@flouthoc flouthoc marked this pull request as ready for review September 29, 2021 07:08
@TomSweeneyRedHat
Copy link
Member

All kinds of test unhappiness @flouthoc

@flouthoc
Copy link
Collaborator Author

flouthoc commented Sep 29, 2021 •
edited

@TomSweeneyRedHat I think its quay flakes

Edit: Some passed, some still flaky

@flouthoc flouthoc force-pushed the buildkit-mount-bind branch 3 times, most recently from 084a36c to dbe2b46 Compare September 30, 2021 08:42
@flouthoc
Copy link
Collaborator Author

resolved conflicts after recent commits in main

TomSweeneyRedHat
TomSweeneyRedHat reviewed Sep 30, 2021
View reviewed changes
tests/bud.bats Outdated Show resolved Hide resolved
@flouthoc
Copy link
Collaborator Author

flouthoc commented Oct 1, 2021

@cevich @containers/buildah-maintainers I am not sure but looks like something is up with CI for Conformance test its failing for all the PR's with error Package containerd.io is not installed

nalind
nalind reviewed Oct 1, 2021
View reviewed changes
run_linux.go Outdated Show resolved Hide resolved
nalind
nalind reviewed Oct 1, 2021
View reviewed changes
run_linux.go Outdated Show resolved Hide resolved
nalind
nalind reviewed Oct 1, 2021
View reviewed changes
pkg/parse/parse.go Outdated Show resolved Hide resolved
nalind
nalind reviewed Oct 1, 2021
View reviewed changes
pkg/parse/parse.go Outdated Show resolved Hide resolved
nalind
nalind reviewed Oct 1, 2021
View reviewed changes
run_linux.go Outdated Show resolved Hide resolved
@cevich
Copy link
Member

cevich commented Oct 1, 2021

Opened #3560 to fix CI

@flouthoc
Copy link
Collaborator Author

flouthoc commented Oct 1, 2021

@nalind Added requested tests, I still need to implement rw for chroot isolation.

nalind
nalind reviewed Oct 1, 2021
View reviewed changes
run.go Outdated Show resolved Hide resolved
@flouthoc
Copy link
Collaborator Author

flouthoc commented Oct 1, 2021

@nalind I think rw is already working properly for isolated chroot i have also written a test for rw on isolated chroot. With rw enabled user can write on bind files and changes are also reflected on host.

@flouthoc flouthoc requested a review from nalind October 1, 2021 21:20
@flouthoc flouthoc force-pushed the buildkit-mount-bind branch 2 times, most recently from f27ab79 to 11f7f93 Compare October 1, 2021 21:25
@flouthoc
Copy link
Collaborator Author

flouthoc commented Oct 1, 2021

Rebasing after #3562

@rhatdan
Copy link
Member

rhatdan commented Oct 4, 2021

LGTM

@flouthoc flouthoc force-pushed the buildkit-mount-bind branch 2 times, most recently from 6f01322 to 63a27a3 Compare October 6, 2021 06:02
@flouthoc
build: Add support for buildkit like --mount=type=bind
f0d3140 
Following commit adds support for using buildkit like
`--mount=type=bind` with `RUN` statements. Mounts created by `--mount`
are transient in nature and only scoped to current RUN statements.

Signed-off-by: Aditya Rajan <arajan@redhat.com>
nalind
nalind reviewed Oct 6, 2021
View reviewed changes
pkg/parse/parse.go
case "readonly":
// Alias for "ro"
newMount.Options = append(newMount.Options, "ro")
case "shared", "rshared", "private", "rprivate", "slave", "rslave", "Z", "z":
case "shared", "rshared", "private", "rprivate", "slave", "rslave", "Z", "z", "U":
Copy link
Member

Choose a reason for hiding this comment

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

Do we have test coverage for the "U" flag when used here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nalind Mount code is being reused from volumes and tests for U is already added here https://github.com/containers/buildah/blob/main/tests/run.bats#L304 therefore I refrained from adding ownership test to prevent redundancy

@nalind
Copy link
Member

nalind commented Oct 6, 2021

Question about test coverage of the "U" flag, otherwise LGTM.

@flouthoc flouthoc requested a review from nalind October 6, 2021 18:03
@rhatdan
Copy link
Member

rhatdan commented Oct 6, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 6, 2021
@openshift-merge-robot openshift-merge-robot merged commit 2f4c517 into containers:main Oct 6, 2021
@flouthoc flouthoc mentioned this pull request Oct 7, 2021
Closed
@flouthoc flouthoc mentioned this pull request Dec 13, 2021
Merged
@guillaume-d guillaume-d mentioned this pull request Apr 29, 2022
Open
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@nalind nalind Awaiting requested review from nalind

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.

RUN --mount=type=bind doesn't seem to work with relative source paths
6 participants
@flouthoc @TomSweeneyRedHat @cevich @rhatdan @nalind @openshift-merge-robot