Add WithVolumeMountForData

[stbau04]

Make it easier to use external volumes for database containers:

• Added WithVolumeFor.../WithBindMountFor... for data, logs and init folders
  • For the data directory on all containers
  • For the logs directory on containers with logs enabled by default (as it might confuse if the WithVolumeForLogs is available but log files are not actually enabled in the container configuration)
  • For the init folder where a /docker-entrypoint-initdb.d folder is avilable (did not find something similar on the other containers)

Fixes #837

Microsoft Reviewers: Open in CodeFlow

[DamianEdwards]

Thanks for this. We should add overloads of this method that support the other database container resources too, with the appropriate path in those cases.

[stbau04]

Before adding the others i would wait for the discussion on the issue (#837 (comment)). If this is still the approch we stick too i will continue adding the overloads.

[stbau04]

@josephaw1022 created a second (duplicate) PR for this one: #1447

[stbau04]

Probably we should wait for #1521 and then see how it affects this pr (especially the volume/bind mount stuff)

[davidfowl]

@stbau04 Would you like to resurrect this PR?

[davidfowl]

@DamianEdwards wdyt?

[DamianEdwards]

@davidfowl yeah I like this a lot actually. @stbau04 if you're around it would be great if you could update this for changes since Dec (mainly the split out of WithBindMount and WithVolume from WithVolumeMount), otherwise I'm happy to take this over for preview.5

[stbau04]

@davidfowl @DamianEdwards Yes sure, i'll update it

[davidfowl]

I'm gonna assign this to you @stbau04. Thanks!

[stbau04]

There are the following container resources that might support some of the methods (the checked ones are implemented):
RabbitMQServerResource ✅
MySqlServerResource ✅
PostgresServerResource ✅
MongoDBServerResource ✅
RedisResource ✅
SqlServerServerResource ✅
KafkaServerResource
MongoExpressContainerResource
PhpMyAdminContainerResource
OracleDatabaseServerResource
PgAdminContainerResource
RedisCommandResource
AzureCosmosDbEmulatorResource
AzureStorageEmulatorResource

[DamianEdwards]

Seems you did a merge instead of a rebase so this PR is showing up as including all changes on main since you originally created it! Can you go back to before you merged and do a rebase on main instead, and then force push to your fork/branch?

[davidfowl]

@DamianEdwards @mitchdenny Do we want to align here with the UsePersistance calls

aspire/src/Aspire.Hosting.Azure/Extensions/AzureStorageExtensions.cs

Lines 123 to 128 in 1d868e8

	public static IResourceBuilder<AzureStorageEmulatorResource> UsePersistence(this IResourceBuilder<AzureStorageEmulatorResource> builder, string? path = null)
	{
	path = path ?? $".azurite/{builder.Resource.Name}";
	var fullyQualifiedPath = Path.GetFullPath(path, builder.ApplicationBuilder.AppHostDirectory);
	return builder.WithBindMount(fullyQualifiedPath, "/data", isReadOnly: false);

?

Feels like we should have a consistent pattern. The above picks a default name as well.

[mitchdenny]

I'm not sure I like the interfaces in this PR. It seems to me that persistence for each container is a fairly container specific thing and we might want to have a specific extension method for each container. Perhaps in that case the overload takes a basePath and then if the container supports init/log/data directories then a bindmount is applied for each one relative to the base path.

[mitchdenny]

Thinking about this some more, I think what we want here is a per container type extension that takes into consideration the various quirks assocaited with each container. So instead of a pattern that would seem people doing this:

builder.AddRedis("redis").WithBindMountForLogs(path).WithBindMountForInit(path).WithBindMountForData(path)

We could just do this:

builder.AddRedis("redis").WithPersistence(path);

The benefit is that it is a simpler gesture for the consumer of the container and it allows us to abstract away wierdness that some containers might have. For example, the Cosmos container which we use for the emulator doesn't seem to work with bind mounts -- so this mechanism wouldn't work. Instead for that we'd need to implement a WithPersistence(...) method which computes a unique volume name and use that instead of a bind mount.

So in summary ... its good to make it easy for people to turn on persistence for a given container (we should do that!) but lets not make it generic when it probably isn't because we have to live with this abstraction for a while ;)

[DamianEdwards]

I think we need more than a single method if for no other reason than to support bind mounts vs. volumes. Perhaps it's just two overloads (one accepting a path and another not) but I think that's less clear than the proposed methods. Personally not a fan of the WithPersistance naming generally.

[DamianEdwards]

Perhaps a simple change to WithDataVolume and WithDataBindMount, etc. for these. The interface approach seems fine for now I think? What kind of thing would trip that approach up?

[DamianEdwards]

To address @mitchdenny's concerns here I think a good compromise would be to get rid of the interfaces and just establish a pattern for these kind of container-based resources by adding methods like WithDataVolume and WithDataBindMount where they support it. If a particular container image has quirks beyond just needing to know the path to mount the volume or bind mount to, it can be handled in that method.

For init and logs, I think it only makes sense to have support for bind mounts for init, and both volume and bind mounts for logs. Reasoning being that init is all about an input to the container (run these scripts) whereas data and logs are potentially input and output.

In the case of volumes, we should support auto-generating a name based on the resource name or specifying a name.

So we'd end up with methods like the following on the relevant container-based resources:

• WithDataVolume(string? name = null)
• WithLogsVolume(string? name = null)
• WithDataBindMount(string source)
• WithLogsBindMount(string source)
• WithInitBindMount(string source)

Open question is whether we should support auto-generating a source location for the bind mount methods (like the azurite one does). I'm not particularly fussed on that, could take it or leave it. Perhaps we leave it for now and can always add it later.

[DamianEdwards]

@stbau04 are you able to update your approach here to what I described above?

[stbau04]

@DamianEdwards Yes sure

[stbau04]

Is that the desired approach we should follow?

[DamianEdwards]

Is that the desired approach we should follow?

That is what I'm currently proposing, Would be good to explicitly hear from @mitchdenny if he has any concerns with that approach.

[stbau04]

@DamianEdwards @mitchdenny I've applied the proposed approach to the first few containers. If you are fine with this solution i would continue implementing it for the remaining resources. Do you have any imput for me?

Especially what about the naming for the volumes, is the default name ok like that? If we keep it as it is, you cannot run a project twice on the same host - or have resources with the same name in different projects - if you want to use the auto-generated name. Are we fine with that?

[DamianEdwards]

@stbau04 thanks for the updates! Were there more resource types you were planning to apply this pattern to?

[davidfowl]

I would leave the azure resources as they are currently being renovated by @mitchdenny. @mitchdenny you can take this pattern to the existing emulators.

[DamianEdwards]

@stbau04 I think this is ready to review now?

[stbau04]

@DamianEdwards I could maybe add some methods for the following: KafkaServerResource, MongoExpressContainerResource, PhpMyAdminContainerResource, OracleDatabaseServerResource, PgAdminContainerResource and RedisCommandResource (might take me some time to figure out the correct paths)
If this is not necessary/wantend at the moment it would be ready to review

[davidfowl]

KafkaServerResource - 👍🏾
OracleDatabaseServerResource - 👍🏾

The rest I would skip.

[stbau04]

@DamianEdwards I've added some method for the KafkaServer and OracleDatabaseServer resoures. I think it should be ready for review now

[davidfowl]

@mitchdenny We need to change cosmos and azure storage to use the same pattern yes? WithVolumeForData?

[davidfowl]

Seq differs a little here:

aspire/src/Aspire.Hosting/Seq/SeqBuilderExtensions.cs

Line 27 in c236e90

string? seqDataDirectory = null)

We should make this follow the same pattern I think.

[davidfowl]

It can be a follow up though

[davidfowl]

Thanks @stbau04 !!!!