Fix limits cause incomplete update for existing silences

This commit fixes a bug where limits can cause an incomplete
update for existing silences, where the old silence can be
expired but the new silence cannot be created because it
would execeed the maximum number of silences.

Signed-off-by: George Robinson <[email protected]>

Author: grobinson-grafana

silence/silence.go

@@ -612,16 +612,8 @@ 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.
@@ -631,6 +623,13 @@ func (s *Silences) Set(sil *pb.Silence) error {
 		}
 	}
 
+	if ok && 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)
+		}
+	}
+
 	uid, err := uuid.NewV4()
 	if err != nil {
 		return fmt.Errorf("generate uuid: %w", err)

silence/silence_test.go

@@ -475,7 +475,7 @@ func TestSilenceLimits(t *testing.T) {
 	// 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),
 	}
@@ -492,10 +492,7 @@ func TestSilenceLimits(t *testing.T) {
 	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,16 +502,16 @@ 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),
 	}
@@ -523,8 +520,44 @@ func TestSilenceLimits(t *testing.T) {
 	// 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)
+
+	// Should be able to insert sil4 and then update it without modifications.
+	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))
+	require.NotEqual(t, "", sil4.Id)
+
+	// Update without modifications.
+	id4 := sil4.Id
+	require.NoError(t, s.Set(sil4))
+	require.Equal(t, id4, sil4.Id)
+
+	// Should be able to update the comment.
+	sil5 := cloneSilence(sil4)
+	sil5.Comment = "m"
+	require.NoError(t, s.Set(sil5))
+	require.Equal(t, sil4.Id, sil5.Id)
+
+	// Should not be able to update the start and end time as this requires
+	// sil5 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.
+	sil6 := cloneSilence(sil5)
+	sil6.StartsAt = time.Now().Add(5 * time.Minute)
+	sil6.EndsAt = time.Now().Add(10 * time.Minute)
+	err = s.Set(sil6)
+	require.Error(t, err)
+	require.EqualError(t, err, "exceeded maximum number of silences: 1 (limit: 1)")
+	require.NotEqual(t, "", sil6.Id)
+
+	// sil5 should not be expired because the update failed.
+	sil5, err = s.QueryOne(QIDs(sil5.Id))
+	require.NoError(t, err)
+	require.Equal(t, types.SilenceStateActive, getState(sil5, s.nowUTC()))
 }
 
 func TestSilenceNoLimits(t *testing.T) {