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.

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

fix podman cp can create an extra directory when the source is the container's root directory #6601

Merged

Conversation

zhangguanzhang
Copy link
Collaborator

@zhangguanzhang zhangguanzhang commented Jun 14, 2020

(Edited, moving comment from @zhangguanzhang to here)

Fixes: #6596
Example of the issue:

$ mkdir test
$ ls
drwxr-xr-x  2 root root         6 Jun 15 16:54 test
$ cat >Dockerfile<<EOF
FROM scratch
COPY Dockerfile /
EOF
$ podman build -t podman-test:1.0.0 .
STEP 1: FROM scratch
STEP 2: COPY Dockerfile /
STEP 3: COMMIT podman-test:1.0.0
--> 839962c7269
839962c7269777d5f50d68428d5def260b6842f7c0607978b8567668f2a8c067

$ podman create podman-test:1.0.0 dummy
53914c7a1191e96b7fee0fcb9b152480640c392321edadfe657c10afda1431a2
$ podman cp 539:/   non_dir
$ ls non_dir
Dockerfile

If you copy the container's root directory to an existing directory, It creates an extra layer of the directory called merged

$ podman cp 539:/ test
$ ls -l  test
total 0
drwxr-xr-x 2 root root 24 Jun 15 23:30 merged
$ ls -l  test/merged/
total 4
-rw-r--r-- 1 root root 31 Jun 13 22:50 Dockerfile

Signed-off-by: zhangguanzhang [email protected]

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 14, 2020
@openshift-ci-robot
Copy link
Collaborator

Hi @zhangguanzhang. Thanks for your PR.

I'm waiting for a containers member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@zhangguanzhang
Copy link
Collaborator Author

fix #6596

@rhatdan
Copy link
Member

rhatdan commented Jun 14, 2020

You need to rebase your PR and remove the merge PR.

@rhatdan
Copy link
Member

rhatdan commented Jun 14, 2020

Also could you add a test for this?

@zhangguanzhang
Copy link
Collaborator Author

I'll try it first and update it later

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2020
@zhangguanzhang zhangguanzhang force-pushed the podman-cp-dir branch 5 times, most recently from 5923c9d to c6a0bac Compare June 15, 2020 09:52
@rhatdan
Copy link
Member

rhatdan commented Jun 15, 2020

/approve
LGTM

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Jun 15, 2020
@TomSweeneyRedHat
Copy link
Member

@zhangguanzhang It would be helpful if you could add a little more information to the description for historical purposes. Can you add a quick example in the description showing the issue that this PR is fixing please?

@zhangguanzhang
Copy link
Collaborator Author

I've added some explanatory information

@zhangguanzhang
Copy link
Collaborator Author

zhangguanzhang commented Jun 15, 2020

$ mkdir test
$ ls
drwxr-xr-x  2 root root         6 Jun 15 16:54 test
$ cat >Dockerfile<<EOF
FROM scratch
COPY Dockerfile /
EOF
$ podman build -t podman-test:1.0.0 .
STEP 1: FROM scratch
STEP 2: COPY Dockerfile /
STEP 3: COMMIT podman-test:1.0.0
--> 839962c7269
839962c7269777d5f50d68428d5def260b6842f7c0607978b8567668f2a8c067

$ podman create podman-test:1.0.0 dummy
53914c7a1191e96b7fee0fcb9b152480640c392321edadfe657c10afda1431a2
$ podman cp 539:/   non_dir
$ ls non_dir
Dockerfile

If you copy the container's root directory to an existing directory, It creates an extra layer of the directory called merged

$ podman cp 539:/ test
$ ls -l  test
total 0
drwxr-xr-x 2 root root 24 Jun 15 23:30 merged
$ ls -l  test/merged/
total 4
-rw-r--r-- 1 root root 31 Jun 13 22:50 Dockerfile

@zhangguanzhang zhangguanzhang changed the title fix -- podman cp can create an extra directory level fix podman cp can create an extra directory when the source is the container's root directory Jun 15, 2020
@rhatdan
Copy link
Member

rhatdan commented Jun 15, 2020

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 15, 2020
@TomSweeneyRedHat
Copy link
Member

LGTM
@zhangguanzhang Thanks a bunch for the follow-up explanation. I copied it up to the main description so it wouldn't get lost in the replies.

@jwhonce
Copy link
Member

jwhonce commented Jun 15, 2020

LGTM, asked @mheon to also take a look

@mheon
Copy link
Member

mheon commented Jun 15, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2020
@openshift-merge-robot openshift-merge-robot merged commit 5a82a55 into containers:master Jun 15, 2020
@zhangguanzhang zhangguanzhang deleted the podman-cp-dir branch August 8, 2020 10:41
@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.
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. ok-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

podman cp can create an extra directory level
7 participants