api: Set CPU/Memory in config or hostconfig

Docker moved the Memory from Config into HostConfig where on windows the
memory information in Config wasn't respected. This commit sets the
memory in both Config and HostConfig to make it compatiable.

CHANGELOG.md

@@ -5,6 +5,7 @@
 * Feature - Enable support for task level CPU and memory constraints.
 * Bug - Fixed a bug where a task can be blocked in creating state. [#1048](https://github.com/aws/amazon-ecs-agent/pull/1048)
 * Bug - Fixed dynamic HostPort in container metadata. [#1052](https://github.com/aws/amazon-ecs-agent/pull/1052)
+* Bug - Fixed bug on Windows where container memory limits are not enforced. [#1069](https://github.com/aws/amazon-ecs-agent/pull/1069)
 
 ## 1.15.0
 * Feature - Support for provisioning tasks with ENIs.

agent/api/task.go

@@ -26,6 +26,7 @@ import (
 	"github.com/aws/amazon-ecs-agent/agent/config"
 	"github.com/aws/amazon-ecs-agent/agent/credentials"
 	"github.com/aws/amazon-ecs-agent/agent/ecscni"
+	"github.com/aws/amazon-ecs-agent/agent/engine/dockerclient"
 	"github.com/aws/amazon-ecs-agent/agent/engine/emptyvolume"
 	"github.com/aws/amazon-ecs-agent/agent/utils/ttime"
 	"github.com/aws/aws-sdk-go/aws/arn"
@@ -414,11 +415,11 @@ func (task *Task) getEarliestKnownTaskStatusForContainers() TaskStatus {
 
 // DockerConfig converts the given container in this task to the format of
 // GoDockerClient's 'Config' struct
-func (task *Task) DockerConfig(container *Container) (*docker.Config, *DockerClientConfigError) {
-	return task.dockerConfig(container)
+func (task *Task) DockerConfig(container *Container, apiVersion dockerclient.DockerVersion) (*docker.Config, *DockerClientConfigError) {
+	return task.dockerConfig(container, apiVersion)
 }
 
-func (task *Task) dockerConfig(container *Container) (*docker.Config, *DockerClientConfigError) {
+func (task *Task) dockerConfig(container *Container, apiVersion dockerclient.DockerVersion) (*docker.Config, *DockerClientConfigError) {
 	dockerVolumes, err := task.dockerConfigVolumes(container)
 	if err != nil {
 		return nil, &DockerClientConfigError{err.Error()}
@@ -447,8 +448,11 @@ func (task *Task) dockerConfig(container *Container) (*docker.Config, *DockerCli
 		ExposedPorts: task.dockerExposedPorts(container),
 		Volumes:      dockerVolumes,
 		Env:          dockerEnv,
-		Memory:       dockerMem,
-		CPUShares:    task.dockerCPUShares(container.CPU),
+	}
+
+	err = task.SetConfigHostconfigBasedOnVersion(container, config, nil, apiVersion)
+	if err != nil {
+		return nil, &DockerClientConfigError{"setting docker config failed, err: " + err.Error()}
 	}
 
 	if container.DockerConfig.Config != nil {
@@ -464,6 +468,47 @@ func (task *Task) dockerConfig(container *Container) (*docker.Config, *DockerCli
 	return config, nil
 }
 
+// SetConfigHostconfigBasedOnVersion sets the fields in both Config and HostConfig based on api version for backward compatibility
+func (task *Task) SetConfigHostconfigBasedOnVersion(container *Container, config *docker.Config, hc *docker.HostConfig, apiVersion dockerclient.DockerVersion) error {
+	// Convert MB to B
+	dockerMem := int64(container.Memory * 1024 * 1024)
+	if dockerMem != 0 && dockerMem < DockerContainerMinimumMemoryInBytes {
+		dockerMem = DockerContainerMinimumMemoryInBytes
+	}
+	cpuShare := task.dockerCPUShares(container.CPU)
+
+	// Docker copied Memory and cpu field into hostconfig in 1.6 with api version(1.18)
+	// https://github.com/moby/moby/commit/837eec064d2d40a4d86acbc6f47fada8263e0d4c
+	dockerAPIVersion, err := docker.NewAPIVersion(string(apiVersion))
+	if err != nil {
+		seelog.Errorf("Creating docker api version failed, err: %v", err)
+		return err
+	}
+
+	dockerAPIVersion_1_18, err := docker.NewAPIVersion("1.18")
+	if err != nil {
+		seelog.Errorf("Creating docker api version 1.18 failed, err: %v", err)
+		return err
+	}
+
+	if dockerAPIVersion.GreaterThanOrEqualTo(dockerAPIVersion_1_18) {
+		// Set the memory and cpu in host config
+		if hc != nil {
+			hc.Memory = dockerMem
+			hc.CPUShares = cpuShare
+		}
+		return nil
+	}
+
+	// Set the memory and cpu in config
+	if config != nil {
+		config.Memory = dockerMem
+		config.CPUShares = cpuShare
+	}
+
+	return nil
+}
+
 // dockerCPUShares converts containerCPU shares if needed as per the logic stated below:
 // Docker silently converts 0 to 1024 CPU shares, which is probably not what we
 // want.  Instead, we convert 0 to 2 to be closer to expected behavior. The
@@ -512,8 +557,8 @@ func (task *Task) dockerConfigVolumes(container *Container) (map[string]struct{}
 }
 
 // DockerHostConfig construct the configuration recognized by docker
-func (task *Task) DockerHostConfig(container *Container, dockerContainerMap map[string]*DockerContainer) (*docker.HostConfig, *HostConfigError) {
-	return task.dockerHostConfig(container, dockerContainerMap)
+func (task *Task) DockerHostConfig(container *Container, dockerContainerMap map[string]*DockerContainer, apiVersion dockerclient.DockerVersion) (*docker.HostConfig, *HostConfigError) {
+	return task.dockerHostConfig(container, dockerContainerMap, apiVersion)
 }
 
 // ApplyExecutionRoleLogsAuth will check whether the task has excecution role
@@ -538,7 +583,7 @@ func (task *Task) ApplyExecutionRoleLogsAuth(hostConfig *docker.HostConfig, cred
 	return nil
 }
 
-func (task *Task) dockerHostConfig(container *Container, dockerContainerMap map[string]*DockerContainer) (*docker.HostConfig, *HostConfigError) {
+func (task *Task) dockerHostConfig(container *Container, dockerContainerMap map[string]*DockerContainer, apiVersion dockerclient.DockerVersion) (*docker.HostConfig, *HostConfigError) {
 	dockerLinkArr, err := task.dockerLinks(container, dockerContainerMap)
 	if err != nil {
 		return nil, &HostConfigError{err.Error()}
@@ -564,6 +609,11 @@ func (task *Task) dockerHostConfig(container *Container, dockerContainerMap map[
 		VolumesFrom:  volumesFrom,
 	}
 
+	err = task.SetConfigHostconfigBasedOnVersion(container, nil, hostConfig, apiVersion)
+	if err != nil {
+		return nil, &HostConfigError{err.Error()}
+	}
+
 	if container.DockerConfig.HostConfig != nil {
 		err := json.Unmarshal([]byte(*container.DockerConfig.HostConfig), hostConfig)
 		if err != nil {

agent/api/task_test.go

@@ -179,6 +179,7 @@ func TestDockerHostConfigRawConfig(t *testing.T) {
 		DNSSearch:      []string{"dns.search"},
 		ExtraHosts:     []string{"extra:hosts"},
 		SecurityOpt:    []string{"foo", "bar"},
+		CPUShares:      2,
 		LogConfig: docker.LogConfig{
 			Type:   "foo",
 			Config: map[string]string{"foo": "bar"},
@@ -257,6 +258,8 @@ func TestDockerHostConfigRawConfigMerging(t *testing.T) {
 		Privileged:       true,
 		SecurityOpt:      []string{"foo", "bar"},
 		VolumesFrom:      []string{"dockername-c2"},
+		Memory:           100 * 1024 * 1024,
+		CPUShares:        50,
 		MemorySwappiness: memorySwappinessDefault,
 	}
 

agent/engine/docker_container_engine.go

@@ -146,6 +146,8 @@ type DockerClient interface {
 
 	// Version returns the version of the Docker daemon.
 	Version() (string, error)
+	// APIVersion returns the api version of the client
+	APIVersion() (dockerclient.DockerVersion, error)
 
 	// InspectImage returns information about the specified image.
 	InspectImage(string) (*docker.Image, error)
@@ -884,6 +886,15 @@ func (dg *dockerGoClient) Version() (string, error) {
 	return info.Get("Version"), nil
 }
 
+// APIVersion returns the client api version
+func (dg *dockerGoClient) APIVersion() (dockerclient.DockerVersion, error) {
+	client, err := dg.dockerClient()
+	if err != nil {
+		return "", err
+	}
+	return dg.clientFactory.FindClientAPIVersion(client), nil
+}
+
 // Stats returns a channel of *docker.Stats entries for the container.
 func (dg *dockerGoClient) Stats(id string, ctx context.Context) (<-chan *docker.Stats, error) {
 	client, err := dg.dockerClient()

agent/engine/docker_task_engine.go

@@ -685,7 +685,12 @@ func (engine *DockerTaskEngine) createContainer(task *api.Task, container *api.C
 	// we have to do this in create, not start, because docker no longer handles
 	// merging create config with start hostconfig the same; e.g. memory limits
 	// get lost
-	hostConfig, hcerr := task.DockerHostConfig(container, containerMap)
+	dockerClientVersion, versionErr := client.APIVersion()
+	if versionErr != nil {
+		return DockerContainerMetadata{Error: CannotGetDockerClientVersionError{versionErr}}
+	}
+
+	hostConfig, hcerr := task.DockerHostConfig(container, containerMap, dockerClientVersion)
 	if hcerr != nil {
 		return DockerContainerMetadata{Error: api.NamedError(hcerr)}
 	}
@@ -697,7 +702,7 @@ func (engine *DockerTaskEngine) createContainer(task *api.Task, container *api.C
 		}
 	}
 
-	config, err := task.DockerConfig(container)
+	config, err := task.DockerConfig(container, dockerClientVersion)
 	if err != nil {
 		return DockerContainerMetadata{Error: api.NamedError(err)}
 	}

agent/engine/dockerclient/dockerclientfactory.go

@@ -27,10 +27,11 @@ const (
 	// https://docs.docker.com/engine/api/version-history/#v125-api-changes
 	minAPIVersionKey = "MinAPIVersion"
 	// apiVersionKey is the docker.Env key for API version
-	apiVersionKey    = "ApiVersion"
+	apiVersionKey = "ApiVersion"
 	// zeroPatch is a string to append patch number zero if the major minor version lacks it
 	zeroPatch = ".0"
 )
+
 // Factory provides a collection of docker remote clients that include a
 // recommended client version as well as a set of alternative supported
 // docker clients.
@@ -57,6 +58,9 @@ type Factory interface {
 	// not necessarily fully support) and the versions that result in
 	// successful responses by the Docker daemon.
 	FindKnownAPIVersions() []DockerVersion
+
+	// FindClientAPIVersion returns the client api version
+	FindClientAPIVersion(dockeriface.Client) DockerVersion
 }
 
 type factory struct {
@@ -106,6 +110,18 @@ func (f *factory) FindKnownAPIVersions() []DockerVersion {
 	return knownVersions
 }
 
+// FindClientAPIVersion returns the version of the client from the map
+// TODO we should let go docker client return this version information
+func (f *factory) FindClientAPIVersion(client dockeriface.Client) DockerVersion {
+	for k, v := range f.clients {
+		if v == client {
+			return k
+		}
+	}
+
+	return getDefaultVersion()
+}
+
 // getClient returns a client specified by the docker version. Its wrapped
 // by GetClient so that it can do platform-specific magic
 func (f *factory) getClient(version DockerVersion) (dockeriface.Client, error) {
@@ -157,7 +173,7 @@ func getDockerClientForVersion(
 	version string,
 	minAPIVersion string,
 	apiVersion string) (dockeriface.Client, error) {
-	if (minAPIVersion != "" && apiVersion != "") {
+	if minAPIVersion != "" && apiVersion != "" {
 		// Adding patch number zero to Docker versions to reuse the existing semver
 		// comparator
 		// TODO: remove this logic later when non-semver comparator is implemented

agent/engine/dockerclient/versionsupport_unix.go

@@ -22,6 +22,7 @@ const (
 	// minDockerAPIVersion is the min Docker API version supported by agent
 	minDockerAPIVersion = Version_1_17
 )
+
 // GetClient on linux will simply return the cached client from the map
 func (f *factory) GetClient(version DockerVersion) (dockeriface.Client, error) {
 	return f.getClient(version)

agent/engine/errors.go

@@ -317,3 +317,16 @@ func (err ContainerNetworkingError) Error() string {
 func (err ContainerNetworkingError) ErrorName() string {
 	return "ContainerNetworkingError"
 }
+
+// CannotGetDockerClientVersionError indicates error when trying to get docker
+// client api version
+type CannotGetDockerClientVersionError struct {
+	fromError error
+}
+
+func (err CannotGetDockerClientVersionError) ErrorName() string {
+	return "CannotGetDockerClientVersionError"
+}
+func (err CannotGetDockerClientVersionError) Error() string {
+	return err.fromError.Error()
+}