Page MenuHomePhabricator

Dagster Celery K8s Job step executor

Authored by nate on Apr 17 2020, 3:48 AM.



Depends on D2787.
Depends on D2811.

Test Plan

tested manually; added buildkite tests

Diff Detail

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
nate edited the summary of this revision. (Show Details)Mon, May 4, 2:52 AM

is this still RFC status or is it ready to go? Would be good for @max to take a pass

53 ↗(On Diff #13070)

we want to keep this or was this from testing?

nate planned changes to this revision.Mon, May 4, 3:54 PM

Almost good to go, I need to fix py2 so planning changes

Harbormaster completed remote builds in B10684: Diff 13141.
nate retitled this revision from RFC: Dagster Celery K8s Job step executor to Dagster Celery K8s Job step executor.Mon, May 4, 9:49 PM
nate edited the test plan for this revision. (Show Details)
nate added a comment.Mon, May 4, 11:21 PM

At last, this is ready for review, after D2746 gets in

max added inline comments.Tue, May 5, 5:03 AM

not to be difficult but i would like to break this out into its own module, dagster_celery.tags


@alangenfeld maybe you can weigh in on whether it's worth trying to unify this with ActiveExecution, formalize some hooks, etc.


i think i agree, it'll reduce the chances of bad invocations / be an obvious place to do validation


why not just use a constant?


for future-proofing, prefer bind=True


hmm... i think this might blow up unexpectedly in horrible ways long after we've forgotten about all this, like if someone is running DinD for completely different reasons but expecting the kubconfig_file param they passed explicitly to be respected. maybe a stricter check


this all seems very tenuous. do we need to autodetect this vs setting an explicit flag in an env variable or similar?

nate updated this revision to Diff 13236.Tue, May 5, 10:47 PM
nate marked 10 inline comments as done.
nate edited the test plan for this revision. (Show Details)



cool, this sgtm!


yep - this exists now, see DagsterK8sJobConfig which wraps up the shared configuration around launching Dagster Jobs on K8s.

The job namespace and kubeconfig file ref are not included in that namedtuple, because the job definition is invariant to those two things. I'm open to adding another layer namedtuple above to bundle all three together if either of you feel strongly that such a thing should exist


yeah, fair enough - I'm replacing this with the approach in D2787, will also align the implementation here with how we do it in the RunLauncher


for future reference: planning labels refresh after this lands as a separate diff


removed :)

jordanbramble added inline comments.

obviously non-blocking, but we should probably use .format() since that is consistently used elsewhere.


Does this have to be specified when cluster provider is kind?

nate updated this revision to Diff 13343.Thu, May 7, 12:31 AM

missed some tag strings

nate updated this revision to Diff 13348.Thu, May 7, 1:41 AM


nate updated this revision to Diff 13354.Thu, May 7, 3:01 AM

speculative rebase on D2811 to fix celery test

nate updated this revision to Diff 13355.Thu, May 7, 3:34 AM

retrigger build

nate edited the summary of this revision. (Show Details)Thu, May 7, 3:49 AM
nate added a parent revision: D2811: Reduce Celery test runtime.
max resigned from this revision.Thu, May 7, 5:43 AM

this lgtm, i am resigning in favor of alex since he had objections previously

nate marked an inline comment as done.Thu, May 7, 3:29 PM
nate added inline comments.

yeah good call - this line copied from k8s tests, but I will put up a follow-up diff - I'd like to clean up all of the k8s-related test configs across dagster-k8s, dagster-airflow, and dagster-celery after this


nope! and I've just mirrored these from the dagster-k8s configs since the pytest fixtures these are associated with are defined over there

alangenfeld accepted this revision.Thu, May 7, 3:57 PM



seems pretty decent as is - most of the stuff outside of [1] is celery specific




safer if this name reflects that its a serialized value, also consider using pack and unpack value wrapped in to / from dict methods like the instance_ref_dict

This revision is now accepted and ready to land.Thu, May 7, 3:57 PM
nate updated this revision to Diff 13415.Thu, May 7, 6:47 PM
nate marked an inline comment as done.


This revision was automatically updated to reflect the committed changes.