Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check task dependencies before processing the task #796

Merged
merged 1 commit into from
May 24, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions agent/engine/docker_task_engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/api"
"github.com/aws/amazon-ecs-agent/agent/config"
"github.com/aws/amazon-ecs-agent/agent/credentials"
"github.com/aws/amazon-ecs-agent/agent/engine/dependencygraph"
"github.com/aws/amazon-ecs-agent/agent/engine/dockerclient"
"github.com/aws/amazon-ecs-agent/agent/engine/dockerstate"
"github.com/aws/amazon-ecs-agent/agent/eventstream"
Expand Down Expand Up @@ -446,12 +447,25 @@ func (engine *DockerTaskEngine) AddTask(task *api.Task) error {

existingTask, exists := engine.state.TaskByArn(task.Arn)
if !exists {
// This will update the container desired status
task.UpdateDesiredStatus()

engine.state.AddTask(task)
engine.startTask(task)
} else {
engine.updateTask(existingTask, task)
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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rephrase this? "Unable to progress task with circular dependencies"

task.SetKnownStatus(api.TaskStopped)
task.SetDesiredStatus(api.TaskStopped)
err := TaskDependencyError{task.Arn}
engine.emitTaskEvent(task, err.Error())
}
return nil
}

// Update task
engine.updateTask(existingTask, task)

return nil
}

Expand Down
25 changes: 25 additions & 0 deletions agent/engine/docker_task_engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1146,3 +1146,28 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a unit test in dependencygraph for ValidDependencies() to test a task with circular dependencies?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this sort of cyclical dependency is being tested here.

// dependencies can't be resolved
func TestTaskWithCircularDependency(t *testing.T) {
ctrl, client, _, taskEngine, _, _ := mocks(t, &defaultConfig)
defer ctrl.Finish()

client.EXPECT().Version().Return("1.12.6", nil)
client.EXPECT().ContainerEvents(gomock.Any())

task := testdata.LoadTask("circular_dependency")

taskEngine.Init()
events := taskEngine.StateChangeEvents()

go taskEngine.AddTask(task)

event := <-events
assert.Equal(t, event.(api.TaskStateChange).Status, api.TaskStopped, "Expected task to move to stopped directly")
_, ok := taskEngine.(*DockerTaskEngine).state.TaskByArn(task.Arn)
assert.True(t, ok, "Task state should be added to the agent state")

_, ok = taskEngine.(*DockerTaskEngine).managedTasks[task.Arn]
assert.False(t, ok, "Task should not be added to task manager for processing")
}
15 changes: 15 additions & 0 deletions agent/engine/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,21 @@ func (CannotGetDockerClientError) ErrorName() string {
return "CannotGetDockerclientError"
}

// TaskDependencyError is the error for task that dependencies can't
// be resolved
type TaskDependencyError struct {
taskArn string
}

func (err TaskDependencyError) Error() string {
return "Task dependencies cannot be resolved" + err.taskArn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need a space at the end here or you'll end up with "resolvedarn:"

}

// ErrorName is the name of the error
func (err TaskDependencyError) ErrorName() string {
return "TaskDependencyError"
}

// TaskStoppedBeforePullBeginError is a type for task errors involving pull
type TaskStoppedBeforePullBeginError struct {
taskArn string
Expand Down
25 changes: 25 additions & 0 deletions agent/engine/testdata/test_tasks/circular_dependency.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
{
"Arn":"arn:aws:ecs:us-west-2:123456789012:task/12345678-90ab-cdef-1234-56780abcdef1-circular-dependency",
"Family":"circular-dependency",
"Version":"2",
"Containers":
[{
"Name": "web",
"Image": "busybox",
"Command": ["sleep", "1000"],
"Links":["web-db:web-db"]
},
{
"Name": "web-db",
"Image": "busybox",
"Command": ["sleep", "1000"],
"volumesFrom": [{
"sourceContainer": "web"
}]
}],
"volumes": [],
"DesiredStatus": "RUNNING",
"KnownStatus": "NONE",
"KnownTime": "0001-01-01T00:00:00Z",
"SentStatus": "NONE"
}