-
Notifications
You must be signed in to change notification settings - Fork 931
Update health check to ensure blob containers created at right time #9159
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
Changes from 15 commits
4ba607c
ec3a2df
6aa390f
e993433
eddebef
5bd0c61
48e2d5d
9737260
80ce587
4ebf0d7
a72f514
c21f230
53fac90
3242c5c
ba67f13
273233b
30641bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -95,6 +95,7 @@ public static IResourceBuilder<AzureStorageResource> AddAzureStorage(this IDistr | |||||
| }; | ||||||
|
|
||||||
| var resource = new AzureStorageResource(name, configureInfrastructure); | ||||||
|
|
||||||
| return builder.AddResource(resource) | ||||||
| .WithDefaultRoleAssignments(StorageBuiltInRole.GetBuiltInRoleName, | ||||||
| StorageBuiltInRole.StorageBlobDataContributor, | ||||||
|
|
@@ -131,34 +132,28 @@ public static IResourceBuilder<AzureStorageResource> RunAsEmulator(this IResourc | |||||
| }); | ||||||
|
|
||||||
| BlobServiceClient? blobServiceClient = null; | ||||||
|
|
||||||
| builder.ApplicationBuilder.Eventing.Subscribe<BeforeResourceStartedEvent>(builder.Resource, async (@event, ct) => | ||||||
| { | ||||||
| var connectionString = await builder.Resource.GetBlobConnectionString().GetValueAsync(ct).ConfigureAwait(false); | ||||||
| if (connectionString is null) | ||||||
| { | ||||||
| throw new DistributedApplicationException($"BeforeResourceStartedEvent was published for the '{builder.Resource.Name}' resource but the connection string was null."); | ||||||
| } | ||||||
| // The BlobServiceClient is created before the health check is run. | ||||||
| // We can't use ConnectionStringAvailableEvent here because the resource doesn't have a connection string, so | ||||||
| // we use BeforeResourceStartedEvent | ||||||
|
|
||||||
| var connectionString = await builder.Resource.GetEmulatorConnectionString().GetValueAsync(ct).ConfigureAwait(false) ?? throw new DistributedApplicationException($"{nameof(ConnectionStringAvailableEvent)} was published for the '{builder.Resource.Name}' resource but the connection string was null."); | ||||||
| blobServiceClient = CreateBlobServiceClient(connectionString); | ||||||
| }); | ||||||
|
|
||||||
| builder.ApplicationBuilder.Eventing.Subscribe<ResourceReadyEvent>(builder.Resource, async (@event, ct) => | ||||||
| { | ||||||
| if (blobServiceClient is null) | ||||||
| { | ||||||
| throw new DistributedApplicationException($"BlobServiceClient was not created for the '{builder.Resource.Name}' resource."); | ||||||
| } | ||||||
| // This event is triggered when the health check is healthy. | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I disagree with the change, that's not what I wanted to convey. I am saying that this event happens after the health check, only if it's healthy. Yes it implied the emulator has started (with more information than just "started") and "BlobServiceClient" is healthy doesn't mean much. The storage itself is healthy, the client is not "marked" anything.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough, but "health check is healthy" isn't providing much information either. There are multiple health checks now; it would be good to clarify which health check is triggering this event. |
||||||
|
|
||||||
| var connectionString = await builder.Resource.GetBlobConnectionString().GetValueAsync(ct).ConfigureAwait(false); | ||||||
| if (connectionString is null) | ||||||
| if (blobServiceClient is null) | ||||||
| { | ||||||
| throw new DistributedApplicationException($"ResourceReadyEvent was published for the '{builder.Resource.Name}' resource but the connection string was null."); | ||||||
| throw new InvalidOperationException("BlobServiceClient is not initialized."); | ||||||
| } | ||||||
|
|
||||||
| foreach (var blobContainer in builder.Resource.BlobContainers) | ||||||
| foreach (var container in builder.Resource.BlobContainers) | ||||||
| { | ||||||
| await blobServiceClient.GetBlobContainerClient(blobContainer.BlobContainerName).CreateIfNotExistsAsync(cancellationToken: ct).ConfigureAwait(false); | ||||||
| await blobServiceClient.GetBlobContainerClient(container.BlobContainerName).CreateIfNotExistsAsync(cancellationToken: ct).WaitAsync(ct).ConfigureAwait(false); | ||||||
|
sebastienros marked this conversation as resolved.
Outdated
|
||||||
| } | ||||||
| }); | ||||||
|
|
||||||
|
|
@@ -182,18 +177,6 @@ public static IResourceBuilder<AzureStorageResource> RunAsEmulator(this IResourc | |||||
| configureContainer?.Invoke(surrogateBuilder); | ||||||
|
|
||||||
| return builder; | ||||||
|
|
||||||
| static BlobServiceClient CreateBlobServiceClient(string connectionString) | ||||||
| { | ||||||
| if (Uri.TryCreate(connectionString, UriKind.Absolute, out var uri)) | ||||||
| { | ||||||
| return new BlobServiceClient(uri, new DefaultAzureCredential()); | ||||||
| } | ||||||
| else | ||||||
| { | ||||||
| return new BlobServiceClient(connectionString); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// <summary> | ||||||
|
|
@@ -308,7 +291,22 @@ public static IResourceBuilder<AzureBlobStorageResource> AddBlobs(this IResource | |||||
| ArgumentException.ThrowIfNullOrEmpty(name); | ||||||
|
|
||||||
| var resource = new AzureBlobStorageResource(name, builder.Resource); | ||||||
| return builder.ApplicationBuilder.AddResource(resource); | ||||||
|
|
||||||
| string? connectionString = null; | ||||||
| builder.ApplicationBuilder.Eventing.Subscribe<ConnectionStringAvailableEvent>(resource, async (@event, ct) => | ||||||
| { | ||||||
| connectionString = await resource.ConnectionStringExpression.GetValueAsync(ct).ConfigureAwait(false); | ||||||
| }); | ||||||
|
|
||||||
| var healthCheckKey = $"{resource.Name}_check"; | ||||||
|
|
||||||
| BlobServiceClient? blobServiceClient = null; | ||||||
| builder.ApplicationBuilder.Services.AddHealthChecks().AddAzureBlobStorage(sp => | ||||||
| { | ||||||
| return blobServiceClient ??= CreateBlobServiceClient(connectionString ?? throw new InvalidOperationException("Connection string is not initialized.")); | ||||||
| }, name: healthCheckKey); | ||||||
|
Comment on lines
+303
to
+309
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to duplicate the HC here? Or why do we need to keep the HC on lines:160-167?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here it's on the Blobs resource it. Line 160 is on the Emulator resource. Doing it on the storage is not sufficient as the If it were just for the existing tests we could probably not have this specific one. But it's more consistent to keep it if we do it for containers. |
||||||
|
|
||||||
| return builder.ApplicationBuilder.AddResource(resource).WithHealthCheck(healthCheckKey); | ||||||
| } | ||||||
|
|
||||||
| /// <summary> | ||||||
|
|
@@ -326,10 +324,26 @@ public static IResourceBuilder<AzureBlobStorageContainerResource> AddBlobContain | |||||
| blobContainerName ??= name; | ||||||
|
|
||||||
| AzureBlobStorageContainerResource resource = new(name, blobContainerName, builder.Resource); | ||||||
|
|
||||||
| builder.Resource.Parent.BlobContainers.Add(resource); | ||||||
|
|
||||||
| return builder.ApplicationBuilder.AddResource(resource); | ||||||
| string? connectionString = null; | ||||||
| builder.ApplicationBuilder.Eventing.Subscribe<ConnectionStringAvailableEvent>(resource, async (@event, ct) => | ||||||
| { | ||||||
| connectionString = await resource.Parent.ConnectionStringExpression.GetValueAsync(ct).ConfigureAwait(false); | ||||||
| }); | ||||||
|
|
||||||
| var healthCheckKey = $"{resource.Name}_check"; | ||||||
|
|
||||||
| BlobServiceClient? blobServiceClient = null; | ||||||
| builder.ApplicationBuilder.Services.AddHealthChecks().AddAzureBlobStorage(sp => | ||||||
| { | ||||||
| return blobServiceClient ??= CreateBlobServiceClient(connectionString ?? throw new InvalidOperationException("Connection string is not initialized.")); | ||||||
| }, | ||||||
| optionsFactory: sp => new HealthChecks.Azure.Storage.Blobs.AzureBlobStorageHealthCheckOptions { ContainerName = blobContainerName }, | ||||||
|
sebastienros marked this conversation as resolved.
Outdated
|
||||||
| name: healthCheckKey); | ||||||
|
|
||||||
| return builder.ApplicationBuilder | ||||||
| .AddResource(resource).WithHealthCheck(healthCheckKey); | ||||||
| } | ||||||
|
|
||||||
| /// <summary> | ||||||
|
|
@@ -362,6 +376,18 @@ public static IResourceBuilder<AzureQueueStorageResource> AddQueues(this IResour | |||||
| return builder.ApplicationBuilder.AddResource(resource); | ||||||
| } | ||||||
|
|
||||||
| private static BlobServiceClient CreateBlobServiceClient(string connectionString) | ||||||
| { | ||||||
| if (Uri.TryCreate(connectionString, UriKind.Absolute, out var uri)) | ||||||
| { | ||||||
| return new BlobServiceClient(uri, new DefaultAzureCredential()); | ||||||
| } | ||||||
| else | ||||||
| { | ||||||
| return new BlobServiceClient(connectionString); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Assigns the specified roles to the given resource, granting it the necessary permissions | ||||||
| /// on the target Azure Storage account. This replaces the default role assignments for the resource. | ||||||
|
|
||||||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,9 +135,10 @@ public async Task VerifyAzureStorageEmulatorResource() | |
|
|
||
| [Fact] | ||
| [RequiresDocker] | ||
| [QuarantinedTest("https://github.com/dotnet/aspire/issues/9139")] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure that this can be dropped? For quarantined tests we want to take it out after it has been green for a certain number of runs (~100 right now).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will add it back then. What else? Reopening the issues? Is tracking automatic or is there a process to follow to unquarantine like for aspnet?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, re-open the issue. And it will be tracked automatically. And I will take care of taking it out of quarantine for now. It will get semi-automated in medium term.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you! |
||
| public async Task VerifyAzureStorageEmulator_blobcontainer_auto_created() | ||
| { | ||
| var cts = new CancellationTokenSource(TimeSpan.FromMinutes(3)); | ||
|
|
||
| using var builder = TestDistributedApplicationBuilder.Create().WithTestAndResourceLogging(testOutputHelper); | ||
| var storage = builder.AddAzureStorage("storage").RunAsEmulator(); | ||
| var blobs = storage.AddBlobs("BlobConnection"); | ||
|
|
@@ -146,16 +147,16 @@ public async Task VerifyAzureStorageEmulator_blobcontainer_auto_created() | |
| using var app = builder.Build(); | ||
| await app.StartAsync(); | ||
|
|
||
| var rns = app.Services.GetRequiredService<ResourceNotificationService>(); | ||
| await rns.WaitForResourceHealthyAsync(blobContainer.Resource.Name, cancellationToken: cts.Token); | ||
|
|
||
| var hb = Host.CreateApplicationBuilder(); | ||
| hb.Configuration["ConnectionStrings:BlobConnection"] = await blobs.Resource.ConnectionStringExpression.GetValueAsync(CancellationToken.None); | ||
| hb.AddAzureBlobClient("BlobConnection"); | ||
|
|
||
| using var host = hb.Build(); | ||
| await host.StartAsync(); | ||
|
|
||
| var rns = app.Services.GetRequiredService<ResourceNotificationService>(); | ||
| await rns.WaitForResourceHealthyAsync(blobContainer.Resource.Name, CancellationToken.None); | ||
|
|
||
| var serviceClient = host.Services.GetRequiredService<BlobServiceClient>(); | ||
| var blobContainerClient = serviceClient.GetBlobContainerClient("testblobcontainer"); | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.