From 156dcb8ccaa9b1d3e932eb1f36523abfd74b3ae4 Mon Sep 17 00:00:00 2001
From: alburthoffman <alburthoffman@gmail.com>
Date: Thu, 19 Dec 2019 18:41:11 +0800
Subject: [PATCH] =?UTF-8?q?avoid=20stopping=20rule=20groups=20if=20new=20r?=
 =?UTF-8?q?ule=20groups=20are=20as=20same=20as=20old=20rule=E2=80=A6=20(#6?=
 =?UTF-8?q?450)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* avoid stopping rule groups if new rule groups are as same as old rule groups

Signed-off-by: alburthoffman <alburthoffman@gmail.com>
---
 rules/manager.go      | 42 ++++++++++++++++++++++++----
 rules/manager_test.go | 64 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 99 insertions(+), 7 deletions(-)

diff --git a/rules/manager.go b/rules/manager.go
index 05ce3798e..21e455c84 100644
--- a/rules/manager.go
+++ b/rules/manager.go
@@ -743,6 +743,33 @@ func (g *Group) RestoreForState(ts time.Time) {
 
 }
 
+// Equals return if two groups are the same
+func (g *Group) Equals(ng *Group) bool {
+	if g.name != ng.name {
+		return false
+	}
+
+	if g.file != ng.file {
+		return false
+	}
+
+	if g.interval != ng.interval {
+		return false
+	}
+
+	if len(g.rules) != len(ng.rules) {
+		return false
+	}
+
+	for i, gr := range g.rules {
+		if gr.String() != ng.rules[i].String() {
+			return false
+		}
+	}
+
+	return true
+}
+
 // The Manager manages recording and alerting rules.
 type Manager struct {
 	opts     *ManagerOptions
@@ -836,16 +863,21 @@ func (m *Manager) Update(interval time.Duration, files []string, externalLabels
 	m.restored = true
 
 	var wg sync.WaitGroup
-
 	for _, newg := range groups {
-		wg.Add(1)
-
-		// If there is an old group with the same identifier, stop it and wait for
-		// it to finish the current iteration. Then copy it into the new group.
+		// If there is an old group with the same identifier,
+		// check if new group equals with the old group, if yes then skip it.
+		// If not equals, stop it and wait for it to finish the current iteration.
+		// Then copy it into the new group.
 		gn := groupKey(newg.name, newg.file)
 		oldg, ok := m.groups[gn]
 		delete(m.groups, gn)
 
+		if ok && oldg.Equals(newg) {
+			groups[gn] = oldg
+			continue
+		}
+
+		wg.Add(1)
 		go func(newg *Group) {
 			if ok {
 				oldg.stop()
diff --git a/rules/manager_test.go b/rules/manager_test.go
index 69ce91490..8fc8513bd 100644
--- a/rules/manager_test.go
+++ b/rules/manager_test.go
@@ -15,15 +15,20 @@ package rules
 
 import (
 	"context"
+	"fmt"
+	"io/ioutil"
 	"math"
+	"os"
 	"sort"
 	"testing"
 	"time"
 
 	"github.com/go-kit/kit/log"
 	"github.com/prometheus/common/model"
+	yaml "gopkg.in/yaml.v2"
 
 	"github.com/prometheus/prometheus/pkg/labels"
+	"github.com/prometheus/prometheus/pkg/rulefmt"
 	"github.com/prometheus/prometheus/pkg/timestamp"
 	"github.com/prometheus/prometheus/pkg/value"
 	"github.com/prometheus/prometheus/promql"
@@ -703,18 +708,73 @@ func TestUpdate(t *testing.T) {
 	err := ruleManager.Update(10*time.Second, files, nil)
 	testutil.Ok(t, err)
 	testutil.Assert(t, len(ruleManager.groups) > 0, "expected non-empty rule groups")
-	for _, g := range ruleManager.groups {
+	ogs := map[string]*Group{}
+	for h, g := range ruleManager.groups {
 		g.seriesInPreviousEval = []map[string]labels.Labels{
 			expected,
 		}
+		ogs[h] = g
 	}
 
 	err = ruleManager.Update(10*time.Second, files, nil)
 	testutil.Ok(t, err)
-	for _, g := range ruleManager.groups {
+	for h, g := range ruleManager.groups {
 		for _, actual := range g.seriesInPreviousEval {
 			testutil.Equals(t, expected, actual)
 		}
+		// Groups are the same because of no updates.
+		testutil.Equals(t, ogs[h], g)
+	}
+
+	// Groups will be recreated if updated.
+	rgs, errs := rulefmt.ParseFile("fixtures/rules.yaml")
+	testutil.Assert(t, len(errs) == 0, "file parsing failures")
+
+	tmpFile, err := ioutil.TempFile("", "rules.test.*.yaml")
+	testutil.Ok(t, err)
+	defer os.Remove(tmpFile.Name())
+	defer tmpFile.Close()
+
+	err = ruleManager.Update(10*time.Second, []string{tmpFile.Name()}, nil)
+	testutil.Ok(t, err)
+
+	for h, g := range ruleManager.groups {
+		ogs[h] = g
+	}
+
+	// Update interval and reload
+	for i, g := range rgs.Groups {
+		if g.Interval != 0 {
+			rgs.Groups[i].Interval = g.Interval * 2
+		} else {
+			rgs.Groups[i].Interval = model.Duration(10)
+		}
+
+	}
+	reloadAndValidate(rgs, t, tmpFile, ruleManager, expected, ogs)
+
+	// Change group rules and reload
+	for i, g := range rgs.Groups {
+		for j, r := range g.Rules {
+			rgs.Groups[i].Rules[j].Expr = fmt.Sprintf("%s * 0", r.Expr)
+		}
+	}
+	reloadAndValidate(rgs, t, tmpFile, ruleManager, expected, ogs)
+}
+
+func reloadAndValidate(rgs *rulefmt.RuleGroups, t *testing.T, tmpFile *os.File, ruleManager *Manager, expected map[string]labels.Labels, ogs map[string]*Group) {
+	bs, err := yaml.Marshal(rgs)
+	testutil.Ok(t, err)
+	tmpFile.Seek(0, 0)
+	_, err = tmpFile.Write(bs)
+	testutil.Ok(t, err)
+	err = ruleManager.Update(10*time.Second, []string{tmpFile.Name()}, nil)
+	testutil.Ok(t, err)
+	for h, g := range ruleManager.groups {
+		if ogs[h] == g {
+			t.Fail()
+		}
+		ogs[h] = g
 	}
 }