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+1
--
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]: 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 19:
(1 comment)
https://gerrit.ovirt.org/#/c/72308/19/tests/network/dpdk_test.py
File tests/network/dpdk_test.py:
PS19, Line 50: _
some of us deslike testing private functions.
better test a public one, mocking whatever disturbs its calculation.
--
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: 19
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: 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 19:
(1 comment)
https://gerrit.ovirt.org/#/c/72308/19/lib/vdsm/network/link/dpdk.py
File lib/vdsm/network/link/dpdk.py:
PS19, Line 37: _create
a "create" called in a getter is confusing
--
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: 19
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: Yes
7 years, 1 month
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 5:
(7 comments)
https://gerrit.ovirt.org/#/c/75009/5//COMMIT_MSG
Commit Message:
Line 7: [RFC] qemuimg: Replace CommandStream with Command
Line 8:
Line 9: Now that operation.Command supports streaming, we can use it to process
Line 10: qemu-img progress reports. This simplifies qemuimg.QemuImgOperation as
Line 11: most of the logic is already implemented in operation.Command.
what logic?
Line 12:
Line 13: This also simplify and make caller code safer, as operation.Command()
Line 14: cannot be used incorrectly, and does not need to be closed when done.
Line 15:
Line 16: Since operation.Command() raises cmdutis.Error(), qemuimg.convert() and
Line 17: qemuimg.commit() raise now cmdutils.Error() instead of
Line 18: qemuimg.QImgError(). Since callers needs an easy way to catch both
Line 19: cmdutils.Error(), and qemuimg.InvalidOutput(), qemuimg.InvalidOutput()
Line 20: inherits now from cmdutils.Error(), and qemuimg.QImgError() was removed.
this sounds big in addition to the replacement.. maybe worth be splited from this patch (if possible.. haven't checked deeply)?
Line 21:
Line 22: Change-Id: Icb29860e7ec8ebc260f129de2066515cc3229743
https://gerrit.ovirt.org/#/c/75009/5/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:
Line 1: #
Line 2: # Copyright 2012-2016 Red Hat, Inc.
2017
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or
Line 23: import logging
Line 24: import os
Line 25: import re
Line 26:
Line 27: from vdsm import cmdutils
from . import cmutils like that rest..
Line 28: from vdsm.common import exception
Line 29: from vdsm.storage import operation
Line 30:
Line 31: from . import commands
https://gerrit.ovirt.org/#/c/75009/5/tests/qemuimg_test.py
File tests/qemuimg_test.py:
Line 1: #
Line 2: # Copyright 2014 Red Hat, Inc.
2017
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or
https://gerrit.ovirt.org/#/c/75009/5/tests/storage_sdm_amend_volume_test.py
File tests/storage_sdm_amend_volume_test.py:
Line 1: #
Line 2: # Copyright 2016 Red Hat, Inc.
2017
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or
https://gerrit.ovirt.org/#/c/75009/5/vdsm/storage/image.py
File vdsm/storage/image.py:
Line 1: #
Line 2: # Copyright 2009-2016 Red Hat, Inc.
2017
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or
--
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: 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: Denis Chaplygin <dchaplyg(a)redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland(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]: 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 4:
(2 comments)
https://gerrit.ovirt.org/#/c/75011/4/lib/vdsm/utils.py
File lib/vdsm/utils.py:
Line 1: #
Line 2: # Copyright 2008-2016 Red Hat, Inc.
2017
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or
https://gerrit.ovirt.org/#/c/75011/4/tests/utils_test.py
File tests/utils_test.py:
Line 1: #
Line 2: # Copyright 2012-2016 Red Hat, Inc.
2017
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or
--
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: 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: Freddy Rolland <frolland(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]: [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):
itr_run is more appropriate name
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
those are not raised from this func .. underline implementation might require changes in this comment. isn't that a recipe for wrong comments? I see you're doing that in more func docs
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: 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]: operation: Prepare for integrating CommandStream
by Code Review
From Yaniv Bronhaim <ybronhei(a)redhat.com>:
Yaniv Bronhaim has posted comments on this change.
Change subject: operation: Prepare for integrating CommandStream
......................................................................
Patch Set 2:
(1 comment)
https://gerrit.ovirt.org/#/c/74618/2/lib/vdsm/storage/operation.py
File lib/vdsm/storage/operation.py:
Line 125: self._proc = compat.CPopen(self._cmd,
Line 126: cwd=self._cwd,
Line 127: stdin=None,
Line 128: stdout=subprocess.PIPE,
Line 129: stderr=subprocess.PIPE)
putting a call to CPopen under is a lock sounds risky. under the lock you should only change the state and release it quickly
Line 130: self._state = RUNNING
Line 131:
Line 132: def _finalize(self, out, err):
Line 133: """
--
To view, visit https://gerrit.ovirt.org/74618
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff363e710fcdc5d6cd5843ca2239973c89a69dd3
Gerrit-PatchSet: 2
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//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-02 05:19:08 +0300
Line 6:
Line 7: [RFC] procutils: Introduce the procutils module
I don't see a reason not to put this code under commands.py
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/8/lib/vdsm/common/procutils.py
File lib/vdsm/common/procutils.py:
Line 45:
Line 46: def communicate(p, timeout=None, bufsize=io.DEFAULT_BUFFER_SIZE):
Line 47: """
Line 48: Communicate with process, yielding data read from stdout and stderr until
Line 49: proccess terminates or timeout expires.
1. please compare the differences between this implementation to subprocess.Popen.communicate [below]- the builtin one waits until ... and this one is until .. and we needed that for 1 2 3 ..
2. for infra its more appropriate to keep the same func signature
def communicate(self, input=None, timeout=None):
"""Interact with process: Send data to stdin. Read data from
stdout and stderr, until end-of-file is reached. Wait for
process to terminate.
The optional "input" argument should be data to be sent to the
child process (if self.universal_newlines is True, this should
be a string; if it is False, "input" should be bytes), or
None, if no data should be sent to the child.
communicate() returns a tuple (stdout, stderr). These will be
bytes or, if self.universal_newlines was True, a string.
"""
Line 50:
Line 51: Unlike Popen.communicate, this support a timeout, and allows reading both
Line 52: stdout and stderr with a single thread.
Line 53:
--
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]: [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 2:
(6 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-03-31 05:18:48 +0300
Line 6:
Line 7: [RFC] procutils: Introduce the procutils module
> I adapted this tag from qemu mailing list, seems like a good way to tag thi
wip is the same thing.. is it wrong description for the work? if I would suggest new header, I guess you would ask me to suggest it in the list first ..
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-03-31 05:18:48 +0300
Line 6:
Line 7: [RFC] procutils: Introduce the procutils module
Line 8:
Line 9: This module provide utilities for working with subprocesses.
> Maybe we can move here helper slike utils.terminating()?
what does it mean that it is based? did you copy it? can we reference to py3 code? why not to backport the exact code?
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
Line 36: test_asyncproc_write 1.00g in 13.71 seconds (0.07g/s) OK
Line 37: test_plain_read 1.00g in 0.45 seconds (2.22g/s) OK
Line 38: test_read 1.00g in 0.53 seconds (1.89g/s) OK
Line 39: test_write 1.00g in 0.49 seconds (2.04g/s) OK
Line 40:
> I don't think so, if you want to buffer both stdout and stderr, the builtin
mention it in the commit msg
Line 41: Change-Id: I7d193caa5da0ed564b4fab12aa85e3751f1a1df7
https://gerrit.ovirt.org/#/c/74927/8/lib/vdsm/common/procutils.py
File lib/vdsm/common/procutils.py:
Line 115: if timeout is not None:
Line 116: timeout = deadline - utils.monotonic_time()
Line 117: if timeout <= 0:
Line 118: raise TimeoutExpired(p.pid)
Line 119: if not fds:
deadline==timeout. call it timeout
Line 124
Line 125
Line 126
Line 127
Line 128
better to work with integers if such calculations are not mandatory. why don't you use simpler counter?
Line 132
Line 133
Line 134
Line 135
Line 136
don't we care if it ended or timeouted?
--
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: 2
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]: vmdevices: stop using self.conf
by Code Review
From Dan Kenigsberg <danken(a)redhat.com>:
Dan Kenigsberg has posted comments on this change.
Change subject: vmdevices: stop using self.conf
......................................................................
Patch Set 19: Code-Review+1
--
To view, visit https://gerrit.ovirt.org/70925
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8918e8e073fc55f2bae3a74d6279bda062d9930
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
7 years, 1 month