Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

engine/dockerclient: Add new DockerClient version support #996

Merged
merged 1 commit into from
Oct 17, 2017
Merged

engine/dockerclient: Add new DockerClient version support #996

merged 1 commit into from
Oct 17, 2017

Conversation

sharanyad
Copy link
Contributor

@sharanyad sharanyad commented Sep 28, 2017 •
edited
Loading

Summary

Add new Docker API version 1.25 support

Implementation details

Added 1.25 to supported agent version for init flag support.

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:

Description for the changelog

Support for Docker Client version 1.25

Licensing

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

samuelkarp
samuelkarp reviewed Sep 28, 2017
View reviewed changes
agent/app/agent_capability.go Outdated
@@ -63,7 +66,12 @@ func (agent *ecsAgent) capabilities() []*ecs.Attribute {
// Determine API versions to report as supported. Supported versions are also used for capability-enablement, except
// logging drivers.
for _, version := range agent.dockerClient.SupportedVersions() {
capabilities = appendNameOnlyAttribute(capabilities, capabilityPrefix+"docker-remote-api."+string(version))
dockerVersion, _ := strconv.ParseFloat(string(version), 64)
if dockerVersion >= dockerVersionUsingNewCapabilityPrefix {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the new agent capabilities have the "ecs.capability" prefix. Shouldn't we maintain the same for new remote api versions too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should leave all the api version capabilities consistent with each other.

agent/app/agent_capability.go Outdated
@@ -63,7 +66,12 @@ func (agent *ecsAgent) capabilities() []*ecs.Attribute {
// Determine API versions to report as supported. Supported versions are also used for capability-enablement, except
// logging drivers.
for _, version := range agent.dockerClient.SupportedVersions() {
capabilities = appendNameOnlyAttribute(capabilities, capabilityPrefix+"docker-remote-api."+string(version))
dockerVersion, _ := strconv.ParseFloat(string(version), 64)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably not parse this as a float. 1.2 is not greater than 1.11.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aah yes, will change it.

@sharanyad sharanyad force-pushed the dockerclient-version-capability branch from 0430a03 to 64cbcbe Compare September 29, 2017 17:14
@samuelkarp
Copy link
Contributor

Have you verified that the vendored copy of go-dockerclient has all known fields for API version 1.25 in the Config and HostConfig structs?

@sharanyad
Copy link
Contributor Author

Yes, it does have all the fields in Config and HostConfig.

petderek
petderek reviewed Sep 29, 2017
View reviewed changes
agent/app/agent_capability.go Outdated
@@ -63,7 +64,11 @@ func (agent *ecsAgent) capabilities() []*ecs.Attribute {
// Determine API versions to report as supported. Supported versions are also used for capability-enablement, except
// logging drivers.
for _, version := range agent.dockerClient.SupportedVersions() {
capabilities = appendNameOnlyAttribute(capabilities, capabilityPrefix+"docker-remote-api."+string(version))
if dockerLegacyCapabilityPrefix[string(version)] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like it will be confusing from an administrative perspective. Can we insist that we keep all docker version capabilities using the old style for attributes?

@sharanyad sharanyad force-pushed the dockerclient-version-capability branch from 64cbcbe to 4f5cf48 Compare October 2, 2017 17:33
@sharanyad
Copy link
Contributor Author

Revert back to using old capability prefix for Docker remote API versions to maintain uniformity.

@aaithal
Copy link
Contributor

aaithal commented Oct 4, 2017

@sharanyad doesn't this also need an update to the go-dockerclient lib? If so, please mention that in your PR so that this gets merged only after #987.

@sharanyad
Copy link
Contributor Author

@aaithal Thanks for bringing it up. Yes, this does depend on #987

@sharanyad sharanyad mentioned this pull request Oct 5, 2017
Merged
8 tasks
aaithal
aaithal approved these changes Oct 6, 2017
View reviewed changes
Copy link
Contributor

@aaithal aaithal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: (only after that other PR gets merged).

@sharanyad sharanyad changed the title engine/dockerclient, app: Add new DockerClient version support and required Capability engine/dockerclient, app: Add new DockerClient version support Oct 10, 2017
@sharanyad sharanyad changed the title engine/dockerclient, app: Add new DockerClient version support engine/dockerclient: Add new DockerClient version support Oct 10, 2017
@sharanyad sharanyad force-pushed the dockerclient-version-capability branch from 4f5cf48 to b3323f9 Compare October 10, 2017 21:00
@sharanyad sharanyad requested a review from jhaynes October 10, 2017 21:01
aaithal
aaithal reviewed Oct 11, 2017
View reviewed changes
CHANGELOG.md Outdated
@@ -2,6 +2,7 @@

## UNRELEASED
* Feature - Support for provisioning Tasks with ENIs
* Enhancement - Add support for Docker remote API client version 1.25 [#996](https://github.com/aws/amazon-ecs-agent/pull/996)
Copy link
Contributor

@aaithal aaithal Oct 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you reword this message to make it more relevant to customers? Something like:

  • Feature - Support running init process in containers by adding support for Docker remote API client version 1.25 #996

samuelkarp
samuelkarp suggested changes Oct 11, 2017
View reviewed changes
Copy link
Contributor

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to fix #1010 before we can merge this.

@sharanyad sharanyad mentioned this pull request Oct 12, 2017
Merged
8 tasks
@sharanyad sharanyad force-pushed the dockerclient-version-capability branch 2 times, most recently from acbbb46 to 9dd0a73 Compare October 17, 2017 00:40
@sharanyad sharanyad force-pushed the dockerclient-version-capability branch from 9dd0a73 to acab52b Compare October 17, 2017 01:10
@sharanyad
Copy link
Contributor Author

Fixed issue #1010 that was a blocker for this PR.

@sharanyad sharanyad merged commit 8270744 into aws:dev Oct 17, 2017
@jhaynes jhaynes mentioned this pull request Oct 17, 2017
Merged
@samuelkarp samuelkarp added this to the 1.15.0 milestone Oct 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petderek petderek petderek left review comments

@aaithal aaithal aaithal approved these changes

@samuelkarp samuelkarp samuelkarp requested changes

@richardpen richardpen Awaiting requested review from richardpen

@jhaynes jhaynes Awaiting requested review from jhaynes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.15.0
Development

Successfully merging this pull request may close these issues.
4 participants
@sharanyad @samuelkarp @aaithal @petderek