Skip to content
This repository was archived by the owner on Aug 14, 2020. It is now read-only.

NewResourceCPUIsolator/NewResourceMemoryIsolator fails if 'request' or 'limit' is empty #562

Open
yifan-gu opened this issue Jan 29, 2016 · 7 comments

Comments

@yifan-gu
Copy link
Contributor

@sjpotter This is the cause of the errors when creating a pod that has only request or limit.
Not sure we should fix this in the spec or in rktnetes.

cc @jonboulle

@jonboulle
Copy link
Contributor

#484

@jonboulle
Copy link
Contributor

@yifan-gu @sjpotter what's the desired experience here?

@yifan-gu
Copy link
Contributor Author

@jonboulle Jumped in the rkt code, seems for now only the Limit is being used? https://github.com/coreos/rkt/blob/master/stage1/init/common/pod.go#L336-L342

IMO, the desired experience would be this kubernetes/kubernetes#14173

I can fix in the rktnetes code for now.

@alban
Copy link
Member

alban commented Feb 1, 2016

@yifan-gu Exact, rkt only use the Limit now...

It is not clear to me what is the semantic of Limit and Request. For example, with CPUShares, the value is relative to the values of other containers in the same slice. It does not really match the explanation given in the resource/cpu documentation.

Kubernetes seems to only care about the cpu "limit" (relative weight) and not the cpu "request" (unused):
https://github.com/kubernetes/kubernetes/blob/master/docs/user-guide/compute-resources.md#how-pods-with-resource-limits-are-run

@jonboulle
Copy link
Contributor

@alban in rkt we use CPUQuota, not CPUShares:

      Assign the specified CPU time quota to the processes executed. Takes a percentage value, suffixed with
       "%". The percentage specifies how much CPU time the unit shall get at maximum, relative to the total CPU
       time available on one CPU. Use values > 100% for allotting CPU time on more than one CPU. This controls
       the "cpu.cfs_quota_us" control group attribute. For details about this control group attribute, see
       sched-design-CFS.txt[2].

@jonboulle
Copy link
Contributor

@yifan-gu I am not really sure what you want here - new constructors? e.g. NewLimitOnlyResourceCPUIsolator? I don't think it makes much sense to change the signature of these ones

@yifan-gu
Copy link
Contributor Author

yifan-gu commented Feb 9, 2016

@jonboulle @alban We don't need to change the name or signature of the function. Do you think #566 is reasonable?

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

No branches or pull requests

3 participants