vmalert-537: allow name duplication for rules within one group. (#559)

Uniqueness of rule is now defined by combination of its name, expression and
labels. The hash of the combination is now used as rule ID and identifies rule within the group.

Set of rules from coreos/kube-prometheus was added for testing purposes to
verify compatibility. The check also showed that `vmalert` doesn't support
`query` template function that was mentioned as limitation in README.

app/vmalert/README.md

@@ -20,6 +20,7 @@ may fail;
 * by default, rules execution is sequential within one group, but persisting of execution results to remote
 storage is asynchronous. Hence, user shouldn't rely on recording rules chaining when result of previous
 recording rule is reused in next one;
+* there is no `query` function support in templates yet;
 * `vmalert` has no UI, just an API for getting groups and rules statuses.
 
 ### QuickStart
@@ -85,6 +86,9 @@ and to send notifications about firing alerts to [Alertmanager](https://github.c
 Recording rules allow you to precompute frequently needed or computationally expensive expressions 
 and save their result as a new set of time series.
 
+`vmalert` forbids to define duplicates - rules with the same combination of name, expression and labels
+within one group. 
+
 ##### Alerting rules
 
 The syntax for alerting rule is following:

app/vmalert/alerting.go

@@ -18,6 +18,7 @@ import (
 
 // AlertingRule is basic alert entity
 type AlertingRule struct {
+	RuleID      uint64
 	Name        string
 	Expr        string
 	For         time.Duration
@@ -39,6 +40,7 @@ type AlertingRule struct {
 
 func newAlertingRule(gID uint64, cfg config.Rule) *AlertingRule {
 	return &AlertingRule{
+		RuleID:      cfg.ID,
 		Name:        cfg.Alert,
 		Expr:        cfg.Expr,
 		For:         cfg.For,
@@ -57,11 +59,7 @@ func (ar *AlertingRule) String() string {
 // ID returns unique Rule ID
 // within the parent Group.
 func (ar *AlertingRule) ID() uint64 {
-	hash := fnv.New64a()
-	hash.Write([]byte("alerting"))
-	hash.Write([]byte("\xff"))
-	hash.Write([]byte(ar.Name))
-	return hash.Sum64()
+	return ar.RuleID
 }
 
 // Exec executes AlertingRule expression via the given Querier.

app/vmalert/config/config.go

@@ -2,14 +2,16 @@ package config
 
 import (
 	"fmt"
-	"gopkg.in/yaml.v2"
+	"hash/fnv"
 	"io/ioutil"
 	"path/filepath"
+	"sort"
 	"strings"
 	"time"
 
 	"github.com/VictoriaMetrics/VictoriaMetrics/app/vmalert/notifier"
 	"github.com/VictoriaMetrics/metricsql"
+	"gopkg.in/yaml.v2"
 )
 
 // Group contains list of Rules grouped into
@@ -33,16 +35,16 @@ func (g *Group) Validate(validateAnnotations, validateExpressions bool) error {
 	if len(g.Rules) == 0 {
 		return fmt.Errorf("group %q can't contain no rules", g.Name)
 	}
-	uniqueRules := map[string]struct{}{}
+	uniqueRules := map[uint64]struct{}{}
 	for _, r := range g.Rules {
 		ruleName := r.Record
 		if r.Alert != "" {
 			ruleName = r.Alert
 		}
-		if _, ok := uniqueRules[ruleName]; ok {
-			return fmt.Errorf("rule name %q duplicate", ruleName)
+		if _, ok := uniqueRules[r.ID]; ok {
+			return fmt.Errorf("rule %q duplicate", ruleName)
 		}
-		uniqueRules[ruleName] = struct{}{}
+		uniqueRules[r.ID] = struct{}{}
 		if err := r.Validate(); err != nil {
 			return fmt.Errorf("invalid rule %q.%q: %s", g.Name, ruleName, err)
 		}
@@ -66,12 +68,56 @@ func (g *Group) Validate(validateAnnotations, validateExpressions bool) error {
 // Rule describes entity that represent either
 // recording rule or alerting rule.
 type Rule struct {
+	ID          uint64
 	Record      string            `yaml:"record,omitempty"`
 	Alert       string            `yaml:"alert,omitempty"`
 	Expr        string            `yaml:"expr"`
 	For         time.Duration     `yaml:"for,omitempty"`
 	Labels      map[string]string `yaml:"labels,omitempty"`
 	Annotations map[string]string `yaml:"annotations,omitempty"`
+
+	// Catches all undefined fields and must be empty after parsing.
+	XXX map[string]interface{} `yaml:",inline"`
+}
+
+// UnmarshalYAML implements the yaml.Unmarshaler interface.
+func (r *Rule) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	type rule Rule
+	if err := unmarshal((*rule)(r)); err != nil {
+		return err
+	}
+	r.ID = HashRule(*r)
+	return nil
+}
+
+// HashRule hashes significant Rule fields into
+// unique hash value
+func HashRule(r Rule) uint64 {
+	h := fnv.New64a()
+	h.Write([]byte(r.Expr))
+	if r.Record != "" {
+		h.Write([]byte("recording"))
+		h.Write([]byte(r.Record))
+	} else {
+		h.Write([]byte("alerting"))
+		h.Write([]byte(r.Alert))
+	}
+	type item struct {
+		key, value string
+	}
+	var kv []item
+	for k, v := range r.Labels {
+		kv = append(kv, item{key: k, value: v})
+	}
+	sort.Slice(kv, func(i, j int) bool {
+		return kv[i].key < kv[j].key
+	})
+	for _, i := range kv {
+		h.Write([]byte(i.key))
+		h.Write([]byte(i.value))
+		h.Write([]byte("\xff"))
+	}
+	return h.Sum64()
 }
 
 // Validate check for Rule configuration errors
@@ -82,7 +128,7 @@ func (r *Rule) Validate() error {
 	if r.Expr == "" {
 		return fmt.Errorf("expression can't be empty")
 	}
-	return nil
+	return checkOverflow(r.XXX, "rule")
 }
 
 // Parse parses rule configs from given file patterns

app/vmalert/config/config_test.go

@@ -5,6 +5,7 @@ import (
 	"os"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/VictoriaMetrics/VictoriaMetrics/app/vmalert/notifier"
 )
@@ -147,6 +148,73 @@ func TestGroup_Validate(t *testing.T) {
 			expErr:              "error parsing annotation",
 			validateAnnotations: true,
 		},
+		{
+			group: &Group{Name: "test",
+				Rules: []Rule{
+					{
+						Alert: "alert",
+						Expr:  "up == 1",
+					},
+					{
+						Alert: "alert",
+						Expr:  "up == 1",
+					},
+				},
+			},
+			expErr: "duplicate",
+		},
+		{
+			group: &Group{Name: "test",
+				Rules: []Rule{
+					{Alert: "alert", Expr: "up == 1", Labels: map[string]string{
+						"summary": "{{ value|query }}",
+					}},
+					{Alert: "alert", Expr: "up == 1", Labels: map[string]string{
+						"summary": "{{ value|query }}",
+					}},
+				},
+			},
+			expErr: "duplicate",
+		},
+		{
+			group: &Group{Name: "test",
+				Rules: []Rule{
+					{Record: "record", Expr: "up == 1", Labels: map[string]string{
+						"summary": "{{ value|query }}",
+					}},
+					{Record: "record", Expr: "up == 1", Labels: map[string]string{
+						"summary": "{{ value|query }}",
+					}},
+				},
+			},
+			expErr: "duplicate",
+		},
+		{
+			group: &Group{Name: "test",
+				Rules: []Rule{
+					{Alert: "alert", Expr: "up == 1", Labels: map[string]string{
+						"summary": "{{ value|query }}",
+					}},
+					{Alert: "alert", Expr: "up == 1", Labels: map[string]string{
+						"description": "{{ value|query }}",
+					}},
+				},
+			},
+			expErr: "",
+		},
+		{
+			group: &Group{Name: "test",
+				Rules: []Rule{
+					{Record: "alert", Expr: "up == 1", Labels: map[string]string{
+						"summary": "{{ value|query }}",
+					}},
+					{Alert: "alert", Expr: "up == 1", Labels: map[string]string{
+						"summary": "{{ value|query }}",
+					}},
+				},
+			},
+			expErr: "",
+		},
 	}
 	for _, tc := range testCases {
 		err := tc.group.Validate(tc.validateAnnotations, tc.validateExpressions)
@@ -161,3 +229,98 @@ func TestGroup_Validate(t *testing.T) {
 		}
 	}
 }
+
+func TestHashRule(t *testing.T) {
+	testCases := []struct {
+		a, b  Rule
+		equal bool
+	}{
+		{
+			Rule{Record: "record", Expr: "up == 1"},
+			Rule{Record: "record", Expr: "up == 1"},
+			true,
+		},
+		{
+			Rule{Alert: "alert", Expr: "up == 1"},
+			Rule{Alert: "alert", Expr: "up == 1"},
+			true,
+		},
+		{
+			Rule{Alert: "alert", Expr: "up == 1", Labels: map[string]string{
+				"foo": "bar",
+				"baz": "foo",
+			}},
+			Rule{Alert: "alert", Expr: "up == 1", Labels: map[string]string{
+				"foo": "bar",
+				"baz": "foo",
+			}},
+			true,
+		},
+		{
+			Rule{Alert: "alert", Expr: "up == 1", Labels: map[string]string{
+				"foo": "bar",
+				"baz": "foo",
+			}},
+			Rule{Alert: "alert", Expr: "up == 1", Labels: map[string]string{
+				"baz": "foo",
+				"foo": "bar",
+			}},
+			true,
+		},
+		{
+			Rule{Alert: "record", Expr: "up == 1"},
+			Rule{Alert: "record", Expr: "up == 1"},
+			true,
+		},
+		{
+			Rule{Alert: "alert", Expr: "up == 1", For: time.Minute},
+			Rule{Alert: "alert", Expr: "up == 1"},
+			true,
+		},
+		{
+			Rule{Alert: "record", Expr: "up == 1"},
+			Rule{Record: "record", Expr: "up == 1"},
+			false,
+		},
+		{
+			Rule{Record: "record", Expr: "up == 1"},
+			Rule{Record: "record", Expr: "up == 2"},
+			false,
+		},
+		{
+			Rule{Alert: "alert", Expr: "up == 1", Labels: map[string]string{
+				"foo": "bar",
+				"baz": "foo",
+			}},
+			Rule{Alert: "alert", Expr: "up == 1", Labels: map[string]string{
+				"baz": "foo",
+				"foo": "baz",
+			}},
+			false,
+		},
+		{
+			Rule{Alert: "alert", Expr: "up == 1", Labels: map[string]string{
+				"foo": "bar",
+				"baz": "foo",
+			}},
+			Rule{Alert: "alert", Expr: "up == 1", Labels: map[string]string{
+				"baz": "foo",
+			}},
+			false,
+		},
+		{
+			Rule{Alert: "alert", Expr: "up == 1", Labels: map[string]string{
+				"foo": "bar",
+				"baz": "foo",
+			}},
+			Rule{Alert: "alert", Expr: "up == 1"},
+			false,
+		},
+	}
+	for i, tc := range testCases {
+		aID, bID := HashRule(tc.a), HashRule(tc.b)
+		if tc.equal != (aID == bID) {
+			t.Fatalf("missmatch for rule %d", i)
+		}
+	}
+}

app/vmalert/config/testdata/rules2-good.rules

@@ -19,6 +19,12 @@ groups:
       - record: code:requests:rate5m
         expr: sum(rate(promhttp_metric_handler_requests_total[5m])) by (code)
         labels:
+          env: dev
+          recording: true
+      - record: code:requests:rate5m
+        expr: sum(rate(promhttp_metric_handler_requests_total[5m])) by (code)
+        labels:
+          env: staging
           recording: true
       - record: successful_requests:ratio_rate5m
         labels:

app/vmalert/group_test.go

@@ -13,19 +13,18 @@ import (
 func TestUpdateWith(t *testing.T) {
 	testCases := []struct {
 		name         string
-		currentRules []Rule
-		// rules must be sorted by ID
-		newRules []Rule
+		currentRules []config.Rule
+		newRules     []config.Rule
 	}{
 		{
 			"new rule",
-			[]Rule{},
-			[]Rule{&AlertingRule{Name: "bar"}},
+			nil,
+			[]config.Rule{{Alert: "bar"}},
 		},
 		{
 			"update alerting rule",
-			[]Rule{&AlertingRule{
-				Name: "foo",
+			[]config.Rule{{
+				Alert: "foo",
 				Expr:  "up > 0",
 				For:   time.Second,
 				Labels: map[string]string{
@@ -36,8 +35,8 @@ func TestUpdateWith(t *testing.T) {
 					"description": "{{$labels}}",
 				},
 			}},
-			[]Rule{&AlertingRule{
-				Name: "foo",
+			[]config.Rule{{
+				Alert: "foo",
 				Expr:  "up > 10",
 				For:   time.Second,
 				Labels: map[string]string{
@@ -50,15 +49,15 @@ func TestUpdateWith(t *testing.T) {
 		},
 		{
 			"update recording rule",
-			[]Rule{&RecordingRule{
-				Name: "foo",
+			[]config.Rule{{
+				Record: "foo",
 				Expr:   "max(up)",
 				Labels: map[string]string{
 					"bar": "baz",
 				},
 			}},
-			[]Rule{&RecordingRule{
-				Name: "foo",
+			[]config.Rule{{
+				Record: "foo",
 				Expr:   "min(up)",
 				Labels: map[string]string{
 					"baz": "bar",
@@ -67,45 +66,56 @@ func TestUpdateWith(t *testing.T) {
 		},
 		{
 			"empty rule",
-			[]Rule{&AlertingRule{Name: "foo"}, &RecordingRule{Name: "bar"}},
-			[]Rule{},
+			[]config.Rule{{Alert: "foo"}, {Record: "bar"}},
+			nil,
 		},
 		{
 			"multiple rules",
-			[]Rule{
-				&AlertingRule{Name: "bar"},
-				&AlertingRule{Name: "baz"},
-				&RecordingRule{Name: "foo"},
+			[]config.Rule{
+				{Alert: "bar"},
+				{Alert: "baz"},
+				{Alert: "foo"},
 			},
-			[]Rule{
-				&AlertingRule{Name: "baz"},
-				&RecordingRule{Name: "foo"},
+			[]config.Rule{
+				{Alert: "baz"},
+				{Record: "foo"},
 			},
 		},
 		{
 			"replace rule",
-			[]Rule{&AlertingRule{Name: "foo1"}},
-			[]Rule{&AlertingRule{Name: "foo2"}},
+			[]config.Rule{{Alert: "foo1"}},
+			[]config.Rule{{Alert: "foo2"}},
 		},
 		{
 			"replace multiple rules",
-			[]Rule{
-				&AlertingRule{Name: "foo1"},
-				&RecordingRule{Name: "foo2"},
-				&AlertingRule{Name: "foo3"},
+			[]config.Rule{
+				{Alert: "foo1"},
+				{Record: "foo2"},
+				{Alert: "foo3"},
 			},
-			[]Rule{
-				&AlertingRule{Name: "foo3"},
-				&AlertingRule{Name: "foo4"},
-				&RecordingRule{Name: "foo5"},
+			[]config.Rule{
+				{Alert: "foo3"},
+				{Alert: "foo4"},
+				{Record: "foo5"},
 			},
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			g := &Group{Rules: tc.currentRules}
-			err := g.updateWith(&Group{Rules: tc.newRules})
+			g := &Group{Name: "test"}
+			for _, r := range tc.currentRules {
+				r.ID = config.HashRule(r)
+				g.Rules = append(g.Rules, g.newRule(r))
+			}
+
+			ng := &Group{Name: "test"}
+			for _, r := range tc.newRules {
+				r.ID = config.HashRule(r)
+				ng.Rules = append(ng.Rules, ng.newRule(r))
+			}
+
+			err := g.updateWith(ng)
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -117,8 +127,11 @@ func TestUpdateWith(t *testing.T) {
 			sort.Slice(g.Rules, func(i, j int) bool {
 				return g.Rules[i].ID() < g.Rules[j].ID()
 			})
+			sort.Slice(ng.Rules, func(i, j int) bool {
+				return ng.Rules[i].ID() < ng.Rules[j].ID()
+			})
 			for i, r := range g.Rules {
-				got, want := r, tc.newRules[i]
+				got, want := r, ng.Rules[i]
 				if got.ID() != want.ID() {
 					t.Fatalf("expected to have rule %q; got %q", want, got)
 				}

app/vmalert/recording.go

@@ -18,6 +18,7 @@ import (
 // to evaluate configured Expression and
 // return TimeSeries as result.
 type RecordingRule struct {
+	RuleID  uint64
 	Name    string
 	Expr    string
 	Labels  map[string]string
@@ -41,15 +42,12 @@ func (rr *RecordingRule) String() string {
 // ID returns unique Rule ID
 // within the parent Group.
 func (rr *RecordingRule) ID() uint64 {
-	hash := fnv.New64a()
-	hash.Write([]byte("alerting"))
-	hash.Write([]byte("\xff"))
-	hash.Write([]byte(rr.Name))
-	return hash.Sum64()
+	return rr.RuleID
 }
 
 func newRecordingRule(gID uint64, cfg config.Rule) *RecordingRule {
 	return &RecordingRule{
+		RuleID:  cfg.ID,
 		Name:    cfg.Record,
 		Expr:    cfg.Expr,
 		Labels:  cfg.Labels,