Change in vdsm[master]: API: streamline and make setLogLevel correct

Francesco Romani has uploaded a new change for review.

Change subject: API: streamline and make setLogLevel correct
......................................................................

API: streamline and make setLogLevel correct

According to the schema, setLogLevel should accept
a log level string ('DEBUG', 'INFO',...). But the code
actually blindly casted the given value to int(), and this suggests
the code actually expected an integer value.

To make things a bit user friendlier and comformant to schema, change
the argument to actually be a log level in string format.

Along the way, this patch streamlines the implementation of setLogLevel
and makes it simpler.

Change-Id: Iaddbb12d13bdbaa7a02255ab209da11e42d2ea97
Signed-off-by: Francesco Romani 
---
M vdsm/API.py
1 file changed, 21 insertions(+), 6 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/25/38425/1

diff --git a/vdsm/API.py b/vdsm/API.py
index 5cbda50..85478b1 100644
--- a/vdsm/API.py
+++ b/vdsm/API.py
@@ -1369,13 +1369,22 @@

Doesn't survive a restart
         """
-        logging.info('Setting loglevel to %s', level)
-        handlers = logging.getLogger().handlers
-        [fileHandler] = [h for h in handlers if
-                         isinstance(h, logging.FileHandler)]
-        fileHandler.setLevel(int(level))
+        LEVELS = {
+            'DEBUG': logging.DEBUG,
+            'INFO': logging.INFO,
+            'WARNING': logging.WARNING,
+            'ERROR': logging.ERROR,
+            'CRITICAL': logging.CRITICAL
+        }

-        return {'status': doneCode}
+        try:
+            log_level = LEVELS[level]
+        except KeyError:
+            return errCode['unavail']
+        else:
+            logging.info('Setting loglevel to %s (%d)', level, log_level)
+            _set_log_level(logging.getLogger(), log_level)
+            return {'status': doneCode}

# VM-related functions
     def getVMList(self, fullStatus=False, vmList=()):
@@ -1772,3 +1781,9 @@
                 logging.warn("options %s is deprecated. Use %s instead" %
                              (k, _translationMap[k]))
                 options[_translationMap[k]] = options.pop(k)
+
+
+def _set_log_level(logger, log_level):
+    for handler in logger.handlers:
+        if isinstance(handler, logging.FileHandler):
+            handler.setLevel(log_level)

[e-mail] has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 1:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 1:

Build Failed

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

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

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/16474/ : FAILURE

[e-mail] has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 2:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 2:

Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/18355/

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 2:

Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1585/

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 2: Verified-1

Build Failed

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

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1585/ : FAILURE

[e-mail] has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 3:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[e-mail] has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 4:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[e-mail] has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 5:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[e-mail] has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 6:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[e-mail] has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 7:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[e-mail] has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 8:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Michal Skrivanek has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 8: Code-Review+1

[e-mail] has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 9:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[e-mail] has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 10:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[e-mail] has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 11:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Francesco Romani has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 14:

(1 comment)

https://gerrit.ovirt.org/#/c/38425/14/vdsm/API.py
File vdsm/API.py:

Line 1379:             logging.warning('Setting loglevel to %s (%d)', level, log_level)
Line 1380:             [fileHandler] = [h for h in handlers if
Line 1381:                             isinstance(h, logging.FileHandler)]
Line 1382:             fileHandler.setLevel(int(level))
Line 1383:             return response.success()
> I think we should create lib/vdsm/logging module and keep API as a bridge c

rebasing and reordering the series to have the move first.
Line 1384:         else:
Line 1385:             return response.error('unavail')
Line 1386: 
Line 1387:     # VM-related functions

gerrit-hooks has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 15:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 15: Code-Review+1

Francesco Romani has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 15:

(1 comment)

https://gerrit.ovirt.org/#/c/38425/15/lib/vdsm/logUtils.py
File lib/vdsm/logUtils.py:

PS15, Line 214: RuntimeError
better: ValueError

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 15: Code-Review-1

(2 comments)

https://gerrit.ovirt.org/#/c/38425/15/lib/vdsm/logUtils.py
File lib/vdsm/logUtils.py:

PS15, Line 235: # for backward compatibility
nobody used this verb; let's break compat and accept ONLY string. And I would also be case insensitive.

This verb should match the current yaml.

https://gerrit.ovirt.org/#/c/38425/15/vdsm/API.py
File vdsm/API.py:

PS15, Line 1380: 'unavail'
'unavail' does not fit here, I'd rather not handle the exception at all.

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 15: -Code-Review

(1 comment)

https://gerrit.ovirt.org/#/c/38425/15/vdsm/API.py
File vdsm/API.py:

PS15, Line 1380: 'unavail'
> 'unavail' does not fit here, I'd rather not handle the exception at all.

I am not sure whether returning internal server error is better. It would be returned if unexpected error is handled by rpc code.

What about value error code here?

gerrit-hooks has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 16:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 16: Code-Review+1

gerrit-hooks has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 17:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

Francesco Romani has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 17: Verified+1

Verified both positive and negative flows:

Thread-111::DEBUG::2016-06-23 09:57:50,330::bindingxmlrpc::1245::vds::(wrapper) client [::1]::call setLogLevel with ('FOOBAR', '') {}
Thread-111::ERROR::2016-06-23 09:57:50,330::bindingxmlrpc::1270::vds::(wrapper) unexpected error
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/vdsm/rpc/bindingxmlrpc.py", line 1248, in wrapper
    res = f(*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/vdsm/rpc/bindingxmlrpc.py", line 649, in setLogLevel
    return api.setLogLevel(level, name)
  File "/usr/share/vdsm/API.py", line 1382, in setLogLevel
    logUtils.set_level(level, name)
  File "/usr/lib/python2.7/site-packages/vdsm/logUtils.py", line 237, in set_level
    raise ValueError("unknown log level: %r" % level)
ValueError: unknown log level: 'FOOBAR'
Thread-111::INFO::2016-06-23 09:57:50,332::xmlrpc::91::vds.XMLRPCServer::(_process_requests) Request handler for ::1:52199 stopped

Thread-114::DEBUG::2016-06-23 09:58:25,394::bindingxmlrpc::1245::vds::(wrapper) client [::1]::call setLogLevel with ('WARNING', '') {}
Thread-114::WARNING::2016-06-23 09:58:25,394::logUtils::243::root::(set_level) Setting loglevel on 'root' to WARNING (30)
Thread-114::DEBUG::2016-06-23 09:58:25,394::bindingxmlrpc::1253::vds::(wrapper) return setLogLevel with {'status': {'message': 'Done', 'code': 0}}

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 17: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/38425/17/lib/vdsm/logUtils.py
File lib/vdsm/logUtils.py:

PS17, Line 233: lev = level.upper()
let's be strict (as the schema!) and avoid this.

gerrit-hooks has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 18:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

gerrit-hooks has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 19:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

Francesco Romani has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 19: Verified+1

v19 fixes comments from Dan and extracts the mapping between names and levels into a private module constant.
Re-verified as before.

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 19: Code-Review+1

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 19: Code-Review+2

gerrit-hooks has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 20:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])

Francesco Romani has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 20: Verified+1

score lost because not-trivial rebase; copied.

[Piotr Kliczewski]

Piotr Kliczewski has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 20: Code-Review+1

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 20: Code-Review+2 Continuous-Integration+1

unrelated failure

FAIL: testGetDeviceByIP (network.netinfo_test.TestNetinfo)
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/home/jenkins/workspace/vdsm_master_check-patch-fc23-x86_64/vdsm/tests/network/netinfo_test.py", line 144, in testGetDeviceByIP
        addresses.getDeviceByIP(addr['address'].split('/')[0]))
    AssertionError: 'bond1' != 'eno2'

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: API: streamline and make setLogLevel correct
......................................................................

API: streamline and make setLogLevel correct

According to the schema, setLogLevel should accept
a log level string ('DEBUG', 'INFO',...). But the code
actually blindly casted the given value to int(), and this suggests
the code actually expected an integer value.

To make things a bit user friendlier and comformant to schema,
change the argument to actually be a log level in string format.

Along the way, this patch streamlines the implementation of
setLogLevel and makes it simpler.

Change-Id: Iaddbb12d13bdbaa7a02255ab209da11e42d2ea97
Backports-To: 4.0
Backports-To: 3.6
Signed-off-by: Francesco Romani 
Reviewed-on: https://gerrit.ovirt.org/38425
Reviewed-by: Piotr Kliczewski 
Reviewed-by: Dan Kenigsberg 
Continuous-Integration: Dan Kenigsberg 
---
M lib/vdsm/logUtils.py
1 file changed, 14 insertions(+), 1 deletion(-)

Approvals:
  Piotr Kliczewski: Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved; Passed CI tests
  Francesco Romani: Verified

Objections:
  Jenkins CI: Failed CI tests

gerrit-hooks has posted comments on this change.

Change subject: API: streamline and make setLogLevel correct
......................................................................

Patch Set 21:

* Update tracker: IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.