Change in vdsm[master]: Adding remove\disable verbs to vdsm-tool for admin usages

Yaniv Bronhaim has uploaded a new change for review.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................

Adding remove\disable verbs to vdsm-tool for admin usages

The spec will be modified separately to use vdsm-tool instead of hard-coded
operations

Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Signed-off-by: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
---
M lib/vdsm/constants.py.in
M lib/vdsm/tool/configurator.py
M lib/vdsm/tool/passwd.py
M lib/vdsm/tool/seboolsetup.py
4 files changed, 118 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/72/21772/1

diff --git a/lib/vdsm/constants.py.in b/lib/vdsm/constants.py.in
index 790a3a0..43e13da 100644
--- a/lib/vdsm/constants.py.in
+++ b/lib/vdsm/constants.py.in
@@ -80,6 +80,15 @@
 P_VDSM_EXEC = '@LIBEXECDIR@'
 
 #
+# Configuration file definitions
+#
+SYSCONF_PATH = '@sysconfdir@'
+P_SYSTEMCTL_CONF = SYSCONF_PATH + '/sysctl.d/vdsm'
+P_VDSM_LCONF = SYSCONF_PATH + '/libvirt/libvirtd.conf'
+P_VDSM_LDCONF = SYSCONF_PATH + '/sysconfig/libvirtd'
+P_VDSM_QCONF = SYSCONF_PATH + '/libvirt/qemu.conf'
+
+#
 # External programs (sorted, please keep in order).
 #
 EXT_BLKID = '@BLKID_PATH@'
diff --git a/lib/vdsm/tool/configurator.py b/lib/vdsm/tool/configurator.py
index 51eda80..a30827a 100644
--- a/lib/vdsm/tool/configurator.py
+++ b/lib/vdsm/tool/configurator.py
@@ -21,11 +21,13 @@
 import sys
 import grp
 import argparse
+import tempfile
+import shutil
 
 from .. import utils
 from . import service, expose
-from ..constants import P_VDSM_EXEC, DISKIMAGE_GROUP
-from ..constants import QEMU_PROCESS_GROUP, VDSM_GROUP
+from ..constants import P_VDSM_EXEC, DISKIMAGE_GROUP, QEMU_PROCESS_GROUP, \
+    VDSM_GROUP, P_VDSM_LCONF, P_VDSM_QCONF, P_VDSM_LDCONF, P_SYSTEMCTL_CONF
 
 
 class _ModuleConfigure(object):
@@ -47,6 +49,35 @@
     def isconfigured(self):
         return True
 
+    def removeConf(self, filename, beginField=None, endField=None):
+        newfile = []
+        if not beginField and not endField:
+            return
+        else:
+            with open(filename, 'r') as f:
+                skip = False
+                for line in f.readlines():
+                    if beginField and endField:
+                        if skip:
+                            if line.startswith(endField):
+                                skip = False
+                            continue
+                        if line.startswith(beginField):
+                            skip = True
+                            continue
+                    elif beginField:
+                        if line.startswith(beginField):
+                            continue
+                    else:
+                        if line.endswith('%s\n' % endField):
+                            continue
+                    newfile.append(line)
+            tmp_file = tempfile.NamedTemporaryFile(delete=False)
+            tname = tmp_file.name
+            tmp_file.writelines(newfile)
+            tmp_file.close()
+            shutil.move(tname, filename)
+
 
 class LibvirtModuleConfigure(_ModuleConfigure):
     def __init__(self):
@@ -57,6 +88,28 @@
 
     def getServices(self):
         return ["supervdsmd", "vdsmd", "libvirtd"]
+
+    def removeConf(self):
+        vdsm_ver = '-4.9.10'
+        conf_prefix = '## beginning of configuration section by vdsm' + \
+                      vdsm_ver
+        conf_suffix = '## end of configuration section by vdsm' + vdsm_ver
+
+        conf_paths = [
+            P_VDSM_LCONF,
+            P_VDSM_QCONF,
+            P_VDSM_LDCONF,
+        ]
+        for path in conf_paths:
+            try:
+                super(LibvirtModuleConfigure, self).removeConf(path,
+                                                               conf_prefix,
+                                                               conf_suffix)
+            except OSError, e:
+                if e.errno != os.errno.EEXIST:
+                    raise
+
+        utils.rmFile(P_SYSTEMCTL_CONF)
 
     def _exec_libvirt_configure(self, action):
         """
@@ -113,6 +166,9 @@
 
     def getServices(self):
         return ['sanlock']
+
+    def removeConf(self):
+        pass
 
     def configure(self):
         """
@@ -257,6 +313,25 @@
         raise RuntimeError("Config is not valid. Check conf files")
 
 
+@expose("remove-config")
+def remove_config(*args):
+    """
+    Remove vdsm configuration from conf files
+    """
+    args = _parse_args('remove-config')
+    m = [
+        c.getName() for c in __configurers
+        if c.getName() in args.modules and not c.removeConf()
+    ]
+    if m:
+        sys.stdout.write(
+            "Could not remove configuration for modules %s\n" %
','.join(m),
+        )
+        ret = False
+    if not ret:
+        raise RuntimeError("Remove configuration failed")
+
+
 def _parse_args(action):
     parser = argparse.ArgumentParser('vdsm-tool %s' % (action))
     allModules = [n.getName() for n in __configurers]
diff --git a/lib/vdsm/tool/passwd.py b/lib/vdsm/tool/passwd.py
index 0b127c8..4e40948 100644
--- a/lib/vdsm/tool/passwd.py
+++ b/lib/vdsm/tool/passwd.py
@@ -43,4 +43,17 @@
         stderr=subprocess.PIPE, close_fds=True)
     output, err = p.communicate()
     if p.returncode != 0:
-        raise Exception("Set password failed: %s" % (err, ))
+        raise RuntimeError("Set password failed: %s" % (err, ))
+
+
+@expose("remove-saslpasswd")
+def remove_saslpasswd():
+    """
+    Remove vdsm password for libvirt connection
+    """
+    script = ['/usr/sbin/saslpasswd2', '-p', '-a',
'libvirt', '-d',
+              constants.SASL_USERNAME]
+    p = subprocess.Popen(script)
+    output, err = p.communicate()
+    if p.returncode != 0:
+        raise RuntimeError("Remove password failed: %s" % (err, ))
diff --git a/lib/vdsm/tool/seboolsetup.py b/lib/vdsm/tool/seboolsetup.py
index f664ecd..7c1effa 100644
--- a/lib/vdsm/tool/seboolsetup.py
+++ b/lib/vdsm/tool/seboolsetup.py
@@ -18,6 +18,7 @@
 # Refer to the README and COPYING files for full details of the license
 #
 
+import selinux
 from . import expose
 
 SEBOOL_ENABLED = "on"
@@ -63,3 +64,20 @@
 def sebool_unconfig():
     """Disable the required selinux booleans"""
     setup_booleans(False)
+
+
+@expose("clear-selinux-policy")
+def clear_selinux_policy():
+    """
+    Clear the changes of selinux policy
+    """
+    selinux_policys = [
+        'virt_use_nfs',
+        'virt_use_sanlock',
+        'sanlock_use_nfs',
+    ]
+
+    if selinux.is_selinux_enabled():
+        for policy in selinux_policys:
+            selinux.security_set_boolean(policy, 0)
+            selinux.security_commit_booleans()


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 1:

(7 comments)

....................................................
File lib/vdsm/tool/configurator.py
Line 48: 
Line 49:     def isconfigured(self):
Line 50:         return True
Line 51: 
Line 52:     def removeConf(self, filename, beginField=None, endField=None):
this is not removeConf in inheritance model, it is utility, the removeConf in interface
should do nothing, while implementations can call a utility function.
Line 53:         newfile = []
Line 54:         if not beginField and not endField:
Line 55:             return
Line 56:         else:


Line 52:     def removeConf(self, filename, beginField=None, endField=None):
Line 53:         newfile = []
Line 54:         if not beginField and not endField:
Line 55:             return
Line 56:         else:
so why else?

won't it better to just net bellow and have positive condition?
Line 57:             with open(filename, 'r') as f:
Line 58:                 skip = False
Line 59:                 for line in f.readlines():
Line 60:                     if beginField and endField:


Line 70:                             continue
Line 71:                     else:
Line 72:                         if line.endswith('%s\n' % endField):
Line 73:                             continue
Line 74:                     newfile.append(line)
we are talked about this structure... it is spaghetti!

you need to use state machine like structure.

 (
     BEFORE,
     WITHIN,
     AFTER,
 ) = range(3)
 newcontent = []
 state = BEFORE if beginField else WITHIN
 with open(filename, 'r') as f:
     content = f.readlines()
 for line in content:
     if state == BEFORE:
         if line.startswith(beginField):
             state = WITHIN
         else:
             newcontent.append(line)
     elif state == WITHIN:
         if endField and line.startswith(endField):
             state = AFTER
     elif state == AFTER:
         newcontent.append(line)
     else:
         raise RuntimeError("Internal error")
 if content != newcontent:
    # write temp, move
Line 75:             tmp_file = tempfile.NamedTemporaryFile(delete=False)
Line 76:             tname = tmp_file.name
Line 77:             tmp_file.writelines(newfile)
Line 78:             tmp_file.close()


Line 103:         for path in conf_paths:
Line 104:             try:
Line 105:                 super(LibvirtModuleConfigure, self).removeConf(path,
Line 106:                                                                conf_prefix,
Line 107:                                                                conf_suffix)
please avoid drawing code...

 super(xxx, self).removeConf(
    p1,
    p2,
 )

so that if name is modified you do not need to revisit the entire indentation.

saying that, and using super for this is completely wrong.
Line 108:             except OSError, e:
Line 109:                 if e.errno != os.errno.EEXIST:
Line 110:                     raise
Line 111: 


Line 106:                                                                conf_prefix,
Line 107:                                                                conf_suffix)
Line 108:             except OSError, e:
Line 109:                 if e.errno != os.errno.EEXIST:
Line 110:                     raise
you should first check if file exist then attempt to modify, do not assume that good
follow has exceptions.
Line 111: 
Line 112:         utils.rmFile(P_SYSTEMCTL_CONF)
Line 113: 
Line 114:     def _exec_libvirt_configure(self, action):


....................................................
File lib/vdsm/tool/passwd.py
Line 52:     Remove vdsm password for libvirt connection
Line 53:     """
Line 54:     script = ['/usr/sbin/saslpasswd2', '-p', '-a',
'libvirt', '-d',
Line 55:               constants.SASL_USERNAME]
Line 56:     p = subprocess.Popen(script)
why do you need this script temp variable? why isn't it tuple?
Line 57:     output, err = p.communicate()
Line 58:     if p.returncode != 0:


....................................................
File lib/vdsm/tool/seboolsetup.py
Line 70: def clear_selinux_policy():
Line 71:     """
Line 72:     Clear the changes of selinux policy
Line 73:     """
Line 74:     selinux_policys = [
why do you need this temp variable? why isn't it tuple?
Line 75:         'virt_use_nfs',
Line 76:         'virt_use_sanlock',
Line 77:         'sanlock_use_nfs',
Line 78:     ]


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 1: Verified-1

Build Failed 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4912/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5712/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5801/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 1:

(2 comments)

....................................................
File lib/vdsm/tool/configurator.py
Line 48: 
Line 49:     def isconfigured(self):
Line 50:         return True
Line 51: 
Line 52:     def removeConf(self, filename, beginField=None, endField=None):
I can you use "sed -i" explictly if it's simpler to understand. let me know
Line 53:         newfile = []
Line 54:         if not beginField and not endField:
Line 55:             return
Line 56:         else:


....................................................
File lib/vdsm/tool/seboolsetup.py
Line 65:     """Disable the required selinux booleans"""
Line 66:     setup_booleans(False)
Line 67: 
Line 68: 
Line 69: @expose("clear-selinux-policy")
sebool_unconfig does the same. I'll remove that
Line 70: def clear_selinux_policy():
Line 71:     """
Line 72:     Clear the changes of selinux policy
Line 73:     """


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-HasComments: Yes

Yaniv Bronhaim has abandoned this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Abandoned

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 16: Code-Review-1

fix to my previous comment: the configurator.py changes can be part of
http://gerrit.ovirt.org/#/c/27130/2 and the passwd.py can stay here.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 16:

the configurator.py changes can be part of http://gerrit.ovirt.org/#/c/27130/2 and the
calls to this verb can be separately

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 16:

Build Successful 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/8406/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/7616/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/8526/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 15:

(2 comments)

http://gerrit.ovirt.org/#/c/21772/15/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 53:     Remove vdsm password for libvirt connection
Line 54:     """
Line 55:     rc, out, err = utils.execCmd(
Line 56:         (constants.EXT_SASLPASSWD2, '-p', '-a',
'libvirt',
Line 57:          '-d', constants.SASL_USERNAME,),
>  please consider to indent: 
Yes it is better.
Line 58:     )
Line 59:     if rc != 0:


Line 56:         (constants.EXT_SASLPASSWD2, '-p', '-a',
'libvirt',
Line 57:          '-d', constants.SASL_USERNAME,),
Line 58:     )
Line 59:     if rc != 0:
Line 60:         raise RuntimeError("Remove password failed: %s" % (err,))
>  % err is sufficient. 
I believe vdsm style usually uses tuple in
such cases in
 preparation to other variables.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 15:

(2 comments)

why don't you merge this? what should it vest in gerrit?

http://gerrit.ovirt.org/#/c/21772/15/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 53:     Remove vdsm password for libvirt connection
Line 54:     """
Line 55:     rc, out, err = utils.execCmd(
Line 56:         (constants.EXT_SASLPASSWD2, '-p', '-a',
'libvirt',
Line 57:          '-d', constants.SASL_USERNAME,),
please consider to indent:

 rc, out, err = utils.execCmd(
     (
         constants.EXT_SASLPASSWD2,
         '-p',
         '-a', 'libvirt',
         '-d', constants.SASL_USERNAME,
     ),
 )
Line 58:     )
Line 59:     if rc != 0:


Line 56:         (constants.EXT_SASLPASSWD2, '-p', '-a',
'libvirt',
Line 57:          '-d', constants.SASL_USERNAME,),
Line 58:     )
Line 59:     if rc != 0:
Line 60:         raise RuntimeError("Remove password failed: %s" % (err,))
% err is sufficient.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 15:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/8349/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/7559/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/8469/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 14: Code-Review-1 Verified-1

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/8216/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/8329/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/7426/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 13: Code-Review-1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 13:

Build Successful 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6751/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7541/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7651/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 12:

Build Successful 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6750/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7540/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7650/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

mooli tayer has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 2:

(6 comments)

I would like to write a unit test, at least for 'removeSectionFromFile'
Where do such a test belong?
Also if I want to test on a file should that file be a resource or
Should I write it on demand?

http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 53:             newcontent.append(line)
Line 54:         else:
Line 55:             raise RuntimeError("Internal Error")
Line 56:     if content != newcontent:
Line 57:         tmpfile = tempfile.NamedTemporaryFile(delete=False)
>  you need to create temp file at same directory of original file...
> see temp 
Done
Line 58:         tname = tmpfile.name
Line 59:         tmpfile.writelines(newcontent)
Line 60:         tmpfile.close()
Line 61:         shutil.move(tname, filename)


Line 56:     if content != newcontent:
Line 57:         tmpfile = tempfile.NamedTemporaryFile(delete=False)
Line 58:         tname = tmpfile.name
Line 59:         tmpfile.writelines(newcontent)
Line 60:         tmpfile.close()
>  you should never use close() but python with structure. 
Done
Line 61:         shutil.move(tname, filename)
Line 62: 
Line 63: 
Line 64: class _ModuleConfigure(object):


Line 147:         conf_prefix = '## beginning of configuration section by vdsm' +
\
Line 148:                       vdsm_ver
Line 149:         conf_suffix = '## end of configuration section by vdsm' +
vdsm_ver
Line 150: 
Line 151:         conf_paths = [
>  consider remove temporary one time used variable. 
Done
Line 152:             P_VDSM_LCONF,
Line 153:             P_VDSM_QCONF,
Line 154:             P_VDSM_LDCONF,
Line 155:         ]


Line 156:         for path in conf_paths:
Line 157:             if os.path.exists(path):
Line 158:                 try:
Line 159:                     removeSectionFromFile(path, conf_prefix, conf_suffix)
Line 160:                 except OSError, e:
>  no need try/except now 
If you are sure about it...
Line 161:                     if e.errno != os.errno.EEXIST:
Line 162:                         raise
Line 163: 
Line 164:         utils.rmFile(P_SYSTEMCTL_CONF)


Line 359: args
I don't see how c.removeConf() can return false.
Am I missing something?


Line 357:     m = [
Line 358:         c.getName() for c in __configurers
Line 359:         if c.getName() in args.modules and not c.removeConf()
Line 360:     ]
Line 361:     if m:
>  well, this is ugly... usually these methods of building lists should
> not ch 
I don't see understand. what do we change an object status here?
Line 362:         sys.stdout.write(
Line 363:             "Could not remove configuration for modules %s\n" %
','.join(m),
Line 364:         )
Line 365:         ret = False


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 2:

(3 comments)

I know we took over of existing patch, but this original implementation is wrong in my
opinion. there are much simpler ways to remove whatever vdsm configs. in gerenal this
patch should do the opposite\clean of vdsm-tool sebool-config, vdsm-tool set-saslpasswd,
vdsm-tool configure.

http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:

Line 86: P_SYSTEMCTL_CONF = SYSCONF_PATH + '/sysctl.d/vdsm'
Line 87: P_VDSM_LCONF = SYSCONF_PATH + '/libvirt/libvirtd.conf'
Line 88: P_VDSM_LDCONF = SYSCONF_PATH + '/sysconfig/libvirtd'
Line 89: P_VDSM_QCONF = SYSCONF_PATH + '/libvirt/qemu.conf'
Line 90: 
no need for that
Line 91: #
Line 92: # External programs (sorted, please keep in order).
Line 93: #
Line 94: EXT_BLKID = '@BLKID_PATH@'


http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 33: removeSectionFromFile
are we sure that its better than just executing ed command as we do in
libvirt_configure.sh?

        ed -s "${confFile}" >/dev/null 2>&1 <<EOF               
               
/${start_conf_section}/,/${end_conf_section}/d                                  
wq                                                                              
EOF                                                                             
        ed -s "${confFile}" >/dev/null 2>&1 <<EOF               
               
g/${by_vdsm}/d                                                                  
wq 

maybe its better to expose remove_conf in  libvirt_configure.sh and call that by
_exec_libvirt_configure

what do you think ?

anyway, we don't need such general function for that. only call to this
libvirt_configure remove_conf and that's all for this part of removal.


Line 147:         conf_prefix = '## beginning of configuration section by vdsm' +
\
Line 148:                       vdsm_ver
Line 149:         conf_suffix = '## end of configuration section by vdsm' +
vdsm_ver
Line 150: 
Line 151:         conf_paths = [
>  Done 
libvirt_configure.sh already keeps those files names in
it, remove_conf verb call is enough without all this bunch of code.
Line 152:             P_VDSM_LCONF,
Line 153:             P_VDSM_QCONF,
Line 154:             P_VDSM_LDCONF,
Line 155:         ]


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

mooli tayer has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:

Line 86: P_SYSTEMCTL_CONF = SYSCONF_PATH + '/sysctl.d/vdsm'
Line 87: P_VDSM_LCONF = SYSCONF_PATH + '/libvirt/libvirtd.conf'
Line 88: P_VDSM_LDCONF = SYSCONF_PATH + '/sysconfig/libvirtd'
Line 89: P_VDSM_QCONF = SYSCONF_PATH + '/libvirt/qemu.conf'
Line 90: 
>  no need for that 
I guess this is related to your comment on
configurator.py in line 151? my response to you there is that i'm not sure what you
want done in this patch instead?
Line 91: #
Line 92: # External programs (sorted, please keep in order).
Line 93: #
Line 94: EXT_BLKID = '@BLKID_PATH@'


http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 29: from ..constants import P_VDSM_EXEC, DISKIMAGE_GROUP, QEMU_PROCESS_GROUP, \
Line 30:     VDSM_GROUP, P_VDSM_LCONF, P_VDSM_QCONF, P_VDSM_LDCONF, P_SYSTEMCTL_CONF
Line 31: 
Line 32: 
Line 33: def removeSectionFromFile(filename, beginField=None, endField=None):
>  I do not think it is that complex, and it can be improved (testing
> the exac
> > testing the exact lines we add instead of signature before/after 
I guess that a patch removing exact lines should also change the code that inserts them?
not sure if it should be done now or or in a different patch.

Also, I do not like the duplicated:
'## beginning of configuration section by vdsm' in this patch.

Just a thought: is there a way we can do a one line import inside libvirt configuration
file/s to a new file containing our conf and then just remove that?
Line 34:     (
Line 35:         BEFORE,
Line 36:         WITHIN,
Line 37:         AFTER,


Line 147:         conf_prefix = '## beginning of configuration section by vdsm' +
\
Line 148:                       vdsm_ver
Line 149:         conf_suffix = '## end of configuration section by vdsm' +
vdsm_ver
Line 150: 
Line 151:         conf_paths = [
>  libvirt_configure.sh already keeps those files names in it,
> remove_conf ver 
I am not sure what you are suggesting to do in this patch instead?
Line 152:             P_VDSM_LCONF,
Line 153:             P_VDSM_QCONF,
Line 154:             P_VDSM_LDCONF,
Line 155:         ]


Line 357:     m = [
Line 358:         c.getName() for c in __configurers
Line 359:         if c.getName() in args.modules and not c.removeConf()
Line 360:     ]
Line 361:     if m:
>  c.removeConf() 
Well I guess this whole section should change
since c.removeConf() can never return false? Some other way should be used to detect
errors.
Line 362:         sys.stdout.write(
Line 363:             "Could not remove configuration for modules %s\n" %
','.join(m),
Line 364:         )
Line 365:         ret = False


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 3:

(5 comments)

http://gerrit.ovirt.org/#/c/21772/3//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2013-11-27 13:23:56 +0200
Line 4: Commit:     Mooli Tayer <mtayer(a)redhat.com&gt;
Line 5: CommitDate: 2014-03-02 20:10:47 +0200
Line 6: 
Line 7: Adding remove\disable verbs to vdsm-tool for admin usages
>  \ is called 'back slash' and should not be used as far as I
> know. 
interesting ... what i wrote is what my english teacher thought me in
highschool. i never doubted about it :/
Line 8: 
Line 9: The spec will be modified separately to use vdsm-tool instead of hard-coded
Line 10: operations
Line 11: 


http://gerrit.ovirt.org/#/c/21772/3/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 139:         except RuntimeError:
Line 140:             return False
Line 141: 
Line 142:     def removeConf(self):
Line 143:         vdsm_ver = '-4.9.10'
>  this is the version of the change not the version of vdsm... like
> signature 
this is how we know in which version of vdsm we introduce new
configuration change. only if this number is different we know if to override the
configuration with the old or new values. I don't understand why you need it when
removing the config.. it does not matter at all. please check first the
libvirt_configure.sh, read the code and see what it does. you just need to do the opposite
of that..
Line 144:         conf_prefix = '## beginning of configuration section by vdsm' +
\
Line 145:                       vdsm_ver
Line 146:         conf_suffix = '## end of configuration section by vdsm' +
vdsm_ver
Line 147: 


Line 360:                 c.removeConf()
Line 361:             except:
Line 362:                 failed.append(c.getName)
Line 363:     if failed:
Line 364:         sys.stdout.write(
>  yes, other patch if required fixing. 
ah? no!
for errors use stderr, for regular output to user such as - start removing... or removed
successfully prints use stdout. do it rightly, we don't need to have set of patches
here with no reason.
Line 365:             "Could not remove configuration for modules %s\n" %
','.join(failed),
Line 366:         )
Line 367:         raise RuntimeError("Remove configuration failed")
Line 368: 


http://gerrit.ovirt.org/#/c/21772/3/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 50: def remove_saslpasswd():
Line 51:     """
Line 52:     Remove vdsm password for libvirt connection
Line 53:     """
Line 54:     p = subprocess.Popen(('/usr/sbin/saslpasswd2', '-p',
'-a', 'libvirt', '-d', constants.SASL_USERNAME))
>  Are you saying /usr/sbin/saslpasswd2 should be in constants.py?
yes
Line 55:     output, err = p.communicate()
Line 56:     if p.returncode != 0:


Line 52:     Remove vdsm password for libvirt connection
Line 53:     """
Line 54:     p = subprocess.Popen(('/usr/sbin/saslpasswd2', '-p',
'-a', 'libvirt', '-d', constants.SASL_USERNAME))
Line 55:     output, err = p.communicate()
Line 56:     if p.returncode != 0:
>  Yaniv? 
utils.execCmd instead of Popen


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 5:

(4 comments)

http://gerrit.ovirt.org/#/c/21772/5/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 27: from .. import utils
Line 28: from . import service, expose
Line 29: from ..constants import P_VDSM_EXEC, DISKIMAGE_GROUP, QEMU_PROCESS_GROUP, \
Line 30:     VDSM_GROUP, P_VDSM_LCONF, P_VDSM_QCONF, P_VDSM_LDCONF, P_SYSTEMCTL_CONF
Line 31: 
>  removing unused DISKIMAGE_GROUP 
odd the pep8 wasn't see it
... but also, not related in this scope. post small patches for such issues, it'll be
accepted much faster upstream
Line 32: 
Line 33: def removeSectionFromFile(filename, beginField=None, endField=None):
Line 34:     (
Line 35:         BEFORE,


Line 140:         except RuntimeError:
Line 141:             return False
Line 142: 
Line 143:     def removeConf(self):
Line 144:         vdsm_ver = '-4.9.10'
so this version is stated twice now. once in libvirt_configure and one in
libvirt_configure script. that's an open to mistakes. I suggest that removeConf
won't count on version number, it'll search for vdsm stamp and remove all
vdsm's scope of the conf file. later on, when libvirt_configure will be part of this
code we'll be able to distinguish between the versions
Line 145:         conf_prefix = '## beginning of configuration section by vdsm' +
\
Line 146:                       vdsm_ver
Line 147:         conf_suffix = '## end of configuration section by vdsm' +
vdsm_ver
Line 148: 


Line 152:             P_VDSM_LDCONF,
Line 153:         ):
Line 154:             if os.path.exists(path):
Line 155:                 removeSectionFromFile(path, conf_prefix, conf_suffix)
Line 156:         utils.rmFile(P_SYSTEMCTL_CONF)
also systemctl conf part will be separated after arranging the libvirt_configure script.
so for now, lets leave it that way without the version (anyway you check only if the line
"startwith" the vdsm string)
Line 157: 
Line 158: 
Line 159: class SanlockModuleConfigure(_ModuleConfigure):
Line 160: 


http://gerrit.ovirt.org/#/c/21772/5/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 43:         script, stdin=f, stdout=subprocess.PIPE,
Line 44:         stderr=subprocess.PIPE, close_fds=True)
Line 45:     output, err = p.communicate()
Line 46:     if p.returncode != 0:
Line 47:         raise RuntimeError("Set password failed: %s" % (err, ))
>  why is this related to this patch? 
if you can do it patch later
that also change the popen usage to execCmd it'd be great...
Line 48: 
Line 49: 
Line 50: @expose("remove-saslpasswd")
Line 51: def remove_saslpasswd():


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 11:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6718/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7508/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7618/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 10: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 10:

Build Successful 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7482/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6691/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7592/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/9/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 31: 
Line 32: P_SYSTEMCTL_CONF = SYSCONF_PATH + '/sysctl.d/vdsm'
Line 33: P_VDSM_LCONF = SYSCONF_PATH + '/libvirt/libvirtd.conf'
Line 34: P_VDSM_LDCONF = SYSCONF_PATH + '/sysconfig/libvirtd'
Line 35: P_VDSM_QCONF = SYSCONF_PATH + '/libvirt/qemu.conf'
>  sorry I mean sysconfdir defined in autogen. so if we already have it
> would  
didn't we talk about it ..? i thought we agreed already that you can
leave it as is, just fix all jenkins errors
Line 36: 
Line 37: def removeSectionFromFile(filename, beginField=None, endField=None):
Line 38:     (
Line 39:         BEFORE,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 9:

I fixed pep8 issues long ago. I am waiting for you to respond before I upload next
version.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 9: Code-Review-1

fix pep8 issues..

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 9: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/9/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 31: 
Line 32: P_SYSTEMCTL_CONF = SYSCONF_PATH + '/sysctl.d/vdsm'
Line 33: P_VDSM_LCONF = SYSCONF_PATH + '/libvirt/libvirtd.conf'
Line 34: P_VDSM_LDCONF = SYSCONF_PATH + '/sysconfig/libvirtd'
Line 35: P_VDSM_QCONF = SYSCONF_PATH + '/libvirt/qemu.conf'
>  you add it in this patch.. i say its redundant. i prefer to avoid
> more cons 
sorry I mean sysconfdir defined in autogen. so if we already have it would
it not be better to use it?
Line 36: 
Line 37: def removeSectionFromFile(filename, beginField=None, endField=None):
Line 38:     (
Line 39:         BEFORE,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/9/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 31: 
Line 32: P_SYSTEMCTL_CONF = SYSCONF_PATH + '/sysctl.d/vdsm'
Line 33: P_VDSM_LCONF = SYSCONF_PATH + '/libvirt/libvirtd.conf'
Line 34: P_VDSM_LDCONF = SYSCONF_PATH + '/sysconfig/libvirtd'
Line 35: P_VDSM_QCONF = SYSCONF_PATH + '/libvirt/qemu.conf'
>  Ok for moving,  
you add it in this patch.. i say its redundant.
i prefer to avoid more constants
Line 36: 
Line 37: def removeSectionFromFile(filename, beginField=None, endField=None):
Line 38:     (
Line 39:         BEFORE,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/9/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 31: 
Line 32: P_SYSTEMCTL_CONF = SYSCONF_PATH + '/sysctl.d/vdsm'
Line 33: P_VDSM_LCONF = SYSCONF_PATH + '/libvirt/libvirtd.conf'
Line 34: P_VDSM_LDCONF = SYSCONF_PATH + '/sysconfig/libvirtd'
Line 35: P_VDSM_QCONF = SYSCONF_PATH + '/libvirt/qemu.conf'
>  its only used in LibvirtModuleConfigure, put it there.  
Ok for
moving, 
as for naming the variables with /etc/ what's the point in that? itsn't it why we
have SYSCONF_PATH for?
Line 36: 
Line 37: def removeSectionFromFile(filename, beginField=None, endField=None):
Line 38:     (
Line 39:         BEFORE,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 9:

(1 comment)

one last nit

http://gerrit.ovirt.org/#/c/21772/9/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 31: 
Line 32: P_SYSTEMCTL_CONF = SYSCONF_PATH + '/sysctl.d/vdsm'
Line 33: P_VDSM_LCONF = SYSCONF_PATH + '/libvirt/libvirtd.conf'
Line 34: P_VDSM_LDCONF = SYSCONF_PATH + '/sysconfig/libvirtd'
Line 35: P_VDSM_QCONF = SYSCONF_PATH + '/libvirt/qemu.conf'
its only used in LibvirtModuleConfigure, put it there. 
SYSCONF_PATH refers to /etc/ , I prefer to leave the constants.py file, abandon the
SYCONF_PATH, and declare the full path here
Line 36: 
Line 37: def removeSectionFromFile(filename, beginField=None, endField=None):
Line 38:     (
Line 39:         BEFORE,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 9: Code-Review-1 Verified-1

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7439/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6646/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7548/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/8/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:

Line 85: SYSCONF_PATH = '@sysconfdir@'
Line 86: P_SYSTEMCTL_CONF = SYSCONF_PATH + '/sysctl.d/vdsm'
Line 87: P_VDSM_LCONF = SYSCONF_PATH + '/libvirt/libvirtd.conf'
Line 88: P_VDSM_LDCONF = SYSCONF_PATH + '/sysconfig/libvirtd'
Line 89: P_VDSM_QCONF = SYSCONF_PATH + '/libvirt/qemu.conf'
>  these files also appear in the spec, is there any way to have them
> only in  
OK so I keep only SYSCONF_PATH here and remove the other 4 to
configurator.py.
Line 90: 
Line 91: #
Line 92: # External programs (sorted, please keep in order).
Line 93: #


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 8:

(2 comments)

http://gerrit.ovirt.org/#/c/21772/8/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:

Line 42: QEMU_PROCESS_GROUP = '@QEMUGROUP@'
Line 43: 
Line 44: #
Line 45: # SASL definitions
Line 46: #
>  why changing that? if comment, at least make it informative, like
> this is b 
My mistake, this was relevant for a previews commit.
Will revert.
Line 47: SASL_USERNAME = "vdsm@ovirt"
Line 48: 
Line 49: # This is the domain version translation list
Line 50: # DO NOT CHANGE OLD VALUES ONLY APPEND


Line 85: SYSCONF_PATH = '@sysconfdir@'
Line 86: P_SYSTEMCTL_CONF = SYSCONF_PATH + '/sysctl.d/vdsm'
Line 87: P_VDSM_LCONF = SYSCONF_PATH + '/libvirt/libvirtd.conf'
Line 88: P_VDSM_LDCONF = SYSCONF_PATH + '/sysconfig/libvirtd'
Line 89: P_VDSM_QCONF = SYSCONF_PATH + '/libvirt/qemu.conf'
>  lets move those to configurator.py ? only there we use it.. maybe
> sysconf_p 
these files also appear in the spec, is there any way to have them only in
one place or is the spec irrelevant to this?

Also they appear in debain scripts.
Line 90: 
Line 91: #
Line 92: # External programs (sorted, please keep in order).
Line 93: #


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 8:

(2 comments)

thanks! only the files path in constants is annoying me. we use it only in
libvirtConfigureModule so hold it there

http://gerrit.ovirt.org/#/c/21772/8/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:

Line 42: QEMU_PROCESS_GROUP = '@QEMUGROUP@'
Line 43: 
Line 44: #
Line 45: # SASL definitions
Line 46: #
why changing that? if comment, at least make it informative, like this is being used for
libvirt communication
Line 47: SASL_USERNAME = "vdsm@ovirt"
Line 48: 
Line 49: # This is the domain version translation list
Line 50: # DO NOT CHANGE OLD VALUES ONLY APPEND


Line 85: SYSCONF_PATH = '@sysconfdir@'
Line 86: P_SYSTEMCTL_CONF = SYSCONF_PATH + '/sysctl.d/vdsm'
Line 87: P_VDSM_LCONF = SYSCONF_PATH + '/libvirt/libvirtd.conf'
Line 88: P_VDSM_LDCONF = SYSCONF_PATH + '/sysconfig/libvirtd'
Line 89: P_VDSM_QCONF = SYSCONF_PATH + '/libvirt/qemu.conf'
lets move those to configurator.py ? only there we use it.. maybe sysconf_path can stay
here
Line 90: 
Line 91: #
Line 92: # External programs (sorted, please keep in order).
Line 93: #


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 8: Code-Review-1 Verified-1

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7429/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6636/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7538/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 7:

I am talking about your new tests that I need to complete:
http://gerrit.ovirt.org/#/c/25263/2 

I will add the other patches soon

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 7:

what tests? please at the same patch calls to remove-conf verbs from vdsm.spec when
needed. replace the inline scripts

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 7: Verified-1

I guess I better put -1 until all patches work...

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 7: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 7:

This patch now works while tests fail as they are not yet ready.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 7: Code-Review-1 Verified-1

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7425/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6632/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7534/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 7: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 6:

Sending another one...

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 6: Code-Review-1 Verified-1

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7423/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6630/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7532/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 6:

(2 comments)

http://gerrit.ovirt.org/#/c/21772/6/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 43:         script, stdin=f, stdout=subprocess.PIPE,
Line 44:         stderr=subprocess.PIPE, close_fds=True)
Line 45:     output, err = p.communicate()
Line 46:     if p.returncode != 0:
Line 47:         raise Exception("Set password failed: %s" % err )
Whoops replaced these two. removing
Line 48: 
Line 49: 
Line 50: @expose("remove-saslpasswd")
Line 51: def remove_saslpasswd():


Line 55:     rc, out, err = utils.execCmd(
Line 56:         (constants.EXT_SASLPASSWD2, '-p', '-a',
'libvirt', '-d', constants.SASL_USERNAME,),
Line 57:     )
Line 58:     if rc != 0:
Line 59:         raise RuntimeError("Remove password failed: %s" % (err, ))
Whoops replaced these two. removing


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/5/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 140:         except RuntimeError:
Line 141:             return False
Line 142: 
Line 143:     def removeConf(self):
Line 144:         vdsm_ver = '-4.9.10'
>  Well after we talked, I removed the vdsm_ver completely. 
we
don't. send your new version
Line 145:         conf_prefix = '## beginning of configuration section by vdsm' +
\
Line 146:                       vdsm_ver
Line 147:         conf_suffix = '## end of configuration section by vdsm' +
vdsm_ver
Line 148: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 5:

(5 comments)

http://gerrit.ovirt.org/#/c/21772/5/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 27: from .. import utils
Line 28: from . import service, expose
Line 29: from ..constants import P_VDSM_EXEC, DISKIMAGE_GROUP, QEMU_PROCESS_GROUP, \
Line 30:     VDSM_GROUP, P_VDSM_LCONF, P_VDSM_QCONF, P_VDSM_LDCONF, P_SYSTEMCTL_CONF
Line 31: 
>  odd the pep8 wasn't see it ... but also, not related in this
> scope. post sm 
It was originally added in this change in a previews patch set
Line 32: 
Line 33: def removeSectionFromFile(filename, beginField=None, endField=None):
Line 34:     (
Line 35:         BEFORE,


Line 140:         except RuntimeError:
Line 141:             return False
Line 142: 
Line 143:     def removeConf(self):
Line 144:         vdsm_ver = '-4.9.10'
>  so this version is stated twice now. once in libvirt_configure and
> one in l 
Well after we talked, I removed the vdsm_ver completely.

As I understood from you there should always be only one section with whatever number and
that is what we want to remove so why do we need to distinguish between versions(in
removeConf)?
Line 145:         conf_prefix = '## beginning of configuration section by vdsm' +
\
Line 146:                       vdsm_ver
Line 147:         conf_suffix = '## end of configuration section by vdsm' +
vdsm_ver
Line 148: 


Line 152:             P_VDSM_LDCONF,
Line 153:         ):
Line 154:             if os.path.exists(path):
Line 155:                 removeSectionFromFile(path, conf_prefix, conf_suffix)
Line 156:         utils.rmFile(P_SYSTEMCTL_CONF)
>  also systemctl conf part will be separated after arranging the
> libvirt_conf 
Done
Line 157: 
Line 158: 
Line 159: class SanlockModuleConfigure(_ModuleConfigure):
Line 160: 


http://gerrit.ovirt.org/#/c/21772/5/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 43:         script, stdin=f, stdout=subprocess.PIPE,
Line 44:         stderr=subprocess.PIPE, close_fds=True)
Line 45:     output, err = p.communicate()
Line 46:     if p.returncode != 0:
Line 47:         raise RuntimeError("Set password failed: %s" % (err, ))
>  if you can do it patch later that also change the popen usage to
> execCmd it 
Ok so two patches more.
Line 48: 
Line 49: 
Line 50: @expose("remove-saslpasswd")
Line 51: def remove_saslpasswd():


Line 55:     rc, out, err = utils.execCmd(
Line 56:         (constants.EXT_SASLPASSWD2, '-p', '-a',
'libvirt', '-d', constants.SASL_USERNAME,),
Line 57:     )
Line 58:     if rc != 0:
Line 59:         raise RuntimeError("Remove password failed: %s" % (err, ))
>  just thought I revisit this... why do you need tuple for err?
Removing


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.ovirt.org/#/c/21772/5/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 43:         script, stdin=f, stdout=subprocess.PIPE,
Line 44:         stderr=subprocess.PIPE, close_fds=True)
Line 45:     output, err = p.communicate()
Line 46:     if p.returncode != 0:
Line 47:         raise RuntimeError("Set password failed: %s" % (err, ))
why is this related to this patch?
Line 48: 
Line 49: 
Line 50: @expose("remove-saslpasswd")
Line 51: def remove_saslpasswd():


Line 55:     rc, out, err = utils.execCmd(
Line 56:         (constants.EXT_SASLPASSWD2, '-p', '-a',
'libvirt', '-d', constants.SASL_USERNAME,),
Line 57:     )
Line 58:     if rc != 0:
Line 59:         raise RuntimeError("Remove password failed: %s" % (err, ))
just thought I revisit this... why do you need tuple for err?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/5/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 27: from .. import utils
Line 28: from . import service, expose
Line 29: from ..constants import P_VDSM_EXEC, DISKIMAGE_GROUP, QEMU_PROCESS_GROUP, \
Line 30:     VDSM_GROUP, P_VDSM_LCONF, P_VDSM_QCONF, P_VDSM_LDCONF, P_SYSTEMCTL_CONF
Line 31: 
removing unused DISKIMAGE_GROUP
Line 32: 
Line 33: def removeSectionFromFile(filename, beginField=None, endField=None):
Line 34:     (
Line 35:         BEFORE,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 5: Code-Review+1

ok, I am good, now vdsm people will probably wake up :)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 5: Code-Review-1 Verified-1

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7407/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6614/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7516/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/4/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 53:     Remove vdsm password for libvirt connection
Line 54:     """
Line 55:     rc, out, err = utils.execCmd(
Line 56:         (constants.P_EXEC_SASL, '-p', '-a', 'libvirt',
'-d', constants.SASL_USERNAME,),
Line 57:         raw=True,
>  I saw that raw=False (default) splits output lines, and we don't
> need that. 
well, I tend to use default unless there is a good reason, not the other
way around :)
Line 58:     )
Line 59:     if rc != 0:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

mooli tayer has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.ovirt.org/#/c/21772/4/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:

Line 43: 
Line 44: #
Line 45: # SASL definitions
Line 46: #
Line 47: P_EXEC_SASL = '/usr/sbin/saslpasswd2'
>  I think it should be similar to other EXT_GREP =
> '@GREP_PATH@', and determi 
Done
Line 48: SASL_USERNAME = "vdsm@ovirt"
Line 49: 
Line 50: # This is the domain version translation list
Line 51: # DO NOT CHANGE OLD VALUES ONLY APPEND


http://gerrit.ovirt.org/#/c/21772/4/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 354:     for c in __configurers:
Line 355:         if c.getName() in args.modules:
Line 356:             try:
Line 357:                 c.removeConf()
Line 358:             except:
>  please do not swallow exceptions. 
Done
Line 359:                 failed.append(c.getName)
Line 360:     if failed:
Line 361:         sys.stderr.write(
Line 362:             "Could not remove configuration for modules %s\n" %
','.join(failed),


Line 359:                 failed.append(c.getName)
Line 360:     if failed:
Line 361:         sys.stderr.write(
Line 362:             "Could not remove configuration for modules %s\n" %
','.join(failed),
Line 363:         )
>  I think this can be dropped in favour of boolean and print above.
Done
Line 364:         raise RuntimeError("Remove configuration failed")
Line 365: 
Line 366: 
Line 367: def _parse_args(action):


http://gerrit.ovirt.org/#/c/21772/4/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 53:     Remove vdsm password for libvirt connection
Line 54:     """
Line 55:     rc, out, err = utils.execCmd(
Line 56:         (constants.P_EXEC_SASL, '-p', '-a', 'libvirt',
'-d', constants.SASL_USERNAME,),
Line 57:         raw=True,
>  why raw? 
I saw that raw=False (default) splits output lines,
and we don't need that. I not sure why its a part of the function?

Remove the raw?
Line 58:     )
Line 59:     if rc != 0:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.ovirt.org/#/c/21772/4/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:

Line 43: 
Line 44: #
Line 45: # SASL definitions
Line 46: #
Line 47: P_EXEC_SASL = '/usr/sbin/saslpasswd2'
I think it should be similar to other EXT_GREP = '@GREP_PATH@', and determined by
configure.ac
Line 48: SASL_USERNAME = "vdsm@ovirt"
Line 49: 
Line 50: # This is the domain version translation list
Line 51: # DO NOT CHANGE OLD VALUES ONLY APPEND


http://gerrit.ovirt.org/#/c/21772/4/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 354:     for c in __configurers:
Line 355:         if c.getName() in args.modules:
Line 356:             try:
Line 357:                 c.removeConf()
Line 358:             except:
please do not swallow exceptions.

worse case it should be except Exception to only catch these who are valid, but you should
send info to logger.

any reason why not:

 except Exception:
     sys.stderr.write('Cannot remove %s\n')
     traceback.print_exc(file=sys.stderr)
Line 359:                 failed.append(c.getName)
Line 360:     if failed:
Line 361:         sys.stderr.write(
Line 362:             "Could not remove configuration for modules %s\n" %
','.join(failed),


Line 355:         if c.getName() in args.modules:
Line 356:             try:
Line 357:                 c.removeConf()
Line 358:             except:
Line 359:                 failed.append(c.getName)
getName->getName()
Line 360:     if failed:
Line 361:         sys.stderr.write(
Line 362:             "Could not remove configuration for modules %s\n" %
','.join(failed),
Line 363:         )


Line 359:                 failed.append(c.getName)
Line 360:     if failed:
Line 361:         sys.stderr.write(
Line 362:             "Could not remove configuration for modules %s\n" %
','.join(failed),
Line 363:         )
I think this can be dropped in favour of boolean and print above.
Line 364:         raise RuntimeError("Remove configuration failed")
Line 365: 
Line 366: 
Line 367: def _parse_args(action):


http://gerrit.ovirt.org/#/c/21772/4/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 53:     Remove vdsm password for libvirt connection
Line 54:     """
Line 55:     rc, out, err = utils.execCmd(
Line 56:         (constants.P_EXEC_SASL, '-p', '-a', 'libvirt',
'-d', constants.SASL_USERNAME,),
Line 57:         raw=True,
why raw?
Line 58:     )
Line 59:     if rc != 0:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove/disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 4: Code-Review-1 Verified-1

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7396/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6603/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7505/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

mooli tayer has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.ovirt.org/#/c/21772/3//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2013-11-27 13:23:56 +0200
Line 4: Commit:     Mooli Tayer <mtayer(a)redhat.com&gt;
Line 5: CommitDate: 2014-03-02 20:10:47 +0200
Line 6: 
Line 7: Adding remove\disable verbs to vdsm-tool for admin usages
>  interesting ... what i wrote is what my english teacher thought me in
> highs 
Done
Line 8: 
Line 9: The spec will be modified separately to use vdsm-tool instead of hard-coded
Line 10: operations
Line 11: 


http://gerrit.ovirt.org/#/c/21772/3/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 360:                 c.removeConf()
Line 361:             except:
Line 362:                 failed.append(c.getName)
Line 363:     if failed:
Line 364:         sys.stdout.write(
>  ah? no! 
I changed this line as part of the next commit. I was
talking about other lines like 342 and others which are not related to this patch...
Line 365:             "Could not remove configuration for modules %s\n" %
','.join(failed),
Line 366:         )
Line 367:         raise RuntimeError("Remove configuration failed")
Line 368: 


http://gerrit.ovirt.org/#/c/21772/3/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 50: def remove_saslpasswd():
Line 51:     """
Line 52:     Remove vdsm password for libvirt connection
Line 53:     """
Line 54:     p = subprocess.Popen(('/usr/sbin/saslpasswd2', '-p',
'-a', 'libvirt', '-d', constants.SASL_USERNAME))
>  yes 
Done
Line 55:     output, err = p.communicate()
Line 56:     if p.returncode != 0:


Line 52:     Remove vdsm password for libvirt connection
Line 53:     """
Line 54:     p = subprocess.Popen(('/usr/sbin/saslpasswd2', '-p',
'-a', 'libvirt', '-d', constants.SASL_USERNAME))
Line 55:     output, err = p.communicate()
Line 56:     if p.returncode != 0:
>  utils.execCmd instead of Popen 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.ovirt.org/#/c/21772/3/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 139:         except RuntimeError:
Line 140:             return False
Line 141: 
Line 142:     def removeConf(self):
Line 143:         vdsm_ver = '-4.9.10'
>  This patch fails due to this line. Where from do I get the current
> version? 
this is the version of the change not the version of vdsm... like
signature... the fact that the version of vdsm was used once and became the signature is
strange but this what we have.
Line 144:         conf_prefix = '## beginning of configuration section by vdsm' +
\
Line 145:                       vdsm_ver
Line 146:         conf_suffix = '## end of configuration section by vdsm' +
vdsm_ver
Line 147: 


Line 360:                 c.removeConf()
Line 361:             except:
Line 362:                 failed.append(c.getName)
Line 363:     if failed:
Line 364:         sys.stdout.write(
>  Done. 
yes, other patch if required fixing.
Line 365:             "Could not remove configuration for modules %s\n" %
','.join(failed),
Line 366:         )
Line 367:         raise RuntimeError("Remove configuration failed")
Line 368: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

mooli tayer has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 3:

(7 comments)

Also rebasing on top of new tests:
http://gerrit.ovirt.org/#/c/25263/

http://gerrit.ovirt.org/#/c/21772/3/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 139:         except RuntimeError:
Line 140:             return False
Line 141: 
Line 142:     def removeConf(self):
Line 143:         vdsm_ver = '-4.9.10'
This patch fails due to this line. Where from do I get the current version?

Yaniv?
Line 144:         conf_prefix = '## beginning of configuration section by vdsm' +
\
Line 145:                       vdsm_ver
Line 146:         conf_suffix = '## end of configuration section by vdsm' +
vdsm_ver
Line 147: 


Line 148:         for path in [
Line 149:             P_VDSM_LCONF,
Line 150:             P_VDSM_QCONF,
Line 151:             P_VDSM_LDCONF,
Line 152:         ]:
>  tuple and not list will be better. 
Done
Line 153:             if os.path.exists(path):
Line 154:                 removeSectionFromFile(path, conf_prefix, conf_suffix)
Line 155:         utils.rmFile(P_SYSTEMCTL_CONF)
Line 156: 


Line 151:             P_VDSM_LDCONF,
Line 152:         ]:
Line 153:             if os.path.exists(path):
Line 154:                 removeSectionFromFile(path, conf_prefix, conf_suffix)
Line 155:         utils.rmFile(P_SYSTEMCTL_CONF)
>  yes, we need to split and arrange - sysctl conf to
> SystemctlModuleConfigure 
So for now I leave this as is
Line 156: 
Line 157: 
Line 158: class SanlockModuleConfigure(_ModuleConfigure):
Line 159: 


Line 168:     def getServices(self):
Line 169:         return ['sanlock']
Line 170: 
Line 171:     def removeConf(self):
Line 172:         pass
>  no need 
Done
Line 173: 
Line 174:     def configure(self):
Line 175:         """
Line 176:         Configure sanlock process groups


Line 360:                 c.removeConf()
Line 361:             except:
Line 362:                 failed.append(c.getName)
Line 363:     if failed:
Line 364:         sys.stdout.write(
>  stderr? 
Done.

We have the same problem in other modules.
Fix in a different patch set?
Line 365:             "Could not remove configuration for modules %s\n" %
','.join(failed),
Line 366:         )
Line 367:         raise RuntimeError("Remove configuration failed")
Line 368: 


http://gerrit.ovirt.org/#/c/21772/3/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 50: def remove_saslpasswd():
Line 51:     """
Line 52:     Remove vdsm password for libvirt connection
Line 53:     """
Line 54:     p = subprocess.Popen(('/usr/sbin/saslpasswd2', '-p',
'-a', 'libvirt', '-d', constants.SASL_USERNAME))
>  should be in P_EXEC_XXXX 
Are you saying /usr/sbin/saslpasswd2
should be in constants.py?
Line 55:     output, err = p.communicate()
Line 56:     if p.returncode != 0:


Line 52:     Remove vdsm password for libvirt connection
Line 53:     """
Line 54:     p = subprocess.Popen(('/usr/sbin/saslpasswd2', '-p',
'-a', 'libvirt', '-d', constants.SASL_USERNAME))
Line 55:     output, err = p.communicate()
Line 56:     if p.returncode != 0:
>  please use vdsm utilities for process execution, I guess there are.
Yaniv?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/3//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2013-11-27 13:23:56 +0200
Line 4: Commit:     Mooli Tayer <mtayer(a)redhat.com&gt;
Line 5: CommitDate: 2014-03-02 20:10:47 +0200
Line 6: 
Line 7: Adding remove\disable verbs to vdsm-tool for admin usages
>  the slash for "or" (afaik) should lean to the opposite side
> of the language 
\ is called 'back slash' and should not be used as far as I
know.

http://www.englishclub.com/writing/punctuation-slash.htm
Line 8: 
Line 9: The spec will be modified separately to use vdsm-tool instead of hard-coded
Line 10: operations
Line 11: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.ovirt.org/#/c/21772/3//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2013-11-27 13:23:56 +0200
Line 4: Commit:     Mooli Tayer <mtayer(a)redhat.com&gt;
Line 5: CommitDate: 2014-03-02 20:10:47 +0200
Line 6: 
Line 7: Adding remove\disable verbs to vdsm-tool for admin usages
>  I must comment.... \ -> / :))) 
the slash for "or"
(afaik) should lean to the opposite side of the language writing direction. in hebrew its
/, english \ (from left to right)
Line 8: 
Line 9: The spec will be modified separately to use vdsm-tool instead of hard-coded
Line 10: operations
Line 11: 


http://gerrit.ovirt.org/#/c/21772/3/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 151:             P_VDSM_LDCONF,
Line 152:         ]:
Line 153:             if os.path.exists(path):
Line 154:                 removeSectionFromFile(path, conf_prefix, conf_suffix)
Line 155:         utils.rmFile(P_SYSTEMCTL_CONF)
>  is this one^ related to libvirt? 
yes, we need to split and
arrange - sysctl conf to SystemctlModuleConfigure, move qemu part to QemuMeduleConfigure.


but this later. in this phase lets just create the removal part, afterwards we'll
arrange to modules (i plan also SelinuxModuleConfigure, VdsmModuleConfigure with flags
(like having --ssl option for configure and inc) and more improvement for this tool to
avoid such scriptish spec file)
Line 156: 
Line 157: 
Line 158: class SanlockModuleConfigure(_ModuleConfigure):
Line 159: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 3: Code-Review-1 Verified-1

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7383/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6592/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7494/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

mooli tayer has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 142:         except RuntimeError:
Line 143:             return False
Line 144: 
Line 145:     def removeConf(self):
Line 146:         vdsm_ver = '-4.9.10'
This patch fails due to this line.
Where do I need to get the correct version from?
Line 147:         conf_prefix = '## beginning of configuration section by vdsm' +
\
Line 148:                       vdsm_ver
Line 149:         conf_suffix = '## end of configuration section by vdsm' +
vdsm_ver
Line 150: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 29: from ..constants import P_VDSM_EXEC, DISKIMAGE_GROUP, QEMU_PROCESS_GROUP, \
Line 30:     VDSM_GROUP, P_VDSM_LCONF, P_VDSM_QCONF, P_VDSM_LDCONF, P_SYSTEMCTL_CONF
Line 31: 
Line 32: 
Line 33: def removeSectionFromFile(filename, beginField=None, endField=None):
>  >> testing the exact lines we add instead of signature
> before/after 
we can change the way vdsm reads its configuration files (vdsm.conf) by
creating vdsm.d dir and under it all specific conf files with num prefix. but here vdsm
modifies libvirtd.conf and qemu.conf . we won't change their way of reading the
config, currently those services use one file for configuration and vdsm part interferes
in it
Line 34:     (
Line 35:         BEFORE,
Line 36:         WITHIN,
Line 37:         AFTER,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 29: from ..constants import P_VDSM_EXEC, DISKIMAGE_GROUP, QEMU_PROCESS_GROUP, \
Line 30:     VDSM_GROUP, P_VDSM_LCONF, P_VDSM_QCONF, P_VDSM_LDCONF, P_SYSTEMCTL_CONF
Line 31: 
Line 32: 
Line 33: def removeSectionFromFile(filename, beginField=None, endField=None):
>  yes.... I remember that, but after seeing below ..  i asked for
> opinions ab 
I do not think it is that complex, and it can be improved (testing the
exact lines we add instead of signature before/after).

I would much like to see that shell script go away.
Line 34:     (
Line 35:         BEFORE,
Line 36:         WITHIN,
Line 37:         AFTER,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 29: from ..constants import P_VDSM_EXEC, DISKIMAGE_GROUP, QEMU_PROCESS_GROUP, \
Line 30:     VDSM_GROUP, P_VDSM_LCONF, P_VDSM_QCONF, P_VDSM_LDCONF, P_SYSTEMCTL_CONF
Line 31: 
Line 32: 
Line 33: def removeSectionFromFile(filename, beginField=None, endField=None):
>  I thought we want to minimize the the shell script processing until
> total r 
yes.... I remember that, but after seeing below ..  i asked for opinions
about it. the current implementation seems to me way too much for removal a well defined
scope of text. at least I prefer the same call to "ed -s" command by execCmd
instead of this confusing loop
Line 34:     (
Line 35:         BEFORE,
Line 36:         WITHIN,
Line 37:         AFTER,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 29: from ..constants import P_VDSM_EXEC, DISKIMAGE_GROUP, QEMU_PROCESS_GROUP, \
Line 30:     VDSM_GROUP, P_VDSM_LCONF, P_VDSM_QCONF, P_VDSM_LDCONF, P_SYSTEMCTL_CONF
Line 31: 
Line 32: 
Line 33: def removeSectionFromFile(filename, beginField=None, endField=None):
>  are we sure that its better than just executing ed command as we do
> in libv 
I thought we want to minimize the the shell script processing until total
removal.
Line 34:     (
Line 35:         BEFORE,
Line 36:         WITHIN,
Line 37:         AFTER,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 2:

you can write ut that imports the removeSectionFromFile func, creates temporary file and
cat in the vdsm section config, then check your code on that. shouldn't be complex. im
working on testing for the configurator itself when vdsm.conf is changed, to verify that
vdsm actually configures the right values for libvirt and qemu. but this doesn't need
to block you from testing that removal funcs..

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 357:     m = [
Line 358:         c.getName() for c in __configurers
Line 359:         if c.getName() in args.modules and not c.removeConf()
Line 360:     ]
Line 361:     if m:
>  I don't see understand. what do we change an object status here?
c.removeConf()
Line 362:         sys.stdout.write(
Line 363:             "Could not remove configuration for modules %s\n" %
','.join(m),
Line 364:         )
Line 365:         ret = False


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 53:             newcontent.append(line)
Line 54:         else:
Line 55:             raise RuntimeError("Internal Error")
Line 56:     if content != newcontent:
Line 57:         tmpfile = tempfile.NamedTemporaryFile(delete=False)
you need to create temp file at same directory of original file... see tempfile.mkstemp,
and then use os.rename for atomic.
Line 58:         tname = tmpfile.name
Line 59:         tmpfile.writelines(newcontent)
Line 60:         tmpfile.close()
Line 61:         shutil.move(tname, filename)


Line 56:     if content != newcontent:
Line 57:         tmpfile = tempfile.NamedTemporaryFile(delete=False)
Line 58:         tname = tmpfile.name
Line 59:         tmpfile.writelines(newcontent)
Line 60:         tmpfile.close()
you should never use close() but python with structure.
Line 61:         shutil.move(tname, filename)
Line 62: 
Line 63: 
Line 64: class _ModuleConfigure(object):


Line 147:         conf_prefix = '## beginning of configuration section by vdsm' +
\
Line 148:                       vdsm_ver
Line 149:         conf_suffix = '## end of configuration section by vdsm' +
vdsm_ver
Line 150: 
Line 151:         conf_paths = [
consider remove temporary one time used variable.

 for path in  [
 ]:
Line 152:             P_VDSM_LCONF,
Line 153:             P_VDSM_QCONF,
Line 154:             P_VDSM_LDCONF,
Line 155:         ]


Line 156:         for path in conf_paths:
Line 157:             if os.path.exists(path):
Line 158:                 try:
Line 159:                     removeSectionFromFile(path, conf_prefix, conf_suffix)
Line 160:                 except OSError, e:
no need try/except now
Line 161:                     if e.errno != os.errno.EEXIST:
Line 162:                         raise
Line 163: 
Line 164:         utils.rmFile(P_SYSTEMCTL_CONF)


Line 357:     m = [
Line 358:         c.getName() for c in __configurers
Line 359:         if c.getName() in args.modules and not c.removeConf()
Line 360:     ]
Line 361:     if m:
well, this is ugly... usually these methods of building lists should not change the object
status...
Line 362:         sys.stdout.write(
Line 363:             "Could not remove configuration for modules %s\n" %
','.join(m),
Line 364:         )
Line 365:         ret = False


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 2: Code-Review-1

Build Unstable 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6517/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7301/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7419/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

mooli tayer has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 401: ':
Removed


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/21772/1/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 106:                                                                conf_prefix,
Line 107:                                                                conf_suffix)
Line 108:             except OSError, e:
Line 109:                 if e.errno != os.errno.EEXIST:
Line 110:                     raise
>  Not sure what you mean here. 
should be:

 if os.path.exists(xxx):
     super(...).removeConf()
Line 111: 
Line 112:         utils.rmFile(P_SYSTEMCTL_CONF)
Line 113: 
Line 114:     def _exec_libvirt_configure(self, action):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

mooli tayer has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 1:

(8 comments)

I am continuing Yaniv's work here.

http://gerrit.ovirt.org/#/c/21772/1/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 48: 
Line 49:     def isconfigured(self):
Line 50:         return True
Line 51: 
Line 52:     def removeConf(self, filename, beginField=None, endField=None):
>  this is not removeConf in inheritance model, it is utility, the
> removeConf  
Done
Line 53:         newfile = []
Line 54:         if not beginField and not endField:
Line 55:             return
Line 56:         else:


Line 52:     def removeConf(self, filename, beginField=None, endField=None):
Line 53:         newfile = []
Line 54:         if not beginField and not endField:
Line 55:             return
Line 56:         else:
>  so why else? 
Done
Line 57:             with open(filename, 'r') as f:
Line 58:                 skip = False
Line 59:                 for line in f.readlines():
Line 60:                     if beginField and endField:


Line 70:                             continue
Line 71:                     else:
Line 72:                         if line.endswith('%s\n' % endField):
Line 73:                             continue
Line 74:                     newfile.append(line)
>  we are talked about this structure... it is spaghetti! 
Done
Line 75:             tmp_file = tempfile.NamedTemporaryFile(delete=False)
Line 76:             tname = tmp_file.name
Line 77:             tmp_file.writelines(newfile)
Line 78:             tmp_file.close()


Line 103:         for path in conf_paths:
Line 104:             try:
Line 105:                 super(LibvirtModuleConfigure, self).removeConf(path,
Line 106:                                                                conf_prefix,
Line 107:                                                                conf_suffix)
>  please avoid drawing code... 
Replaced with utility method
Line 108:             except OSError, e:
Line 109:                 if e.errno != os.errno.EEXIST:
Line 110:                     raise
Line 111: 


Line 106:                                                                conf_prefix,
Line 107:                                                                conf_suffix)
Line 108:             except OSError, e:
Line 109:                 if e.errno != os.errno.EEXIST:
Line 110:                     raise
>  you should first check if file exist then attempt to modify, do not
> assume  
Not sure what you mean here.
Line 111: 
Line 112:         utils.rmFile(P_SYSTEMCTL_CONF)
Line 113: 
Line 114:     def _exec_libvirt_configure(self, action):


Line 320: """
adding
ret = True


http://gerrit.ovirt.org/#/c/21772/1/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 52:     Remove vdsm password for libvirt connection
Line 53:     """
Line 54:     script = ['/usr/sbin/saslpasswd2', '-p', '-a',
'libvirt', '-d',
Line 55:               constants.SASL_USERNAME]
Line 56:     p = subprocess.Popen(script)
>  why do you need this script temp variable? why isn't it tuple?
done
Line 57:     output, err = p.communicate()
Line 58:     if p.returncode != 0:


http://gerrit.ovirt.org/#/c/21772/1/lib/vdsm/tool/seboolsetup.py
File lib/vdsm/tool/seboolsetup.py:

Line 65:     """Disable the required selinux booleans"""
Line 66:     setup_booleans(False)
Line 67: 
Line 68: 
Line 69: @expose("clear-selinux-policy")
>  sebool_unconfig does the same. I'll remove that 
Removing
section
Line 70: def clear_selinux_policy():
Line 71:     """
Line 72:     Clear the changes of selinux policy
Line 73:     """


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 3:

(7 comments)

http://gerrit.ovirt.org/#/c/21772/3//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2013-11-27 13:23:56 +0200
Line 4: Commit:     Mooli Tayer <mtayer(a)redhat.com&gt;
Line 5: CommitDate: 2014-03-02 20:10:47 +0200
Line 6: 
Line 7: Adding remove\disable verbs to vdsm-tool for admin usages
I must comment.... \ -> / :)))
Line 8: 
Line 9: The spec will be modified separately to use vdsm-tool instead of hard-coded
Line 10: operations
Line 11: 


http://gerrit.ovirt.org/#/c/21772/3/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 148:         for path in [
Line 149:             P_VDSM_LCONF,
Line 150:             P_VDSM_QCONF,
Line 151:             P_VDSM_LDCONF,
Line 152:         ]:
tuple and not list will be better.
Line 153:             if os.path.exists(path):
Line 154:                 removeSectionFromFile(path, conf_prefix, conf_suffix)
Line 155:         utils.rmFile(P_SYSTEMCTL_CONF)
Line 156: 


Line 151:             P_VDSM_LDCONF,
Line 152:         ]:
Line 153:             if os.path.exists(path):
Line 154:                 removeSectionFromFile(path, conf_prefix, conf_suffix)
Line 155:         utils.rmFile(P_SYSTEMCTL_CONF)
is this one^ related to libvirt?
Line 156: 
Line 157: 
Line 158: class SanlockModuleConfigure(_ModuleConfigure):
Line 159: 


Line 168:     def getServices(self):
Line 169:         return ['sanlock']
Line 170: 
Line 171:     def removeConf(self):
Line 172:         pass
no need
Line 173: 
Line 174:     def configure(self):
Line 175:         """
Line 176:         Configure sanlock process groups


Line 360:                 c.removeConf()
Line 361:             except:
Line 362:                 failed.append(c.getName)
Line 363:     if failed:
Line 364:         sys.stdout.write(
stderr?
Line 365:             "Could not remove configuration for modules %s\n" %
','.join(failed),
Line 366:         )
Line 367:         raise RuntimeError("Remove configuration failed")
Line 368: 


http://gerrit.ovirt.org/#/c/21772/3/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 50: def remove_saslpasswd():
Line 51:     """
Line 52:     Remove vdsm password for libvirt connection
Line 53:     """
Line 54:     p = subprocess.Popen(('/usr/sbin/saslpasswd2', '-p',
'-a', 'libvirt', '-d', constants.SASL_USERNAME))
should be in P_EXEC_XXXX
Line 55:     output, err = p.communicate()
Line 56:     if p.returncode != 0:


Line 52:     Remove vdsm password for libvirt connection
Line 53:     """
Line 54:     p = subprocess.Popen(('/usr/sbin/saslpasswd2', '-p',
'-a', 'libvirt', '-d', constants.SASL_USERNAME))
Line 55:     output, err = p.communicate()
Line 56:     if p.returncode != 0:
please use vdsm utilities for process execution, I guess there are.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes