From 4db377d7e223f5df3219f165163460a9a0a4d1d0 Mon Sep 17 00:00:00 2001 From: Amogh Rathore Date: Thu, 15 Aug 2024 01:11:03 +0000 Subject: [PATCH 1/8] Add a test to ensure that repo-interacting Docker calls happen after network pause container is RUNNING --- agent/api/task/task.go | 2 +- agent/engine/docker_task_engine_test.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/agent/api/task/task.go b/agent/api/task/task.go index 6a19fb0772c..94eeae32b09 100644 --- a/agent/api/task/task.go +++ b/agent/api/task/task.go @@ -1491,7 +1491,7 @@ func (task *Task) addNetworkResourceProvisioningDependencyAwsvpc(cfg *config.Con if container.IsInternal() { continue } - container.BuildContainerDependency(NetworkPauseContainerName, apicontainerstatus.ContainerResourcesProvisioned, apicontainerstatus.ContainerPulled) + container.BuildContainerDependency(NetworkPauseContainerName, apicontainerstatus.ContainerResourcesProvisioned, apicontainerstatus.ContainerManifestPulled) pauseContainer.BuildContainerDependency(container.Name, apicontainerstatus.ContainerStopped, apicontainerstatus.ContainerStopped) } diff --git a/agent/engine/docker_task_engine_test.go b/agent/engine/docker_task_engine_test.go index be6838a51d5..29b0f3fcbcc 100644 --- a/agent/engine/docker_task_engine_test.go +++ b/agent/engine/docker_task_engine_test.go @@ -41,6 +41,7 @@ import ( "github.com/aws/amazon-ecs-agent/agent/dockerclient" "github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi" mock_dockerapi "github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi/mocks" + "github.com/aws/amazon-ecs-agent/agent/ecscni" mock_ecscni "github.com/aws/amazon-ecs-agent/agent/ecscni/mocks" "github.com/aws/amazon-ecs-agent/agent/engine/dockerstate" "github.com/aws/amazon-ecs-agent/agent/engine/execcmd" @@ -67,12 +68,13 @@ import ( "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/appmesh" ni "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/networkinterface" mock_ttime "github.com/aws/amazon-ecs-agent/ecs-agent/utils/ttime/mocks" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/secretsmanager" "github.com/aws/aws-sdk-go/service/ssm" cniTypesCurrent "github.com/containernetworking/cni/pkg/types/100" "github.com/docker/docker/api/types" + dockerapitypes "github.com/docker/docker/api/types" + dockerapitypescontainer "github.com/docker/docker/api/types/container" dockercontainer "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/network" "github.com/docker/docker/api/types/registry" From 1280ef0ce4c2ebe2e4a87cdeecde85ce8bf67967 Mon Sep 17 00:00:00 2001 From: Amogh Rathore Date: Thu, 15 Aug 2024 15:30:05 +0000 Subject: [PATCH 2/8] Move the new test to Linux specific file and update a corresponding Windows test --- agent/engine/docker_task_engine_linux_test.go | 281 +++++++++++++++++- agent/engine/docker_task_engine_test.go | 3 - .../engine/docker_task_engine_windows_test.go | 33 +- 3 files changed, 311 insertions(+), 6 deletions(-) diff --git a/agent/engine/docker_task_engine_linux_test.go b/agent/engine/docker_task_engine_linux_test.go index c3d0a85eb34..01ed71de436 100644 --- a/agent/engine/docker_task_engine_linux_test.go +++ b/agent/engine/docker_task_engine_linux_test.go @@ -20,6 +20,7 @@ import ( "encoding/json" "errors" "fmt" + "net" "os" "strconv" "strings" @@ -58,10 +59,11 @@ import ( "github.com/aws/amazon-ecs-agent/ecs-agent/credentials" "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/appmesh" ni "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/networkinterface" - "github.com/aws/aws-sdk-go/aws" cniTypesCurrent "github.com/containernetworking/cni/pkg/types/100" "github.com/docker/docker/api/types" + dockerapitypes "github.com/docker/docker/api/types" + dockerapitypescontainer "github.com/docker/docker/api/types/container" dockercontainer "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/network" "github.com/docker/docker/api/types/registry" @@ -1582,3 +1584,280 @@ func TestCredentialSpecResourceTaskFile(t *testing.T) { ret := taskEngine.(*DockerTaskEngine).createContainer(testTask, testTask.Containers[0]) assert.Nil(t, ret.Error) } + +// Tests that any repo-interacting Docker calls are made by the Task Engine after +// the pause container (for awsvpc tasks) has reached ContainerResourcesProvisioned state. +// +// The test adds a simple awsvpc task to the task engine and then verifies that +// any DockerClient calls that interact with an image repository (PullContainerManifest +// and PullContainer, currently) happen after the pause container has reached +// ContainerResourcesProvisioned (RUNNING) state. +// +// If you are updating this test then make sure that you call assertPauseContainerIsRunning() +// in any dockerClient expected calls that are supposed to interact with an image repository. +func TestRepoInteractionAgainstPauseContainerState(t *testing.T) { + // A test task + image := "image" + task := &apitask.Task{ + Containers: []*apicontainer.Container{ + { + Image: image, + Name: "container", + TransitionDependenciesMap: map[apicontainerstatus.ContainerStatus]apicontainer.TransitionDependencySet{}, + Essential: true, + }, + }, + Arn: testTaskARN, + DesiredStatusUnsafe: apitaskstatus.TaskRunning, + NetworkMode: apitask.AWSVPCNetworkMode, + } + + // Set up task engine and mocks + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + cfg := config.DefaultConfig() + cfg.TaskCPUMemLimit.Value = config.ExplicitlyDisabled + ctrl, _, mockTime, taskEngine, _, imageManager, _, serviceConnectManager := mocks(t, ctx, &cfg) + defer ctrl.Finish() + dockerClient := mock_dockerapi.NewMockDockerClient(ctrl) + cniClient := mock_ecscni.NewMockCNIClient(ctrl) + taskEngine.(*DockerTaskEngine).client = dockerClient + taskEngine.(*DockerTaskEngine).cniClient = cniClient + + // Expectations for ServiceConnectManager - loading of AppNet container image + serviceConnectManager.EXPECT().GetAppnetContainerTarballDir().AnyTimes().Return("") + serviceConnectManager.EXPECT(). + LoadImage(gomock.Any(), gomock.Any(), gomock.Any()). + AnyTimes() + + // time.Now() is called to record certain timestamps but we don't care about + // that for this test + mockTime.EXPECT().Now().AnyTimes().Return(time.Now()) + + // A function to assert that the network pause container in the task is in + // ContainerResourcesProvisioned state. This will be used by dockerClient mock later. + assertPauseContainerIsRunning := func() { + assert.Len(t, task.Containers, 2, "expected pause container to be populated") + pauseContainer := task.Containers[1] + assert.Equal(t, apitask.NetworkPauseContainerName, pauseContainer.Name) + assert.Equal(t, apicontainer.ContainerCNIPause, pauseContainer.Type) + assert.Equal(t, + apicontainerstatus.ContainerResourcesProvisioned, + pauseContainer.GetKnownStatus(), + "expected pause container to be running before image repository is called") + } + + // Set expectations on mocks for containers transition to CREATED and RUNNING + eventStream := make(chan dockerapi.DockerContainerChangeEvent) + dockerClient.EXPECT().ContainerEvents(gomock.Any()).Return(eventStream, nil) + imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() + + // To record container container state transition expectations + transitionExpectations := []*gomock.Call{} + + // To track asynchronous sending of Docker events for container create and start + var pauseContainerDockerEventsSent sync.WaitGroup + + // State transition expectations for the pause container + transitionExpectations = append(transitionExpectations, + dockerClient.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil), + dockerClient.EXPECT(). + CreateContainer(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Do( + func(ctx interface{}, config *dockercontainer.Config, y interface{}, + containerName string, z time.Duration, + ) { + pauseContainerDockerEventsSent.Add(1) + go func() { + eventStream <- createDockerEvent(apicontainerstatus.ContainerCreated) + pauseContainerDockerEventsSent.Done() + }() + }). + Return(dockerapi.DockerContainerMetadata{DockerID: "pauseContainer"}), + dockerClient.EXPECT(). + StartContainer(gomock.Any(), "pauseContainer", cfg.ContainerStartTimeout). + Do( + func(ctx interface{}, id string, timeout time.Duration) { + // Simulate some startup time + time.Sleep(5 * time.Millisecond) + pauseContainerDockerEventsSent.Wait() + pauseContainerDockerEventsSent.Add(1) + go func() { + eventStream <- createDockerEvent(apicontainerstatus.ContainerRunning) + pauseContainerDockerEventsSent.Done() + }() + }). + Return(dockerapi.DockerContainerMetadata{DockerID: containerID}), + dockerClient.EXPECT(). + InspectContainer(gomock.Any(), "pauseContainer", gomock.Any()). + Return(&dockerapitypes.ContainerJSON{ + ContainerJSONBase: &dockerapitypes.ContainerJSONBase{ + State: &dockerapitypes.ContainerState{Pid: 5}, + HostConfig: &dockerapitypescontainer.HostConfig{NetworkMode: "none"}, + }, + }, nil), + cniClient.EXPECT(). + SetupNS(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&cniTypesCurrent.Result{ + IPs: []*cniTypesCurrent.IPConfig{ + {Address: net.IPNet{IP: net.IPv4(127, 0, 0, 1)}}, + }, + }, nil), + ) + + // To track asynchronous sending of Docker events for container create and start + var taskContainerDockerEventsSent sync.WaitGroup + + // State transition expectations for the task container + // + // Any expected dockerClient method calls that would interact with the image repository + // must call assertPauseContainerIsRunning() function to ensure that the pause container + // is RUNNING before the method call. + transitionExpectations = append(transitionExpectations, + // Expectations for transition to MANIFEST_PULLED + dockerClient.EXPECT(). + WithVersion(dockerclient.Version_1_35). + Return(dockerClient, nil), + dockerClient.EXPECT().PullImageManifest(gomock.Any(), image, nil). + Do(func(context.Context, string, *apicontainer.RegistryAuthenticationData) { + assertPauseContainerIsRunning() // Ensure that pause container is already RUNNING + }). + Return(registry.DistributionInspect{ + Descriptor: ocispec.Descriptor{Digest: testDigest}, + }, nil), + + // Expectations for transition to PULLED + dockerClient.EXPECT(). + PullImage(gomock.Any(), image+"@"+testDigest.String(), nil, gomock.Any()). + Do(func(context.Context, string, *apicontainer.RegistryAuthenticationData, time.Duration) { + assertPauseContainerIsRunning() // Ensure that pause container is already RUNNING + }). + Return(dockerapi.DockerContainerMetadata{}), + dockerClient.EXPECT(). + TagImage(gomock.Any(), image+"@"+testDigest.String(), image). + Return(nil), + imageManager.EXPECT().RecordContainerReference(task.Containers[0]).Return(nil), + imageManager.EXPECT().GetImageStateFromImageName(image).Return(nil, false), + + // Expectations for transition to CREATED + dockerClient.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil), + dockerClient.EXPECT(). + CreateContainer(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Do( + func(ctx interface{}, config *dockercontainer.Config, y interface{}, + containerName string, z time.Duration, + ) { + taskContainerDockerEventsSent.Add(1) + go func() { + eventStream <- createDockerEvent(apicontainerstatus.ContainerCreated) + taskContainerDockerEventsSent.Done() + }() + }). + Return(dockerapi.DockerContainerMetadata{DockerID: containerID}), + + // Expectations for transition to RUNNING + dockerClient.EXPECT(). + StartContainer(gomock.Any(), containerID, cfg.ContainerStartTimeout). + Do( + func(ctx interface{}, id string, timeout time.Duration) { + taskContainerDockerEventsSent.Wait() + taskContainerDockerEventsSent.Add(1) + go func() { + eventStream <- createDockerEvent(apicontainerstatus.ContainerRunning) + taskContainerDockerEventsSent.Done() + }() + }). + Return(dockerapi.DockerContainerMetadata{DockerID: containerID}), + ) + + gomock.InOrder(transitionExpectations...) + + // Start the task + err := taskEngine.Init(context.Background()) + require.NoError(t, err) + taskEngine.AddTask(task) + + // Wait for task to transition to RUNNING + waitForManifestPulledEvents(t, taskEngine.StateChangeEvents()) + waitForRunningEvents(t, taskEngine.StateChangeEvents()) + pauseContainerDockerEventsSent.Wait() + taskContainerDockerEventsSent.Wait() + + // Expectations for cleanup + cleanup := make(chan time.Time) + mockTime.EXPECT().After(gomock.Any()).Return(cleanup).MinTimes(1) + gomock.InOrder( + // For pause container cleanup + dockerClient.EXPECT(). + InspectContainer(gomock.Any(), "pauseContainer", gomock.Any()). + Return(&dockerapitypes.ContainerJSON{ + ContainerJSONBase: &dockerapitypes.ContainerJSONBase{ + State: &dockerapitypes.ContainerState{Pid: 5}, + HostConfig: &dockerapitypescontainer.HostConfig{NetworkMode: "none"}, + }, + }, nil), + cniClient.EXPECT(). + CleanupNS(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil), + ) + + // Expect IP resource to be released asyncronously + var ipResourceReleased sync.WaitGroup + ipResourceReleased.Add(1) + cniClient.EXPECT(). + ReleaseIPResource(gomock.Any(), gomock.Any(), gomock.Any()). + Do(func(context.Context, *ecscni.Config, time.Duration) { ipResourceReleased.Done() }). + Return(nil) + + // Get dockerContainer for the task container + containerMap, ok := taskEngine.(*DockerTaskEngine).State().ContainerMapByArn(task.Arn) + require.True(t, ok) + taskContainer, ok := containerMap[task.Containers[0].Name] + require.True(t, ok) + pauseContainer, ok := containerMap[task.Containers[1].Name] + require.True(t, ok) + + // Containers can be removed in any order + dockerClient.EXPECT(). + RemoveContainer( + gomock.Any(), pauseContainer.DockerID, dockerclient.RemoveContainerTimeout). + Return(nil) + dockerClient.EXPECT(). + RemoveContainer( + gomock.Any(), taskContainer.DockerID, dockerclient.RemoveContainerTimeout). + Return(nil) + + // Image reference is removed for the task container only + imageManager.EXPECT().RemoveContainerReferenceFromImageState(taskContainer.Container).Return(nil) + + // Simulate container exit + eventStream <- dockerapi.DockerContainerChangeEvent{ + Status: apicontainerstatus.ContainerStopped, + DockerContainerMetadata: dockerapi.DockerContainerMetadata{ + DockerID: containerID, + ExitCode: aws.Int(0), + }, + } + + // StopContainer might be invoked if the test execution is slow, during + // the cleanup phase. Account for that. + dockerClient.EXPECT().StopContainer(gomock.Any(), gomock.Any(), gomock.Any()).Return( + dockerapi.DockerContainerMetadata{DockerID: containerID}).AnyTimes() + + // Wait for task to stop + waitForStopEvents(t, taskEngine.StateChangeEvents(), false, false) + + // trigger cleanup, this ensures all the goroutines were finished + task.SetSentStatus(apitaskstatus.TaskStopped) // Needed to unblock cleanup + cleanup <- time.Now() + for { + tasks, _ := taskEngine.(*DockerTaskEngine).ListTasks() + if len(tasks) == 0 { + break + } + time.Sleep(5 * time.Millisecond) + } + + // Wait for IP resource to be released + ipResourceReleased.Wait() +} diff --git a/agent/engine/docker_task_engine_test.go b/agent/engine/docker_task_engine_test.go index 29b0f3fcbcc..4536cb5eeda 100644 --- a/agent/engine/docker_task_engine_test.go +++ b/agent/engine/docker_task_engine_test.go @@ -41,7 +41,6 @@ import ( "github.com/aws/amazon-ecs-agent/agent/dockerclient" "github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi" mock_dockerapi "github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi/mocks" - "github.com/aws/amazon-ecs-agent/agent/ecscni" mock_ecscni "github.com/aws/amazon-ecs-agent/agent/ecscni/mocks" "github.com/aws/amazon-ecs-agent/agent/engine/dockerstate" "github.com/aws/amazon-ecs-agent/agent/engine/execcmd" @@ -73,8 +72,6 @@ import ( "github.com/aws/aws-sdk-go/service/ssm" cniTypesCurrent "github.com/containernetworking/cni/pkg/types/100" "github.com/docker/docker/api/types" - dockerapitypes "github.com/docker/docker/api/types" - dockerapitypescontainer "github.com/docker/docker/api/types/container" dockercontainer "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/network" "github.com/docker/docker/api/types/registry" diff --git a/agent/engine/docker_task_engine_windows_test.go b/agent/engine/docker_task_engine_windows_test.go index fe74381883d..a24e3ee99b4 100644 --- a/agent/engine/docker_task_engine_windows_test.go +++ b/agent/engine/docker_task_engine_windows_test.go @@ -462,6 +462,15 @@ func TestTaskWithSteadyStateResourcesProvisioned(t *testing.T) { waitForStopEvents(t, taskEngine.StateChangeEvents(), true, false) } +// Tests a happy case scenario for an AWSVPC task. +// +// This test also verifies that +// any DockerClient calls that interact with an image repository (PullContainerManifest +// and PullContainer, currently) happen after the pause container has reached +// ContainerResourcesProvisioned (RUNNING) state. +// +// If you are updating this test then make sure that you call assertPauseContainerIsRunning() +// in any dockerClient expected calls that are supposed to interact with an image repository. func TestPauseContainerHappyPath(t *testing.T) { ctx, cancel := context.WithCancel(context.TODO()) defer cancel() @@ -531,6 +540,19 @@ func TestPauseContainerHappyPath(t *testing.T) { }, nil), ) + // A function to assert that the network pause container in the task is in + // ContainerResourcesProvisioned state. This will be used by dockerClient mock later. + assertPauseContainerIsRunning := func() { + assert.Len(t, sleepTask.Containers, 3, "expected pause container to be populated") + pauseContainer := sleepTask.Containers[2] + assert.Equal(t, apitask.NetworkPauseContainerName, pauseContainer.Name) + assert.Equal(t, apicontainer.ContainerCNIPause, pauseContainer.Type) + assert.Equal(t, + apicontainerstatus.ContainerResourcesProvisioned, + pauseContainer.GetKnownStatus(), + "expected pause container to be running before image repository is called") + } + // For the other container imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() manifestPullClient := mock_dockerapi.NewMockDockerClient(ctrl) @@ -541,6 +563,9 @@ func TestPauseContainerHappyPath(t *testing.T) { manifestPullClient.EXPECT(). PullImageManifest(gomock.Any(), gomock.Any(), gomock.Any()). Times(2). + Do(func(context.Context, string, *apicontainer.RegistryAuthenticationData) { + assertPauseContainerIsRunning() // Ensure that pause container is already RUNNING + }). Return(registry.DistributionInspect{}, nil) dockerClient.EXPECT().PullImage(gomock.Any(), gomock.Any(), nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}).Times(2) imageManager.EXPECT().RecordContainerReference(gomock.Any()).Return(nil).Times(2) @@ -565,8 +590,12 @@ func TestPauseContainerHappyPath(t *testing.T) { }, }, nil) cniClient.EXPECT().SetupNS(gomock.Any(), gomock.Any(), gomock.Any()).Return(nsResult, nil) - dockerClient.EXPECT().StartContainer(gomock.Any(), sleepContainerID2, defaultConfig.ContainerStartTimeout).Return( - dockerapi.DockerContainerMetadata{DockerID: sleepContainerID2}) + dockerClient.EXPECT(). + StartContainer(gomock.Any(), sleepContainerID2, defaultConfig.ContainerStartTimeout). + Do(func(context.Context, string, *apicontainer.RegistryAuthenticationData) { + assertPauseContainerIsRunning() // Ensure that pause container is already RUNNING + }). + Return(dockerapi.DockerContainerMetadata{DockerID: sleepContainerID2}) dockerClient.EXPECT().InspectContainer(gomock.Any(), gomock.Any(), gomock.Any()).Return( &types.ContainerJSON{ ContainerJSONBase: &types.ContainerJSONBase{ From 5a59e9fd94691630333e3e0e8c9664c9fb0ac4c0 Mon Sep 17 00:00:00 2001 From: Amogh Rathore Date: Thu, 15 Aug 2024 15:49:28 +0000 Subject: [PATCH 3/8] Minor reformat --- agent/engine/docker_task_engine_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/agent/engine/docker_task_engine_test.go b/agent/engine/docker_task_engine_test.go index 4536cb5eeda..be6838a51d5 100644 --- a/agent/engine/docker_task_engine_test.go +++ b/agent/engine/docker_task_engine_test.go @@ -67,6 +67,7 @@ import ( "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/appmesh" ni "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/networkinterface" mock_ttime "github.com/aws/amazon-ecs-agent/ecs-agent/utils/ttime/mocks" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/secretsmanager" "github.com/aws/aws-sdk-go/service/ssm" From a0c64277153135f9e9ca2fc4494a0428f82d90e0 Mon Sep 17 00:00:00 2001 From: Amogh Rathore Date: Thu, 15 Aug 2024 17:57:21 +0000 Subject: [PATCH 4/8] Update Windows test --- agent/engine/docker_task_engine_linux_test.go | 36 ++++++++++++++++--- .../engine/docker_task_engine_windows_test.go | 25 ++++++++----- 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/agent/engine/docker_task_engine_linux_test.go b/agent/engine/docker_task_engine_linux_test.go index 01ed71de436..786dd0cc067 100644 --- a/agent/engine/docker_task_engine_linux_test.go +++ b/agent/engine/docker_task_engine_linux_test.go @@ -859,6 +859,19 @@ func TestPauseContainerHappyPath(t *testing.T) { dockerClient.EXPECT().ContainerEvents(gomock.Any()).Return(eventStream, nil) serviceConnectManager.EXPECT().GetAppnetContainerTarballDir().AnyTimes() + // A function to assert that the network pause container in the task is in + // ContainerResourcesProvisioned state. This will be used by dockerClient mock later. + assertPauseContainerIsRunning := func() { + assert.Len(t, sleepTask.Containers, 3, "expected pause container to be populated") + pauseContainer := sleepTask.Containers[2] + assert.Equal(t, apitask.NetworkPauseContainerName, pauseContainer.Name) + assert.Equal(t, apicontainer.ContainerCNIPause, pauseContainer.Type) + assert.Equal(t, + apicontainerstatus.ContainerResourcesProvisioned, + pauseContainer.GetKnownStatus(), + "expected pause container to be running before image repository is called") + } + sleepContainerID1 := containerID + "1" sleepContainerID2 := containerID + "2" pauseContainerID := "pauseContainerID" @@ -872,8 +885,13 @@ func TestPauseContainerHappyPath(t *testing.T) { assert.True(t, ok) assert.Equal(t, apitask.NetworkPauseContainerName, name) }).Return(dockerapi.DockerContainerMetadata{DockerID: "pauseContainerID"}), - dockerClient.EXPECT().StartContainer(gomock.Any(), pauseContainerID, defaultConfig.ContainerStartTimeout).Return( - dockerapi.DockerContainerMetadata{DockerID: "pauseContainerID"}), + dockerClient.EXPECT(). + StartContainer(gomock.Any(), pauseContainerID, defaultConfig.ContainerStartTimeout). + Do(func(ctx interface{}, id string, timeout time.Duration) { + // Simulate some startup time + time.Sleep(5 * time.Millisecond) + }). + Return(dockerapi.DockerContainerMetadata{DockerID: "pauseContainerID"}), dockerClient.EXPECT().InspectContainer(gomock.Any(), gomock.Any(), gomock.Any()).Return( &types.ContainerJSON{ ContainerJSONBase: &types.ContainerJSONBase{ @@ -894,9 +912,19 @@ func TestPauseContainerHappyPath(t *testing.T) { imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() dockerClient.EXPECT().WithVersion(dockerclient.Version_1_35).Times(2).Return(manifestPullClient, nil) manifestPullClient.EXPECT(). - PullImageManifest(gomock.Any(), gomock.Any(), gomock.Any()).Times(2). + PullImageManifest(gomock.Any(), gomock.Any(), gomock.Any()). + Times(2). + Do(func(context.Context, string, *apicontainer.RegistryAuthenticationData) { + assertPauseContainerIsRunning() // Ensure that pause container is already RUNNING + }). Return(registry.DistributionInspect{}, nil) - dockerClient.EXPECT().PullImage(gomock.Any(), gomock.Any(), nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}).Times(2) + dockerClient.EXPECT(). + PullImage(gomock.Any(), gomock.Any(), nil, gomock.Any()). + Do(func(context.Context, string, *apicontainer.RegistryAuthenticationData, time.Duration) { + assertPauseContainerIsRunning() // Ensure that pause container is already RUNNING + }). + Return(dockerapi.DockerContainerMetadata{}). + Times(2) imageManager.EXPECT().RecordContainerReference(gomock.Any()).Return(nil).Times(2) imageManager.EXPECT().GetImageStateFromImageName(gomock.Any()).Return(nil, false).Times(2) dockerClient.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil).Times(2) diff --git a/agent/engine/docker_task_engine_windows_test.go b/agent/engine/docker_task_engine_windows_test.go index a24e3ee99b4..b9668d315f7 100644 --- a/agent/engine/docker_task_engine_windows_test.go +++ b/agent/engine/docker_task_engine_windows_test.go @@ -518,8 +518,13 @@ func TestPauseContainerHappyPath(t *testing.T) { assert.True(t, ok) assert.Equal(t, apitask.NetworkPauseContainerName, name) }).Return(dockerapi.DockerContainerMetadata{DockerID: "pauseContainerID"}), - dockerClient.EXPECT().StartContainer(gomock.Any(), pauseContainerID, defaultConfig.ContainerStartTimeout).Return( - dockerapi.DockerContainerMetadata{DockerID: "pauseContainerID"}), + dockerClient.EXPECT(). + StartContainer(gomock.Any(), pauseContainerID, defaultConfig.ContainerStartTimeout). + Do(func(ctx interface{}, id string, timeout time.Duration) { + // Simulate some startup time + time.Sleep(5 * time.Millisecond) + }). + Return(dockerapi.DockerContainerMetadata{DockerID: "pauseContainerID"}), dockerClient.EXPECT().InspectContainer(gomock.Any(), gomock.Any(), gomock.Any()).Return( &types.ContainerJSON{ ContainerJSONBase: &types.ContainerJSONBase{ @@ -567,7 +572,13 @@ func TestPauseContainerHappyPath(t *testing.T) { assertPauseContainerIsRunning() // Ensure that pause container is already RUNNING }). Return(registry.DistributionInspect{}, nil) - dockerClient.EXPECT().PullImage(gomock.Any(), gomock.Any(), nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}).Times(2) + dockerClient.EXPECT(). + PullImage(gomock.Any(), gomock.Any(), nil, gomock.Any()). + Do(func(context.Context, string, *apicontainer.RegistryAuthenticationData, time.Duration) { + assertPauseContainerIsRunning() // Ensure that pause container is already RUNNING + }). + Return(dockerapi.DockerContainerMetadata{}). + Times(2) imageManager.EXPECT().RecordContainerReference(gomock.Any()).Return(nil).Times(2) imageManager.EXPECT().GetImageStateFromImageName(gomock.Any()).Return(nil, false).Times(2) dockerClient.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil).Times(2) @@ -590,12 +601,8 @@ func TestPauseContainerHappyPath(t *testing.T) { }, }, nil) cniClient.EXPECT().SetupNS(gomock.Any(), gomock.Any(), gomock.Any()).Return(nsResult, nil) - dockerClient.EXPECT(). - StartContainer(gomock.Any(), sleepContainerID2, defaultConfig.ContainerStartTimeout). - Do(func(context.Context, string, *apicontainer.RegistryAuthenticationData) { - assertPauseContainerIsRunning() // Ensure that pause container is already RUNNING - }). - Return(dockerapi.DockerContainerMetadata{DockerID: sleepContainerID2}) + dockerClient.EXPECT().StartContainer(gomock.Any(), sleepContainerID2, defaultConfig.ContainerStartTimeout).Return( + dockerapi.DockerContainerMetadata{DockerID: sleepContainerID2}) dockerClient.EXPECT().InspectContainer(gomock.Any(), gomock.Any(), gomock.Any()).Return( &types.ContainerJSON{ ContainerJSONBase: &types.ContainerJSONBase{ From 9207656a178851d9b2ed4ff9faaffd080c9e6b07 Mon Sep 17 00:00:00 2001 From: Amogh Rathore Date: Thu, 15 Aug 2024 18:00:07 +0000 Subject: [PATCH 5/8] Remove redundant new test --- agent/engine/docker_task_engine_linux_test.go | 280 ------------------ 1 file changed, 280 deletions(-) diff --git a/agent/engine/docker_task_engine_linux_test.go b/agent/engine/docker_task_engine_linux_test.go index 786dd0cc067..535243ea675 100644 --- a/agent/engine/docker_task_engine_linux_test.go +++ b/agent/engine/docker_task_engine_linux_test.go @@ -20,7 +20,6 @@ import ( "encoding/json" "errors" "fmt" - "net" "os" "strconv" "strings" @@ -62,8 +61,6 @@ import ( "github.com/aws/aws-sdk-go/aws" cniTypesCurrent "github.com/containernetworking/cni/pkg/types/100" "github.com/docker/docker/api/types" - dockerapitypes "github.com/docker/docker/api/types" - dockerapitypescontainer "github.com/docker/docker/api/types/container" dockercontainer "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/network" "github.com/docker/docker/api/types/registry" @@ -1612,280 +1609,3 @@ func TestCredentialSpecResourceTaskFile(t *testing.T) { ret := taskEngine.(*DockerTaskEngine).createContainer(testTask, testTask.Containers[0]) assert.Nil(t, ret.Error) } - -// Tests that any repo-interacting Docker calls are made by the Task Engine after -// the pause container (for awsvpc tasks) has reached ContainerResourcesProvisioned state. -// -// The test adds a simple awsvpc task to the task engine and then verifies that -// any DockerClient calls that interact with an image repository (PullContainerManifest -// and PullContainer, currently) happen after the pause container has reached -// ContainerResourcesProvisioned (RUNNING) state. -// -// If you are updating this test then make sure that you call assertPauseContainerIsRunning() -// in any dockerClient expected calls that are supposed to interact with an image repository. -func TestRepoInteractionAgainstPauseContainerState(t *testing.T) { - // A test task - image := "image" - task := &apitask.Task{ - Containers: []*apicontainer.Container{ - { - Image: image, - Name: "container", - TransitionDependenciesMap: map[apicontainerstatus.ContainerStatus]apicontainer.TransitionDependencySet{}, - Essential: true, - }, - }, - Arn: testTaskARN, - DesiredStatusUnsafe: apitaskstatus.TaskRunning, - NetworkMode: apitask.AWSVPCNetworkMode, - } - - // Set up task engine and mocks - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - cfg := config.DefaultConfig() - cfg.TaskCPUMemLimit.Value = config.ExplicitlyDisabled - ctrl, _, mockTime, taskEngine, _, imageManager, _, serviceConnectManager := mocks(t, ctx, &cfg) - defer ctrl.Finish() - dockerClient := mock_dockerapi.NewMockDockerClient(ctrl) - cniClient := mock_ecscni.NewMockCNIClient(ctrl) - taskEngine.(*DockerTaskEngine).client = dockerClient - taskEngine.(*DockerTaskEngine).cniClient = cniClient - - // Expectations for ServiceConnectManager - loading of AppNet container image - serviceConnectManager.EXPECT().GetAppnetContainerTarballDir().AnyTimes().Return("") - serviceConnectManager.EXPECT(). - LoadImage(gomock.Any(), gomock.Any(), gomock.Any()). - AnyTimes() - - // time.Now() is called to record certain timestamps but we don't care about - // that for this test - mockTime.EXPECT().Now().AnyTimes().Return(time.Now()) - - // A function to assert that the network pause container in the task is in - // ContainerResourcesProvisioned state. This will be used by dockerClient mock later. - assertPauseContainerIsRunning := func() { - assert.Len(t, task.Containers, 2, "expected pause container to be populated") - pauseContainer := task.Containers[1] - assert.Equal(t, apitask.NetworkPauseContainerName, pauseContainer.Name) - assert.Equal(t, apicontainer.ContainerCNIPause, pauseContainer.Type) - assert.Equal(t, - apicontainerstatus.ContainerResourcesProvisioned, - pauseContainer.GetKnownStatus(), - "expected pause container to be running before image repository is called") - } - - // Set expectations on mocks for containers transition to CREATED and RUNNING - eventStream := make(chan dockerapi.DockerContainerChangeEvent) - dockerClient.EXPECT().ContainerEvents(gomock.Any()).Return(eventStream, nil) - imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes() - - // To record container container state transition expectations - transitionExpectations := []*gomock.Call{} - - // To track asynchronous sending of Docker events for container create and start - var pauseContainerDockerEventsSent sync.WaitGroup - - // State transition expectations for the pause container - transitionExpectations = append(transitionExpectations, - dockerClient.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil), - dockerClient.EXPECT(). - CreateContainer(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). - Do( - func(ctx interface{}, config *dockercontainer.Config, y interface{}, - containerName string, z time.Duration, - ) { - pauseContainerDockerEventsSent.Add(1) - go func() { - eventStream <- createDockerEvent(apicontainerstatus.ContainerCreated) - pauseContainerDockerEventsSent.Done() - }() - }). - Return(dockerapi.DockerContainerMetadata{DockerID: "pauseContainer"}), - dockerClient.EXPECT(). - StartContainer(gomock.Any(), "pauseContainer", cfg.ContainerStartTimeout). - Do( - func(ctx interface{}, id string, timeout time.Duration) { - // Simulate some startup time - time.Sleep(5 * time.Millisecond) - pauseContainerDockerEventsSent.Wait() - pauseContainerDockerEventsSent.Add(1) - go func() { - eventStream <- createDockerEvent(apicontainerstatus.ContainerRunning) - pauseContainerDockerEventsSent.Done() - }() - }). - Return(dockerapi.DockerContainerMetadata{DockerID: containerID}), - dockerClient.EXPECT(). - InspectContainer(gomock.Any(), "pauseContainer", gomock.Any()). - Return(&dockerapitypes.ContainerJSON{ - ContainerJSONBase: &dockerapitypes.ContainerJSONBase{ - State: &dockerapitypes.ContainerState{Pid: 5}, - HostConfig: &dockerapitypescontainer.HostConfig{NetworkMode: "none"}, - }, - }, nil), - cniClient.EXPECT(). - SetupNS(gomock.Any(), gomock.Any(), gomock.Any()). - Return(&cniTypesCurrent.Result{ - IPs: []*cniTypesCurrent.IPConfig{ - {Address: net.IPNet{IP: net.IPv4(127, 0, 0, 1)}}, - }, - }, nil), - ) - - // To track asynchronous sending of Docker events for container create and start - var taskContainerDockerEventsSent sync.WaitGroup - - // State transition expectations for the task container - // - // Any expected dockerClient method calls that would interact with the image repository - // must call assertPauseContainerIsRunning() function to ensure that the pause container - // is RUNNING before the method call. - transitionExpectations = append(transitionExpectations, - // Expectations for transition to MANIFEST_PULLED - dockerClient.EXPECT(). - WithVersion(dockerclient.Version_1_35). - Return(dockerClient, nil), - dockerClient.EXPECT().PullImageManifest(gomock.Any(), image, nil). - Do(func(context.Context, string, *apicontainer.RegistryAuthenticationData) { - assertPauseContainerIsRunning() // Ensure that pause container is already RUNNING - }). - Return(registry.DistributionInspect{ - Descriptor: ocispec.Descriptor{Digest: testDigest}, - }, nil), - - // Expectations for transition to PULLED - dockerClient.EXPECT(). - PullImage(gomock.Any(), image+"@"+testDigest.String(), nil, gomock.Any()). - Do(func(context.Context, string, *apicontainer.RegistryAuthenticationData, time.Duration) { - assertPauseContainerIsRunning() // Ensure that pause container is already RUNNING - }). - Return(dockerapi.DockerContainerMetadata{}), - dockerClient.EXPECT(). - TagImage(gomock.Any(), image+"@"+testDigest.String(), image). - Return(nil), - imageManager.EXPECT().RecordContainerReference(task.Containers[0]).Return(nil), - imageManager.EXPECT().GetImageStateFromImageName(image).Return(nil, false), - - // Expectations for transition to CREATED - dockerClient.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil), - dockerClient.EXPECT(). - CreateContainer(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). - Do( - func(ctx interface{}, config *dockercontainer.Config, y interface{}, - containerName string, z time.Duration, - ) { - taskContainerDockerEventsSent.Add(1) - go func() { - eventStream <- createDockerEvent(apicontainerstatus.ContainerCreated) - taskContainerDockerEventsSent.Done() - }() - }). - Return(dockerapi.DockerContainerMetadata{DockerID: containerID}), - - // Expectations for transition to RUNNING - dockerClient.EXPECT(). - StartContainer(gomock.Any(), containerID, cfg.ContainerStartTimeout). - Do( - func(ctx interface{}, id string, timeout time.Duration) { - taskContainerDockerEventsSent.Wait() - taskContainerDockerEventsSent.Add(1) - go func() { - eventStream <- createDockerEvent(apicontainerstatus.ContainerRunning) - taskContainerDockerEventsSent.Done() - }() - }). - Return(dockerapi.DockerContainerMetadata{DockerID: containerID}), - ) - - gomock.InOrder(transitionExpectations...) - - // Start the task - err := taskEngine.Init(context.Background()) - require.NoError(t, err) - taskEngine.AddTask(task) - - // Wait for task to transition to RUNNING - waitForManifestPulledEvents(t, taskEngine.StateChangeEvents()) - waitForRunningEvents(t, taskEngine.StateChangeEvents()) - pauseContainerDockerEventsSent.Wait() - taskContainerDockerEventsSent.Wait() - - // Expectations for cleanup - cleanup := make(chan time.Time) - mockTime.EXPECT().After(gomock.Any()).Return(cleanup).MinTimes(1) - gomock.InOrder( - // For pause container cleanup - dockerClient.EXPECT(). - InspectContainer(gomock.Any(), "pauseContainer", gomock.Any()). - Return(&dockerapitypes.ContainerJSON{ - ContainerJSONBase: &dockerapitypes.ContainerJSONBase{ - State: &dockerapitypes.ContainerState{Pid: 5}, - HostConfig: &dockerapitypescontainer.HostConfig{NetworkMode: "none"}, - }, - }, nil), - cniClient.EXPECT(). - CleanupNS(gomock.Any(), gomock.Any(), gomock.Any()). - Return(nil), - ) - - // Expect IP resource to be released asyncronously - var ipResourceReleased sync.WaitGroup - ipResourceReleased.Add(1) - cniClient.EXPECT(). - ReleaseIPResource(gomock.Any(), gomock.Any(), gomock.Any()). - Do(func(context.Context, *ecscni.Config, time.Duration) { ipResourceReleased.Done() }). - Return(nil) - - // Get dockerContainer for the task container - containerMap, ok := taskEngine.(*DockerTaskEngine).State().ContainerMapByArn(task.Arn) - require.True(t, ok) - taskContainer, ok := containerMap[task.Containers[0].Name] - require.True(t, ok) - pauseContainer, ok := containerMap[task.Containers[1].Name] - require.True(t, ok) - - // Containers can be removed in any order - dockerClient.EXPECT(). - RemoveContainer( - gomock.Any(), pauseContainer.DockerID, dockerclient.RemoveContainerTimeout). - Return(nil) - dockerClient.EXPECT(). - RemoveContainer( - gomock.Any(), taskContainer.DockerID, dockerclient.RemoveContainerTimeout). - Return(nil) - - // Image reference is removed for the task container only - imageManager.EXPECT().RemoveContainerReferenceFromImageState(taskContainer.Container).Return(nil) - - // Simulate container exit - eventStream <- dockerapi.DockerContainerChangeEvent{ - Status: apicontainerstatus.ContainerStopped, - DockerContainerMetadata: dockerapi.DockerContainerMetadata{ - DockerID: containerID, - ExitCode: aws.Int(0), - }, - } - - // StopContainer might be invoked if the test execution is slow, during - // the cleanup phase. Account for that. - dockerClient.EXPECT().StopContainer(gomock.Any(), gomock.Any(), gomock.Any()).Return( - dockerapi.DockerContainerMetadata{DockerID: containerID}).AnyTimes() - - // Wait for task to stop - waitForStopEvents(t, taskEngine.StateChangeEvents(), false, false) - - // trigger cleanup, this ensures all the goroutines were finished - task.SetSentStatus(apitaskstatus.TaskStopped) // Needed to unblock cleanup - cleanup <- time.Now() - for { - tasks, _ := taskEngine.(*DockerTaskEngine).ListTasks() - if len(tasks) == 0 { - break - } - time.Sleep(5 * time.Millisecond) - } - - // Wait for IP resource to be released - ipResourceReleased.Wait() -} From bbb971498e9d7497d7c05391773456fee0fdbb83 Mon Sep 17 00:00:00 2001 From: Amogh Rathore Date: Thu, 15 Aug 2024 18:01:24 +0000 Subject: [PATCH 6/8] Add test description --- agent/engine/docker_task_engine_linux_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/agent/engine/docker_task_engine_linux_test.go b/agent/engine/docker_task_engine_linux_test.go index 535243ea675..24b0dae208a 100644 --- a/agent/engine/docker_task_engine_linux_test.go +++ b/agent/engine/docker_task_engine_linux_test.go @@ -821,6 +821,15 @@ func TestTaskWithSteadyStateResourcesProvisioned(t *testing.T) { waitForStopEvents(t, taskEngine.StateChangeEvents(), true, false) } +// Tests a happy case scenario for an AWSVPC task. +// +// This test also verifies that +// any DockerClient calls that interact with an image repository (PullContainerManifest +// and PullContainer, currently) happen after the pause container has reached +// ContainerResourcesProvisioned (RUNNING) state. +// +// If you are updating this test then make sure that you call assertPauseContainerIsRunning() +// in any dockerClient expected calls that are supposed to interact with an image repository. func TestPauseContainerHappyPath(t *testing.T) { ctx, cancel := context.WithCancel(context.TODO()) defer cancel() From fad3fc719a365ddadeb040ec33daf0aa811bd0f8 Mon Sep 17 00:00:00 2001 From: Amogh Rathore Date: Thu, 15 Aug 2024 18:25:09 +0000 Subject: [PATCH 7/8] Minor comment change --- agent/engine/docker_task_engine_linux_test.go | 2 +- agent/engine/docker_task_engine_windows_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/agent/engine/docker_task_engine_linux_test.go b/agent/engine/docker_task_engine_linux_test.go index 24b0dae208a..93c7ab41e7d 100644 --- a/agent/engine/docker_task_engine_linux_test.go +++ b/agent/engine/docker_task_engine_linux_test.go @@ -826,7 +826,7 @@ func TestTaskWithSteadyStateResourcesProvisioned(t *testing.T) { // This test also verifies that // any DockerClient calls that interact with an image repository (PullContainerManifest // and PullContainer, currently) happen after the pause container has reached -// ContainerResourcesProvisioned (RUNNING) state. +// ContainerResourcesProvisioned state. // // If you are updating this test then make sure that you call assertPauseContainerIsRunning() // in any dockerClient expected calls that are supposed to interact with an image repository. diff --git a/agent/engine/docker_task_engine_windows_test.go b/agent/engine/docker_task_engine_windows_test.go index b9668d315f7..8cd1adb5ed5 100644 --- a/agent/engine/docker_task_engine_windows_test.go +++ b/agent/engine/docker_task_engine_windows_test.go @@ -467,7 +467,7 @@ func TestTaskWithSteadyStateResourcesProvisioned(t *testing.T) { // This test also verifies that // any DockerClient calls that interact with an image repository (PullContainerManifest // and PullContainer, currently) happen after the pause container has reached -// ContainerResourcesProvisioned (RUNNING) state. +// ContainerResourcesProvisioned state. // // If you are updating this test then make sure that you call assertPauseContainerIsRunning() // in any dockerClient expected calls that are supposed to interact with an image repository. From bf79ae17947fcbf717d8d63e4d56502de492ac7c Mon Sep 17 00:00:00 2001 From: Amogh Rathore Date: Thu, 15 Aug 2024 22:07:08 +0000 Subject: [PATCH 8/8] Minor reformat --- agent/engine/docker_task_engine_linux_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/agent/engine/docker_task_engine_linux_test.go b/agent/engine/docker_task_engine_linux_test.go index 93c7ab41e7d..b33c8472759 100644 --- a/agent/engine/docker_task_engine_linux_test.go +++ b/agent/engine/docker_task_engine_linux_test.go @@ -58,6 +58,7 @@ import ( "github.com/aws/amazon-ecs-agent/ecs-agent/credentials" "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/appmesh" ni "github.com/aws/amazon-ecs-agent/ecs-agent/netlib/model/networkinterface" + "github.com/aws/aws-sdk-go/aws" cniTypesCurrent "github.com/containernetworking/cni/pkg/types/100" "github.com/docker/docker/api/types"