Add generic docker extra arguments option
NOBLES5E opened this issue · 11 comments
Description
Add an option to toastfile so that we can add arbitrary extra docker arguments
Additional context
I find that I cannot add --devices and --privileged etc. with current toastfile definition. But they are needed for example when we want to use GPU and other devices within the container. Adding an option to support any docker arguments can be a general solution to solve such problems.
Hi @NOBLES5E, thanks for submitting this feature request. I can see how this would be useful.
I would welcome a design proposal for this. Given the way Toast uses Docker, I think we'd be allowing users to pass extra flags to the docker container create ... command.
I have a few concerns that could potentially be addressed:
- Right now Toast is meant to be an abstraction above Docker. In some sense, Docker is an implementation detail. I'm worried that allowing users to pass flags directly to Docker could have some negative consequences if we aren't careful. We don't want users to be able to break any invariants that Toast relies on. I don't have any concrete examples at the moment, so this is mostly a theoretical concern. But I prefer the abstraction to be as non-leaky as possible.
- I'm not too familiar with these flags, and I don't think I have the hardware to test them properly. I'm also not sure what can be tested on the GitHub actions hosted runners. If this feature were to be added, I would want to make sure that it's properly tested.
- I don't have a good sense of how many Toast users would benefit from this, so it's hard for me to evaluate the benefit against the cost of maintaining the feature. On a personal note, I think Toast has already nearly reached the complexity budget that I want to support as a maintainer, so new features need to be sufficiently justified.
- In the past, we have tried to switch from using the Docker CLI to the Docker API (with bollard) for performance reasons. We reverted that change due to some minor issues with Bollard and/or Docker itself, but I would like to keep open the option to switch back to it. I think implementing this feature request would prevent us from ever switching back to Bollard.
For now, I will leave this issue open for discussion and so people can voice their thoughts about this request.
It seems to me that just adding an optional key in toast.yml such like extra_docker_args would be enough in this case. At least users that need a GPU environment for their CI will benefit from it. I can see some other use cases, for example using docker in docker to build image (which is common in the build process for many projects).
To test this, it seems to me we just need to test the final generated docker command matches our expectation. We do not need to actually run it, since this is just a feature enabling users passing arbitrary docker arguments, and if they passed wrong arguments, docker should be responsible for giving an error message.
For 4), yes I agree once we add this we will not be able to switch back to Bollard.
That testing strategy seems reasonable to me: just test that Docker receives the arguments as expected.
However, I am unlikely to support a solution which locks us out of the possibility of switching to using the Docker API directly (rather than indirectly through the Docker CLI) in the future.
Thanks for your reply! I understand your concern. May I know to which extent we plan to support for container options in your mind? I see user and env and some other things already in toast.yml, while privileged is not. They are all in the same container exec options struct in bollard (https://docs.rs/bollard/0.11.0/bollard/exec/struct.CreateExecOptions.html#fields). There are a lot more options for container creation (https://docs.rs/bollard/0.11.0/bollard/container/index.html).
Do you think we need to support container options (regardless of how to implement it)?
Do you think we need to support container options (regardless of how to implement it)?
It's balancing act between trying to support features that people want vs. the consequences of implementing and maintaining those features. For this particular feature, I appreciate that there are valid use cases for these less common Docker features. However, I am not interested in supporting every possible option that Docker supports, which would make Toast as complicated as the Docker CLI/API.
In my opinion, Toast is already too complicated, and I actually wish it had fewer features, not more. I have to balance that against what other people want, and unfortunately sometimes that means I have to turn down requests.
On a separate note, the reason Toast supports user and env is because those options are relatively universal: they are relevant for running any command, even without Docker. I was considering adding support for Windows containers, because Docker already runs on Windows (but only with Linux containers), and that functionality could even be used to build Toast binaries for Windows (so there would be a nice dogfooding aspect to it). However, it's not clear to me how options like --privileged would work with Windows containers, if at all.
Accepting requests often comes with some debt, and it can be overwhelming for me to deal with the compound effect of all of it—even for relatively innocuous seeming features. For example, after your very much appreciated pull request #361, I am now wrestling with how to explain that mount_paths supports host_path:container_path, but similar fields like input_paths and output_paths do not. This is a new complication that we didn't have before. Overall, the feature was worth it in that case (and I am very grateful for your contribution), but I hope this gives you a sense of why I am sometimes reluctant to accept feature requests.
The reason I have kept this issue open for now is because I wanted to give people a chance to chime in and influence my perspective. Right now I don't have a strong sense of how many people will benefit from this, and I want to stay open minded and collect feedback from the community. If it turns out to be just one user, then unfortunately I predict that the feature will not be implemented.
Very reasonable. Thanks for your explanation!
As discussed above, it might not be suitable to add this feature in toast directly since it prevents us from using docker API. Unfortunately in my current workflow I have to customize docker args, so I spent some time implementing it in a fork and created a PR here so that someone can reuse it if they want :)
It adds extra_docker_args: ["args1", "args2"] support in current toast.yml.
late to the party 👋
FYI: I'm following the Frontend Masters course on Docker (and see how far I can go with Toast) and the first part of the course is only doable with privileged option on.
The unshare command requires extra privilege 🤷
@xbbyhiras 👋 Does the extra_docker_arguments option work for your use case?
@xbbyhiras 👋 Does the
extra_docker_argumentsoption work for your use case?
Like a charm 🎉 !
Here's the gist