From 6244caf0584576320ee0de88bb5b0c37ba373cf5 Mon Sep 17 00:00:00 2001 From: liu-sheng Date: Thu, 23 Jul 2015 09:18:50 +0800 Subject: [PATCH] Delete its corresponding history data when deleting an alarm The history data of an alarm will still be stored in storage after the alarm being deleted. that is easy to cause residue issue. Change-Id: I5f6d7a1998104335079b7988e3d0b929b8854b28 Closes-Bug: #1476883 --- aodh/api/controllers/v2/alarms.py | 4 -- aodh/storage/base.py | 2 +- aodh/storage/impl_hbase.py | 5 ++ aodh/storage/impl_log.py | 2 +- aodh/storage/impl_sqlalchemy.py | 5 +- aodh/storage/models.py | 1 - aodh/storage/pymongo_base.py | 3 +- aodh/tests/api/v2/test_alarm_scenarios.py | 60 +++++--------------- aodh/tests/storage/test_storage_scenarios.py | 33 +++++------ 9 files changed, 42 insertions(+), 73 deletions(-) diff --git a/aodh/api/controllers/v2/alarms.py b/aodh/api/controllers/v2/alarms.py index a2cd01ed..1fc41569 100644 --- a/aodh/api/controllers/v2/alarms.py +++ b/aodh/api/controllers/v2/alarms.py @@ -615,10 +615,6 @@ class AlarmController(rest.RestController): self.conn.delete_alarm(alarm.alarm_id) alarm_object = Alarm.from_db_model(alarm) alarm_object.delete_actions() - change = alarm_object.as_dict(models.Alarm) - self._record_change(change, - timeutils.utcnow(), - type=models.AlarmChange.DELETION) @wsme_pecan.wsexpose([AlarmChange], [base.Query]) def history(self, q=None): diff --git a/aodh/storage/base.py b/aodh/storage/base.py index 3d652a50..477d4f3a 100644 --- a/aodh/storage/base.py +++ b/aodh/storage/base.py @@ -105,7 +105,7 @@ class Connection(object): @staticmethod def delete_alarm(alarm_id): - """Delete an alarm.""" + """Delete an alarm and its history data.""" raise aodh.NotImplementedError('Alarms not implemented') @staticmethod diff --git a/aodh/storage/impl_hbase.py b/aodh/storage/impl_hbase.py index 4ef1544f..af444149 100644 --- a/aodh/storage/impl_hbase.py +++ b/aodh/storage/impl_hbase.py @@ -112,9 +112,14 @@ class Connection(hbase_base.Connection, base.Connection): create_alarm = update_alarm def delete_alarm(self, alarm_id): + """Delete an alarm and its history data.""" with self.conn_pool.connection() as conn: alarm_table = conn.table(self.ALARM_TABLE) alarm_table.delete(alarm_id) + q = hbase_utils.make_query(alarm_id=alarm_id) + alarm_history_table = conn.table(self.ALARM_HISTORY_TABLE) + for alarm_id, ignored in alarm_history_table.scan(filter=q): + alarm_history_table.delete(alarm_id) def get_alarms(self, name=None, user=None, state=None, meter=None, project=None, enabled=None, alarm_id=None, diff --git a/aodh/storage/impl_log.py b/aodh/storage/impl_log.py index 3f697f70..1f1ed431 100644 --- a/aodh/storage/impl_log.py +++ b/aodh/storage/impl_log.py @@ -53,7 +53,7 @@ class Connection(base.Connection): @staticmethod def delete_alarm(alarm_id): - """Delete an alarm.""" + """Delete an alarm and its history data.""" @staticmethod def clear_expired_alarm_history_data(alarm_history_ttl): diff --git a/aodh/storage/impl_sqlalchemy.py b/aodh/storage/impl_sqlalchemy.py index d045e5a8..f73f195f 100644 --- a/aodh/storage/impl_sqlalchemy.py +++ b/aodh/storage/impl_sqlalchemy.py @@ -185,7 +185,7 @@ class Connection(base.Connection): return self._row_to_alarm_model(alarm_row) def delete_alarm(self, alarm_id): - """Delete an alarm + """Delete an alarm and its history data. :param alarm_id: ID of the alarm to delete """ @@ -193,6 +193,9 @@ class Connection(base.Connection): with session.begin(): session.query(models.Alarm).filter( models.Alarm.alarm_id == alarm_id).delete() + # FIXME(liusheng): we should use delete cascade + session.query(models.AlarmChange).filter( + models.AlarmChange.alarm_id == alarm_id).delete() @staticmethod def _row_to_alarm_change_model(row): diff --git a/aodh/storage/models.py b/aodh/storage/models.py index 567b4149..dba99b58 100644 --- a/aodh/storage/models.py +++ b/aodh/storage/models.py @@ -109,7 +109,6 @@ class AlarmChange(base.Model): CREATION = 'creation' RULE_CHANGE = 'rule change' STATE_TRANSITION = 'state transition' - DELETION = 'deletion' def __init__(self, event_id, diff --git a/aodh/storage/pymongo_base.py b/aodh/storage/pymongo_base.py index e30a11be..f172434e 100644 --- a/aodh/storage/pymongo_base.py +++ b/aodh/storage/pymongo_base.py @@ -77,8 +77,9 @@ class Connection(base.Connection): create_alarm = update_alarm def delete_alarm(self, alarm_id): - """Delete an alarm.""" + """Delete an alarm and its history data.""" self.db.alarm.remove({'alarm_id': alarm_id}) + self.db.alarm_history.remove({'alarm_id': alarm_id}) def record_alarm_change(self, alarm_change): """Record alarm change event.""" diff --git a/aodh/tests/api/v2/test_alarm_scenarios.py b/aodh/tests/api/v2/test_alarm_scenarios.py index f287ee23..e95a01c7 100644 --- a/aodh/tests/api/v2/test_alarm_scenarios.py +++ b/aodh/tests/api/v2/test_alarm_scenarios.py @@ -2290,26 +2290,18 @@ class TestAlarms(TestAlarmsBase): PayloadMatcher(), mock.ANY) def test_alarm_sends_notification(self): - # Hit the AlarmController (with alarm_id supplied) ... - data = self.get_json('/alarms') - del_alarm_name = "name1" - for d in data: - if d['name'] == del_alarm_name: - del_alarm_id = d['alarm_id'] - + alarm = self._get_alarm('a') with mock.patch.object(messaging, 'get_notifier') as get_notifier: notifier = get_notifier.return_value - - self.delete('/alarms/%s' % del_alarm_id, - headers=self.auth_headers, status=204) + self._update_alarm(alarm, dict(name='new_name')) get_notifier.assert_called_once_with(mock.ANY, publisher_id='aodh.api') calls = notifier.info.call_args_list self.assertEqual(1, len(calls)) args, _ = calls[0] context, event_type, payload = args - self.assertEqual('alarm.deletion', event_type) - self.assertEqual(del_alarm_name, payload['detail']['name']) + self.assertEqual('alarm.rule_change', event_type) + self.assertEqual('new_name', payload['detail']['name']) self.assertTrue(set(['alarm_id', 'detail', 'event_id', 'on_behalf_of', 'project_id', 'timestamp', 'type', 'user_id']).issubset(payload.keys())) @@ -2710,7 +2702,7 @@ class TestAlarmsHistory(TestAlarmsBase): history = self._get_alarm_history(self._get_alarm('a'), auth) self.assertEqual([], history) - def test_get_recorded_alarm_history_preserved_after_deletion(self): + def test_delete_alarm_history_after_deletion(self): alarm = self._get_alarm('a') history = self._get_alarm_history(alarm) self.assertEqual([], history) @@ -2722,45 +2714,24 @@ class TestAlarmsHistory(TestAlarmsBase): headers=self.auth_headers, status=204) history = self._get_alarm_history(alarm) - self.assertEqual(2, len(history)) - self._assert_is_subset(dict(alarm_id=alarm['alarm_id'], - on_behalf_of=alarm['project_id'], - project_id=alarm['project_id'], - type='deletion', - user_id=alarm['user_id']), - history[0]) - alarm['rule'] = alarm['threshold_rule'] - del alarm['threshold_rule'] - self._assert_in_json(alarm, history[0]['detail']) - detail = '{"name": "renamed"}' - self._assert_is_subset(dict(alarm_id=alarm['alarm_id'], - detail=detail, - on_behalf_of=alarm['project_id'], - project_id=alarm['project_id'], - type='rule change', - user_id=alarm['user_id']), - history[1]) + self.assertEqual(0, len(history)) def test_get_alarm_history_ordered_by_recentness(self): alarm = self._get_alarm('a') for i in moves.xrange(10): self._update_alarm(alarm, dict(name='%s' % i)) alarm = self._get_alarm('a') - self._delete_alarm(alarm) history = self._get_alarm_history(alarm) - self.assertEqual(11, len(history), 'hist: %s' % history) + self.assertEqual(10, len(history), 'hist: %s' % history) self._assert_is_subset(dict(alarm_id=alarm['alarm_id'], - type='deletion'), + type='rule change'), history[0]) - alarm['rule'] = alarm['threshold_rule'] - del alarm['threshold_rule'] - self._assert_in_json(alarm, history[0]['detail']) - for i in moves.xrange(1, 10): + for i in moves.xrange(1, 11): detail = '{"name": "%s"}' % (10 - i) self._assert_is_subset(dict(alarm_id=alarm['alarm_id'], detail=detail, type='rule change'), - history[i]) + history[i - 1]) def test_get_alarm_history_constrained_by_timestamp(self): alarm = self._get_alarm('a') @@ -2783,19 +2754,18 @@ class TestAlarmsHistory(TestAlarmsBase): def test_get_alarm_history_constrained_by_type(self): alarm = self._get_alarm('a') - self._delete_alarm(alarm) - query = dict(field='type', op='eq', value='deletion') + self._update_alarm(alarm, dict(name='renamed2')) + query = dict(field='type', op='eq', value='rule change') history = self._get_alarm_history(alarm, query=query) self.assertEqual(1, len(history)) + detail = '{"name": "renamed2"}' self._assert_is_subset(dict(alarm_id=alarm['alarm_id'], + detail=detail, on_behalf_of=alarm['project_id'], project_id=alarm['project_id'], - type='deletion', + type='rule change', user_id=alarm['user_id']), history[0]) - alarm['rule'] = alarm['threshold_rule'] - del alarm['threshold_rule'] - self._assert_in_json(alarm, history[0]['detail']) def test_get_alarm_history_constrained_by_alarm_id_failed(self): alarm = self._get_alarm('a') diff --git a/aodh/tests/storage/test_storage_scenarios.py b/aodh/tests/storage/test_storage_scenarios.py index 3d13331f..40419ac8 100644 --- a/aodh/tests/storage/test_storage_scenarios.py +++ b/aodh/tests/storage/test_storage_scenarios.py @@ -288,6 +288,17 @@ class AlarmHistoryTest(AlarmTestBase, utcnow = datetime.datetime(2014, 4, 7, 7, 45) self._clear_alarm_history(utcnow, 3 * 60, 0) + def test_delete_history_when_delete_alarm(self): + alarms = list(self.alarm_conn.get_alarms()) + self.assertEqual(3, len(alarms)) + history = list(self.alarm_conn.query_alarm_history()) + self.assertEqual(3, len(history)) + for alarm in alarms: + self.alarm_conn.delete_alarm(alarm.alarm_id) + self.assertEqual(3, len(alarms)) + history = list(self.alarm_conn.query_alarm_history()) + self.assertEqual(0, len(history)) + class ComplexAlarmQueryTest(AlarmTestBase, tests_db.MixinTestsWithBackendScenarios): @@ -416,24 +427,9 @@ class ComplexAlarmHistoryQueryTest(AlarmTestBase, self.alarm_conn.record_alarm_change(alarm_change=alarm_change3) - if alarm.name in ["red-alert", "yellow-alert"]: - alarm_change4 = dict(event_id=( - "16fd2706-8baf-433b-82eb-8c7fada847f%s" - % i), - alarm_id=alarm.alarm_id, - type=alarm_models.AlarmChange.DELETION, - detail="detail %s" % (i + 2), - user_id=alarm.user_id, - project_id=alarm.project_id, - on_behalf_of=alarm.project_id, - timestamp=datetime.datetime(2012, 9, 27, - 10 + i, - 30 + i)) - self.alarm_conn.record_alarm_change(alarm_change=alarm_change4) - def test_alarm_history_with_no_filter(self): history = list(self.alarm_conn.query_alarm_history()) - self.assertEqual(11, len(history)) + self.assertEqual(9, len(history)) def test_alarm_history_with_no_filter_and_limit(self): history = list(self.alarm_conn.query_alarm_history(limit=3)) @@ -481,9 +477,8 @@ class ComplexAlarmHistoryQueryTest(AlarmTestBase, filter_expr = {"=": {"alarm_id": "r3d"}} history = list(self.alarm_conn.query_alarm_history( filter_expr=filter_expr, orderby=[{"timestamp": "asc"}])) - self.assertEqual(4, len(history)) + self.assertEqual(3, len(history)) self.assertEqual([alarm_models.AlarmChange.CREATION, alarm_models.AlarmChange.RULE_CHANGE, - alarm_models.AlarmChange.STATE_TRANSITION, - alarm_models.AlarmChange.DELETION], + alarm_models.AlarmChange.STATE_TRANSITION], [h.type for h in history])