dockerTools.pullImage: use skopeo to pull the image #29302

nlewo · 2017-09-13T08:07:43Z

Motivation for this change

Before this patch, a VM was used to spawn docker that pulled the
VM. Now, the tool Skopeo does this job well so we can simplify our
dockerTools since we doesn't need Docker anymore:)

This also fixe the regression described in
#29271 : cntlm proxy doesn't
work in 17.09 while it worked in 17.03.

Note Skopeo doesn't produce the same output than docker pull so, we
have to update sha.

Things done

Successfully run:

nix-build ./ -A dockerTools.examples.nix && docker load -i result

And also the ubuntu:latest image.

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

mention-bot · 2017-09-13T08:07:45Z

@nlewo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @lethalman, @matejc and @0arthur to be potential reviewers.

nlewo · 2017-09-13T08:20:23Z

cc @matejc @grahamc

Mic92 · 2017-09-13T09:47:16Z

+1 for the idea in general because it is faster and better maintainable. I think there should be a release note item or hint that we broke sha256 of the previous version.

Before this patch, a VM was used to spawn docker that pulled the VM. Now, the tool Skopeo does this job well so we can simplify our dockerTools since we doesn't need Docker anymore:) This also fixe the regression described in NixOS#29271 : cntlm proxy doesn't work in 17.09 while it worked in 17.03. Note Skopeo doesn't produce the same output than docker pull so, we have to update sha.

grahamc · 2017-09-13T11:39:26Z

This looks great to me, but I haven't tested it yet.

peti

I've never used docker tools, so I don't have a qualified opinion. The change looks good in principle, though.

copumpkin

Looks great in principle, and the VM has annoyed me in the past!

copumpkin · 2017-09-13T13:55:19Z

pkgs/build-support/docker/default.nix

+      nameReplace = name: builtins.replaceStrings ["/" ":"] ["-" "-"] name;
+    in
+      # For simplicity we only support sha256.
+      { imageName, imageTag ? "latest", imageId ? "${imageName}:${imageTag}"


Does "latest" really make sense as a tag to use here? I know you're just carrying it over from the previous version, but it seems like a recipe for distributing unreproducible code: I use imageTag = "latest";, set the sha256 I get, and then as long as it stays in my store, I don't think about it for a while. I push up the code to my colleagues who don't share my store, and they pull latest and get a different image. If anything I'd try to actively discourage folks from using latest. That might be a topic for a separate review though 😄

I fully agree with you.
Since this PR will be backported into 17.09, maybe it would be better to minimize changes.

So I could open another PR to remove the default "latest" value and go further.
It's possible to pull image by using the image digest (immutable):
https://docs.docker.com/engine/reference/commandline/pull/#pull-an-image-by-digest-immutable-identifier
Moreover, this is supported by Skopeo. So maybe we could add a digest argument and print a warning if the user uses the tag. I'm not sure that removing the tag argument is a good idea because it's not easy to find the digest of an image (I was not able to find it on the docker hub...).

Being able to pull by the image digest, previously called imageId in dockerTools, is what my team has been using. The change earlier this year repurposed imageId to what it is now, which is not nearly as useful and has prevented us from updating nixpkgs on our projects. I am in favor of changing imageId back to what it meant before the change use a VM.

@purefn is there an open issue for that? We shouldn't be breaking interfaces without providing reasonable alternatives, at least

@copumpkin Sadly, no. I had run into another issue with that commit, #27294, and thought when that had been fixed it fixed our only issue. I tried it and encountered more problems, but didn't get to in any further until just recently.

Can we come to a decision and get this merged and backported? My $0.02 is I think it is important to be able to pull by digest. This tooling has been largely broken forever. I'm not convinced too many people are depending on it.

@grahamc we depend on digests but I think we agree that this PR is better than the current state.

I think this is not relevant to this improvement.

Thanks.
And I've open #29505 to continue the discussion :)

globin · 2017-09-28T11:31:35Z

I'm reverting the skopeo stuff for now.. I generally think it's a good idea but creates too many incompatibilities right now.. sorry and I really hope you don't feel discouraged by that as you have been doing awesome work on the dockerTools! But having a clean release is a priority there.. I really hope we can improve this to make this compatible as it is definitely superior otherwise..

Mic92 · 2017-09-28T11:47:51Z

Did you revert on master or on the release branch?

Mic92 · 2017-09-28T11:49:29Z

Ah ok so mesos was the reason here: dabb296
I would say open a new pull request to find a way to fix this, is a good idea.

This reverts commit 01174c5. See #29302 (comment) for more information. This broke image format compatibility and therefore amongst others mesos.

This reverts commit 01174c5. See #29302 (comment) for more information. This broke image format compatibility and therefore amongst others mesos. (cherry picked from commit 5c6dc71)

nlewo · 2017-09-28T18:42:03Z

@globin Don't worry. I don't have any problems with these reverts! I'm waiting for some feedbacks from the Skopeo community before resubmitting improved versions of these patches.

How could I reproduce your issue with Mesos?
Or could you describe a little bit what was your issue?

globin · 2017-09-28T20:39:42Z

Just running nix-build -A nixos/tests/mesos.nix should be enough now that mesos itself is fixed. I hope I'll get a bit more involved with this after the release, but don't really have time right now..

nlewo changed the title ~~dockerTools.pullImage: use skopeo to pull the image~~ [WIP] dockerTools.pullImage: use skopeo to pull the image Sep 13, 2017

nlewo force-pushed the pr/dockerpull branch from bdcdb6f to 19744ec Compare September 13, 2017 08:17

nlewo changed the title ~~[WIP] dockerTools.pullImage: use skopeo to pull the image~~ dockerTools.pullImage: use skopeo to pull the image Sep 13, 2017

nlewo mentioned this pull request Sep 13, 2017

dockerTools.pullImage: change the docker deamon readiness mechanism #28963

Merged

8 tasks

nlewo force-pushed the pr/dockerpull branch from 19744ec to 2aa5dcb Compare September 13, 2017 11:25

nlewo added 2 commits September 13, 2017 13:34

dockerTools.pullImage: release note regarding sha256 argument value

e5e70cb

nlewo force-pushed the pr/dockerpull branch from 2aa5dcb to e5e70cb Compare September 13, 2017 11:35

nlewo force-pushed the pr/dockerpull branch from e5e70cb to 99728df Compare September 13, 2017 12:45

nlewo requested review from copumpkin, edolstra, FRidh, LnL7 and peti as code owners September 13, 2017 12:46

nlewo force-pushed the pr/dockerpull branch from 99728df to e5e70cb Compare September 13, 2017 12:48

peti reviewed Sep 13, 2017

View reviewed changes

copumpkin reviewed Sep 13, 2017

View reviewed changes

domenkozar merged commit ea6d37c into NixOS:master Sep 17, 2017

nlewo deleted the pr/dockerpull branch September 17, 2017 19:26

fpletz added this to the 17.09 milestone Sep 18, 2017

aneeshusa mentioned this pull request Sep 18, 2017

dockerTools.pullImage: use imageDigest #29505

Closed

8 tasks

globin added a commit that referenced this pull request Sep 28, 2017

Revert "dockerTools.pullImage: use skopeo to pull the image"

5c6dc71

This reverts commit 01174c5. See #29302 (comment) for more information. This broke image format compatibility and therefore amongst others mesos.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dockerTools.pullImage: use skopeo to pull the image #29302

dockerTools.pullImage: use skopeo to pull the image #29302

nlewo commented Sep 13, 2017

mention-bot commented Sep 13, 2017

nlewo commented Sep 13, 2017

Mic92 commented Sep 13, 2017

grahamc commented Sep 13, 2017

peti left a comment

copumpkin left a comment

copumpkin Sep 13, 2017

nlewo Sep 13, 2017

purefn Sep 13, 2017 •

edited

copumpkin Sep 14, 2017

purefn Sep 14, 2017

grahamc Sep 17, 2017

puffnfresh Sep 17, 2017

domenkozar Sep 17, 2017

nlewo Sep 17, 2017

globin commented Sep 28, 2017

Mic92 commented Sep 28, 2017

Mic92 commented Sep 28, 2017 •

edited

nlewo commented Sep 28, 2017

globin commented Sep 28, 2017

dockerTools.pullImage: use skopeo to pull the image #29302

dockerTools.pullImage: use skopeo to pull the image #29302

Conversation

nlewo commented Sep 13, 2017

Motivation for this change

Things done

mention-bot commented Sep 13, 2017

nlewo commented Sep 13, 2017

Mic92 commented Sep 13, 2017

grahamc commented Sep 13, 2017

peti left a comment

Choose a reason for hiding this comment

copumpkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purefn Sep 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

globin commented Sep 28, 2017

Mic92 commented Sep 28, 2017

Mic92 commented Sep 28, 2017 • edited

nlewo commented Sep 28, 2017

globin commented Sep 28, 2017

purefn Sep 13, 2017 •

edited

Mic92 commented Sep 28, 2017 •

edited