Page MenuHomeElementl

Move termination status checks from launcher to instance
AbandonedPublic

Authored by johann on Jul 30 2021, 5:11 PM.

Details

Summary

Maybe rather than having the status checks at all (this diff), we should just implement the k8s can terminate check? Due to the negative effect listed below


Unify can_terminate logic for run_launchers-

Currently the k8s run launchers look at pipeline status, while the others use their specific api. This removes these pipeline status concerns from the run launchers and places them on the instance, so that the launchers purely interact with their specific api. This helps in cases where only the api calls are desired.

Negative effects

  • in rare cases, this change may make some runs unable to terminate without force set- e.g. if docker run launcher starts a container but the container doesn't get far enough to emit started, it could currently still be terminated.

TODO

  • haven't done a thorough review of the other launchers
Test Plan

Integration

Diff Detail

Repository
R1 dagster
Branch
terminate-check-status (branched from master)
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

johann published this revision for review.Jul 30 2021, 5:21 PM
johann edited the summary of this revision. (Show Details)
johann edited the summary of this revision. (Show Details)
johann added reviewers: dgibson, alangenfeld.
dgibson requested changes to this revision.Jul 30 2021, 6:21 PM

i like the idea of implementing the k8s termination check an a way that's akin to the other launchers, vs. affecting the other launchers

This revision now requires changes to proceed.Jul 30 2021, 6:21 PM