Update health check to ensure blob containers created at right time

[RussKie]

Resolves #9139
Resolves #9145

The underlying issue is attributed to a lack of health checks for child resources (as those have no lifetime of their own). In the nutshell, whenever an Azurite emulator is starting up, the readiness of the emulator is indicated by the readiness of the "blobs" resources (represented by BloblServiceClient). Previously, child blob contrainers were created on ResourceReadyEvent, but this created an opportunity for a race condition - a client could attempt to connect after the resource reported healthy but before child resources were created - the flaky test highlighted this problem.

To fix the issue we're now creating an individual health check for each blob container resource. To make it simple, a blob container is being created within the health check itself.

[davidfowl]

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]

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]

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]

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]

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.

[RussKie]

Thanks @sebastienros for help and guidance. How does this look now?

[eerhardt]

Looks good.

[RussKie]

Suggested change

	// This event is triggered when the health check is healthy.
	// This event is triggered when the emulator has started, and BlobServiceClient is marked as healthy.

[sebastienros]

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.

[RussKie]

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.

[RussKie]

Why do we need to duplicate the HC here? Or why do we need to keep the HC on lines:160-167?

[sebastienros]

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 WaitForHealthyAsync doesn't bubble up to the parent resources.

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.

[radical]

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).

[sebastienros]

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?

[radical]

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.

[sebastienros]

#9364

[radical]

Thank you!