Restore Seperate test builds, but fix broken pipe #35378
Conversation
* Build CoreClr tests separately * Build coreclr targetGeneric tests separately for CI * Build coreclr targetGeneric tests separately in runtime.yml * Distinguish generic build based on Libraries config * Build OSX release libraries when CoreCLR changed * Always use tar.gz compression for generic tests * Mark all CoreCLR Interop/COM projects OsSpecific
|
Tagging subscribers to this area: @ViktorHofer |
| @@ -57,6 +58,11 @@ jobs: | |||
| - '${{ parameters.runtimeFlavor }}_common_test_build_p0_${{ parameters.managedTestBuildOsGroup }}${{ parameters.managedTestBuildOsSubgroup }}_${{ parameters.archType }}_${{parameters.buildConfig }}' | |||
| - ${{ if ne(parameters.testGroup, 'innerloop') }}: | |||
| - '${{ parameters.runtimeFlavor }}_common_test_build_p1_${{ parameters.managedTestBuildOsGroup }}${{ parameters.managedTestBuildOsSubgroup }}_${{ parameters.archType }}_${{parameters.buildConfig }}' | |||
| - ${{ if eq(parameters.testBuildPhased, true) }}: | |||
There was a problem hiding this comment.
Why do we have to still depend on the non-phased build if this is to true?
There was a problem hiding this comment.
testBuildPhased here means that the tests were built in two phases. TargetGeneric and TargetSpecific tests. We join them here by unzipping into the same directory.
There was a problem hiding this comment.
So if testBuildPhased==true we still depend on:
- '${{ parameters.runtimeFlavor }}_common_test_build_p0_${{ parameters.managedTestBuildOsGroup }}${{ parameters.managedTestBuildOsSubgroup }}_${{ parameters.archType }}_${{parameters.buildConfig }}'
There was a problem hiding this comment.
@trylek And I discussed this briefly during the original review.
We may eventually refactor and build the TargetSpecific tests as part of run-test-job. He was suggesting collecting stats to determine whether the extra build legs added value or not.
There was a problem hiding this comment.
That sounds like a reasonable, interesting idea.
There was a problem hiding this comment.
In fact, I was mostly worried (based on earlier discussions with @jashook) that adding too many legs can be counterproductive by increasing the load on machine allocation and provisioning. I however think that having the specific test build legs initially separate is probably a good idea to get an initial reading on the perf characteristics.
There was a problem hiding this comment.
Right, but if we refactor and just have build-test legs for the generic tests and then the target specific builds are done at the run-test-job, before sending to helix, we would be reducing the number of legs we will have by the number of target specific tests we currently have, so I think it would improve perf and reliability.
There was a problem hiding this comment.
I suspect you are right.
- We have reduce the target specific build by at least 90%.
- The extra leg appears expensive (maybe 50% overhead). I was seeing target specific 10 minute builds in the pipelines. I don't think it was that long on my local machine
- We should only have to rerun a given leg < 1% of the time (if we achieve 95% success).
- Fewer legs should reduce infra burden, so we could se benefits elsewhere.
- Seems like the savings could easily amortize the cost.
Filtering to build the TargetSpecific tests is currently slow. But if we make msbuild improvements this may go away.
sdmaclea
force-pushed
the
debugOnPrReleaseOnRolling
branch
from
9bced7d to
ca27831
Compare
|
I just forced pushed... |
Use isFullMatrix rather than debugOnPrReleaseOnRolling Simplify logic Fix another duplicate job case
| @@ -57,6 +58,11 @@ jobs: | |||
| - '${{ parameters.runtimeFlavor }}_common_test_build_p0_${{ parameters.managedTestBuildOsGroup }}${{ parameters.managedTestBuildOsSubgroup }}_${{ parameters.archType }}_${{parameters.buildConfig }}' | |||
| - ${{ if ne(parameters.testGroup, 'innerloop') }}: | |||
| - '${{ parameters.runtimeFlavor }}_common_test_build_p1_${{ parameters.managedTestBuildOsGroup }}${{ parameters.managedTestBuildOsSubgroup }}_${{ parameters.archType }}_${{parameters.buildConfig }}' | |||
| - ${{ if eq(parameters.testBuildPhased, true) }}: | |||
There was a problem hiding this comment.
In fact, I was mostly worried (based on earlier discussions with @jashook) that adding too many legs can be counterproductive by increasing the load on machine allocation and provisioning. I however think that having the specific test build legs initially separate is probably a good idea to get an initial reading on the perf characteristics.
| unpackFolder: '$(managedTestArtifactRootFolderPath)' | ||
| artifactFileName: '$(managedGenericTestArtifactName).tar.gz' | ||
| artifactName: '$(managedGenericTestArtifactName)' | ||
| displayName: 'generic managed test artifacts' |
There was a problem hiding this comment.
This might be worth 1-2 more comments explaining the generic vs. specific test management. It would be probably too nitpicky to alternate the displayName of the pre-existing download between "managed test artifacts" and "generic managed test artifacts" but we should try to keep these scripts easy to understand and possibly refactor.
|
Hello @sdmaclea! Because this pull request has the 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 (
|
…)" This reverts commit 6d64040.
ghost
locked as resolved and limited conversation to collaborators
No description provided.