Change in vdsm[master]: profiling: Add whole application profile

[Nir Soffer]

Nir Soffer has uploaded a new change for review.

Change subject: profiling: Add whole application profile
......................................................................

profiling: Add whole application profile

The built in Python profilers are good for single thread profile but
given the excessive multi-threaded nature of vdsm, they are useless for
understing where we spend our time.

This patch uses yappi <http://code.google.com/p/yappi> to create whole
application profile. A new profile_enable configuration is added to
enable profiling.

Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Relates-to: https://bugzilla.redhat.com/1074097
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt;
---
M lib/vdsm/config.py.in
M vdsm/vdsm
2 files changed, 14 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/13/26113/1

diff --git a/lib/vdsm/config.py.in b/lib/vdsm/config.py.in
index fb597a8..e208b2a 100644
--- a/lib/vdsm/config.py.in
+++ b/lib/vdsm/config.py.in
@@ -29,6 +29,9 @@
         ('core_dump_enable', 'true',
             'Enable core dump.'),
 
+        ('profile_enable', 'false',
+            'Enable whole process profiling (requires yappi profiler).'),
+
         ('host_mem_reserve', '256',
             'Reserves memory for the host to prevent VMs from using all the '
             'physical pages. The values are in Mbytes.'),
diff --git a/vdsm/vdsm b/vdsm/vdsm
index 652797c..f89ca1d 100755
--- a/vdsm/vdsm
+++ b/vdsm/vdsm
@@ -65,6 +65,11 @@
     signal.signal(signal.SIGUSR1, sigusr1Handler)
     zombiereaper.registerSignalHandler()
 
+    if config.getboolean('vars', 'profile_enable'):
+        logging.debug("Starting profiling")
+        import yappi
+        yappi.start()
+
     libvirtconnection.start_event_loop()
 
     if config.getboolean('irs', 'irs_enable'):
@@ -79,6 +84,12 @@
     try:
         while running[0]:
             signal.pause()
+
+        if config.getboolean('vars', 'profile_enable'):
+            logging.debug("Stopping profiling")
+            yappi.stop()
+            stats = yappi.get_func_stats()
+            stats.save('/tmp/vdsmd.prof', 'pstat')
     finally:
         cif.prepareForShutdown()
 


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 10:

Build Successful 

http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/547/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: profiling: Add an application wide profile
......................................................................


profiling: Add an application wide profile

The built in Python profilers are good for single thread profile but
given the excessive multi-threaded nature of vdsm, they are useless for
understing where we spend our time.

This patch uses yappi <http://code.google.com/p/yappi> to create an
application wide profile. New configuration options added to enable
profiling and selecting profile file format.

The default file format is PSTAT, which can be used with the builtin
pstats module. Another interesting format is CALLGRIND, used by
KCachegrind visualization tool.

Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Relates-to: https://bugzilla.redhat.com/1074097
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt;
Reviewed-on: http://gerrit.ovirt.org/26113
Reviewed-by: Francesco Romani <fromani(a)redhat.com&gt;
Reviewed-by: Adam Litke <alitke(a)redhat.com&gt;
Reviewed-by: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt;
---
M debian/vdsm-python.install
M lib/vdsm/Makefile.am
M lib/vdsm/config.py.in
A lib/vdsm/profile.py
M vdsm.spec.in
M vdsm/vdsm
6 files changed, 71 insertions(+), 0 deletions(-)

Approvals:
  Douglas Schilling Landgraf: Looks good to me, but someone else must approve
  Nir Soffer: Verified
  Adam Litke: Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved
  Francesco Romani: Looks good to me, but someone else must approve



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: profiling: Add an application wide profile

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9: Code-Review+2

Escalating score.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9:

Escalating score.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9: Code-Review+1

(1 comment)

http://gerrit.ovirt.org/#/c/26113/9/lib/vdsm/profile.py
File lib/vdsm/profile.py:

Line 35: def start():
Line 36:     global yappi
Line 37:     if is_enabled():
Line 38:         logging.debug("Starting profiling")
Line 39:         import yappi
>  The config specify clearly: 
I see and assuming this feature is
for developers it's ok. Other approach is not stop vdsm and log a error since profile
is not crucial to make vdsm running.
Line 40:         yappi.start()
Line 41: 
Line 42: 
Line 43: def stop():


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.ovirt.org/#/c/26113/9/lib/vdsm/profile.py
File lib/vdsm/profile.py:

Line 35: def start():
Line 36:     global yappi
Line 37:     if is_enabled():
Line 38:         logging.debug("Starting profiling")
Line 39:         import yappi
>  shouldn't we use try: import yappi ...? Users can enable it
> without the pac 
The config specify clearly:

    # Enable whole process profiling (requires yappi profiler).
    # profile_enable = false

If you ignore the note about yappi, (you know better) and enable the profile, vdsm will
fail to start and you will get this traceback in the log:

  MainThread::ERROR::2014-05-02 22:27:56,152::vdsm::132::vds::(run) Exception raised
  Traceback (most recent call last):
    File "/usr/share/vdsm/vdsm", line 130, in run
      serve_clients(log)
    File "/usr/share/vdsm/vdsm", line 69, in serve_clients
      profile.start()
    File "/usr/lib64/python2.7/site-packages/vdsm/profile.py", line 39, in
start
      import yappi
  ImportError: No module named yappi

How do you suggest to handle it differently?

In the future, when we have a yappi package, we can require yappi in vdsm spec.
Line 40:         yappi.start()
Line 41: 
Line 42: 
Line 43: def stop():


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: profiling: Add an application wide profile

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.ovirt.org/#/c/26113/9/lib/vdsm/profile.py
File lib/vdsm/profile.py:

Line 35: def start():
Line 36:     global yappi
Line 37:     if is_enabled():
Line 38:         logging.debug("Starting profiling")
Line 39:         import yappi
shouldn't we use try: import yappi ...? Users can enable it without the package
installed in the system.
Line 40:         yappi.start()
Line 41: 
Line 42: 
Line 43: def stop():


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: profiling: Add an application wide profile

Adam Litke has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9:

+1 for the initiative! I'll reviw this later. Thanks Nir.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9:

Build Failed 

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

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

http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/542/ : SUCCESS

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

Francesco Romani has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9: Code-Review+1

I was thinking to avoid to the blowing up, by coercing an unexpected value, like
'junk' in your example, to a sane default.
However, on second thought, the current approach is better;
moreover, config.get*() is used directly (without safety nets or input checks) all over
the place in the code, so the patch is good as it is.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9:

Build Failed 

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

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

http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/542/ : SUCCESS

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

Francesco Romani has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.ovirt.org/#/c/26113/9/lib/vdsm/profile.py
File lib/vdsm/profile.py:

Line 45:         logging.debug("Stopping profiling")
Line 46:         yappi.stop()
Line 47:         stats = yappi.get_func_stats()
Line 48:         path = os.path.join(constants.P_VDSM_RUN, 'vdsmd.prof')
Line 49:         stats.save(path, config.get('vars', 'profile_format'))
As personal taste, I'd like to see the input sanitized here.
Line 50: 
Line 51: 
Line 52: def is_enabled():
Line 53:     return config.getboolean('vars', 'profile_enable')


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9:

Build Failed 

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

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

http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/542/ : SUCCESS

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9:

The tests failures are not relevant (segfault in Python while running libvirt test).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9:

Build Failed 

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

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

http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/542/ : SUCCESS

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9:

Build Failed 

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

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

http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/542/ : SUCCESS

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9:

Build Failed 

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

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

http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/542/ : SUCCESS

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9:

Build Failed 

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

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

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

http://jenkins.ovirt.org/job/vdsm_master_install_rpm_sanity_gerrit/542/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9: Verified+1

This patch add the new profile module to the distribution and fix a missing import.

I verified on RHEL 6.5 and Fedora 19 that vdsm runs with profile enabled and disabled and
that the generated profile is valid.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 8:

Build Successful 

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

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 8:

Changes:

- Move profiling details to profile module
- Address Saggi comments regarding stopping the profiler and building profile filename

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 7:

Build Failed 

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

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

Yaniv Bronhaim has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 2: -Code-Review

removing my score. i suggest to start the packaging procedure anyway

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 6:

After talking with Saggi, we agreed to:
- Move profiling logic to separate module under lib/vdsm/profile.py
- Change stop logic so profiler is stopped if it is running, not if yappi was imported.
  - Internally, yappi will still be imported lazily when profiling starts

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.ovirt.org/#/c/26113/6/vdsm/vdsm
File vdsm/vdsm:

Line 84:     try:
Line 85:         while running[0]:
Line 86:             signal.pause()
Line 87: 
Line 88:         if config.getboolean('vars', 'profile_enable'):
>  Right - it is cleaner. 
But wrong :-) since I import yappi only
if enabled.

I do this for two reasons. First, yappi is probably not available, and second, even if we
have yappi, I prefer not to import it if we don't need it, to ensure that we do not
effect the stability of the system.

Checking the config is equal to yappi.is_running() in the way we use it currently.

We can clean this up if I'll move the profiling code to another module, so in vdsm we
will see:

 from vdsm import prof
 ...
 prof.start_profiling(path)
 ...
 prof.stop_profiling()

And the logic of importing yappi (or another profiler) will be encapsulated in the profile
module.

What do you think?
Line 89:             logging.debug("Stopping profiling")
Line 90:             yappi.stop()
Line 91:             stats = yappi.get_func_stats()
Line 92:             stats.save(constants.P_VDSM_RUN + 'vdsmd.prof',


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 6:

(2 comments)

Thanks, will clean up.

http://gerrit.ovirt.org/#/c/26113/6/vdsm/vdsm
File vdsm/vdsm:

Line 84:     try:
Line 85:         while running[0]:
Line 86:             signal.pause()
Line 87: 
Line 88:         if config.getboolean('vars', 'profile_enable'):
>  I assume you mean to check: 
Right - it is cleaner.
Line 89:             logging.debug("Stopping profiling")
Line 90:             yappi.stop()
Line 91:             stats = yappi.get_func_stats()
Line 92:             stats.save(constants.P_VDSM_RUN + 'vdsmd.prof',


Line 88:         if config.getboolean('vars', 'profile_enable'):
Line 89:             logging.debug("Stopping profiling")
Line 90:             yappi.stop()
Line 91:             stats = yappi.get_func_stats()
Line 92:             stats.save(constants.P_VDSM_RUN + 'vdsmd.prof',
>  use os.path.join() You don't want to handle missing trailing
> "/" problems 
Sure
Line 93:                        config.get('vars', 'profile_format'))
Line 94:     finally:
Line 95:         cif.prepareForShutdown()
Line 96: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: profiling: Add an application wide profile

Saggi Mizrahi has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 6: Code-Review-1

(2 comments)

http://gerrit.ovirt.org/#/c/26113/6/vdsm/vdsm
File vdsm/vdsm:

Line 88: ',
I assume you mean to check:
yappi.is_running()

Since you want to close it if it's running and not if it was enabled in the config.


Line 88:         if config.getboolean('vars', 'profile_enable'):
Line 89:             logging.debug("Stopping profiling")
Line 90:             yappi.stop()
Line 91:             stats = yappi.get_func_stats()
Line 92:             stats.save(constants.P_VDSM_RUN + 'vdsmd.prof',
use os.path.join() You don't want to handle missing trailing "/" problems
Line 93:                        config.get('vars', 'profile_format'))
Line 94:     finally:
Line 95:         cif.prepareForShutdown()
Line 96: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 6:

Build Successful 

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

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 6:

Rebase on dependent patch.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 5:

Rebased over dependent patch.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 5:

Build Successful 

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

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 4:

Rebased over dependent patch. Verified on RHEL 6.5 and Fedora 19.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 4:

Build Successful 

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

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 3: Verified+1

Verified running with and without profiling.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 3:

Added profile_format option.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 3:

Build Successful 

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

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 2:

There is no such package, and it will take ages until we can get it into Fedora/EL. So
either we ship it in the ovirt repositories, or it is an optional package. This is why the
help says "Requires yappi profiler".

Since this is development tool, I think it is good enough for now.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

Yaniv Bronhaim has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 2: Code-Review-1

you must to have yappi package on your local setup when running vdsm, and then we need to
require it in vdsm.spec before taking it to in

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

Yaniv Bronhaim has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 2:

Build Successful 

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

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 2: Verified+1

Move profile to /var/run/vdsm, protecting from symlink attacks.

Verified by running with and without profiling and starting and stopping a vm with
profiling.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add whole application profile
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/26113/1/vdsm/vdsm
File vdsm/vdsm:

Line 88:         if config.getboolean('vars', 'profile_enable'):
Line 89:             logging.debug("Stopping profiling")
Line 90:             yappi.stop()
Line 91:             stats = yappi.get_func_stats()
Line 92:             stats.save('/tmp/vdsmd.prof', 'pstat')
>  this would open the door for a symlink attack. Please use
> /var/run/vdsm for 
good point
Line 93:     finally:
Line 94:         cif.prepareForShutdown()
Line 95: 
Line 96: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: profiling: Add whole application profile
......................................................................


Patch Set 1: Code-Review-1

(1 comment)

http://gerrit.ovirt.org/#/c/26113/1/vdsm/vdsm
File vdsm/vdsm:

Line 88:         if config.getboolean('vars', 'profile_enable'):
Line 89:             logging.debug("Stopping profiling")
Line 90:             yappi.stop()
Line 91:             stats = yappi.get_func_stats()
Line 92:             stats.save('/tmp/vdsmd.prof', 'pstat')
this would open the door for a symlink attack. Please use /var/run/vdsm for writing
vdsm-specific temporary data.
Line 93:     finally:
Line 94:         cif.prepareForShutdown()
Line 95: 
Line 96: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

Francesco Romani has posted comments on this change.

Change subject: profiling: Add whole application profile
......................................................................


Patch Set 1: Code-Review+1

Definitely more polished and more refined solution of what I did so far
Looks fine, and since it is self contained and not intrusive I think we should merge
this.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add whole application profile
......................................................................


Patch Set 1: Verified+1

Verified running with and without the profiler and displaying profiler rsults using
pstats.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: profiling: Add whole application profile
......................................................................


Patch Set 1:

Build Successful 

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

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

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Change in vdsm[master]: profiling: Add an application wide profile

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: profiling: Add an application wide profile
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.ovirt.org/#/c/26113/9/lib/vdsm/profile.py
File lib/vdsm/profile.py:

Line 45:         logging.debug("Stopping profiling")
Line 46:         yappi.stop()
Line 47:         stats = yappi.get_func_stats()
Line 48:         path = os.path.join(constants.P_VDSM_RUN, 'vdsmd.prof')
Line 49:         stats.save(path, config.get('vars', 'profile_format'))
>  As personal taste, I'd like to see the input sanitized here.
This is done by yappi, and you will get an exception when saving. In this example i
used this configuration:

  # Profile file format (pstat, callgrind, ystat)
  profile_format = junk

Note the list of valid values in the comment. But I was brave and decided to use the
"junk" format:

  MainThread::ERROR::2014-05-02 13:09:55,248::vdsm::132::vds::(run) Exception raised
  Traceback (most recent call last):
    File "/usr/share/vdsm/vdsm", line 130, in run
      serve_clients(log)
    File "/usr/share/vdsm/vdsm", line 86, in serve_clients
      profile.stop()
    File "/usr/lib64/python2.7/site-packages/vdsm/profile.py", line 49, in stop
      stats.save(path, config.get('vars', 'profile_format'))
    File "/usr/lib64/python2.7/site-packages/yappi.py", line 558, in save
      raise NotImplementedError('Saving in "%s" format is not possible
currently.' % (type))
  NotImplementedError: Saving in "JUNK" format is not possible currently.

I think this is similar to other configuration values - use bad value and the system
blows.

Can you elaborate on how to improve this?
Line 50: 
Line 51: 
Line 52: def is_enabled():
Line 53:     return config.getboolean('vars', 'profile_enable')


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523f52c981f7bb34f3168d3117f00ed5eb6962f8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes