[et-mgmt-tools] Virt-Manager: Supporting additional para-virtual OS's

[Daniel P. Berrange]

On Thu, Nov 29, 2007 at 08:14:50PM -0800, Robert Nelson wrote:
> Daniel P. Berrange wrote:
> >>I spent some time reworking the virtinst code to support OpenSolaris and 
> >>make it much easier to support additional OSes.  I've attached the patch 
> >>file to get some feedback on the work so far.
> >>    
> >
> >Thanks for this. Took me a little while to get to grips with it, but I
> >like the resulting structure.  I'm going todo  a few tests and if it
> >works I'll commit it to the repo.
> >
> >BTW, for any large patches in the future, its handy for review if you
> >do separate patches  for re-factoring existing code, vs adding new
> >capabilities. It would have made it easier to review if the splitting
> >up of the DistroManager.py file were a simple no-functional-change
> >refactoring. Also helps people browsing the SCM history in the future
> >to distinguish the changes. No need to change this existing patch 
> >though, I'll just add as is...
> 
> Please hold off on committing this patch.  I have an updated and better 
> version coming shortly.  I'll post it as two patches to make the SCM 
> history more readable.  I'll also post it to a new thread.

So it turned out to be a little more complicated than I expected. At first
I liked the idea of having OSDistro create & use the ImageFetcher class
directly, however, there are circumstances where we need to use the OSDistro
class without the ImageFetcher, so this dependancy causes problems.

Second, the FullVirtGuest/ParaVirtGuest classes now call into the installer
class  to get the disk target - this is a problem because this code is only
relevant for the DistroInstaller class - the ImageInstaller works in a
different way.

Really, theFullVirt/ParaVirt guest class should not try todo any assignement
of disk targets at all. The 'prepare' method fo the DistroInstaller class
should contain the code to assign disk targets directly. This simplifies 
the get_disk_xml methods in the Full/ParaVirtGuest  classes, and avoids
the hard dependancy.

So, in the end I just committed two small parts. I pulled the ImageFetcher
classes out into a separate python module, and pulled the OSDistro 
classes out into a separate python module. So just 2 no-functional-change
refactorings for now.

If you post your latest patches I'll give them another try & see if there's
more bits we can safely commit as we go.

> >>Unfortunately a bunch for the code from virtinst is duplicated in 
> >>virt-manager in the Add Device code.  This means either moving it back 
> >>to virtinst with the appropriate additional APIs or duplicating work in 
> >>virt-manager.
> >>    
> >
> >We should make virt-manager call into the virtinst  APIs really. virtinst
> >is intended as the library for virt-manager to get all OS install/setup
> >logic from.
> >
> >  
> 
> If you don't mind, I'll put together a prototype of this for review.

Sure. The complicated thing about the 'Add device' code is that we no
longer have any record of the OS Distro type at that point - this info
is only available at install time.  So its not clear how we'd be able
to get an instance of the OSDistro class neccessary to figure out the
disk target names here. Also this is one use case I mentioned where
you need  OSDistro to be independant of the ImageFetcher class.


BTW,  I'm not convinced the Solaris distro class is correct. I know the
paravirt guests have to name their disk targets 0, 1, 2 ... etc isntead
of xvda, xvdb, etc.  For fully-virt  Solaris installs though it must
still use hda, hdb, hdc, etc as per Linux. This is because the hda, hdb,
etc names translate directly to QEMU command line args - they have no
resemblance to what the guest OS calls its disks - indeed modern Linux
will call them  sda, sdb, sdc, etc regardless of what QEMU calls tem.

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|