Libguestfs - Jun 2010 - [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

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: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