diff --git a/inhibit/index.go b/inhibit/index.go index fd60e48701..4be77521e6 100644 --- a/inhibit/index.go +++ b/inhibit/index.go @@ -17,43 +17,112 @@ import ( "sync" "github.com/prometheus/common/model" + + "github.com/prometheus/alertmanager/types" ) -// index contains map of fingerprints to fingerprints. -// The keys are fingerprints of the equal labels of source alerts. -// The values are fingerprints of the source alerts. +// index maps fingerprints of source alert equal labels to source alerts. // For more info see comments on inhibitor and InhibitRule. type index struct { mtx sync.RWMutex - items map[model.Fingerprint]model.Fingerprint + items map[model.Fingerprint]*indexEntry +} + +type indexEntry struct { + alerts map[model.Fingerprint]indexedAlert + + any *types.Alert + sourceOnly *types.Alert +} + +type indexedAlert struct { + alert *types.Alert + sourceOnly bool } func newIndex() *index { return &index{ - items: make(map[model.Fingerprint]model.Fingerprint), + items: make(map[model.Fingerprint]*indexEntry), } } -func (c *index) Get(key model.Fingerprint) (model.Fingerprint, bool) { +func (c *index) Get(key model.Fingerprint, sourceOnly bool) (*types.Alert, bool) { c.mtx.RLock() defer c.mtx.RUnlock() - fp, ok := c.items[key] - return fp, ok + entry, ok := c.items[key] + if !ok { + return nil, false + } + + if sourceOnly { + return entry.sourceOnly, entry.sourceOnly != nil + } + + return entry.any, entry.any != nil } -func (c *index) Set(key, value model.Fingerprint) { +func (c *index) Set(key model.Fingerprint, alert *types.Alert, sourceOnly bool) { c.mtx.Lock() defer c.mtx.Unlock() - c.items[key] = value + entry, ok := c.items[key] + if !ok { + entry = &indexEntry{ + alerts: make(map[model.Fingerprint]indexedAlert), + } + c.items[key] = entry + } + + if sameFingerprint(entry.any, alert) || sameFingerprint(entry.sourceOnly, alert) { + entry.alerts[alert.Fingerprint()] = indexedAlert{ + alert: alert, + sourceOnly: sourceOnly, + } + entry.rebuild() + return + } + + entry.alerts[alert.Fingerprint()] = indexedAlert{ + alert: alert, + sourceOnly: sourceOnly, + } + + if shouldReplaceIndexAlert(entry.any, alert) { + entry.any = alert + } + if sourceOnly && shouldReplaceIndexAlert(entry.sourceOnly, alert) { + entry.sourceOnly = alert + } } -func (c *index) Delete(key model.Fingerprint) { +func (c *index) Delete(key model.Fingerprint, alert *types.Alert) { c.mtx.Lock() defer c.mtx.Unlock() - delete(c.items, key) + entry, ok := c.items[key] + if !ok { + return + } + + fp := alert.Fingerprint() + indexed, ok := entry.alerts[fp] + if !ok { + return + } + if indexed.alert != alert { + return + } + + delete(entry.alerts, fp) + if len(entry.alerts) == 0 { + delete(c.items, key) + return + } + + if entry.any == alert || entry.sourceOnly == alert { + entry.rebuild() + } } func (c *index) Len() int { @@ -62,3 +131,28 @@ func (c *index) Len() int { return len(c.items) } + +func (e *indexEntry) rebuild() { + e.any = nil + e.sourceOnly = nil + + for _, indexed := range e.alerts { + if shouldReplaceIndexAlert(e.any, indexed.alert) { + e.any = indexed.alert + } + if indexed.sourceOnly && shouldReplaceIndexAlert(e.sourceOnly, indexed.alert) { + e.sourceOnly = indexed.alert + } + } +} + +func shouldReplaceIndexAlert(current, candidate *types.Alert) bool { + if current == nil { + return true + } + return current.ResolvedAt(candidate.EndsAt) +} + +func sameFingerprint(a, b *types.Alert) bool { + return a != nil && a.Fingerprint() == b.Fingerprint() +} diff --git a/inhibit/inhibit.go b/inhibit/inhibit.go index c441054be6..3151e38a49 100644 --- a/inhibit/inhibit.go +++ b/inhibit/inhibit.go @@ -259,10 +259,9 @@ type InhibitRule struct { // Cache of alerts matching source labels. scache *store.Alerts - // Index of fingerprints of source alert equal labels to fingerprint of source alert. - // The index helps speed up source alert lookups from scache significantely in scenarios with 100s of source alerts cached. - // The index items might overwrite eachother if multiple source alerts have exact equal labels. - // Overwrites only happen if the new source alert has bigger EndsAt value. + // Index of source alerts by equal-label fingerprint. + // The index avoids scanning scache in scenarios with 100s of source alerts cached. + // Each equal-label bucket keeps the latest source alert, plus the latest source-only alert for self-inhibition checks. sindex *index } @@ -345,62 +344,29 @@ func (r *InhibitRule) fingerprintEquals(lset model.LabelSet) model.Fingerprint { // updateIndex updates the source alert index if necessary. func (r *InhibitRule) updateIndex(alert *types.Alert) { - fp := alert.Fingerprint() - // Calculate source labelset subset which is in equals. eq := r.fingerprintEquals(alert.Labels) - - // Check if the equal labelset is already in the index. - indexed, ok := r.sindex.Get(eq) - if !ok { - // If not, add it. - r.sindex.Set(eq, fp) - return - } - // If the indexed fingerprint is the same as the new fingerprint, do nothing. - if indexed == fp { - return - } - - // New alert and existing index are not the same, compare them. - existing, err := r.scache.Get(indexed) - if err != nil { - // failed to get the existing alert, overwrite the index. - r.sindex.Set(eq, fp) - return - } - - // If the new alert resolves after the existing alert, replace the index. - if existing.ResolvedAt(alert.EndsAt) { - r.sindex.Set(eq, fp) - return - } - // If the existing alert resolves after the new alert, do nothing. + r.sindex.Set(eq, alert, !r.TargetMatchers.Matches(alert.Labels)) } // findEqualSourceAlert returns the source alert that matches the equal labels of the given label set. -func (r *InhibitRule) findEqualSourceAlert(lset model.LabelSet, now time.Time) (*types.Alert, bool) { +func (r *InhibitRule) findEqualSourceAlert(lset model.LabelSet, sourceOnly bool, now time.Time) (*types.Alert, bool) { equalsFP := r.fingerprintEquals(lset) - sourceFP, ok := r.sindex.Get(equalsFP) - if ok { - alert, err := r.scache.Get(sourceFP) - if err != nil { - return nil, false - } - - if alert.ResolvedAt(now) { - return nil, false - } + alert, ok := r.sindex.Get(equalsFP, sourceOnly) + if !ok { + return nil, false + } - return alert, true + if alert.ResolvedAt(now) { + return nil, false } - return nil, false + return alert, true } func (r *InhibitRule) gcCallback(alerts []*types.Alert) { for _, a := range alerts { fp := r.fingerprintEquals(a.Labels) - r.sindex.Delete(fp) + r.sindex.Delete(fp, a) } } @@ -409,13 +375,10 @@ func (r *InhibitRule) gcCallback(alerts []*types.Alert) { // is returned. If excludeTwoSidedMatch is true, alerts that match both the // source and the target side of the rule are disregarded. func (r *InhibitRule) hasEqual(lset model.LabelSet, excludeTwoSidedMatch bool, now time.Time) (model.Fingerprint, bool) { - equal, found := r.findEqualSourceAlert(lset, now) - if found { - if excludeTwoSidedMatch && r.TargetMatchers.Matches(equal.Labels) { - return model.Fingerprint(0), false - } - return equal.Fingerprint(), found + equal, found := r.findEqualSourceAlert(lset, excludeTwoSidedMatch, now) + if !found { + return model.Fingerprint(0), false } - return model.Fingerprint(0), false + return equal.Fingerprint(), true } diff --git a/inhibit/inhibit_bench_test.go b/inhibit/inhibit_bench_test.go index fc93383964..98784248aa 100644 --- a/inhibit/inhibit_bench_test.go +++ b/inhibit/inhibit_bench_test.go @@ -62,6 +62,9 @@ func BenchmarkMutes(b *testing.B) { b.Run("1 inhibition rule, 10000 inhibiting alerts", func(b *testing.B) { benchmarkMutes(b, allRulesMatchBenchmark(b, 1, 10000)) }) + b.Run("1 inhibition rule, 10000 same-equal alerts, source-only candidate", func(b *testing.B) { + benchmarkMutes(b, sameEqualSourceOnlyBenchmark(b, 10000)) + }) b.Run("100 inhibition rules, 1000 inhibiting alerts", func(b *testing.B) { benchmarkMutes(b, allRulesMatchBenchmark(b, 100, 1000)) }) @@ -140,6 +143,58 @@ func allRulesMatchBenchmark(b *testing.B, numInhibitionRules, numInhibitingAlert } } +func sameEqualSourceOnlyBenchmark(b *testing.B, numInhibitingAlerts int) benchmarkOptions { + now := time.Now() + + return benchmarkOptions{ + n: 1, + newRuleFunc: func(_ int) amcommoncfg.InhibitRule { + return amcommoncfg.InhibitRule{ + SourceMatchers: amcommoncfg.Matchers{ + mustNewMatcher(b, labels.MatchEqual, "src", "1"), + }, + TargetMatchers: amcommoncfg.Matchers{ + mustNewMatcher(b, labels.MatchEqual, "dst", "1"), + }, + Equal: []string{"eq"}, + } + }, + newAlertsFunc: func(_ int, _ amcommoncfg.InhibitRule) []types.Alert { + alerts := make([]types.Alert, 0, numInhibitingAlerts+1) + for i := range numInhibitingAlerts { + alerts = append(alerts, types.Alert{ + Alert: model.Alert{ + Labels: model.LabelSet{ + "src": model.LabelValue("1"), + "eq": model.LabelValue("1"), + "idx": model.LabelValue(strconv.Itoa(i)), + }, + EndsAt: now.Add(time.Hour), + }, + }) + } + alerts = append(alerts, types.Alert{ + Alert: model.Alert{ + Labels: model.LabelSet{ + "src": model.LabelValue("1"), + "dst": model.LabelValue("1"), + "eq": model.LabelValue("1"), + "idx": model.LabelValue("two-sided"), + }, + EndsAt: now.Add(2 * time.Hour), + }, + }) + return alerts + }, + benchFunc: func(mutesFunc func(context.Context, model.LabelSet) bool) error { + if ok := mutesFunc(context.Background(), model.LabelSet{"src": "1", "dst": "1", "eq": "1"}); !ok { + return errors.New("expected source-and-target alert to be muted by a source-only alert") + } + return nil + }, + } +} + // lastRuleMatchesBenchmark returns a new benchmark where the last inhibition // rule inhibits the label dst=0. All other inhibition rules are no-ops. // diff --git a/inhibit/inhibit_test.go b/inhibit/inhibit_test.go index 2ae6ef38fc..4bd91f07a7 100644 --- a/inhibit/inhibit_test.go +++ b/inhibit/inhibit_test.go @@ -57,11 +57,13 @@ func TestInhibitRuleHasEqual(t *testing.T) { now := time.Now() cases := []struct { - name string - initial map[model.Fingerprint]*alert.Alert - equal model.LabelNames - input model.LabelSet - result bool + name string + initial map[model.Fingerprint]*alert.Alert + equal model.LabelNames + targetMatchers labels.Matchers + input model.LabelSet + excludeTwoSidedMatch bool + result bool }{ { name: "no source alerts", @@ -141,14 +143,41 @@ func TestInhibitRuleHasEqual(t *testing.T) { input: model.LabelSet{"a": "b"}, result: false, }, + { + name: "matching source-only alert still inhibits when newest equal source is two-sided", + initial: map[model.Fingerprint]*alert.Alert{ + 1: { + Alert: model.Alert{ + Labels: model.LabelSet{"s": "1", "e": "1"}, + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(time.Hour), + }, + }, + 2: { + Alert: model.Alert{ + Labels: model.LabelSet{"s": "1", "t": "1", "e": "1"}, + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(2 * time.Hour), + }, + }, + }, + equal: model.LabelNames{"e"}, + targetMatchers: labels.Matchers{{Type: labels.MatchEqual, Name: "t", Value: "1"}}, + input: model.LabelSet{"s": "1", "t": "1", "e": "1"}, + // The indexed two-sided source must be ignored, but the source-only + // alert with the same equal labels should still inhibit the target. + excludeTwoSidedMatch: true, + result: true, + }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { r := &InhibitRule{ - Equal: map[model.LabelName]struct{}{}, - scache: store.NewAlerts(), - sindex: newIndex(), + Equal: map[model.LabelName]struct{}{}, + TargetMatchers: c.targetMatchers, + scache: store.NewAlerts(), + sindex: newIndex(), } for _, ln := range c.equal { r.Equal[ln] = struct{}{} @@ -158,13 +187,96 @@ func TestInhibitRuleHasEqual(t *testing.T) { r.updateIndex(v) } - if _, have := r.hasEqual(c.input, false, time.Now()); have != c.result { + if _, have := r.hasEqual(c.input, c.excludeTwoSidedMatch, time.Now()); have != c.result { t.Errorf("Unexpected result %t, expected %t", have, c.result) } }) } } +func TestInhibitRuleHasEqualKeepsSourceOnlyAlertAfterGCSameEqual(t *testing.T) { + t.Parallel() + + now := time.Now() + r := &InhibitRule{ + Equal: map[model.LabelName]struct{}{ + "e": {}, + }, + TargetMatchers: labels.Matchers{{Type: labels.MatchEqual, Name: "t", Value: "1"}}, + scache: store.NewAlerts(), + sindex: newIndex(), + } + + sourceOnly := &alert.Alert{ + Alert: model.Alert{ + Labels: model.LabelSet{"s": "1", "e": "1", "id": "source-only"}, + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(time.Hour), + }, + } + expiredSameEqual := &alert.Alert{ + Alert: model.Alert{ + Labels: model.LabelSet{"s": "1", "e": "1", "id": "expired"}, + StartsAt: now.Add(-2 * time.Hour), + EndsAt: now.Add(-time.Hour), + }, + } + + require.NoError(t, r.scache.Set(sourceOnly)) + r.updateIndex(sourceOnly) + require.NoError(t, r.scache.Set(expiredSameEqual)) + r.updateIndex(expiredSameEqual) + + target := model.LabelSet{"s": "1", "t": "1", "e": "1"} + _, found := r.hasEqual(target, true, now) + require.True(t, found) + + r.gcCallback([]*alert.Alert{expiredSameEqual}) + + _, found = r.hasEqual(target, true, now) + require.True(t, found) +} + +func TestInhibitRuleGCCallbackDoesNotRemoveRefreshedSameFingerprintSourceAlert(t *testing.T) { + t.Parallel() + + now := time.Now() + r := &InhibitRule{ + Equal: map[model.LabelName]struct{}{ + "e": {}, + }, + scache: store.NewAlerts(), + sindex: newIndex(), + } + + oldSource := &alert.Alert{ + Alert: model.Alert{ + Labels: model.LabelSet{"s": "1", "e": "1"}, + StartsAt: now.Add(-2 * time.Hour), + EndsAt: now.Add(-time.Hour), + }, + UpdatedAt: now.Add(-time.Hour), + } + refreshedSource := &alert.Alert{ + Alert: model.Alert{ + Labels: model.LabelSet{"s": "1", "e": "1"}, + StartsAt: now.Add(-2 * time.Hour), + EndsAt: now.Add(time.Hour), + }, + UpdatedAt: now, + } + + require.NoError(t, r.scache.Set(oldSource)) + r.updateIndex(oldSource) + require.NoError(t, r.scache.Set(refreshedSource)) + r.updateIndex(refreshedSource) + + r.gcCallback([]*alert.Alert{oldSource}) + + _, found := r.hasEqual(model.LabelSet{"t": "1", "e": "1"}, false, now) + require.True(t, found) +} + func TestInhibitRuleMatches(t *testing.T) { t.Parallel()