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

podman copy files to the volume with a container #3094

Merged
merged 1 commit into from
Jun 15, 2019
Merged

podman copy files to the volume with a container #3094

merged 1 commit into from
Jun 15, 2019

Conversation

QiWang19
Copy link
Contributor

@QiWang19 QiWang19 commented May 9, 2019 •
edited
Loading

Close #3059
enabls podman to copy files between the host machine and the volume related to a container.

Signed-off-by: Qi Wang [email protected]

@openshift-ci-robot openshift-ci-robot requested review from baude and mheon May 9, 2019 18:23
@mheon
Copy link
Member

mheon commented May 9, 2019

This does not fully solve the problem as it was written. It works for named volumes only, and not directories bind-mounted in via --volume and --mount type=bind, and also not for tmpfs mounts (though I'm not sure if Docker can handle those either - needs checking)

@QiWang19
Copy link
Contributor Author

QiWang19 commented May 9, 2019

hm, 👀 will check that

@QiWang19
Copy link
Contributor Author

docker cp doesn't work for tmpfs.
I made changes to support named volumes, --mount volume, type=bind.

mheon
mheon reviewed May 13, 2019
View reviewed changes
Copy link
Member

@mheon mheon left a comment

Choose a reason for hiding this comment

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

I am very worried about the path logic here - I'm looking, and I don't think it holds up for any nontrivial path.

You might want to look into whether there are libraries that can do advanced manipulation like this, matching and trimming path prefixes.

cmd/podman/cp.go Outdated
if path == "" {
return false, specs.Mount{}
}
path = strings.Split(path, separator)[0]
Copy link
Member

Choose a reason for hiding this comment

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

Wait, how does this work? We split the original path on separators, then we compare it against the entire path of the volume (minus leading slash)? This doesn't seem like it makes sense.

Say I have two volumes, /tmp/test/a and /tmp/test/b, and I want to copy into /tmp/test/b - what's to prevent this from matching /tmp/test/a here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mheon, changed the path match logic func matchVolumePath(path, target string) bool, PTAL

cmd/podman/cp.go
return false, specs.Mount{}
}
path = strings.Split(path, separator)[0]
for _, m := range ctr.Config().Spec.Mounts {
Copy link
Member

Choose a reason for hiding this comment

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

You also need to check if the mount is actually a bind mount

Copy link
Member

Choose a reason for hiding this comment

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

m.Type == "bind"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@QiWang19 QiWang19 force-pushed the cpvol branch 2 times, most recently from d66237e to e4fdaee Compare May 13, 2019 19:33
mheon
mheon reviewed May 14, 2019
View reviewed changes
cmd/podman/cp.go
return false, specs.Mount{}
}

func matchVolumePath(path, target string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

You probably want to do a path.Clean() on both paths first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed. PTAL @mheon

@QiWang19 QiWang19 force-pushed the cpvol branch 2 times, most recently from 1cfd775 to b7d0dce Compare May 17, 2019 15:01
@mheon
Copy link
Member

mheon commented May 21, 2019

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, QiWang19

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2019
@rhatdan
Copy link
Member

rhatdan commented May 22, 2019

LGTM

@TomSweeneyRedHat
Copy link
Member

LGTM, would like to get a @mheon head nod.

@QiWang19 QiWang19 mentioned this pull request May 29, 2019
Closed
@mheon
Copy link
Member

mheon commented May 29, 2019

Hold this until the CVE fix lands

@mheon
Copy link
Member

mheon commented May 29, 2019

I need to re-review once the CVE is fixed to make sure we don't regress

@QiWang19 QiWang19 changed the title podman copy files to the volume with a container [WIP] podman copy files to the volume with a container May 29, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 29, 2019
@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #3214) made this pull request unmergeable. Please resolve the merge conflicts.

@mheon
Copy link
Member

mheon commented May 30, 2019

CVE fix is landed. Rebase and we can re-review

@QiWang19 QiWang19 changed the title [WIP] podman copy files to the volume with a container podman copy files to the volume with a container May 30, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 30, 2019
@QiWang19
Copy link
Contributor Author

@mheon PTAL

@mheon
Copy link
Member

mheon commented Jun 3, 2019

@QiWang19 Mind rebasing to fix CI?

@QiWang19
podman copy files to the volume with a container
c1b4060 
enabls podman to cpoy files between the host machine and the volume related with a container.
Close containers#3059

Signed-off-by: Qi Wang <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Jun 8, 2019

@mheon I think this is ready to go in?

@rhatdan
Copy link
Member

rhatdan commented Jun 15, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2019
@openshift-merge-robot openshift-merge-robot merged commit 670fc03 into containers:master Jun 15, 2019
@rh-atomic-bot rh-atomic-bot mentioned this pull request Jun 15, 2019
Closed
@QiWang19 QiWang19 deleted the cpvol branch June 26, 2020 15:11
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mheon mheon mheon left review comments

@baude baude Awaiting requested review from baude

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

podman cp: not working with volumes
7 participants
@QiWang19 @mheon @openshift-ci-robot @rhatdan @TomSweeneyRedHat @rh-atomic-bot @openshift-merge-robot