config: add an option to control memory size calculation

[liwei]

The sandbox memory overhead was not take into account when
calculating sandbox memory quota size, that cause the following
issues:

• scheduler can't get an accurate memory usage information
• memory size info in container not match user specified memory limit,
  that would confuse the user

Before the pod overhead patch1 got merged, let's add an option
to control memory size calculation to workaround these issues.

Fixes: #1822
Signed-off-by: Li Wei wei@hyper.sh

[bergwolf]

How about using the new option to control both CPU and memory calculation whether the initial guest CPU/memory is counted in quota?

[bergwolf]

btw, travis failed at

pkg/katautils/config.go:84:17: struct of size 296 bytes could be of size 288 bytes (maligned)
type hypervisor struct {

[liwei]

How about using the new option to control both CPU and memory calculation whether the initial guest CPU/memory is counted in quota?

Sounds good, I will update the PR.

[liwei]

PR updated, @bergwolf PTAL

[grahamwhaley]

Please ensure any/all documentation relating to any resource calculations is also updated - as that is one of the first places the user checks, or we send them for the answers.

[Pennyzct]

/test

[codecov]

Codecov Report

Merging #1823 into master will increase coverage by 2.02%.
The diff coverage is 53.84%.

@@            Coverage Diff             @@
##           master    #1823      +/-   ##
==========================================
+ Coverage    52.5%   54.52%   +2.02%     
==========================================
  Files         108      106       -2     
  Lines       13951    13059     -892     
==========================================
- Hits         7325     7121     -204     
+ Misses       5756     5090     -666     
+ Partials      870      848      -22

[egernst]

Resource quota is a kubernetes admission controller -- is that what you're referring to in this name?

[liwei]

No, not really, naming is the hardest part, do you have some suggestion?

[egernst]

s/haed/head

[jcvenegas]

Hey @liwei thanks for the PR, this looks good just a few nits but looks good

[jcvenegas]

Nit: Blank

[jcvenegas]

Could you explain here when or how this should be enabled?

[liwei]

Per @grahamwhaley 's comment, I'm planning to update the document to explain this, WDYT?

[jcvenegas]

ack

[jcvenegas]

Lets add in the documentation, I wanted to add a bit more of information but I probably add all the context in a one-line summary may be difficult.

[jcvenegas]

@jodh-intel we should really unify this into one config and customization ? Or something like that

[jodh-intel]

Hi @jcvenegas - slightly confused - are you suggesting we have separate options for CPU and memory?

[liwei]

Please ensure any/all documentation relating to any resource calculations is also updated - as that is one of the first places the user checks, or we send them for the answers.

Hi @grahamwhaley , after some searching, I found that cpu-constraints.md may be a good place to explain this config option, I'm planning to rename that document to "resource-constraints" and explain both memory and CPU constraints in it, WDYT?

Do you know if there are other documents that need an update?

[grahamwhaley]

Hi @grahamwhaley , after some searching, I found that cpu-constraints.md may be a good place to explain this config option, I'm planning to rename that document to "resource-constraints" and explain both memory and CPU constraints in it, WDYT?

Do you know if there are other documents that need an update?

That looks like a good place to me. @jcvenegas worked in that area recently, so may have some useful input?

[egernst]

I’d rather gate this until docs PR is ready to be merge as well.

[egernst]

Thinking through this more...

I generally like the change, I just want to look at it holistically in the stack, since we are introducing "one more configuration" and not entirely fixing the issue.

Problem statements

System tracking, stability

Stability is most important, and anything we can do to address this is wise.

Having said that, the guest overheads for memory (and CPU, if there are any) are only a (small?) piece of the sandbox runtime overhead story. The host-side overhead of running the VMM, the vCPUS, the IOthreads, and any other 'runtime infra' are likely much larger. This change will just slightly improve things. There are still further gaps on how these are managed wrt cgroups today, and @jcvenegas is working to fix this (ie, today we don't place everything in cgroups for fear of being OOMd; we need this to be configurable so we can "do the right thing" and manage in pod/sandbox cgroup on host if the creater of the cgroup sizes it appropriately for sandboxed-runtime (ie, accounts for the overhead).

While it does improve stability slightly (this is most important), if enabled, it does result in unexpected behavior for the workload. If the workload requires all of the memory it requests, things will start getting OOMd, worse case.

User Confusion

If end users are confused, I'd argue this is an issue in our documentation/education, not in our code.

Names, and usage of the default hypervisor parameters

Early on I was skeptical of utilizing the hypervisor default values as an overhead, as opposed to a default value when no resource requirements are specified for the sandbox. In the end, today
we treat it as both a default value, as well as a guest-overhead. I think this is problematic, since appropriate guest artifact overhead (that is, overhead of running the kernel and system services in the guest, from a CPU and memory perspective) would be too small to run both a best-effort workload and minimal services.

• Let's say, though, that we only have users who "do the right thing" -- this is, that we focus on burstable and guaranteed QoS. If that is the case, it would be appropriate to size the 'defaults' to match the intent you are showing here, which is overhead. We can make sure this happens, in a k8s context, by setting default resources for all pods.

Thinking through this, I wonder if it makes sense to have separate "default" for burstable pods(a'la containers with no resource requirements specified) and guest-overhead (we'd need to calculate default value for running just the kernel and agent).

So, before introducing the configuration in this PR, I'd like to consider clarifying, and perhaps separating out default handling and guest-overheads.

[liwei]

Hi @egernst,

I agreed that "the guest overheads are only a piece of the sandbox overhead", for the accuracy of resource limitation, placing everything in cgroup as @jcvenegas doing looks good to me. We should state the sandbox overhead in our document and let the user consider the overhead when setting up the container's resource limit to avoid OOM.

To the separation of default and overhead, I have no strong preference for it but do we need that complexity? It's hard for us to get a perfect solution to satisfy all, so, how about treating the resource setup in kata configuration as the default when no container resource limit is set and obey resource limit if that was set up by the user(set the resource limit on host side cgroup), if user's setting is too small to launch kata guest, just let it fail.

So, what do you think? @egernst @bergwolf @gnawux

[egernst]

(sorry for slow reply - I'm hoping we can have a PR in next several days to clarify all of this in documentation).

[raravena80]

@liwei any updates on this PR? Thx!

[raravena80]

@liwei any updates?

Your weekly Kata herder.

[yichengliu58]

Hi @liwei
Any updates about this PR ?
I've seen there is a wiki explaining resource constraints https://github.com/kata-containers/documentation/wiki/UserGuide#memory but its content is too simple to understand the details. So are there any newly updated docs ? Thanks so much