Redfish: centralize payload inspection logic and OEM logic

[mraineri]

SUMMARY

There is a great deal of copy/paste code that does this in many places prior to invoking a PATCH request to the service. Added common logic for performing inspection of payloads when performing modification requests, which allowed for this copy/paste code to be removed throughout the module. It also alleviated cases where multiple GET requests to the same URI are being made, so this improves the I/O efficiency.

I'm still running tests to make sure I didn't break anything. The intent is to make this 100% backwards compatible, so no users should see a functional change from a module API perspective.

I also still have an outstanding item to centralize OEM checking for performing workarounds. I wanted eyes on these changes while I had them ready to ensure folks like what I'm doing.

Fixes #5210

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

redfish_utils

ADDITIONAL INFORMATION

[ansibullbot]

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

[mraineri]

Update: Everything has checked out with my unit testing with the current changes. Remaining item is to add common vendor checking for various workaround logic.

[mraineri]

@felixfontein would it be possible to remove the image_only=False found in the virtual_media_insert_via_patch and virtual_media_eject_via_patch methods at this point? I was expecting those to be marked as private methods (and only invoked by the public virtual_media_insert and virtual_media_eject methods), but it looks like they were defined as public methods. It's not a problem if we can't at this point, but it would've been good to clean that up to simplify the interface for callers into the module.

[felixfontein]

@mraineri we should probably deprecate them before removing, something like this:

1. Define some object _SENTRY = object() at the top of the file.
2. Define the default value of image_only to be _SENTRY.
3. Add if image_only is not _SENTRY: self.module.deprecate('The image_only paramete will be removed from XXX in redfish module utils', version='7.0.0', collection_name='community.general'); else: image_only = False at the beginning of the function body.

[felixfontein]

@mraineri btw, do you think you can complete this by the end of this week, so we can make sure it will make it into the 6.0.0 release on next Monday?

[mraineri]

@mraineri we should probably deprecate them before removing, something like this:

1. Define some object `_SENTRY = object()` at the top of the file.

2. Define the default value of `image_only` to be `_SENTRY`.

3. Add `if image_only is not _SENTRY: self.module.deprecate('The image_only paramete will be removed from XXX in redfish module utils', version='7.0.0', collection_name='community.general'); else: image_only = False` at the beginning of the function body.

Thanks; I'll do that!

@mraineri btw, do you think you can complete this by the end of this week, so we can make sure it will make it into the 6.0.0 release on next Monday?

Yup, I should have everything in before then; just testing the OEM check changes now.

[mraineri]

@mraineri we should probably deprecate them before removing, something like this:

1. Define some object `_SENTRY = object()` at the top of the file.

2. Define the default value of `image_only` to be `_SENTRY`.

3. Add `if image_only is not _SENTRY: self.module.deprecate('The image_only paramete will be removed from XXX in redfish module utils', version='7.0.0', collection_name='community.general'); else: image_only = False` at the beginning of the function body.

Would it be possible to do something similar to turn public methods into private methods? I'd like to take some of the via_patch methods private since external callers really should be using the top-level method, which will determine the best procedure to take. I could also save this for future clean-up; I'm seeing more things that I think can be done more optimally.

[felixfontein]

Sure, you could rename the method (leading underscore) and add another method with the old name that calls module.deprecate() and then the private method.

[mraineri]

Sure, you could rename the method (leading underscore) and add another method with the old name that calls module.deprecate() and then the private method.

Thanks! I'll save that for a future change so I can get the critical portion in.

[mraineri]

OEM workaround logic has been tested.

It's worth noting that the pattern I'd like to establish is for the module to always try the "correct" approach first, and then fall back to workaround logic when errors are encountered. That way if a vendor patches their BMC to support the correct method of implementing the API, no changes are needed in the module to make use of the better approach.

[felixfontein]

I only glanced over the code, but it looks good to me.

[felixfontein]

@mraineri thanks a lot for improving this!