Details

Reviewers

Commits

R1:0095b7ca04fe: Adds values.yaml fields for user-deployment liveness and startup probe.

Summary

Previously, the liveness and startup probe for user-deployment pods was not exposed in the Dagster helm chart's values.yaml.

The defaults used were:

livenessProbe:
  # ...
  periodSeconds: 20
  timeoutSeconds: 3

startupProbe:
  # ...
  periodSeconds: 10
  timeoutSeconds: 3

This diff will keep these defaults, but expose them through the values.yaml. The following fields are now also exposed:

initialDelaySeconds
successThreshold
failureThreshold

Test Plan

integration
bk

Run helm install on GCP elementl-dev cluster, check that pods and services are all helathy, and check that livenessProbe and startupProbe config is passed correctly (via kubectl describe pod).

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

bob created this revision.Dec 8 2020, 4:39 PM

Harbormaster completed remote builds in B22396: Diff 27223.Dec 8 2020, 4:58 PM

bob requested review of this revision.Dec 8 2020, 4:58 PM

Can we apply a helm schema to this as well?

This revision now requires changes to proceed.Dec 9 2020, 3:47 AM

Adds value.schema for user deployments.

Harbormaster failed remote builds in B22564: Diff 27429!Dec 10 2020, 6:10 PM

left a couple of comments, should be good after a rebase!

helm/dagster/templates/deployment-user.yaml
101–128	[1]
130–156	[2]
python_modules/automation/automation/helm/schema/subschema/userdeployments.py
34–35	nit: since the livenessProbe/startupProbe exists prepopulated in `values.yaml`, it doesn't make sense to make this optional. If we make this required, then we can make [1], [2] super clean.

Sets livenessProbe and startupProbe as required on userDeployments.

Harbormaster failed remote builds in B22579: Diff 27448!Dec 10 2020, 8:07 PM

bob removed reviewers: catherinewu, alangenfeld, rexledesma.Dec 10 2020, 8:53 PM

Hey, I don't think we should make livenessProbe and startupProbe required -- some folks may want to use a different method of monitoring their deployments. Moreover, startupProbe is not available on all kubernetes versions (only available starting 1.16+) so this would break folks on older versions.

I guess this shows that we should probably add testing for 1.15...

This revision now requires changes to proceed.Dec 11 2020, 4:45 AM

Make livenessProbe and startupProbe optional for userdeployments.

Harbormaster failed remote builds in B22774: Diff 27694!Dec 14 2020, 3:59 PM

rebase

Harbormaster completed remote builds in B22781: Diff 27705.Dec 14 2020, 4:52 PM

Marks probes as optional in user deployments.
Tmp: Enables integration tests in BuildKite.

Harbormaster failed remote builds in B22810: Diff 27746!Dec 14 2020, 9:22 PM

Might be missing something, but I think adding integration on its own line under Test Plan should run the integration tests? you can also go to https://buildkite.com/dagster/dagster-integration-tests/builds/, click “New Build”, and enter the commit hash

catherinewu added inline comments.Dec 15 2020, 6:40 AM

helm/dagster/templates/deployment-user.yaml
128	does this make `startupProbe` required?
129	`{{- if hasKey $deployment "startupProbe" }}`?

Removes integration tests from BuildKite pipeline.
Fixes templating for startupProbe on user deployments.

bob edited the test plan for this revision. (Show Details)Dec 15 2020, 5:30 PM

Harbormaster failed remote builds in B22878: Diff 27827!Dec 15 2020, 6:21 PM

might need to kick off BK again? also i don't see the BK integration test suite?

rebase on master

Harbormaster failed remote builds in B23216: Diff 28231!Dec 18 2020, 8:07 PM

rebase on master

catherinewu added inline comments.Jan 4 2021, 6:08 PM

helm/dagster/templates/deployment-user.yaml
103	are users able to opt out of using a livenessProbe?

Harbormaster failed remote builds in B23558: Diff 28643!Jan 4 2021, 6:13 PM

bob added inline comments.Jan 4 2021, 6:54 PM

helm/dagster/templates/deployment-user.yaml
103	no, i don't believe so. currently, the livenessProbe here is required i can move the if-statement around to make the livenessProbe optional. this would make the default livenessProbe live in `values.yaml`, and would need to use a hard-coded port, since I don't think I can access `$deployment.port` from the `values.yaml`. But, hard-coding the port shouldn't be too big of an issue imo