-
Notifications
You must be signed in to change notification settings - Fork 616
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.
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
ecs-init support for old docker engines (pre docker 17.x) and future docker engines (when api 1.25 is deprecated). #4080
Conversation
58be102
to
d5398da
Compare
@@ -157,6 +241,9 @@ func isNetworkError(err error) bool { | |||
func isRetryablePingError(err error) bool { | |||
godockerError, ok := err.(*godocker.Error) | |||
if ok { | |||
if godockerError.Status == http.StatusBadRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when provided with an unsupported docker api version, docker returns "Bad Request" (status code 400), so don't get into a retry loop if the API version is not supported (client errors in general probably shouldn't be retried at all, but just sticking with 400 for now to avoid changing behavior more than necessary).
@@ -1,6 +1,3 @@ | |||
//go:build test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these tests were not running because of these flags, so some had to be fixed as they havent been passing for a long time (or possibly not ever)
func preferredAPIVersions() []string { | ||
return []string{ | ||
"1.25", | ||
"1.26", | ||
"1.27", | ||
"1.28", | ||
"1.29", | ||
"1.30", | ||
"1.31", | ||
"1.32", | ||
"1.33", | ||
"1.34", | ||
"1.35", | ||
"1.36", | ||
"1.37", | ||
"1.38", | ||
"1.39", | ||
"1.40", | ||
"1.41", | ||
"1.42", | ||
"1.43", | ||
"1.44", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a plan to maintain this list and "backup versions" list going forwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No plan in particular, this more just matches what we're doing in ecs-agent:
func GetKnownAPIVersions() []DockerVersion { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So deprecation of older API versions would be the forcing function for us to update this list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I guess deprecation of older versions, or potentially if a new ECS feature comes along that is only supported in a newer version of the docker API (though I think that's fairly unlikely at this point).
func backupAPIVersions() []string { | ||
return []string{ | ||
"1.21", | ||
"1.22", | ||
"1.23", | ||
"1.24", | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC before this change ecs-init was not compatible with environments with Docker engines that do not support API versions >= 1.25. But this PR makes ecs-init compatible with such environments. So, does that mean the older API versions were artificially made incompatible with ecs-init?
I understand that Task ENI is not supported on API versions < 1.25, but ecs-init doesn't actually make any Docker API calls that are not supported on API versions < 1.25?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ecs-init doesn't actually make any Docker API calls that are not supported on API versions < 1.25?
Not by default, but I will check on the various flags that ecs-init can add during startup to see if there are any that are not supported before docker 1.25.
d5398da
to
0061700
Compare
…docker engines (when api 1.25 is deprecated).
0061700
to
c60a86c
Compare
Summary
note: this is an ecs-init followup to a similar change already made to the agent codebase: #4075
Recently, docker engine has deprecated docker API versions 1.17-1.23: https://docs.docker.com/engine/deprecated/#deprecate-legacy-api-versions
In the above announcement, they state that the minimum supported API version (currently 1.24) would continue to be raised in future versions:
This will soon be a problem for ecs-init, because currently ecs-init hardcodes it's API version to 1.25 and does not have any ability to detect or switch to supported versions.
Implementation details
This PR changes ecs-init to keep a list of known API versions rather than a single API version. When ecs-init starts up, it will retry creating the docker client until it finds an API version that works.
As part of this PR, we are both supporting future versions of docker and very old versions of docker which did not have support for API version 1.25.
For example on docker 1.10 ecs-init previously failed, but will now startup with messages like this (note that it fails on versions 1.25-1.44, then succeeds on 1.21):
Testing
New tests cover the changes: yes
Description for the changelog
Enhancement: ecs-init support for old docker engines (pre docker 17.x) and future docker engines (when api 1.25 is deprecated).
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.