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

[Crístian Viana]

On 13-11-2014 05:02, simonjin at linux.vnet.ibm.com wrote:

> +    "KCHVM0043E": _("Failed destory vm %(name)s from migration recover. Details: %(err)s"),

*destroy

> @@ -33,6 +33,7 @@ from cherrypy.process.plugins import BackgroundTask
>   from kimchi import model, vnc
>   from kimchi.config import READONLY_POOL_TYPE
>   from kimchi.exception import InvalidOperation, InvalidParameter
> +from kimchi.model.libvirtconnection import LibvirtConnection
>   from kimchi.exception import NotFoundError, OperationFailed
>   from kimchi.model.config import CapabilitiesModel
>   from kimchi.model.tasks import TaskModel

We keep import statements sorted in alphabetical order. You need to move 
the line "from kimchi.model.libvirtconnection..." two lines below, right 
between "from kimchi.model.config..." and "from kimchi.model.tasks...", 
in alphabetical order.

> +            raise OperationFailed("KCHVM0036E", {'name': name,
> +                                                 'remoteHost': remoteHost})

The error message KCHVM0036E, in src/kimchi/i18n.py, doesn't contain the 
format masks 'name' and 'remoteHost' as provided in the exception above:

> "KCHVM0036E": _("Can't migrate to localhost."),

You need to provide exactly the same parameters declared in the error 
message. You either don't pass 'name' and 'remoteHost' when building 
that exception or you add 'name' and 'remoteHost' somewhere in the error 
message.

> +        destarch = _destConn.getInfo()[0]
> +        if self.conn.get().getInfo()[0] != destarch:
> +            kimchi_log.debug('vm %s can not migrate to different arch server.' % (name, destarch))
> +            raise OperationFailed("KCHVM0037E", {'name': name,
> +                                          'srcarch': self.conn.get().getInfo()[0],
> +                                          'destarch': destarch})

The code above is running "self.conn.get().getInfo()" more than once. 
You can store that value in a variable so you don't have to talk to 
libvirt multiple times.
Actually, "self.conn" is used in more places later in the code, so that 
value can also be stored in a separate value. Try not to contact libvirt 
several times for the same information, it has a performance cost.

> +        desthyp = _destConn.getType()
> +        if self.conn.get().getType() != desthyp:
> +            kimchi_log.debug('vm %s can not migrate to a different hypervisor' % name)
> +            raise OperationFailed("KCHVM0038E", {'name': self.name,
> +                                            'srchyp':self.conn.get().getType(),
> +                                            'desthyp':desthyp})

Again, store the value from "self.conn.getType()" in a local variable so 
you don't need to contact libvirt repeatedly. "self.conn" should already 
be in a separate variable according to my comment above.

Also, the error message KCHVM0038E contains the format masks 'srchyp' 
and 'desthyp' but you're providing 'name' as well. Those parameters 
don't match.

And "self.name" is an invalid reference, you need to use the local 
variable "name".

> +        #check if there is any passthrough devices belong to this vm
> +        vmhostdevs = VMHostDevsModel(conn = self.conn)

Usually we keep the model objects as instance variables. That means that 
"vmhostdevs" should be declared in VMModel's constructor - along with 
many other model variables - as "self.vmhostdevs".

Another useful verification to be added to "_preCheck" is to allow only 
one migration per VM happening at the same time. In order to do that, 
you need to check if a task with the 'target_uri' = 
'/vms/<vm-name>/migrate' is already running; that's the URI used when 
creating a migration task later in this code.

> +    #open destination connect to migrate
> +    @staticmethod
> +    def _getRemoteConn(self, remoteHost, user, transport = 'ssh'):
> +        duri = 'qemu+%s://%s@%s/system' % (transport,
> +                                user,remoteHost)
> +        conn = LibvirtConnection(duri)
> +        return conn

A @staticmethod shouldn't receive the parameter "self". You can read the 
section https://docs.python.org/2/library/functions.html#staticmethod to 
learn more about static methods in Python. When you need to call it, do 
something like:

VMModel._getRemoteConn(...)

Notice that the function is prefixed with the class, not the object 
instance, as its implementation doesn't depend on a specific instance.

> def _do_migrate(self, name, destConn, remoteHost, secure, cb):

This function is supposed to be executed from inside a Task. The 
parameters of such function should be, in that exact order:

def fn(cb, params):

where "params" is a dict containing the other parameters. That means 
that the parameters "name", "destConn", "remoteHost" and "secure" should 
be inside the dict "params", and you need to extract them from the dict 
before using them inside "_do_migrate".

> +        else:
> +            kimchi_log.debug('Can not migrate vm %s when its in %s.' % (name, state))
> +            raise OperationFailed("KCHVM0041E", {'name': self.name})

The error message KCHVM0041E contains the format masks 'name' and 
'state'. You need to provide both parameters when creating an exception 
with that message (i.e.'state' is missing).

Also, "self.name" is an invalid reference - use "name" instead.

> +        except libvirt.libvirtError as e:
> +            kimchi_log.error('migrate %s to %s failed' % (name,
> +                                                          remoteHost))
> +            self._recover(cb)
> +            raise OperationFailed('KCHVM0042E', {'err': e.message,

Are you sure that if an error occurs during migration, we need to 
undefine the remote domain? In my view, if the migration failed, there 
won't be a remote domain to undefine - well, it failed. And I couldn't 
find this on libvirt's documentation so I'm not sure of the behaviour in 
that case.

> +        finally:
> +            self._finish(name, destConn, cb)

The function "_finish" is called from inside the "finally" block, which 
means it will be called regardless of whether an exception was raised or 
not. However, that function calls "cb(..., True)", which indicates that 
the task has finished successfully. We should not call "cb(..., True)" 
if an exception was raised and the migration failed.

> +        destconn = self._getRemoteConn(remoteHost, user, passwd)

The function "_getRemoteConn" was defined as:

> def _getRemoteConn(self, remoteHost, user, transport = 'ssh'):

The parameters provided above don't match the defined parameters. The 
function expects a transport method in its last parameter but the code 
above is passing a password instead.

> +        # Record vm migrate task for future querying
> +        try:
> +            with self.objstore as session:
> +                session.store('migrate', name, task_id)
> +            return task_id
> +        except Exception as e:
> +            raise OperationFailed('KCHPOOL0037E', {'err': e.message})

As I commented in the previous e-mail thread, the function "add_task" 
will add the task to the object store. You don't need to do that.

Before sending every patch, you need to make sure it follows our code 
guideline. You can run the command "make check-local" on your shell and 
check its output. There are a few errors pointed out by that tool for 
this patch, so be sure to fix every one of them.

How do you test this code? Were you able to perform a VM migration 
successfully using this patch? You can try running the command below on 
your shell:

$ curl -s -k -u <username>:<password> -H 'Content-Type: 
application/json' -H 'Accept: application/json' 
https://localhost:8001/vms/<vm-name>/migrate -X POST -d '{"remoteHost": 
"<remote-host>", "user": "<remote-user>", "password": 
"<remote-password>", "secure": True}'

and make sure the migration actually happened.

Best regards,
Crístian.