Martin Sivák has posted comments on this change.
Change subject: Samples: add memory based load-balancing sample module
......................................................................
Patch Set 6: Code-Review-1
(6 comments)
....................................................
File plugins/examples/host_memory_balance.py
Line 2: from ovirtsdk.api import API
Line 3: import sys
Line 4:
Line 5:
Line 6: class host_memory_balance():
Please use "object" as a parent
Line 7: '''migrate vms from over utilized hosts'''
Line 8:
Line 9: #What are the values this module will accept, used to present
Line 10: #the user with options
Line 15: free_memory_cache = {}
Line 16:
Line 17: def quit(self):
Line 18: print ['', []]
Line 19: exit()
exit()? Do you really want to kill the whole process? I do not think that is a
good idea..
Line 20:
Line 21: def _get_connection(self):
Line 22: #open a connection to the rest api
Line 23: connection = None
Line 43: if not host.id in self.free_memory_cache:
Line 44: try:
Line 45: self.free_memory_cache[host.id] =
host.get_statistics().get(
Line 46:
'memory.free').get_values().get_value()[0].get_datum()
Line 47: except:
Add the exception type here (at least Exception), exception: is evil and will
bite you :)
Listing the exceptions here would be better though: (KeyError, AttributeError)
Line 48: self.free_memory_cache[host.id] = -1
Line 49:
Line 50: return self.free_memory_cache[host.id]
Line 51:
Line 54: and a list of under utilized hosts'''
Line 55: over_utilized_host = None
Line 56: under_utilized_hosts = []
Line 57: for host in engine_hosts:
Line 58: if(host):
You could invert the condition to:
if not host:
continue
and get one indentation level back in the rest of the block
Line 59: free_memory = self.getFreeMemory(host)
Line 60: if(free_memory <= 0):
Line 61: continue
Line 62: if free_memory > minimum_host_memory:
Line 137: self.getOverUtilizedHostAndUnderUtilizedList(engine_hosts,
Line 138:
minimum_host_memory))
Line 139:
Line 140: if over_utilized_host is None:
Line 141: self.quit()
I would prefer simple returns here, see my comment in the quit() method
Line 142:
Line 143: maximum_vm_memory =
self.getMaximumVmMemory(under_utilized_hosts,
Line 144:
minimum_host_memory)
Line 145:
Line 161:
Line 162: under_utilized_hosts_ids = [host.id for host in
under_utilized_hosts]
Line 163: print (selected_vm.id, under_utilized_hosts_ids)
Line 164:
Line 165:
Is this test code? Then it should probably be under
if __name__ == "__main__":
--
To view, visit http://gerrit.ovirt.org/19934
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1b4a41064631912f5624723327a2ff5ebf158d5c
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-scheduler-proxy
Gerrit-Branch: master
Gerrit-Owner: Noam Slomianko <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Martin Sivák <[email protected]>
Gerrit-Reviewer: Noam Slomianko <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
