Page MenuHomeElementl

Adds values.yaml fields for user-deployment liveness and startup probe.
ClosedPublic

Authored by bob on Dec 8 2020, 4:39 PM.

Details

Summary

Intended to resolve GitHub issue #3305.

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
Branch
bob--user-deployment-liveness-timeout
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

rexledesma added a subscriber: rexledesma.

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.

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

helm/dagster/templates/deployment-user.yaml
90–98

[1]

100–106

[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.

catherinewu added a subscriber: catherinewu.

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.

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

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

helm/dagster/templates/deployment-user.yaml
95–98

does this make startupProbe required?

96

{{- if hasKey $deployment "startupProbe" }}?

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

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

helm/dagster/templates/deployment-user.yaml
92

are users able to opt out of using a livenessProbe?

helm/dagster/templates/deployment-user.yaml
92

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

Show all probe config for user-deployments in values.yaml, and make probes optional in templates.

Adds replicaCount field to user-deployment integration tests.

Adds user-deployment replica count for daemon integration test.

Sets default probe in template YAML instead of values.yaml.

This revision is now accepted and ready to land.Jan 6 2021, 10:55 PM