initoverlayfs: Add initial initoverlayfs support #4721

dougsland · 2023-12-09T14:33:45Z

Run initoverlayfs in case the binary exists.

Signed-off-by: Douglas Schilling Landgraf [email protected]

openshift-ci · 2023-12-09T14:33:56Z

Hi @dougsland. Thanks for your PR.

I'm waiting for a coreos member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dougsland · 2023-12-09T14:35:02Z

/hold
I need to tests with the patch.

cgwalters

Just for context for everyone else, this is for integration with the https://github.com/containers/initoverlayfs/ project.

It adds InitOverlayFS as option in rpm-ostreed.conf

The rpm-ostreed.conf file is for client side configuration, but this is a build time option that would need to go in our "treefile" config.

Backing up though, I think we could do something basic like just detect the command is installed, and from code in e.g. initramfs.rs we automatically execute it? (The chance of someone including the initoverlay project in their image but not wanting to use it seems very obscure at best)

Glancing at https://github.com/containers/initoverlayfs/blob/main/bin/initoverlayfs-install#L17 though for example this will definitely need fixes because on ostree-based systems we put the initramfs in /usr/lib/modules/$kver/initramfs.img.

cgwalters · 2023-12-10T14:37:46Z

On a procedural level: Thanks for the patch! I think we should do this, though with nontrivial changes like this it probably would have been a good idea to start by filing an issue where we can discuss basic design bits and ensure we're on the same page.

dougsland · 2023-12-11T00:12:44Z

Just for context for everyone else, this is for integration with the https://github.com/containers/initoverlayfs/ project.

It adds InitOverlayFS as option in rpm-ostreed.conf

The rpm-ostreed.conf file is for client side configuration, but this is a build time option that would need to
go in our "treefile" config.

Oh okay. Just for the record, could you please clarify "treefile" config?

Backing up though, I think we could do something basic like just detect the command is installed, and from
code in e.g. initramfs.rs we automatically execute it? (The chance of someone including the initoverlay project
in their image but not wanting to use it seems very obscure at best)

Sounds good to me.

Glancing at https://github.com/containers/initoverlayfs/blob/main/bin/initoverlayfs-install#L17 though for
example this will definitely need fixes because on ostree-based systems we put the initramfs in
/usr/lib/modules/$kver/initramfs.img.

Great thanks for sharing, I am sure we can address it viainitoverlayfs-install quickly.

cgwalters · 2023-12-11T14:18:11Z

Oh okay. Just for the record, could you please clarify "treefile" config?

The build time config. https://github.com/coreos/rpm-ostree/blob/main/docs/treefile.md

There is also /etc/kernel/install.d which is its own sub-universe that rpm-ostree doesn't integrate much with currently. But in theory we probably should try, and if we did it may work to have initoverlayfs drop in a file there after the dracut run.

src/daemon/rpm-ostreed.conf

dougsland · 2023-12-12T15:43:56Z

Glancing at https://github.com/containers/initoverlayfs/blob/main/bin/initoverlayfs-install#L17 though for
example this will definitely need fixes because on ostree-based systems we put the initramfs in
/usr/lib/modules/$kver/initramfs.img.

Should be addressed by: containers/initoverlayfs#43

dougsland · 2023-12-12T16:06:01Z

Oh okay. Just for the record, could you please clarify "treefile" config?

The build time config. https://github.com/coreos/rpm-ostree/blob/main/docs/treefile.md

Thanks! So no real changes in the treefile format for initoverlayfs right? We can add in the rpm schema there to include the package using repo and package params.

There is also /etc/kernel/install.d which is its own sub-universe that rpm-ostree doesn't integrate much with
currently. But in theory we probably should try, and if we did it may work to have initoverlayfs drop in a file there
after the dracut run.

Just to double check, instead of dropping a file in /usr/lib/modules/$kver/initramfs.img we can try /etc/kernel/install.d?

cgwalters · 2023-12-15T22:19:19Z

Just to double check, instead of dropping a file in /usr/lib/modules/$kver/initramfs.img we can try /etc/kernel/install.d?

I filed #4726 related to this, but it's a nontrivial change and I'm not sure I'd block on it.

ericcurtin · 2023-12-16T15:35:32Z

Thanks @cgwalters it's a good idea to track that, useful people who want more local control.

dougsland · 2023-12-18T21:55:15Z

Just to double check, instead of dropping a file in /usr/lib/modules/$kver/initramfs.img we can try /etc/kernel/install.d?

I filed #4726 related to this, but it's a nontrivial change and I'm not sure I'd block on it.

Thanks, I will continue here @cgwalters

rust/src/cliwrap/kernel_install.rs

cgwalters

I still have some basic things I don't understand about initoverlayfs which have implications on their usage with container/ostree systems, but I will start a discussion on that project.

rust/src/cliwrap/kernel_install.rs

cgwalters · 2024-02-14T13:43:58Z

Hmm now that I see osbuild/osbuild#1586 - shouldn't this be the same thing, just invoking initoverlayfs if it exists instead of dracut?

rust/src/cliwrap/kernel_install.rs

ericcurtin · 2024-02-14T15:06:09Z

What initoverlayfs-install basically does is call dracut twice, once to create a really small initrd who's only role is to initialize storage and another dracut builder that builds the bigger one with all bells and whistles (initoverlayfs)

ericcurtin · 2024-02-14T15:32:34Z

Btw, a nice simple way to test this would be to pull this draft PR branch:

https://gitlab.com/CentOS/automotive/sample-images/-/merge_requests/457/diffs

do a

cd osbuild-manifests/
sudo make cs9-qemu-initoverlayfs-ostree.x86_64.qcow2

see if it builds ok, boots ok.

You'd need to use an rpm-ostree rpm from this branch and build an initoverlayfs rpm from it's main branch.

dougsland · 2024-02-15T05:39:07Z

Btw, a nice simple way to test this would be to pull this draft PR branch:

https://gitlab.com/CentOS/automotive/sample-images/-/merge_requests/457/diffs

do a
cd osbuild-manifests/
sudo make cs9-qemu-initoverlayfs-ostree.x86_64.qcow2
see if it builds ok, boots ok.

You'd need to use an rpm-ostree rpm from this branch and build an initoverlayfs rpm from it's main branch.

Thanks @ericcurtin going to execute some tests.

rust/src/cliwrap/kernel_install.rs

ericcurtin

This LGTM, initoverlayfs technically isn't a ramdisk, but the logic here is fine...

dougsland · 2024-02-20T16:11:39Z

This LGTM, initoverlayfs technically isn't a ramdisk, but the logic here is fine...

let's fix it.

ericcurtin

LGTM

Run initoverlayfs in case the binary exists. Signed-off-by: Douglas Schilling Landgraf <[email protected]>

cgwalters

Did you test this? I don't believe it can work as is. I think we'll want an integration test here; I can help with that. There's two paths, one in just doing a base image (treecompose) build, and the other as more of a "kola test" which are like TMT tests that are intended to run live on an existing system.

cgwalters · 2024-02-24T15:24:37Z

rust/src/cliwrap/kernel_install.rs

+    let mut initfs_tool = String::new();
+    let initoverlayfs_path = Path::new(INITOVERLAY_INSTALL_CMD);
+    if initoverlayfs_path.exists() {
+        initfs_tool = INITOVERLAY_INSTALL_CMD.to_string()
+    } else {
+        initfs_tool = "dracut".to_string()
+    }


This can be simplified to a one liner like this:

Suggested change

let mut initfs_tool = String::new();

let initoverlayfs_path = Path::new(INITOVERLAY_INSTALL_CMD);

if initoverlayfs_path.exists() {

initfs_tool = INITOVERLAY_INSTALL_CMD.to_string()

} else {

initfs_tool = "dracut".to_string()

}

let initfs_tool = Path::new(INITOVERLAY_INSTALL_CMD).exists().then_some(INITOVERLAY_INSTALL_COMMAND).unwrap_or("dracut");

It's definitely more idiomatic to assign the result of a condition; the use of then_some as a combinator is more a taste thing.

In fact an instance of this pattern is right below:

let dracut_path = cliwrap_dracut .exists() .then(|| cliwrap_dracut) .unwrap_or_else(|| Utf8Path::new("dracut").to_owned());

cgwalters · 2024-02-24T15:28:58Z

rust/src/cliwrap/kernel_install.rs

-#[context("Running dracut")]
-fn run_dracut(kernel_dir: &str) -> Result<()> {
+#[context("Generate initfs")]
+fn generate_initfs(kernel_dir: &str) -> Result<()> {


Sorry I should have mentioned this earlier but you have found a pre-existing confusing mess because we have two places that we run dracut, one here and the other in rpmostree-kernel.cxx. I am pretty sure what you really want is to modify rpmostree-kernel.cxx because it's the one that runs when creating a "base image" aka new commit. This code is trying to wrap the dracut command when invoked on the client side (which can be convenient for testing I guess).

cgwalters · 2024-02-24T15:31:47Z

rust/src/cliwrap/kernel_install.rs

+    } else {
+        initfs_tool = "dracut".to_string()
+    }
+    let cliwrap_dracut = Utf8Path::new(cliwrap::CLIWRAP_DESTDIR).join(initfs_tool);


I don't think this is right, because we aren't cliwrapping the initoverlay command. So combining with the above, we should just directly assign to dracut_path below.

ericcurtin · 2024-02-25T10:55:11Z

The one thing I was wondering @cgwalters was do you think this should be configurable or a case of if it exists use it?

If it exists, just use it seems ok because if you install the initoverlayfs package as a build dependency you probably want to use it.

But I could see why it might be better for it to be configurable also.

For example it's configurable in osbuild in that you can set it to true or false but it's not so configurable here.

I wasn't expecting initoverlayfs to work with rpm-ostree (but we can make it work in this PR if needs be I'm easy 😊) end to end on merge of this PR, the main thing for me right now is that this doesn't break the existing initramfs flow.

cgwalters · 2024-02-25T14:17:21Z

The one thing I was wondering @cgwalters was do you think this should be configurable or a case of if it exists use it?

There's very likely a need to configure it explicitly in general, yes. Changing global behavior just because a package happens to be installed can be OK, but still wants a mechanism to disable.

I wasn't expecting initoverlayfs to work with rpm-ostree (but we can make it work in this PR if needs be I'm easy 😊) end to end on merge of this PR, the main thing for me right now is that this doesn't break the existing initramfs flow.

I don't quite understand the point of merging a PR that doesn't improve anything?

Backing up a minute...have you considered making initoverlayfs a dracut plugin/module instead of a wrapper? Dracut already has configuration mechanism, pretending initoverlayfs is like a "module" would bring that for free, and also mean that zero changes are required to invoking tools like osbuild/rpm-ostree/etc.

ericcurtin · 2024-02-25T14:56:32Z

The one thing I was wondering @cgwalters was do you think this should be configurable or a case of if it exists use it?

There's very likely a need to configure it explicitly in general, yes. Changing global behavior just because a package happens to be installed can be OK, but still wants a mechanism to disable.

I wasn't expecting initoverlayfs to work with rpm-ostree (but we can make it work in this PR if needs be I'm easy 😊) end to end on merge of this PR, the main thing for me right now is that this doesn't break the existing initramfs flow.

I don't quite understand the point of merging a PR that doesn't improve anything?

Just to make small incremental improvements, so we can do partial testing, progress...

But we don't have to of course, it's no biggie...

Backing up a minute...have you considered making initoverlayfs a dracut plugin/module instead of a wrapper? Dracut already has configuration mechanism, pretending initoverlayfs is like a "module" would bring that for free, and also mean that zero changes are required to invoking tools like osbuild/rpm-ostree/etc.

It has a dracut module, but you still need a wrapper for some things, for example you need to call dracut twice once for mini-initrd which can be super small but you still need at least a small initrd to mounting erofs and an overlay... And once for the initoverlayfs aka erofs... You need to call some things outside of dracut modules to piece these two things together...

cgwalters · 2024-02-27T20:33:23Z

so we can do partial testing, progress...

It seems to me like "partial testing" is best done pre-merge; I don't have strong opposition to merging experimental things (we have many here) but this PR simply cannot work as is (AFAICS) so I don't see what the value of merging would be.

It has a dracut module, but you still need a wrapper for some things, for example you need to call dracut twice once for mini-initrd which can be super small but you still need at least a small initrd to mounting erofs and an overlay... And once for the initoverlayfs aka erofs... You need to call some things outside of dracut modules to piece these two things together...

Right, I think we'd need to do something like run as a dracut wrapper and as a module. The wrapper would do something like:

run dracut
check if the resulting initramfs had the initoverlayfs enabled
if so, then perform the splitting operations
if not, we're done

or so? A bit hacky...but means that "enable initoverayfs" could be done by dropping in a dracut setting (see e.g. things like https://github.com/coreos/fedora-coreos-config/blob/testing-devel/overlay.d/05core/usr/lib/dracut/dracut.conf.d/coreos-omits.conf )

ericcurtin · 2024-02-29T19:26:58Z

so we can do partial testing, progress...

It seems to me like "partial testing" is best done pre-merge; I don't have strong opposition to merging experimental things (we have many here) but this PR simply cannot work as is (AFAICS) so I don't see what the value of merging would be.

It has a dracut module, but you still need a wrapper for some things, for example you need to call dracut twice once for mini-initrd which can be super small but you still need at least a small initrd to mounting erofs and an overlay... And once for the initoverlayfs aka erofs... You need to call some things outside of dracut modules to piece these two things together...

Right, I think we'd need to do something like run as a dracut wrapper and as a module. The wrapper would do something like:

run dracut

check if the resulting initramfs had the initoverlayfs enabled

if so, then perform the splitting operations

if not, we're done

or so? A bit hacky...but means that "enable initoverayfs" could be done by dropping in a dracut setting (see e.g. things like https://github.com/coreos/fedora-coreos-config/blob/testing-devel/overlay.d/05core/usr/lib/dracut/dracut.conf.d/coreos-omits.conf )

This is roughly what we do already in:

https://github.com/containers/initoverlayfs/blob/main/bin/initoverlayfs-install

Here are the two dracut calls for example:

     "initoverlayfs_builder dracut -M -o \"initoverlayfs fcoe\"" \
     "initrd_builder dracut -M -m \"kernel-modules udev-rules initoverlayfs systemd base\" -o \"bash systemd-initrd i18n kernel-modules-extra rootfs-block dracut-systemd usrmount fs-lib microcode_ctl-fw_dir_override shutdown nss-softokn\"" > $INITOVERLAYFS_CONF

One builds with just "kernel-modules udev-rules initoverlayfs systemd base", that's kinda the minimum required to bring up a storage device. The other builds the whole thing except for initoverlayfs dracut module, like you say.

Thanks for pointing us towards coreos-omits.conf , that might be better long term than using dracut CLI.

It's gonna be a bit hacky, there's no way to call things like mkfs.erofs, dracut (with the random cpio appended), python (from osbuild), bash, Rust/C/C++, etc. together without things being a bit hacky (it arguably already is).

But at least in bootc/rpm-ostree/ostree world this is all built server-side so there's a controlled environment.

There is also gonna be another mode (at least it's gonna be there experimentally) specifically for Automotive that builds a systemd-less initrd. We started building some storage drivers directly into the kernel and have found kernelspace is much faster at initializing storage than udev like 300ms faster and Automotive actually cares about these ms.

This mode won't be suitable for non-Automotive generic distros though, in other words all the CentOS Stream/Fedora ones that aren't using Automotive kernels.

openshift-ci bot added the needs-ok-to-test label Dec 9, 2023

openshift-ci bot added the do-not-merge/hold label Dec 9, 2023

dougsland force-pushed the initoverlayfs branch from 11d5d2e to cab12f0 Compare December 9, 2023 21:38

cgwalters requested changes Dec 10, 2023

View reviewed changes

cgwalters added priority/medium difficulty/medium medium complexity/difficutly issue labels Dec 11, 2023

ericcurtin reviewed Dec 12, 2023

View reviewed changes

src/daemon/rpm-ostreed.conf Outdated Show resolved Hide resolved

ericcurtin mentioned this pull request Jan 17, 2024

ostree integration containers/initoverlayfs#37

Open

ericcurtin mentioned this pull request Feb 11, 2024

Add support for initoverlay ostreedev/ostree#3066

Open

dougsland force-pushed the initoverlayfs branch from cab12f0 to ac4b1fb Compare February 14, 2024 05:46

dougsland changed the title ~~initoverlayfs: Add initial initoverlayfs support~~ [WIP] initoverlayfs: Add initial initoverlayfs support Feb 14, 2024

openshift-ci bot added the do-not-merge/work-in-progress label Feb 14, 2024

dougsland commented Feb 14, 2024

View reviewed changes

rust/src/cliwrap/kernel_install.rs Outdated Show resolved Hide resolved

dougsland force-pushed the initoverlayfs branch from ac4b1fb to 80c923b Compare February 14, 2024 06:04

dougsland requested a review from cgwalters February 14, 2024 06:05

dougsland force-pushed the initoverlayfs branch 3 times, most recently from 1561c83 to 85624b5 Compare February 14, 2024 06:29

cgwalters requested changes Feb 14, 2024

View reviewed changes

rust/src/cliwrap/kernel_install.rs Outdated Show resolved Hide resolved

ericcurtin reviewed Feb 14, 2024

View reviewed changes

rust/src/cliwrap/kernel_install.rs Outdated Show resolved Hide resolved

dougsland force-pushed the initoverlayfs branch 3 times, most recently from 96e5270 to 1da68db Compare February 15, 2024 05:30

dougsland force-pushed the initoverlayfs branch from 1da68db to cfd48a7 Compare February 19, 2024 20:49

ericcurtin reviewed Feb 19, 2024

View reviewed changes

rust/src/cliwrap/kernel_install.rs Outdated Show resolved Hide resolved

dougsland force-pushed the initoverlayfs branch 2 times, most recently from b3ece01 to 9700f0c Compare February 20, 2024 16:07

ericcurtin approved these changes Feb 20, 2024

View reviewed changes

dougsland force-pushed the initoverlayfs branch from 9700f0c to b97349d Compare February 20, 2024 16:14

dougsland changed the title ~~[WIP] initoverlayfs: Add initial initoverlayfs support~~ initoverlayfs: Add initial initoverlayfs support Feb 20, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 20, 2024

dougsland force-pushed the initoverlayfs branch 2 times, most recently from a08a2b7 to 4a71149 Compare February 20, 2024 16:19

ericcurtin approved these changes Feb 20, 2024

View reviewed changes

initoverlayfs: Add initial initoverlayfs support

e2dbedd

Run initoverlayfs in case the binary exists. Signed-off-by: Douglas Schilling Landgraf <[email protected]>

dougsland force-pushed the initoverlayfs branch from 4a71149 to e2dbedd Compare February 20, 2024 16:33

dougsland requested a review from cgwalters February 20, 2024 19:36

ericcurtin mentioned this pull request Feb 23, 2024

Integrate veritysetup format command for dm-verity hash generation containers/initoverlayfs#74

Merged

cgwalters requested changes Feb 24, 2024

View reviewed changes

dougsland mentioned this pull request Mar 1, 2024

docs: Add doc about build rpm-ostree+autosd #4849

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initoverlayfs: Add initial initoverlayfs support #4721

initoverlayfs: Add initial initoverlayfs support #4721

dougsland commented Dec 9, 2023 •

edited

Loading

openshift-ci bot commented Dec 9, 2023

dougsland commented Dec 9, 2023

cgwalters left a comment •

edited

Loading

cgwalters commented Dec 10, 2023

dougsland commented Dec 11, 2023

cgwalters commented Dec 11, 2023

dougsland commented Dec 12, 2023

dougsland commented Dec 12, 2023

cgwalters commented Dec 15, 2023

ericcurtin commented Dec 16, 2023

dougsland commented Dec 18, 2023

cgwalters left a comment

cgwalters commented Feb 14, 2024

ericcurtin commented Feb 14, 2024 •

edited

Loading

ericcurtin commented Feb 14, 2024

dougsland commented Feb 15, 2024

ericcurtin left a comment

dougsland commented Feb 20, 2024

ericcurtin left a comment

cgwalters left a comment

cgwalters Feb 24, 2024

cgwalters Feb 24, 2024

cgwalters Feb 24, 2024

ericcurtin commented Feb 25, 2024 •

edited

Loading

cgwalters commented Feb 25, 2024

ericcurtin commented Feb 25, 2024 •

edited

Loading

cgwalters commented Feb 27, 2024

ericcurtin commented Feb 29, 2024 •

edited

Loading

initoverlayfs: Add initial initoverlayfs support #4721

Are you sure you want to change the base?

initoverlayfs: Add initial initoverlayfs support #4721

Conversation

dougsland commented Dec 9, 2023 • edited Loading

openshift-ci bot commented Dec 9, 2023

dougsland commented Dec 9, 2023

cgwalters left a comment • edited Loading

Choose a reason for hiding this comment

cgwalters commented Dec 10, 2023

dougsland commented Dec 11, 2023

cgwalters commented Dec 11, 2023

dougsland commented Dec 12, 2023

dougsland commented Dec 12, 2023

cgwalters commented Dec 15, 2023

ericcurtin commented Dec 16, 2023

dougsland commented Dec 18, 2023

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters commented Feb 14, 2024

ericcurtin commented Feb 14, 2024 • edited Loading

ericcurtin commented Feb 14, 2024

dougsland commented Feb 15, 2024

ericcurtin left a comment

Choose a reason for hiding this comment

dougsland commented Feb 20, 2024

ericcurtin left a comment

Choose a reason for hiding this comment

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters Feb 24, 2024

Choose a reason for hiding this comment

cgwalters Feb 24, 2024

Choose a reason for hiding this comment

cgwalters Feb 24, 2024

Choose a reason for hiding this comment

ericcurtin commented Feb 25, 2024 • edited Loading

cgwalters commented Feb 25, 2024

ericcurtin commented Feb 25, 2024 • edited Loading

cgwalters commented Feb 27, 2024

ericcurtin commented Feb 29, 2024 • edited Loading

dougsland commented Dec 9, 2023 •

edited

Loading

cgwalters left a comment •

edited

Loading

ericcurtin commented Feb 14, 2024 •

edited

Loading

ericcurtin commented Feb 25, 2024 •

edited

Loading

ericcurtin commented Feb 25, 2024 •

edited

Loading

ericcurtin commented Feb 29, 2024 •

edited

Loading