Libguestfs - Feb 2010 - [PATCH 1/2] Fix remapping of block devices

We were assuming that guests wouldn't use multiple interfaces, or if they
did,
drive letters wouldn't clash between the interfaces. An ESX guest with a
CDROM
device breaks this because hard disks will be presented a SCSI, starting at sda,
and CDROM devices are presented as IDE, starting at hda. This is in contrast to
QEMU, which starts at hdc by default.

Firstly, this change causes disks and CDROM devices to be treated differently.
CDROM devices are changed to use IDE, regardless of what interface they were
using before. This also limits the maximum number of CDROM devices in a guest to
4. CDROM devices after 4 will be dropped, and a warning emitted.

Secondly, it is smarter about renaming IDE and SCSI devices, which may be merged
in the guest configuration.

Thirdly, it renames devices consistently based on the order they are listed in
the domain XML.
---
 lib/Sys/VirtV2V/Connection.pm      |   40 ++++++++---
 lib/Sys/VirtV2V/Converter.pm       |   86 +++++++++++++++++++-----
 lib/Sys/VirtV2V/Converter/Linux.pm |   41 ++---------
 lib/Sys/VirtV2V/GuestOS/RedHat.pm  |  132 ++++++++++++++++++++++++++++++------
 v2v/virt-v2v.pl                    |    9 ++-
 5 files changed, 222 insertions(+), 86 deletions(-)

diff --git a/lib/Sys/VirtV2V/Connection.pm b/lib/Sys/VirtV2V/Connection.pm
index 501772b..46bd020 100644
--- a/lib/Sys/VirtV2V/Connection.pm
+++ b/lib/Sys/VirtV2V/Connection.pm
@@ -38,7 +38,8 @@ Sys::VirtV2V::Connection - Obtain domain metadata
 
  $conn = Sys::VirtV2V::Connection::LibVirt->new($uri, $name, $pool);
  $dom = $conn->get_dom();
- @storage = $conn->get_local_storage();
+ $storage = $conn->get_storage_paths();
+ $devices = $conn->get_storage_devices();
 
 =head1 DESCRIPTION
 
@@ -53,18 +54,33 @@ directly. Use one of the subclasses:
 
 =over
 
-=item get_local_storage
+=item get_storage_paths
 
-Return a list of the domain's storage devices. The returned list contains
local
-paths.
+Return an arrayref of local paths to the guest's storage devices. This list
is
+guaranteed to be in the same order as the list returned by get_storage_devices.
 
 =cut
 
-sub get_local_storage
+sub get_storage_paths
 {
     my $self = shift;
 
-    return @{$self->{storage}};
+    return $self->{paths};
+}
+
+=item get_storage_devices
+
+Return an arrayref of libvirt device names for the guest's storage prior to
+conversion. This list is guaranteed to be in the same order as the list
returned
+by get_storage_paths.
+
+=cut
+
+sub get_storage_devices
+{
+    my $self = shift;
+
+    return $self->{devices};
 }
 
 =item get_dom()
@@ -94,8 +110,10 @@ sub _storage_iterate
 
     my $dom = $self->get_dom();
 
-    # Create a hash of guest devices to their paths
-    my @storage;
+    # An list of local paths to guest storage
+    my @paths;
+    # A list of libvirt target device names
+    my @devices;
     foreach my $disk ($dom->findnodes('/domain/devices/disk')) {
         my ($source_e) = $disk->findnodes('source');
 
@@ -167,11 +185,13 @@ sub _storage_iterate
                 }
             }
 
-            push(@storage, $path);
+            push(@paths, $path);
+            push(@devices, $target->getNodeValue());
         }
     }
 
-    $self->{storage} = \@storage;
+    $self->{paths} = \@paths;
+    $self->{devices} = \@devices;
 }
 
 =back
diff --git a/lib/Sys/VirtV2V/Converter.pm b/lib/Sys/VirtV2V/Converter.pm
index cba6db8..a306103 100644
--- a/lib/Sys/VirtV2V/Converter.pm
+++ b/lib/Sys/VirtV2V/Converter.pm
@@ -20,6 +20,8 @@ package Sys::VirtV2V::Converter;
 use strict;
 use warnings;
 
+use Carp;
+
 use Module::Pluggable sub_name => 'modules',
                       search_path => ['Sys::VirtV2V::Converter'],
                       require => 1;
@@ -40,7 +42,7 @@ Sys::VirtV2V::Converter - Convert a guest to run on KVM
  use Sys::VirtV2V::Converter;
 
  my $guestos = Sys::VirtV2V::GuestOS->new($g, $os, $dom, $config);
- Sys::VirtV2V::Converter->convert($vmm, $guestos, $config, $dom, $os);
+ Sys::VirtV2V::Converter->convert($vmm, $guestos, $config, $dom, $os,
$devices);
 
 =head1 DESCRIPTION
 
@@ -122,6 +124,11 @@ An XML::DOM object resulting from parsing the guests's
libvirt domain XML.
 
 The OS description returned by Sys::Guestfs::Lib.
 
+=item devices
+
+An arrayref of libvirt storage device names, in the order they will be
presented
+to the guest.
+
 =back
 
 =cut
@@ -130,19 +137,20 @@ sub convert
 {
     my $class = shift;
 
-    my ($vmm, $guestos, $config, $dom, $desc) = @_;
+    my ($vmm, $guestos, $config, $dom, $desc, $devices) = @_;
     carp("convert called without vmm argument") unless defined($vmm);
     carp("convert called without guestos argument") unless
defined($guestos);
     carp("convert called without config argument") unless
defined($config);
     carp("convert called without dom argument") unless defined($dom);
     carp("convert called without desc argument") unless
defined($desc);
+    carp("convert called without devices argument") unless
defined($devices);
 
     my $guestcaps;
 
     # Find a module which can convert the guest and run it
     foreach my $module ($class->modules()) {
         if($module->can_handle($desc)) {
-            $guestcaps = $module->convert($vmm, $guestos, $dom, $desc);
+            $guestcaps = $module->convert($vmm, $guestos, $desc, $devices);
             last;
         }
     }
@@ -154,7 +162,7 @@ sub convert
     _map_networks($dom, $config);
 
     # Convert the metadata
-    _convert_metadata($vmm, $dom, $desc, $guestcaps);
+    _convert_metadata($vmm, $dom, $desc, $devices, $guestcaps);
 
     my ($name) = $dom->findnodes('/domain/name/text()');
     $name = $name->getNodeValue();
@@ -170,7 +178,7 @@ sub convert
 
 sub _convert_metadata
 {
-    my ($vmm, $dom, $desc, $guestcaps) = @_;
+    my ($vmm, $dom, $desc, $devices, $guestcaps) = @_;
 
     my $arch   = $guestcaps->{arch};
     my $virtio = $guestcaps->{virtio};
@@ -191,8 +199,11 @@ sub _convert_metadata
     # Remove any configuration related to a PV kernel bootloader
     _unconfigure_bootloaders($dom);
 
-    # Configure network and block drivers
-    _configure_drivers($dom, $virtio);
+    # Update storage devices and drivers
+    _configure_storage($dom, $devices, $virtio);
+
+    # Configure network drivers
+    _configure_network($dom, $virtio);
 
     # Ensure guest has a standard set of default devices
     _configure_default_devices($dom, $default_dom);
@@ -383,24 +394,65 @@ sub _unconfigure_bootloaders
     }
 }
 
-sub _configure_drivers
+sub _configure_storage
 {
-    my ($dom, $virtio) = @_;
+    my ($dom, $devices, $virtio) = @_;
 
-    # Convert disks
-    # N.B. <disk> is required to have a <target> element
+    my $prefix = $virtio ? 'vd' : 'sd';
 
-    # Convert alternate bus specifications
-    foreach my $bus
($dom->findnodes('/domain/devices/disk/target/@bus')) {
-        $bus->setNodeValue($virtio ? 'virtio' : 'scsi');
+    my $suffix = 'a';
+    foreach my $device (@$devices) {
+        my ($target) =
$dom->findnodes("/domain/devices/disk[\@device='disk']/".
+                                      
"target[\@dev='$device']");
+
+        die(user_message(__x("Previously detected drive {drive} is no
longer ".
+                             "present in domain XML: {xml}",
+                             drive => $device,
+                             xml => $dom->toString())))
+            unless (defined($target));
+
+        $target->setAttribute('bus', $virtio ? 'virtio' :
'scsi');
+        $target->setAttribute('dev', $prefix.$suffix);
+        $suffix++; # Perl magic means 'z'++ == 'aa'
     }
 
-    # Add an explicit bus specification to targets without one
+    # Convert the first 4 CDROM drives to IDE, and remove the rest
+    $suffix = 'a';
+    my $i = 0;
+    my @removed = ();
     foreach my $target
-        ($dom->findnodes('/domain/devices/disk/target[not(@bus)]'))
+       
($dom->findnodes("/domain/devices/disk[\@device='cdrom']/target"))
     {
-        $target->setAttribute('bus', $virtio ? 'virtio' :
'scsi');
+        if ($i < 4) {
+            $target->setAttribute('bus', 'ide');
+            $target->setAttribute('dev', "hd$suffix");
+            $suffix++;
+        } else {
+            push(@removed, $target->getAttribute('dev'));
+
+            my $disk = $target->getParentNode();
+            $disk->getParentNode()->removeChild($disk);
+        }
+        $i++;
+    }
+
+    if (@removed > 0) {
+        print user_message(__x("WARNING: Only 4 CDROM drives are
supported. ".
+                               "The following CDROM drives have been
removed: ".
+                               "{list}",
+                               list => join(' ', @removed)));
+    }
+
+    # As we just changed and unified all their underlying controllers, device
+    # addresses are no longer relevant
+    foreach my $address
($dom->findnodes('/domain/devices/disk/address')) {
+        $address->getParentNode()->removeChild($address);
     }
+}
+
+sub _configure_network
+{
+    my ($dom, $virtio) = @_;
 
     # Convert network adapters
     # N.B. <interface> is not required to have a <model> element,
but <model>
diff --git a/lib/Sys/VirtV2V/Converter/Linux.pm
b/lib/Sys/VirtV2V/Converter/Linux.pm
index 1186430..c10c963 100644
--- a/lib/Sys/VirtV2V/Converter/Linux.pm
+++ b/lib/Sys/VirtV2V/Converter/Linux.pm
@@ -83,14 +83,15 @@ A Sys::Virt handle to the target libvirt.
 
 An initialised Sys::VirtV2V::GuestOS for manipulating the guest OS>.
 
-=item dom
-
-A parsed XML::DOM of the guest's libvirt domain XML prior to conversion.
-
 =item desc
 
 A description of the guest OS as returned by Sys::Guestfs::Lib.
 
+=item devices
+
+An arrayref of libvirt storage device names, in the order they will be
presented
+to the guest.
+
 =back
 
 =cut
@@ -99,11 +100,11 @@ sub convert
 {
     my $class = shift;
 
-    my ($vmm, $guestos, $dom, $desc) = @_;
+    my ($vmm, $guestos, $desc, $devices) = @_;
     carp("convert called without vmm argument") unless defined($vmm);
     carp("convert called without guestos argument") unless
defined($guestos);
-    carp("convert called without dom argument") unless defined($dom);
     carp("convert called without desc argument") unless
defined($desc);
+    carp("convert called without devices argument") unless
defined($devices);
 
     # Un-configure HV specific attributes which don't require a direct
     # replacement
@@ -117,7 +118,7 @@ sub convert
 
     # Configure the rest of the system
     _configure_display_driver($guestos, $virtio);
-    _remap_block_devices($guestos, $dom, $desc, $virtio);
+    $guestos->remap_block_devices($devices, $virtio);
     _configure_kernel_modules($guestos, $desc, $virtio);
     _configure_boot($guestos, $kernel, $virtio);
 
@@ -130,32 +131,6 @@ sub convert
     return \%guestcaps;
 }
 
-sub _remap_block_devices
-{
-    my ($guestos, $dom, $desc, $virtio) = @_;
-
-    my %map = ();
-
-    # Prefix is vd for virtio or sd for scsi
-    my $prefix = $virtio ? 'vd' : 'sd';
-
-    # Look for devices specified in the device metadata
-    foreach my $dev
($dom->findnodes('/domain/devices/disk/target/@dev')) {
-        if($dev->getNodeValue() =~ m{^(sd|hd|xvd)([a-z]+)\d*$}) {
-            $map{"$1$2"} = "$prefix$2";
-
-            # A guest might present an IDE disk as SCSI
-            if($1 eq 'hd') {
-                $map{"sd$2"} = "$prefix$2";
-            }
-
-            $dev->setNodeValue("$prefix$2");
-        }
-    }
-
-    $guestos->remap_block_devices(\%map);
-}
-
 sub _configure_kernel_modules
 {
     my ($guestos, $desc, $virtio) = @_;
diff --git a/lib/Sys/VirtV2V/GuestOS/RedHat.pm
b/lib/Sys/VirtV2V/GuestOS/RedHat.pm
index 9149cfc..6adf8b8 100644
--- a/lib/Sys/VirtV2V/GuestOS/RedHat.pm
+++ b/lib/Sys/VirtV2V/GuestOS/RedHat.pm
@@ -944,7 +944,7 @@ sub _ensure_transfer_mounted
     $g->mount_ro($transfer, $self->{transfer_mount});
 }
 
-=item remap_block_devices(map)
+=item remap_block_devices(devices, virtio)
 
 See BACKEND INTERFACE in L<Sys::VirtV2V::GuestOS> for details.
 
@@ -953,32 +953,120 @@ See BACKEND INTERFACE in L<Sys::VirtV2V::GuestOS>
for details.
 sub remap_block_devices
 {
     my $self = shift;
-    my ($map) = @_;
+    my ($devices, $virtio) = @_;
 
-    my $g = $self->{g};
+    my $g    = $self->{g};
+    my $desc = $self->{desc};
 
-    # Iterate over fstab. Any entries with a spec in the the map, replace them
-    # with their mapped values
-    eval {
-        foreach my $spec ($g->aug_match('/files/etc/fstab/*/spec'))
{
-            my $device = $g->aug_get($spec);
-
-            next unless($device =~ m{^/dev/((?:sd|hd|xvd)(?:[a-z]+))(\d*)});
-
-            if(defined($map->{$1})) {
-                my $target = '/dev/'.$map->{$1};
-                $target .= $2 if(defined($2));
-                $g->aug_set($spec, $target);
-            } else {
-                print STDERR user_message(__x("No mapping found for block
".
-                                              "device {device}",
-                                              device => $device));
+    # $devices contains an order list of devices, as named by the host. Because
+    # libvirt uses a similar naming scheme to Linux, these will mostly be the
+    # same names as used by the guest. However, if the guest is using libata,
+    # IDE drives could be renamed.
+
+    # Look for IDE and SCSI devices in fstab for the guest
+    my %guestif;
+    foreach my $spec ($g->aug_match('/files/etc/fstab/*/spec')) {
+        my $device = $g->aug_get($spec);
+
+        next unless($device =~ m{^/dev/(sd|hd)([a-z]+)});
+        $guestif{$1} ||= {};
+        $guestif{$1}->{$1.$2} = 1;
+    }
+
+    # If fstab contains references to sdX, these could refer to IDE or SCSI
+    # devices. Need to look at the domain config for clues.
+    if (exists($guestif{sd})) {
+        # Look for IDE and SCSI devices from the domain definition
+        my %domainif;
+        foreach my $device (@$devices) {
+            foreach my $type ('hd', 'sd') {
+                if ($device =~ m{^$type([a-z]+)}) {
+                    $domainif{$type} ||= {};
+                    $domainif{$type}->{$device} = 1;
+                }
             }
         }
-    };
 
-    # Propagate augeas failure
-    die($@) if($@);
+        # If domain defines both IDE and SCSI drives, and fstab contains
+        # references on sdX, but not hdX, we don't know if the guest is
using
+        # libata or not.  This means that we don't know if sdX in fstab
refers
+        # to hdX or sdX in the domain. Warn and assume that libata device
+        # renaming is not in use.
+        if (exists($domainif{hd}) && exists($domainif{sd}) &&
+            !exists($guestif{hd}))
+        {
+            print STDERR user_message(__"WARNING: Unable to determine
whether ".
+                                        "sdX devices in /etc/fstab refer
to ".
+                                        "IDE or SCSI devices. Assuming
they ".
+                                        "refer to SCSI devices. /etc/fstab
".
+                                        "may be incorrect after conversion
if ".
+                                        "guest uses libata.");
+        }
+
+        # If we've got only IDE devices in the domain, and only sdX devices
in
+        # fstab, the guest is renaming them.
+        elsif (exists($domainif{hd}) && !exists($domainif{sd})
&&
+               !exists($guestif{hd}))
+        {
+            my %map;
+
+            my $letter = 'a';
+            foreach my $old (sort { _drivecmp('hd', $a, $b) }
+                                  keys(%{$domainif{hd}}))
+            {
+                $map{$old} = "sd$letter";
+                $letter++;
+            }
+
+            map { $_ = $map{$_} } @$devices;
+        }
+
+        # Otherwise we leave it alone
+    }
+
+    # We now assume that $devices contains an ordered list of device names, as
+    # used by the guest. Create a map of old guest device names to new guest
+    # device names.
+    my %map;
+
+    # Everything will be converted to either vdX or sdX
+    my $prefix = $virtio ? 'vd' : 'sd';
+    my $letter = 'a';
+    foreach my $device (@$devices) {
+        $map{$device} = $prefix.$letter;
+        $letter++;
+    }
+
+    # Go through fstab again, updating bare device references
+    foreach my $spec ($g->aug_match('/files/etc/fstab/*/spec')) {
+        my $device = $g->aug_get($spec);
+
+        next unless($device =~ m{^/dev/([a-z]+)(\d*)});
+        my $name = $1;
+        my $part = $2;
+
+        next unless(exists($map{$name}));
+
+        $g->aug_set($spec, "/dev/".$map{$name}.$part);
+    }
+    $g->aug_save();
+}
+
+sub _drivecmp
+{
+    my ($prefix, $a, $b) = @_;
+
+    map {
+        $_ =~ /^$prefix([a-z]+)/ or die("drive $_ doesn't have prefix
$prefix");
+        $_ = $1;
+    } ($a, $b);
+
+    return -1 if (length($a) < length($b));
+    return 1 if (length($a) > length($b));
+
+    return -1 if ($a lt $b);
+    return 0 if ($a eq $b);
+    return 1;
 }
 
 =item prepare_bootable(version [, module, module, ...])
diff --git a/v2v/virt-v2v.pl b/v2v/virt-v2v.pl
index eb5ba1f..7b389e4 100755
--- a/v2v/virt-v2v.pl
+++ b/v2v/virt-v2v.pl
@@ -262,13 +262,13 @@ my $dom = $conn->get_dom();
 exit(1) unless(defined($dom));
 
 # Get a list of the guest's transfered storage devices
-my @storage = $conn->get_local_storage();
+my $storage = $conn->get_storage_paths();
 
 # Create the transfer iso if required
 my $transferiso = get_transfer_iso($config, $config_file);
 
 # Open a libguestfs handle on the guest's storage devices
-my $g = get_guestfs_handle(\@storage, $transferiso);
+my $g = get_guestfs_handle($storage, $transferiso);
 
 $SIG{'INT'} = \&close_guest_handle;
 $SIG{'QUIT'} = \&close_guest_handle;
@@ -280,7 +280,8 @@ my $os = inspect_guest($g);
 my $guestos = Sys::VirtV2V::GuestOS->new($g, $os, $dom, $config);
 
 # Modify the guest and its metadata for the target hypervisor
-Sys::VirtV2V::Converter->convert($vmm, $guestos, $config, $dom, $os);
+Sys::VirtV2V::Converter->convert($vmm, $guestos, $config, $dom, $os,
+                                 $conn->get_storage_devices());
 
 $vmm->define_domain($dom->toString());
 
@@ -393,7 +394,7 @@ sub get_guestfs_handle
     # Enable autosync to defend against data corruption on unclean shutdown
     $g->set_autosync(1);
 
-    $g->launch ();
+    $g->launch();
 
     return $g;
 }
-- 
1.6.6

We call aug_load() in various places. This call explicitly throws away any
unsaved changes in the tree. For safety, we should always call aug_save() after
making changes to the tree. This change adds 2 missing calls.
---
 lib/Sys/VirtV2V/GuestOS/RedHat.pm |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/lib/Sys/VirtV2V/GuestOS/RedHat.pm
b/lib/Sys/VirtV2V/GuestOS/RedHat.pm
index 6adf8b8..5b7a95a 100644
--- a/lib/Sys/VirtV2V/GuestOS/RedHat.pm
+++ b/lib/Sys/VirtV2V/GuestOS/RedHat.pm
@@ -266,6 +266,7 @@ sub enable_kernel_module
        
$g->aug_set("/files/".$self->{modules}."/alias[last()+1]",
$device);
        
$g->aug_set("/files/".$self->{modules}."/alias[last()]/modulename",
                     $module)
+        $g->aug_save();
     };
 
     # Propagate augeas errors
@@ -1113,6 +1114,7 @@ sub prepare_bootable
                 last;
             }
         }
+        $g->aug_save();
     };
 
     # Propagate augeas failure
-- 
1.6.6

On Fri, Feb 19, 2010 at 04:42:56PM +0000, Matthew Booth
wrote:
> We were assuming that guests wouldn't use multiple interfaces, or if
they did,
> drive letters wouldn't clash between the interfaces. An ESX guest with
a CDROM
> device breaks this because hard disks will be presented a SCSI, starting at
sda,
> and CDROM devices are presented as IDE, starting at hda. This is in
contrast to
> QEMU, which starts at hdc by default.
> 
> Firstly, this change causes disks and CDROM devices to be treated
differently.
> CDROM devices are changed to use IDE, regardless of what interface they
were
> using before. This also limits the maximum number of CDROM devices in a
guest to
> 4. CDROM devices after 4 will be dropped, and a warning emitted.
> 
> Secondly, it is smarter about renaming IDE and SCSI devices, which may be
merged
> in the guest configuration.
> 
> Thirdly, it renames devices consistently based on the order they are listed
in
> the domain XML.
Well, it's a complicated patch ...

I think the real answer to this lies in virt-inspector (especially if
it is able to get extra information from the host / libvirt, which it
doesn't use at the moment).  Or maybe the _real_ answer is to get
everyone to use UUID-mounts.

But yes, it seems like a reasonable change in virt-v2v, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v