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

Hook up blueprint filesystem customizations #124

Merged
merged 7 commits into from
Jun 3, 2024
Merged

Conversation

bcl
Copy link
Contributor

@bcl bcl commented Jan 16, 2024

Really simple change :) The testing feels a bit klunky, suggestions welcome. I thought about checking the manifest, but it boots the image anyway so checking the output of df seemed to be the most straight forward way.

@bcl bcl requested review from achilleas-k and mvo5 January 16, 2024 01:54
test/test_smoke.py Outdated Show resolved Hide resolved
@achilleas-k
Copy link
Member

One tricky part here is that we have rules in osbuild/images about what partitions a user can (not) add to avoid making unbootable images. Unfortunately those aren't exposed (yet) so we can't apply them here.

Maybe we can only allow cutomizing the size of / for now to unblock issues with images failing to build when the base container is too big and then work on bringing in the rules from images.

@bcl
Copy link
Contributor Author

bcl commented Jan 16, 2024

One tricky part here is that we have rules in osbuild/images about what partitions a user can (not) add to avoid making unbootable images. Unfortunately those aren't exposed (yet) so we can't apply them here.

It feels weird to essentially be partially re-implenting things here (and will be totally confusing to users) so I think exposing more of osbuild/images is the right way to go.

@bcl
Copy link
Contributor Author

bcl commented Jan 23, 2024

PR to move the policies into the public pkg - osbuild/images#395

@bcl bcl requested a review from achilleas-k January 24, 2024 00:46
@bcl
Copy link
Contributor Author

bcl commented Jan 24, 2024

NOTE: CI tests will fail until images is updated

Copy link

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale Issue or PR with no activity for extended period of time label Feb 23, 2024
@bcl bcl removed the Stale Issue or PR with no activity for extended period of time label Feb 27, 2024
@bcl bcl marked this pull request as ready for review February 27, 2024 15:52
@bcl bcl force-pushed the main-customfs branch 2 times, most recently from b8bd52f to 2e5dc6e Compare February 28, 2024 16:50
achilleas-k
achilleas-k previously approved these changes Mar 19, 2024
@bcl
Copy link
Contributor Author

bcl commented Mar 21, 2024

I've lost track of the status of this, so if you want it don't wait on me :)

cgwalters
cgwalters previously approved these changes Mar 25, 2024
Copy link
Contributor

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty small and sane to me, I think we also just need to update the docs, right? (Can be done post-merge)

test/test_manifest.py Show resolved Hide resolved
@cgwalters
Copy link
Contributor

Hm, testing farm was failing with

OSError: [Errno 30] Read-only file system: 'boot.1'
But I'm not totally sure that was the root cause?

@cgwalters cgwalters dismissed stale reviews from achilleas-k and themself via 82547de March 25, 2024 17:54
@cgwalters
Copy link
Contributor

I've rebased this on git main, just fixing up the (textual, but not logical) conflict with the new "double rootfs size default") code. Not tested locally beyond building, but let's see what CI says!

@mvo5
Copy link
Collaborator

mvo5 commented Mar 26, 2024

I've rebased this on git main, just fixing up the (textual, but not logical) conflict with the new "double rootfs size default") code. Not tested locally beyond building, but let's see what CI says!

Once we move to the bootc install to-filesystem flow (which is hopefully soon) we will no longer write a custom /etc/fstab which is crucial for this feature to work in the general case. I think we have some options:

  1. add policy so that we only allow to customize the size of "/", "/boot" and disallow adding extra partitions (probably a good starting point)
  2. support writing a custom /etc/fstab from images, I managed to find a bit of time today and looked over the ostree docs and played with guestfish with a fresh image that never booted. The only way to customize /etc/fstab I found was to write into the deployment root /ostree/deploy/default/deploy/d3d806...0/etc/fstab of the unbooted disk. Any other path ("/", "/sysroot", "/ostree/deploy/default/etc/fstab") I experiented with got "shadowed" or was wrong on the first boot. We do have support for this in osbuild, so adding this way of customizing is straightforward to add but I feel uneasy about it because it seems to me that bootc should be in control of it's content and just "side-loading" like this does break any integrity checking.
  3. with (2) solved, it seems we also need some code to deal with custom mountpoints(?). I.e. it seems to support e.g. /data we also need to create the mountpoint in the deploy root?

Please let me know if I'm missing something here or thinking too complicated.

@cgwalters
Copy link
Contributor

but I feel uneasy about it because it seems to me that bootc should be in control of it's content and just "side-loading" like this does break any integrity checking.

I feel like we keep re-treading this, not sure how to solve the logical disconnect. I'll try to write some more about this in the bootc docs. Now for sure I would like to support better things that just "unmanaged regular files" in /etc (configmaps perhaps or other things).

But for broad/generic use cases, we simply have to support machine-local state - and for maximum compatibility we can't just require everything to learn to do configmaps or sysexts or something that doesn't boil down to supporting vi /etc/somefile.conf.

For example Anaconda today is writing an /etc/fstab always...which tangentially I needed to do an elaborate workaround for a corner case and hopefully as we align things here when Anaconda learns to use bootc install to-filesystem too we can centralize this logic more.

The only way to customize /etc/fstab I found was to write into the deployment root /ostree/deploy/default/deploy/d3d806...0/etc/fstab of the unbooted disk.

The configuration definitely needs to be in the deployment root, yes.

Also BTW let's not get too hung up on /etc/fstab specifically as e.g. .mount units also make sense - but all of this is really just a special case of "config files in /etc/ and kernel arguments" which both can form machine-local state necessary for storage, networking etc.

@mvo5
Copy link
Collaborator

mvo5 commented Mar 26, 2024

[..]

The configuration definitely needs to be in the deployment root, yes.

Also BTW let's not get too hung up on /etc/fstab specifically as e.g. .mount units also make sense - but all of this is really just a special case of "config files in /etc/ and kernel arguments" which both can form machine-local state necessary for storage, networking etc.

Thank you! Sorry that we keep coming back to this, but hopefully this was the disconnect I had. In my (wrong) mental model I assumed that there is

  1. /ostree/deploy/default/deploy/ which shouldn't be touched
  2. extra directory with overrides to the above dir to add local state like /etc/fstab (this is why I messed around in /etc on the not-yet-booted-disk)

It seems that ostree is smart enough to know if files in (1) get changed and tracks them as local state. I hope I got this right now and sorry that it took me a while.

@cgwalters
Copy link
Contributor

It seems that ostree is smart enough to know if files in (1) get changed and tracks them as local state.

Yes, https://containers.github.io/bootc/filesystem.html#etc ➡️ https://ostreedev.github.io/ostree/atomic-upgrades/#assembling-a-new-deployment-directory (I'll expand this more over there)

ostree's semantics here are more arguably more "package system" like (vs "simple image system" as is emphasized by e.g. systemd's push for empty /etc) - and this definitely has tradeoffs, but again in a nutshell we support dropping arbitrary files there and they persist.

@cgwalters
Copy link
Contributor

I rebased this again...I think it still makes sense. (Sorry about being initially semi-against...I had just really hoped we also started with the container-based configuration but that is just messier than I thought)

@mvo5
Copy link
Collaborator

mvo5 commented Apr 15, 2024

I rebased this again...I think it still makes sense. (Sorry about being initially semi-against...I had just really hoped we also started with the container-based configuration but that is just messier than I thought)

Once we move to bootc install to-filesystem (hopefully really soon) this will need something like osbuild/images#597 first.

Copy link

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale Issue or PR with no activity for extended period of time label May 16, 2024
@achilleas-k achilleas-k removed the Stale Issue or PR with no activity for extended period of time label May 16, 2024
bcl added 4 commits May 28, 2024 08:40
This also allows the root filesystem size to be increased if the default
of 10GiB isn't enough.
Make sure / is larger than the default of 10GiB and make sure there is a
new /var/log mountpoint.
This requires a new release of images that exposes the policies package.
Users cannot create a mountpoint on /ostree, make sure that an error is
returned when this happens.
@mvo5 mvo5 force-pushed the main-customfs branch from e26533f to 5fc5e6b Compare May 28, 2024 07:18
@mvo5
Copy link
Collaborator

mvo5 commented May 28, 2024

I rebased this again (and slightly tweaked tiny bits). Unfortunately it needs more work because:

...
Installing image: docker://quay.io/centos-bootc/centos-bootc:stream9
ERROR Installing to filesystem: Verifying empty rootfs: Non-empty root filesystem; found "var"
Traceback (most recent call last):
  File "/run/osbuild/bin/org.osbuild.bootc.install-to-filesystem", line 53, in <module>
    r = main(args["options"], args["inputs"], args["paths"])
  File "/run/osbuild/bin/org.osbuild.bootc.install-to-filesystem", line 48, in main
    subprocess.run(pargs, env=env, check=True)
  File "/usr/lib64/python3.9/subprocess.py", line 528, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['bootc', 'install', 'to-filesystem', '--source-imgref', 'containers-storage:[overlay@/run/osbuild/containers/storage+/run/containers/storage]3b612dd1fae2437c00ae3187d0e63daa7a94711560fb1712389edd4121668c96', '--skip-fetch-check', '--generic-image', '--karg', 'rw', '--karg', 'console=tty0', '--karg', 'console=ttyS0', '--karg', 'systemd.journald.forward_to_console=1', '--target-imgref', 'quay.io/centos-bootc/centos-bootc:stream9', '/run/osbuild/mounts']' returned non-zero exit status 1.

I attach the generated osbuild manifest with the exiting code:
fs-customizations.json

The issue is that we run "org.osbuild.bootc.install-to-filesystem" with all the mounts, i.e. "/", "/boot" but also "/var/log" now.
So AFAICT "images" need sto be changed so that:

  1. the "install-to-filesystem" stage only takes the "essential" mounts (/, /boot/, /boot/efi)
  2. after "install-to-filesystem" ran we need a "org.osbuild.mkdir" stage for the extra mount points that also only mounts the "essential" mounts

That should be enough (but is a bit of extra work).

mvo5 added 3 commits May 28, 2024 12:31
This commit tweaks the test setup slightly to use pytest.raises
for exception checking and also run test_mount_ostree_error()
only for the centos image (as the error checking/policies are
exactly the same for both images).
The "images" library does not support custom mount points for
bootc based images just yet. The reason is that images will
generate an osbuild manifest that contains all the "mounts"
for the generated disk. This means that with an extra partition
like `/var/log` this is visible for the "bootc install-to-filesystem"
stage. And that will trip up bootc because it validates the content
of the target directory. Example error with `/var/log` as a custom
mount point:
```
...
Installing image: docker://quay.io/centos-bootc/centos-bootc:stream9
ERROR Installing to filesystem: Verifying empty rootfs: Non-empty root filesystem; found "var"
Traceback (most recent call last):
  File "/run/osbuild/bin/org.osbuild.bootc.install-to-filesystem", line 53, in <module>
    r = main(args["options"], args["inputs"], args["paths"])
  File "/run/osbuild/bin/org.osbuild.bootc.install-to-filesystem", line 48, in main
    subprocess.run(pargs, env=env, check=True)
  File "/usr/lib64/python3.9/subprocess.py", line 528, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['bootc', 'install', 'to-filesystem', '--source-imgref', 'containers-storage:[overlay@/run/osbuild/containers/storage+/run/containers/storage]3b612dd1fae2437c00ae3187d0e63daa7a94711560fb1712389edd4121668c96', '--skip-fetch-check', '--generic-image', '--karg', 'rw', '--karg', 'console=tty0', '--karg', 'console=ttyS0', '--karg', 'systemd.journald.forward_to_console=1', '--target-imgref', 'quay.io/centos-bootc/centos-bootc:stream9', '/run/osbuild/mounts']' returned non-zero exit status 1.
```

So AFAICT "images" need sto be changed so that:

1. The "install-to-filesystem" stage only takes the "essential" mounts (/, /boot/, /boot/efi)
2. After "install-to-filesystem" ran we need a "org.osbuild.mkdir" stage for the extra mount points that also only mounts the "essential" mounts

As a first step on the journy this commit limits customizations to
"/" and "/boot" which is already very useful as many people have
asked for precisely those.
@mvo5 mvo5 force-pushed the main-customfs branch from 5fc5e6b to 75fb7cd Compare May 28, 2024 10:39
@mvo5
Copy link
Collaborator

mvo5 commented May 28, 2024

As suggested by @ochosi I tweaked the PR now so that we initially only allow to customize "/" and "/boot" which will already cover many use-cases. Then we can continue on this once we have updated images to support what bib needs.

Copy link
Collaborator

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, this looks good to me now but please count me as only a a +0.5 because I tweaked/rebased this code a bit.

@mvo5 mvo5 requested a review from achilleas-k May 28, 2024 10:45
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, let's enable / and /boot and enable more later.

@mvo5 mvo5 added this pull request to the merge queue Jun 3, 2024
Merged via the queue into osbuild:main with commit 06e1b2a Jun 3, 2024
6 of 7 checks passed
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

Successfully merging this pull request may close these issues.

4 participants