Streamline DcpExecutor implementation#15721
Merged
Conversation
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15721Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15721" |
karolz-ms
force-pushed
the
dev/karolz/dcp-executor-cleanup
branch
from
d74f57b to
03055d5
Compare
karolz-ms
requested review from
JamesNK,
danegsta,
davidfowl and
mitchdenny
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors DcpExecutor by splitting resource watching/log streaming and resource creation concerns into new focused types (watcher + object creators), aiming to reduce coupling and make the DCP orchestration flow easier to maintain.
Changes:
- Introduces an
IObjectCreator<,>contract and moves executable/container creation logic intoExecutableCreatorandContainerCreator. - Extracts Kubernetes watch + snapshot publishing + log streaming into
DcpResourceWatcher. - Updates DCP executor/test plumbing to use the new abstractions, and adds a multi-semaphore acquisition helper.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/Utils/TestDcpExecutor.cs | Updates test executor stub to satisfy expanded IDcpExecutor contract. |
| tests/Aspire.Hosting.Tests/DistributedApplicationTests.cs | Adjusts formatting and updates expected error message for proxy-less endpoint/replica validation. |
| tests/Aspire.Hosting.Tests/Dcp/DcpExecutorTests.cs | Updates executor construction to inject new creator types and shared dependencies. |
| src/Aspire.Hosting/Utils/ConcurrencyUtils.cs | Adds AcquireAllAsync helper to acquire/release multiple semaphores safely. |
| src/Aspire.Hosting/DistributedApplicationBuilder.cs | Registers ExecutableCreator and ContainerCreator in DI. |
| src/Aspire.Hosting/Dcp/IObjectCreator.cs | Adds the object-creator contract and EmptyCreationContext. |
| src/Aspire.Hosting/Dcp/IDcpExecutor.cs | Expands executor surface area to support creator orchestration and expose AppResources. |
| src/Aspire.Hosting/Dcp/ExecutableCreator.cs | New creator responsible for preparing/creating Executable DCP resources (incl. cert/env/args). |
| src/Aspire.Hosting/Dcp/ContainerCreator.cs | New creator responsible for containers, container execs, networks, and tunnel setup. |
| src/Aspire.Hosting/Dcp/DcpResourceWatcher.cs | New watcher handling k8s watches, snapshot publishing, and log streaming lifecycle. |
| src/Aspire.Hosting/Dcp/DcpResourceState.cs | Generalizes app resource storage from AppResource to IAppResource. |
| src/Aspire.Hosting/Dcp/DcpModelUtilities.cs | New shared helpers (services-produced annotations, target-host normalization). |
| src/Aspire.Hosting/Dcp/DcpExecutor.cs | Rewires orchestration to delegate to watcher + creators; adds unified rendered-resource creation pipeline. |
| src/Aspire.Hosting/Dcp/ContainerCreationContext.cs | Adds ForAdditionalContainers and updates service resource typing. |
| src/Aspire.Hosting/Dcp/CertificateUtilities.cs | Extracts shared PEM certificate list creation helper. |
| src/Aspire.Hosting/Dcp/AppResource.cs | Introduces IAppResource and generic AppResource<T> with per-resource serialization semaphore/init tracking. |
| src/Aspire.Hosting/AspireEventSource.cs | Marks legacy executable creation events as obsolete in favor of generic DCP object events. |
| AGENTS.md | Adds guidance for using --hangdump + --hangdump-timeout with dotnet test. |
karolz-ms
force-pushed
the
dev/karolz/dcp-executor-cleanup
branch
from
c200d44 to
ee3a9a5
Compare
Contributor
Author
|
UPDATE: not caused by this change--seems preexisting issue #15777 |
JamesNK
reviewed
Apr 2, 2026
Member
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
Code review: 5 issues found (1 potential deadlock, 1 latent design issue, 1 dead code, 1 temporal coupling concern, 1 formatting).
JamesNK
reviewed
Apr 2, 2026
JamesNK
reviewed
Apr 2, 2026
# Conflicts: # src/Aspire.Hosting/Dcp/DcpExecutor.cs
- Extract DcpAppResourceStore for shared resource state - Extract IDcpObjectFactory interface for DCP object operations - Use method injection (IDcpObjectFactory parameter on CreateObjectAsync) instead of Lazy<IDcpObjectFactory> constructor injection - Remove Initialize(this) pattern and null! field assignments - Slim IDcpExecutor by moving factory methods to IDcpObjectFactory
karolz-ms
force-pushed
the
dev/karolz/dcp-executor-cleanup
branch
from
046d3db to
5e331d8
Compare
In particular, this ensures that --no-build is used when running the project. Should fix #15777
Contributor
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
Contributor
Author
|
@JamesNK thanks for the review and the patch, I believe I addressed all your feedback |
danegsta
reviewed
Apr 3, 2026
danegsta
reviewed
Apr 3, 2026
danegsta
approved these changes
Apr 3, 2026
Agent-Logs-Url: https://github.com/microsoft/aspire/sessions/425a3861-9761-436e-bdb9-289193163032 Co-authored-by: karolz-ms <15271049+karolz-ms@users.noreply.github.com>
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
Author
|
/azp run |
Contributor
|
🎬 CLI E2E Test Recordings — 55 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #23957256383 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a comprehensive simplification of DcpExecutor. Main ideas:
DcpAppResourceStore(shared resource bag) andIDcpObjectCreator(Kubernetes operations interface), eliminating theInitialize()/null!pattern.The result is that DcpExecutor shrinks from almost 3000 lines of highly coupled code to less than 1200 lines much easier to understand and work with (even with AI).
Description
Comprehensive refactoring of
DcpExecutorto reduce coupling and improve maintainability. Additional cleanup addressed during review:StartResourceAsyncby faulting theTaskCompletionSourcein theRunApplicationAsynccatch block.ServicesProducedshadowing issue onIAppResource/RenderedModelResource.// Materialize to check countcomment and unnecessary.ToList()inExecutableCreator.ConcurrencyUtils.cs.IAppResourceextendIDisposableand ensuredAppResource<T>instances are disposed on shutdown.#pragma warning disableentries to the top ofDcpExecutorTests.cs.Checklist