Page MenuHomeElementl

[1/2] Add helm configuration for dagster compute log manager
ClosedPublic

Authored by rexledesma on Dec 22 2020, 6:52 PM.

Details

Summary

We add configuration in our helm chart to set the compute log manager
for the dagster instance. In a subsequent PR, we will enforce type
constraints using the schema.

Test Plan

integration

The following commands template as expected:

helm template test . -s templates/configmap-instance.yaml --set "computeLogManager.type=AzureBlobComputeLogManager"
helm template test . -s templates/configmap-instance.yaml --set "computeLogManager.type=GCSComputeLogManager"
helm template test . -s templates/configmap-instance.yaml --set "computeLogManager.type=S3ComputeLogManager"

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 22 2020, 7:07 PM
Harbormaster failed remote builds in B23345: Diff 28396!
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 22 2020, 7:43 PM
Harbormaster failed remote builds in B23352: Diff 28405!
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 28 2020, 10:12 PM
Harbormaster failed remote builds in B23506: Diff 28581!
Harbormaster returned this revision to the author for changes because remote builds failed.Jan 5 2021, 7:31 PM
Harbormaster failed remote builds in B23648: Diff 28759!

is there an escape hatch here for providing a custom one?

helm/dagster/templates/configmap-instance.yaml
143–154

may be best to allow this to remain empty and assume default behavior if not specified

helm/dagster/templates/helpers/_compute-log-manager.tpl
36–42

hmm how does this loading work when some of these options are not set? I believe the config schema is setup for these to be keys to be omitted if there isn't a valid value and may choke

the full custom alternative path is less of a concern since that is unlikely - but I do worry that the config setup does not actually work if attempted to be loaded given the provided test plan

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

may be safer to go with a more generic config section like this so things can be omitted more reasonably

helm/dagster/values.yaml
108

what is the behavior of ~ ?

rexledesma marked an inline comment as done.

up

remove fields from configurable class config if optional

im still worried about keeping these config fields up to date - but ill let you make the final call

lets get this set up on the demo cluster right away to ensure it works at runtime

This revision is now accepted and ready to land.Jan 7 2021, 4:46 PM
rexledesma edited the test plan for this revision. (Show Details)