Skip to content
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

Allow an existing ConfigMap to be passed for baseJobTemplate #340

Merged
merged 7 commits into from
May 31, 2024

Conversation

jamiezieziula
Copy link
Contributor

Resolves #334

@jamiezieziula jamiezieziula added enhancement An improvement of an existing feature breaking labels May 31, 2024
@jamiezieziula jamiezieziula marked this pull request as ready for review May 31, 2024 00:40
@jamiezieziula jamiezieziula requested a review from a team as a code owner May 31, 2024 00:40
@jamiezieziula
Copy link
Contributor Author

FYI I tested this locally for the following three use cases:

  • chart installed as is, with only the work-pool name, account & workspace id set
  • chart installed referencing an existing configmap
  • chart generating a configmap with provided json data

All three built as expected!

I would consider this a breaking change, since the existing value's schema for the basejobtemplate has been changed. I will cut a release of this tomorrow and explicitly write out the upgrade notes

jamie zieziula added 2 commits May 30, 2024 20:43
…ctHQ/prefect-helm into allow-config-map-base-job-template
@mitchnielsen
Copy link
Contributor

Testing (from the templating perspective) looks good from my side:

Click to expand testing notes

Using configuration keyword

worker:
  cloudApiConfig:
    accountId: 123-abc
    workspaceId: 456-def
  config:
    workPool: my-work-pool
    baseJobTemplate:

      # Option 1
      configuration: |
        {
          "variables": {
            "type": "object",
            "properties": {
              "env": {
                "type": "object",
                "title": "Environment Variables",
                "description": "Environment variables to set when starting a flow run.",
                "additionalProperties": {
                  "type": "string"
                }
              },
              "name": {
                "type": "string",
                "title": "Name",
                "description": "Name given to infrastructure created by a worker."
              },
              ...
            }
          }
        }

ConfigMap includes the provided configuration:

$ helm template charts/prefect-worker -f test.values.yaml --show-only templates/configmap.yaml | yq '.data'
baseJobTemplate.json: "{\n  \"variables\": {\n    \"type\": \"object\",\n    \"properties\": {\n      \"env\": {\n        \"type\": \"object\",\n        \"title\": \"Environment Variables\",\n        \"description\": \"Environment variables to set when starting a flow run.\",\n        \"additionalProperties\": {\n          \"type\": \"string\"\n        }\n      },\n      \"name\": {\n        \"type\": \"string\",\n        \"title\": \"Name\",\n        \"description\": \"Name given to infrastructure created by a worker.\"\n      },\n      ...\n    }\n  }\n}\n"

Deployment uses the flag and file name:

$ helm template charts/prefect-worker -f test.values.yaml --show-only templates/deployment.yaml | yq '.spec.template.spec.containers[0].args'
- prefect
- worker
- start
- --type
- "kubernetes"
- --pool
- "my-work-pool"
- --install-policy
- "prompt"
- --base-job-template
- baseJobTemplate.json

Deployment mounts the volume:

$ helm template charts/prefect-worker -f test.values.yaml --show-only templates/deployment.yaml | yq '.spec.template.spec.containers[0].volumeMounts[2]'
mountPath: /home/prefect/baseJobTemplate.json
name: base-job-template-file
subPath: baseJobTemplate.json

Deployment specifies the volume:

$ helm template charts/prefect-worker -f test.values.yaml --show-only templates/deployment.yaml | yq '.spec.template.spec.volumes[1]'
name: base-job-template-file
configMap:
  name: prefect-worker-base-job-template

Using existingConfigMapName

worker:
  cloudApiConfig:
    accountId: 123-abc
    workspaceId: 456-def
  config:
    workPool: my-work-pool
    baseJobTemplate:
      existingConfigMapName: my-base-job-template

ConfigMap is not deployed:

$ helm template charts/prefect-worker -f test.values.yaml --show-only templates/configmap.yaml | yq '.data'
Error: could not find template templates/configmap.yaml in chart
null

Deployment uses the flag and file name:

$ helm template charts/prefect-worker -f test.values.yaml --show-only templates/deployment.yaml | yq '.spec.template.spec.containers[0].args'
- prefect
- worker
- start
- --type
- "kubernetes"
- --pool
- "my-work-pool"
- --install-policy
- "prompt"
- --base-job-template
- baseJobTemplate.json

Deployment mounts the volume:

$ helm template charts/prefect-worker -f test.values.yaml --show-only templates/deployment.yaml | yq '.spec.template.spec.containers[0].volumeMounts[2]'
mountPath: /home/prefect/baseJobTemplate.json
name: base-job-template-file
subPath: baseJobTemplate.json

Deployment specifies the volume:

$ helm template charts/prefect-worker -f test.values.yaml --show-only templates/deployment.yaml | yq '.spec.template.spec.volumes[1]'
name: base-job-template-file
configMap:
  name: my-base-job-template

Using neither configuration nor existingConfigMapName

worker:
  cloudApiConfig:
    accountId: 123-abc
    workspaceId: 456-def
  config:
    workPool: my-work-pool
    baseJobTemplate: {}

ConfigMap is not deployed:

$ helm template charts/prefect-worker -f test.values.yaml --show-only templates/configmap.yaml | yq '.data'
Error: could not find template templates/configmap.yaml in chart
null

Deployment does not include the flag arg:

$ helm template charts/prefect-worker -f test.values.yaml --show-only templates/deployment.yaml | yq '.spec.template.spec.containers[0].args'
- prefect
- worker
- start
- --type
- "kubernetes"
- --pool
- "my-work-pool"
- --install-policy
- "prompt"

Deployment does not volume mount the ConfigMap data:

$ helm template charts/prefect-worker -f test.values.yaml --show-only templates/deployment.yaml | yq '.spec.template.spec.containers[0].volumeMounts'
- mountPath: /home/prefect
  name: scratch
  subPathExpr: home
- mountPath: /tmp
  name: scratch
  subPathExpr: tmp

Deployment does not specify the ConfigMap volume:

$ helm template charts/prefect-worker -f test.values.yaml --show-only templates/deployment.yaml | yq '.spec.template.spec.volumes'
- name: scratch
  emptyDir: {}

Copy link
Contributor

@mitchnielsen mitchnielsen left a comment

Choose a reason for hiding this comment

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

Nice 🚀

Copy link
Contributor

@mitchnielsen mitchnielsen left a comment

Choose a reason for hiding this comment

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

Ran a few of the tests from earlier with the new changes and all looks good still.

@jamiezieziula jamiezieziula merged commit 7a03179 into main May 31, 2024
16 checks passed
@jamiezieziula jamiezieziula deleted the allow-config-map-base-job-template branch May 31, 2024 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking enhancement An improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for baseJobTemplate to be passed into helmrelease as configmap
3 participants