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

[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.

>   from kimchi.exception import InvalidOperation, InvalidParameter
> +from kimchi.model.libvirtconnection import LibvirtConnection
>   from kimchi.exception import NotFoundError, OperationFailed

Add that line in alphabetical order.

>   from kimchi.model.templates import TemplateModel
> +from kimchi.model.templates import TemplateModel

Isn't that class already imported on the line above?
>   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.

> +        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.

> +        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.

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.
> +
> +        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".
> +    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"?

> +        # 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.