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

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

Merged
merged 1 commit into from Jan 11, 2020

Conversation

@purefn
Copy link
Contributor

@purefn purefn commented Dec 19, 2019

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • 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 nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @nlewo

@jonringer
Copy link
Contributor

@jonringer jonringer commented Dec 19, 2019

@jonringer jonringer requested a review from grahamc Dec 19, 2019
@purefn
Copy link
Contributor Author

@purefn purefn commented Dec 19, 2019

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
Copy link
Member

@nlewo nlewo commented Dec 20, 2019

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

@purefn purefn force-pushed the Simspace:parallel-docker-buildlayeredimage branch from ce87d79 to fd4ec44 Dec 20, 2019
@purefn
Copy link
Contributor Author

@purefn purefn commented Dec 20, 2019

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

Copy link
Member

@nlewo nlewo left a comment

Otherwise, your first change looks fine to me ;)

Copy link
Member

@nlewo nlewo left a comment

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

@purefn
Copy link
Contributor Author

@purefn purefn commented Dec 30, 2019

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
Copy link
Contributor

@jonringer jonringer commented Dec 30, 2019

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 purefn force-pushed the Simspace:parallel-docker-buildlayeredimage branch from fd4ec44 to 3be7675 Dec 30, 2019
@purefn
Copy link
Contributor Author

@purefn purefn commented Dec 30, 2019

@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.

@jonringer jonringer requested a review from nlewo Dec 30, 2019
Copy link
Member

@nlewo nlewo left a comment

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

@@ -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

This comment has been minimized.

@nlewo

nlewo Dec 31, 2019
Member

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
# 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

This comment has been minimized.

@nlewo

nlewo Dec 31, 2019
Member

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
# 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

This comment has been minimized.

@nlewo

nlewo Dec 31, 2019
Member

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

mkdir -p "$layerPath"
tar --no-recursion -rf "$layerPath/layer.tar" \

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

This comment has been minimized.

@nlewo

nlewo Dec 31, 2019
Member

Suggested change
# make sure /nix and /nix/store appear first in the archive.
# Make sure /nix and /nix/store appear first in the archive.
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

This comment has been minimized.

@nlewo

nlewo Dec 31, 2019
Member

Suggested change
# we create the directories here and use them because
# We create the directories here and use them because
--transform='s,nix,/nix,' \
nix

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

This comment has been minimized.

@nlewo

nlewo Dec 31, 2019
Member

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
# 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

This comment has been minimized.

@nlewo

nlewo Dec 31, 2019
Member

Suggested change
# and something new it added to the nix store while tar
# and something new is added to the nix store while tar
# 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

This comment has been minimized.

@nlewo

nlewo Dec 31, 2019
Member

Suggested change
# path to the absolute store path
# path to the absolute store path.
@nlewo
Copy link
Member

@nlewo nlewo commented Dec 31, 2019

@GrahamcOfBorg test docker-tools

@nlewo nlewo merged commit 0d983f9 into NixOS:master Jan 11, 2020
14 checks passed
14 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
tests.docker-tools on aarch64-linux Success
Details
tests.docker-tools on x86_64-linux Success
Details
@nlewo
Copy link
Member

@nlewo nlewo commented Jan 11, 2020

Note I applied comment fixes in da261e3.

Thanks!

@rimmington
Copy link
Contributor

@rimmington rimmington commented Mar 18, 2020

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

@nlewo
Copy link
Member

@nlewo nlewo commented Mar 25, 2020

@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
Copy link
Contributor

@rimmington rimmington commented Mar 30, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.