Correct calculation of CPU usage. #20
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/assign @BCLAU |
|
@PatrickLang: GitHub didn't allow me to assign the following users: bclau. Note that only kubernetes-sigs members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Indeed, consume_cpu_windows has some accuracy issues because of 3 reasons:
- User and System times improperly collected.
- Number of CPUs was not taken into account.
- print consumes a bit too much time.
Also, there's another thing that comes to mind. Will the -millicores ever be more than 1000? Because if so, then this binary won't be able to satisfy this request for now. A process will consume at most 1 core.
| dT := second.Time.Sub(first.Time).Seconds() | ||
| return 100 * (second.Total - first.Total) / dT | ||
| dT := second.Time.Sub(first.Time).Nanoseconds() | ||
| dUsagePerCPU := (second.Total - first.Total) / float64(numCPUs) |
There was a problem hiding this comment.
Compilation error:
.\consume_cpu_windows.go:61:47: invalid operation: (second.Total - first.Total) / float64(numCPUs) (mismatched types int64 and float64)
You could put here:
dUsagePerCPU := float64(second.Total - first.Total) / float64(numCPUs)
| @@ -72,15 +79,15 @@ func main() { | |||
| handle := syscall.Handle(phandle) | |||
| flag.Parse() | |||
| // convert millicores to percentage | |||
| millicoresPct := float64(*millicores) / float64(10) | |||
| targetPct := float64(*millicores) / float64(10) | |||
| duration := time.Duration(*durationSec) * time.Second | |||
| start := time.Now() | |||
| first := statsNow(handle) | |||
|
|
|||
| for time.Now().Sub(start) < duration { | |||
| cpu := usageNow(first, statsNow(handle)) | |||
| fmt.Println("cpu: ", cpu) | |||
There was a problem hiding this comment.
This is actually a time consuming action. Because time is spent executing this, the CPU utilization does not reach the desired values:
PS C:\workspace\k8s_images\images\resource-consumer\consume-cpu> .\consume_cpu_windows.exe -duration-sec 20 -millicores 1000
cpu: NaN
cpu: 0
cpu: 17.362230010378447
cpu: 17.759334590410074
cpu: 19.17306518719456
cpu: 17.530168719158922
cpu: 17.937045275972206
cpu: 19.612749144674936
cpu: 20.253901989447044
cpu: 19.53195966120102
cpu: 19.054819954817212
cpu: 20.09014672057374
cpu: 19.792669495254426
cpu: 20.508622590592328
cpu: 20.316400379483763
cpu: 21.211256649870368
cpu: 21.159277352213714
cpu: 21.23777335320121
cpu: 21.294004397855133
cpu: 21.2304964770284
cpu: 21.14616192195179
NOTE: my env has 4 cores, so the target value should have been 25. Removing this print and watching the CPU utilization in task manager, I can see that it peaks at 25%.
We should probably remove this print.
k8s-ci-robot
added
size/M
This executable attempts to maintain around a target level of CPU usage. For that to work, it has to calculate its current usage correctly. The calculation is based on a syscall that returns the cpu time this process has used so far. However, the loading of that return value into a struct was incorrect; by summing each cpu usage value with itself, the estimate of current cpu usage was doubled. This results in the container using less than the requested number of millicores. This commit also makes some variable names more readable and add a couple comments explaining the purpose of the helper functions. Doing all calculations in millicores rather than percentages makes the code more aligned with the input it takes. Signed-off-by: Ben Moss <[email protected]>
|
@BCLAU made some additional changes to this, removing the prints and it should be compiling now |
|
So, it seems that the number of CPUs are no longer taken into account. But it seems to be correct, running The PR message could be updated to reflect these new changes. Other than that, LGTM. /lgtm |
|
@BCLAU: changing LGTM is restricted to assignees, and only kubernetes-sigs/windows-testing repo collaborators may be assigned issues. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@PatrickLang It looks like you're on this OWNERS list for this repo. Could you approve this PR? |
|
/approve |
k8s-ci-robot
added
the
approved
|
/lgtm |
|
@benmoss: changing LGTM is restricted to assignees, and only kubernetes-sigs/windows-testing repo collaborators may be assigned issues. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@PatrickLang looks like it needs a lgtm too |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adelina-t, astrieanna, PatrickLang 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:
Approvers can indicate their approval by writing |
This executable attempts to maintain around a target level of CPU usage.
For that to work, it has to calculate its current usage correctly. The
calculation is based on a syscall that returns the cpu time this process
has used so far. However, the loading of that return value into a struct
was incorrect; by summing each cpu usage value with itself, the estimate
of current cpu usage was doubled. This results in the container using
less than the requested number of millicores.
This commit also makes some variable names more readable and add a
couple comments explaining the purpose of the helper functions. Doing
all calculations in millicores rather than percentages makes the code
more aligned with the input it takes.
Relevant to kubernetes/kubernetes#73489