dotnet / aspire

Tools, templates, and packages to accelerate building observable, production-ready apps
https://learn.microsoft.com/dotnet/aspire
MIT License
3.93k stars 480 forks source link

Allow configuring volume behavior when using Aspire.Hosting.Testing #4462

Open DamianEdwards opened 5 months ago

DamianEdwards commented 5 months ago

Currently, when testing AppHost projects that include container resources configured to use volumes, the same volumes are used in the test as that are used in normal development. This isn't great because the test isn't as isolated from the normal running of the AppHost as it could be and could mutate the data in the volume.

We should allow for configuring the behavior of volumes when using Aspire.Hosting.Testing, and by default remove volumes from all container resources under test so that they are better isolated. Volume behavior should be configurable globally for all resources, or on a per resource basis (overriding the configured global behavior), by way of new API to support both scenarios.

mitchdenny commented 5 months ago

I wonder if this is something that we can do in conjunction with DCP where we some how nominate a volume/bindmount as being volatile. So we still generate the volumes and bind mount but when DCP tears down the the container it also deletes the volume/bindmount contents.

/cc @karolz-ms @danegsta

karolz-ms commented 5 months ago

Interesting idea. Currently volumes are never deleted automatically, but we could have an opt-in flag for cleaning them up, sort of like Containers have their persistent flag. Let us know if you want to pursue this idea and we can make it happen in DCP

DamianEdwards commented 5 months ago

I think that's a good idea. It's likely required to make the randomization of volume names in tests reasonable.

mitchdenny commented 5 months ago

@karolz-ms I'll assign this to you for the DCP side first. Once you've got something that we can set in the spec for volumes/bindmounts then you can assign back to me and I'll work on the app model hook up.

karolz-ms commented 5 months ago

@mitchdenny Roger, makes sense. @DamianEdwards I do not think this will fit into 8.1. Scheduling for 8.2 tentatively, please make a noise if you disagree.

DamianEdwards commented 5 months ago

@karolz-ms sounds fair.

What do folks think of doing a click-stop improvement in 8.1 of simply removing all volumes when using DistributedApplicationTestingBuilder so that tests are better isolated from non-tests scenarios? The DCP side isn't required for that. We could choose to make this behavior the new default or make it opt-in/mutable via a new extension method void SetContainerResourceVolumeBehavior(this IDistributedApplicationTestingBuilder, VolumeBehavior behavior)?

karolz-ms commented 5 months ago

We can try an experiment with the DeleteResourcesOnShutdown option that we have: https://github.com/dotnet/aspire/blob/main/src/Aspire.Hosting/Dcp/DcpHostService.cs#L75 But AFAIK this is not getting much use as the default is to rely on DCP to do the cleanup, so I do not know if we have enough time in 8.1 to test this alternate way of cleaning things up.

DamianEdwards commented 5 months ago

Sorry I wasn't clear, I mean simply removing mount annotations from resources when under test.

karolz-ms commented 5 months ago

Oh, sounds like I completely misunderstood the ask here, sorry. You want the tests to use ephemeral volumes instead of persistent volumes, correct? Makes perfect sense. If I am not mistaken, this should work today with DCP. That is, you can have a Container resource that has VolumeMount in the spec, but no corresponding ContainerVolume objects. This should result in a creation of ephemeral volume with the lifetime corresponding to the lifetime of the Container object. LMK if that does not work for you.

@mitchdenny If my understanding of the ask is correct, there should be no work on the DCP orchestrator side to satisfy it.

DamianEdwards commented 5 months ago

Ah that's wonderful to hear, thanks! So now it comes down to timing/resourcing/scheduling/sequencing 😄

Do we push to implement support for two volume behavior under test modes in 8.1: no volumes, and ephemeral volumes; or do we just for the initial click-stop of a "no volumes" behavior in 8.1 and introduce the ephemeral volumes option later, given that the ephemeral option is likely to be more work.

Thoughts @mitchdenny @davidfowl?

mitchdenny commented 4 months ago

@DamianEdwards help me understand the ask a little bit more.

So we've written a test case and the test case includes a WithDataVolume or some other method that results in a volume being configured for the container. By default, this volume will have a deterministic name based on the apphost path etc.

This becomes a problem when running a test because you may execute it multiple times and you don't want the old data volume getting in the way.

You could just not add a volume, and allow the container to use its own ephemeral storage without the path being bound. I don't think we have any resources today that require a volume mount that wouldn't be better served by a bind mount (e.g. Keycloak for realm import).

HOWEVER ... sometimes you might have a resource that you created via an extension method and it sets up the volumes within the extension and you can't change that default behavior.

Perhaps what we need here is an extension API in Aspire.Hosting.Testing that allows you to mutate the volume annotation? Something like this:

builder.AddSomeResourceThatAutomaticallyAddsAVolume(...)
       .WithVolume("thevolume", mount => mount.IsEmphemeral = true);

We would add a property to ContainerMountAnnotation which would then be processed by the AppliationExecutor when setting up the mounts with DCP.

Thoughts?

DamianEdwards commented 3 months ago

@mitchdenny the case is a bit more straightforward than that.

In your AppHost project you have a container resource that uses a volume, e.g.:

var pg = builder.AddPostgres("server").WithDataVolume();
var db = pg.AddDatabase("db");

This is done because you want the data to be persistent between runs during development.

Then you write a test for your AppHost project using DistributedApplicationTestingBuilder. When that test runs and spins up the AppHost, you don't want the volumes to be used because:

  1. you don't want data mutations by the test to impact the data persisted during runs in development, and vice-versa
  2. you want the test data isolated across repeated runs

In the samples repo tests, I have an extension method that the tests call that discovers volume annotations on resources and updates them so that they're scoped appropriately for the test, specifically:

https://github.com/dotnet/aspire-samples/blob/b013f64e063dce2a4c72b840eab83d43991a5d22/tests/SamplesIntegrationTests/Infrastructure/DistributedApplicationExtensions.cs#L41-L76

The problem with this at the moment is that the second case above (randomized names for volumes shared across resources) results in volumes being created on each test run that are never cleaned up. That's where a new feature in DCP would likely help.

A full solution here likely involves adding new API to Aspire.Hosting.Testing that allows controlling the behavior of container volumes on resources when using DistributedApplicationTestingBuilder, so that folks can opt-in to the behavior that meets the needs of the test, e.g. (names totally TBD):

mitchdenny commented 3 months ago

Moving to backlog. If we want to bring it into 9.0 then lets look at whether it can be squeezed in given our other priorities.