Add amd64->386 fallback by tianon · Pull Request #4528 · containerd/containerd

tianon · 2020-09-04T00:21:01Z

This adds a test for platforms.Only (previously untested, as far as I can tell), improves the hard-coded list of ARM fallbacks in the platform.Only implementation (by doing a descending loop over variant numbers instead, which is all the hard-coded list was doing), and adds 386 as a fallback architecture for amd64.

I'd like to do something similar for arm64 (falling back to linux/arm/v8, linux/arm/v7, etc, which I think would more accurately fix what was reported in #3990 and allow us to revert #4013), but figured I'd start with this and see what the interest in the approach is. ❤️

theopenlab-ci · 2020-09-04T00:27:36Z

Build succeeded.

containerd-build-arm64 : FAILURE in 5m 03s (non-voting)

tianon · 2020-09-04T16:42:47Z

I'm digging into the integration test failure on TestImagePullWithDistSourceLabel, and it appears that it's doing a client.Pull on docker.io/library/busybox:latest using the current platform (which will return the linux/amd64 image), then doing something with the full original list of matching manifests (which will include the linux/386 image that was not fetched), and that's what's causing ##[error] image_test.go:134: content digest sha256:b7da4dcd7c393548647008c62a44c591b53d2a40716ecafed818c9af7b671b81: not found (that's the digest of the 386 image on docker.io/library/busybox:latest):

containerd/image_test.go

Lines 83 to 136 in d4e7820

    
           func TestImagePullWithDistSourceLabel(t *testing.T) { 
        
           	var ( 
        
           		source   = "docker.io" 
        
           		repoName = "library/busybox" 
        
           		tag      = "latest" 
        
           	) 
        
           	ctx, cancel := testContext(t) 
        
           	defer cancel() 
        
           	client, err := newClient(t, address) 
        
           	if err != nil { 
        
           		t.Fatal(err) 
        
           	} 
        
           	defer client.Close() 
        
           	imageName := fmt.Sprintf("%s/%s:%s", source, repoName, tag) 
        
           	pMatcher := platforms.Default() 
        
           	// pull content without unpack and add distribution source label 
        
           	image, err := client.Pull(ctx, imageName, WithPlatformMatcher(pMatcher)) 
        
           	if err != nil { 
        
           		t.Fatal(err) 
        
           	} 
        
           	defer client.ImageService().Delete(ctx, imageName) 
        
           	cs := client.ContentStore() 
        
           	key := fmt.Sprintf("containerd.io/distribution.source.%s", source) 
        
           	// only check the target platform 
        
           	childrenHandler := images.FilterPlatforms(images.ChildrenHandler(cs), pMatcher) 
        
           	checkLabelHandler := func(ctx context.Context, desc ocispec.Descriptor) ([]ocispec.Descriptor, error) { 
        
           		children, err := childrenHandler(ctx, desc) 
        
           		if err != nil { 
        
           			return nil, err 
        
           		} 
        
           		info, err := cs.Info(ctx, desc.Digest) 
        
           		if err != nil { 
        
           			return nil, err 
        
           		} 
        
           		// check the label 
        
           		if got := info.Labels[key]; !strings.Contains(got, repoName) { 
        
           			return nil, fmt.Errorf("expected to have %s repo name in label, but got %s", repoName, got) 
        
           		} 
        
           		return children, nil 
        
           	} 
        
           	if err := images.Dispatch(ctx, images.HandlerFunc(checkLabelHandler), nil, image.Target()); err != nil { 
        
           		t.Fatal(err) 
        
           	} 
        
           }

Is someone willing to help me understand what the second half (after the client.Pull) of this test is doing/testing? 😇

(I imagine this test would also fail on master if run on an arm platform 😅)

theopenlab-ci · 2020-09-04T17:05:20Z

Build succeeded.

containerd-build-arm64 : FAILURE in 5m 07s (non-voting)

estesp · 2020-09-04T17:09:42Z

The source label test is just validating that the annotation with distribution source is working properly on pulls. That annotation is used to support cross-repo mount requests on push, if I remember correctly, so this test was added just to make sure that feature doesn't break if someone goes messing with pull handlers in the future and breaks the annotation handler. :)

tianon · 2020-09-04T17:10:45Z

Thanks for the explainer, Phil! I think I managed to find the right fix (by using images.LimitManifests instead of image.FilterPlatforms to both filter and only parse the first, which is the image we pulled).

I'm not sure why openlab is failing, trying to dig into that one now. 😅

estesp · 2020-09-04T17:12:35Z

yup, your fix smells right to me 😂 it should only need to check one manifest, the one pulled, to make sure the labeling is working properly.

tianon · 2020-09-04T17:15:29Z

I've looked into the openlab failure, and I can reproduce it on current master too: 😅

$ go test -v -run TestContent ./metadata/
=== RUN   TestContent
=== RUN   TestContent/Writer
--- FAIL: TestContent (0.01s)
    --- FAIL: TestContent/Writer (0.01s)
panic: sha256 not available (make sure it is imported) [recovered]
	panic: sha256 not available (make sure it is imported)

goroutine 20 [running]:
testing.tRunner.func1.1(0x8e3440, 0xc0000a1490)
	/usr/local/go/src/testing/testing.go:1057 +0x30d
testing.tRunner.func1(0xc00008ad80)
	/usr/local/go/src/testing/testing.go:1060 +0x41a
panic(0x8e3440, 0xc0000a1490)
	/usr/local/go/src/runtime/panic.go:969 +0x175
github.com/opencontainers/go-digest.Algorithm.Hash(0x98aab6, 0x6, 0x0, 0xc000192c00)
	/go/pkg/mod/github.com/opencontainers/[email protected]/algorithm.go:132 +0x1a5
github.com/opencontainers/go-digest.Algorithm.Digester(...)
	/go/pkg/mod/github.com/opencontainers/[email protected]/algorithm.go:112
github.com/opencontainers/go-digest.Algorithm.FromBytes(0x98aab6, 0x6, 0xc000192c00, 0x100, 0x600, 0x600, 0x0)
	/go/pkg/mod/github.com/opencontainers/[email protected]/algorithm.go:159 +0x39
github.com/opencontainers/go-digest.FromBytes(...)
	/go/pkg/mod/github.com/opencontainers/[email protected]/digest.go:93
github.com/containerd/containerd/content/testsuite.createContent(0x100, 0x416025, 0x40e430, 0x28, 0x200, 0x140)
	/wd/content/testsuite/testsuite.go:1086 +0x1d3
github.com/containerd/containerd/content/testsuite.checkContentStoreWriter(0xa24b20, 0xc00019e540, 0xc00008ad80, 0xa29660, 0xc0000e8e00)
	/wd/content/testsuite/testsuite.go:147 +0x6a
github.com/containerd/containerd/content/testsuite.makeTest.func1(0xc00008ad80)
	/wd/content/testsuite/testsuite.go:138 +0x431
testing.tRunner(0xc00008ad80, 0xc00019e1b0)
	/usr/local/go/src/testing/testing.go:1108 +0xef
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1159 +0x386
FAIL	github.com/containerd/containerd/metadata	0.014s
FAIL

tianon · 2020-09-04T17:49:45Z

I adjusted the implementation a little in https://github.com/containerd/containerd/compare/d0ce6697179e59674e11911c7d697849379423bb..f84bcfe291aabf289b21d10cd37481e22cb5c7a1 to move the "platform vector" calculation into a separate function (which can then more easily be recursive later for handling arm64->arm fallback), but I think it makes the code a lot more clear too (so we're calculating a vector of platforms separately from building a matcher object).

tianon · 2020-09-04T17:52:36Z

Hmm, with my new implementation, singlePlatformComparer becomes unused. Should I remove it, or should I adjust my implementation to use when len(vector) == 1?

Edit: removed for now -- happy to bring it back and adjust the approach if preferred 😄

theopenlab-ci · 2020-09-04T17:55:51Z

Build succeeded.

containerd-build-arm64 : FAILURE in 6m 53s (non-voting)

theopenlab-ci · 2020-09-04T18:14:48Z

Build succeeded.

containerd-build-arm64 : FAILURE in 6m 23s (non-voting)

theopenlab-ci · 2020-09-10T16:42:07Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 20s (non-voting)

estesp

LGTM

cpuguy83

LGTM

thaJeztah · 2020-09-15T12:59:51Z

@dmcgowan ptal

cpuguy83 · 2020-12-09T00:09:11Z

Ping, this needs a merge.

tianon · 2020-12-09T00:16:12Z

(As always, I'll reiterate explicitly that I'm happy to discuss, adjust, rebase, amend, split, etc! 😇 ❤️ 👍)

thaJeztah · 2020-12-09T01:02:48Z

@dmcgowan @AkihiroSuda ptal 🤗

theopenlab-ci · 2020-12-29T17:59:25Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 6m 26s (non-voting)

dmcgowan

LGTM

Signed-off-by: Tianon Gravi <[email protected]>

tianon · 2021-01-12T00:23:07Z

(I've just pushed a pure "git pull --rebase origin master" rebase to hopefully help the CI)

theopenlab-ci · 2021-01-12T00:31:03Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 42s (non-voting)

estesp

LGTM

AkihiroSuda · 2021-01-15T05:41:15Z

This breaks downstream projects using this library. We need to create another function that preserves the old behavior.

AkihiroSuda · 2021-01-15T05:57:11Z

Opened #4943

tianon force-pushed the platforms-compare branch from d99ff5a to d0ce669 Compare September 4, 2020 16:58

tianon force-pushed the platforms-compare branch from d0ce669 to f84bcfe Compare September 4, 2020 17:47

tianon force-pushed the platforms-compare branch from f84bcfe to af3ee43 Compare September 4, 2020 18:07

tianon force-pushed the platforms-compare branch from af3ee43 to c0f0b56 Compare September 10, 2020 16:35

estesp approved these changes Sep 10, 2020

View reviewed changes

fuweid requested a review from dmcgowan September 11, 2020 02:56

cpuguy83 approved these changes Sep 11, 2020

View reviewed changes

thaJeztah mentioned this pull request Dec 14, 2020

Multi-arch: add option to configure architecture preference moby/moby#41788

Open

This was referenced Dec 23, 2020

Add platforms.Only test #4879

Merged

Refactor platforms.Only with a "platformVector" helper #4891

Merged

tianon force-pushed the platforms-compare branch from c0f0b56 to 2e2f7e3 Compare December 29, 2020 17:51

tianon changed the title ~~Add platforms.Only test and amd64->386 fallback~~ Add amd64->386 fallback Dec 29, 2020

dmcgowan approved these changes Jan 12, 2021

View reviewed changes

dmcgowan added the status/merge-on-green label Jan 12, 2021

Add amd64->386 fallback

5fa5f15

Signed-off-by: Tianon Gravi <[email protected]>

tianon force-pushed the platforms-compare branch from 2e2f7e3 to 5fa5f15 Compare January 12, 2021 00:22

estesp approved these changes Jan 12, 2021

View reviewed changes

estesp merged commit 3b6a386 into containerd:master Jan 12, 2021

tianon deleted the platforms-compare branch January 12, 2021 16:59

AkihiroSuda mentioned this pull request Jan 18, 2021

platforms: add subarchless version of Only() #4943

Merged

Conversation

tianon commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci Bot commented Sep 4, 2020

Uh oh!

tianon commented Sep 4, 2020

Uh oh!

theopenlab-ci Bot commented Sep 4, 2020

Uh oh!

estesp commented Sep 4, 2020

Uh oh!

tianon commented Sep 4, 2020

Uh oh!

estesp commented Sep 4, 2020

Uh oh!

tianon commented Sep 4, 2020

Uh oh!

tianon commented Sep 4, 2020

Uh oh!

tianon commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci Bot commented Sep 4, 2020

Uh oh!

theopenlab-ci Bot commented Sep 4, 2020

Uh oh!

theopenlab-ci Bot commented Sep 10, 2020

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 15, 2020

Uh oh!

cpuguy83 commented Dec 9, 2020

Uh oh!

tianon commented Dec 9, 2020

Uh oh!

thaJeztah commented Dec 9, 2020

Uh oh!

theopenlab-ci Bot commented Dec 29, 2020

Uh oh!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

tianon commented Jan 12, 2021

Uh oh!

theopenlab-ci Bot commented Jan 12, 2021

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Jan 15, 2021

Uh oh!

AkihiroSuda commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tianon commented Sep 4, 2020 •

edited

Loading

tianon commented Sep 4, 2020 •

edited

Loading