Compute diff from the upper directory of overlayfs-based snapshotter #2181
Conversation
|
cc: @tonistiigi @fuweid |
cache/blobs.go
Outdated
|
|
||
| // Check if this is an opaque directory | ||
| if f.IsDir() { | ||
| opaque, err := sysx.LGetxattr(filepath.Join(diffDir, path), "trusted.overlay.opaque") |
There was a problem hiding this comment.
Added check for user.overlay.opaque as well.
cache/blobs.go
Outdated
| } | ||
| if maj == 0 && min == 0 { | ||
| // This file is deleted from base directory | ||
| if _, err := os.Stat(filepath.Join(base, path)); err != nil { |
There was a problem hiding this comment.
I think this needs to be Lstat instead of Stat, otherwise if this is a symlink we'll be checking the existence of its target, not the symlink itself
cache/blobs.go
Outdated
| return "", false, err | ||
| } | ||
| // This file doesn't exist even in the base dir. We don't need whiteout. | ||
| return "", true, nil |
There was a problem hiding this comment.
Why is skip set to true here? If this dir exists in upper but not base, shouldn't it be included as part of the diff (as a dir creation)?
There was a problem hiding this comment.
Thank you. Fixed this.
ktock
force-pushed
the
overlaydiff
branch
3 times, most recently
from
c723b4c to
8cd99ac
Compare
|
@ktock probably this PR also fixes docker/buildx#537 |
|
@ktock will take a look later. thanks for the /cc |
|
Removing milestone as seems to depend on unmerged upstream patches. I haven't reviewed yet but why do we need extra labels instead of just parsing the mount options and comparing them? |
Sure. I keep on working on these patches.
Because they are implementation details of overlayfs snapshotter. This can easily change without notification in the future. |
Wdym? If you parse the mount opts then this is what the kernel defines. |
|
@tonistiigi It's possible to write the parser considering this logic but I worry about that this logic can change without notification in the future and we cannot notice this change until we encounter the failure of the diff calculation. So I tried to introduce the label that is defined to point to the upperdir of the snapshotter. WDYT? Is it fine to have the parser assuming the current snapshotter implementation? |
|
The parser should have the same logic that kernel uses when parsing the mount options. Eg. no difference in order between upper and lower, but order important for the lowers list. If changes happen they can't happen in a way that kernel would not accept the mount anymore. This is similar to how the applier detects that it can extract fast on overlay I believe. |
Sure. I'll take a deeper look at this. |
ktock
force-pushed
the
overlaydiff
branch
2 times, most recently
from
fff84a8 to
460c9ed
Compare
|
Updated the patch to remove the dependency to the forks of containerd and continuity. |
cache/blobs_linux.go
Outdated
| } | ||
| } | ||
| } | ||
| return append([]string{u}, l...) |
There was a problem hiding this comment.
If l[0] is the bottom layer, then shouldn't the top layer, u, be at the end of the slice here? i.e. return append(l, u)?
There was a problem hiding this comment.
Thank you. Fixed this.
| if f != nil { | ||
| if isOpaque, err := checkOpaque(upperdir, path, base, f); err != nil { | ||
| return err | ||
| } else if isOpaque { |
There was a problem hiding this comment.
nit: when isOpaque is true, could we return filepath.SkipDir as the error? Seems like it might be a simpler way to prevent checking files/dirs under an opaque dir than maintaining the opaqueDirs map and checking prefixes.
There was a problem hiding this comment.
Great idea! Fixed to use filepath.SkipDir instead of the map.
|
rebased. |
There was a problem hiding this comment.
I wonder if it is worth to set up an additional integration test worker for this. Or maybe for some specific tests that do use differ. May be follow-up if tricky. A start would be to add env var to configure this. If it is set but overlay diff can't be done then it is a fatal error.
cache/blobs_linux.go
Outdated
| return "", err | ||
| } | ||
| default: | ||
| return "", fmt.Errorf("cannot get layer information from mount option (type = %q)", lowerM.Type) |
There was a problem hiding this comment.
nit: errors.Errorf and elsewhere
cache/blobs.go
Outdated
| // Try optimized diff for overlayfs | ||
| desc, err = sr.computeOverlayBlob(ctx, lower, upper, mediaType, sr.ID(), compressorFunc) | ||
| if err != nil { | ||
| logrus.Debugf("failed to compute blob (%s) from diff of overlay snapshotter: %+v", sr.ID(), err) |
There was a problem hiding this comment.
we should avoid printing this if overlay is not used. Maybe tryComputeOverlayBlob and return desc, ok, err so we can determine between actual error and mount just not being overlay.
There was a problem hiding this comment.
can't this just be
if err != nil {
fail
}
if ok {
desc = computed
}
and in upperdir, err := getOverlayUpperdir don't throw away the error but do same bool check. if the mount is not type overlay then return false and the regular Compare is called here. If it is type overlay then all next errors will be actual errors. Or if you think we should gracefully handle errors in case there are kernel changes then fail on BUILDKIT_DEBUG_FORCE_OVERLAY_DIFF only here and log with Warn for the rest(if bool is false then never log).
There was a problem hiding this comment.
Thank you for the review. Fixed the error handling logic.
ktock
force-pushed
the
overlaydiff
branch
5 times, most recently
from
2a4af80 to
d236f05
Compare
cache/blobs_nolinux.go
Outdated
| ) | ||
|
|
||
| func (sr *immutableRef) tryComputeOverlayBlob(ctx context.Context, lower, upper []mount.Mount, mediaType string, ref string, compressorFunc compressor) (_ ocispecs.Descriptor, ok bool, err error) { | ||
| return ocispecs.Descriptor{}, true, fmt.Errorf("overlayfs-based diff computing is unsupported") |
cache/blobs.go
Outdated
| } | ||
| if desc.Digest == "" && debugMustOverlayDiff(sr.cm.ManagerOpt.Snapshotter.Name()) { | ||
| // fallback is disabled. used for testing/debugging. | ||
| return nil, errors.Errorf("failed to compute overlay blob (ok=%v): %v", ok, err) |
There was a problem hiding this comment.
possibly this could be warning if implied from snapshotter, and error if from env. In follow up tests we would set the env to make sure it is working and not fallback.
ktock
force-pushed
the
overlaydiff
branch
2 times, most recently
from
3906be9 to
f105370
Compare
cache/blobs.go
Outdated
| if desc.Digest == "" { | ||
| switch sr.cm.ManagerOpt.Snapshotter.Name() { | ||
| case "overlayfs", "fuse-overlayfs", "stargz": | ||
| if os.Getenv("BUILDKIT_DEBUG_FORCE_OVERLAY_DIFF") == "1" { |
There was a problem hiding this comment.
this condition should be before switch. If overlay diff is forced but native snapshotter is used then it is an error as well.
There was a problem hiding this comment.
it is better if these env use ParseBool instead of 1|0 constants. use a helper method to avoid duplication if reused.
There was a problem hiding this comment.
This is still not correct.
If env true
- if mount not overlay: error with "overlay mounts not detected"
- if diff error: error while keeping the original error
if env false
never call tryComputeOverlayBlob
no env:
- if mount not overlay or diff error
- if snapshotter should support overlay: warn with original error, or "overlay mounts not detected"
- else: do nothing
- else: success
No double warn
There was a problem hiding this comment.
Thank you for the suggestion. Implemented the logic.
cache/blobs.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| var desc ocispecs.Descriptor | ||
| if len(lower) > 0 && !isTypeWindows(sr) { |
There was a problem hiding this comment.
BUILDKIT_DEBUG_FORCE_OVERLAY_DIFF=0 should also be a condition to skip this. This can be follow-up to the time when tests would actually set it.
ktock
force-pushed
the
overlaydiff
branch
3 times, most recently
from
0dfd541 to
0c62fd8
Compare
|
All green |
| desc = computed | ||
| } // never call overlay diff if env is false | ||
| } else if !isTypeWindows(sr) { | ||
| computed, ok, err := sr.tryComputeOverlayBlob(ctx, lower, upper, mediaType, sr.ID(), compressorFunc) |
There was a problem hiding this comment.
the logic looks correct now but if you can, try to call tryComputeOverlayBlob only once. Even if it doesn't save lines, because env is(will be) used by the tests it is better if it doesn't go to a different code path. I can imagine a future change that modifies one branch only and we can't catch the error with tests. For the "overlay mounts not detected" case you can print the mount params, both for env and snapshotter case.
Signed-off-by: Kohei Tokunaga <[email protected]>
| if runtime.GOOS != "windows" { | ||
| cmd.Env = append(cmd.Env, "BUILDKIT_DEBUG_FORCE_OVERLAY_DIFF=true") | ||
| } |
There was a problem hiding this comment.
Enabled overlay differ tests in integration tests to see if it doesn't break the current behaviour.
|
All tests passed. |
Fixes: #1704
Fixes: #1192
Currently, BuildKit creates diff blobs by comparing the whole filesystem object of a snapshot to its parent, using walking differ but it turned out it can long time.
This commit is a PoC for the faster diff approach specifically aiming at overlayfs-based snapshotter.
This PR computes a diff blob of an overlayfs-based snapshot using its "upper" directory instead of using walking differ. That upper directory contains the diff between that snapshot and its parent so we don't need to compare the whole filesystem objects between them.
Snapshotter must provide the labelcontainerd.io/snapshot/overlay.upperdirwhich contains the path to the upper directory of this snapshot. When this label is provided,cache.computeBlobChaintries to compute diff blob using that upper directory. The current version of upstream overlayfs snapshotter doesn't provide this label so I'll open a PR to make it happen if this design looks good to you. This PR uses the patched version of containerd (ktock/containerd@0a3a77b...50ed319).EDIT now this patch directly parses
mount.Mountpassed from the snapshotter for identifying the directory that contains the changeset.Computing diff relies on continuity library. This PR usesfs.diffDirChangesof continuity for computing the diff blob from the upper directory. This PR implementsoverlayDeleteChangefor detecting files to delete and configuresfs.diffDirChangesto use this.fs.diffDirChangesis a private method in continuity as of now. I hope containerd/continuity#145 by @fuweid is merged or I'll open a PR if needed. This PR uses the patched version of continuity (ktock/continuity@ce76fcc).EDIT now this patch re-implements the differ that can be used for overlayfs layers.
Comparison of exporting layers
Dockerfile:
command:
This is an one-shot measurement.
Appreciate any suggestion about benchmarking.
full log
walking differ
ubuntu:20.04
python:3.9
nvidia/cuda:11.3.1-devel-ubuntu20.04
overlayfs diff
ubuntu:20.04
python:3.9
nvidia/cuda:11.3.1-devel-ubuntu20.04