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

Re: [et-mgmt-tools] [PATCH] Add support for Solaris PV



On Wed, Dec 03, 2008 at 08:31:28PM -0800, john levon sun com wrote:
> # HG changeset patch
> # User john levon sun com
> # Date 1228365031 28800
> # Node ID 35baacfe79834400949ebdba69f952ed873ae442
> # Parent  7dd32b4d7915937025f42c3519711db50e5a7ce3
> Add support for Solaris PV
> 
> Solaris PV comes in two flavours: Nevada and OpenSolaris.  In order to
> correctly build network installs for Nevada, we need to pass down guest
> options into OSDistro.
> 
> os_type was horribly overloaded between the Installer and Guest classes,
> meaning two different things ('solaris' or 'hvm'). Clean this up by
> naming the latter 'virt_type'.

The new Installer class for Solaris looks fine to me, but I don't really
like the renaming of os_type to virt_type, since its conflicting naming
to the libvirt usage.

'virt type' is usually used to refer to hypervisor type, eg this bit
of the XML

  <domain type='xen'>
  <domain type='kvm'>
  <domain type='qemu'>

While 'os type' is used to refer to the Operating System guest ABI

  <os>
    <type>linux</type>  (badly named, should really be 'xen' but
                         this is more compatible with older libvirt)

   <type>hvm</type>   (native fully virt)
   <type>uml</type>   (User mode linux) 



Unfortunaltey the virt-install CLI flag is --os-type - should really
have been --os-distro-type. So I think I'd rather we changed the
insternal variables for this to be os_distro_type and os_distro_variant



> @@ -239,26 +249,29 @@ class DistroInstaller(Guest.Installer):
>              arch = os.uname()[4]
>              if hasattr(guest, "arch"):
>                  arch = guest.arch
> -            (kernelfn, initrdfn, args) = acquireKernel(self.location,
> -                                                       meter,
> -                                                       scratchdir = self.scratchdir,
> -                                                       type = self.os_type,
> -                                                       distro = distro,
> -                                                       arch=arch)
> +
> +            (kernelfn, initrdfn, args), os_type = \
> +                self._acquire_kernel(guest, meter, distro = distro, arch = arch)
> +
> +            guest.os_type = os_type
>              self.install["kernel"] = kernelfn
>              self.install["initrd"] = initrdfn
> -            if not self.extraargs is None:
> -                self.install["extraargs"] = self.extraargs + " " + args
> -            else:
> -                self.install["extraargs"] = args
> +            self.install["extraargs"] = args

This chunk looks a little questionable to me - IIRC this is reverting a fix
we previously made, so perhaps this is just a merge error.

Baiscally the existing  "extraargs" data will contain params from --extra-args
command line flag, and the 'self.extraargs' needs to append more data provided
by the DistroInstaller class, such as kickstart URL. This change means we loose
the former.

> @@ -172,18 +174,21 @@ class Distro:
>  
>          return False
>  
> -    def _kernelFetchHelper(self, fetcher, progresscb, kernelpath, initrdpath):
> +    def _kernelFetchHelper(self, fetcher, guest, progresscb, kernelpath, initrdpath):
>          # Simple helper for fetching kernel + initrd and performing
>          # cleanup if neccessary
>          kernel = fetcher.acquireFile(kernelpath, progresscb)
> +        args = ''
> +
> +        if not fetcher.location.startswith("/"):
> +            args += "method=" + fetcher.location
> +
> +        if guest.extraargs:
> +            args += guest.extraargs
> +
>          try:
>              initrd = fetcher.acquireFile(initrdpath, progresscb)
> -            if fetcher.location.startswith("/"):
> -                # Local host path, so can't pass a location to guest
> -                #for install method
> -                return (kernel, initrd, "")
> -            else:
> -                return (kernel, initrd, "method=" + fetcher.location)
> +            return kernel, initrd, args


Hmm, this existing code was dubious  even before this change. method= is
the Fedora / RHEL anaconda syntax for specifying install location. It
should never havebeen in Distro class in the first place - instead it
belongs in the RedHatDistro subclass.
> @@ -692,3 +707,194 @@ class MandrivaDistro(Distro):
>  
>          return False
>  
> +# Solaris and OpenSolaris distros
> +class SunDistro(Distro):
> +
> +    name = "Solaris"
> +    os_type = "solaris"
> +
> +    def isValidStore(self, fetcher, progresscb):
> +        """Determine if uri points to a tree of the store's distro"""
> +        raise NotImplementedError
> +
> +    def acquireBootDisk(self, fetcher, progresscb):
> +        return fetcher.acquireFile("images/solarisdvd.iso", progresscb)
> +
> +    def process_extra_args(self, argstr):
> +        """Collect additional arguments."""
> +        if not argstr:
> +            return (None, None, None, None)
> +
> +        kopts = ''
> +        kargs = ''
> +        smfargs = ''
> +        Bargs = ''
> +
> +        args = argstr.split()
> +        i = 0
> +        while i < len(args):
> +            exarg = args[i]
> +            if exarg == '-B':
> +                i += 1
> +                if i == len(args):
> +                    continue
> +
> +                if not Bargs:
> +                    Bargs = args[i]
> +                else:
> +                    Bargs = ','.join([Bargs, args[i]])
> +        
> +            elif exarg == '-m':
> +                i += 1
> +                if i == len(args):
> +                    continue
> +                smfargs = args[i]
> +            elif exarg.startswith('-'):
> +                if kopts is None:
> +                    kopts = exarg[1:]
> +                else:
> +                    kopts = kopts + exarg[1:]
> +            else:
> +                if kargs is None:
> +                    kargs = exarg
> +                else:
> +                    kargs = kargs + ' ' + exarg
> +            i += 1
> +
> +        return kopts, kargs, smfargs, Bargs
> +
> +class SolarisDistro(SunDistro):
> +    kernelpath = "boot/platform/i86xpv/kernel/unix"
> +    initrdpath = "boot/x86.miniroot"
> +
> +    def isValidStore(self, fetcher, progresscb):
> +        if fetcher.hasFile(self.kernelpath):
> +            logging.debug("Detected Solaris")
> +            return True
> +        return False
> +
> +    def install_args(self, guest):
> +        """Construct kernel cmdline args for the installer, consisting of:
> +           the pathname of the kernel (32/64) to load, kernel options
> +           and args, and '-B' boot properties."""
> +
> +        # XXX: ignoring smfargs for the time being
> +        (kopts, kargs, smfargs, kbargs) = \
> +            self.process_extra_args(guest.extraargs)
> +
> +        bargs = ''
> +        if kopts:
> +            bargs += '-' + kopts + ' -'
> +
> +        if guest.graphics['enabled'] == False:
> +            bargs += ' nowin'
> +
> +        if guest.autocf:
> +            bargs += ' install'
> +
> +        if kargs:
> +            bargs += ' ' + kargs
> +
> +        if guest.location.startswith('nfs:'):
> +            try:
> +                guestIP = socket.gethostbyaddr(guest.name)[2][0]
> +            except:
> +                bargs += ' dhcp'
> +            else:
> +                iserver = guest.location.split(':')[1]
> +                ipath = guest.location.split(':')[2]
> +                iserverIP = socket.gethostbyaddr(iserver)[2][0]
> +                bargs += " -B install_media=" + iserverIP + ":" + ipath
> +                bargs += ",host-ip=" + guestIP
> +                droute = util.default_route(guest.nics[0].bridge)
> +                if droute is not None:
> +                    bargs += ",router-ip=" + droute
> +                if guest.nics[0].macaddr is not None:
> +                    en = guest.nics[0].macaddr.split(':')
> +                    for i in range(len(en)):
> +                        # remove leading '0' from mac address element
> +                        if len(en[i]) > 1 and en[i][0] == '0':
> +                            en[i] = en[i][1]
> +                    boot_mac = ":".join(en)
> +                    bargs += ",boot-mac=" + boot_mac
> +                if guest.autocf:
> +                    acf_host = guest.autocf.split(":")[1]
> +                    acf_path = guest.autocf.split(":")[2]
> +                    try:
> +                        acf_hostip = socket.gethostbyaddr(acf_host)[2][0]
> +                        bargs += ",sysid_config=" + acf_hostip + ":" + acf_path
> +                        bargs += ",install_config=" + acf_hostip + ":" + acf_path
> +                    except:
> +                        print "failed to lookup host %s" % acf_host
> +        else:
> +            bargs += " -B install_media=cdrom"
> +
> +        if kbargs:
> +            bargs += "," + kbargs
> +
> +        return bargs
> +
> +    def acquireKernel(self, guest, fetcher, progresscb):
> +
> +        try:
> +            kernel = fetcher.acquireFile(self.kernelpath, progresscb)
> +        except:
> +            raise RuntimeError("Solaris PV kernel not found at %s" %
> +                self.kernelpath)
> +
> +        # strip boot from the kernel path
> +        kpath = self.kernelpath.split('/')[1:]
> +        args = "/" + "/".join(kpath) + self.install_args(guest)
> +
> +        try:
> +            initrd = fetcher.acquireFile(self.initrdpath, progresscb)
> +            return (kernel, initrd, args)
> +        except:
> +            os.unlink(kernel)
> +            raise RuntimeError(_("Solaris miniroot not found at %s") %
> +                self.initrdpath)
> +
> +class OpenSolarisDistro(SunDistro):
> +    kernelpath = "platform/i86xpv/kernel/unix"
> +    initrdpath = "boot/x86.microroot"
> +
> +    def isValidStore(self, fetcher, progresscb):
> +        if fetcher.hasFile(self.kernelpath):
> +            logging.debug("Detected OpenSolaris")
> +            return True
> +        return False
> +
> +    def install_args(self, guest):
> +        """Construct kernel cmdline args for the installer, consisting of:
> +           the pathname of the kernel (32/64) to load, kernel options
> +           and args, and '-B' boot properties."""
> +
> +        # XXX: ignoring smfargs and kargs for the time being
> +        (kopts, kargs, smfargs, kbargs) = \
> +            self.process_extra_args(guest.extraargs)
> +
> +        bargs = ''
> +        if kopts:
> +            bargs += ' -' + kopts
> +        if kbargs:
> +            bargs += ' -B ' + kbargs
> +
> +        return bargs
> +
> +    def acquireKernel(self, guest, fetcher, progresscb):
> +
> +        try:
> +            kernel = fetcher.acquireFile(self.kernelpath, progresscb)
> +        except:
> +            raise RuntimeError(_("OpenSolaris PV kernel not found at %s") %
> +                self.kernelpath)
> +
> +        args = "/" + self.kernelpath + self.install_args(guest)
> +
> +        try:
> +            initrd = fetcher.acquireFile(self.initrdpath, progresscb)
> +            return (kernel, initrd, args)
> +        except:
> +            os.unlink(kernel)
> +            raise RuntimeError(_("OpenSolaris microroot not found at %s") %
> +                self.initrdpath)


These new classes seeem to be more or less independant of the bit
os_type/virt_type renaming (baring the obvious fact that it uses the
new names). I've no problem add this bit right away.

> diff --git a/virtinst/VirtualDisk.py b/virtinst/VirtualDisk.py
> --- a/virtinst/VirtualDisk.py
> +++ b/virtinst/VirtualDisk.py
> @@ -519,6 +519,9 @@ class VirtualDisk(VirtualDevice):
>  
>          if self.target:
>              disknode = self.target
> +        if self.device == VirtualDisk.DEVICE_CDROM and disknode.startswith('xvd'):
> +            disknode = disknode + ':cdrom'
> +

This seems wrong - you should not pass "xvda:cdrom" into the libvirt
XML for a disk. There is an explicit attribute for specifying CDROM
media

    <disk type='file'  device='cdrom'>
        <source file='/some/path'/>
        <target dev='xvda'/>
   </disk>

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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