BareMetalHost: Add Restart action #4862
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
|
|
|
Cc @andybraren I did not follow the original design doc when implementing this because the later reboot interface (published 3 months after the design) does not reflect it on the backend. Please comment. |
Bare Metal Host list and detail pages are extended for the "Restart" action (available for a running BMH only). The status field is enriched to reflect new state.
mareklibra
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
|
||
| return ( | ||
| <form onSubmit={onSubmit} name="form" className="modal-content"> | ||
| <ModalTitle>Restart host {getName(host)}</ModalTitle> |
There was a problem hiding this comment.
| <ModalTitle>Restart host {getName(host)}</ModalTitle> | |
| <ModalTitle>Restart Bare Metal Host</ModalTitle> |
I think this aligns more closely to other modals in the Console from what I can see.
| <ModalTitle>Restart host {getName(host)}</ModalTitle> | ||
| <ModalBody> | ||
| The bare metal host will be scheduled for restart which will be executed once all managed | ||
| workloads are moved to nodes on other hosts. |
There was a problem hiding this comment.
The second part of this sentence only applies to hosts that are currently provisioned as nodes. If they aren't being used as nodes, then there aren't any workloads to move. I remember this was a bit tricky to explain/show in the design. It's been a while! 😄
If we'd prefer to add logic that checks for that in a future PR, maybe we could use this sentence for now:
The bare metal host {getName(host)} will be restarted gracefully. If the host is provisioned as a node, managed workloads will be moved to other nodes before the host powers off.
There was a problem hiding this comment.
Thank you. I have rephrased the text for both cases (w/o node).
| import { HOST_POWER_STATUS_POWERED_ON, HOST_REGISTERING_STATES } from '../../constants'; | ||
|
|
||
| type BareMetalHostSecondaryStatusProps = { | ||
| host: BareMetalHostKind; | ||
| }; | ||
|
|
||
| export const HOST_SCHEDULED_FOR_RESTART = 'Scheduled for restart'; |
There was a problem hiding this comment.
"Scheduled for restart" basically means the host is "Restarting" right? I worry a little bit that "Scheduled for" makes it sound like the restart will happen at some scheduled time in the far future instead of "very soon, like in 10 seconds."
It also seems like the host "Restarting" is more important than it being "Provisioned." Should "Restarting" just become the primary status (with an in-progress icon)? If we think saying that the host being "Provisioned" is still important, maybe that could be the sub-status instead? FYI @jtomasek
There was a problem hiding this comment.
When reading Reboot API, the semantics of the action is just "restart is requested", means it is "planed to happen when all conditions are met". There is no guarantee this to ever happen.
If/when the restart actually starts, the progress will be denoted by Powering off status.
I understand this does not conform the common sense for this action but it follows the declarative semantics implemented on the backend.
If the user needs "to force the restart now", there's still the Power on|off button.
Wdyt?
There was a problem hiding this comment.
Ah okay, I read the link more carefully this time. 🙂
How about we change "Scheduled for restart" to "Restart pending" then, which aligns a little more closely with the API's pendingRebootSince field?
I personally like the word "Restart", but I wonder if we should align even more closely to the API and call this "Reboot" everywhere in the UI instead. So "Reboot pending" and tweaks to the action label, modal text, etc.
WDYT?
There was a problem hiding this comment.
I think we do not need to be aligned on this with the backend since there are loose constraints for naming.
The openshift/console uses restart. The Cluster incidents in ACM use reststart as well. So I am for keeping consistency on that level.
There was a problem hiding this comment.
I have changed "Scheduled for restart" to "Restart pending".
There was a problem hiding this comment.
Sticking with Restart sounds good, thanks Marek!
|
/retest |
|
/retest |
|
/LGTM |
openshift-ci-robot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andybraren, jtomasek, mareklibra The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Bare Metal Host list and detail pages are extended for the "Restart" action (available for a running BMH only).
The status field is enriched to reflect new state.
Implementation is based on https://github.com/metal3-io/metal3-docs/blob/master/design/reboot-interface.md