Change in vdsm[master]: vdsm: virt: add optional container support

[Code Review]

From Dan Kenigsberg :

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vdsm: virt: add optional container support
..


vdsm: virt: add optional container support

We add an additional pluggable package for
container support in Vdsm.

This package, previously known with the codename `convirt`[1],
allows Vdsm to run container images alongside VMs, invoking
and managing docker directly. The supervision is done
by systemd and by docker daemon, so no extra packages are needed.

The user will run a container by creating a VM using unmodifed
Engine >= 3.6, adding a few extra custom variables.
Those variables will inform Vdsm to treat the VM as container,
but from Engine perspective there will be no observable change.

For the same reason, no Engine changes are needed at all; actions
not supported by containers (e.g. live migration) will fail using
standard Vdsm errors. In the future, the unsupported actions will
be grayed out in the Engine UI, to prevent confusion.

+++

[1] https://github.com/mojaves/convirt

Change-Id: Id236a30a5c875994c037b8d00c7463bceaab143f
Signed-off-by: Francesco Romani 
Reviewed-on: https://gerrit.ovirt.org/53820
Continuous-Integration: Jenkins CI
Reviewed-by: Dan Kenigsberg 
---
M lib/api/vdsm-api.yml
M lib/vdsm/Makefile.am
M lib/vdsm/config.py.in
A lib/vdsm/containersconnection.py
M lib/vdsm/virt/Makefile.am
M lib/vdsm/virt/periodic.py
A lib/vdsm/virt/xmlconstants.py
M tests/vmRecoveryTests.py
M tests/vmfakelib.py
M vdsm.spec.in
M vdsm/caps.py
M vdsm/clientIF.py
M vdsm/virt/recovery.py
M vdsm/virt/vm.py
14 files changed, 209 insertions(+), 13 deletions(-)

Approvals:
  Jenkins CI: Passed CI tests
  Dan Kenigsberg: Looks good to me, approved
  Francesco Romani: Verified



-- 
To view, visit https://gerrit.ovirt.org/53820
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Id236a30a5c875994c037b8d00c7463bceaab143f
Gerrit-PatchSet: 65
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[Code Review]

From Dan Kenigsberg :

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 64: Code-Review+2

copying score

-- 
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: 64
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[Code Review]

From Dan Kenigsberg :

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 63: Code-Review+2

(1 comment)

https://gerrit.ovirt.org/#/c/53820/18//COMMIT_MSG
Commit Message:

Line 6: 
Line 7: vdsm: virt: add optional container support
Line 8: 
Line 9: We add an additional pluggable package for
Line 10: container support in Vdsm.
put a nice explanation to convirt, including a github link.
Line 11: 
Line 12: Change-Id: Id236a30a5c875994c037b8d00c7463bceaab143f


-- 
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: 63
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[mpolednik]

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 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 58:

* Update Tracker::IGNORE, no bug url/s found
* Check Bug-Url::IGNORE, not relevant for branch: master
* Check Public Bug::WARN, no public bug url found
* 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: 58
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 57:

* Update Tracker::IGNORE, no bug url/s found
* Check Bug-Url::IGNORE, not relevant for branch: master
* Check Public Bug::WARN, no public bug url found
* 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: 57
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 56:

* Update Tracker::IGNORE, no bug url/s found
* Check Bug-Url::IGNORE, not relevant for branch: master
* Check Public Bug::WARN, no public bug url found
* 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: 56
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 55:

* Update Tracker::IGNORE, no bug url/s found
* Check Bug-Url::IGNORE, not relevant for branch: master
* Check Public Bug::WARN, no public bug url found
* 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: 55
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 54:

* Update Tracker::IGNORE, no bug url/s found
* Check Bug-Url::IGNORE, not relevant for branch: master
* Check Public Bug::WARN, no public bug url found
* 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: 54
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[fromani]

Francesco Romani has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 51:

(1 comment)

https://gerrit.ovirt.org/#/c/53820/51/lib/vdsm/containerslib.py
File lib/vdsm/containerslib.py:

PS51, Line 99: def _is_cmd(argv, reqv):
> Yeah I'm slightly torn. I understand only coupling state dependent function
Looks slightly better if is a method, because of the functionality link.

A nested function would probably be the better tool, but makes
_execute too hard to read. let's stick with the method.


-- 
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 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 51:

(1 comment)

https://gerrit.ovirt.org/#/c/53820/51/lib/vdsm/containerslib.py
File lib/vdsm/containerslib.py:

PS51, Line 99: def _is_cmd(argv, reqv):
> true, but OTOH this code doesn't use the SuperVdsmCommand state at all, so 
Yeah I'm slightly torn. I understand only coupling state dependent 
functionality, but still dislike top-level (although "protected") functions 
that do share the "hidden" functionality link to some object. Not sure if it 
doesn't cross the edge of being personal dislike rather than proper improvement.


-- 
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 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 53:

* Update Tracker::IGNORE, no bug url/s found
* Check Bug-Url::IGNORE, not relevant for branch: master
* Check Public Bug::WARN, no public bug url found
* 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: 53
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[fromani]

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.


-- 
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 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

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 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[mpolednik]

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 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 51:

* 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: 51
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 50:

* 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: 50
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 49:

* 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: 49
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 48:

* 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: 48
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 47:

* 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: 47
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 46:

* 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: 46
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


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/53820
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id236a30a5c875994c037b8d00c7463bceaab143f
Gerrit-PatchSet: 45
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 44:

* 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: 44
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[fromani]

Francesco Romani has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 43: Code-Review-1

(15 comments)

Martin's comments needs to be addressed.

https://gerrit.ovirt.org/#/c/53820/43/lib/vdsm/containerslib.py
File lib/vdsm/containerslib.py:

PS43, Line 41: _container_runtimes = vdsm.virt.containers.runtime.supported()
> How is this used?
it is used in a boolean context, to detect if the 'container runtimes' (aka the 
'runc', 'rkt', whatever) tools are present.
Will also be reported in caps, so Engine could know;
this is not needed in the current form, but doesn't hurt either.


PS43, Line 42: from vdsm.virt.containers import monitorAllDomains as 
monitor_containers
 : from vdsm.virt.containers import recoveryAllDomains as 
recovery
> This doesn't even exist in this patch, why? ;; Answering myself and future 
exactly.

The idea is that Vdsm should run as today with minimal changes, besides the 
ones made on this topic, and other minor cleanups;
If one wants to play with containers, the vdsm-containers subpackage needs to 
be installed, which brings in all the code and dependencies needed for the job.

This is also the reason for containerconnection/containerlib split.
containerconnection is the facade/frontend, will always be present, but will do 
nothing if containerlib is missing.
The latter will be part of vdsm-containers.


PS43, Line 55: RUNTIME_PATH = '/var/run/ovirt-containers'
> Shouldn't we be under vdsm dir?
yes, this is a relic of the past, will change to /var/run/vdsm/containers


PS43, Line 60: _connections = {}
> Do we expect multiple connections? What is the use case?
No use case, just tryint to mimic libvirtconnection as closely as possible 
-were feasible.
Applies here and to convirtconnection.py


PS43, Line 79: if _is_cmd(argv, ['rkt', 'status', '*']):
 : out = _retval(sv.rkt_status(argv[2]))
 : elif _is_cmd(argv, ['machinectl', 'poweroff', '*']):
 : out = _retval(sv.machinectl_poweroff(argv[2]))
 : elif _is_cmd(argv, ['systemctl', 'stop', '*']):
 : out = _retval(sv.systemctl_stop(argv[2]))
 : elif _is_cmd(argv, ['systemd-run', '*']):
 : opts, args = 
self._parser.parse_known_args(argv[1:])
 : out = _retval(sv.systemd_run(opts.unit, 
opts.slice, *args))
 : else:
 : out = _retval(commands.execCmd(argv))
> I'd prefer a dict if possible.
Me too, just didn't find a better way yet. Will try harder.

More context:
what I want to do here is to do some basic validation.
Here we have one junction between vdsm proper and container world.

Vdsm could run commands as root, so he knows "how".
But containers module knows "what".

Vdsm should not blindly trust containers, so it should not just run as root 
whatever it is given, so here we do minimal validation.

Please note that no, there is no obvious reason for this additional checks, 
meaning that the user/guest *cannot* inject code here, so we are not more in 
danger than from the other parts of Vdsm.

This is a legacy of when the containers module was out of tree and indepedent; 
I'd like to keep this additional validation for the time being, until this 
feature is marked as no-longer-experimental;
Also, as matter of personal preference, I'd like not to blindly run code as 
root, even if it is supposed to be trusted, so I'd love to keep this, if it is 
practical


PS43, Line 105: if req == '*':
> Makes the function extremely general, do you expect that for
We could stop our validation at first '*' (or any other marker is less 
confusing), actually.
The problem is that 'systemd-run' is complex and I didn't yet find a way to do 
better validation.


PS43, Line 131: get()
> get_connection()?
TL;DR: yes

it was a ripoff of libvirtconnection (perhaps too brutal).
I will rename it because it is convirtconnection that needs to have the same 
API of libvirtconnection. Here we have more freedom.


PS43, Line 161: ev
> 'event' would be slightly more obvious.
will rename.


https://gerrit.ovirt.org/#/c/53820/43/tests/vmTests.py
File tests/vmTests.py:

PS43, Line 108: 'vmType': 'kvm'
> This is unrelated change and it lowers my trust for BC. I would prefer this
will change, it is also possible this is not needed anymore.


https://gerrit.ovirt.org/#/c/53820/43/vdsm/caps.py
File vdsm/caps.py:

PS43, Line 205: containers
> Does this really refer to 'containers'? Wouldn't keeping the name 'containe
seems better indeed, will rename.


https://gerrit.ovirt.org/#/c/53820/43/vdsm/supervdsm_api/containers.py
File vdsm/supervdsm_api/containers.py:

PS43, Line 31: # actions:
 : #if ! [ -d "${container_path}" ]; then
 : #

Change in vdsm[master]: vdsm: virt: add optional container support

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 43:

(15 comments)

Basic review, I still haven't wrapped my head around the bigger picture.

https://gerrit.ovirt.org/#/c/53820/43/lib/vdsm/containerslib.py
File lib/vdsm/containerslib.py:

PS43, Line 41: _container_runtimes = vdsm.virt.containers.runtime.supported()
How is this used?


PS43, Line 42: from vdsm.virt.containers import monitorAllDomains as 
monitor_containers
 : from vdsm.virt.containers import recoveryAllDomains as 
recovery
This doesn't even exist in this patch, why? ;; Answering myself and future 
readers: it is added later in the series. It should be explicitly noted that 
this patch itself *will* fail the import.


PS43, Line 55: RUNTIME_PATH = '/var/run/ovirt-containers'
Shouldn't we be under vdsm dir?


PS43, Line 60: _connections = {}
Do we expect multiple connections? What is the use case?


PS43, Line 79: if _is_cmd(argv, ['rkt', 'status', '*']):
 : out = _retval(sv.rkt_status(argv[2]))
 : elif _is_cmd(argv, ['machinectl', 'poweroff', '*']):
 : out = _retval(sv.machinectl_poweroff(argv[2]))
 : elif _is_cmd(argv, ['systemctl', 'stop', '*']):
 : out = _retval(sv.systemctl_stop(argv[2]))
 : elif _is_cmd(argv, ['systemd-run', '*']):
 : opts, args = 
self._parser.parse_known_args(argv[1:])
 : out = _retval(sv.systemd_run(opts.unit, 
opts.slice, *args))
 : else:
 : out = _retval(commands.execCmd(argv))
I'd prefer a dict if possible.


PS43, Line 105: if req == '*':
Makes the function extremely general, do you expect that for

 argv = ['a', 'stop', '123']
 reqv = ['a', '*', '*']

is_cmd is valid?  Feels slightly confusing.


PS43, Line 131: get()
get_connection()?


PS43, Line 161: ev
'event' would be slightly more obvious.


https://gerrit.ovirt.org/#/c/53820/43/tests/vmTests.py
File tests/vmTests.py:

PS43, Line 108: 'vmType': 'kvm'
This is unrelated change and it lowers my trust for BC. I would prefer this 
code unchanged in this patch, and a test added that verifies 'vmType': 'kvm' is 
valid in is_kvm.


https://gerrit.ovirt.org/#/c/53820/43/vdsm/caps.py
File vdsm/caps.py:

PS43, Line 205: containers
Does this really refer to 'containers'? Wouldn't keeping the name 
'container_runtimes' be slightly more sane?


https://gerrit.ovirt.org/#/c/53820/43/vdsm/supervdsm_api/containers.py
File vdsm/supervdsm_api/containers.py:

PS43, Line 31: # actions:
 : #if ! [ -d "${container_path}" ]; then
 : #/usr/bin/mkdir -p "${container_path}" > /dev/null 2>&1
 : #fi
 : #"/usr/bin/chown" vdsm:qemu "${container_path}"
 : #"/usr/bin/chmod" 0775 "${container_path}" > /dev/null 2>&1
 : ## TODO: selinux label
 : ## "/usr/bin/chcon" "system_u:object_r:qemu_var_run_t:s0"
Care to explain the existence of the comment?


https://gerrit.ovirt.org/#/c/53820/43/vdsm/supervdsm_api/rkt.py
File vdsm/supervdsm_api/rkt.py:

PS43, Line 34: pod
I suppose this is term used throughout rkt - is it possible to have a more 
explanatory name / add some quick read rkt terminology?


https://gerrit.ovirt.org/#/c/53820/43/vdsm/supervdsm_api/systemd.py
File vdsm/supervdsm_api/systemd.py:

Line 35: "/bin/machinectl",
Line 36: "/usr/bin/machinectl",
Line 37: )
Line 38: 
Line 39: 
This is unrelated and should've been done in previous patch.
Line 40: @expose
Line 41: def systemd_run(unit_name, cgroup_slice, *args):
Line 42: return commands.execCmd(
Line 43: cmdutils.systemd_run(


https://gerrit.ovirt.org/#/c/53820/43/vdsm/vdsm
File vdsm/vdsm:

PS43, Line 92: containersconnection.start_event_loop()
This is dummy function, is that right?


https://gerrit.ovirt.org/#/c/53820/43/vdsm/virt/vm.py
File vdsm/virt/vm.py:

PS43, Line 1654: if is_kvm(self.conf):
I have a feeling that this block justifies thoughts of domDependentInit 
duplicate.


-- 
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: 43
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 43:

* 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: 43
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 42:

* 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: 42
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 41:

* 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: 41
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 40:

* 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: 40
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[fromani]

Francesco Romani has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 39:

(1 comment)

https://gerrit.ovirt.org/#/c/53820/39/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 1830: self.log.error("Failed to make a agent channel 
symlink "
Line 1831:"from %s -> %s for channel %s", 
path,
Line 1832:uuidPath, name)
Line 1833: 
Line 1834: def _domDependentInit(self):
alternative approach: dup this method, like I did in 
https://gerrit.ovirt.org/#/c/54179/24/vdsm/virt/vm.py
Line 1835: if self._destroy_requested.is_set():
Line 1836: # reaching here means that Vm.destroy() was called 
before we could
Line 1837: # handle it. We must handle it now
Line 1838: try:


-- 
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: 39
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 39:

* 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: 39
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 38:

* 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: 38
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 37:

* 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: 37
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 36:

* 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: 36
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 35:

* 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: 35
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 34:

* 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: 34
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 33:

* 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: 33
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


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/53820
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id236a30a5c875994c037b8d00c7463bceaab143f
Gerrit-PatchSet: 32
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 31:

* 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: 31
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 30:

* 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: 30
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: vdsm: virt: add optional container support

[automation]

gerrit-hooks has posted comments on this change.

Change subject: vdsm: virt: add optional container support
..


Patch Set 29:

* 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: 29
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org