Page MenuHomePhabricator

[2/2] Expose config for k8s run launcher in helm chart
ClosedPublic

Authored by rexledesma on Oct 20 2020, 9:20 PM.

Details

Summary

As the title.

Test Plan

bk, helm

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 20 2020, 9:44 PM
Harbormaster failed remote builds in B19878: Diff 24122!
rexledesma retitled this revision from Expose config for k8s run launcher in helm chart to [2/2] Expose config for k8s run launcher in helm chart.Oct 20 2020, 11:08 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 20 2020, 11:34 PM
Harbormaster failed remote builds in B19898: Diff 24147!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 21 2020, 12:29 AM
Harbormaster failed remote builds in B19901: Diff 24150!
catherinewu added inline comments.
helm/dagster/templates/configmap-instance.yaml
84

do we need this with statement? would be good to reduce statement nesting if possible, especially for the helm chart since it's already a bit difficult to read

helm/dagster/values.yaml
270

I think we should describe this as using the K8sRunLauncher as opposed to the default CeleryK8sRunLauncher; I think describing this as the "celery-less run launcher" is less precise since there are actually a few run launchers that are "celery-less". Also, we should enable this run launcher here instead of within the celery config and then assert that if the k8s run launcher is enabled, that celery is not enabled. This is much more discoverable for a user who is reading the high level sections and wants to know how to enable this run launcher or who is reading the file and wants to know whether the config specified in this section is being used

272

would prefer to use camel case since most of the helm chart is in camel case. opening a GH issue (https://github.com/dagster-io/dagster/issues/3129) to standardize the casing

284

tilde? would prefer this value to be the same format as elsewhere kubeconfigFile: ""

This revision now requires changes to proceed.Oct 21 2020, 2:57 PM
rexledesma added inline comments.
helm/dagster/values.yaml
270

assert that if the k8s run launcher is enabled, that celery is not enabled.

I'll put in this assertion for now, and fail if they're both enabled. Obviously, not an ideal solution, but restructuring the helm file via this proposal will make it easier to maintain this in the future.

284

The default value for kubeconfig_file in the launcher is actually None, which is why i preferred ~ over "". But looks like they're interpreted the same way via the configurable class so it doesn't really matter

rexledesma marked 2 inline comments as done.

up

helm/dagster/templates/configmap-instance.yaml
61

i think this should just check whether .Values.k8sRunLauncher.enabled is enabled and the "not (.Values.celery.enabled and .Values.k8sRunLauncher.enabled)" should be checked elsewhere

helm/dagster/values.yaml
201

I would perhaps word this as "When enabling the K8sRunLauncher, make sure to set celery.enabled to be false"

201–202

"Celery run launcher" -> "CeleryK8sRunLauncher"

python_modules/libraries/dagster-k8s/dagster_k8s/launcher.py
60

hmm, this part isn't true anymore. We should remove: This image will be run with the command dagster-graphql -p startPipelineExecution -v {executionParams}`. It uses 'dagster api execute_run_with_structured_logs' instead

rexledesma marked 3 inline comments as done.

up

This revision is now accepted and ready to land.Oct 22 2020, 8:23 PM
This revision was landed with ongoing or failed builds.Oct 22 2020, 9:09 PM
This revision was automatically updated to reflect the committed changes.