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

@@ -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()

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))
+			}
+		})
+	}
+}