Skip to content

bdoverlay suggestions #1

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.

Sign up for GitHub

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

Merged
HileQAQ merged 1 commit into from
Jun 9, 2023
Merged

bdoverlay suggestions #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 20 additions & 21 deletions cache/blobs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,18 +101,6 @@ 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" {
snStat, err := sr.cm.Snapshotter.Stat(ctx, sr.getSnapshotID())
if err != nil {
return nil, err
}
if snStat.Labels[obdlabel.LocalOverlayBDPath] != "" {
isOverlaybd = true
mediaType = ocispecs.MediaTypeImageLayer
}
}
var lowerRef *immutableRef
switch sr.kind() {
case Diff:
Expand Down Expand Up @@ -184,6 +172,19 @@ func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool
enableOverlay = false
}
}
mediaType := comp.Type.MediaType()
if sr.cm.Snapshotter.Name() == "overlaybd" {
snStat, err := sr.cm.Snapshotter.Stat(ctx, sr.getSnapshotID())
if err != nil {
return nil, errors.Wrapf(err, "failed to Stat overlaybd")
}
if bdPath := snStat.Labels[obdlabel.LocalOverlayBDPath]; bdPath != "" {
if err := commitOverlayBD(ctx, sr, &desc); err != nil {
return nil, err
}
mediaType = desc.MediaType
}
}
Comment on lines +175 to +187
Copy link
Author

@thaJeztah thaJeztah Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to move this closer to where it's used, but wasn't sure if enableOverlay (below) is possible with this snapshotter. It's disabled by default for overlaybd, but can be enabled with the BUILDKIT_DEBUG_FORCE_OVERLAY_DIFF env-var.

If it is not supported, then probably there should be an error, and otherwise we may have to split this block (for the mediaType part), and move the commit back to below.

Copy link
Owner

@HileQAQ HileQAQ Jun 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acturally, commitOverlayBD do similar things like tryComputeOverlayBlob but is for block device. If enableOverlay is true, commitOverlayBD should overcover desc generated by tryComputeOverlayBlob. We set enableOverlay to false to avoid this (because it will make overlaybd build slower).

Copy link
Author

@thaJeztah thaJeztah Jun 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HileQAQ Thanks, yes that was the part I was wondering about; should we ignore BUILDKIT_DEBUG_FORCE_OVERLAY_DIFF for overlaybd altogether? We disable enableOverlay by default, but (technically) the BUILDKIT_DEBUG_FORCE_OVERLAY_DIFF could still enable this (and perhaps that should either produce an error, or a warning and be ignored?)

Copy link
Owner

@HileQAQ HileQAQ Jun 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling enableOverlay has no effect on the result of the overlaybd build, it just makes it a little slower(in compute diff between lower and upper). So I think it shouldn't be an error, even don't need a warning.

Copy link
Author

@thaJeztah thaJeztah Jun 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha; so, yes, perhaps we should disable it altogether, or put it in a branch where it's != "overlaybd"

Copy link
Owner

@HileQAQ HileQAQ Jun 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will merge this PR and make some changes related to this.

if enableOverlay {
computed, ok, err := sr.tryComputeOverlayBlob(ctx, lower, upper, mediaType, sr.ID(), compressorFunc)
if !ok || err != nil {
Expand All @@ -204,13 +205,6 @@ func computeBlobChain(ctx context.Context, sr *immutableRef, createIfNeeded bool
}
}

if isOverlaybd {
if err := commitOverlaybd(ctx, sr, &desc); err != nil {
return nil, err
}
desc.MediaType = mediaType
}

if desc.Digest == "" && !isTypeWindows(sr) && comp.Type.NeedsComputeDiffBySelf() {
// These compression types aren't supported by containerd differ. So try to compute diff on buildkit side.
// This case can be happen on containerd worker + non-overlayfs snapshotter (e.g. native).
Expand Down Expand Up @@ -486,12 +480,16 @@ func ensureCompression(ctx context.Context, ref *immutableRef, comp compression.
return err
}

func commitOverlaybd(ctx context.Context, sr *immutableRef, desc *ocispecs.Descriptor) error {
func commitOverlayBD(ctx context.Context, sr *immutableRef, desc *ocispecs.Descriptor) error {
snStat, err := sr.cm.Snapshotter.Stat(ctx, sr.getSnapshotID())
if err != nil {
return errors.Wrapf(err, "failed to Stat overlaybd")
}
dir := path.Dir(snStat.Labels[obdlabel.LocalOverlayBDPath])
bdPath := snStat.Labels[obdlabel.LocalOverlayBDPath]
if bdPath == "" {
return errors.New("missing overlaybd path label")
}
dir := path.Dir(bdPath)
Comment on lines +488 to +492
Copy link
Author

@thaJeztah thaJeztah Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe redundant; mostly thought; perhaps we need to make sure that this code path isn't hit if there's no label

Copy link
Owner

@HileQAQ HileQAQ Jun 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This label is generated from overlaybd.Prepare, but I think it needs to be judged here too.

commitPath := path.Join(dir, "overlaybd.commit")
err = obdcmd.Commit(ctx, dir, dir, true, "-t", "-z")
if err != nil {
Expand Down Expand Up @@ -521,6 +519,7 @@ func commitOverlaybd(ctx context.Context, sr *immutableRef, desc *ocispecs.Descr
}
desc.Digest = dgst
desc.Size = sz
desc.MediaType = ocispecs.MediaTypeImageLayer
Copy link
Author

@thaJeztah thaJeztah Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this here, as commitOverlayBD was already mutating the descriptor 🤔

Copy link
Owner

@HileQAQ HileQAQ Jun 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it is more appropriate to put together the modification of overlaybd for desc.

desc.Annotations = map[string]string{
obdlabel.OverlayBDBlobDigest: string(desc.Digest),
obdlabel.OverlayBDBlobSize: fmt.Sprintf("%d", desc.Size),
Expand Down