[PATCH 1/3] Fix RHEV cleanup on unclean shutdown
Cleanup was not happening properly if a migration to RHEV was killed prematurely with a Ctrl-C. Firstly, the SIGINT and SIGQUIT handlers were not being registered early enough in virt-v2v.pl. Secondly, if Ctrl-C killed the guestfs qemu process first it would deliver a SIGPIPE to v2v, which caused an unclean shutdown without cleanup. Fixes RHBZ#596015 --- v2v/virt-v2v.pl | 17 ++++++++++++++--- 1 files changed, 14 insertions(+), 3 deletions(-) diff --git a/v2v/virt-v2v.pl b/v2v/virt-v2v.pl index bed69a0..36297ed 100755 --- a/v2v/virt-v2v.pl +++ b/v2v/virt-v2v.pl @@ -214,6 +214,14 @@ Display version number and exit. =cut +$SIG{'INT'} = \&signal_exit; +$SIG{'QUIT'} = \&signal_exit; + +# SIGPIPE will cause an untidy exit of the perl process, without calling +# destructors. We don't rely on it anywhere, as we check for errors when reading +# from or writing to a pipe. +$SIG{'PIPE'} = 'IGNORE'; + # Initialise the message output prefix Sys::VirtV2V::UserMessage->set_identifier('virt-v2v'); @@ -362,9 +370,6 @@ if ($output_method eq 'rhev') { $> = "0"; } -$SIG{'INT'} = \&close_guest_handle; -$SIG{'QUIT'} = \&close_guest_handle; - # Inspect the guest my $os = inspect_guest($g); @@ -425,6 +430,12 @@ sub close_guest_handle } } +sub signal_exit +{ + close_guest_handle(); + exit(1); +} + sub get_guestfs_handle { my ($storage, $transferiso) = @_; -- 1.7.0.1
* Convert an error message to use warn * Ensure failure to cleanup the mount directory causes non-zero exit status --- lib/Sys/VirtV2V/Target/RHEV.pm | 21 ++++++++++----------- 1 files changed, 10 insertions(+), 11 deletions(-) diff --git a/lib/Sys/VirtV2V/Target/RHEV.pm b/lib/Sys/VirtV2V/Target/RHEV.pm index 65800cd..b80c66a 100644 --- a/lib/Sys/VirtV2V/Target/RHEV.pm +++ b/lib/Sys/VirtV2V/Target/RHEV.pm @@ -476,21 +476,20 @@ sub DESTROY my $eh = Sys::VirtV2V::ExecHelper->run('umount', $self->{mountdir}); if ($eh->status() != 0) { - print STDERR user_message(__x("Failed to unmount {path}. Command ". - "exited with status {status}. Output ". - "was: {output}", - path => $self->{domain_path}, - status => $eh->status(), - output => $eh->output())); + warn user_message(__x("Failed to unmount {path}. Command exited with ". + "status {status}. Output was: {output}", + path => $self->{domain_path}, + status => $eh->status(), + output => $eh->output())); # Exit with an error if the child failed. $retval ||= $eh->status(); } - rmdir($self->{mountdir}) - or print STDERR user_message(__x("Failed to remove mount directory ". - "{dir}: {error}", - dir => $self->{mountdir}, - error => $!)); + unless (rmdir($self->{mountdir})) { + warn user_message(__x("Failed to remove mount directory {dir}: {error}", + dir => $self->{mountdir}, error => $!)); + $retval ||= 1; + } $? = $retval; } -- 1.7.0.1
On Tue, Jun 08, 2010 at 03:55:49PM +0100, Matthew Booth wrote:
* Convert an error message to use warn
ACK
* Ensure failure to cleanup the mount directory causes non-zero exit status
I thought this sometimes happens, and we want to ignore it? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
Currently image files will be left on the export domain if a conversion fails. It isn't possible to delete these through the RHEV UI, requiring an administrator to clean them up manually. This change causes images to be written to a temporary directory on the export domain, and moved to the correct location immediately before writing the OVF file. It also removes this temporary directory automatically on unclean shutdown. This means that a failed conversion shouldn't leave anything on the export domain. Even if it does, for example because of a power failure, the temporary files are clearly separated in their own directory. Fixes RHBZ#596071 --- lib/Sys/VirtV2V/Target/RHEV.pm | 63 ++++++++++++++++++++++++++++++++++++++-- 1 files changed, 60 insertions(+), 3 deletions(-) diff --git a/lib/Sys/VirtV2V/Target/RHEV.pm b/lib/Sys/VirtV2V/Target/RHEV.pm index b80c66a..f7608a2 100644 --- a/lib/Sys/VirtV2V/Target/RHEV.pm +++ b/lib/Sys/VirtV2V/Target/RHEV.pm @@ -160,6 +160,8 @@ sub DESTROY package Sys::VirtV2V::Target::RHEV::Vol; +use File::Path; +use File::Temp qw(tempdir); use POSIX; use Sys::VirtV2V::UserMessage qw(user_message); @@ -167,6 +169,8 @@ use Sys::VirtV2V::UserMessage qw(user_message); use Locale::TextDomain 'virt-v2v'; our %vols_by_path; +our @vols; +our $tmpdir; sub _new { @@ -188,12 +192,25 @@ sub _new $self->{voluuid} = $voluuid; $self->{domainuuid} = $domainuuid; - $self->{dir} = "$mountdir/$domainuuid/images/$imageuuid"; - $self->{path} = $self->{dir}."/$voluuid"; + my $root = "$mountdir/$domainuuid"; + unless (defined($tmpdir)) { + my $nfs = Sys::VirtV2V::Target::RHEV::NFSHelper->new(sub { + print tempdir("v2v.XXXXXXXX", DIR => $root); + }); + my $fromchild = $nfs->{fromchild}; + ($tmpdir) = <$fromchild>; + $nfs->check_exit(); + } + + $self->{dir} = "$root/images/$imageuuid"; + $self->{tmpdir} = "$tmpdir/$imageuuid"; + + $self->{path} = $self->{tmpdir}."/$voluuid"; $self->{creation} = time(); $vols_by_path{$self->{path}} = $self; + push(@vols, $self); return $self; } @@ -263,7 +280,7 @@ sub open $self->{written} = 0; $self->{writer} = Sys::VirtV2V::Target::RHEV::NFSHelper->new(sub { - my $dir = $self->{dir}; + my $dir = $self->{tmpdir}; my $path = $self->{path}; mkdir($dir) @@ -352,6 +369,33 @@ sub close delete($self->{written}); } +sub _move_vols +{ + my $class = shift; + + foreach my $vol (@vols) { + rename($vol->{tmpdir}, $vol->{dir}) + or die(user_message(__x("Unable to move volume from temporary ". + "location {tmpdir} to {dir}", + tmpdir => $vol->{tmpdir}, + dir => $vol->{dir}))); + } + + $class->_cleanup(); +} + +sub _cleanup +{ + my $class = shift; + + return unless (defined($tmpdir)); + + rmtree($tmpdir) or warn(user_message(__x("Unable to remove temporary ". + "directory {dir}", + dir => $tmpdir))); + $tmpdir = undef; +} + package Sys::VirtV2V::Target::RHEV; use File::Temp qw(tempdir); @@ -474,6 +518,17 @@ sub DESTROY # status. my $retval = $?; + eval { + my $nfs = Sys::VirtV2V::Target::RHEV::NFSHelper->new(sub { + Sys::VirtV2V::Target::RHEV::Vol->_cleanup(); + }); + $nfs->check_exit(); + }; + if ($@) { + warn($@); + $retval ||= 1; + } + my $eh = Sys::VirtV2V::ExecHelper->run('umount', $self->{mountdir}); if ($eh->status() != 0) { warn user_message(__x("Failed to unmount {path}. Command exited with ". @@ -669,6 +724,8 @@ EOF dir => $dir, error => $!))); + return Sys::VirtV2V::Target::RHEV::Vol->_move_vols(); + my $vm; my $ovfpath = $dir.'/'.$vmuuid.'.ovf'; open($vm, '>', $ovfpath) -- 1.7.0.1
On Tue, Jun 08, 2010 at 03:55:50PM +0100, Matthew Booth wrote:
Currently image files will be left on the export domain if a conversion fails. It isn't possible to delete these through the RHEV UI, requiring an administrator to clean them up manually.
This change causes images to be written to a temporary directory on the export domain, and moved to the correct location immediately before writing the OVF file. It also removes this temporary directory automatically on unclean shutdown. This means that a failed conversion shouldn't leave anything on the export domain. Even if it does, for example because of a power failure, the temporary files are clearly separated in their own directory.
The patch is fairly confusing because of the use of NFSHelper, but I have two potential concerns: (a) I don't think Perl 'rename' function will work if the temporary directory is on a different filesystem from the final destination. You might be better off using the 'mv' shell command (possibly with suitable shell quoting) instead. (b) Where is $tmpdir typically? If it's on /tmp you might find you have limited space, eg. if /tmp is a ramdisk. (We have the same problem in libguestfs, and people can override it by setting $TMPDIR). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
On 08/06/10 16:24, Richard W.M. Jones wrote:
On Tue, Jun 08, 2010 at 03:55:50PM +0100, Matthew Booth wrote:
Currently image files will be left on the export domain if a conversion fails. It isn't possible to delete these through the RHEV UI, requiring an administrator to clean them up manually.
This change causes images to be written to a temporary directory on the export domain, and moved to the correct location immediately before writing the OVF file. It also removes this temporary directory automatically on unclean shutdown. This means that a failed conversion shouldn't leave anything on the export domain. Even if it does, for example because of a power failure, the temporary files are clearly separated in their own directory.
The patch is fairly confusing because of the use of NFSHelper, but I have two potential concerns:
NFSHelper is unfortunate. As is NFS...
(a) I don't think Perl 'rename' function will work if the temporary directory is on a different filesystem from the final destination. You might be better off using the 'mv' shell command (possibly with suitable shell quoting) instead.
This is ok, because the temporary directory is explicitly created on the export storage domain, so it won't cross a filesystem boundary. I do this specifically to avoid an additional data copy. + my $root = "$mountdir/$domainuuid"; $root is the top level directory of the export storage domain. Because it's a subdirectory of $mountdir, it's on NFS. + unless (defined($tmpdir)) { + my $nfs = Sys::VirtV2V::Target::RHEV::NFSHelper->new(sub { + print tempdir("v2v.XXXXXXXX", DIR => $root); + }); The NFS helper creates a new temporary directory will the given template under $root, so is also on NFS. NFSHelper works with stdout, so it prints the result. + my $fromchild = $nfs->{fromchild}; + ($tmpdir) = <$fromchild>; $tmpdir is the first line of output from the child. + $nfs->check_exit(); check_exit() will die() with error output if there was any error.
(b) Where is $tmpdir typically? If it's on /tmp you might find you have limited space, eg. if /tmp is a ramdisk. (We have the same problem in libguestfs, and people can override it by setting $TMPDIR).
See above. The location of tmpdir is chosen deliberately. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
On Tue, Jun 08, 2010 at 04:43:06PM +0100, Matthew Booth wrote:
On 08/06/10 16:24, Richard W.M. Jones wrote:
(a) I don't think Perl 'rename' function will work if the temporary directory is on a different filesystem from the final destination. You might be better off using the 'mv' shell command (possibly with suitable shell quoting) instead.
This is ok, because the temporary directory is explicitly created on the export storage domain, so it won't cross a filesystem boundary. I do this specifically to avoid an additional data copy.
+ my $root = "$mountdir/$domainuuid";
$root is the top level directory of the export storage domain. Because it's a subdirectory of $mountdir, it's on NFS.
+ unless (defined($tmpdir)) { + my $nfs = Sys::VirtV2V::Target::RHEV::NFSHelper->new(sub { + print tempdir("v2v.XXXXXXXX", DIR => $root); + });
The NFS helper creates a new temporary directory will the given template under $root, so is also on NFS. NFSHelper works with stdout, so it prints the result.
+ my $fromchild = $nfs->{fromchild}; + ($tmpdir) = <$fromchild>;
$tmpdir is the first line of output from the child.
+ $nfs->check_exit();
check_exit() will die() with error output if there was any error.
Right, I understand it now. ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
On Tue, Jun 08, 2010 at 03:55:48PM +0100, Matthew Booth wrote:
Cleanup was not happening properly if a migration to RHEV was killed prematurely with a Ctrl-C. Firstly, the SIGINT and SIGQUIT handlers were not being registered early enough in virt-v2v.pl. Secondly, if Ctrl-C killed the guestfs qemu process first it would deliver a SIGPIPE to v2v, which caused an unclean shutdown without cleanup.
Yes, looks like a correct use of signal handlers and exit (so the atexit handler is called inside libguestfs), so ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
participants (2)
-
Matthew Booth -
Richard W.M. Jones