Add WithBindMount method

[JamesNK]

Fixes #1437

The WithVolumeMount method today confuses two container concepts:

• Volumes mount - https://docs.docker.com/storage/volumes/
• Bind mounts - https://docs.docker.com/storage/bind-mounts/

I noticed that DCP does the same thing. I'm not sure who influenced who, but while it doesn't matter to a user's experience what things are called in DCP data, the Aspire.Hosting model should use the right terminology.

Changes:

• Add WithBindMount
• Remove mount type argument from WithVolumeMount. Now you must either use WithVolumeMount or WithBindMount to get the mount you want. IMPORTANT: The mount type on WithVolumeMount previously defaulted to binding mount. IMO this was confusing. However, with the change to split the method in two, WithVolumeMount now always creates a volume mount. This is a breaking change.
  • Before: WithVolumeMount(@"c:\data", @"/etc/data") creates a binding mount with c:\data as source path
  • After: WithVolumeMount(@"c:\data", @"/etc/data") creates a volume mount with the name c:\data.
• Changed WithVolumeMount's source parameter to be nullable. A volume can have a name or be anonymous. I haven't tested that DCP is cool with anonymous volumes yet. @karolz-ms Do you know?
• Renamed various types from VolumeMountXXX to ContainerMountXXX.

New API:

public static IResourceBuilder<T> WithVolumeMount<T>(
    this IResourceBuilder<T> builder, string? source, string target, bool isReadOnly = false) where T : ContainerResource

public static IResourceBuilder<T> WithBindMount<T>(
    this IResourceBuilder<T> builder, string source, string target, bool isReadOnly = false) where T : ContainerResource

There are experiments with making volumes a first-class concept: #1521. If this is implemented, then I expect WithVolumeMount will have an overload that takes the volume as source instead of a string:

public static IResourceBuilder<T> WithVolumeMount<T>(
    this IResourceBuilder<T> builder, Volume source, string target, bool isReadOnly = false) where T : ContainerResource

Microsoft Reviewers: Open in CodeFlow

[JamesNK]

What was the intended volume mount type here? The previous WithVolumeMount usage would have created a bind mount (unintentionally?), but it looks like what was desired was a volume mount with a random name.

Can someone who knows this code confirm?

[JamesNK]

cc @timheuer @mitchdenny

[karolz-ms]

IMO in this, and the other case, a volume mount makes more sense, both for "I do not care about the data being deleted at the end of the run" scenario, as well as for "I want the data to be preserved" scenario.

[JamesNK]

If there isn't a decision made either way by someone who knows this code before this PR is ready to merge, then I'll create an issue to look into it.

[mitchdenny]

So in the code highlighted the file that is there is produced by the app model and injected into the container, so i think that bind mount is what is preferred - although it is an internal implementation detail.

[JamesNK]

Same as previous comment. What mount type should this be?

[mitchdenny]

This should be a bind mount as well.

[JamesNK]

cc @stbau04 @josephaw1022

[karolz-ms]

The DCP unification of bind mounts and volume mounts is loosely based on the syntax of the --mount option for docker run command https://docs.docker.com/engine/reference/commandline/container_run/#mount

I do not think we support anonymous volume mounts today, but that could be added if desired

[karolz-ms]

LGTM. I like how it makes the API clearer.

[karolz-ms]

IMO in this, and the other case, a volume mount makes more sense, both for "I do not care about the data being deleted at the end of the run" scenario, as well as for "I want the data to be preserved" scenario.

[JamesNK]

The DCP unification of bind mounts and volume mounts is loosely based on the syntax of the --mount option for docker run command docs.docker.com/engine/reference/commandline/container_run/#mount

Ok. IMO it's confusing that there is a volume mount of bind. Volume vs bind are exclusive, at least in Docker.

But it's not a big deal in DCP's data model if volumeMount has a weird name (just mount would be less confusing) because no one will see it.

I do not think we support anonymous volume mounts today, but that could be added if desired

Ok, I'll remove nullable option from volume mount source. If someone requests anonymous volumes in the future, then we can add support then.

[mitchdenny]

I think separating the concepts is the right thing to do. In the future we want to add volume mount support for resources when they are deployed so that an appropriate cloud-backed storage provider can be plugged in (it's essential for some containerization scenarios). Bind mounts wouldn't make sense in this context so separating the concepts works well.

I think it also leaves room for things like secret mounts etc.

[davidfowl]

In the future we want to add volume mount support for resources when they are deployed so that an appropriate cloud-backed storage provider can be plugged in (it's essential for some containerization scenarios).

This came up in the workshop as well (many questions about how local volumes get mapped to cloud).

[mitchdenny]

One thought ... should we introduce separate annotations entirely? We can merge them together for the bridge to DCP but we can keep the concepts isolated in the app model side.

[JamesNK]

If you think it’s a good idea, then I’ll do it.

Edit: What is the benefit? Right now, the data on the annotation is exactly the same for both mount types. What would change if we moved to two attributes? (other than the type disappearing).

I'm guessing you're thinking about a VolumeMountAnnotation having a public Volume Volume { get; set; } property. Then the following code...

public static IResourceBuilder<T> WithVolumeMount<T>(
    this IResourceBuilder<T> builder, Volume source, string target, bool isReadOnly = false) where T : ContainerResource
{
    var annotation = new VolumeMountAnnotation(source, target, ContainerMountType.Named, isReadOnly);
    return builder.WithAnnotation(annotation);
}

var data = builder.AddVolume("data");

var catalogDb = builder.AddPostgres("postgres")
                       .WithPgAdmin()
                       .AddDatabase("catalogdb")
                       .WithVolumeMount(data, "/etc/data");

...would end up with the volume on the annotation.

Why would that be better than setting whatever name is on the volume (data in this case) onto the annotation?

public static IResourceBuilder<T> WithVolumeMount<T>(
    this IResourceBuilder<T> builder, Volume source, string target, bool isReadOnly = false) where T : ContainerResource
{
    var annotation = new ContainerMountAnnotation(source.Name, target, ContainerMountType.Named, isReadOnly);
    return builder.WithAnnotation(annotation);
}

[mitchdenny]

When we tackle volume support at deployment time in the future, we'll probably need to have some kind of concept of where the source for the volume is. For Azure-based deployments this might be either Azure Files (assuming an ACA compute) or something else. None of these options align to a bind mount which is why I thought splitting them out made sense.

[mitchdenny]

.. in short though, yes I think we'll have something like AddVolume(...) in the future, and I'm not sure if it'll be a child resource off a cloud storage provider, or whether it'll be a top level resource. I'm leaning towards top level resource right now.

[davidfowl]

I have one change to request since we're touching this code. Can you make the default path relative to the apphost directory?

aspire/src/Aspire.Hosting.Azure/AzureResourceExtensions.cs

Line 189 in 2c45e3c

var fullyQualifiedPath = Path.GetFullPath(path, builder.ApplicationBuilder.AppHostDirectory);

[JamesNK]

When we tackle volume support at deployment time in the future, we'll probably need to have some kind of concept of where the source for the volume is. For Azure-based deployments this might be either Azure Files (assuming an ACA compute) or something else. None of these options align to a bind mount which is why I thought splitting them out made sense.

But won't it just involve setting the existing source property to whatever value the volume has? What is the scenario where a volume mount needs something other than public string Source { get; init; }?

[JamesNK]

I have one change to request since we're touching this code. Can you make the default path relative to the apphost directory?

aspire/src/Aspire.Hosting.Azure/AzureResourceExtensions.cs

Line 189 in 2c45e3c

var fullyQualifiedPath = Path.GetFullPath(path, builder.ApplicationBuilder.AppHostDirectory);

So remove the Path.GetFullPath call and use path directly?

[davidfowl]

So remove the Path.GetFullPath call and use path directly?

Yes. Move the GetFullPath to the underlying path resolution.

[JamesNK]

I'm going to merge this as is, and follow up stuff (maybe splitting up annotation, azure path change) can be follow up PRs.