feat: initial commit for erofs support #25

rchincha · 2024-11-09T06:49:33Z

A second attempt, converging faster.

pkg/fs <- main interface .. everything calls into here, stacker, atomfs, atomfs-snapshotter
pkg/squashfs
pkg/erofs
pkg/common <- common fs routines .. should probably become "internal"
pkg/verity <- verity stuff
pkg/molecule
pkg/log
pkg/mount

rchincha · 2024-11-10T07:48:19Z

project-stacker/stacker#626

^ have a stacker PR that builds against this PR.

pkg/common/exclude.go

pkg/common/mount.go

pkg/erofs/erofs.go

pkg/erofs/superblock.go

pkg/erofs/verity.go

pkg/fs/fs.go

pkg/verity/verity.go

rchincha · 2024-11-13T03:43:36Z

not ok 1 guestmount works ignoring verity in 130ms                                                                                                                        # (in test file unpriv-guestmount.bats, line 20)                                                                                                                          #   `lxc-usernsexec -s <<EOF' failed                                                                                                                                      # + export ATOMFS_TEST_RUN_DIR=/tmp/bats-run-Hh1tGT/suite/run/atomfs                                                                                                      # + export PERSIST_DIR=/tmp/bats-run-Hh1tGT/test/1/persist-dir
# + mkdir -p /tmp/bats-run-Hh1tGT/test/1/persist-dir                                                                                                                      # + readlink /proc/self/ns/mnt                                                                                                                                            # + cut -c 6-15                                                                                                                                                           # + export INNER_MNTNSNAME=4026533012                                                                                                                                     # + set +e                                                                                                                                                                # + atomfs --debug mount --persist=/tmp/bats-run-Hh1tGT/test/1/persist-dir /tmp/bats-run-Hh1tGT/suite/oci:test-squashfs /tmp/bats-run-Hh1tGT/test/1/testmountpoint
# 2024/11/13 03:39:43 debug -> ws.recurse             digest=sha256:e1f7649d181d720b424c96aa8911f00fb16a9f95be1c7526caef859008de235e
# 2024/11/13 03:39:43 debug <- ws.recurse             digest=sha256:e1f7649d181d720b424c96aa8911f00fb16a9f95be1c7526caef859008de235e                                      # 2024/11/13 03:39:43 debug casext.ResolveReference(test-squashfs) got these descriptors refs=[{[{application/vnd.oci.image.manifest.v1+json sha256:e1f7649d181d720b424c96aa8911f00fb16a9f95be1c7526caef859008de235e 1226 [] map[org.opencontainers.image.ref.name:test-squashfs] [] <nil> }]}]                                                     # 2024/11/13 03:39:43  info Failure detected: cleaning up "/tmp/bats-run-Hh1tGT/suite/run/atomfs/meta/4026533012/tmp-bats-run-Hh1tGT-test-1-testmountpoint"
# Error: won't guestmount an image with verity data without --allow-missing-verity

Just one bats test is failing

raharper · 2024-11-13T22:58:28Z

Thanks for the update. I think I'm still missing something. I see squashfsFile or squashFile passed around in the common and per-fs code... are we really passing in a path or sha256sum of a squashfile?

pkg/common/mount.go

raharper · 2024-11-13T23:09:05Z

pkg/erofs/erofs.go

+	tmpErofs.Close()
+	os.Remove(tmpErofs.Name())
+	defer os.Remove(tmpErofs.Name())
+	args := []string{tmpErofs.Name(), rootfs}
+	compression := GzipCompression


This probably should be parameter right?

pkg/erofs/fs.go

pkg/erofs/verity.go

pkg/verity/verity.go

codecov · 2024-11-16T05:28:59Z

Codecov Report

Attention: Patch coverage is 30.21421% with 619 lines in your changes missing coverage. Please review.

Project coverage is 38.71%. Comparing base (fb656de) to head (c114b13).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/erofs/erofs.go	6.53%	336 Missing and 7 partials ⚠️
pkg/common/mount.go	33.33%	46 Missing and 8 partials ⚠️
pkg/common/exclude.go	0.00%	45 Missing ⚠️
pkg/erofs/fs.go	0.00%	39 Missing ⚠️
pkg/erofs/superblock.go	59.55%	24 Missing and 12 partials ⚠️
pkg/common/utils.go	71.57%	18 Missing and 9 partials ⚠️
pkg/squashfs/fs.go	51.28%	15 Missing and 4 partials ⚠️
pkg/verity/verity.go	68.08%	10 Missing and 5 partials ⚠️
pkg/squashfs/squashfs.go	43.47%	13 Missing ⚠️
pkg/molecule/molecule.go	42.85%	5 Missing and 7 partials ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
- Coverage   45.15%   38.71%   -6.45%     
==========================================
  Files          14       24      +10     
  Lines        1694     2255     +561     
==========================================
+ Hits          765      873     +108     
- Misses        779     1207     +428     
- Partials      150      175      +25

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

raharper · 2025-02-23T19:09:55Z

pkg/erofs/erofs.go

+func erofsfuseSupportsMountNotification(erofsfuse string) (string, bool) {
+	cmd := exec.Command(erofsfuse)
+
+	// `erofsfuse` always returns an error...  so we ignore it.


Copied over from existing squashfs pattern, no change there

Not a fan of the pattern. Let's just be explicit.

package main import ( "fmt" "os" "os/exec" "strings" ) func erofsfuseVersion() (string, error) { cmd := exec.Command("erofsfuse", "--help") out, err := cmd.CombinedOutput() if err != nil { // erofsfuse --help always returns 1 if exitErr, ok := err.(*exec.ExitError); ok { rc := exitErr.ExitCode() switch rc { case 1: break default: return "", fmt.Errorf("erofsfuse --help failed with error code %d: %s", rc, err) } } else { // all other failures handled here, like no erofsfuse command return "", fmt.Errorf("failed to execute 'erofsfuse --help'; %s", err) } } /* parse the erofsfuse version $ erofsfuse --help erofsfuse 1.7.1 usage: [options] IMAGE MOUNTPOINT Options: --offset=# skip # bytes when reading IMAGE --dbglevel=# set output message level to # (maximum 9) --device=# specify an extra device to be used together --help display this help and exit FUSE options: -d -o debug enable debug output (implies -f) -f foreground operation -s disable multi-threaded operation */ // Note: iterating over the output as the line number where the version is // printed seemed to change when exec'ed vs run on the command line lines := strings.Split(string(out), "\n") for _, line := range lines { if strings.HasPrefix(line, "erofsfuse") { return strings.Split(line, " ")[1], nil } } return "", fmt.Errorf("failed to get erofsfuse version") } func erofsfuseSupportsMountNotification() (string, bool, error) { version, err := erofsfuseVersion() if err != nil { return "", false, fmt.Errorf("failed to get erofsfuse version: %s", err) } switch version { case "1.7.1": return version, true, nil } return version, false, fmt.Errorf("erofsfuse version %s does not support mount notification", version) } func main() { version, supportsNotify, err := erofsfuseSupportsMountNotification() if err != nil { fmt.Println(err) os.Exit(1) } fmt.Printf("erofsfuse version %s supports mount notification: %v\n", version, supportsNotify) }

rchincha · 2025-02-24T07:02:05Z

As expected the CI now passes with the new stacker release from my private branch.

rchincha · 2025-02-28T06:36:32Z

erofsfuse - there is really no version that supports notify. We can remove that path completely.

pkg/erofs/erofs.go

raharper · 2025-03-08T22:33:32Z

pkg/erofs/erofs.go

+func ExtractSingleErofs(erofsFile string, extractDir string) error {
+	exPolInfo.once.Do(func() {
+		const envName = "STACKER_EROFS_EXTRACT_POLICY"
+		const defPolicy = "kmount erofsfuse fsc.erofs"


default policy should probably be exported variable so callers can use it.
Individual policies should be const export variables.

Looking for parity with squash. I would defer these changes to a different PR if we need it.

raharper · 2025-03-08T22:42:46Z

pkg/erofs/erofs.go

+		// Ignore errs here as `mkerofs --help` exit status code is 1
+		_ = cmd.Run()
+
+		if strings.Contains(stdoutBuffer.String(), "zstd") ||


Probably should check for the the Available compressors prefix:

% mkfs.erofs --help 2>&1 | grep compressors Available compressors are: lz4hc, lz4

Also, why do we care about the compression used? won't kernel or fuse erofs mount handle the decompression?

Also, not your fault, but mkfs.erofs has radically different output...

-zX[,level=Y] X=compressor (Y=compression level, Z=dictionary size, optional) [,dictsize=Z] alternative compressors can be separated by colons(:) [:...] supported compressors and their option ranges are: lz4 lz4hc [,level=<0-12>] (default=9) deflate [,level=<0-9>] (default=1) [,dictsize=<dictsize>] (default=32768, max=32768) zstd [,level=<0-22>] (default=3) [,dictsize=<dictsize>] (default=<auto>, max=1048576)

we're not checking for deflate, or the different lz4 modes..

zstd is the prefered compression, if supported and we can get there. For erofs, we cannot just yet.

pkg/erofs/erofs.go

pkg/erofs/mediatype.go

rchincha · 2025-03-13T05:58:28Z

#29

^ filed an issue to track post-merge cleanup. There are quite a few. Would keep these two PRs separate in what we want to achieve.

raharper

I do feel like we talk past each other in these reviews. I'm not sure what the value of merging this into atomfs itself if it's functional but with potential bugs/issues and code quality (which was inherited from squashfs) that should be addressed. There are quite a number of items I've pointed out that are ignored or deferred.

My biggest concern is that once approved the PR to fix up what was pointed out won't come; and then a stacker release happens with erofs support and no one will follow up on the to-be fixed items (for squashfs or eorfs).

I'm not trying to block things just because but rather prefer things to be fixed along the way so we don't accumulate additional tech debt.

@hallyn @mikemccracken Thoughts? I don't want to be the only one slowing this down if folks are happy with this PR.

pkg/erofs/mediatype.go

mikemccracken · 2025-03-17T21:44:17Z

I do feel like we talk past each other in these reviews. I'm not sure what the value of merging this into atomfs itself if it's functional but with potential bugs/issues and code quality (which was inherited from squashfs) that should be addressed. There are quite a number of items I've pointed out that are ignored or deferred.

My biggest concern is that once approved the PR to fix up what was pointed out won't come; and then a stacker release happens with erofs support and no one will follow up on the to-be fixed items (for squashfs or eorfs).

I'm not trying to block things just because but rather prefer things to be fixed along the way so we don't accumulate additional tech debt.

@hallyn @mikemccracken Thoughts? I don't want to be the only one slowing this down if folks are happy with this PR.

@raharper thanks for doing the in depth reviews here, I have not really had time, and EROFS support isn't something I have any use for so I was being slow.

@rchincha I also don't fully understand the motivation for postponing fixes. If there's an urgent need to merge this, let us know. I saw that you mentioned upthread that tests won't pass without a private release of stacker - is your plan to merge this ASAP so we can get a real stacker release with erofs support, then come back and update stacker here, and fix any remaining concerns at that point? That circular dep is uncomfortable, but I suppose it's ok to have a release of stacker with experimental erofs support, if we are clear about it.

rchincha · 2025-03-18T22:30:01Z

is your plan to merge this ASAP so we can get a real stacker release with erofs support, then come back and update stacker here, and fix any remaining concerns at that point? That circular dep is uncomfortable, but I suppose it's ok to have a release of stacker with experimental erofs support, if we are clear about it.

Yes.

rchincha · 2025-03-18T22:37:47Z

@raharper thanks for doing the in depth reviews here, I have not really had time, and EROFS support isn't something I have any use for so I was being slow.

Of course, hence issue #29

Before this commit, only squashfs was supported. However, there are other filesystems such as erofs that fit the same theme, and additional filesystem support requires refactoring and exposing a more generic filesystem interface. pkg/fs/fs.go - Filesystem interface pkg/squashfs - squashfs pkg/erofs - erofs pkg/common - filesystem-agnostic common routines pkg/verity - verity routines Signed-off-by: Ramkumar Chinchani <[email protected]>

Signed-off-by: Ramkumar Chinchani <[email protected]>

BREAKING-CHANGE: the layer media-type no longer contains "+verity" For a layer media-type, we add the fstype+compression+verity_present. Only one '+' is allowed as per following RFC. https://datatracker.ietf.org/doc/html/rfc6838#section-4.2 Instead just rely on "root_hash" annotation. Signed-off-by: Ramkumar Chinchani <[email protected]>

Signed-off-by: Ramkumar Chinchani <[email protected]>

hallyn · 2025-03-19T01:26:08Z

I do feel like we talk past each other in these reviews. I'm not sure what the value of merging this into atomfs itself if it's functional but with potential bugs/issues and code quality (which was inherited from squashfs) that should be addressed. There are quite a number of items I've pointed out that are ignored or deferred.

My biggest concern is that once approved the PR to fix up what was pointed out won't come; and then a stacker release happens with erofs support and no one will follow up on the to-be fixed items (for squashfs or eorfs).

I'm not trying to block things just because but rather prefer things to be fixed along the way so we don't accumulate additional tech debt.

@hallyn @mikemccracken Thoughts? I don't want to be the only one slowing this down if folks are happy with this PR.

I'm satisfied with them being recorded in issue #29. If anything you've mentioned here is missing there, please let me know.

rchincha force-pushed the ero branch 5 times, most recently from d0480ec to 4f4ab76 Compare November 10, 2024 07:36

rchincha mentioned this pull request Nov 10, 2024

feat: initial commit for erofs support #14

Closed

rchincha changed the title ~~feat: initial commit~~ feat: initial commit for erofs support Nov 10, 2024

rchincha requested review from mikemccracken, rchamarthy, hallyn, raharper and ariel-miculas November 10, 2024 18:41

raharper requested changes Nov 11, 2024

View reviewed changes

rchincha force-pushed the ero branch from 4f4ab76 to f808b69 Compare November 13, 2024 03:42

rchincha force-pushed the ero branch 3 times, most recently from 1b6bc44 to 5940164 Compare November 13, 2024 03:53

raharper reviewed Nov 13, 2024

View reviewed changes

rchincha force-pushed the ero branch 2 times, most recently from fb69535 to 3541b62 Compare November 16, 2024 05:27

rchincha force-pushed the ero branch 6 times, most recently from 88dfd93 to 9dc7dfd Compare November 17, 2024 06:36

raharper requested changes Feb 23, 2025

View reviewed changes

rchincha force-pushed the ero branch from 7b3f6be to 5148bc6 Compare February 24, 2025 06:52

rchincha force-pushed the ero branch from 5148bc6 to 5484661 Compare February 28, 2025 06:45

rchincha requested a review from raharper March 7, 2025 05:48

raharper requested changes Mar 8, 2025

View reviewed changes

rchincha force-pushed the ero branch 2 times, most recently from ce923b0 to 8bba1a6 Compare March 9, 2025 00:12

rchincha mentioned this pull request Mar 13, 2025

Post-merge cleanup after erofs feature merge #29

Open

rchincha force-pushed the ero branch from 8bba1a6 to 9e2102e Compare March 13, 2025 06:02

raharper requested changes Mar 17, 2025

View reviewed changes

pkg/erofs/mediatype.go Show resolved Hide resolved

Ramkumar Chinchani and others added 6 commits March 18, 2025 20:15

fix: disable "advanced/incompatible" features for now

2a44173

Signed-off-by: Ramkumar Chinchani <[email protected]>

test: add a stacker.yaml for tests

21b98ca

fix: rebasing after Mike's changes

03ba082

Signed-off-by: Ramkumar Chinchani <[email protected]>

fix: some more refactoring

2651a85

hallyn force-pushed the ero branch from 9e2102e to 0f54fac Compare March 19, 2025 01:21

hallyn approved these changes Mar 19, 2025

View reviewed changes

rchincha added 2 commits March 18, 2025 20:23

fix: remove notify support from erofs

64bc8c2

fix: try a released stacker version

c114b13

Signed-off-by: Ramkumar Chinchani <[email protected]>

hallyn force-pushed the ero branch from 0f54fac to c114b13 Compare March 19, 2025 01:24

hallyn approved these changes Mar 19, 2025

View reviewed changes

rchincha merged commit 9f28d93 into project-machine:main Mar 22, 2025
3 of 5 checks passed

feat: initial commit for erofs support #25

feat: initial commit for erofs support #25

Uh oh!

Conversation

rchincha commented Nov 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchincha commented Nov 10, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rchincha commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raharper commented Nov 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raharper Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

raharper Feb 23, 2025

Choose a reason for hiding this comment

Uh oh!

rchincha commented Feb 24, 2025

Uh oh!

rchincha commented Feb 28, 2025

Uh oh!

Uh oh!

Uh oh!

raharper Mar 8, 2025

Choose a reason for hiding this comment

Uh oh!

rchincha Mar 8, 2025

Choose a reason for hiding this comment

Uh oh!

raharper Mar 8, 2025

Choose a reason for hiding this comment

Uh oh!

raharper Mar 8, 2025

Choose a reason for hiding this comment

Uh oh!

rchincha Mar 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rchincha commented Mar 13, 2025

Uh oh!

raharper left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikemccracken commented Mar 17, 2025

Uh oh!

rchincha commented Mar 18, 2025

Uh oh!

rchincha commented Mar 18, 2025

Uh oh!

hallyn commented Mar 19, 2025

Uh oh!

Uh oh!

rchincha commented Nov 9, 2024 •

edited

Loading

rchincha commented Nov 13, 2024 •

edited

Loading

codecov bot commented Nov 16, 2024 •

edited

Loading