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

Fri Nov 14 09:04:23 UTC 2014

Thanks for your review.

于 2014年11月14日 01:54, 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.
>
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.