From 69fe3f81faefbb31eb9bab22959d1f32f8328582 Mon Sep 17 00:00:00 2001 From: Ethan Hunter Date: Wed, 23 Oct 2024 10:27:35 -0600 Subject: [PATCH] only increment silences version if a silence is added (#3961) --- silence/silence.go | 21 ++++++++++++++------- silence/silence_test.go | 16 ++++++++++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/silence/silence.go b/silence/silence.go index 829d46bb..3a4288f0 100644 --- a/silence/silence.go +++ b/silence/silence.go @@ -580,7 +580,8 @@ func (s *Silences) setSilence(msil *pb.MeshSilence, now time.Time) error { if err != nil { return err } - if s.st.merge(msil, now) { + _, added := s.st.merge(msil, now) + if added { s.version++ } s.broadcast(b) @@ -927,8 +928,11 @@ func (s *Silences) Merge(b []byte) error { now := s.nowUTC() for _, e := range st { - if merged := s.st.merge(e, now); merged { - s.version++ + merged, added := s.st.merge(e, now) + if merged { + if added { + s.version++ + } if !cluster.OversizedMessage(b) { // If this is the first we've seen the message and it's // not oversized, gossip it to other nodes. We don't @@ -953,10 +957,13 @@ func (s *Silences) SetBroadcast(f func([]byte)) { type state map[string]*pb.MeshSilence -func (s state) merge(e *pb.MeshSilence, now time.Time) bool { +// merge returns two bools: the first is true when merge caused a state change. The second +// is true if that state change added a new silence. In other words, the second return is +// true whenever a silence with a new ID has been added to the state as a result of merge. +func (s state) merge(e *pb.MeshSilence, now time.Time) (bool, bool) { id := e.Silence.Id if e.ExpiresAt.Before(now) { - return false + return false, false } // Comments list was moved to a single comment. Apply upgrade // on silences received from peers. @@ -969,9 +976,9 @@ func (s state) merge(e *pb.MeshSilence, now time.Time) bool { prev, ok := s[id] if !ok || prev.Silence.UpdatedAt.Before(e.Silence.UpdatedAt) { s[id] = e - return true + return true, !ok } - return false + return false, false } func (s state) MarshalBinary() ([]byte, error) { diff --git a/silence/silence_test.go b/silence/silence_test.go index cb14ff45..d870613a 100644 --- a/silence/silence_test.go +++ b/silence/silence_test.go @@ -428,8 +428,10 @@ func TestSilenceSet(t *testing.T) { StartsAt: start1.Add(2 * time.Minute), EndsAt: start1.Add(5 * time.Minute), } + versionBeforeOp := s.Version() require.NoError(t, s.Set(sil1)) require.NotEqual(t, "", sil1.Id) + require.NotEqual(t, versionBeforeOp, s.Version()) want := state{ sil1.Id: &pb.MeshSilence{ @@ -453,8 +455,10 @@ func TestSilenceSet(t *testing.T) { Matchers: []*pb.Matcher{{Name: "a", Pattern: "b"}}, EndsAt: start2.Add(1 * time.Minute), } + versionBeforeOp = s.Version() require.NoError(t, s.Set(sil2)) require.NotEqual(t, "", sil2.Id) + require.NotEqual(t, versionBeforeOp, s.Version()) want = state{ sil1.Id: want[sil1.Id], @@ -474,15 +478,19 @@ func TestSilenceSet(t *testing.T) { // Should be able to update silence without modifications. It is expected to // keep the same ID. sil3 := cloneSilence(sil2) + versionBeforeOp = s.Version() require.NoError(t, s.Set(sil3)) require.Equal(t, sil2.Id, sil3.Id) + require.Equal(t, versionBeforeOp, s.Version()) // Should be able to update silence with comment. It is also expected to // keep the same ID. sil4 := cloneSilence(sil3) sil4.Comment = "c" + versionBeforeOp = s.Version() require.NoError(t, s.Set(sil4)) require.Equal(t, sil3.Id, sil4.Id) + require.Equal(t, versionBeforeOp, s.Version()) // Extend sil4 to expire at a later time. This should not expire the // existing silence, and so should also keep the same ID. @@ -490,6 +498,7 @@ func TestSilenceSet(t *testing.T) { start5 := s.nowUTC() sil5 := cloneSilence(sil4) sil5.EndsAt = start5.Add(100 * time.Minute) + versionBeforeOp = s.Version() require.NoError(t, s.Set(sil5)) require.Equal(t, sil4.Id, sil5.Id) want = state{ @@ -507,6 +516,7 @@ func TestSilenceSet(t *testing.T) { }, } require.Equal(t, want, s.st, "unexpected state after silence creation") + require.Equal(t, versionBeforeOp, s.Version()) // Replace the silence sil5 with another silence with different matchers. // Unlike previous updates, changing the matchers for an existing silence @@ -518,6 +528,7 @@ func TestSilenceSet(t *testing.T) { sil6 := cloneSilence(sil5) sil6.Matchers = []*pb.Matcher{{Name: "a", Pattern: "c"}} + versionBeforeOp = s.Version() require.NoError(t, s.Set(sil6)) require.NotEqual(t, sil5.Id, sil6.Id) want = state{ @@ -546,6 +557,7 @@ func TestSilenceSet(t *testing.T) { }, } require.Equal(t, want, s.st, "unexpected state after silence creation") + require.NotEqual(t, versionBeforeOp, s.Version()) // Re-create the silence that we just replaced. Changing the start time, // just like changing the matchers, creates a new silence with a different @@ -555,6 +567,7 @@ func TestSilenceSet(t *testing.T) { sil7 := cloneSilence(sil5) sil7.StartsAt = start1 sil7.EndsAt = start1.Add(5 * time.Minute) + versionBeforeOp = s.Version() require.NoError(t, s.Set(sil7)) require.NotEqual(t, sil2.Id, sil7.Id) want = state{ @@ -574,12 +587,14 @@ func TestSilenceSet(t *testing.T) { }, } require.Equal(t, want, s.st, "unexpected state after silence creation") + require.NotEqual(t, versionBeforeOp, s.Version()) // Updating an existing silence with an invalid silence should not expire // the original silence. clock.Advance(time.Millisecond) sil8 := cloneSilence(sil7) sil8.EndsAt = time.Time{} + versionBeforeOp = s.Version() require.EqualError(t, s.Set(sil8), "invalid silence: invalid zero end timestamp") // sil7 should not be expired because the update failed. @@ -587,6 +602,7 @@ func TestSilenceSet(t *testing.T) { sil7, err = s.QueryOne(QIDs(sil7.Id)) require.NoError(t, err) require.Equal(t, types.SilenceStateActive, getState(sil7, s.nowUTC())) + require.Equal(t, versionBeforeOp, s.Version()) } func TestSilenceLimits(t *testing.T) {