GNOME Bugzilla – Bug 681599
vm-creator: Explicitly fetch inactive domain configuration
Last modified: 2016-03-31 14:01:30 UTC
In commit 1fa5ae4 we actually started to keep the active config in LibvirtMachine.domain_config which revealed the issue of us forgetting about our previous changes to inactive configuration as we were always modifying the active configuration. This fixes the side effect of broken Fedora installation: Installer will keep on looping forever.
Created attachment 220874 [details] [review] vm-creator: Explicitly fetch inactive domain configuration In commit 1fa5ae4 we actually started to keep the active config in LibvirtMachine.domain_config which revealed the issue of us forgetting about our previous changes to inactive configuration as we were always modifying the active configuration. This fixes the side effect of broken Fedora installation: Installer will keep on looping forever.
Comment on attachment 220874 [details] [review] vm-creator: Explicitly fetch inactive domain configuration >From be098775019356cca74d5cc7f8eb69d7a494ad48 Mon Sep 17 00:00:00 2001 >From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> >Date: Fri, 10 Aug 2012 16:07:08 +0300 >Subject: [PATCH] vm-creator: Explicitly fetch inactive domain configuration > >In commit 1fa5ae4 we actually started to keep the active config in >LibvirtMachine.domain_config which revealed the issue of us forgetting >about our previous changes to inactive configuration as we were always >modifying the active configuration. > I don't understand how commit 1fa5ae4 enters into play here, it seems obviously correct to me. >This fixes the side effect of broken Fedora installation: Installer will >keep on looping forever. > >https://bugzilla.gnome.org/show_bug.cgi?id=681599 >--- > src/vm-creator.vala | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > >diff --git a/src/vm-creator.vala b/src/vm-creator.vala >index 83a1227..7a12e39 100644 >--- a/src/vm-creator.vala >+++ b/src/vm-creator.vala >@@ -118,12 +118,9 @@ private void on_machine_state_changed (GLib.Object object, GLib.ParamSpec? pspec > private void set_post_install_config (LibvirtMachine machine) { > debug ("Setting post-installation configuration on '%s'", machine.name); > try { >- // We need to clone the domain_config as that is supposed to reflect the active config while we are >- // modifying the inactive config, which are different when domain is running. >- 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); >+ var config = machine.domain.get_config (GVir.DomainXMLFlags.INACTIVE); >+ VMConfigurator.post_install_setup (config); >+ machine.domain.set_config (config); This seems to just be a cleanup? > } catch (GLib.Error error) { > warning ("Failed to set post-install configuration on '%s' failed: %s", machine.name, error.message); > } >@@ -133,8 +130,9 @@ private void mark_as_installed (LibvirtMachine machine) requires (machine.state > machine.state == Machine.MachineState.UNKNOWN) { > debug ("Marking '%s' as installed.", machine.name); > try { >- VMConfigurator.mark_as_installed (machine.domain_config); >- machine.domain.set_config (machine.domain_config); >+ var config = machine.domain.get_config (GVir.DomainXMLFlags.INACTIVE); >+ VMConfigurator.mark_as_installed (config); >+ machine.domain.set_config (config); And this would be the real fix? > } catch (GLib.Error error) { > warning ("Failed to marking domain '%s' as installed: %s", machine.name, error.message); > } >-- >1.7.11.2
(In reply to comment #2) > (From update of attachment 220874 [details] [review]) > >From be098775019356cca74d5cc7f8eb69d7a494ad48 Mon Sep 17 00:00:00 2001 > >From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> > >Date: Fri, 10 Aug 2012 16:07:08 +0300 > >Subject: [PATCH] vm-creator: Explicitly fetch inactive domain configuration > > > >In commit 1fa5ae4 we actually started to keep the active config in > >LibvirtMachine.domain_config which revealed the issue of us forgetting > >about our previous changes to inactive configuration as we were always > >modifying the active configuration. > > > > I don't understand how commit 1fa5ae4 enters into play here, it seems obviously > correct to me. Without the explict flags in 1fa5ae4, we were fetching the INACTIVE config so we were always accessing and modifying the INACTIVE config. So here is what was happening after 1fa5ae4: * For postinstall changes, we copy the ACTIVE config, modify the copy and set it as the INACTIVE config. * After the machine reboot, we once again take the ACTIVE config (the one that we did not modify), mark the os state in it and set that as INACTIVE config. Now at this stage we lost the postinstall changes. > >This fixes the side effect of broken Fedora installation: Installer will > >keep on looping forever. > > > >https://bugzilla.gnome.org/show_bug.cgi?id=681599 > >--- > > src/vm-creator.vala | 14 ++++++-------- > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > >diff --git a/src/vm-creator.vala b/src/vm-creator.vala > >index 83a1227..7a12e39 100644 > >--- a/src/vm-creator.vala > >+++ b/src/vm-creator.vala > >@@ -118,12 +118,9 @@ private void on_machine_state_changed (GLib.Object object, GLib.ParamSpec? pspec > > private void set_post_install_config (LibvirtMachine machine) { > > debug ("Setting post-installation configuration on '%s'", machine.name); > > try { > >- // We need to clone the domain_config as that is supposed to reflect the active config while we are > >- // modifying the inactive config, which are different when domain is running. > >- 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); > >+ var config = machine.domain.get_config (GVir.DomainXMLFlags.INACTIVE); > >+ VMConfigurator.post_install_setup (config); > >+ machine.domain.set_config (config); > > This seems to just be a cleanup? Kinda, we are now fetching our own copy of the (now) obviously inactive config, modifying that and setting it. > > } catch (GLib.Error error) { > > warning ("Failed to set post-install configuration on '%s' failed: %s", machine.name, error.message); > > } > >@@ -133,8 +130,9 @@ private void mark_as_installed (LibvirtMachine machine) requires (machine.state > > machine.state == Machine.MachineState.UNKNOWN) { > > debug ("Marking '%s' as installed.", machine.name); > > try { > >- VMConfigurator.mark_as_installed (machine.domain_config); > >- machine.domain.set_config (machine.domain_config); > >+ var config = machine.domain.get_config (GVir.DomainXMLFlags.INACTIVE); > >+ VMConfigurator.mark_as_installed (config); > >+ machine.domain.set_config (config); > > And this would be the real fix? Yes.
(In reply to comment #3) > Without the explict flags in 1fa5ae4, we were fetching the INACTIVE config so > we were always accessing and modifying the INACTIVE config. 1fa5ae4 is: - domain_config = domain.get_config (0); + domain_config = domain.get_config (GVir.DomainXMLFlags.NONE); and libvirt-gobject-domain.h has: GVIR_DOMAIN_XML_NONE = 0 so I'm missing how 1fa5ae4 could cause a behaviour change.
(In reply to comment #4) > (In reply to comment #3) > > > Without the explict flags in 1fa5ae4, we were fetching the INACTIVE config so > > we were always accessing and modifying the INACTIVE config. > > > 1fa5ae4 is: > - domain_config = domain.get_config (0); > + domain_config = domain.get_config (GVir.DomainXMLFlags.NONE); > > and libvirt-gobject-domain.h has: > GVIR_DOMAIN_XML_NONE = 0 > > so I'm missing how 1fa5ae4 could cause a behaviour change. I should have checked that so now its also a mystery for me how that broke fedora install. My next guess would be that its the libvirt event issue (I keep forgetting to file a bug about that): we don't get the signal for config change and therefore LibvirtMachine.domain_config never (even when machine shuts down) gets updated for post_install.
<zeenix> danpb: when a domain shuts down, do i get the shutdown event first or the config change (if config was updated) <danpb> the shutdown event is emitted before the config change applies <danpb> but you can't invoke any APIs until the config change is complete <danpb> since the domain will be locked at this point <danpb> so if you see the shutdown event, and then try to query the config, you should see the update config <zeenix> danpb: ah <zeenix> that would explain why we need the patch in bug#681599 <bebot> Bug http://bugzilla.gnome.org/show_bug.cgi?id=681599 normal, Normal, ---, gnome-boxes-maint, NEW, vm-creator: Explicitly fetch inactive domain configuration <zeenix> though i wonder how it worked till recently
Review of attachment 220874 [details] [review]: looks good to me
The commit log is definitely not good
Created attachment 221017 [details] [review] vm-creator: Explicitly fetch inactive domain configuration We were modifying the inactive domain configuration after domain launch and later we were modifying it yet again after domain shutdown. Trouble is that libvirt sends shutdown event before the configuration update event so the 2nd time around, we were modifying the yet to be updated configuration and therefore trashing the first modification. This patch fixes the side effect of broken Fedora installation: Installer will keep on looping forever. Its still a mystery to me why we this hasn't been a problem until recently.
Attachment 221017 [details] pushed as 7e8926c - vm-creator: Explicitly fetch inactive domain configuration