-
Notifications
You must be signed in to change notification settings - Fork 715
feat(cli): aspire exec against container #10380
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
|
TODO:
note: this PR also merged #10422 in to include test changes |
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.
Still reviewing, but I thought I will send you these comments so you can consider them sooner.
| } | ||
|
|
||
| private Task CreateExecutablesAsync(IEnumerable<AppResource> executableResources, CancellationToken cancellationToken) | ||
| private Task CreateExecutableResourcesAsync( |
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 it is a mistake to handle Executable and ContainerExec together in a function that is named "CreateExecutableResourceAsync`. The Executables and ContainerExecutables are very different and the naming in code should emphasize the difference, not blur it.
My preference would be naming this method CreateSnapshotableResourcesAsync or similar.
Also the way this method is using the AspireEventSource is completely wrong and looks like it has been broken for a while. The DcpExecutablesCreateStart and DcpExecutablesCreateStop method pair was meant to measure the time it takes to actually create all Executable resources. With the way the code is now written, it measures the time to schedule Executable resource creation--a very different thing. We should push these calls to the place where the resource creation is actually happening. Also, we need a different (new) event to measure creation of ContainerExec resources, they deserve their own and should not share the same event with proper Executables.
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.
makes sense. I moved DcpExecutablesCreateStart and DcpExecutablesCreateStop to actual executable resource creation; introduced DcpContainerExecutablesCreateStart and DcpContainerExecutablesCreateStop which also show creation the actual containerExec resource
removed the aggregated event as was before - should we introduce even another one?
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.
Hope this helps! (I mean the previous review mostly)
|
This feature has some interesting implications. Here we are limiting its scope to But then you need to think about how that translates to deployment because not every compute environment is going to support that kind of model - and how does that relate to a sidecar concept. Finally - there is some intersection here with healthchecks as well. We could potentially hide the underlying implemnetation here a bit but allow people to have container health check commands which run a command in the container to determine whether it is healthy or not and flow that back out into the apphost. At deployment time that could get translated into a health check for orchestrators that support it. |
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.
Looks really good, just one more thing (related to EventSource events)
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 extends the aspire exec CLI command to support execution inside containers. The implementation allows users to run commands like aspire exec --resource nginxcontainer ls to execute commands within running containers and stream the output back to the CLI.
Key changes implemented:
- Added support for
ContainerExecresources in the DCP layer with proper lifecycle management - Introduced
ContainerExecutableResourceas an internal model resource for container execution - Extended
ExecResourceManagerto handle container resources alongside existing project resources
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/Aspire.Hosting.Tests/Backchannel/Exec/ContainerResourceExecTests.cs |
New test file validating container exec functionality with nginx container |
src/Shared/Model/KnownResourceTypes.cs |
Added ContainerExec resource type constant |
src/Aspire.Hosting/Exec/ExecResourceManager.cs |
Extended to support container resources and added container-specific exec logic |
src/Aspire.Hosting/Dcp/ResourceSnapshotBuilder.cs |
Added snapshot building support for ContainerExec resources |
src/Aspire.Hosting/Dcp/Model/ContainerExec.cs |
Enhanced Create method to accept args and working directory parameters |
src/Aspire.Hosting/Dcp/DcpResourceState.cs |
Added ContainerExecsMap for tracking container exec resources |
src/Aspire.Hosting/Dcp/DcpNameGenerator.cs |
Extended name generation to handle ContainerExecutableResource |
src/Aspire.Hosting/Dcp/DcpExecutor.cs |
Major updates to support container exec lifecycle, logging, and resource management |
src/Aspire.Hosting/ContainerExecutableResourceExtensions.cs |
New extension methods for retrieving container executable resources |
src/Aspire.Hosting/BuiltInDistributedApplicationEventSubscriptionHandlers.cs |
Added DCP name population for container executables |
src/Aspire.Hosting/AspireEventSource.cs |
Added telemetry events for container executable creation and updated existing events |
src/Aspire.Hosting/ApplicationModel/ContainerExecutableResource.cs |
New internal resource class representing executable commands in containers |
Comments suppressed due to low confidence (1)
tests/Aspire.Hosting.Tests/Backchannel/Exec/ContainerResourceExecTests.cs:55
- The 'file' access modifier should be 'file-scoped' instead of 'file'. Use 'file sealed class' or place the class declaration at file scope level.
file sealed class TestContainerResource : ContainerResource
Co-authored-by: Korolev Dmitry <[email protected]>
| /// Executable resource that runs in a container. | ||
| /// </summary> | ||
| internal class ContainerExecutableResource(string name, ContainerResource containerResource, string command, string? workingDirectory) | ||
| : Resource(name), IResourceWithEnvironment, IResourceWithArgs, IResourceWithEndpoints, IResourceWithWaitSupport |
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.
@DeagleGross this merged in before I got a chance to fully review it - but one thing we should do here is ditch the TargetContainerResource property and make this resource implement the IResourceWithParent interface.
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 should also get rid of the Args property and instead make it work similar to the ExecutableResource. Because you are implementing IResourceWithArgs it means that this resource should be able to work with the WithArgs method - and it should honor all of the semantics around deferred evaluation etc.
Description
Is an extension of
aspire execcli command, but now targeting execution the commands inside of the container. Aspire exec allows to run for example such commandaspire exec --resource nginxcontainer lsto get a list of files in the container "nginxcontainer". Aspire streams the logs into cli for the user.Implementation
The implementation is following the #10061 bits;
DcpExecutordid not have a proper support forContainerExecso I've added that + I've introduced an internalContainerExecutableResourcewhich is being built atExecResourceManager. The point is that there has to be a "model resource" which is then converted into "dcp resource" byDcpExecutor.Related to #10301 (aspire cli part)
Checklist