Skip to content
This repository has been archived by the owner on Sep 28, 2020. It is now read-only.

feature: flatten with scope #34

Open
erikh opened this issue Nov 1, 2016 · 7 comments
Open

feature: flatten with scope #34

erikh opened this issue Nov 1, 2016 · 7 comments

Comments

@erikh
Copy link
Member

erikh commented Nov 1, 2016

WIP, this'll probably be edited a few times.

Right now flatten does a few things wrong:

  • it merges the from image with the rest of the images; that's probably not what people want most of the time (although the option should be added in at some point)
  • it cannot scope what is flattened and what is not, e.g., flatten two run statements into one, but leave the next copy statement alone.
  • it cannot influence the cache, and any post-flattening commands will have their cache invalidated.

Proposal:

Surface-wise, we need to extend the flatten statement itself.

flatten "cacheKey" do
  run "foo"
  run "bar"
end

The cache key can be literally supplied as an argument. Corresponding with a cache key of some sort (this could be derived from environment, local files, other container's files, etc to ensure cache validation occurs safely) could influence the cache in a way that was safe to handle for multiple invocations of each run without corrupting or otherwise poorly influencing the cache. This, however, is completely driven by the end user and could lead to some very bad abuses, consistency problems in their internal build systems, etc. Maybe this should be a separate block statement so it can be filtered without losing flatten functionality.

Block mode is shown here which would allow all the statements inside the flatten to be flattened. Blocks should not be required and it is assumed that flatten without a block will flatten everything to the container before the run statement.

Internals-wise, we need to do two things:

  • solve flatten copies everything back in as root #33
  • implement a layer recording device, since 99% of this is here already I don't think it'll take too much effort.
  • flatten hooks into new executor calls: FlattenAll() and FlattenLast(n int) which flattens the entire run or the last n elements in the run respectively.

All of the above should satisfy our flatten needs. The full flatten can have the from omitted for free if we track only the layers we created.

@errordeveloper
Copy link
Contributor

errordeveloper commented Dec 5, 2016

Also, simply flattening all layers above the base image is probably a good and simple optimisation for many use-cases. Being able to that without having to indent all of the line inside a block would be very handy, I think, e.g. something like flatten :above_base_image at the end of a file.

@erikh
Copy link
Member Author

erikh commented Dec 5, 2016

that should already work (and is the only way flatten works now); I'll test in a bit.

@erikh
Copy link
Member Author

erikh commented Dec 5, 2016

nope. I'll make this the default tomorrow, now that we have the layer editing framework this should not be hard.

@erikh
Copy link
Member Author

erikh commented Dec 8, 2016

this is going to take more work than anticipated. Will be a week or so.

@erikh
Copy link
Member Author

erikh commented Dec 18, 2016

Ok, I looked into this tonight and realized that a few problems exist:

whiteout files (which are preserved in docker images) will not be handled right in a cross-platform way. This means we cannot just unpack to the host (regardless of all the safety issues that brings). Copying into a fresh container is a good solution.

However, because of the copy bug I've been trying to get patched into docker, that would reset all the files in the layers back into root.

There aren't really any good alternatives. At some point these files have to be extracted on top of each other, and to do it in a way that doesn't lock people into linux is not possible as far as I can tell.

Happily would accept any solution. For context, the existing flatten copies all files OUT of docker (which does not have the permissions issue) into a tarfile, builds an image based on that, and then blows it back into docker.

@errordeveloper
Copy link
Contributor

errordeveloper commented Dec 18, 2016 via email

@erikh
Copy link
Member Author

erikh commented Dec 18, 2016 via email

@Xe Xe mentioned this issue Feb 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants