Re: [Libguestfs] [PATCH] Use RHN to retrieve replacement packages

Milan, Some comments inline. On 14/05/10 15:06, Milan Zazrivec wrote: > For guests registered with Red Hat Network, this patch allows for > the conversion process to use RHN to download appropriate kernel > package and its dependencies. > > We use yum on RHEL-5, up2date libraries on RHEL-4. > > We install matching kernel version whenever possible, latest available > kernel version otherwise. > > _discover_kernel routine has been extended to return version-release > of the default kernel found. > --- > lib/Sys/VirtV2V/GuestOS/RedHat.pm | 166 > ++++++++++++++++++++++++++++-------- 1 files changed, 129 insertions(+), > 37 deletions(-) > > diff --git a/lib/Sys/VirtV2V/GuestOS/RedHat.pm > b/lib/Sys/VirtV2V/GuestOS/RedHat.pm index 77f0f3a..38a485c 100644 > --- a/lib/Sys/VirtV2V/GuestOS/RedHat.pm > +++ b/lib/Sys/VirtV2V/GuestOS/RedHat.pm > @@ -475,7 +475,7 @@ sub add_kernel > { > my $self = shift; > > - my ($kernel_pkg, $kernel_arch) = $self->_discover_kernel(); > + my ($kernel_pkg, $kernel_ver, $kernel_arch) = > $self->_discover_kernel(); > > # If the guest is using a Xen PV kernel, choose an appropriate > normal kernel # replacement > @@ -547,56 +547,142 @@ sub add_kernel > } > } > > - my ($app, $depnames); > - eval { > + my $version; > + > + # systemid exists, assume the system is registered w/ RHN > + if ($self->{g}->exists('/etc/sysconfig/rhn/systemid')) { Can we make this less of an explicit check for RHN registration that a check for the availability of an update mechanism? I was thinking you'd check to see if up2date/yum was installed, and if it was, try to run it. If running it failed, fall back to the config.

> my $desc = $self->{desc}; > > - ($app, $depnames) = > - $self->{config}->match_app($desc, $kernel_pkg, > $kernel_arch); - }; > - # Return undef if we didn't find a kernel > - if ($@) { > - print STDERR $@; > - return undef; > - } > + my ($min_virtio_ver, @kern_vr, @preinst_cmd, @inst_cmd, > $inst_fmt); > > - my @missing; > - if (!$self->{g}->exists($self->_transfer_path($app))) { > - push(@missing, $app); > + # filter out xen/xenU from release field > + if ($kernel_ver =~ /^(\S+)-(\S+?)(xen)?(U)?$/) { > + @kern_vr = ($1, $2); > + $kernel_ver = join('-', @kern_vr); > + } > + > + # RHEL-5 > + if ($desc->{distro} eq 'rhel' && $desc->{major_version} eq '5') > { Again, with these tests, could we just check for the existence of /usr/bin/yum? I guess we'd need to use up2date in preference to yum if it was installed, as the most likely scenario here is RHEL 3/4 using yum for non-core repos.

> + if (_rpmvercmp($kernel_ver, '2.6.18-128.el5') >= 0) { > + # Install matching kernel version > + @inst_cmd = ('/usr/bin/yum', '-y', 'install', > + "$kernel_pkg-$kernel_ver"); > + } else { > + # Install latest available kernel version > + @inst_cmd = ('/usr/bin/yum', '-y', 'install', > $kernel_pkg); + > + # We need to upgrade kernel deps. first to avoid > possible conflicts Why can't yum do it all in one go?

> + my @deps; > + my $deps = ($self->{config}->match_app($desc, > $kernel_pkg, $kernel_arch))[1]; + > + if ($deps) { > + @preinst_cmd = (('/usr/bin/yum', '-y', 'upgrade'), > @$deps); + } > + } > + } > + # RHEL-4 > + elsif ($desc->{distro} eq 'rhel' && $desc->{major_version} eq > '4') { Using RHEL version to distinguish between the methods below is ok, I guess, as nothing except RHEL used up2date (as far as I'm aware). > + if (_rpmvercmp($kernel_ver, '2.6.9-89.EL') >= 0) { > + # Install matching kernel version > + @inst_cmd = ('/usr/bin/python', '-c', > + "import sys; sys.path.append('/usr/share/rhn');" . > + "import actions.packages;" . > + "actions.packages.cfg['forceInstall'] = 1;" . > + "actions.packages.update([['$kernel_pkg', " . > + "'$kern_vr[0]', '$kern_vr[1]', '']])"); Yuk, but ok ;) > + } else { > + # Install latest available kernel version > + @inst_cmd = ('/usr/sbin/up2date', '-fi', $kernel_pkg); > + > + # We need to upgrade kernel deps. first to avoid > possible conflicts + my $deps = > ($self->{config}->match_app($desc, $kernel_pkg, $kernel_arch))[1]; + > + if ($deps) { > + @preinst_cmd = (('/usr/sbin/up2date', '-fu'), > @$deps); + } > + } > + } > + else { > + @inst_cmd = ('/usr/sbin/up2date', '-fi', $kernel_pkg); > + } > + > + my (@k_before, @k_new); > + > + # List of kernels before the new kernel installation > + @k_before = $self->{g}->glob_expand('/boot/vmlinuz-*'); > + > + eval { > + # Upgrade dependencies if needed > + if (@preinst_cmd) { > + $self->{g}->command(\@preinst_cmd); > + } > + # Install new kernel > + $self->{g}->command(\@inst_cmd); Again, why can't we do these in a single transaction?

> + }; > + > + $self->_augeas_error($@) if ($@); Just ditch the eval {} block here. I recently ditched these elsewhere in the code.

> + > + # Figure out which kernel has just been installed > + foreach my $k ($self->{g}->glob_expand('/boot/vmlinuz-*')) { > + grep(/^$k$/, @k_before) or push(@k_new, $k); > + } > + > + # version-release of the new kernel package > + $version = ($self->{g}->command_lines( > + ['rpm', '-qf', '--qf=%{VERSION}-%{RELEASE}', > $k_new[0]]))[0]; As above, can we arrange this block to fall-through if the execution of up2date/yum failed for some reason?

> } else { > - return undef if ($self->_newer_installed($app)); > - } > > - my $user_arch = $kernel_arch eq 'i686' ? 'i386' : $kernel_arch; > + my ($app, $depnames); > + eval { > + my $desc = $self->{desc}; > > - my @deps = $self->_get_deppaths(\@missing, $user_arch, @$depnames); > + ($app, $depnames) = > + $self->{config}->match_app($desc, $kernel_pkg, > $kernel_arch); + }; > + # Return undef if we didn't find a kernel > + if ($@) { > + print STDERR $@; > + return undef; > + } > > - # We can't proceed if there are any files missing > - _die_missing(@missing) if (@missing > 0); > + my @missing; > + if (!$self->{g}->exists($self->_transfer_path($app))) { > + push(@missing, $app); > + } else { > + return undef if ($self->_newer_installed($app)); > + } > > - # Install any required kernel dependencies > - $self->_install_rpms(1, @deps); > + my $user_arch = $kernel_arch eq 'i686' ? 'i386' : $kernel_arch; > > - # Inspect the rpm to work out what kernel version it contains > - my $version; > - my $g = $self->{g}; > - foreach my $file ($g->command_lines > - (["rpm", "-qlp", $self->_transfer_path($app)])) > - { > - if($file =~ m{^/boot/vmlinuz-(.*)$}) { > - $version = $1; > - last; > + my @deps = $self->_get_deppaths(\@missing, $user_arch, > @$depnames); + > + # We can't proceed if there are any files missing > + _die_missing(@missing) if (@missing > 0); > + > + # Install any required kernel dependencies > + $self->_install_rpms(1, @deps); > + > + # Inspect the rpm to work out what kernel version it contains > + my $g = $self->{g}; > + foreach my $file ($g->command_lines > + (["rpm", "-qlp", $self->_transfer_path($app)])) > + { > + if($file =~ m{^/boot/vmlinuz-(.*)$}) { > + $version = $1; > + last; > + } > } > - } > > - die(user_message(__x("{path} doesn't contain a valid kernel", > - path => $app))) if(!defined($version)); > + die(user_message(__x("{path} doesn't contain a valid kernel", > + path => $app))) if(!defined($version)); > > - $self->_install_rpms(0, ($app)); > + $self->_install_rpms(0, ($app)); > + > + } > > # Make augeas reload so it'll find the new kernel > eval { > - $g->aug_load(); > + $self->{g}->aug_load(); Why?

> }; > > $self->_augeas_error($@) if ($@); > @@ -632,6 +718,7 @@ sub _discover_kernel > > # Get a current bootable kernel, preferrably the default > my $kernel_pkg; > + my $kernel_ver; > my $kernel_arch; > > foreach my $i (@configs) { > @@ -647,6 +734,11 @@ sub _discover_kernel > > # Get the kernel package name > $kernel_pkg = $kernel->{package}; > + > + # Get the kernel package version > + $kernel_ver = $kernel->{version}; > + > + last; > } > > # Default to 'kernel' if package name wasn't discovered > @@ -664,7 +756,7 @@ sub _discover_kernel > # a very long time. > $kernel_arch = 'i686' if('i386' eq $kernel_arch); > > - return ($kernel_pkg, $kernel_arch); > + return ($kernel_pkg, $kernel_ver, $kernel_arch); > } > > =item remove_kernel(version) > -- 1.7.1 Thanks, Matt