From d1fa74b5e024646854e24fd15e456952b9fcf24e Mon Sep 17 00:00:00 2001 From: Prateek Chaudhry Date: Mon, 28 Nov 2022 19:43:41 +0000 Subject: [PATCH] fix cgroup unit test --- .github/workflows/linux.yml | 2 +- agent/api/task/task_linux_test.go | 13 ++- agent/engine/docker_task_engine_linux_test.go | 108 ++++++++++++------ agent/taskresource/cgroup/cgroup_test.go | 32 ++++++ 4 files changed, 121 insertions(+), 34 deletions(-) diff --git a/.github/workflows/linux.yml b/.github/workflows/linux.yml index 9afe72fd0ba..13efe56a20f 100644 --- a/.github/workflows/linux.yml +++ b/.github/workflows/linux.yml @@ -6,7 +6,7 @@ permissions: read-all jobs: unit-tests: name: Linux unit tests - runs-on: ubuntu-20.04 + runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 with: diff --git a/agent/api/task/task_linux_test.go b/agent/api/task/task_linux_test.go index e69fc7a8d16..a1ad635e75e 100644 --- a/agent/api/task/task_linux_test.go +++ b/agent/api/task/task_linux_test.go @@ -51,7 +51,8 @@ const ( validTaskArn = "arn:aws:ecs:region:account-id:task/task-id" invalidTaskArn = "invalid:task::arn" - expectedCgroupRoot = "/ecs/task-id" + expectedCgroupV1Root = "/ecs/task-id" + expectedCgroupV2Root = "ecstasks-task-id.slice" taskVCPULimit = 2.0 taskMemoryLimit = 512 @@ -81,6 +82,13 @@ var ( scPauseContainerName = fmt.Sprintf(ServiceConnectPauseContainerNameFormat, scContainerName) ) +func getExpectedCgroupRoot() string { + if config.CgroupV2 { + return expectedCgroupV2Root + } + return expectedCgroupV1Root +} + func TestAddNetworkResourceProvisioningDependencyNop(t *testing.T) { testTask := &Task{ Containers: []*apicontainer.Container{ @@ -244,6 +252,7 @@ func TestBuildCgroupRootHappyPath(t *testing.T) { } cgroupRoot, err := task.BuildCgroupRoot() + expectedCgroupRoot := getExpectedCgroupRoot() assert.NoError(t, err) assert.Equal(t, expectedCgroupRoot, cgroupRoot) @@ -450,6 +459,7 @@ func TestOverrideCgroupParentHappyPath(t *testing.T) { } hostConfig := &dockercontainer.HostConfig{} + expectedCgroupRoot := getExpectedCgroupRoot() assert.NoError(t, task.overrideCgroupParent(hostConfig)) assert.NotEmpty(t, hostConfig) @@ -482,6 +492,7 @@ func TestPlatformHostConfigOverride(t *testing.T) { } hostConfig := &dockercontainer.HostConfig{} + expectedCgroupRoot := getExpectedCgroupRoot() assert.NoError(t, task.platformHostConfigOverride(hostConfig)) assert.NotEmpty(t, hostConfig) diff --git a/agent/engine/docker_task_engine_linux_test.go b/agent/engine/docker_task_engine_linux_test.go index 6dfbd6c89f7..49ca3aaf0c4 100644 --- a/agent/engine/docker_task_engine_linux_test.go +++ b/agent/engine/docker_task_engine_linux_test.go @@ -97,8 +97,10 @@ func TestResourceContainerProgression(t *testing.T) { mockControl := mock_control.NewMockControl(ctrl) mockIO := mock_ioutilwrapper.NewMockIOUtil(ctrl) taskID := sleepTask.GetID() - cgroupMemoryPath := fmt.Sprintf("/sys/fs/cgroup/memory/ecs/%s/memory.use_hierarchy", taskID) cgroupRoot := fmt.Sprintf("/ecs/%s", taskID) + if config.CgroupV2 { + cgroupRoot = fmt.Sprintf("ecstasks-%s.slice", taskID) + } cgroupResource := cgroup.NewCgroupResource(sleepTask.Arn, mockControl, mockIO, cgroupRoot, cgroupMountPath, specs.LinuxResources{}) sleepTask.ResourcesMapUnsafe = make(map[string][]taskresource.TaskResource) @@ -110,35 +112,69 @@ func TestResourceContainerProgression(t *testing.T) { containerEventsWG := sync.WaitGroup{} client.EXPECT().ContainerEvents(gomock.Any()).Return(eventStream, nil) serviceConnectManager.EXPECT().GetAppnetContainerTarballDir().AnyTimes() - gomock.InOrder( - // Ensure that the resource is created first - mockControl.EXPECT().Exists(gomock.Any()).Return(false), - mockControl.EXPECT().Create(gomock.Any()).Return(nil), - mockIO.EXPECT().WriteFile(cgroupMemoryPath, gomock.Any(), gomock.Any()).Return(nil), - imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes(), - client.EXPECT().PullImage(gomock.Any(), sleepContainer.Image, nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}), - imageManager.EXPECT().RecordContainerReference(sleepContainer).Return(nil), - imageManager.EXPECT().GetImageStateFromImageName(sleepContainer.Image).Return(nil, false), - client.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil), - client.EXPECT().CreateContainer(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Do( - func(ctx interface{}, config *dockercontainer.Config, hostConfig *dockercontainer.HostConfig, containerName string, z time.Duration) { - assert.True(t, strings.Contains(containerName, sleepContainer.Name)) - containerEventsWG.Add(1) - go func() { - eventStream <- createDockerEvent(apicontainerstatus.ContainerCreated) - containerEventsWG.Done() - }() - }).Return(dockerapi.DockerContainerMetadata{DockerID: containerID + ":" + sleepContainer.Name}), - // Next, the sleep container is started - client.EXPECT().StartContainer(gomock.Any(), containerID+":"+sleepContainer.Name, defaultConfig.ContainerStartTimeout).Do( - func(ctx interface{}, id string, timeout time.Duration) { - containerEventsWG.Add(1) - go func() { - eventStream <- createDockerEvent(apicontainerstatus.ContainerRunning) - containerEventsWG.Done() - }() - }).Return(dockerapi.DockerContainerMetadata{DockerID: containerID + ":" + sleepContainer.Name}), - ) + + // Hierarchical memory accounting is always enabled in CgroupV2 and no controller file exists to configure it + if config.CgroupV2 { + gomock.InOrder( + // Ensure that the resource is created first + mockControl.EXPECT().Exists(gomock.Any()).Return(false), + mockControl.EXPECT().Create(gomock.Any()).Return(nil), + imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes(), + client.EXPECT().PullImage(gomock.Any(), sleepContainer.Image, nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}), + imageManager.EXPECT().RecordContainerReference(sleepContainer).Return(nil), + imageManager.EXPECT().GetImageStateFromImageName(sleepContainer.Image).Return(nil, false), + client.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil), + client.EXPECT().CreateContainer(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Do( + func(ctx interface{}, config *dockercontainer.Config, hostConfig *dockercontainer.HostConfig, containerName string, z time.Duration) { + assert.True(t, strings.Contains(containerName, sleepContainer.Name)) + containerEventsWG.Add(1) + go func() { + eventStream <- createDockerEvent(apicontainerstatus.ContainerCreated) + containerEventsWG.Done() + }() + }).Return(dockerapi.DockerContainerMetadata{DockerID: containerID + ":" + sleepContainer.Name}), + // Next, the sleep container is started + client.EXPECT().StartContainer(gomock.Any(), containerID+":"+sleepContainer.Name, defaultConfig.ContainerStartTimeout).Do( + func(ctx interface{}, id string, timeout time.Duration) { + containerEventsWG.Add(1) + go func() { + eventStream <- createDockerEvent(apicontainerstatus.ContainerRunning) + containerEventsWG.Done() + }() + }).Return(dockerapi.DockerContainerMetadata{DockerID: containerID + ":" + sleepContainer.Name}), + ) + } else { + cgroupMemoryPath := fmt.Sprintf("/sys/fs/cgroup/memory/ecs/%s/memory.use_hierarchy", taskID) + gomock.InOrder( + // Ensure that the resource is created first + mockControl.EXPECT().Exists(gomock.Any()).Return(false), + mockControl.EXPECT().Create(gomock.Any()).Return(nil), + mockIO.EXPECT().WriteFile(cgroupMemoryPath, gomock.Any(), gomock.Any()).Return(nil), + imageManager.EXPECT().AddAllImageStates(gomock.Any()).AnyTimes(), + client.EXPECT().PullImage(gomock.Any(), sleepContainer.Image, nil, gomock.Any()).Return(dockerapi.DockerContainerMetadata{}), + imageManager.EXPECT().RecordContainerReference(sleepContainer).Return(nil), + imageManager.EXPECT().GetImageStateFromImageName(sleepContainer.Image).Return(nil, false), + client.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil), + client.EXPECT().CreateContainer(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Do( + func(ctx interface{}, config *dockercontainer.Config, hostConfig *dockercontainer.HostConfig, containerName string, z time.Duration) { + assert.True(t, strings.Contains(containerName, sleepContainer.Name)) + containerEventsWG.Add(1) + go func() { + eventStream <- createDockerEvent(apicontainerstatus.ContainerCreated) + containerEventsWG.Done() + }() + }).Return(dockerapi.DockerContainerMetadata{DockerID: containerID + ":" + sleepContainer.Name}), + // Next, the sleep container is started + client.EXPECT().StartContainer(gomock.Any(), containerID+":"+sleepContainer.Name, defaultConfig.ContainerStartTimeout).Do( + func(ctx interface{}, id string, timeout time.Duration) { + containerEventsWG.Add(1) + go func() { + eventStream <- createDockerEvent(apicontainerstatus.ContainerRunning) + containerEventsWG.Done() + }() + }).Return(dockerapi.DockerContainerMetadata{DockerID: containerID + ":" + sleepContainer.Name}), + ) + } addTaskToEngine(t, ctx, taskEngine, sleepTask, mockTime, &containerEventsWG) cleanup := make(chan time.Time, 1) @@ -266,6 +302,9 @@ func TestResourceContainerProgressionFailure(t *testing.T) { mockControl := mock_control.NewMockControl(ctrl) taskID := sleepTask.GetID() cgroupRoot := fmt.Sprintf("/ecs/%s", taskID) + if config.CgroupV2 { + cgroupRoot = fmt.Sprintf("ecstasks-%s.slice", taskID) + } cgroupResource := cgroup.NewCgroupResource(sleepTask.Arn, mockControl, nil, cgroupRoot, cgroupMountPath, specs.LinuxResources{}) sleepTask.ResourcesMapUnsafe = make(map[string][]taskresource.TaskResource) @@ -343,7 +382,6 @@ func TestTaskCPULimitHappyPath(t *testing.T) { mockControl := mock_control.NewMockControl(ctrl) mockIO := mock_ioutilwrapper.NewMockIOUtil(ctrl) taskID := sleepTask.GetID() - cgroupMemoryPath := fmt.Sprintf("/sys/fs/cgroup/memory/ecs/%s/memory.use_hierarchy", taskID) if tc.taskCPULimit.Enabled() { // TODO Currently, the resource Setup() method gets invoked multiple // times for a task. This is really a bug and a fortunate occurrence @@ -360,7 +398,10 @@ func TestTaskCPULimitHappyPath(t *testing.T) { } mockControl.EXPECT().Exists(gomock.Any()).Return(false) mockControl.EXPECT().Create(gomock.Any()).Return(nil) - mockIO.EXPECT().WriteFile(cgroupMemoryPath, gomock.Any(), gomock.Any()).Return(nil) + if !config.CgroupV2 { + cgroupMemoryPath := fmt.Sprintf("/sys/fs/cgroup/memory/ecs/%s/memory.use_hierarchy", taskID) + mockIO.EXPECT().WriteFile(cgroupMemoryPath, gomock.Any(), gomock.Any()).Return(nil) + } } for _, container := range sleepTask.Containers { @@ -412,6 +453,9 @@ func TestTaskCPULimitHappyPath(t *testing.T) { taskEngine.AddTask(sleepTaskStop) taskEngine.AddTask(sleepTaskStop) cgroupRoot := fmt.Sprintf("/ecs/%s", taskID) + if config.CgroupV2 { + cgroupRoot = fmt.Sprintf("ecstasks-%s.slice", taskID) + } if tc.taskCPULimit.Enabled() { mockControl.EXPECT().Remove(cgroupRoot).Return(nil) } diff --git a/agent/taskresource/cgroup/cgroup_test.go b/agent/taskresource/cgroup/cgroup_test.go index 168655c1d93..12f1fc48978 100644 --- a/agent/taskresource/cgroup/cgroup_test.go +++ b/agent/taskresource/cgroup/cgroup_test.go @@ -22,6 +22,7 @@ import ( "testing" "time" + "github.com/aws/amazon-ecs-agent/agent/config" cgroup "github.com/aws/amazon-ecs-agent/agent/taskresource/cgroup/control" "github.com/aws/amazon-ecs-agent/agent/taskresource/cgroup/control/mock_control" resourcestatus "github.com/aws/amazon-ecs-agent/agent/taskresource/status" @@ -42,6 +43,9 @@ const ( ) func TestCreateHappyPath(t *testing.T) { + if config.CgroupV2 { + t.Skip("Skipping TestCreateHappyPath for CgroupV2 as memory.use_hierarchy is not created when cgroupV2=true") + } ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -68,6 +72,9 @@ func TestCreateCgroupPathExists(t *testing.T) { mockIO := mock_ioutilwrapper.NewMockIOUtil(ctrl) cgroupRoot := fmt.Sprintf("/ecs/%s", taskID) + if config.CgroupV2 { + cgroupRoot = fmt.Sprintf("ecstasks-%s.slice", taskID) + } gomock.InOrder( mockControl.EXPECT().Exists(gomock.Any()).Return(true), @@ -85,6 +92,9 @@ func TestCreateCgroupError(t *testing.T) { mockIO := mock_ioutilwrapper.NewMockIOUtil(ctrl) cgroupRoot := fmt.Sprintf("/ecs/%s", taskID) + if config.CgroupV2 { + cgroupRoot = fmt.Sprintf("ecstasks-%s.slice", taskID) + } gomock.InOrder( mockControl.EXPECT().Exists(gomock.Any()).Return(false), @@ -101,6 +111,9 @@ func TestCleanupHappyPath(t *testing.T) { mockControl := mock_control.NewMockControl(ctrl) cgroupRoot := fmt.Sprintf("/ecs/%s", taskID) + if config.CgroupV2 { + cgroupRoot = fmt.Sprintf("ecstasks-%s.slice", taskID) + } mockControl.EXPECT().Remove(cgroupRoot).Return(nil) @@ -114,6 +127,9 @@ func TestCleanupRemoveError(t *testing.T) { mockControl := mock_control.NewMockControl(ctrl) cgroupRoot := fmt.Sprintf("/ecs/%s", taskID) + if config.CgroupV2 { + cgroupRoot = fmt.Sprintf("ecstasks-%s.slice", taskID) + } mockControl.EXPECT().Remove(gomock.Any()).Return(errors.New("cgroup remove error")) @@ -127,6 +143,9 @@ func TestCleanupCgroupDeletedError(t *testing.T) { mockControl := mock_control.NewMockControl(ctrl) cgroupRoot := fmt.Sprintf("/ecs/%s", taskID) + if config.CgroupV2 { + cgroupRoot = fmt.Sprintf("ecstasks-%s.slice", taskID) + } err := cgroups.ErrCgroupDeleted wrappedErr := fmt.Errorf("cgroup remove: unable to obtain controller: %w", err) @@ -144,6 +163,11 @@ func TestMarshal(t *testing.T) { "\"createdAt\":\"0001-01-01T00:00:00Z\",\"desiredStatus\":\"CREATED\",\"knownStatus\":\"NONE\",\"resourceSpec\":{}}" cgroupRoot := "/ecs/taskid" + if config.CgroupV2 { + cgroupRoot = fmt.Sprintf("ecstasks-%s.slice", "taskid") + cgroupStr = "{\"cgroupRoot\":\"ecstasks-taskid.slice\",\"cgroupMountPath\":\"/sys/fs/cgroup\"," + + "\"createdAt\":\"0001-01-01T00:00:00Z\",\"desiredStatus\":\"CREATED\",\"knownStatus\":\"NONE\",\"resourceSpec\":{}}" + } cgroupMountPath := "/sys/fs/cgroup" cgroup := NewCgroupResource("", cgroup.New(), nil, cgroupRoot, cgroupMountPath, specs.LinuxResources{}) @@ -160,6 +184,14 @@ func TestUnmarshal(t *testing.T) { cgroupMountPath := "/sys/fs/cgroup" bytes := []byte("{\"CgroupRoot\":\"/ecs/taskid\",\"CgroupMountPath\":\"/sys/fs/cgroup\"," + "\"CreatedAt\":\"0001-01-01T00:00:00Z\",\"DesiredStatus\":\"CREATED\",\"KnownStatus\":\"NONE\"}") + + if config.CgroupV2 { + cgroupRoot = fmt.Sprintf("ecstasks-%s.slice", "taskid") + bytes = []byte("{\"CgroupRoot\":\"ecstasks-taskid.slice\",\"CgroupMountPath\":\"/sys/fs/cgroup\"," + + "\"CreatedAt\":\"0001-01-01T00:00:00Z\",\"DesiredStatus\":\"CREATED\",\"KnownStatus\":\"NONE\"}") + + } + unmarshalledCgroup := &CgroupResource{} err := unmarshalledCgroup.UnmarshalJSON(bytes) assert.NoError(t, err)