From 5a49b6f647598805badca1442d03bb9bad9e289c Mon Sep 17 00:00:00 2001 From: Mitch Denny Date: Tue, 22 Oct 2024 15:32:14 +1100 Subject: [PATCH 1/5] Make RedisInsight work with WithLifetime(...). --- .../RedisBuilderExtensions.cs | 74 ++++++++++++++++--- 1 file changed, 64 insertions(+), 10 deletions(-) diff --git a/src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs b/src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs index 8e5b90fcdeb..7dfd5681116 100644 --- a/src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs +++ b/src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs @@ -2,8 +2,10 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Globalization; +using System.Net.Http.Json; using System.Text; using System.Text.Json; +using System.Text.Json.Serialization; using Aspire.Hosting.ApplicationModel; using Aspire.Hosting.Redis; using Aspire.Hosting.Utils; @@ -192,20 +194,50 @@ public static IResourceBuilder WithRedisInsight(this IResourceBui return builder; } - static async Task ImportRedisDatabases(ILogger resourceLogger, IEnumerable redisInstances, HttpClient client, CancellationToken ct) + static async Task ImportRedisDatabases(ILogger resourceLogger, IEnumerable redisInstances, HttpClient client, CancellationToken cancellationToken) { + var databasesPath = "/api/databases"; + + var pipeline = new ResiliencePipelineBuilder().AddRetry(new Polly.Retry.RetryStrategyOptions + { + Delay = TimeSpan.FromSeconds(2), + MaxRetryAttempts = 5, + }).Build(); + using (var stream = new MemoryStream()) { + // As part of configuring RedisInsight we need to factor in the possibility that the + // container resource is being run with persistence turned on. In this case we need + // to get the list of existing databases because we might need to delete some. + var lookup = await pipeline.ExecuteAsync(async (ctx) => + { + var getDatabasesResponse = await client.GetFromJsonAsync(databasesPath, cancellationToken).ConfigureAwait(false); + return getDatabasesResponse?.ToLookup( + i => i.Name ?? throw new InvalidDataException("Database name is missing."), + i => i.Id ?? throw new InvalidDataException("Database ID is missing.")); + }, cancellationToken).ConfigureAwait(false); + + var databasesToDelete = new List(); + using var writer = new Utf8JsonWriter(stream); writer.WriteStartArray(); foreach (var redisResource in redisInstances) { + if (lookup is { } && lookup.Contains(redisResource.Name)) + { + // It is possible that there are multiple databases with + // a conflicting name so we delete them all. This just keeps + // track of the specific ID that we need to delete. + databasesToDelete.AddRange(lookup[redisResource.Name]); + } + if (redisResource.PrimaryEndpoint.IsAllocated) { var endpoint = redisResource.PrimaryEndpoint; writer.WriteStartObject(); + writer.WriteString("host", redisResource.Name); writer.WriteNumber("port", endpoint.TargetPort!.Value); writer.WriteString("name", redisResource.Name); @@ -218,7 +250,7 @@ static async Task ImportRedisDatabases(ILogger resourceLogger, IEnumerable + { + // Create a DELETE request to send to the existing instance of + // RedisInsight with the IDs of the database to delete. + var deleteContent = JsonContent.Create(new + { + ids = databasesToDelete + }); + + var deleteRequest = new HttpRequestMessage(HttpMethod.Delete, databasesPath) + { + Content = deleteContent + }; + + var deleteResponse = await client.SendAsync(deleteRequest, cancellationToken).ConfigureAwait(false); + deleteResponse.EnsureSuccessStatusCode(); + + }, cancellationToken).ConfigureAwait(false); + await pipeline.ExecuteAsync(async (ctx) => { var response = await client.PostAsync(apiUrl, content, ctx) .ConfigureAwait(false); response.EnsureSuccessStatusCode(); - }, ct).ConfigureAwait(false); + }, cancellationToken).ConfigureAwait(false); } catch (Exception ex) @@ -254,6 +299,15 @@ await pipeline.ExecuteAsync(async (ctx) => } } + private class RedisDatabaseDto + { + [JsonPropertyName("id")] + public Guid? Id { get; set; } + + [JsonPropertyName("name")] + public string? Name { get; set; } + } + /// /// Configures the host port that the Redis Commander resource is exposed on instead of using randomly assigned port. /// From 2fa2774c88e55c7a86cce70ce50ee8a7d7a2a6c0 Mon Sep 17 00:00:00 2001 From: Mitch Denny Date: Tue, 22 Oct 2024 21:05:55 +1100 Subject: [PATCH 2/5] Fix failing test. --- .../RedisBuilderExtensions.cs | 35 ++++++++++--------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs b/src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs index 7dfd5681116..80977138167 100644 --- a/src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs +++ b/src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs @@ -263,24 +263,27 @@ static async Task ImportRedisDatabases(ILogger resourceLogger, IEnumerable + if (databasesToDelete.Any()) { - // Create a DELETE request to send to the existing instance of - // RedisInsight with the IDs of the database to delete. - var deleteContent = JsonContent.Create(new - { - ids = databasesToDelete - }); - - var deleteRequest = new HttpRequestMessage(HttpMethod.Delete, databasesPath) + await pipeline.ExecuteAsync(async (ctx) => { - Content = deleteContent - }; - - var deleteResponse = await client.SendAsync(deleteRequest, cancellationToken).ConfigureAwait(false); - deleteResponse.EnsureSuccessStatusCode(); - - }, cancellationToken).ConfigureAwait(false); + // Create a DELETE request to send to the existing instance of + // RedisInsight with the IDs of the database to delete. + var deleteContent = JsonContent.Create(new + { + ids = databasesToDelete + }); + + var deleteRequest = new HttpRequestMessage(HttpMethod.Delete, databasesPath) + { + Content = deleteContent + }; + + var deleteResponse = await client.SendAsync(deleteRequest, cancellationToken).ConfigureAwait(false); + deleteResponse.EnsureSuccessStatusCode(); + + }, cancellationToken).ConfigureAwait(false); + } await pipeline.ExecuteAsync(async (ctx) => { From 03721bbbcb8f3cf284e2b8059c88449b154ebaf8 Mon Sep 17 00:00:00 2001 From: Mitch Denny Date: Wed, 23 Oct 2024 16:11:17 +1100 Subject: [PATCH 3/5] Test. --- .../RedisFunctionalTests.cs | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs b/tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs index ce9520d99c3..4f5d0be964e 100644 --- a/tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs +++ b/tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs @@ -120,6 +120,98 @@ public async Task VerifyRedisResource() Assert.Equal("value", value); } + [Fact] + [RequiresDocker] + public async Task VerifyDatabasesAreNotDuplicatedForPersistentRedisInsightContainer() + { + var randomResourceSuffix = Random.Shared.Next(10000).ToString(); + var cts = new CancellationTokenSource(TimeSpan.FromMinutes(5)); + + var configure = (DistributedApplicationOptions options) => + { + options.ContainerRegistryOverride = TestConstants.AspireTestContainerRegistry; + }; + + using var builder1 = TestDistributedApplicationBuilder.Create(configure, testOutputHelper); + builder1.Configuration[$"DcpPublisher:ResourceNameSuffix"] = randomResourceSuffix; + + IResourceBuilder? redisInsightBuilder = null; + var redis1 = builder1.AddRedis("redisForInsightPersistence") + .WithRedisInsight(c => + { + redisInsightBuilder = c; + c.WithLifetime(ContainerLifetime.Persistent); + }); + + // Wire up an additional event subcription to ResourceReadyEvent on the RedisInsightResource + // instance. This works because the ResourceReadyEvent fires non-blocking sequential so the + // wire-up that WithRedisInsight does is guaranteed to execute before this one does. So we then + // use this to block pulling the list of databases until we know they've been updated. This + // will repeated below for the second app. + Assert.NotNull(redisInsightBuilder); + var redisInsightsReady = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + builder1.Eventing.Subscribe(redisInsightBuilder.Resource, (evt, ct) => + { + redisInsightsReady.TrySetResult(); + return Task.CompletedTask; + }); + + using var app1 = builder1.Build(); + + await app1.StartAsync(cts.Token); + + await redisInsightsReady.Task.WaitAsync(cts.Token); + + using var client1 = app1.CreateHttpClient($"{redis1.Resource.Name}-insight", "http"); + var firstRunDatabases = await client1.GetFromJsonAsync("/api/databases", cts.Token); + + await app1.StopAsync(cts.Token); + + Assert.NotNull(firstRunDatabases); + Assert.Single(firstRunDatabases); + Assert.Equal($"{redis1.Resource.Name}", firstRunDatabases[0].Name); + + using var builder2 = TestDistributedApplicationBuilder.Create(configure, testOutputHelper); + builder2.Configuration[$"DcpPublisher:ResourceNameSuffix"] = randomResourceSuffix; + + var redis2 = builder2.AddRedis("redisForInsightPersistence") + .WithRedisInsight(c => + { + redisInsightBuilder = c; + c.WithLifetime(ContainerLifetime.Persistent); + }); + + // Wire up an additional event subcription to ResourceReadyEvent on the RedisInsightResource + // instance. This works because the ResourceReadyEvent fires non-blocking sequential so the + // wire-up that WithRedisInsight does is guaranteed to execute before this one does. So we then + // use this to block pulling the list of databases until we know they've been updated. This + // will repeated below for the second app. + Assert.NotNull(redisInsightBuilder); + redisInsightsReady = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + builder2.Eventing.Subscribe(redisInsightBuilder.Resource, (evt, ct) => + { + redisInsightsReady.TrySetResult(); + return Task.CompletedTask; + }); + + using var app2 = builder2.Build(); + await app2.StartAsync(cts.Token); + + await redisInsightsReady.Task.WaitAsync(cts.Token); + + using var client2 = app2.CreateHttpClient($"{redis2.Resource.Name}-insight", "http"); + var secondRunDatabases = await client2.GetFromJsonAsync("/api/databases", cts.Token); + + await app2.StopAsync(cts.Token); + + Assert.NotNull(secondRunDatabases); + Assert.Single(secondRunDatabases); + Assert.Equal($"{redis2.Resource.Name}", secondRunDatabases[0].Name); + Assert.NotEqual(secondRunDatabases.Single().Id, firstRunDatabases.Single().Id); + + // TODO: Make sure we don't leave the persistent container running around. + } + [Fact] [RequiresDocker] public async Task VerifyWithRedisInsightImportDatabases() From bd30469db50e030362011cbd768585fe9efafaed Mon Sep 17 00:00:00 2001 From: Mitch Denny Date: Wed, 23 Oct 2024 16:20:47 +1100 Subject: [PATCH 4/5] WIP tests --- tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs b/tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs index 4f5d0be964e..4cb40d0529e 100644 --- a/tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs +++ b/tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs @@ -165,12 +165,12 @@ public async Task VerifyDatabasesAreNotDuplicatedForPersistentRedisInsightContai using var client1 = app1.CreateHttpClient($"{redis1.Resource.Name}-insight", "http"); var firstRunDatabases = await client1.GetFromJsonAsync("/api/databases", cts.Token); - await app1.StopAsync(cts.Token); - Assert.NotNull(firstRunDatabases); Assert.Single(firstRunDatabases); Assert.Equal($"{redis1.Resource.Name}", firstRunDatabases[0].Name); + await app1.StopAsync(cts.Token); + using var builder2 = TestDistributedApplicationBuilder.Create(configure, testOutputHelper); builder2.Configuration[$"DcpPublisher:ResourceNameSuffix"] = randomResourceSuffix; @@ -202,14 +202,12 @@ public async Task VerifyDatabasesAreNotDuplicatedForPersistentRedisInsightContai using var client2 = app2.CreateHttpClient($"{redis2.Resource.Name}-insight", "http"); var secondRunDatabases = await client2.GetFromJsonAsync("/api/databases", cts.Token); - await app2.StopAsync(cts.Token); - Assert.NotNull(secondRunDatabases); Assert.Single(secondRunDatabases); Assert.Equal($"{redis2.Resource.Name}", secondRunDatabases[0].Name); Assert.NotEqual(secondRunDatabases.Single().Id, firstRunDatabases.Single().Id); - // TODO: Make sure we don't leave the persistent container running around. + await app2.StopAsync(cts.Token); } [Fact] From 508ec28678f18168cc8bb3d37963b196d30c393a Mon Sep 17 00:00:00 2001 From: Mitch Denny Date: Wed, 23 Oct 2024 21:11:28 +1100 Subject: [PATCH 5/5] Work around for stopping resource. --- .../RedisFunctionalTests.cs | 18 ++++++++++++++++- .../Dcp/ApplicationExecutorProxy.cs | 20 +++++++++++++++++++ .../TestDistributedApplicationBuilder.cs | 4 ++++ 3 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 tests/Aspire.Hosting.Tests/Dcp/ApplicationExecutorProxy.cs diff --git a/tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs b/tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs index 4cb40d0529e..ff6e91d44cc 100644 --- a/tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs +++ b/tests/Aspire.Hosting.Redis.Tests/RedisFunctionalTests.cs @@ -15,6 +15,7 @@ using StackExchange.Redis; using Xunit; using Xunit.Abstractions; +using Aspire.Hosting.Tests.Dcp; namespace Aspire.Hosting.Redis.Tests; @@ -148,6 +149,8 @@ public async Task VerifyDatabasesAreNotDuplicatedForPersistentRedisInsightContai // wire-up that WithRedisInsight does is guaranteed to execute before this one does. So we then // use this to block pulling the list of databases until we know they've been updated. This // will repeated below for the second app. + // + // Issue: https://github.com/dotnet/aspire/issues/6455 Assert.NotNull(redisInsightBuilder); var redisInsightsReady = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); builder1.Eventing.Subscribe(redisInsightBuilder.Resource, (evt, ct) => @@ -199,7 +202,7 @@ public async Task VerifyDatabasesAreNotDuplicatedForPersistentRedisInsightContai await redisInsightsReady.Task.WaitAsync(cts.Token); - using var client2 = app2.CreateHttpClient($"{redis2.Resource.Name}-insight", "http"); + using var client2 = app2.CreateHttpClient($"{redisInsightBuilder.Resource.Name}", "http"); var secondRunDatabases = await client2.GetFromJsonAsync("/api/databases", cts.Token); Assert.NotNull(secondRunDatabases); @@ -207,6 +210,19 @@ public async Task VerifyDatabasesAreNotDuplicatedForPersistentRedisInsightContai Assert.Equal($"{redis2.Resource.Name}", secondRunDatabases[0].Name); Assert.NotEqual(secondRunDatabases.Single().Id, firstRunDatabases.Single().Id); + // HACK: This is a workaround for the fact that ApplicationExecutor is not a public type. What I have + // done here is I get the latest event from RNS for the insights instance which gives me the resource + // name as known from a DCP perspective. I then use the ApplicationExecutorProxy (introduced with this + // change to call the ApplicationExecutor stop method. The proxy is a public type with an internal + // constructor inside the Aspire.Hosting.Tests package. This is a short term solution for 9.0 to + // make sure that we have good test coverage for WithRedisInsight behavior, but we need a better + // long term solution in 9.x for folks that will want to do things like execute commands against + // resources to stop specific containers. + var rns = app2.Services.GetRequiredService(); + var latestEvent = await rns.WaitForResourceHealthyAsync(redisInsightBuilder.Resource.Name, cts.Token); + var executorProxy = app2.Services.GetRequiredService(); + await executorProxy.StopResourceAsync(latestEvent.ResourceId, cts.Token); + await app2.StopAsync(cts.Token); } diff --git a/tests/Aspire.Hosting.Tests/Dcp/ApplicationExecutorProxy.cs b/tests/Aspire.Hosting.Tests/Dcp/ApplicationExecutorProxy.cs new file mode 100644 index 00000000000..543ae16d3dc --- /dev/null +++ b/tests/Aspire.Hosting.Tests/Dcp/ApplicationExecutorProxy.cs @@ -0,0 +1,20 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Aspire.Hosting.Dcp; + +namespace Aspire.Hosting.Tests.Dcp; + +public class ApplicationExecutorProxy +{ + internal ApplicationExecutorProxy(ApplicationExecutor executor) + { + _executor = executor; + } + + private readonly ApplicationExecutor _executor; + + public Task StartResourceAsync(string resourceName, CancellationToken cancellationToken) => _executor.StartResourceAsync(resourceName, cancellationToken); + + public Task StopResourceAsync(string resourceName, CancellationToken cancellationToken) => _executor.StopResourceAsync(resourceName, cancellationToken); +} diff --git a/tests/Aspire.Hosting.Tests/Utils/TestDistributedApplicationBuilder.cs b/tests/Aspire.Hosting.Tests/Utils/TestDistributedApplicationBuilder.cs index c04a4ddb44f..aed5c6891f8 100644 --- a/tests/Aspire.Hosting.Tests/Utils/TestDistributedApplicationBuilder.cs +++ b/tests/Aspire.Hosting.Tests/Utils/TestDistributedApplicationBuilder.cs @@ -6,8 +6,10 @@ using System.Reflection; using Aspire.Components.Common.Tests; using Aspire.Hosting.Dashboard; +using Aspire.Hosting.Dcp; using Aspire.Hosting.Eventing; using Aspire.Hosting.Testing; +using Aspire.Hosting.Tests.Dcp; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; @@ -76,6 +78,8 @@ private TestDistributedApplicationBuilder(Action? o.OtlpGrpcEndpointUrl ??= "http://localhost:4317"; }); + _innerBuilder.Services.AddSingleton(sp => new ApplicationExecutorProxy(sp.GetRequiredService())); + _innerBuilder.Services.AddHttpClient(); _innerBuilder.Services.ConfigureHttpClientDefaults(http => http.AddStandardResilienceHandler()); if (testOutputHelper is not null)