[Kimchi-devel] [v2 1/1] Virtual machine migration

[simonjin]

Thanks for the review.

于 2014年11月13日 03:22, Crístian Viana 写道:
> On 11-11-2014 02:37, simonjin at linux.vnet.ibm.com wrote:
>> +from kimchi.utils import add_task
>
> There's already an import "kimchi.utils" below. Add the "add_task" 
> first to that list.
Will fix.
>
>>   from kimchi.exception import InvalidOperation, InvalidParameter
>> +from kimchi.model.libvirtconnection import LibvirtConnection
>>   from kimchi.exception import NotFoundError, OperationFailed
>
> Add that line in alphabetical order.
Will fix.
>
>>   from kimchi.model.templates import TemplateModel
>> +from kimchi.model.templates import TemplateModel
>
> Isn't that class already imported on the line above?
Will fix.
>>   from kimchi.model.utils import get_vm_name
>>   from kimchi.model.utils import get_metadata_node
>>   from kimchi.model.utils import set_metadata_node
>>   from kimchi.screenshot import VMScreenshot
>> +#from kimchi.model.vmhostdevs import VMHostDevsModel
>
> Unless that line is here due to debug status of this patch, there's no 
> need to keep it commented.
>
Will fix.
>> +        if remoteHost in ['localhost', '127.0.0.1']:
>> +            kimchi_log.debug('vm %s is not in running, only running 
>> vm can be migrated' % name)
>> +            raise OperationFailed("KCHVM00033E", {'name': name,
>> +                                                 'remoteHost': 
>> remoteHost})
>
> I don't think that is the reason why the VM cannot be migrated... By 
> looking at the code and at the message KCHVM00033E, the reason is that 
> the user is trying to migrate to localhost.
>
Will fix.
>> +        if self.conn.get().getInfo()[0] != _destConn.getInfo()[0]:
>> +            kimchi_log.debug('vm %s can not migrate to different 
>> arch server.' % (name, _destConn.getInfo()[0]))
>> +            raise OperationFailed("KCHVM00034E", {'name': name,
>> +                                          'srcarch': 
>> self.conn.get().getType(),
>> +                                          'destarch': 
>> _destConn.getType()})
>
> 'srcarch' and 'destarch' don't seem to contain the servers' 
> architectures, but their hypervisor type.
>
_destConn.getInfo()[0] contains the servers' architectures like x86_64 
or ppc.
> Also, only run "getInfo" (and other connection functions) once and 
> store its value on a local variable, especially because we're dealing 
> with remote connections and it might take a little more time to 
> execute them.
OK.
>> +
>> +        if self.conn.get().getType() != _destConn.getType():
>> +            kimchi_log.debug('vm %s can not migrate to a different 
>> hypervisor' % name)
>> +            raise OperationFailed("KCHVM00035E", {'name': self.name,
>> + 'srchyp':self.conn.get().getType(),
>> +                                            'desthyp': 
>> _destConn.getType()})
>
> As I commented earlier, only run "getType" once and store the values 
> on local variables.
>
>> +    def _getRemoteConn(self, remoteHost, user):
>> +        #open destination connect to migrate
>> +        transport = 'ssh'
>> +        duri = 'qemu+%s://%s@%s/system' % (transport,
>> +                                user,remoteHost)
>> +
>> +        conn = LibvirtConnection(duri)
>> +        return conn
>
> That function can be static, it doesn't use "self".
OK.
>> +    def _do_migrate(self, name, destConn, remoteHost, cb):
>> +        _destConn = destConn.get()
>> +        cb('starting a migration')
>> +        kimchi_log.debug('migrate %s start' % name)
>> +        #import pdb
>> +        #pdb.set_trace()
>> +        dom = self.get_vm(name, self.conn)
>> +        flag = 0
>> +        flag |= (libvirt.VIR_MIGRATE_PEER2PEER |
>> +                 libvirt.VIR_MIGRATE_TUNNELLED)
>> +
>> +        state = DOM_STATE_MAP[dom.info()[0]]
>> +        if state == 'running':
>> +            flag |= libvirt.VIR_MIGRATE_LIVE
>> +            try:
>> +                dom.migrateSetMaxDowntime(0)
>
> Isn't "migrateSetMaxDowntime" called in "_preMigrate", which is called 
> before "_do_migrate"?
>
Will clean this up.
>> +        # Record vm migrate task for future querying
>> +        try:
>> +            with self.objstore as session:
>> +                session.store('migrate', name, task_id)
>> +            return task_id
>
> If I'm not mistaken, the AsyncTask constructor (which is called inside 
> the function "add_task" above) already registers the task in the local 
> object store.
No, it doesn't .

-Simon