Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

Use standard archive/tar instead of docker's untar implementation #435

Closed
wants to merge 50 commits into from

Conversation

ulyssessouza
Copy link
Contributor

@ulyssessouza ulyssessouza commented Dec 14, 2018

!!! Note that this PR is implemented based in another !!!
The only 2 exclusive files on this PR are:

  • internal/packager/packing_test.go
  • internal/tar/untar.go

- What I did
Substitute docker's untar package with a standard golang implementation

- How I did it
Adapted an implementation of untar to fit our needs and use it in TestPackInvocationImageContext

- How to verify it
Tests on packing_test.go passing without the need of running inside a container (just with make test and not make -f docker.Makefile test)

- Description for the changelog
Substitute docker's untar package with a standard golang implementation

- A picture of a cute animal
kitty_orig

chris-crone and others added 15 commits December 5, 2018 17:50
Signed-off-by: Christopher Crone <christopher.crone@docker.com>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Update code owners according to current project setup 😉
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Question : Why is it required ?

  • What are the reasons behind moving away form github.com/docker/docker/pkg/archive ?
  • If it's related to lchown, I'm pretty sure using TarOptions field NoLchown should work.
  • If not, I still believe it's better to fix/contribute upstream (it's even in the same docker org…) that duplicate this code.

@ulyssessouza
Copy link
Contributor Author

@vdemeester The long story is that:

What are the reasons behind moving away form github.com/docker/docker/pkg/archive ?

The idea is to be able to run this test with just a make test and not make -f docker.Makefile test the first runs in the host and the second will be executed inside a container.
The problem of using github.com/docker/docker/pkg/archive here is that it assumes it's inside a container. Like when assuming that infos about UID and GID are located in /etc/subuid and /etc/subgid (as in idtools.go:L33-34)

If it's related to lchown, I'm pretty sure using TarOptions field NoLchown should work.

That was my first guess. The problem here is that according to idtools.go:L55 it not chown the exisiting files, but will systematically do it for the new ones.
The TarOptions::NoLchown is just used in archive.go:L651 that will prevent the os.Lchown in archive.go:L655 to be executed, but not the firstly described.

If not, I still believe it's better to fix/contribute upstream (it's even in the same docker org…) that duplicate this code.

My concern with this is that from what it looks, github.com/docker/docker/pkg/archive was built to be used in a different environment, which seems to be inside a container where it can read /etc/sub{uid|gid} to retrieve the user and group. Without this assumption, it should retrieve the username and group for different OSs, and also docker-app is compiled without CGO, so even a user.Current() work. So all of this makes me think its not buggy, but just thought to be used in a different context.

Given that, one possibility of contribution for the upstream would be add a new func in idtools.go which is similar to MkdirAllAndChownNew (given that it's a public func I cannot just change it's signature) that tells mkdirAs (having as side-effect the change of it's signature) to not chown even the new files. That would work.

Talking with @silvin-lubecki, he came with a simplier solution, that consists in just iterate through the entries in the archive, instead of really extracting and iterating through it.
This solution seems to be cleaner for our needs.

@thaJeztah
Copy link
Member

We should probably be careful replacing this package; there may be special cases for backward compatibility in this package @tonistiigi @dmcgowan

@ulyssessouza
Copy link
Contributor Author

@vdemeester And what about now?
I just pushed a simple solution described in the last paragraph of last message.

@ulyssessouza ulyssessouza force-pushed the std-untar branch 2 times, most recently from d60659b to ddbedfd Compare December 28, 2018 10:42
tullo and others added 22 commits January 23, 2019 17:38
If the voting app is deployed using production params, the result service
fails to deploy as port 80 already is taken by vote service

Steps to reproduce:
voting-app$ docker-app render
  --parameters-files voting-app.dockerapp/parameters/production.yml
| docker stack deploy --compose-file - voting

Signed-off-by: Andreas Amstutz <andreasamstutz@gmail.com>
Signed-off-by: Andreas Amstutz <andreasamstutz@gmail.com>
Correct build instruction for experimental build
Closes docker#448

Signed-off-by: Andreas Amstutz <andreasamstutz@gmail.com>
Add missing 'TraverseChildren: true' in the cobra.Command
fix port '80' is already in use (voting-app production params)
Signed-off-by: Joe Abbey <joe.abbey@gmail.com>
* Fix tag generation by truncating the git sha.
* Fix saving cnab base image to a global tmp directory and put it in a local git ignored directory (_build).
* Remove the cnab base image from the CI local daemon
* Load invocation image directly instead of using a make target.

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Install command also detects if the argument is a Docker Application Package. It will then bundle it and run the CNAB action Install.
It is now a CNAB installer, and all the docker-app deployment logic is moved to the docker-app backend binary run in cmd/run/install.go.
The installation is added in the duffle claim store.

* Rename deploy command to install (with still alias to deploy)

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
It will run the Uninstall CNAB action in the cmd/run/uninstall.go to remove a Docker Application Package deployment.

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
For a Docker Application Package, it will run a "docker stack services" on the deployed stack.

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
*Fix running docker-app validate only on application package examples and skip CNAB examples.

Signed-off-by: Christopher Crone <christopher.crone@docker.com>
…xt has not been specified.

Factorize credentialset and target-context flags.
Extract all helper functions to their own go file.

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Introduce CNAB core commands
All outstanding issues will be done on followups. 
We also need to rebase every other PRs targeting the CNAB branch
@ulyssessouza ulyssessouza force-pushed the std-untar branch 2 times, most recently from 58d17c4 to 6c84a2b Compare February 1, 2019 14:54
@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "std-untar" git@github.com:ulyssessouza/app.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842357813216
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@codecov
Copy link

codecov bot commented Feb 1, 2019

Codecov Report

❗ No coverage uploaded for pull request base (cnab-preview@133a2c6). Click here to learn what that means.
The diff coverage is 48.42%.

Impacted file tree graph

@@               Coverage Diff               @@
##             cnab-preview     #435   +/-   ##
===============================================
  Coverage                ?   59.04%           
===============================================
  Files                   ?       56           
  Lines                   ?     2803           
  Branches                ?        0           
===============================================
  Hits                    ?     1655           
  Misses                  ?      931           
  Partials                ?      217
Impacted Files Coverage Δ
cmd/docker-app/init.go 100% <ø> (ø)
internal/names.go 100% <ø> (ø)
types/metadata/metadata.go 100% <ø> (ø)
types/parameters/opts.go 100% <ø> (ø)
cmd/docker-app/image-add.go 0% <0%> (ø)
cmd/docker-app/cnab.go 0% <0%> (ø)
cmd/docker-app/validate.go 88.88% <100%> (ø)
internal/renderer/renderer.go 100% <100%> (ø)
internal/packager/parameter.go 100% <100%> (ø)
loader/loader.go 79.24% <100%> (ø)
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 133a2c6...6c84a2b. Read the comment docs.

@ulyssessouza
Copy link
Contributor Author

Closing it since it has been merged by #443

@ulyssessouza ulyssessouza deleted the std-untar branch February 1, 2019 16:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants