Skip to content

Support setting what type of mount configuration to set in disk.yaml - #1834

Merged
achilleas-k merged 1 commit into
osbuild:mainfrom
alexlarsson:mount-config-option
Sep 11, 2025
Merged

achilleas-k merged 1 commit into
osbuild:mainfrom
alexlarsson:mount-config-option

Conversation

@alexlarsson

Copy link
Copy Markdown
Contributor

This extends the current internal boolean mountUnits options to an enum that also allows to disable both fstab and mount units.

Additionally, it also adds the option "mount_configuration" to the bootc disk.yaml embedded config file, allowing bootc images to configure this.

For now, distro.ImageConfig still has MountUnits, because changing that would break the yaml format. However, this could be changed in the future.

MOUNT_CONFIGURATION_FSTAB has value 0, to make the default match the default for a mountUnit boolean (i.e. false).

Note: The previous MountUnits option also affected the generated kernel config options in some cases. We do the same now even if the option is set to none.

@alexlarsson
alexlarsson requested a review from a team as a code owner September 4, 2025 14:23
@alexlarsson
alexlarsson requested review from lzap, mvo5 and thozza and removed request for a team September 4, 2025 14:23
@achilleas-k
achilleas-k self-requested a review September 4, 2025 14:24
@supakeen

supakeen commented Sep 4, 2025 •

Copy link
Copy Markdown
Member

I know I'm on PTO but please take a look at this branch as well where I implemented the where each part of the 'how to mount' is a separate option: main...supakeen:images:train/refactor-mount-config (though maybe we want to group around some 'strategy' enum).

With the above branch I build fully auto-gpt images, I was planning to submit it after I get back, they probably won't conflict too much but maybe some bits can be shared/adjusted to eachother.

@alexlarsson

Copy link
Copy Markdown
Contributor Author

@supakeen These branches are sort of complementary, as your focuses a lot more on the distro yaml format, which mine leaves as is. However, I prefer how my branch uses an enum rather than two bools for fstab/units/none, because it doesn't really make any sense to allow both fstab and units.

I see you have split out the root_partition_kernel arg handling to its own thing though, and that also makes a lot of sense.

So, I think keeping my work as is and then rebasing your work on that is going to be good.

@supakeen

supakeen commented Sep 4, 2025 •

Copy link
Copy Markdown
Member

@supakeen These branches are sort of complementary, as your focuses a lot more on the distro yaml format, which mine leaves as is. However, I prefer how my branch uses an enum rather than two bools for fstab/units/none, because it doesn't really make any sense to allow both fstab and units.

I see you have split out the root_partition_kernel arg handling to its own thing though, and that also makes a lot of sense.

So, I think keeping my work as is and then rebasing your work on that is going to be good.

Sounds good to me, they'll probably merge together fine but I wanted to show my homework as well (I considered a mount.strategy enum myself) in case it's useful (and staying aligned between the two implementations is nice) :)

achilleas-k
achilleas-k previously approved these changes Sep 4, 2025

@achilleas-k achilleas-k left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you! I like how this turned out.

@achilleas-k

Copy link
Copy Markdown
Member

@supakeen Thanks for noticing this PR. I must admit I forgot you were working on the other side of this. Looking forward to getting this into the image config as well.

@mvo5 mvo5 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you! This looks super nice. +0.9 already. One question about a (maybe?) accidental line but otherwise consider it approved.

Comment thread pkg/manifest/os_test.go
Comment thread pkg/osbuild/disk.go
Comment thread pkg/osbuild/disk.go Outdated
Comment thread pkg/osbuild/disk.go Outdated

@mvo5 mvo5 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Love it! Thank you

@alexlarsson

Copy link
Copy Markdown
Contributor Author

CI unit test for image-builder-cli fails with:

2025-09-08T08:06:05.9116350Z === RUN TestManifestIntegrationOstreeSmokeErrors
2025-09-08T08:06:05.9116425Z Manifest generation step
2025-09-08T08:06:05.9116517Z Building manifest for fedora-42-iot-raw-xz
2025-09-08T08:06:05.9116586Z main_test.go:310:
2025-09-08T08:06:05.9116835Z Error Trace: /__w/images/images/image-builder-cli/cmd/image-builder/main_test.go:310
2025-09-08T08:06:05.9116924Z Error: Error message not equal:
2025-09-08T08:06:05.9117055Z expected: "iot-raw-xz: ostree commit URL required"
2025-09-08T08:06:05.9117291Z actual : "options validation failed for image type "iot-raw-xz": ostree.url: required"
2025-09-08T08:06:05.9117426Z Test: TestManifestIntegrationOstreeSmokeErrors
2025-09-08T08:06:05.9117498Z Manifest generation step
2025-09-08T08:06:05.9117599Z Building manifest for fedora-42-server-qcow2
2025-09-08T08:06:05.9117757Z --- FAIL: TestManifestIntegrationOstreeSmokeErrors (14.13s)

However, I get the same with osbuild-images HEAD, without this MR.

@alexlarsson

Copy link
Copy Markdown
Contributor Author

Git bisect says this is the commit that broke it:

1c70e97 is the first bad commit
commit 1c70e97
Author: Achilleas Koutsou achilleas@koutsou.net
Date: Wed Sep 3 00:54:36 2025 +0200

distro/generic: fix ostree url requirement for Fedora IoT disk images

Fedora IoT disk images (iot-qcow2, iot-raw-xz) require an ostree URL to
pull a commit from, but we never checked for it.  When generating
manifests, the manifest instantiation function did produce an error, but
we should have been catching it in the options checker first.

Check for Bootable images as well as BootISO images with RPMOSTree when
validating the required ostree options.

pkg/distro/generic/fedora_test.go | 2 +-
pkg/distro/generic/options.go | 2 +-
pkg/distro/generic/options_test.go | 12 +++++++-----
3 files changed, 9 insertions(+), 7 deletions(-)

@alexlarsson

Copy link
Copy Markdown
Contributor Author

And indeed, the unit test fails in the MR for that: https://github.com/osbuild/images/pull/1823/checks

@achilleas-k

Copy link
Copy Markdown
Member

CI unit test for image-builder-cli fails with:

2025-09-08T08:06:05.9116350Z === RUN TestManifestIntegrationOstreeSmokeErrors
2025-09-08T08:06:05.9116425Z Manifest generation step
2025-09-08T08:06:05.9116517Z Building manifest for fedora-42-iot-raw-xz
2025-09-08T08:06:05.9116586Z main_test.go:310:
2025-09-08T08:06:05.9116835Z Error Trace: /__w/images/images/image-builder-cli/cmd/image-builder/main_test.go:310
2025-09-08T08:06:05.9116924Z Error: Error message not equal:
2025-09-08T08:06:05.9117055Z expected: "iot-raw-xz: ostree commit URL required"
2025-09-08T08:06:05.9117291Z actual : "options validation failed for image type "iot-raw-xz": ostree.url: required"
2025-09-08T08:06:05.9117426Z Test: TestManifestIntegrationOstreeSmokeErrors
2025-09-08T08:06:05.9117498Z Manifest generation step
2025-09-08T08:06:05.9117599Z Building manifest for fedora-42-server-qcow2
2025-09-08T08:06:05.9117757Z --- FAIL: TestManifestIntegrationOstreeSmokeErrors (14.13s)

However, I get the same with osbuild-images HEAD, without this MR.

yeah, the reverse dependency integration test has been updated to not fail in these cases now.

This extends the current internal boolean mountUnits options to an
enum that also allows to disable both fstab and mount units.

Additionally, it also adds the option "mount_configuration" to the
bootc disk.yaml embedded config file, allowing bootc images to
configure this.

For now, distro.ImageConfig still has MountUnits, because changing
that would break the yaml format. However, this could be changed
in the future.

MOUNT_CONFIGURATION_FSTAB has value 0, to make the default match the
default for a mountUnit boolean (i.e. false).

Note: The previous MountUnits option also affected the generated
kernel config options in some cases. We do the same now even if the
option is set to none.
@achilleas-k
achilleas-k added this pull request to the merge queue Sep 11, 2025
Merged via the queue into osbuild:main with commit 24b7228 Sep 11, 2025
25 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