GNOME Bugzilla – Bug 735685
Can't snapshot host-passthrough cpu model using (old) VMs
Last modified: 2016-03-31 13:22:07 UTC
Since snapshot feature doesn't work for VMs using host-passthrough, we better not show snapshot page for such VMs in properties.
Setting the cpu mode of existing VMs seems to be possible and relatively easy -- the only downside is that running VMs won't pick the change up until they are restarted (suspending it also seems to work). Just hiding the snapshots page would also be possible of course, but I guess that would be very surprising to the user.
(In reply to comment #1) > Setting the cpu mode of existing VMs seems to be possible and relatively easy > -- the only downside is that running VMs won't pick the change up until they > are restarted (suspending it also seems to work). > > Just hiding the snapshots page would also be possible of course, but I guess > that would be very surprising to the user. I think we should do both then: * At start-up, we update cpu model if needed. If they are running already (which they typically shouldn't since Boxes saves the VMs on exit), they'll just get the new config on shutdown. * On going to props, we check if running config (rather than saved config that we typically change) has the right cpu model and only show snaphsots page if cpu model is correct.
Created attachment 285415 [details] [review] Don't show snapshots page for HOST_PASSTHROUGH VMs Since HOST_PASSTHROUGH can't cope with snapshots, just don't add the snapshtos page to the sidebar in the properties view in that case.
Zeenix, can you provide some guidance on where that cpu model change at startup should be made (in the code, I mean)?
Review of attachment 285415 [details] [review]: Short log could be shorter and should have prefix: "libvirt-machine-props: No snapshots for host-passthrough". In the details then you can keep it clear that its about cpu model of VMs that we are talking about. Oh and while I'm nitpicking already, best we refer to it so it can be more easily found in libvirt docs: "host-passthrough". ::: src/libvirt-machine-properties.vala @@ +280,3 @@ + try { + var config = machine.domain.get_config (0); + if (config.get_cpu ().get_mode () != GVirConfig.DomainCpuMode.HOST_PASSTHROUGH) Maybe a comment above this line like "Snapshots currently don't work w/ host-passthrough" would be nice.
(In reply to comment #4) > Zeenix, can you provide some guidance on where that cpu model change at startup > should be made (in the code, I mean)? From back of my head, I'd think libvirt-broker.vala but we gotta check in there that we only do it for 'session' source.
(In reply to comment #6) > (In reply to comment #4) > > Zeenix, can you provide some guidance on where that cpu model change at startup > > should be made (in the code, I mean)? > > From back of my head, I'd think libvirt-broker.vala but we gotta check in there > that we only do it for 'session' source. Also we probably we want to only do this to Boxes created/managed VMs. I guess you can look for the presence of custom xml we insert. There is 'is_live_config' and 'is_install_config' static methods in VMConfigurator. I guess we could add a 'is_installed_config' there and then use that and "is_live_config" to decide to change the cpu model or not. BTW, this reminds me. Have you given any thoughts/testing to snapshots during installation?
Created attachment 285675 [details] [review] libvirt-machine-props: No snapshots for host-passthrough Since VMs with a cpu mode of host-passthrough can't cope with snapshots, just don't add the snapshtos page to the sidebar in the properties view in that case.
(In reply to comment #6) > (In reply to comment #4) > > Zeenix, can you provide some guidance on where that cpu model change at startup > > should be made (in the code, I mean)? > > From back of my head, I'd think libvirt-broker.vala but we gotta check in there > that we only do it for 'session' source. How would I check if the source is the 'session' source? Just check if the source's name contains "session? (In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #4) > > > Zeenix, can you provide some guidance on where that cpu model change at startup > > > should be made (in the code, I mean)? > > > > From back of my head, I'd think libvirt-broker.vala but we gotta check in there > > that we only do it for 'session' source. > > Also we probably we want to only do this to Boxes created/managed VMs. I guess > you can look for the presence of custom xml we insert. There is > 'is_live_config' and 'is_install_config' static methods in VMConfigurator. I > guess we could add a 'is_installed_config' there and then use that and > "is_live_config" to decide to change the cpu model or not. I'm not really sure what is_live_config is used for. is_installed_config would return true if the domain's xm description contains the custom Boxes XML at all? > BTW, this reminds me. Have you given any thoughts/testing to snapshots during > installation? My thoughts were "don't allow it" at some point but I haven't tested anything. Maybe also hide the snapshots entry in the properties if the machine is currently installing?
(In reply to comment #9) > (In reply to comment #6) > > (In reply to comment #4) > > > Zeenix, can you provide some guidance on where that cpu model change at startup > > > should be made (in the code, I mean)? > > > > From back of my head, I'd think libvirt-broker.vala but we gotta check in there > > that we only do it for 'session' source. > > How would I check if the source is the 'session' source? Just check if the > source's name contains "session? Check if its the same as App.default_source. > (In reply to comment #7) > > (In reply to comment #6) > > > (In reply to comment #4) > > > > Zeenix, can you provide some guidance on where that cpu model change at startup > > > > should be made (in the code, I mean)? > > > > > > From back of my head, I'd think libvirt-broker.vala but we gotta check in there > > > that we only do it for 'session' source. > > > > Also we probably we want to only do this to Boxes created/managed VMs. I guess > > you can look for the presence of custom xml we insert. There is > > 'is_live_config' and 'is_install_config' static methods in VMConfigurator. I > > guess we could add a 'is_installed_config' there and then use that and > > "is_live_config" to decide to change the cpu model or not. > I'm not really sure what is_live_config is used for. For checking if the VM is considered to be 'live'. i-e a transient VM unless it writes some bytes on disk before shutting down. > is_installed_config would > return true if the domain's xm description contains the custom Boxes XML at > all? No. I think a better name would be "is_boxes_installed" since we'd consider the case of no custom Boxes XML to be an installed VM. > > BTW, this reminds me. Have you given any thoughts/testing to snapshots during > > installation? > > My thoughts were "don't allow it" at some point but I haven't tested anything. > Maybe also hide the snapshots entry in the properties if the machine is > currently installing? Yeah, I agree.
Comment on attachment 285675 [details] [review] libvirt-machine-props: No snapshots for host-passthrough Attachment 285675 [details] pushed as 33d08a7 - libvirt-machine-props: No snapshots for host-passthrough
Created attachment 285964 [details] [review] libvirt-machine-props: No snapshots during installations We don't want to allow taking snapshots during an unattended installation, so just don't show the snapshots page in the sidebar if an installation if in progress.
Review of attachment 285964 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +282,3 @@ // Snapshots currently don't work with host-passthrough + if (config.get_cpu ().get_mode () != GVirConfig.DomainCpuMode.HOST_PASSTHROUGH && + machine.vm_creator == null) this also covers non-express installation and live session case and IMHO we should allow snapshots at least for live sessions. Wouldn't you agree?
Created attachment 286063 [details] [review] VMConfigurator: add is_boxes_installed is_boxes_installed determines if the given Domain is done installing.
Created attachment 286064 [details] [review] libvirt-machine-props: No snapshots during installations We don't want to allow taking snapshots during an unattended installation, so just don't show the snapshots page in the sidebar if an installation if in progress.
Created attachment 286065 [details] [review] VMConfigurator: Make set_cpu_config public We will use it later in libvirt-broker.vala.
Created attachment 286066 [details] [review] LibvirtBroker: Make add_domain async ... and in turn, some of the other private functions in LibvirtBroker. This will later be necessary when we adjust the domain's cpu mode when addding it.
Created attachment 286067 [details] [review] app: Add constant for default source name
Created attachment 286068 [details] [review] LibvirtBroker: Adjust the cpu mode when adding domains If we see a domain added that: - has a host-passthrough cpu mode - and is from the default source - and is either created by boxes it is probably a VM that was created when Boxes still used host-passthrough for the VM cpu modes. Since host-passthrough doesn't play very well with snapshots, we can change the VM's cpu mode upon adding it.
Review of attachment 286063 [details] [review]: ack, no need to capitalize 'domain' though but rather 'add' (as per https://wiki.gnome.org/Git/CommitMessages).
Review of attachment 286064 [details] [review]: typo: "if in progress". Log seems to suggest that we already don't allow snapshots during unattended installation (do we?). Also its not clear whether we don't allow it for express install or any installation. We shouldn't allow for any type of installation. ::: src/libvirt-machine-properties.vala @@ +282,3 @@ // Snapshots currently don't work with host-passthrough + if (config.get_cpu ().get_mode () != GVirConfig.DomainCpuMode.HOST_PASSTHROUGH && + (VMConfigurator.is_boxes_installed (config) || VMConfigurator.is_live_config (config))) I think we both got confused. :( You want to use is_boxes_installed for deciding whether to change cpu config. Otherwise, this will mean we don't show snapshots page for VMs not installed/imported by Boxes. You want to only check for !VMConfigurator.is_install_config
Review of attachment 286065 [details] [review]: ack, just one nitpick: For consistency, refer to class names rather than file names in log descriptions.
Review of attachment 286066 [details] [review]: ::: src/libvirt-broker.vala @@ +106,3 @@ connection.domain_added.connect ((connection, domain) => { debug ("New domain '%s'", domain.get_name ()); + try_add_new_domain.begin (source, connection, domain); I recall we did a similar change many months ago and ended up with issues. Could you check against this and ensure you are not doing the same: https://bugzilla.gnome.org/show_bug.cgi?id=700238
Review of attachment 286067 [details] [review]: ack
Review of attachment 286068 [details] [review]: Looks good. * " - and is either created by boxes" "either" is redundant and wrong here. * probably -> most likely. :)
(In reply to comment #23) > Review of attachment 286066 [details] [review]: > > ::: src/libvirt-broker.vala > @@ +106,3 @@ > connection.domain_added.connect ((connection, domain) => { > debug ("New domain '%s'", domain.get_name ()); > + try_add_new_domain.begin (source, connection, domain); > > I recall we did a similar change many months ago and ended up with issues. > Could you check against this and ensure you are not doing the same: > https://bugzilla.gnome.org/show_bug.cgi?id=700238 I don't want to pretend that I install evertything about brokers and/or libvirt, but the issues in the bug mentioned are slightly different since this patch doesn't serialize all the brokers, but rather the domains within one broker. So if you have a slow and a fast source, you'll still see the domains from the fast source. We could also just call try_add_existing_domain.begin in add_source instead (if that works).
Created attachment 286072 [details] [review] VMConfigurator: Add is_boxes_installed is_boxes_installed determines if the given domain is done installing.
Created attachment 286073 [details] [review] libvirt-machine-props: No snapshots during installations We don't want to allow taking snapshots during an installation, so just don't show the snapshots page in the sidebar if an installation is in progress.
Created attachment 286074 [details] [review] VMConfigurator: Make set_cpu_config public We will use it later in LibvirtBroker.
Created attachment 286075 [details] [review] LibvirtBroker: Adjust the cpu mode when adding domains If we see a domain added that: - has a host-passthrough cpu mode - and is from the default source - and is created by boxes it is a VM that was created when Boxes still used host-passthrough for the VM cpu modes. Since host-passthrough doesn't play very well with snapshots, we can change the VM's cpu mode upon adding it.
(In reply to comment #26) > (In reply to comment #23) > > Review of attachment 286066 [details] [review] [details]: > > > > ::: src/libvirt-broker.vala > > @@ +106,3 @@ > > connection.domain_added.connect ((connection, domain) => { > > debug ("New domain '%s'", domain.get_name ()); > > + try_add_new_domain.begin (source, connection, domain); > > > > I recall we did a similar change many months ago and ended up with issues. > > Could you check against this and ensure you are not doing the same: > > https://bugzilla.gnome.org/show_bug.cgi?id=700238 > > I don't want to pretend that I install evertything about brokers and/or > libvirt, but the issues in the bug mentioned are slightly different since this > patch doesn't serialize all the brokers, but rather the domains within one > broker. So if you have a slow and a fast source, you'll still see the domains > from the fast source. We could also just call try_add_existing_domain.begin in > add_source instead (if that works). Hmm.. could you please test that this doesn't break things the same way the "Asynchronously load VM sources" patch did? commit: 62e0e7b 62e0e7bb049606a04c2bdf6a0b3da50dcd617cea Author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org> Date: Mon Sep 2 17:57:52 2013 +0300 Revert "Asynchronously load VM sources" This patch broke launching of VMs from commandline and therefore from gnome-shell search results as well. Reverting it for now but lets re-implement it in a more robust way later. This reverts commit 14484a31f775d73412cd49dd42e8baae5e75366f. https://bugzilla.gnome.org/show_bug.cgi?id=706742
Review of attachment 286072 [details] [review]: ack
Review of attachment 286073 [details] [review]: ack
Review of attachment 286074 [details] [review]: Actually it'd probably better if VMConfigurator updated the CPU config rather than exposing this internal function. Maybe we could instead add "update_existing_domain" here and just call that from LibvirtBroker? That way CPU config assignment is always done in this module.
(In reply to comment #34) > Review of attachment 286074 [details] [review]: > > Actually it'd probably better if VMConfigurator updated the CPU config rather > than exposing this internal function. Maybe we could instead add > "update_existing_domain" here and just call that from LibvirtBroker? That way > CPU config assignment is always done in this module. Seems like update_existing_domain would do nothing but call set_cpu_config, is that really what you mean? (In reply to comment #31) > (In reply to comment #26) > > (In reply to comment #23) > > > Review of attachment 286066 [details] [review] [details] [details]: > > > > > > ::: src/libvirt-broker.vala > > > @@ +106,3 @@ > > > connection.domain_added.connect ((connection, domain) => { > > > debug ("New domain '%s'", domain.get_name ()); > > > + try_add_new_domain.begin (source, connection, domain); > > > > > > I recall we did a similar change many months ago and ended up with issues. > > > Could you check against this and ensure you are not doing the same: > > > https://bugzilla.gnome.org/show_bug.cgi?id=700238 > > > > I don't want to pretend that I install evertything about brokers and/or > > libvirt, but the issues in the bug mentioned are slightly different since this > > patch doesn't serialize all the brokers, but rather the domains within one > > broker. So if you have a slow and a fast source, you'll still see the domains > > from the fast source. We could also just call try_add_existing_domain.begin in > > add_source instead (if that works). > > Hmm.. could you please test that this doesn't break things the same way the > "Asynchronously load VM sources" patch did? > > commit: 62e0e7b 62e0e7bb049606a04c2bdf6a0b3da50dcd617cea > Author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org> > Date: Mon Sep 2 17:57:52 2013 +0300 > > Revert "Asynchronously load VM sources" > > This patch broke launching of VMs from commandline and therefore > from gnome-shell search results as well. Reverting it for now but lets > re-implement it in a more robust way later. > > This reverts commit 14484a31f775d73412cd49dd42e8baae5e75366f. > > https://bugzilla.gnome.org/show_bug.cgi?id=706742 I have no idea how to test search providers and got no reply on IRC today. Do you have some more information on that?
(In reply to comment #35) > (In reply to comment #34) > > Review of attachment 286074 [details] [review] [details]: > > > > Actually it'd probably better if VMConfigurator updated the CPU config rather > > than exposing this internal function. Maybe we could instead add > > "update_existing_domain" here and just call that from LibvirtBroker? That way > > CPU config assignment is always done in this module. > > Seems like update_existing_domain would do nothing but call set_cpu_config, is > that really what you mean? not really, it will also check if cpu config is old and update it if necessary. We could use the same function later for other similar updates. E.g I was thinking of updating network configuration to use NAT network if they are still using the slow user networking. > (In reply to comment #31) > > (In reply to comment #26) > > > (In reply to comment #23) > > > > Review of attachment 286066 [details] [review] [details] [details] [details]: > > > > > > > > ::: src/libvirt-broker.vala > > > > @@ +106,3 @@ > > > > connection.domain_added.connect ((connection, domain) => { > > > > debug ("New domain '%s'", domain.get_name ()); > > > > + try_add_new_domain.begin (source, connection, domain); > > > > > > > > I recall we did a similar change many months ago and ended up with issues. > > > > Could you check against this and ensure you are not doing the same: > > > > https://bugzilla.gnome.org/show_bug.cgi?id=700238 > > > > > > I don't want to pretend that I install evertything about brokers and/or > > > libvirt, but the issues in the bug mentioned are slightly different since this > > > patch doesn't serialize all the brokers, but rather the domains within one > > > broker. So if you have a slow and a fast source, you'll still see the domains > > > from the fast source. We could also just call try_add_existing_domain.begin in > > > add_source instead (if that works). > > > > Hmm.. could you please test that this doesn't break things the same way the > > "Asynchronously load VM sources" patch did? > > > > commit: 62e0e7b 62e0e7bb049606a04c2bdf6a0b3da50dcd617cea > > Author: Zeeshan Ali (Khattak) <zeeshanak@gnome.org> > > Date: Mon Sep 2 17:57:52 2013 +0300 > > > > Revert "Asynchronously load VM sources" > > > > This patch broke launching of VMs from commandline and therefore > > from gnome-shell search results as well. Reverting it for now but lets > > re-implement it in a more robust way later. > > > > This reverts commit 14484a31f775d73412cd49dd42e8baae5e75366f. > > > > https://bugzilla.gnome.org/show_bug.cgi?id=706742 > > I have no idea how to test search providers and got no reply on IRC today. > Do you have some more information on that? Not search providers. You need to test if you can launch into a box directly from commandline.
> Not search providers. You need to test if you can launch into a box directly > from commandline. You do that by just passing the title (not name) of box. E.g $ gnome-boxes "Debian Wheezy"
(In reply to comment #37) > > Not search providers. You need to test if you can launch into a box directly > > from commandline. > > You do that by just passing the title (not name) of box. E.g $ gnome-boxes > "Debian Wheezy" Ah (--help isn't much of a help :/). Seems to work after a (very) quick test.
(In reply to comment #38) > (In reply to comment #37) > > > Not search providers. You need to test if you can launch into a box directly > > > from commandline. > > > > You do that by just passing the title (not name) of box. E.g $ gnome-boxes > > "Debian Wheezy" > > Ah (--help isn't much of a help :/). Seems to work after a (very) quick test. Yeah, I just noticed that --help is borked. :( Glad to year that it doesn't fail.
Created attachment 286170 [details] [review] VMConfigurator: Add update_existing_domain Updates existing domains from the old host-passthrough to the new custom cpu configuration.
Created attachment 286171 [details] [review] LibvirtBroker: Adjust the cpu mode when adding domains If we see a domain added that: - has a host-passthrough cpu mode - and is from the default source - and is created by boxes it is a VM that was created when Boxes still used host-passthrough for the VM cpu modes. Since host-passthrough doesn't play very well with snapshots, we can change the VM's cpu mode upon adding it.
Review of attachment 286170 [details] [review]: ::: src/vm-configurator.vala @@ +227,3 @@ } + public static async void update_existing_domain (GVir.Domain domain, VMConfigurator is supposed to deal with GVirConfig.Domain rather. You want to get the config in the caller and pass the config domain here instead. @@ +228,3 @@ + public static async void update_existing_domain (GVir.Domain domain, + GVir.Connection connection) { nitpick: extra space before argument names. @@ +231,3 @@ + try { + var config = domain.get_config (0); + if (config.get_cpu ().get_mode () == GVirConfig.DomainCpuMode.HOST_PASSTHROUGH && 'GVirConfig.' is redundant in VMConfigurator context and is even one of the reasons why VMConfigurator was separated from VMCreator. @@ +232,3 @@ + var config = domain.get_config (0); + if (config.get_cpu ().get_mode () == GVirConfig.DomainCpuMode.HOST_PASSTHROUGH && + VMConfigurator.is_boxes_installed (config)) { you can ditch 'VMConfigurator.' now. @@ +238,3 @@ + } + } catch (GLib.Error e) { + warning (e.message); Maybe prefix message with some context? "Failed to update CPU config for '%s': %s", domain.name, e.message);
Review of attachment 286171 [details] [review]: Since most of the log actually describe the parent patch, I think it wants to be squashed with it.
Created attachment 286212 [details] [review] LibvirtBroker: Add & use update_existing_domain Add a function to VMConfigurator to update a existing domain and use this function in LibvirtBroker if we see a domain added that: - has a host-passthrough cpu mode - and is from the default source - and is created by boxes In this case, it's a VM that was created when Boxes still used host-passthrough for the VM cpu modes. Since host-passthrough doesn't play very well with snapshots, we can change the VM's cpu mode upon adding it.
Review of attachment 286066 [details] [review]: ack
Review of attachment 286212 [details] [review]: While commit log could be slightly improved, patch is good. I'll change the log. P.s. while I asked you to use class names instead of module's, the convention we've followed for shortlog prefix is the other way around. I'll fix that while pushing as well.
Attachment 286067 [details] pushed as a8265fb - app: Add constant for default source name Attachment 286073 [details] pushed as 9cfc104 - libvirt-machine-props: No snapshots during installations Attachment 286212 [details] pushed as aa60b14 - LibvirtBroker: Add & use update_existing_domain