Thanks for your review. 于 2014年11月14日 01:54, Crístian Viana 写道:
On 13-11-2014 05:02, simonjin@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.
OK
+ 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.
self.conn it self doesn't cost anything.
+ 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".
Due to the reason in code that VMHostDevsModel can't be imported at the top of the class, the workaround is import and get the instance here.
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.
Will do.
+ #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.
OK
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".
OK
+ 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.
OK
+ 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.
OK
+ 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.
Will need this to verify whether the vm is already in migration, right ?
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.