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