Libguestfs - Oct 2013 - Re: [PATCH] virt-v2v: Convert RedHat.pm to Linux.pm - for SUSE support

Thanks for the comments, Matt.

On Thursday, October 03, 2013 05:34:27 PM Matthew Booth
wrote:
> > -    # There should be an installed virtio capable kernel if virtio
was
> > installed -    die("virtio configured, but no virtio kernel
found")
> > +    # Warn if there is no installed virtio capable kernel. It is safe
to
> > +    # continue and attempt to install a virtio kernel next.
> > +    logmsg NOTICE, __x('virtio capable guest, but no virtio
kernel
> > found.')> 
> >          if ($virtio && !defined($boot_kernel));
> 
> No. This indicates an error in the program rather than something the
> user can fix. This is also why it's a plain die with no translation.
If a virtio kernel must be installed prior to _configure_kernel, why is there 
the ability to install a replacement kernel immediately after this message? It 
doesn't really matter to SUSE though - adding 'kernel' to the deps
for virtio
in virt-v2v.db will trigger the installation of the correct kernel in the 
install_capabilities step.
> > @@ -1631,14 +1752,38 @@ sub _install_any
> > 
> >      # If we're installing a kernel, check which kernels are there
first
> >      my @k_before = $g->glob_expand('/boot/vmlinuz-*') if
> >      defined($kernel);
> > 
> > +    # If a SLES11 kernel is being added, add -base kernel
> > +    if (defined($kernel)) {
> > +        if (_is_suse_family($g, $root) &&
> > +            ($g->inspect_get_major_version($root) == 11)) {
> > +            my $tmp;
> > +            push(@$tmp, $kernel);
> > +            $tmp->[1][0] = $tmp->[0][0].'-base';
> > +            for my $count (1..4) {
> > +                if (defined($tmp->[0][$count])) {
> > +                    $tmp->[1][$count] = $tmp->[0][$count];
> > +                }
> > +            }
> > +            $kernel = $tmp;
> > +        }
> > +    }
> 
> For sanity, I think we need $kernel to be the same data structure in
> all cases. Lets either unconditionally convert $kernel into a list (of
> lists), or see of -base can be added to the @install list or something
> like that. Bear in mind, though, that in the former case you'd also
> have to update _install_yum and _install_up2date.
The problem with @install is that it is processed separately from $kernel in 
_install_config. This causes the two packages to be installed by separate rpm 
commands, and the dependencies cannot be satisfied. This is why I had to add 
the -base package to $kernel somehow...  (Note - This doesn't effect zypper 
installs at all as it will satisfy dependencies automatically.)

I'll think about it some more...
> > @@ -2235,6 +2541,10 @@ sub _remap_block_devices
...
> I've had a change to think about this change properly, and I'm
pretty
> sure it's not required. The reason is that the only place %idemap is
> modified is in the section guarded:
> 
> if ($libata) {
> ...
> }
> 
> If the guest is using libata it will not have any references to
> /dev/hdX devices, as IDE devices are presented as SCSI devices. That's
> a NAK to this bit of the patch. Instead, you need to work out when SLES
> switched to libata and set $libata accordingly above.
If there are never any hdX devices in libata machines, why are they renamed in 
the first place? The code I proposed just tracks any such changes and adds them 
to the list of remaps to perform. (i.e. if hda --> sda, and sda --> vda,
then
hda --> vda as well.)

However, SLES seems to be similar to RHEL5, where libata is in use, but udev 
calls them hdX anyway (for hvm guests). I can add SLES to the list of 
$libata=0 environments, but wasn't sure if that was the best long term 
approach.

Thanks,
Mike

On Thu, 2013-10-03 at 11:48 -0600, Mike Latimer wrote:
> Thanks for the comments, Matt.
> 
> On Thursday, October 03, 2013 05:34:27 PM Matthew Booth wrote:
> > > -    # There should be an installed virtio capable kernel if
virtio was
> > > installed -    die("virtio configured, but no virtio kernel
found")
> > > +    # Warn if there is no installed virtio capable kernel. It is
safe to
> > > +    # continue and attempt to install a virtio kernel next.
> > > +    logmsg NOTICE, __x('virtio capable guest, but no virtio
kernel
> > > found.')> 
> > >          if ($virtio && !defined($boot_kernel));
> > 
> > No. This indicates an error in the program rather than something the
> > user can fix. This is also why it's a plain die with no
translation.
> 
> If a virtio kernel must be installed prior to _configure_kernel, why is
there
> the ability to install a replacement kernel immediately after this message?
It
> doesn't really matter to SUSE though - adding 'kernel' to the
deps for virtio
> in virt-v2v.db will trigger the installation of the correct kernel in the 
> install_capabilities step.
It's specifically an error if we're attempting to configure virtio, and
there's no detected virtio kernel. It shouldn't have been possible to
get here in that state, hence it's a programmer error. The code below
attempts to install *any* kernel in the case that we aren't configuring
virtio.
> > > @@ -1631,14 +1752,38 @@ sub _install_any
> > > 
> > >      # If we're installing a kernel, check which kernels are
there first
> > >      my @k_before = $g->glob_expand('/boot/vmlinuz-*')
if
> > >      defined($kernel);
> > > 
> > > +    # If a SLES11 kernel is being added, add -base kernel
> > > +    if (defined($kernel)) {
> > > +        if (_is_suse_family($g, $root) &&
> > > +            ($g->inspect_get_major_version($root) == 11)) {
> > > +            my $tmp;
> > > +            push(@$tmp, $kernel);
> > > +            $tmp->[1][0] = $tmp->[0][0].'-base';
> > > +            for my $count (1..4) {
> > > +                if (defined($tmp->[0][$count])) {
> > > +                    $tmp->[1][$count] = $tmp->[0][$count];
> > > +                }
> > > +            }
> > > +            $kernel = $tmp;
> > > +        }
> > > +    }
> > 
> > For sanity, I think we need $kernel to be the same data structure in
> > all cases. Lets either unconditionally convert $kernel into a list (of
> > lists), or see of -base can be added to the @install list or something
> > like that. Bear in mind, though, that in the former case you'd
also
> > have to update _install_yum and _install_up2date.
> 
> The problem with @install is that it is processed separately from $kernel
in
> _install_config. This causes the two packages to be installed by separate
rpm
> commands, and the dependencies cannot be satisfied. This is why I had to
add
> the -base package to $kernel somehow...  (Note - This doesn't effect
zypper
> installs at all as it will satisfy dependencies automatically.)
> 
> I'll think about it some more...
You can still make $kernel a list, you just need to ensure it's always a
list. The updates to the other installation methods should be trivial.

> > > @@ -2235,6 +2541,10 @@ sub _remap_block_devices
> ...
> > I've had a change to think about this change properly, and I'm
pretty
> > sure it's not required. The reason is that the only place %idemap
is
> > modified is in the section guarded:
> > 
> > if ($libata) {
> > ...
> > }
> > 
> > If the guest is using libata it will not have any references to
> > /dev/hdX devices, as IDE devices are presented as SCSI devices.
That's
> > a NAK to this bit of the patch. Instead, you need to work out when
SLES
> > switched to libata and set $libata accordingly above.
> 
> If there are never any hdX devices in libata machines, why are they renamed
in
> the first place? The code I proposed just tracks any such changes and adds
them
> to the list of remaps to perform. (i.e. if hda --> sda, and sda -->
vda, then
> hda --> vda as well.)
This is because there are 2 names here: the 'libvirt' name, which calls
ide devices hdX and is independent of the guest OS because it's at the
level of the hypervisor, and the name used by the guest os, which may
will be different if the guest uses libata.

The only direct information we have about the guest comes from the
hypervisor, so the only names we have to start off with are the names
the hypervisor uses. Everything else comes from subsequent inspection.
If the guest uses something different we need to handle that, hence this
whole complicated and fragile dance.
> However, SLES seems to be similar to RHEL5, where libata is in use, but
udev
> calls them hdX anyway (for hvm guests). I can add SLES to the list of 
> $libata=0 environments, but wasn't sure if that was the best long term 
> approach.
Looks good to me.

Matt

On Friday, October 04, 2013 09:38:58 AM Matthew Booth
wrote:
> It's specifically an error if we're attempting to configure virtio,
and
> there's no detected virtio kernel. It shouldn't have been possible
to
> get here in that state, hence it's a programmer error. The code below
> attempts to install *any* kernel in the case that we aren't configuring
> virtio.
Ok. I've added a 'kernel' dependency to the virtio capability for
SUSE, and
this has resolved the above problem. It did bring up a versioning convention 
difference between RedHat and SUSE, but I've worked around the problem.
(SUSE
kernels include a build number that is not seen in the 'Linux kernel'
string,
or in the name of the kernel. However, this complete version string is 
required for kernel package installations. I now get this full version in 
_inspect_linux_kernel, and use it later in a couple of places.)

Speaking of _inspect_linux_kernel, in my testing (under SUSE),
$g->file('somefile') does not include the expected
'somefile:' at the beginning
of the return string. Because of this, the following code isn't correct:

  if($filedesc =~ /^$path: Linux kernel .*\bversion\s+(\S+)\b/) {

This works though:

  if($filedesc =~ /^Linux kernel .*\bversion\s+(\S+)\b/) {

The above "Linux kernel" string doesn't match the SUSE version
output.
However, it's better if I don't modify the pattern to catch both version
strings, as the next 'else' case will determine the correct version from
the
vmlinuz file. As the above pattern change doesn't really effect SUSE, I was 
hoping you could check the output under RedHat and let me know if you'd like
'$path: ' removed from my version.

One other item I was hoping you could doublecheck is in the filtering out of 
xen/xenU in _install_capability. Shouldn't the xen/xenU portion of the
release
string actually be '-xen/xenU'? In other words, would the inclusion of
the
hypen in the following code also be applicable for RedHat?

                        if (defined($kernel_release) &&
                            $kernel_release =~ /^(\S+?)(-xen)?(U)?$/)
                        {
                            $kernel_release = $1;
                        }
> You can still make $kernel a list, you just need to ensure it's always
a
> list. The updates to the other installation methods should be trivial.
Rather than doing that, I've added the -base kernel as an app dependency for
the kernel. (It seems appropriate anyway.) At the end of _install_config, I 
catch this condition, and install the packages together. This approach 
shouldn't impact any non-SUSE environments.
> This is because there are 2 names here: the 'libvirt' name, which
calls
> ide devices hdX and is independent of the guest OS because it's at the
> level of the hypervisor, and the name used by the guest os, which may
> will be different if the guest uses libata.
> 
> The only direct information we have about the guest comes from the
> hypervisor, so the only names we have to start off with are the names
> the hypervisor uses. Everything else comes from subsequent inspection.
> If the guest uses something different we need to handle that, hence this
> whole complicated and fragile dance.
Agreed, but I still think there could be environments where hdX devices exist 
(from the guest perspective) in a libata environment. If that ever is the 
case, I think any such devices should be included in the rename map.

I've set all SUSE environments to $libata=0 for now, but I'm still doing
research to see if there is ever a condition where we want $prefix set to
'sd'
(which would mean that setting will need to be changed).

Thanks,
Mike