gerrit-hooks has posted comments on this change.
Change subject: tests: Add tests for poll and wait failures
......................................................................
Patch Set 7:
* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
--
To view, visit https://gerrit.ovirt.org/65294
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8674c9c3c2118041c74213cd8ce0d383086d6cbf
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
gerrit-hooks has posted comments on this change.
Change subject: network: supervdsm: configure container networks
......................................................................
Patch Set 45:
* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
--
To view, visit https://gerrit.ovirt.org/54998
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I338ca2d3abb0b1447c5a18c97afb9e14314f4107
Gerrit-PatchSet: 45
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
gerrit-hooks has posted comments on this change.
Change subject: vdsm: virt: add optional container support
......................................................................
Patch Set 52:
* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
--
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: 52
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
gerrit-hooks has posted comments on this change.
Change subject: supervdsm: expose systemd utilities
......................................................................
Patch Set 32:
* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
--
To view, visit https://gerrit.ovirt.org/56491
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e9a346da784fc200a82d9e5d9fdf665e752987
Gerrit-PatchSet: 32
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
Martin Polednik has posted comments on this change.
Change subject: network: supervdsm: configure container networks
......................................................................
Patch Set 44:
The code is somewhat fine considering the TODOs, but question remains: if I install vdsm-containers AFTER already using VDSM, do I explicitly have to setup_networks again? How is user informed?
--
To view, visit https://gerrit.ovirt.org/54998
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I338ca2d3abb0b1447c5a18c97afb9e14314f4107
Gerrit-PatchSet: 44
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
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 <fromani(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
Martin Polednik has posted comments on this change.
Change subject: supervdsm: expose systemd utilities
......................................................................
Patch Set 31: Code-Review+1
--
To view, visit https://gerrit.ovirt.org/56491
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38e9a346da784fc200a82d9e5d9fdf665e752987
Gerrit-PatchSet: 31
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No