Mike Latimer
2013-Oct-03 17:48 UTC
Re: [Libguestfs] [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
Matthew Booth
2013-Oct-04 08:38 UTC
Re: [Libguestfs] [PATCH] virt-v2v: Convert RedHat.pm to Linux.pm - for SUSE support
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
Mike Latimer
2013-Oct-07 16:58 UTC
Re: [Libguestfs] [PATCH] virt-v2v: Convert RedHat.pm to Linux.pm - for SUSE support
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
Maybe Matching Threads
- Re: [PATCH] virt-v2v: Convert RedHat.pm to Linux.pm - for SUSE support
- Re: [PATCH] virt-v2v: Convert RedHat.pm to Linux.pm - for SUSE support
- Re: [PATCH] virt-v2v: Convert RedHat.pm to Linux.pm - for SUSE support
- Re: [PATCH] virt-v2v: Convert RedHat.pm to Linux.pm - for SUSE support
- [PATCH] virt-v2v: Convert RedHat.pm to Linux.pm - for SUSE support