Skip to content

Conversation

RincewindsHat
Copy link
Member

On uncommon system configurations (number of sockets > 1) the TotalLoad perfdata of Invoke-IcingaCheckCpu was to high due to it being just summed up over all sockets.

This patch changes this computation, the load weighted by the number of sockets is summed up and then divided by the total number of CPUs.

@cla-bot
Copy link

cla-bot bot commented Oct 11, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Lorenz Kästle.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@LordHepipud
Copy link
Collaborator

LordHepipud commented Nov 3, 2023

Thank you for the PR. Wouldn't it be enough to simple divide the total load by the number of sockets?

$TotalCpuLoad /= $SocketList.Count;

At least in my tests I see no reason against that.

@LordHepipud LordHepipud added the Bug There is an issue present label Nov 3, 2023
@LordHepipud LordHepipud added this to the v1.11.1 milestone Nov 3, 2023
@RincewindsHat
Copy link
Member Author

you could do that, but if I understand this correctly you would loose the weighting by number of threads/socket.
If you get two sockets, one with one Thread and one with four, there would be no balancing between them and the single one would weigh as much as the four others.

@LordHepipud
Copy link
Collaborator

I honestly don't see a problem with that. I agree, that calculating based on threads is more precise for the total load over all sockets. How ever, I doubt you will ever come across a system where the amount of threads a CPU provides is not identical to each other. You wouldn't build a server with two sockets, containing different CPU's and even on virtual machines you can only specify the number of sockets and amount of threads assigned in total.

@LordHepipud LordHepipud modified the milestones: v1.11.1, v1.12.0 Nov 7, 2023
@RincewindsHat
Copy link
Member Author

I would kindly disagree. This patch is the result of encountering multiple systems where, without this patch, Invoke-IcingaCheckCPU caused alarms due to excessively high (and wrong) numbers.

@lazyfrosch
Copy link

The main problem, is that the total over multiple sockets is summed and not averaged currently :)

@AlexMilotin
Copy link

What if we would do the divide in the Invoke-IcingaCheckCPU. Not fancy enough, but it would work.
You have the socket numbers collected from the metrics anyway, you just need to divide it before output the result.
image
And if where count > 1 to divide would be also fine, yet something divided by 1 is itself, so it would hurt to to exclude the if
image

@AlexMilotin
Copy link

Also not sure, going on assuming that if hyper-Threading is enabled dividing by thread count you might be end up and dividing against double the number of needed threads (logical processor)
image

@lazyfrosch
Copy link

@LordHepipud ping

@lazyfrosch
Copy link

ref/NC/809410

@LordHepipud LordHepipud force-pushed the cpu_total_multiple_sockets branch 2 times, most recently from 7755e40 to 9697134 Compare February 26, 2024 11:42
@LordHepipud LordHepipud self-assigned this Feb 26, 2024
@LordHepipud LordHepipud force-pushed the cpu_total_multiple_sockets branch from 9697134 to 97134d7 Compare March 13, 2024 09:59
@LordHepipud LordHepipud merged commit 323acca into master Mar 13, 2024
@LordHepipud LordHepipud deleted the cpu_total_multiple_sockets branch March 13, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug There is an issue present cla/signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants