Cleanup Redfish module based on growth of functionality

[mraineri]

Summary

I would like to review and clean redfish_utils.py to ensure it aligns with best practices for the standard. Some examples of things to cleanup:

• Simplify logic for applying new settings from a user
• Centralize vendor-specific checks/workarounds for standard management functionality

Issue Type

Feature Idea

Component Name

redfish_utils

Additional Information

Some logic I've seen copy/pasted around the module looks like a good candidate to prune/remove altogether. This block of code is a good example of the type of logic we have in the module today for many of the generic "set" routines:

        for property in nic_config.keys():
            value = nic_config[property]
            if property not in target_ethernet_current_setting:
                return {'ret': False, 'msg': "Property %s in nic_config is invalid" % property}
            if isinstance(value, dict):
                if isinstance(target_ethernet_current_setting[property], dict):
                    payload[property] = value
                elif isinstance(target_ethernet_current_setting[property], list):
                    payload[property] = list()
                    payload[property].append(value)
                else:
                    return {'ret': False, 'msg': "Value of property %s in nic_config is invalid" % property}
            else:
                payload[property] = value

This type of code does up-front comparisons between the API response containing the current state of the resource to change and the requested changes by the user. It performs two types of checks: does the API support the requested property and is the property of the correct format. In both cases, the API is expected to return an HTTP 400 Bad Request for either of these cases. So, this type of upfront checks really just saves us a request/response cycle. In my opinion, it's not worth adding this much bloat to the module since it can lead to future bugs if not done correctly.

There was also a recent addition to add a _get_vendor method to redfish_utils. I think this is a good thing to enhance and use in a more common manner in the module. There are some checks in various methods to perform workarounds if a given vendor or other firmware identifier is detected. Some workarounds exist today in virtual media methods for various vendors, and there is an open issue for defining a workaround for one-time boot override for a particular Dell implementation.

Code of Conduct

• I agree to follow the Ansible Code of Conduct

[ansibullbot]

Files identified in the description:

• plugins/module_utils/redfish_utils.py

If these files are incorrect, please update the component name section of the description or use the !component bot command.

click here for bot help

[ansibullbot]

cc @Bhavya06 @rajeevkallur @renxulei @tomasg2012 @xmadsen
click here for bot help

[mraineri]

@felixfontein I'd like your opinion on one thing I'm finding. There are some cases where checks are made for both "is the property supported" as well as "is the property in the desired state already". For example, take a look at the code starting here: https://github.com/ansible-collections/community.general/blob/main/plugins/module_utils/redfish_utils.py#L1171

I wanted to delete the block at line 1171 since a PATCH with unsupported properties will result in a 400 from the service, which also means we can save a GET request and simply send a PATCH right away. However, there's a dependence on performing a GET starting at line 1181 to see if a PATCH is even needed in the first place. Is it important from an Ansible perspective to avoid sending unnecessary configuration requests? If it's not important, then update_accountservice_properties can simply send a PATCH with the desired changes, and everything can be driven from the response from the service in terms of error/success indication.

[felixfontein]

@mraineri from the Ansible perspective, it's important that a change does not break backwards compatibility (for example, if the module simply didn't apply changes that were not supported in the past, and now fails, that would be bad). Another thing that are potentially important is check mode support. If a module was using checks to first determine (conclusively) whether the change is needed, and only applies it when needed, it usually supports check mode. If you change that to "do the change and then ask the API whether something changed" this would be a problem. But the way I understand the code and your proposed change, I think neither of these two apply, so it should be good. It might be a change that affects the observable result for the user in case of errors, but that's fine IMO.

[mraineri]

Thanks for the feedback. I think in this case pulling out these checks should be okay since none of the code performs a follow-up check to see if the properties really changed. At least stepping through the code paths to the different results, the outcomes should be the same, with the exception of 'changed' always returning 'true' in the success path.

Scenario 1: User provides unsupported property

• Today's code: 'ret' contains 'false' and a message saying 'unsupported property.
• Proposed change: 'ret' contains 'false' and a message from the API saying the property requested is unsupported (PATCH to the API performed and resulted in a 400 Bad Request)

Scenario 2: User provides property that is different from the existing configuration

• Today's code: 'ret' contains 'true' and 'changed' contains 'true' (PATCH to the API performed to change the property successfully)
• Proposed change: 'ret' contains 'true' and 'changed' contains 'true' (PATCH to the API performed to change the property successfully)

Scenario 3: User provides property that is already configured to the desired state

• Today's code: 'ret' contains 'true' and 'changed' contains 'false' (no PATCH attempted since the GET showed the property is configured in the desired state)
• Proposed change: 'ret' contains 'true' and 'changed' contains 'true' (PATCH to the API performed to change the property successfully)
  • In this case the burden is shifted to the API to essentially perform a "no operation" since no changes are really made

[mraineri]

@felixfontein one more question: how important is the usage of 'changed' in results? I'm finding inconsistencies where some methods that perform a property modification in an API request don't return 'changed' in the results, where as others do.

[mraineri]

I think in the interest of maintaining the 'changed' behavior, I'll be revising my course of action. I'll certainly be looking to have more generic "is change needed" code to avoid all of the copying/pasting there is right now.

[felixfontein]

@felixfontein one more question: how important is the usage of 'changed' in results? I'm finding inconsistencies where some methods that perform a property modification in an API request don't return 'changed' in the results, where as others do.

Doing changes without indicating changed=True sounds like a bug to me. Or is there some additional code that checks whether something really changed and only then sets changed=True? I've seen such code before (as some APIs make it impossible to predict whether there will be a change).

I'll certainly be looking to have more generic "is change needed" code to avoid all of the copying/pasting there is right now.

👍

[mraineri]

@felixfontein one more question: how important is the usage of 'changed' in results? I'm finding inconsistencies where some methods that perform a property modification in an API request don't return 'changed' in the results, where as others do.

Doing changes without indicating changed=True sounds like a bug to me. Or is there some additional code that checks whether something really changed and only then sets changed=True? I've seen such code before (as some APIs make it impossible to predict whether there will be a change).

Not that I could find; it just seems like a gap in some of the methods.

[mraineri]

Looks like you were right; I found at the very end of redfish_command.py it will apply "changed=True" by default.