pkg: oci: support OCI spec memory limit #341

wcwxyz · 2017-08-15T07:24:13Z

We should run VM(container) regarding OCI spec config.json memory limit.

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

--

This pull request resolves clearcontainers/runtime#381
The original patch is at clearcontainers/runtime#390

coveralls · 2017-08-15T07:31:06Z

Coverage remained the same at 66.502% when pulling a2fa066 on wcwxyz:per-container-memory into 58ebe95 on containers:master.

sboeuf · 2017-08-16T00:21:01Z

pkg/oci/utils.go

@@ -286,6 +287,35 @@ func (spec *CompatOCISpec) PodID() (string, error) {
 	return "", fmt.Errorf("Could not find pod ID")
 }

+func ociResourceUpdateRuntimeConfig(ocispec CompatOCISpec, config *RuntimeConfig) error {


Please create instead a function:

func vmConfig(ocispec CompatOCISpec, runtime RuntimeConfig) (Resources, error) { ... }

In case there is no memory limit defined by the config.json file, we fallback to what is provided by RuntimeConfig. Otherwise, we take the value provided by the config.json.

sboeuf · 2017-08-16T00:23:31Z

pkg/oci/utils.go

@@ -286,6 +287,35 @@ func (spec *CompatOCISpec) PodID() (string, error) {
 	return "", fmt.Errorf("Could not find pod ID")
 }

+func ociResourceUpdateRuntimeConfig(ocispec CompatOCISpec, config *RuntimeConfig) error {
+	if ocispec.Linux == nil {


Make sure you gather all the statements together as they end up with the same result:

if ocispec.Linux == nil || ocispec.Linux.Resources == nil || ocispec.Linux.Resources.Memory == nil || ocispec.Linux.Resources.Memory.Limit == nil { return nil }

sboeuf · 2017-08-16T00:31:01Z

pkg/oci/utils.go

+	memBytes := *ocispec.Linux.Resources.Memory.Limit
+
+	// round up memory bytes to MB
+	mem := int(math.Ceil(float64(memBytes) / 1024 / 1024))


We don't need that much precision, I think this should be enough

mem := uint(memBytes / (1024 * 1024))

I'm ok with less precision, because this is trivial.

But @grahamwhaley suggested that we should round up at clearcontainers/runtime#390 (comment). I just quoted here:

maybe we should round this up. Docker allows fine granularity (down to bytes). I think it is probably better we allocate more than requested rather than too little

@grahamwhaley what do you think now?

I still think rounding up is the technically right thing to do. I'd rather hand a container slightly more than requested, rather than artificially starve it. tbh though, the amount we are likely to drop as a % from not rounding up is tiny - given the smallest memory we can probably support might be 128m or so.
If we don't like the float math, we could do something integer like this to round up:

mem = (memBytes + ((1024*1024)-1))) / (1024*1024)

@grahamwhaley yes I like this one, you add 1MiB to make sure we round this up. My main concern was to avoid playing with float64 numbers whilst we don't really need that much precision.

Thanks @grahamwhaley @sboeuf . I'll update the patch.

sboeuf · 2017-08-16T08:01:42Z

@wcwxyz Global question here, why do you use [RFC] prefix ? What does that mean ?

wcwxyz · 2017-08-16T08:08:01Z

@sboeuf It's Request For Comment, meaning the idea or design needs to be discussed or commented.

sboeuf · 2017-08-16T08:15:28Z

@wcwxyz oh okay, I didn't know. I think you can remove it since we consider every PR as something we want to review thoroughly :)

wcwxyz · 2017-08-16T08:17:49Z

@sboeuf Sure. Thanks for heads-up.

grahamwhaley · 2017-08-16T08:49:49Z

:-) RFC sort of comes from here: https://www.ietf.org/rfc.html, but funnily enough most of those RFCs pretty much become 'standards', but are still called RFCs :-)
Adding RFC here I think is fine, it is a common idiom - but, you should probably also add our 'do-not-merge' github label at the same time, to indicate this is something to discuss and work on and not to immediately merge, until agreement has been found.

coveralls · 2017-08-16T10:13:36Z

Coverage remained the same at 66.527% when pulling 551b0ec on wcwxyz:per-container-memory into af35aeb on containers:master.

sboeuf

Two tiny comments here.
Also, could you please add some unit tests for vmConfig().

sboeuf · 2017-08-16T13:16:20Z

pkg/oci/utils.go

+
+	// round up memory to 1MB
+	mem := uint((memBytes + (1024*1024 - 1)) / (1024 * 1024))
+	if mem <= 0 {


We should check memBytes instead of mem, that's the one which could be negative. You should make this check right after you retrieved memBytes value.

You're right. I missed this one while updating my code last time.

sboeuf · 2017-08-16T13:16:24Z

pkg/oci/utils.go

+	cpu := config.VMConfig.VCPUs
+
+	return vc.Resources{
+		VCPUs:  cpu,


Assign directly VCPUs to config.VMConfig.VCPUs here. If we need to use a temporary variable for later, let's do that in a follow up patch.

Sure. Will update.

We should run VM(container) regarding OCI spec config.json memory limit. Signed-off-by: WANG Chao <chao.wang@ucloud.cn>

coveralls · 2017-08-16T18:02:31Z

Coverage remained the same at 66.527% when pulling 1b02f69 on wcwxyz:per-container-memory into af35aeb on containers:master.

sboeuf

Thanks for the changes :)
LGTM

"docker run -m" is now supported [1], so it no longer needs to be specified in the limitations document. Partially fixes clearcontainers#494. [1] - See containers/virtcontainers#341 and clearcontainers#381. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

wcwxyz changed the title ~~pkg: oci: support OCI spec memory limit~~ [RFC] pkg: oci: support OCI spec memory limit Aug 15, 2017

sboeuf suggested changes Aug 16, 2017

View reviewed changes

sboeuf mentioned this pull request Aug 16, 2017

[RFC] create: support oci spec memory limit clearcontainers/runtime#390

Closed

wcwxyz changed the title ~~[RFC] pkg: oci: support OCI spec memory limit~~ pkg: oci: support OCI spec memory limit Aug 16, 2017

sboeuf suggested changes Aug 16, 2017

View reviewed changes

pkg: oci: support OCI spec memory limit

1b02f69

We should run VM(container) regarding OCI spec config.json memory limit. Signed-off-by: WANG Chao <chao.wang@ucloud.cn>

sboeuf approved these changes Aug 16, 2017

View reviewed changes

sboeuf merged commit 710c4bb into containers:master Aug 16, 2017

jodh-intel mentioned this pull request Sep 8, 2017

Add support for managing compute resources clearcontainers/runtime#341

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg: oci: support OCI spec memory limit #341

pkg: oci: support OCI spec memory limit #341

wcwxyz commented Aug 15, 2017 •

edited

coveralls commented Aug 15, 2017 •

edited

sboeuf Aug 16, 2017

wcwxyz Aug 16, 2017

sboeuf Aug 16, 2017

wcwxyz Aug 16, 2017

sboeuf Aug 16, 2017

wcwxyz Aug 16, 2017

grahamwhaley Aug 16, 2017

sboeuf Aug 16, 2017

wcwxyz Aug 16, 2017

sboeuf commented Aug 16, 2017

wcwxyz commented Aug 16, 2017

sboeuf commented Aug 16, 2017

wcwxyz commented Aug 16, 2017

grahamwhaley commented Aug 16, 2017

coveralls commented Aug 16, 2017 •

edited

sboeuf left a comment

sboeuf Aug 16, 2017

wcwxyz Aug 16, 2017

sboeuf Aug 16, 2017

wcwxyz Aug 16, 2017

coveralls commented Aug 16, 2017 •

edited

sboeuf left a comment

pkg: oci: support OCI spec memory limit #341

pkg: oci: support OCI spec memory limit #341

Conversation

wcwxyz commented Aug 15, 2017 • edited

coveralls commented Aug 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sboeuf commented Aug 16, 2017

wcwxyz commented Aug 16, 2017

sboeuf commented Aug 16, 2017

wcwxyz commented Aug 16, 2017

grahamwhaley commented Aug 16, 2017

coveralls commented Aug 16, 2017 • edited

sboeuf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 16, 2017 • edited

sboeuf left a comment

Choose a reason for hiding this comment

wcwxyz commented Aug 15, 2017 •

edited

coveralls commented Aug 15, 2017 •

edited

coveralls commented Aug 16, 2017 •

edited

coveralls commented Aug 16, 2017 •

edited