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

redfish: always do off/delay/on for power cycle #149

Merged
merged 4 commits into from
Feb 25, 2024

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Feb 25, 2024

Per discussion in #148

I think the first and second commit are pretty obvious given the discussion.

Commit number 3 is more debatable. It would break backwards compatibility if anyone copied/modified an older redfishpower device file.

The primary reason I wanted to go down this path to remove redfishpower "cycle" is b/c parent support (#81) cannot work with native power cycle b/c of the inherent raciness of checking the power status after a power cycle.

@garlick
Copy link
Member

garlick commented Feb 25, 2024

It would break backwards compatibility if anyone copied/modified an older redfishpower device file.

This would only affect the cycle command, right? Seems like that one's probably not all that commonly used anyway...

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines 57 to 59
send "cycle %s\n"
send "off %s\n"
expect "redfishpower> "
send "on %s\n"
expect "redfishpower> "
Copy link
Member

Choose a reason for hiding this comment

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

Usually we put a delay in here?

Copy link
Member Author

Choose a reason for hiding this comment

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

lol, i guess the one i copied and pasted from is missing a delay. I'll fix up everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

i just remembered that delay isn't strictly necessary here because we "wait until off" for the off command. But a couple seconds delay couldn't hurt since that is the common pattern.

Copy link
Member

Choose a reason for hiding this comment

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

The delay is to actually have the hardware in the off state for a period of time before reapplying power. The "wait until on|off" thang is presumed to be how things work throughout powerman.

Problem: When a power cycle is implemented as an off followed
by an on, a delay between the two is typically added in case the
system needs to "settle" a bit.

This is not strictly necessary for redfishpower, since it verifies
the power is off before performing the on.  However, since it is
the common pattern in powerman, it couldn't hurt to add a small
delay just in case the system needs "settling" time.

Add a small 2 second delay in cycle_ranged in
etc/devices/redfishpower-cray-windom.dev.
Problem: A native power cycle with the redfish protocol is
inherently racy.  After performing a power cycle and immediately
getting the power status, there is no way to know if the power
status of "on" is from before or after the cycle.  This can be
confusing to users.

In all redfishpower devices files, define power cycles as an
off, delay, and on.  Do not use native redfish cycle/restart.

Fixes chaos#148
Problem: We now define all redfishpower cycles as an off, delay,
and on.  Some configuration notes are now out of date due to this
change.

Update redfishpower configuration notes to account for this change.
Problem: Due to the raciness of power status after a power cycle,
we now define power cycle as off, delay, on in all redfishpower
device files.  Now power "cycle" is no longer needed in redfishpower.

Remove all power cycle support in redfishpower.  Remove all documentation
in redfishpower(8) as well.
@chu11
Copy link
Member Author

chu11 commented Feb 25, 2024

re-pushed adding an extra commit to add the delay. Wanna take an extra skim @garlick?

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

LGTM!

@mergify mergify bot merged commit 560c926 into chaos:master Feb 25, 2024
8 checks passed
@chu11 chu11 deleted the redfish_nocycle branch February 27, 2024 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants