when building a layered docker image, ignore it if tar encounters cha… #75911

purefn · 2019-12-19T01:09:28Z

Motivation for this change

Allows building layered docker images in parallel with other derivations. Takes the second approach mentioned in #75888, telling tar to ignore the changes to /nix/store. tar exit codes are defined as

       0      Successful termination.

       1      Some files differ.  If tar was invoked with the --compare (--diff, -d) command line option, this means that some files in the  archive  differ
              from  their  disk counterparts.  If tar was given one of the --create, --append or --update options, this exit code means that some files were
              changed while being archived and so the resulting archive does not contain the exact copy of the file set.

       2      Fatal error.  This means that some fatal, unrecoverable error occurred.

So as long as tar exits with a 0 or 1 we should be fine. The rationale for that is that any derivations that building the layered docker image depends on would have already been built and in the nix store by the time we get to this point. Therefore, any changes to /nix/store don't pertain to the docker image under construction.

Things done

Notify maintainers

cc @nlewo

jonringer · 2019-12-19T06:19:51Z

cc @grahamc

purefn · 2019-12-19T23:30:25Z

Hmm I'm still seeing an intermittent error

docker-image-learning-server-nix.tar.gz> Cooking the image...
docker-image-learning-server-nix.tar.gz>  tar: ./6773e5a7a063b5386300f1675b617904d21ee4b8c7f9d75b275c67a62cf18e0a/layer.tar: file changed as we read it

That seems to be coming from this bit

echo "Cooking the image..."                                                                                                                        
tar -C image --dereference --hard-dereference --sort=name --mtime="@$SOURCE_DATE_EPOCH" --owner=0 --group=0  --mode=a-w --xform s:'^./':: -c . |       pigz -nT > $out

I'm a bit confused. I don't see how that file could be modified at this point and I'm hesitant to give it the same treatment without better understanding.

nlewo · 2019-12-20T08:14:08Z

I'm not in favor of this approach. I would prefer to explore the approach described in #75888 (comment).

purefn · 2019-12-20T18:28:15Z

@nlewo - I've updated it with a different approach which should work better. Let me know what you think!

nlewo

Otherwise, your first change looks fine to me ;)

pkgs/build-support/docker/store-path-to-layer.sh

nlewo

Could you reformat your commit message?
Otherwise, the code looks good to me ;)

pkgs/build-support/docker/store-path-to-layer.sh

purefn · 2019-12-30T21:27:53Z

Could you reformat your commit message?

Reformat it how? It's formatted according to the CONTRIBUTING.md unless there is a detail I'm missing.

jonringer · 2019-12-30T21:38:13Z

Reformat it how? It's formatted according to the CONTRIBUTING.md unless there is a detail I'm missing.

Since dockerTools refers to an attr set, I'm assuming something like:

dockerTools.buildLayeredImage: fix building images in parallel

when tar'ing store paths into layered archives when building layered images, don't use the absolute nix store path so that tar won't complain if something new is added to the nix store when building the final docker image, ignore any file changes tar detects in the layers. they are all immutable and the only thing that might change is the number of hard links due to store optimization

purefn · 2019-12-30T21:51:31Z

@nlewo Changed the final image tar to ignore the file changes it detected, added comments, and reformatted git commit as per @jonringer's suggestion. Should be good to merge now. Let me know if there is anything else.

nlewo

Otherwise, LGTM! And thanks for all of these comments:)

nlewo · 2019-12-31T08:40:45Z

pkgs/build-support/docker/default.nix

@@ -625,7 +624,22 @@ rec {
          -i "$imageName" > image/repositories

        echo "Cooking the image..."
-        tar -C image --dereference --hard-dereference --sort=name --mtime="@$SOURCE_DATE_EPOCH" --owner=0 --group=0  --mode=a-w --xform s:'^./':: -c . | pigz -nT > $out
+        # tar exits with an exit code of 1 if files changed while it was
+        # reading them. it considers a change in the number of hard links


Suggested change

# reading them. it considers a change in the number of hard links

# reading them. It considers a change in the number of hard links

nlewo · 2019-12-31T08:41:16Z

pkgs/build-support/docker/default.nix

+        # tar exits with an exit code of 1 if files changed while it was
+        # reading them. it considers a change in the number of hard links
+        # to be a "change", which can cause this to fail if images are being
+        # built concurrently and auto-optimise-store is turned on. since


Suggested change

# built concurrently and auto-optimise-store is turned on. since

# built concurrently and the auto-optimise-store nix option is turned on. Since

nlewo · 2019-12-31T08:42:04Z

pkgs/build-support/docker/default.nix

+        # reading them. it considers a change in the number of hard links
+        # to be a "change", which can cause this to fail if images are being
+        # built concurrently and auto-optimise-store is turned on. since
+        # know the contents of these files will not change, we can reasonably


Suggested change

# know the contents of these files will not change, we can reasonably

# the contents of these files will not change, we can reasonably

nlewo · 2019-12-31T08:42:54Z

pkgs/build-support/docker/store-path-to-layer.sh


 mkdir -p "$layerPath"
-tar --no-recursion -rf "$layerPath/layer.tar" \
+
+# make sure /nix and /nix/store appear first in the archive.


Suggested change

# make sure /nix and /nix/store appear first in the archive.

# Make sure /nix and /nix/store appear first in the archive.

nlewo · 2019-12-31T08:43:07Z

pkgs/build-support/docker/store-path-to-layer.sh

-tar --no-recursion -rf "$layerPath/layer.tar" \
+
+# make sure /nix and /nix/store appear first in the archive.
+# we create the directories here and use them because


Suggested change

# we create the directories here and use them because

# We create the directories here and use them because

nlewo · 2019-12-31T08:43:29Z

pkgs/build-support/docker/store-path-to-layer.sh

+    --transform='s,nix,/nix,' \
+    nix
+
+# we change into the /nix/store in order to avoid a similar


Suggested change

# we change into the /nix/store in order to avoid a similar

# We change into the /nix/store in order to avoid a similar

nlewo · 2019-12-31T08:44:04Z

pkgs/build-support/docker/store-path-to-layer.sh

+# we change into the /nix/store in order to avoid a similar
+# "file changed as we read it" error as above. Namely,
+# if we use the absolute path of /nix/store/123-pkg
+# and something new it added to the nix store while tar


Suggested change

# and something new it added to the nix store while tar

# and something new is added to the nix store while tar

nlewo · 2019-12-31T08:44:54Z

pkgs/build-support/docker/store-path-to-layer.sh

+# the relative nix store path, tar will ignore changes
+# to /nix/store. In order to create the correct structure
+# in the tar file, we transform the relative nix store
+# path to the absolute store path


Suggested change

# path to the absolute store path

# path to the absolute store path.

nlewo · 2019-12-31T08:48:56Z

@GrahamcOfBorg test docker-tools

nlewo · 2020-01-11T08:05:34Z

Note I applied comment fixes in da261e3.

Thanks!

rimmington · 2020-03-18T02:42:54Z

@nlewo Would it be possible to backport this fix to 19.09?

nlewo · 2020-03-25T07:45:33Z

@rimmington I'm sorry, I don't have time to backport it. Note also this is actually not a trivial backport: we need to backport another patch (because this one is buggy) and there is a merge conflict.

rimmington · 2020-03-30T05:38:13Z

@nlewo Ah, I didn't realise the backport is non-trivial; I may just wait for 20.03 then. Thanks for checking!

ofborg bot added 8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Dec 19, 2019

jonringer requested a review from grahamc December 19, 2019 06:19

purefn mentioned this pull request Dec 20, 2019

dockerTools.buildLayeredImage can fail when other nix builds are in progress #75888

Open

purefn force-pushed the parallel-docker-buildlayeredimage branch from ce87d79 to fd4ec44 Compare December 20, 2019 18:26

nlewo requested changes Dec 25, 2019

View reviewed changes

pkgs/build-support/docker/store-path-to-layer.sh Show resolved Hide resolved

nlewo requested changes Dec 28, 2019

View reviewed changes

pkgs/build-support/docker/store-path-to-layer.sh Show resolved Hide resolved

pkgs/build-support/docker/store-path-to-layer.sh Outdated Show resolved Hide resolved

purefn force-pushed the parallel-docker-buildlayeredimage branch from fd4ec44 to 3be7675 Compare December 30, 2019 21:47

jonringer requested a review from nlewo December 30, 2019 22:37

nlewo requested changes Dec 31, 2019

View reviewed changes

nlewo merged commit 0d983f9 into NixOS:master Jan 11, 2020

purcell mentioned this pull request Feb 13, 2020

Backported fix for building layered images in parallel standard-ai/nixpkgs-tensorflow-rust#9

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

when building a layered docker image, ignore it if tar encounters cha… #75911

when building a layered docker image, ignore it if tar encounters cha… #75911

purefn commented Dec 19, 2019

jonringer commented Dec 19, 2019

purefn commented Dec 19, 2019

nlewo commented Dec 20, 2019

purefn commented Dec 20, 2019

nlewo left a comment

nlewo left a comment

purefn commented Dec 30, 2019

jonringer commented Dec 30, 2019 •

edited

Loading

purefn commented Dec 30, 2019

nlewo left a comment

nlewo Dec 31, 2019

nlewo Dec 31, 2019

nlewo Dec 31, 2019

nlewo Dec 31, 2019

nlewo Dec 31, 2019

nlewo Dec 31, 2019

nlewo Dec 31, 2019

nlewo Dec 31, 2019

nlewo commented Dec 31, 2019

nlewo commented Jan 11, 2020

rimmington commented Mar 18, 2020

nlewo commented Mar 25, 2020

rimmington commented Mar 30, 2020

	# reading them. it considers a change in the number of hard links
	# reading them. It considers a change in the number of hard links

	# built concurrently and auto-optimise-store is turned on. since
	# built concurrently and the auto-optimise-store nix option is turned on. Since

	# know the contents of these files will not change, we can reasonably
	# the contents of these files will not change, we can reasonably

	# make sure /nix and /nix/store appear first in the archive.
	# Make sure /nix and /nix/store appear first in the archive.

	# we create the directories here and use them because
	# We create the directories here and use them because

	# we change into the /nix/store in order to avoid a similar
	# We change into the /nix/store in order to avoid a similar

	# and something new it added to the nix store while tar
	# and something new is added to the nix store while tar

	# path to the absolute store path
	# path to the absolute store path.

when building a layered docker image, ignore it if tar encounters cha… #75911

when building a layered docker image, ignore it if tar encounters cha… #75911

Conversation

purefn commented Dec 19, 2019

Motivation for this change

Things done

Notify maintainers

jonringer commented Dec 19, 2019

purefn commented Dec 19, 2019

nlewo commented Dec 20, 2019

purefn commented Dec 20, 2019

nlewo left a comment

Choose a reason for hiding this comment

nlewo left a comment

Choose a reason for hiding this comment

purefn commented Dec 30, 2019

jonringer commented Dec 30, 2019 • edited Loading

purefn commented Dec 30, 2019

nlewo left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlewo commented Dec 31, 2019

nlewo commented Jan 11, 2020

rimmington commented Mar 18, 2020

nlewo commented Mar 25, 2020

rimmington commented Mar 30, 2020

jonringer commented Dec 30, 2019 •

edited

Loading