SGX: set EPC limits via NRI annotations

[mythi]

No description provided.

[codecov-commenter]

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (eb0de47) 51.94% compared to head (430a729) 51.68%.

❗ Current head 430a729 differs from pull request most recent head c876e22. Consider uploading reports for the commit c876e22 to get more accurate results

Files	Patch %	Lines
pkg/controllers/sgx/controller.go	0.00%	29 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1582      +/-   ##
==========================================
- Coverage   51.94%   51.68%   -0.26%     
==========================================
  Files          42       42              
  Lines        4911     4941      +30     
==========================================
+ Hits         2551     2554       +3     
- Misses       2210     2237      +27     
  Partials      150      150              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ffuerste]

Maybe the node selector for the NRI plugin could also including the intel.feature.node.kubernetes.io/sgx label that adds the NFD add-on to SGX enabled nodes?

This label is already used for the NFD NodeFeatureRule (see here). This more specific node selector could be an optional kustomize patch (or added to the epc-nfd overlay) and perhaps used for the entire DaemonSet? This would ensure that the components only run on the nodes on which they are required.

[mythi]

I can address this in the next rev. The current draft just pulls the NRI bits + the SGX webhook modifications and does not add support for any other deployment options just yet.

I was also checking if NFD can check "NRI is enabled" but that does not seem to be available.

[ffuerste]

So in case an additional node selector would be added, you would like to add it to all deployment options and not just for the one based on the kustomize files (described here)?

Should I create an official feature request for that?

[mythi]

This more specific node selector could be an optional kustomize patch (or added to the epc-nfd overlay) and perhaps used for the entire DaemonSet?

I could add intel.feature.node.kubernetes.io/sgx to the epc-nfd overlay by default. The "base" daemonSet is kept minimal and I'm likely going to drop the NRI bits to an overlay in the final version to make it user "opt-in".

[mythi]

@ffuerste I made some updates to this. let me know how it looks

[ffuerste]

Sorry for the late reply.

I really like the implementation of the NFD label as a kustomize patch as part of the overlay for the intel-sgx-plugin DaemonSet.

Also, I think the defining the NFD label as a nodeSelector for the new cAdvisor DaemonSet is a pretty good idea since you named the directory for the resource files sgx_epc_metrics. This clearly points to the use case of providing metrics on EPC consumption.

However, maybe we should add a short comment to the label defined as the nodeSelector in the patches? E.g. # intel.feature.node.kubernetes.io/sgx: 'true' # added to SGX enabled nodes by NFD add-on. Maybe this is not necessary for the epc-nfd overlay, but I think it's beneficial for the cAdvisor resource file, because currently the dependency to the NFD add-on is not specified anywhere.

Moreover, maybe the new cAdvisor component should be mentioned in the README of the sgx plugin? Or you don't want to add it there, because it's maybe only needed until the cAdvisor in the kubelet is supporting the misc metrics?

[mythi]

but I think it's beneficial for the cAdvisor resource file, because currently the dependency to the NFD add-on is not specified anywhere.

I can add the comment my next rebase. I'm not sure the label is going to be needed in the final version because cAdvisor could be used for cluster telemetry in general. Unless of course the desire is to only get the EPC+memory metrics... Let's see.

Moreover, maybe the new cAdvisor component should be mentioned in the README of the sgx plugin? Or you don't want to add it there, because it's maybe only needed until the cAdvisor in the kubelet is supporting the misc metrics?

I will add it later once I have better visibility on the PR readiness (which depends on runc 1.2 release I believe). We are most likely going to need the "standalone" deployment for a while.

[eero-t]

Or you don't want to add it there, because it's maybe only needed until the cAdvisor in the kubelet is supporting the misc metrics?

I do not think kubelet will be adding metrics, as there are plans on reducing them:

• https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/2371-cri-pod-container-stats/README.md
• Reduce the set of metrics exposed by the kubelet kubernetes/kubernetes#68522

[mythi]

Right. I had checked with the KEP author on what the best approach for me would be and the suggestion was to start with runc+cAdvisor side of things. Longer run we'd need misc to be part of runtimes' metrics which is the future anyway but for now we'd use a standalone cAdvisor.