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

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@billdodd
Copy link
Contributor

commented May 3, 2019

SUMMARY

See issue #55869 for a description of the original problem. The module was updated to check the current system power state against the requested power command and avoid sending a Reset when the system is already in the target state. The returned changed value was also updated accordingly.

Fixes #55869

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

redfish_command.py
redfish_utils.py

ADDITIONAL INFORMATION

Before

Issue PowerGracefulShutdown when the system power is already in the "Off" state.

$ ansible-playbook -i myinventory.yml redfish-ansible-module/playbooks/power/power_graceful_shutdown.yml 

PLAY [Manage System Power - Graceful shutdown] *********************************

TASK [Shutdown system power gracefully] ****************************************
 
changed: [mockup-localstorage]

PLAY RECAP *********************************************************************
mockup-localstorage        : ok=1    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   

After

Issue PowerGracefulShutdown when the system power is already in the "Off" state.

$ ansible-playbook -i myinventory.yml redfish-ansible-module/playbooks/power/power_graceful_shutdown.yml 

PLAY [Manage System Power - Graceful shutdown] *********************************

TASK [Shutdown system power gracefully] ****************************************
 
ok: [mockup-localstorage]

PLAY RECAP *********************************************************************
mockup-localstorage        : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   
@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@ansibot ansibot removed the owner_pr label May 3, 2019

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.

@ansibot ansibot removed the needs_triage label May 6, 2019

@jose-delarosa

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Tested it, looks good, thank you @billdodd

shipit

@billdodd

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Tested it, looks good, thank you @billdodd

Thanks, @jose-delarosa!

@ansibot ansibot added shipit and removed community_review labels May 14, 2019

@mraineri

This comment has been minimized.

Copy link

commented May 20, 2019

shipit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.