Change in vdsm[master]: v2v: support for importing Xen VMs via libvirt on RHEL hosts

nsoffer at redhat.com nsoffer at redhat.com
Wed Jan 6 09:25:33 UTC 2016


Nir Soffer has posted comments on this change.

Change subject: v2v: support for importing Xen VMs via libvirt on RHEL hosts
......................................................................


Patch Set 10:

(20 comments)

https://gerrit.ovirt.org/#/c/49505/10/lib/api/vdsmapi-schema.json
File lib/api/vdsmapi-schema.json:

Line 4017: # Convert VM from RHEL Xen hypervisor
Line 4018: #
Line 4019: # @uri:          libvirt connection uri
Line 4020: #
Line 4021: # @vminfo:       information of the VM such as name, id etc
> minor: this is not very clear, could you please expand/rephrase a bit?
This returns a ExternalVmInfo, the reader should check that type documentation.
Line 4022: #
Line 4023: # @jobid:        Assign a UUID to this operation which can be used
Line 4024: #                to identify it in @HostStats
Line 4025: #


Line 4022: #
Line 4023: # @jobid:        Assign a UUID to this operation which can be used
Line 4024: #                to identify it in @HostStats
Line 4025: #
Line 4026: # Since: 4.17.0
4.18
Line 4027: ##
Line 4028: {'command': {'class': 'Host', 'name': 'convertXenVm'},
Line 4029:   'data': {'uri': 'str', 'vminfo': 'ExternalVmInfo', 'jobid': 'UUID'}}
Line 4030: 


https://gerrit.ovirt.org/#/c/49505/10/vdsm/API.py
File vdsm/API.py:

Line 1466:         found = jobs.info(job_type=job_type, job_ids=job_ids)
Line 1467:         return response.success(jobs=found)
Line 1468: 
Line 1469:     def convertXenVm(self, uri, vminfo, jobid):
Line 1470:         return v2v.convert_xen_vm(uri, vminfo, jobid, self._irs)
Why do we need separate verb for this? why not run the special code for zen based on the uri?
Line 1471: 
Line 1472:     def getConvertedVm(self, jobid):
Line 1473:         return v2v.get_converted_vm(jobid)
Line 1474: 


https://gerrit.ovirt.org/#/c/49505/10/vdsm/rpc/bindingxmlrpc.py
File vdsm/rpc/bindingxmlrpc.py:

Line 379:     def convertExternalVmFromOva(self, ova_path, vminfo, jobid):
Line 380:         api = API.Global()
Line 381:         return api.convertExternalVmFromOva(ova_path, vminfo, jobid)
Line 382: 
Line 383:     def convertXenVm(self, uri, username, password, vminfo, jobid):
Why this requires a password if the underlying call ignore the password?
Line 384:         password = ProtectedPassword(password)
Line 385:         api = API.Global()
Line 386:         return api.convertXenVm(uri, vminfo, jobid)
Line 387: 


https://gerrit.ovirt.org/#/c/49505/10/vdsm/v2v.py
File vdsm/v2v.py:

Line 155
Line 156
Line 157
Line 158
Line 159
We can parse the uri and use the right coommand, instead of adding a new (and identical) function.


Line 174:     _add_job(job_id, job)
Line 175:     return response.success()
Line 176: 
Line 177: 
Line 178: def convert_xen_vm(uri, vminfo, job_id, irs):
Is this temporary api we plan to drop soon? If not, using convert_external_vm seems better.
Line 179:     command = XenCommand(uri, vminfo, job_id, irs)
Line 180:     job = ImportVm(job_id, command)
Line 181:     job.start()
Line 182:     _add_job(job_id, job)


Line 291: 
Line 292: class SSHAgent(object):
Line 293:     def __init__(self):
Line 294:         self._auth = None
Line 295:         self._ssh_agend_pid = None
_ssh_agend_pid -> _agent_pid

We don't manage here multiple processes, so there is no need to repeat the class name.
Line 296:         self._ssh_auth_re = re.compile(_SSH_AUTH_RE)
Line 297: 
Line 298:     def init(self):
Line 299:         rc, out, err = execCmd([_SSH_AGENT.cmd],


Line 294:         self._auth = None
Line 295:         self._ssh_agend_pid = None
Line 296:         self._ssh_auth_re = re.compile(_SSH_AUTH_RE)
Line 297: 
Line 298:     def init(self):
> minor: this is not the best name around, reminds too much __init__. I'd jus
+1 for using conetxt manager
Line 299:         rc, out, err = execCmd([_SSH_AGENT.cmd],
Line 300:                                deathSignal=signal.SIGTERM,
Line 301:                                nice=NICENESS.HIGH,
Line 302:                                ioclass=IOCLASS.IDLE)


Line 298:     def init(self):
Line 299:         rc, out, err = execCmd([_SSH_AGENT.cmd],
Line 300:                                deathSignal=signal.SIGTERM,
Line 301:                                nice=NICENESS.HIGH,
Line 302:                                ioclass=IOCLASS.IDLE)
We don't need nice and ionice for this, this does not do heavy storage operations.

deathSignal is not needed since you are waiting for the commnad, execCmd should ensure that the command is killed on errors. We must get rid of deathSignal for Python 3, so new code should not use it.
Line 303:         if rc != 0:
Line 304:             raise V2VError('Error init ssh-agent, exit code: %r, msg: %r' %
Line 305:                            (rc, err))
Line 306: 


Line 301:                                nice=NICENESS.HIGH,
Line 302:                                ioclass=IOCLASS.IDLE)
Line 303:         if rc != 0:
Line 304:             raise V2VError('Error init ssh-agent, exit code: %r, msg: %r' %
Line 305:                            (rc, err))
Include also the output in the error, in case it contains helpful information.
Line 306: 
Line 307:         m = self._ssh_auth_re.match(''.join(out))
Line 308:         # looking for: SSH_AUTH_SOCK=/tmp/ssh-VEE74ObhTWBT/agent.29917
Line 309:         self._auth = {m.group(1): m.group(2)}


Line 305:                            (rc, err))
Line 306: 
Line 307:         m = self._ssh_auth_re.match(''.join(out))
Line 308:         # looking for: SSH_AUTH_SOCK=/tmp/ssh-VEE74ObhTWBT/agent.29917
Line 309:         self._auth = {m.group(1): m.group(2)}
Can you explain how authentication using ssh agent works?

A class docstring explaining the flow would be best, making it easy for future developer to maintain this code.
Line 310:         self._ssh_agend_pid = m.group(2).split('.')[1]
Line 311: 
Line 312:         rc, out, err = execCmd([_SSH_ADD.cmd],
Line 313:                                deathSignal=signal.SIGTERM,


Line 311: 
Line 312:         rc, out, err = execCmd([_SSH_ADD.cmd],
Line 313:                                deathSignal=signal.SIGTERM,
Line 314:                                nice=NICENESS.HIGH,
Line 315:                                ioclass=IOCLASS.IDLE,
nice, ionice, deathSignal not needed
Line 316:                                env=self._auth)
Line 317: 
Line 318:         if rc != 0:
Line 319:             raise V2VError('Error init ssh-add, exit code: %r, msg: %r' %


Line 322:     def delete(self):
Line 323:         rc, out, err = execCmd([_SSH_ADD.cmd, '-D'],
Line 324:                                deathSignal=signal.SIGTERM,
Line 325:                                nice=NICENESS.HIGH,
Line 326:                                ioclass=IOCLASS.IDLE,
nice, ionice, deathSignal not needed
Line 327:                                env=self._auth)
Line 328: 
Line 329:         if rc != 0:
Line 330:             logging.error('Error deleting ssh-add, exit code: %r, msg: %r' %


Line 323:         rc, out, err = execCmd([_SSH_ADD.cmd, '-D'],
Line 324:                                deathSignal=signal.SIGTERM,
Line 325:                                nice=NICENESS.HIGH,
Line 326:                                ioclass=IOCLASS.IDLE,
Line 327:                                env=self._auth)
What about the agent you started in init()? who is going to stop it?
Line 328: 
Line 329:         if rc != 0:
Line 330:             logging.error('Error deleting ssh-add, exit code: %r, msg: %r' %
Line 331:                           (rc, err))


Line 330:             logging.error('Error deleting ssh-add, exit code: %r, msg: %r' %
Line 331:                           (rc, err))
Line 332: 
Line 333:     @property
Line 334:     def authenticatoin(self):
Use same name for this and the underlying instance variable (_auth). Either auth and _auth, or authentication and _authentication.
Line 335:         return self._auth
Line 336: 
Line 337: 
Line 338: class V2VCommand(object):


Line 505: class XenCommand(V2VCommand):
Line 506:     def __init__(self, uri, vminfo, job_id, irs):
Line 507:         super(XenCommand, self).__init__(vminfo, job_id, irs)
Line 508:         self._uri = uri
Line 509:         self._ssh_agent = SSHAgent()
We don't need to keep the agent as instance variable.
Line 510: 
Line 511:     def _command(self):
Line 512:         cmd = [_VIRT_V2V.cmd,
Line 513:                '-ic', self._uri,


Line 527:         return cmd
Line 528: 
Line 529:     @contextmanager
Line 530:     def execute(self):
Line 531:         with self._volumes(), self._agent():
If the agent implements a context manager, we can do:

   with self._volumes(), self._ssh_agent:
       yield self._start_virt_v2v()

And since we don't really need to keep the agent as instance variable:

   with self._volumes(), SSHAgent():
       yield self._start_virt_v2v()
Line 532:             yield self._start_virt_v2v()
Line 533: 
Line 534:     @contextmanager
Line 535:     def _agent(self):


Line 531:         with self._volumes(), self._agent():
Line 532:             yield self._start_virt_v2v()
Line 533: 
Line 534:     @contextmanager
Line 535:     def _agent(self):
If the agent implements a context manager, we don't need this.
Line 536:         self._ssh_agent.init()
Line 537:         try:
Line 538:             yield
Line 539:         finally:


Line 532:             yield self._start_virt_v2v()
Line 533: 
Line 534:     @contextmanager
Line 535:     def _agent(self):
Line 536:         self._ssh_agent.init()
This should start the ssh agent and add the auth to it...
Line 537:         try:
Line 538:             yield
Line 539:         finally:
Line 540:             self._ssh_agent.delete()


Line 536:         self._ssh_agent.init()
Line 537:         try:
Line 538:             yield
Line 539:         finally:
Line 540:             self._ssh_agent.delete()
And this removes the auth from the agent and should stop it - right?
Line 541: 
Line 542:     def _environment(self):
Line 543:         env = super(XenCommand, self)._environment()
Line 544:         env.update(self._ssh_agent.authenticatoin)


-- 
To view, visit https://gerrit.ovirt.org/49505
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If544c0247003b6bc7189f72130553b9402dc915d
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list