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

Revision history for this message
Rick Harris (rconradharris) wrote :

Thanks for the fixes jk0. Have a couple of more potential cleanups, nothing big:

> 136 @property
> 137 def name(self):
> 138 - return FLAGS.instance_name_template % self.id
> 139 + base_name = FLAGS.instance_name_template % self.id
> 140 + if getattr(self, '_rescue', False):
> 141 + base_name += "-rescue"
> 142 + return base_name

I know I suggested this, but I'm having second thoughts about putting this
logic here. At least right now, this is XenAPI hackiness, I'm not sure this
belongs in the generic instance model.

Instead, I think the XenAPI code should have a method, `get_name_label` which
it uses instead of instance.name to generate the VM name-labels. That code
would look like:

def get_name_label(instance):
    """Generate name label for the XenServer VM record

    We have two scenarios:

        1. We're in normal mode and the INSTANCE VM record uses the instance name as
           its name label

        2. We're in rescue mode and the RESCUE VM Record uses instance name +
           '-rescue' as its name label
    """
    base_name = instance.name
    if getattr(self, '_rescue', False):
        base_name += "-rescue"
    return base_name

The code is really pretty readable, but, it still might be a good idea to
include a big doc-string on the XenAPI rescue method that explains, at a
high-level, what's involved in a rescue, like:

"""
Rescue consists of a number of steps

1. We shutdown the INSTANCE VM

2. We apply a 'bootlock' to prevent the instance VM from being started while
   in rescue

2. We spawn a RESCUE VM (the vm name-label in this case will be
   instance-0002-rescue)

3. We attach the INSTANCE VM's VBDs to the RESCUE VM

Unrescue consists of:

1. We unplug the INSTANCE VM's from the RESCUE VM

2. We teardown the RESCUE VM

3. Release the bootlock to allow the INSTANCE VM to be started

4. Start the INSTANCE VM
"""

« Back to merge proposal