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

Moved cpu_shares and cpuset to unsupported keys #543

Merged
merged 1 commit into from
Apr 6, 2017

Conversation

surajnarwade
Copy link
Contributor

Resolves #272 and #267

As there is no direct mapping of cpushares and cpuset to kubernetes,
it should not be read and should be moved to unsupported keys.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 5, 2017
@cdrage
Copy link
Member

cdrage commented Apr 5, 2017

LGTM other than the small comment.

Perhaps it would be good (in the future) to advise users to use cpu_quota instead of cpu_share or cpu_set!

Thanks for the contribution!

@@ -67,8 +67,6 @@ type ServiceConfig struct {
Network []string `compose:"network" bundle:"Networks"`
Labels map[string]string `compose:"labels" bundle:"Labels"`
Annotations map[string]string `compose:"" bundle:""`
CPUSet string `compose:"cpuset" bundle:""`
CPUShares int64 `compose:"cpu_shares" bundle:""`
Copy link
Member

Choose a reason for hiding this comment

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

Please don't remove these, as you can see in the "unsupported keys" section in compose.go, we still grab these from the docker-compose file for (possible) future integration!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, got it :)

Resolves kubernetes#272 and kubernetes#267

As there is no direct mapping of `cpushares` and `cpuset` to kubernetes,
it should not be read and should be moved to unsupported keys.
Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

it looks like only issue with this was addressed
LGTM
but will let @cdrage to merge it

@cdrage
Copy link
Member

cdrage commented Apr 6, 2017

LGTM

@cdrage cdrage merged commit 533df01 into kubernetes:master Apr 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants