Skip to content

Adding envs key Helm values to gateway resources - #688

Merged
consideRatio merged 8 commits into
dask:mainfrom
JoeJasinski:feature/add-envs-to-controller-and-gateway-helm-chart
Mar 2, 2023
Merged

Adding envs key Helm values to gateway resources#688
consideRatio merged 8 commits into
dask:mainfrom
JoeJasinski:feature/add-envs-to-controller-and-gateway-helm-chart

Conversation

@JoeJasinski

@JoeJasinski JoeJasinski commented Feb 27, 2023

Copy link
Copy Markdown
Contributor

This MR is to enhance this use-case where one wishes to customize the extraConfig that's passed to the gateway via the configmap. Currently, according to the documentation, you can create additional python configuration for any of the Traitlet-enhanced classes from the "Configuration Reference".

However, it would be nice to be able to pass in environment variables to that config, so you could set the python config additions, but tune them by making a change to an environment variable rather than via making python code changes.

Take this simple Helm values file example:

gateway:
  extraConfig:
    # Note that the key name here doesn't matter. Values in the
    # `extraConfig` map are concatenated, sorted by key name.
    clusteroptions: |
      c.KubeClusterConfig.idle_timeout = 300
      c.KubeClusterConfig.cluster_max_memory = "20G"

If I wanted to customize the config values, I'd have to override the entire config, or create a new "extraConfig" key.

However, it would be nice to be able to do the following:

gateway:
  envs:
  - name: IDLE_TIMEOUT
    value: "500"
  extraConfig:
    # Note that the key name here doesn't matter. Values in the
    # `extraConfig` map are concatenated, sorted by key name.
    clusteroptions: |
      import os
      idle_timeout =  int(os.environ.get("IDLE_TIMEOUT", 300))
      cluster_max_memory = os.environ.get("CLUSTER_MAX_MEMORY", "20G")
      c.KubeClusterConfig.idle_timeout = idle_timeout
      c.KubeClusterConfig.cluster_max_memory = cluster_max_memory

Then, you could easily change those config values by passing in environment variables to the helm chart without having to change the python code.

This MR is to change the helm chart so that it accepts an "envs" list of dicts that get passed to the gateway deployment where the dask gateway config ConfigMap gets passed into the gateway.

Does this sound like a feature that would be useful?

@JoeJasinski JoeJasinski changed the title Adding envs key Helm values to controller and gateway resources Adding envs key Helm values to gateway resources Feb 28, 2023

@consideRatio consideRatio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @JoeJasinski for your thorough descriptions in PR and issue!!! ❤️ 🎉 🌻

I'm 👍 for supporting gateway.env as a configuration, but I want it to be fully k8s compatible and support valueFrom for example.

I've made comments to help you work towards that!

Comment thread resources/helm/dask-gateway/templates/gateway/deployment.yaml Outdated
Comment on lines +111 to +129
env: &env-spec
type: array
items:
type: object
additionalProperties: false
properties:
name:
type: string
description: |
The name of the environment variable to pass to the pod
value:
type: string
description: |
The value of the environment variable to pass to the pod
required:
- name
- value
description: |
Additional environment variables to apply to the resource

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should allow for configuration as declared in https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.26/#envvar-v1-core, including name (string), value (string), and valueFrom (object).

I'd like to see that:

  • the description mention that users should use the k8s official configuration syntax
  • properties entries for name/value/valueFrom declaring them string/string/object, you can skip the descriptions here to defer to the k8s official docs instead
  • for the valueFrom object, just declare additionalProperties: true and don't declare its properties
  • remove the &env-spec anchor definition if its not used elsewhere
  • Only name is required as otherwise you can't choose from either value or valueFrom.

Comment thread resources/helm/testing/chart-install-values.yaml
@JoeJasinski

Copy link
Copy Markdown
Contributor Author

@consideRatio Thank you for the feedback. I like that approach of allowing the downward API access to set environment variables. I'll make these changes.

JoeJasinski and others added 5 commits March 1, 2023 18:27
MR Feedback from @considerRadio to support the valueFrom syntax for environment variables as well.

Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
MR Feedback from @considerRadio to support the valueFrom syntax for environment variables as well.

Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
…ng environment variables to the gateway via the valueFrom syntax of the downard api
@JoeJasinski

Copy link
Copy Markdown
Contributor Author

@consideRatio Thank you again for the review. I believe I made the changes that you pointed out. I like the toYaml loop that you added for the environment variable spec; I'll have to include that in my Helm charts. I've updated the jsonschema to allow for the valuesFrom key.

Let me know if there are any other updates you'd like me to make.

@consideRatio
consideRatio merged commit 327e44c into dask:main Mar 2, 2023
@consideRatio

Copy link
Copy Markdown
Collaborator

Wieee this looks great! Thank you @JoeJasinski!! ❤️ 🎉 🌻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants