Page MenuHomeElementl

k8s executor client config
ClosedPublic

Authored by johann on Jul 7 2021, 8:20 PM.

Details

Summary

Add proper kubeconfig options to the k8s executor, and allow a mocked k8s client to be inserted. Add a check that the k8s run launcher is in use (this is currently required because we read config from the launcher. If a user requested to use this with another launcher, we could loosen the requirement)

Test Plan

Integration, mocked test

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.Jul 7 2021, 8:44 PM
Harbormaster failed remote builds in B33350: Diff 41133!
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 7 2021, 9:59 PM
Harbormaster failed remote builds in B33365: Diff 41149!
johann edited the test plan for this revision. (Show Details)
johann published this revision for review.Jul 7 2021, 10:39 PM

why don't we use DagsterKubernetesClient for this k8s stuff?

python_modules/libraries/dagster-k8s/dagster_k8s/executor.py
113–115

why default away these args? prefer explicit setting at each callsite for clarity

python_modules/libraries/dagster-k8s/dagster_k8s/executor.py
121–135

is there a reason to create a new BatchV1Api() each time versus just holding on to one at __init__ time?

Would make sense to move to the DagsterKubernetesClient, that said we don't currently use it directly in either of the k8s launchers or the celery k8s executor. So I think it's a general refactor to move our classes away from using the k8s lib directly

python_modules/libraries/dagster-k8s/dagster_k8s/executor.py
121–135

Python k8s clients can expire if they're kept around- they don't know to refresh their authentication. Same pattern as the launcher https://github.com/dagster-io/dagster/blob/master/python_modules/libraries/dagster-k8s/dagster_k8s/launcher.py#L143

ah only in the celery worker interesting

This revision is now accepted and ready to land.Jul 8 2021, 5:46 PM
This revision was landed with ongoing or failed builds.Jul 8 2021, 6:28 PM
This revision was automatically updated to reflect the committed changes.