chore/deployment: add resources requests and limits for helm and Kustomize

[TessaIO]

Resolves #1616

Note: for the moment all of the resource requests and limits for all components are the same (I don't have full visibility on what is needed especially in prod environments). I'd be happy if you want me to change them.

[netlify]

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	d02414c
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/65fd8757a7181200084f04a0
😎 Deploy Preview	https://deploy-preview-1631--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[TessaIO]

/cc @marquiz @ArangoGutierrez

[marquiz]

Thanks @TessaIO for the PR!

Below some comments about the resource requests/limits. Let's try to base them on some rational basis.

[marquiz]

Thx for the update @TessaIO

And thx for the input @PiotrProkop. Do you have something to say for the requests/limits of other daemons?

[marquiz]

Looks good to me.

PTAL @PiotrProkop

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marquiz, TessaIO

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [marquiz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[cmontemuino]

Setting limits.memory: 4Gi means you're ok with an OOM Kill at that number. Hence, the expectation is for the master pod to requesting more memory.
Setting a lower requests.memory is not helping at all. Let's say you get an OOM Kill, then the scheduler spawns a new pod in a node with enough memory according to the requests, but not enough for the limits. This is perfectly fine for the scheduler, but your pod will die again.
Because of the above and because memory cannot be returned to the pool (after GC for example), this is why the recommendation is to set limits equal to requests for memory. You only help the scheduler this way.

[marquiz]

Thanks @cmontemuino for the feedback. I think one reason to set limits is to really oom kill the pod if it get's crazy (with memory leaks like #1614). It might be better to set it to 8Gi than 4Gi, though. The memory consumption very heavily depends on the number of nodes in the cluster (on the ballpark of ~1.5MB per node or so, it seems). We probablty want the default deployment to work in a wide variety of clusters so we cannot set requests to 4Gi or 8Gi as it's total waste (or cannot be satisfied) in smaller clusters and lowering limits to 128Mi will cause problems in bigger clusters. Thoughts?

[PiotrProkop]

I believe that setting proper resource constraints should be up to administrator of specific cluster. Our example deployment manifests should only provide some guidance. Some platforms operators would prefer to OOM nfd-master quicker not to occupy resources from other components other would prefer to do it later etc. There is no golden rule.

[PiotrProkop]

In my clusters I am setting limits 25% above actual usage. As I prefer to be alerted about OOMs in my control plane quicker than later so It's easier to triage issues wrt to resource starving but I understand that other can disagree with this approach 😄

[cmontemuino]

@marquiz -- I'm not suggesting to left limits unset. What I'm saying is to use the same value for both resources.limits.memory and resources.requests.memory.
Having them with different values won't help because memory is not an "elastic" resource.

[marquiz]

@cmontemuino as I explained I don't think we can do that. We don't want to set requests=limits=4Gi as that will hurt/block small clusters and similarly we don't want to set requests=limits=128Mi (or smth on that range) because we'll get oom'ed in bigger clusters.

I think we should document the resource requests/limits better, though.

[TessaIO]

I think we should document the resource requests/limits better, though.

Any suggestions?

[cmontemuino]

@cmontemuino as I explained I don't think we can do that. We don't want to set requests=limits=4Gi as that will hurt/block small clusters and similarly we don't want to set requests=limits=128Mi (or smth on that range) because we'll get oom'ed in bigger clusters.

I think we should document the resource requests/limits better, though.

@marquiz -- got it 👍

Thus it becomes a documentation issue. It must be super clear what implications are there when having different limits and requests for memory. Otherwise OOM Kill will start happening for smaller clusters; and likely non-availability for clusters with memory-pressure scenarios.

Would you like me to try documenting it in a PR?

[marquiz]

Would you like me to try documenting it in a PR?

@cmontemuino please do, thanks for volunteering, really appreciated 👍

[ArangoGutierrez]

I would like the LGTM to come from @PiotrProkop on this PR

[PiotrProkop]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: a0f72811fbbd7dec7bbd3d91a65445bb6281c517

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 30.52%. Comparing base (0132544) to head (d02414c).
Report is 20 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1631      +/-   ##
==========================================
- Coverage   30.91%   30.52%   -0.40%     
==========================================
  Files         102      102              
  Lines        9546     9646     +100     
==========================================
- Hits         2951     2944       -7     
- Misses       6317     6431     +114     
+ Partials      278      271       -7     

see 10 files with indirect coverage changes