Change in vdsm[master]: virt: periodic: reduce NotConnectedError noise
Milan Zamazal has posted comments on this change. Change subject: virt: periodic: reduce NotConnectedError noise .. Patch Set 14: Code-Review-1 (2 comments) Looks fine to me, I'd just suggest to polish typos in the commit message (they may be more than those I marked). https://gerrit.ovirt.org/#/c/44813/14//COMMIT_MSG Commit Message: Line 5: CommitDate: 2015-10-13 16:15:34 +0200 Line 6: Line 7: virt: periodic: reduce NotConnectedError noise Line 8: Line 9: periodic operations have well known races on Periodic ... Line 10: VM startup and VM shutdown. Line 11: These races are well known, because periodic are Line 12: asynchronous with respect VM startup/shutdown, Line 13: and are benign because operations are just supposed Line 8: Line 9: periodic operations have well known races on Line 10: VM startup and VM shutdown. Line 11: These races are well known, because periodic are Line 12: asynchronous with respect VM startup/shutdown, ... respect to VM ... Line 13: and are benign because operations are just supposed Line 14: to be retried next cycle. Line 15: Line 16: Under high load and/or unresponsive libvirt, we may -- To view, visit https://gerrit.ovirt.org/44813 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I920e3b0b0e80e0a66ad199607068424986933d3a Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: periodic: factor out common code
Milan Zamazal has posted comments on this change. Change subject: virt: periodic: factor out common code .. Patch Set 20: Code-Review-1 I'd prefer if virdomain.NotConnectedError change remained in a separate patch. It's actually not part of the refactoring as such and it's already clearly explained in the commit message that patches that benefit from the refactoring will follow. -- To view, visit https://gerrit.ovirt.org/44548 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I283331ecfa5b47c147f0a42cd4a6b51a49308fe7 Gerrit-PatchSet: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: periodic: explicitely track domain availability
Milan Zamazal has posted comments on this change. Change subject: periodic: explicitely track domain availability .. Patch Set 5: (4 comments) I think this change is tricky enough to deserve at least some comment in the source code. If I understand it right then: - If everything works well (op < op_timeout), no problem. - If period > sampling.TIMEOUT then this change is no-op. What happens in such a case on the next operation calls? Just harmless exceptions, or unnecessarily repeated operation attempts when the period is short, or even problem escalation, or something else? Perhaps consistent with the declared "no false negatives" strategy, but it should be appropriately explained. - The only case handled by the change is if op > period & period < sampling.TIMEOUT. False negatives possible, but at most for sampling.TIMEOUT minus op. Again, maybe worth a little note? If I don't understand it right then explanation is especially needed! Moreover, adding comment why we still insist on trying different periodic operations in case another kind of operation blocks us would be useful (as it seems to be new behavior introduced by this change). https://gerrit.ovirt.org/#/c/47246/5//COMMIT_MSG Commit Message: Line 27: We need to try another way to solve this problem, without Line 28: depend on libvirt so much. Line 29: Line 30: Now VDSM counts explicitely the uses of a given domain, using Line 31: a ExpiringCache per operation, in order to minimize the an/one ExpiringCache ..., in ... Line 32: inter-operation interference. We want to avoid false negative Line 33: as top-priority, but once this is granted, we also want to minimize Line 34: the false positive! Line 35: Line 30: Now VDSM counts explicitely the uses of a given domain, using Line 31: a ExpiringCache per operation, in order to minimize the Line 32: inter-operation interference. We want to avoid false negative Line 33: as top-priority, but once this is granted, we also want to minimize Line 34: the false positive! I'd suggest defining false negative (VM available, considered unavailable?) and false positive (VM unavailable, considered available?) here to avoid confusion. Line 35: Line 36: The rational for this change is Line 37: - isDomainReadyForCommands, which we used previously, needs Line 38: o access libvirt, thus can actually make things worse on extreme Line 34: the false positive! Line 35: Line 36: The rational for this change is Line 37: - isDomainReadyForCommands, which we used previously, needs Line 38: o access libvirt, thus can actually make things worse on extreme to access ... Line 39: load conditions. Line 40: - under normal load, reduce the interference and false positives Line 41: between operations Line 42: https://gerrit.ovirt.org/#/c/47246/5/vdsm/virt/periodic.py File vdsm/virt/periodic.py: Line 229: continue Line 230: Line 231: op = self._create(vm_obj, self._skip_doms) Line 232: Line 233: if not op.required: Shouldn't vm_id be added to `skipped' here? Line 234: continue Line 235: # When dealing with blocked domains, we also want to avoid Line 236: # to pile up jobs that libvirt can't handle and eventually Line 237: # clog it. -- To view, visit https://gerrit.ovirt.org/47246 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idd90750ab36cb90bad401fab0ff7ff98429e78a5 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: periodic: reduce NotConnectedError noise
Milan Zamazal has posted comments on this change. Change subject: virt: periodic: reduce NotConnectedError noise .. Patch Set 16: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/44813 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I920e3b0b0e80e0a66ad199607068424986933d3a Gerrit-PatchSet: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: periodic: factor out periodic operations
Milan Zamazal has posted comments on this change. Change subject: virt: periodic: factor out periodic operations .. Patch Set 23: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/44548 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I283331ecfa5b47c147f0a42cd4a6b51a49308fe7 Gerrit-PatchSet: 23 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vmstats: take in account missing bulk stats fields
Milan Zamazal has posted comments on this change.
Change subject: vmstats: take in account missing bulk stats fields
..
Patch Set 3: Code-Review-1
(2 comments)
Fine, just typos in the comment.
https://gerrit.ovirt.org/#/c/47760/3/vdsm/virt/vmstats.py
File vdsm/virt/vmstats.py:
Line 361: for idx in six.moves.xrange(stats.get('%s.count' % group, 0)):
Line 362: try:
Line 363: name = stats['%s.%d.name' % (group, idx)]
Line 364: except KeyError:
Line 365: # bulk stats accumulate what they can get, raising errors
Bulk ...
Line 366: # only in the critical cases. This includes fundamntal
Line 367: # attributes like names, so count has to be considered
Line 368: # an upper bound more like a precise indicator.
Line 369: pass
Line 362: try:
Line 363: name = stats['%s.%d.name' % (group, idx)]
Line 364: except KeyError:
Line 365: # bulk stats accumulate what they can get, raising errors
Line 366: # only in the critical cases. This includes fundamntal
... fundamental
Line 367: # attributes like names, so count has to be considered
Line 368: # an upper bound more like a precise indicator.
Line 369: pass
Line 370: else:
--
To view, visit https://gerrit.ovirt.org/47760
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5a0eae867305dead8a96a23801ff2429605522ea
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Ido Barkan
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vmstats: take in account missing bulk stats fields
Milan Zamazal has posted comments on this change. Change subject: vmstats: take in account missing bulk stats fields .. Patch Set 4: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/47760 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5a0eae867305dead8a96a23801ff2429605522ea Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Ido Barkan Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: tests: add tests for sampling.VMBulkSampler
Milan Zamazal has posted comments on this change.
Change subject: tests: add tests for sampling.VMBulkSampler
..
Patch Set 36: Code-Review-1
(3 comments)
The tests take relatively long time to run. It would be nice to make them
faster.
https://gerrit.ovirt.org/#/c/40053/36/tests/bulkSamplingTests.py
File tests/bulkSamplingTests.py:
Line 206: self._perform_sampling(sampler, cache, times=3,
Line 207:timeout=timeout, flush_at=1)
Line 208:
Line 209: cache.done.wait(timeout * 2)
Line 210: self.assertEqual(conn.calls, ['FAST', 'SLOW', 'FAST'])
This fails but it's not a bug in the test, see _perform_sampling.
Line 211: self.assertVmsInBulkStats(vms.keys(), cache.data[-1][0])
Line 212:
Line 213: def _perform_sampling(self, sampler, cache, times, timeout=1,
Line 214: flush_at=None):
Line 216:
Line 217: for i in range(times):
Line 218: self.exc.dispatch(sampler, timeout)
Line 219: settle_timeout += timeout
Line 220: if flush_at is not None and i == flush_at:
This doesn't work as intended: The flush is immediate here, while the
dispatched tasks are executed asynchronously.
Line 221: sampler.clear()
Line 222:
Line 223: self.assertTrue(cache.done.wait(settle_timeout))
Line 224:
https://gerrit.ovirt.org/#/c/40053/36/vdsm/virt/sampling.py
File vdsm/virt/sampling.py:
Line 489: self._sampling = threading.Semaphore() # used as glorified
counter
Line 490: self._log = logging.getLogger("virt.sampling.VMBulkSampler")
Line 491:
Line 492: # for test purposes only
Line 493: def clear(self):
Is it really needed, as a public method? I'd prefer not polluting the public
interface and calling a private method from the tests.
Line 494: self._skip_doms.clear()
Line 495:
Line 496: def __call__(self):
Line 497: timestamp = self._stats_cache.clock()
--
To view, visit https://gerrit.ovirt.org/40053
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id66dbd420ca29d08ae4063dc83b858be34b8940f
Gerrit-PatchSet: 36
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: periodic: explicitely track domain availability
Milan Zamazal has posted comments on this change.
Change subject: periodic: explicitely track domain availability
..
Patch Set 9:
(3 comments)
https://gerrit.ovirt.org/#/c/47246/9/vdsm/virt/periodic.py
File vdsm/virt/periodic.py:
Line 211: """
Line 212:
Line 213: _log = logging.getLogger("virt.periodic.VmDispatcher")
Line 214:
Line 215: def __init__(self, get_vms, executor, create, timeout, ttl):
Please document ttl argument.
Line 216: """
Line 217: get_vms: callable which will return a dict which maps
Line 218: vm_ids to vm_instances
Line 219: executor: executor.Executor instance
Line 217: get_vms: callable which will return a dict which maps
Line 218: vm_ids to vm_instances
Line 219: executor: executor.Executor instance
Line 220: create: callable to obtain the real callable to
Line 221: dispatch, with its timeout
I wouldn't mind if `timeout' was properly documented as part of this change as
well. (It's not clear from the docstring itself what `timeout' refers to here.)
Line 222: """
Line 223: self._get_vms = get_vms
Line 224: self._executor = executor
Line 225: self._create = create
Line 232:
Line 233: for vm_id, vm_obj in vms.iteritems():
Line 234: try:
Line 235: to_skip = self._skip_doms.get(vm_id, False)
Line 236: if to_skip:
Sorry about my previously misplaced note regarding `skipped'. What I actually
meant was: Originally, if we skipped on `not op.runnable' and op was required,
vm_id was added to `skipped'. It's not so now under similar circumstances. Is
it intentional?
Line 237: continue
Line 238:
Line 239: op = self._create(vm_obj, self._skip_doms)
Line 240:
--
To view, visit https://gerrit.ovirt.org/47246
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd90750ab36cb90bad401fab0ff7ff98429e78a5
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: periodic: explicitely track domain availability
Milan Zamazal has posted comments on this change. Change subject: periodic: explicitely track domain availability .. Patch Set 12: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/47246 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idd90750ab36cb90bad401fab0ff7ff98429e78a5 Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: lib: utils: consolidate Error class in one place
Milan Zamazal has posted comments on this change. Change subject: lib: utils: consolidate Error class in one place .. Patch Set 1: (2 comments) https://gerrit.ovirt.org/#/c/47964/1//COMMIT_MSG Commit Message: Line 10: they raise when one command run through utils.execCmd fails. Line 11: Line 12: Since this Error is closely related to utils.execCmd, we Line 13: remove the duplicate definition of Error and we move it Line 14: in one place, alogside execCmd itself. ... alongside ... Line 15: Line 16: Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5 https://gerrit.ovirt.org/#/c/47964/1/lib/vdsm/udevadm.py File lib/vdsm/udevadm.py: Line 23 Line 24 Line 25 Line 26 Line 27 > Could be good solution for a future patch, when we have a clear need. For t Referring the same class via different paths raises some red flags in my head. I also can't say it's an ultimately bad thing to do, but I can e.g. imagine someone, once having the clear need without realizing the aliases, debugging his code for a while and then hitting his head against a wall. Not a big issue for me, just thinking aloud about what "simplest" actually means here. -- To view, visit https://gerrit.ovirt.org/47964 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: enhance/fix migration.SourceThread.stop()
Milan Zamazal has posted comments on this change. Change subject: virt: enhance/fix migration.SourceThread.stop() .. Patch Set 11: Verified+1 I checked the migration cancellation by starting and canceling migration from engine. As migration is currently generally broken in VDSM, it's a bit difficult to verify, but let's say cancellation works the same way for me before and after this change. -- To view, visit https://gerrit.ovirt.org/40520 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0ab50fc789dde969b2fb9ab969241ed4ad12545c Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: lib: utils: consolidate Error class in one place
Milan Zamazal has posted comments on this change. Change subject: lib: utils: consolidate Error class in one place .. Patch Set 3: Fine for me now except the "alogside" typo in the commit message. -- To view, visit https://gerrit.ovirt.org/47964 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: lib: sparsify: use common Error class
Milan Zamazal has posted comments on this change. Change subject: lib: sparsify: use common Error class .. Patch Set 3: (1 comment) OK except for the commit message. https://gerrit.ovirt.org/#/c/47965/3//COMMIT_MSG Commit Message: Line 5: CommitDate: 2015-11-05 08:55:41 +0100 Line 6: Line 7: lib: sparsify: use common Error class Line 8: Line 9: the virtsparsify module used a slightly variant The ... ... slightly different variant Line 10: of the now-common Error class. Line 11: This patch makes it conform to the common API, Line 12: and use the common implementation, reducing duplication. Line 13: -- To view, visit https://gerrit.ovirt.org/47965 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1cd56e513d99c82f237fb53876b6ced8fbebfde4 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: lib: utils: consolidate Error class in one place
Milan Zamazal has posted comments on this change. Change subject: lib: utils: consolidate Error class in one place .. Patch Set 4: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/47964 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia490c055e6e5c637eba82ccffb9d2dfc748db8f5 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: sampling: HostStatsThread as periodic operation
Milan Zamazal has posted comments on this change.
Change subject: sampling: HostStatsThread as periodic operation
..
Patch Set 32: Code-Review-1
(6 comments)
https://gerrit.ovirt.org/#/c/40431/32/vdsm/virt/periodic.py
File vdsm/virt/periodic.py:
Line 102: DriveWatermarkMonitor,
Line 103: config.getint('vars', 'vm_watermark_interval')),
Line 104:
Line 105: Operation(
Line 106: sampling.HostMonitor(cif.log, sampling.host_samples),
This doesn't match HostMonitor.__init__ arguments.
Line 107: config.getint('vars', 'host_sample_stats_interval'))
Line 108:
Line 109: ]
Line 110:
Line 107: config.getint('vars', 'host_sample_stats_interval'))
Line 108:
Line 109: ]
Line 110:
Line 111: hoststats.start(clock)
`clock' undefined here.
Line 112:
Line 113: for op in _operations:
Line 114: op.start()
Line 115:
https://gerrit.ovirt.org/#/c/40431/32/vdsm/virt/sampling.py
File vdsm/virt/sampling.py:
Line 35
Line 36
Line 37
Line 38
Line 39
Unused.
Line 540
Line 541
Line 542
Line 543
Line 544
Still referred in samplingTests.py.
Line 541:
Line 542: class HostMonitor(object):
Line 543: _CONNLOG = logging.getLogger('connectivity')
Line 544:
Line 545: def __init__(self, samples=host_samples,
clock=utils.monotonic_time):
What is `clock' here for?
Line 546: self._samples = samples
Line 547:
Line 548: self._pid = os.getpid()
Line 549:
Line 543: _CONNLOG = logging.getLogger('connectivity')
Line 544:
Line 545: def __init__(self, samples=host_samples,
clock=utils.monotonic_time):
Line 546: self._samples = samples
Line 547:
Extra empty line here.
Line 548: self._pid = os.getpid()
Line 549:
Line 550: def __call__(self):
Line 551: sample = HostSample(self._pid)
--
To view, visit https://gerrit.ovirt.org/40431
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I39c2e6e4bca286a513992b7231f1356e8dd871a1
Gerrit-PatchSet: 32
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Roman Mohr
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: gitignore: Missing autogenerated files added
Milan Zamazal has posted comments on this change. Change subject: gitignore: Missing autogenerated files added .. Patch Set 1: Verified+1 -- To view, visit https://gerrit.ovirt.org/48328 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If4a1e9c2607d817ccca2a321060ab4f452422c32 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: periodic: more cautious return to fast path
Milan Zamazal has posted comments on this change. Change subject: periodic: more cautious return to fast path .. Patch Set 2: Code-Review-1 (6 comments) Looks like a good idea to me. -1 just due to the typos. https://gerrit.ovirt.org/#/c/48190/2//COMMIT_MSG Commit Message: Line 24: However, we go back in the fast path as soon as we can, when we Line 25: don't see any more timeouts (good) and when the blacklist is expired. Line 26: Line 27: And eventually here comes the trouble. When the blacklist expires, Line 28: we assume eveything is fine again, but the actual check is done by ... everything ... Line 29: the next fast path cycle. Line 30: This works, but can leak a thread. Line 31: Line 32: There are circumnstances on which we must take the risk of leaking Line 26: Line 27: And eventually here comes the trouble. When the blacklist expires, Line 28: we assume eveything is fine again, but the actual check is done by Line 29: the next fast path cycle. Line 30: This works, but can leak a thread. Maybe it's worth to define what kind of leak is experienced here. Is it a blocked libvirt call, until it finishes (i.e. unblocks or fails with an exception)? Line 31: Line 32: There are circumnstances on which we must take the risk of leaking Line 33: one thread, but in this case we can do another cautionary slow path Line 34: cycle sampling all domains to see if they seems actually right, and Line 28: we assume eveything is fine again, but the actual check is done by Line 29: the next fast path cycle. Line 30: This works, but can leak a thread. Line 31: Line 32: There are circumnstances on which we must take the risk of leaking ... circumstances ... Line 33: one thread, but in this case we can do another cautionary slow path Line 34: cycle sampling all domains to see if they seems actually right, and Line 35: only after that jump back in the fast path. Line 36: Line 33: one thread, but in this case we can do another cautionary slow path Line 34: cycle sampling all domains to see if they seems actually right, and Line 35: only after that jump back in the fast path. Line 36: Line 37: Please note that this is approach has still some amount of risk: ... this approach ... Line 38: maybe a domain gets stuck between the last succesfull slow cycle Line 39: and the new fast cycle. Line 40: Line 41: But we don't have any protection for this cases. Adding a cautionary Line 37: Please note that this is approach has still some amount of risk: Line 38: maybe a domain gets stuck between the last succesfull slow cycle Line 39: and the new fast cycle. Line 40: Line 41: But we don't have any protection for this cases. Adding a cautionary ... for these cases. ... Line 42: extra slow path cycles gives us all the protection we can possibly Line 43: get. Line 44: Line 45: The drawback of this patch is that the slow path is more consuming, https://gerrit.ovirt.org/#/c/48190/2/vdsm/virt/sampling.py File vdsm/virt/sampling.py: Line 493: Line 494: def __call__(self): Line 495: timestamp = self._stats_cache.clock() Line 496: # we need to update our state variable here, not after the call, Line 497: # because if the call get stuck, the update may be stale and harmful ... gets ... Line 498: on_fast_path = self._on_fast_path Line 499: # we are deep in the hot path. bool(ExpiringCache) Line 500: # *is* costly so we should avoid it if we can. Line 501: fast_path = ( -- To view, visit https://gerrit.ovirt.org/48190 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40dda0a308d6be6b2c8b8217a26ca5e7e9717546 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Make BIOS messages available on vmconsole
Milan Zamazal has posted comments on this change.
Change subject: virt: Make BIOS messages available on vmconsole
..
Patch Set 1:
(4 comments)
https://gerrit.ovirt.org/#/c/48404/1/tests/vmTests.py
File tests/vmTests.py:
Line 1: #
Line 2: # Copyright IBM Corp. 2012
Line 3: # Copyright 2013-2015 Red Hat, Inc.
> correct, but unneeded for this patch.
Are the copyright years in VDSM updated in a different way than on the first
change in given file each year?
Line 4: #
Line 5: # This program is free software; you can redistribute it and/or modify
Line 6: # it under the terms of the GNU General Public License as published by
Line 7: # the Free Software Foundation; either version 2 of the License, or
Line 1418:'specParams': {'consoleType': console_type}},
Line 1419: with fake.VM(devices=devices,
create_device_objects=True) as v:
Line 1420: dom_xml = v._buildDomainXML()
Line 1421: self.assertEqual('' in
dom_xml,
Line 1422: use_serial)
> Kudos for the test. However, I'd prefer something more like
The problem is that presence of useserial is dependent on presence of the
serial console. So we have to invoke more of the XML domain building process to
test it, not just the part.
Line 1423:
Line 1424:
Line 1425: def _load_xml(name):
Line 1426: test_path = os.path.realpath(__file__)
https://gerrit.ovirt.org/#/c/48404/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 1593: if consoleDev.isSerial:
Line 1594: break
Line 1595: else:
Line 1596: consoleDev = None
Line 1597: self.conf['_serialConsoleAvailable'] = consoleDev is not None
> we should avoid to add attributes to Vm.conf unless it is really needed. _h
I agree, so let's try passing an argument to appendOs instead.
Line 1598:
Line 1599: domxml = vmxml.Domain(self.conf, self.log, self.arch)
Line 1600: domxml.appendOs()
Line 1601:
https://gerrit.ovirt.org/#/c/48404/1/vdsm/virt/vmxml.py
File vdsm/virt/vmxml.py:
Line 276: if utils.tobool(self.conf.get('bootMenuEnable', False)):
Line 277: oselem.appendChildWithArgs('bootmenu', enable='yes')
Line 278:
Line 279: if self.conf.get('_serialConsoleAvailable'):
Line 280: oselem.appendChildWithArgs('bios', useserial='yes')
> what happens if we inconditionally enable useserial?
If serial console is not available then libvirt complains and the VM doesn't
start.
Line 281:
Line 282: def appendSysinfo(self, osname, osversion, serialNumber):
Line 283: """
Line 284: Add element to domain:
--
To view, visit https://gerrit.ovirt.org/48404
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Make BIOS messages available on vmconsole
Milan Zamazal has posted comments on this change. Change subject: virt: Make BIOS messages available on vmconsole .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/48404/1//COMMIT_MSG Commit Message: Line 14: This change is the first step towards the goal. It enables BIOS serial Line 15: console interaction in a VM's XML domain description when a serial Line 16: console is available. Line 17: Line 18: Note that at least oVirt Engine 3.6 snapshot is required to make the > this is true for oVirt/RHEV. For VDSM, is sufficient that the console is 't Done Line 19: functionality actually working. The reason is that access to BIOS Line 20: messages requires presence of serial (not virtio) console. Line 21: Line 22: Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973 -- To view, visit https://gerrit.ovirt.org/48404 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Make BIOS messages available on vmconsole
Milan Zamazal has posted comments on this change.
Change subject: virt: Make BIOS messages available on vmconsole
..
Patch Set 1:
(4 comments)
https://gerrit.ovirt.org/#/c/48404/1/tests/vmTests.py
File tests/vmTests.py:
Line 1: #
Line 2: # Copyright IBM Corp. 2012
Line 3: # Copyright 2013-2015 Red Hat, Inc.
> there is not such habit. A separate copyright-bump patch will probably a be
Done
Line 4: #
Line 5: # This program is free software; you can redistribute it and/or modify
Line 6: # it under the terms of the GNU General Public License as published by
Line 7: # the Free Software Foundation; either version 2 of the License, or
Line 1418:'specParams': {'consoleType': console_type}},
Line 1419: with fake.VM(devices=devices,
create_device_objects=True) as v:
Line 1420: dom_xml = v._buildDomainXML()
Line 1421: self.assertEqual('' in
dom_xml,
Line 1422: use_serial)
> Could'nt that be solved by some monkeypatching?
I don't get how monkeypatching could help here. Basically, we need to check
that _buildDomainXML correctly identifies presence of a serial console, passes
correct argument to appendOs and appendOs produces correct output.
Line 1423:
Line 1424:
Line 1425: def _load_xml(name):
Line 1426: test_path = os.path.realpath(__file__)
https://gerrit.ovirt.org/#/c/48404/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 1: #
Line 2: # Copyright 2008-2015 Red Hat, Inc.
> correct, but unneeded here
Done
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/48404/1/vdsm/virt/vmxml.py
File vdsm/virt/vmxml.py:
Line 1: #
Line 2: # Copyright 2008-2015 Red Hat, Inc.
> correct, but unneeded for this patch.
Done
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/48404
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: periodic: add executor-compatible naming
Milan Zamazal has posted comments on this change. Change subject: periodic: add executor-compatible naming .. Patch Set 6: (2 comments) https://gerrit.ovirt.org/#/c/48193/6//COMMIT_MSG Commit Message: Line 8: Line 9: We want to make as easy as possible to debug Line 10: thread leaks from executor, so we make the periodic Line 11: framework support names for Operations, using the Line 12: way Executor expects them. What's the executor-compatible naming? Is NAME utilized anywhere outside periodic.py, perhaps in another patch (I can't see it in the current source code)? Line 13: Line 14: Change-Id: If8d1180b727571cce34e8304ac48390ed2135f79 Line 15: Bug-Url: https://bugzilla.redhat.com/1250839 Line 16: Backport-To: 3.6 https://gerrit.ovirt.org/#/c/48193/6/vdsm/virt/periodic.py File vdsm/virt/periodic.py: Line 262: return '' % ( Line 263: self._create, id(self) Line 264: ) Line 265: Line 266: __repr__ = __str__ Why renaming __repr__ to __str__ and then defining this alias here? Wasn't having just __repr__ method enough? Line 267: Line 268: Line 269: class _RunnableOnVm(object): Line 270: -- To view, visit https://gerrit.ovirt.org/48193 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If8d1180b727571cce34e8304ac48390ed2135f79 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: periodic: add executor-compatible naming
Milan Zamazal has posted comments on this change. Change subject: periodic: add executor-compatible naming .. Patch Set 6: (2 comments) https://gerrit.ovirt.org/#/c/48193/6/vdsm/virt/periodic.py File vdsm/virt/periodic.py: Line 262: return '' % ( Line 263: self._create, id(self) Line 264: ) Line 265: Line 266: __repr__ = __str__ > Back in time I added __repr__ to have something nicer output in the logs, b Just thought about retaining __repr__ here (as `str' falls back to it), but if you think using __str__ here (and in other classes here) is more appropriate then it's fine for me. Line 267: Line 268: Line 269: class _RunnableOnVm(object): Line 270: Line 292: Line 293: def __str__(self): Line 294: return '<%s vm=%s at 0x%x>' % ( Line 295: self.NAME, self._vm.id, id(self) Line 296: ) Why not simply using self.__class__.__name__ here and introducing NAME? Line 297: Line 298: Line 299: class UpdateVolumes(_RunnableOnVm): Line 300: -- To view, visit https://gerrit.ovirt.org/48193 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If8d1180b727571cce34e8304ac48390ed2135f79 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: gitignore: Missing autogenerated files added
Milan Zamazal has uploaded a new change for review. Change subject: gitignore: Missing autogenerated files added .. gitignore: Missing autogenerated files added Change-Id: If4a1e9c2607d817ccca2a321060ab4f452422c32 Signed-off-by: Milan Zamazal --- M .gitignore 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/28/48328/1 diff --git a/.gitignore b/.gitignore index 3c6c9e8..a42efb1 100644 --- a/.gitignore +++ b/.gitignore @@ -13,6 +13,7 @@ Makefile.in aclocal.m4 autom4te.cache +build-aux/compile build-aux/depcomp build-aux/install-sh build-aux/missing @@ -29,6 +30,7 @@ init/systemd/vdsm-network.service init/systemd/mom-vdsm.service init/vdsmd_init_common.sh +lib/api/vdsm-api.html lib/vdsm/config.py lib/vdsm/constants.py lib/vdsm/tool/validate_ovirt_certs.py -- To view, visit https://gerrit.ovirt.org/48328 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: If4a1e9c2607d817ccca2a321060ab4f452422c32 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: automat...@ovirt.org ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Make BIOS messages available on vmconsole
Milan Zamazal has posted comments on this change.
Change subject: virt: Make BIOS messages available on vmconsole
..
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/48404/3/vdsm/virt/vmxml.py
File vdsm/virt/vmxml.py:
Line 230: self.dom.setAttr('xmlns:' + METADATA_VM_TUNE_PREFIX,
Line 231: METADATA_VM_TUNE_URI)
Line 232: self.dom.appendChild(self._metadata)
Line 233:
Line 234: def appendOs(self, serial_console_available=False):
> I'd go for the shorter and equally expressive
Done
Line 235: """
Line 236: Add element to domain:
Line 237:
Line 238:
--
To view, visit https://gerrit.ovirt.org/48404
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logging: Don't crash on non-ASCII in SimpleLogAdapter
Milan Zamazal has uploaded a new change for review.
Change subject: logging: Don't crash on non-ASCII in SimpleLogAdapter
..
logging: Don't crash on non-ASCII in SimpleLogAdapter
The values and messages passed to SimpleLogAdapter can be mixtures of
strings and unicodes containing various non-ASCII characters. If they
are not properly handled, concatenation of incompatible strings raises
UnicodeDecodeError.
We avoid that problem in SimpleLogAdapter by converting all the
processed texts to unicodes before joining them. We assume non-ASCII
strings are in UTF-8 but if they are not then we shouldn't crash at
least.
Change-Id: I1930817ade1799b218913348ffa099aab2211ef7
Bug-Url: https://bugzilla.redhat.com/1276906
Bug-Url: https://bugzilla.redhat.com/1260131
Signed-off-by: Milan Zamazal
---
M tests/utilsTests.py
M vdsm/logUtils.py
2 files changed, 28 insertions(+), 3 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/42/48542/1
diff --git a/tests/utilsTests.py b/tests/utilsTests.py
index 68547ce..f4f1a58 100644
--- a/tests/utilsTests.py
+++ b/tests/utilsTests.py
@@ -1,3 +1,4 @@
+# -*- coding: utf-8 -*-
#
# Copyright 2012-2013 Red Hat, Inc.
#
@@ -41,6 +42,7 @@
from vdsm import taskset
from vdsm import utils
from vdsm import cmdutils
+import logUtils
from monkeypatch import MonkeyPatch
from vmTestsData import VM_STATUS_DUMP
@@ -1070,3 +1072,22 @@
proc.start()
self._noIntrWatchFd(myPipe, isEpoll=False, mask=select.POLLIN)
proc.join()
+
+
+class LoggingTests(TestCaseBase):
+
+def setUp(self):
+self._log = FakeLogger(logging.DEBUG)
+
+def test_nonascii(self):
+# Just test it doesn't crash with mixture of string and unicodes in
+# various codings.
+invalid = chr(254) + chr(0)
+u_invalid = unichr(253) + unichr(254)
+extra = dict(e1=u'Hello', e2='Здра́вствуйте', e3=u'שלום', e4='你好',
+ e5=invalid, e6=u_invalid)
+log = logUtils.SimpleLogAdapter(self._log, extra)
+log.debug("Dobrý večer")
+log.debug(u"Dobrý večer")
+log.debug(invalid)
+log.debug(u_invalid)
diff --git a/vdsm/logUtils.py b/vdsm/logUtils.py
index 5caa167..49bb2cf 100644
--- a/vdsm/logUtils.py
+++ b/vdsm/logUtils.py
@@ -90,10 +90,14 @@
warn = logging.LoggerAdapter.warning
def process(self, msg, kwargs):
-result = ''
+result = u''
for key, value in self.extra.iteritems():
-result += '%s=`%s`' % (key, value)
-result += '::%s' % msg
+if isinstance(value, str):
+value = value.decode('utf-8', 'replace')
+result += u'%s=`%s`' % (key, value)
+if isinstance(msg, str):
+msg = msg.decode('utf-8', 'replace')
+result += u'::%s' % msg
return (result, kwargs)
--
To view, visit https://gerrit.ovirt.org/48542
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1930817ade1799b218913348ffa099aab2211ef7
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logging: Don't crash on non-ASCII in SimpleLogAdapter
Milan Zamazal has posted comments on this change.
Change subject: logging: Don't crash on non-ASCII in SimpleLogAdapter
..
Patch Set 1:
(3 comments)
https://gerrit.ovirt.org/#/c/48542/1/tests/utilsTests.py
File tests/utilsTests.py:
Line 1084: # various codings.
Line 1085: invalid = chr(254) + chr(0)
Line 1086: u_invalid = unichr(253) + unichr(254)
Line 1087: extra = dict(e1=u'Hello', e2='Здра́вствуйте', e3=u'שלום',
e4='你好',
Line 1088: e5=invalid, e6=u_invalid)
> This test is valid, if we don't control the extra parameters - but is this
Who knows where unicode parameters come from... In this particular case it's
self.conf['vmId'] in Vm. But you can never be sure that no unicode appears
anytime around.
Line 1089: log = logUtils.SimpleLogAdapter(self._log, extra)
Line 1090: log.debug("Dobrý večer")
Line 1091: log.debug(u"Dobrý večer")
Line 1092: log.debug(invalid)
Line 1087: extra = dict(e1=u'Hello', e2='Здра́вствуйте', e3=u'שלום',
e4='你好',
Line 1088: e5=invalid, e6=u_invalid)
Line 1089: log = logUtils.SimpleLogAdapter(self._log, extra)
Line 1090: log.debug("Dobrý večer")
Line 1091: log.debug(u"Dobrý večer")
> This should be forbidden by the logging framework. There is no reason to us
Complete control? In theory, yes. But how do you ensure it? If any single
non-ASCII unicode passes and nothing handles it then we are in trouble.
As for conversions, I wasn't sure about the right direction, thanks for
explanation. I can change it, i.e. converting unicodes to strings, in the
common case it's just a couple of extra isinstance calls, which is negligible.
And we'll be much safer.
Line 1092: log.debug(invalid)
https://gerrit.ovirt.org/#/c/48542/1/vdsm/logUtils.py
File vdsm/logUtils.py:
Line 96: value = value.decode('utf-8', 'replace')
Line 97: result += u'%s=`%s`' % (key, value)
Line 98: if isinstance(msg, str):
Line 99: msg = msg.decode('utf-8', 'replace')
Line 100: result += u'::%s' % msg
> Why return unicode string? There is no reason to do this, and encode it bac
OK, I'll change the conversion direction.
Line 101: return (result, kwargs)
Line 102:
Line 103:
Line 104: class TracebackRepeatFilter(logging.Filter):
--
To view, visit https://gerrit.ovirt.org/48542
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1930817ade1799b218913348ffa099aab2211ef7
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logging: Don't crash on non-ASCII in SimpleLogAdapter
Milan Zamazal has posted comments on this change.
Change subject: logging: Don't crash on non-ASCII in SimpleLogAdapter
..
Patch Set 1:
(1 comment)
We now check for unicodes and convert them to strings.
https://gerrit.ovirt.org/#/c/48542/1/vdsm/logUtils.py
File vdsm/logUtils.py:
Line 96: value = value.decode('utf-8', 'replace')
Line 97: result += u'%s=`%s`' % (key, value)
Line 98: if isinstance(msg, str):
Line 99: msg = msg.decode('utf-8', 'replace')
Line 100: result += u'::%s' % msg
> OK, I'll change the conversion direction.
Done
Line 101: return (result, kwargs)
Line 102:
Line 103:
Line 104: class TracebackRepeatFilter(logging.Filter):
--
To view, visit https://gerrit.ovirt.org/48542
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1930817ade1799b218913348ffa099aab2211ef7
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logging: Don't crash on non-ASCII in SimpleLogAdapter
Milan Zamazal has posted comments on this change.
Change subject: logging: Don't crash on non-ASCII in SimpleLogAdapter
..
Patch Set 1:
(2 comments)
https://gerrit.ovirt.org/#/c/48542/1/tests/utilsTests.py
File tests/utilsTests.py:
Line 1084: # various codings.
Line 1085: invalid = chr(254) + chr(0)
Line 1086: u_invalid = unichr(253) + unichr(254)
Line 1087: extra = dict(e1=u'Hello', e2='Здра́вствуйте', e3=u'שלום',
e4='你好',
Line 1088: e5=invalid, e6=u_invalid)
> VmId is a uuid, cannot contain unicode.
Believe me or not, vm_id as passed to SimpleLogAdapter constructor in
Vm.__init__ is a unicode instance (I didn't investigated why). It's not a
problem per se, but it makes a unicode from the formatted string in
SimpleLogAdapter.perform (thus the returned result is going to be unicode as
well!) and when a non-ASCII message is logged, it crashes.
No objections against validating input, it's definitely a right thing to do,
but a few additional checks don't harm, especially at a place where we've
already been hit.
Line 1089: log = logUtils.SimpleLogAdapter(self._log, extra)
Line 1090: log.debug("Dobrý večer")
Line 1091: log.debug(u"Dobrý večer")
Line 1092: log.debug(invalid)
Line 1087: extra = dict(e1=u'Hello', e2='Здра́вствуйте', e3=u'שלום',
e4='你好',
Line 1088: e5=invalid, e6=u_invalid)
Line 1089: log = logUtils.SimpleLogAdapter(self._log, extra)
Line 1090: log.debug("Dobrý večer")
Line 1091: log.debug(u"Dobrý večer")
> We control whats in the source, for input, we should encode/decode values a
I agree, and the check in SimpleLogAdapter conforms to that rule now.
Line 1092: log.debug(invalid)
--
To view, visit https://gerrit.ovirt.org/48542
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1930817ade1799b218913348ffa099aab2211ef7
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: periodic: add __str__ methods
Milan Zamazal has posted comments on this change. Change subject: periodic: add __str__ methods .. Patch Set 8: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/48193 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If8d1180b727571cce34e8304ac48390ed2135f79 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: more robust support of event strings
Milan Zamazal has posted comments on this change. Change subject: vm: more robust support of event strings .. Patch Set 5: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/48517 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iae41085c49a76ad568ebfbfafe711fd7619f0421 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vm: add support for "Crashed" event
Milan Zamazal has posted comments on this change. Change subject: vm: add support for "Crashed" event .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/48594 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I96e98cf7dc9c2c9e7507cc39e181fb2953032989 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: vm: Use formatting string to log domxml
Milan Zamazal has uploaded a new change for review.
Change subject: virt: vm: Use formatting string to log domxml
..
virt: vm: Use formatting string to log domxml
Variable data shouldn't be passed to logging as the first (message)
argument, to prevent its interpretation as a formatting string leading
to a crash.
Change-Id: I1e71653ecd7267ecf28d121b68282e3b3c4e519a
Signed-off-by: Milan Zamazal
---
M vdsm/virt/vm.py
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/24/48624/1
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 86d968f..2f02f45 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -1875,7 +1875,7 @@
domxml = hooks.before_vm_start(self._buildDomainXML(), self.conf)
# TODO: this is debug information. For 3.6.x we still need to
# see the XML even with 'info' as default level.
-self.log.info(domxml)
+self.log.info('%s', domxml)
flags = libvirt.VIR_DOMAIN_NONE
with self._confLock:
--
To view, visit https://gerrit.ovirt.org/48624
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1e71653ecd7267ecf28d121b68282e3b3c4e519a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logging: Don't crash on non-ASCII in SimpleLogAdapter
Milan Zamazal has posted comments on this change.
Change subject: logging: Don't crash on non-ASCII in SimpleLogAdapter
..
Patch Set 2:
(2 comments)
https://gerrit.ovirt.org/#/c/48542/2/tests/utilsTests.py
File tests/utilsTests.py:
Line 1086: u_invalid = unichr(253) + unichr(254)
Line 1087: extra = dict(e1=u'Hello', e2='Здра́вствуйте', e3=u'שלום',
e4='你好',
Line 1088: e5=invalid, e6=u_invalid)
Line 1089: log = logUtils.SimpleLogAdapter(self._log, extra)
Line 1090: log.debug("Dobrý večer")
> Again, this is invalid test. Format strings are part of the code and using
SimpleLogAdapter doesn't interpret the argument as a format string. Regardless
of that, we are testing robustness here.
Line 1091: log.debug(u"Dobrý večer")
Line 1092: log.debug(invalid)
https://gerrit.ovirt.org/#/c/48542/2/vdsm/logUtils.py
File vdsm/logUtils.py:
Line 92: def process(self, msg, kwargs):
Line 93: result = ''
Line 94: for key, value in self.extra.iteritems():
Line 95: if isinstance(value, unicode):
Line 96: value = value.encode('utf-8', 'replace')
> No need to replace, all unicode code points can be encoded as utt8 (or any
'replace' is here for robustness, as this is what this patch is about.
Line 97: result += '%s=`%s`' % (key, value)
Line 98: if isinstance(msg, unicode):
Line 99: msg = msg.encode('utf-8', 'replace')
Line 100: result += '::%s' % msg
--
To view, visit https://gerrit.ovirt.org/48542
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1930817ade1799b218913348ffa099aab2211ef7
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: vm: Use formatting string to log domxml
Milan Zamazal has abandoned this change. Change subject: virt: vm: Use formatting string to log domxml .. Abandoned SimpleLogAdapter doesn't interpret the first argument as a format string. -- To view, visit https://gerrit.ovirt.org/48624 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I1e71653ecd7267ecf28d121b68282e3b3c4e519a Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: gerrit-hooks ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[ovirt-3.6]: vm: periodic: NumaInfoMonitor doesn't need jobs
Milan Zamazal has posted comments on this change. Change subject: vm: periodic: NumaInfoMonitor doesn't need jobs .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/48411 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I50c68b8b00ea0f9c137ce8149f3a806c62be7f6d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.6 Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logging: Don't crash on non-ASCII in SimpleLogAdapter
Milan Zamazal has posted comments on this change. Change subject: logging: Don't crash on non-ASCII in SimpleLogAdapter .. Patch Set 3: Let's summarize what we can do about this change: # "Only strings, no unicodes in VDSM internals" rule was discussed. If there is a rule, it must be visibly documented. Any objections against adding it to VDSM Coding Guidelines on Wiki? # According to the rule above, this change is indeed not a solution of the original bug, just a workaround. But it's still useful IMHO, see the updated commit message. So we can work on it independently of the original bug. # Another solution should be found for the original bug. There are already some proposals by Nir, unrelated to this change, no need to further discuss that here. -- To view, visit https://gerrit.ovirt.org/48542 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1930817ade1799b218913348ffa099aab2211ef7 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: vmstats: reformat to make the code nicer
Milan Zamazal has posted comments on this change. Change subject: vmstats: reformat to make the code nicer .. Patch Set 2: Code-Review+1 It's arguable whether inserting two more lines makes the code nicer or the other way round, but being consistent with surrounding code is indeed nice. -- To view, visit https://gerrit.ovirt.org/48492 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I87de5b265b73b662c890b84fd53b2b0f29b4228a Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Make BIOS messages available on vmconsole
Milan Zamazal has posted comments on this change. Change subject: virt: Make BIOS messages available on vmconsole .. Patch Set 4: Verified+1 I verified the patch works by: - Building Engine version ovirt-engine-3.6.0.3 + fbfaf94 (vmconsole didn't work for me in newer versions). - Disabling VirtIO serial console in the engine for a given VM an enabling it again (to convert it to serial type in the engine). - Starting the VM in pause mode. - Connecting to the console: ssh -t -p ovirt-vmconsole@ENGINE-HOST - Unpausing the VM and interacting with the console. -- To view, visit https://gerrit.ovirt.org/48404 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: startup: Change system default encoding to utf8
Milan Zamazal has posted comments on this change. Change subject: startup: Change system default encoding to utf8 .. Patch Set 4: Code-Review+1 (3 comments) I made some checks and it seems this patch indeed solves the string/unicode troubles we currently have. https://gerrit.ovirt.org/#/c/48661/4//COMMIT_MSG Commit Message: Line 5: CommitDate: 2015-11-17 15:57:54 +0200 Line 6: Line 7: startup: Change system default encoding to utf8 Line 8: Line 9: In Python 2, the system default encoding is 'ascii'. This cause mixing ... causes ... Line 10: of unicode and non-ascii strings (e.g. utf8 encoded) to fail with Line 11: UnicodeDecodeError or UnicodeEncodeError. The trigger for this failures Line 12: is starting using the built-in json library, that return all values as Line 13: unicode strings, even if the value is ascii. Line 8: Line 9: In Python 2, the system default encoding is 'ascii'. This cause mixing Line 10: of unicode and non-ascii strings (e.g. utf8 encoded) to fail with Line 11: UnicodeDecodeError or UnicodeEncodeError. The trigger for this failures Line 12: is starting using the built-in json library, that return all values as ... returns ... Line 13: unicode strings, even if the value is ascii. Line 14: Line 15: For example: Line 16: Line 19: File "", line 1, in Line 20: UnicodeDecodeError: 'ascii' codec can't decode byte 0xd7 in position 0: ordinal Line 21: not in range(128) Line 22: Line 23: Python tries to decode the second value implicitly, and fail since this ... fails ... Line 24: is a utf8 encoded string. Line 25: Line 26: To avoid such issues, the entire application must be changed to use only Line 27: strings or only unicode internally, and never mix these types. In the -- To view, visit https://gerrit.ovirt.org/48661 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icc3f072a499ba4034bdbedd09eb60d7a3a32f9c8 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logging: Don't crash on non-ASCII in SimpleLogAdapter
Milan Zamazal has posted comments on this change.
Change subject: logging: Don't crash on non-ASCII in SimpleLogAdapter
..
Patch Set 2:
(1 comment)
https://gerrit.ovirt.org/#/c/48542/2/vdsm/logUtils.py
File vdsm/logUtils.py:
Line 92: def process(self, msg, kwargs):
Line 93: result = ''
Line 94: for key, value in self.extra.iteritems():
Line 95: if isinstance(value, unicode):
Line 96: value = value.encode('utf-8', 'replace')
> Can you show unicode value that cannot be encoded as utf8 and needs the 're
You're right, there seems to be no such value; the unicode->string direction
should be safe.
Line 97: result += '%s=`%s`' % (key, value)
Line 98: if isinstance(msg, unicode):
Line 99: msg = msg.encode('utf-8', 'replace')
Line 100: result += '::%s' % msg
--
To view, visit https://gerrit.ovirt.org/48542
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1930817ade1799b218913348ffa099aab2211ef7
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logging: Don't crash on non-ASCII in SimpleLogAdapter
Milan Zamazal has posted comments on this change. Change subject: logging: Don't crash on non-ASCII in SimpleLogAdapter .. Patch Set 3: > this issue should be fixed by https://gerrit.ovirt.org/48661 Indeed, I think that patch fixes the problems and makes the logging change unnecessary. -- To view, visit https://gerrit.ovirt.org/48542 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1930817ade1799b218913348ffa099aab2211ef7 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Make BIOS messages available on vmconsole
Milan Zamazal has posted comments on this change.
Change subject: virt: Make BIOS messages available on vmconsole
..
Patch Set 4:
(3 comments)
Is the test better in patch set 5?
https://gerrit.ovirt.org/#/c/48404/4/tests/vmTests.py
File tests/vmTests.py:
Line 1407: self.assertEqual(res, response.error("thawErr",
Line 1408: message="fake error"))
Line 1409:
Line 1410:
Line 1411: class UseSerialTests(TestCaseBase):
> I'd rename 'SerialBiosTests'. Maybe put it alongside the other xml tests on
Right, there is no reason to put the test in a separate class, I moved it
alongside the other XML tests.
Line 1412:
Line 1413: def testUseSerial(self):
Line 1414: test_cases = (('serial', True),
Line 1415: ('virtio', False),)
Line 1411: class UseSerialTests(TestCaseBase):
Line 1412:
Line 1413: def testUseSerial(self):
Line 1414: test_cases = (('serial', True),
Line 1415: ('virtio', False),)
> please use permutations, there are many examples under tests/*
Done
Line 1416: for console_type, use_serial in test_cases:
Line 1417: devices = {'device': 'console', 'type': 'console',
Line 1418:'specParams': {'consoleType': console_type}},
Line 1419: with fake.VM(devices=devices,
create_device_objects=True) as v:
Line 1417: devices = {'device': 'console', 'type': 'console',
Line 1418:'specParams': {'consoleType': console_type}},
Line 1419: with fake.VM(devices=devices,
create_device_objects=True) as v:
Line 1420: dom_xml = v._buildDomainXML()
Line 1421: self.assertEqual('' in
dom_xml,
> please use smarter approach than string check, using ElementTree facilities
Done
Line 1422: use_serial)
Line 1423:
Line 1424:
Line 1425: def _load_xml(name):
--
To view, visit https://gerrit.ovirt.org/48404
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: logging: Don't crash on non-ASCII in SimpleLogAdapter
Milan Zamazal has posted comments on this change.
Change subject: logging: Don't crash on non-ASCII in SimpleLogAdapter
..
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/48542/3/tests/utilsTests.py
File tests/utilsTests.py:
Line 1082: def test_nonascii(self):
Line 1083: # Just test it doesn't crash with mixture of string and
unicodes in
Line 1084: # various codings.
Line 1085: invalid = chr(254) + chr(0)
Line 1086: u_invalid = unichr(253) + unichr(254)
> These values are not handled https://gerrit.ovirt.org/#/c/48661.
Hm, not very good. I think a typical situation where we may log binary strings
is something like
log.error("Binary/Invalid data received: %s", data)
That should be safe as long as data is unicode or no unicode is involved, e.g.
the following may be a problem:
log.debug("Data from %s: %s", unicode_id, str_data)
or
simple_log_adapter.debug(str_data)
I've got no idea how much real problem it is. If it is then we probably don't
have any reasonable option to handle that globally except for switching to
Python 3. We can just provide a utility conversion function if needed, e.g.
safe_unicode(data).
Anyway, 48611 is significant improvement, we should no longer crash with valid
text data and any unicode at least.
Line 1087: extra = dict(e1=u'Hello', e2='Здра́вствуйте', e3=u'שלום',
e4='你好',
Line 1088: e5=invalid, e6=u_invalid)
Line 1089: log = logUtils.SimpleLogAdapter(self._log, extra)
Line 1090: log.debug("Dobrý večer")
--
To view, visit https://gerrit.ovirt.org/48542
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1930817ade1799b218913348ffa099aab2211ef7
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[ovirt-3.6]: periodic: make VmDispatcher ignore TooManyTasks
Milan Zamazal has posted comments on this change. Change subject: periodic: make VmDispatcher ignore TooManyTasks .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/48409 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iad2d186327f8607aaeb41ca132a9a8a0806869cf Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.6 Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[ovirt-3.6]: vm: add support for "Crashed" event
Milan Zamazal has posted comments on this change. Change subject: vm: add support for "Crashed" event .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/48790 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I96e98cf7dc9c2c9e7507cc39e181fb2953032989 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.6 Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: devices: initial support for device updates with etree's
Milan Zamazal has posted comments on this change. Change subject: virt: devices: initial support for device updates with etree's .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/46525/1/vdsm/virt/domain_descriptor.py File vdsm/virt/domain_descriptor.py: Line 52: return self._devices Line 53: Line 54: def get_device_elements(self, tagName, etree=False): Line 55: return self._elements_by_tag_name(tagName, devices_only=True, Line 56: etree=etree) > Ad etree argument: What are our options? This is probably the primary remaining issue of this patch. Line 57: Line 58: @property Line 59: def devices_hash(self): Line 60: return self._devices_hash -- To view, visit https://gerrit.ovirt.org/46525 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7afe81af334ab9d8e7da6626cb902b95202cc05c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: config: Trivial typo fix in option description
Milan Zamazal has uploaded a new change for review.
Change subject: config: Trivial typo fix in option description
..
config: Trivial typo fix in option description
Change-Id: I527f3f64601d1845e5cca2c73b44d69f957df78f
Signed-off-by: Milan Zamazal
---
M lib/vdsm/config.py.in
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/20/48820/1
diff --git a/lib/vdsm/config.py.in b/lib/vdsm/config.py.in
index 1d0aa3a..bc8c9b2 100644
--- a/lib/vdsm/config.py.in
+++ b/lib/vdsm/config.py.in
@@ -110,7 +110,7 @@
('default_bridge', 'engine', None),
('migration_downtime', '500',
-'Maxmium allowed downtime for live migration in milliseconds '
+'Maximum allowed downtime for live migration in milliseconds '
'(anything below 100ms is ignored) if you do not care about '
'liveness of migration, set to a very high value, such as '
'60.'),
--
To view, visit https://gerrit.ovirt.org/48820
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I527f3f64601d1845e5cca2c73b44d69f957df78f
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: stomp: Drain pending bytes from SSLConnection
Milan Zamazal has posted comments on this change. Change subject: stomp: Drain pending bytes from SSLConnection .. Patch Set 2: Verified+1 I verified that it fixes https://bugzilla.redhat.com/1274670 for me: - I downloaded the change and installed it on two of my VDSM hosts. - In Engine, I started VM on one of the hosts. - In Engine, I migrated the VM to the other host. - It succeeded without errors. -- To view, visit https://gerrit.ovirt.org/48803 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ea37c35c871ae286863c5d4403bcf71ac803da5 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yeela Kaplan Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: vm: Update time on VM after resume
Milan Zamazal has uploaded a new change for review.
Change subject: virt: vm: Update time on VM after resume
..
virt: vm: Update time on VM after resume
When a VM is resumed from suspension and/or migrated, its clock
continues from the time of suspension, i.e. it's delayed. Even when NTP
is running on the VM, it may refuse to correct the time after a long
pause. This needs to be fixed.
There was some discussion whether libvirt should be automatically
responsible for correcting the time inside its VM operations such as
virDomainResume, see the bug below. The conclusion is that whatever the
right approach is, we currently have to handle the time correction
outside libvirt.
This change asks libvirt to correct the time after a VM is resumed.
It's not guaranteed that the call succeeds, e.g. it doesn't work without
QEMU guest agent running on the VM. So the time may still be incorrect
after our effort but we shouldn't make the original VM operation fail
just because of that.
Change-Id: Ieb583cd5d21e56d7730b0ba21d75ed93b9d34025
Bug-Url: https://bugzilla.redhat.com/1156194
Signed-off-by: Milan Zamazal
---
M vdsm/virt/vm.py
1 file changed, 20 insertions(+), 0 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/60/48860/1
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index db00480..4318a61 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -1191,6 +1191,25 @@
if not guestCpuLocked:
self._guestCpuLock.release()
+def _setTime(self):
+result = None
+t = time.time()
+seconds = int(t)
+nseconds = int((t - seconds) * 10**9)
+try:
+# The set time will be inherently inexact, but it won't be a future
+# time at least. If more precise time is needed, NTP should take
+# care of the rest and this call just ensures the time shift is not
+# out of common NTP tolerance.
+self._dom.setTime(time=dict(seconds=seconds, nseconds=nseconds))
+self.log.debug('Time updated')
+except Exception as e:
+# It may fail for various reasons, e.g. QEMU guest agent not
+# running on the VM.
+self.log.exception("Failed to set time")
+result = unicode(e)
+return result
+
def shutdown(self, delay, message, reboot, timeout, force):
if self.lastStatus == vmstatus.DOWN:
return response.error('noVM')
@@ -2790,6 +2809,7 @@
for dev in self._customDevices():
hooks.after_device_migrate_destination(
dev._deviceXML, self.conf, dev.custom)
+self._setTime()
if 'guestIPs' in self.conf:
del self.conf['guestIPs']
--
To view, visit https://gerrit.ovirt.org/48860
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ieb583cd5d21e56d7730b0ba21d75ed93b9d34025
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: vm: Update time on VM after resume
Milan Zamazal has posted comments on this change.
Change subject: virt: vm: Update time on VM after resume
..
Patch Set 1:
(5 comments)
https://gerrit.ovirt.org/#/c/48860/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 1200: # The set time will be inherently inexact, but it won't
be a future
Line 1201: # time at least. If more precise time is needed, NTP
should take
Line 1202: # care of the rest and this call just ensures the time
shift is not
Line 1203: # out of common NTP tolerance.
Line 1204: self._dom.setTime(time=dict(seconds=seconds,
nseconds=nseconds))
> why not use
No problem, done.
Line 1205: self.log.debug('Time updated')
Line 1206: except Exception as e:
Line 1207: # It may fail for various reasons, e.g. QEMU guest agent
not
Line 1208: # running on the VM.
Line 1201: # time at least. If more precise time is needed, NTP
should take
Line 1202: # care of the rest and this call just ensures the time
shift is not
Line 1203: # out of common NTP tolerance.
Line 1204: self._dom.setTime(time=dict(seconds=seconds,
nseconds=nseconds))
Line 1205: self.log.debug('Time updated')
> let's do this log in an else: clause
Done
Line 1206: except Exception as e:
Line 1207: # It may fail for various reasons, e.g. QEMU guest agent
not
Line 1208: # running on the VM.
Line 1209: self.log.exception("Failed to set time")
Line 1202: # care of the rest and this call just ensures the time
shift is not
Line 1203: # out of common NTP tolerance.
Line 1204: self._dom.setTime(time=dict(seconds=seconds,
nseconds=nseconds))
Line 1205: self.log.debug('Time updated')
Line 1206: except Exception as e:
> seems a bit too generic. isn't catching libvirtError enough?
I wanted to be on the safe side, but thinking about it again catching just
libvirtError is probably better. Done.
Line 1207: # It may fail for various reasons, e.g. QEMU guest agent
not
Line 1208: # running on the VM.
Line 1209: self.log.exception("Failed to set time")
Line 1210: result = unicode(e)
Line 1207: # It may fail for various reasons, e.g. QEMU guest agent
not
Line 1208: # running on the VM.
Line 1209: self.log.exception("Failed to set time")
Line 1210: result = unicode(e)
Line 1211: return result
> do we ever use this result?
No, we'll see what reviewers will say about calling the method. If it's OK to
ignore the result, we can remove it.
Line 1212:
Line 1213: def shutdown(self, delay, message, reboot, timeout, force):
Line 1214: if self.lastStatus == vmstatus.DOWN:
Line 1215: return response.error('noVM')
Line 2808:
Line 2809: for dev in self._customDevices():
Line 2810: hooks.after_device_migrate_destination(
Line 2811: dev._deviceXML, self.conf, dev.custom)
Line 2812: self._setTime()
> do we want to do this also in the migration destination path?
> do we want to do this also in the migration destination path?
I think so: There's a shorter or longer window when clock is not running during
migration, isn't it?
> Is this call safe to do on this path?
Not sure, it works for me, but someone more knowledgeable should tell.
Line 2813:
Line 2814: if 'guestIPs' in self.conf:
Line 2815: del self.conf['guestIPs']
Line 2816: if 'guestFQDN' in self.conf:
--
To view, visit https://gerrit.ovirt.org/48860
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb583cd5d21e56d7730b0ba21d75ed93b9d34025
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: debuging: Use __repr__ instead of __str__
Milan Zamazal has posted comments on this change. Change subject: debuging: Use __repr__ instead of __str__ .. Patch Set 1: Code-Review+1 I'd suggest documenting __str__ vs. __repr__ usage on http://www.ovirt.org/Vdsm_Coding_Guidelines once we settle on it. -- To view, visit https://gerrit.ovirt.org/48865 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2fd724e32dfde1ca484343730f2e55c37a59fdf3 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Ido Barkan Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: lib: executor: log pool status on discard
Milan Zamazal has posted comments on this change.
Change subject: lib: executor: log pool status on discard
..
Patch Set 5:
(1 comment)
https://gerrit.ovirt.org/#/c/48333/5/lib/vdsm/executor.py
File lib/vdsm/executor.py:
Line 111: if self._running:
Line 112: self._add_worker()
Line 113: # this is a debug aid, it is not that important to be precise;
Line 114: # intentionally done outside the lock
Line 115: self._log.info('pool[%i]: (%s)',
s/pool/executor pool/ or s/pool/worker pool/ ?
Line 116:len(self._workers),
Line 117:' '.join(str(w) for w in self._workers))
Line 118:
Line 119: def _worker_stopped(self, worker):
--
To view, visit https://gerrit.ovirt.org/48333
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I949181968a97a7bcec703bc36ef9e1a0f30c6858
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[ovirt-3.6]: daemon: ignore cpu affinity on single processor
Milan Zamazal has posted comments on this change. Change subject: daemon: ignore cpu affinity on single processor .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/48961 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0652189704cbce71d20ec809c9c26f081516758a Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.6 Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: debuging: Use __repr__ instead of __str__
Milan Zamazal has posted comments on this change. Change subject: debuging: Use __repr__ instead of __str__ .. Patch Set 1: > do you want to update this page now? OK, I'll do it once we are in confirmed agreement on the method use here. -- To view, visit https://gerrit.ovirt.org/48865 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2fd724e32dfde1ca484343730f2e55c37a59fdf3 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Ido Barkan Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: gitignore: Missing autogenerated files added
Milan Zamazal has posted comments on this change. Change subject: gitignore: Missing autogenerated files added .. Patch Set 1: Dan, is it OK to merge this trivial change to get rid of it? -- To view, visit https://gerrit.ovirt.org/48328 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If4a1e9c2607d817ccca2a321060ab4f452422c32 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Make BIOS messages available on vmconsole
Milan Zamazal has posted comments on this change.
Change subject: virt: Make BIOS messages available on vmconsole
..
Patch Set 5:
(1 comment)
Just renamed a one-char variable in tests.
https://gerrit.ovirt.org/#/c/48404/5/tests/vmTests.py
File tests/vmTests.py:
Line 292: @permutations([['serial', True], ['virtio', False]])
Line 293: def testSerialBios(self, console_type, use_serial):
Line 294: devices = {'device': 'console', 'type': 'console',
Line 295:'specParams': {'consoleType': console_type}},
Line 296: with fake.VM(devices=devices, create_device_objects=True) as
v:
> minor: we usually avoid so short names. "fakevm" or "testvm" could be a bet
Done
Line 297: dom_xml = v._buildDomainXML()
Line 298: tree = etree.fromstring(dom_xml)
Line 299: element = tree.find(".//bios[@useserial='yes']")
Line 300: self.assertEqual(element is not None, use_serial)
--
To view, visit https://gerrit.ovirt.org/48404
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: vm: Update time on VM after resume
Milan Zamazal has posted comments on this change. Change subject: virt: vm: Update time on VM after resume .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/48860/1/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2808: Line 2809: for dev in self._customDevices(): Line 2810: hooks.after_device_migrate_destination( Line 2811: dev._deviceXML, self.conf, dev.custom) Line 2812: self._setTime() > ok, let's save this for another (trivial) patch. For this patch, please cal Done here, I'll prepare the other patch. Line 2813: Line 2814: if 'guestIPs' in self.conf: Line 2815: del self.conf['guestIPs'] Line 2816: if 'guestFQDN' in self.conf: -- To view, visit https://gerrit.ovirt.org/48860 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieb583cd5d21e56d7730b0ba21d75ed93b9d34025 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: vm: Update time on VM after resume
Milan Zamazal has posted comments on this change.
Change subject: virt: vm: Update time on VM after resume
..
Patch Set 2:
(1 comment)
Jenkins happy after rebase.
https://gerrit.ovirt.org/#/c/48860/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 1208: self.log.exception("Failed to set time")
Line 1209: result = unicode(e)
Line 1210: else:
Line 1211: self.log.debug('Time updated to: %d.%09d', seconds,
nseconds)
Line 1212: return result
> (continuing discussion from patchset 1) I think we don't need this
OK, removed.
Line 1213:
Line 1214: def shutdown(self, delay, message, reboot, timeout, force):
Line 1215: if self.lastStatus == vmstatus.DOWN:
Line 1216: return response.error('noVM')
--
To view, visit https://gerrit.ovirt.org/48860
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb583cd5d21e56d7730b0ba21d75ed93b9d34025
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[ovirt-3.6]: vm: reformat _EVENT_STRINGS
Milan Zamazal has posted comments on this change. Change subject: vm: reformat _EVENT_STRINGS .. Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/48789 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6e8d577ebcee9e72cf70aab214f9ff42d46fcb9a Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.6 Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Make BIOS messages available on vmconsole
Milan Zamazal has posted comments on this change. Change subject: virt: Make BIOS messages available on vmconsole .. Patch Set 6: Verified+1 It still works, verified in the same way as in Patch Set 4. -- To view, visit https://gerrit.ovirt.org/48404 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: lib: executor: log pool status on discard
Milan Zamazal has posted comments on this change. Change subject: lib: executor: log pool status on discard .. Patch Set 6: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/48333 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I949181968a97a7bcec703bc36ef9e1a0f30c6858 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: vm: Update time on VM after resume
Milan Zamazal has posted comments on this change. Change subject: virt: vm: Update time on VM after resume .. Patch Set 3: (1 comment) https://gerrit.ovirt.org/#/c/48860/3/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 1190: finally: Line 1191: if not guestCpuLocked: Line 1192: self._guestCpuLock.release() Line 1193: Line 1194: def _setTime(self): > this is good, but let's do the final mile: > - let's rename this to something more intent-revealing, like 'resyncTime' Done. > - let's add a docstring. Done. > - let's make this public, so we can have one simple test in vmTests.py Making a method public means encouraging its use outside the class. Doing so just for tests doesn't look like a good idea to me, even when it's stated in the docstring. I can't see a problem in calling non-public methods from tests -- if the method changes we simply update the test; it's the same for public methods serving just for tests, unless we want to keep them stable just for the testing purpose (really?). Line 1195: t = time.time() Line 1196: seconds = int(t) Line 1197: nseconds = int((t - seconds) * 10**9) Line 1198: try: -- To view, visit https://gerrit.ovirt.org/48860 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieb583cd5d21e56d7730b0ba21d75ed93b9d34025 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: vm: Update time on VM after resume
Milan Zamazal has posted comments on this change. Change subject: virt: vm: Update time on VM after resume .. Patch Set 4: Additionally virdomain.NotConnectedError handled in this patch set. -- To view, visit https://gerrit.ovirt.org/48860 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieb583cd5d21e56d7730b0ba21d75ed93b9d34025 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[ovirt-3.6]: vmstats: network: avoid ZeroDivisionError
Milan Zamazal has posted comments on this change. Change subject: vmstats: network: avoid ZeroDivisionError .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/48478/1/vdsm/virt/vmstats.py File vdsm/virt/vmstats.py: Line 213: return None Line 214: if interval <= 0: Line 215: logging.warning( Line 216: 'invalid interval %i when computing network stats for vm %s', Line 217: interval, vm.id) Is interval really integer? Line 218: return None Line 219: Line 220: first_indexes = _find_bulk_stats_reverse_map(first_sample, 'net') Line 221: last_indexes = _find_bulk_stats_reverse_map(last_sample, 'net') -- To view, visit https://gerrit.ovirt.org/48478 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ff99e3706212179ef9dc56e92e7d5decdd6bd7a Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.6 Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[ovirt-3.6]: vmstats: network: avoid ZeroDivisionError
Milan Zamazal has posted comments on this change.
Change subject: vmstats: network: avoid ZeroDivisionError
..
Patch Set 2:
(1 comment)
https://gerrit.ovirt.org/#/c/48478/2/tests/vmStatsTests.py
File tests/vmStatsTests.py:
Line 357: stats = {}
Line 358: self.assertTrue(
Line 359: vmstats.networks(vm, stats,
Line 360: self.bulk_stats, self.bulk_stats,
Line 361: 0) is None
A bit too late, but still worth to fix in a future patch: `interval' unused
here.
Line 362: )
Line 363:
Line 364:
Line 365: class DiskStatsTests(VmStatsTestCase):
--
To view, visit https://gerrit.ovirt.org/48478
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ff99e3706212179ef9dc56e92e7d5decdd6bd7a
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[ovirt-3.6]: periodic: add __str__ methods
Milan Zamazal has posted comments on this change. Change subject: periodic: add __str__ methods .. Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/49143 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id0a0be2704f4b46093cd02406644121fc3c5fa5b Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.6 Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Make BIOS messages available on vmconsole
Milan Zamazal has posted comments on this change.
Change subject: virt: Make BIOS messages available on vmconsole
..
Patch Set 6:
(5 comments)
https://gerrit.ovirt.org/#/c/48404/6//COMMIT_MSG
Commit Message:
Line 8:
Line 9: ovirt-vmconsole allows access to VMs via serial console. However, this
Line 10: currently works only for processes running on the VM operating system,
Line 11: such as getty. It would be useful if vmconsole interacted immediately
Line 12: since the start of a VM.
> This is not clear - how do you wan to interact with the vm if not with some
Clarified.
Line 13:
Line 14: This change is the first step towards the goal. It enables BIOS serial
Line 15: console interaction in a VM's XML domain description when a serial
Line 16: console is available.
https://gerrit.ovirt.org/#/c/48404/6/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 1592: for consoleDev in self._devices[hwclass.CONSOLE]:
Line 1593: if consoleDev.isSerial:
Line 1594: break
Line 1595: else:
Line 1596: consoleDev = None
> Please avoid for-else syntax, it is always confusing and unneeded.
Done
Line 1597: bios_use_serial = consoleDev is not None
Line 1598:
Line 1599: domxml = vmxml.Domain(self.conf, self.log, self.arch)
Line 1600: domxml.appendOs(bios_use_serial=bios_use_serial)
Line 1593: if consoleDev.isSerial:
Line 1594: break
Line 1595: else:
Line 1596: consoleDev = None
Line 1597: bios_use_serial = consoleDev is not None
> This variable it not needed.
Removed.
Line 1598:
Line 1599: domxml = vmxml.Domain(self.conf, self.log, self.arch)
Line 1600: domxml.appendOs(bios_use_serial=bios_use_serial)
Line 1601:
Line 1596: consoleDev = None
Line 1597: bios_use_serial = consoleDev is not None
Line 1598:
Line 1599: domxml = vmxml.Domain(self.conf, self.log, self.arch)
Line 1600: domxml.appendOs(bios_use_serial=bios_use_serial)
> This should be:
Done
Line 1601:
Line 1602: if self.arch == caps.Architecture.X86_64:
Line 1603: osd = caps.osversion()
Line 1604:
https://gerrit.ovirt.org/#/c/48404/6/vdsm/virt/vmxml.py
File vdsm/virt/vmxml.py:
Line 230: self.dom.setAttr('xmlns:' + METADATA_VM_TUNE_PREFIX,
Line 231: METADATA_VM_TUNE_URI)
Line 232: self.dom.appendChild(self._metadata)
Line 233:
Line 234: def appendOs(self, bios_use_serial=False):
> Do we have other useserial attributes in this element?
Done
Line 235: """
Line 236: Add element to domain:
Line 237:
Line 238:
--
To view, visit https://gerrit.ovirt.org/48404
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Make BIOS messages available on vmconsole
Milan Zamazal has posted comments on this change. Change subject: virt: Make BIOS messages available on vmconsole .. Patch Set 7: Verified+1 Verified by manually testing it as usually. -- To view, visit https://gerrit.ovirt.org/48404 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Make BIOS messages available on vmconsole
Milan Zamazal has posted comments on this change.
Change subject: virt: Make BIOS messages available on vmconsole
..
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/48404/7/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 1561: return response.error('migCancelErr')
Line 1562: finally:
Line 1563: self._guestCpuLock.release()
Line 1564:
Line 1565: def _serialConsole(self):
> please use _getSerialConsole or _findSerialConsole() like we do elsewhere i
Done (originally wanted to be consistent with _customDevices immediately below).
Line 1566: """
Line 1567: Return serial console device.
Line 1568: If no serial console device is available, return 'None'.
Line 1569: """
--
To view, visit https://gerrit.ovirt.org/48404
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: introduce "Async" helper
Milan Zamazal has posted comments on this change. Change subject: virt: introduce "Async" helper .. Patch Set 2: Code-Review-1 (11 comments) I can't get rid of feeling that the implementation is too complicated for the simple purpose of checking the start success. Maybe using a semaphore inside do() and _set_started() would be a bit easier, not sure, already confused now :-). Or perhaps I completely miss something. https://gerrit.ovirt.org/#/c/49114/2/tests/vmUtilsTests.py File tests/vmUtilsTests.py: Line 195: done.set() Line 196: Line 197: self.async.do(helper) Line 198: Line 199: self.assertTrue(done.wait(1.)) Too many blank lines in all the tests? Are they needed at all? Line 200: Line 201: def test_raises_if_busy(self): Line 202: Line 203: def helper(): https://gerrit.ovirt.org/#/c/49114/2/vdsm/virt/utils.py File vdsm/virt/utils.py: Line 133: Line 134: Async takes care only of the startup phase; Line 135: it doesn't report or handle any error that Line 136: may happen in the callable right after the startup. Line 137: """ It should be documented here that the instance can't be reused, i.e. `do()' shouldn't be called twice (it'd be even better if it was additionally prevented or perhaps the instance made reusable). Line 138: Line 139: def __init__(self): Line 140: self._func = None Line 141: self._started = False Line 143: self._starting = threading.Condition() Line 144: Line 145: @property Line 146: def error(self): Line 147: """ Why property? It's quite confusing to have self._error and self.error -- self._error and self.error() would be better. And do we need this public method at all? do() already provides the information in some way. So unless we want to examine the result in a separate thread (really?), this public method is not needed, as demonstrated in vm.py. Line 148: Returns the startup error of the callable, or Line 149: None if operation succeded. Line 150: Block until the callable started, either succesfully Line 151: or failing. Line 162: `func' may be callable without arguments. Line 163: The return value of `func' is ignored. Line 164: `daemon' and `logger' arguments behave like Line 165: concurrent.thread. Line 166: """ Please document that the method throws an exception on start error. Line 167: Line 168: self._func = func Line 169: self._thread = concurrent.thread( Line 170: self._run, Line 164: `daemon' and `logger' arguments behave like Line 165: concurrent.thread. Line 166: """ Line 167: Line 168: self._func = func Why using the attribute and not pass `func' to concurrent.thread? Line 169: self._thread = concurrent.thread( Line 170: self._run, Line 171: daemon=daemon, Line 172: logger=logger) Line 172: logger=logger) Line 173: self._thread.start() Line 174: Line 175: err = self.error Line 176: if err: According to error() docstring anything but None is error, so `if err is not None:' should be used here. Line 177: raise AsyncStartError(err) Line 178: Line 179: def _run(self): Line 180: self._set_started(error=None) Line 189: Line 190: class AsyncThrottler(Async): Line 191: """ Line 192: AsyncThrottler runs a callable into a background thread, Line 193: like Async, but only if can acquire an user-provided ... if it can acquire a ... Line 194: Semaphore-like object. Line 195: Line 196: AsyncThrottler fails to start (do() returns error) Line 197: if it can't acquire such semaphore. Line 192: AsyncThrottler runs a callable into a background thread, Line 193: like Async, but only if can acquire an user-provided Line 194: Semaphore-like object. Line 195: Line 196: AsyncThrottler fails to start (do() returns error) ... do() raises AsyncStartError ... Line 197: if it can't acquire such semaphore. Line 198: In that case returns the user-provided error code. Line 199: """ Line 200: Line 194: Semaphore-like object. Line 195: Line 196: AsyncThrottler fails to start (do() returns error) Line 197: if it can't acquire such semaphore. Line 198: In that case returns the user-provided error code. ... case it returns ... Line 199: """ Line 200: Line 201: def __init__(self, sem, errcode): Line 202: super(AsyncThrottler, self).__init__() Line 201: def __init__(self, sem, errcode): Line 202: super(AsyncThrottler, self).__init__() Line 203: self._sem = sem Line 204: self._errcode = errcode Line 205: _error_code and error_code would be better names. Line 206: def _run(self): Line 207: acquired = self._sem.acquire(False) Line 208:
Change in vdsm[master]: startup: Change system default encoding to utf8
Milan Zamazal has posted comments on this change. Change subject: startup: Change system default encoding to utf8 .. Patch Set 5: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/48661 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icc3f072a499ba4034bdbedd09eb60d7a3a32f9c8 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: introduce "Async" helper
Milan Zamazal has posted comments on this change. Change subject: virt: introduce "Async" helper .. Patch Set 2: Martin, I think we can go with Async if we can simplify it. E.g. how about something like the following in do(): self._started = threading.Event() ... start the thread .. self._started.wait() # for set() in _set_started We could eliminate some Async attributes + maybe error(). My primary concern is what error() is actually for. -- To view, visit https://gerrit.ovirt.org/49114 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibd277197c7c3819008ae71a8cf2c0679b901397e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Betak Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Make BIOS messages available on vmconsole
Milan Zamazal has posted comments on this change.
Change subject: virt: Make BIOS messages available on vmconsole
..
Patch Set 8:
(2 comments)
https://gerrit.ovirt.org/#/c/48404/8/tests/vmTests.py
File tests/vmTests.py:
Line 289: xml = find_xml_element(domxml.dom.toxml(), './sysinfo')
Line 290: self.assertXMLEqual(xml, sysinfoXML)
Line 291:
Line 292: @permutations([['serial', True], ['virtio', False]])
Line 293: def testSerialBios(self, console_type, use_serial):
> I would like to only lowercase names in new code, even if it is less consis
No problem for me. Francesco?
Line 294: devices = {'device': 'console', 'type': 'console',
Line 295:'specParams': {'consoleType': console_type}},
Line 296: with fake.VM(devices=devices, create_device_objects=True) as
fakevm:
Line 297: dom_xml = fakevm._buildDomainXML()
https://gerrit.ovirt.org/#/c/48404/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 1569: """
Line 1570: for console in self._devices[hwclass.CONSOLE]:
Line 1571: if console.isSerial:
Line 1572: return console
Line 1573: return None
> Maybe raise NotFound error instead returning None?
It may be a valid concern but at least in the current code it would require us
to handle the exception without any benefit. Not having serial console is
perfectly OK, not an exceptional situation, after all.
Line 1574:
Line 1575: def _customDevices(self):
Line 1576: """
Line 1577: Get all devices that have custom properties
--
To view, visit https://gerrit.ovirt.org/48404
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: vm: Update time on VM after migration
Milan Zamazal has uploaded a new change for review.
Change subject: virt: vm: Update time on VM after migration
..
virt: vm: Update time on VM after migration
When a VM is resumed from suspension and/or migrated, its clock
continues from the time of suspension, i.e. it's delayed. We already
addressed the problem by introducing VM._syncTime() method. But for
safety reasons we call the method only in the restoreState path, see
https://gerrit.ovirt.org/48860.
This patch calls _syncTime in migrationDest path as well. It serves the
originally intended purpose of restoring the clock also after migration,
just separates it from the original solution in case something bad could
happen with migration due to it.
Change-Id: I21ebeecf52bc3f72a2cb7fe9cd9da7b6fd1a86d7
Bug-Url: https://bugzilla.redhat.com/1156194
Signed-off-by: Milan Zamazal
---
M vdsm/virt/vm.py
1 file changed, 2 insertions(+), 4 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/12/49212/1
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 9aa08bb..7f33c8d 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -2801,10 +2801,6 @@
fromSnapshot = self.conf.pop('restoreFromSnapshot', False)
hooks.after_vm_dehibernate(self._dom.XMLDesc(0), self.conf,
{'FROM_SNAPSHOT': fromSnapshot})
-# TODO: _syncTime() should probably be called for both the
-# restore_state and migration_dest paths, but let's handle just the
-# safer case first.
-self._syncTime()
elif 'migrationDest' in self.conf:
if self._needToWaitForMigrationToComplete():
usedTimeout = self._waitForUnderlyingMigration()
@@ -2819,6 +2815,8 @@
hooks.after_device_migrate_destination(
dev._deviceXML, self.conf, dev.custom)
+self._syncTime()
+
if 'guestIPs' in self.conf:
del self.conf['guestIPs']
if 'guestFQDN' in self.conf:
--
To view, visit https://gerrit.ovirt.org/49212
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I21ebeecf52bc3f72a2cb7fe9cd9da7b6fd1a86d7
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: enhance/fix migration.SourceThread.stop()
Milan Zamazal has posted comments on this change. Change subject: virt: enhance/fix migration.SourceThread.stop() .. Patch Set 12: Verified+1 With migration working again in master, I rebased and tried to cancel VM migration at several different times after migration initiation and it worked. -- To view, visit https://gerrit.ovirt.org/40520 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0ab50fc789dde969b2fb9ab969241ed4ad12545c Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Make BIOS messages available on vmconsole
Milan Zamazal has posted comments on this change. Change subject: virt: Make BIOS messages available on vmconsole .. Patch Set 8: Verified+1 I verified by: - Building Engine version ovirt-engine-3.6.0.3 + fbfaf94 (vmconsole didn't work for me in newer versions). - Disabling VirtIO serial console in the engine for a given VM and enabling it again (to convert it to serial type in the engine). - Starting the VM in pause mode and with boot menu enabled. - Connecting to the console: ssh -t -p ovirt-vmconsole@ENGINE-HOST - Unpausing the VM. - Watching boot menu, selecting its items, watching GRUB menu and basic interaction with it (selecting menu items, typing in the command prompt after `c'). -- To view, visit https://gerrit.ovirt.org/48404 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: vm: Update time on VM after resume
Milan Zamazal has posted comments on this change. Change subject: virt: vm: Update time on VM after resume .. Patch Set 4: Verified+1 Verified by: - Suspending a VM (without NTP running) from engine for a while. - Awaking the VM and checking time on it is current. - Checking the presence of "Time updated" message in vdsm.log and that there are no errors around. -- To view, visit https://gerrit.ovirt.org/48860 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieb583cd5d21e56d7730b0ba21d75ed93b9d34025 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: tests: proper cleanup in periodic tests
Milan Zamazal has posted comments on this change. Change subject: tests: proper cleanup in periodic tests .. Patch Set 5: Code-Review+1 Just for curiosity: Why unreliable test results -- is it due to extra resource consumption by old threads or something else? -- To view, visit https://gerrit.ovirt.org/47249 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If07198bb368f79ace4b944612c71ee135c4458de Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[ovirt-3.6]: clientIF: add logs during the recovery
Milan Zamazal has posted comments on this change. Change subject: clientIF: add logs during the recovery .. Patch Set 4: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/49140 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I31dddf0a2bc760c5ad383ff6bfee9a72adc87c4f Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.6 Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: packaging: updating for VDSM 4.17.4 on Debian jessie
Milan Zamazal has posted comments on this change.
Change subject: packaging: updating for VDSM 4.17.4 on Debian jessie
..
Patch Set 35: Code-Review-1
(10 comments)
Current Debian packaging looks like a reasonable start, let's polish it. I
think the first steps should be:
- Packaging missing things not present in Debian. I can see you filed some
ITPs, do you have already something or are we starting from scratch?
- Fixing installation directories, making them Debian/FHS compliant.
- Avoiding running tests during the build.
- Deciding about hooks: single package or many small packages?
Then we can continue with other things. IMO the ultimate goal should be making
VDSM an official Debian package; I guess we have about somewhat less than one
year to achieve that (till the next Debian freeze); that should be enough.
BTW, please use `lintian -i vdsm*.changes' to check the built packages, it
catches a lot of problems.
https://gerrit.ovirt.org/#/c/37737/35/debian/changelog
File debian/changelog:
Line 1: vdsm (4.17.4-1) unstable; urgency=low
Line 2:
Line 3: * vdsm 4.17.4 release on Debian Jessie
Let's use something like "New upstream version" here. We should start with
packaging for unstable then we can backport if needed.
Line 4:
Line 5: -- Simone Tiraboschi Fri, 04 Sep 2015 12:00:00
+0200
Line 6:
Line 7: vdsm (4.17.0-1) unstable; urgency=low
https://gerrit.ovirt.org/#/c/37737/35/debian/control
File debian/control:
Line 5: Build-Depends: debhelper (>= 8.0.0),
Line 6: autoconf,
Line 7: automake,
Line 8: dosfstools,
Line 9: gcc,
gcc shouldn't be here, it's available automatically.
Line 10: genisoimage,
Line 11: gettext,
Line 12: libasprintf-dev,
Line 13: libgettextpo-dev,
Line 27: python-pyinotify,
Line 28: python-pthreading (>=0.1.2),
Line 29: python-rpm,
Line 30: python-selinux,
Line 31: sudo (>= 1.7.3),
We shouldn't build-depend on this. I'd suggest not to run tests when building
the package, it probably causes more trouble than benefit. The following build
dependencies are probably unneeded when tests are not run: dosfstools,
genisoimage, gettext, libasprintf-dev, libgettextpo-dev, libtool, openssl, mom,
psmisc, python, python-dmidecode, python-rpm, python-simplejson,
python-ioprocess.
Line 32: python-simplejson (>= 2.0.9),
Line 33: python-ioprocess (>= 0.14)
Line 34: Standards-Version: 3.9.4
Line 35: Homepage: http://www.ovirt.org/wiki/Vdsm
Line 29: python-rpm,
Line 30: python-selinux,
Line 31: sudo (>= 1.7.3),
Line 32: python-simplejson (>= 2.0.9),
Line 33: python-ioprocess (>= 0.14)
The following build dependencies seem to be missing: git, dh-python.
Line 34: Standards-Version: 3.9.4
Line 35: Homepage: http://www.ovirt.org/wiki/Vdsm
Line 36: Vcs-Git: git://gerrit.ovirt.org/vdsm
Line 37: Vcs-Browser: http://gerrit.ovirt.org/gitweb?p=vdsm.git
Line 30: python-selinux,
Line 31: sudo (>= 1.7.3),
Line 32: python-simplejson (>= 2.0.9),
Line 33: python-ioprocess (>= 0.14)
Line 34: Standards-Version: 3.9.4
Please update the version to the current one.
Line 35: Homepage: http://www.ovirt.org/wiki/Vdsm
Line 36: Vcs-Git: git://gerrit.ovirt.org/vdsm
Line 37: Vcs-Browser: http://gerrit.ovirt.org/gitweb?p=vdsm.git
Line 38:
Line 346: Architecture: all
Line 347: Depends: ${shlibs:Depends}, ${misc:Depends}, python (>=2.7.3), vdsm
(>= ${source:Version})
Line 348: Description: To configure spice options for vm
Line 349: This vdsm hook can be used to configure some of
Line 350: the spice optimization attributes and values.
Wouldn't it be better to make just a single vdsm-hooks package containing all
the hooks? The user could enable hooks by linking them to /etc/vdsm/hooks or
so. I don't like polluting the package space with many miniature packages
although there are some precedents (e.g. localization packages). Additionally,
each new hook would block the package from upload as new packages require
manual approval from ftpmasters.
Line 351:
Line 352: Package: vdsm-infra
Line 353: Architecture: all
Line 354: Depends: ${shlibs:Depends}, ${misc:Depends}, python (>=2.7.3)
https://gerrit.ovirt.org/#/c/37737/35/debian/vdsm-client.docs
File debian/vdsm-client.docs:
Line 1
This shouldn't be installed, GPL is always available in
/usr/share/common-licenses/GPL-2. (This applies to all *.docs files here.)
https://gerrit.ovirt.org/#/c/37737/35/debian/vdsm-hook-allocate_net.install
File debian/vdsm-hook-allocate_net.install:
Line 1: usr/libexec/vdsm/hooks/before_device_create/10_allocate_net
There is no /usr/libexec on Debian. I think /usr/share is a good location.
The same applies to the other hooks.
https://gerrit.ovirt.org/#/c/37737/35/debian/vdsm-xmlrpc.install
File debian/vdsm-xmlrpc.install:
Line 1
This package is essentially empty!
https://gerrit.ovirt.org/#/c/37737/35/debian/vdsm.dirs
File debian/vdsm.dirs:
Line 4: etc/pki/vdsm/keys
Line 5: etc/pki/vdsm/libvirt-spice
Line 6: etc/v
Change in vdsm[master]: guestagent: Send complete messages
Milan Zamazal has posted comments on this change. Change subject: guestagent: Send complete messages .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/49339 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic832634c4d670aa88fe79a50b2e57246d08b4a08 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: guestagent: Remove unneeded encoding
Milan Zamazal has posted comments on this change. Change subject: guestagent: Remove unneeded encoding .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/49338 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I95e0875d6a2428714e9d979cb9db34a6399f4623 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: guestagent: Use %r for logging message
Milan Zamazal has posted comments on this change. Change subject: guestagent: Use %r for logging message .. Patch Set 1: Code-Review+1 Of course, this is going to escape all such characters in the message, but I assume it's a desired side effect. -- To view, visit https://gerrit.ovirt.org/49340 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e895800b3074bec942003c29b6f0766c01b0424 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: guestagent: Log guest agent life cycle events
Milan Zamazal has posted comments on this change. Change subject: guestagent: Log guest agent life cycle events .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/49341 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d6706e10d37a1268894185b08f59010c5e26850 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: guestagent: Rename connect() to start()
Milan Zamazal has posted comments on this change. Change subject: guestagent: Rename connect() to start() .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/49342 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I649075cf67cb0db4e1aede14586d0b26f2bf59b4 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: guestagent: Add missing log on disconnect
Milan Zamazal has posted comments on this change. Change subject: guestagent: Add missing log on disconnect .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/49343 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I608aa0e47acb120c2a5a150ad77e813f6f546397 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: lib: daemon: autodetect online cpus for affinity
Milan Zamazal has posted comments on this change.
Change subject: lib: daemon: autodetect online cpus for affinity
..
Patch Set 2: Code-Review-1
(8 comments)
Basically fine, just please polish it a bit (especially documentation).
https://gerrit.ovirt.org/#/c/49402/2//COMMIT_MSG
Commit Message:
Line 16: is offline, it fails to start.
Line 17:
Line 18: This was indeed expected behaviour, and the fix is trivial (just
Line 19: reconfigure /etc/vdsm/vdsm.conf), but it is very unpractical in large
Line 20: settings, where thw admin has to manually fix many configuration
... the admin ...
Line 21: files. This obviously breaks automated flow.
Line 22:
Line 23: We still want to leverage the gains the affinity gives us, so
Line 24: the only option is to make VDSM smarter, and let it lear the
Line 20: settings, where thw admin has to manually fix many configuration
Line 21: files. This obviously breaks automated flow.
Line 22:
Line 23: We still want to leverage the gains the affinity gives us, so
Line 24: the only option is to make VDSM smarter, and let it lear the
... learn ...
Line 25: set of online cpus when it starts.
Line 26: After VDSM starts, the kernel is smart enough to handle the offlining
Line 27: of one cpu where some processes are pinned, so the problem
Line 28: is just the startup.
https://gerrit.ovirt.org/#/c/49402/2/lib/vdsm/taskset.py
File lib/vdsm/taskset.py:
Line 78:
Line 79:
Line 80: def online_cpus():
Line 81: """
Line 82: Return a frozenset which contains the cpu number of
It was difficult for me to parse the "number of" phrase. How about something
like: ... contains identifiers of online CPUs, as non-negative integers.
Line 83: online cpus.
Line 84: """
Line 85: return _expand_range(_read_contents(_SYS_ONLINE_CPUS))
Line 86:
Line 86:
Line 87:
Line 88: def pick_cpu(cpu_set):
Line 89: """
Line 90: Autoselect the best CPU VDSM should pin to.
Please document cpu_set argument.
Line 91: """
Line 92: cpu_list = list(sorted(cpu_set))
Line 93:
Line 94: if len(cpu_list) == 1:
Line 88: def pick_cpu(cpu_set):
Line 89: """
Line 90: Autoselect the best CPU VDSM should pin to.
Line 91: """
Line 92: cpu_list = list(sorted(cpu_set))
Isn't `list' redundant here?
Line 93:
Line 94: if len(cpu_list) == 1:
Line 95: # we don't really have a choice here
Line 96: return cpu_list[0]
Line 94: if len(cpu_list) == 1:
Line 95: # we don't really have a choice here
Line 96: return cpu_list[0]
Line 97:
Line 98: if cpu_list[0] == 0:
Shorter version of the 4 lines above is
if cpu_list[0] == 0 and len(cpu_list) > 1:
Line 99: # cpu #0 is the default target, so it could get overcrowded.
Line 100: # Let's play nice, and try to find a quieter spot.
Line 101: return cpu_list[1]
Line 102:
https://gerrit.ovirt.org/#/c/49402/2/tests/tasksetTests.py
File tests/tasksetTests.py:
Line 138: [set((0,)), 0],
Line 139: [set((1,)), 1],
Line 140: [set(range(4)), 1],
Line 141: [set(range(1, 4)), 1],
Line 142: [set(range(3, 9)), 3],
frozenset should be used here instead of set.
Line 143: ])
Line 144: def test_pick_cpu(self, cpu_set, expected):
Line 145: self.assertEqual(taskset.pick_cpu(cpu_set), expected)
Line 146:
https://gerrit.ovirt.org/#/c/49402/2/vdsm/vdsm
File vdsm/vdsm:
Line 231: for cpu in cpu_affinity.split(","))
Line 232:
Line 233: log = logging.getLogger('vds')
Line 234:
Line 235: if len(online_cpus) == 1:
Shouldn't this be checked before computing cpu_set?
Line 236: log.warning('Only one cpu detected: affinity disabled')
Line 237: return
Line 238:
Line 239: log.info('VDSM will run with cpu affinity: %s', cpu_set)
--
To view, visit https://gerrit.ovirt.org/49402
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba834f67a43ce766308036cbd079107340ed69d8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik
Gerrit-Reviewer: Michal Skrivanek
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Vinzenz Feenstra
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: lib: daemon: autodetect online cpus for affinity
Milan Zamazal has posted comments on this change. Change subject: lib: daemon: autodetect online cpus for affinity .. Patch Set 3: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/49402 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba834f67a43ce766308036cbd079107340ed69d8 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: lib: daemon: autodetect online cpus for affinity
Milan Zamazal has posted comments on this change. Change subject: lib: daemon: autodetect online cpus for affinity .. Patch Set 4: (1 comment) https://gerrit.ovirt.org/#/c/49402/4//COMMIT_MSG Commit Message: Line 34: l ... add fragility. -- To view, visit https://gerrit.ovirt.org/49402 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba834f67a43ce766308036cbd079107340ed69d8 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Vinzenz Feenstra Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: externalVMList Xen+Kvm support
Milan Zamazal has posted comments on this change. Change subject: v2v: externalVMList Xen+Kvm support .. Patch Set 12: (2 comments) https://gerrit.ovirt.org/#/c/48672/12/vdsm/v2v.py File vdsm/v2v.py: Line 647: if e.get_error_code() != libvirt.VIR_ERR_NO_SUPPORT: Line 648: raise Line 649: seen = set() Line 650: for name in conn.listDefinedDomains(): Line 651: vm = conn.lookupByName(name) Shouldn't vmid be added to `seen'? Line 652: yield vm Line 653: for vmid in conn.listDomainsID(): Line 654: if vmid in seen: Line 655: continue Line 653: for vmid in conn.listDomainsID(): Line 654: if vmid in seen: Line 655: continue Line 656: vm = conn.lookupByID(vmid) Line 657: seen.add(vmid) And is this one needed (duplicates in listDomainsID?)? Line 658: yield vm Line 659: Line 660: Line 661: def _add_vm(conn, vms, vm): -- To view, visit https://gerrit.ovirt.org/48672 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic7d7e211a9343a528f260da2686b34cea00c53a4 Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: externalVMList Xen+Kvm support
Milan Zamazal has posted comments on this change. Change subject: v2v: externalVMList Xen+Kvm support .. Patch Set 13: Code-Review+1 (4 comments) Please fix the typos, otherwise fine for me. https://gerrit.ovirt.org/#/c/48672/13//COMMIT_MSG Commit Message: Line 11: We want to use one verb to probe VMs information from Xen, KVM and Line 12: vCenter, as well as old libvirt versions. Line 13: Tested with RHEL 5.x and above. Line 14: Line 15: Currently we externalVMList tested only with vCenter: ... we tested externalVMList ... Line 16: vpx://... Line 17: Line 18: Adding support for Kvm via libvirt: Line 19: qemu+tcp://user@host/system https://gerrit.ovirt.org/#/c/48672/13/tests/v2vTests.py File tests/v2vTests.py: Line 117: def listAllDomains(self): Line 118: return [vm for vm in self._vms] Line 119: Line 120: def listDefinedDomains(self): Line 121: # listDefinedDomains return only inactive domains ... returns ... Line 122: return [vm.name for vm in self._vms if not vm.isActive] Line 123: Line 124: def listDomainsID(self): Line 125: # listDomainsID return only active domains Line 121: # listDefinedDomains return only inactive domains Line 122: return [vm.name for vm in self._vms if not vm.isActive] Line 123: Line 124: def listDomainsID(self): Line 125: # listDomainsID return only active domains ... returns ... Line 126: return [vm.id for vm in self._vms if vm.isActive] Line 127: Line 128: def lookupByName(self, name): Line 129: for vm in self._vms: https://gerrit.ovirt.org/#/c/48672/13/vdsm/v2v.py File vdsm/v2v.py: Line 641: try: Line 642: for vm in conn.listAllDomains(): Line 643: yield vm Line 644: except libvirt.libvirtError as e: Line 645: # TODO: use new API: listAllDomain() when supported in Xen ... listAllDomains() ... Line 646: # under RHEL 5.x Line 647: if e.get_error_code() != libvirt.VIR_ERR_NO_SUPPORT: Line 648: raise Line 649: seen = set() -- To view, visit https://gerrit.ovirt.org/48672 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic7d7e211a9343a528f260da2686b34cea00c53a4 Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: enhance migration.SourceThread.stop()
Milan Zamazal has posted comments on this change. Change subject: virt: enhance migration.SourceThread.stop() .. Patch Set 13: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/40520 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0ab50fc789dde969b2fb9ab969241ed4ad12545c Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: add run_async helper
Milan Zamazal has posted comments on this change.
Change subject: virt: add run_async helper
..
Patch Set 1:
(2 comments)
Looks nice to me. Just docstrings are missing and see my comments on tests.
https://gerrit.ovirt.org/#/c/49570/1/tests/vmUtilsTests.py
File tests/vmUtilsTests.py:
Line 169: def test_ignores_error(self):
Line 170: def helper():
Line 171: raise RuntimeError("Async() doesn't care")
Line 172:
Line 173: error = utils.run_async(helper)
run_async doesn't return anything. Technically, it should return None, so the
test is formally OK, but it's misleading. Let's just check it doesn't raise an
exception.
Line 174: self.assertEqual(error, None)
Line 175:
Line 176:
Line 177: class RunAsyncThrottledTests(TestCaseBase):
Line 179: def setUp(self):
Line 180: self.sem = threading.BoundedSemaphore(1)
Line 181:
Line 182: def test_do(self):
Line 183:
Please use less blank lines (IMHO none are needed inside such short methods,
even around inner single-line functions).
Line 184: done = threading.Event()
Line 185:
Line 186: def helper():
Line 187: done.set()
--
To view, visit https://gerrit.ovirt.org/49570
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icfb405389c465d2a8b8fc8b6f958926d58167a26
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Tomas Jelinek
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: add try/except to get_external_vms
Milan Zamazal has posted comments on this change.
Change subject: v2v: add try/except to get_external_vms
..
Patch Set 2:
(2 comments)
https://gerrit.ovirt.org/#/c/49507/2/vdsm/v2v.py
File vdsm/v2v.py:
Line 175: e.message, vm.XMLDesc(0))
Line 176: continue
Line 177: try:
Line 178: _add_networks(root, params)
Line 179: except libvirt.libvirtError as e:
Can this be raised here? It seems to me that _add_networks just processes
parsed DOM data.
Line 180: logging.error('error reading network configuration:
%s', e)
Line 181: continue
Line 182: try:
Line 183: _add_disks(root, params)
Line 180: logging.error('error reading network configuration:
%s', e)
Line 181: continue
Line 182: try:
Line 183: _add_disks(root, params)
Line 184: except libvirt.libvirtError as e:
The same.
Line 185: logging.error('error reading disks configuration:
%s', e)
Line 186: continue
Line 187: for disk in params['disks']:
Line 188: _add_disk_info(conn, disk)
--
To view, visit https://gerrit.ovirt.org/49507
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec63dea83ec1805cf08d35be7c078edfd2f70966
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Shahar Havivi
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: add try/except to get_external_vms
Milan Zamazal has posted comments on this change.
Change subject: v2v: add try/except to get_external_vms
..
Patch Set 2:
(1 comment)
https://gerrit.ovirt.org/#/c/49507/2/vdsm/v2v.py
File vdsm/v2v.py:
Line 175: e.message, vm.XMLDesc(0))
Line 176: continue
Line 177: try:
Line 178: _add_networks(root, params)
Line 179: except libvirt.libvirtError as e:
> if we raise here we will not present the user any VMs for one rouge VM.
My point is that libvirtError is never raised inside _add_networks or
_add_disks (unless I miss something) so this except clause doesn't make sense.
Line 180: logging.error('error reading network configuration:
%s', e)
Line 181: continue
Line 182: try:
Line 183: _add_disks(root, params)
--
To view, visit https://gerrit.ovirt.org/49507
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec63dea83ec1805cf08d35be7c078edfd2f70966
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Shahar Havivi
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: add try/except to get_external_vms
Milan Zamazal has posted comments on this change.
Change subject: v2v: add try/except to get_external_vms
..
Patch Set 3:
(2 comments)
https://gerrit.ovirt.org/#/c/49507/3/vdsm/v2v.py
File vdsm/v2v.py:
Line 154: params = {}
Line 155: try:
Line 156: _add_vm_info(vm, params)
Line 157: except libvirt.libvirtError as e:
Line 158: logging.error("error getting domain infomation: %s",
e)
Maybe adding vm.name() to the log message could be useful?
Should we use logging.error or logging.exception here (i.e. do we want to have
traceback in the log)?
Line 159: continue
Line 160: try:
Line 161: xml = vm.XMLDesc(0)
Line 162: except libvirt.libvirtError as e:
Line 165: continue
Line 166: try:
Line 167: root = ET.fromstring(xml)
Line 168: except ET.ParseError as e:
Line 169: logging.error('error parsing domain xml: %s', e)
The same.
Line 170: continue
Line 171: try:
Line 172: _add_general_info(root, params)
Line 173: except InvalidVMConfiguration as e:
--
To view, visit https://gerrit.ovirt.org/49507
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec63dea83ec1805cf08d35be7c078edfd2f70966
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Shahar Havivi
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[ovirt-3.6]: v2v: add try/except to get_external_vms
Milan Zamazal has posted comments on this change. Change subject: v2v: add try/except to get_external_vms .. Patch Set 2: Code-Review-1 I think the corresponding master patch is actually https://gerrit.ovirt.org/49507. Not yet merged, so let's update this one after the master patch is finished. -- To view, visit https://gerrit.ovirt.org/49438 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iec63dea83ec1805cf08d35be7c078edfd2f70966 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.6 Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: add try/except to get_external_vms
Milan Zamazal has posted comments on this change.
Change subject: v2v: add try/except to get_external_vms
..
Patch Set 3: Code-Review+1
(1 comment)
https://gerrit.ovirt.org/#/c/49507/3/vdsm/v2v.py
File vdsm/v2v.py:
Line 154: params = {}
Line 155: try:
Line 156: _add_vm_info(vm, params)
Line 157: except libvirt.libvirtError as e:
Line 158: logging.error("error getting domain infomation: %s",
e)
> The vm.name() can be the exception.
I see, thanks for clarification.
Line 159: continue
Line 160: try:
Line 161: xml = vm.XMLDesc(0)
Line 162: except libvirt.libvirtError as e:
--
To view, visit https://gerrit.ovirt.org/49507
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec63dea83ec1805cf08d35be7c078edfd2f70966
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal
Gerrit-Reviewer: Shahar Havivi
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: add run_async helper
Milan Zamazal has posted comments on this change. Change subject: virt: add run_async helper .. Patch Set 2: (3 comments) https://gerrit.ovirt.org/#/c/49570/2/vdsm/virt/utils.py File vdsm/virt/utils.py: Line 121: Line 122: Line 123: class AsyncStartError(Exception): Line 124: """ Line 125: Impossible to start the execution of a callable. ... the asynchronous execution ... Line 126: """ Line 127: def __init__(self, error): Line 128: self.error = error Line 129: Line 131: def run_async(func, name=None, daemon=False, logger=None, Line 132: semaphore=None, error='failed to start'): Line 133: """ Line 134: Execute one callable, `func', in a background thread. Line 135: If `name' is not None set, set thread name. Better: If `name' is not None, set it as the thread name. Line 136: If `daemon' is True, create a daemon thread. Line 137: If `logger` is set, unhandled exceptions which occurs Line 138: after the execution started will be logged on this logger; Line 139: Otherwise the root logger will be used. Line 141: sempahore before to start the `func' callable, and will Line 142: release it once `func' exits. Line 143: If `error' is not None, will use as return value to Line 144: feed AsyncStartError. Line 145: """ Please edit the whole docstring, there are several typos and grammar problems there, making it difficult to read. Line 146: starting_error = [None] Line 147: started = threading.Event() Line 148: Line 149: def _throttle(): -- To view, visit https://gerrit.ovirt.org/49570 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icfb405389c465d2a8b8fc8b6f958926d58167a26 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Betak Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Tomas Jelinek Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
