Martin Polednik has posted comments on this change. Change subject: vdsm: virt: add optional container support ......................................................................
Patch Set 58: (11 comments) I believe the code is ready, just leaving some food for thought in comments. https://gerrit.ovirt.org/#/c/53820/58/lib/vdsm/containersconnection.py File lib/vdsm/containersconnection.py: PS58, Line 27: XML = containerslib.XML I have difficulty figuring out how this is further used - can you elaborate? PS58, Line 30: XML = None Symmetric issue as ^^^. https://gerrit.ovirt.org/#/c/53820/58/lib/vdsm/containerslib.py File lib/vdsm/containerslib.py: PS58, Line 59: class SuperVdsmCommand(SubProcCommand): The class overall isn't wrong, I'm just having hard time fitting it to VDSMs "way of things". We normally restrict the supervdsm by making small functions that encompass the call directly. My main "problem" is that it's very clever - and clever things can get tricky in future. Please weight the options of going with current approach VS this class. If you decide for the class, please add some high level documentation ("This is restriction of supervdsm...."). Not a blocker though. PS58, Line 134: global _connections needed? PS58, Line 135: global _lock needed? https://gerrit.ovirt.org/#/c/53820/58/tests/vmTests.py File tests/vmTests.py: PS58, Line 108: 'vmType': 'kvm' I don't think this is a good change if you want to prove that the patches don't break current communication. https://gerrit.ovirt.org/#/c/53820/58/vdsm.spec.in File vdsm.spec.in: PS58, Line 713: Looks like random tab happened. PS58, Line 718: containersm typo :) https://gerrit.ovirt.org/#/c/53820/58/vdsm/virt/vm.py File vdsm/virt/vm.py: PS58, Line 1697: # Currently there is no protection agains mirroring : # a network twice, Unrelated & offset didn't change in this PS (I assume it's from the past). PS58, Line 1718: self.log.exception( : "Unexpected error on guest event notification") Unrelated, similar to above. Line 1725: con.prepare() Line 1726: Line 1727: self._guestCpuRunning = self._isDomainRunning() Line 1728: self._logGuestCpuStatus('domain initialization') Line 1729: Unrelated, although not harmful (I'd seriously leave it here). Line 1730: if self.lastStatus not in (vmstatus.MIGRATION_DESTINATION, Line 1731: vmstatus.RESTORING_STATE): Line 1732: self._initTimePauseCode = self._readPauseCode() Line 1733: if not self.recovering and self._initTimePauseCode: -- To view, visit https://gerrit.ovirt.org/53820 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id236a30a5c875994c037b8d00c7463bceaab143f Gerrit-PatchSet: 58 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
