GNOME Bugzilla – Bug 690757
Support VM creation out of ready-made/installed disk images
Last modified: 2016-03-31 13:59:16 UTC
I'm trying to create a box from a couple of "image" formats in files with extensions .img and .dsk. The select from file dialog doesn't even show them.
(In reply to comment #0) > I'm trying to create a box from a couple of "image" formats in files with > extensions .img and .dsk. The select from file dialog doesn't even show them. We'll need some way to detect which OS is installed on the image to be able to create a suitable VM for it and the only way to do that is to use libguesfs but that takes a looong time to extract information: guestfish. I think it'd be more important to implement saving and loading of VMs first: bug#677691.
(In reply to comment #1) > (In reply to comment #0) > > I'm trying to create a box from a couple of "image" formats in files with > > extensions .img and .dsk. The select from file dialog doesn't even show them. > > We'll need some way to detect which OS is installed on the image to be able to > create a suitable VM for it and the only way to do that is to use libguesfs but > that takes a looong time to extract information: guestfish. On second thought, we can make some safe guesses at resource allocation and user can customize them if needed. I'll look into this next week.
Created attachment 246205 [details] [review] vm-configurator: Add a sanity check
Created attachment 246206 [details] [review] wizard: Set max-width on summary labels We were setting the label to ellipsize but without setting max-width on labels, this was useless.
Created attachment 246207 [details] [review] os-database: Bump default memory allocation to 1G If recommended/min memory for an OS or OS itself is unknown, lets allocate 1G rather than 500M. 500M is too little for most modern OSs.
Created attachment 246208 [details] [review] installer-media: Exclude extensions from VM name When creating label/VM name from an unknown media, we simply use the basename of the media filename. There is no point in taking the extensions and it usually only makes the name longer and is unlikely to be desired by users.
Created attachment 246209 [details] [review] InstallerMedia now creates VMCreator instance Later we utilize this to create subclasses of InstallerMedia that uses a a custom subclass of VMCreator rather than VMCreator itself.
Created attachment 246210 [details] [review] os-database: Make get_default_resources() public
Created attachment 246211 [details] [review] util: Add is_mime_type() Add utility function to determine if given file is on given mimetype.
Created attachment 246212 [details] [review] vm-configurator: Keep source media path in custom config This will be later useful in case where source media is not going to be part of domain's own config.
Created attachment 246213 [details] [review] installer-media: Make label setup available to subclasses Make setup_label() accessible to subclasses and rename it to label_setup() to avoid conflict with UnattendedInstaller.setup_label.
Created attachment 246214 [details] [review] Add InstalledMedia class A new subclass of InstallerMedia that represents ready-made/installed media in the form of raw or qcow2 images. It also provides means to convert raw image to our native (qcow2) format. This patch adds direct (runtime only) dependency on qemu-img binary.
Created attachment 246215 [details] [review] Add VMImporter class This subclass of VMCreator will be responsible for importing the VM after its creation.
Created attachment 246216 [details] [review] Add support for importing ready-made VMs Currently only qcow2 and raw images are supported. TODO: * Report progress of import
Created attachment 246217 [details] [review] Add support for importing gzip-compression images To reduce download time, images are sometimes compressed by distributors (e.g GNOME OSTree images).
Created attachment 246218 [details] [review] vm-creator: Set thyself on LibvirtMachine soon after creation Set 'vm_creator' property of LibvirtMachine soon after creating it because otherwise its not clear if machine is under construction during the wizard/before launch.
Created attachment 246219 [details] [review] libvirt-machine: Add 'importing' property Add a property to indicate if machine is currently being imported.
Created attachment 246220 [details] [review] libvirt-machine-props,wizard: Hide storage volume when importing When machine is in the process of being imported, hide its storage volume from UI because the actual storage capacity will only be known after import is finished. This actually make me wonder if we should import the media during wizard's 'preparation' step instead, keeping in mind that that we intend to do downloading of medias in there as well.
Created attachment 246221 [details] [review] collection-view: Don't let user launch under-import VMs Launching of such VMs doesn't make any sense.
Created attachment 246222 [details] [review] selectionbar: Don't let user delete under-construction VMs While the ideal solution is probably to make it possible for user to delete under construciton VMs too, it would be pretty difficult to implement with us launching external processes as part of such construction. Keeping in mind also that this only affects imported (from installed media) VMs and importing usually takes much less time than installations, I think this is not bad at least as a temporary work around.
Review of attachment 246216 [details] [review]: I'm looking into displaying spinner on top of under construction boxes. Since we are in a much better position to report actual progress of import (compared to installation), I was hoping to show a progressbar for importing case instead. Something like the first box here: https://github.com/gnome-design-team/gnome-mockups/raw/master/boxes/boxes-install6.png
Review of attachment 246205 [details] [review]: ack
Review of attachment 246206 [details] [review]: Why is this needed? setting a max-width just means we specify some minimal size. Even without this we should be able to resize the window to see more of the strings, no?
Review of attachment 246207 [details] [review]: ack
Review of attachment 246208 [details] [review]: ack
Review of attachment 246209 [details] [review]: ack
Review of attachment 246210 [details] [review]: ack
Review of attachment 246211 [details] [review]: ack
Review of attachment 246212 [details] [review]: I think we never use get_source_media_path() outside the installation phase, which means we don't need the old code for fallback for old VMs without the new media node. Is that right? If so, ack.
Review of attachment 246213 [details] [review]: ack
Review of attachment 246214 [details] [review]: ::: src/installed-media.vala @@ +24,3 @@ + debug ("Failed to guess architecture for media '%s', assuming 'x86_64'", device_file); + + return "x86_64"; Maybe we should assume the current host arch instead? Its kinda painful if it guesses wrong and there is no way to change what it guesses. Like, say you're on i686 and it guesses x86-64 on your i686 VM, then you can't run it and there is no way to override this. If you guess wrong based on the current arch at least you can *try* it, and get a runtime failure later. @@ +32,3 @@ + + public InstalledMedia (string path) throws GLib.Error { + if (!path.has_suffix (".qcow2") && !path.has_suffix (".img")) Can't qemu convert from other formats too? @@ +44,3 @@ + public async void convert_to_native_format () throws GLib.Error { + if (device_file.has_suffix (".qcow2")) + return; Do we maybe want to copy the VM file into boxes? Not sure what semantics we want for this. Use the supplied one at runtime, or use it as a template. Its kinda weird that we have different behaviour depending on the type though.
Review of attachment 246214 [details] [review]: ::: src/installed-media.vala @@ +88,3 @@ + + public override VMCreator get_vm_creator () { + return new VMImporter (this); This needs to come after the patch that introduces VMImporter
Review of attachment 246214 [details] [review]: ::: src/installed-media.vala @@ +44,3 @@ + public async void convert_to_native_format () throws GLib.Error { + if (device_file.has_suffix (".qcow2")) + return; Ah, i see we copy in the other patch. But doesn't it make more sense to convert it to the right place instead?
Review of attachment 246215 [details] [review]: ::: src/vm-importer.vala @@ +43,3 @@ + var destination = File.new_for_path (destination_path); + + yield source.copy_async (destination, FileCopyFlags.OVERWRITE, Priority.DEFAULT, null); This copy is unnecessary if we already made a copy during conversion. I think it makes more sense to pass the target destination to source_media.convert_to_native_format() and have it do the copy if no conversion is needed.
Review of attachment 246216 [details] [review]: ::: src/media-manager.vala @@ +26,3 @@ public async InstallerMedia create_installer_media_for_path (string path, Cancellable? cancellable = null) throws GLib.Error { + var media = is_mime_type (path, "application/x-cd-image") ? Seems clearer to use a normal if() here.
Review of attachment 246217 [details] [review]: ::: src/installed-media.vala @@ +121,3 @@ + do { + length = yield input_stream.read_async (buffer); + output_stream.write (buffer[0:length]); This can do a short write, which will lose data. You can use write_all() to avoid that. Or just use output_stream.splice().
Review of attachment 246218 [details] [review]: ack
Review of attachment 246219 [details] [review]: ack
Review of attachment 246220 [details] [review]: ack
Review of attachment 246221 [details] [review]: ::: src/collection-view.vala @@ +316,3 @@ + // FIXME: Better error message! + var msg = _("Launching of a box that is being imported, is not possible."); + App.app.notificationbar.display_error (msg); Can't we make them insensitive instead in some way? Allowing something to be done only to show an error is stupid.
Review of attachment 246222 [details] [review]: ::: src/selectionbar.vala @@ +162,3 @@ + var sensitive = true; + foreach (var item in App.app.selected_items) { + if (item is LibvirtMachine && (item as LibvirtMachine).vm_creator != null) { This is kinda magic, we should have a machine.deletable property instead. And, we probably need notification on that so we can update the sensitive state when it changes.
Review of attachment 246212 [details] [review]: Not sure I quite understand the latter part of the question but yeah, get_source_media_path() is only used in the context of installation only.
Review of attachment 246214 [details] [review]: ::: src/installed-media.vala @@ +32,3 @@ + + public InstalledMedia (string path) throws GLib.Error { + if (!path.has_suffix (".qcow2") && !path.has_suffix (".img")) actually it also supports virtual box and vmware formats amongst a few others and I was hoping to add support for those after wards. First I'd like to get these patches in.
Review of attachment 246206 [details] [review]: when string are too long, the review page looks really ugly, especially in cases like import where we don't have much details to show. Besides some names don't exactly fit even in fullscreen (as was the case with older (than 2 weeks) gnome ostree images).
Review of attachment 246217 [details] [review]: ::: src/installed-media.vala @@ +121,3 @@ + do { + length = yield input_stream.read_async (buffer); + output_stream.write (buffer[0:length]); write_all() doesn't seem to have an async variant and both splice and write_all will make it hard for me to later add progress reporting (that I was planning for) here. I'll correct this code to take into account the return value of write().
Review of attachment 246221 [details] [review]: ::: src/collection-view.vala @@ +316,3 @@ + // FIXME: Better error message! + var msg = _("Launching of a box that is being imported, is not possible."); + App.app.notificationbar.display_error (msg); I was thinking that with a black thumbnail that we show while importing, it won't be obvious to user that he/she can't click and they'll think its a bug. However, I intend to add indication on top of the thumbnails for box under construction/import so I can just make it insensitive.
Review of attachment 246221 [details] [review]: ::: src/collection-view.vala @@ +316,3 @@ + // FIXME: Better error message! + var msg = _("Launching of a box that is being imported, is not possible."); + App.app.notificationbar.display_error (msg); Hmm.. the only way I see is this very code w/o the error message.
Created attachment 246578 [details] [review] installer-media: Add protected architecture property We keep this new property overridable as later we add subclass of InstallerMedia that can't get the architecture info from Libosinfo. This patch comes just before 'Add InstalledMedia class'.
Created attachment 246579 [details] [review] Add InstalledMedia class * Acted on review * Now also converts even if source media is already in 'qcow2' format, in which case the conversion is simply a copying operation.
Created attachment 246580 [details] [review] Add VMImporter class This subclass of VMCreator will be responsible for importing the VM after its creation.
Created attachment 246581 [details] [review] Add support for importing ready-made VMs Currently only qcow2 and raw images are supported. TODO: * Report progress of import
Created attachment 246582 [details] [review] Add support for importing gzip-compression images To reduce download time, images are sometimes compressed by distributors (e.g GNOME OSTree images).
Created attachment 246583 [details] [review] collection-view: Don't let user launch under-import VMs Launching of such VMs doesn't make any sense.
Created attachment 246584 [details] [review] selectionbar: Don't let user delete under-construction VMs While the ideal solution is probably to make it possible for user to delete under construciton VMs too, it would be pretty difficult to implement with us launching external processes as part of such construction. Keeping in mind also that this only affects imported (from installed media) VMs and importing usually takes much less time than installations, I think this is not bad at least as a temporary work around.
Review of attachment 246212 [details] [review]: ack then
Review of attachment 246206 [details] [review]: Ah, i see what you mean. It still ellipsized before, but only when the window was wery small. You want it ellipsized in order to not look bad.
Review of attachment 246578 [details] [review]: ack
Review of attachment 246579 [details] [review]: ack
Review of attachment 246580 [details] [review]: ack
Review of attachment 246581 [details] [review]: ack
Review of attachment 246582 [details] [review]: ack
Review of attachment 246583 [details] [review]: ack
Review of attachment 246584 [details] [review]: ::: src/selectionbar.vala @@ +167,3 @@ + ulong can_delete_id = 0; + can_delete_id = item.notify["can-delete"].connect (() => { + item.disconnect (can_delete_id); This is a bit iffy. You will create this callback any time the selection changes and one item is not deletable, which will hand around until its finished installing...
Review of attachment 246584 [details] [review]: ::: src/selectionbar.vala @@ +167,3 @@ + ulong can_delete_id = 0; + can_delete_id = item.notify["can-delete"].connect (() => { + item.disconnect (can_delete_id); yes?
Review of attachment 246584 [details] [review]: ::: src/selectionbar.vala @@ +167,3 @@ + ulong can_delete_id = 0; + can_delete_id = item.notify["can-delete"].connect (() => { + item.disconnect (can_delete_id); So, you can queue up arbitrarily many outstanding signal handlers which seems a bit unnecessary. Also it doesn't handle a machine going from can_delete to !can_delete (which can't happen today, but may later). I think we should add this in a bit more generic way so that we can track any kind state changes in all selected items in a robust way.
Created attachment 246819 [details] [review] selectionbar: Don't let user delete under-construction VMs Is this better?
Created attachment 246926 [details] [review] selectionbar: Don't let user delete under-construction VMs Yikes, last version of this patch disabled deletion for boxes under install as well.
Review of attachment 246926 [details] [review]: ack
Attachment 246205 [details] pushed as f18da82 - vm-configurator: Add a sanity check Attachment 246206 [details] pushed as d96fd40 - wizard: Set max-width on summary labels Attachment 246207 [details] pushed as 7e44932 - os-database: Bump default memory allocation to 1G Attachment 246208 [details] pushed as d3e133e - installer-media: Exclude extensions from VM name Attachment 246209 [details] pushed as df95e88 - InstallerMedia now creates VMCreator instance Attachment 246210 [details] pushed as f80e3ac - os-database: Make get_default_resources() public Attachment 246211 [details] pushed as a9e6af5 - util: Add is_mime_type() Attachment 246212 [details] pushed as 33b3a9d - vm-configurator: Keep source media path in custom config Attachment 246213 [details] pushed as 3dfab92 - installer-media: Make label setup available to subclasses Attachment 246218 [details] pushed as 686df6e - vm-creator: Set thyself on LibvirtMachine soon after creation Attachment 246219 [details] pushed as 39cea35 - libvirt-machine: Add 'importing' property Attachment 246220 [details] pushed as 148428b - libvirt-machine-props,wizard: Hide storage volume when importing Attachment 246578 [details] pushed as 0b9da1b - installer-media: Add protected architecture property Attachment 246579 [details] pushed as 8886b47 - Add InstalledMedia class Attachment 246580 [details] pushed as 93308b3 - Add VMImporter class Attachment 246581 [details] pushed as d231c27 - Add support for importing ready-made VMs Attachment 246582 [details] pushed as 2435284 - Add support for importing gzip-compression images Attachment 246583 [details] pushed as 5e27d33 - collection-view: Don't let user launch under-import VMs Attachment 246926 [details] pushed as 99db5ac - selectionbar: Don't let user delete under-construction VMs