Saggi Mizrahi has posted comments on this change.
Change subject: Move VM logic to it's own module (pt.1)
......................................................................
Patch Set 4: (3 inline comments)
....................................................
File vdsm/API.py
Line 134:
Line 135: for methName in GUEST_AGENT_METHODS:
Line 136: self.setattr(attrName, partial(guestTrampoline, methName))
Line 137:
Line 138: def _runThinMethod(self, methodName, *args, **kwargs):
Yea, because everytime you want to change something you have to do it 2000
times.
This is what functions are for. It's easier to vet 1 method instead of a 1000.
Repeated flows should be put to methods.
You could argue that this might be easier to understand:
def Foo(*args, **kwargs):
self._runThinMethod("Foo", *args, **kwargs)
Instead of generating the 2 line methods dynamically. But this is just not
pythonic IMO.
In any case, the big issue is that the internal methods mess with the outbound
structures making the API layer a bit weird.
I suspect that as we go along and clean vm.py \ libvirtvm.py. More of these
methods would gain substance and will not be thin wrappers anymore.
The funny thing about python, is that it's easier to make sure you have no
mistakes in dynamically generated methods then in concrete methods, making them
safer.
By doing this I don't need to go through every API call making sure I don't
have a typo. I know it will be generated correctly as intended.
Further more, this way I make it more visible that most changes should actually
be done in the underlying layer because this layer is obviously redundant.
Line 139: v = self._cif.vmContainer.get(self._UUID)
Line 140: if not v:
Line 141: return errCode['noVM']
Line 142:
....................................................
File vdsm/vmContainer.py
Line 19: self._vms = {}
Line 20:
Line 21: def createVm(self, vmParams):
Line 22: vmId = vmParams['vmId']
Line 23: self.log.info("vmContainerLock acquired by vm %s", vmId)
This was in the original code, I personally, don't understand why we even
announce this. And if we do want to announce it, you could add logging to the
lock object itself and make it reusable.
Line 24:
Line 25: with self._syncRoot:
Line 26: if 'recover' not in vmParams:
Line 27: if vmId in self._vms:
Line 34:
Line 35: container_len = len(self._vms)
Line 36:
Line 37: vm.run()
Line 38: self.log.debug("Total desktops after creation of %s is %d" %
Again, as written in original code, I don't mind changing it. I just didn't
want to start arguing about such small changes and have this block. But, alas,
it was a futile attempt as these will always attract -1s whether I change it or
I don't.
Line 39: (vmId, container_len))
Line 40:
Line 41: return vm
Line 42:
--
To view, visit http://gerrit.ovirt.org/7423
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc54c1d0ab7ad535d825dcd72fd2f4583b690f44
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Adam Litke <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
