1:21 p.m.
Matt,
Thanks for the extensive comments. I'll go through them in detail, but it
won't be until next week. For the time being, here's a quick take on some of
the main issues:
I do have a major concern, though, which is this is basically a fork
of
RedHat.pm. There are well over 1,000 identical lines of code between the
2 modules. Many of the differences are relatively minor, and could be
handled with additional cases. I've also fixed a couple of bits in
RedHat.pm since you forked it, and you've fixed at least 1 thing in
SUSE.pm which is relevant to RedHat.pm.
Yes - I inherited this task from another engineer who started down this road.
The majority of the code is identical to RedHat, but it seemed to be quicker
to go this route than completely change things around (especially since the
future seems to be guestconv).
I don't think I could accept this as-is purely from a maintenance
POV.
Could you have a look through and see what it would take to re-use code
currently in RedHat.pm? This might be:
* Renaming it, and handling SUSE as additional cases
* Pulling common code into a separate module
That would definitely make sense. I'll look at it from this point of view next
week.
> +++ b/lib/Sys/VirtConvert/Converter/SUSE.pm
> @@ -0,0 +1,2527 @@
> +# Sys::VirtConvert::Converter::SUSE
> +# Copyright (C) 2009-2012 SUSE Inc.
That should probably be:
Copyright (C) 2009-2012 Red Hat Inc.
Copyright (C) 2013 SUSE Inc.
Fixed.
> +sub get_initrd
> +{
> + my $self = shift;
> + my ($path) = @_;
> +
> + my $g = $self->{g};
> +
> + # The default name of the initrd is the same as the kernel name, with
> + # initrd replacing vmlinuz. This can be confirmed with grubby, or
> + # Bootloader::Tools->GetSection, but for now just do the replacement
> + # if the file exists.
> + my $initrd;
> + ($initrd = $path) =~ s/vmlinuz/initrd/;
> + if ($g->exists($initrd)) {
> + return $initrd;
> + }
> +
> + v2vdie __x('Didn\'t find initrd for kernel {path}', path =>
$path);
> +}
I would accept this, but in general I've tried to avoid heuristics.
Could you make this more reliable?
Yes, I can change it to perl-Bootloader, but I think those are the only two
options I have (without grubby in SUSE).
How confident can you be that perl and Bootloader::Tools are
available
on the guest? As a comparison, grubby is a dependency of the kernel on
Fedora, which is good enough :) Much less than that and you want to be
looking elsewhere.
perl-Bootloader is a dependency of the kernel in SUSE, so I think we are safe
there.
> + # Look for an EFI configuration
> + foreach my $cfg ($g->glob_expand('/boot/*efi/*/grub.conf')) {
I noticed you've changed this. Out of curiosity, where does SUSE mount
it?
Typically, it's mounted at /boot/efi, but the next level efi is lowercase.
However, the minor efi changes are what I inherited (well, that and the
copyright changes), so I have some more investigation to do here.
> + # Nothing to do if the kernel already has a grub entry
> + return if $g->aug_match("/files$grub_conf/title/kernel".
> + "[. = '$grub_path']") ne '';
I notice this is a change from RedHat.pm. Why did you add the "ne ''"?
Augeas will only return kernel entries, and I would expect a blank
kernel entry to be a parse error, hence this would be redundant.
In my testing (and according to RHBZ#974441), a null string is returned, and
the following error is encountered:
Argument "/files/boot/grub/menu.lst/title[1]/kernel" isn't numeric in
numeric
gt (>) at /usr/share/perl5/vendor_perl/Sys/VirtConvert/Converter/RedHat.pm
line 206.
This causes the routine to continue, and an unecessary entry is added to
menu.lst.
> + # No point in dying if /etc/SuSE-release can't be read
> + my ($title) = eval { $g->read_lines('/etc/SuSE-release') };
> + $title ||= 'Linux';
Another change from RedHat.pm, obviously :) I think there's a guestfs
inspection api to return this data. If so, that would help turn this
into a common module.
This really isn't all that important anyway, as it's just used in the naming
of a new boot entry, but I'll look into it.
> + $g->command(['grub-install.unsupported',
$device]);
I guess that's unsupported :) Another RedHat.pm diff which we could
probably detect.
:) Ya, I plan on changing this to a more correct grub cli script.
> + # Look for an EFI configuration
> + foreach my $cfg ($g->glob_expand('/boot/grub-efi/*/grub.cfg')) {
This is different to the other glob. Which is better?
*cough* Please see previous excuse... (I think grub-efi is incorrect.)
> + # Start by adding the default kernel
> + my $default = $self->get_default_image;
V2V always uses parens for function calls.
I'll fix that (in all places).
> + # Delete the fstab entry for the EFI boot partition
> + eval {
> + foreach my $node ($g->aug_match("/files/etc/fstab/*[file =
> '/boot/efi']")) + {
> + $g->aug_rm($node);
> + }
> + };
> + augeas_error($g, $@) if $@;
This is a diff from RedHat.pm, and it looks wrong. It removes the call
to aug_save(). In fact, I have a sneaking suspicion I may have fixed
this in RedHat.pm since you forked RedHat.pm.
Fixed.
> +# Configure a console on ttyS0. Make sure existing console
references use
> it. +# N.B. Note that the RHEL 6 xen guest kernel presents a console
> device called +# /dev/hvc0, where previous xen guest kernels presented
> /dev/xvc0. The regular
You don't like whereas ;)
:) Not sure what happened there...
> + foreach my $path
($g->aug_match('/files'.$xorg.'/Device/Driver'))
> { + $g->aug_set($path, 'cirrus');
I've changed the default to qxl now.
SUSE still uses cirrus.
> + v2vdie __x('Failed to find a {name} package
to install',
> + name => "$kernel_pkg");
This is $kernel_pkg.$kernel_arch in RedHat.pm. Is there any reason you
removed the arch?
No reason, just didn't seem useful at that time. :)
> + $g->aug_rm("$entry");
$g->aug_rm($entry);
Fixed.
> + $modified = 1;
> + }
> + }
> + }
> + $g->aug_save if ($modified == 1);
> +}
Obvious RedHat.pm diff.
I thought about also adding the virtio drivers into the above variables, but
mkinitrd will find the required modules without that change. Even removing the
xen modules is not required, but it cleans up later runs of mkinitrd.
> + if (defined($kernel_release)
&&
> + $kernel_release =~ /^(\S+?)(xen)?$/)
This is xen/xenU in RedHat.pm. Could leave it alone as it would work
here, despite being (presumably) unnecessary.
Sure, fixed.
> + # If a SLES11 kernel is being added, add -base kernel
> + if (defined($kernel)) {
> + if (_is_sles_family($g, $root) &&
> + $g->inspect_get_major_version($root) == 11) {
> + $kernel->[1][0] = $kernel->[0][0].'-base';
> + for my $count (1..4) {
> + if (defined($kernel->[0][$count])) {
> + $kernel->[1][$count] = $kernel->[0][$count];
> + }
> + }
> + }
> + }
Ah... I see what you're doing here. Still, _install_any() only takes a
single kernel. You can convert it into a list within this function and
leave the rest untouched.
Yes, I wasn't completely sure where I should pull the trigger on adding the -
base kernel. I'll clean the 'single kernel' references up and follow your
recommendations.
> + logmsg NOTICE, __x('Falling back to local virt-v2v
repo:');
> + foreach my $pkg (@rpms) {
> + (my $pkgname = $pkg) =~ s/(.*\/)//;
basename would have saved me a moment of squinting :)
Yes, but would it have been as fun? ;) Using basename would require
File::Basename, and I wasn't sure if that would be acceptable.
> + logmsg NOTICE, __x(" $pkgname");
Logging must not be used for formatting. The log message must be
self-contained. You should construct this list and emit a single log
message.
Good point. I'll clean that up. In general, do agree with printing out the
packages that are being installed?
> + eval { $g->command(['rpm', $update == 1 ?
'-U' : '-i', @rpms]) };
> + if ($@) {
> + my $error = $@;
> + # Try to isolate error to a meaningful string
> + $error =~ /^(command.*error:)\s+(.*:)\s+(.*)\s(at \/usr\/.*)/;
> + logmsg WARN, __x('Failed to install packages. '.
> + 'Error was: {error}', error => "$2
$3");
What error message is this trying to parse? We should avoid trying to
parse error message wherever possible as they are notorious for
differing between versions.
The warning I was trying to isolate is:
command: command: error: Failed dependencies:
kernel-default-base_x86_64 = 3.0.76-0.11.1 is needed by kernel-
default-3.0.76-0.11.1.x86_64 at
/usr/lib/perl5/vendor_perl/5.16.2/Sys/VirtConvert/GuestfsHandle.pm line 198.
at /usr/lib/perl5/vendor_perl/5.16.2/Sys/VirtConvert/Converter/SUSE.pm line
1885.
This error should only be seen if someone has misconfigured their kernel and
kernel-base entries in the virt-v2v .conf (or didn't copy the rpms down
properly), but my goal was to strip out the 'command: command: error:' and 'at
/usr/lib....' parts of the error, as they don't help. Is a better approach to
leave the entire string in, or is there a more acceptable way to clean it up?
Also, this should almost certainly die (with v2vdie) rather than be
a
warning.
Yes, good point. An error here is fatal, and the condition will be caught in
shortly, but dying here would be better.
> + my $idedev;
> + my @newdevices;
> + my $suffix = 'a';
> + foreach my $device (@devices) {
> + if ($device =~ /(?:h|s)d[a-z]+/) {
> + $idedev = $device;
> + $device = 'sd'.$suffix++;
> + $idemap{$device} = $idedev if ($device ne $idedev);
RedHat.pm diff, and probably generally applicable. I'd definitely want
to look at this as a separate patch.
I'll submit this as a general patch.
> + # Update bare device references in fstab and grub's
menu.lst and
> device.map + foreach my $spec
> ($g->aug_match('/files/etc/fstab/*/spec'), +
> $g->aug_match("/files$grub->{grub_conf}/*/kernel/root"), +
>
$g->aug_match("/files$grub->{grub_conf}/*/kernel/resume"),
This doesn't work for grub2. However, it does need to be done, and we
don't do it in RedHat.pm. Again, I'd like to see a general solution to
this.
I wondered about that. I'll come up with something that should fit both cases.
> + elsif ($g->exists('/sbin/mkinitrd')) {
> + # Create a new initrd which probes the required kernel modules
> + my @module_args = ();
> + push(@module_args, "-m \"");
> + foreach my $module (@modules) {
> + push(@module_args, "$module ");
> + }
> + push(@module_args, "\"");
> +
> + my @env;
> +
> + $g->sh(join(' ', @env).'/sbin/mkinitrd '.join('
', @module_args).
> + " -i $grub_initrd -k /boot/vmlinuz-$version");
I think you're looking for:
$g->sh(join(' ', @env).' /sbin/mkinitrd -m "'.join(' ',
@modules).'" '.
We should probably modularise this who section somewhere.
Ok. I'll change it.
> +# The next two functions are a temporary workaround to
bnc#836521. The
> actual +# fix is in a new perl-Bootloader, which will likely not be on
> most guests. +sub _modify_perlBootloader
Welcome to being forced to work around bugs which were fixed long ago ;)
Thanks! It's an exciting place to be ;)
Thanks again for all the comments. I'll clean this up and post a rev2 next
week.
-Mike