topology-aware: add CPU class support. - #729
Conversation
65af27a to
a216b23
Compare
askervin
left a comment
askervin
left a comment
There was a problem hiding this comment.
Adding one note... I'll continue review soon...
| errs = append(errs, err) | ||
| } | ||
|
|
||
| if c.SharedCpuClass == "" { |
There was a problem hiding this comment.
For convenience, balloons policy handles CPU class name "default" differently than others: if a balloon type does not specify any CPU class, the "default" CPU class will be applied on CPUs of its instances.
https://containers.github.io/nri-plugins/stable/docs/resource-policy/policy/balloons.html#cpu-tuning
I was thinking if it would make sense to use c.{Shared,Reserved,DefaultExclusive}CpuClass = "default" when a user has specified "default" CPU class. Pros: one could use the same CPU class configuration if evaluating and switching between policies, and it might be more intuitive than exclusive allocations using sharedCPUClass by default. Cons: if the existence of the "default" CPU class is not required and the current mechanism for figuring out a default class would be used in that case, it would make make the whole thing unnecessarily complex.
Having special semantics for a class named "default" fits quite naturally in balloons, as there is the "default" balloon type, too. In T-A, there is no similar context, so this might not be a good idea.
| }) | ||
| if len(hints.Prefer) > 0 { | ||
| ccCpus := cpus.Intersection(hints.Prefer[0].Cpus) | ||
| if ccCpus.Equals(cpus) { |
There was a problem hiding this comment.
This will be more useful with #719 because then we can go through alternative (equally acceptable) cpuset hints. Then it'll be more likely that there is a cpuset that is aligned with the set from GetCPUOffer().
| if !p.cpuClasses.IsKnownClass(cc) { | ||
| return nil, policyError("%s: nonexistent CPU class %q requested", | ||
| container.PrettyName(), cc) | ||
| } |
There was a problem hiding this comment.
Would it be good to log a warning, an error, or even return policyError if a non-Guaranteed container is annotated with cpu-class annotation?
There was a problem hiding this comment.
Logging a warning definitely makes sense. Though we need to do it for containers not eligible for exclusive CPU allocation, which is not quite the same as non-Guaranteed QoS class.
Why it's not that obvious/I'm a bit hesitant on outright erroring out is that with effective annotations a common pattern is to assign a pod-wide default with .../pod: default-class and then punch exceptions into it with container-specific further annotations like .../container.$CTR0: some-class and .../container.$CTR1: other-class. And in that case if you have containers with partial/shared pool CPU allocations, it is normal/an expected side-effect that their CPU class resolves to default-class. Though, it would be technically possible to possible to check for this and allow it, but error out in other cases...
Avoid using 'balloon' in cpuclass configuration comments / description. Use the more neutral 'policy' term. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Rework pool CPU allocation, splitting out exclusive and shared CPU picking logic to separate functions and making them dry-runnable. This should allow scoring to evaluate the actual pool offered CPUs at will. Add pool/supply.GetCPUOffer() which dry-runs pool CPU allocation for a given request. Rename existing GetOffer() to GetMemOffer() for symmetricity/better consistency. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Add basic CPU class support. Exlusively allocated CPUs (of
Guaranteed QoS class containers), CPUs of shared pools and
CPUs of the reserved pool can be assigned to CPU classes.
Shared and reserved pool classes are configured. Exclusive
CPU classes can be configured (default class) or annotated
using an effective annotation with key 'cpu-class' on the
container's pod.
There are still missing bits in the current implementation:
- extra checks on reconfiguration
o all CPU classes in use are defined
- (maybe) extra checks on startup
o all CPU classes in use are defined
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
a216b23 to
ff182b1
Compare
2 participants
This patch series adds basic CPU class support to the topology-aware policy.
With these patches in place, CPU classes can now be assigned to
CPU class support is turned on by defining some CPU classes in the configuration and specifying the CPU class for shared pool CPU by setting
sharedCPUClassin the configuration. This is the minimal required/valid configuration. Additionally, an optional reserved pool CPU class can be specified, for CPUs of the reserved pool, by settingreservedCPUClassin the configuration. An unset reserved class defaults to the shared class.Also optionally a default CPU class for exclusive CPUs can be specified by setting
defaultExclusiveCPUClassin the configuration. If set, exclusive CPUs of otherwise unnannotated containers are assigned to the default exclusive class. Containers can be annotated with a CPU class using thecpu-class[.resource-policy.nri.io]effective annotation base key. Exclusive CPUs of annotated containers are assigned to the annotated class.Once CPUs are released from exclusive use and returned to a shared pool, they are assigned to the shared CPU class.
This patch series still misses some checks for reconfiguration and end-to-end test cases. Hence it is marked as draft.