-
Notifications
You must be signed in to change notification settings - Fork 707
Add initial support for modeling deployment pipelines in Aspire #11953
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
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 11953Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 11953" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a pipeline-based deployment architecture to Aspire, refactoring the Azure deployment process from a monolithic approach into a composable, step-based pipeline system. This enables developers to customize and extend deployment workflows through explicit pipeline steps with dependency management.
- Introduces
IDistributedApplicationPipelineinterface and implementation for managing deployment workflows with dependency resolution - Refactors Azure deployment logic from
AzureDeployingContextinto discrete pipeline steps withinAzureEnvironmentResource - Adds well-known pipeline step names and annotations for resources to contribute their own deployment steps
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/Aspire.Hosting.Tests/Pipelines/DistributedApplicationPipelineTests.cs |
Comprehensive test suite covering pipeline execution, dependency resolution, error handling, and topological sorting |
src/Aspire.Hosting/Publishing/PublishingActivityReporter.cs |
Removes step-in-progress validation logic for interaction handling |
src/Aspire.Hosting/Publishing/Publisher.cs |
Updates deployment process to use pipeline execution instead of callback-based approach |
src/Aspire.Hosting/Publishing/DeployingContext.cs |
Adds pipeline output storage and retrieval methods for step communication |
src/Aspire.Hosting/Pipelines/WellKnownPipelineSteps.cs |
Defines standard pipeline step names for infrastructure, build, and deployment phases |
src/Aspire.Hosting/Pipelines/PipelineStepAnnotation.cs |
Annotation allowing resources to contribute pipeline steps during deployment |
src/Aspire.Hosting/Pipelines/PipelineStep.cs |
Core pipeline step class with dependency management capabilities |
src/Aspire.Hosting/Pipelines/IDistributedApplicationPipeline.cs |
Interface defining pipeline operations for adding and executing steps |
src/Aspire.Hosting/Pipelines/DistributedApplicationPipeline.cs |
Main pipeline implementation with topological sorting and parallel execution |
src/Aspire.Hosting/IDistributedApplicationBuilder.cs |
Exposes pipeline property for builder configuration |
src/Aspire.Hosting/DistributedApplicationBuilder.cs |
Integrates pipeline into builder and DI container |
src/Aspire.Hosting.Testing/DistributedApplicationTestingBuilder.cs |
Updates testing infrastructure to support pipeline functionality |
src/Aspire.Hosting.Azure/Provisioning/Provisioners/BicepProvisioner.cs |
Adds null-conditional operator for safe property access |
src/Aspire.Hosting.Azure/AzureEnvironmentResource.cs |
Refactors deployment logic into discrete pipeline steps with proper dependency chains |
src/Aspire.Hosting.Azure/AzureDeployingContext.cs |
Removes monolithic deployment class in favor of pipeline approach |
src/Aspire.Hosting.Azure.AppService/AzureAppServiceWebsiteContext.cs |
Adds website name output for pipeline consumption |
playground/pipelines/ |
Example playground demonstrating custom pipeline steps and Azure integration |
Comments suppressed due to low confidence (1)
src/Aspire.Hosting.Azure/AzureEnvironmentResource.cs:1
- Inconsistent casing in output key 'storagE_VOLUME_ACCOUNT_NAME' - should be 'STORAGE_VOLUME_ACCOUNT_NAME' to match the output definition.
// Licensed to the .NET Foundation under one or more agreements.
| { | ||
| try | ||
| { | ||
| var siteName = websiteResource.Outputs[$"{Infrastructure.NormalizeBicepIdentifier(websiteResource.Name)}_name"]?.ToString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had made a change to the AppServiceContext object to support setting this as an output but I reverted it in favor of #11931 so I don't have to deal with all the deltas to the Bicep snapshot tests in this PR. :D
This does technically mean this won't work as is.
| /// </summary> | ||
| [Experimental("ASPIREPIPELINES001", UrlFormat = "https://aka.ms/aspire/diagnostics/{0}")] | ||
| public class PipelineStep | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point, we'll probably want to add a State property to this type so that we can support retries and better auditing of which steps have failed/succeeded.
| /// Steps can declare dependencies on other steps to control execution order. | ||
| /// </remarks> | ||
| [Experimental("ASPIREPIPELINES001", UrlFormat = "https://aka.ms/aspire/diagnostics/{0}")] | ||
| public IDistributedApplicationPipeline Pipeline { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "Pipeline" too general of a name here? "DeploymentPipeline"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "Pipeline" is just generic enough. I can see a universe where we can hook into this pipeline infrastructure outside of the aspire deploy command since this is just about orchaestrating steps and their dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is pipeline really something that is publically global or is it an internal representation of annotations that are fully captured within resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is pipeline really something that is publically global
It's truly global and we support registering steps directly on the pipeline (not attached to any resources). During pipeline registration, we do a two-pass registration where we capture pipeline steps from resources and directly on the global object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way to see it is that there's a pipeline that resources and integrations contribute to.
| object? dependsOn = null, | ||
| object? requiredBy = null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this API more strongly typed? Should the base interface have an IEnumerable<string> and then have an extension method that takes just a single string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was kinda loosey-goosey with the typing (but there are checks inside) since I wasn't sure how we might want to evolve this shape. For example, maybe we have a StepBuilder that we can add dependencies onto instead of taking them as parameters in the original handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API needs work. Definitely feels a bit odd. The fact that it returns void is ripe for a buidler pattern 😍 .
builder.Pipeline.AddStep("foo", context => { })
.DependsOn("y")
.RequiredBy("x");| else if (exceptions.Count > 1) | ||
| { | ||
| throw new AggregateException( | ||
| $"Multiple pipeline steps failed at the same level: {string.Join(", ", exceptions.OfType<InvalidOperationException>().Select(e => e.Message))}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| $"Multiple pipeline steps failed at the same level: {string.Join(", ", exceptions.OfType<InvalidOperationException>().Select(e => e.Message))}", | |
| $"Multiple pipeline steps failed at the same level: {string.Join(", ", exceptions.Select(e => e.Message))}", |
Why the cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only want to report exceptions that were wrapped by the handling in the ExecuteStepAsync below. There's a chance that an exception is through during parallel execution that doesn't come from the individual step. We're ignoring those in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sus. Why would we only handle one type of error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the exception isn't wrapped in an IOE, it means it came from the parallel task execution for all the steps in the level, not from the steps itself. For those types of "runtime" exceptions, I think we'll want to handle the error case separately. For example, don't surface it as an aggregate error but implement retries on all steps in that level so we can make sure the state is set before we execute the next one.
| // Collect all exceptions from failed tasks | ||
| var exceptions = tasks | ||
| .Where(t => t.IsFaulted) | ||
| .SelectMany(t => t.Exception?.InnerExceptions ?? Enumerable.Empty<Exception>()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the InnerExceptions here? Is it because Task will wrap the actual exception thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see what's happening, ExecuteStepAsync is wrapping the exception in an InvalidOperationException.
I wonder if this logic is working correctly when paired with the OfType check below.
| .Where(r => r.ProvisioningTaskCompletionSource == null || | ||
| !r.ProvisioningTaskCompletionSource.Task.IsCompleted) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the pipeline is broken up into steps, there is a chance that someone inserts a step to provision a resource before our provisioning step. This avoids duplicate provisioning if deployment has already happened within the same instance. The ConfigureResource check isn't enough in this case since the provision state is in memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to tweak this but it's a decent work around for now. We might consider breaking up each one into dependent steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking up each one would help increase the amount of work we can do concurrently and speed up certain flows (like provisioning an ACR and pushing images to it), but the problem of checking to see if a previous step has already provisioned the resource you want would still remain. How are you thinking of tweaking this?
| await deploymentStateManager.SaveStateAsync( | ||
| provisioningContext.DeploymentState, | ||
| context.CancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it odd that before the deploymentStateManager state was across the whole deployment. Now it is only for the "provision" step. What if more steps wanted the deploymentState?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the only deployment state that we save is the Azure provisioning state.
If other steps want to interact with deployment state, they would have to resolve their own instance of the IDeploymentStateManager and then load, edit, and save the state.
This goes back to some of our conversatiosn around what the unit of deployment is and what we can consider to be a deployment transaction that will persist its state.
29e8672 to
0e421ec
Compare
1c3a227 to
2d4d80a
Compare
|
This is really simple to use but the APIs need work, that's not a blocker. We should change deployment to error when no steps exist vs no resources contribute steps. #:sdk Aspire.AppHost.Sdk@13.0.0-pr.11953.g2d4d80a6
var builder = DistributedApplication.CreateBuilder(args);
builder.Pipeline.AddStep("step", async context =>
{
await using var step = await context.ActivityReporter.CreateStepAsync("hello-step");
await Task.Delay(1000);
await using var task = await step.CreateTaskAsync("Hello from the pipeline!");
});
builder.Build().Run(); |
|
@davidfowl Good find! The fact that we don't get a full set of pipeline steps from both the global builder and resources might be a bit of a bug farm for these kinds of things. :/ Fixed in c981c59. |
| { | ||
| private readonly List<PipelineStep> _steps = []; | ||
|
|
||
| public bool HasSteps => _steps.Count > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not put this on the interface so the check is less hacky?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm...maybe...I kinda lowkey hate this property though 😭
|
|
||
| private static async Task PushImagesToAllRegistriesAsync(Dictionary<IContainerRegistry, List<IResource>> resourcesByRegistry, DeployingContext context, IResourceContainerImageBuilder containerImageBuilder) | ||
| { | ||
| var totalImageCount = resourcesByRegistry.Values.SelectMany(resources => resources).Count(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of low hanging performance fruit to clean up 😄
|
I'm pretty happy with this. I think we need lots of API polish but this is pretty good in my testing so far. I'll give it another go and will likely merge it. @captainsafia When you get a chance can you file some follow up issues. |
| using var content = new StreamContent(zipStream); | ||
| content.Headers.ContentType = new MediaTypeHeaderValue("application/zip"); | ||
|
|
||
| var response = await httpClient.PostAsync(kuduUrl, content, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No upload progress!?!? 😄 Biggest downside of the new ux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, we lacked the ability to do percentage-based progress in the old UX too. Assuming that's what you mean about upload progress. We'd likely need some deltas in the PublishingActivityReporter and backchannel types to support percentage-based progress.
|
Lots of things to follow up on but this will let us explore more scenarios and reshape the API. Some follow up issues:
Exploration
|
|
The file container idea will require the ability to build a container image but not deploy it. This should enable us to do that when in deployment mode. |
Contributes toward #11750
This PR introduces a new pipeline-based deployment architecture to Aspire, refactoring the Azure deployment process from a monolithic
AzureDeployingContextclass into a composable, step-based pipeline system. This allows developers to customize and extend the deployment process through explicit pipeline steps with dependency management.Note: this takes a dependency on #11780 since we are now executing deployment steps concurrently.
New Pipeline Infrastructure
IDistributedApplicationPipelineinterface andDistributedApplicationPipelineimplementation for managing deployment workflowsPipelineStepconcept with explicit dependency management (DependsOn,RequiredBy)PipelineStepAnnotationto allow resources to contribute their own pipeline stepsDistributedApplicationBuilder.PipelinepropertyRefactored Azure Deployment
AzureEnvironmentResourceas static pipeline step methods:ValidateAzureCliLoginAsync- Validates Azure CLI authenticationProvisionAzureBicepResourcesAsync- Provisions Bicep infrastructureBuildContainerImagesAsync- Builds container images for deploymentPushContainerImagesAsync- Pushes images to Azure Container RegistryDeployComputeResourcesAsync- Deploys compute resources (ACA/App Service)PrintDashboardUrlAsync- Displays dashboard URL after deploymentPipelineStepAnnotationonAzureEnvironmentResourceWell-Known Pipeline Steps
Introduced standardized step names in
WellKnownPipelineSteps:ProvisionInfrastructure- Infrastructure provisioning phaseBuildCompute- Container image building phaseDeployCompute- Compute resource deployment phase