From e20c2fcf196f9d9eebc13ef04f440a4240e2e843 Mon Sep 17 00:00:00 2001 From: liusheng Date: Tue, 22 Sep 2015 11:02:10 +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. taken from: I5f6d7a1998104335079b7988e3d0b929b8854b28 Change-Id: I4ceb3de37640de6ba1534e70735410333810a488 Closes-Bug: #1476883 --- ceilometer/alarm/storage/base.py | 2 +- ceilometer/alarm/storage/impl_hbase.py | 5 ++ ceilometer/alarm/storage/impl_log.py | 2 +- ceilometer/alarm/storage/impl_sqlalchemy.py | 5 +- ceilometer/alarm/storage/models.py | 1 - ceilometer/alarm/storage/pymongo_base.py | 3 +- ceilometer/api/controllers/v2/alarms.py | 4 -- .../functional/api/v2/test_alarm_scenarios.py | 60 +++++-------------- .../storage/test_storage_scenarios.py | 33 +++++----- 9 files changed, 42 insertions(+), 73 deletions(-) diff --git a/ceilometer/alarm/storage/base.py b/ceilometer/alarm/storage/base.py index e331f2573c..6aaa82f81e 100644 --- a/ceilometer/alarm/storage/base.py +++ b/ceilometer/alarm/storage/base.py @@ -72,7 +72,7 @@ class Connection(object): @staticmethod def delete_alarm(alarm_id): - """Delete an alarm.""" + """Delete an alarm and its history data.""" raise ceilometer.NotImplementedError('Alarms not implemented') @staticmethod diff --git a/ceilometer/alarm/storage/impl_hbase.py b/ceilometer/alarm/storage/impl_hbase.py index a3c80fe7f8..bd61266548 100644 --- a/ceilometer/alarm/storage/impl_hbase.py +++ b/ceilometer/alarm/storage/impl_hbase.py @@ -114,9 +114,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/ceilometer/alarm/storage/impl_log.py b/ceilometer/alarm/storage/impl_log.py index d480bc41aa..0dc02aeabe 100644 --- a/ceilometer/alarm/storage/impl_log.py +++ b/ceilometer/alarm/storage/impl_log.py @@ -47,7 +47,7 @@ class Connection(base.Connection): return alarm def delete_alarm(self, alarm_id): - """Delete an alarm.""" + """Delete an alarm and its history data.""" def clear_expired_alarm_history_data(self, alarm_history_ttl): """Clear expired alarm history data from the backend storage system. diff --git a/ceilometer/alarm/storage/impl_sqlalchemy.py b/ceilometer/alarm/storage/impl_sqlalchemy.py index d53c51df04..3efd74bbad 100644 --- a/ceilometer/alarm/storage/impl_sqlalchemy.py +++ b/ceilometer/alarm/storage/impl_sqlalchemy.py @@ -216,7 +216,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 """ @@ -224,6 +224,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/ceilometer/alarm/storage/models.py b/ceilometer/alarm/storage/models.py index 6865c190c0..97be879822 100644 --- a/ceilometer/alarm/storage/models.py +++ b/ceilometer/alarm/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/ceilometer/alarm/storage/pymongo_base.py b/ceilometer/alarm/storage/pymongo_base.py index c5955c8261..7173720be6 100644 --- a/ceilometer/alarm/storage/pymongo_base.py +++ b/ceilometer/alarm/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/ceilometer/api/controllers/v2/alarms.py b/ceilometer/api/controllers/v2/alarms.py index 39ab242537..a110958384 100644 --- a/ceilometer/api/controllers/v2/alarms.py +++ b/ceilometer/api/controllers/v2/alarms.py @@ -625,10 +625,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(alarm_models.Alarm) - self._record_change(change, - timeutils.utcnow(), - type=alarm_models.AlarmChange.DELETION) @wsme_pecan.wsexpose([AlarmChange], [base.Query]) def history(self, q=None): diff --git a/ceilometer/tests/functional/api/v2/test_alarm_scenarios.py b/ceilometer/tests/functional/api/v2/test_alarm_scenarios.py index 0dfdb6cf2e..8b53777d92 100644 --- a/ceilometer/tests/functional/api/v2/test_alarm_scenarios.py +++ b/ceilometer/tests/functional/api/v2/test_alarm_scenarios.py @@ -2457,7 +2457,7 @@ class TestAlarms(v2.FunctionalTest, 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) @@ -2469,45 +2469,24 @@ class TestAlarms(v2.FunctionalTest, 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') @@ -2530,19 +2509,18 @@ class TestAlarms(v2.FunctionalTest, 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('b') @@ -2627,26 +2605,18 @@ class TestAlarms(v2.FunctionalTest, 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='ceilometer.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())) diff --git a/ceilometer/tests/functional/storage/test_storage_scenarios.py b/ceilometer/tests/functional/storage/test_storage_scenarios.py index a86dd2e12f..53249fbc74 100644 --- a/ceilometer/tests/functional/storage/test_storage_scenarios.py +++ b/ceilometer/tests/functional/storage/test_storage_scenarios.py @@ -2932,6 +2932,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): @@ -3060,24 +3071,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)) @@ -3125,11 +3121,10 @@ 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])