Skip to content

Conversation

@vatsan-madhavan
Copy link
Member

@vatsan-madhavan vatsan-madhavan commented Jun 25, 2019

YAML improvements - various.

Fixes #1121

Intended to prevent build backlogs due to slow and overlapping builds that were observed to bottleneck PR'son 6/24.

  • Introduces a new AzDo build public pipeline - dotnet-wpf-citests
    • This is a test-signed pipeline
    • CI builds are disabled on this pipeline. Only PR builds are allowed [This is an AzDo pipeline setting]
      • image
    • Will create and test Release builds only on Helix
    • Helix test-runs can be disabled by disabling the pipeline on AzDo
      • image
    • Main AzDo pipeline dotnet-wpf CI will no longer run any tests (in Helix or on build machines locally).
      • It will only validate that the build succeeds.
      • We will open a new issue to re-add the capability to run tests locally on build-machines (could be something we want to turn on selectively).
  • CI builds will be batched
    • PR builds will remain un-batched as before
  • Documentation/* updates will not trigger builds.

/cc @dotnet/wpf-developers

@ghost ghost requested review from rladuca, ryalanms and stevenbrix June 25, 2019 05:14
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Jun 25, 2019
@vatsan-madhavan vatsan-madhavan force-pushed the dev/vatsan/yml-improvements branch from 1dc91f8 to 3d24e36 Compare June 25, 2019 05:15
@vatsan-madhavan
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@vatsan-madhavan
Copy link
Member Author

/azp where

@azure-pipelines
Copy link

Azure DevOps orgs getting events for this repository:

@vatsan-madhavan
Copy link
Member Author

/azp help stop

@vatsan-madhavan vatsan-madhavan marked this pull request as ready for review June 25, 2019 06:43
@vatsan-madhavan vatsan-madhavan added the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 25, 2019
@vatsan-madhavan
Copy link
Member Author

vatsan-madhavan commented Jun 25, 2019

@stevenbrix This PR attempts to separate build-validation from test-validation and attempts to run them in parallel in two separate build-pipelines.

I'm not sure that we really get much out of it - maybe it gives us a small dial. For e.g., we can configure msftbot to check for dotnet-wpf CI and dotnet-wpf-citests routinely when auto_merge label is set, but when a different label is used (say, fast_auto_merge), perhaps then it will only check for dotnet-wpf CI).

/cc @rladuca

@stevenbrix
Copy link
Contributor

@vatsan-madhavan does creating two build pipelines cause issues with packaging? Are the terms "pipeline" and "build definition" synonyms, or do they represent different things? If I remember correctly, there was an issue with some Arcade packages being produced from two different build definitions. I guess as long as the CI definition doesn't publish packages then we should be good?

@vatsan-madhavan
Copy link
Member Author

does creating two build pipelines cause issues with packaging? Are the terms "pipeline" and "build definition" synonyms, or do they represent different things?

I think it can.

They are the same. Pipeline is less ambiguous. Build Definition may refer to the pipeline, or the YAML (although I suspect that it really refers to the pipeline only).

Using multiple pipelines like corefx means we should ensure that only one pipeline ever runs for CI builds. That's easy to do - we can disable CI builds at pipeline definition level in AzDo and enable it just for PR builds.

=

My goal here is to experiment and learn what capabilities exist so we might choose from a palette over the next few days. I'm not really recommending that we employ 2 or more pipelines. If it turns out that using 2 pipelines would be helpful, we can do that - otherwise just stick to 1.

@vatsan-madhavan
Copy link
Member Author

I spent looking around to see what others are doing.

Here is what AspNet looks like (e.g. dotnet/aspnetcore#11660). Only their main CI pipeline is marked as required:

image

CoreClr is likewise splitting their PR builds into multiple pipelines, with several dedicated to testing (e.g., dotnet/coreclr#25350). None of the pipelines are marked as required (maybe not the setting we want), and the PR-merge history suggests that merging PR's with less-than-perfect pass-rate is the norm.

There are others that use only 1 pipeline (for e.g., MSBuild which is closer to us - averaging 34 mins, which includes testing. I suspect msbuild can run tests safely on the build-box...)

Overall, I like AspNet's approach.

@dotnet dotnet deleted a comment from azure-pipelines bot Jun 28, 2019
@dotnet dotnet deleted a comment from azure-pipelines bot Jun 28, 2019
@vatsan-madhavan vatsan-madhavan force-pushed the dev/vatsan/yml-improvements branch from dfdf5c2 to 9ceb6eb Compare June 28, 2019 22:29
@vatsan-madhavan
Copy link
Member Author

vatsan-madhavan commented Jun 28, 2019

@rladuca Take a look at this version. <10 mins to complete the main (required) portion of the CI, and generally holds only 2 machines while helix baesd tests are being run.

Disabling helix runs is as easy as disabling the dotnet-wpf-citests pipeline on AzDo (no yaml changes required).

What we don't get are local test-runs on build-machines.

@vatsan-madhavan vatsan-madhavan changed the title WIP: yml improvements yml improvements and helix decoupling to separate pipeline Jun 28, 2019
@rladuca
Copy link
Member

rladuca commented Jun 28, 2019

@rladuca Rob LaDuca FTE Take a look at this version. <10 mins to complete the main (required) portion of the CI, and generally holds only 2 machines while helix baesd tests are being run.

Disabling helix runs is as easy as disabling the dotnet-wpf-citests pipeline on AzDo (no yaml changes required).

What we don't get are local test-runs on build-machines.

This seems fine, but I think we should get a quick PR out and just disable Helix in the current builds.
@stevenbrix can we just quickly shut Helix off while we figure out the grand scheme of things w.r.t. multiple pipelines and possibly DRTs on build machines? I was thinking we turn it off, then try out DRTs on build machines after your PR, see what shakes out failure wise. Then we can start doing some of this more complicated work. At least we won't wait an hour every build in the interim.

edit: I guess we can just merge this and then iterate, but I would prefer if we shut off the test pipeline because it's just going to hold build resource with little real benefit.

@vatsan-madhavan
Copy link
Member Author

@rladuca, @stevenbrix, Still leaving as *NO MERGE*`, but removing WIP label. I want to merge this if you are ok with it.

@rladuca
Copy link
Member

rladuca commented Jun 28, 2019

@rladuca, @stevenbrix, Still leaving as *NO MERGE*`, but removing WIP label. I want to merge this if you are ok with it.

Can we turn off the test pipeline? It's going to consume 2 build machines each PR build and it's not really that useful.

@vatsan-madhavan
Copy link
Member Author

This seems fine, but I think we should get a quick PR out and just disable Helix in the current builds.

@rladuca This pretty much does exactly that - disables Helix in the current CI builds. It also spins up another pipeline for CI builds, which we don't have to keep enabled just yet (that's a policy conversation)

@vatsan-madhavan
Copy link
Member Author

@rladuca, @stevenbrix, Still leaving as *NO MERGE*`, but removing WIP label. I want to merge this if you are ok with it.

Can we turn off the test pipeline? It's going to consume 2 build machines each PR build and it's not really that useful.

easily done - yes.

@rladuca
Copy link
Member

rladuca commented Jun 28, 2019

@rladuca, @stevenbrix, Still leaving as *NO MERGE*`, but removing WIP label. I want to merge this if you are ok with it.

Can we turn off the test pipeline? It's going to consume 2 build machines each PR build and it's not really that useful.

easily done - yes.

Yeah I'm fine with that then, we should iterate further on getting @stevenbrix 's DRT changes to run on build machine and see the fail rates, etc. Then we can change things.

@stevenbrix
Copy link
Contributor

stevenbrix commented Jun 29, 2019

I like what you've done!

This seems fine, but I think we should get a quick PR out and just disable Helix in the current builds.
@stevenbrix can we just quickly shut Helix

Simplest thing would be to just make the condition for the "Run Developer Regression Tests in Helix" step always evaluate to false. Maybe adding a pipeline variable that we can pass to turn them on/off from the pipeline itself (default should be on). If we want to disable tests, this is probably the PR to do it. I don't think there is anything weird going on with Helix for us, what we're seeing is about what everyone else is seeing as well. I do agree that is probably overkill for what we are trying to accomplish right now. Once we have more tests running it will make more sense.

The WinUI team has done some cool stuff around determining whether a PR requires tests (for example, not running tests for simple documentation updates) that we can probably piggy back on!

@vatsan-madhavan
Copy link
Member Author

For now, I'm going to disable citests pipeline and merge this. We can make improvements to yml even more if we want to. Let's regroup and chat more.

@vatsan-madhavan vatsan-madhavan removed the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 1, 2019
@vatsan-madhavan vatsan-madhavan merged commit 3a42862 into master Jul 1, 2019
@vatsan-madhavan vatsan-madhavan deleted the dev/vatsan/yml-improvements branch July 1, 2019 17:49
@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

PR metadata: Label to tag PRs, to facilitate with triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve CI Build Times

4 participants