9:55 a.m.
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
9:55 a.m.
New subject: [PATCH 2/3] RHEV: Minor cleanups on RHEV shutdown
* 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
10:18 a.m.
New subject: [PATCH 2/3] RHEV: Minor cleanups on RHEV shutdown
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
9:55 a.m.
New subject: [PATCH 3/3] RHEV: Cleanup volumes on unclean shutdown
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
10:24 a.m.
New subject: [PATCH 3/3] RHEV: Cleanup volumes on unclean shutdown
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
10:43 a.m.
New subject: [PATCH 3/3] RHEV: Cleanup volumes on unclean shutdown
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
11 a.m.
New subject: [PATCH 3/3] RHEV: Cleanup volumes on unclean shutdown
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
10:17 a.m.
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
5281
days inactive
5281
days old
7 comments
2 participants
participants (2)
-
Matthew Booth -
Richard W.M. Jones