GNOME Bugzilla – Bug 678900
express installation files not unique enough
Last modified: 2016-03-31 13:54:45 UTC
Currently we name the various (cached) express installation files/directories based on the OS name or media path. This isn't a reliable way to guarantee uniqueness at all so we are pretty much screwed if user installs same OS at the same time or even when the same OS was previous installed but for a different architecture. So we need to somehow make them more unique.
Created attachment 217328 [details] [review] express: Create uniquely named unattended files Create uniquely named unattended files (disk image, kernel, initrd and other temporary files/directories) for each VM installation.
Created attachment 217329 [details] [review] Rename 'domainname' parameter to 'vm_name' This better reflects what this parameter really is.
Review of attachment 217329 [details] [review]: Part of this patch should be squashed in the previous one to avoid adding a 'hostname' argument to some functions in the first patch, and then renaming them here.
Review of attachment 217328 [details] [review]: ::: src/fedora-installer.vala @@ +117,3 @@ } + private async void mount_media (string hostname, Cancellable? cancellable) throws GLib.Error { Passing hostname all over the place feels conterproductive, I'd just add this as a member of UnattendedInstall.. Why not directly use the more descriptive name from next patch when you are adding a new argument? ::: src/unattended-installer.vala @@ +278,3 @@ + + return; + } Why reuse existing images if one exists? Shouldn't we overwrite it with the newer one?
(In reply to comment #4) > Review of attachment 217328 [details] [review]: > > ::: src/fedora-installer.vala > @@ +117,3 @@ > } > > + private async void mount_media (string hostname, Cancellable? cancellable) > throws GLib.Error { > > Passing hostname all over the place feels conterproductive, I'd just add this > as a member of UnattendedInstall.. We already discussed this IIRC and since the code remains the same, I didn't think we reached a verdict there. > Why not directly use the more descriptive name from next patch when you are > adding a new argument? You mean 'vm_name', its basically the same thing, just that that patch came after. > ::: src/unattended-installer.vala > @@ +278,3 @@ > + > + return; > + } > > Why reuse existing images if one exists? Shouldn't we overwrite it with the > newer one? We overwrite the content anyway so there is no point in recreating the image again.
(In reply to comment #5) > > ::: src/unattended-installer.vala > > @@ +278,3 @@ > > + > > + return; > > + } > > > > Why reuse existing images if one exists? Shouldn't we overwrite it with the > > newer one? > > We overwrite the content anyway so there is no point in recreating the image > again. Also thats what the existing code was doing so just didn't see the point of changing that.
(In reply to comment #5) > > Why not directly use the more descriptive name from next patch when you are > > adding a new argument? > > You mean 'vm_name', its basically the same thing, just that that patch came > after. You should use vm_name directly in that patch when you add a new argument instead of adding the argument with a non-descriptive name in this patch and then renaming it right away. Less noise.
(In reply to comment #6) > Also thats what the existing code was doing so just didn't see the point of > changing that. Not really relevant what the existing code is doing since the point of this patch is that the existing code was wrong. We are switching from a situation where it's expected that the file exists, to a situation where it's unexpected that it exists.
(In reply to comment #5) > We overwrite the content anyway so there is no point in recreating the image > again. Better to recreate it, there may be files that needs to be removed, not overwritten.
(In reply to comment #8) > (In reply to comment #6) > > Also thats what the existing code was doing so just didn't see the point of > > changing that. > > Not really relevant what the existing code is doing since the point of this > patch is that the existing code was wrong. We are switching from a situation > where it's expected that the file exists, to a situation where it's unexpected > that it exists. The point of the these changes is to create unique files names for each domain, nothing more. We shouldn't unnecessarily add I/O operations to an already slow process of setting-up/creating VM.
(In reply to comment #9) > (In reply to comment #5) > > > We overwrite the content anyway so there is no point in recreating the image > > again. > > Better to recreate it, there may be files that needs to be removed, not > overwritten. Whats the usescase in Boxes? We don't put any files that needs removal.
A(In reply to comment #11) > Whats the usescase in Boxes? We don't put any files that needs removal. We don't put any files *now* that needs removal. I don't own a crystal ball and prefer code to be future proof. And the automatic windows driver installation code will add more files.
(In reply to comment #12) > A(In reply to comment #11) > > Whats the usescase in Boxes? We don't put any files that needs removal. > > We don't put any files *now* that needs removal. I don't own a crystal ball and > prefer code to be future proof. > And the automatic windows driver installation code will add more files. I really dont prefer to add potentially slow operations to the code to address a purely hypothetical issue just to be future safe.
(In reply to comment #13) > (In reply to comment #12) > > A(In reply to comment #11) > > > Whats the usescase in Boxes? We don't put any files that needs removal. > > > > We don't put any files *now* that needs removal. I don't own a crystal ball and > > prefer code to be future proof. > > And the automatic windows driver installation code will add more files. > > I really dont prefer to add potentially slow operations to the code to address > a purely hypothetical issue just to be future safe. But its one operation so if it makes you feel better, I'll just do it..
Created attachment 217554 [details] [review] Rename 'hostname' parameter to 'vm_name'
Created attachment 217555 [details] [review] express: Create uniquely named unattended files
I believe my latest patches addresses your concerns, at least the ones I promise to address. :)
Comment on attachment 217555 [details] [review] express: Create uniquely named unattended files >+ protected override async void prepare_direct_boot (string vm_name, Cancellable? cancellable) throws GLib.Error { >+ yield mount_media (vm_name, cancellable); >+ yield extract_boot_files (vm_name, cancellable); >+ private async void mount_media (string vm_name, Cancellable? cancellable) throws GLib.Error { >+ mount_point = get_user_unattended (vm_name); >+ private async void extract_boot_files (string vm_name, Cancellable? cancellable) throws GLib.Error { >+ kernel_path = get_user_unattended (vm_name + "-kernel"); >+ initrd_path = get_user_unattended (vm_name + "-initrd"); >+ yield create_disk_image (vm_name, cancellable); >+ yield prepare_direct_boot (vm_name, cancellable); >+ protected virtual async void prepare_direct_boot (string vm_name, Cancellable? cancellable) throws GLib.Error {} >+ private async void create_disk_image (string vm_name, Cancellable? cancellable) throws GLib.Error { This is just getting silly, let's add UnattendedInstaller::vm_name once and for all, and stop passing it around everywhere. We are not passing username or password around, nor are we passing around the timezone or the language, there is no reason for vm_name/hostname to be passed around like that.
Comment on attachment 217554 [details] [review] Rename 'hostname' parameter to 'vm_name' Looks good, but let's addressed the issue raised in the other patch first.
Created attachment 217980 [details] [review] express: Create uniquely named unattended files Create uniquely named unattended files (disk image, kernel, initrd and other temporary files/directories) for each VM installation.
Review of attachment 217980 [details] [review]: Looks good ::: src/unattended-installer.vala @@ +36,3 @@ public DataStreamNewlineType newline_type; + public File? disk_file; + public string hostname; I wouldn't move 'hostname' from its previous place @@ -128,3 @@ try { - if (yield unattended_disk_exists (cancellable)) - debug ("Found previously created unattended disk image for '%s', re-using..", os.short_id); I think I'd keep this log so that we have a trace if something really unexpected happens. @@ +284,1 @@ debug ("Floppy image for unattended installation created at '%s'", disk_path); If copy_async throws, we'll have disk_file set but no corresponding file on disk, the previous code was better thanks to the 'created_disk' variable. Maybe this should be wrapped in a try/catch which sets disk_file to null if this fails and rethrows?
(In reply to comment #21) > Review of attachment 217980 [details] [review]: > > Looks good > > ::: src/unattended-installer.vala > @@ +36,3 @@ > public DataStreamNewlineType newline_type; > + public File? disk_file; > + public string hostname; > > I wouldn't move 'hostname' from its previous place I would, to keep private, protected and public fields together. > @@ -128,3 @@ > try { > - if (yield unattended_disk_exists (cancellable)) > - debug ("Found previously created unattended disk image for > '%s', re-using..", os.short_id); > > I think I'd keep this log so that we have a trace if something really > unexpected happens. We now overwrite the file so I don't see why we should keep this check function. The disk file creation/writing can fail for many reasons and we should and will handle it the same way as any other errors IMO. > @@ +284,1 @@ > debug ("Floppy image for unattended installation created at '%s'", > disk_path); > > If copy_async throws, we'll have disk_file set but no corresponding file on > disk, the previous code was better thanks to the 'created_disk' variable. Maybe > this should be wrapped in a try/catch which sets disk_file to null if this > fails and rethrows? Yes, that should fix that.
(In reply to comment #22) > (In reply to comment #21) > > Review of attachment 217980 [details] [review] [details]: > > > > Looks good > > > > ::: src/unattended-installer.vala > > @@ +36,3 @@ > > public DataStreamNewlineType newline_type; > > + public File? disk_file; > > + public string hostname; > > > > I wouldn't move 'hostname' from its previous place > > I would, to keep private, protected and public fields together. Ah, didn't pay attention that it was changed from protected to public. > > > @@ -128,3 @@ > > try { > > - if (yield unattended_disk_exists (cancellable)) > > - debug ("Found previously created unattended disk image for > > '%s', re-using..", os.short_id); > > > > I think I'd keep this log so that we have a trace if something really > > unexpected happens. > > We now overwrite the file so I don't see why we should keep this check > function. This is a situation that should never happen, so having a trace that this happened may be helful if this happens and causes a bug. Actually, I don't think we need predictable names in all of this, so why don't we generate truly unique names with g_mkstemp and use that instead of building a should-be-unique name using the hostname? This would eliminate the need to make 'hostname' public I think, this would also address my question about logging when we overwrite an existing file.
(In reply to comment #23) > why don't we generate truly unique names with g_mkstemp Not sure g_mkstemp is the best choice since it uses g_get_tmp_dir () but you get the idea
(In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > Review of attachment 217980 [details] [review] [details] [details]: > > > > > > Looks good > > > > > > ::: src/unattended-installer.vala > > > @@ +36,3 @@ > > > public DataStreamNewlineType newline_type; > > > + public File? disk_file; > > > + public string hostname; > > > > > > I wouldn't move 'hostname' from its previous place > > > > I would, to keep private, protected and public fields together. > > Ah, didn't pay attention that it was changed from protected to public. > > > > > > @@ -128,3 @@ > > > try { > > > - if (yield unattended_disk_exists (cancellable)) > > > - debug ("Found previously created unattended disk image for > > > '%s', re-using..", os.short_id); > > > > > > I think I'd keep this log so that we have a trace if something really > > > unexpected happens. > > > > We now overwrite the file so I don't see why we should keep this check > > function. > > This is a situation that should never happen, so having a trace that this > happened may be helful if this happens and causes a bug. So is all the other errors. That log being removed was not telling of some error. We'll get a warning for all of them. > Actually, I don't think we need predictable names in all of this, so why don't > we generate truly unique names with g_mkstemp and use that instead of building > a should-be-unique name using the hostname? This would eliminate the need to > make 'hostname' public I think, this would also address my question about > logging when we overwrite an existing file. We don't need 'truely' unique names at all, just need to ensure each domain gets its own unattended files.
Created attachment 218026 [details] [review] express: Create uniquely named unattended files This version makes use of Util.delete_file() to not error out if disk_file was already removed.
(In reply to comment #22) > (In reply to comment #21) > > Review of attachment 217980 [details] [review] [details]: > > @@ +284,1 @@ > > debug ("Floppy image for unattended installation created at '%s'", > > disk_path); > > > > If copy_async throws, we'll have disk_file set but no corresponding file on > > disk, the previous code was better thanks to the 'created_disk' variable. Maybe > > this should be wrapped in a try/catch which sets disk_file to null if this > > fails and rethrows? > > Yes, that should fix that. I meant "I should fix that" but anyways, I fixed it another way: If disk_file was initialized but file creation failed for some reason, we'll now not error out on deletion but rather just spit a debug log on console..
Created attachment 218027 [details] [review] express: Create uniquely named unattended files Forgot to unset disk_file in the previous patch..
(In reply to comment #25) > > We don't need 'truely' unique names at all, just need to ensure each domain > gets its own unattended files. Yeah, and the day the user can change the hostname to something not unique through templates, configuration, ... we'll have to hunt down mysterious failures when during several installs at the same time, and rework this code because we were lazy and used something vaguely unique.
(In reply to comment #25) > So is all the other errors. That log being removed was not telling of some > error. We'll get a warning for all of them. We strongly assume the filename we build to be unique, so if there's already a filename with that name that's an indication of something really unexpected going on. And we are silently overwriting the file right after, so no error whatsoever because of this. Having this log is just being nice to your future self who will have to figure out what is going on with autoinstall not working on someone else's machine...
Review of attachment 218027 [details] [review]: ::: src/unattended-installer.vala @@ +36,3 @@ public DataStreamNewlineType newline_type; + public File? disk_file; + public string hostname; The patch is okayish except for making hostname public, UnattendedInstall can do without it.
(In reply to comment #29) > (In reply to comment #25) > > > > We don't need 'truely' unique names at all, just need to ensure each domain > > gets its own unattended files. > > Yeah, and the day the user can change the hostname to something not unique > through templates, configuration, ... we'll have to hunt down mysterious > failures when during several installs at the same time, and rework this code > because we were lazy and used something vaguely unique. What the hell are you talking about? This hostname was just generated in memory from the vm name, the user can't possible change it from some template or configuration. In case I didn't explain already, there is *no* reason to ensure global uniqueness. Since we are basing it on domain name, the only reason we come-up with the same name as before on the same host is that previously created domain was deleted. I seriously doubt we can get into problems by overwriting files of a deleted domain.
(In reply to comment #31) > Review of attachment 218027 [details] [review]: > > ::: src/unattended-installer.vala > @@ +36,3 @@ > public DataStreamNewlineType newline_type; > + public File? disk_file; > + public string hostname; > > The patch is okayish except for making hostname public, UnattendedInstall can > do without it. Ah yes, leftover from before rebasing on your hostname change.
(In reply to comment #33) > (In reply to comment #31) > > Review of attachment 218027 [details] [review] [details]: > > > > ::: src/unattended-installer.vala > > @@ +36,3 @@ > > public DataStreamNewlineType newline_type; > > + public File? disk_file; > > + public string hostname; > > > > The patch is okayish except for making hostname public, UnattendedInstall can > > do without it. > > Ah yes, leftover from before rebasing on your hostname change. Spoke too soon, its accessed by UnattendedFile subclasses in this patch.
(In reply to comment #32) > What the hell are you talking about? "the day the user can change the hostname" -> potential future
(In reply to comment #34) > Spoke too soon, its accessed by UnattendedFile subclasses in this patch. I know, but it has no reason to.
(In reply to comment #35) > (In reply to comment #32) > > What the hell are you talking about? > > "the day the user can change the hostname" -> potential future Very unlikely that we offer such an option before creation since all we are doing is giving it a sane default and user is free to change it later from the guest. Even if we provide that option, we will also then want to ensure uniqueness somehow. If it makes you happy, I can make the UnattendedInstaller only keep vm_name that was passed to it and hostname could be generated where its needed from that. Then filenames will be directly based on vm_name and any modification to hostname related code won't affect filenames. (In reply to comment #36) > (In reply to comment #34) > > Spoke too soon, its accessed by UnattendedFile subclasses in this patch. > > I know, but it has no reason to. You mean if filenames are not based on hostname?
(In reply to comment #37) > If it makes you happy, I can make the UnattendedInstaller only keep vm_name > that was passed to it and hostname could be generated where its needed from > that. Then filenames will be directly based on vm_name and any modification to > hostname related code won't affect filenames. > Don't bother, not that important right now. > (In reply to comment #36) > > (In reply to comment #34) > > > Spoke too soon, its accessed by UnattendedFile subclasses in this patch. > > > > I know, but it has no reason to. > > You mean if filenames are not based on hostname? Yes, I don't think the files have to be derived from hostname, they just need to be unique.
Review of attachment 218027 [details] [review]: ::: src/fedora-installer.vala @@ +148,3 @@ return; + kernel_path = get_user_unattended (hostname + "-kernel"); it would make for simpler code to have get_user_unattended ("kernel") so that any future location or path change would be done in only one place. I would make move it to a UnattendedInstaller.get_user_unattended ()
Created attachment 218226 [details] [review] express: Create uniquely named unattended files New version: - UnattendedInstaller.get_user_unattended() - Put all the unattended files under XDG_CACHE_HOME directory
Review of attachment 218226 [details] [review]: looks good to me
Attachment 218226 [details] pushed as 1da63b8 - express: Create uniquely named unattended files