Change the executor state machine #109
Conversation
Pull Request Test Coverage Report for Build 849
💛 - Coveralls |
Codecov Report
@@ Coverage Diff @@
## master #109 +/- ##
==========================================
+ Coverage 35.04% 35.48% +0.44%
==========================================
Files 65 65
Lines 7294 7349 +55
==========================================
+ Hits 2556 2608 +52
- Misses 4433 4436 +3
Partials 305 305
|
sargun
force-pushed
the
change-state-machine
branch
from
91546f3
to
714fe2a
Compare
sargun
force-pushed
the
change-state-machine
branch
from
714fe2a
to
8e610fd
Compare
|
btw -- coveralls ONLY looks at local tests, whereas codecov merges standalone + local tests. |
| @@ -349,6 +352,26 @@ no_launchguard: | |||
| } | |||
| } | |||
|
|
|||
| func (r *Runner) handleTaskRunningMessage(ctx context.Context, msg string, lastMessage *string, runningSent *bool, startTime time.Time, details *runtimeTypes.Details) { | |||
There was a problem hiding this comment.
This exists mostly to keep the cyclomatic complexity of the monitorContainer function low.
sargun
force-pushed
the
change-state-machine
branch
2 times, most recently
from
2d00719
to
b9a9c9b
Compare
executor/runner/runner.go
Outdated
| r.logger.Info("Status: ", titusTaskStatus.String()) | ||
| // TODO: Generate Update | ||
| r.updateStatus(ctx, titusTaskStatus, msg) | ||
| case statusMessage := <-statusChan: |
There was a problem hiding this comment.
this needs to handle statusChan being closed too, no? It seems it will be closed when we can't talk to the docker daemon anymore, in which case this will cause a panic: nil pointer. At least failing more gracefully and sending the StatusUpdate.Lost message may be better.
executor/runtime/docker/docker.go
Outdated
| defer close(statusMessageChan) | ||
| defer cancel() | ||
|
|
||
| // This context should be tied to the lifetime of the container -- it wille get significantly less broken |
There was a problem hiding this comment.
minor: typo in it wille?
| } | ||
| case "health_status": | ||
| statusMessageChan <- runtimeTypes.StatusMessage{ | ||
| Status: runtimeTypes.StatusRunning, |
There was a problem hiding this comment.
add a Msg here indicating the change in health_status?
sargun
force-pushed
the
change-state-machine
branch
from
517ff28
to
1e71bfc
Compare
| } | ||
|
|
||
| r.metrics.Timer("titus.executor.dockerStartTime", time.Since(dockerStartStartTime), c.ImageTagForMetrics()) | ||
|
|
||
| if c.Allocation.IPV4Address == "" { |
There was a problem hiding this comment.
Should we check c.Allocation.ENI or DeviceIndex, as well?
There was a problem hiding this comment.
When you're running this in "old network driver mode" those are unset.
| return | ||
| } else if details == nil { | ||
| r.logger.Error("Unable to fetch task details") | ||
| if details == nil { |
There was a problem hiding this comment.
Worth checking this before err != nil above? Otherwise, we just updated a bunch of status with nil details.
There was a problem hiding this comment.
Nah -- because details might be nil, for example, if we weren't able to start the image, because of a bad entrypoint. This here is more a sanity-check, and should never happen.
There was a problem hiding this comment.
-- In those cases, the error will get reported back to the user, and the program wont shut down entirely.
No description provided.