GNOME Bugzilla – Bug 678894
Live VM regressions (deleted before launch and not automatically deleted at postinstall)
Last modified: 2016-03-31 13:55:56 UTC
Seems I broke this with commit 93c1ef7 in git master. While I look at how to solve this, I wonder if we want this to happen. Is there a usecase for user wanting to keep using live image as such?
(In reply to comment #0) > Seems I broke this with commit 93c1ef7 in git master. While I look at how to > solve this, I wonder if we want this to happen. Is there a usecase for user > wanting to keep using live image as such? I think not, however that makes me think if live images shouldn't be handled like ready to create&start machines. So they would show up in the collection by default if the image exists somewhere. The wizard would be only used for non-live VMs...
(In reply to comment #1) > (In reply to comment #0) > > Seems I broke this with commit 93c1ef7 in git master. While I look at how to > > solve this, I wonder if we want this to happen. Is there a usecase for user > > wanting to keep using live image as such? > > I think not, however that makes me think if live images shouldn't be handled > like ready to create&start machines. So they would show up in the collection by > default if the image exists somewhere. The wizard would be only used for > non-live VMs... Sounds like a cool idea but let me think more about it first.
Created attachment 217432 [details] [review] Update check for live session as per commit 93c1ef7 Now that post-installation configuration is already set on the domain, it becomes active as soon as domain shuts down so we need to check the older config to see if it was a live session or not.
This patch *should* fix the issue but I haven't yet tested it because of another live regression in git master. I'll test after fixing that..
Comment on attachment 217432 [details] [review] Update check for live session as per commit 93c1ef7 This won't solve the problem for domains that are shutdown while boxes is not running.
Created attachment 217445 [details] [review] Stop postinstall steps until actual first shutdown Domain is in stopped state when its created so we were mistakenly taking it for a domain thats been shutdown. The result was that we were deleting the domain/volume in case of live before even first launch.
Created attachment 217466 [details] [review] vm-creator: Keep signal IDs with associated machines We were only keeping the last connected signal. This meant that we were only doing postinstall setup for latest machine added to collection. Also, we were trying to disconnect signals on wrong domains and ending-up getting an annoying warning on console.
Created attachment 217467 [details] [review] vm-creator: Ensure hooking to Domain.stopped signal
Created attachment 217468 [details] [review] vm-creator: Stop postinstall steps until first shutdown Domain is in stopped state when its created so we were mistakenly taking it for a domain thats been shutdown. The result was that we were deleting the domain/volume in case of live before even first launch.
Created attachment 217469 [details] [review] vm-creator: Tag installed status only after installation While rest of the configuration really should be set after launch of domain, we should not tag the domain as 'installed' before installation is actually finished. This fixes the regression of live domains not getting automatically deleted.
The attached patches fixes the issues with live domains but I'm seeing crashes with this bt against both live and non-live cases: http://fpaste.org/LiI3/ Need to investigate the reason before these patches could go in..
(In reply to comment #11) > Need to investigate the reason before these patches could go in.. > http://fpaste.org/LiI3/ that backtrace isn't very helpful.. valgrind?
Aha, this crash seem to have nothing to do with my recent changes. I can reproduce it against v3.5.3 tag. Steps I take to reproduce this: 1. Create a machine for live media (I used F17) through wizard. 2. Shut it down from inside the guest. Seems more like a regression from Alex's zoom in/out patches.
Just hit something similar with the v3.5.3 tag as well when starting a fedora autoinstall, deleting the box during installation, and then trying to create a winxp autoinstall box. (gdb) bt
+ Trace 230449
I've filed bug #679040 to track this crash.
Review of attachment 217468 [details] [review]: Should we wait for shutdown before taking these steps, or should we do them as soon as libvirt tells us the domain has started?
(In reply to comment #16) > Review of attachment 217468 [details] [review]: > > Should we wait for shutdown before taking these steps, or should we do them as > soon as libvirt tells us the domain has started? We need to wait till domain goes to shutdown. The steps don't even make sense before first run of VM is finished.
(In reply to comment #17) > (In reply to comment #16) > > Review of attachment 217468 [details] [review] [details]: > > > > Should we wait for shutdown before taking these steps, or should we do them as > > soon as libvirt tells us the domain has started? > > We need to wait till domain goes to shutdown. The steps don't even make sense > before first run of VM is finished. I thought we wanted to do them as early as possible since VM configuration changes won't take effect until the domain is shutdown anyway? Doing it early also covers corner cases like qemu crashing, which might be or might not be what we want.
commit 93c1ef795463c8204f58b354f237de4afd0a8ab8 Author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org> Date: Sat Jun 16 05:15:52 2012 +0300 Set post install config soon after starting domain There is no need to wait for domain to shutdown first. Since our VMs are shared with all apps using libvirt, its a very real scenerio that first post-install reboot happens while Boxes is not runnnig: User launches the saved (in-installation) domain from another (libvirt-based) app. This patch fixes that. A small bonus of doing this is that it also makes the issue of libvirt events not getting in[1] much less severe (i-e Fedora installer wont keep installing over and over again). This is what I'm thinking about. Am I misunderstanding something?
(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #16) > > > Review of attachment 217468 [details] [review] [details] [details]: > > > > > > Should we wait for shutdown before taking these steps, or should we do them as > > > soon as libvirt tells us the domain has started? > > > > We need to wait till domain goes to shutdown. The steps don't even make sense > > before first run of VM is finished. > > I thought we wanted to do them as early as possible since VM configuration > changes won't take effect until the domain is shutdown anyway? Doing it early > also covers corner cases like qemu crashing, which might be or might not be > what we want. VM configuration change, yes but not the rest. I even separated out one step of VM config change to 'after' install.
Here is how things go after all my patches: 1. After launching a domain, we already set postinstall config on it, which gets activated as soon as domain shuts down (which is why we want domain to shtudown on first reboot). 2. We wait till domain has shutdown. After which we check if os was installed on the volume. a. If it was, we just unhook from 'shutdown' signal and mark the domain as 'installed' in the config. b. If not, we delete the domain/vol if its a live domain, otherwise we just keep hooked to shutdown signal waiting for the OS install to take place at some point. If domain wasn't deleted, we also start the domain. Hope this clarifies things..
Review of attachment 217466 [details] [review]: Makes sense to me, though isn't it an indication that we should have instantiate a new VMCreator for every box creation?
Review of attachment 217467 [details] [review]: The ChangeLog needs more details as to what was happening (if we started an install, quit boxes and then resumed the install, the on_domain_stop signal would never get connected, so with this commit we make sure the signal gets connected since it takes care of important cleanup in XXX case). ::: src/vm-creator.vala @@ +34,3 @@ var state = machine.domain.get_info ().state; if (state == DomainState.SHUTOFF || state == DomainState.CRASHED || state == DomainState.NONE) on_domain_stopped (machine); Won't this get called right away if boxes was quit with a live image running? (in other words, is a box which was suspended at exit time in the SHUTOFF state?) If so, I think on_domain_stopped will get called too early, which will destroy the live domain, which is bad )
Review of attachment 217467 [details] [review]: ::: src/vm-creator.vala @@ +34,3 @@ var state = machine.domain.get_info ().state; if (state == DomainState.SHUTOFF || state == DomainState.CRASHED || state == DomainState.NONE) on_domain_stopped (machine); bleh, seems this is the next patch in the series.
Review of attachment 217466 [details] [review]: ::: src/vm-creator.vala @@ +34,3 @@ else { + var stopped_id = machine.domain.stopped.connect (() => { on_domain_stopped (machine); }); + machine.set_data<ulong> ("vm-creator-stopped-id", stopped_id); On second though, wouldn't it be better to use g_signal_handlers_disconnect_by_func rather than going through convolutions to keep the signal-id around?
Review of attachment 217469 [details] [review]: I'd rephrase the beginning of the log message a little bit to make it clearer that now we are setting the post install configuration of domains much earlier than before, right after starting it, but that the decision whether the domain has been installed or not must be delayed until the first domain shutdown. "installation is actually finished" is inaccurate in at least the win7 case.
(In reply to comment #22) > Review of attachment 217466 [details] [review]: > > Makes sense to me, though isn't it an indication that we should have > instantiate a new VMCreator for every box creation? Hmm.. interesting suggestion. I could try that afterwards and submit separate patches for it but for now I'd want to get regressions in git master fixed asap.
(In reply to comment #25) > Review of attachment 217466 [details] [review]: > > ::: src/vm-creator.vala > @@ +34,3 @@ > else { > + var stopped_id = machine.domain.stopped.connect (() => { > on_domain_stopped (machine); }); > + machine.set_data<ulong> ("vm-creator-stopped-id", stopped_id); > > On second though, wouldn't it be better to use > g_signal_handlers_disconnect_by_func rather than going through convolutions to > keep the signal-id around? I was about to change this but then got reminded that we are using a lambda here so thats not possible. This convolution will disapear when I act on comment#22.
(In reply to comment #23) > Review of attachment 217467 [details] [review]: > > The ChangeLog needs more details as to what was happening (if we started an > install, quit boxes and then resumed the install, the on_domain_stop signal > would never get connected, so with this commit we make sure the signal gets > connected since it takes care of important cleanup in XXX case). The log already describes the issue and its not what you are saying. The issue here is not quitting of boxes, thats other patches. > ::: src/vm-creator.vala > @@ +34,3 @@ > var state = machine.domain.get_info ().state; > if (state == DomainState.SHUTOFF || state == DomainState.CRASHED > || state == DomainState.NONE) > on_domain_stopped (machine); > > Won't this get called right away if boxes was quit with a live image running? > (in other words, is a box which was suspended at exit time in the SHUTOFF > state?) If so, I think on_domain_stopped will get called too early, which will > destroy the live domain, which is bad ) No, we check for saved VMs in on_domain_stopped.
(In reply to comment #26) > Review of attachment 217469 [details] [review]: > > "installation is actually finished" is inaccurate in at least the win7 case. True but we currently have no reliable way of knowing when the install actually finished. OTOH it doesn't seem to matter if we assume installation is finished on first reboot/shutdown.
*** Bug 679064 has been marked as a duplicate of this bug. ***
Review of attachment 217468 [details] [review]: ::: src/vm-creator.vala @@ +68,3 @@ + var machine = App.app.add_domain (App.app.default_source, App.app.default_connection, domain); + App.app.collection.item_added.connect (on_item_added); + Nope, this looks like a band-aid, it would be better to fix VmCreator::on_item_added and VmCreator::on_domain_stopped to properly handle live VMs. Maybe on_domain_stopped should be split into 2 callbacks, one which would only clean up live VMs data? Maybe we could connect the on_domain_stopped callback from a on_domain_started callback and do some "cold-plug" of already runnig machines at startup? Just throwing out possible solutions I haven't thought out at all. All I know is that we can do better than this signal blocking imo. @@ +93,3 @@ + + var stopped_id = machine.domain.stopped.connect (() => { on_domain_stopped (machine); }); + machine.set_data<ulong> ("vm-creator-stopped-id", stopped_id); I'd do this from a callback connected to the 'domain-started' event (would work if the VM is started externally), but it's not very important if all VM are started through this function.
(In reply to comment #23) > ::: src/vm-creator.vala > @@ +34,3 @@ > var state = machine.domain.get_info ().state; > if (state == DomainState.SHUTOFF || state == DomainState.CRASHED > || state == DomainState.NONE) > on_domain_stopped (machine); > > Won't this get called right away if boxes was quit with a live image running? > (in other words, is a box which was suspended at exit time in the SHUTOFF > state?) If so, I think on_domain_stopped will get called too early, which will > destroy the live domain, which is bad ) This (and comment #24) are indeed addressed by the next patch in the series.
Review of attachment 217468 [details] [review]: ::: src/vm-creator.vala @@ +68,3 @@ + var machine = App.app.add_domain (App.app.default_source, App.app.default_connection, domain); + App.app.collection.item_added.connect (on_item_added); + This isn't just about live VMs, we want to do this for non-live as well. In case of nonlive, we don't do anything bad (at least not as bad as deleting VM and volume) but we shoudn't be calling on_domain_stopped on that. Having said that, I agree that its kinda of a band-aid so I'm more tilting in the direction of having a separate VMCreator instance for each installation.
Created attachment 217629 [details] [review] Make all VMConfigurator methods static This class doesn't have any state and we'll need to call its methods from different contexts in future so it'll be nice to call those methods without having to instantiate needlessly.
Created attachment 217630 [details] [review] Separate VMCreator instances for each VM creation This breaks first-shutdown setup in case of boxes exiting before installation/live session is finished but it will be fixed in the subsequent patches along with other race-conditions/issues in that code.
Created attachment 217631 [details] [review] Rename VMCreator.post_install_setup to set_post_install_config Since we also do some other post-installation setup, its better to make it clear that this function only sets post-install configuration on the new domain and nothing else.
Created attachment 217632 [details] [review] vm-creator: More reliable post-installation setup We need to make sure that post-installation setup is only done once. App.add_domain() gets called twice (assuming libvirt events are working) at domain creation time. Then we have the case of this function being called for existing domains when Boxes starts-up. This function adds the new machine to collection, that emits 'item_added' signal and ultimately that trigered the post_installation setup. We no longer listen to the 'item_added' signal on the collection but launch the post-installation setup (or hook it to domain shutdown) where/when its appropriate. This fixes the recent regression of live VMs failing on first launch. It also re-adds the launch of post-installation setup for existing domains on boxes startup.
Created attachment 217633 [details] [review] vm-creator: Tag installed status only after installation While rest of the configuration really should be set after launch of domain, we should not tag the domain as 'installed' before installation is actually finished. This fixes the regression of live domains not getting automatically deleted.
I just realized that attachment#217632 [details] could be done in a cleaner way. I'll provide a modified version during the weekend.
Created attachment 217736 [details] [review] vm-creator: Separate instances for each VM creation This breaks first-shutdown setup in case of boxes exiting before installation/live session is finished but it will be fixed in the subsequent patches along with other race-conditions/issues in that code.
Created attachment 217737 [details] [review] vm-creator: Rename post_install_setup to set_post_install_config Since we also do some other post-installation setup, its better to make it clear that this function only sets post-install configuration on the new domain and nothing else.
Created attachment 217738 [details] [review] vm-creator: More reliable post-installation setup We need to make sure that post-installation setup is only done once. App.add_domain() gets called twice (assuming libvirt events are working) at domain creation time. Then we have the case of this function being called for existing domains when Boxes starts-up. This function adds the new machine to collection, that emits 'item_added' signal and ultimately that trigered the post_installation setup. We no longer listen to the 'item_added' signal on the collection but launch the post-installation setup (or hook it to domain shutdown) where/when its appropriate. This fixes the recent regression of live VMs failing on first launch. It also re-adds the launch of post-installation setup for existing domains on boxes startup.
Created attachment 217739 [details] [review] vm-creator: Tag installed status only after installation While rest of the configuration really should be set after launch of domain, we should not tag the domain as 'installed' before installation is actually finished. This fixes the regression of live domains not getting automatically deleted.
Created attachment 217740 [details] [review] vm-creator: No need to fetch domain configuration LibvirtMachine keeps domain configuration cached for us so no need for potentially slow blocking call to GVir.Domain.get_config ().
Created attachment 217741 [details] [review] vm-creator: Automatically start saved installations If Boxes was quit while a domain was under installation, we should awake the domain from the saved state next time Boxes is launched.
Created attachment 217742 [details] [review] vm-creator: Automatically start saved installations V2: Start the domain async to not block UI (initialization).
Comment on attachment 217629 [details] [review] Make all VMConfigurator methods static Looks good >+++ b/src/vm-creator.vala >+ if (!VMConfigurator.is_install_config (config) && !VMConfigurator.is_live_config (config)) { >+ var config = VMConfigurator.create_domain_config (install_media, volume.get_path (), caps); >+ if (!VMConfigurator.is_live_config (config)) >+ VMConfigurator.post_install_setup (config); >+ var config = VMConfigurator.create_volume_config (name, storage); >+ var config = VMConfigurator.get_pool_config (); This external API makes things look like this could be a private VMConfiguration: GVir.Config class instead (falls in the "future changes" category)
Comment on attachment 217736 [details] [review] vm-creator: Separate instances for each VM creation >Subject: [PATCH] vm-creator: Separate instances for each VM creation Looks like a pretty mechanical conversion >+ (vm_creator.install_media as UnattendedInstaller).populate_setup_vbox (setup_vbox); This kind of casts makes me wonder if at some point in the future it wouldn't be better to have an UnattendedVMCreator class and have some of the UnattendedInstaller logic move there.
Comment on attachment 217737 [details] [review] vm-creator: Rename post_install_setup to set_post_install_config Yup, more readable this way imo.
Comment on attachment 217739 [details] [review] vm-creator: Tag installed status only after installation >diff --git a/src/vm-creator.vala b/src/vm-creator.vala >index 300ed16..0376b31 100644 >--- a/src/vm-creator.vala >+++ b/src/vm-creator.vala >@@ -129,6 +130,17 @@ private class Boxes.VMCreator { > } > } > >+ private void mark_as_installed (Domain domain) { >+ debug ("Marking '%s' as installed.", domain.get_name ()); >+ try { >+ var config = domain.get_config (0); >+ VMConfigurator.mark_as_installed (config); >+ domain.set_config (config); A domain.update_config () or domain.commit_config () API in libvirt-glib would make it more obvious that modified configurations need to be explicitly set on the domain object (not very important now). Patch looks good!
Comment on attachment 217740 [details] [review] vm-creator: No need to fetch domain configuration At some point we need to stop assuming that domain_config will always be non-null, or enforce it (for general stability...), but this is not the point of this bug. You could fix set_post_install_config and mark_as_installed while you are at it since they are in the same filed, and touched by this series.
Comment on attachment 217742 [details] [review] vm-creator: Automatically start saved installations Patch looks good, but I'm not sure this is a behaviour change we want. Maybe the domain was suspended to save resources or for some other reason? Do we want to do it for all installations, or only express ones?
(In reply to comment #48) > (From update of attachment 217629 [details] [review]) > Looks good > > >+++ b/src/vm-creator.vala > >+ if (!VMConfigurator.is_install_config (config) && !VMConfigurator.is_live_config (config)) { > >+ var config = VMConfigurator.create_domain_config (install_media, volume.get_path (), caps); > >+ if (!VMConfigurator.is_live_config (config)) > >+ VMConfigurator.post_install_setup (config); > >+ var config = VMConfigurator.create_volume_config (name, storage); > >+ var config = VMConfigurator.get_pool_config (); > > This external API makes things look like this could be a > private VMConfiguration: GVir.Config class instead (falls in the "future > changes" category) If we don't go for the changes in attachment#217738 [details], perhaps but I try not to put way too much code in one file and that was the reason I separated out configuration-related bits into vm-configurator.vala.
(In reply to comment #49) > > >+ (vm_creator.install_media as UnattendedInstaller).populate_setup_vbox (setup_vbox); > > This kind of casts makes me wonder if at some point in the future it wouldn't > be better to have an UnattendedVMCreator class and have some of the > UnattendedInstaller logic move there. The task of getting rid of 'InstallerMedia is Unattended' checks' has been on my todo for 2 weeks now, hopefully I get to look at it this week/soon. I'll keep this suggestion in mind when i look into that. (In reply to comment #52) > > You could fix set_post_install_config and mark_as_installed while you are at it > since they are in the same filed, and touched by this series. Fix them in what way?
(In reply to comment #55) > (In reply to comment #52) > > > > You could fix set_post_install_config and mark_as_installed while you are at it > > since they are in the same filed, and touched by this series. > > Fix them in what way? They are the only functions calling "var config = domain.get_config (0);" in git master with your patches applied, which is what this patch is changing.
(In reply to comment #53) > (From update of attachment 217742 [details] [review]) > Patch looks good, but I'm not sure this is a behaviour change we want. I'm pretty sure we want it for the particular case I'm having in mind: Domain was saved by Boxes due to UI quit while installation was still going on. User will expect the installation to continue next time he/she runs Boxes. > Maybe > the domain was suspended to save resources or for some other reason? Good question but does libvirt currently do that? If so, I'd like a libvirt API to be able to ask libvirt if thats the case for a domain. > Do we want > to do it for all installations, or only express ones? Don't think user will want us to differential between these two types in this case. Actually it might confuse them about what Boxes does and doesn't do for them.
(In reply to comment #56) > (In reply to comment #55) > > (In reply to comment #52) > > > > > > You could fix set_post_install_config and mark_as_installed while you are at it > > > since they are in the same filed, and touched by this series. > > > > Fix them in what way? > > They are the only functions calling "var config = domain.get_config (0);" in > git master with your patches applied, which is what this patch is changing. After making those changes, I realized that set_post_install_config() is different in the sense that it modifies the config and set it as the inactive one on the domain. Since the domain is launched before this change, we'll have different configs given to us by LibvirtMachine.domain_config() and domain.get_config() while they are expected to be synonyms. Would have been nice to have a GVirConfig.Domain.clone() for these cases.
(In reply to comment #58) > (In reply to comment #56) > > (In reply to comment #55) > > > (In reply to comment #52) > > > > > > > > You could fix set_post_install_config and mark_as_installed while you are at it > > > > since they are in the same filed, and touched by this series. > > > > > > Fix them in what way? > > > > They are the only functions calling "var config = domain.get_config (0);" in > > git master with your patches applied, which is what this patch is changing. > > After making those changes, I realized that set_post_install_config() is > different in the sense that it modifies the config and set it as the inactive > one on the domain. Since the domain is launched before this change, we'll have > different configs given to us by LibvirtMachine.domain_config() and > domain.get_config() while they are expected to be synonyms. Would have been > nice to have a GVirConfig.Domain.clone() for these cases. NM, gvir_config_object_to_xml() + gvir_config_domain_new_from_xml() should do the trick. :)
Created attachment 217851 [details] [review] vm-creator: No need to fetch domain configuration V2: Remove all usage of domain.get_config() from vm-creator.vala
(In reply to comment #58) > After making those changes, I realized that set_post_install_config() is > different in the sense that it modifies the config and set it as the inactive > one on the domain. Since the domain is launched before this change, we'll have > different configs given to us by LibvirtMachine.domain_config() and > domain.get_config() while they are expected to be synonyms. Are you saying that LibvirtMachine::domain_config should track the inactive XML config? If so, should GVir.Domain emit a "notify::config" signal when it's modified so that LibivrtMachine::domain_config can track this?
(In reply to comment #61) > (In reply to comment #58) > > After making those changes, I realized that set_post_install_config() is > > different in the sense that it modifies the config and set it as the inactive > > one on the domain. Since the domain is launched before this change, we'll have > > different configs given to us by LibvirtMachine.domain_config() and > > domain.get_config() while they are expected to be synonyms. > > Are you saying that LibvirtMachine::domain_config should track the inactive XML > config? No, there is no need for that. I'm just saying that we shouldn't change LibvirtMachine.domain_config itself as that is supposed to reflect the current config. Check my V2 patch for details.
Comment on attachment 217738 [details] [review] vm-creator: More reliable post-installation setup >diff --git a/src/app.vala b/src/app.vala >index 6c40220..496b539 100644 >--- a/src/app.vala >+++ b/src/app.vala >@@ -184,9 +184,18 @@ private class Boxes.App: Boxes.UI { > return machine; > } > >- private void try_add_domain (CollectionSource source, GVir.Connection connection, GVir.Domain domain) { >+ private void try_add_domain (CollectionSource source, >+ GVir.Connection connection, >+ GVir.Domain domain, >+ bool existing = true) { > try { >- add_domain (source, connection, domain); >+ var machine = add_domain (source, connection, domain); >+ var config = machine.domain_config; >+ >+ if (existing && (VMConfigurator.is_install_config (config) || VMConfigurator.is_live_config (config))) { I'd do if (!existing) //some explanation as to what 'existing' means return; I'm even wondering why there couldn't be 2 different functions, one doing the try { add_domain() } and the other one restoring the installer configuration if needed. (still need to look more at that patch :(
Created attachment 217867 [details] [review] vm-creator: More reliable post-installation setup V3: Separate functions for adding new and existing domains and comments indicating what 'existing' and 'new' means.
Comment on attachment 217851 [details] [review] vm-creator: No need to fetch domain configuration >+ var config_xml = machine.domain_config.to_xml (); >+ var post_install_config = new GVirConfig.Domain.from_xml (config_xml); >+ VMConfigurator.post_install_setup (post_install_config); >+ machine.domain.set_config (post_install_config); This part is ugy imo, I'd stick with the old code + a comment explaining why we can't use machine.domain_config.
Comment on attachment 217867 [details] [review] vm-creator: More reliable post-installation setup Looks good >+ public VMCreator.for_install_completion (LibvirtMachine machine) { not a big fan of this name, but I don't have a better suggestion so I'll live with it. >+ state_changed_id = machine.notify["state"].connect (on_machine_state_changed); >+ machine.vm_creator = this; > >- var state = machine.domain.get_info ().state; >- if (state == DomainState.SHUTOFF || state == DomainState.CRASHED || state == DomainState.NONE) >- on_domain_stopped (machine); >- else { >- stopped_id = machine.domain.stopped.connect (() => { on_domain_stopped (machine); }); >- } >- } catch (GLib.Error error) { >- warning ("Failed to get information on domain '%s': %s", machine.domain.get_name (), error.message); >- } >+ on_machine_state_changed (machine); Why do we need to call this explicitly? (not saying it's not needed, it's just something I didn't get while looking at this patch).
(In reply to comment #66) > (From update of attachment 217867 [details] [review]) > Looks good > > > >+ public VMCreator.for_install_completion (LibvirtMachine machine) { > > not a big fan of this name, but I don't have a better suggestion so I'll live > with it. Same here. :) We can always change it later if we come-up with something better. > >+ state_changed_id = machine.notify["state"].connect (on_machine_state_changed); > >+ machine.vm_creator = this; > > > >- var state = machine.domain.get_info ().state; > >- if (state == DomainState.SHUTOFF || state == DomainState.CRASHED || state == DomainState.NONE) > >- on_domain_stopped (machine); > >- else { > >- stopped_id = machine.domain.stopped.connect (() => { on_domain_stopped (machine); }); > >- } > >- } catch (GLib.Error error) { > >- warning ("Failed to get information on domain '%s': %s", machine.domain.get_name (), error.message); > >- } > >+ on_machine_state_changed (machine); > > Why do we need to call this explicitly? (not saying it's not needed, it's just > something I didn't get while looking at this patch). For machines that were already shutdown (this happens when VM got shutdown while Boxes wasn't running). From log of 93c1ef7 in git master: ".. Since our VMs are shared with all apps using libvirt, its a very real scenerio that first post-install reboot happens while Boxes is not runnnig: User launches the saved (in-installation) domain from another (libvirt-based) app."
(In reply to comment #65) > (From update of attachment 217851 [details] [review]) > > >+ var config_xml = machine.domain_config.to_xml (); > >+ var post_install_config = new GVirConfig.Domain.from_xml (config_xml); > >+ VMConfigurator.post_install_setup (post_install_config); > >+ machine.domain.set_config (post_install_config); > > This part is ugy imo, I'd stick with the old code + a comment explaining why we > can't use machine.domain_config. But we can use it. Its ugly yes but from experience the lesser we make blocking calls in boxes the better so I'd say its worth the few lines of ugliness. How about I add a comment there explaining why we clone the config?
(In reply to comment #67) > ".. Since our VMs are shared with all apps using libvirt, its a very real > scenerio that first post-install reboot happens while Boxes is not runnnig: > User launches the saved (in-installation) domain from another (libvirt-based) > app." I'd mention it in a comment above this call then.
(In reply to comment #68) > (In reply to comment #65) > > (From update of attachment 217851 [details] [review] [details]) > > > > >+ var config_xml = machine.domain_config.to_xml (); > > >+ var post_install_config = new GVirConfig.Domain.from_xml (config_xml); > > >+ VMConfigurator.post_install_setup (post_install_config); > > >+ machine.domain.set_config (post_install_config); > > > > This part is ugy imo, I'd stick with the old code + a comment explaining why we > > can't use machine.domain_config. > > But we can use it. Its ugly yes but from experience the lesser we make blocking > calls in boxes the better so I'd say its worth the few lines of ugliness. Some persons consider XML parsing in the main thread as 'blocking' ;) > How about I add a comment there explaining why we clone the config? Like this: ? ;) // We didn't add the right API to libvirt-gconfig so we go with this hack instead Yeah a comment explaining why we can't modify the existing config would be nice.
(In reply to comment #69) > (In reply to comment #67) > > > ".. Since our VMs are shared with all apps using libvirt, its a very real > > scenerio that first post-install reboot happens while Boxes is not runnnig: > > User launches the saved (in-installation) domain from another (libvirt-based) > > app." > > I'd mention it in a comment above this call then. Well its not actually known to this part of the code. Think of this as the state change from previousl unknown to what it is now and hence the need to call state-change handler.
Created attachment 217981 [details] [review] vm-creator: No need to fetch domain configuration Added the comment to domain config cloning code.
(In reply to comment #71) > (In reply to comment #69) > > (In reply to comment #67) > > > > > ".. Since our VMs are shared with all apps using libvirt, its a very real > > > scenerio that first post-install reboot happens while Boxes is not runnnig: > > > User launches the saved (in-installation) domain from another (libvirt-based) > > > app." > > > > I'd mention it in a comment above this call then. > > Well its not actually known to this part of the code. Think of this as the > state change from previousl unknown to what it is now and hence the need to > call state-change handler. Since this is an important fix and you said it looks good except for this readability issue, I'll already push this with others that you ACK'ed. We can add a comment later if you think its needed.
Attachment 217629 [details] pushed as 776b66d - Make all VMConfigurator methods static Attachment 217736 [details] pushed as 0d87fa3 - vm-creator: Separate instances for each VM creation Attachment 217737 [details] pushed as 0f4d154 - vm-creator: Rename post_install_setup to set_post_install_config Attachment 217739 [details] pushed as 4b3fd86 - vm-creator: Tag installed status only after installation Attachment 217867 [details] pushed as 16177fb - vm-creator: More reliable post-installation setup
Review of attachment 217981 [details] [review]: Looks good, just 2 tiny things. ::: src/vm-creator.vala @@ +118,3 @@ try { + // We need to clone the domain_config as that is supposed to reflect the active config while we are setting + // the inactive config, which are different when domain is running. I think the important bit is that we'll be *modifying* our copy of machine.domain_config, and we want machine.domain_config to always reflect the active config, so we can't change it. @@ +128,3 @@ } + // Assumes machine is shutoff return_if_fail (machine.state != xxxx) ? (assuming this is not expensive)
(In reply to comment #75) > Review of attachment 217981 [details] [review]: > > Looks good, just 2 tiny things. > > ::: src/vm-creator.vala > @@ +118,3 @@ > try { > + // We need to clone the domain_config as that is supposed to > reflect the active config while we are setting > + // the inactive config, which are different when domain is > running. > > I think the important bit is that we'll be *modifying* our copy of > machine.domain_config, and we want machine.domain_config to always reflect the > active config, so we can't change it. I don't see how my comment doesn't say that: 'as that is supposed to reflect the active config' 'while we are setting the inactive config'.
Created attachment 218029 [details] [review] vm-creator: No need to fetch domain configuration This version adds a check for machine state.
Did i convince you about attachement#217742 in Comment#57?
Created attachment 218032 [details] [review] wizard: Do create VM for unattended installers This fixes another critical regression from commit 13bca42.
Created attachment 218033 [details] [review] wizard: Do create VM for non-unattended installers V2: Fix small typo in commit log.
(In reply to comment #76) > I don't see how my comment doesn't say that: > > 'as that is supposed to reflect the active config' > 'while we are setting the inactive config'. To me, this 'setting' refers to machine.domain.set_config (post_install_config); while the reason the cloning is needed is the call to VMConfigurator.post_install_setup (post_install_config); so no you're not putting an emphasis on the 'modification' part.
Comment on attachment 218033 [details] [review] wizard: Do create VM for non-unattended installers Attachment 218033 [details] pushed as c54ae47 - wizard: Do create VM for non-unattended installers
(In reply to comment #78) > Did i convince you about attachement#217742 in Comment#57? I don't really care actually, I was just curious if this were things you had considered.
Comment on attachment 218029 [details] [review] vm-creator: No need to fetch domain configuration >+ private void mark_as_installed (LibvirtMachine machine) requires (machine.state == Machine.MachineState.STOPPED || >+ machine.state == Machine.MachineState.UNKNOWN) { Makes things hard to read without a helper (far too long to be stuck like that at the end of the function declaration), is there no regular "return_if_fail ()" which could be used in the function body?
Comment on attachment 217742 [details] [review] vm-creator: Automatically start saved installations Attachment 217742 [details] pushed as 07d994a - vm-creator: Automatically start saved installations
(In reply to comment #84) > (From update of attachment 218029 [details] [review]) > > >+ private void mark_as_installed (LibvirtMachine machine) requires (machine.state == Machine.MachineState.STOPPED || > >+ machine.state == Machine.MachineState.UNKNOWN) { > > Makes things hard to read without a helper (far too long to be stuck like that > at the end of the function declaration), Looks good to me but I can put it on newline aligned to 'LibvirtMachine..' above it. > is there no regular "return_if_fail > ()" which could be used in the function body? There is I think but I prefer the built-in syntax rather: Its more cleaner and obvious IMHO.
Created attachment 218111 [details] [review] vm-creator: No need to fetch domain configuration LibvirtMachine keeps domain configuration cached for us so no need for potentially slow blocking call to GVir.Domain.get_config ().
Comment on attachment 218111 [details] [review] vm-creator: No need to fetch domain configuration Whatever, equally unreadable.
(In reply to comment #88) > (From update of attachment 218111 [details] [review]) > Whatever, equally unreadable. Sorry I failed. I'll revert that part of the change as IMO this looks uglier and its the same for you..
Attachment 218111 [details] pushed as e18ba16 - vm-creator: No need to fetch domain configuration