Change in vdsm[master]: [RFC] qemuimg: Replace CommandStream with Command
by Code Review
From Yaniv Bronhaim <ybronhei(a)redhat.com>:
Yaniv Bronhaim has posted comments on this change.
Change subject: [RFC] qemuimg: Replace CommandStream with Command
......................................................................
Patch Set 6:
(1 comment)
https://gerrit.ovirt.org/#/c/75009/6/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:
Line 230: def run(self):
Line 231: out = bytearray()
Line 232: for data in self._operation.irun():
Line 233: out += data
Line 234: self._update_progress(out)
but what about all those issues that leaded to deathSignal.. if the process is still stuck and vdsm moved on, it will stay alive. if vdsm will crash we will have zombies
Line 235:
Line 236: def abort(self):
Line 237: """
Line 238: Aborts running operation by sending a termination signal to the
--
To view, visit https://gerrit.ovirt.org/75009
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb29860e7ec8ebc260f129de2066515cc3229743
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Denis Chaplygin <dchaplyg(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 1 month
Change in vdsm[master]: [RFC] operation: Support streaming command output
by Code Review
From Yaniv Bronhaim <ybronhei(a)redhat.com>:
Yaniv Bronhaim has posted comments on this change.
Change subject: [RFC] operation: Support streaming command output
......................................................................
Patch Set 4:
(1 comment)
https://gerrit.ovirt.org/#/c/75008/4/lib/vdsm/storage/operation.py
File lib/vdsm/storage/operation.py:
Line 98: if src == procutils.OUT:
Line 99: yield data
Line 100: else:
Line 101: err += data
Line 102: self._finalize(b"", err)
v2v will need it as well.. no?
Line 103:
Line 104: def abort(self):
Line 105: """
Line 106: Attempt to terminate the child process from another thread.
--
To view, visit https://gerrit.ovirt.org/75008
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I976314ce3e9a1b4ba77d74c3c1ad742cc0e9fe80
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Denis Chaplygin <dchaplyg(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 1 month
Change in vdsm[master]: [RFC] procutils: Introduce the procutils module
by Code Review
From Yaniv Bronhaim <ybronhei(a)redhat.com>:
Yaniv Bronhaim has posted comments on this change.
Change subject: [RFC] procutils: Introduce the procutils module
......................................................................
Patch Set 9:
(4 comments)
https://gerrit.ovirt.org/#/c/74927/2//COMMIT_MSG
Commit Message:
Line 3: AuthorDate: 2017-03-28 03:38:45 +0300
Line 4: Commit: Nir Soffer <nsoffer(a)redhat.com>
Line 5: CommitDate: 2017-04-03 01:19:15 +0300
Line 6:
Line 7: [RFC] procutils: Introduce the procutils module
> WIP is too general, and usually means, don't waste your time on reviewing t
ok
Line 8:
Line 9: This module provide utilities for working with subprocesses.
Line 10:
Line 11: The first utility is procutils.communicate(), replacing both AsyncProc,
https://gerrit.ovirt.org/#/c/74927/9//COMMIT_MSG
Commit Message:
Line 3: AuthorDate: 2017-03-28 03:38:45 +0300
Line 4: Commit: Nir Soffer <nsoffer(a)redhat.com>
Line 5: CommitDate: 2017-04-03 01:19:15 +0300
Line 6:
Line 7: [RFC] procutils: Introduce the procutils module
please move to commands
Line 8:
Line 9: This module provide utilities for working with subprocesses.
Line 10:
Line 11: The first utility is procutils.communicate(), replacing both AsyncProc,
Line 5: CommitDate: 2017-04-03 01:19:15 +0300
Line 6:
Line 7: [RFC] procutils: Introduce the procutils module
Line 8:
Line 9: This module provide utilities for working with subprocesses.
then you don't introduce new module
Line 10:
Line 11: The first utility is procutils.communicate(), replacing both AsyncProc,
Line 12: and CommandStream. This function allows reading from 2 streams in the
Line 13: same time, without using callbacks as in CommandStream, and avoiding the
https://gerrit.ovirt.org/#/c/74927/9/lib/vdsm/common/procutils.py
File lib/vdsm/common/procutils.py:
Line 132: raise TimeoutExpired(p.pid)
Line 133: time.sleep(min(timeout, remaining))
Line 134: if timeout < 1.0:
Line 135: timeout *= 2
Line 136: log.debug("Process (pid=%d) terminated", p.pid)
if communicate is responsible for the wait, shouldn't it also be responsible to terminate it if its stuck for more than timeout?
--
To view, visit https://gerrit.ovirt.org/74927
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d193caa5da0ed564b4fab12aa85e3751f1a1df7
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Denis Chaplygin <dchaplyg(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 1 month
Change in vdsm[master]: utils: Remove CommandStream
by Code Review
From Yaniv Bronhaim <ybronhei(a)redhat.com>:
Yaniv Bronhaim has posted comments on this change.
Change subject: utils: Remove CommandStream
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit https://gerrit.ovirt.org/75011
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e4d890aeee02f0ad5e003d05579670b4d1682cc
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
7 years, 1 month
Change in vdsm[master]: [RFC] operation: Support streaming command output
by Code Review
From Yaniv Bronhaim <ybronhei(a)redhat.com>:
Yaniv Bronhaim has posted comments on this change.
Change subject: [RFC] operation: Support streaming command output
......................................................................
Patch Set 3:
(2 comments)
https://gerrit.ovirt.org/#/c/75008/3/lib/vdsm/storage/operation.py
File lib/vdsm/storage/operation.py:
Line 78: out, err = self._proc.communicate()
Line 79: self._finalize(out, err)
Line 80: return out
Line 81:
Line 82: def irun(self):
> Why, this conform to many python names with i prefix for the iterative vers
ok
Line 83: """
Line 84: Run a command, iterating on data received from underlying command
Line 85: stdout.
Line 86:
Line 89:
Line 90: Raises:
Line 91: `RuntimeError` if invoked more then once
Line 92: `exception.ActionStopped` if the command was aborted
Line 93: `cmdutils.Error` if the command failed
> This is the API contract. Without it, you have to read all the code under t
ok
Line 94: """
Line 95: self._start_process()
Line 96: err = bytearray()
Line 97: for src, data in procutils.communicate(self._proc):
--
To view, visit https://gerrit.ovirt.org/75008
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I976314ce3e9a1b4ba77d74c3c1ad742cc0e9fe80
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Denis Chaplygin <dchaplyg(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 1 month
Change in vdsm[master]: [RFC] procutils: Introduce the procutils module
by Code Review
From Yaniv Bronhaim <ybronhei(a)redhat.com>:
Yaniv Bronhaim has posted comments on this change.
Change subject: [RFC] procutils: Introduce the procutils module
......................................................................
Patch Set 8:
(2 comments)
https://gerrit.ovirt.org/#/c/74927/8/lib/vdsm/common/procutils.py
File lib/vdsm/common/procutils.py:
Line 115:
Line 116: _wait(p, deadline)
Line 117:
Line 118:
Line 119: def _wait(p, deadline=None):
> No no, this is a deadline, not a timeout - check how we treat it in line 13
how? if deadline-time<=0 you raise timeoutExpire. should it be deadlineReached?
Line 120: """
Line 121: Wait until process terminate or if deadline is specified,
Line 122: utils.monotonic_time() exceeeds deadline.
Line 123: """
Line 124: log.debug("Waiting for process (pid=%d)", p.pid)
Line 125: if deadline is None:
Line 126: p.wait()
Line 127: else:
Line 128: timeout = 1.0 / 2**8
> Why is it better to use integers? how do you suggest to use a counter?
don't you just count here for 1 to 8, then you can multiply it by IN MILISECOND\IN SECONDS in the sleep call
Line 129: while p.poll() is None:
Line 130: remaining = deadline - utils.monotonic_time()
Line 131: if remaining <= 0:
Line 132: raise TimeoutExpired(p.pid)
--
To view, visit https://gerrit.ovirt.org/74927
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d193caa5da0ed564b4fab12aa85e3751f1a1df7
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Denis Chaplygin <dchaplyg(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 1 month
Change in vdsm[master]: vdsm-tool: Introducing firewalld configurator
by Code Review
From Dan Kenigsberg <danken(a)redhat.com>:
Dan Kenigsberg has posted comments on this change.
Change subject: vdsm-tool: Introducing firewalld configurator
......................................................................
Patch Set 25: -Code-Review
--
To view, visit https://gerrit.ovirt.org/74883
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b9b5f533de0b609c15a2482b2fba0ae713c3c33
Gerrit-PatchSet: 25
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Leon Goldberg <lgoldber(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Leon Goldberg <lgoldber(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
7 years, 1 month
Change in vdsm[master]: vdsm-tool: Introducing firewalld configurator
by Code Review
From Dan Kenigsberg <danken(a)redhat.com>:
Dan Kenigsberg has posted comments on this change.
Change subject: vdsm-tool: Introducing firewalld configurator
......................................................................
Patch Set 25:
(2 comments)
https://gerrit.ovirt.org/#/c/74883/25/lib/vdsm/tool/configurators/firewal...
File lib/vdsm/tool/configurators/firewalld.py:
PS25, Line 36: investigate
please add a ref to http://lists.ovirt.org/pipermail/devel/2017-April/030074.html
I simply want this file to hold all that we know and all we need to know about firewalld configuration.
Line 35:
Line 36: # TODO: investigate/add missing ports/services
Line 37: #
Line 38: # virt:
Line 39: # serial consoles (tcp/2223)
please add ovirt-imageio and a url to your pull-request
Line 40: # ovn host tunnels (udp/6081)
Line 41: #
Line 42: # gluster:
Line 43: # gluster swift (tcp/8080)
--
To view, visit https://gerrit.ovirt.org/74883
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b9b5f533de0b609c15a2482b2fba0ae713c3c33
Gerrit-PatchSet: 25
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Leon Goldberg <lgoldber(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Leon Goldberg <lgoldber(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 1 month
Change in vdsm[master]: network: report dpdk ports as nics
by Code Review
From Dan Kenigsberg <danken(a)redhat.com>:
Dan Kenigsberg has posted comments on this change.
Change subject: network: report dpdk ports as nics
......................................................................
Patch Set 12:
(1 comment)
https://gerrit.ovirt.org/#/c/73987/12/lib/vdsm/network/netinfo/cache.py
File lib/vdsm/network/netinfo/cache.py:
PS12, Line 165: iface = get_net_iface_from_config(net, attrs)
nit: this can never raise NetworkIsMissing, so it should sit outside of the try-except block.
--
To view, visit https://gerrit.ovirt.org/73987
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I67b2083e9c6f032472fd75f6800c864083ffdef5
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman <igoihman(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 1 month
Change in vdsm[master]: nics: list dpdk ports
by Code Review
From Dan Kenigsberg <danken(a)redhat.com>:
Dan Kenigsberg has posted comments on this change.
Change subject: nics: list dpdk ports
......................................................................
Patch Set 23: Code-Review+1
--
To view, visit https://gerrit.ovirt.org/72308
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia07f5a1052146c0eea6f0e19aa22d68ff4148646
Gerrit-PatchSet: 23
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman <igoihman(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Peřina <mperina(a)redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
7 years, 1 month