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

support container to container copy #10728

Closed
wants to merge 3 commits into from
Closed

support container to container copy #10728

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c31cc84
support container to container copy
Jun 19, 2021
f0a0b12
Merge branch 'containers:master' into c2c-copy
infiniteregrets Jun 25, 2021
67cbb74
fix bugs
Jun 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 53 additions & 1 deletion cmd/podman/containers/cp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,14 @@ func cp(cmd *cobra.Command, args []string) error {
return err
}

if len(sourceContainerStr) > 0 {
if len(sourceContainerStr) > 0 && len(destContainerStr) > 0 {
return copyContainerToContainer(sourceContainerStr, sourcePath, destContainerStr, destPath)
} else if len(sourceContainerStr) > 0 {
return copyFromContainer(sourceContainerStr, sourcePath, destPath)
}

return copyToContainer(destContainerStr, destPath, sourcePath)

}

// containerMustExist returns an error if the specified container does not
Expand Down Expand Up @@ -113,6 +116,55 @@ func doCopy(funcA func() error, funcB func() error) error {
return errorhandling.JoinErrors(copyErrors)
}

func copyContainerToContainer(fromContainer string, sourcePath string, toContainer string, destPath string) error {
if err := containerMustExist(fromContainer); err != nil {
return err
}

if err := containerMustExist(toContainer); err != nil {
return err
}

fromContainerInfo, err := registry.ContainerEngine().ContainerStat(registry.GetContext(), fromContainer, sourcePath)
if err != nil {
return errors.Wrapf(err, "%q could not be found on container %s", sourcePath, fromContainer)
}

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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@infiniteregrets infiniteregrets Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@vrothberg vrothberg Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@infiniteregrets infiniteregrets Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

@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

}

fromContainerTarget, toContainerTarget := fromContainerInfo.LinkTarget, toContainerInfo.LinkTarget
reader, writer := io.Pipe()

fromContainerCopy := func() error {
Copy link
Contributor

@cdoern cdoern Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

defer writer.Close()
copyFunc, err := registry.ContainerEngine().ContainerCopyToArchive(registry.GetContext(), fromContainer, fromContainerTarget, writer)
if err != nil {
return err
}
if err := copyFunc(); err != nil {
return errors.Wrap(err, "error copying from container")
}
return nil
}

toContainerCopy := func() error {
defer reader.Close()
copyFunc, err := registry.ContainerEngine().ContainerCopyFromArchive(registry.GetContext(), toContainer, toContainerTarget, reader)
if err != nil {
return err
}
if err := copyFunc(); err != nil {
return errors.Wrap(err, "error copying to container")
}
return nil
}

return doCopy(fromContainerCopy, toContainerCopy)
}

// copyFromContainer copies from the containerPath on the container to hostPath.
func copyFromContainer(container string, containerPath string, hostPath string) error {
if err := containerMustExist(container); err != nil {
Expand Down
12 changes: 0 additions & 12 deletions pkg/copy/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,6 @@ func ParseSourceAndDestination(source, destination string) (string, string, stri
sourceContainer, sourcePath := parseUserInput(source)
destContainer, destPath := parseUserInput(destination)

numContainers := 0
if len(sourceContainer) > 0 {
numContainers++
}
if len(destContainer) > 0 {
numContainers++
}

if numContainers != 1 {
return "", "", "", "", errors.Errorf("invalid arguments %q, %q: exactly 1 container expected but %d specified", source, destination, numContainers)
}

if len(sourcePath) == 0 || len(destPath) == 0 {
return "", "", "", "", errors.Errorf("invalid arguments %q, %q: you must specify paths", source, destination)
}
Expand Down