-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Add support for EROFS fsmerge feature #12374
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.
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
Conversation
96934d4 to
6076bcb
Compare
6076bcb to
69d2a33
Compare
4a2b254 to
fca4a46
Compare
fca4a46 to
53d11cc
Compare
19056af to
c93c21d
Compare
c93c21d to
84a35fc
Compare
2359163 to
8de5fb9
Compare
plugins/snapshots/erofs/erofs.go
Outdated
| td = "" | ||
|
|
||
| if !strings.Contains(key, snapshots.UnpackKeyPrefix) { | ||
| s.generateFsMeta(ctx, snap.ParentIDs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to handle this outside the transaction, in case the command gets stuck?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like it's nice to have. so it should be safe to move it outside the transaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very sorry about this, I thought I replied this but it seems not, yeah, I've moved the generation part out the transaction, thanks for your suggestion!
3a9e16e to
79d8783
Compare
79d8783 to
c7cd13c
Compare
|
@dmcgowan @fuweid @azr @AkihiroSuda @cpuguy83 |
e8502d4 to
f5400b5
Compare
cpuguy83
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although it doesn't look like it's being tested.
I tried, but then I found I'm not sure how to handle this easily, could we address the tests later? |
azr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !
EROFS has supported a tiny metadata-only image to reference external blobs since Linux 5.16. This eliminates the need to mount each EROFS layer one by one and is also useful for VM-based containers (e.g. nerdbox and Kata containers.) Similar to LCOW/CimFS, `snapshots.UnpackKeyPrefix` is used to trigger fsmerge generation (typically < 100 ms) on demand in Prepare(). In the future, we can also generate fsmeta in Commit() of the final unpacking layer (by introducing an annotation to keep the chainID). However, in the case of intermediate layer reuse, the Prepare() handling will still be required. ```toml [plugins."io.containerd.snapshotter.v1.erofs"] max_unmerged_layers = 1 # enable fsmerge if image layers >= 2 ``` Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
f5400b5 to
9a7500a
Compare
a prerequisite for containerd/nerdbox#30
EROFS has supported a tiny metadata-only image to reference external blobs since Linux 5.16. This eliminates the need to mount each EROFS layer one by one and is also useful for VM-based containers (e.g. nerdbox and Kata containers.)
Similar to LCOW/CimFS,
snapshots.UnpackKeyPrefixis used to trigger fsmerge generation (typically < 100 ms) on demand in Prepare().In the future, we can also generate fsmeta in Commit() of the final unpacking layer (by introducing an annotation to keep the chainID). However, in the case of intermediate layer reuse, the Prepare() handling will still be required.
This change is based on #12333merged