Change in vdsm[master]: virt: tolerate missing ovirt-vmconsole group

Francesco Romani has uploaded a new change for review.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

virt: tolerate missing  ovirt-vmconsole group

To support the virtio serial console feature,
VDSM must bind the virtio channel to an unix
domain socket, and fix its permissions and
group in order to let ovirt-vmconsole-host
connect to that socket.

ovirt-vmconsole-host it is a soft dependency.
VDSM should happily run without that package,
but incorrectly assumed the ovirt-vmconsole
group existed in the system

This patch lift this requirement:
VDSM still try to fix the group of the socket,
but if the group doesn't exist, it happily
goes ahead.

Change-Id: I55b708953e5871053f0063bb82434c5c0dcb6103
Related-To: https://bugzilla.redhat.com/show_bug.cgi?id=1223671
Signed-off-by: Francesco Romani 
---
M vdsm/virt/vmdevices/core.py
1 file changed, 8 insertions(+), 3 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/51/42451/1

diff --git a/vdsm/virt/vmdevices/core.py b/vdsm/virt/vmdevices/core.py
index a847ce0..4d56005 100644
--- a/vdsm/virt/vmdevices/core.py
+++ b/vdsm/virt/vmdevices/core.py
@@ -99,9 +99,14 @@

def prepare(self):
         if self._path:
-            supervdsm.getProxy().prepareVmChannel(
-                self._path,
-                constants.OVIRT_VMCONSOLE_GROUP)
+            try:
+                supervdsm.getProxy().prepareVmChannel(
+                    self._path,
+                    constants.OVIRT_VMCONSOLE_GROUP)
+            except KeyError:
+                self.log.warn('unknown group %s, permissions not updated',
+                              constants.OVIRT_VMCONSOLE_GROUP)
+

def cleanup(self):
         if self._path:

[e-mail] has posted comments on this change.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

Patch Set 1:

* 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.5', 'ovirt-3.4', 'ovirt-3.3'])

Francesco Romani has posted comments on this change.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

Patch Set 1: Verified+1

without this patch:
VM startup triggers getgrnam() with fails, and KeyError is raised.
With this patch: warning in the logs, socket bound but with "wrong" group, VM boots.

Alternative option: just add dependency on ovirt-vmconsole-host package, which owns the ovirt-vmconsole user/group.

[e-mail] has posted comments on this change.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

Patch Set 2:

* 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.5', 'ovirt-3.4', 'ovirt-3.3'])

Francesco Romani has posted comments on this change.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

Patch Set 2: Verified+1

V2 fixes a pep8 violation, copied score.

Francesco Romani has posted comments on this change.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

Patch Set 2:

the package we could depend on is ovirt-vmconsole, which owns user, group, and provides common utilities for -proxy and -host. It doesn't start any service.

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://gerrit.ovirt.org/#/c/42451/2/vdsm/virt/vmdevices/core.py
File vdsm/virt/vmdevices/core.py:

Line 102:             try:
Line 103:                 supervdsm.getProxy().prepareVmChannel(
Line 104:                     self._path,
Line 105:                     constants.OVIRT_VMCONSOLE_GROUP)
Line 106:             except KeyError:
KeyError is pretty bad error to check here, but this is stdlib fault.

We should fix this later in fileUtils, raise an error that cannot mask a bug in the calling code.
Line 107:                 self.log.warn('unknown group %s, permissions not updated',
Line 108:                               constants.OVIRT_VMCONSOLE_GROUP)
Line 109: 
Line 110:     def cleanup(self):

[e-mail] has posted comments on this change.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

Patch Set 3:

* 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.5', 'ovirt-3.4', 'ovirt-3.3'])

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

Patch Set 3: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/42451/3//COMMIT_MSG
Commit Message:

Line 12: group in order to let ovirt-vmconsole-host
Line 13: connect to that socket.
Line 14: 
Line 15: ovirt-vmconsole-host it is a soft dependency.
Line 16: VDSM should happily run without that package,
...however, Vdsm must not happily start a VM where this new feature was explicitly requested by Engine, but Vdsm cannot deliver.
Line 17: but incorrectly assumed the ovirt-vmconsole
Line 18: group existed in the system
Line 19: 
Line 20: This patch lift this requirement:

[e-mail] has posted comments on this change.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

Patch Set 4:

* 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.5', 'ovirt-3.4', 'ovirt-3.3'])

Francesco Romani has posted comments on this change.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/42451/3//COMMIT_MSG
Commit Message:

Line 12: group in order to let ovirt-vmconsole-host
Line 13: connect to that socket.
Line 14: 
Line 15: ovirt-vmconsole-host it is a soft dependency.
Line 16: VDSM should happily run without that package,
> ...however, Vdsm must not happily start a VM where this new feature was exp

Right, than the real fix is engine-side. Will abandon this patch (set) once Engine fix is in
Line 17: but incorrectly assumed the ovirt-vmconsole
Line 18: group existed in the system
Line 19: 
Line 20: This patch lift this requirement:

[e-mail] has posted comments on this change.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

Patch Set 5:

* 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.5', 'ovirt-3.4', 'ovirt-3.3'])

Francesco Romani has abandoned this change.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

Abandoned

VDSM should not take initiatives. Alternative approaches: 42479, 42542

[e-mail] has posted comments on this change.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

Patch Set 5:

* Update tracker::IGNORE, no Bug-Url found

Francesco Romani has restored this change.

Change subject: virt: tolerate missing  ovirt-vmconsole group
......................................................................

Restored

Change in vdsm[master]: virt: abort if missing ovirt-vmconsole group

[e-mail] has posted comments on this change.

Change subject: virt: abort if missing  ovirt-vmconsole group
......................................................................

Patch Set 6:

* 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.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: virt: abort if missing ovirt-vmconsole group

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: virt: abort if missing  ovirt-vmconsole group
......................................................................

Patch Set 6: Code-Review+2

Change in vdsm[master]: virt: abort if missing ovirt-vmconsole group

Francesco Romani has posted comments on this change.

Change subject: virt: abort if missing  ovirt-vmconsole group
......................................................................

Patch Set 7: Verified+1

Works.
- run patched VDSM.
- renamed ovirt-vmconsole group (same as deleting it, I was lazy).
- tricked VDSM to always start new console
- booted VM with console device enabled
- VM boot fails:

Thread-122::DEBUG::2015-06-22 15:26:29,484::vmchannels::203::vds::(register) Add fileno 28 to listener's channels.
Thread-122::WARNING::2015-06-22 15:26:29,485::core::122::vm.Vm::(prepare) vmId=`995a5f50-9f77-479d-9f05-acc00965dd3e`::unknown group ovirt-vmconsole, cannot create console
Thread-122::INFO::2015-06-22 15:26:29,492::vm::1294::vm.Vm::(setDownStatus) vmId=`995a5f50-9f77-479d-9f05-acc00965dd3e`::Changed state to Down: VM cannot run on this host due to unsupported device (code=10)

later:

Thread-131::DEBUG::2015-06-22 15:26:41,129::__init__::496::jsonrpc.JsonRpcServer::(_serveRequest) Calling 'VM.destroy' in bridge with {u'vmID': u'995a5f50-9f77-479d-9f05-acc00965dd3e'}
JsonRpcServer::DEBUG::2015-06-22 15:26:41,129::__init__::533::jsonrpc.JsonRpcServer::(serve_requests) Waiting for request
Thread-131::INFO::2015-06-22 15:26:41,129::API::335::vds::(destroy) vmContainerLock acquired by vm 995a5f50-9f77-479d-9f05-acc00965dd3e
Thread-131::DEBUG::2015-06-22 15:26:41,129::vm::3684::vm.Vm::(destroy) vmId=`995a5f50-9f77-479d-9f05-acc00965dd3e`::destroy Called
Thread-131::INFO::2015-06-22 15:26:41,130::vm::3616::vm.Vm::(releaseVm) vmId=`995a5f50-9f77-479d-9f05-acc00965dd3e`::Release VM resources
Thread-131::WARNING::2015-06-22 15:26:41,130::vm::372::vm.Vm::(_set_lastStatus) vmId=`995a5f50-9f77-479d-9f05-acc00965dd3e`::trying to set state to Powering down when already Down
Thread-131::DEBUG::2015-06-22 15:26:41,627::task::595::Storage.TaskManager.Task::(_updateState) Task=`6950b9af-bf28-4cdd-9dc5-f0f0867968d4`::moving from state init -> state preparing
libvirtEventLoop::DEBUG::2015-06-22 15:26:41,627::vm::4344::vm.Vm::(_onLibvirtLifecycleEvent) vmId=`995a5f50-9f77-479d-9f05-acc00965dd3e`::event Stopped detail 1 opaque None
Thread-131::INFO::2015-06-22 15:26:41,627::logUtils::48::dispatcher::(wrapper) Run and protect: teardownImage(sdUUID=u'12f68692-2a5a-4e48-af5e-4679bca7fd44', spUUID=u'00000002-0002-0002-0002-00000000014b', imgUUID=u'ee1295ee-7ddc-4030-be5e-4557538bc4d2', volUUID=None)
libvirtEventLoop::INFO::2015-06-22 15:26:41,627::vm::815::vm.Vm::(_onQemuDeath) vmId=`995a5f50-9f77-479d-9f05-acc00965dd3e`::underlying process disconnected

also:

BENji> 15:26:36 root [~]$ vdsClient -s 0 getAllVmStats

995a5f50-9f77-479d-9f05-acc00965dd3e
    Status = Down
    exitMessage = VM cannot run on this host due to unsupported device
    exitReason = 10
    timeOffset = 0
    statusTime = 4296290010
    exitCode = 1

also verified with virsh -r list that no leftovers remains.

Change in vdsm[master]: virt: abort if missing ovirt-vmconsole group

Francesco Romani has posted comments on this change.

Change subject: virt: abort if missing  ovirt-vmconsole group
......................................................................

Patch Set 7: Code-Review-1

We discussed the vmconsole upgrade path inside the virt group, so
please let's wait for Michal's ACK here.

Change in vdsm[master]: virt: abort if missing ovirt-vmconsole group

[e-mail] has posted comments on this change.

Change subject: virt: abort if missing  ovirt-vmconsole group
......................................................................

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.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: virt: abort if missing ovirt-vmconsole group

[e-mail] has posted comments on this change.

Change subject: virt: abort if missing  ovirt-vmconsole group
......................................................................

Patch Set 8:

* 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.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: virt: abort if missing ovirt-vmconsole group

Michal Skrivanek has posted comments on this change.

Change subject: virt: abort if missing  ovirt-vmconsole group
......................................................................

Patch Set 8: Code-Review+1

Change in vdsm[master]: virt: abort if missing ovirt-vmconsole group

Francesco Romani has posted comments on this change.

Change subject: virt: abort if missing  ovirt-vmconsole group
......................................................................

Patch Set 8: -Code-Review

Change in vdsm[master]: virt: abort if missing ovirt-vmconsole group

[e-mail] has posted comments on this change.

Change subject: virt: abort if missing  ovirt-vmconsole group
......................................................................

Patch Set 9:

* 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.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: virt: abort if missing ovirt-vmconsole group

Francesco Romani has posted comments on this change.

Change subject: virt: abort if missing  ovirt-vmconsole group
......................................................................

Patch Set 9: Verified+1

forgot to document the new vmexitreason in schema. Now fixed.
re-verified with make-check the new tiny change since last upload.

Change in vdsm[master]: virt: abort if missing ovirt-vmconsole group

Michal Skrivanek has posted comments on this change.

Change subject: virt: abort if missing  ovirt-vmconsole group
......................................................................

Patch Set 9: Code-Review-1

I would favor solution in 42479. Then this is not needed

Change in vdsm[master]: virt: abort if missing ovirt-vmconsole group

Francesco Romani has abandoned this change.

Change subject: virt: abort if missing  ovirt-vmconsole group
......................................................................

Abandoned

Change in vdsm[master]: virt: abort if missing ovirt-vmconsole group

[e-mail] has posted comments on this change.

Change subject: virt: abort if missing  ovirt-vmconsole group
......................................................................

Patch Set 9:

* Update tracker::IGNORE, no Bug-Url found