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

redfish: support boolean BIOS attributes #68254

Closed

Conversation

jyundt
Copy link
Contributor

@jyundt jyundt commented Mar 16, 2020

Currently the redfish_config module will convert boolean bios_attribute_value
settings to strings (type str). This will cause BMCs expecting booleans to
error out.

This PR will change the default type of bios_attribute_value to 'raw' in order
to support strings and booleans. Additionally, it makes a minor tweak to
set_bios_attributes() in redfish_utils.py to correctly serialize the python dict
to JSON.

Fixes #68251

SUMMARY

This fixes the behavior in Ansible 2.9 (also present in Ansible 2.8) that converts boolean based BIOS values to strings.

Note that I based this PR on stable-2.9 and not devel. I think the changes introduced in #62764 will require slightly different change.

Please let me know if I should rebase this PR on devel and create different PRs for backport requests for 2.8 and 2.9.

Fixes #68251

Backport of #68382

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

redfish_config
redfish_utils.py

ADDITIONAL INFORMATION

Using the same ansible snippet from 68251, the playbook now works as expected:

(68251) [jyundt@aurora ansible]$ cat tests/supermicro_redfish.yml 
---
- hosts: all
  gather_facts: false
  vars:
    bmc_username: admin
    bmc_password: hunter2
    bmc_address: 192.168.100.100
  tasks:
  - name: Configure boolean BIOS Settings
    redfish_config:
      category: Systems
      command: SetBiosAttributes
      bios_attribute_name: "ConsoleRedirection"
      bios_attribute_value: True
      baseuri: "{{ bmc_address }}"
      username: "{{ bmc_username }}"
      password: "{{ bmc_password }}"

  - name: Configure string BIOS Settings
    redfish_config:
      category: Systems
      command: SetBiosAttributes
      bios_attribute_name: "IPv4HTTPSupport"
      bios_attribute_value: "Disabled"
      baseuri: "{{ bmc_address }}"
      username: "{{ bmc_username }}"
      password: "{{ bmc_password }}"
(68251) [jyundt@aurora ansible]$ time ansible-playbook -i 10.39.67.209, tests/supermicro_redfish.yml

PLAY [all] *********************************************************************************************************************************

TASK [Configure boolean BIOS Settings] *****************************************************************************************************
[DEPRECATION WARNING]: Distribution Ubuntu 18.04 on host 10.39.67.209 should use /usr/bin/python3, but is using /usr/bin/python for 
backward compatibility with prior Ansible releases. A future Ansible release will default to using the discovered platform python for this 
host. See https://docs.ansible.com/ansible/2.9/reference_appendices/interpreter_discovery.html for more information. This feature will be 
removed in version 2.12. Deprecation warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.
changed: [10.39.67.209]

TASK [Configure string BIOS Settings] ******************************************************************************************************
ok: [10.39.67.209]

PLAY RECAP *********************************************************************************************************************************
10.39.67.209               : ok=2    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   


real	0m7.701s
user	0m2.157s
sys	0m0.270s
(68251) [jyundt@aurora ansible]$ 

cc @nlopez

(edit: fix formatting for ansible output.)

Currently the redfish_config module will convert boolean bios_attribute_value
settings to strings (type str). This will cause BMCs expecting booleans to
error out.

This PR will change the default type of bios_attribute_value to 'raw' in order
to support strings and booleans. Additionally, it makes a minor tweak to
set_bios_attributes() in redfish_utils.py to correctly serialize the python dict
to JSON.

Fixes ansible#68251
@ansibot
Copy link
Contributor

ansibot commented Mar 16, 2020

@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 backport This PR does not target the devel branch. bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. python3 remote_management Working Group: https://docs.ansible.com/ansible/latest/community/communication.html support:community This issue/PR relates to code supported by the Ansible community. labels Mar 16, 2020
@billdodd
Copy link
Contributor

Thanks for opening the issue and providing the PR! LGTM

shipit

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Mar 19, 2020
@mraineri
Copy link

shipit

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Mar 19, 2020
@jyundt
Copy link
Contributor Author

jyundt commented Mar 20, 2020

@billdodd / @mraineri A few logistical questions: I created this PR for stable-2.9 and was planning on opening a separate backport request for stable-2.8. Should I have instead opened this against devel and modified the stable-2.9 and stable-2.8 backport requests accordingly in order to accommodate the changes in devel / #62764?

Also, I forgot to include a changelog fragment, just added with 9f00f2f.

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed shipit This PR is ready to be merged by Core labels Mar 20, 2020
@billdodd
Copy link
Contributor

Good question. I just took a peek at the docs. It does look like the flow should start with devel and backport to previous releases:

All Ansible PRs must be merged to the devel branch first. After a pull request has been accepted and merged to the devel branch, the following instructions will help you create a pull request to backport the change to a previous stable branch.

See:

https://docs.ansible.com/ansible/devel/community/development_process.html#backporting-merged-prs

But note that devel is frozen right now while they do some migration activity. And once it is unfrozen, the PR will need to be made in the new Ansible Collections repo. More info on that:

https://github.com/ansible-collections/overview/blob/master/README.rst

@ansibot
Copy link
Contributor

ansibot commented Mar 20, 2020

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

changelogs/fragments/68251-redfish_config-add-boolean-bios-attr-support.yaml:0:0: section "bugfixes" list items must be type str not dict

click here for bot help

@ansibot ansibot added 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. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Mar 20, 2020
@ansibot ansibot added community_review In order to be merged, this PR must follow the community 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 Mar 20, 2020
@billdodd
Copy link
Contributor

BTW - I think the change needed for devel is even simpler than the one for 2.9. It should just be to change the type for bios_attribute_value to raw. Should be no changes needed in redfish_utils.py and the new bios_attributes option already supports attribute values that are, for example, boolean or int.

@jyundt
Copy link
Contributor Author

jyundt commented Mar 20, 2020

Hmm, I didn't know about this "freeze". Lemme at least get a new PR rolling for devel and I can work with that for 2.10 or Collections or whatever.

@jyundt
Copy link
Contributor Author

jyundt commented Mar 20, 2020

New PR for devel: #68382

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 28, 2020
@mattclay
Copy link
Member

@jyundt Since this module has been migrated to the community.general collection, please open a PR for it there: https://github.com/ansible-collections/community.general/commits/master/plugins/modules/remote_management/redfish/redfish_config.py

Once that PR has been merged open a backport PR and reference the merged PR in community.general.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.9 This issue/PR affects Ansible v2.9 backport This PR does not target the devel branch. bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. python3 remote_management Working Group: https://docs.ansible.com/ansible/latest/community/communication.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants