Support for building overlaybd images #3867

HileQAQ · 2023-05-12T10:46:52Z

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 [email protected]

HileQAQ · 2023-05-14T09:32:40Z

@tonistiigi @AkihiroSuda @ktock
We add some code to support building overlaybd images with buildkit. Do you have time to review it?

AkihiroSuda · 2023-05-22T07:39:45Z

Looks good but needs rebase

AkihiroSuda · 2023-05-22T07:41:10Z

cache/manager.go

@@ -615,6 +616,10 @@ func (cm *cacheManager) New(ctx context.Context, s ImmutableRef, sess session.Gr
 		}); rerr != nil {
 			return nil, rerr
 		}
+	} else if cm.Snapshotter.Name() == "overlaybd" && parent != nil {
+		rwLabels := make(map[string]string)
+		rwLabels["containerd.io/snapshot/overlaybd.writable"] = "dev"


What’s “dev”?

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.

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

https://github.com/containerd/accelerated-container-image/blob/6dff475f080557cf8a8b5564a29ddee118258f56/pkg/snapshot/overlay.go#LL909C4-L909C4

Thanks. Could you add that explanation in the code comment lines?

ktock · 2023-05-22T14:02:38Z

cache/refs.go

@@ -1232,7 +1291,8 @@ func (sr *immutableRef) unlazyLayer(ctx context.Context, dhs DescHandlers, pg pr

 	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)})


This should not be enabled by default.

Fixed, now this label is only for overlaybd.

thaJeztah · 2023-05-22T14:23:54Z

cache/blobs.go

+					if snStat.Labels[obdlabel.LocalOverlayBDPath] != "" {
+						isOverlaybd = true
+					}
+					mediaType = ocispecs.MediaTypeImageLayer


This is overwritten regardless if isOverlaybd is true/false, that's correct?

Sorry, mediaType should be overwritten only when isOverlaybd is true. It is now fixed.

thaJeztah · 2023-05-22T15:08:23Z

cache/blobs.go

@@ -105,7 +121,7 @@ func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool
 					lowerRef = sr.layerParent
 				}
 				var lower []mount.Mount
-				if lowerRef != nil {
+				if !isOverlaybd && lowerRef != nil {


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;

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, err

This code has been modified, because desc will be overwritten in commitOverlaybd, so there is no effect if !isOverlaybd is not added.

thaJeztah · 2023-05-22T15:09:38Z

cache/blobs.go

@@ -96,7 +102,17 @@ func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool

 				compressorFunc, finalize := comp.Type.Compress(ctx, comp)
 				mediaType := comp.Type.MediaType()
-
+				var isOverlaybd = false
+				if sr.cm.Snapshotter.Name() == "overlaybd" {


Should we have a const somewhere for overlaybd ? (not sure what the best place is for it though)

I think it's irrelevant since stargz does the same. And it doesn't make the code harder to understand:)

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).

HileQAQ · 2023-05-30T06:26:19Z

@AkihiroSuda @thaJeztah @ktock
The code has been modified according to the opinions of the comments, and it has been rebased to the latest version. Please review it again. Thanks.

AkihiroSuda · 2023-06-02T03:31:28Z

Compilation is failing for Windows

Error: vendor\github.com\containerd\accelerated-container-image\pkg\snapshot\overlay.go:1066:30: undefined: syscall.Stat_t
Error: vendor\github.com\containerd\accelerated-container-image\pkg\snapshot\storage.go:396:29: undefined: unix.MS_RDONLY
Error: vendor\github.com\containerd\accelerated-container-image\pkg\snapshot\storage.go:433:21: undefined: unix.Mount

HileQAQ · 2023-06-05T02:46:59Z

Compilation is failing for Windows

Error: vendor\github.com\containerd\accelerated-container-image\pkg\snapshot\overlay.go:1066:30: undefined: syscall.Stat_t
Error: vendor\github.com\containerd\accelerated-container-image\pkg\snapshot\storage.go:396:29: undefined: unix.MS_RDONLY
Error: vendor\github.com\containerd\accelerated-container-image\pkg\snapshot\storage.go:433:21: undefined: unix.Mount

@AkihiroSuda We remove a package that only uses in Unix, now it can pass all checks.

thaJeztah

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.

thaJeztah · 2023-06-08T07:36:03Z

cache/blobs.go

@@ -96,7 +102,17 @@ func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool

 				compressorFunc, finalize := comp.Type.Compress(ctx, comp)
 				mediaType := comp.Type.MediaType()
-
+				var isOverlaybd = false
+				if sr.cm.Snapshotter.Name() == "overlaybd" {


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).

thaJeztah · 2023-06-08T08:13:27Z

cache/blobs.go

+					}
+					if snStat.Labels[obdlabel.LocalOverlayBDPath] != "" {
+						isOverlaybd = true
+						mediaType = ocispecs.MediaTypeImageLayer


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.

thaJeztah · 2023-06-08T08:49:27Z

cache/blobs.go

+					if snStat.Labels[obdlabel.LocalOverlayBDPath] != "" {
+						isOverlaybd = true


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).

thaJeztah · 2023-06-08T08:51:51Z

cache/blobs.go

+					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


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.

buildkit/cache/blobs.go

Lines 151 to 156 in 13cca5a

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

Because if it's not needed, we can combine the isOverlayBD code from above, and the if isOverlaybd code from below.

thaJeztah · 2023-06-08T08:53:56Z

cache/blobs.go

@@ -96,7 +102,17 @@ func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool

 				compressorFunc, finalize := comp.Type.Compress(ctx, comp)
 				mediaType := comp.Type.MediaType()
-
+				var isOverlaybd = false


nit; probably should be isOverlayBD

thaJeztah · 2023-06-08T08:54:53Z

cache/blobs.go

@@ -462,3 +485,45 @@ func ensureCompression(ctx context.Context, ref *immutableRef, comp compression.
 	})
 	return err
 }
+
+func commitOverlaybd(ctx context.Context, sr *immutableRef, desc *ocispecs.Descriptor) error {


Nit: should probably be commitOverlayBD (capital BD)

thaJeztah · 2023-06-08T09:51:49Z

opened HileQAQ#1 with things I was drafting / playing with (includes some of my suggestions)

HileQAQ · 2023-06-26T02:56:55Z

@thaJeztah I made some modifications as suggested, except your suggestions, I set enableOverlay=false after commitOverlayBD for compatibility. Sorry for the late reply.

AkihiroSuda · 2023-07-11T10:38:00Z

@tonistiigi @ktock PTAL

shuaichang · 2024-11-26T06:34:40Z

+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?

AkihiroSuda · 2024-11-26T06:40:52Z

At least this needs rebasing

BigVan · 2024-11-26T06:43:11Z

I will solve this... This PR has been pending for a long time😂

At least this needs rebasing

liulanzheng · 2025-01-17T03:26:58Z

@AkihiroSuda @BigVan I have rebased the code, PTAL

BigVan · 2025-01-17T06:47:45Z

The related feature has been supported in Alibaba for almost two years.
PTAL @thaJeztah @ktock and Hope you guys can approve this PR

liulanzheng · 2025-01-17T08:07:25Z

@AkihiroSuda A lint error has been fixed, please help rerun the checks

fuweid · 2025-01-24T01:42:57Z

ping @AkihiroSuda @thaJeztah @tonistiigi

liulanzheng · 2025-02-05T03:23:52Z

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

Signed-off-by: liulanzheng <[email protected]>

HileQAQ force-pushed the main branch from 4bef36c to cfc6b0b Compare May 14, 2023 13:51

AkihiroSuda reviewed May 22, 2023

View reviewed changes

ktock reviewed May 22, 2023

View reviewed changes

thaJeztah reviewed May 22, 2023

View reviewed changes

HileQAQ force-pushed the main branch 4 times, most recently from fa544ab to 5cc5636 Compare May 30, 2023 06:13

AkihiroSuda approved these changes Jun 2, 2023

View reviewed changes

HileQAQ force-pushed the main branch from 5cc5636 to a1718eb Compare June 2, 2023 06:04

thaJeztah reviewed Jun 8, 2023

View reviewed changes

thaJeztah mentioned this pull request Jun 8, 2023

bdoverlay suggestions HileQAQ/buildkit#1

Merged

thaJeztah mentioned this pull request Jun 9, 2023

[Moved to #4057] Apply SOURCE_DATE_EPOCH to the files and the whiteouts inside OCI tar layers #3560

Closed

HileQAQ force-pushed the main branch from abe1004 to 83a14ca Compare June 26, 2023 02:47

AkihiroSuda requested a review from tonistiigi June 28, 2023 10:02

HileQAQ force-pushed the main branch from 83a14ca to 53acd17 Compare July 21, 2023 09:07

BigVan mentioned this pull request Jul 25, 2023

Build overlaybd image format from Dockerfile instead of conversion containerd/accelerated-container-image#135

Open

HileQAQ force-pushed the main branch 2 times, most recently from 48906ad to ff0522f Compare July 26, 2023 03:46

liulanzheng force-pushed the main branch from ff0522f to 9ef30d0 Compare January 17, 2025 03:20

github-actions bot added area/dependencies Pull requests that update a dependency file area/remotecache area/docs area/source labels Jan 17, 2025

liulanzheng force-pushed the main branch from 9ef30d0 to 24841ae Compare January 17, 2025 08:05

Support for building overlaybd images

2b6ab8c

Signed-off-by: liulanzheng <[email protected]>

liulanzheng force-pushed the main branch from 24841ae to 2b6ab8c Compare February 27, 2025 08:53

		if snStat.Labels[obdlabel.LocalOverlayBDPath] != "" {
		isOverlaybd = true

	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

Support for building overlaybd images #3867

Are you sure you want to change the base?

Support for building overlaybd images #3867

Conversation

HileQAQ commented May 12, 2023

HileQAQ commented May 14, 2023 • edited Loading

AkihiroSuda commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktock May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HileQAQ commented May 30, 2023

AkihiroSuda commented Jun 2, 2023

HileQAQ commented Jun 5, 2023

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Jun 8, 2023

HileQAQ commented Jun 26, 2023

AkihiroSuda commented Jul 11, 2023

shuaichang commented Nov 26, 2024

AkihiroSuda commented Nov 26, 2024

BigVan commented Nov 26, 2024

liulanzheng commented Jan 17, 2025

BigVan commented Jan 17, 2025

liulanzheng commented Jan 17, 2025

fuweid commented Jan 24, 2025

liulanzheng commented Feb 5, 2025

HileQAQ commented May 14, 2023 •

edited

Loading

ktock May 22, 2023 •

edited

Loading