Optimize CI by categorizing tests and parallelizing builds even more - #5715
Optimize CI by categorizing tests and parallelizing builds even more#5715johnsimons wants to merge 9 commits into
Conversation
| @@ -0,0 +1,93 @@ | |||
| name: Setup IBM MQ | |||
| description: > | |||
| Runs IBM MQ in a Linux container for the duration of the job. On Linux runners the container runs | |||
There was a problem hiding this comment.
Will be ready soon. I think this is total overkill to introduce this now
There was a problem hiding this comment.
this is just temp till is ready
| # The test matrix only builds each category's own dependency closure, so this job is what keeps | ||
| # projects outside every closure (HealthCheckApp, Particular.PlatformSample.ServiceControl, | ||
| # LegacyArtifacts) compiling. It runs alongside the matrix and is not on its critical path. | ||
| compile: |
There was a problem hiding this comment.
Have you considered building once, upload artifacts and download artifacts in the test jobs?
There was a problem hiding this comment.
Might be worth a spike: build once per OS in a dedicated job, upload the test outputs, and have each test job download its category and run. needs: would give us the no-resources-on-failed-build guarantee structurally instead of by ordering, and the runner minute count would drop well below 149m: two builds plus a download per test job instead of a scoped build per job. The catch is the size of the artifacts. The Release output adds up to roughly 10 GB per OS, with each RavenDB test project carrying its own copy of the RavenDB server for both net8.0 and net10.0, and the org's Team plan allows 2 GB of artifact storage per repository. Artifacts would only fit once the RavenDB server moves out of the test output (a shared download, tools/download-ravendb-server.ps1 already exists) and test runs go single-TFM
There was a problem hiding this comment.
I am spiking it, but according the Claude, that is not going to buy much time back
There was a problem hiding this comment.
So I looked at this, and the only way to make it come close to the 8 mins mark is to compile it all in Linux, but that is not possible because we need .exe.
So compiling per OS would mean everything is waiting on the Windows compiler, which can take up to 5mins!
|
One thing I want to raise before this merges: backgrounding the build changes when we provision Azure resources. On master the build runs before any setup step, so a broken commit fails the job before If the runner is killed or times out, that teardown does not happen at all. This is exactly the case we deliberately avoided. We would rather create no resources for a build that cannot run tests than create them and clean them up afterwards. The fix is small. Move - name: Setup WSL
# ...SQL, PostgreSQL, RabbitMQ, IBM MQ, azure-cli install stay here, still overlapping the build...
- name: Wait for build
wait: build
- name: Azure login
if: matrix.test-category == 'AzureServiceBus' || matrix.test-category == 'AzureStorageQueues'
# ...Setup ASB / Setup ASQ...
- name: Run tests A failed build then fails the wait step and the job stops before login. The container categories keep the full overlap, since their infrastructure is local and never touches Azure. Only ASB and ASQ (4 jobs) lose it, roughly a minute each by my estimate, which puts it back around 9.5m instead of 8.6m. I think that is a fair price for keeping the invariant. |
This refactors the CI workflow to improve efficiency and reduce execution time. Test projects are now assigned `TestCategory` properties, enabling matrix jobs to dynamically select, build, and run only the relevant projects for their category. This significantly reduces build times and resource consumption. A new `compile` job ensures the entire `src` directory remains buildable. The build step for test jobs is now backgrounded to overlap with infrastructure setup, further improving parallelization. Also corrects inconsistent `PostgreSQL` casing to `PostgreSql` in the workflow.
Refines the CI workflow by breaking down the broad `Default` and `RabbitMQ` test categories into more granular ones. This addresses instances where these categories were running several slow assemblies in a single job. - `Default` is split into `DefaultCore`, `DefaultAudit`, and `DefaultMonitoring`. - `RabbitMQ` is split into `RabbitMQClassicConventional`, `RabbitMQClassicDirect`, `RabbitMQQuorumConventional`, and `RabbitMQQuorumDirect`. This change enables finer-grained parallelization of tests in the CI workflow, which reduces overall execution time and improves efficiency. It also standardizes test category declarations across projects using the `<TestCategory>` property and `IncludeInTestCategory` attribute, and updates CI conditions accordingly.
RabbitMQ now uses Particular/setup-rabbitmq-action@v2.0.0, which gained WSL support upstream, so no local action is needed for it.
The `ServiceControl.MultiInstance.AcceptanceTests` now explicitly creates both the primary and audit event sources on Windows. This ensures the necessary EventLog sources are available for the tests. Previously, the audit event source was implicitly created by other acceptance tests, which is no longer guaranteed due to CI changes like parallelization.
Notifications settings are rarely read and primarily edited by hand. Aggressive caching, which invalidates asynchronously via the Changes API, could lead to stale reads immediately following a save. This change ensures the latest settings are always retrieved, preventing potential consistency issues.
Improves the reliability of GitHub Actions for IBM MQ and RabbitMQ. IBM MQ's health check now directly probes the listener port, as Docker's internal check was insufficient for detecting connectivity issues, especially on Windows/WSL. RabbitMQ setup is made more robust by automatically restarting the container once if it hangs during boot within 90 seconds. Additionally, WSL memory allocation is increased to 8GB to provide more headroom and prevent resource-related failures during concurrent builds.
Moves the Azure Service Bus and Storage Queues setup steps to occur only after the build is successful. This prevents the creation of real cloud resources if the build fails, reducing unnecessary provisioning and potential orphaned resources.
38a27a7 to
de708ab
Compare
Removes redundant `TestsFilter.cs` files and specialized `IncludeIn...TestsAttribute.cs` attributes. Test categories are now declared once via the `` MSBuild property in each test project, which `Directory.Build.props` uses to automatically generate the corresponding `IncludeInTestCategoryAttribute`. This ensures consistency between CI test filtering, which uses the MSBuild property, and local test runs filtered by `ServiceControl_TESTS_FILTER`, which relies on the assembly attribute. It also simplifies test project configuration by removing boilerplate files.
Introduces a build-time check in `select-test-projects.ps1` to prevent test projects from being silently ignored by CI if they reference the test SDK but do not declare a ``. Updates `README.md` with enhanced documentation on adding test projects and categories, utilizing GitHub Admonitions for improved clarity and visibility of important configuration details.
@danielmarbach this is now also fixed, and it seems we are still under 9mins |
Why
CI took ~14.3 minutes wall clock. The assumption was that Windows runners are just slow, so pre-compiling on Linux would fix it. Measuring run 31189671891 showed the real cause was redundant work:
dotnet build srcbuilt all 76 projects in all 25 jobs: 180-281s on WindowsAssert.Ignore, so irrelevant ones still spin up. Windows-AzureServiceBus burned 73s of its 372s on thisServiceControlInstaller.Packagingneeds itBut the dependency closures are tiny: a transport test project needs 6 of 76 projects.
A cross-OS binary artifact was considered and rejected. It makes the build a serial stage (est. ~12.4m, barely better) and risks the
net10.0-windowsWPF/WinUI projects, Linux ELF apphosts, and absolute Linux NuGet paths inruntimeconfig.dev.json.Result
Windows-AzureServiceBus, the old bottleneck, went 14.3m → 7.0m.
What changed
Scoped builds. Each test project declares
<TestCategory>.tools/select-test-projects.ps1scans for it and generates atests.projtraversal file (usingMicrosoft.Build.NoTargets, already pinned inglobal.json), so a job builds only its category's closure.tools/run-tests.ps1then runs only that category's assemblies.Overlapped build and infrastructure, using the new parallel steps feature. The build is
background: trueand the job joins it atwait: buildafter the infra chain.backgroundrather than aparallel:group because the infra steps are an ordered chain (WSL before the database actions) that must run concurrently with the build, which a group can't express.Split the two fat categories.
Default→DefaultCore/DefaultAudit/DefaultMonitoring, andRabbitMQ→ one per routing topology. Both were single jobs running several slow assemblies back to back.Containerised the Windows infrastructure. Local
setup-rabbitmqandsetup-ibmmqactions run their Linux containers in WSL2 on Windows, replacing Azure Container Instances for RabbitMQ and enabling IBM MQ on Windows for the first time.Setup WSLis now gated to the categories that need it, andAzure logindropped to just the two categories that actually provision cloud resources.Added a
compilejob building all 76 projects on both OSes. Scoped builds mean projects outside every test closure (HealthCheckApp,Particular.PlatformSample.ServiceControl,LegacyArtifacts) would otherwise stop being compiled at all.Bugs found along the way
PostgreSQL tests had never run. The matrix used
PostgreSQL/PostgreSQLPersistence; the attributes declarePostgreSql/PostgreSqlPersistence; the comparison is ordinal. 310 persistence + 23 transport tests were silently skipped, and four jobs burned ~21 minutes testing nothing. They now run.Notifications settings could read stale after a write.
NotificationsManager.LoadSettingsread throughAggressivelyCacheForAsync(5 minutes), which invalidates asynchronously via the Changes API, so a read straight after a save could return the pre-save document. This is user-visible: save SMTP settings, re-read, see the old values. Removed.MultiInstance tests depended on assembly ordering. They host a primary and an audit instance but only pre-created the
ServiceControlevent source, relying onAudit.AcceptanceTests.RavenDBrunning earlier in the same job to create the audit one. Now explicit.Six required status checks point at job names this PR renames, so they will never report:
Linux-Default/Windows-Default-DefaultCore,-DefaultAudit,-DefaultMonitoringLinux-PostgreSQL/Windows-PostgreSQL-PostgreSql(casing)Linux-RabbitMQ/Windows-RabbitMQ-RabbitMQ{ClassicConventional,ClassicDirect,QuorumConventional,QuorumDirect}Worth also requiring the new
Linux-Compile/Windows-Compilejobs, since they are now the only thing compiling projects outside the test closures.Notes for reviewers
tools/run-tests.ps1replacesParticular/run-tests-action, which has no way to be told which projects to run. It should fold back into that action once it grows aprojectsinput.setup-rabbitmq/setup-ibmmqactions are deliberately self-contained so they can move to their ownParticular/setup-*-actionrepos. They have no teardown because composite actions can't declarepost:steps; hosted-runner VMs are destroyed anyway.concurrency, so two overlapping runs will queue. Addingcancel-in-progresson PR builds would likely free more capacity than this costs.