Francesco Romani has posted comments on this change.
Change subject: vdsm: virt: add optional container support ......................................................................
Patch Set 51:
(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 th
no, we don't really need a frozenset, we can probably kill 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 implementat
true, but OTOH this code doesn't use the SuperVdsmCommand state at all, so could also be a free function
PS51, Line 113: def _call(ret):
Same as above, I believe it should be __call__ on top of that (internally c
SubProcCommand.__call__ already does that!
Again, this is a free function because it doesn't use any of the SuperVdsmCommand state. Anyway it's open for discussion, I'm not entirely happy with both designs. If you think it's clearer as method, I'll make this a method (same for _is_cmd above)
PS51, Line 153:
container connection
Done
PS51, Line 161: ev
s/ev/event
Done
PS51, Line 177: For clearing connections during the tests.
Only? Runtime code shouldn't be polluted by test helpers.
And for consistency with libvirtconnection. But we don't really care here, so let's kill this.
https://gerrit.ovirt.org/#/c/53820/51/vdsm/virt/recovery.py File vdsm/virt/recovery.py:
PS51, Line 167: all_vms
s/vms/domains ?
yes, a bit better. Changed.
PS51, Line 177: def _all_vms_from_runtime(cif):
s/vms/domains ?
_all_running_domains(cif) is probably clearer and a better name
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.
Let's retry with a different partiotioning.
vdsm-patches@lists.fedorahosted.org