ohai icon indicating copy to clipboard operation
ohai copied to clipboard
verified publisher icon chef

Use a minimum of 1 thread per core for CPU total calculation

Open stanhu opened this issue 3 years ago • 6 comments

Description

In some LXD containers running with VT-x hardware virtualization, it appears that threads per core is 0, resulting in a total number of CPUs of 0. Add a test case and handle some quirks with the lscpu output.

Related Issue

Closes https://github.com/chef/ohai/issues/1755

Types of changes

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)
  • [ ] Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • [x] I have read the CONTRIBUTING document.
  • [x] I have run the pre-merge tests locally and they pass.
  • [ ] I have updated the documentation accordingly.
  • [x] I have added tests to cover my changes.
  • [x] All new and existing tests passed.
  • [x] All commits have been signed-off for the Developer Certificate of Origin.

stanhu avatar Jul 05 '22 17:07 stanhu

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
21.4% 21.4% Duplication

sonarqubecloud[bot] avatar Jul 05 '22 17:07 sonarqubecloud[bot]

LGTM

ramereth avatar Jul 07 '22 23:07 ramereth

Please update the description to show why the default cpu is configurable.

johnmccrae avatar Jul 19 '22 19:07 johnmccrae

Please update the description to show why the default cpu is configurable.

@johnmccrae Could you elaborate? I don't quite understand.

stanhu avatar Jul 19 '22 20:07 stanhu

https://github.com/chef/ohai/pull/1761 might be an alternative solution: if we get 0, give up and use /proc/cpuinfo.

stanhu avatar Aug 02 '22 22:08 stanhu

Please update the description to show why the default cpu is configurable.

@johnmccrae Could you elaborate? I don't quite understand.

@stanhu - Your change to do max[val, 1] is obviously correct. But you changed all the tests to stop checking for CPU 0 and start looking for some configurable "default_cpu" - but your PR description doesn't mention this change at all or why you are making it.

jaymzh avatar Aug 09 '22 19:08 jaymzh

Ping @stanhu ?

jaymzh avatar Aug 23 '22 19:08 jaymzh

@jaymzh Oh, right, thanks. It's been a while since I worked on this so I forgot that I did that. Updated. 😄

stanhu avatar Aug 24 '22 16:08 stanhu

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
21.4% 21.4% Duplication

sonarqubecloud[bot] avatar Aug 30 '22 20:08 sonarqubecloud[bot]

Thanks. Looks good, minus the lint failures. Can you run the rubocop autocorrecting rake task? @tpowell-progress is trying to find the exact command.

I manually corrected them. Running git diff --name-only main | xargs bundle exec rubocop -a corrected almost the entire file. 😄

stanhu avatar Aug 30 '22 20:08 stanhu

Thanks!

jaymzh avatar Sep 06 '22 19:09 jaymzh