Page MenuHomePhabricator

[dagster-airflow] Fix k8s pod operator in dagster-airflow and get it under test
ClosedPublic

Authored by nate on Fri, Nov 15, 12:08 AM.

Details

Summary

well, it only took me 30 diffs, but this is now working and ready for review.

This diff fixes the Dagster k8s operator and provides a kind-based unit test, getting it under test.

To speed up this test, I've also moved the Docker image build step out to the nightly; it was getting extremely slow.

Test Plan

unit

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

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, Nov 18, 12:26 AM
nate added a reviewer: Restricted Project.
alangenfeld added inline comments.Mon, Nov 18, 4:10 PM
.buildkite/pipeline.py
229–230

later

is later before landing or something else? Seems worth taking care of before committing

236–237

i think this wont run if the tests fail - is that a problem?

alangenfeld requested changes to this revision.Mon, Nov 18, 4:17 PM
alangenfeld added inline comments.
.buildkite/nightly.py
39–65

what exactly is in these images? I feel like this might introduce some weird corner case problems.

Either way I think this is a substantial enough change it should be pulled in to its own diff - it shouldn't block the rest of this diff

python_modules/dagster-airflow/dagster_airflow/operators/kubernetes_operator.py
252–254

rm debug

This revision now requires changes to proceed.Mon, Nov 18, 4:17 PM
nate marked 2 inline comments as done.Tue, Nov 19, 5:19 AM
nate added inline comments.
.buildkite/nightly.py
39–65

trying depends_on in the main pipeline, maybe this has an easy solution

.buildkite/pipeline.py
229–230

Moved to build image, removed

236–237

yeah I was thinking about this. not sure if there's a better way to do this cleanup. in the failure case, the "cluster" container will stay alive for the lifecycle of the host, definitely not ideal. any ideas?

python_modules/dagster-airflow/dagster_airflow/operators/kubernetes_operator.py
252–254

thx, done

nate updated this revision to Diff 6707.Tue, Nov 19, 5:20 AM
nate marked 2 inline comments as done.

comments

nate planned changes to this revision.Tue, Nov 19, 5:39 AM
nate added inline comments.
.buildkite/pipeline.py
207

depends_on works!

TODO for self tomorrow:

  1. this should push/pull from ECR not docker hub (so that these are private)
  2. need to tag each build w/ a build ID so that the right images are used; rn everything is clobbering the latest tag
alangenfeld added inline comments.Tue, Nov 19, 3:56 PM
.buildkite/pipeline.py
236–237

one way is to move it to a bash script with a trap cleanup function

nate edited reviewers, added: max; removed: Restricted Project.Tue, Nov 19, 4:12 PM
nate updated this revision to Diff 6714.Tue, Nov 19, 6:13 PM

address TODOs

alangenfeld accepted this revision.Tue, Nov 19, 11:45 PM

verygood

.buildkite/pipeline.py
645

make this look like the rest for readability

This revision is now accepted and ready to land.Tue, Nov 19, 11:45 PM
nate updated this revision to Diff 6728.Wed, Nov 20, 12:19 AM

comments