-
Notifications
You must be signed in to change notification settings - Fork 74
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
Confirmation dialog for VM shutdown, forceoff, reboot and non-maskable interrupt #1069
Conversation
This is nice progress so far. Kind of a huge omission right now: Where's the VM's name? How do you know what you're shutting down or restarting? |
3cb456b
to
0527c4d
Compare
@skobyda Just in case you didn't see, there's still several test failures here. |
ae7a9ca
to
8d4755c
Compare
02b9445
to
2582cda
Compare
a0487c8
to
0f349f5
Compare
src/components/vm/vmActions.jsx
Outdated
handler: () => { | ||
setOperationInProgress(true); | ||
onReboot(vm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this one could have a test. Everything else not really relevant. Will have look at that tomorrow.
1466199
to
1377147
Compare
@garrett Hey, mind giving this a review? I decided not to include the message which says that VM is running x time |
c77bda8
to
327dbba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat minor: Is there a way to make the labels "fluid" width, so it'd look like this?
If you could do that, it would be wonderful. Either in this PR or as a followup.
Additionally, the title should have a question (note: and it shouldn't be bold; only the title of the machine should be) according to PF docs.
https://www.patternfly.org/v4/components/modal/design-guidelines#confirmation-dialogs
@garrett Fixed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Some nitpicks which are optional, but the error handling nees fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! LGTM! 👍
72ede35
to
ca21440
Compare
useEffect(() => { | ||
return domainGetStartTime({ connectionName: vm.connectionName, vmName: vm.name }) | ||
.then(res => setUptime(res)) | ||
.catch(e => console.error(JSON.stringify(e))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test. Details
const onReboot = (vm) => domainReboot({ name: vm.name, id: vm.id, connectionName: vm.connectionName }).catch(ex => { | ||
store.dispatch( | ||
updateVm({ | ||
connectionName: vm.connectionName, | ||
name: vm.name, | ||
error: { | ||
text: cockpit.format(_("VM $0 failed to reboot"), vm.name), | ||
detail: ex.message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 8 added lines are not executed by any test. Details
const onForceReboot = (vm) => domainForceReboot({ name: vm.name, id: vm.id, connectionName: vm.connectionName }).catch(ex => { | ||
store.dispatch( | ||
updateVm({ | ||
connectionName: vm.connectionName, | ||
name: vm.name, | ||
error: { | ||
text: cockpit.format(_("VM $0 failed to force reboot"), vm.name), | ||
detail: ex.message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 8 added lines are not executed by any test. Details
.then(() => !vm.persistent && cockpit.location.go(["vms"])) | ||
.catch(ex => { | ||
setOperationInProgress(false); | ||
store.dispatch( | ||
updateVm({ | ||
connectionName: vm.connectionName, | ||
name: vm.name, | ||
error: { | ||
text: cockpit.format(_("VM $0 failed to shutdown"), vm.name), | ||
detail: ex.message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 10 added lines are not executed by any test. Details
const onPause = (vm) => domainPause({ name: vm.name, id: vm.id, connectionName: vm.connectionName }).catch(ex => { | ||
store.dispatch( | ||
updateVm({ | ||
connectionName: vm.connectionName, | ||
name: vm.name, | ||
error: { | ||
text: cockpit.format(_("VM $0 failed to pause"), vm.name), | ||
detail: ex.message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 8 added lines are not executed by any test. Details
const onResume = (vm) => domainResume({ name: vm.name, id: vm.id, connectionName: vm.connectionName }).catch(ex => { | ||
store.dispatch( | ||
updateVm({ | ||
connectionName: vm.connectionName, | ||
name: vm.name, | ||
error: { | ||
text: cockpit.format(_("VM $0 failed to resume"), vm.name), | ||
detail: ex.message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 8 added lines are not executed by any test. Details
.catch(ex => { | ||
store.dispatch( | ||
updateVm({ | ||
connectionName: vm.connectionName, | ||
name: vm.name, | ||
error: { | ||
text: cockpit.format(_("VM $0 failed to force shutdown"), vm.name), | ||
detail: ex.message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 8 added lines are not executed by any test. Details
const onSendNMI = (vm) => domainSendNMI({ name: vm.name, id: vm.id, connectionName: vm.connectionName }).catch(ex => { | ||
store.dispatch( | ||
updateVm({ | ||
connectionName: vm.connectionName, | ||
name: vm.name, | ||
error: { | ||
text: cockpit.format(_("VM $0 failed to send NMI"), vm.name), | ||
detail: ex.message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 8 added lines are not executed by any test. Details
* - /var/log/libvirt/qemu/ - For privileged (qemu:///system) VMs | ||
* - ~/.cache/libvirt/qemu/logs - For non-privileged (qemu:///session) VMs | ||
*/ | ||
const logFile = connectionName === "system" ? `/var/log/libvirt/qemu/${vmName}.log` : `${loggedUser.home}/.cache/libvirt/qemu/log/${vmName}.log`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This added line is not executed by any test. Details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! PLease update the screenshots in the description for the release note, and reformat the text.
// 2023-05-05 11:22:03.043+0000: starting up libvirt version: 8.6.0, package: 3.fc37 (Fedora Project, 2022-08-09-13:54:03, ), qemu version: 7.0.0qemu-7.0.0-9.fc37, kernel: 6.0.6-300.fc37.x86_64, hostname: fedora | ||
|
||
// Alternatively regex line.match(/\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}/g) can be used | ||
const timeStr = line.split(': starting')[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: due to | tail -1
the script will never fail. line
will then be empty, and thus timeStr will also be empty.
.then(res => setUptime(res)) | ||
.catch(e => console.error(JSON.stringify(e))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per above, domainGetStartTime() now returns ''
if it didn't find any time. Thus the .catch
will never happen (and console.error is fine then, as second-level defense), but it will call setUptime('')
.
AFAICS this is handled correctly below.
Garrett +1'ed above
Machines: Confirm shutdown actions
Ask for confirmation for every action which can cause downtime. The modal also shows uptime to help user from accidentally taking down a VM with a long uptime.