Code review comment for lp:~jk0/nova/xs-rescue

Good work jk0, this is a great start.

Some small nits:

> 80 + self.driver.rescue(instance_ref,
> 81 + lambda result: self._update_state_callback(self,
> 82 + context,
> 83 + instance_id,
> 84 + result))

Breaking this into two lines would probably aid readability.

> 145 + def rescue(self, instance, callback=None):

The callback function isn't firing for rescue or unrescue. It looks like the
other methods are using

        self._wait_with_callback(instance.id, task, callback)

    To fire it off.

> 342 + def _bootlock(self, vm, unlock=False):
> 343 + """Prevent an instance from booting"""
> 344 + if unlock:
> 345 + self._session.call_xenapi(
> 346 + "VM.remove_from_blocked_operations",
> 347 + vm,
> 348 + "start")
> 349 + else:
> 350 + self._session.call_xenapi(
> 351 + "VM.set_blocked_operations",
> 352 + vm,
> 353 + {"start": ""})
> 354 +

I think this make more sense as two methods:
_acquire_bootlock and _release_bootlock.

Another thing you could do, is assert that we're in the correct state. For
example, that we're not trying to unlock a VM that's already unlocked (that'd
indicate a bug in the code).

> 394 + try:
> 395 + task = self._session.call_xenapi("Async.VM.clean_shutdown", vm)
> 396 + self._session.wait_for_task(task, instance.id)
> 397 + except self.XenAPI.Failure:
> 398 + task = self._session.call_xenapi("Async.VM.hard_shutdown", vm)
> 399 + self._session.wait_for_task(task, instance.id)

By default, I think _shutdown should perform either a clean or hard shutdown
depending on whats requested (Dietz added a hard=true kwarg in his
xs-migrations branch).

If at all possible, I'd like to avoid silent-fall-backs (like from clean to
hard), because they're usually indicative of a larger problem

That said, a _force_shutdown that attempts a clean and then falls-back to
hard would be okay since it's very explicity when that behavior is being
requested (And will be easier to remove later on if we want to).

> 484 + if vbd["userdevice"] == str(1):

str(1) can just be "1".

> 488 + task1 = self._session.call_xenapi("Async.VM.hard_shutdown", rescue_vm)
> 489 + self._session.wait_for_task(task1, instance.id)

This could be something like _shutdown(instance, rescue_vm, hard=True).

> 491 + vdis = VMHelper.lookup_vm_vdis(self._session, rescue_vm)
> 492 + for vdi in vdis:
> 493 + try:
> 494 + task = self._session.call_xenapi('Async.VDI.destroy', vdi)
> 495 + self._session.wait_for_task(task, instance.id)
> 496 + except self.XenAPI.Failure:
> 497 + continue

Should be able to use `_destroy_vdis` here.

> 499 + task2 = self._session.call_xenapi('Async.VM.destroy', rescue_vm)
> 500 + self._session.wait_for_task(task2, instance.id)

Should be able to use _destroy_vm here.

> 514 + try:
> 515 + device = "1" if instance._rescue else "0"
> 516 + except AttributeError:
> 517 + device = "0"

Might be clearer written as:

device = "1" if VMHelper.in_rescue(instance) else "0"

By abstracting out in_rescue, we can choose later to change how we check for
the in_rescue state.

@staticmethod
def in_rescue(cls, instance)
    return getattr(instance, '_rescue', False)

Of course, this too is a little messy. Ultimately, I think we should OO-ify
much of this code--we have way to much instance-behavior being implemented in
the form of functions and classmethods, IMHO.

545 - self._session.wait_for_task(vol_rec['deviceNumber'], task)
546 + self._session.wait_for_task(task, vol_rec['deviceNumber'])

It looks like _wait_for_task takes an instance_id, so passing in a
deviceNumber doesn't make a whole lot of sense.

> 574 + def wait_for_task(self, task, id=None):

We should probably change `id` here to `instance_id` to eliminate some of the
confusion.