WIP: Kubernetes: Add basic support for volumes

I'm currently running this in GKE along with !327 (merged) with great success

Works great for me (binding docker socket), thanks for this missing piece!

Mentioned in merge request !338 (closed)

Thanks for this addition. I think this could actually come in very useful in a few other scenarios too... for example, the EmptyDir volume can be shared between all containers within a pod - this could make it really easy to share data between services & the build step itself.

We need to add in a check for if the volume mount doesn't exist (eg. if the user specifies a VolumeMount that requires a non-existent VolumeSource. Else builds will sit waiting to start until the timeout)

We should also have a test to test this edge-case to ensure the build actually fails in this scenario :)

Thanks @munnerz , I'll get working on your comments and push when I have something.

Perhaps you have some input to the config layout. coming at it with fresh eyes after a few days, I think we need to change it slightly

originally proposed:

    [[runners.kubernetes.mounts]]

and

    [[runners.kubernetes.host_path]]

which would lead to...

    [[runners.kubernetes.secret]]
    [[runners.kubernetes.config_map]]
    [[runners.kubernetes.empty_dir]]

which seems messy

When we start adding secrets, config maps, emptydir as volume sources, this might not look ideal. Should we instead put the volume sources underneath a volume key, following closer to the kubernetes config files?

volume mounts stay the same?

    [[runners.kubernetes.mounts]]

put the volume sources under a volume key

    [[runners.kubernetes.volume.host_path]]
        name = "docker"
        path = "/var/run/docker.sock"

    [[runners.kubernetes.volume.secret]]
        name = "gcloud-auth"
        secret_name = "mysecret"

    [[runners.kubernetes.volume.empty_dir]]
        name = "cache"

or

    [[runners.kubernetes.volumes]]
        type = "host_path"
        name = "docker"
        path = "/var/run/docker.sock"

    [[runners.kubernetes.volumes]]
        type = "secret"
        name = "gcloud-auth"
        secret_name = "mysecret"

    [[runners.kubernetes.volumes]]
        type = "empty_dir"
        name = "cache"

Added 92 commits:

78359b4c...1fd3fd4f - 91 commits from branch gitlab-org:master
a894f8ea - Kubernetes: Add basic support for volumes

Compare with previous version

Added 1 commit:

8b58b0ba - Add kubernetes secrets

Compare with previous version

Resolved all discussions

Mentioned in issue #1759 (closed)

mentioned in issue #1876 (closed)

@jon-walton sorry for the massive delay in response

I much prefer the second of the 3 code snippets you commented. It more closely resembles the k8s API, and saves having a single KubernetesVolume structure that contains a union of all fields from all k8s volume types (that struct will bloat very quickly!).

Are you still able/interested to work on this MR?

@nick.thomas @tmaczukin @ayufan have you any opinion on how this is best to be expressed in config? I think this is something needed, and some users are currently relying on 3rd-party forks in order to get this functionality (see https://gitlab.com/gitlab-org/gitlab-ci-multi-runner/issues/1876, https://gitlab.com/gitlab-org/gitlab-ci-multi-runner/issues/1759)

Also, if we're adding support for Secrets as well within this MR, how should we handle the case where the specified secret doesn't exist? Given this is at the runner config level it might make sense to just hang? I'm undecided. Either way, we should probably have an integration test asserting whatever behaviour is decided upon.

@munnerz @jon-walton If I understand correctly how this suppose to work, then we could have a such example config:

[runners.kubernetes.mounts]
  name = "docker"
  mount_path = "/var/run/docker.sock"
  read_only = false
[runners.kubernetes.volumes.host_paths]
  name = "docker"
  path = "/var/run/docker.sock"

[runners.kubernets.mounts]
  name = "cache"
  mount_path = "/cache"
  read_only = false
[runners.kubernets.volumes.host_paths]
  name = "cache"
  path = "/cache"

[runners.kubernetes.mounts]
  name = "my-secret"
  mount_path = "/my-secret"
  read_only = true
[runners.kubernetes.volumes.secrets]
  name = "my-secret"
  secret_name = "my-secret-name"

The problem I see here is that I need to write two sections and repeat e.g. name = "docker" twice for each mount that I want to have. The obvious disadvantage is that I need to write it twice, the less obvious one is that the information about one mount is separated into two parts which can be in two different and distant places in the config.toml file. I understand that this is how k8s internal API handles this, but it's not user friendly.

I would rather define the same config in such way:

[runners.kubernetes.volumes.host_paths]
  name = "docker"
  path = "/var/run/docker.sock"
  mount_path = "/var/run/docker.sock"
  read_only = false

[runners.kubernets.volumes.host_paths]
  name = "cache"
  path = "/cache"
  mount_path = "/cache"
  read_only = false

[runners.kubernetes.volumes.secrets]
  name = "my-secret"
  secret_name = "my-secret-name"
  mount_path = "/my-secret"
  read_only = true

We have all needed information in one place, we don't need to type twice the name part or add the redundant [runners.kubernetes.mounts] header. And we still can recreate the separated structures which will be internally pushed to k8s API. And of course such structure is open for other k8s volumes types.

Also, if we're adding support for Secrets as well within this MR, how should we handle the case where the specified secret doesn't exist? Given this is at the runner config level it might make sense to just hang?

I think we should write a warning (e.g. Secret "my-secret-name" doesn't exists. Skipping mount) and just go over it. If user's build will depend on the secret it will still fail at the end and the warning will be a good information to know why. And if a build doesn't depend on the secret, but is executed on this Runner, failing the build would be not nice :)

We should also add documentation for new configuration parameters.

@jon-walton @munnerz Do you need any help to move this forward? We have already few good improvements for Kubernetes executor merged into 1.10 and it would be excellent if we could finish work on this before 20th of January, so it'll be released this month :)

@tmaczukin I'll happily pick this up to get it in for the 1.10 release, unless @jon-walton is able to.

Won't be able to start until tomorrow pm GMT however, but I think there's only a few very small bits left to do so should be fine.

@munnerz sorry for my slow reply! Unfortunately life (new ) has taken all my spare time over the past few months, so i can't commit to getting the changes made in time. I'm not sure what's easiest for you; if you want to continue with this MR, I've given you permissions to my fork; otherwise @tmaczukin feel free to close this one and @munnerz can open a new one

Sorry for not being able to get this through to completion

@jon-walton Thanks for the initial work! And no need to be sorry - take your time with the family!

@munnerz Thanks. And in case of any help needed I can also jump into this, starting at Thursday afternoon (GMT+1), so ping me here if you need me :).

@tmaczukin I'm happy to implement a check for secrets existence and just warn the user if it doesn't exist, but that will require an additional call to the api server to actually check for the secrets existence. In k8s, if you specify a secret to use and it doesn't exist, the pod won't start.

Sound okay to you? The racy nature shouldn't be too much of an issue I'd imagine... unless the user deletes the secret in between the check for the existence and it getting submitted to the apiserver.

@jon-walton congratulations on the new addition!

@munnerz Let's do it this way. It's unlikely that a user will remove the secret just in the moment between the check and the API call. And if pod will not start without a configured secret I think it's worth to check this and fail the build with a proper warning.

Is there some way to make this configuration more generic, to not force us to implement any volume type that is supported by Kubernetes?

Can we just use some struct type to define all parameters?

https://kubernetes.io/docs/user-guide/volumes/

@ayufan So maybe add type field to the volumes struct and then dynamically create structs for k8s API?

[runners.kubernetes.volumes]
  type = "hostPaths"
  name = "docker"
  source = "/var/run/docker.sock"
  target = "/var/run/docker.sock"
  read_only = false

[runners.kubernets.volumes]
  type = "hostPath"
  name = "cache"
  source = "/cache"
  target = "/cache"
  read_only = false

[runners.kubernetes.volumes]
  type = "secret"
  name = "my-secret"
  source = "my-secret-name"
  target = "/my-secret"
  read_only = true

I don't think we can dynamically create structs based on field names that are unknown.

We can't knowingly predict the types of the fields we're converting into. Some volume types include permissions bits (as integers), some you've already documented (read_only), and others strings. There's also no guarantee there won't be a more complex structure there in future (and I'm not 100% sure there isn't already).

@ayufan how do you envision keeping a generic structure type given the above?

We could limit support initially to just hostPath, persistentVolumeClaim (which encapsulates almost all other volume types, albeit in a slightly different way), secrets & ConfigMaps (two types that cannot be used in conjunction with PVC) - then see how that works for people.

@munnerz

How about then allowing you to pass a volume yaml in Kubernetes syntax, but using golang's templates to evaluate the fields that will be parsed to structure?

[runners.kubernetes.mounts]
  name = "docker"
  mount_path = "/var/run/docker.sock"
  read_only = false
[runners.kubernets.volumes]
  data = "name: test-volume
    hostPath:
      path: /data"
[runners.kubernets.volumes]
  data = "name: test-volume
    awsElasticBlockStore:
      volumeID: <volume-id>
      fsType: ext4"
[runners.kubernets.volumes]
  file = "/path/to/my/volume/specification.yaml"

changed milestone to %v1.11

added executorkubernetes improvement labels

This MR hasn't changes since 2 months ago, So I have rebased this changes with current master in my repo.

@ayufan Embedding yaml inside of config.toml looks strange. I think the last example with file = "/path/to/my/volume/specification.yaml" is the only acceptable. But how about passing all volumes configuration to a separated yaml file and loading this whole file once for an executor:

[[runners]]
  name = "Kubernetes Runner"
  url = "https://gitlab.com/ci"
  token = "TOKEN"
  executor = "kubernetes"
  [runners.kubernetes]
  namespace = "gitlab-ci"
  volumes_specification = "path/to/volumes/specification.yaml"
  (...)

Then you would manage volumes specification using k8s yaml syntax.

This is definitely interesting. Do we need then also templates for this volume specification?

@jon-walton What is the status of this MR? Could you post an update?

@zj I haven't had time to touch this since my last update, I also don't foresee myself having time for this coming release

@jon-walton If you don't mind, I'll try and finish this in this release. This is a really nice feature, which I'd like to include in the upcoming release.

assigned to @zj

@zj please do, it'll be good to get this across the line

mentioned in merge request !516 (merged)

@zj I think we can close this one in favor of !516 (merged), right?

closed

mentioned in issue #2258 (closed)

WIP: Kubernetes: Add basic support for volumes

What does this MR do?

Why was this MR needed?

Are there points in the code the reviewer needs to double check?

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Activity

Admin message

Admin message

WIP: Kubernetes: Add basic support for volumes

What does this MR do?

Why was this MR needed?

Are there points in the code the reviewer needs to double check?

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Merge request reports

Activity