Check task dependencies before processing the task #796
Conversation
| @@ -1146,3 +1146,27 @@ func TestEngineDisableConcurrentPull(t *testing.T) { | |||
| assert.False(t, dockerTaskEngine.enableConcurrentPull, | |||
| "Task engine should not be able to perform concurrent pulling for version < 1.11.1") | |||
| } | |||
|
|
|||
| // TestTaskWithCircularDependency tests the task with containers of which the | |||
There was a problem hiding this comment.
Can you also add a unit test in dependencygraph for ValidDependencies() to test a task with circular dependencies?
There was a problem hiding this comment.
Weren't this have already tested it: https://github.com/aws/amazon-ecs-agent/blob/master/agent/engine/dependencygraph/graph_test.go#L48
There was a problem hiding this comment.
Indeed, this sort of cyclical dependency is being tested here.
agent/engine/docker_task_engine.go
Outdated
| seelog.Errorf("Task can not move forward due to the dependencies of containers cannot be resolved, task:%s", task.String()) | ||
| task.SetKnownStatus(api.TaskStopped) | ||
| task.SetDesiredStatus(api.TaskStopped) | ||
| engine.emitTaskEvent(task, "TaskTransitionError: Can not resolve the dependencies of containers in the task definition") |
There was a problem hiding this comment.
can you use the impossibleTransitionError type here instead?
agent/engine/docker_task_engine.go
Outdated
| if dependencygraph.ValidDependencies(task) { | ||
| engine.startTask(task) | ||
| } else { | ||
| seelog.Errorf("Task can not move forward due to the dependencies of containers cannot be resolved, task:%s", task.String()) |
There was a problem hiding this comment.
We should either use can not or cannot. Let's not use both. I'd vote for cannot. Also, can you leave a space after task: ?
agent/engine/docker_task_engine.go
Outdated
| engine.state.AddTask(task) | ||
| engine.startTask(task) | ||
| if dependencygraph.ValidDependencies(task) { | ||
| engine.startTask(task) |
There was a problem hiding this comment.
you can also just return nil here and get rid of else in line 464.
There was a problem hiding this comment.
@aaithal Can you explain that? There is no codepath where we could call AddTask on an existing task?
There was a problem hiding this comment.
I think @aaithal means get rid of the else word but keeps the statement inside the else, see my updates.
agent/engine/docker_task_engine.go
Outdated
| engine.state.AddTask(task) | ||
| engine.startTask(task) | ||
| if dependencygraph.ValidDependencies(task) { | ||
| engine.startTask(task) |
There was a problem hiding this comment.
@aaithal Can you explain that? There is no codepath where we could call AddTask on an existing task?
agent/engine/docker_task_engine.go
Outdated
| seelog.Errorf("Task can not move forward due to the dependencies of containers cannot be resolved, task:%s", task.String()) | ||
| task.SetKnownStatus(api.TaskStopped) | ||
| task.SetDesiredStatus(api.TaskStopped) | ||
| engine.emitTaskEvent(task, "TaskTransitionError: Can not resolve the dependencies of containers in the task definition") |
There was a problem hiding this comment.
Cannot vs. Can not. Cannot is "more correct"
richardpen
force-pushed
the
issue-756
branch
2 times, most recently
from
1a9e032
to
2c88b2f
Compare
|
@richardpen these changes look good to me |
agent/engine/docker_task_engine.go
Outdated
| seelog.Errorf("Task cannot move forward due to the dependencies of containers cannot be resolved, task: %s", task.String()) | ||
| task.SetKnownStatus(api.TaskStopped) | ||
| task.SetDesiredStatus(api.TaskStopped) | ||
| engine.emitTaskEvent(task, "impossibleTransitionError: Cannot resolve the dependencies of containers in the task definition") |
There was a problem hiding this comment.
Apologies, I should have made this clearer in my earlier comment. Please use the impossibleTransitionError type instead:
amazon-ecs-agent/agent/engine/errors.go
Line 35 in f88e52e
agent/engine/errors.go
Outdated
|
|
||
| // ErrorName is the name of the error | ||
| func (err TaskCircularDependencyError) ErrorName() string { | ||
| return "TAskCircularDependencyError" |
agent/engine/errors.go
Outdated
| @@ -112,6 +112,21 @@ func (CannotGetDockerClientError) ErrorName() string { | |||
| return "CannotGetDockerclientError" | |||
| } | |||
|
|
|||
| // TaskCircularDependencyError is the error for task that dependencies can't | |||
| // be resolved | |||
| type TaskCircularDependencyError struct { | |||
There was a problem hiding this comment.
I wonder if TaskDependencyError is a better type name. Because, the ValidDependencies method might fail for other reasons as well.
| if dependencygraph.ValidDependencies(task) { | ||
| engine.startTask(task) | ||
| } else { | ||
| seelog.Errorf("Task cannot move forward due to the dependencies of containers cannot be resolved, task: %s", task.String()) |
There was a problem hiding this comment.
Can you rephrase this? "Unable to progress task with circular dependencies"
| } | ||
|
|
||
| func (err TaskDependencyError) Error() string { | ||
| return "Task dependencies cannot be resolved" + err.taskArn |
There was a problem hiding this comment.
You need a space at the end here or you'll end up with "resolvedarn:"
Summary
Fix #756 Check the task dependencies before processing the task and provide a more clear log message.
Implementation details
Check the dependencies before processing the task in task engine.
Testing
make release)go build -out amazon-ecs-agent.exe ./agent)make test) passgo test -timeout=25s ./agent/...) passmake run-integ-tests) pass.\scripts\run-integ-tests.ps1) passmake run-functional-tests) pass.\scripts\run-functional-tests.ps1) passNew tests cover the changes:
yes
Description for the changelog
Licensing
This contribution is under the terms of the Apache 2.0 License:
yes