Helm chart for pcm-sensor-server by ppalucki · Pull Request #727 · intel/pcm

ppalucki · 2024-04-26T07:59:18Z

Initial version of Helm chart for deploying PCM

Features include:

Two methods of collecting metrics (indirect the default, by using Linux interfaces perf/resctrl) or direct by accessing msr registers directly through linux msr module,
Support for bare-metal (full set of metrics) and VM cloud instances (limited set of metrics),
Optional integration with Prometheus opeartor, node-feature-discovery and NRI balloons policy plugin,
Other described here: https://github.com/ppalucki/pcm/blob/ppalucki/helm/deployment/pcm/README.md

TODO:

Must have:

Testing:

NMI watch is properly reconfigued (disabled/enabled) basd on VM/env flags (now it is set to RO and it works?!)
Testing in Cluster Manager Systems like (e.g. Ranger,Gardener) different node types VM(1socket,all sockets), bare-metal
Test in different cloud GCP/Azure/AWS

Follow up tasks (in another PRs ideas):

pcm-sensor-server: expose missing metrics available in other tools:
- energy,
- PCI-e as from pcm-pcie,
- numa-split
Change metrics names (follow Prometheus best practices) - it will be separate PR
init container to check permission for all required components (devices/CPU)
Configurable collection period (and aggregators history) to limit cpu/memory usage (TODO: do some memory profiling first)

jcpunk · 2024-04-26T14:47:53Z

It would be helpful if you could put custom labels on the podMonitor to let prometheus filter based on that.

rdementi · 2024-04-29T11:11:54Z

the FreeBSD check failure is unrelated

rdementi

great start! Your TODO list makes sense to me. Thank you!

deployment/pcm/README.md

rdementi · 2024-04-29T11:20:27Z

src/cpucounters.cpp

 bool PCM::CoreLocalMemoryBWMetricAvailable() const
 {
-    if (cpu_model == SKX && cpu_stepping < 5) return false; // SKZ4 errata
+    //if (cpu_model == SKX && cpu_stepping < 5) return false; // SKZ4 errata


did you remove these checks for testing purposes?

rdementi · 2024-04-29T11:20:40Z

src/cpucounters.cpp

 bool PCM::CoreRemoteMemoryBWMetricAvailable() const
 {
-    if (cpu_model == SKX && cpu_stepping < 5) return false; // SKZ4 errata
+    //if (cpu_model == SKX && cpu_stepping < 5) return false; // SKZ4 errata


fmuyassarov

I only checked the commit that adds the Helm chart and it looks good to me. The only thing I wondered is do you really expect users to modify all the available values in the values.yaml. To me some looked not very necessary but I can't judge since I'm not familiar with PCM.
LGTM

deployment/pcm/templates/podmonitor.yaml

deployment/pcm/values.yaml

ppalucki · 2024-05-08T17:59:57Z

It would be helpful if you could put custom labels on the podMonitor to let prometheus filter based on that.

@jcpunk Thanks for this comment, that is very helpfull indeed - having that there is no longer need to hack prometheus-operator chart to disable podMonitorSelector - I added comments in README/values file about this (check this changes in commit for details (7f2c707#diff-618d3b78482c88190c469bb01731f774bb931bcdc14db7b8980691f5745ba08aR151-R152) or this documentation added in values

pcm/deployment/pcm/values.yaml

Lines 87 to 91 in 7f2c707

    
           # Extra PodMonitor labels to let Prometheus operator filter based on that 
        
           # e.g. default "kube-prometheus-stack" helm chart requires additional release:"{name of chart release}" label in podMonitor to be considered 
        
           # here is example how to check extra labels required to be added to PodMonitor 
        
           # 1) kubectl get prometheus -o jsonpath='{.items[].spec.podMonitorSelector.matchLabels}' # e.g. release: prometheus 
        
           # 2) helm install pcm . --set podMonitor=true --set podMonitorLabels.release=prometheus

Anyway help for pointing this.

ppalucki · 2024-05-08T18:44:52Z

I only checked the commit that adds the Helm chart and it looks good to me. The only thing I wondered is do you really expect users to modify all the available values in the values.yaml. To me some looked not very necessary but I can't judge since I'm not familiar with PCM. LGTM

That is the trade of between flexibility and complexity that I'm finding hard to balance.

I see two options here:

Limit number of features (supported collection methods direct/indirect, podmonitor, NFD) - worth considering, but is hard to predict which features are valueable for others (even for me modest needs I see cases where I want all of them).
Move logic from values to templates (less "raw" values) - e.g. instead of requiring to explicit enviornment values directly, expose values that represent set of them like "--set directCollection=false or true" will set proper combination of (PCM_NO_MSR, PMC_NO_PERF) - but I alread tried that and it would add a lot of complexity in templates (e.g. PCM_NO_MSR conflicts with PCM_NO_PERF and both are related to PCM_USE_UNCORE_PERF), imagine I could do something like this inside template:

if direct:
  if vm:
    if rdt:
       PCM_NO_MSR=0, PCM_NO_PERF=1, PCM_USE_UNCORE_PERF=0
  else:
       PCM_NO_MSR=1 PCM_NO_PERF=0, PCM_USE_UNCORE_PERF=1
  ....
else:
  if vm:
    PCM_NO_MSR=0, PCM_NO_PERF=1, PCM_USE_UNCORE_PERF=0
  else:
    PCM_NO_MSR=1 PCM_NO_PERF=0, PCM_USE_UNCORE_PERF=1

and try to handle all possible (both proper and inproper combination) - but now rewrite this using go/helm template language - not very maintanable nor readable - I don't really want chart to be validator of possible PCM envs and I'm terified of hardcoding all proper possible combinations - in other words - if you don't like defaults (or other example value files) - then you're responsible for validating that pcm-sensor-server binary will run - but finding right combination is your job in your case.

(there is also another option - e.g. allow to pass *any enviornment/options to pcm - but that is discouraged security practice)

I the end, I decided to just allow use of this pcm chart to pass those "all PCM suppored" environment variables directly - and tried to cover possible value of combinations as different values files.

One more comment, I agree that value file is alread quite big, but not yet as scary as the I see in prometheus node exporter official chart values.yaml - it is 500 lines vs my so far about 100 (but I miss some feature though RBACs, vertical scaling) which I'm using as example of good practicies :)

old comments: sys/pci/mcfg mounts are unnessesary for indirect method fix old wrong defaults in README fix formatting possible fix for issue with resctrl remove hacks to handle /pcm/resctrl and unessesary out-of-date files update License to use the same as pcm itself update README, remove out-of-date info links do values formatting + links do values update README an values comments update README address jcfunk comments: interval and extra labels for PodMonitor + refactor readme fix typos readme: reminder about removing msr kernel module after rebasing: point to correct default pcm image from intel organization Refactoring: - explicit values file for privileged direct method, - hide (into docs directory) "unprivileged" direct method (and fixes), - remove unnessesary mounts (mcfg, /dev/cpu/dev/mem for privileged access), - add instructions to collection methods, - fixes (extra builder) for build local development image, - silent mode - move collection methods to the top fix values files for direct privileged method New: support for PERFMON capability, silent mode and some extra env debug variables VPA: v1 - first version of vertical pod autoscaler Grafana dashboard: instructions rename resctrlHostMount to resctrlMount fix dashboard rate interval pcm-sensor-server: add new metrics DRAM Local percantage Fix dockerbuild by using separate Dockerfile + build in dockerignore improve dockerfile.debug extra env PCM_NO_MAIN_EXCEPTION_HANDLER

rdementi

thanks!

rdementi · 2024-06-18T07:05:30Z

src/pcm-sensor-server.cpp

+
+        if (pcm->localMemoryRequestRatioMetricAvailable())
+            printCounter( "DRAM Local Percentage",         getLocalMemoryRequestRatio( before, after ) );
+


I think the addition of a new metric should be split out into a separate merge request or at least a separate commit.

rdementi · 2024-06-18T07:06:09Z

src/pcm-sensor-server.cpp

+
+        if (pcm->localMemoryRequestRatioMetricAvailable())
+            printCounter( "DRAM Local Percentage",         getLocalMemoryRequestRatio( before, after ) );
+


Damenus · 2024-06-26T11:52:10Z

Hello,
I've attempted to deploy the new Helm charts introduced in this pull request, but I'm encountering an error during the installation process. Here are the details:

I think that I have successfully made all steps before 6) Deploy PCM helm chart in paragraph Validation on local kind cluster
Could you add information on how to deal with it when this error happens?

Damenus · 2024-06-26T12:05:04Z

deployment/pcm/README.md

+
+- kubectl/kind/helm/jq binaries available in PATH,
+- docker service up and running.
+- full set of metrics available only bare-metal instance or Cloud .metal instance.


I think you could add information on how to check it or enabled it.

Damenus · 2024-06-26T12:06:07Z

deployment/pcm/README.md

+helm install ... --set nfd=true --set podMonitor=true --set verticalPodAutoscaler.enabled=true
+```
+
+### Requirements


I think you could add information on how to check it or enable all requirements.

Damenus · 2024-07-17T12:16:09Z

I was trying to run e2e test make e2e-default, but it doesn't work without extra steps. I got error in pod pcm:

Linux Perf: Error when programming INST_RETIRED, error: Permission denied with config 0x1 config1 0x0 for tid -1 leader -1
try running with environment variable PCM_NO_PERF=1

Please, add information about how to add this env

Also, the file ./_kind_with_registry.sh which is downloading during the test execution from Makefile is broken. The Cluster yaml definition is not valid. Removing a broken line helped.

ppalucki mentioned this pull request Apr 26, 2024

[Feature] PCM helm chart intel/helm-charts#33

Open

ppalucki changed the title ~~Helm chart for pcm - initial version~~ Helm chart for pcm-sensor-server Apr 26, 2024

rdementi requested changes Apr 29, 2024

View reviewed changes

fmuyassarov reviewed Apr 30, 2024

View reviewed changes

jcpunk reviewed May 2, 2024

View reviewed changes

deployment/pcm/templates/podmonitor.yaml Show resolved Hide resolved

jcpunk reviewed May 2, 2024

View reviewed changes

deployment/pcm/templates/podmonitor.yaml Outdated Show resolved Hide resolved

rdementi reviewed May 3, 2024

View reviewed changes

deployment/pcm/values.yaml Outdated Show resolved Hide resolved

ppalucki force-pushed the ppalucki/helm branch 3 times, most recently from a30b342 to 8edb9dc Compare May 22, 2024 15:49

ppalucki force-pushed the ppalucki/helm branch from cc88779 to cdb10cd Compare June 5, 2024 14:22

ppalucki added 6 commits June 6, 2024 16:34

First version of linter + tests

d75b013

README update + better Dockerfile.debug

92fbe0c

Chart testing using helm test

513b7c9

improve helm test - fix proper namespace

6f3d9eb

Initial version of e2e for pcm/prometheus and VPA

a088ab0

fix with proper names and add NFD/metal case

cb36269

rdementi requested changes Jun 18, 2024

View reviewed changes

e2e tests: cont

91b445e

Damenus reviewed Jun 26, 2024

View reviewed changes


		if (pcm->localMemoryRequestRatioMetricAvailable())
		printCounter( "DRAM Local Percentage", getLocalMemoryRequestRatio( before, after ) );

Conversation

ppalucki commented Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Features include:

TODO:

Follow up tasks (in another PRs ideas):

Uh oh!

jcpunk commented Apr 26, 2024

Uh oh!

rdementi commented Apr 29, 2024

Uh oh!

rdementi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rdementi Apr 29, 2024

Choose a reason for hiding this comment

Uh oh!

rdementi Apr 29, 2024

Choose a reason for hiding this comment

Uh oh!

fmuyassarov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ppalucki commented May 8, 2024

Uh oh!

ppalucki commented May 8, 2024

Uh oh!

rdementi left a comment

Choose a reason for hiding this comment

Uh oh!

rdementi Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

rdementi Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

Damenus commented Jun 26, 2024

Uh oh!

Damenus Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

Damenus Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

Damenus commented Jul 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

ppalucki commented Apr 26, 2024 •

edited

Loading