Skip to content

Some small test improvements#4427

Merged
estesp merged 3 commits intocontainerd:masterfrom
TBBle:test_improvements
Jul 28, 2020
Merged

Some small test improvements#4427
estesp merged 3 commits intocontainerd:masterfrom
TBBle:test_improvements

Conversation

@TBBle
Copy link
Contributor

@TBBle TBBle commented Jul 28, 2020

Some stuff I hit while trying to reproduce an integration test failure seen on CI:

  • When running the integration tests with -v, output the server log even on success, to help diagnose whatever you're trying to diagnose by adding -v.
  • Add test image repository for Windows 10 2004/Windows Server 2004
  • Fail early and with a useful course of action, if a test image cannot be determined on Windows, since there is no reasonable fallback option.

TBBle added 3 commits July 28, 2020 22:33
If you're adding `-v` to TESTFLAGS, you probably want to see the server
logs, as well as the extra output from the testing framework.

Signed-off-by: Paul "TBBle" Hampson <[email protected]>
@theopenlab-ci
Copy link

theopenlab-ci bot commented Jul 28, 2020

Build succeeded.

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

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

Comment on lines 52 to +58
case 18363: // this isn't in osversion yet, but the image should be available
testImage = "mcr.microsoft.com/windows/nanoserver:1909"
case 19041: // this isn't in osversion yet, but the image should be available
testImage = "mcr.microsoft.com/windows/nanoserver:2004"
case 9200: // Missing manifest, so it's running in compatibility mode
fmt.Println("You need to copy Microsoft/hcsshim/test/functional/manifest/rsrc_amd64.syso into the containerd checkout")
panic("Running in Windows 8/Windows Server 2012 compatibility mode, failed to detect Windows build version")
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have these added to https://github.com/microsoft/hcsshim/tree/master/osversion to give the versions a bit more meaning

Copy link
Member

Choose a reason for hiding this comment

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

Wondering; would it work to use (e.g.) the official golang image for testing? It will be slightly larger (as it adds the go binaries), but that image is multi-arch, so should pull the right variant

Copy link
Contributor Author

@TBBle TBBle Jul 28, 2020

Choose a reason for hiding this comment

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

I'd prefer not to use a larger image. These tests already took ages to start on my own machine. -_- In the scheme of things, probably not a real problem to switch though, just me whinging. ^_^

Hmm. It's a shame there's not a multiarch nanoserver image, now I look. That would have been rather useful.

hcsshim already has Windows 2004... I just noticed it's missing 1909, so yeah, worth adding it there, and then when it's vendored next, these magic numbers can turn into named constants. Edit: I raised microsoft/hcsshim#857 to add a V19H2 constant.

9200 is trickier... It's slightly magic. It might be better to just add a copy of the syso, and/or work out how to generate it during the build. It apparently just appeared out of thin air one day which I guess means it's MIT License and can just be copied.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer not to use a larger image. These tests already took ages to start on my own machine. -- In the scheme of things, probably not a real problem to switch though, just me whinging. ^^

Yeah, the Windows images are huge. I think the Golang images are ~200M larger than the base images (which is a lot, but compared to the 2.5 .. 5.5 GB of the base image 😓 😂)

Hmm. It's a shame there's not a multiarch nanoserver image, now I look. That would have been rather useful.

There used to be, but Microsoft deprecated those in favour of users explicitly specifying the variant they need 😞

We could use @tianon's https://hub.docker.com/r/hell/win image (which is a multi-arch manifest referencing the upstream images) 😂

9200 is trickier... It's slightly magic. It might be better to just add a copy of the syso, and/or work out how to generate it during the build

Yes, I saw that version in moby/moby@9e587fa, which also mentions that's the version returned if the binary is not manifested.

Utility that's used in moby to get the correct version for that situation is here; https://github.com/moby/moby/blob/master/pkg/parsers/kernel/kernel_windows.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about it, if the _test file had something like import _ "github.com/Microsoft/hcsshim/test/functional/manifest", since we already vendor hcsshim, wouldn't that "just work"?

Copy link
Contributor Author

@TBBle TBBle Jul 28, 2020

Choose a reason for hiding this comment

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

I gave it a try (134e82505b78fc7a560d6ee7bd818c9d3dfd5e5f), and it seems to work. I had to use vndr -whitelist rsrc_amd64.syso as, for some reason I'm not clear on, vndr doesn't consider the .syso as used, even though it did pick up the manifest.go in the same directory. Maybe a bug in vndr? Edit: I raised LK4D4/vndr#94 to fix this, so once that flows through, we can make retry change.

And of course, that fails CI because when it runs vndr the .syso is deleted again. -_- So I'll drop the commit again, and we can try again elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yes, vndr has a list of known non-go files that it keeps (like header files), and other files will be removed. Should be easy to add this to that; https://github.com/LK4D4/vndr/blob/master/clean.go#L15-L50 feel free to open a PR there to add it (Alexander is usually fast to review/merge PR's)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR's already merged. If I understand correctly, it should be consumed by CI now, so I can try my previous commit again (in a new PR, since this one landed)

Copy link
Member

Choose a reason for hiding this comment

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

yes, now that vndr has merged your PR, you can try again in a new PR

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jul 28, 2020

Build succeeded.

@TBBle TBBle force-pushed the test_improvements branch from fc98ae4 to 134e825 Compare July 28, 2020 17:33
@theopenlab-ci
Copy link

theopenlab-ci bot commented Jul 28, 2020

Build succeeded.

@TBBle TBBle force-pushed the test_improvements branch from 134e825 to 1ec1e9e Compare July 28, 2020 17:37
@theopenlab-ci
Copy link

theopenlab-ci bot commented Jul 28, 2020

Build succeeded.

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.

3 participants