Change in vdsm[ovirt-3.2]: Don't user sudo in fuser

[Federico Simoncelli]

Hello Saggi Mizrahi, Antoni Segura Puimedon, Dan Kenigsberg,

I'd like you to do a code review.  Please visit

    http://gerrit.ovirt.org/13810

to review the following change.

Change subject: Don't user sudo in fuser
......................................................................

Don't user sudo in fuser

Change-Id: I74842ffda1bc1d5ebb54de5e1933fedd851e745b
Signed-off-by: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Reviewed-on: http://gerrit.ovirt.org/13779
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt;
Tested-by: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
---
M vdsm/storage/blockSD.py
M vdsm/storage/fuser.py
M vdsm/sudoers.vdsm.in
M vdsm/supervdsmServer.py
4 files changed, 9 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/10/13810/1

diff --git a/vdsm/storage/blockSD.py b/vdsm/storage/blockSD.py
index e66256d..e22a1e2 100644
--- a/vdsm/storage/blockSD.py
+++ b/vdsm/storage/blockSD.py
@@ -48,7 +48,7 @@
 from storage_mailbox import MAILBOX_SIZE
 import resourceManager as rm
 import mount
-from fuser import fuser
+import supervdsm as svdsm
 
 STORAGE_DOMAIN_TAG = "RHAT_storage_domain"
 STORAGE_UNREADY_DOMAIN_TAG = STORAGE_DOMAIN_TAG + "_UNREADY"
@@ -1122,7 +1122,8 @@
                 masterMount.umount()
             except mount.MountError:
                 # umount failed, try to kill that processes holding mount point
-                pids = fuser(masterMount.fs_file, mountPoint=True)
+                svdsmp = svdsm.getProxy()
+                pids = svdsmp.fuser(masterMount.fs_file, mountPoint=True)
 
                 # It was unmounted while I was checking no need to do anything
                 if not masterMount.isMounted():
diff --git a/vdsm/storage/fuser.py b/vdsm/storage/fuser.py
index 135dad6..0f235ee 100644
--- a/vdsm/storage/fuser.py
+++ b/vdsm/storage/fuser.py
@@ -27,7 +27,7 @@
         cmd.append("-m")
 
     cmd.append(path)
-    (rc, out, err) = misc.execCmd(cmd, raw=True, sudo=True)
+    (rc, out, err) = misc.execCmd(cmd, raw=True, sudo=False)
     if rc != 0:
         return []
 
diff --git a/vdsm/sudoers.vdsm.in b/vdsm/sudoers.vdsm.in
index 4fc75f9..70628f8 100644
--- a/vdsm/sudoers.vdsm.in
+++ b/vdsm/sudoers.vdsm.in
@@ -12,7 +12,6 @@
     @FSCK_PATH@ -p *, \
     @TUNE2FS_PATH@ -j *, \
     @MKFS_PATH@ -q -j *, \
-    @FUSER_PATH@ -m *, \
     @KILL_PATH@, \
     @CHOWN_PATH@ @VDSMUSER@\:@QEMUGROUP@ *, \
     @CHOWN_PATH@ @METADATAUSER@\:@METADATAGROUP@ *, \
diff --git a/vdsm/supervdsmServer.py b/vdsm/supervdsmServer.py
index 21e7c94..3f7ce1c 100755
--- a/vdsm/supervdsmServer.py
+++ b/vdsm/supervdsmServer.py
@@ -26,6 +26,7 @@
 import errno
 import threading
 import re
+import fuser
 from time import sleep
 import signal
 from multiprocessing import Pipe, Process
@@ -307,6 +308,10 @@
     def removeFs(self, path):
         return mkimage.removeFs(path)
 
+    @logDecorator
+    def fuser(self, *args):
+        return fuser.fuser(*args)
+
     def __udevReloadRules(self, guid):
         if self.__udevOperationReload():
             reload = "--reload"


--
To view, visit http://gerrit.ovirt.org/13810
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I74842ffda1bc1d5ebb54de5e1933fedd851e745b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.2
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

[Federico Simoncelli]

Federico Simoncelli has submitted this change and it was merged.

Change subject: Don't user sudo in fuser
......................................................................


Don't user sudo in fuser

Change-Id: I74842ffda1bc1d5ebb54de5e1933fedd851e745b
Signed-off-by: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Reviewed-on: http://gerrit.ovirt.org/13779
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt;
Tested-by: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
---
M vdsm/storage/blockSD.py
M vdsm/storage/fuser.py
M vdsm/sudoers.vdsm.in
M vdsm/supervdsmServer.py
4 files changed, 9 insertions(+), 4 deletions(-)

Approvals:
  Federico Simoncelli: Verified; Looks good to me, approved
  Dan Kenigsberg: Looks good to me, but someone else must approve


--
To view, visit http://gerrit.ovirt.org/13810
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I74842ffda1bc1d5ebb54de5e1933fedd851e745b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.2
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Don't user sudo in fuser
......................................................................


Patch Set 1: Verified; Looks good to me, approved

Same as master.

--
To view, visit http://gerrit.ovirt.org/13810
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I74842ffda1bc1d5ebb54de5e1933fedd851e745b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.2
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Antoni Segura Puimedon has posted comments on this change.

Change subject: Don't user sudo in fuser
......................................................................


Patch Set 1: No score

--
To view, visit http://gerrit.ovirt.org/13810
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I74842ffda1bc1d5ebb54de5e1933fedd851e745b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.2
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Don't user sudo in fuser
......................................................................


Patch Set 1: Looks good to me, but someone else must approve

yeah, without this patch, the branch unit-tests are broken on a vanilla host.

--
To view, visit http://gerrit.ovirt.org/13810
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I74842ffda1bc1d5ebb54de5e1933fedd851e745b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.2
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Antoni Segura Puimedon has posted comments on this change.

Change subject: Don't user sudo in fuser
......................................................................


Patch Set 1: Fails

For F19 this is not enough, as the following fails:
======================================================================
ERROR: testThreadLeak (storageMailboxTests.SPM_MailMonitorTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/celebdor/rpmbuild/BUILD/vdsm-4.10.3/tests/storageMailboxTests.py",
line 53, in testThreadLeak
    mailer = sm.SPM_MailMonitor(StoragePoolStub(), 100)
  File
"/home/celebdor/rpmbuild/BUILD/vdsm-4.10.3/vdsm/storage/storage_mailbox.py",
line 495, in __init__
    (rc, out, err) = misc.execCmd(cmd, sudo=False, data=self._outgoingMail)
  File "/home/celebdor/rpmbuild/BUILD/vdsm-4.10.3/vdsm/storage/misc.py", line
208, in execCmd
    (out, err) = p.communicate(data)
  File "/home/celebdor/rpmbuild/BUILD/vdsm-4.10.3/vdsm/storage/misc.py", line
960, in communicate
    self.stdin.write(data)
  File "/home/celebdor/rpmbuild/BUILD/vdsm-4.10.3/vdsm/storage/misc.py", line
838, in write
    self._parent._processStreams()
  File "/home/celebdor/rpmbuild/BUILD/vdsm-4.10.3/vdsm/storage/misc.py", line
902, in _processStreams
    written = os.write(fd, buff)
OSError: [Errno 32] Broken pipe
-------------------- >> begin captured logging << --------------------
Misc.ThreadPool: DEBUG: Enter - numThreads: 5.0, waitTimeout: 3, maxTasks: 500.0
Storage.MailBox.SpmMailMonitor: DEBUG: SPM_MailMonitor - clearing outgoing mail, command
is: ['dd',
'of=/tmp/tmpVOZj5Z/d34ed30b-9481-4b53-8a9d-19070fb99fe9/mastersd/dom_md/outbox',
'oflag=direct', 'iflag=fullblock', 'conv=notrunc',
'count=1']
Storage.Misc.excCmd: DEBUG: 'dd
of=/tmp/tmpVOZj5Z/d34ed30b-9481-4b53-8a9d-19070fb99fe9/mastersd/dom_md/outbox oflag=direct
iflag=fullblock conv=notrunc count=1 bs=409600' (cwd None)
--------------------- >> end captured logging << ---------------------


I believe the patches that fixed that should maybe be backported as well, and this rebased
on top (or those rebased on top of this). V-1 for visibility.

--
To view, visit http://gerrit.ovirt.org/13810
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I74842ffda1bc1d5ebb54de5e1933fedd851e745b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.2
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;