[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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
-- 
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team

M:       +44 (0)7977 267231
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]