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

Revision history for this message
Josh Kearney (jk0) wrote :

Thanks Rick, this is some great feedback. I was able to fix everything with the exception of a few things below (comments inline):

> It looks like you only defined an implementation in xenapi_conn.py. We may
> want to add `poll_rescued_instances` to the other drivers with a
> NotImplementedError so someone can come along add at that in for us.

I did implement a `pass` method for libvirt, however, I didn't bother with HyperV since that layer is extremely bare at the moment. I suspect that will improve and change quite rapidly over time.

> As above, I think it would be better to use the regular destroy_*
> implementations and just pass the rescue_vm_ref instead of the usual vm_ref.

Unfortunately since the `instance` object isn't available in these circumstances, I had to break them down into their own rescue methods respectively. Looking at the bright side, this will allow me to provide more thorough test coverage for Rescue/Unrescue once this lands. Another possibility is the big XenAPI refactor that we've talked about, but that will likely warrant its own BP for a future release.

« Back to merge proposal