Page MenuHomeFreeBSD
Differential D12777

Implenent power cycle option to reboot
ClosedPublic
Actions

Authored by imp on Oct 24 2017, 5:25 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Oct 21, 7:31 AM
Unknown Object (File)
Mon, Oct 21, 6:57 AM
Unknown Object (File)
Mon, Oct 21, 6:57 AM
Unknown Object (File)
Mon, Oct 21, 6:57 AM
Unknown Object (File)
Mon, Oct 21, 6:56 AM
Unknown Object (File)
Mon, Oct 21, 6:56 AM
Unknown Object (File)
Mon, Oct 21, 3:02 AM
Unknown Object (File)
Mon, Oct 21, 3:02 AM
View All Files
Subscribers
dab
dhw
vangyzen

Details

Reviewers
kib
scottl
jhb
jtl
vangyzen
dhw
dab
Summary

Define RB_POWERCYCLE

RB_POWERCYCLE instructs the platform to power off and then power back
on a short time later, if that's possible. Otherwise, degrade to the
RB_POWEROFF behavior.

Add power cycle support (-c) to shutdown.

Add power cycle support to reboot/halt as -c.

When -c is specified, the system will be power cycled if the
underlying hardware supports it. Otherwise the system will be halted
or rebooted depending on which command was used.

Implement power cycle in init.

If SIGWINCH is received, then halt with power cycle.

Document RB_POWERCYCLE.

Handle RB_POWERCYCLE.

Signal init with SIGWINCH in shutdown_nice for RB_POWERCYCLE.

Handle RB_POWERCYCLE in ada driver

Allow the disks to be spun down when doing a POWERCYCLE as well as
POWEROFF.

Implement IPMI support for RB_POWRECYCLE

Some BMCs support power cycling the chassis via the chassis control
command 2 subcommand 2 (ipmitool called it 'chassis power cycle'). If
the BMC supports the chassis device, register a shutdown_final handler
that sends the power cycle command if request and waits up to 10s for
it to take effect. To minimize stack strain, we preallocate a ipmi
request in the softc. At the moment, we're verbose about what we're
doing.

Sponsored by: Netflix

for review this is one review, but will be several commits

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 12210
Build 12506: arc lint + arc unit

Event Timeline

imp created this revision.Oct 24 2017, 5:25 PM
Harbormaster completed remote builds in B12203: Diff 34293.Oct 24 2017, 5:25 PM
imp added reviewers: kib, scottl, jhb, jonlooney_gmail.com.Oct 24 2017, 5:32 PM
jtl edited reviewers, added: jtl; removed: jonlooney_gmail.com.Oct 24 2017, 5:46 PM
scottl accepted this revision.Oct 24 2017, 5:49 PM
This revision is now accepted and ready to land.Oct 24 2017, 5:49 PM
jtl added a comment.Oct 24 2017, 6:09 PM

Seems like a good idea! See in-line comments.

sbin/reboot/reboot.c
125

Suggest adding a similar check for -p and -c used together.

sbin/shutdown/shutdown.8
75

It looks like it might be clearer if this was turned into a separate sentence.

For example:

The system is halted and the power is turned off at the specified
.Ar time .
If the hardware does not support this option, the system is halted instead.
sys/dev/ipmi/ipmi.c
697

Where is ipmi_req defined? Or, is this supposed to be sc->ipmi_static_req?

Is there a reason not to use IPMI_ALLOC_DRIVER_REQUEST()?

sys/dev/ipmi/ipmivars.h
123

Is this what you used as ipmi_req before? Also, as I noted before, does IPMI_ALLOC_DRIVER_REQUEST() - which uses alloca - obviate the need for this?

sys/kern/kern_shutdown.c
267

SIGWINCH?

kib accepted this revision.Oct 24 2017, 6:28 PM
kib added inline comments.
lib/libc/sys/reboot.2
91

I think that being more specific there and saying that the power-reset cycle only supported for IPMI with chassis device capabilities would be useful.

imp added inline comments.Oct 24 2017, 6:41 PM
lib/libc/sys/reboot.2
91

I'll find some way to say this. We didn't say that POWEROFF was only supported on apm machines when that flag went into the tree, so I get that we need to provide some guidance about what can do this, but don't want to get too specific... thanks!

sbin/reboot/reboot.c
125

Good idea.

sbin/shutdown/shutdown.8
75

Good suggestion. Might also be good to sprinkle kib's suggestion in as well.

sys/dev/ipmi/ipmi.c
697

Doh! It was a last minute change to ipmi_static_req in the header...

I wanted to make sure there was sufficient stack space, but I suppose that's a good assumption...

sys/dev/ipmi/ipmivars.h
123

Yes. I think I'll simplify as you suggest. I think I may have been paranoid about the stack for no good reason.

vangyzen accepted this revision.Oct 24 2017, 7:44 PM
vangyzen added a subscriber: vangyzen.
vangyzen added inline comments.
sys/dev/ipmi/ipmi.c
899

Update this comment.

sys/sys/ipmi.h
59

CHASIS -> CHASSIS, here and the following line

imp updated this revision to Diff 34300.Oct 24 2017, 8:13 PM
  • Define RB_POWERCYCLE
  • Add power cycle support (-c) to shutdown.
  • Add power cycle support to reboot/halt as -c.
  • Implement power cycle in init.
  • Handle RB_POWERCYCLE.
  • Handle RB_POWERCYCLE in ada driver
  • Implement IPMI support for RB_POWRECYCLE
  • RB_POWERCYCLE needs to be handled like RB_POWEROFF for decoding.
This revision now requires review to proceed.Oct 24 2017, 8:13 PM
Harbormaster completed remote builds in B12208: Diff 34300.Oct 24 2017, 8:13 PM
imp updated this revision to Diff 34301.Oct 24 2017, 8:16 PM
  • Add power cycle support to reboot/halt as -c.
  • Implement power cycle in init.
  • Handle RB_POWERCYCLE.
  • Handle RB_POWERCYCLE in ada driver
  • Implement IPMI support for RB_POWRECYCLE
  • RB_POWERCYCLE needs to be handled like RB_POWEROFF for decoding.
Harbormaster completed remote builds in B12209: Diff 34301.Oct 24 2017, 8:16 PM
imp updated this revision to Diff 34302.Oct 24 2017, 8:18 PM

with this change, all the comments should be addressed

  • Define RB_POWERCYCLE
  • Add power cycle support (-c) to shutdown.
  • Add power cycle support to reboot/halt as -c.
  • Implement power cycle in init.
  • Handle RB_POWERCYCLE.
  • Handle RB_POWERCYCLE in ada driver
  • Implement IPMI support for RB_POWRECYCLE
  • RB_POWERCYCLE needs to be handled like RB_POWEROFF for decoding.
Harbormaster completed remote builds in B12210: Diff 34302.Oct 24 2017, 8:18 PM
vangyzen accepted this revision.Oct 24 2017, 8:21 PM
vangyzen added inline comments.
share/man/man4/ipmi.4
99

PMC -> BMC

This revision is now accepted and ready to land.Oct 24 2017, 8:21 PM
imp updated this revision to Diff 34303.Oct 24 2017, 8:25 PM

Fix the comment I cut and pasted...

This revision now requires review to proceed.Oct 24 2017, 8:25 PM
Harbormaster completed remote builds in B12211: Diff 34303.Oct 24 2017, 8:25 PM
imp marked 13 inline comments as done.Oct 24 2017, 8:28 PM
imp updated this revision to Diff 34304.Oct 24 2017, 8:28 PM
  • Implement IPMI support for RB_POWRECYCLE
vangyzen accepted this revision.Oct 24 2017, 8:29 PM
This revision is now accepted and ready to land.Oct 24 2017, 8:29 PM
dhw added a subscriber: dhw.Oct 24 2017, 10:16 PM
dhw added inline comments.
sbin/shutdown/shutdown.8
64

"at the specified time"? Where is (or should) it be specified? (A pointer to where one might find the information would be fine -- no point in duplicating the information & risking a change in one, but not the other.)

imp added inline comments.Oct 24 2017, 10:34 PM
sbin/shutdown/shutdown.8
64

Already in the shutdown man page. this language is identical to the power off language, so I don't think we need a pointer.

dhw added inline comments.Oct 24 2017, 10:41 PM
sbin/shutdown/shutdown.8
64

Silly me! -- overlooked the context; sorry for the noise.

dhw accepted this revision.Oct 24 2017, 10:41 PM
jtl accepted this revision.Oct 24 2017, 11:17 PM
imp updated this revision to Diff 34308.Oct 24 2017, 11:20 PM

wordsmith the power off interval

This revision now requires review to proceed.Oct 24 2017, 11:20 PM
jtl accepted this revision.Oct 24 2017, 11:37 PM
This revision is now accepted and ready to land.Oct 24 2017, 11:37 PM
imp closed this revision.Oct 26 2017, 4:23 AM

This was committed w/o the right tags to have it close automatically in r324983-r324990.

dab added a subscriber: dab.Oct 31 2017, 5:36 PM
dab added inline comments.
lib/libsysdecode/flags.c
652

Not a big deal, but you might consider putting the flags in the same order here as in the comment. It would make it easier to see that the two correspond. Plus, I like alphabetical order. :-)

sys/dev/ipmi/ipmi.c
726

s/hanlder/handler/

839

s/system off/system/

(power cycle implies off/on, not just off)

dab added a reviewer: dab.Oct 31 2017, 5:36 PM

Revision Contents
Changeset List

PathSize
lib/
libc/
sys/
10 lines
libsysdecode/
4 lines
sbin/
init/
1 line
7 lines
reboot/
19 lines
15 lines
shutdown/
15 lines
29 lines
share/
man/
man4/
11 lines
sys/
cam/
ata/
2 lines
dev/
ipmi/
56 lines
2 lines
kern/
4 lines
sys/
17 lines
1 line

Diff 34302

View Options

lib/libc/sys/reboot.2

Loading...
View Options

lib/libsysdecode/flags.c

Loading...
View Options

sbin/init/init.8

Loading...
View Options

sbin/init/init.c

Loading...
View Options

sbin/reboot/reboot.8

Loading...
View Options

sbin/reboot/reboot.c

Loading...
View Options

sbin/shutdown/shutdown.8

Loading...
View Options

sbin/shutdown/shutdown.c

Loading...
View Options

share/man/man4/ipmi.4

Loading...
View Options

sys/cam/ata/ata_da.c

Loading...
View Options

sys/dev/ipmi/ipmi.c

Loading...
View Options

sys/dev/ipmi/ipmivars.h

Loading...
View Options

sys/kern/kern_shutdown.c

Loading...
View Options

sys/sys/ipmi.h

Loading...
View Options

sys/sys/reboot.h

Loading...