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

Calculate sha256 on the fly on dockerTools.buildLayeredImage #89075

Closed
wants to merge 1 commit into from

Conversation

@utdemir
Copy link
Member

utdemir commented May 28, 2020

Motivation for this change

Previously, we were calculating layer checksums at a separate pass after
the creation of the tarball. This is not ideal when packaging containers
with large layers, since it requires reading the whole tarball again
from disk in order to create the checksum.

This commit calculates the sha256 in parallel when creating the tarball
and saves it to a file for later use by buildLayeredImage function.

Things done

Relevant NixOS tests (nixosTests.docker-tools) pass.

  • 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 nixpkgs-review --run "nixpkgs-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.
Previously, we were calculating layer checksums at a separate pass after
the creation of the tarball. This is not ideal when packaging containers
with large layers, since it requires reading the whole tarball again
from disk in order to create the checksum.

This commit calculates the sha256 in parallel when creating the tarball
and saves it to a file for later use by buildLayeredImage function.
@utdemir
Copy link
Member Author

utdemir commented May 28, 2020

Please hold off on merging this since there seems to be a scary non-deterministic error somewhere. On every 2-3rd try, the tests fail with an error like:

docker: must succeed: docker load --input='/nix/store/p968alylhi1h5friqdbq7yrjmnkhk4xh-docker-image-layered-image.tar.gz'
docker # [  222.830684] dockerd[815]: time="2020-05-28T05:14:44.245386204Z" level=info msg="Layer sha256:2d94a3328eda167fc447ab475a6989bee2a44250e62e9876457c26e7eafdc0ad cleaned up"
docker # invalid diffID for layer 0: expected "sha256:d112f136191cdb3b9c91ff638d33f0aded087d584dcec5e2af27ce12e5d71f90", got "sha256:2d94a3328eda167fc447ab475a6989bee2a44250e62e9876457c26e7eafdc0ad"

I couldn't figure out the reason yet, so I'd appreciate another set of eyes.

@nlewo
Copy link
Member

nlewo commented May 28, 2020

@utdemir

  • are you sure it is related to this patchset? Can you confirm you are not able reproduce without this patch?
  • is it always on layer 0?
@utdemir
Copy link
Member Author

utdemir commented May 28, 2020

Thank you @nlewo.

  • are you sure it is related to this patchset? Can you confirm you are not able reproduce without this patch?

I think so. The error is not deterministic, so I can't be 100% sure, but I ran things without this patch multiple times and couldn't reproduce the error.

  • is it always on layer 0?

No, I just got the same error for layer 6.

@nlewo
Copy link
Member

nlewo commented May 28, 2020

I built and loaded it 6 times and I can't reproduce locally:

nix-build  -A dockerTools.examples.layered-image ; docker load -i ./result

(I ran the gc before each attempt)

Would you be able to keep a copy of each produced image in order to compare them? You could first try to run sha256 on each produced image to see if they are equal. If not, you could then use diffoscope to identity the differences (it know how to unpack Docker images).

@utdemir
Copy link
Member Author

utdemir commented Jun 10, 2020

@nlewo thank you for taking a look! It is bit hard to reproduce, so I couldn't find enough time. But I am trying a different approach now, will update this PR in a week or so.

@purcell
Copy link
Contributor

purcell commented Jun 25, 2020

This can be closed when #91084 is merged.

@flokli
Copy link
Contributor

flokli commented Jun 29, 2020

#91084 got merged, closing.

@flokli flokli closed this Jun 29, 2020
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.