-
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
snapshots: Improve memory path handling #1730
snapshots: Improve memory path handling #1730
Conversation
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. |
245caff
to
e6d3102
Compare
e6d3102
to
5746ff7
Compare
7724d82
to
5cd4b06
Compare
db058b4
to
e2fdd3c
Compare
@garrett, what do you think of this? |
d2bc635
to
6d15eda
Compare
9d705dd
to
d29ac9b
Compare
Yeah. We can also omit the "RAM state location" completely and always just use a sensible default. |
Alright, let's do it! :-) |
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. |
8c18d48
to
4ec946d
Compare
Done, and updated the screenshot in the description. |
80dc5fe
to
66768e1
Compare
681addd
to
2e6a750
Compare
a623d6b
to
67fd9b7
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.
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.
- 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.
67fd9b7
to
368b9cb
Compare
} else { | ||
if (vm.connectionName === LIBVIRT_SYSTEM_CONNECTION) | ||
return "/var/lib/libvirt/memory/" + snapName; | ||
return "/var/lib/libvirt/memory"; |
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.
else if (current_user) | ||
return current_user.home + "/.local/share/libvirt/memory/" + snapName; | ||
return current_user.home + "/.local/share/libvirt/memory"; |
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.
const info = JSON.parse(output); | ||
callback(info.free * info.unit); | ||
}) | ||
.catch(exc => { |
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.
}) | ||
.catch(exc => { | ||
// channel has already logged the error | ||
callback(null); |
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.
mpath = mpath + "/"; | ||
mpath = mpath + vm.name + "." + name + ".save"; | ||
} | ||
const superuser = (vm.connectionName === LIBVIRT_SYSTEM_CONNECTION) ? "require" : false; |
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.
const validationError = this.onValidate(); | ||
|
||
const body = ( | ||
<Form onSubmit={e => e.preventDefault()} isHorizontal> | ||
<Form onSubmit={e => e.preventDefault()}> |
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.
export function dirname(path) { | ||
const i = path.lastIndexOf("/"); | ||
if (i < 0) | ||
return "."; |
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.
if (i < 0) | ||
return "."; | ||
else if (i == 0) | ||
return "/"; |
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.
Looks fairly good to me, but on ubuntu there are some test flakes? |
Yes, let's see. |
Demo: https://www.youtube.com/watch?v=wyrBSaIb93k
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.