Support for building overlaybd images - #3867
Conversation
|
@tonistiigi @AkihiroSuda @ktock |
|
Looks good but needs rebase |
| } | ||
| } else if cm.Snapshotter.Name() == "overlaybd" && parent != nil { | ||
| rwLabels := make(map[string]string) | ||
| rwLabels["containerd.io/snapshot/overlaybd.writable"] = "dev" |
There was a problem hiding this comment.
This is a type of upper layer in overlaybd config that can specify by label, maybe we should provide a const value in overlaybd code.
There was a problem hiding this comment.
When the "containerd.io/snapshot/overlaybd.writable" has been set as "dev", Snapshotter will create a R/W block device directly as rootfs without overlayfs // local directory (upper) + RO overlaybd (lower)
Just like devmapper :D
There was a problem hiding this comment.
Thanks. Could you add that explanation in the code comment lines?
| key := fmt.Sprintf("extract-%s %s", identity.NewID(), sr.getChainID()) | ||
|
|
||
| err = sr.cm.Snapshotter.Prepare(ctx, key, parentID) | ||
| opts := snapshots.WithLabels(map[string]string{"containerd.io/snapshot.ref": string(desc.Digest)}) |
There was a problem hiding this comment.
This should not be enabled by default.
There was a problem hiding this comment.
Fixed, now this label is only for overlaybd.
| if snStat.Labels[obdlabel.LocalOverlayBDPath] != "" { | ||
| isOverlaybd = true | ||
| } | ||
| mediaType = ocispecs.MediaTypeImageLayer |
There was a problem hiding this comment.
This is overwritten regardless if isOverlaybd is true/false, that's correct?
There was a problem hiding this comment.
Sorry, mediaType should be overwritten only when isOverlaybd is true. It is now fixed.
| } | ||
| var lower []mount.Mount | ||
| if lowerRef != nil { | ||
| if !isOverlaybd && lowerRef != nil { |
There was a problem hiding this comment.
The code is becoming quite complex with these conditions added. Quite some branches are excluded if !isOverlaybd, and stripping away that code shows that there's actually some of the other branches that are "dead code" in that case.
Perhaps out of scope for this PR, but I wonder if we need to branch earlier if isOverlaybd is true, instead of all these "micro-conditions".
I did a very quick / naive remove of all the !isOverlaybd paths, to get a "rough" idea what parts were unused, which gave me;
Details
diff --git a/cache/blobs.go b/cache/blobs.go
index a8af0a96f..b3abd863b 100644
--- a/cache/blobs.go
+++ b/cache/blobs.go
@@ -7,7 +7,6 @@ import (
"io"
"os"
"path"
- "strconv"
obdlabel "github.com/containerd/accelerated-container-image/pkg/label"
obdcmd "github.com/containerd/accelerated-container-image/pkg/utils"
@@ -113,97 +112,11 @@ func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool
}
mediaType = ocispecs.MediaTypeImageLayer
}
- var lowerRef *immutableRef
- switch sr.kind() {
- case Diff:
- lowerRef = sr.diffParents.lower
- case Layer:
- lowerRef = sr.layerParent
- }
var lower []mount.Mount
- if !isOverlaybd && lowerRef != nil {
- m, err := lowerRef.Mount(ctx, true, s)
- if err != nil {
- return nil, err
- }
- var release func() error
- lower, release, err = m.Mount()
- if err != nil {
- return nil, err
- }
- if release != nil {
- defer release()
- }
- }
-
- var upperRef *immutableRef
- switch sr.kind() {
- case Diff:
- upperRef = sr.diffParents.upper
- default:
- upperRef = sr
- }
var upper []mount.Mount
- if !isOverlaybd && upperRef != nil {
- m, err := upperRef.Mount(ctx, true, s)
- if err != nil {
- return nil, err
- }
- var release func() error
- upper, release, err = m.Mount()
- if err != nil {
- return nil, err
- }
- if release != nil {
- defer release()
- }
- }
-
var desc ocispecs.Descriptor
var err error
- // Determine differ and error/log handling according to the platform, envvar and the snapshotter.
- var enableOverlay, fallback, logWarnOnErr bool
- if forceOvlStr := os.Getenv("BUILDKIT_DEBUG_FORCE_OVERLAY_DIFF"); forceOvlStr != "" && sr.kind() != Diff {
- enableOverlay, err = strconv.ParseBool(forceOvlStr)
- if err != nil {
- return nil, errors.Wrapf(err, "invalid boolean in BUILDKIT_DEBUG_FORCE_OVERLAY_DIFF")
- }
- fallback = false // prohibit fallback on debug
- } else if !isTypeWindows(sr) {
- enableOverlay, fallback = true, true
- switch sr.cm.Snapshotter.Name() {
- case "overlayfs", "stargz":
- // overlayfs-based snapshotters should support overlay diff except when running an arbitrary diff
- // (in which case lower and upper may differ by more than one layer), so print warn log on unexpected
- // failure.
- logWarnOnErr = sr.kind() != Diff
- case "fuse-overlayfs", "native":
- // not supported with fuse-overlayfs snapshotter which doesn't provide overlayfs mounts.
- // TODO: add support for fuse-overlayfs
- enableOverlay = false
- }
- }
- if !isOverlaybd && enableOverlay {
- computed, ok, err := sr.tryComputeOverlayBlob(ctx, lower, upper, mediaType, sr.ID(), compressorFunc)
- if !ok || err != nil {
- if !fallback {
- if !ok {
- return nil, errors.Errorf("overlay mounts not detected (lower=%+v,upper=%+v)", lower, upper)
- }
- if err != nil {
- return nil, errors.Wrapf(err, "failed to compute overlay diff")
- }
- }
- if logWarnOnErr {
- bklog.G(ctx).Warnf("failed to compute blob by overlay differ (ok=%v): %v", ok, err)
- }
- }
- if ok {
- desc = computed
- }
- }
-
if isOverlaybd {
if err := commitOverlaybd(ctx, sr, &desc); err != nil {
return nil, errThere was a problem hiding this comment.
This code has been modified, because desc will be overwritten in commitOverlaybd, so there is no effect if !isOverlaybd is not added.
| mediaType := comp.Type.MediaType() | ||
|
|
||
| var isOverlaybd = false | ||
| if sr.cm.Snapshotter.Name() == "overlaybd" { |
There was a problem hiding this comment.
Should we have a const somewhere for overlaybd ? (not sure what the best place is for it though)
There was a problem hiding this comment.
I think it's irrelevant since stargz does the same. And it doesn't make the code harder to understand:)
There was a problem hiding this comment.
Yes, definitely not a "show-stopper", and if we do, we should consider doing it for all the snapshotters (which would be out of scope for this PR).
I agree with you ("in general") that a const is not always needed for these, and sometimes a const may make ik even harder to understand the code (so .. "it depends").
The reason I was looking if we should add a const is that there's various code-paths in multiple packages, where the code-flow depends on the snapshotter used. Having a const for these could help discovery of those flows (i.e., find all uses of the const in the code). If these consts would also have a strong type, it could also act as an "enum" to help discovery of "accepted values". (See #3694 for something similar).
fa544ab to
5cc5636
Compare
|
@AkihiroSuda @thaJeztah @ktock |
|
Compilation is failing for Windows |
@AkihiroSuda We remove a package that only uses in Unix, now it can pass all checks. |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
Thanks! New version looks a lot better; I left some comments/questions and was drafting along some ideas, but not sure if they're correct / make sense. I'll open a PR against your fork for consideration.
| mediaType := comp.Type.MediaType() | ||
|
|
||
| var isOverlaybd = false | ||
| if sr.cm.Snapshotter.Name() == "overlaybd" { |
There was a problem hiding this comment.
Yes, definitely not a "show-stopper", and if we do, we should consider doing it for all the snapshotters (which would be out of scope for this PR).
I agree with you ("in general") that a const is not always needed for these, and sometimes a const may make ik even harder to understand the code (so .. "it depends").
The reason I was looking if we should add a const is that there's various code-paths in multiple packages, where the code-flow depends on the snapshotter used. Having a const for these could help discovery of those flows (i.e., find all uses of the const in the code). If these consts would also have a strong type, it could also act as an "enum" to help discovery of "accepted values". (See #3694 for something similar).
| } | ||
| if snStat.Labels[obdlabel.LocalOverlayBDPath] != "" { | ||
| isOverlaybd = true | ||
| mediaType = ocispecs.MediaTypeImageLayer |
There was a problem hiding this comment.
Not new, as it was already defined at the top, but looks like mediaType is not used until line 188, so we could move this block closer to where it's used.
| if snStat.Labels[obdlabel.LocalOverlayBDPath] != "" { | ||
| isOverlaybd = true |
There was a problem hiding this comment.
Sorry if it's a silly question, but what happens if the label isn't set? Is that an expected (valid) situation, or should that be an error condition?
I'm also looking at this code, and possibly part of it could be moved at the end (where it's used).
| case "fuse-overlayfs", "native", "overlaybd": | ||
| // not supported with fuse-overlayfs snapshotter which doesn't provide overlayfs mounts. | ||
| // TODO: add support for fuse-overlayfs | ||
| enableOverlay = false |
There was a problem hiding this comment.
Curious; is the enableOverlay supported with this snapshotter? I see there's code above that allows it to be forced, and I wonder if that is a valid case (so if the if enableOverlay code below should be executed also for overlaybd if that env-var is set.
Lines 151 to 156 in 13cca5a
Because if it's not needed, we can combine the isOverlayBD code from above, and the if isOverlaybd code from below.
| compressorFunc, finalize := comp.Type.Compress(ctx, comp) | ||
| mediaType := comp.Type.MediaType() | ||
|
|
||
| var isOverlaybd = false |
There was a problem hiding this comment.
nit; probably should be isOverlayBD
| return err | ||
| } | ||
|
|
||
| func commitOverlaybd(ctx context.Context, sr *immutableRef, desc *ocispecs.Descriptor) error { |
There was a problem hiding this comment.
Nit: should probably be commitOverlayBD (capital BD)
|
opened HileQAQ#1 with things I was drafting / playing with (includes some of my suggestions) |
|
@thaJeztah I made some modifications as suggested, except your suggestions, I set |
|
@tonistiigi @ktock PTAL |
48906ad to
ff0522f
Compare
|
+1 for this very useful feature! We depend on Overlaybd image format and being able to build directly instead of doing conversion would be a major enhancement. Do we know if we have plan to merge this soon? |
|
At least this needs rebasing |
|
I will solve this... This PR has been pending for a long time😂
|
github-actions
Bot
added
area/dependencies
|
@AkihiroSuda @BigVan I have rebased the code, PTAL |
|
The related feature has been supported in Alibaba for almost two years. |
|
@AkihiroSuda A lint error has been fixed, please help rerun the checks |
|
These two failed checks should be unrelated to the code in this PR. The logic related to overlaybd includes appropriate checks and will not affect the existing logic. Additionally, I also encounter these failures when using the upstream code on my GitHub Actions occasionally. Could you please help me retry or provide some assistance? @AkihiroSuda |
55fae9f to
c556d69
Compare
| labels = make(map[string]string) | ||
| } | ||
| maps.Copy(labels, estargz.SnapshotLabels(dsl.ref, descs, index)) | ||
| labels["containerd.io/snapshot/image-ref"] = dsl.ref |
There was a problem hiding this comment.
This is overlaybd-specific label, right? Can it be enabled only for overlaybd snapshotter?
There was a problem hiding this comment.
Or, "containerd.io/snapshot/image-ref" should be clearly defined somewhere (e.g. by defining this in util pkg with comments)
There was a problem hiding this comment.
resolved.
we use "containerd.io/snapshot/cri.image-ref" to replace it
| labels = make(map[string]string) | ||
| } | ||
| maps.Copy(labels, estargz.SnapshotLabels(p.manifest.Ref, p.manifest.Descriptors, i)) | ||
| labels["containerd.io/snapshot/image-ref"] = p.manifest.Ref |
There was a problem hiding this comment.
Same as the above. containerd.io/snapshot/image-ref should be explained somewhere if it's specific to overlaybd.
There was a problem hiding this comment.
we have fixed this label.
Could you re-run the test?
It should be some environmental reason that caused the test to fail
Signed-off-by: liulanzheng <lanzheng.liulz@alibaba-inc.com>
|
@thaJeztah @tonistiigi |
|
3 approvals, and all comments from thaJeztah have been resolved yet. |
|
|
||
| ## Performance | ||
|
|
||
| In our test case Dockerfile, we used a 5GB OCI image (and corresponding overlaybd format), wrote some new layers of identical size, and recorded the time cost of image pull (as **pull** in the table below), building all lines in Dockerfile (as **build**), and exporting to image and pushing (as **push**). |
9 participants
Overlaybd is a remote image format based on the block device, which can load data on demand. You can see the paper for more information.
In order to build overlaybd images using buildkit, some changes have been made that are separate from the original process. Please refer to the docs/overlaybd.md for guidance on building overlaybd images.
Signed-off-by: Haoqi Miao miaohaoqi.mhq@alibaba-inc.com