Change in vdsm[master]: vm: migration: exponential downtime increment

danken at redhat.com danken at redhat.com
Mon Sep 15 10:48:43 UTC 2014


Dan Kenigsberg has posted comments on this change.

Change subject: vm: migration: exponential downtime increment
......................................................................


Patch Set 7: Code-Review-1

(6 comments)

http://gerrit.ovirt.org/#/c/25820/7/tests/vmMigrationTests.py
File tests/vmMigrationTests.py:

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 value.
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 specific asserts when possible.
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.


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.
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.
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)
> Minor: I'd go with yield int(offset + base ** i) in order to guarantee the 
+1, particularly since on Python 3, (downtime / steps) may be in ℝ∖ℕ, anyway.
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 <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Betak <mbetak at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list