Add support for specifying custom container run arguments

[danegsta]

PR adds a new annotation for specifying custom arguments to the container run command issued by the orchestrator. This is intended to support advanced scenarios like enabling GPU support in Docker containers, where adding annotations for individual flags may not make sense.

Microsoft Reviewers: Open in CodeFlow

[davidfowl]

What goes into the manifest?

[danegsta]

I was just writing up a message to ask that very question. I'm assuming we will want manifest entries for this annotation along with the rest of the container specific data?

[danegsta]

Only concern I have with adding the run arguments to the manifest is that they can potentially be runtime specific (the --gpus flag, for example, isn't supported in podman).

[DamianEdwards]

Yeah I think these can't be in the manifest as they're opaque and very much tied to the container host. We can build higher-level container runtime feature constructs (e.g. GPU support) and those could go in the manifest and could leverage this PR's changes when running locally, if we want to.

[DamianEdwards]

I'd really like to get this in as it enables GPU support for AI scenarios with Aspire which we have some folks experimenting with right now. @davidfowl @danegsta how do you feel about my comment above RE not putting this in the manifest?

[danegsta]

I'm already a bit nervous about adding them to the manifest without any additional runtime context, so I'd be fine with backing out my changes to write to the manifest until we've had a chance to figure out exactly how we want to add this data.

[davidfowl]

If they don't go into the manifest does that mean this is a runtime only scenario?

[DamianEdwards]

I think so yes. We could choose to add first-class modeling for GPU support later as a portable concept, but this is low level for when you want to pass args directly to the container runtime.

[davidfowl]

When you run on kuberentes aren't those args potentially applicable? How is this different from docker build args?

[DamianEdwards]

Honestly I don't know. Are they? But even just enabling GPU uses different runtime args between docker and podman.

[danegsta]

It looks like Podman 5.0 added support for the same --gpu argument as Docker. Doesn't really change the calculous on adding generic container run arguments to the manifest, but maybe this should become a more targeted feature for enabling GPU support rather than exposing the low level ability to add extra run arguments?

[DamianEdwards]

@danegsta I think both should exist, with GPU support being something that would go to the manifest, but container runtime args being something that only apply during run. Doing the container runtime args feature first allows us to unblock any scenario where args have to be passed to the container runtime locally (including enabling GPUs, setting resource constraints, configuring networks, changing privileges, and setting the user). I think that's worth doing as an escape hatch to allow folks to get the full capabilities of their local container runtime during dev until we decide which higher level concepts to model.

[danegsta]

@DamianEdwards I backed out the logic to write the manifest.

[davidfowl]

@danegsta backport?

[davidfowl]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/aspire/actions/runs/8626993889

[github-actions]

@davidfowl backporting to release/8.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Add support for new run arguments option
Using index info to reconstruct a base tree...
M	src/Aspire.Hosting/Dcp/ApplicationExecutor.cs
M	src/Aspire.Hosting/Dcp/Model/Container.cs
M	src/Aspire.Hosting/ResourceBuilderExtensions.cs
M	tests/Aspire.Hosting.Tests/DistributedApplicationTests.cs
Falling back to patching base and 3-way merge...
Auto-merging tests/Aspire.Hosting.Tests/DistributedApplicationTests.cs
Auto-merging src/Aspire.Hosting/ResourceBuilderExtensions.cs
Auto-merging src/Aspire.Hosting/Dcp/Model/Container.cs
Auto-merging src/Aspire.Hosting/Dcp/ApplicationExecutor.cs
CONFLICT (content): Merge conflict in src/Aspire.Hosting/Dcp/ApplicationExecutor.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add support for new run arguments option
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

[github-actions]

@davidfowl an error occurred while backporting to release/8.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

[prom3theu5]

This is cool
Would it be possible to emit them to the manifest please as currently they aren't so downstream tools have no way to know that run args are expected to be passed.

[DamianEdwards]

We decided not to emit them as they're container-host specific and thus don't translate well, e.g. args for docker run are different than those for podman run. But we can always change that decision. Please log an issue and we can discuss there.