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

expose the ActionInfo property under Actions under the Managers colle… #345

Merged
merged 3 commits into from
May 23, 2024

Conversation

iamsli
Copy link
Contributor

@iamsli iamsli commented May 23, 2024

exposes the ActionInfo property under Actions under the Managers collection

per Redfish docs ActionInfo is a common property across the Actions object. Exposing that property in the Managers collection

@stmcginnis
Copy link
Owner

Thanks. It's a recommended property, but from what I've seen there are only a small number of vendors that actually implement it so far.

This just captures the string to a private property. Is there more you are looking at adding to do something with this? It may be better to add it to the ActionInfo struct and update this spot to use a struct that extends that to get the AllowedResetTypes.

@iamsli
Copy link
Contributor Author

iamsli commented May 23, 2024

ah thank you. i didn't see that struct. i will update this now. for reference this is the output i'm seeing:

  "Actions": {
    "#Manager.Reset": {
      "@Redfish.ActionInfo": "/redfish/v1/Managers/Bluefield_BMC/ResetActionInfo",
      "target": "/redfish/v1/Managers/Bluefield_BMC/Actions/Manager.Reset"
    },

a call to manager.Reset(<RESET_TYPE>) is failing because nothing is reported under allowableValues even though the vendor has this implemented, but to find this info we have to get it from the ActionInfo endpoint. How do you think this should be handled?

@stmcginnis
Copy link
Owner

a call to manager.Reset(<RESET_TYPE>) is failing because nothing is reported under allowableValues

That wouldn't be why the reset call is failing. If there are no reported allowable reset types, it will just skip performing the check up front. So the type of reset you are requesting must not be one of the supported kinds, but that wouldn't change the result you are getting. It's just a little more efficient for Gofish to tell you right away if something isn't supported, versus attempting to perform the action and the system telling you the same thing.

So you most likely just need to change the ResetType that you are passing in. Gofish would just give you a slightly better failure message maybe, but it still wouldn't tell you what the valid values would be.

@iamsli
Copy link
Contributor Author

iamsli commented May 23, 2024

correct me if i'm wrong but the gofish Reset() makes a Reset call with an empty payload when AllowableValues is empty. In this case, AllowableValues is empty so i'm are making an empty call and getting an error message. The reset type i'm passing in via gofish and via curl are the same. the curl call is working but not the gofish call

@stmcginnis
Copy link
Owner

Ah, you're right. Looks like we had to do that to make HPE happy.

So I think this PR is part of the way there. If supported types is empty, then we could check if there is an action info link and make an extra call to query the system for supported types. Or to minimize overhead, I guess the presence of that action info link could be enough to differentiate from the HPE systems that don't report anything at all, so if so just put the ResetType in the payload.

Actually querying the system using the link would be more correct, just thinking about minimizing that round trip time. If you want to try things out and see what works best, I could go either way.

Thanks!

@iamsli
Copy link
Contributor Author

iamsli commented May 23, 2024

I agree with you on minimizing roundtrip time and implemented it so that it just checks if actionInfo is there. LMK what you think!

Copy link
Owner

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

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

Thanks!

@stmcginnis stmcginnis merged commit 8061ec5 into stmcginnis:main May 23, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants