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.