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

Quick infrastructure fix-ups #25545

Merged
merged 2 commits into from
Sep 3, 2020
Merged

Conversation

dougbu
Copy link
Member

@dougbu dougbu commented Sep 2, 2020

  • pin Microsoft.Net.Compilers.Toolset version to isolate us from Arcade
    • the version now matches dotnet/runtime
    • will move the pin to previous version if anything breaks
  • double the macOS job max. length in our quarantined PR runs
    • have been seeing this timeout too often

- pin Microsoft.Net.Compilers.Toolset version to isolate us from Arcade
  - the version now matches dotnet/runtime
  - will move the pin to previous version if anything breaks
- double the macOS job max. length in our quarantined PR runs
  - have been seeing this timeout too often
@dougbu dougbu requested review from pranavkm and a team September 2, 2020 21:16
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Sep 2, 2020
@@ -329,9 +329,9 @@
<Uri>https://github.com/dotnet/arcade</Uri>
<Sha>4be47e467013f8a07a1ed7b6e49e39c8150bde54</Sha>
</Dependency>
<Dependency Name="Microsoft.Net.Compilers.Toolset" Version="3.8.0-2.20407.3">
<Dependency Name="Microsoft.Net.Compilers.Toolset" Version="3.8.0-2.20403.2" Pinned="true">
Copy link
Member Author

Choose a reason for hiding this comment

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

@pranavkm the previous version came in through adea663 aka #24738. Do you know if we took a dependency on anything changed after 3.8.0-2.20403.2❔ It's a small window compared to the upgrade in that PR but I'm confirming❕

Copy link
Member

Choose a reason for hiding this comment

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

Why pin this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@TanayParikh this won't protect us from downgrades in the 'master' branch 'til it's merged forward and also won't impact the aspnetcore ➡️ aspnetcore-tooling dependency updates 'til then. Suggest we trigger our Roslyn subscription to get back to being ahead of Arcade in this version on our 'master' branch. Do you know how to do that❔ If not, ping me in Teams.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why pin this?

This is for safety. We've seen runtime changes break roslyn and then roslyn break runtime and extensions too late in the game. Had an offline conversation about this w/ @Anipik @ericstj @jaredpar @mmitche @safern and we landed on this version. (Might move forward slightly later in RC2 but that'll be deliberate and coordinated.)

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 all. This version has been discussed in a few different places, the latest being dotnet/roslyn#46575. In RC2, runtime, aspnetcore, and extensions will use 3.8.0-2.20403.2. (aspnetcore-tooling doesn't have release/5.0* branches.)

The plan is different in our master branches. At least arcade, aspnetcore and aspnetcore-tooling will follow the 'VS 16.8' channel to get newer Microsoft.Net.Compilers.Toolset versions. We will likely move to use the 'VS Master' channel later in 6.0.

@@ -86,7 +86,7 @@ jobs:
jobName: MacOS_Quarantined_Test
jobDisplayName: "Tests: macOS 10.14"
agentOs: macOS
timeoutInMinutes: 60
timeoutInMinutes: 120
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, crap, I forgot to fix this in the regular job. Will go do that now

Copy link
Member Author

Choose a reason for hiding this comment

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

That job hasn't been timing out has it i.e. weren't the timeouts specific to the quarantined pipeline❔

Copy link
Member Author

Choose a reason for hiding this comment

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

And, oops, forgot you were doing this part

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, I thought you meant it was both - I can close #25546 if it's just this one

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, ci.yml doesn't specify timeouts (except for the Helix job) which means its macOS test job gets the default-build.yml default of 180 minutes. Since that's so close to when https://dev.azure.com/dnceng/public/_build/results?buildId=795980&view=results passed, I'll use 240 minutes there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me clarify, the aspnetcore-ci run completed after 90 minutes so it still passed without timing out. I think a default timeout of 180 minutes is still fine. I just wanted to mention that build because 90 minutes is more than 3x the usual time of about 25 mins. It's slow, but I don't think we need to adjust our timeout for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed a change to err on the safe side. The extra half hour timeout is less likely to cause problems elsewhere than having to retry a 1.5 hour job.

Copy link
Member Author

Choose a reason for hiding this comment

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

(Note I didn't double the timeout there😃)

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't paid too much attention to it after my ops rotation so my sample size is limited, but I've seen slowness in macos builds both on aspnetcore-ci and aspnetcore-quarantined-pr.

About the same for me as well. Seen occasional slowness for macos specifically.

@dougbu
Copy link
Member Author

dougbu commented Sep 2, 2020

@Pilchie 🆗 for RC2❔

TanayParikh added a commit to dotnet/razor that referenced this pull request Sep 2, 2020
- Pin Microsoft.Net.Compilers.Toolset version to isolate us from Arcade
  - The version now matches dotnet/runtime
  - Will move the pin to previous version if anything breaks

In conjunction with: dotnet/aspnetcore#25545

cc/ @dougbu
@Pilchie
Copy link
Member

Pilchie commented Sep 2, 2020

Yes, tell mode okay with me when CI is happy.

@ghost
Copy link

ghost commented Sep 3, 2020

Hello @dougbu!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost
Copy link

ghost commented Sep 3, 2020

Apologies, while this PR appears ready to be merged, it looks like release/5.0-rc2 is a protected branch and I have not been granted permission to perform the merge.

1 similar comment
@ghost
Copy link

ghost commented Sep 3, 2020

Apologies, while this PR appears ready to be merged, it looks like release/5.0-rc2 is a protected branch and I have not been granted permission to perform the merge.

@dougbu dougbu merged commit 3ef9fbe into release/5.0-rc2 Sep 3, 2020
@dougbu dougbu deleted the dougbu/timeouts.n.versions branch September 3, 2020 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants