From 435b6b57ed1ca5902339cdfd83f5fff755e62cd1 Mon Sep 17 00:00:00 2001 From: Derek Petersen Date: Mon, 18 Feb 2019 14:44:00 -0800 Subject: [PATCH] dependencygraph: Enforce shutdown order We now apply shutdown order in any dependency case, including dependsOn directives, links, or volumes. What this means is that agent will now make a best effort attempt to shutdown containers in the inverse order they were created. For example, a container using a link for communication will wait until the linked container has terminated before terminating itself. Likewise, a container named in another container's dependsOn directive will wait until that dependent container terminates. One note about the current implementation is that the dependencies aren't assumed to be transitive, so if a chain exists such that: A -> B -> C Container "C" will shutdown before "B", but it won't check status against container "A" explicity. If A depends on C, we expect: A -> B -> C A -> C The lack of transitive dependency logic applies is consistent with startup order as of this commit. --- agent/engine/dependencygraph/graph.go | 53 ++++++++-- agent/engine/dependencygraph/graph_test.go | 115 ++++++++++++++++++++- 2 files changed, 158 insertions(+), 10 deletions(-) diff --git a/agent/engine/dependencygraph/graph.go b/agent/engine/dependencygraph/graph.go index 0a728a329a1..cd2238b99f8 100644 --- a/agent/engine/dependencygraph/graph.go +++ b/agent/engine/dependencygraph/graph.go @@ -41,16 +41,9 @@ var ( ErrResourceDependencyNotResolved = errors.New("dependency graph: dependency on resources not resolved") ) -// Because a container may depend on another container being created -// (volumes-from) or running (links) it makes sense to abstract it out -// to each container having dependencies on another container being in any -// particular state set. For now, these are resolved here and support only -// volume/link (created/run) - // ValidDependencies takes a task and verifies that it is possible to allow all // containers within it to reach the desired status by proceeding in some -// order. ValidDependencies is called during DockerTaskEngine.AddTask to -// verify that a startup order can exist. +// order. func ValidDependencies(task *apitask.Task) bool { unresolved := make([]*apicontainer.Container, len(task.Containers)) resolved := make([]*apicontainer.Container, 0, len(task.Containers)) @@ -136,6 +129,10 @@ func DependenciesAreResolved(target *apicontainer.Container, return err } + if err := verifyShutdownOrder(target, nameMap); err != nil { + return err + } + return nil } @@ -339,6 +336,46 @@ func containerOrderingDependenciesIsResolved(target *apicontainer.Container, } } +func verifyShutdownOrder(target *apicontainer.Container, existingContainers map[string]*apicontainer.Container) error { + // If the target is already terminal or if it doesn't need to be terminal, shutdown order doesn't apply + if !target.DesiredTerminal() || target.KnownTerminal() { + return nil + } + + // Considered adding this to the task state, but this will be at most 45 loops, + // so we err'd on the side of having less state. + missingShutdownDependencies := []string{} + + for _, existingContainer := range existingContainers { + for _, dependency := range existingContainer.DependsOn { + // If another container declares a dependency on our target, we will want to verify that the container is + // stopped. + if dependency.Container == target.Name { + if !existingContainer.KnownTerminal() { + missingShutdownDependencies = append(missingShutdownDependencies, existingContainer.Name) + } + } + } + } + + if len(missingShutdownDependencies) == 0 { + return nil + } + + return fmt.Errorf("dependency graph: target %s needs other containers stopped before it can stop: [%s]", + target.Name, strings.Join(missingShutdownDependencies, "], [")) +} + +func verifyContainerOrderingStatus(dependsOnContainer *apicontainer.Container) bool { + dependsOnContainerDesiredStatus := dependsOnContainer.GetDesiredStatus() + // The 'target' container desires to be moved to 'Created' or the 'steady' state. + // Allow this only if the dependency container also desires to be started + // i.e it's status is any of 'Created', 'steady state' or 'Stopped' + return dependsOnContainerDesiredStatus == apicontainerstatus.ContainerCreated || + dependsOnContainerDesiredStatus == apicontainerstatus.ContainerStopped || + dependsOnContainerDesiredStatus == dependsOnContainer.GetSteadyStateStatus() +} + func onSteadyStateCanResolve(target *apicontainer.Container, run *apicontainer.Container) bool { return target.GetDesiredStatus() >= apicontainerstatus.ContainerCreated && run.GetDesiredStatus() >= run.GetSteadyStateStatus() diff --git a/agent/engine/dependencygraph/graph_test.go b/agent/engine/dependencygraph/graph_test.go index 1281070f53c..17b31fefd8e 100644 --- a/agent/engine/dependencygraph/graph_test.go +++ b/agent/engine/dependencygraph/graph_test.go @@ -26,6 +26,7 @@ import ( "github.com/aws/amazon-ecs-agent/agent/taskresource/mocks" resourcestatus "github.com/aws/amazon-ecs-agent/agent/taskresource/status" + "github.com/aws/aws-sdk-go/aws" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" ) @@ -112,7 +113,6 @@ func TestValidDependenciesWithUnresolvedReference(t *testing.T) { assert.False(t, resolveable, "Nonexistent reference shouldn't resolve") } - func TestDependenciesAreResolvedWhenSteadyStateIsRunning(t *testing.T) { task := &apitask.Task{ Containers: []*apicontainer.Container{ @@ -188,7 +188,6 @@ func TestRunDependencies(t *testing.T) { assert.NoError(t, DependenciesAreResolved(c1, task.Containers, "", nil, nil), "Dependencies should be resolved") } - func TestRunDependenciesWhenSteadyStateIsResourcesProvisionedForOneContainer(t *testing.T) { // Webserver stack php := steadyStateContainer("php", []apicontainer.DependsOn{{Container: "db", Condition: "START"}}, apicontainerstatus.ContainerRunning, apicontainerstatus.ContainerRunning) @@ -759,3 +758,115 @@ func assertContainerOrderingResolved(f func(target *apicontainer.Container, dep } } +// TestVerifyShutdownOrderTargetNotStopping validates that shutdown order returns nil when the target is running or +// already stopped +func TestVerifyShutdownOrderTargetNotStopping(t *testing.T) { + testCases := map[string]apicontainer.Container{ + "Desired running": { + Name: "one", + DesiredStatusUnsafe: apicontainerstatus.ContainerRunning, + }, + "Already Stopped": { + Name: "one", + DesiredStatusUnsafe: apicontainerstatus.ContainerStopped, + KnownStatusUnsafe: apicontainerstatus.ContainerStopped, + }, + } + + others := map[string]*apicontainer.Container{ + "two": { + Name: "two", + DependsOn: []apicontainer.DependsOn{{ + Container: "one", + }}, + }, + } + + for testName, tc := range testCases { + t.Run(testName, func(t *testing.T) { + assert.NoError(t, verifyShutdownOrder(&tc, others)) + }) + } +} + +func dependsOn(vals ...string) []apicontainer.DependsOn { + d := make([]apicontainer.DependsOn, len(vals)) + for i, val := range vals { + d[i] = apicontainer.DependsOn{Container: val} + } + return d +} + +// TestVerifyShutdownOrder validates that the shutdown graph works in the inverse order of the startup graph +// This test always uses a running target +func TestVerifyShutdownOrder(t *testing.T) { + + // dependencies aren't transitive, so we can check multiple levels within here + others := map[string]*apicontainer.Container{ + "A": { + Name: "A", + DependsOn: dependsOn("B"), + KnownStatusUnsafe: apicontainerstatus.ContainerStopped, + }, + "B": { + Name: "B", + DependsOn: dependsOn("C", "D"), + KnownStatusUnsafe: apicontainerstatus.ContainerRunning, + }, + "C": { + Name: "C", + DependsOn: dependsOn("E", "F"), + KnownStatusUnsafe: apicontainerstatus.ContainerStopped, + }, + "D": { + Name: "D", + DependsOn: dependsOn("E"), + KnownStatusUnsafe: apicontainerstatus.ContainerRunning, + }, + } + + testCases := []struct { + TestName string + TargetName string + ShouldResolve bool + }{ + { + TestName: "Dependency is already stopped", + TargetName: "F", + ShouldResolve: true, + }, + { + TestName: "Running with a running dependency", + TargetName: "D", + ShouldResolve: false, + }, + { + TestName: "One dependency running, one stopped", + TargetName: "E", + ShouldResolve: false, + }, + { + TestName: "No dependencies declared", + TargetName: "Z", + ShouldResolve: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.TestName, func(t *testing.T) { + // create a target container + target := &apicontainer.Container{ + Name: tc.TargetName, + KnownStatusUnsafe: apicontainerstatus.ContainerRunning, + DesiredStatusUnsafe: apicontainerstatus.ContainerStopped, + } + + // Validation + if tc.ShouldResolve { + assert.NoError(t, verifyShutdownOrder(target, others)) + } else { + assert.Error(t, verifyShutdownOrder(target, others)) + } + }) + } +}