Skip to content

WIP: Reactivate containerd 1.2 in the CI tests#1387

Closed
TBBle wants to merge 5 commits intomoby:masterfrom
TBBle:containerd-1.2-tests
Closed

WIP: Reactivate containerd 1.2 in the CI tests#1387
TBBle wants to merge 5 commits intomoby:masterfrom
TBBle:containerd-1.2-tests

Conversation

@TBBle
Copy link
Copy Markdown
Collaborator

@TBBle TBBle commented Mar 1, 2020

This is a pre-requisite for #1348.

Per #1314 (comment) and #1314 (comment), containerd 1.2 is still supported, but CI testing was disabled in c4f0305.

Since that point, 200-something tests now fail due to calling a particular API LeaseManager.AddResource which was newly-added in containerd 1.3. (Two other APIs, DeleteResource and ListResource are unused in BuildKt.

The intention here is to skip those tests, as I don't understand enough about what LeaseManager does to actually do the surgery to make the tests pass, e.g., catching and ignoring the not-implemented failure in util/leaseutil/manager.go.

TBBle added 5 commits March 1, 2020 22:58
The CI script goes looking for the branch to which a commit was pushed,
but was applying the branch name to moby/buildkit even if being built on
a different fork.

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
This reverts commit c4f0305.

containerd 1.2 is still supported, per discussion at
moby#1314 (comment)

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
It's not OCI-specific, so it makes more sense in the file that exports
it to the world.

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
Per moby#1314 (comment)
BuildKit should still function with containerd 1.2 daemons that do not
have this API, but less efficiently.

However, a couple of hundred tests fail on CI when they hit this, so
just skip them for now.

WIP because this doesn't work, see the TODO in Run in run.go.

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
@TBBle
Copy link
Copy Markdown
Collaborator Author

TBBle commented Mar 1, 2020

I could do with some advice on this. I spent the evening looking into this, and there's 200-something tests failing with containerd 1.2, all hitting LeaseManager.AddResource.

I don't think it's a great solution to annotate those 200 tests to be skipped in the containerd 1.2 case, and I don't see a good way to catch the failure-case and skip the test when it happens, as once the test is marked Failed(), it can't be skipped, and Go's testing framework doesn't have the concept of Expected Failure.

I'm not sure what AddResource is doing, and I don't really parse !1176 well-enough to know if AddResource can just be skipped, or if we need to track some ref-count locally, i.e. what is the less-efficient option mentioned by @tonistiigi in #1314 (comment)

@tonistiigi
Copy link
Copy Markdown
Member

@dmcgowan Is this expected? I thought only gc.flat required containerd 1.3? But seems nothing around leases really works in v1.2.

@TBBle If there is no way to add objects to leases with v1.2 daemon we just need to drop support. Don't see any other way unfortunately.

@TBBle
Copy link
Copy Markdown
Collaborator Author

TBBle commented Mar 3, 2020

containerd/containerd@8a388d6 definitely appears to be containerd 1.3-only.

@TBBle
Copy link
Copy Markdown
Collaborator Author

TBBle commented Jun 21, 2020

@tonistiigi @dmcgowan I haven't heard anything on this for a while, can we assume that BuildKit master now requires containerd 1.3, and go ahead on that basis, e.g. unblocking a version of #1348 that doesn't try to support containerd 1.2's shim.

@TBBle
Copy link
Copy Markdown
Collaborator Author

TBBle commented Jul 16, 2020

BuildKit 0.7.0 formally dropped Containerd 1.2 support for the containerd worker, so I'm going to drop this, in favour of removing the hard-coded containerd 1.2 runner as in my initial work. (1, 2): #1570

@TBBle TBBle closed this Jul 16, 2020
@TBBle TBBle deleted the containerd-1.2-tests branch July 16, 2020 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants