Please consider this patch to update processor count and create a couple new facts.#437
Please consider this patch to update processor count and create a couple new facts.#437riffraff169 wants to merge 4 commits intopuppetlabs:masterfrom
Conversation
(#19249) Split ProcessorCount to separate TotalProcessorCount and ActiveProcessorCount facts
|
Thank you for this pull request! This is a really unusual pull request, because it brings up some concerns that I haven't really seen before. Do you foresee any cases where both of these facts would need to be used? Can we just treat active CPUs as the meaningful value and ignore the total count? |
|
The only time I would see would be for reporting, if you want a report of all systems with lest active processors than present. I know the foreman statistics page shows inaccurate cpu counts because of the incorrect cpu determination, so I'm hoping to get a patch to foreman after this to get the correct number, but I don't know of anything else. I would only say that since the code is so easy, we may as well add it rather than not and find it is useful later on. If it was really long and complex for little return, then I would say ignore it. Thanks
|
lib/facter/processor.rb
Outdated
There was a problem hiding this comment.
All facts are downcased for later use, so using uppercase when declaring the fact itself is a little obfuscated.
There was a problem hiding this comment.
Ok, no problem, I was just using the same format as other facts.
|
I've gone through and added a bunch of inline comments about this implementation. In addition this should have unit tests to help prevent regressions; would you be willing/comfortable to do that? |
|
I will re-pull the repo and work on this stuff. I'll look at unit tests, although I will admit I've never done ruby/facter/puppet unit tests before, so it will take me a little bit to get up to speed.
|
|
@riffraff169 If you would like help with rspec tests, IRC would be one of the best ways to get ahold of me. I'm generally available on irc.freenode.net from 9:00 AM - 5:00 PM in #puppet and #puppet-dev, and my nickname is finch. |
|
I noticed this hasn't seen any activity in about a week, do you think you'll be able to work on it in the near future? No pressure, just checking in. |
|
Yes, I'm going to try to work on it with Adrien this coming week; work and family stuff was taking my time away.
|
|
The other related ticket is kind of interesting, I might check on that too, although in our case every socket is populated (as far as I know, I will have to double-check that). We are spanning activated cpus across sockets so we can access all physical memory.
|
|
No rush on adding those tests, just wanted to check in and make sure this pull request was still active. And you definitely don't need to worry about the other ticket if it's not directly related to your issue, I just wanted to associate those two tickets in case they were stemming from the same problem. |
change processorcount to be same as totalprocessorcount, probably will remove totalprocessorcount
lib/facter/processor.rb
Outdated
There was a problem hiding this comment.
We can delete this copy of :processorcount, since this is more or less defined twice.
|
For my own sake, I'm noting that we have to check sysfs for :processorcount of /proc/cpuinfo indicates we have 0 CPUs because of https://projects.puppetlabs.com/issues/2945. |
|
summary: merged into master in ecc5f90. Since the existing processor specs were pretty messy I reworked them to make things a bit clearer. Thanks again for this contribution! |
3 participants
(#19249) Split ProcessorCount to separate TotalProcessorCount and ActiveProcessorCount facts