Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

facts: fix double-counting of CPUs on POWER systems #58360

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
4 participants
@yselkowitz
Copy link

commented Jun 25, 2019

On POWER systems, /proc/cpuinfo provides a 'processor' entry as a
counter, and a 'cpu' entry with a description (similar to 'model name'
on x86). Support for POWER in get_cpu_facts was added via the 'cpu'
entry in commit 8746e69. Subsequent
support for ARM64 in commit ce4ada9
used the 'processor' entry, resulting in double-counting of cores on
POWER systems.

Signed-off-by: Yaakov Selkowitz yselkowi@redhat.com

SUMMARY
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

facts

ADDITIONAL INFORMATION

On a ppc64le node, ansible_processor_vcpus currently shows ansible_processor_vcpus twice the number listed in /proc/cpuinfo. With this patch, the count matches.

Before:

$ ansible all -v -m setup -a 'filter=ansible_processor_vcpus'
Using /etc/ansible/ansible.cfg as config file
localhost | SUCCESS => {
    "ansible_facts": {
        "ansible_processor_vcpus": 352, 
        "discovered_interpreter_python": "/usr/bin/python"
    }, 
    "changed": false
}

$ grep '^processor[[:space:]]*:' /proc/cpuinfo
processor	: 0
[snip]
processor	: 175

After:

$ ansible all -v -m setup -a 'filter=ansible_processor_vcpus'
Using /etc/ansible/ansible.cfg as config file
localhost | SUCCESS => {
    "ansible_facts": {
        "ansible_processor_vcpus": 176, 
        "discovered_interpreter_python": "/usr/bin/python"
    }, 
    "changed": false
}
@yselkowitz

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

We would like to see this merged to supported stable branches as well.

@mattclay

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

@yselkowitz yselkowitz force-pushed the multi-arch:devel branch from dabf660 to 4711328 Jun 27, 2019

@yselkowitz

This comment has been minimized.

Copy link
Author

commented Jun 27, 2019

The problem was in the test, just pushed a new patch to fix that as well.

@ansibot ansibot removed the ci_verified label Jun 27, 2019

facts: fix double-counting of CPUs on POWER systems
On POWER systems, /proc/cpuinfo provides a 'processor' entry as a
counter, and a 'cpu' entry with a description (similar to 'model name'
on x86). Support for POWER in get_cpu_facts was added via the 'cpu'
entry in commit 8746e69.  Subsequent
support for ARM64 in commit ce4ada9
used the 'processor' entry, resulting in double-counting of cores on
POWER systems.

When unit tests were later written for this code in
commit 5530690, the erroneous values
were just accepted in the test instead of being diagnosed.

Signed-off-by: Yaakov Selkowitz <yselkowi@redhat.com>

@yselkowitz yselkowitz force-pushed the multi-arch:devel branch from 4711328 to 0a922a1 Jun 27, 2019

@yselkowitz

This comment has been minimized.

Copy link
Author

commented Jun 27, 2019

0a922a1 fixes the unit tests, I'm not sure what the failure in T=linux/fedora29/4 was.

@ansibot ansibot added core_review and removed needs_revision labels Jun 27, 2019

@jillr jillr added needs_verified P3 and removed needs_triage labels Jun 27, 2019

@yselkowitz

This comment has been minimized.

Copy link
Author

commented Jul 3, 2019

All checks have passed now; the issue with linux/fedora29/4 must have been unrelated. Anything else needed to get this merged?

@ansibot ansibot added the stale_ci label Jul 11, 2019

@yselkowitz

This comment has been minimized.

Copy link
Author

commented Jul 15, 2019

/ready_for_review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.