Martin Polednik has posted comments on this change. Change subject: vdsm: virt: add optional container support ......................................................................
Patch Set 51: Code-Review-1 (9 comments) https://gerrit.ovirt.org/#/c/53820/51/lib/vdsm/containersconnection.py File lib/vdsm/containersconnection.py: PS51, Line 33: _runtimes = frozenset() I have inner hate for how this is further "calculated" (also considering the followup patch) - do we *have to* use the frozenset? I can't see the benefit of overwriting frozenset 3 times over |='ing it. https://gerrit.ovirt.org/#/c/53820/51/lib/vdsm/containerslib.py File lib/vdsm/containerslib.py: PS51, Line 99: def _is_cmd(argv, reqv): Why this isn't attribute of SuperVdsmCommand class? It's slight implementation leak imho. PS51, Line 113: def _call(ret): Same as above, I believe it should be __call__ on top of that (internally calling self._execute). PS51, Line 153: container connection PS51, Line 161: ev s/ev/event PS51, Line 177: For clearing connections during the tests. Only? Runtime code shouldn't be polluted by test helpers. https://gerrit.ovirt.org/#/c/53820/51/vdsm/virt/recovery.py File vdsm/virt/recovery.py: PS51, Line 167: all_vms s/vms/domains ? PS51, Line 177: def _all_vms_from_runtime(cif): s/vms/domains ? Also, what does runtime mean in this context? We're not in any runtime in recovery flow afaik. https://gerrit.ovirt.org/#/c/53820/51/vdsm/virt/vm.py File vdsm/virt/vm.py: PS51, Line 1657: if is_kvm(self.conf): My vote for split or helper function remains. -- 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: 51 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- [email protected] To unsubscribe send an email to [email protected]
