Change in vdsm[master]: WIP
by fromani＠redhat.com
Francesco Romani has uploaded a new change for review.
Change subject: WIP
......................................................................
WIP
Change-Id: I0beb75cc1d4a09b07402d16e4b8057e391daad8c
Signed-off-by: Francesco Romani <fromani(a)redhat.com>
---
M lib/vdsm/taskset.py
M lib/vdsm/udevadm.py
M lib/vdsm/utils.py
M lib/vdsm/virtsparsify.py
4 files changed, 11 insertions(+), 15 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/67/47967/1
diff --git a/lib/vdsm/taskset.py b/lib/vdsm/taskset.py
index 2ec6a16..c33c6cf 100644
--- a/lib/vdsm/taskset.py
+++ b/lib/vdsm/taskset.py
@@ -37,10 +37,7 @@
"""
command = [constants.EXT_TASKSET, '--pid', str(pid)]
- rc, out, err = utils.execCmd(command, resetCpuAffinity=False)
-
- if rc != 0:
- raise Error(rc, out, err)
+ utils.exec_cmd(command, resetCpuAffinity=False)
return _cpu_set_from_output(out[-1])
@@ -65,10 +62,7 @@
str(pid)
))
- rc, out, err = utils.execCmd(command, resetCpuAffinity=False)
-
- if rc != 0:
- raise Error(rc, out, err)
+ utils.exec_cmd(command, resetCpuAffinity=False)
def _cpu_set_from_output(line):
diff --git a/lib/vdsm/udevadm.py b/lib/vdsm/udevadm.py
index e4e3a9c..dd13120 100644
--- a/lib/vdsm/udevadm.py
+++ b/lib/vdsm/udevadm.py
@@ -100,6 +100,4 @@
def _run_command(args):
cmd = [_UDEVADM.cmd]
cmd.extend(args)
- rc, out, err = utils.execCmd(cmd, raw=True)
- if rc != 0:
- raise Error(rc, out, err)
+ utils.exec_cmd(cmd, raw=True)
diff --git a/lib/vdsm/utils.py b/lib/vdsm/utils.py
index d16f059..e447529 100644
--- a/lib/vdsm/utils.py
+++ b/lib/vdsm/utils.py
@@ -687,6 +687,13 @@
self.rc, self.out, self.err)
+def exec_cmd(cmd, *a, **kw):
+ rc, out, err = execCmd(cmd, *a, **kw)
+ if rc != 0:
+ raise Error(rc, out, err)
+ return rc, out, err
+
+
def stripNewLines(lines):
return [l[:-1] if l.endswith('\n') else l for l in lines]
diff --git a/lib/vdsm/virtsparsify.py b/lib/vdsm/virtsparsify.py
index 187e3bd..35099dc 100644
--- a/lib/vdsm/virtsparsify.py
+++ b/lib/vdsm/virtsparsify.py
@@ -50,7 +50,4 @@
cmd.extend((src_vol, dst_vol))
- rc, out, err = utils.execCmd(cmd, deathSignal=signal.SIGKILL)
-
- if rc != 0:
- raise Error(rc, out, err)
+ utils.exec_cmd(cmd, deathSignal=signal.SIGKILL)
--
To view, visit https://gerrit.ovirt.org/47967
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0beb75cc1d4a09b07402d16e4b8057e391daad8c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
8 years, 6 months
Change in vdsm[master]: virt: assign system-wide name to threads
by automation＠ovirt.org
automation(a)ovirt.org has posted comments on this change.
Change subject: virt: assign system-wide name to threads
......................................................................
Patch Set 10:
* Update tracker::IGNORE, no Bug-Url found
--
To view, visit https://gerrit.ovirt.org/41058
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa6f2afec140c9ae54601151cd51d87cba3ee232
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No
8 years, 6 months
Change in vdsm[master]: virt: assign system-wide name to threads
by fromani＠redhat.com
Francesco Romani has abandoned this change.
Change subject: virt: assign system-wide name to threads
......................................................................
Abandoned
we will have a fresh (re)start later.
--
To view, visit https://gerrit.ovirt.org/41058
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ifa6f2afec140c9ae54601151cd51d87cba3ee232
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
8 years, 6 months
Change in vdsm[master]: lib: set system name for threads
by automation＠ovirt.org
automation(a)ovirt.org has posted comments on this change.
Change subject: lib: set system name for threads
......................................................................
Patch Set 10:
* Update tracker::IGNORE, no Bug-Url found
--
To view, visit https://gerrit.ovirt.org/41057
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e8ffe2c598221d93ca154e4ee4f345568c5b35f
Gerrit-PatchSet: 10
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: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No
8 years, 6 months
Change in vdsm[master]: lib: set system name for threads
by fromani＠redhat.com
Francesco Romani has abandoned this change.
Change subject: lib: set system name for threads
......................................................................
Abandoned
we will have a fresh (re)start later.
--
To view, visit https://gerrit.ovirt.org/41057
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I9e8ffe2c598221d93ca154e4ee4f345568c5b35f
Gerrit-PatchSet: 10
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: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
8 years, 6 months
Change in vdsm[master]: config: Resolve crash when IPv6 disabled in kernel
by ibarkan＠redhat.com
Ido Barkan has posted comments on this change.
Change subject: config: Resolve crash when IPv6 disabled in kernel
......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/47978/1/vdsm/network/configurators/ifcfg.py
File vdsm/network/configurators/ifcfg.py:
Line 102: self._addSourceRoute(bridge)
Line 103: _ifup(bridge)
Line 104:
Line 105: # Check if ipv6 is disabled by kernel arguments
Line 106: has_ipv6 = True
> hmm, I think that we'd better expose this as a memoized function in caps.py
agreed.
Line 107: try:
Line 108: socket.socket(socket.AF_INET6, socket.SOCK_DGRAM)
Line 109: except socket.error:
Line 110: has_ipv6 = False
--
To view, visit https://gerrit.ovirt.org/47978
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I774cac868ddee08ac072adf2104a811594779052
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ollie Armstrong <ollie(a)fubra.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ollie Armstrong <ollie(a)fubra.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
8 years, 6 months
Change in vdsm[master]: lib: grub2 module
by fromani＠redhat.com
Francesco Romani has posted comments on this change.
Change subject: lib: grub2 module
......................................................................
Patch Set 3:
(9 comments)
few comments, -1 for visibility
Please add one-liners (or more, of course, if you need) defining "workaround" and "stub".
https://gerrit.ovirt.org/#/c/47946/3/lib/vdsm/grub2.py
File lib/vdsm/grub2.py:
Line 45: return "Process failed with rc=%d out=%r err=%r" % (
Line 46: self.rc, self.out, self.err)
Line 47:
Line 48:
Line 49: def mkconfig(args):
please add docstrings to functions. No need to document every parameter or to be verbose, just outline what functions should do.
Do we have a wiki page or anything describing the flow?
I mean the intended flow, not the implemented flow.
Line 50: cmd = [_GRUB2_MKCONFIG.cmd]
Line 51: cmd.extend(args)
Line 52: rc, out, err = utils.execCmd(_GRUB2_MKCONFIG, raw=True)
Line 53: if rc != 0:
Line 160:
Line 161: device_vendor = device_vendor.lstrip('0x')
Line 162: device_product = device_product.lstrip('0x')
Line 163:
Line 164: if stub_ids:
does the ordering matter? can we use a set?
Line 165: for vendor, product in stub_ids:
Line 166: if device_vendor == vendor and device_product == product:
Line 167: return
Line 168:
Line 163:
Line 164: if stub_ids:
Line 165: for vendor, product in stub_ids:
Line 166: if device_vendor == vendor and device_product == product:
Line 167: return
this check you implemented is duplicating the default behaviour of python. I think you can just check like this:
if [vendor, product] in stub_ids:
return
Line 168:
Line 169: stub_ids.append([device_vendor, device_product])
Line 170:
Line 171:
Line 165: for vendor, product in stub_ids:
Line 166: if device_vendor == vendor and device_product == product:
Line 167: return
Line 168:
Line 169: stub_ids.append([device_vendor, device_product])
why list and not tuple?
Line 170:
Line 171:
Line 172: def _del_from_stub(stub_ids, device_vendor, device_product):
Line 173:
Line 173:
Line 174: device_vendor = device_vendor.lstrip('0x')
Line 175: device_product = device_product.lstrip('0x')
Line 176:
Line 177: if stub_ids:
does it work without the if? To iterate in an empty collection should exit immediately.
Line 178: for idx, (vendor, product) in enumerate(stub_ids):
Line 179: if device_vendor == vendor and device_product == product:
Line 180: del(stub_ids[idx])
Line 181: break
Line 193:
Line 194:
Line 195: def _add_workaround(workarounds, workaround):
Line 196: try:
Line 197: workarounds.index(workaround)
same: do we care about the ordering? If so, please document it why. Otherwise, let's just use a set
Line 198: except ValueError:
Line 199: workarounds.append(workaround)
Line 200:
Line 201:
https://gerrit.ovirt.org/#/c/47946/3/tests/grub2Tests.py
File tests/grub2Tests.py:
Line 55:
Line 56: self._assertIntegrity(data)
Line 57: self.assertIn('"pci-stub.ids=cafe:beef"', data)
Line 58:
Line 59: def testAddDuplicateStub2(self):
please use a more describing name, and higlight the differences with respect to the other test
Line 60: with tempfile.NamedTemporaryFile() as f:
Line 61: grub2.add_to_stub(
Line 62: ['cafe', '0xcafe'], ['beef', '0xbeef'],
Line 63: cfg_source=self._test_file('grub2_default.out'),
Line 71: def testAddMultipleStubs(self):
Line 72: with tempfile.NamedTemporaryFile() as f:
Line 73: grub2.add_to_stub(
Line 74: ['cafe', '0xdead', '0xf00d', 'b002'],
Line 75: ['0xbeef', 'feed', '0xc00l', 'fa11'],
I missed this reviewing the code! so question: why is it helpful to support lists and not just one argument?
I mean, what's the use case?
Furthermore, maybe having a single list of pairs (tuples) is a bit more easier to read. Did you try that approach?
Line 76: cfg_source=self._test_file('grub2_default.out'),
Line 77: cfg_dest=f.name)
Line 78:
Line 79: data = f.read()
Line 169:
Line 170: self._assertIntegrity(data)
Line 171: self.assertIn('VDSM_WORKAROUNDS="a=b"', data)
Line 172:
Line 173: def testAddDuplicateWorkaround2(self):
same as for stub
Line 174: with tempfile.NamedTemporaryFile() as f:
Line 175: grub2.add_workaround(
Line 176: ['a=b', 'a=b'],
Line 177: cfg_source=self._test_file('grub2_default.out'),
--
To view, visit https://gerrit.ovirt.org/47946
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e7e0b327882f64196a71fe20dcbd99017b80800
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
8 years, 6 months
Change in vdsm[master]: lib: grub2 module
by fromani＠redhat.com
Francesco Romani has posted comments on this change.
Change subject: lib: grub2 module
......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/47946/1/lib/vdsm/grub2.py
File lib/vdsm/grub2.py:
Line 143: if not val:
Line 144: continue
Line 145: workarounds.append(val)
Line 146:
Line 147: return workarounds
> Not really, as split would introduce empty element.
uhm I probably miss something but:
workarounds = [
val for val in (grub_cfg[_CFG_NAME_WORKAROUNDS].strip('"').split(' '))
if val
]
?
btw, it is very minor
Line 148:
Line 149:
Line 150: def _add_workaround(workarounds, workaround):
Line 151: try:
--
To view, visit https://gerrit.ovirt.org/47946
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e7e0b327882f64196a71fe20dcbd99017b80800
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
8 years, 6 months
Change in vdsm[master]: vdsm: introduce cpuinfo module
by fromani＠redhat.com
Francesco Romani has posted comments on this change.
Change subject: vdsm: introduce cpuinfo module
......................................................................
Patch Set 10:
nice, we are asking changes in the opposite directions :)
to unlock the checkmate, let's step back and do what Dan asks, which is pretty much what you had before my comment. We'll see in a later patch if my idea works better or not
--
To view, visit https://gerrit.ovirt.org/46912
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa702b05f3825ebdcfed16d86d39a8c38fcf224c
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(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: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No
8 years, 6 months
