Skip to content
This repository has been archived by the owner on Apr 3, 2018. It is now read-only.

pkg: oci: support OCI spec cpu quota/period #344

Merged
merged 1 commit into from Aug 17, 2017
Merged

pkg: oci: support OCI spec cpu quota/period #344

merged 1 commit into from Aug 17, 2017

Conversation

wcwxyz
Copy link
Contributor

@wcwxyz wcwxyz commented Aug 17, 2017

We should run VM(container) regarding OCI spec config.json cpu quota and
period.

The number of VM VCPUs is defined by quota/period (round up to 1).

Signed-off-by: WANG Chao chao.wang@ucloud.cn

--
Original issues is clearcontainers/runtime#393

@coveralls
Copy link

coveralls commented Aug 17, 2017

Coverage Status

Coverage remained the same at 66.527% when pulling 8807c49 on wcwxyz:per-container-cpu into 710c4bb on containers:master.

Copy link
Collaborator

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

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

One comment but the change looks fine :)

pkg/oci/utils.go Outdated
quota := *ocispec.Linux.Resources.CPU.Quota
period := *ocispec.Linux.Resources.CPU.Period

if quota < 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We want to test:

if quota <= 0 {

as we did for the memory. Otherwise, in case quota == 0, we are gonna end up with resources.VCPUs = 0 because (period -1) / period will return 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I also will check period is not zero.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh yes good point !

We should run VM(container) regarding OCI spec config.json cpu quota and
period.

The number of VM VCPUs is defined by quota/period (round up to 1).

Signed-off-by: WANG Chao <chao.wang@ucloud.cn>
@coveralls
Copy link

coveralls commented Aug 17, 2017

Coverage Status

Coverage remained the same at 66.527% when pulling f82fc60 on wcwxyz:per-container-cpu into 710c4bb on containers:master.

Copy link
Collaborator

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

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

LGTM

@sboeuf
Copy link
Collaborator

sboeuf commented Aug 17, 2017

LGTM

@sboeuf
Copy link
Collaborator

sboeuf commented Aug 17, 2017

@jodh-intel @grahamwhaley please give us your opinion before I merge this.

@jodh-intel
Copy link
Collaborator

jodh-intel commented Aug 17, 2017

lgtm

Approved with PullApprove

@sboeuf sboeuf merged commit 71b5c95 into containers:master Aug 17, 2017
jodh-intel added a commit to jodh-intel/runtime that referenced this pull request Sep 8, 2017
Mention that although `--cpus` is not supported, `--cpu-quota` and `--cpu-period` are [1].

Fixes clearcontainers#494.

[1] - See containers/virtcontainers#344 and
clearcontainers#436.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
jodh-intel added a commit to jodh-intel/runtime that referenced this pull request Sep 8, 2017
Mention that although `--cpus` is not supported, `--cpu-quota` and `--cpu-period` are [1].

Fixes clearcontainers#494.

[1] - See containers/virtcontainers#344 and
clearcontainers#436.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants