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

shavivi at redhat.com shavivi at redhat.com
Thu Jan 7 09:36:15 UTC 2016


Shahar Havivi has posted comments on this change.

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


Patch Set 10:

(16 comments)

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 (a
Done


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
Done
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
Done
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 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):
> +1 for using conetxt manager
Done
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 oper
Done
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 informati
Done
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?
Done
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
Done
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
Done
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?
when you use the ssh-add -d the process is terminated (not documented - from what I saw, I got exception when trying to kill it via ssh-agent -k)
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
Done
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.
we do need it,
look at the environment method
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:
sure but we need the _ssh_agent for its environment
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.
Done
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...
yes, now the __enter__ in the context manager
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?
yes
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