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.

Sign up for GitHub

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

Jump to bottom

Add erofs support #1117

Merged
merged 7 commits into from
Jan 14, 2025
Merged

Add erofs support #1117

merged 7 commits into from
Jan 14, 2025

Conversation

bcl
Copy link
Contributor

@bcl bcl commented Dec 20, 2024 •
edited by ondrejbudai
Loading

Things that are needed:

/jira-epic RHELBU-2806

@bcl
Copy link
Contributor Author

bcl commented Dec 20, 2024

Currently works to build an erofs image-installer image on RHEL10 but will not boot due to the kernel not supporting zstd on erofs.

@bcl bcl force-pushed the main-rootfs-erofs branch from 2544bd2 to 0c05cbb Compare December 20, 2024 23:37
@ondrejbudai ondrejbudai requested a review from mvo5 January 6, 2025 10:08
mvo5
mvo5 reviewed Jan 6, 2025
View reviewed changes
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.

This looks very nice, one question about a possible test. Also, what is keeping this in draft? It seems quite complete (code-wise) or am I missing something?

pkg/manifest/anaconda_installer_iso_tree.go Show resolved Hide resolved
pkg/manifest/anaconda_installer_iso_tree.go Show resolved Hide resolved
@supakeen supakeen mentioned this pull request Jan 8, 2025
Open
@thozza thozza force-pushed the main-rootfs-erofs branch from 0c05cbb to c201271 Compare January 9, 2025 12:15
@thozza
Copy link
Member

thozza commented Jan 9, 2025

Rebased due to image build cache layout change (#1130)

@bcl bcl force-pushed the main-rootfs-erofs branch from c201271 to 9cc0d18 Compare January 10, 2025 19:28
@bcl bcl marked this pull request as ready for review January 10, 2025 19:28
@bcl bcl changed the title Add erofs support and use it on RHEL 10 Add erofs support Jan 10, 2025
@bcl
Copy link
Contributor Author

bcl commented Jan 10, 2025

RHEL10 will be a followup commit, added tests as suggested by @mvo5

@bcl bcl requested a review from mvo5 January 10, 2025 19:29
supakeen
supakeen previously approved these changes Jan 10, 2025
View reviewed changes
Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

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

Very nice. I would only wonder if we want to make RootfsCompression also some kinda-enum; or if we want to move them together into a single property just called rootfs that contains both the rootfstype and the rootfscompression, just so we can (more easily) validate them together in case some compressions do not fit some rootfstypes?

@bcl
Copy link
Contributor Author

bcl commented Jan 10, 2025

I would only wonder if we want to make RootfsCompression also some kinda-enum;

I pondered that, but chose to leave it free-form for now. It's used by different compression stages so I don't think it makes sense to make it an enum when the allowed values are really up to the osbuild stage.

mvo5
mvo5 previously approved these changes Jan 13, 2025
View reviewed changes
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.

Thank you! This looks very nice (small idea inline for your consideration but no blockers from my pov).

pkg/manifest/anaconda_installer_iso_tree_test.go Outdated Show resolved Hide resolved
pkg/manifest/anaconda_installer_iso_tree_test.go Outdated Show resolved Hide resolved
pkg/manifest/anaconda_installer_iso_tree_test.go Outdated Show resolved Hide resolved
pkg/osbuild/erofs_stage.go Show resolved Hide resolved
thozza
thozza previously approved these changes Jan 13, 2025
View reviewed changes
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

LGTM

achilleas-k
achilleas-k reviewed Jan 13, 2025
View reviewed changes
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.

LGTM but I'd like to see the comments resolved. They're all minor (the append()ing and the test that @mvo5 already wrote), but good to have it clean from the start.

I don't mind it being a follow-up PR if merging this unblocks other work.

bcl and others added 7 commits January 13, 2025 14:46
@bcl
erofs_stage: Add support for osbuild org.osbuild.erofs stage
5cfc45d 
Supports passing the compression type, extended erofs options, and
maximum compress physical cluster size.
@bcl
Rename SquashfsCompression to RootfsCompression
1ae8c7d 
In preparation for adding erofs support rename the variable since it
will be used for more than squashfs.
@bcl
manifest: Add RootfsType to the AnacondaInstallerISOTree
a3760a7 
This will make it easier to select other rootfs types without depending
on side-effects like rootfsPipeline being nil.
@bcl
manifest: Move squashfs stage creation into NewSquashfsStage
d3c20e6
@bcl
manifest: Add support for plain erofs root filesystem on the iso
e644d73 
Add support for erofs by setting the RootfsType on
AnacondaInstallerISOTree to ErofsRootfs to select a plain erofs
compressed root filesystem for the Anaconda ISO.

The mkfs.erofs arguments will look like this:
zstd,8 -E all-fragments,dedupe -C 131072
@bcl
manifest: Add checks for the rootfs stages
77d2a8b 
Add tests to make sure the squashfs or erofs (without squashfs) stages
are present in the manifest when the RootfsType is set.
@mvo5 @bcl
osbuild: add minimal json test for the erofs stage
85183d3 
This commit adds a minimal json test for the erof stage to ensure
that the json struct tags are correct and to also in the
"test-as-documentation" sense to hint at what the json looks like.
@bcl bcl dismissed stale reviews from thozza, mvo5, and supakeen via 85183d3 January 13, 2025 22:57
@bcl bcl force-pushed the main-rootfs-erofs branch from 9cc0d18 to 85183d3 Compare January 13, 2025 22:57
@mvo5 mvo5 enabled auto-merge January 14, 2025 08:29
@mvo5 mvo5 added this pull request to the merge queue Jan 14, 2025
Merged via the queue into osbuild:main with commit afcde39 Jan 14, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achilleas-k achilleas-k achilleas-k left review comments

@thozza thozza thozza approved these changes

@supakeen supakeen supakeen left review comments

@mvo5 mvo5 mvo5 left review comments

@ondrejbudai ondrejbudai Awaiting requested review from ondrejbudai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bcl @thozza @mvo5 @achilleas-k @supakeen