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

Fix AIX processor facts and add unit test #78223

Merged
merged 1 commit into from
Jul 13, 2022

Conversation

zorun
Copy link
Contributor

@zorun zorun commented Jul 7, 2022

SUMMARY
  • processor_count was erroneously set to the number of cores
  • processor_cores was erroneously set to the number of threads per core
  • processor_vcpus and processor_threads_per_core were not set
  • processor was a string, while it's supposed to be a list
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

facts/hardware/aix

ADDITIONAL INFORMATION

Before:

"ansible_processor": "PowerPC_POWER7",
"ansible_processor_cores": 4,
"ansible_processor_count": 12,

After:

"ansible_processor": [
    "PowerPC_POWER7"
],
"ansible_processor_cores": 12,
"ansible_processor_count": 1,
"ansible_processor_threads_per_core": 4,
"ansible_processor_vcpus": 48,

@ansibot ansibot added affects_2.14 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jul 7, 2022
@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jul 8, 2022
@ansibot
Copy link
Contributor

ansibot commented Jul 8, 2022

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

test/units/module_utils/facts/hardware/test_aix_processor.py:25:1: W391: blank line at end of file

The test ansible-test sanity --test pylint [explain] failed with 1 error:

test/units/module_utils/facts/hardware/test_aix_processor.py:25:0: trailing-newlines: Trailing newlines

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jul 8, 2022
@zorun zorun changed the title Fix AIX processor facts Fix AIX processor facts and add unit test Jul 8, 2022
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 8, 2022
@zorun
Copy link
Contributor Author

zorun commented Jul 8, 2022

I did some archeology, and it turns out that this code has been here forever, with only minor changes since 2013: 6a89177 introduced in Ansible 1.1.

Maybe processors had just one core at the time, which may explain why they confused the total number of threads and the number of threads per core.

@Akasurde
Copy link
Member

@zorun It is very difficult to get an AIX system for testing, maybe that is the reason this part of the code is untouched.

@zorun
Copy link
Contributor Author

zorun commented Jul 11, 2022

@Akasurde this is why I added unit tests from the two AIX machines I have access on https://cfarm.tetaneutral.net/machines/list/

@mkrizek mkrizek removed the needs_triage Needs a first human triage before being processed. label Jul 12, 2022
@mkrizek
Copy link
Contributor

mkrizek commented Jul 12, 2022

Please create a changelog fragment.

@zorun
Copy link
Contributor Author

zorun commented Jul 12, 2022

Done!

- `processor_count` was erroneously set to the number of cores
- `processor_cores` was erroneously set to the number of threads per core
- `processor_vcpus` and `processor_threads_per_core` were not set
- `processor` was a string, while it's supposed to be a list

Before:

```
"ansible_processor": "PowerPC_POWER7",
"ansible_processor_cores": 4,
"ansible_processor_count": 12,
```

After:

```
"ansible_processor": [
    "PowerPC_POWER7"
],
"ansible_processor_cores": 12,
"ansible_processor_count": 1,
"ansible_processor_threads_per_core": 4,
"ansible_processor_vcpus": 48,
```

Also add a unit test.
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 13, 2022
@mkrizek mkrizek merged commit a6e671d into ansible:devel Jul 13, 2022
@ansible ansible locked and limited conversation to collaborators Jul 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.14 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants