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

@@ -609,16 +609,8 @@ func (s *Silences) Set(sil *pb.Silence) (string, error) {
 		return "", ErrNotFound
 	}
 
-	if ok {
-		if canUpdate(prev, sil, now) {
-			return sil.Id, 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 sil.Id, s.setSilence(sil, now, false)
 	}
 
 	// If we got here it's either a new silence or a replacing one.
@@ -628,6 +620,13 @@ func (s *Silences) Set(sil *pb.Silence) (string, 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

@@ -481,7 +481,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),
 	}
@@ -500,11 +500,7 @@ func TestSilenceLimits(t *testing.T) {
 	require.NoError(t, err)
 	require.NotEqual(t, "", id2)
 
-	// Should be able to update sil2 without hitting the limit.
-	_, err = s.Set(sil2)
-	require.NoError(t, err)
-
-	// Expire sil2.
+	// Expire sil2 and run the GC.
 	require.NoError(t, s.Expire(id2))
 	n, err = s.GC()
 	require.NoError(t, err)
@@ -514,16 +510,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),
 	}
@@ -533,6 +529,44 @@ func TestSilenceLimits(t *testing.T) {
 	// due to padding.
 	require.Contains(t, err.Error(), "silence exceeded maximum size")
 	require.Equal(t, "", id3)
+
+	// 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),
+	}
+	id4, err := s.Set(sil4)
+	require.NoError(t, err)
+	require.NotEqual(t, "", id4)
+
+	// Update without modifications.
+	updateId4, err := s.Set(sil4)
+	require.NoError(t, err)
+	require.Equal(t, id4, updateId4)
+
+	// Should be able to update the comment.
+	sil5 := cloneSilence(sil4)
+	sil5.Comment = "m"
+	id5, err := s.Set(sil5)
+	require.NoError(t, err)
+	require.Equal(t, id4, id5)
+
+	// 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)
+	id6, err := s.Set(sil6)
+	require.EqualError(t, err, "exceeded maximum number of silences: 1 (limit: 1)")
+	require.Equal(t, "", id6)
+
+	// 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 TestSetActiveSilence(t *testing.T) {