From 8666d3e65f0bcb27545eb8bf99e258ef3f3ca7ba Mon Sep 17 00:00:00 2001 From: Alin Sinpalean Date: Mon, 2 Oct 2017 14:36:29 +0200 Subject: [PATCH 1/3] Prevent alerts from inhibiting themselves. --- inhibit/inhibit.go | 12 ++++++--- inhibit/inhibit_test.go | 56 +++++++++++++++++++++++++++++++++++------ 2 files changed, 56 insertions(+), 12 deletions(-) diff --git a/inhibit/inhibit.go b/inhibit/inhibit.go index 101846a662..db6a252e6b 100644 --- a/inhibit/inhibit.go +++ b/inhibit/inhibit.go @@ -117,7 +117,7 @@ func (ih *Inhibitor) Mutes(lset model.LabelSet) bool { fp := lset.Fingerprint() for _, r := range ih.rules { - if inhibitedByFP, eq := r.hasEqual(lset); r.TargetMatchers.Match(lset) && eq { + if inhibitedByFP, eq := r.hasEqual(lset, fp); r.TargetMatchers.Match(lset) && eq { ih.marker.SetInhibited(fp, fmt.Sprintf("%d", inhibitedByFP)) return true } @@ -190,14 +190,18 @@ func (r *InhibitRule) set(a *types.Alert) { r.scache[a.Fingerprint()] = a } -// hasEqual checks whether the source cache contains alerts matching -// the equal labels for the given label set. -func (r *InhibitRule) hasEqual(lset model.LabelSet) (model.Fingerprint, bool) { +// hasEqual checks whether the source cache contains alerts other than itself +// matching the equal labels for the given label set. +func (r *InhibitRule) hasEqual(lset model.LabelSet, alertFp model.Fingerprint) (model.Fingerprint, bool) { r.mtx.RLock() defer r.mtx.RUnlock() Outer: for fp, a := range r.scache { + // Don't match itself. + if fp == alertFp { + continue + } // The cache might be stale and contain resolved alerts. if a.Resolved() { continue diff --git a/inhibit/inhibit_test.go b/inhibit/inhibit_test.go index d063fb9505..a7b62fe2ee 100644 --- a/inhibit/inhibit_test.go +++ b/inhibit/inhibit_test.go @@ -29,6 +29,7 @@ func TestInhibitRuleHasEqual(t *testing.T) { initial map[model.Fingerprint]*types.Alert equal model.LabelNames input model.LabelSet + inputFp model.Fingerprint result bool }{ { @@ -66,26 +67,26 @@ func TestInhibitRuleHasEqual(t *testing.T) { result: false, }, { - // Matching but already resolved. + // Matching and unresolved. initial: map[model.Fingerprint]*types.Alert{ 1: &types.Alert{ Alert: model.Alert{ - Labels: model.LabelSet{"a": "b", "c": "d"}, + Labels: model.LabelSet{"a": "b", "b": "f"}, StartsAt: now.Add(-time.Minute), EndsAt: now.Add(-time.Second), }, }, 2: &types.Alert{ Alert: model.Alert{ - Labels: model.LabelSet{"a": "b", "c": "f"}, + Labels: model.LabelSet{"a": "b", "b": "c"}, StartsAt: now.Add(-time.Minute), - EndsAt: now.Add(-time.Second), + EndsAt: now.Add(time.Hour), }, }, }, equal: model.LabelNames{"a"}, input: model.LabelSet{"a": "b"}, - result: false, + result: true, }, { // Equal label does not match. @@ -94,14 +95,14 @@ func TestInhibitRuleHasEqual(t *testing.T) { Alert: model.Alert{ Labels: model.LabelSet{"a": "c", "c": "d"}, StartsAt: now.Add(-time.Minute), - EndsAt: now.Add(-time.Second), + EndsAt: now.Add(time.Hour), }, }, 2: &types.Alert{ Alert: model.Alert{ Labels: model.LabelSet{"a": "c", "c": "f"}, StartsAt: now.Add(-time.Minute), - EndsAt: now.Add(-time.Second), + EndsAt: now.Add(time.Hour), }, }, }, @@ -109,6 +110,45 @@ func TestInhibitRuleHasEqual(t *testing.T) { input: model.LabelSet{"a": "b"}, result: false, }, + { + // Matching only itself. + initial: map[model.Fingerprint]*types.Alert{ + 13: &types.Alert{ + Alert: model.Alert{ + Labels: model.LabelSet{"a": "b", "b": "f"}, + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(time.Hour), + }, + }, + }, + equal: model.LabelNames{"a"}, + input: model.LabelSet{"a": "b"}, + inputFp: 13, + result: false, + }, + { + // Matching itself and one other. + initial: map[model.Fingerprint]*types.Alert{ + 13: &types.Alert{ + Alert: model.Alert{ + Labels: model.LabelSet{"a": "b", "b": "f"}, + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(time.Hour), + }, + }, + 2: &types.Alert{ + Alert: model.Alert{ + Labels: model.LabelSet{"a": "b", "b": "c"}, + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(time.Hour), + }, + }, + }, + equal: model.LabelNames{"a"}, + input: model.LabelSet{"a": "b"}, + inputFp: 13, + result: true, + }, } for _, c := range cases { @@ -123,7 +163,7 @@ func TestInhibitRuleHasEqual(t *testing.T) { r.scache[k] = v } - if _, have := r.hasEqual(c.input); have != c.result { + if _, have := r.hasEqual(c.input, c.inputFp); have != c.result { t.Errorf("Unexpected result %t, expected %t", have, c.result) } if !reflect.DeepEqual(r.scache, c.initial) { From be864e9cd662275dc8678582a822456388a21c92 Mon Sep 17 00:00:00 2001 From: Alin Sinpalean Date: Thu, 2 Nov 2017 10:19:53 +0100 Subject: [PATCH 2/3] Don't inhibit alerts that match the source filter. --- inhibit/inhibit.go | 12 ++--- inhibit/inhibit_test.go | 110 ++++++++++++++++++++++++---------------- 2 files changed, 69 insertions(+), 53 deletions(-) diff --git a/inhibit/inhibit.go b/inhibit/inhibit.go index d1cad81f54..ecf8c8790e 100644 --- a/inhibit/inhibit.go +++ b/inhibit/inhibit.go @@ -139,7 +139,7 @@ func (ih *Inhibitor) Mutes(lset model.LabelSet) bool { fp := lset.Fingerprint() for _, r := range ih.rules { - if inhibitedByFP, eq := r.hasEqual(lset, fp); r.TargetMatchers.Match(lset) && eq { + if inhibitedByFP, eq := r.hasEqual(lset); !r.SourceMatchers.Match(lset) && r.TargetMatchers.Match(lset) && eq { ih.marker.SetInhibited(fp, fmt.Sprintf("%d", inhibitedByFP)) return true } @@ -212,18 +212,14 @@ func (r *InhibitRule) set(a *types.Alert) { r.scache[a.Fingerprint()] = a } -// hasEqual checks whether the source cache contains alerts other than itself -// matching the equal labels for the given label set. -func (r *InhibitRule) hasEqual(lset model.LabelSet, alertFp model.Fingerprint) (model.Fingerprint, bool) { +// hasEqual checks whether the source cache contains alerts matching +// the equal labels for the given label set. +func (r *InhibitRule) hasEqual(lset model.LabelSet) (model.Fingerprint, bool) { r.mtx.RLock() defer r.mtx.RUnlock() Outer: for fp, a := range r.scache { - // Don't match itself. - if fp == alertFp { - continue - } // The cache might be stale and contain resolved alerts. if a.Resolved() { continue diff --git a/inhibit/inhibit_test.go b/inhibit/inhibit_test.go index a7b62fe2ee..9695c998af 100644 --- a/inhibit/inhibit_test.go +++ b/inhibit/inhibit_test.go @@ -19,6 +19,7 @@ import ( "time" "github.com/kylelemons/godebug/pretty" + "github.com/prometheus/alertmanager/config" "github.com/prometheus/alertmanager/types" "github.com/prometheus/common/model" ) @@ -29,7 +30,6 @@ func TestInhibitRuleHasEqual(t *testing.T) { initial map[model.Fingerprint]*types.Alert equal model.LabelNames input model.LabelSet - inputFp model.Fingerprint result bool }{ { @@ -71,14 +71,14 @@ func TestInhibitRuleHasEqual(t *testing.T) { initial: map[model.Fingerprint]*types.Alert{ 1: &types.Alert{ Alert: model.Alert{ - Labels: model.LabelSet{"a": "b", "b": "f"}, + Labels: model.LabelSet{"a": "b", "c": "d"}, StartsAt: now.Add(-time.Minute), EndsAt: now.Add(-time.Second), }, }, 2: &types.Alert{ Alert: model.Alert{ - Labels: model.LabelSet{"a": "b", "b": "c"}, + Labels: model.LabelSet{"a": "b", "c": "f"}, StartsAt: now.Add(-time.Minute), EndsAt: now.Add(time.Hour), }, @@ -95,14 +95,14 @@ func TestInhibitRuleHasEqual(t *testing.T) { Alert: model.Alert{ Labels: model.LabelSet{"a": "c", "c": "d"}, StartsAt: now.Add(-time.Minute), - EndsAt: now.Add(time.Hour), + EndsAt: now.Add(-time.Second), }, }, 2: &types.Alert{ Alert: model.Alert{ Labels: model.LabelSet{"a": "c", "c": "f"}, StartsAt: now.Add(-time.Minute), - EndsAt: now.Add(time.Hour), + EndsAt: now.Add(-time.Second), }, }, }, @@ -110,45 +110,6 @@ func TestInhibitRuleHasEqual(t *testing.T) { input: model.LabelSet{"a": "b"}, result: false, }, - { - // Matching only itself. - initial: map[model.Fingerprint]*types.Alert{ - 13: &types.Alert{ - Alert: model.Alert{ - Labels: model.LabelSet{"a": "b", "b": "f"}, - StartsAt: now.Add(-time.Minute), - EndsAt: now.Add(time.Hour), - }, - }, - }, - equal: model.LabelNames{"a"}, - input: model.LabelSet{"a": "b"}, - inputFp: 13, - result: false, - }, - { - // Matching itself and one other. - initial: map[model.Fingerprint]*types.Alert{ - 13: &types.Alert{ - Alert: model.Alert{ - Labels: model.LabelSet{"a": "b", "b": "f"}, - StartsAt: now.Add(-time.Minute), - EndsAt: now.Add(time.Hour), - }, - }, - 2: &types.Alert{ - Alert: model.Alert{ - Labels: model.LabelSet{"a": "b", "b": "c"}, - StartsAt: now.Add(-time.Minute), - EndsAt: now.Add(time.Hour), - }, - }, - }, - equal: model.LabelNames{"a"}, - input: model.LabelSet{"a": "b"}, - inputFp: 13, - result: true, - }, } for _, c := range cases { @@ -163,7 +124,7 @@ func TestInhibitRuleHasEqual(t *testing.T) { r.scache[k] = v } - if _, have := r.hasEqual(c.input, c.inputFp); have != c.result { + if _, have := r.hasEqual(c.input); have != c.result { t.Errorf("Unexpected result %t, expected %t", have, c.result) } if !reflect.DeepEqual(r.scache, c.initial) { @@ -173,6 +134,65 @@ func TestInhibitRuleHasEqual(t *testing.T) { } } +func TestInhibitRuleMatches(t *testing.T) { + // Simple inhibut rule + cr := config.InhibitRule{ + SourceMatch: map[string]string{"s": "1"}, + TargetMatch: map[string]string{"t": "1"}, + Equal: model.LabelNames{"e"}, + } + m := types.NewMarker() + ih := NewInhibitor(nil, []*config.InhibitRule{&cr}, m, nil) + ir := ih.rules[0] + now := time.Now() + // Active alert that matches the source filter + sourceAlert := types.Alert{ + Alert: model.Alert{ + Labels: model.LabelSet{"s": "1", "e": "1"}, + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(time.Hour), + }, + } + ir.scache = map[model.Fingerprint]*types.Alert{1: &sourceAlert} + + cases := []struct { + target model.LabelSet + expected bool + }{ + { + // Matches target filter, inhibited + target: model.LabelSet{"t": "1", "e": "1"}, + expected: true, + }, + { + // Matches target filter (plus noise), inhibited + target: model.LabelSet{"t": "1", "t2": "1", "e": "1"}, + expected: true, + }, + { + // Doesn't match target filter, not inhibited + target: model.LabelSet{"t": "0", "e": "1"}, + expected: false, + }, + { + // Matches both source and target filters, not inhibited + target: model.LabelSet{"s": "1", "t": "1", "e": "1"}, + expected: false, + }, + { + // Matches target filter, equal label doesn't match, not inhibited + target: model.LabelSet{"t": "1", "e": "0"}, + expected: false, + }, + } + + for _, c := range cases { + if actual := ih.Mutes(c.target); actual != c.expected { + t.Errorf("Expected (*Inhibitor).Mutes(%v) to return %t but got %t", c.target, c.expected, actual) + } + } +} + func TestInhibitRuleGC(t *testing.T) { // TODO(fabxc): add now() injection function to Resolved() to remove // dependency on machine time in this test. From e103d01c541417bb6664511a5dcdfe5a9f036268 Mon Sep 17 00:00:00 2001 From: Alin Sinpalean Date: Thu, 2 Nov 2017 10:36:19 +0100 Subject: [PATCH 3/3] No-op, nudge CircleCI. --- inhibit/inhibit.go | 1 + 1 file changed, 1 insertion(+) diff --git a/inhibit/inhibit.go b/inhibit/inhibit.go index ecf8c8790e..1d4834fab6 100644 --- a/inhibit/inhibit.go +++ b/inhibit/inhibit.go @@ -139,6 +139,7 @@ func (ih *Inhibitor) Mutes(lset model.LabelSet) bool { fp := lset.Fingerprint() for _, r := range ih.rules { + // Only inhibit if target matchers match but source matchers don't. if inhibitedByFP, eq := r.hasEqual(lset); !r.SourceMatchers.Match(lset) && r.TargetMatchers.Match(lset) && eq { ih.marker.SetInhibited(fp, fmt.Sprintf("%d", inhibitedByFP)) return true