[checkmk-commits] Fix configuration input for f5_bigip_conns check

Module: check_mk Branch: master Commit: e1ad79bdae0d4e52d6ab73ae6f6eb259ecca3e50 URL: http://git.mathias-kettner.de/git/?p=check_mk.git;a=commit;h=e1ad79bdae0d4e… Author: Óscar Nájera <on(a)mathias-kettner.de> Date: Mon Nov 26 12:05:02 2018 +0100 Fix configuration input for f5_bigip_conns check In the F5 Loadbalance Connections there was an inconsistent use of configuration parameters being predictive levels but requiring for connection rates to use simple fixed levels tuples. This fix avoids the misconfiguration and warns the user at runtime. Change-Id: Iac99b51ab0e0bbc0d4f9ef27446e9ee9af2f823a --- checks/f5_bigip_conns | 33 ++++++++++++++----- cmk/gui/plugins/wato/check_parameters.py | 7 ++-- tests/unit/checks/test_f5_bigip_conns.py | 55 ++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 10 deletions(-) diff --git a/checks/f5_bigip_conns b/checks/f5_bigip_conns index f868d74..28e610d 100644 --- a/checks/f5_bigip_conns +++ b/checks/f5_bigip_conns @@ -36,6 +36,23 @@ def inventory_f5_bigip_conns(info): return [(None, {})] +def get_conn_rate_params(params): + # upper_bound is dict, tuple or None + upper_bound = params.get("connections_rate", (None, None)) + # lower_bound is tuple or None + lower_bound = params.get("connections_rate_lower", (None, None)) + if isinstance(upper_bound, tuple): + return upper_bound + lower_bound + + # Lower bound was not configured, all good + if isinstance(upper_bound, dict) and lower_bound == (None, None): + return upper_bound + raise ValueError("Can't configure minimum connections per second when the maximum " + "connections per second is setup in predictive levels. Please use the given " + "lower bound specified in the maximum connections, or set maximum " + "connections to use fixed levels.") + + def check_f5_bigip_conns(item, params, info): # Connection rate now = time.time() @@ -68,8 +85,11 @@ def check_f5_bigip_conns(item, params, info): conns_dict.setdefault("total_ssl", 0) conns_dict["total_ssl"] += int(line[1]) - conn_rate_params = params.get("connections_rate") or (None, None) - conn_rate_params += params.get("connections_rate_lower") or (None, None) + try: + conn_rate_params = get_conn_rate_params(params) + except ValueError as err: + yield 3, err.message + return # Current connections for val, dsname, params_values, perfkey, title in [ @@ -90,13 +110,10 @@ def check_f5_bigip_conns(item, params, info): if val is None: infotext += " not configured" else: - # The check_parameters are a bit tricky: If thresholds are disabled, - # the value is None; otherwise it's a Tuple. - # TODO: Refactor the check parameter f5_connections and this check plugin! - if params_values is None: - warn, crit = (None, None) - else: + if isinstance(params_values, tuple): warn, crit = params_values[:2] + else: + warn, crit = (None, None) perfdata.append((perfkey, val, warn, crit)) state, extrainfo, extraperf = check_levels(val, dsname, params_values) diff --git a/cmk/gui/plugins/wato/check_parameters.py b/cmk/gui/plugins/wato/check_parameters.py index ff04da8..c416dce 100644 --- a/cmk/gui/plugins/wato/check_parameters.py +++ b/cmk/gui/plugins/wato/check_parameters.py @@ -11111,9 +11111,12 @@ register_check_parameters( default_value=None, default_levels=(500, 1000))), ("connections_rate_lower", - Levels( + Tuple( title=_("Minimum connections per second"), - default_value=None, + elements=[ + Integer(title=_("Warning at")), + Integer(title=_("Critical at")), + ], )), ("http_req_rate", Levels( diff --git a/tests/unit/checks/test_f5_bigip_conns.py b/tests/unit/checks/test_f5_bigip_conns.py new file mode 100644 index 0000000..94e5061 --- /dev/null +++ b/tests/unit/checks/test_f5_bigip_conns.py @@ -0,0 +1,55 @@ +import pytest + +pytestmark = pytest.mark.checks + +from cmk_base.check_api import MKGeneralException +from checktestlib import CheckResult, assertCheckResultsEqual + + +(a)pytest.mark.parametrize("configonfig, result", [ + ({}, (None, None, None, None)), + ({ + "connections_rate": (2, 5), + "connections_rate_lower": (1, 0), + }, (2, 5, 1, 0)), + ({ + "connections_rate_lower": (1, 0), + }, (None, None, 1, 0)), + ({ + "connections_rate": (2, 5), + }, (2, 5, None, None)), + ({ + 'connections_rate': { + 'levels_upper': ('absolute', (0.0, 0.0)), + 'levels_lower': ('stdev', (2.0, 4.0)), + 'period': 'wday', + 'horizon': 90 + }, + }, { + 'levels_upper': ('absolute', (0.0, 0.0)), + 'levels_lower': ('stdev', (2.0, 4.0)), + 'period': 'wday', + 'horizon': 90 + }), +]) +def test_get_conn_rate_params(check_manager, config, result): + check = check_manager.get_check("f5_bigip_conns") + assert check.context["get_conn_rate_params"](config) == result + + +(a)pytest.mark.parametrize("configonfig, exception_msg", [({ + 'connections_rate': { + 'levels_upper': ('absolute', (0.0, 0.0)), + 'levels_lower': ('stdev', (2.0, 4.0)), + 'period': 'wday', + 'horizon': 90 + }, + "connections_rate_lower": (1, 0), +}, ("Can't configure minimum connections per second when the maximum " + "connections per second is setup in predictive levels. Please use the given " + "lower bound specified in the maximum connections, or set maximum " + "connections to use fixed levels."))]) +def test_get_conn_rate_params_exception(check_manager, config, exception_msg): + check = check_manager.get_check("f5_bigip_conns") + with pytest.raises(ValueError, message=exception_msg): + check.context["get_conn_rate_params"](config)