[Ovirt-devel] [PATCH server] Taskomatic Refactoring and Qpidification Take 3

[Ian Main]

I just snipped this up.  I'm making up a new patch that addresses almost
all of your concerns.. the ones that it doesn't address I'll discuss
here.  I think this is a bit of a work in progress and I'll have some
patches to clean stuff up in the following days.

On Tue, 06 Jan 2009 10:39:05 +0100
Chris Lalancette <clalance at redhat.com> wrote:

> Ian Main wrote:
> > This update adds a few small bugfixes, removes LVM storage pool/volume
> > scanning and reindents to 2 spaces.

[snip]

> > +  return LibvirtPool.factory(phys_volume.storage_pool)
> > +end
> > +
> > +class LibvirtPool
> > +
> > +  attr_reader :remote_pool
> 
> I've tried to avoid this before by trying to make accessor methods for the
> class; I think it is a little cleaner to not let external entities look into the
> class.   What are you using this for now?

Yes, understood.  The difficulty comes from the new search method used
by qpid. Instead of there being an API it now looks more like the
activerecord stuff, as I'm sure you've seen.  This attribute is used in
3 places to look up volumes that belong to the pool etc.  I'm 50/50 on
whether to change it so I'm thinking I'll leave it for now.

> > +  def task_start_vm(task)
> > +    db_vm = find_vm(task, false)
> > +
> > +    # XXX: Kinda silly?  I dunno about these intermediate states..
> > +    set_vm_state(db_vm, Vm::STATE_STARTING)
> > +
> > +    vm = @session.object(:class => "domain", 'uuid' => db_vm.uuid)
> > +
> > +    if vm
> > +      case vm.state
> > +        when "running"
> > +          return
> > +        when "blocked"
> > +          raise "Virtual machine state is blocked, cannot start VM."
> > +        when "paused"
> > +          raise "Virtual machine is currently paused, cannot start, must resume."
> >        end
> > -    else
> > -      begin
> > -        database_connect
> > -      rescue => ex
> > -        puts "3 #{ex.class}: #{ex.message}"
> > +    end
> > +    # FIXME: There's a bug here in that a host that's already running the vm won't be
> > +    # returned.  I think that's supposed to be for migration but it just breaks stuff.
> > +    db_host = find_capable_host(db_vm)
> 
> I'm not quite sure what this FIXME means.  If we are here in task_start_vm, and
> the vm is already running, then we have some sort of logical error.  Now, we
> should probably be robust to that error, but I think overloading
> find_capable_host() for that is the wrong way to go; we should just have another
> set of checks before this.  Oh, and it doesn't have anything to do with
> migration, as far as I can remember.

It's used to find the host to migrate a VM to as well as for the host
to start a VM on.  The problem I was thinking of is that if the VM is
defined, but not running on a single host, it will not allow it to use
that one and may not restart it, but I'm not sure that's really true.
I'll check this again later..  for now I rephrased my comment to say it
might be the case as a note to myself :).

> 
> > +
> > +    node = @session.object(:class => "node", 'hostname' => db_host.hostname)
> > +
> > +    raise "Unable to find host #{db_host.hostname} to create VM on." unless node
> > +
> > +    if (db_vm.boot_device == Vm::BOOT_DEV_CDROM) && db_vm.uses_cobbler? && (db_vm.cobbler_type == Vm::IMAGE_PREFIX)
> 
> I know you didn't put this here, but since you are moving things around...
> It would seem like a good idea to make a helper method with the Cobbler stuff in
> it.  I think that just makes it look cleaner, as the details in getting the
> Cobbler stuff isn't really interesting when you are reading the start_vm task.

Very good idea, moved it out.

[snip]

> > +    # Reget the db record or you can get 'dirty' errors.
> 
> Out of curiosity, why?  I'm not opposed to it, but this makes me think that
> there might be a problem lurking here.

I think you saw the other patch posted in regards to this.  The new
patch makes
judicious use of .reload to ensure we don't have a stale object.

[snip]

> General comments:
> 1.  Probably replace all of the XXX with FIXME, or vice-versa, so the whole
> thing is consistent.  That way people looking for things to fix can just look
> for one string (I prefer FIXME, but it's up to you).

Done.

> 2.  Try to make everything fit 80 columns again, just for ease of reading.

Mostly done.. it's better anyway.

> 3.  Despite what I said about it when we talked earlier, I find this kind of
> hard to read being all in one file.  The problem is that it is not clear which
> methods are top-level methods (i.e. task_start_vm) and which one are helper
> methods from a quick glance.  I think I would much prefer to have the helper
> methods in a separate file and/or separate class, but I'm not sure how difficult
> that would be.  Thoughts?

Hrrm, I dunno, I find it fine.  All the main methods that implement
tasks start with task_.  The difficulty comes from having to keep track
of @session and also possibly in the future, locking.. which is what I
was thinking of when I moved these operations into a single class..
Maybe there is a way to make 'friend' type classes?  Not sure..

Anyway, everything else is addressed in the patch I'll send out in a
bit.

	Ian