feat: initial commit for erofs support#25
Conversation
rchincha
force-pushed
the
ero
branch
5 times, most recently
from
d0480ec to
4f4ab76
Compare
|
^ have a stacker PR that builds against this PR. |
rchincha
requested review from
ariel-miculas,
hallyn,
mikemccracken,
raharper and
rchamarthy
Just one bats test is failing |
rchincha
force-pushed
the
ero
branch
3 times, most recently
from
1b6bc44 to
5940164
Compare
|
Thanks for the update. I think I'm still missing something. I see |
| tmpErofs.Close() | ||
| os.Remove(tmpErofs.Name()) | ||
| defer os.Remove(tmpErofs.Name()) | ||
| args := []string{tmpErofs.Name(), rootfs} | ||
| compression := GzipCompression |
There was a problem hiding this comment.
This probably should be parameter right?
rchincha
force-pushed
the
ero
branch
2 times, most recently
from
fb69535 to
3541b62
Compare
Codecov ReportAttention: Patch coverage is
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. 🚀 New features to boost your workflow:
|
rchincha
force-pushed
the
ero
branch
6 times, most recently
from
88dfd93 to
9dc7dfd
Compare
| func erofsfuseSupportsMountNotification(erofsfuse string) (string, bool) { | ||
| cmd := exec.Command(erofsfuse) | ||
|
|
||
| // `erofsfuse` always returns an error... so we ignore it. |
There was a problem hiding this comment.
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)
}
|
As expected the CI now passes with the new stacker release from my private branch. |
|
erofsfuse - there is really no version that supports notify. We can remove that path completely. |
| func ExtractSingleErofs(erofsFile string, extractDir string) error { | ||
| exPolInfo.once.Do(func() { | ||
| const envName = "STACKER_EROFS_EXTRACT_POLICY" | ||
| const defPolicy = "kmount erofsfuse fsc.erofs" |
There was a problem hiding this comment.
default policy should probably be exported variable so callers can use it.
Individual policies should be const export variables.
There was a problem hiding this comment.
Looking for parity with squash. I would defer these changes to a different PR if we need it.
| // Ignore errs here as `mkerofs --help` exit status code is 1 | ||
| _ = cmd.Run() | ||
|
|
||
| if strings.Contains(stdoutBuffer.String(), "zstd") || |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
zstd is the prefered compression, if supported and we can get there. For erofs, we cannot just yet.
rchincha
force-pushed
the
ero
branch
2 times, most recently
from
ce923b0 to
8bba1a6
Compare
|
^ 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
left a comment
raharper
left a comment
There was a problem hiding this comment.
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. |
Yes. |
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 <rchincha@cisco.com>
Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>
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 <rchincha@cisco.com>
Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>
Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>
I'm satisfied with them being recorded in issue #29. If anything you've mentioned here is missing there, please let me know. |
4 participants
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