fix(power): correct power attribution by separating used vs idle power #2125
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sthaha
force-pushed
the
fix-power-attribution
branch
2 times, most recently
from
32eedaa to
1c74d29
Compare
vimalk78
reviewed
Jun 5, 2025
sthaha
force-pushed
the
fix-power-attribution
branch
from
1c74d29 to
cf402c6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## reboot #2125 +/- ##
==========================================
+ Coverage 92.46% 92.65% +0.19%
==========================================
Files 33 33
Lines 2587 2779 +192
==========================================
+ Hits 2392 2575 +183
- Misses 154 160 +6
- Partials 41 44 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sthaha
force-pushed
the
fix-power-attribution
branch
2 times, most recently
from
962c3c2 to
f461dfa
Compare
sthaha
changed the title
sthaha
force-pushed
the
fix-power-attribution
branch
4 times, most recently
from
38d74ab to
2b1b99b
Compare
sthaha
force-pushed
the
fix-power-attribution
branch
from
2b1b99b to
c1d1b52
Compare
…ibution Previously, workloads were attributed power from total node consumption, causing over-attribution during low CPU usage periods. This commit implements proportional power attribution using CPU utilization ratios. Changes: - Add NodeUsage type with active/idle power breakdown - Calculate CPU usage ratio from /proc/stat - Attribute only active power to workloads based on CPU time - Export separate active/idle power metrics Fixes power attribution accuracy by ensuring workloads are only attributed power proportional to their actual CPU utilization. Signed-off-by: Sunil Thaha <[email protected]>
Signed-off-by: Sunil Thaha <[email protected]>
sthaha
force-pushed
the
fix-power-attribution
branch
from
c1d1b52 to
913a0be
Compare
vimalk78
approved these changes
Jun 6, 2025
Comment on lines
+105
to
+106
| // active over total, where active = total - idle | ||
| // and total = user + nice + system + idle + iowait + irq + softirq + steal |
There was a problem hiding this comment.
update comment to say active = total - (idle + iowait)
| @@ -14,32 +14,37 @@ import ( | |||
| "k8s.io/utils/clock" | |||
| ) | |||
|
|
|||
| type Node struct { | |||
| CPUTimeDelta float64 | |||
There was a problem hiding this comment.
nit: this field contains sum(proc.CPUTimeDelta) , not related to node
vimalk78
reviewed
Jun 6, 2025
Comment on lines
+222
to
+236
| ch <- prometheus.MustNewConstMetric( | ||
| c.nodeCPUActiveJoulesDesc, | ||
| prometheus.CounterValue, | ||
| energy.ActiveEnergy.Joules(), | ||
| zoneName, path, | ||
| ) | ||
|
|
||
| ch <- prometheus.MustNewConstMetric( | ||
| c.nodeCPUIdleJoulesDesc, | ||
| prometheus.CounterValue, | ||
| energy.IdleEnergy.Joules(), | ||
| zoneName, path, | ||
| ) | ||
|
|
||
| // watts |
There was a problem hiding this comment.
the node active/idle joules goes up and down, it is more like a gauge than a counter
There was a problem hiding this comment.
what are you plotting? rate ?
You are right ... I am not incrementing (prev + current) the counter and only calculating the irate.
|
@vimalk78 I am merging this so that pod can be rebased on this. I will continue to fix the incorrect implementation of ActiveEnergy counter as a follow up commit. |
|
also: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a critical power attribution bug where workloads (processes/containers/VMs)
were being attributed the full node power consumption instead of only the power
corresponding to their actual CPU utilization.
Key Changes:
kepler_node_cpu_[active|idle]_[watts|joules_total]kepler_node_cpu_usage_ratiometricImpact: Provides significantly more accurate power attribution that correctly reflects the relationship between CPU utilization and energy consumption, eliminating over-attribution to workloads.