GNOME Bugzilla – Bug 700374
wizard: Make shrinking the disk image actually work
Last modified: 2016-03-31 13:22:07 UTC
When customizing the disk size at the end of the VM creation wizard, we end up in the generic property code which uses virStoragePoolResize to change the disk size. However, this has limitations as the disk can not be shrunk, only grown. Combined with the fact that it's automatically resized after a few seconds of inactivity, this can be inconvenient to use, especially as there are no hints of resizing failures. As we are creating a new VM, we could do without resize and just recreate a new disk image of the right size. Steps to reproduce: 1) Create a new VM 2) Pick 'customize' at the last wizard step 3) move the disk slider to 60GB 4) wait a few seconds 5) move the disk slider to 40GB -> warning in the console that the disk cannot be shrunk
(In reply to comment #0) > When customizing the disk size at the end of the VM creation wizard, we end up > in the generic property code which uses virStoragePoolResize to change the disk > size. You meant to write virStorageVolResize I think. > However, this has limitations as the disk can not be shrunk, only grown. True. > Combined with the fact that it's automatically resized after a few seconds of > inactivity, this can be inconvenient to use, especially as there are no hints > of resizing failures. I don't think there is any automatic resizing, only that resizing fails when shrinking and there is no indication of that. > As we are creating a new VM, we could do without resize and just recreate a new > disk image of the right size. I think so yeah and also we should in this case not act on the size change so immediately.
(In reply to comment #1) > (In reply to comment #0) > > When customizing the disk size at the end of the VM creation wizard, we end up > > in the generic property code which uses virStoragePoolResize to change the disk > > size. > > You meant to write virStorageVolResize I think. Yup, most likely. Whatever is used in the code ;) > > Combined with the fact that it's automatically resized after a few seconds of > > inactivity, this can be inconvenient to use, especially as there are no hints > > of resizing failures. > > I don't think there is any automatic resizing, only that resizing fails when > shrinking and there is no indication of that. What I meant by 'automatic' is what happens at step 4) of the steps to reproduce, if you set it to a bigger size and wait a bit, virStoragexxxResize will get called, and then you can't shrink it back. Just checked this is the behaviour in 3.10 at least: (gnome-boxes:7033): Boxes-WARNING **: libvirt-machine-properties.vala:576: Failed to change storage capacity of volume 'debian6' to 6574819481: Unable to resize volume storage: invalid argument: Can't shrink capacity below current capacity with shrink flag explicitly specified > > > As we are creating a new VM, we could do without resize and just recreate a new > > disk image of the right size. > > I think so yeah and also we should in this case not act on the size change so > immediately. Ah, looks we agree on the current behaviour, but I confused you with my use of 'automatic'
Created attachment 271641 [details] [review] libvirt-machine-props: Lets user shrink the disk size As the disk format used by boxex (qcow2) doesn't support shrink lets delete the used storage volume and create a new one using the disk size set by the user, when the user is creating a new VM.
Review of attachment 271641 [details] [review]: Minor spelling mistake in commit log: * Lets -> Let ('Lets' is short for 'Let us') * boxex -> boxes :) Would be actually nice to not mislead w/ shortlog: libvirt-machine-props: Allow shrinking volume in wizard ::: src/libvirt-machine-properties.vala @@ +467,3 @@ + if (min_storage == 0) + min_storage = volume_info.capacity; I'm not quite understanding the role of 'min_storage' field here. Can you explain a bit? @@ +502,3 @@ if (disk != null) disk.resize (value, 0); + } else { I think you are assuming in here that this is the case of new vm customization. You need to check for that by looking at machine.under_construction. @@ +505,3 @@ + var volume_info = machine.storage_volume.get_info (); + + if (value < volume_info.capacity) { Since vm_creator does this already, I think we should ask it to do that for us in here too. Maybe it then even makes sense to simply check for vm_creator to be non-NULL (instead of 'under_construction' I recommended above: if (machine.vm_creator != null) { vm_creator.resize_volume(); } else { // existing code }
(In reply to comment #4) > ::: src/libvirt-machine-properties.vala > @@ +467,3 @@ > > + if (min_storage == 0) > + min_storage = volume_info.capacity; > > I'm not quite understanding the role of 'min_storage' field here. Can you > explain a bit? Basically keeping the first value of min disk size. If we don't do that, the min disk size always will be the volume_info.capacity and the user could not shrink the image after increase the size, leave and re-enter in the customize window.
(In reply to comment #5) > (In reply to comment #4) > > > ::: src/libvirt-machine-properties.vala > > @@ +467,3 @@ > > > > + if (min_storage == 0) > > + min_storage = volume_info.capacity; > > > > I'm not quite understanding the role of 'min_storage' field here. Can you > > explain a bit? > > Basically keeping the first value of min disk size. > If we don't do that, the min disk size always will be the volume_info.capacity > and the user could not shrink the image after increase the size, leave and > re-enter in the customize window. But what does min size mean now that we allow shrinking the size? It should be always 0 in this, no?
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > > > ::: src/libvirt-machine-properties.vala > > > @@ +467,3 @@ > > > > > > + if (min_storage == 0) > > > + min_storage = volume_info.capacity; > > > > > > I'm not quite understanding the role of 'min_storage' field here. Can you > > > explain a bit? > > > > Basically keeping the first value of min disk size. > > If we don't do that, the min disk size always will be the volume_info.capacity > > and the user could not shrink the image after increase the size, leave and > > re-enter in the customize window. > > But what does min size mean now that we allow shrinking the size? It should be > always 0 in this, no? 0 -> some sane value based on current volume size.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > (In reply to comment #4) > > > > > > > ::: src/libvirt-machine-properties.vala > > > > @@ +467,3 @@ > > > > > > > > + if (min_storage == 0) > > > > + min_storage = volume_info.capacity; > > > > > > > > I'm not quite understanding the role of 'min_storage' field here. Can you > > > > explain a bit? > > > > > > Basically keeping the first value of min disk size. > > > If we don't do that, the min disk size always will be the volume_info.capacity > > > and the user could not shrink the image after increase the size, leave and > > > re-enter in the customize window. > > > > But what does min size mean now that we allow shrinking the size? It should be > > always 0 in this, no? > > 0 -> some sane value based on current volume size. IIUC, the storage volume is created with the size: MAX (2 * minimum required, recommended size). And it is the first volume_info.capacity, no?
(In reply to comment #4) > > Since vm_creator does this already, I think we should ask it to do that for us > in here too. Are you sure that vm_creator does this already?
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > (In reply to comment #5) > > > > (In reply to comment #4) > > > > > > > > > ::: src/libvirt-machine-properties.vala > > > > > @@ +467,3 @@ > > > > > > > > > > + if (min_storage == 0) > > > > > + min_storage = volume_info.capacity; > > > > > > > > > > I'm not quite understanding the role of 'min_storage' field here. Can you > > > > > explain a bit? > > > > > > > > Basically keeping the first value of min disk size. > > > > If we don't do that, the min disk size always will be the volume_info.capacity > > > > and the user could not shrink the image after increase the size, leave and > > > > re-enter in the customize window. > > > > > > But what does min size mean now that we allow shrinking the size? It should be > > > always 0 in this, no? > > > > 0 -> some sane value based on current volume size. > > IIUC, the storage volume is created with the size: MAX (2 * minimum required, > recommended size). And it is the first volume_info.capacity, no? Yes? how is that related to min size? If we are allowing to shrink the volume, we should also allow it to go below the original assigned size.
(In reply to comment #9) > (In reply to comment #4) > > > > Since vm_creator does this already, I think we should ask it to do that for us > > in here too. > > Are you sure that vm_creator does this already? Not exactly the same thing but it sets up the volume, we'll need to add a new method that uses the existing one for the creation part.
(In reply to comment #10) > Yes? how is that related to min size? If we are allowing to shrink the volume, > we should also allow it to go below the original assigned size. Ok. I'm changing the slider values. min size: minimum required value spinner value: recommended value. Is it okay for you?
(In reply to comment #12) > (In reply to comment #10) > > > Yes? how is that related to min size? If we are allowing to shrink the volume, > > we should also allow it to go below the original assigned size. > > Ok. I'm changing the slider values. > min size: minimum required value or if that is unknown, then the default capacity > spinner value: recommended value. What? spinner's intial value should be the current volume capacity.
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #10) > > > > > Yes? how is that related to min size? If we are allowing to shrink the volume, > > > we should also allow it to go below the original assigned size. > > > > Ok. I'm changing the slider values. > > min size: minimum required value > > or if that is unknown, then the default capacity Okay. > > > spinner value: recommended value. > > What? spinner's intial value should be the current volume capacity. s/spinner/scale/ Yeah, yeah. Right, the current volume capacity.
Created attachment 271669 [details] [review] libvirt-machine-props: Allow shrinking volume in wizard As the disk format used by Boxes (qcow2) doesn't support shrink, lets delete the used storage volume and create a new one using the disk size set by the user, when the user is creating a new VM.
Review of attachment 271669 [details] [review]: Looks good otherwise, I'm relying on you testing each version btw. ::: src/libvirt-machine-properties.vala @@ +464,3 @@ var pool_info = pool.get_info (); + var min_storage = volume_info.capacity; I think this should be initialized in the 'else' part of the 'if' below for readability. @@ +467,3 @@ + + if (machine.vm_creator != null) { + if (machine.vm_creator.install_media.os != null) { * I don't think there is any need to have two 'if' in here just combine the conditions with '&&' * Since we disable the properties during install going on (i-e VM itself is already created and user is out of wizard already) we don't need to check for previous_ui_state here to be WIZARD but a small comment here saying that would be nice. @@ +471,3 @@ + var architecture = machine.domain_config.get_os ().get_arch (); + var resources = OSDatabase.get_minimum_resources_for_os (os, architecture); + if (resources != null) You want {} here as the corresponding else got them. @@ +474,3 @@ + min_storage = resources.storage; + else { + // Don't need to check if resources is null, default resources are always created statically Redundant comment. Code is more obvious than the comment. :) @@ +476,3 @@ + // Don't need to check if resources is null, default resources are always created statically + resources = OSDatabase.get_default_resources (); + min_storage = uint64.min (volume_info.capacity, resources.storage); not sure about this formula but can't suggest anything better either. Lets do it this way for now then.. @@ +517,3 @@ } + } else { + // New VM Customization This needs to be under the if. @@ +519,3 @@ + // New VM Customization + if (machine.vm_creator != null) + machine.vm_creator.resize_volume.begin (machine, value); And who is going to catch the error from this call? :) You either want to make use of '.end' here to show an error to user and throwing the details on the console in a warning or do that in VMCreator.resize_volume(). ::: src/vm-creator.vala @@ +63,3 @@ } + public async void resize_volume (LibvirtMachine machine, uint64 storage) throws GLib.Error { Since VMCreator is per-machine, I think we better have a ref to machine in VMCreator rather than passing it always to it. If you can do that in a separate patch, that would be nice. Keep in mind that we should only keep a weak ref to machine as otherwise we'll get a cyclic ref. @@ +66,3 @@ + var volume_info = machine.storage_volume.get_info (); + + if (storage < volume_info.capacity) { and we simply ignore the other case? If this method is only supposed to be called for this case: * it should be named shrink_volume * this condition should be in 'requires' line for this method. * caller should check this condition and call this method only then. @@ +71,3 @@ + + config.set_capacity (storage); + machine.storage_volume = yield create_target_volume_from_config (config); ok, this statement makes me convinced that this method shouldn't be here. Sorry, you were right I think.
(In reply to comment #16) > @@ +519,3 @@ > + // New VM Customization > + if (machine.vm_creator != null) > + machine.vm_creator.resize_volume.begin (machine, > value); > > And who is going to catch the error from this call? :) You either want to make > use of '.end' here to show an error to user and throwing the details on the > console in a warning or do that in VMCreator.resize_volume(). > > ::: src/vm-creator.vala > @@ +63,3 @@ > } > > + public async void resize_volume (LibvirtMachine machine, uint64 storage) > throws GLib.Error { > > Since VMCreator is per-machine, I think we better have a ref to machine in > VMCreator rather than passing it always to it. If you can do that in a separate > patch, that would be nice. > > Keep in mind that we should only keep a weak ref to machine as otherwise we'll > get a cyclic ref. > > @@ +66,3 @@ > + var volume_info = machine.storage_volume.get_info (); > + > + if (storage < volume_info.capacity) { > > and we simply ignore the other case? If this method is only supposed to be > called for this case: > > * it should be named shrink_volume > * this condition should be in 'requires' line for this method. > * caller should check this condition and call this method only then. > > @@ +71,3 @@ > + > + config.set_capacity (storage); > + machine.storage_volume = yield create_target_volume_from_config > (config); > > ok, this statement makes me convinced that this method shouldn't be here. > Sorry, you were right I think. So, I'll move this code to libvirt-machine-properties.vala again and ignore (part of) your comments above :-). Or can you see a better place to this code?
(In reply to comment #17) > (In reply to comment #16) > > > @@ +519,3 @@ > > + // New VM Customization > > + if (machine.vm_creator != null) > > + machine.vm_creator.resize_volume.begin (machine, > > value); > > > > And who is going to catch the error from this call? :) You either want to make > > use of '.end' here to show an error to user and throwing the details on the > > console in a warning or do that in VMCreator.resize_volume(). > > > > ::: src/vm-creator.vala > > @@ +63,3 @@ > > } > > > > + public async void resize_volume (LibvirtMachine machine, uint64 storage) > > throws GLib.Error { > > > > Since VMCreator is per-machine, I think we better have a ref to machine in > > VMCreator rather than passing it always to it. If you can do that in a separate > > patch, that would be nice. > > > > Keep in mind that we should only keep a weak ref to machine as otherwise we'll > > get a cyclic ref. > > > > @@ +66,3 @@ > > + var volume_info = machine.storage_volume.get_info (); > > + > > + if (storage < volume_info.capacity) { > > > > and we simply ignore the other case? If this method is only supposed to be > > called for this case: > > > > * it should be named shrink_volume > > * this condition should be in 'requires' line for this method. > > * caller should check this condition and call this method only then. > > > > @@ +71,3 @@ > > + > > + config.set_capacity (storage); > > + machine.storage_volume = yield create_target_volume_from_config > > (config); > > > > ok, this statement makes me convinced that this method shouldn't be here. > > Sorry, you were right I think. > > So, I'll move this code to libvirt-machine-properties.vala again and ignore > (part of) your comments above :-). Which ones? I think they still apply. > Or can you see a better place to this code? Nope. The only other place would be LibvirMachine but unless there is other users of it, I don't feel good about it being there.
Created attachment 271738 [details] [review] libvirt-machine-props: Allow shrinking volume in wizard As the disk format used by Boxes (qcow2) doesn't support shrink, lets delete the used storage volume and create a new one using the disk size set by the user, when the user is creating a new VM.
Created attachment 271739 [details] [review] libvirt-machine-props: Allow shrinking volume in wizard As the disk format used by Boxes (qcow2) doesn't support shrink, lets delete the used storage volume and create a new one using the disk size set by the user, when the user is creating a new VM.
Review of attachment 271738 [details] [review]: getting there.. ::: src/libvirt-machine-properties.vala @@ +474,3 @@ + } else { + resources = OSDatabase.get_default_resources (); + min_storage = uint64.min (volume_info.capacity, resources.storage); I only spot it now that this doesn't require the os to be known (non-null) so we should be doing that also for the case of os being null. So sorry but that means you'll have to revert back to putting 'os != null' in a separate 'if' again. :( Sorry for for being blind in my previous review but this change itself would still have been needed. @@ +524,3 @@ + + var pool = get_storage_pool (machine.connection); + machine.storage_volume = pool.create_volume (config); You completely ditched the new method. :( We agred to move it in this module, not inline it.
Review of attachment 271739 [details] [review]: Last review still applies
Created attachment 271742 [details] [review] libvirt-machine-props: Allow shrinking volume in wizard As the disk format used by Boxes (qcow2) doesn't support shrink, lets delete the used storage volume and create a new one using the disk size set by the user, when the user is creating a new VM.
Review of attachment 271742 [details] [review]: almost there :) ::: src/libvirt-machine-properties.vala @@ +530,3 @@ + private uint64 get_minimum_disk_size () throws GLib.Error { + // Since we disable the properties during install going on we don't need to check for + // previous_ui_state_here to be WIZARD. This comment is not valid for for the whole function but rather in 'machine.vm_creator == null' case only so please move it under that if. @@ +537,3 @@ + var default_resources = OSDatabase.get_default_resources (); + if (machine.vm_creator.install_media.os == null) + return uint64.min (volume_info.capacity, default_resources.storage); we do the same below so i think it would be cleaner to do in one place. How about this: Osinfo.Resources miminum_resources = null; if (machine.vm_creator.install_media.os != null) { var os = machine.vm_creator.install_media.os; var architecture = machine.domain_config.get_os ().get_arch (); var minimum_resources = OSDatabase.get_minimum_resources_for_os (os, architecture); } if (minimum_resources != null) return minimum_resources.storage; else return uint64.min (volume_info.capacity, default_resources.storage); @@ +548,3 @@ + } + + private void resize_storage (uint64 storage) throws GLib.Error { * I'd call it resize_storage_volume * Also storage -> size, would be better IMHO
Created attachment 271775 [details] [review] libvirt-machine-props: Allow shrinking volume in wizard As the disk format used by Boxes (qcow2) doesn't support shrink, lets delete the used storage volume and create a new one using the disk size set by the user, when the user is creating a new VM.
Review of attachment 271775 [details] [review]: ACK with the typo pointed out fixed along with these fixes to commit log: * shrink -> shrinking * "lets delete" -> "lets implement it by deleting" and "create" -> creating" etc. * used -> existing Assuming that: * You rebased this just before uploading here * You tested this patch for the case this patch is about and other cases too. ::: src/libvirt-machine-properties.vala @@ +532,3 @@ + if (machine.vm_creator == null) { + // Since we disable the properties during install going on we don't need to check for + // previous_ui_state_here to be WIZARD. previous_ui_state_here -> previous_ui_state here :)
Created attachment 271806 [details] [review] libvirt-machine-props: Allow shrinking volume in wizard As the disk format used by Boxes (qcow2) doesn't support shrinking, lets implement it by deleting the existing storage volume and creating a new one using the disk size set by the user, when the user is creating a new VM.
Attachment 271806 [details] pushed as a6aecae (gnome-boxes 3.11.92+)