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

dockerTools.buildLayeredImage: store all paths passed in final layer #78786

Closed

Conversation

@purefn
Copy link
Contributor

purefn commented Jan 29, 2020

Motivation for this change

Fixes #78744

My previous change broke when there are more packages than the maximum
number of layers. I had assumed that the store-path-to-layer.sh was
only ever passed a single store path, but that is not the case if
there are multiple packages going into the final layer. To fix this, we
loop through the paths going into the final layer, appending them to the
tar file and making sure they end up at the right path.

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

This comment has been minimized.

Copy link
Contributor Author

purefn commented Jan 29, 2020

@grahamc

This comment has been minimized.

Copy link
Member

grahamc commented Jan 29, 2020

Can you add a test to the examples list which forces the last layer to have many, and runs a program which would appear in the "bulk" layer? This way we can test it more easily next time.

Fixes #78744

My previous change broke when there are more packages than the maximum
number of layers. I had assumed that the `store-path-to-layer.sh` was
only ever passed a single store path, but that is not the case if
there are multiple packages going into the final layer. To fix this, we
loop through the paths going into the final layer, appending them to the
tar file and making sure they end up at the right path.
@purefn purefn force-pushed the Simspace:fix-including-more-than-max-layers branch from a31daa3 to 0cf37e5 Jan 29, 2020
@purefn

This comment has been minimized.

Copy link
Contributor Author

purefn commented Jan 29, 2020

@grahamc Great idea! Done.

@nlewo

This comment has been minimized.

Copy link
Member

nlewo commented Jan 30, 2020

@purefn Thanks for you quick fix!
The test is not working as expected.
It will be easier to write it when #78834 is merged since we could build an image with 2 layers.

If we want to quickly fix this issue, we could only merge the fix (without the test) and I could work on a test tomorrow, once #78834 is merged.

@gustavderdrache

This comment has been minimized.

Copy link

gustavderdrache commented Jan 30, 2020

If we want to avoid that bug, why not package bashInteractive instead of bash in the image and increase maxLayers? bashInteractive has readline in its closure so there will be a few extra layers to work with in the test case.

@purefn

This comment has been minimized.

Copy link
Contributor Author

purefn commented Jan 30, 2020

@nlewo

The test is not working as expected.

How is it not working as expected? It builds an image with 3 layers: the first is the layer with glibc, the second layer has both bash and hello, the third layer is the "customization" layer.

That's exactly what I expected it to produce when I wrote the test. Were your expectations different?

@nlewo

This comment has been minimized.

Copy link
Member

nlewo commented Jan 30, 2020

@purefn Sorry, I should have provided more explanations ;)

In the image you are building, the second layer contains both bash and hello store paths, and hello is the first store path in the list. So, it is added to the layer and the image command would works (while bash is not in the layer since it is the second element of the store path).

To easily write a test, I think we should write a test loading an image containing only 2 layers: the "bulk" and customization layers. In the bulk layer, we could add several store paths and ensure all of them are in the image. This test is more complicated to write with several layers because it hard to know the destination layer of store paths.
However, when I tried to build a 2 layers images, i got an issue which I'm fixing in #78834...

@nlewo

This comment has been minimized.

Copy link
Member

nlewo commented Jan 30, 2020

@purefn I was thinking on a test such as in nlewo@bad4290

@purefn

This comment has been minimized.

Copy link
Contributor Author

purefn commented Jan 30, 2020

I'm not opposed to your test, I think it's fine. I'm just curious, wouldn't we get effectively the same result if we changed the docker image command to something along the lines of ls ${hello} ${bash}?

@nlewo

This comment has been minimized.

Copy link
Member

nlewo commented Jan 30, 2020

Yes, you are right, ls ${hello} ${bash} could do the job.

Actually, I was initially thinking on just creating these two files (because they don't have any dependencies)... but coreutils is actually needed to test their existence :/

@nlewo

This comment has been minimized.

Copy link
Member

nlewo commented Feb 7, 2020

@purefn It would be nice to merge your fix ;)
Could you write a regression test as you suggested (ls ${hello} ${bash}), or remove the example from this PR (in that case I could write the test)?

@nlewo

This comment has been minimized.

Copy link
Member

nlewo commented Feb 14, 2020

I finally merged your fix commit in #80102 (where I added a regression test).
Thanks @purefn!

@nlewo nlewo closed this Feb 14, 2020
@purefn

This comment has been minimized.

Copy link
Contributor Author

purefn commented Feb 14, 2020

Oh, sorry! I've been swamped with work and life the past few weeks.

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.

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