Skip to content

Conversation

@nikita-dubrovskii
Copy link
Contributor

This adds possibility to specify filesystem of rootfs.img.

Issue: coreos/fedora-coreos-tracker#1852

@dustymabe
Copy link
Contributor

      "properties": {
        "live_rootfs_fstype": {
          "descripton": "Filesystem of live-rootfs.img",
          "type": "string",
          "enum": [
            "erofs",
            "squashfs"
          ],
          "default": "squashfs"
        },
        "live_rootfs_fsoptions": {
          "descripton": "Parameters for generating live-rootfs.img",
          "type": "string",
          "default": "-comp zstd"
        },

We discussed this in a meeting last week with @mvo5. What we (CoreOS team) would really like to do here is actually define these settings inside of the payload that is the CoreOS (Fedora CoreOS or RHEL CoreOS) filesystem/target image.

The question we have here is: can we inspect and do things based on the target image?

If yes then what's preferred? Should we base our behavior on:

  • target OS versions?
    • Seems hacky to have business-y logic as part of an osbuild stage
  • target OS capabilities?
    • i.e. if erofs is supported then do erofs
  • some configuration files inside the target OS?
    • (e.g. bootc print-config/or file)

Our preference at this point is to base it on some configuration intentionally applied inside the target image.

@achilleas-k achilleas-k self-requested a review February 5, 2025 13:55
@nikita-dubrovskii
Copy link
Contributor Author

We discussed this in a meeting last week with @mvo5. What we (CoreOS team) would really like to do here is actually define these settings inside of the payload that is the CoreOS (Fedora CoreOS or RHEL CoreOS) filesystem/target image.

Right, i've opened this PR to start the discussion. Reading configuration file(s) is in progress right now.

@mvo5 mvo5 self-requested a review February 10, 2025 10:46
Copy link
Contributor

@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.

Some quick drive-by code-reivew

As for the questions how to enable erofs/squashfs: in general our policy is that this should come via configuration (like this PR). However AIUI this is not the desired behavior for you so I'm fine with a different approach that looks at e.g. some configuration inside the target and determine based on that if erofs should be used or not. While not something we generally do or encourage this stage here is already unusual so doing seems okay to me.

@nikita-dubrovskii nikita-dubrovskii requested a review from a team as a code owner February 10, 2025 11:53
@nikita-dubrovskii nikita-dubrovskii requested review from schuellerf and thozza and removed request for a team February 10, 2025 11:53
@nikita-dubrovskii
Copy link
Contributor Author

nikita-dubrovskii commented Feb 10, 2025

@mvo5, @dustymabe , @jlebon as discussed, 2nd patch here adds ability to get options from file

@dustymabe
Copy link
Contributor

I'll review and test this today. Thanks @nikita-dubrovskii

Copy link
Contributor

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @nikita-dubrovskii. This mostly LGTM. a few comments!

Copy link
Contributor

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Thanks. Other than Dusty's comments, this all LGTM. Feel free to address those comments in a separate PR if you'd rather have this merged sonner.

@dustymabe dustymabe merged commit ab1f487 into osbuild:main Feb 11, 2025
49 checks passed
@nikita-dubrovskii nikita-dubrovskii deleted the erofs branch February 12, 2025 06:52
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.

5 participants