Handle pull for "internal" containers #990

samuelkarp · 2017-09-25T07:30:41Z

Summary

Internal containers should be treated differently during the pull step of the container lifecycle.

We currently have two types of internal containers: "empty host volume" containers and pause containers. "Empty host volume" containers should either be pulled from a remote image (as on Windows) or imported from a locally-generated tarball (as on Linux). The pause container image is either imported from a locally-embedded tarball inside the agent's rootfs or expected to already be present on the host (if the image is overridden via config).

A second commit here changes the empty host volume image on Windows from microsoft/windowsservercore:latest to microsoft/nanoserver:latest, as it's a significantly smaller image.

This addresses #729 (comment).

Implementation details

New constant exposed in the emptyvolume package to control whether or not the image is present locally to be imported, or it should be pulled. New logic in DockerTaskEngine to dispatch to different methods based on the container's Type field.

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: yes

Description for the changelog

Bug - microsoft/windowsservercore:latest was not pulled on Windows under certain conditions

Licensing

This contribution is under the terms of the Apache 2.0 License: yes

petderek

Neat! Had a few comments but it looks good.

petderek · 2017-09-25T16:44:19Z

agent/engine/docker_task_engine_test.go

+	}
+
+	// Windows is special-cased here and just performs a normal pull
+	if runtime.GOOS == "windows" {


I'd prefer this test to be in platform specific files if the expectations are different.

Yeah, wasn't sure about that since this is the only difference in all these tests. I can move it though.

Have this check at the top of the function and do a t.Skip() if you don't want to make a separate Linux file?

You could also make this more OO instead of using platform constants. That would make testing more straightforward here.

You could also just leave this as is. I don't have a strong preference.

Have this check at the top of the function and do a t.Skip() if you don't want to make a separate Linux file?

There's both a Linux- and Windows-test, sharing most of the test structure except some of the expectations.

You could also make this more OO instead of using platform constants.

Not sure I understand what you mean here.

You branch based on a constant boolean that's defined for each platform. You could instead attach these functions to a struct with this configuration embedded.

The tests would then be driven by configuration instead of driven by platform.

I went and separated them into platform-specific files.

petderek · 2017-09-25T16:45:58Z

agent/engine/docker_task_engine.go

@@ -508,6 +509,20 @@ func (engine *DockerTaskEngine) GetTaskByArn(arn string) (*api.Task, bool) {
 }

 func (engine *DockerTaskEngine) pullContainer(task *api.Task, container *api.Container) DockerContainerMetadata {
+	if container.Type == api.ContainerCNIPause {
+		// ContainerCNIPause images are managed at startup
+		return DockerContainerMetadata{}


This is redundant.

What part is redundant? The comment? That explains why there's an early return.

The whole if block doesn't seem necessary. The switch block that runs the same check.

Oh! Yeah...oops. Will remove!

petderek · 2017-09-25T16:48:24Z

agent/engine/docker_task_engine.go

+	case api.ContainerEmptyHostVolume:
+		// ContainerEmptyHostVolume image is either local (must be imported) or remote (must be pulled)
+		if emptyvolume.LocalImage {
+			return engine.client.ImportLocalEmptyVolumeImage()


Did create better describe what was happening behind the scenes here?

Import is the verb that is used in the Docker API.

jhaynes · 2017-09-25T16:56:48Z

Can you edit CHANGELOG.md with the description of this change also?

samuelkarp · 2017-09-25T17:38:19Z

@petderek @jhaynes Done, please take a look.

petderek

vsiddharth

LGTM!
Had just a minor question ?

vsiddharth · 2017-09-25T19:23:23Z

agent/engine/docker_container_engine.go

+	case resp := <-response:
+		return resp
+	case <-timeout:
+		return DockerContainerMetadata{Error: &DockerTimeoutError{pullImageTimeout, "pulled"}}


Can the transition string be a const ?

Any particular reason why? I generally like constants if there's string reuse, string comparison, or if it aids clarity. I suppose reuse might apply here, but it's only used in two places right now.

I was primarily concerned with reuse but it was more of a suggestion.

vsiddharth · 2017-09-25T20:37:26Z

agent/engine/docker_container_engine.go

+	case resp := <-response:
+		return resp
+	case <-timeout:
+		return DockerContainerMetadata{Error: &DockerTimeoutError{pullImageTimeout, "pulled"}}


I was primarily concerned with reuse but it was more of a suggestion.

This commit treats internal containers differently during the pull step of the container lifecycle. We currently have two types of internal containers: "empty host volume" containers and pause containers. "Empty host volume" containers should either be pulled from a remote image (as on Windows) or imported from a locally-generated tarball (as on Linux). The pause container image is either imported from a locally-embedded tarball inside the agent's rootfs or expected to already be present on the host (if the image is overridden via config).

microsoft/nanoserver:latest is significantly smaller than microsoft/windowsservercore:latest and should pull more quickly.

aaithal

samuelkarp added kind/bug os/linux os/windows labels Sep 25, 2017

samuelkarp requested review from cabbruzzese, aaithal, petderek and richardpen September 25, 2017 07:30

samuelkarp mentioned this pull request Sep 25, 2017

Support for Docker >= 1.13.0 on Windows #729

Closed

petderek approved these changes Sep 25, 2017

View reviewed changes

samuelkarp force-pushed the internal-pull branch from e62456c to 4537635 Compare September 25, 2017 17:32

petderek approved these changes Sep 25, 2017

View reviewed changes

vsiddharth reviewed Sep 25, 2017

View reviewed changes

richardpen approved these changes Sep 25, 2017

View reviewed changes

vsiddharth approved these changes Sep 25, 2017

View reviewed changes

samuelkarp added 3 commits September 25, 2017 14:12

emptyvolume: Use nanoserver on Windows

fba28c6

microsoft/nanoserver:latest is significantly smaller than microsoft/windowsservercore:latest and should pull more quickly.

changelog: Add entry for pull bug

7a26f74

samuelkarp force-pushed the internal-pull branch from c133031 to 7a26f74 Compare September 25, 2017 21:13

aaithal approved these changes Sep 25, 2017

View reviewed changes

samuelkarp merged commit 7a26f74 into aws:dev Sep 25, 2017

jhaynes mentioned this pull request Sep 30, 2017

V1.14.5 release #999

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle pull for "internal" containers #990

Handle pull for "internal" containers #990

samuelkarp commented Sep 25, 2017 •

edited

Loading

petderek left a comment

petderek Sep 25, 2017

samuelkarp Sep 25, 2017

petderek Sep 25, 2017

samuelkarp Sep 25, 2017

petderek Sep 25, 2017

samuelkarp Sep 25, 2017

petderek Sep 25, 2017

samuelkarp Sep 25, 2017

petderek Sep 25, 2017

samuelkarp Sep 25, 2017

petderek Sep 25, 2017

samuelkarp Sep 25, 2017

jhaynes commented Sep 25, 2017

samuelkarp commented Sep 25, 2017

petderek left a comment

vsiddharth left a comment

vsiddharth Sep 25, 2017

samuelkarp Sep 25, 2017

vsiddharth Sep 25, 2017

vsiddharth Sep 25, 2017

aaithal left a comment

Handle pull for "internal" containers #990

Handle pull for "internal" containers #990

Conversation

samuelkarp commented Sep 25, 2017 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

petderek 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

jhaynes commented Sep 25, 2017

samuelkarp commented Sep 25, 2017

petderek left a comment

Choose a reason for hiding this comment

vsiddharth 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

aaithal left a comment

Choose a reason for hiding this comment

samuelkarp commented Sep 25, 2017 •

edited

Loading