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

Update redfish module for compatibility with VirtualMedia resource location #5124

Merged
merged 4 commits into from
Aug 24, 2022

Conversation

jixj5
Copy link
Contributor

@jixj5 jixj5 commented Aug 17, 2022

SUMMARY

Update redfish module for compatibility due to redfish spec changes the virtualMedia resource location from Manager to Systems.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

redfish

ADDITIONAL INFORMATION

Virtual media resource locates on Manager or Systems category.

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module module module_utils module_utils plugins plugin (any type) remote_management labels Aug 17, 2022
@github-actions
Copy link

github-actions bot commented Aug 17, 2022

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution.

plugins/module_utils/redfish_utils.py Outdated Show resolved Hide resolved
# if ejected, 'Inserted' should be False
if (not data.get('Inserted', False)):
# Base on current XCC capability, slot RDOC1/2 and Remote1/2/3/4 are not supported to Insert/Eject.
if ("/Systems/1/" in uri or "/Manager/1/" in uri) and ('RDOC' in uri or 'Remote' in uri):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct for all kind of Redfish devices out there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may use another way to identify lenovo XCC, like 'Vendor' in /redfish/v1.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also avoid hard coding things like "RDOC" and "Remote" as methods to detect the capability of setting the image. We may need to think of different logic to determine when a slot cannot be modified. I was hoping the Allow HTTP header would be able to tell us this, but it seems like you can PATCH other properties on this type of virtual media resource. I'll ask around if we can think of more hardened logic for this.

plugins/module_utils/redfish_utils.py Outdated Show resolved Hide resolved
plugins/module_utils/redfish_utils.py Outdated Show resolved Hide resolved
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 labels Aug 18, 2022
@jixj5 jixj5 closed this Aug 18, 2022
@jixj5 jixj5 reopened this Aug 18, 2022
# if ejected, 'Inserted' should be False
if (not data.get('Inserted', False)):
# Base on current Lenovo server capability, filter out slot RDOC1/2 and Remote1/2/3/4 which are not supported to Insert/Eject.
if vendor == 'Lenovo' and ('RDOC' in uri or 'Remote' in uri):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still working to see if there are alternatives to this type of inspection. I'm not coming up on much at the moment though, so this may be the only path forward.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I'm not finding other good alternatives, so this is the best we can work with at this time.

@@ -202,6 +202,16 @@ def _get_extended_message(error):
def _init_session(self):
pass

def _get_vendor(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a note to myself, I may want to consider making more use of this to handle vendor-specific checks in the future (and possibly revisit existing vendor checks). This is a good addition to have.

@mraineri
Copy link
Contributor

shipit

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Aug 24, 2022
@felixfontein felixfontein merged commit 766c109 into ansible-collections:main Aug 24, 2022
@patchback
Copy link

patchback bot commented Aug 24, 2022

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/766c109d470fdaeaa04e5b16143c4b34904f1511/pr-5124

Backported as #5180

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Aug 24, 2022
…cation (#5124)

* Update redfish module for compatibility with VirtualMedia resource location from Manager to Systems

* Add changelogs fragments for PR 5124

* Update some issue according to the suggestions

* update changelogs fragment to list new features in the minor_changes catagory

Co-authored-by: Tami YY3 Pan <panyy3@lenovo.com>
(cherry picked from commit 766c109)
@felixfontein
Copy link
Collaborator

@jixj5 @panyy3 thanks for your contribution!
@mraineri thanks for reviewing!

felixfontein pushed a commit that referenced this pull request Aug 24, 2022
…cation (#5124) (#5180)

* Update redfish module for compatibility with VirtualMedia resource location from Manager to Systems

* Add changelogs fragments for PR 5124

* Update some issue according to the suggestions

* update changelogs fragment to list new features in the minor_changes catagory

Co-authored-by: Tami YY3 Pan <panyy3@lenovo.com>
(cherry picked from commit 766c109)

Co-authored-by: jixj5 <66418293+jixj5@users.noreply.github.com>
bratwurzt pushed a commit to bratwurzt/community.general that referenced this pull request Nov 7, 2022
…cation (ansible-collections#5124)

* Update redfish module for compatibility with VirtualMedia resource location from Manager to Systems

* Add changelogs fragments for PR 5124

* Update some issue according to the suggestions

* update changelogs fragment to list new features in the minor_changes catagory

Co-authored-by: Tami YY3 Pan <panyy3@lenovo.com>
bratwurzt pushed a commit to bratwurzt/community.general that referenced this pull request Nov 7, 2022
…cation (ansible-collections#5124)

* Update redfish module for compatibility with VirtualMedia resource location from Manager to Systems

* Add changelogs fragments for PR 5124

* Update some issue according to the suggestions

* update changelogs fragment to list new features in the minor_changes catagory

Co-authored-by: Tami YY3 Pan <panyy3@lenovo.com>
This was referenced Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug module_utils module_utils module module plugins plugin (any type) remote_management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants