Skip to content
This repository was archived by the owner on Oct 13, 2023. It is now read-only.

[19.03] Add chroot back to Tar/Untar without the previously introduced regression #280

Merged
andrewhsu merged 7 commits into from
Jun 17, 2019
Merged

[19.03] Add chroot back to Tar/Untar without the previously introduced regression #280

andrewhsu merged 7 commits into from
Jun 17, 2019

Conversation

@tiborvass
Copy link

@tiborvass tiborvass commented Jun 14, 2019

Reverts #275 which reverted #254 and cherry-picks the needed fix from moby#39357

Related to moby#39348

Tibor Vass and others added 5 commits June 14, 2019 01:37
Revert "Revert "Pass root to chroot to for chroot Untar""
29ff280 
This reverts commit 60013ba.

Signed-off-by: Tibor Vass <[email protected]>
Revert "Revert "Add chroot for tar packing operations""
44023af 
This reverts commit 96df6d4.

Signed-off-by: Tibor Vass <[email protected]>
@cpuguy83
Add test for copying entire container rootfs
e3f83e7 
CID=$(docker create alpine)
docker cp $CID:/ out

Signed-off-by: Brian Goff <[email protected]>
Signed-off-by: Tibor Vass <[email protected]>
(cherry picked from commit 6db9f1c)
Signed-off-by: Tibor Vass <[email protected]>
add more tests
9b97965 
Signed-off-by: Tibor Vass <[email protected]>
(cherry picked from commit 02f1eb8)
Signed-off-by: Tibor Vass <[email protected]>
daemon: fix docker cp when container source is /
a0063c5 
Before 7a7357d, archive.TarResourceRebase was being used to copy files
and folders from the container. That function splits the source path
into a dirname + basename pair to support copying a file:
if you wanted to tar `dir/file` it would tar from `dir` the file `file`
(as part of the IncludedFiles option).

However, that path splitting logic was kept for folders as well, which
resulted in weird inputs to archive.TarWithOptions:
if you wanted to tar `dir1/dir2` it would tar from `dir1` the directory
`dir2` (as part of IncludedFiles option).

Although it was weird, it worked fine until we started chrooting into
the container rootfs when doing a `docker cp` with container source set
to `/` (cf 3029e76).

The fix is to only do the path splitting logic if the source is a file.

Unfortunately, 7a7357d added support for LCOW by duplicating some of
this subtle logic. Ideally we would need to do more refactoring of the
archive codebase to properly encapsulate these behaviors behind well-
documented APIs.

This fix does not do that. Instead, it fixes the issue inline.

Signed-off-by: Tibor Vass <[email protected]>
(cherry picked from commit 171538c)
Signed-off-by: Tibor Vass <[email protected]>
@tiborvass tiborvass changed the title 19.03 chroot tar untar and cp slash fix [19.03] Add chroot back to Tar/Untar without the previously introduced regression Jun 14, 2019
pkg/archive: keep walkRoot clean if source is /
186afe3 
Previously, getWalkRoot("/", "foo") would return "//foo"
Now it returns "/foo"

Signed-off-by: Tibor Vass <[email protected]>
(cherry picked from commit 7410f1a)
Signed-off-by: Tibor Vass <[email protected]>
@tiborvass tiborvass force-pushed the 19.03-chroot-tar-untar-and-cp-slash-fix branch from 852e7c7 to 186afe3 Compare June 14, 2019 04:02
@thaJeztah
Copy link
Member

thaJeztah commented Jun 14, 2019

Failing on

04:15:28 # github.com/docker/docker/integration/container [github.com/docker/docker/integration/container.test]
04:15:28 integration/container/copy_test.go:108:25: cannot use ctx (type context.Context) as type *testing.T in argument to "github.com/docker/docker/integration/internal/container".Create
04:15:28 integration/container/copy_test.go:108:25: cannot use t (type *testing.T) as type context.Context in argument to "github.com/docker/docker/integration/internal/container".Create:
04:15:28 	*testing.T does not implement context.Context (missing Deadline method)

To easify future backports, I think we should backport;

(But #279 ("[19.03 backport] Handle the error case when a container reattaches to the same network") has to be merged first)

So;

  1. 1. merge [19.03 backport] Handle the error case when a container reattaches to the same network #279 ("[19.03 backport] Handle the error case when a container reattaches to the same network") has to be merged first.
  2. 2. rebase and merge [19.03 backport] Integration: change signatures to fix golint warnings #281 [19.03 backport] Integration: change signatures to fix golint warnings
  3. 3. rebase and merge this PR

@thaJeztah thaJeztah added this to the 19.03.1 milestone Jun 14, 2019
@tiborvass tiborvass marked this pull request as ready for review June 14, 2019 18:06
integration: have container.Create call compile
8f4b96f 
For reference on why this is needed:
docker-archive#280 (comment)

Signed-off-by: Tibor Vass <[email protected]>
@tiborvass tiborvass force-pushed the 19.03-chroot-tar-untar-and-cp-slash-fix branch from 7ad2521 to 8f4b96f Compare June 14, 2019 18:28
@thaJeztah
Copy link
Member

thaJeztah commented Jun 14, 2019

discussed with @tiborvass - well do a patch in this PR itself, and keep the other changes for 19.03.1

thaJeztah
thaJeztah approved these changes Jun 14, 2019
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

cpuguy83
cpuguy83 approved these changes Jun 17, 2019
View reviewed changes
Copy link

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewhsu andrewhsu modified the milestones: 19.03.1, 19.03.0 Jun 17, 2019
andrewhsu
andrewhsu approved these changes Jun 17, 2019
View reviewed changes
Copy link

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewhsu andrewhsu merged commit 3452f74 into docker-archive:19.03 Jun 17, 2019
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Jun 17, 2019
integration: have container.Create call compile
428f953 
For reference on why this is needed:
docker-archive/engine#280 (comment)

Signed-off-by: Tibor Vass <[email protected]>
Upstream-commit: 8f4b96f19e64b96df9d8c43208cefb113715ccbf
Component: engine
thaJeztah pushed a commit to thaJeztah/docker that referenced this pull request Jun 18, 2019
@thaJeztah
integration: have container.Create call compile
584c085 
For reference on why this is needed:
docker-archive#280 (comment)

Signed-off-by: Tibor Vass <[email protected]>
(cherry picked from commit 8f4b96f)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Jun 20, 2019
@thaJeztah
integration: have container.Create call compile
b0ff7b7 
For reference on why this is needed:
docker-archive/engine#280 (comment)

Signed-off-by: Tibor Vass <[email protected]>
(cherry picked from commit 8f4b96f19e64b96df9d8c43208cefb113715ccbf)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Upstream-commit: 584c0857ab21895e62feac686448085113c6c977
Component: engine
@dubo-dubon-duponey dubo-dubon-duponey mentioned this pull request Jul 1, 2019
Closed
@thaJeztah thaJeztah mentioned this pull request Aug 13, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@andrewhsu andrewhsu andrewhsu approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

19.03.0

Development

Successfully merging this pull request may close these issues.

4 participants

@tiborvass @thaJeztah @andrewhsu @cpuguy83