GNOME Bugzilla – Bug 756140
Resizing hard disk is broken if VM has snapshots
Last modified: 2018-01-11 10:27:52 UTC
After resizing my VM's hard disk, when I reopen the Properties dialog the setting has not been saved: it's back to the original size. The problem is this: Boxes-WARNING **: libvirt-machine-properties.vala:594: Failed to change storage capacity of volume 'boxes-unknown' to 40149525456: Unable to resize volume storage: internal error: Child process (/usr/bin/qemu-img resize /home/mcatanzaro/.local/share/gnome-boxes/images/boxes-unknown 40149525504) unexpected exit status 1: qemu-img: Can't resize an image which has snapshots qemu-img: This image does not support resize Deleting my snapshot makes resizing work.
I guess the solution would be to: 1. Delete the snapshots before resizing. 2. Inform the user with an infobar, that resizing will result in snapshots being deleted. However, we can't do that for 3.18 as it breaks UI/string freeze. Any ideas on how to handle this in 3.18? And no, we can't just not show resize slider, cause users will get really confused if a UI disappears without any clue of why.
I think you can either get a freeze exception or else just leave in broken in 3.18. Fixing it in 3.18 would be nice, but most important is to make sure it's fixed going forward....
And now we'll already need a 3.20 freeze exception...
Of course being able to resize without throwing away snapshots would be the right way™ forward. But if technically not feasible, we should notify the action will throw away the snapshots. Having a modal "are you sure" dialog might be the most straight forward approach, but people habituate to click through and then realize they didn't actually mean to throw away the snapshots. A better approach would be to do the size change and throw away the snapshots, showing a "Box has been resized, pruning all snapshots. [undo]" in-app notification.
File a bug on Qemu about this issue, let's see if its technically possible to fix the actual issue: https://bugs.launchpad.net/qemu/+bug/1563931
Created attachment 325193 [details] [review] libvirt-machine-props: Allow resize of disk w/ snapshots Since it is currently not possible to resize a disk image if it has snapshots on it, resize operation ends up not working with a warning on the console. Let's fix that by deleting all associated snapshots before attempting to resize the disk image but not before notifying user about what we are about to do.
I'm not sure of the wording in the notification I used: 1 snapshot: "Storage resize requires deleting associated snapshot." multiple snapshots: "Storage resize requires deleting %llu associated snapshots." Better alternatives? Also it's a be weird that we give the impression that disk has been resized already even though it's not yet been resized cause snapshots has to be deleted first and we can't just delete them and then bring them back if user clicks "Undo". So that's another reason I didn't push this yet.
Yeah, that's one issue.... I don't think an undo notification is a good solution for this. It's not like a delete button where the user knows the option is destructive. Here, the user has no expectation that the resize will delete snapshots; the first clue will be the notification that snapshots have been deleted (even if they haven't been yet). I almost think a pop-up confirmation dialog would make sense in this case. Maybe we could make the undo notification work by adding an explanatory label under the resize slider, warning that resizing will delete your snapshots; then I think the undo notification would be sufficien. We probably should also add a label to make clear that the action is irreversible; i.e. you can only resize up, never down. That's currently confusing.
In retrospect I agree learning about the behavior only after the destructive behavior has occurred is bad. Zeenix suggested a less intrusive infobar for this, but as the resizing is instant apply, it would have to have the equivalent of "delete snapshots and resize" action button. It would essentially act like a modal dialog you can choose to ignore. It's odd [1]. Maybe a modal is still the best option we have. [1] Odd - https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/boxes/wires/odd-resizing-inline-modal.png - don't do this ;)
Keep in mind: not only the act of deleting snapshots is destructive, but also the act of resizing, as it is impossible to size down the VM once the size has been increased. If that slider is really instant-apply, then I am confused as to how the slider could possibly work properly if it's slid to the right and then back left.
No guys, the slider is NOT instantly applied. :) It only applies after properties window is closed.
*** Bug 766690 has been marked as a duplicate of this bug. ***
(In reply to Michael Catanzaro from comment #10) > Keep in mind: not only the act of deleting snapshots is destructive, but > also the act of resizing, as it is impossible to size down the VM once the > size has been increased. If that slider is really instant-apply, then I am > confused as to how the slider could possibly work properly if it's slid to > the right and then back left. Would it be possible to see where is the last partition in MBR or GPT? Any space afterwards should be free and resizable.
OK, I've discovered this bug is exacerbated quite badly by the fact that Boxes now takes an initial "Just installed" snapshot when creating a VM. So... yeah. (In reply to Zeeshan Ali (Khattak) from comment #6) > Created attachment 325193 [details] [review] [review] > libvirt-machine-props: Allow resize of disk w/ snapshots I think I would push this patch. Even though it's a weird restriction, warning the user is a lot better than silently failing to perform the resize operation.
Also I noticed today that after deleting a snapshot, Boxes tries to delete it again and again forever, once every five seconds or so. Each time, it prints this error message: (gnome-boxes:17794): Boxes-WARNING **: snapshot-list-row.vala:188: Error while deleting snapshot 2016-09-22-23-42-04: Unable to delete snapshot `2016-09-22-23-42-04' I think the first attempt to delete the snapshot succeeded (since I was able to resize ;) and the subsequent attempts are all failing because it's already deleted.
Review of attachment 325193 [details] [review]: (In reply to Michael Catanzaro from comment #14) > OK, I've discovered this bug is exacerbated quite badly by the fact that > Boxes now takes an initial "Just installed" snapshot when creating a VM. > So... yeah. > > (In reply to Zeeshan Ali (Khattak) from comment #6) > > Created attachment 325193 [details] [review] [review] [review] > > libvirt-machine-props: Allow resize of disk w/ snapshots > > I think I would push this patch. Even though it's a weird restriction, > warning the user is a lot better than silently failing to perform the resize > operation. +1 ::: src/libvirt-machine-properties.vala @@ +629,3 @@ + }; + + var msg = ngettext ("Storage resize requires deleting associated snapshot.", What if instead of "Undo" we would go for something like "Restore Snapshots"?
Anyway, I would land this for now and we can revisit the feature later. (In reply to Michael Catanzaro from comment #15) > Also I noticed today that after deleting a snapshot, Boxes tries to delete > it again and again forever, once every five seconds or so. Each time, it > prints this error message: > > (gnome-boxes:17794): Boxes-WARNING **: snapshot-list-row.vala:188: Error > while deleting snapshot 2016-09-22-23-42-04: Unable to delete snapshot > `2016-09-22-23-42-04' > > I think the first attempt to delete the snapshot succeeded (since I was able > to resize ;) and the subsequent attempts are all failing because it's > already deleted. Which gnome-boxes --version do you have? Could you please file another bug for this one?
Comment on attachment 325193 [details] [review] libvirt-machine-props: Allow resize of disk w/ snapshots Attachment 325193 [details] pushed as 1c61d1b - libvirt-machine-props: Allow resize of disk w/ snapshots Attachment 325193 [details] pushed as 458fc08f - libvirt-machine-props: Allow resize of disk w/ snapshots
Review of attachment 325193 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +618,3 @@ + var msg = ngettext ("Storage resize requires deleting associated snapshot.", + "Storage resize requires deleting %llu associated snapshots.", + num_snapshots).printf (num_snapshots); Please be aware that this will not work as you think it will. First, there are languages (Chinese, Japanese, Korean, and probably more) which don't have a plural number. Translators have a choice of only one form. This means that "Storage resize requires deleting %llu associated snapshots" will be always displayed, even if the number is 1. Second, even worse, some languages (Russian, Serbian, and probably more) reuse the singular form if the number is 21, 31, 41…. So if there are 21 snapshots to delete it will say there is a single snapshot to delete. If you want to achieve the correct result you must make it explicit: if (num_snapshots == 1) msg = "Storage resize requires deleting associated snapshot." else msg = ngettext ("Storage resize requires deleting %llu associated snapshot.", "Storage resize requires deleting %llu associated snapshots." num_snapshots).printf (num_snapshots); While at this, I can see the same problem with "_Open in new window"/"_Open in %u new windows". One more nitpick: shouldn't it be "Storage resize requires deleting *an* associated snapshot"? (Native English speakers, please help.)
(In reply to Rafal Luzynski from comment #19) > Review of attachment 325193 [details] [review] [review]: > > ::: src/libvirt-machine-properties.vala > @@ +618,3 @@ > + var msg = ngettext ("Storage resize requires deleting > associated snapshot.", > + "Storage resize requires deleting %llu > associated snapshots.", > + num_snapshots).printf (num_snapshots); > > Please be aware that this will not work as you think it will. Also, because Vala is wonderful, that's going to compile to a g_strdup_printf() followed by ngettext(), not vice versa as we desire, so it won't be translated at all. The workaround is to call ngettext() first, store it into a local variable, and then call printf() on the next line. See bug #788387. > One more nitpick: shouldn't it be "Storage resize requires deleting *an* > associated snapshot"? (Native English speakers, please help.) Yes, either "an associated snapshot" or "one associated snapshot"
My understanding is that it really does not matter how many snapshots are there as long as there is One which would prevent the resizing. So I would suggest a single sentence such as "Storage resizing requires deleting the associated snapshots." Thoughts?
(In reply to Rafal Luzynski from comment #19) > While at this, I can see the same problem with "_Open in new window"/"_Open > in %u new windows". In Czech there are three different inflections: - for the single case = okno - for [2, 3, 4] = okna - quantities greater than 5 = oken So for the plural, if any, we could say "Open in multiple windows". What do you think?
Is there a wiki page or something of this sort where we could list these particularities and promote awareness about it for the whole gnome community?
Felipe: https://wiki.gnome.org/TranslationProject/DevGuidelines
(In reply to Felipe Borges from comment #22) > [...] > In Czech there are three different inflections: > - for the single case = okno > - for [2, 3, 4] = okna > - quantities greater than 5 = oken This is handled fine by gettext, you should not deal with this manually. My only point is that you should not assume that: - all languages have singular and plural numbers, - a value equal to 1 uses a singular number and no other value uses a singular number. > So for the plural, if any, we could say "Open in multiple windows". What do > you think? No, I don't think we should change the design in order to workaround bugs. Instead we should fix bugs. But, taking into account what Michael said in comment 20 this time it may be difficult. (In reply to André Klapper from comment #24) Good link, André. Also, more: http://www.unicode.org/cldr/charts/latest/supplemental/language_plural_rules.html https://en.wikipedia.org/wiki/Grammatical_number https://github.com/abrt/gnome-abrt/issues/112#issuecomment-152846843
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-boxes/issues/72.