Change in vdsm[master]: config: introduce a devel section

Francesco Romani has uploaded a new change for review.

Change subject: config: introduce a devel section
......................................................................

config: introduce a devel section

This patch introduces a 'devel' configuration section,
in order to unclutter a bit the already crowded 'vars'
section.

An additional important benefit of this move is to clearly
mark that some tunables should never turned on in production
environments, unless exceptional circumstances, like a
performance problem being debugged.

For the same reason, is acceptable to move these options
across minor versions (e.g. 3.5 to 3.6).

This patch also makes room for future options which are
currently being discussed, like memory profiling or
a debug manhole service.

Change-Id: I7f9fc14c6fffd7ba111af985586eada83ec3c345
Signed-off-by: Francesco Romani 
---
M lib/vdsm/config.py.in
M lib/vdsm/profile.py
M tests/profileTests.py
3 files changed, 24 insertions(+), 18 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/64/32664/1

diff --git a/lib/vdsm/config.py.in b/lib/vdsm/config.py.in
index 69185e0..63b82d5 100644
--- a/lib/vdsm/config.py.in
+++ b/lib/vdsm/config.py.in
@@ -29,19 +29,6 @@
         ('core_dump_enable', 'true',
             'Enable core dump.'),

-        ('profile_enable', 'false',
-            'Enable whole process profiling (requires yappi profiler).'),
-
-        ('profile_format', 'pstat',
-            'Profile file format (pstat, callgrind, ystat)'),
-
-        ('profile_builtins', 'false',
-            'Profile builtin functions used by standard Python modules. '
-            'false by default.'),
-
-        ('profile_clock', 'cpu',
-            'Sets the underlying clock type (cpu, wall)'),
-
         ('host_mem_reserve', '256',
             'Reserves memory for the host to prevent VMs from using all the '
             'physical pages. The values are in Mbytes.'),
@@ -349,6 +336,25 @@
         ('guests_gateway_ip', '', None),

]),
+
+    # Section: [devel]
+    ('devel', [
+
+        ('profile_enable', 'false',
+            'Enable whole process profiling (requires yappi profiler).'),
+
+        ('profile_format', 'pstat',
+            'Profile file format (pstat, callgrind, ystat)'),
+
+        ('profile_builtins', 'false',
+            'Profile builtin functions used by standard Python modules. '
+            'false by default.'),
+
+        ('profile_clock', 'cpu',
+            'Sets the underlying clock type (cpu, wall)'),
+
+    ]),
+
 ]

diff --git a/lib/vdsm/profile.py b/lib/vdsm/profile.py
index 93dfacc..9710225 100644
--- a/lib/vdsm/profile.py
+++ b/lib/vdsm/profile.py
@@ -36,9 +36,9 @@
 # Defaults

_FILENAME = os.path.join(constants.P_VDSM_RUN, 'vdsmd.prof')
-_FORMAT = config.get('vars', 'profile_format')
-_BUILTINS = config.getboolean('vars', 'profile_builtins')
-_CLOCK = config.get('vars', 'profile_clock')
+_FORMAT = config.get('devel', 'profile_format')
+_BUILTINS = config.getboolean('devel', 'profile_builtins')
+_CLOCK = config.get('devel', 'profile_clock')
 _THREADS = True

_lock = threading.Lock()
@@ -61,7 +61,7 @@

def is_enabled():
-    return config.getboolean('vars', 'profile_enable')
+    return config.getboolean('devel', 'profile_enable')

def is_running():
diff --git a/tests/profileTests.py b/tests/profileTests.py
index 4e9ff7c..95bcfb1 100644
--- a/tests/profileTests.py
+++ b/tests/profileTests.py
@@ -44,7 +44,7 @@
 def make_config(enable='false'):
     cfg = ConfigParser.ConfigParser()
     config.set_defaults(cfg)
-    cfg.set('vars', 'profile_enable', enable)
+    cfg.set('devel', 'profile_enable', enable)
     return cfg

Antoni Segura Puimedon has posted comments on this change.

Change subject: config: introduce a devel section
......................................................................

Patch Set 1: Code-Review+1

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: config: introduce a devel section
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12345/ : SUCCESS

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

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

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: config: introduce a devel section
......................................................................

Patch Set 1:

(2 comments)

Looks fine but can do more - I think we can do all changes needed for additional profiler in one step, simplifying review, verification and even backporting.

http://gerrit.ovirt.org/#/c/32664/1//COMMIT_MSG
Commit Message:

Line 19: across minor versions (e.g. 3.5 to 3.6).
Line 20: 
Line 21: This patch also makes room for future options which are
Line 22: currently being discussed, like memory profiling or
Line 23: a debug manhole service.
To do this we much change the config names so we can have many profile_xxx_enable options,

And make the lib/vdsm/profile.py a package: lib/vdsm/profile/cpu.py
Line 24: 
Line 25: Change-Id: I7f9fc14c6fffd7ba111af985586eada83ec3c345

http://gerrit.ovirt.org/#/c/32664/1/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 339: 
Line 340:     # Section: [devel]
Line 341:     ('devel', [
Line 342: 
Line 343:         ('profile_enable', 'false',
Lests make room for the memory profiler by renaming all "profile_" configs to "profile_cpu". We can do this in another patch, but I don't a need.

This patch: make room for more profilers
Next patch: add memory profiler

What do you think?
Line 344:             'Enable whole process profiling (requires yappi profiler).'),
Line 345: 
Line 346:         ('profile_format', 'pstat',
Line 347:             'Profile file format (pstat, callgrind, ystat)'),

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: config: introduce a devel section
......................................................................

Patch Set 1: Code-Review+1

On the other hand, we can keep this small and add another patch for the package.

Francesco Romani has posted comments on this change.

Change subject: config: introduce a devel section
......................................................................

Patch Set 1:

I will implement Nir suggestion in a followup patch.

Francesco Romani has posted comments on this change.

Change subject: config: introduce a devel section
......................................................................

Patch Set 1: Verified+1

verified by enabling the profiler and running a few test runs.

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: config: introduce a devel section
......................................................................

config: introduce a devel section

This patch introduces a 'devel' configuration section,
in order to unclutter a bit the already crowded 'vars'
section.

An additional important benefit of this move is to clearly
mark that some tunables should never turned on in production
environments, unless exceptional circumstances, like a
performance problem being debugged.

For the same reason, is acceptable to move these options
across minor versions (e.g. 3.5 to 3.6).

This patch also makes room for future options which are
currently being discussed, like memory profiling or
a debug manhole service.

Change-Id: I7f9fc14c6fffd7ba111af985586eada83ec3c345
Signed-off-by: Francesco Romani 
Reviewed-on: http://gerrit.ovirt.org/32664
Reviewed-by: Antoni Segura Puimedon 
Reviewed-by: Nir Soffer 
Reviewed-by: Dan Kenigsberg 
---
M lib/vdsm/config.py.in
M lib/vdsm/profile.py
M tests/profileTests.py
3 files changed, 24 insertions(+), 18 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, but someone else must approve
  Antoni Segura Puimedon: Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved
  Francesco Romani: Verified

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: config: introduce a devel section
......................................................................

Patch Set 1: Code-Review+2

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: config: introduce a devel section
......................................................................

Patch Set 2:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged/1842/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged_test_debug/183/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5816/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/3975/ : SUCCESS