snapshots: Improve memory path handling #1730

mvollmer · 2024-07-18T13:49:46Z

Demo: https://www.youtube.com/watch?v=wyrBSaIb93k

We only ask for the directory of the memory snapshot, not for the full filename. That way we don't have to worry about what to do when the user changes the name and it makes more sense when we talk about how much space is available "at this location". Also, people don't have to worry about the filenames for disk snapshots, why would they worry about them for the memory part?
The default for the memory save location is also taken from previous snapshots. That way, people only have to change it for the first snapshot when they are not happy with putting it next to the primary disk.
The snapshot name does not need to include the VM name. Snapshots are already scoped to VMs.
The memory save file gets the extension ".save", to mirror what "virsh managedsave" is doing.

Machines: Improvements to snapshots of running machines

The snapshot of a running machine includes its memory state, and Cockpit sometimes asks where to store it. This is now done in a slightly improved fashion: you only need to specify the directory, and Cockpit will provide the name of the file. The default for this directory now also takes previous snapshots into account, so that you don't need to touch it if you always store the memory state of all snapshots in the same directory. Plus, Cockpit now gives estimates for the amount of space that will be needed and warns you if there doesn't seem to be enough.

mvollmer · 2024-07-18T13:50:51Z

Previous version of the dialog:

mvollmer · 2024-07-18T13:55:28Z

It's not yet clear to me how to compute the estimate for the maximum size of the memory save file.

In my experiments, the file size corresponds pretty closely with the memory usage shown by Cockpit. A snapshot taken while compiling the kernel consistently used 2 GB on disk, for a VM with 2 GB RAM. A snapshot of a freshly booted machine that showed 500 MB RAM usage (out of 2 GB) in Cockpit used 450 MB for the memory snapshot.

So I guess we can use the current memory usage as the estimate. Or to be conservative, we can use the total RAM.

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx

mvollmer · 2024-07-30T12:03:34Z

@garrett, what do you think of this?

mvollmer · 2024-09-05T05:43:11Z

Generally, it's better to prefer non-jargon text when possible and to avoid exposing implementation details.

Yeah. We can also omit the "RAM state location" completely and always just use a sensible default.

mvollmer · 2024-09-05T05:44:37Z

I'm asking for us to start adopting top labelling for modals.

Alright, let's do it! :-)

mvollmer · 2024-09-05T06:19:54Z

libvirt calls these files "RAM state" too, according to https://wiki.libvirt.org/Snapshots.html
[...]
if we did want to say something like "Memory state directory".

Here is the high-level description of the various snapshotting things that libvirt can do, and it doesn't mention "RAM", only "memory", and more specifically calls it "guest memory".

Cockpit only uses "memory" in its UI and not "RAM".

So we should stick with "memory".

"Guest memory state file directory" or "Guest memory state file location" would be most descriptive (if a bit ridiculous) and with top labels we have the space.

I'll change it to "Memory state file location", and change this dialog to use top-labels. All others are done in #1804.

mvollmer · 2024-09-05T06:33:36Z

I'll change it to "Memory state file location", and change this dialog to use top-labels. All others are done in #1804.

Done, and updated the screenshot in the description.

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx

src/helpers.js

garrett

I think this PR is fine to merge.

But we should consider if we want to move to top-labelling overall and have side labelling be the exception, or have top-labelling be the exception. Naturally, things should be internally consistent (that is: the same in the same context; for example: this modal should have all labels be the same way, like it does).

Considering PatternFly uses top labeling by default, that's another argument aside from strings, to use that.

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx

test/check-machines-snapshots

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx

- We only ask for the directory of the memory snapshot, not for the full filename. - The default for the memory save location is taken from previous snapshots. - The default snapshot name does not include the VM name. - Available and needed storage space is shown in the dialog. - New directories can be created from the dialog. - The memory save file gets the extension ".save", to mirror what "virsh managedsave" is doing.

cockpituous · 2024-09-11T11:34:30Z

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx

    } else {
        if (vm.connectionName === LIBVIRT_SYSTEM_CONNECTION)
-            return "/var/lib/libvirt/memory/" + snapName;
+            return "/var/lib/libvirt/memory";


This added line is not executed by any test.

cockpituous · 2024-09-11T11:34:30Z

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx

        else if (current_user)
-            return current_user.home + "/.local/share/libvirt/memory/" + snapName;
+            return current_user.home + "/.local/share/libvirt/memory";


This added line is not executed by any test.

cockpituous · 2024-09-11T11:34:30Z

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx

+                const info = JSON.parse(output);
+                callback(info.free * info.unit);
+            })
+            .catch(exc => {


This added line is not executed by any test.

cockpituous · 2024-09-11T11:34:30Z

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx

+            })
+            .catch(exc => {
+                // channel has already logged the error
+                callback(null);


This added line is not executed by any test.

cockpituous · 2024-09-11T11:34:30Z

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx

+                    mpath = mpath + "/";
+                mpath = mpath + vm.name + "." + name + ".save";
+            }
+            const superuser = (vm.connectionName === LIBVIRT_SYSTEM_CONNECTION) ? "require" : false;


This added line is not executed by any test.

cockpituous · 2024-09-11T11:34:31Z

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx

        const validationError = this.onValidate();

        const body = (
-            <Form onSubmit={e => e.preventDefault()} isHorizontal>
+            <Form onSubmit={e => e.preventDefault()}>


This added line is not executed by any test.

cockpituous · 2024-09-11T11:34:31Z

src/helpers.js

+export function dirname(path) {
+    const i = path.lastIndexOf("/");
+    if (i < 0)
+        return ".";


This added line is not executed by any test.

cockpituous · 2024-09-11T11:34:31Z

src/helpers.js

+    if (i < 0)
+        return ".";
+    else if (i == 0)
+        return "/";


This added line is not executed by any test.

test/machineslib.py

jelly · 2024-09-11T11:57:59Z

Looks fairly good to me, but on ubuntu there are some test flakes?

mvollmer · 2024-09-11T12:06:03Z

Looks fairly good to me, but on ubuntu there are some test flakes?

Yes, let's see.

mvollmer added the no-test label Jul 18, 2024

mvollmer force-pushed the snapshots-memorypath-polish branch from 245caff to e6d3102 Compare July 19, 2024 07:34

github-advanced-security bot found potential problems Jul 19, 2024

View reviewed changes

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx Fixed Show fixed Hide fixed

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx Fixed Show fixed Hide fixed

mvollmer requested a review from garrett July 19, 2024 08:08

mvollmer mentioned this pull request Jul 19, 2024

lib: Add onlyDirectories option to FileAutoComplete cockpit-project/cockpit#20777

Merged

mvollmer force-pushed the snapshots-memorypath-polish branch from e6d3102 to 5746ff7 Compare July 19, 2024 08:43

mvollmer added the blocked label Jul 19, 2024

mvollmer force-pushed the snapshots-memorypath-polish branch 2 times, most recently from 7724d82 to 5cd4b06 Compare July 19, 2024 12:17

mvollmer removed no-test blocked labels Jul 19, 2024

mvollmer marked this pull request as ready for review July 19, 2024 12:18

mvollmer force-pushed the snapshots-memorypath-polish branch 3 times, most recently from db058b4 to e2fdd3c Compare July 19, 2024 13:34

mvollmer added no-test and removed no-test labels Jul 30, 2024

mvollmer force-pushed the snapshots-memorypath-polish branch 6 times, most recently from d2bc635 to 6d15eda Compare July 31, 2024 07:52

mvollmer force-pushed the snapshots-memorypath-polish branch 2 times, most recently from 9d705dd to d29ac9b Compare August 29, 2024 08:43

mvollmer requested a review from martinpitt August 29, 2024 09:14

martinpitt added the release-note label Sep 2, 2024

mvollmer dismissed martinpitt’s stale review via 8c18d48 September 5, 2024 06:21

mvollmer force-pushed the snapshots-memorypath-polish branch 2 times, most recently from 8c18d48 to 4ec946d Compare September 5, 2024 06:31

mvollmer force-pushed the snapshots-memorypath-polish branch 3 times, most recently from 80dc5fe to 66768e1 Compare September 9, 2024 11:41

mvollmer requested a review from jelly September 10, 2024 07:07

mvollmer force-pushed the snapshots-memorypath-polish branch 3 times, most recently from 681addd to 2e6a750 Compare September 10, 2024 08:52

github-advanced-security bot found potential problems Sep 10, 2024

View reviewed changes

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx Fixed Show fixed Hide fixed

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx Fixed Show fixed Hide fixed

src/helpers.js Fixed Show fixed Hide fixed

mvollmer force-pushed the snapshots-memorypath-polish branch 2 times, most recently from a623d6b to 67fd9b7 Compare September 10, 2024 10:23

garrett previously approved these changes Sep 10, 2024

View reviewed changes

jelly reviewed Sep 11, 2024

View reviewed changes

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx Outdated Show resolved Hide resolved

test/check-machines-snapshots Show resolved Hide resolved

src/components/vm/snapshots/vmSnapshotsCreateModal.jsx Show resolved Hide resolved

mvollmer dismissed garrett’s stale review via 368b9cb September 11, 2024 11:14

mvollmer force-pushed the snapshots-memorypath-polish branch from 67fd9b7 to 368b9cb Compare September 11, 2024 11:14

mvollmer requested a review from jelly September 11, 2024 11:21

cockpituous reviewed Sep 11, 2024

View reviewed changes

jelly reviewed Sep 11, 2024

View reviewed changes

test/machineslib.py Show resolved Hide resolved

jelly approved these changes Sep 11, 2024

View reviewed changes

jelly merged commit e1d3bb8 into cockpit-project:main Sep 12, 2024
27 checks passed

martinpitt removed the release-note label Sep 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snapshots: Improve memory path handling #1730

snapshots: Improve memory path handling #1730

mvollmer commented Jul 18, 2024 •

edited by martinpitt

Loading

mvollmer commented Jul 18, 2024

mvollmer commented Jul 18, 2024

mvollmer commented Jul 30, 2024

mvollmer commented Sep 5, 2024

mvollmer commented Sep 5, 2024

mvollmer commented Sep 5, 2024

mvollmer commented Sep 5, 2024

garrett left a comment

cockpituous Sep 11, 2024

cockpituous Sep 11, 2024

cockpituous Sep 11, 2024

cockpituous Sep 11, 2024

cockpituous Sep 11, 2024

cockpituous Sep 11, 2024

cockpituous Sep 11, 2024

cockpituous Sep 11, 2024

jelly commented Sep 11, 2024

mvollmer commented Sep 11, 2024

snapshots: Improve memory path handling #1730

snapshots: Improve memory path handling #1730

Conversation

mvollmer commented Jul 18, 2024 • edited by martinpitt Loading

Machines: Improvements to snapshots of running machines

mvollmer commented Jul 18, 2024

mvollmer commented Jul 18, 2024

mvollmer commented Jul 30, 2024

mvollmer commented Sep 5, 2024

mvollmer commented Sep 5, 2024

mvollmer commented Sep 5, 2024

mvollmer commented Sep 5, 2024

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

jelly commented Sep 11, 2024

mvollmer commented Sep 11, 2024

mvollmer commented Jul 18, 2024 •

edited by martinpitt

Loading