Support Docker API v1.24 on windows #783

petderek · 2017-04-27T20:30:02Z

Summary

Enables docker API v1.24 for windows, and sets it as the default version.

Implementation details

Refactored some of the versioning and docker client code.

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

Description for the changelog

Moving docker API to 1.24 for windows

Licensing

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

cabbruzzese

lgtm

aaithal · 2017-04-27T23:19:01Z

agent/engine/dockerclient/dockerclientfactory.go

+	docker "github.com/fsouza/go-dockerclient"
+)
+
+type Factory interface {


lint: // Factory ...

aaithal · 2017-04-27T23:19:20Z

agent/engine/dockerclient/dockerclientfactory.go

+	GetClient(version DockerVersion) (dockeriface.Client, error)
+
+	// FindSupportedAPIVersions tests each agent supported API version
+	// against the docker daemon and returns a slice of supported


nit: Docker

aaithal · 2017-04-27T23:20:02Z

agent/engine/dockerclient/dockerclientfactory.go

+	return docker.NewVersionedClient(endpoint, version)
+}
+
+func NewFactory(endpoint string) Factory {


lint: //NewFactory ...

aaithal · 2017-04-27T23:21:38Z

agent/engine/dockerclient/dockerclientfactory_test.go

-		default:
-			t.Fatal("Unrecognized version")
+		client, ok := mockClients[version]
+		if !ok {


Can you use assert.True(t, ok) instead?

aaithal · 2017-04-27T23:22:39Z

agent/engine/dockerclient/dockerclientfactory_test.go

-	if len(versions) != len(expectedVersions) {
-		t.Errorf("Expected %d versions but got %d", len(expectedVersions), len(versions))
+	actualVersions := factory.FindSupportedAPIVersions()
+	if len(expectedVersions) != len(actualVersions) {


Can you use assert.Equal(t, exptectedVersions, actualVersions) or a variation of it?

aaithal · 2017-04-27T23:23:28Z

agent/engine/dockerclient/logging_drivers.go

-	FluentdDriver  LoggingDriver = "fluentd"
-	AwslogsDriver  LoggingDriver = "awslogs"
-	SplunklogsDriver  LoggingDriver = "splunk"
+	JsonFileDriver   LoggingDriver = "json-file"


While you're here, %s/JsonFileDriver/JSONFileDriver

aaithal · 2017-04-27T23:23:44Z

agent/engine/dockerclient/logging_drivers.go

+	JournaldDriver   LoggingDriver = "journald"
+	GelfDriver       LoggingDriver = "gelf"
+	FluentdDriver    LoggingDriver = "fluentd"
+	AwslogsDriver    LoggingDriver = "awslogs"


lint for this as well AWSLogsDriver

aaithal · 2017-04-27T23:25:05Z

scripts/run-integ-tests.ps1

@@ -19,7 +19,7 @@ Invoke-Expression "${PSScriptRoot}\..\misc\image-cleanup-test-images\build.ps1"
 $cwd = (pwd).Path
 try {
  cd "${PSScriptRoot}"
-  go test -tags integration -timeout=10m -v ../agent/engine ../agent/stats
+  go test -tags integration -timeout=15m -v ../agent/engine ../agent/stats


Does this resolve #759 as well?

I had a chat with @adnxn yesterday. The answer is maybe. I want to see if there is a way to run this test more deterministically.

samuelkarp · 2017-04-28T05:56:55Z

agent/engine/dockerclient/dockerclientfactory.go

+	return client, nil
+}
+
+func (f *factory) FindSupportedAPIVersions() []DockerVersion {


Can you add a comment for this?

// FindSupportedAPIVersions returns the API versions which are both known to exist for the Docker daemon to which the agent is connected and are supported by the agent.

Does it need to be on both the interface and the function?

samuelkarp · 2017-04-28T05:59:41Z

agent/engine/dockerclient/dockerclientfactory_test.go

 	newVersionedClient = func(endpoint, version string) (dockeriface.Client, error) {
 		if endpoint != expectedEndpoint {
 			t.Errorf("Expected endpoint %s but was %s", expectedEndpoint, endpoint)
 		}
-		if version != string(defaultVersion) {
-			t.Errorf("Expected version %s but was %s", defaultVersion, version)
+		if version != string(getDefaultVersion()) {


Since you're in this file...want to change it to use github.com/stretchr/testify/assert?

assert.Equal(t, string(getDefaultVersion()), version)

samuelkarp · 2017-04-28T06:00:10Z

agent/engine/dockerclient/versions.go

-func init() {
-	supportedVersions = []DockerVersion{
+// getKnownAPIVersions returns all of the API versions that we know about.
+// It doesn't care if the version is supported by docker or ECS agent


s/docker/Docker/

samuelkarp · 2017-04-28T06:01:40Z

agent/engine/dockerclient/versionsupport_windows.go

+		Version_1_21,
+		Version_1_22,
+		Version_1_23,
+		Version_1_24,


Why is 1.24 included on Windows but not Linux? Should this be 1.23 on both instead?

samuelkarp · 2017-04-28T06:01:57Z

agent/engine/dockerclient/versionsupport_windows.go

-// running with a build tag
-func TestTest(t *testing.T) {}
+func getDefaultVersion() DockerVersion {
+	return Version_1_24


I think the minimum on Windows is 1.23.

You're right. I didn't do the other things mentioned on #687, I can address that separately

samuelkarp · 2017-04-28T06:02:51Z

agent/engine/dockerclient/versionsupport_windows.go

@@ -1,3 +1,4 @@
+// +build windows


I have no idea why git thinks you moved a file here...did you intentionally get rid of dummy_test.go?

Yes! Dummy Tests don't appear to be needed anymore.

samuelkarp · 2017-04-28T06:03:05Z

agent/functional_tests/tests/generated/simpletests_windows/dummy_test.go

@@ -1,22 +0,0 @@
-// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved.


How come this file is removed?

I didn't have trouble running the functional tests when this file was excluded.

richardpen · 2017-04-28T22:41:36Z

agent/engine/dockerclient/dockerclientfactory.go

@@ -0,0 +1,116 @@
+// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved.


Header: 2017 Amaazon.com

Doh! I'll fix it.

richardpen · 2017-04-28T22:46:24Z

agent/engine/dockerclient/versions.go

+		Version_1_25,
+		Version_1_26,
+		Version_1_27,
+		Version_1_28,


Is there any reason put the 25-28 here, while agent doesn't support?

Yeah! Its for future use cases. In the interface for the factory, we only currently expose what is supported by both agent and docker.

@samuelkarp had mentioned to me that it would be useful to understand all of the API versions that the host's docker daemon supports even if we don't leverage it to the main engine code.

aaithal · 2017-05-08T16:53:36Z

agent/functional_tests/util/utils_unix.go

@@ -207,7 +207,7 @@ func (agent *TestAgent) StartAgent() error {
 		return errors.New("Could not inspect agent container: " + err.Error())
 	}
 	agent.IntrospectionURL = "http://localhost:" + containerMetadata.NetworkSettings.Ports["51678/tcp"][0].HostPort
-	err = agent.platformIndependentStartAgent()
+	err = agent.verifyIntrospectionAPI()


minor: you can just do return agent.verifyIntrospectionAPI() here.

aaithal

Neat stuff! Some of the comments are me linting stuff. If we could do the getClient refactor, this would be good to go.

aaithal · 2017-05-31T17:19:53Z

agent/engine/dockerclient/dockerclientfactory_windows_test.go

+)
+
+// We replace docker APIs 1.17 - 12.23 on the windows agent
+const NUMBER_OF_REPLACED_VERSIONS = 7


GoStyle: NumberOfReplacedVersions. Also, this doesn't need to be global scoped.

Re-factoring allowed me to test this differently: I removed this line

aaithal · 2017-05-31T17:20:32Z

agent/engine/dockerclient/versionsupport_windows.go

+	"github.com/aws/amazon-ecs-agent/agent/engine/dockeriface"
+)
+
+const MIN_DOCKER_API_WINDOWS = Version_1_24


GoStyle: MinDockerAPIWindows

aaithal · 2017-05-31T17:21:49Z

agent/engine/dockerclient/versionsupport_unix.go

+	"github.com/aws/amazon-ecs-agent/agent/engine/dockeriface"
+)
+
+func (f *factory) GetClient(version DockerVersion) (dockeriface.Client, error) {


// GetClient gets ...

aaithal · 2017-05-31T17:22:31Z

agent/engine/dockerclient/versionsupport_windows.go

+
+const MIN_DOCKER_API_WINDOWS = Version_1_24
+
+// Override newVersionedClient. We need this because agent makes assumptions


// GetClient overridses ..

aaithal · 2017-05-31T17:35:17Z

agent/engine/dockerclient/dockerclientfactory.go

+		return client, nil
+	}
+
+	f.lock.Lock()


I think we should refactor this method to not hold the lock while we do expensive operations like client.Ping(). Can you instead add get and set methods for the map and acquire the lock only when accessing the lock?

Sounds good. I changed this so that agent detect all API versions on init. This lets me get rid of the lock entirely.

aaithal · 2017-05-31T17:40:10Z

agent/functional_tests/util/utils.go

@@ -115,7 +115,7 @@ type AgentOptions struct {
 }

 // PLatform Independent piece of Agent Startup. Gets executed on both linux and Windows.


While you're here, can you also fix the comment? // verifyIntrospectionAPI verifies ...

aaithal · 2017-05-31T17:40:38Z

agent/functional_tests/util/utils_windows.go

@@ -127,7 +127,7 @@ func (agent *TestAgent) StartAgent() error {
 	}
 	agent.Process = agentInvoke.Process
 	agent.IntrospectionURL = "http://localhost:51678"
-	err = agent.platformIndependentStartAgent()
+	err = agent.verifyIntrospectionAPI()


nit: can just be return agent.verifyIntrospectionAPI()

aaithal · 2017-05-31T17:42:16Z

misc/windows-deploy/hostsetup.ps1

-   route -p delete 169.254.170.2/32
+# This address will only exist on systems that have already set up the routes
+$ip = (Get-NetIPAddress -InterfaceIndex $ifIndex -IPAddress 169.254.170.2 -PrefixLength 32)
+if(!($ip)) {


As a windows noob, I'd really appreciate it if you could add a comment for each line here.

khannag · 2017-06-02T00:02:41Z

misc/windows-deploy/hostsetup.ps1

+Add-VMNetworkAdapter -VMNetworkAdapterName "APIPA" -SwitchName nat -ManagementOS
+$ifIndex = (Get-NetAdapter -Name "*APIPA*" | Sort-Object | Select ifIndex).ifindex
+New-NetIPAddress -InterfaceIndex $ifIndex -IPAddress 169.254.170.2 -PrefixLength 32
+New-NetRoute -DestinationPrefix 172.20.128.0/20 -ifIndex $ifindex


I do not think we should hard code the 172.20.128.0/20 here. The value corresponds to the Subnet CIDR under the IPAM section for the default nat network. Essentially, obtained from the output of docker network inspect nat (default network). I am thinking we should discover the value rather than hardcode it here.

khannag · 2017-06-02T00:10:18Z

misc/windows-deploy/hostsetup.ps1

+# Enable NAT for container-to-host communication
+Add-VMNetworkAdapter -VMNetworkAdapterName "APIPA" -SwitchName nat -ManagementOS
+$ifIndex = (Get-NetAdapter -Name "*APIPA*" | Sort-Object | Select ifIndex).ifindex
+New-NetIPAddress -InterfaceIndex $ifIndex -IPAddress 169.254.170.2 -PrefixLength 32


The only thing which may require a change around reboots is the piece which involves ifIndex - For instance, what if someone adds a second ENI prior to Windows rebooting and running this startup script. The interface index could change in that situation. I dont know though as to how common this possibility or similar possibilities like this are.

So, to handle reboot case: You can change the script to check if a route of the below from currently exists - using Get-NetRoute -DestinationPrefix 172.20.128.0/20. If it exists, you could delete it first using Remove-NetRoute and then add it using the right interface index which will be discovered as done in line 18 above.

khannag · 2017-06-02T00:13:01Z

misc/windows-deploy/amazon-ecs-agent.ps1

@@ -78,18 +78,7 @@ try {

    if([System.Environment]::GetEnvironmentVariable("ECS_ENABLE_TASK_IAM_ROLE", "Machine") -eq "true") {
        LogMsg "IAM roles environment variable is set."
-
-        .\loopback.ps1
        .\hostsetup.ps1


Does the IAM role test failing issue you mentioned to me with docker 17.03.1-ee-3, get resolved now ?

khannag

My comments on the proposed changes below,

samuelkarp · 2017-06-05T18:51:28Z

agent/engine/dockerclient/dockerclientfactory_test.go

@@ -0,0 +1,96 @@
+// +build !integration


Is this !integration just so that the tests don't run multiple times?

samuelkarp · 2017-06-05T18:54:24Z

agent/engine/dockerclient/dockerclientfactory_windows_test.go

+
+	newVersionedClient = func(endpoint, version string) (dockeriface.Client, error) {
+		mockClient := mock_dockeriface.NewMockClient(ctrl)
+		if version == string(MinDockerAPIWindows) {


Rather than doing this where you assert.Equal(t, expectedClient, actualClient), why not assert.Equal(t, MinDockerAPIWindows, version)?

This function is called for every client at agent start-up -- assertion that way won't work anymore.

samuelkarp · 2017-06-05T18:56:12Z

agent/engine/dockerclient/versionsupport_windows.go

+func (f *factory) GetClient(version DockerVersion) (dockeriface.Client, error) {
+	for _, v := range getWindowsReplaceableVersions() {
+		if v == version {
+			version = MinDockerAPIWindows


khannag

LGTM.

samuelkarp · 2017-06-14T17:31:42Z

Can you rebase and update go-dockerclient to include fsouza/go-dockerclient#654?

petderek · 2017-06-14T17:32:53Z

@samuelkarp 👍 I'm working on that now

aaithal

Looks good overall. I have a few minor comments.

aaithal · 2017-06-15T20:18:52Z

agent/engine/dockerclient/dockerclientfactory.go

+	if ok {
+		return client, nil
+	} else {
+		return nil, errors.New("Client not found for docker version: " + string(version))


Error messages should be begin with lowercase. It would also be great to include some context here: "docker client factory: client not found..."

aaithal · 2017-06-15T20:19:56Z

agent/engine/dockerclient/dockerclientfactory_test.go

+	// Validate that agentVersions is a subset of allVersions
+	for _, agentVersion := range getAgentVersions() {
+		if !isKnown(agentVersion) {
+			t.Errorf("Version not found in known Docker Versions: %s", agentVersion)


Can you use assert.True here instead?

aaithal · 2017-06-15T20:21:28Z

agent/engine/dockerclient/dockerclientfactory_windows_test.go

+	}
+
+	factory := NewFactory(expectedEndpoint)
+	actualClient, _ := factory.GetClient(Version_1_19)


Can you also do assert.NoError(t, err) by catching the error when it's returned?

samuelkarp

Looks good after you address @aaithal's feedback.

aaithal

* Separates docker API version support for windows * Sets docker API version default to 1.23 for windows

petderek requested review from cabbruzzese and samuelkarp April 27, 2017 20:30

cabbruzzese approved these changes Apr 27, 2017

View reviewed changes

aaithal requested changes Apr 27, 2017

View reviewed changes

aaithal mentioned this pull request Apr 28, 2017

Enable 1.24 API #687

Closed

samuelkarp reviewed Apr 28, 2017

View reviewed changes

richardpen reviewed Apr 28, 2017

View reviewed changes

samuelkarp added the os/windows label Apr 29, 2017

aaithal reviewed May 8, 2017

View reviewed changes

aaithal approved these changes May 8, 2017

View reviewed changes

aaithal requested changes May 31, 2017

View reviewed changes

khannag reviewed Jun 2, 2017

View reviewed changes

samuelkarp mentioned this pull request Jun 5, 2017

Support for Docker >= 1.13.0 on Windows #729

Closed

samuelkarp reviewed Jun 5, 2017

View reviewed changes

khannag approved these changes Jun 14, 2017

View reviewed changes

petderek force-pushed the windows-api-versioning branch from 09be263 to 266983f Compare June 15, 2017 18:45

aaithal reviewed Jun 15, 2017

View reviewed changes

samuelkarp approved these changes Jun 15, 2017

View reviewed changes

aaithal approved these changes Jun 15, 2017

View reviewed changes

petderek added 2 commits June 15, 2017 14:20

Changing version defaults for Windows.

db0fafb

* Separates docker API version support for windows * Sets docker API version default to 1.23 for windows

updating go-dockerclient and dependencies

1cb0001

petderek force-pushed the windows-api-versioning branch from 36c4da9 to 1cb0001 Compare June 15, 2017 21:23

petderek merged commit 1cb0001 into aws:dev Jun 15, 2017

samuelkarp added this to the 1.14.4 milestone Jul 3, 2017

petderek deleted the windows-api-versioning branch August 15, 2017 19:15

petderek restored the windows-api-versioning branch August 15, 2017 19:15

cabbruzzese mentioned this pull request Aug 30, 2017

ECS Windows Agent v1.14.4 #954

Closed

petderek deleted the windows-api-versioning branch October 10, 2017 21:28

		@@ -1,22 +0,0 @@
		// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved.

		@@ -0,0 +1,116 @@
		// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved.


		const MIN_DOCKER_API_WINDOWS = Version_1_24

		// Override newVersionedClient. We need this because agent makes assumptions

		@@ -115,7 +115,7 @@ type AgentOptions struct {
		}

		// PLatform Independent piece of Agent Startup. Gets executed on both linux and Windows.

Support Docker API v1.24 on windows #783

Support Docker API v1.24 on windows #783

Conversation

petderek commented Apr 27, 2017

Summary

Implementation details

Testing

Description for the changelog

Licensing

cabbruzzese 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

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

aaithal left a comment • edited Loading

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

khannag Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

khannag Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

khannag Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

khannag 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

khannag left a comment

Choose a reason for hiding this comment

samuelkarp commented Jun 14, 2017

petderek commented Jun 14, 2017

aaithal 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

samuelkarp left a comment

Choose a reason for hiding this comment

aaithal left a comment

Choose a reason for hiding this comment

aaithal left a comment •

edited

Loading

khannag Jun 2, 2017 •

edited

Loading

khannag Jun 2, 2017 •

edited

Loading

khannag Jun 2, 2017 •

edited

Loading