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.

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

Move cpu_shares and cpuset_cpu to HostConfig when API >= 1.18 #1160

Merged
merged 1 commit into from
Aug 23, 2016

Conversation

joshpurvis
Copy link
Contributor

@joshpurvis joshpurvis commented Aug 15, 2016

This PR moves the cpu_share and cpuset_cpus options to HostConfig, per the Docker remote API >= 1.18.

Previously cpu_share and cpuset arguments were passed into create_container as kwargs. I've added an InvalidVersion exception there, when the API version is < 1.18 and these arguments are passed. I can't seem to find documentation for < 1.18 to know if this should be a warning instead?

I should also note I've renamed cpuset to cpuset_cpus to match the existing container_update functionality, which is already expecting these being in HostConfig.

Related issues: #1030, docker/compose#3284, docker-archive/classicswarm#1983

Signed-off-by: Josh Purvis <joshua.purvis@gmail.com>
@joshpurvis
Copy link
Contributor Author

joshpurvis commented Aug 15, 2016

I've pushed a change/rebased the branch to fix an issue with the InvalidVersion error on the earlier API versions. (I had the API version comparison reversed in create_container_config.)

I also switched it from an error to a deprecation warning, as this mistake made me realize the old cpushare/cpuset unit tests would be broken when testing against newer API versions. (those that still pass these as kwargs into create_container:

def test_create_container_with_cpu_shares(self):
)

Let me know if the old unit tests should simply be removed.

@shin-
Copy link
Contributor

shin- commented Aug 16, 2016

Thanks, changes LGTM!

@shin- shin- added this to the 1.10.0 milestone Aug 16, 2016
@shin- shin- merged commit 7d147c8 into docker:master Aug 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants