support container to container copy #10728

infiniteregrets · 2021-06-19T06:14:42Z

This would fix #7370. I haven't done anything yet, but this seems to work:

[mehul@fedora bin]$ ./podman container exec 59aef1582431 /bin/sh -c 'mkdir test'
[mehul@fedora bin]$ ./podman container cp ffae1af127e0:/tmp 59aef1582431:/test
[mehul@fedora bin]$ ./podman container exec 59aef1582431 /bin/sh -c 'cd test && ls'
tmp

I am missing some cases where a file to file copy would occur or if the path is a symbolic link and read/write should be recursive. Once I am sure about how everything works I will fix that. I think checking whether a path exists on a container and getting containerinfo can be speeded up by doing so in a separate thread

I am unable to test much locally because my laptop gets really hot and laggy, so I am just pushing this here.
Thanks!
Signed-off-by: Mehul Arora [email protected]

Signed-off-by: Mehul Arora <[email protected]>

rhatdan · 2021-06-20T09:43:04Z

You will need to add tests for this.

@vrothberg PTAL

vrothberg

Thanks!

Yes, we need a lot of tests for this one. Look at the tests in https://github.com/containers/podman/blob/master/test/system/065-cp.bats which have a decent coverage of all corner cases and the basic copy functionality.

vrothberg · 2021-06-21T07:21:38Z

cmd/podman/containers/cp.go

+
+	toContainerInfo, err := registry.ContainerEngine().ContainerStat(registry.GetContext(), toContainer, destPath)
+	if err != nil {
+		return errors.Wrapf(err, "%q could not be found on container %s", destPath, toContainer)


That should not be an error per-se. If I do cp 1:/foo.txt 2:/foo.txt then "foo.txt" should be created at the destination. Have a look at the copy functions above for the wiring.

I didn't understand why we are checking for this condition here: https://github.com/containers/podman/blob/master/cmd/podman/containers/cp.go#L266
If possible, could you please give an example

If we do a cp 1:/dir 2:/tmp/foo/ foo must exist. If we do a cp 1:/dir 2:/tmp/foo foo would be created if it doesn't exist.

The condition enforces that rule. The podman-cp man pages mention these rules.

Thanks for adding those comments btw, they are very helpful! I will work on the missing cases soon

Happy to help! Feel free to ping me here on IRC at any time. I am reachable during regular CEST working hours.

@vrothberg a bit confused about git, how do I get the changes you made to the branch on my fork?
would it be like checking out to main and then pulling the changes and checking out to my branch c2c-copy and then merging main to my branch?
wouldn't that mean losing the changes I made?

What I usually do is the following (upstream is the remote branch pointing to github.com/containers/podman):

$ git remote update $ git pull --rebase upstream master

Note that while rebasing you may have to resolve merge conflicts.

been on this for hours now, cant seem to figure this. I was able to update the c2c-copy branch to the recent changes on podman but the changes you made yesterday don't reflect? even git log shows your merged PR
And surprisingly, I did not get any merge conflicts?

I think I might have to just re-add those changes and continue

Nevermind, I figured it out. Didn't have to do anything manually

@vrothberg so I am not sure how we will go about renaming files for container-container copy in a case like:
podman container cp distracted_tesla:ok.txt pedantic_hertz:ok.txt
In host->container or container->host I can see that the buildah copier package is handling that for us but how do we do this for this case?

I was able to fix the segmentation fault

[mehul@fedora bin]$ ./podman cp c1:file.txt c2:/skbxlswx [mehul@fedora bin]$ ./podman exec c2 /bin/sh -c "ls" bin dev etc file.txt home proc root run sys tmp usr var

openshift-ci · 2021-06-25T10:53:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: infiniteregrets
To complete the pull request process, please assign saschagrunert after the PR has been reviewed.
You can assign the PR to them by writing /assign @saschagrunert in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Mehul Arora <[email protected]>

cdoern · 2021-06-25T19:37:30Z

cmd/podman/containers/cp.go

+		return errors.Wrapf(err, "%q could not be found on container %s", sourcePath, fromContainer)
+	}
+
+	var toContainerBaseName string


can you change to toContainerBaseName := "" removes an unnecessary line.

Also, I am seeing you are getting an ineffectual assignment error here. Where are you using toContainerBaseName after setting it?

can you change to toContainerBaseName := "" removes an unnecessary line.

sure, will make the changes once I figure everything out

Also, I am seeing you are getting an ineffectual assignment error here. Where are you using toContainerBaseName after setting it?

I am not completely sure tbh, I had to do _ = toContainerBaseName to get rid of that unused variable error

ok @infiniteregrets you're getting that error because you set the base name and never actually use it. the _ = is just a clever way to trick golang. You should use that name somewhere in your copy code.

@cdoern I am using it?

if toContainerInfo != nil { toContainerBaseName = filepath.Base(toContainerInfo.LinkTarget) } else { toContainerBaseName = filepath.Base(destPath) }

This looks like a shadowing issue? Im not sure but I'll try to figure it. Thanks!

cdoern · 2021-06-25T19:45:46Z

cmd/podman/containers/cp.go

+	}
+	reader, writer := io.Pipe()
+
+	fromContainerCopy := func() error {


potentially could consolidate toContainerCopy and fromContainercopy into a function you call twice that takes specific parameters. Seems slightly redundant to have nearly identical code twice.

there might be some changes to the functions, so depending on what the final outcome looks like i'll refactor the code

vrothberg · 2021-06-30T15:12:31Z

Please create a new PR against the new main branch. @rhatdan, the master->main migration closes on PRs against master.

support container to container copy

c31cc84

Signed-off-by: Mehul Arora <[email protected]>

vrothberg reviewed Jun 21, 2021

View reviewed changes

Merge branch 'containers:master' into c2c-copy

f0a0b12

fix bugs

67cbb74

Signed-off-by: Mehul Arora <[email protected]>

cdoern reviewed Jun 25, 2021

View reviewed changes

rhatdan closed this Jun 30, 2021

rhatdan deleted the branch containers:master June 30, 2021 15:09

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 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support container to container copy #10728

support container to container copy #10728

infiniteregrets commented Jun 19, 2021

rhatdan commented Jun 20, 2021

vrothberg left a comment

vrothberg Jun 21, 2021

infiniteregrets Jun 22, 2021 •

edited

Loading

vrothberg Jun 22, 2021 •

edited

Loading

infiniteregrets Jun 22, 2021

vrothberg Jun 23, 2021

infiniteregrets Jun 24, 2021

vrothberg Jun 25, 2021

infiniteregrets Jun 25, 2021 •

edited

Loading

infiniteregrets Jun 25, 2021

infiniteregrets Jun 25, 2021

openshift-ci bot commented Jun 25, 2021

cdoern Jun 25, 2021

cdoern Jun 25, 2021

infiniteregrets Jun 25, 2021

cdoern Jun 25, 2021

infiniteregrets Jun 26, 2021

cdoern Jun 25, 2021 •

edited

Loading

infiniteregrets Jun 25, 2021

vrothberg commented Jun 30, 2021

support container to container copy #10728

support container to container copy #10728

Conversation

infiniteregrets commented Jun 19, 2021

rhatdan commented Jun 20, 2021

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infiniteregrets Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

vrothberg Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infiniteregrets Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdoern Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg commented Jun 30, 2021

infiniteregrets Jun 22, 2021 •

edited

Loading

vrothberg Jun 22, 2021 •

edited

Loading

infiniteregrets Jun 25, 2021 •

edited

Loading

cdoern Jun 25, 2021 •

edited

Loading