-
Notifications
You must be signed in to change notification settings - Fork 139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for default volume mounts #157
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for much for the PR!
I think this is a really useful option to add, for the sorts of use cases you mentioned.
One major change though, if you don't mind making it, is can we change it a standard option, without having to introduce a new env var prefix?
In the option description perhaps we can document that it's most commonly used as an environment variable on the agent machine?
Apologies if I've missed something, but how does this differ from https://github.com/buildkite-plugins/docker-compose-buildkite-plugin#volumes-optional-run-only in the use-cases it addresses? Is it just that it's designed to be agent level vs pipeline level? |
@toolmantim I'm happy to change the variable name and/or configuration flow. I'm not sure I fully understand what you mean by "change it as a standard option". Would that mean naming it as My, potentially minor, concern with that idea is that this configuration value isn't tightly coupled to |
@lox Yes, this is intended to provide an additional layer of agent level volume configuration, rather than the pipeline level configuration in the plugin |
Thanks for the full explanation! Yep, I was suggesting to rename it to the standard variable names, just because then it's behaviour matches all the config options (which can also be set via env vars), if we add env/yaml validation it'll work with it, and if we auto-gen docs based on plugin.yml it won't be left out. And it's just the thing people are familiar with. It'd mean that in your agent hook, you'd need to do something like this: DEFAULT_VOLUMES="/buildkite:/buildkite;./app/app"
export BUILDKITE_PLUGIN_DOCKER_COMPOSE_DEFAULT_VOLUMES="${DEFAULT_VOLUMES}"
export BUILDKITE_PLUGIN_DOCKER_DEFAULT_VOLUMES ="${DEFAULT_VOLUMES}"
# any other plugins Is that okay? If there was some way for plugins to access hook exported env vars, separately to pipeline.yml settings, then maybe we wouldn't need to this additional We should probably add the (semi-colon delimited) string support to the existing My vote as a way forward would be for each of the Docker plugins to:
|
After some thought, I wonder whether this change is at the right layer of abstraction. I suspect there should be a distinction between agent-wide docker customizations and docker settings for a particular step. I'd always imagined that we'd apply them via a custom docker bootstrap and a docker socket proxy like https://github.com/buildkite/sockguard. What if we added a way to ensure certain volumes are mounted in sockguard \
--upstream-socket /var/run/docker.sock
--volume /my/llamas/dir:/llamas &
export DOCKER_HOST="unix://$PWD/sockguard.sock" Then docker invocations subsequently in the job would always include the I think using |
That sounds like an reasonable solution to me; I wasn't aware of My experience with docker & docker-compose isn't exhaustive, particularly as we're getting into territory like proxying the docker socket. Just to restate/clarify, to make sure I'm not missing something: At a high level, my goal is to enable a solution that enables build commands and build plugins to run within docker containers with a shared build directory state. This is roughly equivalent to the container builder execution model, in which every build step runs in separate container with a shared We've solved this at
We need this mount process to be "transparent" from the level of We're currently developing several "reporting" style pipeline plugins, which will execute the plugin steps within a defined container. Several buildkite plugins adopt this strategy, however the majority assume that the Current Solution at
|
I see the advantage of making the additional volume mount transparent from the perspective of command/plugin docker invocations, however I'm not a huge fan of the level of indirection this adds to the system. This means that proper execution of the build command will be tightly, and implicitly, coupled to I'd also be hesitant to overload |
Interesting @lox. I always thought sockguard was more for doing the opposite of what we’re doing here: hiding a sensitive volume mount from a Docker command in a build job, when running untrusted builds. Rather than adding a host volume for trusted builds, to help with shared state. 🤔 |
I’m not really sure what’s more correct. Maybe both are? Perhaps it’s worth just moving ahead with this small change in the meantime, and we can always relook at it in a world when peeps are using |
Yup, after some more thinking, I think I agree that sockguard isn't quite the right spot for it. It seems like the root problem here is that we are seeing lots of plugins use a volume mount of I think I agree that there should be some sort of convention for overriding that in plugins that use docker @asford. What if rather than the generic For instance https://github.com/buildkite-plugins/golang-buildkite-plugin/blob/e5624da1a545afd740adfa8e9cae745a85c3fd1c/hooks/command#L14 would become:
Then we'd have to add support to every plugin for that specific env, and probably a linter check too. Almost all of the plugins in existence now basically don't work unless Not sure how we apply that same fix for other things that plugins try and mount in, for instance It's gross either way 🤷🏼♂️ |
Worth noting, I reckon for the model you are talking about @asford, you absolutely want a container per job-run using the bootstrap like http://github.com/buildkite/docker-bootstrap-example. |
I don’t want to block simply adding another option to the plugin which lets you set up some default values for the The It’d be adding this to both Docker plugins:
I like your idea @lox of a default bind mount environment variable convention that we can use everywhere. I’m happy to ship that too, across the board. But probably just in addition to the above changes? |
Yup yup, I'm interested to hear what you think @asford of whether |
The There are a number of different bits of state that may need to be available within the step container including:
My goal with the My intention with the To be specific, our end value of Admittedly, this is getting into a relatively specialized configuration but that's where Buildkite really shines. @lox I believe that this approach will work over kubernetes, but as I'm not running on kube I'm not certain. Would you mind commenting on that? |
Yup, I hear you. I'm keen to see how your vision on this plays out, so let's 🚢it. @toolmantim my take would be to roll with |
One random thing @asford, we discovered the other day that |
Yep! In that case, let’s |
Thank you for helping us talk it through @asford! 🙏🏼 |
I wondered what that line was up to 🤔 |
Yikes, sorry about that. I was just running the bats tests locally. Fixed the underlying issue in #159. Shameless plug, but y'all should consider using this github checks buildkite plugin so external contributors can see CI output from pulls. |
This was released in v2.5.0 ( |
Add support for the generic environment variable
BUILDKITE_DOCKER_DEFAULT_VOLUMES
, specifying a set of volume mounts that are always passed intodocker-compose run
invocations.This feature is intended to support scenarios where the
buildkite-agent
is run viadocker-compose
with theBUILDKITE_BUILD_PATH
located on a external volume mount.BUILDKITE_DOCKER_DEFAULT_VOLUMES
can the be used to ensure that this mount is available indocker-compose
based pipeline steps, giving simplified access to the build directory.For a working example see uw-ipd/buildkite-agent.
My hope is that a feature of this type is elevated to a roughly standard status for
buildkite-agent
-within-docker
configurations and thatBUILDKITE_DOCKER_DEFAULT_VOLUMES
can be adopted bydocker
-based pipeline plugins as a mechanism to pass state between the build command and pipeline-based hook implementations in cases where build artifacts are not an suitable choice. For example, our github-checks integration plugin will adopt this feature to ensure that build outputs are available as check output.