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

support container to container copy #11049

Merged
merged 2 commits into from
Jul 27, 2021
Merged

support container to container copy #11049

merged 2 commits into from
Jul 27, 2021

Conversation

vrothberg
Copy link
Member

Implement container to container copy. Previously data could only be
copied from/to the host.

Fixes: #7370
Co-authored-by: Mehul Arora [email protected]
Signed-off-by: Valentin Rothberg [email protected]

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 26, 2021
@vrothberg
Copy link
Member Author

@containers/podman-maintainers PTAL

rhatdan
rhatdan approved these changes Jul 26, 2021
View reviewed changes
cmd/podman/containers/cp.go Show resolved Hide resolved
cmd/podman/containers/cp.go Show resolved Hide resolved
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 26, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, vrothberg

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

@edsantiago
Copy link
Member

The tests look good, but I wonder if they could be cut down a little? Each one takes over ten minutes (and the cp test is already ultra-long). Twenty minutes seems like a big cost to add to every future CI run.

@edsantiago
Copy link
Member

It looks like the ten-minute problem is caused by #10701: there are a lot of podman execs in these tests, and each one takes 25-30 seconds.

@vrothberg
Copy link
Member Author

vrothberg commented Jul 27, 2021 •
edited
Loading

It looks like the ten-minute problem is caused by #10701: there are a lot of podman execs in these tests, and each one takes 25-30 seconds.

Yes, the tests are noticeably slow. They used to run much faster. I have absolutely no idea what may be the cause, so I'll try bisecting.

@vrothberg
Copy link
Member Author

Yes, the tests are noticeably slow. They used to run much faster. I have absolutely no idea what may be the cause, so I'll try bisecting.

I do not notice a difference between Podman v3.0.1-rhel and v3.2.3 on my machine. Could it be conmon?

@vrothberg
Copy link
Member Author

Yes, the tests are noticeably slow. They used to run much faster. I have absolutely no idea what may be the cause, so I'll try bisecting.

I do not notice a difference between Podman v3.0.1-rhel and v3.2.3 on my machine. Could it be conmon?

Scratch that. Found a reproducer and the difference is very noticeable.

@vrothberg
Copy link
Member Author

I stop spamming here now. Let's continue over in #10701.

I think we should wait for #10701 to be fixed before merging this PR.

@vrothberg
Copy link
Member Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2021
@vrothberg
Copy link
Member Author

With the commit from #11055, the execution time of the cp tests on my machine dropped from ~8 min down to 3min 40sec.

I will still look into reducing the number of tests a bit.

Mehul Arora and others added 2 commits July 27, 2021 15:32
@vrothberg
support container to container copy
6fe03b2 
Implement container to container copy.  Previously data could only be
copied from/to the host.

Fixes: containers#7370
Co-authored-by: Mehul Arora <[email protected]>
Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg
cp system tests: reduce number of exec's
0aec93e 
Reduce the amount of `podman exec`s in the cp system tests.
Exec is expensive and a number of them could easily be combined
into the container command.

This cuts down the costs of running the tests by around 25 percent
on my local machine.

Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg
Copy link
Member Author

Rebased on top of the exec fixes. I also took a shot at reducing the exec's in the cp tests and cut down the execution time by around 25 percent on my machine.

@vrothberg vrothberg mentioned this pull request Jul 27, 2021
Closed
@edsantiago
Copy link
Member

edsantiago commented Jul 27, 2021 •
edited
Loading

The exec changes cut test time on my laptop from 3:46 to 3:36 (4% improvement). How did you get 25%? Are you comparing to old-slow-broken-exec time? (Edit: not that I'm complaining! Shaving 10 seconds from CI makes me happy!)

@vrothberg
Copy link
Member Author

The exec changes cut test time on my laptop from 3:46 to 3:36 (4% improvement). How did you get 25%? Are you comparing to old-slow-broken-exec time?

Old & slow: > 8min
This PR without cutting down execs: 3min 40sec
This with with cutting down execs: 2min 50sec

@edsantiago
Copy link
Member

LGTM!

@rhatdan
Copy link
Member

rhatdan commented Jul 27, 2021

/lgtm
Thanks @infiniteregrets and @vrothberg

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2021
@rhatdan
Copy link
Member

rhatdan commented Jul 27, 2021

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2021
@openshift-merge-robot openshift-merge-robot merged commit a5de831 into containers:main Jul 27, 2021
@vrothberg vrothberg deleted the fix-7370 branch July 28, 2021 07:40
@vrothberg vrothberg mentioned this pull request Jul 28, 2021
Merged
vrothberg added a commit to vrothberg/libpod that referenced this pull request Jul 28, 2021
@vrothberg
cp: consolidate and simplify
0132670 
Consolidate and simplify code in `podman cp` a bit.  PR containers#11049
introduced some code duplicates that were worth tackling.

[NO TESTS NEEDED]

Signed-off-by: Valentin Rothberg <[email protected]>
@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 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 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

@rhatdan rhatdan rhatdan approved these changes

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.

support container-to-container copy
5 participants
@vrothberg @edsantiago @rhatdan @TomSweeneyRedHat @openshift-merge-robot