Francesco Romani has posted comments on this change. Change subject: vm: migration: exponential downtime increment ......................................................................
Patch Set 7: -Verified (7 comments) -Verified because needs more work. http://gerrit.ovirt.org/#/c/25820/7/tests/vmMigrationTests.py File tests/vmMigrationTests.py: Line 33: return izip(a, b) Line 34: Line 35: Line 36: def _linear_downtime(downtime, steps): Line 37: for i in range(steps): > maybe range(1, steps + 1) and later downtime * i / steps for consistence wi Good point, will do. Line 38: yield downtime * (i + 1) / steps Line 39: Line 40: Line 41: class TestVmMigrationDowntimeSequence(TestCaseBase): Line 38: yield downtime * (i + 1) / steps Line 39: Line 40: Line 41: class TestVmMigrationDowntimeSequence(TestCaseBase): Line 42: # defaults as per config > read them from config, instead of having this comment and duplicating the v Sure, will fix. Line 43: _DOWNTIME = 500 Line 44: Line 45: _STEPS = 10 Line 46: Line 61: self.assertTrue(len(self._default) >= 2) Line 62: Line 63: def test_downtime_increasing(self): Line 64: for a, b in pairwise(self._default): Line 65: self.assertTrue(a <= b) > assertLessEqual() etc gives more info upon failure - please use more specif I'd love to, but https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertLessEqual -> new in version 2.7 And AFAIK it wasn't backported (git grep confirms) Line 66: Line 67: def test_exponential_dowtime_never_zero(self): Line 68: for dt in self._default: Line 69: self.assertTrue(dt > 0) Line 78: self.assertTrue(a <= b) Line 79: Line 80: def test_exponential_same_end_value(self): Line 81: self.assertAlmostEqual(self._default[-1], self._linear[-1], Line 82: delta=self._DELTA) > You'd also want to assert that the final step is the maximum allow downtime right, I'll add. http://gerrit.ovirt.org/#/c/25820/7/vdsm/virt/migration.py File vdsm/virt/migration.py: Line 354: raise Line 355: Line 356: Line 357: def exponential_downtime(downtime, steps): Line 358: offset = downtime / steps > Let's do a float division on Python2, too. will fix Line 359: base = (downtime - offset) ** (1 / float(steps)) Line 360: Line 361: for i in range(1, steps+1): Line 362: yield offset + int(base ** i) Line 357: def exponential_downtime(downtime, steps): Line 358: offset = downtime / steps Line 359: base = (downtime - offset) ** (1 / float(steps)) Line 360: Line 361: for i in range(1, steps+1): > use spaces around the + sign. oops, will fix. Line 362: yield offset + int(base ** i) Line 363: Line 364: Line 365: class DowntimeThread(threading.Thread): Line 358: offset = downtime / steps Line 359: base = (downtime - offset) ** (1 / float(steps)) Line 360: Line 361: for i in range(1, steps+1): Line 362: yield offset + int(base ** i) > +1, particularly since on Python 3, (downtime / steps) may be in ℝ∖ℕ, anywa Good point, that's exactly the comment I was looking for :) Thanks! Line 363: Line 364: Line 365: class DowntimeThread(threading.Thread): Line 366: DOWNTIME_STEPS = config.getint('vars', 'migration_downtime_steps') -- To view, visit http://gerrit.ovirt.org/25820 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6401772f52ea28144452e67198bddff18f6703eb Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches