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 <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

Reply via email to