Change in vdsm[master]: configure verb: replace libvirt_configure.sh call with pytho...

mooli tayer has uploaded a new change for review.

Change subject: configure verb: replace libvirt_configure.sh call with python code.
......................................................................

configure verb: replace libvirt_configure.sh call with python code.

Change-Id: Ie37988ef230f889e7154e504a889f28db5be7328
Signed-off-by: Mooli Tayer 
---
M lib/vdsm/constants.py.in
M lib/vdsm/tool/configurator.py
M lib/vdsm/utils.py
M tests/toolTests.py
4 files changed, 168 insertions(+), 21 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/30/27130/1

diff --git a/lib/vdsm/constants.py.in b/lib/vdsm/constants.py.in
index 90a04a0..35ead6d 100644
--- a/lib/vdsm/constants.py.in
+++ b/lib/vdsm/constants.py.in
@@ -42,7 +42,11 @@
 QEMU_PROCESS_GROUP = '@QEMUGROUP@'

# Sanlock definitions
+SANLOCK_ENABLED = '@ENABLE_LIBVIRT_SANLOCK@' == 'yes'
 SANLOCK_USER = '@SNLKUSER@'
+
+# Libvirt selinux
+LIBVIRT_SELINUX = '@ENABLE_LIBVIRT_SELINUX@' == 'yes'

#
 # The username of SASL authenticating for libvirt connection
@@ -75,6 +79,7 @@
 P_VDSM_CONF = '@CONFDIR@/'
 P_VDSM_KEYS = '/etc/pki/vdsm/keys/'
 P_VDSM_LIBVIRT_PASSWD = P_VDSM_KEYS + 'libvirt_password'
+P_VDSM_CERT = '/etc/pki/vdsm/certs/vdsmcert.pem'

P_VDSM_CLIENT_LOG = '@VDSMRUNDIR@/client.log'
 P_VDSM_LOG = '@VDSMLOGDIR@'
diff --git a/lib/vdsm/tool/configurator.py b/lib/vdsm/tool/configurator.py
index 53d9875..e782762 100644
--- a/lib/vdsm/tool/configurator.py
+++ b/lib/vdsm/tool/configurator.py
@@ -26,23 +26,122 @@
 import rpm
 import shutil
 import traceback
+import uuid

from .. import utils
-from . import service, expose
+from . import service, expose, validate_ovirt_certs
 from .configfile import ConfigFile
-from ..constants import P_VDSM_EXEC, QEMU_PROCESS_GROUP, VDSM_GROUP, \
-    SANLOCK_USER, SYSCONF_PATH
+from ..constants import P_VDSM_EXEC, QEMU_PROCESS_GROUP, \
+    SANLOCK_USER, VDSM_GROUP, SYSCONF_PATH, P_VDSM_CERT, \
+    SANLOCK_ENABLED, LIBVIRT_SELINUX
+from vdsm.config import config
+
+try:
+    from ovirtnode import ovirtfunctions
+except ImportError:
+    pass

CONF_PREFIX = '## beginning of configuration section by vdsm'
 CONF_SUFFIX = '## end of configuration section by vdsm'
+CONF_VER = '4.13.0'

+PKI = os.path.join(SYSCONF_PATH, 'pki/vdsm')
+CA_FILE = os.path.join(PKI, 'certs/cacert.pem')
+CERT_FILE = os.path.join(PKI, 'certs/vdsmcert.pem')
+KEY_FILE = os.path.join(PKI, 'keys/vdsmkey.pem')
+LS_CERT_DIR = os.path.join(PKI, 'libvirt-spice')
+
+VDSMM_CONF = os.path.join(SYSCONF_PATH, '/etc/vdsm/vdsm.conf')
+
+# Libvirt daemon configuration
 LCONF = os.path.join(SYSCONF_PATH, '/etc/libvirt/libvirtd.conf')

+LCONF_GENERAL = {
+    'listen_addr': '"0.0.0.0"',
+    'unix_sock_group': '"qemu"',
+    'unix_sock_rw_perms': '"0770"',
+    'auth_unix_rw': '"sasl"',
+    'host_uuid': uuid.uuid4(),
+    'keepalive_interval': -1,
+    # FIXME until we are confident with libvirt integration,
+    # let us have a verbose log
+    'log_outputs': '"1:file:/var/log/libvirt/libvirtd.log"',
+    'log_filters': '"3:virobject 3:virfile 2:virnetlink \
+                    3:cgroup 3:event 3:json 1:libvirt 1:util 1:qemu"',
+}
+LCONF_SSL = {
+    'ca_file': '"' + CA_FILE + '"',
+    'cert_file': '"' + CERT_FILE + '"',
+    'key_file': '"' + KEY_FILE + '"',
+}
+LCONF_NO_SSL = {
+    'auth_tcp': '"none"',
+    'listen_tcp': 1,
+    'listen_tls': 0,
+}
+
+# qemu configuration
 QCONF = os.path.join(SYSCONF_PATH, 'libvirt/qemu.conf')

+QCONF_GENERAL = {
+    'dynamic_ownership': 0,
+    'save_image_format': '"lzop"',
+    'remote_display_port_min': 5900,
+    'remote_display_port_max': 6923,
+    'auto_dump_path': "/var/log/core",
+}
+
+QCONF_NO_SELINUX = {
+    'security_driver': '"none"',
+}
+
+QCONF_SANLOCK = {
+    'lock_manager': '"sanlock"'
+}
+
+QCONF_SSL = {
+    'spice_tls': 1
+}
+
+QCONF_NO_SSL = {
+    'spice_tls': 0
+}
+
+QCONF_SSL_CERTS = {
+    'spice_tls_x509_cert_dir': '"' + LS_CERT_DIR + '"'
+}
+
+# libvirt sysconfig file
 LDCONF = os.path.join(SYSCONF_PATH, '/sysconfig/libvirtd')

+LDCONF_GENERAL = {
+    'LIBVIRTD_ARGS': '--listen',
+    'DAEMON_COREFILE_LIMIT': 'unlimited',
+}
+
+# sanlock configuration file
 QLCONF = os.path.join(SYSCONF_PATH, 'libvirt/qemu-sanlock.conf')
+
+QLCONF_SANLOCK = {
+    'auto_disk_leases': 0,
+    'require_lease_for_disks': 0,
+}
+
+# libvirt log rotate configuration
+LRCONF = os.path.join(SYSCONF_PATH, '/etc/logrotate.d/libvirtd')
+
+LLOGR_CONF = """
+/var/log/libvirt/libvirtd.log {
+    rotate 100
+    missingok
+    copytruncate
+    size 15M
+    compress
+    compresscmd /usr/bin/xz
+    uncompresscmd /usr/bin/unxz
+    compressext .xz
+}
+"""

class _ModuleConfigure(object):
@@ -114,7 +213,58 @@
         return (os.path.join(P_VDSM_EXEC, 'libvirt_configure.sh'), )

def configure(self):
-        self._exec_libvirt_configure("reconfigure")
+        self.libvirtd_sysv2upstart()
+        if utils.isOvirtNode():
+            # TODO mtayer: Move the existance check to validate_ovirt_certs?
+            if not os.path.exists(P_VDSM_CERT):
+                raise RuntimeError(
+                    "vdsm: Missing certificate, vdsm not registered")
+            validate_ovirt_certs.validate_ovirt_certs()
+        # Remove a previous configuration (if present)
+        self.removeConf()
+        lconf_maps = [LCONF_GENERAL]
+        qconf_maps = [QCONF_GENERAL]
+        ldconf_maps = [LDCONF_GENERAL]
+        qlconf_maps = []
+        # determine configuration
+        config.read(VDSMM_CONF)
+        if config.getboolean('vars', 'ssl'):
+            qconf_maps.append(QCONF_SSL)
+            if all(os.path.isfile(f) for f in
+                   [CA_FILE, CERT_FILE, KEY_FILE]):
+                lconf_maps.append(LCONF_SSL)
+                qconf_maps.append(QCONF_SSL_CERTS)
+            else:
+                lconf_maps.append(LCONF_NO_SSL)
+        else:
+            qconf_maps.append(QCONF_NO_SSL)
+            lconf_maps.append(LCONF_NO_SSL)
+        if SANLOCK_ENABLED:
+            qconf_maps.append(QCONF_SANLOCK)
+            qlconf_maps.append(QLCONF_SANLOCK)
+        if not LIBVIRT_SELINUX:
+            qconf_maps.append(QCONF_NO_SELINUX)
+
+        # write configuration
+        for file_name, configuration_maps in \
+                [(LCONF, lconf_maps), (QCONF, qconf_maps),
+                 (LDCONF, ldconf_maps), (QLCONF, qlconf_maps)]:
+            with ConfigFile(file_name,
+                            '-'.join((CONF_PREFIX, CONF_VER)),
+                            '-'.join((CONF_SUFFIX, CONF_VER))) as conff:
+                for key, val in dict(configuration_maps):
+                    conff.addEntry(key, val)
+
+        os.remove('/etc/libvirt/qemu/networks/autostart/default.xml')
+
+        with ConfigFile(LRCONF, CONF_PREFIX, CONF_SUFFIX) as conf:
+            conf.prefixLines('# VDSM backup')
+            conf.prependSection(LLOGR_CONF)
+
+        for fname in (LCONF, QCONF, LDCONF, QLCONF, LRCONF):
+            if utils.isOvirtNode() and ovirtfunctions:
+                ovirtfunctions.ovirt_store_config(fname)
+        sys.stdout.write("Reconfiguration of libvirt is done.")

def libvirtd_sysv2upstart(self):
         """
@@ -178,7 +328,7 @@
                     conff.removeConf()

def _get_conf_files(self):
-        return LCONF, QCONF, LDCONF, QLCONF
+        return LCONF, QCONF, QLCONF, LDCONF

class SanlockModuleConfigure(_ModuleConfigure):
diff --git a/lib/vdsm/utils.py b/lib/vdsm/utils.py
index cd178da..b1ed86c 100644
--- a/lib/vdsm/utils.py
+++ b/lib/vdsm/utils.py
@@ -1149,6 +1149,14 @@
     sys.exit(-3)

+@memoized
+def isOvirtNode():
+    return (
+        os.path.exists('/etc/rhev-hypervisor-release') or
+        glob.glob('/etc/ovirt-node-*-release')
+    )
+
+
 # Copied from
 # http://docs.python.org/2.6/library/itertools.html?highlight=grouper#recipes
 def grouper(iterable, n, fillvalue=None):
diff --git a/tests/toolTests.py b/tests/toolTests.py
index 8752a99..cfa7ed5 100644
--- a/tests/toolTests.py
+++ b/tests/toolTests.py
@@ -172,22 +172,6 @@
         self._setConfig('LCONF', 'empty')
         self.assertFalse(libvirtConfigure.isconfigured())

-    def testLibvirtConfigureToSSLTrue(self):
-        libvirtConfigure = configurator.LibvirtModuleConfigure(test_env)
-        self._setConfig('LCONF', 'empty')
-        self._setConfig('VDSM_CONF_FILE', 'withssl')
-        self.assertFalse(libvirtConfigure.isconfigured())
-        libvirtConfigure.configure()
-        self.assertTrue(libvirtConfigure.isconfigured())
-
-    def testLibvirtConfigureToSSLFalse(self):
-        libvirtConfigure = configurator.LibvirtModuleConfigure(test_env)
-        self._setConfig('LCONF', 'empty')
-        self._setConfig('VDSM_CONF_FILE', 'withnossl')
-        self.assertFalse(libvirtConfigure.isconfigured())
-        libvirtConfigure.configure()
-        self.assertTrue(libvirtConfigure.isconfigured())
-

class ConfigFileTests(TestCase):
     def setUp(self):

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: configure verb: replace libvirt_configure.sh call with python code.
......................................................................

Patch Set 1:

Build Failed

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

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

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: configure verb: replace libvirt_configure.sh call with python code.
......................................................................

Patch Set 2:

Build Successful

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

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

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

Yaniv Bronhaim has posted comments on this change.

Change subject: configure verb: replace libvirt_configure.sh call with python code.
......................................................................

Patch Set 2: Code-Review-1

(11 comments)

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

Line 141:     uncompresscmd /usr/bin/unxz
Line 142:     compressext .xz
Line 143: }
Line 144: """
Line 145: 
all of only libvirt related constant put under LibvirtModuleConfigure and not global
Line 146: 
Line 147: class _ModuleConfigure(object):
Line 148:     def __init__(self):
Line 149:         pass

Line 210:             raise RuntimeError("Failed to perform libvirt action.")
Line 211: 
Line 212:     def _get_libvirt_exec(self):
Line 213:         return (os.path.join(P_VDSM_EXEC, 'libvirt_configure.sh'), )
Line 214: 
you can remove _get_libvirt_exec and _exec_libvirt_configure ^^
Line 215:     def configure(self):
Line 216:         self.libvirtd_sysv2upstart()
Line 217:         if utils.isOvirtNode():
Line 218:             # TODO mtayer: Move the existance check to validate_ovirt_certs?

Line 220:                 raise RuntimeError(
Line 221:                     "vdsm: Missing certificate, vdsm not registered")
Line 222:             validate_ovirt_certs.validate_ovirt_certs()
Line 223:         # Remove a previous configuration (if present)
Line 224:         self.removeConf()
always remove?
Line 225:         lconf_maps = [LCONF_GENERAL]
Line 226:         qconf_maps = [QCONF_GENERAL]
Line 227:         ldconf_maps = [LDCONF_GENERAL]
Line 228:         qlconf_maps = []

Line 225:         lconf_maps = [LCONF_GENERAL]
Line 226:         qconf_maps = [QCONF_GENERAL]
Line 227:         ldconf_maps = [LDCONF_GENERAL]
Line 228:         qlconf_maps = []
Line 229:         # determine configuration
this is the valid part, put it on validate and call it
Line 230:         config.read(VDSMM_CONF)
Line 231:         if config.getboolean('vars', 'ssl'):
Line 232:             qconf_maps.append(QCONF_SSL)
Line 233:             if all(os.path.isfile(f) for f in

Line 265:             if utils.isOvirtNode() and ovirtfunctions:
Line 266:                 ovirtfunctions.ovirt_store_config(fname)
Line 267:         sys.stdout.write("Reconfiguration of libvirt is done.")
Line 268: 
Line 269:     def libvirtd_sysv2upstart(self):
i prefer - switchSysvHandlingToUpstart
Line 270:         """
Line 271:         On RHEL 6, libvirtd can be started by either SysV init or Upstart.
Line 272:         We prefer upstart because it respawns libvirtd if when libvirtd
Line 273:         crashed.

Line 297: "reload-configuration"))
we really need to call another script here? can't we avoid it by python's code?

Line 318:         try:
Line 319:             self._exec_libvirt_configure("check_if_configured")
Line 320:             return True
Line 321:         except RuntimeError:
Line 322:             return False
this is easy to check with the config tool ^
Line 323: 
Line 324:     def removeConf(self):
Line 325:         for path in self._get_conf_files():
Line 326:             if os.path.exists(path):

Line 327:                 with ConfigFile(path, CONF_PREFIX, CONF_SUFFIX) as conff:
Line 328:                     conff.removeConf()
Line 329: 
Line 330:     def _get_conf_files(self):
Line 331:         return LCONF, QCONF, QLCONF, LDCONF
lets keep standard with the function names "getConfFiles"
Line 332: 
Line 333: 
Line 334: class SanlockModuleConfigure(_ModuleConfigure):
Line 335:

Line 328:                     conff.removeConf()
Line 329: 
Line 330:     def _get_conf_files(self):
Line 331:         return LCONF, QCONF, QLCONF, LDCONF
Line 332: 
why not explicitly use the file names? why do you need the constants ?
Line 333: 
Line 334: class SanlockModuleConfigure(_ModuleConfigure):
Line 335: 
Line 336:     SANLOCK_GROUPS = (QEMU_PROCESS_GROUP, VDSM_GROUP)

http://gerrit.ovirt.org/#/c/27130/2/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 1154:     return (
Line 1155:         os.path.exists('/etc/rhev-hypervisor-release') or
Line 1156:         glob.glob('/etc/ovirt-node-*-release')
Line 1157:     )
Line 1158: 
no rejects for that? i recall many rejections, and don't remember why
Line 1159: 
Line 1160: # Copied from
Line 1161: # http://docs.python.org/2.6/library/itertools.html?highlight=grouper#recipes
Line 1162: def grouper(iterable, n, fillvalue=None):

http://gerrit.ovirt.org/#/c/27130/2/tests/toolTests.py
File tests/toolTests.py:

Line 185
Line 186
Line 187
Line 188
Line 189
removing this help for something? why not leaving it here?

mooli tayer has posted comments on this change.

Change subject: configure verb: replace libvirt_configure.sh call with python code.
......................................................................

Patch Set 2:

(9 comments)

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

Line 141:     uncompresscmd /usr/bin/unxz
Line 142:     compressext .xz
Line 143: }
Line 144: """
Line 145:
> all of only libvirt related constant put under LibvirtModuleConfigure and n

ok
Line 146: 
Line 147: class _ModuleConfigure(object):
Line 148:     def __init__(self):
Line 149:         pass

Line 210:             raise RuntimeError("Failed to perform libvirt action.")
Line 211: 
Line 212:     def _get_libvirt_exec(self):
Line 213:         return (os.path.join(P_VDSM_EXEC, 'libvirt_configure.sh'), )
Line 214:
> you can remove _get_libvirt_exec and _exec_libvirt_configure ^^

ok so I will join all the verbs this patch and then I can drop the two methods and configura_libvier.sh and all its uses.
Line 215:     def configure(self):
Line 216:         self.libvirtd_sysv2upstart()
Line 217:         if utils.isOvirtNode():
Line 218:             # TODO mtayer: Move the existance check to validate_ovirt_certs?

Line 220:                 raise RuntimeError(
Line 221:                     "vdsm: Missing certificate, vdsm not registered")
Line 222:             validate_ovirt_certs.validate_ovirt_certs()
Line 223:         # Remove a previous configuration (if present)
Line 224:         self.removeConf()
> always remove?

that's what the script does.
do you think we should test is configured?

any way I think we should have one  "replace sh with py" patch and things that change behavior in dependant commits
Line 225:         lconf_maps = [LCONF_GENERAL]
Line 226:         qconf_maps = [QCONF_GENERAL]
Line 227:         ldconf_maps = [LDCONF_GENERAL]
Line 228:         qlconf_maps = []

Line 225:         lconf_maps = [LCONF_GENERAL]
Line 226:         qconf_maps = [QCONF_GENERAL]
Line 227:         ldconf_maps = [LDCONF_GENERAL]
Line 228:         qlconf_maps = []
Line 229:         # determine configuration
> this is the valid part, put it on validate and call it

no it is the configure part, 
I build the configuration values according to vdsm conf.
Line 230:         config.read(VDSMM_CONF)
Line 231:         if config.getboolean('vars', 'ssl'):
Line 232:             qconf_maps.append(QCONF_SSL)
Line 233:             if all(os.path.isfile(f) for f in

Line 265:             if utils.isOvirtNode() and ovirtfunctions:
Line 266:                 ovirtfunctions.ovirt_store_config(fname)
Line 267:         sys.stdout.write("Reconfiguration of libvirt is done.")
Line 268: 
Line 269:     def libvirtd_sysv2upstart(self):
> i prefer - switchSysvHandlingToUpstart

sysvToUpstart ?
Line 270:         """
Line 271:         On RHEL 6, libvirtd can be started by either SysV init or Upstart.
Line 272:         We prefer upstart because it respawns libvirtd if when libvirtd
Line 273:         crashed.

Line 297: "reload-configuration"))
> we really need to call another script here? can't we avoid it by python's c

Could not find any bindings. our service.py could not be used
 either(it does not have the "reload-configuration verb" and
 it is meant to be init system agnostic and here we want to
 specifically call Upstart).

Line 318:         try:
Line 319:             self._exec_libvirt_configure("check_if_configured")
Line 320:             return True
Line 321:         except RuntimeError:
Line 322:             return False
> this is easy to check with the config tool ^

will do now that i'm replacing the verbs in this patch
Line 323: 
Line 324:     def removeConf(self):
Line 325:         for path in self._get_conf_files():
Line 326:             if os.path.exists(path):

Line 328:                     conff.removeConf()
Line 329: 
Line 330:     def _get_conf_files(self):
Line 331:         return LCONF, QCONF, QLCONF, LDCONF
Line 332:
> why not explicitly use the file names? why do you need the constants ?

This function is going to be removed (it is only needed for
 old way for testing.) I will convert all other new ones to
 camelCase and old ones in a different patch
Line 333: 
Line 334: class SanlockModuleConfigure(_ModuleConfigure):
Line 335: 
Line 336:     SANLOCK_GROUPS = (QEMU_PROCESS_GROUP, VDSM_GROUP)

http://gerrit.ovirt.org/#/c/27130/2/tests/toolTests.py
File tests/toolTests.py:

Line 185
Line 186
Line 187
Line 188
Line 189
> removing this help for something? why not leaving it here?

These tests now fail. 
 There is a problem in testing complete configure now due to
 operations the we don't want to do in testing.

I don't like the skipOnTest approach. what I try to do is
 crate utilities and unit test them.

Testing entire flows is also important in this case so I will
 try to create a new test using mocks.

mooli tayer has abandoned this change.

Change subject: configure verb: replace libvirt_configure.sh call with python code.
......................................................................

Abandoned

squashed with http://gerrit.ovirt.org/#/c/27298/1