GNOME Bugzilla – Bug 672554
Allow tweaking VM props before creation
Last modified: 2016-03-31 14:01:46 UTC
We should allow user to tweak storage and ram allocations before VM is created, especially since storage can't be reduced and changing of RAM requires a restart of guest.
*** Bug 675357 has been marked as a duplicate of this bug. ***
https://github.com/gnome-design-team/gnome-mockups/raw/master/boxes/boxes-install5.png and https://github.com/gnome-design-team/gnome-mockups/raw/master/boxes/boxes-install5.5.png show the design for this, removing the ui-review keyword.
Created attachment 222885 [details] [review] collection-view: Set current UI state on new items Don't assume we are in collection state when new items are added.
Created attachment 222886 [details] [review] media-manager: Add missing namespace This code is supposed to catch the generic GLib.Error but was catching only Boxes.error due to lack of namespace specification.
Created attachment 222887 [details] [review] Allow customization of box in creation wizard This patch adds a 'Customize..." button at the review page, as shown in the design mockup[1], that takes users to properties view to allow them to configure various properties before launching the box. The properties view should be showing 'media-optical' theme icon like in the mockup[2] but we can add that later. Current priority is to have this feature working before 3.6 release. Known issue: If you configure the spice props in wizard, you'll get warnings like these on hitting 'Create' button: (gnome-boxes:8748): GSpice-CRITICAL **: spice_main_set_display: assertion `channel != NULL' failed (gnome-boxes:8748): GSpice-WARNING **: Warning no automount-inhibiting implementation available [1] https://github.com/gnome-design-team/gnome-mockups/raw/master/boxes/boxes-install5.png [2] https://github.com/gnome-design-team/gnome-mockups/raw/master/boxes/boxes-install5.5.png
Review of attachment 222885 [details] [review]: ack
Review of attachment 222886 [details] [review]: ack
Review of attachment 222887 [details] [review]: looks good, although the "review" page is not updated when changing properties value.
Created attachment 223016 [details] [review] libvirt-machine: Keep associated storage volume around So that it can be just accessed directly instead of every piece of code having to search it in storage pool all the time.
Created attachment 223017 [details] [review] wizard: Show allocated resources at 'review' page We were showing resources dictated by installer media, which until now has meant the same thing but it wont after we allow users to modify resource allocation in wizard.
Created attachment 223018 [details] [review] Rename Property.changes_pending to reboot_required Name was rather misleading and will be even more so after I introduce a similarly named property in the next patch.
Created attachment 223019 [details] [review] Unified way to set & flush defered property changes Add new API to Boxes.Property class to easily schedule defered changes to it and to flush it when needed.
Created attachment 223020 [details] [review] Allow customization of box in creation wizard V2: This one updates the review page as well.
Review of attachment 223019 [details] [review]: s/defered/deferred
Review of attachment 223016 [details] [review]: ACK
Review of attachment 223017 [details] [review]: Looks good apart from a small question ::: src/wizard.vala @@ +364,3 @@ + if (machine.storage_volume != null) { + try { + var volume_info = machine.storage_volume.get_info (); Are we sure this call is not expensive? (similar to the get_config call above)
(In reply to comment #16) > Review of attachment 223017 [details] [review]: > > Looks good apart from a small question > > ::: src/wizard.vala > @@ +364,3 @@ > + if (machine.storage_volume != null) { > + try { > + var volume_info = machine.storage_volume.get_info (); > > Are we sure this call is not expensive? (similar to the get_config call above) TBH, I don't know. It makes a call to libvirt so its probably also doing sync IO.
Review of attachment 223018 [details] [review]: Can you check with Marc-André if he has no objections to this change?
Review of attachment 223019 [details] [review]: Nice change! ::: src/i-properties-provider.vala @@ +44,3 @@ + defered_change (); + + defered_change = null; As I understand it, all this does is _defered_change = null; defered_change_id = 0; defered_change = null is a convoluted way of achieving this, but avoid poking at too much internal implementation details, so not sure which one is better
Review of attachment 223019 [details] [review]: ::: src/i-properties-provider.vala @@ +44,3 @@ + defered_change (); + + defered_change = null; It also does: Source.remove (defered_change_id); This is the best I could come-up with in the few hours I had to finish this last night. Good that I didn't publish the other approaches that would have made you puke. :) Anyways, suggestions welcome!
Review of attachment 223019 [details] [review]: ::: src/i-properties-provider.vala @@ +44,3 @@ + defered_change (); + + defered_change = null; sorry for the lack of context quoting... The Source.remove will be done automatically as you return false from the Timeout.add_seconds callback ::: src/properties.vala @@ +274,3 @@ + page.flush_changes (); + } + break; After posting the first review I realized I hadn't noticed this change, and I don't really understand why it's needed :(
Review of attachment 223018 [details] [review]: I still prefer changes-pending, since there is no direct relation with reboot, but I have no strong objection...
Review of attachment 223019 [details] [review]: ::: src/i-properties-provider.vala @@ +44,3 @@ + defered_change (); + + defered_change = null; No, that callback is not directly connected to the Timeout so its return value is always ignored. In this place, we are directly calling the callback (before the timeout happens) and then setting it to 'null' to unset both the callback and the timeout. ::: src/properties.vala @@ +274,3 @@ + page.flush_changes (); + } + break; To ensure that all pending deferred (hey, i wrote it right this time!) changes get flushed (I don't know if thats the right term) right at the time user moves away from properties view. This is needed to ensure that wizard's review page gets the latest values when doing wizard -> properties -> wizard. Although not sure if its really needed for the case of display -> properties -> display but AFAIKT it shouldn't be harmful for that case either.
Review of attachment 223020 [details] [review]: ::: src/app.vala @@ +526,3 @@ action_shutdown.set_enabled (ui_state == UIState.DISPLAY && current_item is LibvirtMachine); + foreach (var ui in new Boxes.UI[] { sidebar, searchbar, topbar, view, properties, wizard }) { Why this change in the order? ::: src/libvirt-machine.vala @@ +297,3 @@ }); add_string_property (ref list, _("Virtualizer"), source.uri); + if (display != null) Not really a big fan of all these null checks :-/ When is the create_display at the beginning of this function failing? ::: src/machine.vala @@ +583,3 @@ + if (previous_ui_state == UIState.WIZARD) + // FIXME: We should draw a CD instead as in the mockup: + // https://github.com/gnome-design-team/gnome-mockups/raw/master/boxes/boxes-install5.5.png Maybe we can use the same thumbnail as the one we use for boxes for which we don't have a thumbnail yet? ::: src/wizard.vala @@ +306,3 @@ summary.clear (); + if (vm_creator != null && machine == null) { Not quite sure what changed in this series which can cause us to reach here with a non-null machine? @@ +378,3 @@ + summary.append_customize_button (() => { + App.app.select_item (machine); Can we do something more explicit to get into the properties view? Maybe this deserves a comment if we want to be smart?
Review of attachment 223019 [details] [review]: Leaving as needs-work for the s/defered/deferred change, but apart from this looks good to me. ::: src/i-properties-provider.vala @@ +44,3 @@ + defered_change (); + + defered_change = null; Ah, we're not talking about the same thing, I was referring to the timeout callback set above which unconditionnally return false, and you are talking about the properties.vala changes. ::: src/properties.vala @@ +274,3 @@ + page.flush_changes (); + } + break; Ah ok, makes sense, fwiw I would have split this change to a different commit, since the rest of the changes are a simple refactoring/cleanup, and this one adds better behaviour.
Review of attachment 223020 [details] [review]: ::: src/app.vala @@ +526,3 @@ action_shutdown.set_enabled (ui_state == UIState.DISPLAY && current_item is LibvirtMachine); + foreach (var ui in new Boxes.UI[] { sidebar, searchbar, topbar, view, properties, wizard }) { So that properties sees the UI state change before wizard and therefore all changes are flushed before wizard refreshes its review page. ::: src/libvirt-machine.vala @@ +297,3 @@ }); add_string_property (ref list, _("Virtualizer"), source.uri); + if (display != null) Me neither. When domain config doesn't have the required nodes/attrirbutes for display or unrecognised prototocal (not spice or vnc). ::: src/machine.vala @@ +583,3 @@ + if (previous_ui_state == UIState.WIZARD) + // FIXME: We should draw a CD instead as in the mockup: + // https://github.com/gnome-design-team/gnome-mockups/raw/master/boxes/boxes-install5.5.png Probably, don't think we do that in properties. Since we almost always have a thumbnail for running machine, never got to see anything other than thumbnail in the props view and don't know if its easily possible and that it looks good or not. I'll have a look into this next if time permits. ::: src/wizard.vala @@ +306,3 @@ summary.clear (); + if (vm_creator != null && machine == null) { The rerun of 'review' call from ui_state_changed below @@ +378,3 @@ + summary.append_customize_button (() => { + App.app.select_item (machine); IMO we *are* simply selectiong a machine and that just means different things in different states. I can add a comment, yes.
Review of attachment 223019 [details] [review]: ::: src/i-properties-provider.vala @@ +44,3 @@ + defered_change (); + + defered_change = null; No, I was talking about this exact function (flush) here. Its only being called from properties.vala so in a way, you are correct. ::: src/properties.vala @@ +274,3 @@ + page.flush_changes (); + } + break; I would have also probably have split it more but to split or not to split is hard decision since yours and elmarco's preference on that is very different and I'm not sure who will review.
Review of attachment 223020 [details] [review]: ::: src/app.vala @@ +526,3 @@ action_shutdown.set_enabled (ui_state == UIState.DISPLAY && current_item is LibvirtMachine); + foreach (var ui in new Boxes.UI[] { sidebar, searchbar, topbar, view, properties, wizard }) { Seems very non-obvious and thus error-prone. Do we already have some order-dependent side-effects when iterating over this list? Should we mention this in a comment? ::: src/libvirt-machine.vala @@ +298,3 @@ add_string_property (ref list, _("Virtualizer"), source.uri); + if (display != null) + add_string_property (ref list, _("URI"), display.uri); The spice uri does not look good with the default, it shows port=0, which is not the value that will be used afterwards. ::: src/machine.vala @@ +583,3 @@ + if (previous_ui_state == UIState.WIZARD) + // FIXME: We should draw a CD instead as in the mockup: + // https://github.com/gnome-design-team/gnome-mockups/raw/master/boxes/boxes-install5.5.png ok ::: src/wizard.vala @@ +306,3 @@ summary.clear (); + if (vm_creator != null && machine == null) { ok thanks @@ +378,3 @@ + summary.append_customize_button (() => { + App.app.select_item (machine); We could also have a do_stuff() function and get rid of plenty of functions with different names ;) The machine is already selected because we are already acting on it (creating it)
Review of attachment 223019 [details] [review]: ::: src/i-properties-provider.vala @@ +44,3 @@ + defered_change (); + + defered_change = null; + defered_change_id = Timeout.add_seconds (defer_interval, () => { + flush (); + + return false; + }); ::: src/properties.vala @@ +274,3 @@ + page.flush_changes (); + } + break; Well, Marc-Andre is wrong ;) Latest example to date is one of his make dist fixes which break make -j from clean working dirs, and first thing I did was to untangle unrelated changes... NB: not asking you to split this more, this was just a coment.
Review of attachment 223020 [details] [review]: ::: src/app.vala @@ +526,3 @@ action_shutdown.set_enabled (ui_state == UIState.DISPLAY && current_item is LibvirtMachine); + foreach (var ui in new Boxes.UI[] { sidebar, searchbar, topbar, view, properties, wizard }) { > Seems very non-obvious and thus error-prone. Indeed but I just couldn't figure a better not-very-intrusive way of achieving the same. A comment it is then. ::: src/libvirt-machine.vala @@ +298,3 @@ add_string_property (ref list, _("Virtualizer"), source.uri); + if (display != null) + add_string_property (ref list, _("URI"), display.uri); True, Any easy way to fix this? ::: src/wizard.vala @@ +378,3 @@ + summary.append_customize_button (() => { + App.app.select_item (machine); Oh come on, thats hardly a fair comparison. You do have a point though about machine already being selected. As promised, I'll add comment anyway.
Review of attachment 223019 [details] [review]: ::: src/properties.vala @@ +274,3 @@ + page.flush_changes (); + } + break; I know that he is wrong but just don't feel strongly enough about this to have the motivation/energy to fight him over this. :)
Created attachment 223068 [details] [review] Unified way to set & flush deferred property changes V2: Correct spelling of 'deferred' everywhere.
Created attachment 223069 [details] [review] Allow customization of box in creation wizard V3: Comments added in two places to make things more explicit.
Review of attachment 223069 [details] [review]: Failed to publish review: Undefined subroutine &extensions::splinter::lib::WSSplinter::ThrowCodeError called at /var/www/bugzilla.gnome.org/extensions/splinter/lib/WSSplinter.pm line 88.
Review of attachment 223068 [details] [review]: Failed to publish review: Undefined subroutine &extensions::splinter::lib::WSSplinter::ThrowCodeError called at /var/www/bugzilla.gnome.org/extensions/splinter/lib/WSSplinter.pm line 88.
Created attachment 223089 [details] [review] Allow customization of box in creation wizard V4: Use 'u' as mnemonic for "Customize..." rather than 'C' to not conflict with "Cancel" button.
Attachment 222885 [details] pushed as 80ad2aa - collection-view: Set current UI state on new items Attachment 222886 [details] pushed as 766f12a - media-manager: Add missing namespace Attachment 223016 [details] pushed as 7a4d759 - libvirt-machine: Keep associated storage volume around Attachment 223017 [details] pushed as 2c87e19 - wizard: Show allocated resources at 'review' page Attachment 223018 [details] pushed as f2dd3b0 - Rename Property.changes_pending to reboot_required Attachment 223068 [details] pushed as 2c36aeb - Unified way to set & flush deferred property changes Attachment 223089 [details] pushed as e37e4da - Allow customization of box in creation wizard
This is not working as expected when trying to increase the RAM provided to the guest. Right after creation, the libvirt XML has: <memory unit='KiB'>1179648</memory> <currentMemory unit='KiB'>1179648</currentMemory> currentMemory is automatically added for us: « The actual allocation of memory for the guest. This value can be less than the maximum allocation, to allow for ballooning up the guests memory on the fly. If this is omitted, it defaults to the same value as the memory element. The unit attribute behaves the same as for memory. » (from http://libvirt.org/formatdomain.html#elementsMemoryAllocation ) After setting the box RAM to 2GiB, the XML becomes: <memory unit='KiB'>2116216</memory> <currentMemory unit='KiB'>1179648</currentMemory> We updated the 'memory' tag, but not the 'currentMemory' one. 'currentMemory' is not bound yet in libvirt-glib, so we'll fix that after the release. The UI is in place, this is just a bug fix.
Created attachment 224016 [details] [review] Fix memory customization libvirt uses 2 XML elements to set memory, a /domain/memory node, and a /domain/currentMemory node. The former sets the maximum amount available, and the latter sets the amount of memory currently available to the guest. If only /domain/memory is specified, libvirt will automatically insert a /domain/currentMemory node. This means that after creating a domain, if we want to change the amount of RAM available to the guest, both values must be changed, otherwise /domain/currentMemory will limit the amount of available memory. This commit sets the 'current-memory' property in addition to the 'memory' property when changing the memory. This fixes raising the amount of memory available to the guest. It's done by using GObject properties instead of directly using gvir_config_domain_set_current_memory to avoid raising our libvirt-glib dependency.
ack on this, but we need to ensure that we actually ship the later libvirt-glib in e.g. F18 so that this works.
Yes I'll make sure a libvirt-glib release happens in a few weeks at most.
Attachment 224016 [details] pushed as c72b443 - Fix memory customization