Add WithVolumeMount Extension Method to Container Resource Class

[josephaw1022]

Pr for issue #837

TLDR
Scaffolded out an optional extension method for volume creation. This is made so that a developer can make a simple named volume but doesn't need to go and figure out the correct volume path.

Microsoft Reviewers: Open in CodeFlow

[stbau04]

I've also implemented this one monstly: #1317

In my PR draft the only thing that is missing is the automatically generated volume name
A random guid (as you did) is not working out in my opinion. It would cause the creation of a new volume on each start, at least while developing (i don't know any better approach yet, but i am not sure whether guids are the way to go)

Which PR should we keep? Mine is still a draft, but ready to review as soon as the automatically generated volume name is implemented properly. As this PR an mine are both using the same approach for that at the moment we either have two finished PRs or none at all

Edit: Questions regarding automatically generated volume names in issue: #837 (comment)

[josephaw1022]

@stbau04 I think just requiring it to be named would be the easiest approach for now then, if it's going to create a new volume on each reload or rebuild. But, what I am wondering is how if you just require the developer to add a volume name manually to a method, then how would we know that they didn't just use the same volume name?

var db1 = builder.AddPostgresContainer("db1")
                       .WithNamedVolume("db");

var db2 = builder.AddPostgresContainer("db2")
                       .WithNamedVolume("db");

Currently, if you ran this an error will occur but there's no specific exception throw for this.

Like we would have to have a way to store the "volume to resource" state so to speak

And, before the creation of the db2 container, it would need to check the current state of the volume naming, let's just say it's a dictionary<string, string> and some method would check the current state of this

{
"db1": "db"
}

and would see oh "db" is already a value for some key in the dictionary, we would throw an error and tell the user to the change their volume name directory.

I was thinking initially we might be able to just append a number to it and warn the developer to change the volume name, however, this would mean a whole suite of volumes would be created. And, we kind of just go back to our original problem.

So, I personally think a straightforward approach would to be (at a high level)

1. figure out how to store state in a way that can be accessed during the container creation lifecycle
2. have a resource with a volume have it's volume name checked (seeing if the name of volume trying to be created already exists (in the state, not from the docker command 'docker volume ls')

Ok so if what I am saying about volume state doesn't make too much sense. let me give a hand wavy / high level example

Example:

so, let's say this variable named volumeNamesToContainers and it represents our volume names to container name

/// under the hood aspire code 
///...
var volumeNamesToContainers = new Dictionary<string, string>();
/// ...

And, let's say we have our app host file that has three postgres databases.

var db1 = builder.AddPostgresContainer("db1-container")
                       .WithNamedVolume("db1");

var db2 = builder.AddPostgresContainer("db2-container")
                       .WithNamedVolume("db2");
                       
var db3 = builder.AddPostgresContainer("db3-container")
                       .WithNamedVolume("db1");

and just for simplicity sake, lets say db1 container is built first , then db2, and db3.

db1 container Creation

before the db1 container is created, we would check volumeNamesToContainers by seeing if it has a value

if (volumeNamesToContainers .TryGetValue("db1-container", out string value))
{
    throw some error
}
else {
volumeNamesToContainers["db1"] = "db1-container"
}

db2 container Creation

before the db2 container is created, we would check volumeNamesToContainers by seeing if it has a value

if (volumeNamesToContainers .TryGetValue("db2-container", out string value))
{
    throw some error
}
else {
volumeNamesToContainers["db2"] = "db2-container"
}

db3 container Creation

before the db3 container is created, we would check volumeNamesToContainers by seeing if it has a value

if (volumeNamesToContainers .TryGetValue("db3-container", out string value))
{
    throw some error // we would see an error thrown here obviously
}
else {
volumeNamesToContainers["db1"] = "db3-container"
}

and obviously we would see an error be thrown at this third step based off the volume name

🚧Yes, I know the key names and values will need to be dynamic, just manually wrote them to get point across 🚧

This is extremely high-level implementation and will look very different in the repo, but the approach to solving this is what I am getting at to solving the issue of volume duplication.

Didn't mean for this to be a book, but just wanted thought fully explaining it would speed up the implementation.

[josephaw1022]

@stbau04

Added a commit that should do what I was getting at here

aspire/src/Aspire.Hosting/Dcp/ApplicationExecutor.cs

Line 521 in 4b289a0

private static void ValidateNamedVolumes(Container? container)

[stbau04]

I think volume names can only be used once at a time? So if we do some error checking, in my opinion we should also verify that the same volume isn't used in different containers

We maybe could actually use the application name for naming the volumes i think, as the case of running the same aspire application multiple times would probably cause the same problems with manually named volumes.
I mean so that the name for the volume would e.g. be AssemblyName-ResourceName or something like that. Are there any drawbacks i've overseen? (ofc this is a problem if two assemblies have the same name, but i think this is just a minor problem?)

[josephaw1022]

@stbau04 the image above shows what happens when you use two of the same volume names (if the volume isn't created), the output for it when it's created shows nothing is wrong. If you run the volumes example from the aspire sample repo and add another sql server to it with the same volume target name, then you'll see that the two containers shouldn't start, but the logs for the sql servers dont say much besides an exit 0 code. If you then shut down the app host and restart it, they both db containers should then start up, however, obviously a bunch of errors start being thrown for the secondly created database once you try and do anything with it because it's db doesn't exist.

But with what you stated here, "So if we do some error checking, in my opinion we should also verify that the same volume isn't used in different containers" this is exactly what I was talking about and added commits for.

#1447 (comment)

And, I am not against having a way to use the WithNamedVolume method without having to manually add a volume name, but I think it's some potential challenges and that if just the manually named volume is added initially, then it would be at least something for now. And then another pr could then build on top of that. Because I am sure there a ton of different ways to solve that problem and certain trade offs to each. And, I just wouldn't want a "nice to have" (not needing to name the named volumes) hold the "low hanging fruit" (simply manually naming the named volumes) hostage from being merged.

[stbau04]

@josephaw1022

But with what you stated here, "So if we do some error checking, in my opinion we should also verify that the same volume isn't used in different containers" this is exactly what I was talking about and added commits for.

But the current version of the PR allows the following code without any exceptions:

var catalogDb = builder.AddPostgresContainer("postgres")
      .WithNamedVolume("VolumeMount.example.data");

// Add another Postgres container with the same named volume to see if named volumes name validation works correctly
builder.AddPostgresContainer("randomdb")
       .WithNamedVolume("VolumeMount.example.data");

In my opinion the same volume is mounted twice, so the exception should occure? But if i run the code everything is working without the exception being thrown
Further i would still put that into a seperate PR as it has nothing to do with the original issue?

[stbau04]

@josephaw1022
We still have two PRs. I created one a few days ago, that is basically ready to review (the only thing left to do would be to disable generated volume names), which is roughly ten minutes of work i think.

I think we should stick to my PR #1317 as it might scale better (a resource defines it's directories, not the dictinoary in the WithNamedVolume method). Further i added support for logs/init folders and both volumes/bind mounts for each of them.

The only thing my PR is missing would be the exception if the volume name is used twice, but in my opinion there are still bugs (#1447 (comment)) and i think it might be better of in a spereate PR as it is not that directly related to the issue

[stbau04]

As there were no further actions on this one i will update my draft and mark it as ready for review (#1317)

[mitchdenny]

I think this PR is fairly out of date and we've made some changes to the API surface here already so I am closing this PR. This conversation did influence our decision making process so it was very worthwhile.