GNOME Bugzilla – Bug 666185
Import existing VMs from system libvirt
Last modified: 2016-03-31 13:59:18 UTC
It would be nice to use the monitor interface by default for read-only operations. Only connect to "manage" interface when needed. When this is done, we can enable qemu:///system source by default
Created attachment 233201 [details] [review] app: Separate out collection sources management Separate out collection sources management from App to separate class. App is kinda bloated so this helps a bit with that. I initially tried to create a libvirt-specific subclass for CollectionSource but that turned out to be much harder to achieve without making a lot of intrusive and/or ugly changes, mainly because type of the source comes from key file and that is loaded during construction and not before (which is where you need to decide, which constructor to use).
Created attachment 233202 [details] [review] Only access storage pool on default source/connection We only need to access storage pools for VMs we create and we only create them on default connection/source.
Created attachment 233203 [details] [review] Connect in read-only to everything but default source/connection Note that this is very limited, you can't even launch domains in read-only mode so we probably want to be able to switch to full access connection when needed. Requires my awaiting-review patches to libvirt-glib.
Created attachment 233204 [details] [review] Setup system libvirt source for user, not just session
Created attachment 233205 [details] [review] Reconnect to libvirt in read-write when needed This patch is the sum of my attempts to simulate on-the-fly upgrade of a read-only libvirt connection to a read-write one. It kinda works but is not at all reliable. We really need libvirt to do this for us and at least one libvirt developer agrees completely: http://www.mail-archive.com/libvir-list@redhat.com/msg69327.html Unless someone can make this work reliably, I suggest we give-up on the idea of 'upgrade connection on demand' for the moment and: 1. Add 'read-only' property to collection source, which is false by default. This will dicate which kind of connection we make. 2. Set this property to true in the system libvirt source we install. i-e we connect as read-only to system libvirt. 3. Don't allow/show privileged actions on read-only machines.
(In reply to comment #1) > Created an attachment (id=233201) [details] [review] > app: Separate out collection sources management > > Separate out collection sources management from App to separate class. > App is kinda bloated so this helps a bit with that. Bleh, this most likely heavily conflicts with the 'broker' part of https://bugzilla.gnome.org/show_bug.cgi?id=681747 on which I never got any feedback on :(
(In reply to comment #6) > (In reply to comment #1) > > Created an attachment (id=233201) [details] [review] [details] [review] > > app: Separate out collection sources management > > > > Separate out collection sources management from App to separate class. > > App is kinda bloated so this helps a bit with that. > > Bleh, this most likely heavily conflicts with the 'broker' part of > https://bugzilla.gnome.org/show_bug.cgi?id=681747 on which I never got any > feedback on :( I was under the impression (based on what you said in last comment and what elmarco told me) that all your patches are now awaiting for you to make the ovirt release.
(In reply to comment #7) > I was under the impression (based on what you said in last comment and what > elmarco told me) that all your patches are now awaiting for you to make the > ovirt release. Answered in https://bugzilla.gnome.org/show_bug.cgi?id=681747#c116 as this has nothing to do with this bug.
Due to the ovirt conflicts the initial patches probably have to wait, but they seem ok to me. I don't think trying to work around the lack of dynamic upgrade is ever gonna work well, so i think for now we should just use readonly by default and ensure all non-supported options are insensitive in the UI. However, we might be able to add some kind of manual support for switching to a non-readonly connection, from the menu or something that just asks for auth.
(In reply to comment #9) > Due to the ovirt conflicts the initial patches probably have to wait, but they > seem ok to me. > I don't particularly mind having to rebase the ovirt patches as long as I know beforehand whether I should keep or drop the broker part (iow I don't want to do a painful rebase to be told afterwards that I should drop it).
(In reply to comment #10) > (In reply to comment #9) > > Due to the ovirt conflicts the initial patches probably have to wait, but they > > seem ok to me. > > > > I don't particularly mind having to rebase the ovirt patches as long as I know > beforehand whether I should keep or drop the broker part (iow I don't want to > do a painful rebase to be told afterwards that I should drop it). Understood. Don't know if it was clear in my reviews so far, I like the approach so I don't think you'll need to drop it.
So, should we go ahead and land the preparatory patches for this then? I don't think we want to land the show-it-readonly patch before we properly make non-working widgetry insensitive.
(In reply to comment #12) > So, should we go ahead and land the preparatory patches for this then? I don't > think we want to land the show-it-readonly patch before we properly make > non-working widgetry insensitive. Yeah but launching of VMs isn't exactly a button so we need to: 1. Ignore activation of read-only machines that are not running (since we can't change state of VMs on read-only connection). 2. Communicate to user that non-running VMs are not launchable. Not sure how to do this one. A lock emblem on the icons?
Its hard to find a reasonable representation of them, because shaded already means something else. Additionally, having them in the collection view but non-launchable seems pretty useless to me. Maybe we could instead not show them at all by default, but then have some menu item to connect to the system libvirt, which requires auth?
*** Bug 692604 has been marked as a duplicate of this bug. ***
Had some discussion about this face2face with mccann and jimmac last week, unfortunately not at the same time though. Firstly the conclusion of discussion with mccann was that (local) system libvirt is not something we want to support like we do libvirt session so what we want is 'import' of existing VMs from system libvirt (if any) for users who were previously using virt-manager. Later I continue discussion with jimmac and he agreed with this conclusion. I proposed the following solution that jimmac seemed to have been in favor of: 1. We connect read-only to system libvirt on startup (perhaps only on first-time launch?) 2. If there is any VMs on that connection, we launch a notification saying something like "Import existing boxes from system broker? [Yes] [x]". The notification will not time out and will only go away on user hitting 'x'. 3. If user hits 'Yes', connect to system libvirt read-write (polkit dialog will obviously be launched) and import all VMs from there. Import will copy all hard disk image to volumes dir as well. 4. Once import is finished, we present another notification saying something like "Import of %d boxes completed. Delete boxes from system broker? [Yes] [x]". This one will timeout in a few seconds. 5. If user hits 'Yes', we delete VMs from system libvirt, and associated hard disk images. Soon after discussion with jimmac, I bumped into teuf. He didn't like the idea but I'll let him explain why. Alex on the other hand was in favor of this idea. There! Its all now on record. :)
This is very disappointing. Can't you at least add some hidden pref to show system VMs (like with qemu+unix:///system now)? I don't need gnome-boxes to be able to add machines there, but I want to use them (and not "import").
(In reply to comment #17) > This is very disappointing. Can't you at least add some hidden pref to show > system VMs (like with qemu+unix:///system now)? I don't need gnome-boxes to be > able to add machines there, but I want to use them (and not "import"). You still have the option to add the connection manually. Its not that hard for users who use virt-manager. Nobody is going to force you into importing. :)
Step 2) seems very bad, hard to understand, very vague on what 'import' means, ... Do we ask every time Boxes start, or just once? If only once, how does a user gets back to getting the dialog if he dismissed it at first, and now wants it? And I have very big doubts that this is what *anyone* wants when they ask about system libvirt. They just want to use an already existing VM, not go through the lengthy process of copying everything (not just the VM they are interested in). I'm not really sure either why qemu://localhost/system gets a special treatment compared to qemu://example.com/system, imo they are not that different, and it would be nice to have a way of handling remote libvirtd as well.
(In reply to comment #19) > Step 2) seems very bad, hard to understand, very vague on what 'import' means, > ... Do we ask every time Boxes start, or just once? If only once, how does a > user gets back to getting the dialog if he dismissed it at first, and now wants > it? The key point here is that local system libvirtd is not a first class citizen and therefore we don't at all need to care about corner cases, at least in first iteration. If user chose not to import, tough luck! you can't do import anymore. > I'm not really sure either why qemu://localhost/system gets a special treatment > compared to qemu://example.com/system, imo they are not that different, and it > would be nice to have a way of handling remote libvirtd as well. They are totally different. Remote connections will always be added manually and that is already (and still will be) possible for local system libvirtd. For a user who uses system libvirtd with virt-manager, I doubt having to add its URL in the wizard is such a big deal. Designers can explain more/better so putting them in CC.
(In reply to comment #20) > (In reply to comment #19) > > Step 2) seems very bad, hard to understand, very vague on what 'import' means, > > ... Do we ask every time Boxes start, or just once? If only once, how does a > > user gets back to getting the dialog if he dismissed it at first, and now wants > > it? > > The key point here is that local system libvirtd is not a first class citizen > and therefore we don't at all need to care about corner cases, at least in > first iteration. If user chose not to import, tough luck! you can't do import > anymore. So we put a hard to understand dialog in their face, and then blame them if they did not understand it the first time? It seems this design assumes that if VMs have been created in system://, this was only done by mistake, and what the user really wants is to only have per-user VMs? > > > I'm not really sure either why qemu://localhost/system gets a special treatment > > compared to qemu://example.com/system, imo they are not that different, and it > > would be nice to have a way of handling remote libvirtd as well. > > They are totally different. Why? It's legitimate to want to use qemu:///system as a shared resource (ie without import), and I can also see a few use cases for importing VMs from a remote libvirtd instance, so what makes them so different that we introduce a qemu:///system-only UI here?
I see that this is going to be yet another long fruitless discussion. Since you are already discussing this on IRC with jimmac, I'll simply defer to what jimmac decides at the end of this discussion.
I didn't expect "import" to make a copy of the VM though. That will generally not work even, as a system image may have a configuration that needs more privs (like system network). I was expecting import to merely make the system VM show up in boxes...
(In reply to comment #23) > not work even, as a system image may have a configuration that needs more privs > (like system network). But that is precisely the reason I don't like boxes imported in this manner to behave differently than those set up within Boxes. All local boxes should behave the same, without exposing the requirement to authorize when you want to run them.
(In reply to comment #22) > I see that this is going to be yet another long fruitless discussion. Since you > are already discussing this on IRC with jimmac, I'll simply defer to what > jimmac decides at the end of this discussion. Jimmac was kind enough to provide explanations/rationales on IRC, which was enough to make the discussion fruitful as far as I'm concerned. What is described in comment #16 really seems to address the following situation: the user has used $VIRTAPP so far to create/manage their VMs. $VIRTAPP defaults to using qemu:///system, the user never changed that, so they have all their personal VMs stored in qemu:///system. By suggesting to import these VMs to the right place the first time we start Boxes, we are making this user's experience better. Another use-case is a user who intentionally created VMs in qemu:///system for sharing purpose or to be able to use features qemu:///session does not have enough permissions to use. This use-case is different from the former, and is not what the design in comment #16 tries to fix as far as I understood.
(In reply to comment #25) > (In reply to comment #22) > > I see that this is going to be yet another long fruitless discussion. Since you > > are already discussing this on IRC with jimmac, I'll simply defer to what > > jimmac decides at the end of this discussion. > > > Jimmac was kind enough to provide explanations/rationales on IRC, which was > enough to make the discussion fruitful as far as I'm concerned. Lets just say that based on past experience, I'm quite incapable of explaining to you. If someone else can do what I can't, thats awesome. :) > What is described in comment #16 really seems to address the following > situation: the user has used $VIRTAPP so far to create/manage their VMs. > $VIRTAPP defaults to using qemu:///system, the user never changed that, so they > have all their personal VMs stored in qemu:///system. By suggesting to import > these VMs to the right place the first time we start Boxes, we are making this > user's experience better. I clearly remember telling you all this in our very short discussion during FOSDEM but as I said, I seem to be incapable of communicating. > Another use-case is a user who intentionally created VMs in qemu:///system for > sharing purpose or to be able to use features qemu:///session does not have > enough permissions to use. This use-case is different from the former, and is > not what the design in comment #16 tries to fix as far as I understood. That sounds much less like a use-case than a work around for limitations in libvirt so not surprisingly, we don't want to address it.
(In reply to comment #23) > I didn't expect "import" to make a copy of the VM though. That will generally > not work even, as a system image may have a configuration that needs more privs > (like system network). > > I was expecting import to merely make the system VM show up in boxes... Hmm.. How would that work? Also, how is there any 'import' involved?
Well, its not *really* an import then i guess. I more saw it as a way to mark a system vm as "really a users vm" and make that particular one show up in boxes (with auth dialogs as needed). Might not be the best approach though.
Pushed a variant of the solution specified in comment#16. Instead of launching notifications, we present an option to import VMs from system libvirt on wizard's source page. Showed both variants to jimmac and we both agreed that this is a better approach since its consistent with existing UI/flow of creating/adding new VMs.