From cc6de9c666a179beda15460d692e4c55c7c8ac32 Mon Sep 17 00:00:00 2001 From: George Robinson Date: Mon, 17 Jun 2024 11:45:31 +0100 Subject: [PATCH] Remove Id return from silences.Set(*pb.Silence) This commit removes the Id from the method silences.Set(*pb.Silence) as it is redundant. The Id is still set even when creating a silence fails. This will be fixed in a later change. Signed-off-by: George Robinson --- api/v2/api.go | 5 +- api/v2/api_test.go | 24 +++---- notify/notify_test.go | 8 +-- silence/silence.go | 18 ++--- silence/silence_bench_test.go | 8 +-- silence/silence_test.go | 129 ++++++++++++++++------------------ 6 files changed, 86 insertions(+), 106 deletions(-) diff --git a/api/v2/api.go b/api/v2/api.go index 6f034d25..b279b619 100644 --- a/api/v2/api.go +++ b/api/v2/api.go @@ -660,8 +660,7 @@ func (api *API) postSilencesHandler(params silence_ops.PostSilencesParams) middl return silence_ops.NewPostSilencesBadRequest().WithPayload(msg) } - sid, err := api.silences.Set(sil) - if err != nil { + if err = api.silences.Set(sil); err != nil { level.Error(logger).Log("msg", "Failed to create silence", "err", err) if errors.Is(err, silence.ErrNotFound) { return silence_ops.NewPostSilencesNotFound().WithPayload(err.Error()) @@ -670,7 +669,7 @@ func (api *API) postSilencesHandler(params silence_ops.PostSilencesParams) middl } return silence_ops.NewPostSilencesOK().WithPayload(&silence_ops.PostSilencesOKBody{ - SilenceID: sid, + SilenceID: sil.Id, }) } diff --git a/api/v2/api_test.go b/api/v2/api_test.go index d520dbde..5cabf2e0 100644 --- a/api/v2/api_test.go +++ b/api/v2/api_test.go @@ -159,8 +159,7 @@ func TestDeleteSilenceHandler(t *testing.T) { EndsAt: now.Add(time.Hour), UpdatedAt: now, } - unexpiredSid, err := silences.Set(unexpiredSil) - require.NoError(t, err) + require.NoError(t, silences.Set(unexpiredSil)) expiredSil := &silencepb.Silence{ Matchers: []*silencepb.Matcher{m}, @@ -168,9 +167,8 @@ func TestDeleteSilenceHandler(t *testing.T) { EndsAt: now.Add(time.Hour), UpdatedAt: now, } - expiredSid, err := silences.Set(expiredSil) - require.NoError(t, err) - require.NoError(t, silences.Expire(expiredSid)) + require.NoError(t, silences.Set(expiredSil)) + require.NoError(t, silences.Expire(expiredSil.Id)) for i, tc := range []struct { sid string @@ -181,11 +179,11 @@ func TestDeleteSilenceHandler(t *testing.T) { 404, }, { - unexpiredSid, + unexpiredSil.Id, 200, }, { - expiredSid, + expiredSil.Id, 200, }, } { @@ -223,8 +221,7 @@ func TestPostSilencesHandler(t *testing.T) { EndsAt: now.Add(time.Hour), UpdatedAt: now, } - unexpiredSid, err := silences.Set(unexpiredSil) - require.NoError(t, err) + require.NoError(t, silences.Set(unexpiredSil)) expiredSil := &silencepb.Silence{ Matchers: []*silencepb.Matcher{m}, @@ -232,9 +229,8 @@ func TestPostSilencesHandler(t *testing.T) { EndsAt: now.Add(time.Hour), UpdatedAt: now, } - expiredSid, err := silences.Set(expiredSil) - require.NoError(t, err) - require.NoError(t, silences.Expire(expiredSid)) + require.NoError(t, silences.Set(expiredSil)) + require.NoError(t, silences.Expire(expiredSil.Id)) t.Run("Silences CRUD", func(t *testing.T) { for i, tc := range []struct { @@ -259,14 +255,14 @@ func TestPostSilencesHandler(t *testing.T) { }, { "with an active silence ID - it extends the silence", - unexpiredSid, + unexpiredSil.Id, now.Add(time.Hour), now.Add(time.Hour * 2), 200, }, { "with an expired silence ID - it re-creates the silence", - expiredSid, + expiredSil.Id, now.Add(time.Hour), now.Add(time.Hour * 2), 200, diff --git a/notify/notify_test.go b/notify/notify_test.go index 5c2297e9..eb14c58b 100644 --- a/notify/notify_test.go +++ b/notify/notify_test.go @@ -718,11 +718,11 @@ func TestMuteStageWithSilences(t *testing.T) { if err != nil { t.Fatal(err) } - silID, err := silences.Set(&silencepb.Silence{ + sil := &silencepb.Silence{ EndsAt: utcNow().Add(time.Hour), Matchers: []*silencepb.Matcher{{Name: "mute", Pattern: "me"}}, - }) - if err != nil { + } + if err = silences.Set(sil); err != nil { t.Fatal(err) } @@ -799,7 +799,7 @@ func TestMuteStageWithSilences(t *testing.T) { } // Expire the silence and verify that no alerts are silenced now. - if err := silences.Expire(silID); err != nil { + if err := silences.Expire(sil.Id); err != nil { t.Fatal(err) } diff --git a/silence/silence.go b/silence/silence.go index 91c11429..ecd3f840 100644 --- a/silence/silence.go +++ b/silence/silence.go @@ -602,24 +602,24 @@ func (s *Silences) setSilence(sil *pb.Silence, now time.Time, skipValidate bool) // Set the specified silence. If a silence with the ID already exists and the modification // modifies history, the old silence gets expired and a new one is created. -func (s *Silences) Set(sil *pb.Silence) (string, error) { +func (s *Silences) Set(sil *pb.Silence) error { s.mtx.Lock() defer s.mtx.Unlock() now := s.nowUTC() prev, ok := s.getSilence(sil.Id) if sil.Id != "" && !ok { - return "", ErrNotFound + return ErrNotFound } if ok { if canUpdate(prev, sil, now) { - return sil.Id, s.setSilence(sil, now, false) + return s.setSilence(sil, now, false) } if getState(prev, s.nowUTC()) != types.SilenceStateExpired { // We cannot update the silence, expire the old one. if err := s.expire(prev.Id); err != nil { - return "", fmt.Errorf("expire previous silence: %w", err) + return fmt.Errorf("expire previous silence: %w", err) } } } @@ -627,13 +627,13 @@ func (s *Silences) Set(sil *pb.Silence) (string, error) { // If we got here it's either a new silence or a replacing one. if s.limits.MaxSilences != nil { if m := s.limits.MaxSilences(); m > 0 && len(s.st)+1 > m { - return "", fmt.Errorf("exceeded maximum number of silences: %d (limit: %d)", len(s.st), m) + return fmt.Errorf("exceeded maximum number of silences: %d (limit: %d)", len(s.st), m) } } uid, err := uuid.NewV4() if err != nil { - return "", fmt.Errorf("generate uuid: %w", err) + return fmt.Errorf("generate uuid: %w", err) } sil.Id = uid.String() @@ -641,11 +641,7 @@ func (s *Silences) Set(sil *pb.Silence) (string, error) { sil.StartsAt = now } - if err = s.setSilence(sil, now, false); err != nil { - return "", err - } - - return sil.Id, nil + return s.setSilence(sil, now, false) } // canUpdate returns true if silence a can be updated to b without diff --git a/silence/silence_bench_test.go b/silence/silence_bench_test.go index 146316e3..e1d39a94 100644 --- a/silence/silence_bench_test.go +++ b/silence/silence_bench_test.go @@ -58,8 +58,7 @@ func benchmarkMutes(b *testing.B, n int) { var silenceIDs []string for i := 0; i < n; i++ { - var silenceID string - silenceID, err = silences.Set(&silencepb.Silence{ + s := &silencepb.Silence{ Matchers: []*silencepb.Matcher{{ Type: silencepb.Matcher_EQUAL, Name: "foo", @@ -67,9 +66,10 @@ func benchmarkMutes(b *testing.B, n int) { }}, StartsAt: now, EndsAt: now.Add(time.Minute), - }) + } + require.NoError(b, silences.Set(s)) require.NoError(b, err) - silenceIDs = append(silenceIDs, silenceID) + silenceIDs = append(silenceIDs, s.Id) } require.Len(b, silenceIDs, n) diff --git a/silence/silence_test.go b/silence/silence_test.go index 4cc67f8c..ca425afe 100644 --- a/silence/silence_test.go +++ b/silence/silence_test.go @@ -321,14 +321,13 @@ func TestSilenceSet(t *testing.T) { StartsAt: start1.Add(2 * time.Minute), EndsAt: start1.Add(5 * time.Minute), } - id1, err := s.Set(sil1) - require.NoError(t, err) - require.NotEqual(t, "", id1) + require.NoError(t, s.Set(sil1)) + require.NotEqual(t, "", sil1.Id) want := state{ - id1: &pb.MeshSilence{ + sil1.Id: &pb.MeshSilence{ Silence: &pb.Silence{ - Id: id1, + Id: sil1.Id, Matchers: []*pb.Matcher{{Name: "a", Pattern: "b"}}, StartsAt: start1.Add(2 * time.Minute), EndsAt: start1.Add(5 * time.Minute), @@ -347,15 +346,14 @@ func TestSilenceSet(t *testing.T) { Matchers: []*pb.Matcher{{Name: "a", Pattern: "b"}}, EndsAt: start2.Add(1 * time.Minute), } - id2, err := s.Set(sil2) - require.NoError(t, err) - require.NotEqual(t, "", id2) + require.NoError(t, s.Set(sil2)) + require.NotEqual(t, "", sil2.Id) want = state{ - id1: want[id1], - id2: &pb.MeshSilence{ + sil1.Id: want[sil1.Id], + sil2.Id: &pb.MeshSilence{ Silence: &pb.Silence{ - Id: id2, + Id: sil2.Id, Matchers: []*pb.Matcher{{Name: "a", Pattern: "b"}}, StartsAt: start2, EndsAt: start2.Add(1 * time.Minute), @@ -373,15 +371,14 @@ func TestSilenceSet(t *testing.T) { sil3 := cloneSilence(sil2) sil3.EndsAt = start3.Add(100 * time.Minute) - id3, err := s.Set(sil3) - require.NoError(t, err) - require.Equal(t, id2, id3) + require.NoError(t, s.Set(sil3)) + require.Equal(t, sil2.Id, sil3.Id) want = state{ - id1: want[id1], - id2: &pb.MeshSilence{ + sil1.Id: want[sil1.Id], + sil2.Id: &pb.MeshSilence{ Silence: &pb.Silence{ - Id: id2, + Id: sil2.Id, Matchers: []*pb.Matcher{{Name: "a", Pattern: "b"}}, StartsAt: start2, EndsAt: start3.Add(100 * time.Minute), @@ -399,16 +396,15 @@ func TestSilenceSet(t *testing.T) { sil4 := cloneSilence(sil3) sil4.Matchers = []*pb.Matcher{{Name: "a", Pattern: "c"}} - id4, err := s.Set(sil4) - require.NoError(t, err) + require.NoError(t, s.Set(sil4)) // This new silence gets a new id. - require.NotEqual(t, id2, id4) + require.NotEqual(t, sil2.Id, sil4.Id) want = state{ - id1: want[id1], - id2: &pb.MeshSilence{ + sil1.Id: want[sil1.Id], + sil2.Id: &pb.MeshSilence{ Silence: &pb.Silence{ - Id: id2, + Id: sil2.Id, Matchers: []*pb.Matcher{{Name: "a", Pattern: "b"}}, StartsAt: start2, EndsAt: start4, // Expired @@ -416,9 +412,9 @@ func TestSilenceSet(t *testing.T) { }, ExpiresAt: start4.Add(s.retention), }, - id4: &pb.MeshSilence{ + sil4.Id: &pb.MeshSilence{ Silence: &pb.Silence{ - Id: id4, + Id: sil4.Id, Matchers: []*pb.Matcher{{Name: "a", Pattern: "c"}}, StartsAt: start4, EndsAt: start3.Add(100 * time.Minute), @@ -437,17 +433,16 @@ func TestSilenceSet(t *testing.T) { sil5.StartsAt = start1 sil5.EndsAt = start1.Add(5 * time.Minute) - id5, err := s.Set(sil5) - require.NoError(t, err) - require.NotEqual(t, id2, id4) + require.NoError(t, s.Set(sil5)) + require.NotEqual(t, sil2.Id, sil5.Id) want = state{ - id1: want[id1], - id2: want[id2], - id4: want[id4], - id5: &pb.MeshSilence{ + sil1.Id: want[sil1.Id], + sil2.Id: want[sil2.Id], + sil4.Id: want[sil4.Id], + sil5.Id: &pb.MeshSilence{ Silence: &pb.Silence{ - Id: id5, + Id: sil5.Id, Matchers: []*pb.Matcher{{Name: "a", Pattern: "b"}}, StartsAt: start5, // New silences have their start time set to "now" when created. EndsAt: start1.Add(5 * time.Minute), @@ -474,9 +469,8 @@ func TestSilenceLimits(t *testing.T) { StartsAt: time.Now(), EndsAt: time.Now().Add(5 * time.Minute), } - id1, err := s.Set(sil1) - require.NoError(t, err) - require.NotEqual(t, "", id1) + require.NoError(t, s.Set(sil1)) + require.NotEqual(t, "", sil1.Id) // Insert sil2 should fail because maximum number of silences // has been exceeded. @@ -485,27 +479,24 @@ func TestSilenceLimits(t *testing.T) { StartsAt: time.Now(), EndsAt: time.Now().Add(5 * time.Minute), } - id2, err := s.Set(sil2) - require.EqualError(t, err, "exceeded maximum number of silences: 1 (limit: 1)") - require.Equal(t, "", id2) + require.EqualError(t, s.Set(sil2), "exceeded maximum number of silences: 1 (limit: 1)") + require.Equal(t, "", sil2.Id) // Expire sil1 and run the GC. This should allow sil2 to be // inserted. - require.NoError(t, s.Expire(id1)) + require.NoError(t, s.Expire(sil1.Id)) n, err := s.GC() require.NoError(t, err) require.Equal(t, 1, n) - id2, err = s.Set(sil2) - require.NoError(t, err) - require.NotEqual(t, "", id2) + require.NoError(t, s.Set(sil2)) + require.NotEqual(t, "", sil2.Id) // Should be able to update sil2 without hitting the limit. - _, err = s.Set(sil2) - require.NoError(t, err) + require.NoError(t, s.Set(sil2)) // Expire sil2. - require.NoError(t, s.Expire(id2)) + require.NoError(t, s.Expire(sil2.Id)) n, err = s.GC() require.NoError(t, err) require.Equal(t, 1, n) @@ -527,12 +518,13 @@ func TestSilenceLimits(t *testing.T) { StartsAt: time.Now(), EndsAt: time.Now().Add(5 * time.Minute), } - id3, err := s.Set(sil3) + err = s.Set(sil3) require.Error(t, err) // Do not check the exact size as it can change between consecutive runs // due to padding. require.Contains(t, err.Error(), "silence exceeded maximum size") - require.Equal(t, "", id3) + // TODO: Once we fix https://github.com/prometheus/alertmanager/issues/3878 this should be require.Equal. + require.NotEqual(t, "", sil3.Id) } func TestSilenceNoLimits(t *testing.T) { @@ -548,9 +540,8 @@ func TestSilenceNoLimits(t *testing.T) { EndsAt: time.Now().Add(5 * time.Minute), Comment: strings.Repeat("c", 2<<9), } - id, err := s.Set(sil) - require.NoError(t, err) - require.NotEqual(t, "", id) + require.NoError(t, s.Set(sil)) + require.NotEqual(t, "", sil.Id) } func TestSetActiveSilence(t *testing.T) { @@ -571,7 +562,7 @@ func TestSetActiveSilence(t *testing.T) { StartsAt: startsAt, EndsAt: endsAt, } - id1, _ := s.Set(sil1) + require.NoError(t, s.Set(sil1)) // Update silence with 2 extra nanoseconds so the "seconds" part should not change @@ -579,20 +570,19 @@ func TestSetActiveSilence(t *testing.T) { newEndsAt := endsAt.Add(2 * time.Minute) sil2 := cloneSilence(sil1) - sil2.Id = id1 + sil2.Id = sil1.Id sil2.StartsAt = newStartsAt sil2.EndsAt = newEndsAt clock.Add(time.Minute) now = s.nowUTC() - id2, err := s.Set(sil2) - require.NoError(t, err) - require.Equal(t, id1, id2) + require.NoError(t, s.Set(sil2)) + require.Equal(t, sil1.Id, sil2.Id) want := state{ - id2: &pb.MeshSilence{ + sil2.Id: &pb.MeshSilence{ Silence: &pb.Silence{ - Id: id1, + Id: sil1.Id, Matchers: []*pb.Matcher{{Name: "a", Pattern: "b"}}, StartsAt: newStartsAt, EndsAt: newEndsAt, @@ -624,8 +614,7 @@ func TestSilencesSetFail(t *testing.T) { }, } for _, c := range cases { - _, err := s.Set(c.s) - checkErr(t, c.err, err) + checkErr(t, c.err, s.Set(c.s)) } } @@ -1190,22 +1179,22 @@ func TestSilencer(t *testing.T) { require.False(t, s.Mutes(model.LabelSet{"foo": "bar"}), "expected alert not silenced without any silences") - _, err = ss.Set(&pb.Silence{ + sil1 := &pb.Silence{ Matchers: []*pb.Matcher{{Name: "foo", Pattern: "baz"}}, StartsAt: now.Add(-time.Hour), EndsAt: now.Add(5 * time.Minute), - }) - require.NoError(t, err) + } + require.NoError(t, ss.Set(sil1)) require.False(t, s.Mutes(model.LabelSet{"foo": "bar"}), "expected alert not silenced by non-matching silence") - id, err := ss.Set(&pb.Silence{ + sil2 := &pb.Silence{ Matchers: []*pb.Matcher{{Name: "foo", Pattern: "bar"}}, StartsAt: now.Add(-time.Hour), EndsAt: now.Add(5 * time.Minute), - }) - require.NoError(t, err) - require.NotEmpty(t, id) + } + require.NoError(t, ss.Set(sil2)) + require.NotEmpty(t, sil2.Id) require.True(t, s.Mutes(model.LabelSet{"foo": "bar"}), "expected alert silenced by matching silence") @@ -1216,8 +1205,8 @@ func TestSilencer(t *testing.T) { require.False(t, s.Mutes(model.LabelSet{"foo": "bar"}), "expected alert not silenced by expired silence") // Update silence to start in the future. - _, err = ss.Set(&pb.Silence{ - Id: id, + err = ss.Set(&pb.Silence{ + Id: sil2.Id, Matchers: []*pb.Matcher{{Name: "foo", Pattern: "bar"}}, StartsAt: now.Add(time.Hour), EndsAt: now.Add(3 * time.Hour), @@ -1233,7 +1222,7 @@ func TestSilencer(t *testing.T) { // Exposes issue #2426. require.True(t, s.Mutes(model.LabelSet{"foo": "bar"}), "expected alert silenced by activated silence") - _, err = ss.Set(&pb.Silence{ + err = ss.Set(&pb.Silence{ Matchers: []*pb.Matcher{{Name: "foo", Pattern: "b..", Type: pb.Matcher_REGEXP}}, StartsAt: now.Add(time.Hour), EndsAt: now.Add(3 * time.Hour),