Skip to content
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.

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

[master] Switch to Windows Server queues #2927

Merged
merged 5 commits into from
Mar 4, 2020
Merged

Conversation

dougbu
Copy link
Member

@dougbu dougbu commented Feb 3, 2020

  • agents should start much faster

@dougbu dougbu requested a review from a team February 3, 2020 20:15
@dougbu dougbu added this to the 5.0.0-preview1 milestone Feb 3, 2020
@@ -42,7 +42,7 @@ public void RunTest_Win10_RS4()
Assert.NotNull(versionKey);
var currentVersion = (string)versionKey.GetValue("CurrentBuildNumber");
Assert.NotNull(currentVersion);
Assert.True(17134 >= int.Parse(currentVersion));
Assert.True(17134 >= int.Parse(currentVersion), $"Unexpected build number {currentVersion} on {Environment.OSVersion}.");
Copy link
Member Author

Choose a reason for hiding this comment

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

@MattGal this displays very strange information when the assertion fails:

Unexpected build number 17763 on Microsoft Windows NT 6.2.9200.0.

But, according to https://en.wikipedia.org/wiki/List_of_Microsoft_Windows_versions, 17763 is the build number of the latest Windows Server 2019 release. But, 6.2.9200.0 indicates the agent is running Windows Server 2012 (not R2) and 9200 is the build number of the latest release there.

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI I need to revise our testing attributes to handle server machines and get them working in the new world. Right now they seem to work as expected only for the client OS.

Copy link
Member

Choose a reason for hiding this comment

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

Tagging @Tratcher

Copy link
Member

Choose a reason for hiding this comment

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

Environment.OSVersion lies for back-compat reasons unless you have an app manifest. VS and dotnet test both have manifests so that's not usually a problem?

What version did this queue use to run on?

Maybe server 2019 has a new manifest guid?
https://docs.microsoft.com/en-us/windows/win32/sbscs/application-manifests

Copy link
Member

Choose a reason for hiding this comment

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

What Tratcher said. From our perspective though, whenever we say server we are aiming for "latest non-core fully patched server as of when the image was generated" now.

Copy link
Member

Choose a reason for hiding this comment

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

Being current is great, we just need to figure out why our version detection broke.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, the app manifest docs only go up to Server 2016, I can't find any information about 2019.

Copy link
Member

Choose a reason for hiding this comment

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

The owners of the app manifest docs claim the GUID is unchanged for server 2019. Might have to set up some machines and confirm.
MicrosoftDocs/feedback#2477 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

If it used the same GUID, these tests wouldn't have failed and we wouldn't see 6.2.9200.0

@@ -42,7 +42,7 @@ public void RunTest_Win10_RS4()
Assert.NotNull(versionKey);
var currentVersion = (string)versionKey.GetValue("CurrentBuildNumber");
Assert.NotNull(currentVersion);
Assert.True(17134 >= int.Parse(currentVersion));
Assert.True(17134 >= int.Parse(currentVersion), $"Unexpected build number {currentVersion} on {Environment.OSVersion}.");
Copy link
Member

Choose a reason for hiding this comment

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

Environment.OSVersion lies for back-compat reasons unless you have an app manifest. VS and dotnet test both have manifests so that's not usually a problem?

What version did this queue use to run on?

Maybe server 2019 has a new manifest guid?
https://docs.microsoft.com/en-us/windows/win32/sbscs/application-manifests

@dougbu
Copy link
Member Author

dougbu commented Feb 6, 2020

All: Environment.OSVersion lies when VS 2019 is installed too. The displayed version in the assertion failure didn't change when I moved from the VS 2017 queues to the VS 2019 ones. See https://dev.azure.com/dnceng/public/_build/results?buildId=509395&view=ms.vss-test-web.build-test-results-tab&runId=16160634&resultId=100056&paneView=debug

If the problem is the manifests and we had the GUID, we could contribute a PR that updates files like https://github.com/microsoft/vstest/blob/master/src/testhost.x86/app.manifest and https://github.com/microsoft/vstest/blob/master/src/vstest.console/app.manifest Those files currently list the supportedOS GUIDs up to Windows 10 / Windows Server 2016. But, we need the answer to the question (that @Tratcher highlighted) here: https://stackoverflow.com/questions/57320490/what-is-the-supportedos-guid-for-windows-server-2019

So, @Pilchie do we have any contacts on the Windows Server team?


Another option is to stick w/ the Windows.10 queues in all repos x branches using [MaximumOSVersion(...)] or [MinimumOSVersion(...)]. The "hold back repos" would be dotnet/extensions and dotnet/aspnetcore; 'release/3.1' and 'master' are affected in both. Thoughts?

Or, @MattGal do we have any queues running Windows Server 2016?

@Tratcher
Copy link
Member

Tratcher commented Feb 6, 2020

I also filed MicrosoftDocs/feedback#2477 to help track this down.

@Pilchie
Copy link
Member

Pilchie commented Feb 6, 2020

The OS is always known to lie about version. See https://devblogs.microsoft.com/oldnewthing/20100927-00/?p=12733 and it's linked articles for some of the reasons why. AFAIK, the way to get new OS versions to be returned is to compile with a newer Windows SDK.

We should start an internal thread with some contacts to figure out what to do here.

Also, I hear that dotnet/runtime has similar test attributes, we should look at how they work.

@dougbu
Copy link
Member Author

dougbu commented Feb 15, 2020

I suspect the relevant dotnet/runtime class is the Windows portion of PlatformDetection but I doubt it's exactly what we need. The available properties include (say) IsWindows8xOrLater but not IsWindows10OrLater and IsWindowsServerCore but not IsWindowsServer. (dotnet/runtime uses these properties in lambdas passed to [ConditionalTheory] attributes. I can't find that attribute but https://github.com/dotnet/runtime/blob/0b25063ec056b0d6668bf0848d108d8c873f3fce/src/libraries/System.Globalization/tests/CultureInfo/GetCultureInfo.cs#L12-L14 is an example of its use.)

Then again, IsWindows10Version1903OrGreater and similar properties may be useful and the externs might be what we need, especially GetProductInfo.

/cc @stephentoub and @ViktorHofer (feel free to take this offline). We're trying to get our testing attributes to treat Windows Server 2019 as if it were Windows 10 but don't have the right manifest to do it with our current approach. See MaximumOSVersionAttribute and MinimumOsVersionAttribute

Any suggestions much appreciated❕

dougbu added a commit to dotnet-maestro-bot/Common that referenced this pull request Feb 21, 2020
- wait for unblock of dotnet#2927
- however, do switch to VS2019 queues
dougbu added a commit to dotnet-maestro-bot/Common that referenced this pull request Feb 21, 2020
@Tratcher Tratcher force-pushed the dougbu/master/server.queues branch from 50cac19 to 28dd096 Compare March 3, 2020 20:50
@Tratcher
Copy link
Member

Tratcher commented Mar 3, 2020

I wasn't able to reproduce this issue locally using the current master. I set up a Win Server 2019 VM and the tests passed fine. Another app worked fine with the normal Win10 app manifest.

Microsoft Visual Studio Professional 2019
Version 16.4.5
VisualStudio.16.Release/16.4.5+29806.167
Microsoft .NET Framework
Version 4.7.03190

Installed Version: Professional

ASP.NET and Web Tools 2019   16.4.460.23317
ASP.NET and Web Tools 2019

ASP.NET Web Frameworks and Tools 2019   16.4.460.23317
For additional information, visit https://www.asp.net/
...

Doug, I rebased your changes on master to see if anything has changed.

@Tratcher
Copy link
Member

Tratcher commented Mar 3, 2020

Nope, still failing. Interesting observation though, it's only failing on the net472 target, Core is fine.

Tests succeeded: F:\workspace\_work\1\s\artifacts\bin\Microsoft.AspNetCore.Testing.Tests\Debug\netcoreapp5.0\Microsoft.AspNetCore.Testing.Tests.dll [netcoreapp5.0|x64]

XUnit : error : Tests failed: F:\workspace\_work\1\s\artifacts\TestResults\Debug\Microsoft.AspNetCore.Testing.Tests_net472_x64.html [net472|x64]

@MattGal any chance of getting access to one of these agents to debug this more?

@MattGal
Copy link
Member

MattGal commented Mar 3, 2020

Nope, still failing. Interesting observation though, it's only failing on the net472 target, Core is fine.

Tests succeeded: F:\workspace\_work\1\s\artifacts\bin\Microsoft.AspNetCore.Testing.Tests\Debug\netcoreapp5.0\Microsoft.AspNetCore.Testing.Tests.dll [netcoreapp5.0|x64]

XUnit : error : Tests failed: F:\workspace\_work\1\s\artifacts\TestResults\Debug\Microsoft.AspNetCore.Testing.Tests_net472_x64.html [net472|x64]

@MattGal any chance of getting access to one of these agents to debug this more?

Set @Tratcher up on teams with a machine using this image, he'll report back with findings.

@Tratcher
Copy link
Member

Tratcher commented Mar 4, 2020

Progress, I was able to repro this locally on Server 2019.

PS C:\git\extensions\src\TestingUtils\Microsoft.AspNetCore.Testing\test> dotnet test passes
PS C:\git\extensions\src\TestingUtils\Microsoft.AspNetCore.Testing\test> dotnet msbuild -target:test fails
build -test fails
build -test -ci fails
eng\common\CIBuild.cmd fails

Found it: dotnet msbuild -target:test` uses a different test runner on core and .net. On core it uses dotnet.exe, but on .net it uses xunit.console.exe.

Unpacked success command:

C:\git\extensions\.dotnet\dotnet.exe exec --depsfile "C:\git\extensions\artifacts\bin\Microsoft.AspNetCore.Testing.Tests\Debug\netcoreapp5.0\Microsoft.AspNetCore.Testing.Tests.deps.json" --runtimeconfig "C:\git\extensions\artifacts\bin\Microsoft.AspNetCore.Testing.Tests\Debug\netcoreapp5.0\Microsoft.AspNetCore.Testing.Tests.runtimeconfig.json"  "C:\Users\myuser\.nuget\packages\xunit.runner.console/2.4.1/tools/netcoreapp2.0/xunit.console.dll" "C:\git\extensions\artifacts\bin\Microsoft.AspNetCore.Testing.Tests\Debug\netcoreapp5.0\Microsoft.AspNetCore.Testing.Tests.dll" -noautoreporters -xml "C:\git\extensions\artifacts\TestResults\Debug\Microsoft.AspNetCore.Testing.Tests_netcoreapp5.0_x64.xml" -html "C:\git\extensions\artifacts\TestResults\Debug\Microsoft.AspNetCore.Testing.Tests_netcoreapp5.0_x64.html" -notrait "Flaky:All=true"

Unpacked failure command:

C:\Users\myuser\.nuget\packages\xunit.runner.console\2.4.1\tools\net472\xunit.console.exe "C:\git\extensions\artifacts\bin\Microsoft.AspNetCore.Testing.Tests\Debug\net472\Microsoft.AspNetCore.Testing.Tests.dll" -noshadow -xml "C:\git\extensions\artifacts\TestResults\Debug\Microsoft.AspNetCore.Testing.Tests_net472_x64.xml" -html "C:\git\extensions\artifacts\TestResults\Debug\Microsoft.AspNetCore.Testing.Tests_net472_x64.html" -notrait "Flaky:All=true"

dotnet.exe has a manifest that opts into Win10 support. xunit.console.exe by comparison doesn't have any compat information in its manifest.

Why don't these tests fail on other Win10 queues? Theory: This is our first queue that uses RS4 or later. There aren't any Win10 tests for lower versions.

@dougbu @MattGal Does this make sense?

@dougbu
Copy link
Member Author

dougbu commented Mar 4, 2020

Does this make sense?

Sounds reasonable. Should we switch to use the PlatformDetection extern approach? Or do you have a different solution in mind?

@Tratcher
Copy link
Member

Tratcher commented Mar 4, 2020

Let's start by skipping this test on .NET. The attribute isn't currently used for any .NET scenarios, only Core. I'll update the PR.

Second, I'll file a bug on xunit.

@Tratcher
Copy link
Member

Tratcher commented Mar 4, 2020

xunit/xunit#2076

Copy link
Member

@MattGal MattGal left a comment

Choose a reason for hiding this comment

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

LGTM

@dougbu
Copy link
Member Author

dougbu commented Mar 4, 2020

@JunTaoLuo @maryamariyan FYI our [MaximumOSVersion] and [MinimumOSVersion] testing attributes do not work correctly when targeting .NET Framework. Be careful (😺) if you plan to use these in dotnet/runtime.

@dougbu dougbu merged commit 26ff835 into master Mar 4, 2020
@dougbu dougbu deleted the dougbu/master/server.queues branch March 4, 2020 23:12
maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Mar 11, 2020
- add debug information for `Assert` failures in `MaximumOSVersionTest`
- update src/TestingUtils/Microsoft.AspNetCore.Testing/test/MaximumOSVersionTest.cs
  - co-Authored-By: @Tratcher 
- try with VS2019 queues
- skip `MaximumOSVersion` tests on .NET due to xunit issue
  - co-authored-by: @Tratcher 


Commit migrated from dotnet/extensions@26ff835
@Tratcher
Copy link
Member

Related: dotnet/runtime#33643

maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Mar 27, 2020
- add debug information for `Assert` failures in `MaximumOSVersionTest`
- update src/TestingUtils/Microsoft.AspNetCore.Testing/test/MaximumOSVersionTest.cs
  - co-Authored-By: @Tratcher 
- try with VS2019 queues
- skip `MaximumOSVersion` tests on .NET due to xunit issue
  - co-authored-by: @Tratcher 


Commit migrated from dotnet/extensions@26ff835
maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Mar 27, 2020
- add debug information for `Assert` failures in `MaximumOSVersionTest`
- update src/TestingUtils/Microsoft.AspNetCore.Testing/test/MaximumOSVersionTest.cs
  - co-Authored-By: @Tratcher 
- try with VS2019 queues
- skip `MaximumOSVersion` tests on .NET due to xunit issue
  - co-authored-by: @Tratcher 


Commit migrated from dotnet/extensions@26ff835
@ghost ghost locked as resolved and limited conversation to collaborators May 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants