Confirmation dialog for VM shutdown, forceoff, reboot and non-maskable interrupt #1069

skobyda · 2023-05-05T13:26:07Z

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.

src/components/vm/vmActions.jsx

garrett · 2023-05-08T09:43:08Z

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?

skobyda · 2023-05-11T00:26:14Z

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?

Oh right, forgot about it. Updated it:

martinpitt · 2023-05-16T05:50:27Z

@skobyda Just in case you didn't see, there's still several test failures here.

skobyda · 2023-05-25T10:53:06Z

Also, if VM was started as a result of some secondary operation, e.g. snapshot-revert, migration let's show the message next to it. We don't know for how long has the VM been running before, since after snapshot-revert or migration, the new VM instance is spawned:

skobyda · 2023-06-21T22:18:15Z

src/components/vm/vmActions.jsx

+                                    handler: () => {
+                                        setOperationInProgress(true);
+                                        onReboot(vm);


Oh this one could have a test. Everything else not really relevant. Will have look at that tomorrow.

skobyda · 2023-06-23T09:35:52Z

@garrett Hey, mind giving this a review? I decided not to include the message which says that VM is running x time since snapshot revert (e.g. like seen in this screenshot), because there was some race condition it tests related to that (I need to look at that more). Now the UI only shows that VM is running x time (without saying what was the reason for VM startup), e.g. like this:

garrett

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

skobyda · 2023-06-26T10:33:07Z

@garrett Fixed:

martinpitt

Thanks! Some nitpicks which are optional, but the error handling nees fixing.

src/libvirtApi/domain.js

src/components/vm/confirmDialog.jsx

garrett

Thanks for the changes! LGTM! 👍

Fixes cockpit-project#994 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2180281 Fixes https://issues.redhat.com/browse/RHELPLAN-152520

cockpituous · 2023-06-26T14:09:33Z

src/components/vm/confirmDialog.jsx

+    useEffect(() => {
+        return domainGetStartTime({ connectionName: vm.connectionName, vmName: vm.name })
+                .then(res => setUptime(res))
+                .catch(e => console.error(JSON.stringify(e)));


This added line is not executed by any test. Details

cockpituous · 2023-06-26T14:09:33Z

src/components/vm/vmActions.jsx

+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,


These 8 added lines are not executed by any test. Details

cockpituous · 2023-06-26T14:09:33Z

src/components/vm/vmActions.jsx

+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,


These 8 added lines are not executed by any test. Details

cockpituous · 2023-06-26T14:09:33Z

src/components/vm/vmActions.jsx

+        .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,


These 10 added lines are not executed by any test. Details

cockpituous · 2023-06-26T14:09:33Z

src/components/vm/vmActions.jsx

+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,


These 8 added lines are not executed by any test. Details

cockpituous · 2023-06-26T14:09:34Z

src/components/vm/vmActions.jsx

+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,


These 8 added lines are not executed by any test. Details

cockpituous · 2023-06-26T14:09:34Z

src/components/vm/vmActions.jsx

+        .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,


These 8 added lines are not executed by any test. Details

cockpituous · 2023-06-26T14:09:34Z

src/components/vm/vmActions.jsx

+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,


These 8 added lines are not executed by any test. Details

cockpituous · 2023-06-26T14:09:34Z

src/libvirtApi/domain.js

+         * - /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`;


This added line is not executed by any test. Details

martinpitt

Thank you! PLease update the screenshots in the description for the release note, and reformat the text.

martinpitt · 2023-06-26T19:17:28Z

src/libvirtApi/domain.js

+                // 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];


Note to self: due to | tail -1 the script will never fail. line will then be empty, and thus timeStr will also be empty.

martinpitt · 2023-06-26T19:20:02Z

src/components/vm/confirmDialog.jsx

+                .then(res => setUptime(res))
+                .catch(e => console.error(JSON.stringify(e)));


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

skobyda force-pushed the confirmShutdown branch from 44f637b to 348ef46 Compare May 5, 2023 13:37

KKoukiou previously requested changes May 5, 2023

View reviewed changes

src/components/vm/vmActions.jsx Outdated Show resolved Hide resolved

src/components/vm/vmActions.jsx Show resolved Hide resolved

src/components/vm/vmActions.jsx Outdated Show resolved Hide resolved

skobyda force-pushed the confirmShutdown branch from 348ef46 to 2b051cc Compare May 11, 2023 00:22

skobyda requested a review from garrett May 11, 2023 00:26

skobyda force-pushed the confirmShutdown branch 3 times, most recently from 3cb456b to 0527c4d Compare May 15, 2023 12:17

skobyda marked this pull request as ready for review May 15, 2023 12:17

skobyda added the release-note label May 15, 2023

skobyda force-pushed the confirmShutdown branch 5 times, most recently from ae7a9ca to 8d4755c Compare May 16, 2023 12:12

skobyda force-pushed the confirmShutdown branch 4 times, most recently from 02b9445 to 2582cda Compare May 25, 2023 10:51

skobyda force-pushed the confirmShutdown branch 4 times, most recently from a0487c8 to 0f349f5 Compare June 21, 2023 19:36

skobyda commented Jun 21, 2023

View reviewed changes

skobyda requested review from garrett and removed request for garrett June 22, 2023 08:26

skobyda force-pushed the confirmShutdown branch from 0f349f5 to 9800666 Compare June 22, 2023 09:54

skobyda force-pushed the confirmShutdown branch 2 times, most recently from 1466199 to 1377147 Compare June 22, 2023 17:16

skobyda force-pushed the confirmShutdown branch 3 times, most recently from c77bda8 to 327dbba Compare June 23, 2023 09:42

garrett previously requested changes Jun 26, 2023

View reviewed changes

skobyda force-pushed the confirmShutdown branch from 327dbba to 88dbde0 Compare June 26, 2023 10:30

skobyda requested a review from garrett June 26, 2023 10:33

skobyda force-pushed the confirmShutdown branch from 88dbde0 to 2484010 Compare June 26, 2023 10:52

skobyda requested review from martinpitt, KKoukiou and marusak June 26, 2023 11:18

martinpitt requested changes Jun 26, 2023

View reviewed changes

garrett reviewed Jun 26, 2023

View reviewed changes

skobyda force-pushed the confirmShutdown branch 2 times, most recently from 72ede35 to ca21440 Compare June 26, 2023 12:46

skobyda added 3 commits June 26, 2023 15:51

Get VM's start time

8d2b111

Move vm actions handlers out of component's scope for reusability

5b06cc9

Add confirmation dialog for shutdown and reboot operation

6d1bd25

Fixes cockpit-project#994 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2180281 Fixes https://issues.redhat.com/browse/RHELPLAN-152520

skobyda force-pushed the confirmShutdown branch from ca21440 to 6d1bd25 Compare June 26, 2023 13:51

cockpituous reviewed Jun 26, 2023

View reviewed changes

skobyda requested a review from martinpitt June 26, 2023 15:19

martinpitt approved these changes Jun 26, 2023

View reviewed changes

martinpitt merged commit d58afde into cockpit-project:main Jun 26, 2023

martinpitt mentioned this pull request Jun 26, 2023

Show virtual interface's TAP device #1077

Merged

martinpitt removed the release-note label Jul 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Confirmation dialog for VM shutdown, forceoff, reboot and non-maskable interrupt #1069

Confirmation dialog for VM shutdown, forceoff, reboot and non-maskable interrupt #1069

skobyda commented May 5, 2023 •

edited by martinpitt

Loading

garrett commented May 8, 2023

skobyda commented May 11, 2023

martinpitt commented May 16, 2023

skobyda commented May 25, 2023 •

edited

Loading

skobyda Jun 21, 2023

skobyda commented Jun 23, 2023

garrett left a comment

skobyda commented Jun 26, 2023

martinpitt left a comment

garrett left a comment

cockpituous Jun 26, 2023

cockpituous Jun 26, 2023

cockpituous Jun 26, 2023

cockpituous Jun 26, 2023

cockpituous Jun 26, 2023

cockpituous Jun 26, 2023

cockpituous Jun 26, 2023

cockpituous Jun 26, 2023

cockpituous Jun 26, 2023

martinpitt left a comment

martinpitt Jun 26, 2023

martinpitt Jun 26, 2023

		.then(res => setUptime(res))
		.catch(e => console.error(JSON.stringify(e)));

Confirmation dialog for VM shutdown, forceoff, reboot and non-maskable interrupt #1069

Confirmation dialog for VM shutdown, forceoff, reboot and non-maskable interrupt #1069

Conversation

skobyda commented May 5, 2023 • edited by martinpitt Loading

Machines: Confirm shutdown actions

garrett commented May 8, 2023

skobyda commented May 11, 2023

martinpitt commented May 16, 2023

skobyda commented May 25, 2023 • edited Loading

Choose a reason for hiding this comment

skobyda commented Jun 23, 2023

garrett left a comment

Choose a reason for hiding this comment

skobyda commented Jun 26, 2023

martinpitt left a comment

Choose a reason for hiding this comment

garrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skobyda commented May 5, 2023 •

edited by martinpitt

Loading

skobyda commented May 25, 2023 •

edited

Loading