Conversation
LGTM |
This actually isn't right. Per the spec:
So really you should read this from the top level |
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 |
There was a problem hiding this comment.
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())
}
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:
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).
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. |
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. |
414a308
to
3dc3c9e
Compare
@jonboulle: What is the status of your rework? I see that @DTadrzak modified the commits, so could you have a look? |
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 |
ra := &apps[i] | ||
app := ra.App | ||
mem, cpus := findResources(app.Isolators) | ||
cpusSpecified = cpusSpecified && cpus != 0 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Could you add https://github.com/jonboulle/rkt/blob/d2f5e283af49539d4e24046b42ec4e38055de3e0/stage1/init/kvm/resources_test.go please? (probably extended a bit..) |
01bc2a3
to
f06e72a
Compare
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>
@jonboulle could You rereview this? |
Sorry, been swamped this week. |
thx |
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).