From: Thomas Markwalder Date: Tue, 26 Mar 2024 18:58:20 +0000 (-0400) Subject: [#3278] Addressed review comments X-Git-Tag: Kea-2.5.8~108 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=60902db63f0ad7f8d578ca3901867ea5d3fe4aac;p=thirdparty%2Fkea.git [#3278] Addressed review comments Minor corrections in ARM and UTs --- diff --git a/doc/sphinx/arm/hooks-perfmon.rst b/doc/sphinx/arm/hooks-perfmon.rst index b5207cc4bc..224a0ad4d4 100644 --- a/doc/sphinx/arm/hooks-perfmon.rst +++ b/doc/sphinx/arm/hooks-perfmon.rst @@ -47,7 +47,7 @@ shown below: #. process_completed - Server has constructed the response and is ready to send it This list is likely to expand over time. It will also be possible for hook developers -to add their own events. This will be detailed in a future release in t. +to add their own events. This will be detailed in a future release. Passive Event Logging ~~~~~~~~~~~~~~~~~~~~~ @@ -90,8 +90,10 @@ uniquely identified by a "duration key" which consists of the following values: * stop event label - Event that defines the end of the task (e.g. buffer_read, process_completed) * subnet id - subnet selected during message processing (or 0 for global durations) -As client queries are responded to their event stacks are used add to the monitored -durations. When the sampling interval has been reached for a given duration, it is reported. +Once the server has finished constructing a response to a query, the query's event stack +is processed into a series of updates to monitored durations. If upon updating, a +duration's sample interval is found to have been completed, it is sent to reporting +and a new sample interval is begun. .. Note: Monitoring is not yet functional. @@ -112,7 +114,7 @@ below the low-water mark the alarm is "cleared" and an INFO level log will be em API Commands ~~~~~~~~~~~~ - Commands to enable or disable monitoring, clear or alter alarms, and fetch duration datax + Commands to enable or disable monitoring, clear or alter alarms, and fetch duration data are anticipated but not yet supported. Configuration diff --git a/src/hooks/dhcp/perfmon/tests/alarm_parser_unittests.cc b/src/hooks/dhcp/perfmon/tests/alarm_parser_unittests.cc index dfc0959973..afb4c4a5e7 100644 --- a/src/hooks/dhcp/perfmon/tests/alarm_parser_unittests.cc +++ b/src/hooks/dhcp/perfmon/tests/alarm_parser_unittests.cc @@ -182,6 +182,26 @@ public: )", "'high-water-ms' parameter is not an integer" }, + { + // Invalid zero value for high-water-ms + __LINE__, + R"( + "enable-alarm": true, + "high-water-ms": 0, + "low-water-ms": 25 + )", + "high-water-ms: '0', must be greater than 0" + }, + { + // Invalid negative value for high-water-ms + __LINE__, + R"( + "enable-alarm": true, + "high-water-ms": -1, + "low-water-ms": 25 + )", + "high-water-ms: '-1', must be greater than 0" + }, { // Missing low-water-ms __LINE__, @@ -201,6 +221,26 @@ public: )", "'low-water-ms' parameter is not an integer" }, + { + // Invalid zero value for low-water-ms + __LINE__, + R"( + "enable-alarm": true, + "high-water-ms": 500, + "low-water-ms": 0 + )", + "low-water-ms: '0', must be greater than 0" + }, + { + // Invalid negative value for low-water-ms + __LINE__, + R"( + "enable-alarm": true, + "high-water-ms": 500, + "low-water-ms": -1 + )", + "low-water-ms: '-1', must be greater than 0" + }, { // Invalid threshold combination __LINE__, diff --git a/src/hooks/dhcp/perfmon/tests/duration_key_parser_unittests.cc b/src/hooks/dhcp/perfmon/tests/duration_key_parser_unittests.cc index e7b6445612..2e22841f7a 100644 --- a/src/hooks/dhcp/perfmon/tests/duration_key_parser_unittests.cc +++ b/src/hooks/dhcp/perfmon/tests/duration_key_parser_unittests.cc @@ -165,19 +165,6 @@ TEST_F(DurationKeyParserTest, validScenarios4) { })", DHCPDISCOVER, DHCPOFFER, "start_here", "stop_there", SUBNET_ID_GLOBAL }, - { - // Empty message types should map to DHCP_NOTYPE - __LINE__, - R"( - { - "query-type": "", - "response-type": "", - "start-event": "start_here", - "stop-event": "stop_there", - "subnet-id": 701 - })", - DHCP_NOTYPE, DHCP_NOTYPE, "start_here", "stop_there", 701 - }, { // "*" message types should map to DHCP_NOTYPE __LINE__, @@ -412,19 +399,6 @@ TEST_F(DurationKeyParserTest, validScenarios6) { })", DHCPV6_SOLICIT, DHCPV6_ADVERTISE, "start_here", "stop_there", SUBNET_ID_GLOBAL }, - { - // Empty message types should map to DHCPV6_NOTYPE - __LINE__, - R"( - { - "query-type": "", - "response-type": "", - "start-event": "start_here", - "stop-event": "stop_there", - "subnet-id": 701 - })", - DHCPV6_NOTYPE, DHCPV6_NOTYPE, "start_here", "stop_there", 701 - }, { // "*" message types should map to DHCPV6_NOTYPE __LINE__, diff --git a/src/hooks/dhcp/perfmon/tests/perfmon_config_unittests.cc b/src/hooks/dhcp/perfmon/tests/perfmon_config_unittests.cc index ea16a25156..a46d95320f 100644 --- a/src/hooks/dhcp/perfmon/tests/perfmon_config_unittests.cc +++ b/src/hooks/dhcp/perfmon/tests/perfmon_config_unittests.cc @@ -193,6 +193,18 @@ public: })", "'enable-monitoring' parameter is not a boolean" }, + { + // Invalid type for interval-width-secs + __LINE__, + R"( + { + "enable-monitoring" : false, + "interval-width-secs" : "bogus", + "stats-mgr-reporting" : false, + "alarm-report-secs" : 90 + })", + "'interval-width-secs' parameter is not an integer" + }, { // Value of interval-width-secs is zero __LINE__, @@ -229,6 +241,18 @@ public: })", "'stats-mgr-reporting' parameter is not a boolean" }, + { + // Invalid type for alarm-report-secs + __LINE__, + R"( + { + "enable-monitoring" : false, + "interval-width-secs" : 5, + "stats-mgr-reporting" : false, + "alarm-report-secs" : "bogus" + })", + "'alarm-report-secs' parameter is not an integer" + }, { // Value of alarm-report-secs is zero __LINE__, diff --git a/src/hooks/dhcp/perfmon/tests/perfmon_mgr_unittests.cc b/src/hooks/dhcp/perfmon/tests/perfmon_mgr_unittests.cc index 30bd5af799..62f2dbe021 100644 --- a/src/hooks/dhcp/perfmon/tests/perfmon_mgr_unittests.cc +++ b/src/hooks/dhcp/perfmon/tests/perfmon_mgr_unittests.cc @@ -133,13 +133,11 @@ public: ASSERT_EQ(durations->size(), 0); } - /// @brief Exercises PerfMonConfig parameter parsing with valid configuration - /// permutations. - /// @todo add alarms + /// @brief Verifies that PerfMonConfig handles a configuration error properly. void testInvalidConfig() { - std::string valid_config = + std::string invalid_config = R"({ - "enable-monitoring": false, + "enable-monitoring": true, "interval-width-secs": 5, "stats-mgr-reporting": false, "alarm-report-secs": 600, @@ -148,15 +146,17 @@ public: // Convert JSON texts to Element map. ConstElementPtr json_elements; - ASSERT_NO_THROW_LOG(json_elements = Element::fromJSON(valid_config)); + ASSERT_NO_THROW_LOG(json_elements = Element::fromJSON(invalid_config)); PerfMonMgrPtr mgr(new PerfMonMgr(family_)); - ASSERT_NO_THROW_LOG(mgr->configure(json_elements)); + ASSERT_THROW_MSG(mgr->configure(json_elements), DhcpConfigError, + "PerfMonMgr::configure failed - " + "'alarms' parameter is not a list"); EXPECT_FALSE(mgr->getEnableMonitoring()); - EXPECT_EQ(mgr->getIntervalDuration(), seconds(5)); - EXPECT_FALSE(mgr->getStatsMgrReporting()); - EXPECT_EQ(mgr->getAlarmReportInterval(), seconds(600)); + EXPECT_EQ(mgr->getIntervalDuration(), seconds(60)); + EXPECT_TRUE(mgr->getStatsMgrReporting()); + EXPECT_EQ(mgr->getAlarmReportInterval(), seconds(300)); // Alarm store should exist but be empty. EXPECT_TRUE(mgr->getAlarmStore()); @@ -213,4 +213,12 @@ TEST_F(PerfMonMgrTest6, validConfig) { testValidConfig(); } +TEST_F(PerfMonMgrTest4, invalidConfig) { + testInvalidConfig(); +} + +TEST_F(PerfMonMgrTest6, invalidConfig) { + testInvalidConfig(); +} + } // end of anonymous namespace