Updating Task Model and implementing docker flags --pid and --ipc #1584

linkar-ec2 · 2018-09-27T00:02:58Z

Summary

Updates the Agent Task model to include PidMode and IpcMode for sharing PID namespace and IPC resource namespace, respectively.
Adding unit tests for Pid and Ipc resource namespace sharing

Implementation details

Agent Task struct is updated to support the PidMode and IpcMode that is in the Task struct received in the payload message from ACS.
In PostUnmarshalTask(), the task is used to determine if a Pause container for namespace sharing needs to be provisioned. If so, it is added as an internal container to the task.
In CreateContainer(), the DockerHostConfig() method determines if the current container's PidMode and IpcOption needs to be changed.

Testing

Builds on Linux (make release)
Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
Unit tests on Linux (make test) pass
Unit tests on Windows (go test -timeout=25s ./agent/...) pass
Integration tests on Linux (make run-integ-tests) pass
Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
Functional tests on Linux (make run-functional-tests) pass
Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes:
Unit Tests cover the added Pause container provisioning methods and DockerHostConfig updates

Description for the changelog

Adding ECS Agent support for the --pid and --ipc Docker flags

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

samuelkarp · 2018-09-27T00:19:11Z

Is there any particular reason that you're not sharing the pause container between awsvpc mode and these new options? They serve the same purpose: to provide root namespaces that can be shared.

linkar-ec2 · 2018-09-27T00:34:41Z

The purpose of the ENI Pause Container was to attach the individual containers within a task to an ENI. We did this by creating a common network namespace and then attaching the Pause container to the ENI. Reusing this Pause container would tightly couple this ENI implementation with the namespace sharing for PID and IPC resources. With the resources consumed by the Pause container considered to be negligible, creating another container would allow single responsibility of the internal containers we create. A concrete example of this would be the case when an ENI has custom DNS IPs and search list associated with it, in which case we will configure the HostConfig for the ENI Pause, but not for the PID/IPC namespace Pause container. As regards to the possible question “why not create 2 Pause containers for the PID and IPC resources for this same reason of single responsibility?” this Namespace Pause container is simply provided as a parameter to the other containers within a task, for either PidMode or IpcMode, or both. There is no other HostConfig changes done to the Pause container itself (a difference from the ENI Pause).

samuelkarp · 2018-09-27T00:41:23Z

That's not exactly accurate. The purpose of the existing pause container is to create a network namespace that can be shared among the containers in a task. We don't attach the pause container to the ENI, but rather move the ENI into the network namespace that was created as a side-effect of running the pause container. PID and IPC namespaces are similar in that they're just namespaces of the container.

haikuoliu · 2018-09-27T04:56:22Z

agent/api/task/task.go

 		pauseContainer.BuildContainerDependency(container.Name, apicontainerstatus.ContainerStopped, apicontainerstatus.ContainerStopped)
 	}
 }

+func (task *Task) addNamespaceSharingProvisioningDependency(cfg *config.Config) {
+	// Pause container does not need to be created if no namespace sharing will be done
+	if task.getIpcMode() == "" && task.getPidMode() == "" {


I would change it to if task.getIpcMode() != "task" && task.getPidMode() != "task" so you don't need to check if it's task mode below.

haikuoliu · 2018-09-27T05:03:43Z

agent/api/task/task.go

@@ -167,6 +171,14 @@ type Task struct {
 	terminalReason     string
 	terminalReasonOnce sync.Once

+	// PidMode is used to determine how PID namespaces are organized between
+	// containers of the Task
+	PidMode string


See https://github.com/golang/go/wiki/CodeReviewComments#initialisms, Golang convention is a bit different from Java convention, so use PIDMode and IPCMode instead, same for other namings.

haikuoliu · 2018-09-27T05:04:10Z

agent/api/task/task.go

+	}
+
+	// Create a Pause container if either IPC resource or PID namespace needs to be shared
+	if task.getIpcMode() == "task" || task.getPidMode() == "task" {


Instead of using "task" directly, you can declare constants like PIDModeNone="none", PIDModeHost="host", PIDModeTask="task" and use them here (and everywhere).

Done, will update in next commit with integ tests

haikuoliu · 2018-09-27T05:18:40Z

agent/api/task/task.go

 		}
+	}
+
+	shouldOverridePid, pidMode := task.shouldOverridePidMode(container, dockerContainerMap)


ok, pidMode would be sufficient, see https://github.com/golang/go/wiki/CodeReviewComments#variable-names

Same below.

agent/api/task/task.go

agent/api/container/container.go

haikuoliu · 2018-09-27T07:30:43Z

agent/api/task/task.go

+				pidPauseContName = cont.Name
+				break
+			}
+		}


The logic to find pause container within all of the containers appears three times (CNI, pid, ipc), you can abstract them into a method.

haikuoliu · 2018-09-27T07:31:49Z

agent/api/task/task.go

+	if strings.EqualFold(task.getPidMode(), "host") {
+		return true, "host"
+	} else if strings.EqualFold(task.getPidMode(), "task") {
+		//need to find the ContainerNamespacePause and assign


nit: // need, this happens in several places.

haikuoliu · 2018-09-27T07:49:59Z

agent/api/task/task.go

+		hostConfig.NetworkMode = networkMode
+		// Override 'awsvpc' parameters if needed
+		if container.Type == apicontainer.ContainerCNIPause {
+


Why extra line here?

haikuoliu · 2018-09-27T08:40:41Z

agent/api/task/task.go

+		namespacePauseContainer := apicontainer.NewContainerWithSteadyState(apicontainerstatus.ContainerResourcesProvisioned)
+		namespacePauseContainer.TransitionDependenciesMap = make(map[apicontainerstatus.ContainerStatus]apicontainer.TransitionDependencySet)
+		namespacePauseContainer.Name = NamespacePauseContainerName
+		namespacePauseContainer.Image = fmt.Sprintf("%s:%s", cfg.PauseContainerImageName, cfg.PauseContainerTag)


To avoid PauseContainerImageName being overridden by Fargate, one option is you can use DefaultPauseContainerImageName here. And you will also need to make change here and pause_linux.go to make Agent always load default pause container tarball

linkar-ec2 · 2018-09-27T17:00:43Z

[After syncing with Sam and Derek]
For closure, it is better to use separate Pause containers for the ENI/Network sharing and the PID/IPC resources namespace sharing. The image for the Pause container for the ENI attachment is specific to the requirements of the ENI. The image for the PID/IPC sharing Pause will be the default image.

yhlee-aws

Can we add functional test for these flags? is it blocked by backend work?

yhlee-aws · 2018-09-28T03:53:09Z

agent/api/task/task.go

 		pauseContainer.BuildContainerDependency(container.Name, apicontainerstatus.ContainerStopped, apicontainerstatus.ContainerStopped)
 	}
 }

+func (task *Task) addNamespaceSharingProvisioningDependency(cfg *config.Config) {
+	// Pause container does not need to be created if no namespace sharing will be done at task level
+	if task.getIPCMode() != "task" && task.getPIDMode() != "task" {


move "task" into const block?

yhlee-aws · 2018-09-28T03:58:26Z

agent/api/task/task.go

+		return false, ""
+	}
+
+	if strings.EqualFold(task.getPIDMode(), "host") {


move "host" to const block also.
and all the rest of magic strings you are using, like "none" and "shareable"

@yunhee-l Functional tests need the Frontend changes to be complete before we can register task definitions. Integ tests are currently being made and will probably be in another PR

haikuoliu

Almost there 😊

agent/api/task/task.go

haikuoliu · 2018-09-28T22:11:36Z

agent/api/task/task.go

-		if pidPauseContName == "" {
-			seelog.Critical("Namespace Pause Container required, but not found in the task: %s", task.String())
-			return false, "" // cannot override the Pid without the Pause container
+	} else if strings.EqualFold(task.getPIDMode(), "task") {


Any particular reason using strings.EqualFold instead of ==? I think == should be sufficient here as we already know the result of task.getPIDMode(), i.e. it's "task" instead of "Task" or "tAsk", this is more like a contract between Agent and ACS.

agent/app/agent_unix.go

sharanyad · 2018-10-01T17:02:01Z

agent/api/container/container.go

@@ -401,8 +401,8 @@ func (c *Container) GetNextKnownStateProgression() apicontainerstatus.ContainerS
 	return c.GetKnownStatus() + 1
 }

-// IsInternal returns true if the container type is either `ContainerEmptyHostVolume`
-// or `ContainerCNIPause`. It returns false otherwise
+// IsInternal returns true if the container type is `ContainerEmptyHostVolume`,


We don't use the ContainerEmptyHostVolume anymore, it can be removed from the comment.

sharanyad · 2018-10-01T17:10:44Z

agent/api/task/task.go

+	if task.getIPCMode() != ipcModeTask && task.getPIDMode() != pidModeTask {
+		return
+	}
+	namespacePauseContainer := apicontainer.NewContainerWithSteadyState(apicontainerstatus.ContainerResourcesProvisioned)


What is the definition of ContainerResourcesProvisioned state for namespacePauseContainer?

ContainerResourcesProvisioned seems to be the norm for internal containers. Even though this state checks to see if ENI resources need to be added (for our namespacePauseContainer, nothing additional will be done). Because we don't need anything in this method anyways, I set the Dependency to use Running. I don't think there should be difference in runtime though.

sharanyad · 2018-10-01T17:12:24Z

agent/api/task/task.go

+		return false, ""
+	}
+
+	if task.getPIDMode() == pidModeHost {


These can be switch statements instead of if, else. That would be easier to read.

The first if block tests if the container is internal. This is unrelated to the Task's pidMode, whose logic is an if and else-if block. For separation of logic, I think this if-else works better than the switch. What do you think?

sharanyad · 2018-10-01T17:13:11Z

agent/api/task/task.go

+			seelog.Critical("Namespace Pause container not found in the task: %s", task.String())
+			return false, ""
+		}
+		pauseToGiveDocker, ok := dockerContainerMap[pauseContName]


what does pauseToGiveDocker mean?

This is the Docker container name that is contained in the map. We will 'give' this container to the Docker api for the --pid or --ipc flag

Got it. it's a little confusing. I would prefer just calling it pauseContainerID or something similar.

sharanyad · 2018-10-01T17:16:11Z

agent/api/task/task.go

+		}
+		pauseToGiveDocker, ok := dockerContainerMap[pauseContName]
+		if !ok || pauseToGiveDocker == nil {
+			// Docker container shouldn't be nill or not exist if the Container definition within task exists; implies code-bug


sharanyad · 2018-10-01T17:16:37Z

agent/api/task/task.go

+		pauseToGiveDocker, ok := dockerContainerMap[pauseContName]
+		if !ok || pauseToGiveDocker == nil {
+			// Docker container shouldn't be nill or not exist if the Container definition within task exists; implies code-bug
+			seelog.Criticalf("Namespace Pause container required, but not found in container map for container: [%s] in task: %s",


This is a failure case, which should be handled in a way that the task is not run. If pidMode is task and pause container is not found, we should stop the task imo. same for ipcMode.

haikuoliu

And please update the year in the header of the files you changed, thanks!

agent/api/task/task.go

haikuoliu · 2018-10-01T20:34:55Z

agent/api/task/task.go

@@ -76,6 +76,16 @@ const (
 	// awslogsCredsEndpointOpt is the awslogs option that is used to pass in an
 	// http endpoint for authentication
 	awslogsCredsEndpointOpt = "awslogs-credentials-endpoint"
+
+	// ipcModeNone specifies the string used to define the `none` docker IPC resource namespace sharing mode


Any reason that ipcModeNone is not grouped with other pid/ipc constants?

Just to distinguish that None is an option for IPC but thinking again, it makes more sense to just group it and leave out the comment.

haikuoliu

LGTM 🎉! Make sure all tests pass and squash the commits

sharanyad

We would need new capabilities for the two flags that can be added in the next PR.

sharanyad · 2018-10-01T21:37:48Z

agent/api/task/task.go

 		pauseContainer.BuildContainerDependency(container.Name, apicontainerstatus.ContainerStopped, apicontainerstatus.ContainerStopped)
 	}
 }

+func (task *Task) addNamespaceSharingProvisioningDependency(cfg *config.Config) {


Please add unit tests for this method.

@sharanyad The new capabilities for the two flags will be updated with the Agent FE model changes in an upcoming PR!

sharanyad · 2018-10-01T21:41:15Z

agent/api/task/task_test.go

+		DesiredStatus: strptr("RUNNING"),
+		Family:        strptr("myFamily"),
+		PidMode:       strptr("host"),
+		IpcMode:       strptr("task"),


Could you please add test cases for the other modes as well? You could use table driven tests for this. same for pidMode.

sharanyad · 2018-10-01T21:50:51Z

agent/api/task/task.go

+	case task.getIPCMode() == ipcModeTask:
+		pauseContName, ok := task.findPauseContainerName(apicontainer.ContainerNamespacePause)
+		if !ok {
+			seelog.Critical("Namespace Pause container not found in the task: %s", task.String())


nit: seelog.Criticalf
Logging just the task ARN is enough here

sharanyad · 2018-10-01T21:52:51Z

agent/api/task/task.go

+		pauseContName, ok := task.findPauseContainerName(apicontainer.ContainerNamespacePause)
+		if !ok {
+			seelog.Critical("Namespace Pause container not found in the task: %s", task.String())
+			seelog.Criticalf("Setting Task Desired Status to Stopped. Cannot set up PID sharing.")


This log statement can be combined with previous one.
also *IPC sharing

sharanyad · 2018-10-01T21:56:24Z

agent/api/task/task.go

+		return false, ""
+	}
+
+	switch {


this can be:

switch task.getIPCMode() { case ipcModeNone: ...

sharanyad

Do we know what is the test coverage of the files that are changed?
This tool can help you find it.

sharanyad · 2018-10-02T20:38:52Z

agent/api/task/task_test.go

@@ -635,6 +635,183 @@ func TestPostUnmarshalTaskWithLocalVolumes(t *testing.T) {

 }

+// Slice of structs for Table Driven testing for sharing PID and IPC resources
+var namespaceTests = []struct {


Please add tests for IPCMode None and Shareable as well.

The customer will not be allowed to provide Shareable as an option. Shareable is internally set for the Pause container and is tested within a unit test.

agent/api/task/task_test.go

linkar-ec2 · 2018-10-03T04:36:31Z

Code Coverage:
Task: 70.1%
Container: 57.3%

Adding unit tests for Pid and Ipc resource namespace sharing

sharanyad · 2018-10-03T18:38:01Z

CHANGELOG.md

@@ -1,4 +1,9 @@
 # Changelog
+## 1.20.4-dev


Looks like this file should not be there in this PR.

I rebased and squashed all commits. Awaiting merge.

sharanyad

sharanyad · 2018-10-03T20:32:07Z

agent/api/task/task.go

+		pauseDockerID, ok := dockerContainerMap[pauseCont.Name]
+		if !ok || pauseDockerID == nil {
+			// Docker container shouldn't be nil or not exist if the Container definition within task exists; implies code-bug
+			seelog.Criticalf("Namespace Pause container not found in the task: %s; Setting Task's Desired Status to Stopped", task.Arn)


nit: this error message is same as the previous condition.
Please change it to something like not found in container map, so its easier while debugging.

linkar-ec2 added this to the 1.21.0 milestone Sep 27, 2018

linkar-ec2 requested review from adnxn, petderek, sharanyad, haikuoliu, yhlee-aws and fenxiong September 27, 2018 00:02

haikuoliu requested a review from a team September 27, 2018 00:05

add changelogs for 1.20.4

24caaeb

haikuoliu reviewed Sep 27, 2018

View reviewed changes

yhlee-aws reviewed Sep 28, 2018

View reviewed changes

haikuoliu reviewed Sep 28, 2018

View reviewed changes

sharanyad reviewed Oct 1, 2018

View reviewed changes

haikuoliu reviewed Oct 1, 2018

View reviewed changes

haikuoliu approved these changes Oct 1, 2018

View reviewed changes

sharanyad reviewed Oct 1, 2018

View reviewed changes

update license files

bffeee2

yhlee-aws approved these changes Oct 2, 2018

View reviewed changes

sharanyad reviewed Oct 2, 2018

View reviewed changes

Updating Task Model and implementing docker flags --pid and --ipc

302c6bc

Adding unit tests for Pid and Ipc resource namespace sharing

linkar-ec2 force-pushed the DockerFlagSupportLocal branch from c95d00a to 302c6bc Compare October 3, 2018 17:13

sharanyad reviewed Oct 3, 2018

View reviewed changes

sharanyad approved these changes Oct 3, 2018

View reviewed changes

linkar-ec2 merged commit b72bebf into aws:DockerFlagSupport Oct 3, 2018

haikuoliu removed this from the 1.21.0 milestone Oct 4, 2018

haikuoliu added this to the 1.22.0 milestone Oct 4, 2018

Updating Task Model and implementing docker flags --pid and --ipc #1584

Updating Task Model and implementing docker flags --pid and --ipc #1584

Conversation

linkar-ec2 commented Sep 27, 2018 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

samuelkarp commented Sep 27, 2018

linkar-ec2 commented Sep 27, 2018 via email • edited Loading

samuelkarp commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linkar-ec2 commented Sep 27, 2018

yhlee-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haikuoliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haikuoliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haikuoliu left a comment

Choose a reason for hiding this comment

sharanyad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharanyad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linkar-ec2 commented Oct 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharanyad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linkar-ec2 commented Sep 27, 2018 •

edited

Loading

linkar-ec2 commented Sep 27, 2018 via email •

edited

Loading