From 493267eff16b4690decba43d86ddf61e323efc12 Mon Sep 17 00:00:00 2001 From: Craig Bryant Date: Tue, 13 Jan 2015 11:39:36 -0700 Subject: [PATCH] Restrict changes to existing Alarm Definitions Once an Alarm Definition has been created, match_by and any metrics in the expression cannot be changed. This is because those fields control the metrics used to create Alarms and Alarms may already have been created. The function, operator, period, periods and any boolean operators can change, but not the metrics in subexpressions or of the number sub expressions. All other fields in an Alarm Definition can be changed Add tests for new functionality and some old for patch and update Fix a bug where match-by was being cleared in patch if not given Update api docs for the restrictions Fix minor typos in api docs Change-Id: I89a46838847b9cb73a073f9dd91db8f9391b6019 --- docs/monasca-api-spec.md | 31 +- .../api/app/AlarmDefinitionService.java | 80 ++-- .../api/resource/AlarmDefinitionResource.java | 6 +- .../api/app/AlarmDefinitionServiceTest.java | 390 ++++++++++++++++-- 4 files changed, 432 insertions(+), 75 deletions(-) diff --git a/docs/monasca-api-spec.md b/docs/monasca-api-spec.md index 3e26348f8..64c18a48a 100644 --- a/docs/monasca-api-spec.md +++ b/docs/monasca-api-spec.md @@ -19,6 +19,7 @@ Document Version: v2.0 - [Simple Example](#simple-example) - [More Complex Example](#more-complex-example) - [Compound alarm example](#compound-alarm-example) + - [Changing Alarm Definitions](#changing-alarm-definitions) - [Common Request Headers](#common-request-headers) - [Common Http Request Headers](#common-http-request-headers) - [Non-standard request headers](#non-standard-request-headers) @@ -357,7 +358,7 @@ Alarm 2 - Metrics: cpu.idle_perc{service=monitoring,hostname=devstack} Alarm Definition 1 is evaluating the status of the monitoring service as a whole, while Alarm Definition 2 evaluates each system in the service. -Now if another system is configured into the monitoring service, then its cpu.idle_perc metric will be added to the Alarm for Alarm Definition 1 and a new Alarm will be created for Alarm Definition 2, all without any user intervention. The system will be monitored without requiring the user to explictly add alarms for the new system as other monitoring systems require. +Now if another system is configured into the monitoring service, then its cpu.idle_perc metric will be added to the Alarm for Alarm Definition 1 and a new Alarm will be created for Alarm Definition 2, all without any user intervention. The system will be monitored without requiring the user to explicitly add alarms for the new system as other monitoring systems require. If an Alarm Definition expression has multiple subexpressions, for example, `avg(cpu.idle_perc{service=monitoring}) < 10 or avg(cpu.user_perc{service=monitoring}) > 60` and a match_by value set, then the metrics for both subexpressions must have the same value for the dimension specified in match_by. For example, assume this Alarm Definition: @@ -461,7 +462,7 @@ Each subexpression is made up of several parts with a couple of options: | '(' expression ')' ```` -Period must an interger multiple of 60. The default period is 60 seconds. +Period must be an integer multiple of 60. The default period is 60 seconds. The logical_operators are: `and` (also `&&`), `or` (also `||`). @@ -549,6 +550,12 @@ In this example a compound alarm expression is evaluated involving two threshold avg(cpu.system_perc{hostname=hostname.domain.com}) > 90 or avg(disk_read_ops{hostname=hostname.domain.com, device=vda}, 120) > 1000 ``` +### Changing Alarm Definitions + +Once an Alarm Definition has been created, the value for match_by and any metrics in the expression cannot be changed. This is because those fields control the metrics used to create Alarms and Alarms may already have been created. The function, operator, period, periods and any boolean operators can change, but not the metrics in subexpressions or the number of subexpressions. All other fields in an Alarm Definition can be changed. + +The only option to change metrics or match_by is to delete the existing Alarm Definition and create a new one. Deleting an Alarm Definition will delete all Alarms associated with it. + # Common Request Headers This section documents the common request headers that are used in requests. @@ -1031,7 +1038,7 @@ None. None. #### Request Body -* name (string(250), required) - A descriptive name of the notifcation method. +* name (string(250), required) - A descriptive name of the notification method. * type (string(100), required) - The type of notification method (`EMAIL` or `WEBHOOK` ). * address (string(100), required) - The email/url address to notify. @@ -1333,7 +1340,7 @@ Consists of an alarm definition. An alarm has the following properties: * name (string(255), required) - A unique name of the alarm. Note, the name must be unique. * description (string(255), optional) - A description of an alarm. * expression (string, required) - An alarm expression. -* match_by ([string], optional) - The metric dimensions to match to the alarm dimensions. +* match_by ([string], optional) - The metric dimensions to use to create unique alarms * severity (string, optional) - Severity of an alarm. Must be either `LOW`, `MEDIUM`, `HIGH` or `CRITICAL`. Default is `LOW`. * alarm_actions ([string(50)], optional) - Array of notification method IDs that are invoked when the alarm transitions to the `ALARM` state. * ok_actions ([string(50)], optional) - Array of notification method IDs that are invoked when the alarm transitions to the `OK` state. @@ -1468,7 +1475,7 @@ Returns a JSON array of alarm objects with the following fields: * description (string) - Description of alarm definition. * expression (string) - The alarm definition expression. * expression_data (JSON object) - The alarm definition expression as a JSON object. -* match_by ([string]) - The metric dimensions to match to the alarm dimensions +* match_by ([string]) - The metric dimensions to use to create unique alarms * severity (string) - The severity of an alarm definition. Either `LOW`, `MEDIUM`, `HIGH` or `CRITICAL`. * actions_enabled (boolean) - If true actions for all alarms related to this definition are enabled. * alarm_actions ([string]) - Array of notification method IDs that are invoked when the alarms for this definition transition to the `ALARM` state. @@ -1550,7 +1557,7 @@ Returns a JSON alarm object with the following fields: * description (string) - Description of alarm definition. * expression (string) - The alarm definition expression. * expression_data (JSON object) - The alarm definition expression as a JSON object. -* match_by ([string]) - The metric dimensions to match to the alarm dimensions +* match_by ([string]) - The metric dimensions to use to create unique alarms * severity (string) - The severity of an alarm definition. Either `LOW`, `MEDIUM`, `HIGH` or `CRITICAL`. * actions_enabled (boolean) - If true actions for all alarms related to this definition are enabled. * alarm_actions ([string]) - Array of notification method IDs that are invoked when the alarms for this definition transition to the `ALARM` state. @@ -1621,7 +1628,7 @@ Consists of an alarm definition. An alarm has the following properties: * name (string(255), required) - A name of the alarm definition. * description (string(255), optional) - A description of an alarm definition. * expression (string, required) - An alarm expression. -* match_by ([string], optional) - The metric dimensions to match to the alarm dimensions +* match_by ([string], optional) - The metric dimensions to use to create unique alarms. If specified, this MUST be the same as the existing value for match_by * severity (string, optional) - Severity of an alarm definition. Must be either `LOW`, `MEDIUM`, `HIGH` or `CRITICAL`. * alarm_actions ([string(50)], optional) * ok_actions ([string(50)], optional) @@ -1629,6 +1636,8 @@ Consists of an alarm definition. An alarm has the following properties: If optional parameters are not specified they will be reset to their default state. +Only the parameters that are specified will be updated. See Changing Alarm Definitions for restrictions on changing expression and match_by. + #### Request Examples ``` PUT /v2.0/alarm-definitions/f9935bcc-9641-4cbf-8224-0993a947ea83 HTTP/1.1 @@ -1669,7 +1678,7 @@ Returns a JSON alarm object with the following parameters: * description (string) - Description of alarm definition. * expression (string) - The alarm definition expression. * expression_data (JSON object) - The alarm definition expression as a JSON object. -* match_by ([string]) - The metric dimensions to match to the alarm dimensions +* match_by ([string]) - The metric dimensions to use to create unique alarms * severity (string) - The severity of an alarm definition. Either `LOW`, `MEDIUM`, `HIGH` or `CRITICAL`. * actions_enabled (boolean) - If true actions for all alarms related to this definition are enabled. * alarm_actions ([string]) - Array of notification method IDs that are invoked when the alarms for this definition transition to the `ALARM` state. @@ -1738,14 +1747,14 @@ Consists of an alarm with the following properties: * name (string) - Name of alarm definition. * description (string) - Description of alarm definition. * expression (string) - The alarm definition expression. -* match_by ([string], optional) - The metric dimensions to match to the alarm dimensions +* match_by ([string], optional) - The metric dimensions to use to create unique alarms. If specified, this MUST be the same as the existing value for match_by * severity (string) - The severity of an alarm definition. Either `LOW`, `MEDIUM`, `HIGH` or `CRITICAL`. * actions_enabled (boolean) - If true actions for all alarms related to this definition are enabled. * alarm_actions ([string]) - Array of notification method IDs that are invoked when the alarms for this definition transition to the `ALARM` state. * ok_actions ([string]) - Array of notification method IDs that are invoked when the alarms for this definition transition to the `OK` state. * undetermined_actions ([string]) - Array of notification method IDs that are invoked when the alarms for this definition transition to the `UNDETERMINED` state. -Only the parameters that are specified will be updated. +Only the parameters that are specified will be updated. See Changing Alarm Definitions for restrictions on changing expression and match_by. #### Request Examples ``` @@ -1788,7 +1797,7 @@ Returns a JSON alarm definition object with the following fields: * description (string) - Description of alarm definition. * expression (string) - The alarm definition expression. * expression_data (JSON object) - The alarm definition expression as a JSON object. -* match_by ([string]) - The metric dimensions to match to the alarm dimensions +* match_by ([string]) - The metric dimensions to use to create unique alarms * severity (string) - The severity of an alarm definition. Either `LOW`, `MEDIUM`, `HIGH` or `CRITICAL`. * actions_enabled (boolean) - If true actions for all alarms related to this definition are enabled. * alarm_actions ([string]) - Array of notification method IDs that are invoked when the alarms for this definition transition to the `ALARM` state. diff --git a/src/main/java/monasca/api/app/AlarmDefinitionService.java b/src/main/java/monasca/api/app/AlarmDefinitionService.java index 94eb72411..4ad447845 100644 --- a/src/main/java/monasca/api/app/AlarmDefinitionService.java +++ b/src/main/java/monasca/api/app/AlarmDefinitionService.java @@ -41,7 +41,6 @@ import monasca.common.model.event.AlarmDefinitionDeletedEvent; import monasca.common.model.event.AlarmDefinitionUpdatedEvent; import monasca.common.model.event.AlarmDeletedEvent; import monasca.common.model.alarm.AlarmExpression; -import monasca.common.model.alarm.AlarmState; import monasca.common.model.alarm.AlarmSubExpression; import monasca.common.model.metric.MetricDefinition; import monasca.api.domain.exception.EntityExistsException; @@ -179,17 +178,50 @@ public class AlarmDefinitionService { */ public AlarmDefinition update(String tenantId, String alarmDefId, AlarmExpression alarmExpression, UpdateAlarmDefinitionCommand command) { - assertAlarmDefinitionExists(tenantId, alarmDefId, command.alarmActions, command.okActions, + final AlarmDefinition oldAlarmDefinition = assertAlarmDefinitionExists(tenantId, alarmDefId, command.alarmActions, command.okActions, command.undeterminedActions); + final SubExpressions subExpressions = + subExpressionsFor(repo.findSubExpressions(alarmDefId), alarmExpression); + validateChangesAllowed(command.matchBy, oldAlarmDefinition, subExpressions); updateInternal(tenantId, alarmDefId, false, command.name, command.description, command.expression, command.matchBy, command.severity, alarmExpression, command.actionsEnabled, command.alarmActions, command.okActions, - command.undeterminedActions); + command.undeterminedActions, subExpressions); return new AlarmDefinition(alarmDefId, command.name, command.description, command.severity, command.expression, command.matchBy, command.actionsEnabled, command.alarmActions, command.okActions, command.undeterminedActions); } + /** + * Don't allow changes that would cause existing Alarms for this AlarmDefinition to be invalidated. + * + * matchBy can't change and the expression can't change the metrics used or number of subexpressions + */ + private void validateChangesAllowed(final List newMatchBy, + final AlarmDefinition oldAlarmDefinition, final SubExpressions subExpressions) { + final boolean matchBySame; + if (oldAlarmDefinition.getMatchBy() == null || oldAlarmDefinition.getMatchBy().isEmpty()) { + matchBySame = newMatchBy == null || newMatchBy.isEmpty(); + } + else { + matchBySame = oldAlarmDefinition.getMatchBy().equals(newMatchBy); + } + if (!matchBySame) { + throw monasca.api.resource.exception.Exceptions.unprocessableEntity("match_by must not change"); + } + if (!subExpressions.oldAlarmSubExpressions.isEmpty() || !subExpressions.newAlarmSubExpressions.isEmpty()) { + final int newCount = subExpressions.newAlarmSubExpressions.size() + + subExpressions.changedSubExpressions.size() + + subExpressions.unchangedSubExpressions.size(); + if (newCount != AlarmExpression.of(oldAlarmDefinition.getExpression()).getSubExpressions().size()) { + throw monasca.api.resource.exception.Exceptions.unprocessableEntity("number of subexpressions must not change"); + } + else { + throw monasca.api.resource.exception.Exceptions.unprocessableEntity("metrics in subexpression must not change"); + } + } + } + /** * Patches the alarm definition for the {@code tenantId} and {@code alarmDefId} to the state of * the {@code fields}. @@ -199,32 +231,35 @@ public class AlarmDefinitionService { */ public AlarmDefinition patch(String tenantId, String alarmDefId, String name, String description, String severity, String expression, AlarmExpression alarmExpression, List matchBy, - AlarmState state, Boolean enabled, List alarmActions, List okActions, + Boolean enabled, List alarmActions, List okActions, List undeterminedActions) { - AlarmDefinition alarm = + AlarmDefinition oldAlarmDefinition = assertAlarmDefinitionExists(tenantId, alarmDefId, alarmActions, okActions, undeterminedActions); - name = name == null ? alarm.getName() : name; - description = description == null ? alarm.getDescription() : description; - expression = expression == null ? alarm.getExpression() : expression; - severity = severity == null ? alarm.getSeverity() : severity; + name = name == null ? oldAlarmDefinition.getName() : name; + description = description == null ? oldAlarmDefinition.getDescription() : description; + expression = expression == null ? oldAlarmDefinition.getExpression() : expression; + severity = severity == null ? oldAlarmDefinition.getSeverity() : severity; alarmExpression = alarmExpression == null ? AlarmExpression.of(expression) : alarmExpression; - enabled = enabled == null ? alarm.isActionsEnabled() : enabled; + enabled = enabled == null ? oldAlarmDefinition.isActionsEnabled() : enabled; + matchBy = matchBy == null ? oldAlarmDefinition.getMatchBy() : matchBy; + final SubExpressions subExpressions = + subExpressionsFor(repo.findSubExpressions(alarmDefId), alarmExpression); + validateChangesAllowed(matchBy, oldAlarmDefinition, subExpressions); updateInternal(tenantId, alarmDefId, true, name, description, expression, matchBy, severity, - alarmExpression, enabled, alarmActions, okActions, undeterminedActions); + alarmExpression, enabled, alarmActions, okActions, undeterminedActions, subExpressions); return new AlarmDefinition(alarmDefId, name, description, severity, expression, matchBy, - enabled, alarmActions == null ? alarm.getAlarmActions() : alarmActions, - okActions == null ? alarm.getOkActions() : okActions, - undeterminedActions == null ? alarm.getUndeterminedActions() : undeterminedActions); + enabled, alarmActions == null ? oldAlarmDefinition.getAlarmActions() : alarmActions, + okActions == null ? oldAlarmDefinition.getOkActions() : okActions, + undeterminedActions == null ? oldAlarmDefinition.getUndeterminedActions() : undeterminedActions); } private void updateInternal(String tenantId, String alarmDefId, boolean patch, String name, String description, String expression, List matchBy, String severity, AlarmExpression alarmExpression, Boolean enabled, List alarmActions, - List okActions, List undeterminedActions) { - SubExpressions subExpressions = subExpressionsFor(alarmDefId, alarmExpression); + List okActions, List undeterminedActions, SubExpressions subExpressions) { try { LOG.debug("Updating alarm definition {} for tenant {}", name, tenantId); @@ -250,9 +285,10 @@ public class AlarmDefinitionService { * Returns an entry containing Maps of old, changed, and new sub expressions by comparing the * {@code alarmExpression} to the existing sub expressions for the {@code alarmDefId}. */ - SubExpressions subExpressionsFor(String alarmDefId, AlarmExpression alarmExpression) { + SubExpressions subExpressionsFor(final Map initialSubExpressions, + AlarmExpression alarmExpression) { BiMap oldExpressions = - HashBiMap.create(repo.findSubExpressions(alarmDefId)); + HashBiMap.create(initialSubExpressions); Set oldSet = oldExpressions.inverse().keySet(); Set newSet = new HashSet<>(alarmExpression.getSubExpressions()); @@ -300,13 +336,11 @@ public class AlarmDefinitionService { } /** - * Returns whether all of the fields of {@code a} and {@code b} are the same except the operator - * and threshold. + * Returns whether all of the metrics of {@code a} and {@code b} are the same. The Threshold + * Engine can handle any other type of change to the expression */ private boolean sameKeyFields(AlarmSubExpression a, AlarmSubExpression b) { - return a.getMetricDefinition().equals(b.getMetricDefinition()) - && a.getFunction().equals(b.getFunction()) && a.getPeriod() == b.getPeriod() - && a.getPeriods() == b.getPeriods(); + return a.getMetricDefinition().equals(b.getMetricDefinition()); } /** diff --git a/src/main/java/monasca/api/resource/AlarmDefinitionResource.java b/src/main/java/monasca/api/resource/AlarmDefinitionResource.java index 6d42a0ec1..c7f77808f 100644 --- a/src/main/java/monasca/api/resource/AlarmDefinitionResource.java +++ b/src/main/java/monasca/api/resource/AlarmDefinitionResource.java @@ -50,7 +50,6 @@ import monasca.api.domain.model.alarmdefinition.AlarmDefinition; import monasca.api.domain.model.alarmdefinition.AlarmDefinitionRepository; import monasca.api.resource.annotation.PATCH; import monasca.common.model.alarm.AlarmExpression; -import monasca.common.model.alarm.AlarmState; /** * Alarm definition resource implementation. @@ -136,9 +135,6 @@ public class AlarmDefinitionResource { String severity = (String) fields.get("severity"); String expression = (String) fields.get("expression"); List matchBy = (List) fields.get("match_by"); - String stateStr = (String) fields.get("state"); - AlarmState state = - stateStr == null ? null : Validation.parseAndValidate(AlarmState.class, stateStr); Boolean enabled = (Boolean) fields.get("actions_enabled"); List alarmActions = (List) fields.get("alarm_actions"); List okActions = (List) fields.get("ok_actions"); @@ -149,7 +145,7 @@ public class AlarmDefinitionResource { expression == null ? null : AlarmValidation.validateNormalizeAndGet(expression); return Links.hydrate(service.patch(tenantId, alarmDefinitionId, name, description, severity, - expression, alarmExpression, matchBy, state, enabled, alarmActions, okActions, + expression, alarmExpression, matchBy, enabled, alarmActions, okActions, undeterminedActions), uriInfo, true); } diff --git a/src/test/java/monasca/api/app/AlarmDefinitionServiceTest.java b/src/test/java/monasca/api/app/AlarmDefinitionServiceTest.java index 5393eef45..f6c4c64d2 100644 --- a/src/test/java/monasca/api/app/AlarmDefinitionServiceTest.java +++ b/src/test/java/monasca/api/app/AlarmDefinitionServiceTest.java @@ -1,11 +1,11 @@ /* * Copyright (c) 2014 Hewlett-Packard Development Company, L.P. - * + * * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except * in compliance with the License. You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software distributed under the License * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express * or implied. See the License for the specific language governing permissions and limitations under @@ -14,20 +14,30 @@ package monasca.api.app; +import com.google.common.collect.BiMap; +import com.google.common.collect.HashBiMap; + import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.times; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertNotEquals; import static org.testng.Assert.assertTrue; +import static org.testng.Assert.fail; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; + +import javax.ws.rs.WebApplicationException; import kafka.javaapi.producer.Producer; import kafka.producer.KeyedMessage; @@ -42,6 +52,8 @@ import monasca.api.app.AlarmDefinitionService.SubExpressions; import monasca.api.app.command.UpdateAlarmDefinitionCommand; import monasca.common.model.alarm.AlarmExpression; import monasca.common.model.alarm.AlarmSubExpression; +import monasca.common.model.event.AlarmDefinitionUpdatedEvent; +import monasca.common.util.Serialization; import monasca.api.domain.model.alarm.AlarmRepository; import monasca.api.domain.model.alarmdefinition.AlarmDefinition; import monasca.api.domain.model.alarmdefinition.AlarmDefinitionRepository; @@ -49,6 +61,12 @@ import monasca.api.domain.model.notificationmethod.NotificationMethodRepository; @Test public class AlarmDefinitionServiceTest { + private static final String EXPR2 = "avg(bar{instance_id=777}) > 80"; + + private static final String EXPR1 = "avg(foo{instance_id=123}) > 90"; + + final static String TENANT_ID = "bob"; + AlarmDefinitionService service; MonApiConfiguration config; Producer producer; @@ -87,55 +105,329 @@ public class AlarmDefinitionServiceTest { List okActions = Arrays.asList("2", "3"); List undeterminedActions = Arrays.asList("3"); - when(notificationMethodRepo.exists(eq("bob"), anyString())).thenReturn(true); + when(notificationMethodRepo.exists(eq(TENANT_ID), anyString())).thenReturn(true); AlarmDefinition alarm = - service.create("bob", "90% CPU", "foo", "LOW", exprStr, AlarmExpression.of(exprStr), + service.create(TENANT_ID, "90% CPU", "foo", "LOW", exprStr, AlarmExpression.of(exprStr), matchBy, alarmActions, okActions, undeterminedActions); AlarmDefinition expected = new AlarmDefinition(alarm.getId(), "90% CPU", "foo", "LOW", exprStr, matchBy, true, alarmActions, okActions, undeterminedActions); assertEquals(expected, alarm); - verify(repo).create(eq("bob"), anyString(), eq("90% CPU"), eq("foo"), eq("LOW"), eq(exprStr), + verify(repo).create(eq(TENANT_ID), anyString(), eq("90% CPU"), eq("foo"), eq("LOW"), eq(exprStr), any(Map.class), eq(matchBy), eq(alarmActions), eq(okActions), eq(undeterminedActions)); verify(producer).send(any(KeyedMessage.class)); } - @SuppressWarnings("unchecked") - public void shouldUpdate() { - String exprStr = "avg(foo{instance_id=123}) > 90 or avg(bar{instance_id=777}) > 80"; + public void updateFailsDueToMatchBy() { + final List matchBy = Arrays.asList("hostname", "service"); + final AlarmDefinition oldAlarmDef = setupInitialAlarmDefinition(matchBy); + + final List newMatchBy = Arrays.asList("service"); + verifyChangeFails(oldAlarmDef.getId(), oldAlarmDef.getName(), oldAlarmDef.getDescription(), + oldAlarmDef.getExpression(), newMatchBy, oldAlarmDef.getSeverity(), oldAlarmDef.isActionsEnabled(), + oldAlarmDef.getAlarmActions(), oldAlarmDef.getOkActions(), + oldAlarmDef.getUndeterminedActions(), "match_by"); + } + + public void changeFailsDueToDeletedExpression() { + final List matchBy = Arrays.asList("hostname", "service"); + final AlarmDefinition oldAlarmDef = setupInitialAlarmDefinition(matchBy); + + verifyChangeFails(oldAlarmDef.getId(), oldAlarmDef.getName(), oldAlarmDef.getDescription(), + EXPR1, matchBy, oldAlarmDef.getSeverity(), oldAlarmDef.isActionsEnabled(), + oldAlarmDef.getAlarmActions(), oldAlarmDef.getOkActions(), + oldAlarmDef.getUndeterminedActions(), "subexpressions"); + } + + public void changeFailsDueToAddedExpression() { + final List matchBy = Arrays.asList("hostname", "service"); + final AlarmDefinition oldAlarmDef = setupInitialAlarmDefinition(matchBy); + + final String newExpression = EXPR1 + " or " + EXPR2 + " or " + "min(cpu.idle) < 10"; + verifyChangeFails(oldAlarmDef.getId(), oldAlarmDef.getName(), oldAlarmDef.getDescription(), + newExpression, matchBy, oldAlarmDef.getSeverity(), oldAlarmDef.isActionsEnabled(), + oldAlarmDef.getAlarmActions(), oldAlarmDef.getOkActions(), + oldAlarmDef.getUndeterminedActions(), "subexpressions"); + } + + public void changeFailsDueToChangedMetricName() { + final List matchBy = Arrays.asList("hostname", "service"); + final AlarmDefinition oldAlarmDef = setupInitialAlarmDefinition(matchBy); + + final String newExpression = EXPR1 + " or " + EXPR2.replace("bar", "barz"); + verifyChangeFails(oldAlarmDef.getId(), oldAlarmDef.getName(), oldAlarmDef.getDescription(), + newExpression, matchBy, oldAlarmDef.getSeverity(), oldAlarmDef.isActionsEnabled(), + oldAlarmDef.getAlarmActions(), oldAlarmDef.getOkActions(), + oldAlarmDef.getUndeterminedActions(), "metric"); + } + + public void changeFailsDueToChangedMetricDimension() { + final List matchBy = Arrays.asList("hostname", "service"); + final AlarmDefinition oldAlarmDef = setupInitialAlarmDefinition(matchBy); + + final String newExpression = EXPR1 + " or " + EXPR2.replace("777", "888"); + verifyChangeFails(oldAlarmDef.getId(), oldAlarmDef.getName(), oldAlarmDef.getDescription(), + newExpression, matchBy, oldAlarmDef.getSeverity(), oldAlarmDef.isActionsEnabled(), + oldAlarmDef.getAlarmActions(), oldAlarmDef.getOkActions(), + oldAlarmDef.getUndeterminedActions(), "metric"); + } + + private void verifyChangeFails(final String id, String name, String description, + final String expression, List matchBy, String severity, boolean actionsEnabled, List alarmActions, + List okActions, List undeterminedActions, String expected) { + UpdateAlarmDefinitionCommand command = + new UpdateAlarmDefinitionCommand(name, description, expression, matchBy, severity, + actionsEnabled, alarmActions, okActions, undeterminedActions); + try { + service.update(TENANT_ID, id, AlarmExpression.of(command.expression), command); + fail("Update of AlarmDefinition succeeded when it should have failed"); + } + catch(WebApplicationException e) { + assertEquals(e.getResponse().getStatus(), 422); + assertTrue(e.getResponse().getEntity().toString().contains(expected)); + } + + try { + service.patch(TENANT_ID, id, name, description, severity, expression, + AlarmExpression.of(expression), matchBy, actionsEnabled, alarmActions, okActions, + undeterminedActions); + fail("Patch of AlarmDefinition succeeded when it should have failed"); + } + catch(WebApplicationException e) { + assertEquals(e.getResponse().getStatus(), 422); + assertTrue(e.getResponse().getEntity().toString().contains(expected)); + } + } + + public void shouldChange() { + final List matchBy = Arrays.asList("hostname", "service"); + final AlarmDefinition oldAlarmDef = setupInitialAlarmDefinition(matchBy); + + final String newExprStr = oldAlarmDef.getExpression().replace("90", "75").replace(" or ", " and "); + final List newAlarmActions = Arrays.asList("5", "6", "7"); + final List newOkActions = Arrays.asList("6", "7"); + final List newUndeterminedActions = Arrays.asList("7"); + final String newSeverity = "HIGH"; + final String newName = "foo bar baz"; + final String newDescription = "foo bar baz"; + final boolean newEnabled = false; + UpdateAlarmDefinitionCommand command = + new UpdateAlarmDefinitionCommand(newName, newDescription, newExprStr, matchBy, newSeverity, + newEnabled, newAlarmActions, newOkActions, newUndeterminedActions); + + final AlarmDefinition expected = + new AlarmDefinition(oldAlarmDef.getId(), newName, newDescription, newSeverity, newExprStr, matchBy, + newEnabled, newAlarmActions, newOkActions, newUndeterminedActions); + + final AlarmDefinition updatedAlarmDef = + service.update(TENANT_ID, oldAlarmDef.getId(), AlarmExpression.of(command.expression), + command); + assertEquals(updatedAlarmDef, expected); + + final AlarmDefinition patchedAlarmDef = + service.patch(TENANT_ID, oldAlarmDef.getId(), newName, newDescription, newSeverity, + newExprStr, AlarmExpression.of(newExprStr), matchBy, newEnabled, newAlarmActions, + newOkActions, newUndeterminedActions); + assertEquals(patchedAlarmDef, expected); + + final Map emptyMap = new HashMap<>(); + final Map changedSubExpressions = new HashMap<>(); + final Map unchangedSubExpressions = new HashMap<>(); + changedSubExpressions.put("444", AlarmSubExpression.of("avg(foo{instance_id=123}) > 75")); + unchangedSubExpressions.put("555", AlarmSubExpression.of(EXPR2)); + final AlarmDefinitionUpdatedEvent event = + new AlarmDefinitionUpdatedEvent(TENANT_ID, oldAlarmDef.getId(), newName, newDescription, + newExprStr, matchBy, newEnabled, newSeverity, emptyMap, changedSubExpressions, + unchangedSubExpressions, emptyMap); + verify(producer, times(2)).send(new KeyedMessage(config.eventsTopic, TENANT_ID, Serialization.toJson(event))); + } + + public void shouldPatchExpression() { + final List matchBy = Arrays.asList("hostname", "service"); + final AlarmDefinition oldAlarmDef = setupInitialAlarmDefinition(matchBy); + + final String newExprStr = oldAlarmDef.getExpression().replace("90", "75").replace(" or ", " and "); + assertNotEquals(newExprStr, oldAlarmDef.getExpression()); + + final Map changedSubExpressions = new HashMap<>(); + final Map unchangedSubExpressions = new HashMap<>(); + changedSubExpressions.put("444", AlarmSubExpression.of(EXPR1.replace("90", "75"))); + unchangedSubExpressions.put("555", AlarmSubExpression.of(EXPR2)); + + patchExpression(newExprStr, oldAlarmDef, changedSubExpressions, unchangedSubExpressions); + + final String newExprStr2 = EXPR2.replace("avg", "min") + " and " + EXPR1.replace("avg", "max"); + assertNotEquals(newExprStr2, oldAlarmDef.getExpression()); + changedSubExpressions.clear(); + unchangedSubExpressions.clear(); + changedSubExpressions.put("444", AlarmSubExpression.of(EXPR1.replace("avg", "max"))); + changedSubExpressions.put("555", AlarmSubExpression.of(EXPR2.replace("avg", "min"))); + + patchExpression(newExprStr2, oldAlarmDef, changedSubExpressions, unchangedSubExpressions); + } + + private void patchExpression(final String newExprStr, final AlarmDefinition oldAlarmDef, + final Map changedSubExpressions, + final Map unchangedSubExpressions) { + BiMap oldExpressions = + HashBiMap.create(new HashMap()); + final Set oldSubAlarmIds = oldExpressions.keySet(); + Map newSubAlarms = new HashMap<>(); + final AlarmDefinition patchedAlarmDef = + service.patch(TENANT_ID, oldAlarmDef.getId(), null, null, null, + newExprStr, AlarmExpression.of(newExprStr), null, null, null, + null, null); + + final AlarmDefinition expected = + new AlarmDefinition(oldAlarmDef.getId(), oldAlarmDef.getName(), oldAlarmDef.getDescription(), + oldAlarmDef.getSeverity(), newExprStr, oldAlarmDef.getMatchBy(), + oldAlarmDef.isActionsEnabled(), oldAlarmDef.getAlarmActions(), + oldAlarmDef.getOkActions(), oldAlarmDef.getUndeterminedActions()); + assertEquals(patchedAlarmDef, expected); + + final Map emptyMap = new HashMap<>(); + final AlarmDefinitionUpdatedEvent event = + new AlarmDefinitionUpdatedEvent(TENANT_ID, oldAlarmDef.getId(), oldAlarmDef.getName(), + oldAlarmDef.getDescription(), newExprStr, oldAlarmDef.getMatchBy(), + oldAlarmDef.isActionsEnabled(), oldAlarmDef.getSeverity(), emptyMap, + changedSubExpressions, unchangedSubExpressions, emptyMap); + verify(producer).send(new KeyedMessage(config.eventsTopic, TENANT_ID, Serialization.toJson(event))); + verify(repo).update(TENANT_ID, oldAlarmDef.getId(), true, oldAlarmDef.getName(), + oldAlarmDef.getDescription(), newExprStr, oldAlarmDef.getMatchBy(), + oldAlarmDef.getSeverity(), oldAlarmDef.isActionsEnabled(), oldSubAlarmIds, + changedSubExpressions, newSubAlarms, null, null, null); + } + + public void shouldPatchIndividual() { + final List matchBy = Arrays.asList("hostname", "service"); + final AlarmDefinition oldAlarmDef = setupInitialAlarmDefinition(matchBy); + + final List newAlarmActions = Arrays.asList("5", "6", "7"); + final List newOkActions = Arrays.asList("6", "7"); + final List newUndeterminedActions = Arrays.asList("7"); + final String newSeverity = "HIGH"; + final String newName = "foo bar baz"; + final String newDescription = "foo bar baz"; + final boolean newEnabled = false; + + doPatch(matchBy, oldAlarmDef, newName, newName, null, oldAlarmDef.getDescription(), null, + oldAlarmDef.getSeverity(), null, oldAlarmDef.isActionsEnabled(), null, + oldAlarmDef.getAlarmActions(), null, + oldAlarmDef.getOkActions(), null, + oldAlarmDef.getUndeterminedActions(), 1); + + doPatch(matchBy, oldAlarmDef, null, oldAlarmDef.getName(), newDescription, newDescription, + null, oldAlarmDef.getSeverity(), null, oldAlarmDef.isActionsEnabled(), null, + oldAlarmDef.getAlarmActions(), null, + oldAlarmDef.getOkActions(), null, + oldAlarmDef.getUndeterminedActions(), 1); + + doPatch(matchBy, oldAlarmDef, null, oldAlarmDef.getName(), null, oldAlarmDef.getDescription(), + newSeverity, newSeverity, null, oldAlarmDef.isActionsEnabled(), null, + oldAlarmDef.getAlarmActions(), null, + oldAlarmDef.getOkActions(), null, + oldAlarmDef.getUndeterminedActions(), 1); + + doPatch(matchBy, oldAlarmDef, null, oldAlarmDef.getName(), null, oldAlarmDef.getDescription(), + null, oldAlarmDef.getSeverity(), newEnabled, newEnabled, null, + oldAlarmDef.getAlarmActions(), null, oldAlarmDef.getOkActions(), null, + oldAlarmDef.getUndeterminedActions(), 1); + + doPatch(matchBy, oldAlarmDef, null, oldAlarmDef.getName(), null, oldAlarmDef.getDescription(), + null, oldAlarmDef.getSeverity(), null, oldAlarmDef.isActionsEnabled(), + newAlarmActions, newAlarmActions, + null, oldAlarmDef.getOkActions(), null, + oldAlarmDef.getUndeterminedActions(), 1); + + doPatch(matchBy, oldAlarmDef, null, oldAlarmDef.getName(), null, oldAlarmDef.getDescription(), + null, oldAlarmDef.getSeverity(), newEnabled, newEnabled, null, + oldAlarmDef.getAlarmActions(), newOkActions, newOkActions, null, + oldAlarmDef.getUndeterminedActions(), 2); + + doPatch(matchBy, oldAlarmDef, null, oldAlarmDef.getName(), null, oldAlarmDef.getDescription(), + null, oldAlarmDef.getSeverity(), newEnabled, newEnabled, null, + oldAlarmDef.getAlarmActions(), null, oldAlarmDef.getOkActions(), newUndeterminedActions, + newUndeterminedActions, 3); + + final List emptyActionList = new ArrayList<>(); + doPatch(matchBy, oldAlarmDef, null, oldAlarmDef.getName(), null, oldAlarmDef.getDescription(), + null, oldAlarmDef.getSeverity(), newEnabled, newEnabled, emptyActionList, + emptyActionList, null, oldAlarmDef.getOkActions(), null, + oldAlarmDef.getUndeterminedActions(), 4); + + doPatch(matchBy, oldAlarmDef, null, oldAlarmDef.getName(), null, oldAlarmDef.getDescription(), + null, oldAlarmDef.getSeverity(), newEnabled, newEnabled, null, + oldAlarmDef.getAlarmActions(), emptyActionList, emptyActionList, null, + oldAlarmDef.getUndeterminedActions(), 5); + + doPatch(matchBy, oldAlarmDef, null, oldAlarmDef.getName(), null, oldAlarmDef.getDescription(), + null, oldAlarmDef.getSeverity(), newEnabled, newEnabled, null, + oldAlarmDef.getAlarmActions(), null, oldAlarmDef.getOkActions(), emptyActionList, + emptyActionList, 6); + } + + private void doPatch(final List matchBy, final AlarmDefinition oldAlarmDef, + final String newName, final String expectedName, String newDescription, + String expectedDescription, final String newSeverity, final String expectedSeverity, + final Boolean actionsEnabled, final boolean expectedActionsEnabled, + final List newAlarmActions, final List expectedAlarmActions, + final List newOkActions, final List expectedOkActions, + final List newUndeterminedActions, final List expectedUndeterminedActions, + int expectedTimes) { + final Map emptyMap = new HashMap<>(); + final Map changedSubExpressions = new HashMap<>(); + final Map unchangedSubExpressions = new HashMap<>(); + unchangedSubExpressions.put("444", AlarmSubExpression.of(EXPR1)); + unchangedSubExpressions.put("555", AlarmSubExpression.of(EXPR2)); + + BiMap oldExpressions = + HashBiMap.create(new HashMap()); + final Set oldSubAlarmIds = oldExpressions.keySet(); + Map changedSubAlarms = new HashMap<>(); + Map newSubAlarms = new HashMap<>(); + final AlarmDefinition patchedAlarmDef = + service.patch(TENANT_ID, oldAlarmDef.getId(), newName, newDescription, newSeverity, + null, null, null, actionsEnabled, newAlarmActions, + newOkActions, newUndeterminedActions); + + final AlarmDefinition expected = + new AlarmDefinition(oldAlarmDef.getId(), expectedName, expectedDescription, + expectedSeverity, oldAlarmDef.getExpression(), matchBy, + expectedActionsEnabled, expectedAlarmActions, + expectedOkActions, expectedUndeterminedActions); + assertEquals(patchedAlarmDef, expected); + + final AlarmDefinitionUpdatedEvent event = + new AlarmDefinitionUpdatedEvent(TENANT_ID, oldAlarmDef.getId(), expectedName, + expectedDescription, oldAlarmDef.getExpression(), matchBy, + expectedActionsEnabled, expectedSeverity, emptyMap, + changedSubExpressions, unchangedSubExpressions, emptyMap); + verify(producer, times(expectedTimes)).send(new KeyedMessage(config.eventsTopic, TENANT_ID, Serialization.toJson(event))); + verify(repo).update(TENANT_ID, oldAlarmDef.getId(), true, expectedName, + expectedDescription, oldAlarmDef.getExpression(), matchBy, + expectedSeverity, expectedActionsEnabled, oldSubAlarmIds, + changedSubAlarms, newSubAlarms, newAlarmActions, newOkActions, newUndeterminedActions); + } + + private AlarmDefinition setupInitialAlarmDefinition(final List matchBy) { + final String alarmDefId = "123"; + String exprStr = EXPR1 + " or " + EXPR2; List alarmActions = Arrays.asList("1", "2", "3"); List okActions = Arrays.asList("2", "3"); List undeterminedActions = Arrays.asList("3"); - - AlarmDefinition oldAlarm = - new AlarmDefinition("123", "foo bar", "foo bar", "LOW", exprStr, null, true, alarmActions, + AlarmDefinition oldAlarmDef = + new AlarmDefinition(alarmDefId, "foo bar", "foo bar", "LOW", exprStr, matchBy, true, alarmActions, okActions, undeterminedActions); Map oldSubExpressions = new HashMap<>(); - oldSubExpressions.put("444", AlarmSubExpression.of("avg(foo{instance_id=123}) > 90")); - oldSubExpressions.put("555", AlarmSubExpression.of("avg(bar{instance_id=777}) > 80")); + oldSubExpressions.put("444", AlarmSubExpression.of(EXPR1)); + oldSubExpressions.put("555", AlarmSubExpression.of(EXPR2)); - when(repo.findById(eq("bob"), eq("123"))).thenReturn(oldAlarm); - when(repo.findSubExpressions(eq("123"))).thenReturn(oldSubExpressions); - when(notificationMethodRepo.exists(eq("bob"), anyString())).thenReturn(true); - - String newExprStr = - "avg(foo{instance_id=123}) > 90 or avg(bar{instance_id=xxxx}) > 10 or avg(baz{instance_id=654}) > 123"; - List newAlarmActions = Arrays.asList("5", "6", "7"); - List newOkActions = Arrays.asList("6", "7"); - List newUndeterminedActions = Arrays.asList("7"); - UpdateAlarmDefinitionCommand command = - new UpdateAlarmDefinitionCommand("foo bar baz", "foo bar baz", newExprStr, null, "LOW", - false, newAlarmActions, newOkActions, newUndeterminedActions); - - AlarmDefinition alarm = service.update("bob", "123", AlarmExpression.of(newExprStr), command); - - AlarmDefinition expected = - new AlarmDefinition(alarm.getId(), "foo bar baz", "foo bar baz", "LOW", newExprStr, null, - false, newAlarmActions, newOkActions, newUndeterminedActions); - assertEquals(expected, alarm); - verify(producer).send(any(KeyedMessage.class)); + when(repo.findById(eq(TENANT_ID), eq(alarmDefId))).thenReturn(oldAlarmDef); + when(repo.findSubExpressions(eq(alarmDefId))).thenReturn(oldSubExpressions); + when(notificationMethodRepo.exists(eq(TENANT_ID), anyString())).thenReturn(true); + return oldAlarmDef; } public void testOldAndNewSubExpressionsFor() { @@ -143,13 +435,12 @@ public class AlarmDefinitionServiceTest { oldSubExpressions.put("111", AlarmSubExpression.of("avg(foo{instance_id=123}) > 1")); oldSubExpressions.put("222", AlarmSubExpression.of("avg(foo{instance_id=456}) > 2")); oldSubExpressions.put("333", AlarmSubExpression.of("avg(foo{instance_id=789}) > 3")); - when(repo.findSubExpressions(eq("123"))).thenReturn(oldSubExpressions); String newExprStr = "avg(foo{instance_id=123}) > 1 or avg(foo{instance_id=456}) <= 22 or avg(foo{instance_id=444}) > 4"; AlarmExpression newExpr = AlarmExpression.of(newExprStr); - SubExpressions expressions = service.subExpressionsFor("123", newExpr); + SubExpressions expressions = service.subExpressionsFor(oldSubExpressions, newExpr); // Assert old expressions assertEquals(expressions.oldAlarmSubExpressions, @@ -167,4 +458,31 @@ public class AlarmDefinitionServiceTest { assertTrue(expressions.newAlarmSubExpressions.containsValue(AlarmSubExpression .of("avg(foo{instance_id=444}) > 4"))); } + + public void testSubExpressionsForUnchanged() { + Map oldSubExpressions = new HashMap<>(); + final String expr1 = "avg(foo{instance_id=123}) > 1"; + oldSubExpressions.put("111", AlarmSubExpression.of(expr1)); + final String expr2 = "avg(foo{instance_id=123}) < 4"; + oldSubExpressions.put("222", AlarmSubExpression.of(expr2)); + + String newExprStr = expr2 + " and " + expr1; + AlarmExpression newExpr = AlarmExpression.of(newExprStr); + + SubExpressions expressions = service.subExpressionsFor(oldSubExpressions, newExpr); + + // Assert old expressions + assertTrue(expressions.oldAlarmSubExpressions.isEmpty()); + + // Assert changed expressions + assertTrue(expressions.changedSubExpressions.isEmpty()); + + // Assert unchanged expressions + assertEquals(expressions.unchangedSubExpressions.size(), 2); + assertEquals(expressions.unchangedSubExpressions.get("111"), AlarmSubExpression.of(expr1)); + assertEquals(expressions.unchangedSubExpressions.get("222"), AlarmSubExpression.of(expr2)); + + // Assert new expressions + assertTrue(expressions.newAlarmSubExpressions.isEmpty()); + } }