-
Notifications
You must be signed in to change notification settings - Fork 65
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: Allow disk-only snapshots of running machines #1560
Conversation
mvollmer
commented
Apr 16, 2024
•
edited
edited
- Use "Full system" instead of "Complete". That's the libvirt terminology.
- Detect guest agent and add "quiesce" flag is found.
- Test quiesce functionality.... Maybe switch from cirros to alpine, which has qemu-guest-agent
I wasn't able to revert to a disk-only snapshot: Probably tracked by https://issues.redhat.com/browse/RHEL-21549 and fixed by https://www.mail-archive.com/devel@lists.libvirt.org/msg01703.html, but not yet in rhel-9-4. Should this feature here go to rhel-9-4 or be put on hold until rhel-9-5? |
Alright, current "main" goes to rhel-9-5 already, this is good to go. |
I should've used the term "disk-only" instead of "partial" in the code, but I figured out too late that that's what libvirt calls them. |
c072368
to
f6bfe0c
Compare
Fixed. |
This is blocked on https://issues.redhat.com/browse/RHEL-21549 Cockpit should only offer the creation of disk-only external snapshots when they can also be restored. We might have to detect that at run-time, maybe by looking at the libvirt version. |
f6bfe0c
to
8f8bf4a
Compare
For the record: I will never approve this, this is a dangerous and crazy practice to offer and train admins to do. I won't make a fuss if someone else approves, but I refuse any responsibility for this, sorry. |
8f8bf4a
to
bf4b583
Compare
e04d1a0
to
00c0e93
Compare
Hmm, reverting disk-only snapshots might not be that important. Maybe they are only used as a mechanism to get a stable view of disks for the purposes of backup. If that is the case, I would argue that Cockpit doesn't need to offer disk-only snapshots. Completing the backup requires a deep understanding of how external snapshots are implemented and this is best done on the command line or from a program. |
What Cockpit could do is to expose the snapshotted disks as NBD devices so that people can then copy them to their backups. |
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 is very dangerous, both for stateful live snapshots (with state and memory) and disk-only, as it breaks expectations, which means it'll break someone's backups, which means someone using Cockpit will eventually lose their data.
We need to talk about snapshots holistically (the complete picture, including internal, external disk-only, and external live snapshots and the supportability and purpose of each) and work on designs that might help to mitigate the very real and likely risk for catastrophic data loss due to external snapshots.
00c0e93
to
32aca3d
Compare
<p>{_("This type of snapshot only saves the storage devices of the virtual machine, and not the state of running processes.")}</p> | ||
</FlexItem> | ||
</Flex>}> | ||
<button onClick={e => e.preventDefault()} className="pf-v5-c-form__group-label-help"> |
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.
<Split hasGutter> | ||
<Radio isChecked={!isDiskOnly} | ||
id="snapshot-create-dialog-type-full" | ||
onChange={() => onValueChanged("isDiskOnly", 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.
if (new_vm.connectionName == vm.connectionName && new_vm.id == vm.id) | ||
return new_vm; | ||
} | ||
return 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.
This is dead for now. No disk-only snapshots of running VMs in Cockpit, see https://issues.redhat.com/browse/COCKPIT-1112 |