Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

kvm: resource isolators #1404

Merged
merged 6 commits into from Dec 5, 2015
Merged

kvm: resource isolators #1404

merged 6 commits into from Dec 5, 2015

Conversation

DTadrzak
Copy link
Contributor

KVM Added: new functionality to read cpus and mem from resource isolators. Currently kvm uses hard-coded values.
Notes-cpus: By default containers start working on all cpus if at least one app does not have specfied cpus. In the other case, container will be working on aggregate amount of cpus.
Notes-memory: Container memory is a sum of memory required by each app in pod and additional 128MB required by system. If memory of some app is not specified, app memory will be set on default value (128MB).

@jellonek
Copy link
Contributor

LGTM

@DTadrzak DTadrzak changed the title Pod isolators kvm: resource isolators Sep 16, 2015
@jonboulle
Copy link
Contributor

Notes-cpus: By default containers start working on all cpus if at least one app does not have specfied cpus. > In the other case, container will be working on aggregate amount of cpus.
Notes-memory: Container memory is a sum of memory required by each app in pod and additional 128MB
required by system. If memory of some app is not specified, app memory will be set on default value (128MB).

This actually isn't right. Per the spec:

Any isolators applied to the pod will bound any individual isolators applied to applications within the pod.

So really you should read this from the top level isolators field of the pod manifest, e.g.:
https://github.com/appc/spec/blame/master/spec/pods.md#L128
https://github.com/appc/spec/blame/master/spec/pods.md#L171

@jonboulle
Copy link
Contributor

I think the behaviour you've implemented is reasonable in the case that the pod manifest does not specify a pod-level isolation but does have per-app isolators; although note in that case that systemd should be performing that app-level isolation.

app := ra.App
mem, cpus := findResources(app.Isolators)
flag = cpus != 0
cpusSpecified = cpusSpecified && flag
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just:

foundUnspecifiedCpu := false
... loop ... {
  if cpus == 0 {
     foundUnspecifiedCpu = true
  }
}
...
if foundUnspecifiedCpu {
  totalCpu = int64(runtime.NumCPU())
}

@ppalucki
Copy link
Contributor

@jonboulle

Rationale behind this PR was to fix hardcoded values for cpu/mem paramerters passed to hypervisor (lkvm) (now is one 1cpu and 128MB in total).

We have tried to find good scheme for this and decided to:

  • use values from pod manifest if given or use some sane defaults:
    • all cpus (similar to what containers do, not very intrusive, one thread per cpu, but can have impact for super cpus with many >100 cores)
    • 128MB per app and addtionally 128MB for system (kernel+systemd) (giving all the memory, similiar to containers, ended up with significant startup time penalty)

but the part for reading isolators at pod level wasn't implemented (probably because it isn't implemented in non-kvm flavors either).

It wasn't our purpose to implement "resource isolators" in general for pod/app level (title is a little misleading).

although note in that case (with just app isolatores) that systemd should be performing that app-level isolation.

We need to specify cpu/memory for virtual machine and that's way we treated aggregated values from app isolators as hint to configure hypervisor.
It wasn't our goal to replace existing quest level systemd isolation.

@jonboulle jonboulle modified the milestones: v0.10.0, v0.9.0 Oct 6, 2015
@jonboulle jonboulle self-assigned this Oct 6, 2015
@jonboulle
Copy link
Contributor

I am in the process of reworking this a bit with tests but got a bit distracted by appc/spec#517 so I'm going to bump this to the next milestone and fix it after that lands.

@DTadrzak DTadrzak force-pushed the pod_isolators branch 3 times, most recently from 414a308 to 3dc3c9e Compare October 15, 2015 11:03
@jonboulle jonboulle modified the milestones: v0.11.0, v0.10.0 Oct 20, 2015
@krnowak
Copy link
Collaborator

krnowak commented Oct 23, 2015

@jonboulle: What is the status of your rework? I see that @DTadrzak modified the commits, so could you have a look?

@jonboulle jonboulle removed their assignment Oct 23, 2015
@jonboulle
Copy link
Contributor

Clearly I dropped this one on the floor so unassigning in case someone else can pick it up. My WIP branch is here: https://github.com/jonboulle/rkt/tree/intelsdi-x-pod_isolators

@alban alban modified the milestones: v0.12.0, v0.11.0 Nov 13, 2015
@alban alban modified the milestones: v0.13.0, v0.12.0 Nov 26, 2015
ra := &apps[i]
app := ra.App
mem, cpus := findResources(app.Isolators)
cpusSpecified = cpusSpecified && cpus != 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably in place of && there should be || ?
Now cpusSpecified will be always false

Copy link
Contributor

Choose a reason for hiding this comment

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

Right you are. Mea culpa.

@jonboulle
Copy link
Contributor

@squall0gd squall0gd force-pushed the pod_isolators branch 4 times, most recently from 01bc2a3 to f06e72a Compare December 4, 2015 10:43
DTadrzak and others added 6 commits December 4, 2015 04:43
Signed-off-by: DTadrzak <daniel.tadrzak@intel.com>
Required CPUs was wrong calculating in case when there wasn't any CPU assigned
in POD manifest. Now It should allocate all CPUs from platform in scenario w/o
defined CPU isolators in POD manifest.
This test covers resource counting and limiting in (l)KVM pod

Co-Authored: Jonathan Boulle <jonathanboulle@gmail.com>
@jellonek
Copy link
Contributor

jellonek commented Dec 4, 2015

@jonboulle could You rereview this?
IMO in this form this is ready to be merged...

@jonboulle
Copy link
Contributor

Sorry, been swamped this week.

jonboulle added a commit that referenced this pull request Dec 5, 2015
@jonboulle jonboulle merged commit 753c17e into rkt:master Dec 5, 2015
@jellonek
Copy link
Contributor

jellonek commented Dec 7, 2015

thx

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants