Change in vdsm[master]: mom: add mom balloon functional tests for running vms

danken at redhat.com danken at redhat.com
Mon Jun 10 20:03:23 UTC 2013


Dan Kenigsberg has posted comments on this change.

Change subject: mom: add mom balloon functional tests for running vms
......................................................................


Patch Set 6: I would prefer that you didn't submit this

(8 inline comments)

Thanks, Mei Liu for your functional test. I think we can get to an even prettier code.

....................................................
File tests/functional/momTests.py
Line 80:     def _prepare(self, operation, vmsStats):
Line 81:         # Filter all vms' statistics to get balloon operation candidates.
Line 82:         candidateStats = []
Line 83:         for stats in vmsStats:
Line 84:                 if self._statsOK(stats):
indentation issue
Line 85:                     candidateStats.append(stats)
Line 86: 
Line 87:         # Set the balloon target to initial value before shrink
Line 88:         # or grow operation.


Line 81:         # Filter all vms' statistics to get balloon operation candidates.
Line 82:         candidateStats = []
Line 83:         for stats in vmsStats:
Line 84:                 if self._statsOK(stats):
Line 85:                     candidateStats.append(stats)
a more pythonic way to do it is:

  candidateStats = [stats for stats in vmstats if self._statsOK(stats)]
Line 86: 
Line 87:         # Set the balloon target to initial value before shrink
Line 88:         # or grow operation.
Line 89:         # The initial value is max for shrink operation and


Line 88:         # or grow operation.
Line 89:         # The initial value is max for shrink operation and
Line 90:         # 0.9*max for grow operation.
Line 91:         ratioName = operation.lower() + "Ratio"
Line 92:         for vmStats in candidateStats:
here you override the input argument vmStats with a loop variable. Also, few lines above you have called the same item simply "stats". That's confusing.
Line 93:             initial = vmStats['balloonInfo']['balloon_max'] * \
Line 94:                 getattr(self, ratioName).initial
Line 95:             if vmStats['balloonInfo']['balloon_cur'] != initial:
Line 96:                 r = self.s.setBalloonTarget(


Line 106:         file_name = os.path.join(curpath, policies[operation])
Line 107:         try:
Line 108:             with open(file_name, 'r') as f:
Line 109:                 testPolicyStr = f.read()
Line 110:         except IOError:
please make sure it's indeed an ENOENT error, or at least include the error in the reported string.
Line 111:             raise SkipTest('The Policy file is missing.')
Line 112: 
Line 113:         r = self.s.setMOMPolicy(testPolicyStr)
Line 114:         self.assertOK(r)


Line 107:         try:
Line 108:             with open(file_name, 'r') as f:
Line 109:                 testPolicyStr = f.read()
Line 110:         except IOError:
Line 111:             raise SkipTest('The Policy file is missing.')
Better report the missing file name, too.
Line 112: 
Line 113:         r = self.s.setMOMPolicy(testPolicyStr)
Line 114:         self.assertOK(r)
Line 115: 


Line 145:         # expected value and adjustment step in policy.
Line 146:         # This range also takes accuracy impact into account(The number is
Line 147:         # rounded to integer).
Line 148:         self.shrinkRatio = self.BalloonRatio(1, 0.9, 0.9025)
Line 149:         self.growRatio = self.BalloonRatio(0.9, 0.99225, 1)
Adding attributes outside __init__ is allowed by Python, but it is still confusing and not recommended.

Specifically, your code tries to be general, and support any kind of operation, but this assignment shows that it is limited to shrink and grow alone. Note that you set both values TWICE: once in every time that _basicBalloon is called.

I think that your code would become much cleaner if you pass the BalloonRatio() as argument to _basicBalloon, which would then propagate it to _prepare() and  _checkResult. You could then avoid the hacky getattr() calls.
Line 150: 
Line 151:         vmsOldStats = self._prepare(operation, r['statsList'])
Line 152: 
Line 153:         if not vmsOldStats:


Line 154:             raise SkipTest('No VM can be candidate of ballooning operation.')
Line 155: 
Line 156:         self._setPolicy(operation)
Line 157:         # Wait for the policy taking effect
Line 158:         time.sleep(35)
where is this 35 constant coming from? It is quite long, can it be squeezed a bit?
Line 159: 
Line 160:         self._checkResult(operation, vmsOldStats)
Line 161: 
Line 162:     @testValidation.ValidateRunningAsRoot


Line 158:         time.sleep(35)
Line 159: 
Line 160:         self._checkResult(operation, vmsOldStats)
Line 161: 
Line 162:     @testValidation.ValidateRunningAsRoot
why do we need root? Any user should be able to run xmlrpc commands (as long as it has a signed key/cert).
Line 163:     def testBalloonShrink(self):
Line 164:         self._basicBalloon("Shrink")
Line 165: 
Line 166:     @testValidation.ValidateRunningAsRoot


-- 
To view, visit http://gerrit.ovirt.org/13156
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I922568233dc769d83e2fdffe1c24439d13d03d7e
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mei Liu <liumbj at linux.vnet.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Martin Sivák <msivak at redhat.com>
Gerrit-Reviewer: Mei Liu <liumbj at linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list