search

buildkite / agent-stack-k8s

Spin up an autoscaling stack of Buildkite Agents on Kubernetes
MIT License
77 stars 30 forks source link

Allow patching pod spec from controller configuration and steps #282

Closed triarius closed 5 months ago

triarius commented 5 months ago

This is our spin on @42atomys' excellent suggestion to allow applying a strategic merge patch of the PodSpec. It builds on their work in #262 but adds a few things we suggested in PR review.

Notably

  1. Adds integration and unit tests
  2. Added pod-spec-patch to the helm value file's JSON schema
  3. Parsing of the user provided PodSpecPatch YAML as corev1.PodSpec
  4. It will NOT be possible to set Command and Args on a container via a pod spec patch. This is because we've found that the interaction between a step's command: attribute and these containers fields is too complex to expose to users. Now, using pod spec patch, the steps's command: will be the only way to customise the process executed in a container. However, the existing sidecarContainers functionality is not affected by this, so for users that need additional containers for which they need to customise Command and Args, we recommend they use that instead.

Now instead of specifying a verbose PodSpec in every step, users can specify a common patch in their helm chart. The controller will take the command of a step and attempt to create a JK8s ob around it. Further customisation of the PodSpec is supported by applying the patches. Thus, most of the complexity of the Kubernetes YAML can be shifted to the Helm chart's values rather than the Buildkite YAML. Any further step level customization should be performed with the step level podSpecPatch rather than specifying a step level podSpec.

Fixes: #247 Closes: #262

Reviewers should note that a significant detour had to be made in order to reuse the corev1.PodSpec go struct in parsing the config level podSpecPatch. The essence of the issue that the library used to parse configuration, viper, does not support the struct tag json:",inline" because it relies on mapstructure for which the equivalent struct tag is mapstructure:",squash". I've made a fork of mapstructure that rectifies this, and used a replace directive in go.mod to force viper to use that the forked version of mapstructure. I plan on contributing the fork back upstream, so hopefully this is a temporary situation. There is a more detailed explanation in the commit message for https://github.com/buildkite/agent-stack-k8s/pull/282/commits/8f253f55b807acf309c112157a9e6df235c62552

artem-zinnatullin commented 3 months ago

I just wanted to say that this is a very good change, makes common infra configuration much easier 👍