GNOME Bugzilla – Bug 683489
Sync libvirt machines, delete Boxes config/metadata
Last modified: 2016-03-31 14:01:02 UTC
The GNOME Shell search results are based on cached information. Since bug 683467, we delete those data when the VM is deleted. However, if the libvirt VM/domain is deleted by a third party app, Boxes data may not be in sync, and the deleted VM data will stay around, ending in search results and wasting some space.
Won't all people who removed boxes from Boxes prior to this patch also have stale entries in their config file?
(In reply to comment #1) > Won't all people who removed boxes from Boxes prior to this patch also have > stale entries in their config file? yes
Created attachment 240094 [details] [review] app: Avoid adding duplicate sources Without this fix, we were adding the default 'QEMU Session' source twice. It hasn't been a real problem so far but it will be after we starting deleting stale box configs in the following patches. This patch could have been a slightly simpler if ForeachFilenameFromDirFunc could be async but async delegates are not yet supported (bug#604827).
Created attachment 240095 [details] [review] box-config: Make group prop publically readable
Created attachment 240096 [details] [review] collection-source: Add purge_stale_box_configs() method Add a method to purge all stale box configs under the collection source. A box config can become stale if box (e.g libvirt domain) is removed using a UI other than Boxes (e.g virt-manager, virsh etc).
Created attachment 240097 [details] [review] libvirt-broker: try_add_existing_domain return machine try_add_existing_domain() method now returns the machine it creates/adds.
Created attachment 240098 [details] [review] ovirt-broker: add_vm() returns machine add_vm() method now returns the machine it creates/adds.
Created attachment 240099 [details] [review] broker: add_source() now returns list of added configs add_source() method implementations are now required to return a list of all box configs they added.
Created attachment 240100 [details] [review] broker: Purge stale box configs from newly added sources After adding a new source, purge all stale box configs.
Review of attachment 240094 [details] [review]: ::: src/app.vala @@ +428,3 @@ + + foreach (var source in new_sources) + yield add_collection_source (source); As usual, I'm missing something :( why do we need this hunk? I was under the impression that the "Already added" bit would be enough here?
Review of attachment 240095 [details] [review]: 'publicly' in the subject
Review of attachment 240096 [details] [review]: ::: src/collection-source.vala @@ +145,3 @@ + try { + keyfile.remove_group (group); + save (); I'd do only one call to save() out of the foreach loop.
Review of attachment 240094 [details] [review]: ::: src/app.vala @@ +428,3 @@ + + foreach (var source in new_sources) + yield add_collection_source (source); hmm. did you notice the change just above? We are simply moving the add_collection_source out of foreach_filename_from_dir's callback as we can't yield in there because of the reason I mentioned in the commit log. We were simply launching add_collection_source *async* for each source we found. Since add_collection_source() checks if source is already added *before* defering to mainloop, we could have a scenario like this: 1. source 'A' is found and add_collection source() defers to mainloop. 2. source 'B' (which is a duplicate of 'A') is found and add_collection_source is launched for it too. 3. The add_collection source() from .1 has not yet added source 'A' so the check for duplicate will fail and we'll still have a duplicated source added.
Created attachment 240407 [details] [review] box-config: Make group prop publicly readable Fixed typo in commit log.
Created attachment 240408 [details] [review] collection-source: Add purge_stale_box_configs() method Took 'save' call out of the loop.
Review of attachment 240094 [details] [review]: ::: src/app.vala @@ +428,3 @@ + + foreach (var source in new_sources) + yield add_collection_source (source); Ok, makes more sense now, I had not really considered the yields. Can you add this with the commit log? However, this seems to be papering over the real bug from setup_sources(): "QEMU Session" is copied from /usr/share to ~/.config, and it's added with add_collection_source, and then we iterate over all the files in ~/.config and call add_collection_source on those, so "QEMU Session" is added twice. After your patch, the explicit yield add_collection_source (source); for "QEMU Session" is no longer required, so you should get rid of this.
Review of attachment 240094 [details] [review]: ::: src/app.vala @@ +428,3 @@ + + foreach (var source in new_sources) + yield add_collection_source (source); Actually I'm not understanding why we needed the explicit addition of 'QEMU Session' and also why is it now redundant with my changes?
Review of attachment 240094 [details] [review]: ::: src/app.vala @@ +428,3 @@ + + foreach (var source in new_sources) + yield add_collection_source (source); Before your patch, the addition of "QEMU Session" was done with a yield because we want to make sure default_session is set in this methode. The other sources were 'fire and forget' (add_collection_source().begin()) Now you always yield, even for the other sources, so "QEMU Session" needs less special casing.
Review of attachment 240098 [details] [review]: Hmm isn't this information pretty close to what is in App.app.collection.items?
Review of attachment 240408 [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 240407 [details] [review]: I'd squash that in the next commit fwiw
Review of attachment 240097 [details] [review]: Imo this can be squashed in "broker: add_source() now returns list of added configs"
Review of attachment 240098 [details] [review]: Wrong patch, ignore the previous comment! I'd squash this in "broker: add_source() now returns list of added configs" though
Review of attachment 240099 [details] [review]: Hmm isn't this information pretty close to what is in App.app.collection.items?
(In reply to comment #16) > Review of attachment 240094 [details] [review]: > > ::: src/app.vala > @@ +428,3 @@ > + > + foreach (var source in new_sources) > + yield add_collection_source (source); > > Ok, makes more sense now, I had not really considered the yields. > Can you add this with the commit log? What should I put there?
Review of attachment 240099 [details] [review]: Yeah, I first tried to use that but I bumped into some problem and now I don't recall what the problem was. :(
(In reply to comment #25) > (In reply to comment #16) > > Review of attachment 240094 [details] [review] [details]: > > > > ::: src/app.vala > > @@ +428,3 @@ > > + > > + foreach (var source in new_sources) > > + yield add_collection_source (source); > > > > Ok, makes more sense now, I had not really considered the yields. > > Can you add this with the commit log? > > What should I put there? What I was replying too was a good fit.
Created attachment 240611 [details] [review] app: Avoid adding duplicate sources Without this fix, we were adding the default 'QEMU Session' source twice. It hasn't been a real problem so far but it will be after we starting deleting stale box configs in the following patches. We are now simply moving the add_collection_source out of foreach_filename_from_dir's callback as we can't yield in there because of bug#604827 and removing the explicit addition of 'QEMU Session' source as it becomes redundant with this patch. We were launching add_collection_source *async* for each source we found. Since add_collection_source() checks if source is already added *before* defering to mainloop, we could have a scenario like this: 1. source 'A' is found and add_collection source() defers to mainloop. 2. source 'B' (which is a duplicate of 'A') is found and add_collection_source is launched for it too. 3. The add_collection source() from #1 has not yet added source 'A' so the check for duplicate will fail and we'll still have a duplicated source added.
Review of attachment 240611 [details] [review]: please verify that you can still open a box from command line, this used to work...
Review of attachment 240611 [details] [review]: Yeah, its broken here too but its broken in git master too for me so not relevant to these changes afaict.
Review of attachment 240611 [details] [review]: Oh, actually I had forgotten that you can only pass a UUID and that you have to use --uuid option. `gnome-boxes --open-uuid f84ac373-64e1-b537-e915-961fdb52a596` works fine for me with and without these patches.
Review of attachment 240611 [details] [review]: Looks good now apart from one small comment. Since you also checked that starting a box directly from the command line is not broken, ACK. ::: src/app.vala @@ +367,3 @@ + if (sources.get (source.name) != null) + return; // Already added I think this should not be hit with the removal of var source = new CollectionSource.with_file ("QEMU Session"); so we probably should warn or at least debug if this occurs.
Review of attachment 240099 [details] [review]: Would be nice to remember as I'd rather we don't add a parallel code path doing the same as App.app.collection.items if possible at all.
Review of attachment 240099 [details] [review]: Correction: I'd rather we don't add it without a good reason.
Review of attachment 240099 [details] [review]: Well for one thing App.app.collection.items is not as specific. We only want box configs and we only want them to be of machines added for the source being/just added.
Created attachment 240753 [details] [review] broker: Purge stale box configs from newly added sources This version makes use of App.app.collection.item and hence requires less changes to the codebase.
Created attachment 240755 [details] [review] app: Avoid adding duplicate sources This version adds warning about attempt to add duplicate source.
(In reply to comment #36) > Created an attachment (id=240753) [details] [review] > broker: Purge stale box configs from newly added sources > > This version makes use of App.app.collection.item and hence requires less > changes to the codebase. This seem to work fine here. I think the issue I faced was because of duplicated sources and since that is resoved before this patch, there shouldn't be any problems.
Review of attachment 240753 [details] [review]: ::: src/app.vala @@ +19,3 @@ + } + + protected async abstract void add_source_real (CollectionSource source); I'd tend to not introduce add_source_real and just call the base class add_source from overloaded implementations.
Review of attachment 240753 [details] [review]: ::: src/app.vala @@ +13,3 @@ + continue; + + used_configs.append ((item as Machine).config); (and usual comment about the use of .append() in a loop)
Review of attachment 240753 [details] [review]: ::: src/app.vala @@ +19,3 @@ + } + + protected async abstract void add_source_real (CollectionSource source); good point, this is from the previous approach.
Created attachment 240954 [details] [review] broker: Purge stale box configs from newly added sources After adding a new source, purge all stale box configs.
Review of attachment 240954 [details] [review]: Still ACK
Attachment 240407 [details] pushed as 7e3c303 - box-config: Make group prop publicly readable Attachment 240408 [details] pushed as c9d6e2e - collection-source: Add purge_stale_box_configs() method Attachment 240755 [details] pushed as 6b95a50 - app: Avoid adding duplicate sources Attachment 240954 [details] pushed as 3d71b44 - broker: Purge stale box configs from newly added sources
Review of attachment 240755 [details] [review]: ::: src/app.vala @@ +419,3 @@ + + foreach (var source in new_sources) + yield add_collection_source (source); I'm afraid serializing the add_collection_source() call is not very good. add_collection_source() fetches all the VMs available from source and adds them to the UI. This can take quite some time with remote sources. If the various calls are serialized, this means if a slow source is handled before a fast sourcee, then the VMs from the faster source won't show up until the slow source is done fetching its VMs. Using add_collection_source().begin() was more correct imo.
Review of attachment 240755 [details] [review]: ::: src/app.vala @@ +419,3 @@ + + foreach (var source in new_sources) + yield add_collection_source (source); The check for default_connection that follows needs to wait for all connections/sources to be added (at least until default connection is not available). I think add_collection_source() does not necessarily need to do fetch all VMs etc and it should launch things async as appropriate to return as quickly as possible. Alternative would be: var i = 0; foreach (var source in new_sources) add_collection_source.begin (source, (o, res) => { add_collection_source.end (o, res); i++; if (i == new_sources.length) // Check for default_connection availability goes here });
(In reply to comment #47) > I think add_collection_source() does not necessarily need to do fetch all VMs > etc and it should launch things async as appropriate to return as quickly as > possible. This is exactly what it was doing prior to your patches in this bug, expect for the default source. > > Alternative would be: > > var i = 0; > foreach (var source in new_sources) > add_collection_source.begin (source, (o, res) => { > add_collection_source.end (o, res); > i++; > if (i == new_sources.length) > // Check for default_connection availability goes here > }); Yup, not very nice, but would probably work.
Created attachment 246083 [details] [review] ovirt: Fix cancelling of authentication When authentication to an oVirt broker is cancelled, yield OvirtBroker::add_source() would never return until the authentication times out. Together with the changes 6b95a50c2 'app: Avoid adding duplicate sources', this could cause the UI to appear empty for the duration of the timeout if an oVirt source is created first, and if the user cancelled the connection (or even if the connection takes quite some time).
Created attachment 246084 [details] [review] Asynchronously load VM sources The fix for bug #683489 changed the various VM sources to be loaded sequentially. As some sources can be slower than others (remote oVirt VS local libvirt connection), this means a slow source could block processing of a faster source, causing the UI to be empty for longer than it should. This commit follows one of Zeeshan's suggestion to make that processing asynchronous again.
Review of attachment 246083 [details] [review]: ACK
Review of attachment 246084 [details] [review]: ACK with or without suggested change. ::: src/app.vala @@ +434,3 @@ + add_collection_source.end (res); + i++; + if ((i == new_sources.length ()) && (default_connection == null)) { Putting every simple condition inside '()' serves contrary to improving readability IMHO but I leave that up to you.
Review of attachment 246084 [details] [review]: ::: src/app.vala @@ +434,3 @@ + add_collection_source.end (res); + i++; + if ((i == new_sources.length ()) && (default_connection == null)) { This is not about making things readable, this is about not having to think about operator priority when writing or reading the code.
Review of attachment 246084 [details] [review]: ::: src/app.vala @@ +434,3 @@ + add_collection_source.end (res); + i++; + if ((i == new_sources.length ()) && (default_connection == null)) { 'Not having to think about X while reading" is an improvement to readability IMO. I don't understand though, there is only one operator here. If there would have been more than 2 conditions and '||' was also involved, then I'll also put braces. Just to be clear, I'm just having a curious discussion here. No objections. :)
Review of attachment 246084 [details] [review]: ::: src/app.vala @@ +434,3 @@ + add_collection_source.end (res); + i++; + if ((i == new_sources.length ()) && (default_connection == null)) { There are 2 operators, == and &&
Review of attachment 246084 [details] [review]: ::: src/app.vala @@ +434,3 @@ + add_collection_source.end (res); + i++; + if ((i == new_sources.length ()) && (default_connection == null)) { Ah, I didn't think they would require any thought as it doesn't make any sense for logical operator to have any precedence over comparison operators but I guess its just that this concept got sunk too well into my brain long time ago.
Attachment 246083 [details] pushed as 62bfded - ovirt: Fix cancelling of authentication Attachment 246084 [details] pushed as 14484a3 - Asynchronously load VM sources
Review of attachment 246084 [details] [review]: ::: src/app.vala @@ +434,3 @@ + add_collection_source.end (res); + i++; + if ((i == new_sources.length ()) && (default_connection == null)) { ! is a logical operator that has precedence over comparison ;) But I get your point, you had in mind && and ||.