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

Add GetSupportedFirmwareUpdateMethods to redfish_facts/Update commands #54268

Open
wants to merge 16 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@xmadsen
Copy link
Contributor

commented Mar 22, 2019

SUMMARY

This pull request implements GetSupportedFirmwareUpdateMethods, which returns as a list the supported firmware update methods as shown in the UpdateService schema.

Fixes #54234

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

redfish_facts
redfish_utils

ADDITIONAL INFORMATION
- name: Test Redfish modules
  hosts: localhost
  gather_facts: false
  vars:
    baseuri: 10.243.5.31
    user: USERID
    password: PASSW0RD

  tasks:

    - name: Get all information available in Update category
      redfish_facts:
        category: Update
        command: GetSupportedFirmwareUpdateMethods
        baseuri: "{{ baseuri }}"
        username: "{{ user }}"
        password: "{{ password }}"
 ansible-playbook 2.8.0.dev0
  config file = None
  configured module search path = [u'/Users/xandermadsen/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/xandermadsen/ansible/lib/ansible
  executable location = /Users/xandermadsen/ansible/bin/ansible-playbook
  python version = 2.7.15 (default, Feb 12 2019, 11:00:12) [GCC 4.2.1 Compatible Apple LLVM 10.0.0 (clang-1000.11.45.5)]
No config file found; using defaults
host_list declined parsing /etc/ansible/hosts as it did not pass it's verify_file() method
Skipping due to inventory source not existing or not being readable by the current user
script declined parsing /etc/ansible/hosts as it did not pass it's verify_file() method
auto declined parsing /etc/ansible/hosts as it did not pass it's verify_file() method
Skipping due to inventory source not existing or not being readable by the current user
yaml declined parsing /etc/ansible/hosts as it did not pass it's verify_file() method
Skipping due to inventory source not existing or not being readable by the current user
ini declined parsing /etc/ansible/hosts as it did not pass it's verify_file() method
Skipping due to inventory source not existing or not being readable by the current user
toml declined parsing /etc/ansible/hosts as it did not pass it's verify_file() method
 [WARNING]: No inventory was parsed, only implicit localhost is available

 [WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match 'all'


PLAYBOOK: test_redfish_facts.yml **************************************************************************************************************************************************************************
1 plays in ../test_redfish_facts.yml

PLAY [Test Redfish modules] *******************************************************************************************************************************************************************************
META: ran handlers

TASK [Get all information available in Update category] ***************************************************************************************************************************************************
task path: /Users/xandermadsen/test_redfish_facts.yml:11
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: xandermadsen
<127.0.0.1> EXEC /bin/sh -c 'echo ~xandermadsen && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /Users/xandermadsen/.ansible/tmp/ansible-tmp-1553292386.15-246599246160697 `" && echo ansible-tmp-1553292386.15-246599246160697="` echo /Users/xandermadsen/.ansible/tmp/ansible-tmp-1553292386.15-246599246160697 `" ) && sleep 0'
Using module file /Users/xandermadsen/ansible/lib/ansible/modules/remote_management/redfish/redfish_facts.py
<127.0.0.1> PUT /Users/xandermadsen/.ansible/tmp/ansible-local-419185AEjdJ/tmpDvBITr TO /Users/xandermadsen/.ansible/tmp/ansible-tmp-1553292386.15-246599246160697/AnsiballZ_redfish_facts.py
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /Users/xandermadsen/.ansible/tmp/ansible-tmp-1553292386.15-246599246160697/ /Users/xandermadsen/.ansible/tmp/ansible-tmp-1553292386.15-246599246160697/AnsiballZ_redfish_facts.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '/usr/local/opt/python@2/bin/python2.7 /Users/xandermadsen/.ansible/tmp/ansible-tmp-1553292386.15-246599246160697/AnsiballZ_redfish_facts.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c 'rm -f -r /Users/xandermadsen/.ansible/tmp/ansible-tmp-1553292386.15-246599246160697/ > /dev/null 2>&1 && sleep 0'
ok: [localhost] => (item=GetFirmwareInventory) => {
    "ansible_facts": {
        "redfish_facts": {
            "firmware_update_methods": {
                "entries": [
                    "TFTP", 
                    "SFTP"
                ], 
                "ret": true
            }
        }
    }, 
    "changed": false, 
    "failed_when_result": false, 
    "invocation": {
        "module_args": {
            "baseuri": "10.243.5.31", 
            "category": [
                "Update"
            ], 
            "command": [
                "GetSupportedFirmwareUpdateMethods"
            ], 
            "password": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER", 
            "username": "USERID"
        }
    }, 
    "item": "GetFirmwareInventory"
}
META: ran handlers
META: ran handlers

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

@xmadsen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

@ansibot

This comment has been minimized.

@samerhaj

This comment has been minimized.

Copy link

commented Mar 27, 2019

Some feedback:

  • We should return whether the service supports updating via SimpleUpdate Action, and/or HttpPushUri, or other methods in the future

  • The returned protocols (TFTP/SFTP/etc...) are for SimpleUpdate

  • Maybe rename to GetFirmwareUpdateCapabilities ?

@billdodd
Copy link
Contributor

left a comment

Agree with @samerhaj's comments, including the command name suggestion of GetFirmwareUpdateCapabilities.

Also, see one other comment inline about the UpdateService URI.

result['entries'] = []
data = response['data']
if "UpdateService" in data:
response = self.get_request(self.root_uri + "/redfish/v1/UpdateService")

This comment has been minimized.

Copy link
@billdodd

billdodd Mar 28, 2019

Contributor

Rather than hardcode "/redfish/v1/UpdateService", better to read the value of the "@odata.id" property from the "UpdateService" property.

@ansibot ansibot removed the needs_triage label Mar 28, 2019

@ansibot ansibot added the stale_ci label Apr 5, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/module_utils/redfish_utils.py:637:0: trailing-whitespace Trailing whitespace
lib/ansible/module_utils/redfish_utils.py:646:41: trailing-whitespace Trailing whitespace

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/module_utils/redfish_utils.py:637:1: W293 blank line contains whitespace
lib/ansible/module_utils/redfish_utils.py:646:42: W291 trailing whitespace

click here for bot help

Xander Madsen
@billdodd
Copy link
Contributor

left a comment

Thanks for the update @xmadsen. A couple of suggestions:

  1. In general, when adding a new command, good to add an example in the EXAMPLES section of the module.

  2. See inline comment below for suggestion to avoid KeyError exceptions.

actions = data['Actions']
if len(actions) > 0:
for action in actions.values():
result['entries'][action['title']] = action['TransferProtocol@Redfish.AllowableValues']

This comment has been minimized.

Copy link
@billdodd

billdodd Apr 8, 2019

Contributor

Neither the 'title' nor the 'TransferProtocol@Redfish.AllowableValues' properties are required, so these are likely to cause KeyError exceptions. Suggest changing the for loop to something like this:

        for key in actions.keys():
            action = actions.get(key)
            if 'title' in action:
                title = action['title']
            else:
                title = key
            result['entries'][title] = action.get(
                'TransferProtocol@Redfish.AllowableValues',
                ["key TransferProtocol@Redfish.AllowableValues not found"]
            )

Xander Madsen and others added some commits Apr 10, 2019

Xander Madsen
@billdodd
Copy link
Contributor

left a comment

Thanks! Looks good.

shipit

@jose-delarosa
Copy link
Contributor

left a comment

See comments.

On Dell servers the key TransferProtocol@Redfish.AllowableValues is not found, but it clearly detects that is not found.

Show resolved Hide resolved lib/ansible/module_utils/redfish_utils.py Outdated
Xander Madsen
@xmadsen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

@billdodd @jose-delarosa this should be good to check

@billdodd
Copy link
Contributor

left a comment

Looks good.

shipit

@billdodd

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@xmadsen - may need to add ready_for_review comment to clear the needs_revision label.

@xmadsen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

ready_for_review

@jose-delarosa

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

I am not able to test this patch in my dev environment. I've tried 2 ways:

  1. As described here.
  2. Download patch and apply with either git am or git apply.

Open to suggestions.

@billdodd

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@jose-delarosa I just downloaded the 2 changed files into my devel branch sandbox and was able to run successfully.

@jose-delarosa

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@billdodd I tried again, no luck. Not sure what is going on, I've been able to test every PR so far. I rebase my dev branch with upstream and then apply the patch in a separate branch, no luck. Will try again.

If @mraineri or @tomasg2012 can test and approve, I'm OK with that.

@billdodd

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

@billdodd I tried again, no luck. Not sure what is going on, I've been able to test every PR so far. I rebase my dev branch with upstream and then apply the patch in a separate branch, no luck. Will try again.

What kind of error(s) are you getting? I assume it must be an issue related to a recent merge. But there hasn't been very many, so it seems like it shouldn't be too hard to work around. If I saw the error, I might be able to give some input.

If @mraineri or @tomasg2012 can test and approve, I'm OK with that.

Absolutely. That would be good too.

@jose-delarosa

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@xmadsen @billdodd When I apply this patch to my devel branch and try to test it, I get HTTP Error 406, and sure enough my patch #55193 for adding those extra HEADERS is not there. Looks to me like you originally submitted this PR before #55193, but not sure why this would cause a problem since these are different blocks of code. Could be a GitHub bug.

In any regard, I am not able to test as-is. Unless you want to rebase your repo to inculde the HEADERS PR?

@billdodd

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@jose-delarosa Could you test on a 14G system with the previous firmware or a 13G system?

@jose-delarosa

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

Well best I can do is 13G, and at least the key-not-found message is captured correctly:

{
    "ansible_facts": {
        "redfish_facts": {
            "firmware_update_capabilities": {
                "entries": {
                    "#UpdateService.SimpleUpdate": [
                        "Key TransferProtocol@Redfish.AllowableValues not found"
                    ]
                },
                "ret": true
            }
        }
    },
    "changed": false,
    "failed": false
}

I'm OK with it.

shipit

@ansibot ansibot added shipit and removed community_review labels Apr 23, 2019

@jose-delarosa

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@billdodd @xmadsen There are about 4 unmerged PRs with 2 shipt its, wonder if we need to reach out to repo maintainers?

@billdodd

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@billdodd @xmadsen There are about 4 unmerged PRs with 2 shipt its, wonder if we need to reach out to repo maintainers?

I'll ping the ansible-devel channel

@xmadsen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

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.