Add power operations to physical server #5
Conversation
miq-bot
changed the title
|
@miq-bot remove_label wip |
|
@miq-bot assign @gtanzillo |
miq-bot
changed the title
| end | ||
|
|
||
| def restart_to_sys_setup(_args, _options = {}) | ||
| $redfish_log.error("Restarting to system setup is not supported.") |
There was a problem hiding this comment.
Would it make sense to raise error as well?
There was a problem hiding this comment.
I just noticed the note above the code. Does that note imply that raised errors are not interpreted properly in the UI? I wonder if the notification system should be used? Like https://github.com/ManageIQ/manageiq/pull/15624/files#diff-0811ad92c7ad82e70f192721e7f910d0R56
@gtanzillo what do you think?
There was a problem hiding this comment.
I raised an exception in my initial implementation here, but since this only fills evm log with useless stack traces with no UI change whatsoever, I changed this to simply log an error message at this stage.
I did implement a quick prototype that uses notification system to inform user about the errors, but in order to have it fully functional, I would need to update core a bit by adding our messages.
There was a problem hiding this comment.
Thanks, @tadeboro.
I have checked Lenovo provider and they are also not raising any errors. As you pointed out, this should be handled in the core. The PR that I referenced was also used in the end instead of having the Notifications in the provider, so I would keep this as it is for the time being.
| def restart_mgmt_controller(_server, _options = {}) | ||
| # TODO(tadeboro): This operation is not well defined, since server can | ||
| # (and usually is) managed by more that one manager. | ||
| $redfish_log.error("Restarting management controller is not supported.") |
There was a problem hiding this comment.
Again, I would rather raise an error as well.
There was a problem hiding this comment.
See previous comment.
|
|
||
| def reset_server(reset_type, server, options = {}) | ||
| $redfish_log.info("Requesting #{reset_type} for #{server.ems_ref}.") | ||
| with_provider_connection(options) do |client| |
There was a problem hiding this comment.
Are the options that above methods receieve actually related to the connection options that you are using here?
There was a problem hiding this comment.
I checked again and no, they are not used for this purpose. They are basically useless since in the current implementation they only contain subset of fields from the server object that we can access anyhow. I will rework the power operations and ignore them completely for now.
| with_provider_connection(options) do |client| | ||
| system = client.find(server.ems_ref) | ||
| if system.nil? | ||
| $redfish_log.error("#{server.ems_ref} does not exist anymore.") |
There was a problem hiding this comment.
We have to understand if log entry is sufficient, because this will again not get the correct into to the caller - it just silently failed.
There was a problem hiding this comment.
Again, see other comments on why things are the way they are.
| return | ||
| end | ||
|
|
||
| response = system.Actions["#ComputerSystem.Reset"].post( |
There was a problem hiding this comment.
Would it make sense to move such const strings into more resilient form? At least a predefined const that will define this string.
There was a problem hiding this comment.
I feel that extracting a string just for the sake of extraction is useless and in this particular case harmful to the readability and comprehension of the code. At the moment, this is the only place that uses this operation name. If we need to restart server from more places than one in the future, we will extract the whole "restart the server" functionality into separate function and not replicate the post call invocation all over the place, still keeping this operation name confined to a single place.
This commit implements parts of the functionality that is used by the power operations controls, exposed in physical server UI.
|
Checked commit xlab-si@caa1cd9 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 **
|
|
@agrare Can you help with the review of this one? |
| # completely useless tool. | ||
|
|
||
| def power_on(server, _options) | ||
| reset_server(server, "On") |
There was a problem hiding this comment.
I was going to say reset_server didn't make sense for the function name, but then I saw the API call was ComputerSystem.Reset so 👍
| end | ||
|
|
||
| def restart_to_sys_setup(_args, _options) | ||
| $redfish_log.error("Restarting to system setup is not supported.") |
There was a problem hiding this comment.
Hm we should have a supports feature for this so the UI/API can check if the method is supported before calling it, something to do in a followup as I'm not sure any of that exists for physical server power ops.
| def restart_mgmt_controller(_server, _options) | ||
| # TODO(tadeboro): This operation is not well defined, since server can | ||
| # (and usually is) managed by more that one manager. | ||
| $redfish_log.error("Restarting management controller is not supported.") |
| :field => "target", :payload => { "ResetType" => reset_type } | ||
| ) | ||
| if [200, 202, 204].include?(response.status) | ||
| $redfish_log.info("#{reset_type} for #{server.ems_ref} started.") |
There was a problem hiding this comment.
Is there a task id/ref returned that can be used to wait for the operation to complete? If so we might want to optionally wait for that to finish since most of our operations are expected to be synchronous.
| if [200, 202, 204].include?(response.status) | ||
| $redfish_log.info("#{reset_type} for #{server.ems_ref} started.") | ||
| else | ||
| $redfish_log.error("#{reset_type} for #{server.ems_ref} failed.") |
There was a problem hiding this comment.
When the API call fails is there any error message returned that we can print to help diagnose the failure?
Add power operations to physical server
This commit implements parts of the functionality that is used by the
power operations controls, exposed in physical server UI.
@miq-bot add_label wip