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

On 13-11-2014 05:02, simonjin(a)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.