diff --git a/silence/silence.go b/silence/silence.go index 828c282c..37eadc60 100644 --- a/silence/silence.go +++ b/silence/silence.go @@ -564,6 +564,13 @@ func (s *Silences) getSilence(id string) (*pb.Silence, bool) { return msil.Silence, true } +func (s *Silences) toMeshSilence(sil *pb.Silence) *pb.MeshSilence { + return &pb.MeshSilence{ + Silence: sil, + ExpiresAt: sil.EndsAt.Add(s.retention), + } +} + func (s *Silences) setSilence(sil *pb.Silence, now time.Time, skipValidate bool) error { sil.UpdatedAt = now @@ -573,10 +580,7 @@ func (s *Silences) setSilence(sil *pb.Silence, now time.Time, skipValidate bool) } } - msil := &pb.MeshSilence{ - Silence: sil, - ExpiresAt: sil.EndsAt.Add(s.retention), - } + msil := s.toMeshSilence(sil) b, err := marshalMeshSilence(msil) if err != nil { return err @@ -612,25 +616,27 @@ func (s *Silences) Set(sil *pb.Silence) error { return ErrNotFound } - if ok { - if canUpdate(prev, sil, now) { - 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) - } - } + if ok && canUpdate(prev, sil, now) { + return s.setSilence(sil, now, false) } - // If we got here it's either a new silence or a replacing one. + // If we got here it's either a new silence or a replacing one (which would + // also create a new silence) so we need to make sure we have capacity for + // the new silence. 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) } } + if ok && getState(prev, s.nowUTC()) != types.SilenceStateExpired { + // We cannot update the silence, expire the old one to leave a history of + // the silence before modification. + if err := s.expire(prev.Id); err != nil { + return fmt.Errorf("expire previous silence: %w", err) + } + } + uid, err := uuid.NewV4() if err != nil { return fmt.Errorf("generate uuid: %w", err) diff --git a/silence/silence_test.go b/silence/silence_test.go index 98e70cda..155a1544 100644 --- a/silence/silence_test.go +++ b/silence/silence_test.go @@ -15,6 +15,7 @@ package silence import ( "bytes" + "fmt" "os" "runtime" "sort" @@ -470,32 +471,24 @@ func TestSilenceLimits(t *testing.T) { EndsAt: time.Now().Add(5 * time.Minute), } require.NoError(t, s.Set(sil1)) - require.NotEqual(t, "", sil1.Id) - // Insert sil2 should fail because maximum number of silences - // has been exceeded. + // Insert sil2 should fail because maximum number of silences has been + // exceeded. sil2 := &pb.Silence{ - Matchers: []*pb.Matcher{{Name: "a", Pattern: "b"}}, + Matchers: []*pb.Matcher{{Name: "c", Pattern: "d"}}, StartsAt: time.Now(), EndsAt: time.Now().Add(5 * time.Minute), } 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. + // Expire sil1 and run the GC. This should allow sil2 to be inserted. require.NoError(t, s.Expire(sil1.Id)) n, err := s.GC() require.NoError(t, err) require.Equal(t, 1, n) - - require.NoError(t, s.Set(sil2)) - require.NotEqual(t, "", sil2.Id) - - // Should be able to update sil2 without hitting the limit. require.NoError(t, s.Set(sil2)) - // Expire sil2. + // Expire sil2 and run the GC. require.NoError(t, s.Expire(sil2.Id)) n, err = s.GC() require.NoError(t, err) @@ -505,26 +498,55 @@ func TestSilenceLimits(t *testing.T) { sil3 := &pb.Silence{ Matchers: []*pb.Matcher{ { - Name: strings.Repeat("a", 2<<9), - Pattern: strings.Repeat("b", 2<<9), + Name: strings.Repeat("e", 2<<9), + Pattern: strings.Repeat("f", 2<<9), }, { - Name: strings.Repeat("c", 2<<9), - Pattern: strings.Repeat("d", 2<<9), + Name: strings.Repeat("g", 2<<9), + Pattern: strings.Repeat("h", 2<<9), }, }, - CreatedBy: strings.Repeat("e", 2<<9), - Comment: strings.Repeat("f", 2<<9), + CreatedBy: strings.Repeat("i", 2<<9), + Comment: strings.Repeat("j", 2<<9), StartsAt: time.Now(), EndsAt: time.Now().Add(5 * time.Minute), } - 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") - // TODO: Once we fix https://github.com/prometheus/alertmanager/issues/3878 this should be require.Equal. - require.NotEqual(t, "", sil3.Id) + require.EqualError(t, s.Set(sil3), fmt.Sprintf("silence exceeded maximum size: %d bytes (limit: 4096 bytes)", s.toMeshSilence(sil3).Size())) + + // Should be able to insert sil4. + sil4 := &pb.Silence{ + Matchers: []*pb.Matcher{{Name: "k", Pattern: "l"}}, + StartsAt: time.Now(), + EndsAt: time.Now().Add(5 * time.Minute), + } + require.NoError(t, s.Set(sil4)) + + // Should be able to update sil4 without modifications. It is expected to + // keep the same ID. + sil5 := cloneSilence(sil4) + require.NoError(t, s.Set(sil5)) + require.Equal(t, sil4.Id, sil5.Id) + + // Should be able to update the comment. It is also expected to keep the + // same ID. + sil6 := cloneSilence(sil5) + sil6.Comment = "m" + require.NoError(t, s.Set(sil6)) + require.Equal(t, sil5.Id, sil6.Id) + + // Should not be able to update the start and end time as this requires + // sil6 to be expired and a new silence to be created. However, this would + // exceed the maximum number of silences, which counts both active and + // expired silences. + sil7 := cloneSilence(sil6) + sil7.StartsAt = time.Now().Add(5 * time.Minute) + sil7.EndsAt = time.Now().Add(10 * time.Minute) + require.EqualError(t, s.Set(sil7), "exceeded maximum number of silences: 1 (limit: 1)") + + // sil6 should not be expired because the update failed. + sil6, err = s.QueryOne(QIDs(sil6.Id)) + require.NoError(t, err) + require.Equal(t, types.SilenceStateActive, getState(sil6, s.nowUTC())) } func TestSilenceNoLimits(t *testing.T) {