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

Add (and possibly require) ostree container commit #159

Closed
cgwalters opened this issue Nov 16, 2021 · 11 comments
Closed

Add (and possibly require) ostree container commit #159

cgwalters opened this issue Nov 16, 2021 · 11 comments
Assignees

Comments

@cgwalters
Copy link
Member

cgwalters commented Nov 16, 2021

The current layered container code performs "ostree importing" client side - we accept arbitrary layer tarballs that could come from a Dockerfile or other container build system.

Let's consider adding a command line ostree container container that is required to be invoked
in the container at the end of a build process. For example in a Dockerfile:

FROM quay.io/fedora/coreos
ADD somefile.conf /etc/systemd/system
RUN curl ... && yum install .. && ostree container commit

The last step would effectively do most of the heavy lifting we're already doing client side:

  • Move /etc -> /usr/etc (with backcompat link)
  • Discard content outside of /usr - or better, error out
  • Perform selinux labeling
  • Compute ostree sha256

In this, I think the biggest benefit is that we can error out if people try to do things like ship content in /boot or /var at build time. But avoiding doing selinux labeling on the client is a not-small benefit too.

Note that in order for this to work, it must be invoked for each layer.

@cgwalters
Copy link
Member Author

This issue also relates to ostreedev/ostree#2480 so the CLI can just be ostree instead of ostree-ext-cli.

@lucab
Copy link
Member

lucab commented Dec 13, 2021

Below some notes from an out-of-band chat that I forgot to write down earlier:

  • this sounds like a subset of what rpm-ostree compose postprocess does. There is possibly some logic which could be shared between the two.
  • rpm-ostree does automatic translation of /var into tmpfiles.d fragment too. But I'm unsure whether the logic here does assume systemd-tmpfiles (and other systemd mechanisms in general).
  • depending on the starting image, it may not readily be in a runnable state. For example, FCOS images require some inflating steps (for processing tmpfiles.d, sysusers.d, and more). This in-container flow may need a symmetrical prepare/finalize logical scope to take care of that.

@cgwalters
Copy link
Member Author

One thing that would likely ease implementation of this significantly is if we structure it as:

And that may be it!

@cgwalters cgwalters changed the title Add (and possibly require) ostree container finalize Add (and possibly require) ostree container commit Jan 5, 2022
@cgwalters
Copy link
Member Author

EDIT: actually what I wrote above is wrong because the ostree C code doesn't know about the tar format or how to write it.

We discussed a simpler option to start of just adding ostree container commit as a verb now and only having it do validation to start - for example, erroring out on content in /var that will be ignored.

(Also, a whole thread here around whether we should allow new toplevel directories outside of /usr or not - I think today we do)

Then later we can actually move all the commit logic mentioned above from per-machine (client side) to build time i.e. ostree container commit.

@cgwalters
Copy link
Member Author

The above landed in #205

But let's leave this issue open to doing

Then later we can actually move all the commit logic mentioned above from per-machine (client side) to build time i.e. ostree container commit.

@srstsavage
Copy link

Just to check on where this stands, is it correct that derived Dockerfiles should use

RUN ostree container commit

as the final RUN command, and that it currently just does some sanity checking but may do more build time preprocessing in the future?

cgwalters added a commit to cgwalters/coreos-layering-examples that referenced this issue Mar 29, 2022
Even though it doesn't do much right now, let's train
people to do this because I would like to require this in
the future.

See ostreedev/ostree-rs-ext#159
@cgwalters
Copy link
Member Author

Hi, thanks for the ping here! I think we'd forgotten to follow up on this because it'd stalled a little bit on getting the new functionality shipped out, but that has happened now.

So I updated coreos/layering-examples#5 as one example.

But yes, please do start adding the command now! Does that make sense?

I'll see if we can update documentation in this repository too. I fixed the example Dockerfile here too - it can't just be RUN ostree container commit because that would normally create its own separate layer; instead it needs to be at the end of each RUN invocation. (Alternatively, in many cases where Dockerfile layers don't provide much value, one can just have a RUN build.sh that does everything)

@srstsavage
Copy link

Thanks! This does make sense. Two questions though:

it can't just be RUN ostree container commit because that would normally create its own separate layer; instead it needs to be at the end of each RUN invocation

If we don't mind the separate layer, is there any harm in just running ostree container commit as its own RUN at the end of the Dockerfile? To me that seems a bit cleaner/more maintainable. Is the main concern adding bloat in the layers once ostree container commit starts moving files around (e.g. /etc -> /usr/etc)?

I think the biggest benefit is that we can error out if people try to do things like ship content in /boot or /var at build time

Should ostree container commit be returning non-zero when it finds files in /var? Currently it reports them to stderr but returns 0, so Docker builds still succeed.

$ docker build --pull --no-cache - <<EOF
FROM quay.io/coreos-assembler/fcos:next-devel
RUN touch /var/bad && ostree container commit
EOF

Sending build context to Docker daemon  2.048kB
Step 1/2 : FROM quay.io/coreos-assembler/fcos:next-devel
next-devel: Pulling from coreos-assembler/fcos
Digest: sha256:5b556d343a9d527f9f1e336e09ee0b75f52d1894cd585d17b31bcd7b3392503e
Status: Image is up to date for quay.io/coreos-assembler/fcos:next-devel
 ---> 3662acb8d28c
Step 2/2 : RUN touch /var/bad && ostree container commit
 ---> Running in b1e4f1829a37
Checking /var for files
Found file: "/var/bad"
Removing intermediate container b1e4f1829a37
 ---> 0f0408790ce6
Successfully built 0f0408790ce6

@cgwalters
Copy link
Member Author

If we don't mind the separate layer, is there any harm in just running ostree container commit as its own RUN at the end of the Dockerfile?

That won't help unless the build is done with podman|docker build --squash (which is not the default, and also not configurable in every environment that accepts Dockerfile as input).

Remember, docker/OCI containers are just tarballs with JSON metadata. A layer is just a tarball.

So without squashing, we'd end up with a series of layers (tarballs) and then one final one which shuffles things around - and the client has to download all those tarballs - duplicating space.

On the client, we still need to handle the non-ostree-committed layers because we can't "know" that the files were changed in a later layer.

In other words, having RUN ostree container commit as a separate layer would currently be an anti-optimization i.e. pessimization.

@cgwalters
Copy link
Member Author

Tangentially related to this in lieu of better docs elsewhere: It will not work to invoke e.g. useradd foo in a Dockerfile today, because we entirely drop all content in /var when importing the image. This relates to https://ostreedev.github.io/ostree/adapting-existing/#system-layout

ostree makes a strong guarantee it will not affect user data on upgrades, and /home is user data.

@cgwalters
Copy link
Member Author

Should ostree container commit be returning non-zero when it finds files in /var? Currently it reports them to stderr but returns 0, so Docker builds still succeed.

Also sorry...I missed this comment before, and it's now fixed in #364 along with #367

I think where we are now with ostree container commit is sufficient to close this.

However: it seems very likely that we will want to drain some logic that lives in rpm-ostree today for things like generating tmpfiles.d and sysusers.d snippets from data.

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

No branches or pull requests

4 participants