Add command annotations and wire up #5538

JamesNK · 2024-09-04T13:13:40Z

Fixes #1410 (cleaned up test while I was here)
Fixes #295

Changes in PR:

Add resource command annotations API
Resource commands talking to host and invoking annotation callbacks
Start/stop/restart added to containers/projects/executable
Start/stop/restart unit and functional tests

Checklist

Is this feature complete?
- Yes. Ready to ship.
- No. Follow-up changes expected.
Are you including unit tests for the changes and scenario tests if relevant?
- Yes
- No
Did you add public API?
- Yes
  - If yes, did you have an API Review for it?
    - Yes - will schedule
    - No
  - Did you add <remarks /> and <code /> elements on your triple slash comments?
    - Yes
    - No
- No
Does the change make any security assumptions or guarantees?
- Yes
  - If yes, have you done a threat model and had a security review?
    - Yes
    - No
- No
Does the change require an update in our Aspire docs?
- Yes
  - Link to aspire-docs issue: [New article]: Resource commands for 9.0 docs-aspire#1661, Start/stop/restart for 9.0 docs-aspire#1662
- No

Microsoft Reviewers: Open in CodeFlow

src/Aspire.Hosting/ApplicationModel/ResourceCommandAnnotation.cs

src/Aspire.Hosting/Dashboard/CommandsConfigurationExtensions.cs

src/Aspire.Hosting/ApplicationModel/ResourceCommandAnnotation.cs

src/Aspire.Hosting/Dashboard/CommandsConfigurationExtensions.cs

src/Aspire.Hosting/ExecutableResourceBuilderExtensions.cs

mitchdenny · 2024-09-04T13:50:05Z

Looking good overall. This was pretty close to what I was envisaging. Left a few comments to discuss.

src/Aspire.Hosting/ApplicationModel/ResourceCommandAnnotation.cs

src/Aspire.Hosting/Dashboard/CommandsConfigurationExtensions.cs

src/Aspire.Hosting/Dashboard/DashboardServiceData.cs

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs

JamesNK · 2024-09-05T07:27:45Z

Made a bunch of changes and applied PR feedback.

I think this is close to being good. (Ignore lack of API xml comments and the fact that start/stop/restart don't actually do anything yet 😄)

src/Aspire.Hosting/ApplicationModel/ResourceCommandAnnotation.cs

src/Aspire.Hosting/Dashboard/CommandsConfigurationExtensions.cs

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs

src/Aspire.Hosting/Dashboard/DashboardLifecycleHook.cs

src/Aspire.Hosting/Dashboard/DashboardServiceHost.cs

Directory.Packages.props

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs

src/Aspire.Hosting/Dcp/ApplicationExecutor.cs

davidfowl · 2024-09-11T03:13:07Z

src/Aspire.Hosting/Dashboard/CommandsConfigurationExtensions.cs

+                await executor.StopResourceAsync(context.ResourceName, context.CancellationToken).ConfigureAwait(false);
+                await executor.StartResourceAsync(context.ResourceName, context.CancellationToken).ConfigureAwait(false);


Does this work? Start/Stop in quick succession?

I talked with @karolz-ms about restarting containers, and it sounds like that work might not get done for 9 because wait for is prioritized. Therefor to restart both containers and executables from the app host, we'll delete the resource and then recreate it.

That means the StopResourceAsync may or may not be needed here. Do we want to gracefully stop a resource before it starting it up again? Or do we just delete and recreate it? Delete+create would be faster.

src/Aspire.Hosting/Dcp/ApplicationExecutor.cs

src/Aspire.Hosting/ExecutableResourceBuilderExtensions.cs

JamesNK · 2024-09-18T23:40:39Z

However, when applied more broadly the resource state snapshot is not the only input into deciding whether a command might be enabled or not. For example you might want to query the state of the resource directly to determine whether a command should be available. This would require resolving the connection string to the resource which is an async operation.

Here is the problem with what you're saying: the update method is only triggered when the resource snapshot updates. If you need to talk to the database to figure out the state, you can only do that if something updates the resource snapshot. The resource might be in a happy Running state since it starts up and the command state methods are never called.

For example, there is a command to perform an EF migration. It has an update state method that queries the DB to see if the command is enabled or disabled. The command could be enabled and some other tool could perform the migration - so the command should be disabled - but the command stays enabled because the resource snapshot doesn't update.

The way this could work instead is when the resource starts there is a background task that queries the database (or file, or web service, etc) every 10 seconds to figure out whether the command is valid or not. That task uses ResourceNotificationService to set a property on the resource snapshot. The command update method then looks at that property to figure out whether it is enabled or not.

We probably shouldn't be polling this multiple times a second anyway (its UX behind a context menu). If we are concerned about the overhead of running the state updates for commands then we can always poll at a lower interval.

This isn't from polling. The methods are re-evaluated when the resource state updates, which is pushed from the notification service. A resource starting up or shutting down has half a dozen rapid resource state updates as the state and endpoints change.

If there is a command state update method that does something like query a web service, and the network is down, then it could take seconds to time out. Resource updates in Asoure are now going to be slowed down while waiting for that timeout, multipled by the number of times it is called.

mitchdenny · 2024-09-19T09:09:19Z

src/Aspire.Dashboard/Components/Controls/ResourceActions.razor

@@ -2,12 +2,12 @@
 @using Aspire.Dashboard.Model
 @using Microsoft.FluentUI.AspNetCore.Components

-@foreach (var highlightedCommand in Commands.Where(c => c.IsHighlighted))
+@foreach (var highlightedCommand in Commands.Where(c => c.IsHighlighted && c.State != CommandViewModelState.Hidden))


Should there be a maximum number of highlighted commands? Or rather only the first N highlighted commands?

Maybe. It's easy to change if necessary.

mitchdenny · 2024-09-19T09:10:41Z

src/Aspire.Dashboard/Components/Controls/ResourceActions.razor.cs

@@ -49,7 +49,7 @@ protected override void OnParametersSet()
            OnClick = OnConsoleLogs.InvokeAsync
        });

-        var menuCommands = Commands.Where(c => !c.IsHighlighted).ToList();
+        var menuCommands = Commands.Where(c => !c.IsHighlighted && c.State != CommandViewModelState.Hidden).ToList();


Should the context menu show all commands regardless of whether they are highlighted or not?

Dunno. It's easy to change.

mitchdenny · 2024-09-19T09:13:17Z

src/Aspire.Dashboard/Components/ResourcesGridColumns/StateColumnDisplay.razor

@@ -33,7 +33,7 @@
                    Class="severity-icon" />
    }
 }
-else if (Resource.IsStartingOrBuildingOrWaiting())
+else if (Resource.IsUnusableTransitoryState())


It's a race with #5770 :)

src/Aspire.Hosting/Dcp/ApplicationExecutor.cs

mitchdenny

LGTM. Some minor points of feedback that can be follow up PRs if necessary.

JamesNK · 2024-09-20T02:12:10Z

/azp run

azure-pipelines · 2024-09-20T02:12:24Z

Azure Pipelines successfully started running 1 pipeline(s).

davidfowl · 2024-09-20T02:22:18Z

src/Aspire.Hosting/Dcp/ApplicationExecutor.cs

+            if (!resourceNotFound)
+            {
+                // Limit polling to 5 attempts to avoid hanging with an infinite loop.
+                for (var i = 0; i < 5; i++)


Why don't you use polly 😄

I've used polly zero times and I've used for loops a million times 😋

Next time I make start/stop/restart changes in this area I'll try it out.

It's in this same file

Since the build is failing anyway, changed to polly.

Observation: I haven't used polly before, but should the polly options and pipelines be cached? They're created every run right now. Might not be necessary.

JamesNK · 2024-09-20T03:26:03Z

/azp run

azure-pipelines · 2024-09-20T03:26:17Z

Azure Pipelines successfully started running 1 pipeline(s).

JamesNK added the area-dashboard label Sep 4, 2024

JamesNK requested review from davidfowl, drewnoakes, mitchdenny and karolz-ms September 4, 2024 13:13

JamesNK commented Sep 4, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceCommandAnnotation.cs Outdated Show resolved Hide resolved

src/Aspire.Hosting/Dashboard/CommandsConfigurationExtensions.cs Outdated Show resolved Hide resolved

mitchdenny reviewed Sep 4, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceCommandAnnotation.cs Show resolved Hide resolved

mitchdenny reviewed Sep 4, 2024

View reviewed changes

src/Aspire.Hosting/Dashboard/CommandsConfigurationExtensions.cs Outdated Show resolved Hide resolved

mitchdenny reviewed Sep 4, 2024

View reviewed changes

src/Aspire.Hosting/Dashboard/CommandsConfigurationExtensions.cs Outdated Show resolved Hide resolved

mitchdenny reviewed Sep 4, 2024

View reviewed changes

src/Aspire.Hosting/ExecutableResourceBuilderExtensions.cs Show resolved Hide resolved

karolz-ms reviewed Sep 4, 2024

View reviewed changes

davidfowl reviewed Sep 4, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs Outdated Show resolved Hide resolved

karolz-ms approved these changes Sep 6, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceCommandAnnotation.cs Show resolved Hide resolved

Base automatically changed from jamesnk/commands-glow-up to main September 6, 2024 10:38

davidfowl reviewed Sep 7, 2024

View reviewed changes

src/Aspire.Hosting/Dashboard/CommandsConfigurationExtensions.cs Outdated Show resolved Hide resolved

davidfowl reviewed Sep 7, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs Outdated Show resolved Hide resolved

JamesNK force-pushed the jamesnk/command-annotations-and-wireup branch from c1cfac8 to 45b8706 Compare September 9, 2024 00:30

davidfowl reviewed Sep 9, 2024

View reviewed changes

src/Aspire.Hosting/Dashboard/DashboardLifecycleHook.cs Outdated Show resolved Hide resolved

davidfowl reviewed Sep 9, 2024

View reviewed changes

src/Aspire.Hosting/Dashboard/DashboardServiceHost.cs Outdated Show resolved Hide resolved

JamesNK force-pushed the jamesnk/command-annotations-and-wireup branch from 8f49cda to 9996ae5 Compare September 10, 2024 00:10

davidfowl reviewed Sep 11, 2024

View reviewed changes

Directory.Packages.props Show resolved Hide resolved

davidfowl reviewed Sep 11, 2024

View reviewed changes

src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs Outdated Show resolved Hide resolved

davidfowl reviewed Sep 11, 2024

View reviewed changes

src/Aspire.Hosting/Dcp/ApplicationExecutor.cs Outdated Show resolved Hide resolved

davidfowl reviewed Sep 11, 2024

View reviewed changes

davidfowl reviewed Sep 12, 2024

View reviewed changes

src/Aspire.Hosting/Dcp/ApplicationExecutor.cs Show resolved Hide resolved

JamesNK force-pushed the jamesnk/command-annotations-and-wireup branch from e4cb2b8 to 5c17fb3 Compare September 12, 2024 05:24

davidfowl reviewed Sep 12, 2024

View reviewed changes

src/Aspire.Hosting/ExecutableResourceBuilderExtensions.cs Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Sep 12, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

JamesNK force-pushed the jamesnk/command-annotations-and-wireup branch from 3987b04 to d7fefb8 Compare September 19, 2024 02:25

build-analysis bot mentioned this pull request Sep 19, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

JamesNK changed the title ~~[WIP] Add command annotations and wire up~~ Add command annotations and wire up Sep 19, 2024

JamesNK force-pushed the jamesnk/command-annotations-and-wireup branch from e9babf4 to 97ac713 Compare September 19, 2024 08:41

JamesNK marked this pull request as ready for review September 19, 2024 08:41

JamesNK requested a review from ReubenBond as a code owner September 19, 2024 08:41

mitchdenny reviewed Sep 19, 2024

View reviewed changes

src/Aspire.Hosting/Dcp/ApplicationExecutor.cs Outdated Show resolved Hide resolved

mitchdenny approved these changes Sep 19, 2024

View reviewed changes

JamesNK enabled auto-merge (squash) September 19, 2024 14:41

Add command annotations and wire up

63a9a90

JamesNK force-pushed the jamesnk/command-annotations-and-wireup branch from 3506e18 to 63a9a90 Compare September 20, 2024 00:32

This was referenced Sep 20, 2024

[New article]: Resource commands for 9.0 dotnet/docs-aspire#1661

Open

Start/stop/restart for 9.0 dotnet/docs-aspire#1662

Closed

davidfowl reviewed Sep 20, 2024

View reviewed changes

JamesNK added 3 commits September 20, 2024 12:39

Fix flaky test?

b5d692d

Use polly

caaa224

Add delete failure test

223d9eb

JamesNK merged commit a5b3bcf into main Sep 20, 2024
11 checks passed

JamesNK deleted the jamesnk/command-annotations-and-wireup branch September 20, 2024 07:19

davidfowl mentioned this pull request Sep 20, 2024

Inject commands on all DCP based resources #5802

Merged

16 tasks

davidfowl added this to the 9.0 milestone Sep 20, 2024

github-actions bot locked and limited conversation to collaborators Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add command annotations and wire up #5538

Add command annotations and wire up #5538

JamesNK commented Sep 4, 2024 •

edited by davidfowl

Loading

mitchdenny commented Sep 4, 2024

JamesNK commented Sep 5, 2024

davidfowl Sep 11, 2024

JamesNK Sep 11, 2024 •

edited

Loading

JamesNK commented Sep 18, 2024 •

edited

Loading

mitchdenny Sep 19, 2024 •

edited

Loading

JamesNK Sep 19, 2024

mitchdenny Sep 19, 2024

JamesNK Sep 19, 2024

mitchdenny Sep 19, 2024

mitchdenny left a comment

JamesNK commented Sep 20, 2024

azure-pipelines bot commented Sep 20, 2024

davidfowl Sep 20, 2024

JamesNK Sep 20, 2024

davidfowl Sep 20, 2024

JamesNK Sep 20, 2024

JamesNK commented Sep 20, 2024

azure-pipelines bot commented Sep 20, 2024

		await executor.StopResourceAsync(context.ResourceName, context.CancellationToken).ConfigureAwait(false);
		await executor.StartResourceAsync(context.ResourceName, context.CancellationToken).ConfigureAwait(false);

Add command annotations and wire up #5538

Add command annotations and wire up #5538

Conversation

JamesNK commented Sep 4, 2024 • edited by davidfowl Loading

Checklist

Microsoft Reviewers: Open in CodeFlow

mitchdenny commented Sep 4, 2024

JamesNK commented Sep 5, 2024

Choose a reason for hiding this comment

JamesNK Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

JamesNK commented Sep 18, 2024 • edited Loading

mitchdenny Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchdenny left a comment

Choose a reason for hiding this comment

JamesNK commented Sep 20, 2024

azure-pipelines bot commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK commented Sep 20, 2024

azure-pipelines bot commented Sep 20, 2024

JamesNK commented Sep 4, 2024 •

edited by davidfowl

Loading

JamesNK Sep 11, 2024 •

edited

Loading

JamesNK commented Sep 18, 2024 •

edited

Loading

mitchdenny Sep 19, 2024 •

edited

Loading