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

Avoid power on/off commands if system already in target state #56069

Merged
merged 2 commits into from Jun 10, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -0,0 +1,3 @@
---
bugfixes:
- redfish_command - make power commands idempotent (https://github.com/ansible/ansible/issues/55869)
@@ -505,16 +505,21 @@ def manage_indicator_led(self, command):
return result

def manage_system_power(self, command):
result = {}
key = "Actions"

# Search for 'key' entry and extract URI from it
response = self.get_request(self.root_uri + self.systems_uris[0])
if response['ret'] is False:
return response
result['ret'] = True
data = response['data']
power_state = data["PowerState"]

if power_state == "On" and command == 'PowerOn':
return {'ret': True, 'changed': False}

This comment has been minimized.

Copy link
@samerhaj

samerhaj May 6, 2019

should we also add support to "ForceOn", "ForceRestart", "PushPowerButton", "Nmi", and "PowerCycle" that are supported in the Redfish ComputerSystem ResetType enum ?

This comment has been minimized.

Copy link
@billdodd

billdodd May 6, 2019

Author Contributor

We certainly can add those commands if you think they are needed/valuable. If so, please open a new issue to add that support. (For the current issue/PR I want to fix the existing commands.)

This comment has been minimized.

Copy link
@mraineri

mraineri May 6, 2019

I agree with ForceOn, but the other values I think would have some immediate impact.

  • ForceRestart I wouldn't expect to affect the power state, but would cause a reset of the CPU complex
  • Nmi would likely just trigger an NMI immediately; no power state change, but affects the state of the host software
  • PushPowerButton I think could have different behaviors; I think it would likely turn the system off if it's already on, or turn the system on if it's already off
  • PowerCycle would have similar semantics to ForceRestart; the end power state doesn't change, but some action was taken that affected the system

This comment has been minimized.

Copy link
@billdodd

billdodd May 6, 2019

Author Contributor

Thanks, Mike and Samer for the comments.

Mike - agree with your comments about the impact of those commands (system changed vs. not changed).

But just to clarify in case it wasn't obvious - the Ansible redfish_command module does not currently have commands for "ForceOn", "ForceRestart", "PushPowerButton", "Nmi", or "PowerCycle". We can add those, but would rather do that as a separate feature issue.

This comment has been minimized.

Copy link
@mraineri

mraineri May 6, 2019

Ah, gotcha. Yes, I agree it would make more sense to add those checks when Ansible makes use of new reset types.

This comment has been minimized.

Copy link
@samerhaj

samerhaj Jun 4, 2019

I agree on making these a separate pull request


if power_state == "Off" and command in ['PowerGracefulShutdown', 'PowerForceOff']:
return {'ret': True, 'changed': False}

reset_action = data[key]["#ComputerSystem.Reset"]
action_uri = reset_action["target"]
allowable_vals = reset_action.get("ResetType@Redfish.AllowableValues", [])
@@ -542,8 +547,7 @@ def manage_system_power(self, command):
response = self.post_request(self.root_uri + action_uri, payload)
if response['ret'] is False:
return response
result['ret'] = True
return result
return {'ret': True, 'changed': True}

def list_users(self):
result = {}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.