builder: define GetRemotes for the worker

[tonistiigi]

Fixes #44918

The function signature has changed since v0.10.

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

[tonistiigi]

This fixes the panic and exports cache. For the import-export cycle to work properly moby/buildkit#3580 is also needed.

[neersighted]

Can we get some additional CI tasks here to make sure we're exercising this code path?

[tonistiigi]

@neersighted It's tricky because it is one of these cases where Moby plugs into BuildKit libraries to disable the default behavior and replace it with compatibility code that would work without containerd. #44908 enables the BuildKit tests that check these flows.

[thaJeztah]

LGTM

[thaJeztah]

The function signature has changed since v0.10.

Should this have been "since v0.8"? I started backporting this for 23.0, then noticed the commit message, so was wondering if that branch needed a different signature, but comparing them, they looked the same;

https://github.com/moby/buildkit/blob/v0.10.6/worker/result.go#L29-L32

// GetRemotes method abstracts ImmutableRef's GetRemotes to allow a Worker to override.
// This is needed for moby integration.
// Use this method instead of calling ImmutableRef.GetRemotes() directly.
func (wr *WorkerRef) GetRemotes(ctx context.Context, createIfNeeded bool, refCfg cacheconfig.RefConfig, all bool, g session.Group) ([]*solver.Remote, error) {

https://github.com/moby/buildkit/blob/v0.11.2/worker/result.go#L36-L39

// GetRemotes method abstracts ImmutableRef's GetRemotes to allow a Worker to override.
// This is needed for moby integration.
// Use this method instead of calling ImmutableRef.GetRemotes() directly.
func (wr *WorkerRef) GetRemotes(ctx context.Context, createIfNeeded bool, refCfg cacheconfig.RefConfig, all bool, g session.Group) ([]*solver.Remote, error) {

Using that line for guidance, I think signature changes happened in v0.9.0-rc1 and v0.10.0-rc1;

• moby/buildkit@3152eab (exporter: Enable to specify the compression type for all layers of the finally exported image buildkit#2057) (v0.9.0-rc1)
• moby/buildkit@f9e0346 (Propagate compression options to the inline cache export buildkit#2405) (v0.10.0-rc1)
• moby/buildkit@893bfdd (Do not re-tag non-distributable blob descriptors buildkit#2561) (v0.10.0-rc1)
• moby/buildkit@cab33b1 (exporter: support for compression-level buildkit#2591) (v0.10.0-rc1)

[thaJeztah]

While there's no exported interface to match yet, perhaps it would be good to add a "permalink" to the code in BuildKit for reference (to save people from hunting down the interface to look for)

[thaJeztah]

re-ran CI 4 times, but it keeps timing out on BuildKit integration tests; don't know if it's related, or if I'm just unlucky; https://github.com/moby/moby/actions/runs/4107961433/jobs/7100496258

=== CONT  TestIntegration/TestDiffUpperScratch/worker=dockerd
    mergediff_test.go:1197: failed to handle changes: lstat ... no such file or directory: https://github.com/moby/buildkit/pull/2726#issuecomment-1070978499
=== CONT  TestIntegration/TestDiffLowerScratchDeletes/worker=dockerd
    client_test.go:6253: checkAllReleasable: skipping check for exported tars in non-containerd test
    client_test.go:6253: checkAllReleasable: skipping check for exported tars in non-containerd test
=== CONT  TestIntegration/TestDiffLowerScratch/worker=dockerd
    client_test.go:6253: checkAllReleasable: skipping check for exported tars in non-containerd test
    client_test.go:6253: checkAllReleasable: skipping check for exported tars in non-containerd test
=== CONT  TestIntegration/TestDiffSelfDeletes/worker=dockerd
    client_test.go:6253: checkAllReleasable: skipping check for exported tars in non-containerd test
panic: test timed out after 30m0s

goroutine 23509 [running]:
testing.(*M).startAlarm.func1()
	/usr/local/go/src/testing/testing.go:2036 +0x8e
created by time.goFunc
	/usr/local/go/src/time/sleep.go:176 +0x32
...

[neersighted]

I am no longer confident these changes are correct as every CI run has timed out at 30 minutes in the same buildkit integration test. Something seems to be deadlocking, or otherwise misbehaving.

[neersighted]

It looks like what changed since the last push was another re-vendor? How can we backport this to 23.0 -- was the same BuildKit fix made on the 0.10 branch, so we can simply cherry-pick the first commit & use a different BuildKit version?

[tonistiigi]

@neersighted No, the patch changed https://github.com/moby/moby/compare/40e89225367c6d402b2f2fb22a30402829b71715..eaeaa7b7fde455335623809efb3944c58f248d6e#diff-cea55065cd7f6cf43128f33bb9ad7b223c244fa6ad23a9a3852f9981b3d71e64R268 . All other changes was just the rebase.

[neersighted]

Ah, I see. No other changes?

[tonistiigi]

@neersighted old 79409e3...40e8922, new 5f369ff...eaeaa7b

[thaJeztah]

I updated the cherry-pick to point to the new commit-sha, but will rebase the backport after #44952 is merged

[thaJeztah]

LGTM

[thaJeztah]

doh! and now forgot about the commit message;

The function signature has changed since v0.10.

well 🤷