Skip to content

Prepare tests for Windows containerd support #42164

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

Jump to bottom
Merged
thaJeztah merged 1 commit into from
Apr 22, 2021
Merged

Prepare tests for Windows containerd support #42164

thaJeztah merged 1 commit into from
Apr 22, 2021

Conversation

@olljanat
Copy link
Contributor

@olljanat olljanat commented Mar 18, 2021

- What I did
As part of Windows + ContainerD CI work on #41479 I noticed that some tests need to be slightly modified that for that purpose. Additionally I enabled some tests which have been disabled on Windows earlier.

- How I did it
On way that those should works both with and without ContainerD on Windows.

- How to verify it
Pass CI here and you can also check from #41479 build log that these tests have passed.

- A picture of a cute animal (not mandatory but encouraged)
image

@olljanat
Copy link
Contributor Author

olljanat commented Mar 18, 2021

Failed ppc64le test looks to be unrelated and known flaky #42057

@olljanat olljanat mentioned this pull request Mar 18, 2021
Merged
TBBle
TBBle reviewed Mar 18, 2021
View reviewed changes
cpuguy83
cpuguy83 approved these changes Mar 19, 2021
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@olljanat olljanat force-pushed the tests/win-prepare-for-containerd branch from e9c9c81 to 878a1d7 Compare March 23, 2021 06:53
TBBle
TBBle reviewed Mar 23, 2021
View reviewed changes
@olljanat olljanat force-pushed the tests/win-prepare-for-containerd branch 4 times, most recently from 05b3b01 to 4b55512 Compare March 29, 2021 14:48
@olljanat
Copy link
Contributor Author

olljanat commented Mar 29, 2021

@TBBle @thaJeztah are we good to go with this one?

@olljanat olljanat force-pushed the tests/win-prepare-for-containerd branch from 4b55512 to dd22c94 Compare April 9, 2021 11:11
@olljanat olljanat force-pushed the tests/win-prepare-for-containerd branch from dd22c94 to bffa730 Compare April 22, 2021 07:50
@olljanat
Copy link
Contributor Author

olljanat commented Apr 22, 2021

Looks to be hard to pass CI nowadays here but afaiu those failures are not related to my changes...

@TBBle
Copy link
Contributor

TBBle commented Apr 22, 2021

I agree, I can't see how Linux daemon startup (two of the failures) or a random bash script (the other one) could be affected by these changes.

thaJeztah
thaJeztah approved these changes Apr 22, 2021
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

but we should have a look at handling those errors, looks like there's some iffy code in how we handle them (discarding error types that could possibly be useful)

integration/container/pause_test.go

err := client.ContainerPause(ctx, cID)
assert.Check(t, is.ErrorContains(err, "cannot pause Windows Server Containers"))
assert.Check(t, is.ErrorContains(err, containerderrdefs.ErrNotImplemented.Error()))
Copy link
Member

@thaJeztah thaJeztah Apr 22, 2021

Choose a reason for hiding this comment

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

w.r.t. https://github.com/moby/moby/pull/42164/files#r602423617

The daemon may be converting it from the containerd error type;

moby/errdefs/http_helpers.go

Lines 186 to 187 in 81dbed4

case containerderrors.IsNotImplemented(err):
return http.StatusNotImplemented

And on the client side, will handle it as an errdefs.InotImplemented();

moby/errdefs/is.go

Lines 79 to 83 in 81dbed4

// IsNotImplemented returns if the passed in error is an ErrNotImplemented
func IsNotImplemented(err error) bool {
_, ok := getImplementer(err).(ErrNotImplemented)
return ok
}

Oh, actually, it may not be converted; looking at

moby/daemon/pause.go

Lines 41 to 43 in 7b9275c

if err := daemon.containerd.Pause(context.Background(), container.ID); err != nil {
return fmt.Errorf("Cannot pause container %s: %s", container.ID, err)
}

It just trumps the error-type, and converts it into a generic error (likely will return a 500 error at the API level)

@thaJeztah thaJeztah merged commit 7f0fb3e into moby:master Apr 22, 2021
@thaJeztah thaJeztah added this to the 21.xx milestone Apr 22, 2021
@olljanat olljanat mentioned this pull request Jan 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

+1 more reviewer

@TBBle TBBle TBBle approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/runtime Runtime area/testing platform/windows status/4-merge

Projects

None yet

Milestone

23.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@olljanat @TBBle @cpuguy83 @thaJeztah