Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update health check to ensure blob containers created at right time #9159

wants to merge 1 commit into from
+8 −18

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented May 8, 2025 •
edited
Loading

Resolves #9139
Resolves #9145

I removed ResourceReadyEvent handler, and moved blob container creation logic into the BlobStorage health check. This way, once we connected to the storage emulator and have the service client, we can ensure the requested blob containers are created.

Kudos to @mitchdenny for giving me some ideas.

@RussKie RussKie self-assigned this May 8, 2025
@github-actions github-actions bot added the area-integrations Issues pertaining to Aspire Integrations packages label May 8, 2025
@davidfowl
Copy link
Member

Hmm, we had this debate when @sebastienros did the database creation and I thought we decided to use the ResourceReady event (which runs after health checks).

@eerhardt
Copy link
Member

eerhardt commented May 8, 2025

Hmm, we had this debate when @sebastienros did the database creation and I thought we decided to use the ResourceReady event (which runs after health checks).

For databases, do we:

aspire/src/Aspire.Hosting.SqlServer/SqlServerBuilderExtensions.cs

Lines 58 to 77 in 88ff939

builder.Eventing.Subscribe<ResourceReadyEvent>(sqlServer, async (@event, ct) =>
{
if (connectionString is null)
{
throw new DistributedApplicationException($"ResourceReadyEvent was published for the '{sqlServer.Name}' resource but the connection string was null.");
}
using var sqlConnection = new SqlConnection(connectionString);
await sqlConnection.OpenAsync(ct).ConfigureAwait(false);
if (sqlConnection.State != System.Data.ConnectionState.Open)
{
throw new InvalidOperationException($"Could not open connection to '{sqlServer.Name}'");
}
foreach (var sqlDatabase in sqlServer.DatabaseResources)
{
await CreateDatabaseAsync(sqlConnection, sqlDatabase, @event.Services, ct).ConfigureAwait(false);
}
});

aspire/src/Aspire.Hosting.PostgreSQL/PostgresBuilderExtensions.cs

Lines 66 to 90 in 88ff939

builder.Eventing.Subscribe<ResourceReadyEvent>(postgresServer, async (@event, ct) =>
{
if (connectionString is null)
{
throw new DistributedApplicationException($"ResourceReadyEvent was published for the '{postgresServer.Name}' resource but the connection string was null.");
}
// Non-database scoped connection string
using var npgsqlConnection = new NpgsqlConnection(connectionString + ";Database=postgres;");
await npgsqlConnection.OpenAsync(ct).ConfigureAwait(false);
if (npgsqlConnection.State != System.Data.ConnectionState.Open)
{
throw new InvalidOperationException($"Could not open connection to '{postgresServer.Name}'");
}
foreach (var name in postgresServer.Databases.Keys)
{
if (builder.Resources.FirstOrDefault(n => string.Equals(n.Name, name, StringComparisons.ResourceName)) is PostgresDatabaseResource postgreDatabase)
{
await CreateDatabaseAsync(npgsqlConnection, postgreDatabase, @event.Services, ct).ConfigureAwait(false);
}
}
});

In general, I dislike using health checks to mutate state. They should just be used to check health, not make something "healthy".

@RussKie
Copy link
Member Author

RussKie commented May 8, 2025

I am not super fond of this result, but I couldn't find a way to add a healthcheck for individual blob containers.
I originally added blob container creation within ResourceReadyEvent, however, it appears this makes the test flaky - there's a race condition, and the client may attempt to access blob container before those get created.
If I add a check alongside the blob storage, then ResourceReadyEvent never get fired (since no containers yet exist). I couldn't add a healthcheck within the even - the service collection at this point is already locked.

Any suggestions?

@eerhardt
Copy link
Member

eerhardt commented May 9, 2025

but I couldn't find a way to add a healthcheck for individual blob containers.

I don't think we do healthchecks for child resources anywhere else. For example, CosmosDB seems to honly have it for the whole service.

aspire/src/Aspire.Hosting.Azure.CosmosDB/AzureCosmosDBExtensions.cs

Lines 104 to 122 in e3d170c

builder.ApplicationBuilder.Eventing.Subscribe<ResourceReadyEvent>(builder.Resource, async (@event, ct) =>
{
if (cosmosClient is null)
{
throw new InvalidOperationException("CosmosClient is not initialized.");
}
await cosmosClient.ReadAccountAsync().WaitAsync(ct).ConfigureAwait(false);
foreach (var database in builder.Resource.Databases)
{
var db = (await cosmosClient.CreateDatabaseIfNotExistsAsync(database.DatabaseName, cancellationToken: ct).ConfigureAwait(false)).Database;
foreach (var container in database.Containers)
{
await db.CreateContainerIfNotExistsAsync(container.ContainerName, container.PartitionKeyPath, cancellationToken: ct).ConfigureAwait(false);
}
}
});

Because the ResourceReadyEvent blocks the resource's "healthy" state until all ResourceReadyEvent listeners complete, the parent resource won't be marked "healthy" until creating the child resources is complete. And the child resources won't be "healthy" until the parent resource is "healthy".

I think we should be able to follow the existing patterns in Sql, Postgres, and in Azure CosmosDB. What doesn't work about the existing pattern?

Any other suggestions here @sebastienros or @mitchdenny ?

@sebastienros
Copy link
Member

I don't think we do healthchecks for child resources anywhere else

SqlServer/Postgres databases have one. It's done by using their own connection string which has the Database= property in it so establishing the connection retrieved from ConnectionStringAvailableEvent is sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfowl davidfowl Awaiting requested review from davidfowl

@mitchdenny mitchdenny Awaiting requested review from mitchdenny

@eerhardt eerhardt Awaiting requested review from eerhardt

At least 1 approving review is required to merge this pull request.

Assignees

@RussKie RussKie

Labels
area-integrations Issues pertaining to Aspire Integrations packages
Projects
None yet
Milestone
No milestone
4 participants
@RussKie @davidfowl @eerhardt @sebastienros