snapshots: Improve memory path handling

mvollmer commented 1 month ago

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.

--

[x] get the actual available space
[x] warn when available space is not enough
[x] only show directories in autocomplete
[x] https://github.com/cockpit-project/cockpit/pull/20777
[x] allow creation of new directories
[x] use total ram as estimate when rssMemory is not available
[x] tests
[x] coverage

mvollmer commented 1 month ago

Previous version of the dialog:

mvollmer commented 1 month ago

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.

mvollmer commented 1 month ago

@garrett, what do you think of this?

garrett commented 3 days ago

I'm trying to figure out the context of this. I think we talked about this before, but I can't find it. I don't know what this was like before compared to now, either. Trying to figure it out now, but it'll take some time.

garrett commented 3 days ago

OK, so the current UI is this?

Where we have a bogus path to a file that doesn't work by default and is so long you can't see where it is?

garrett commented 3 days ago

OK, now I've switched branches, did a git clean, and make devel-install, and I get this:

It's better, as I can actually see the location. But it still fails by default, which isn't good. I don't know why it's failing either, or what to do.

Why does it complain about this path not existing?

And shouldn't it be something like "Snapshot path" or "Snapshot directory" instead of "Memory save location"?

For reference:

garrett@drought ~>  ls /var/home/garrett/.local/share/libvirt/memory
"/var/home/garrett/.local/share/libvirt/memory": No such file or directory (os error 2)
garrett@drought ~ [2]>  ls /var/home/garrett/.local/share/libvirt/
"/var/home/garrett/.local/share/libvirt/": No such file or directory (os error 2)

This is with user VMs of course (as you can see by the path).

garrett commented 3 days ago

System VMs seem better, though, as they at least have a working path:

mvollmer commented 3 days ago

But it still fails by default, which isn't good. I don't know why it's failing either, or what to do.

These are bugs in our FileAutocomplete component. There is usually some way of getting around them with creative clicking around.

It's not a regression in this PR, so we shouldn't block on it.

mvollmer commented 3 days ago

And shouldn't it be something like "Snapshot path" or "Snapshot directory" instead of "Memory save location"?

You tell us, but note that this is not the location for disk snapshots, only for the saved RAM.

garrett commented 3 days ago

These are bugs in our FileAutocomplete component. There is usually some way of getting around them with creative clicking around.

It's not a regression in this PR, so we shouldn't block on it.

No, that's not a bug in the autocomplete. The path does not exist locally, but it should create it if that's what's specified. This is the default path for user session machines, which is what my point is, not the autocomplete.

garrett commented 3 days ago

OK, that was a local VM problem. Disks were removed somehow, perhaps resetting VM states before vacation for testing some issue with VMs for Anaconda test machines or something. Apologies!

garrett commented 3 days ago

Instead of:

117 GB available at this location. About 1.09 GB are needed to save the memory state of the machine.

What about this?

Each memory snapshot will use around 1 GB. Total space available: 117 GB.

Or, better yet, as disks will also take up space, we should split this out from the dropdown and have it below, saying how much disks and how much ram will be used.

Snapshot will use 95 GB. Total space available: 117 GB.

I did have this, but I think it's too granular, as who cares how the disk and memory is split? It's all going to be on the disk one way or another.

~Snapshot will use 95 GB (95 GB disk + 1 GB memory). Total space available: 117 GB.~

I guess if the memory is stored somewhere other than the same storage device, it might matter more? But then, you're asking for problems. And if they're stored elsewhere, then we probably care about the space that will be used and the free space. And it matters when it's low or not enough, mostly. Otherwise, it's a footnote that doesn't matter too much, right? Although you'd probably want to know how much space each snapshot costs you?

mvollmer commented 3 days ago

saying how much disks and how much ram will be used.

Disk snapshots are implemented via a "copy on write" scheme. A snapshot of a disk starts out needing practically zero bytes, and then grows over time as the disk content at the time of taking the snapshot and the current content diverges.

Thus, disk snapshots don't take any appreciable amount of space at the time of creating the snapshot.

garrett commented 3 days ago

Thus, disk snapshots don't take any appreciable amount of space at the time of creating the snapshot.

Right, but this modal isn't for the first time only, and people update the software on their VMs... sooooo...

mvollmer commented 3 days ago

Thus, disk snapshots don't take any appreciable amount of space at the time of creating the snapshot.

Right, but this modal isn't for the first time only, and people update the software on their VMs... sooooo...

Sooooo... you want Cockpit to show the current sizes of snapshots in the list of snapshots?

I think we can do that as a total, like "XXX GB disk space used for snapshots of this machine", but not for individual snapshots. I think it's hard to predict how much space you get back when deleting a snapshot, for example.

garrett commented 2 days ago

Sooooo... you want Cockpit to show the current sizes of snapshots in the list of snapshots?

I think we can do that as a total, like "XXX GB disk space used for snapshots of this machine", but not for individual snapshots. I think it's hard to predict how much space you get back when deleting a snapshot, for example.

Oh, that's not what I was talking about, but that'd be neat and useful.

I was saying that each snapshot takes a certain size, and we need to check that there's space for it on disk, so we should also show it too. (You're already doing this somewhat.) But that snapshots also take disk space, so we should show the total size.

I guess what you're saying is that whatever happens on disk happens transparently though, when things diverge by deleting or changing things after a snapshot, the disk grows? But it doesn't grow at the time of the snapshot? And that the only thing that gets larger at the time of the snapshot would be the storage of RAM? So, effectively, at the time of snapshotting it would be always be 0 GB disk + whatever the RAM usage is?

garrett commented 2 days ago

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

However, if we are going to do this bonkers thing to let people shoot themselves in the foot by making snapshots with mandatory backing files in completely different locations, and we have very limited space due to Cockpit having left-side labeling and PatternFly still not having that flexible (so it forces words to wrap or causes big gaps, depending on the form), then I guess we might as well say "RAM state" or "RAM file"... instead of memory? libvirt calls these files "RAM state" too, according to https://wiki.libvirt.org/Snapshots.html

(Note: PF uses top labelling by default. We're the outliers. GNOME uses a mix, with labels on top for section groups and on the left for individual widgets.)

I'm not done yet, but I started to work on a mockup for this... you can already see how we are so space constrained with left side labelling in PatternFly, otherwise it'd wrap if we did want to say something like "Memory state directory". You can see the difference if we had top labelling (at least in places), like this:

And here's if we wanted longer text, it'd be something more like this:

...and some languages (I'm specifically thinking of German, but it's not the only one) are often longer than English, so we'd have the wrapping or too-long issues even when things are translated.

For example, here's a humorously bad German translation for that already long string :wink: :

(Yes, yet again, I'm asking for us to start adopting top labelling for modals. We should finally just do it, and port over as many as it makes sense. It wouldn't be horrible to mix and match where appropriate, but we should try to have most similar. Some modals probably would work better with side labelling versus top, but many work better with top labelling. The alternative would be to adopt subgrid (or use display: contents and adjust the grid) to fix PF's static widths for side labelling.)

garrett commented 2 days ago

Previous post was basically me saying that our approach to labelling is really cramping what we can do with respect to friendly labelling of fields, with examples.

We can try to find something short, but we'll probably have to sacrifice the best option for the text and ignore the translations... or decide to use top labelling (and make an effort to switch Cockpit over in other places, where we can, outside of this PR naturally).

mvollmer commented 2 days ago

So, effectively, at the time of snapshotting it would be always be 0 GB disk + whatever the RAM usage is?

Yes.

mvollmer commented 2 days ago

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 commented 2 days ago

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

Alright, let's do it! :-)

mvollmer commented 2 days ago

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 commented 2 days ago

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.

cockpit-project / cockpit-machines

snapshots: Improve memory path handling #1730