From 8b8013ab21c5154f13bae223aa551acdd87b9f86 Mon Sep 17 00:00:00 2001 From: Emmanuel Lodovice Date: Wed, 21 Feb 2024 17:44:21 -0800 Subject: [PATCH 01/12] Allowing ruler replication to be configurable Signed-off-by: Emmanuel Lodovice --- docs/configuration/config-file-reference.md | 9 +++++++++ pkg/ruler/ruler_ring.go | 18 +++++++++++++----- pkg/ruler/ruler_test.go | 11 ++++++++--- 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/docs/configuration/config-file-reference.md b/docs/configuration/config-file-reference.md index f28da2b8cf5..52be35d17e1 100644 --- a/docs/configuration/config-file-reference.md +++ b/docs/configuration/config-file-reference.md @@ -4233,6 +4233,15 @@ ring: # CLI flag: -ruler.ring.heartbeat-timeout [heartbeat_timeout: | default = 1m] + # The replication factor to use when loading rule groups for API HA. + # CLI flag: -ruler.ring.replication-factor + [replication_factor: | default = 1] + + # True to enable zone-awareness and load rule groups across different + # availability zones for API HA. + # CLI flag: -ruler.ring.zone-awareness-enabled + [zone_awareness_enabled: | default = false] + # Name of network interface to read address from. # CLI flag: -ruler.ring.instance-interface-names [instance_interface_names: | default = [eth0 en0]] diff --git a/pkg/ruler/ruler_ring.go b/pkg/ruler/ruler_ring.go index a2078d56dbe..7e7a5279601 100644 --- a/pkg/ruler/ruler_ring.go +++ b/pkg/ruler/ruler_ring.go @@ -38,15 +38,18 @@ var ListRuleRingOp = ring.NewOp([]ring.InstanceState{ring.ACTIVE, ring.LEAVING}, // is used to strip down the config to the minimum, and avoid confusion // to the user. type RingConfig struct { - KVStore kv.Config `yaml:"kvstore"` - HeartbeatPeriod time.Duration `yaml:"heartbeat_period"` - HeartbeatTimeout time.Duration `yaml:"heartbeat_timeout"` + KVStore kv.Config `yaml:"kvstore"` + HeartbeatPeriod time.Duration `yaml:"heartbeat_period"` + HeartbeatTimeout time.Duration `yaml:"heartbeat_timeout"` + ReplicationFactor int `yaml:"replication_factor"` + ZoneAwarenessEnabled bool `yaml:"zone_awareness_enabled"` // Instance details InstanceID string `yaml:"instance_id" doc:"hidden"` InstanceInterfaceNames []string `yaml:"instance_interface_names"` InstancePort int `yaml:"instance_port" doc:"hidden"` InstanceAddr string `yaml:"instance_addr" doc:"hidden"` + InstanceZone string `yaml:"instance_availability_zone" doc:"hidden"` NumTokens int `yaml:"num_tokens"` FinalSleep time.Duration `yaml:"final_sleep"` @@ -70,6 +73,8 @@ func (cfg *RingConfig) RegisterFlags(f *flag.FlagSet) { f.DurationVar(&cfg.HeartbeatPeriod, "ruler.ring.heartbeat-period", 5*time.Second, "Period at which to heartbeat to the ring. 0 = disabled.") f.DurationVar(&cfg.HeartbeatTimeout, "ruler.ring.heartbeat-timeout", time.Minute, "The heartbeat timeout after which rulers are considered unhealthy within the ring. 0 = never (timeout disabled).") f.DurationVar(&cfg.FinalSleep, "ruler.ring.final-sleep", 0*time.Second, "The sleep seconds when ruler is shutting down. Need to be close to or larger than KV Store information propagation delay") + f.IntVar(&cfg.ReplicationFactor, "ruler.ring.replication-factor", 1, "The replication factor to use when loading rule groups for API HA.") + f.BoolVar(&cfg.ZoneAwarenessEnabled, "ruler.ring.zone-awareness-enabled", false, "True to enable zone-awareness and load rule groups across different availability zones for API HA.") // Instance flags cfg.InstanceInterfaceNames = []string{"eth0", "en0"} @@ -77,6 +82,7 @@ func (cfg *RingConfig) RegisterFlags(f *flag.FlagSet) { f.StringVar(&cfg.InstanceAddr, "ruler.ring.instance-addr", "", "IP address to advertise in the ring.") f.IntVar(&cfg.InstancePort, "ruler.ring.instance-port", 0, "Port to advertise in the ring (defaults to server.grpc-listen-port).") f.StringVar(&cfg.InstanceID, "ruler.ring.instance-id", hostname, "Instance ID to register in the ring.") + f.StringVar(&cfg.InstanceZone, "ruler.ring.instance-availability-zone", "", "The availability zone where this instance is running. Required if zone-awareness is enabled.") f.IntVar(&cfg.NumTokens, "ruler.ring.num-tokens", 128, "Number of tokens for each ruler.") } @@ -93,6 +99,7 @@ func (cfg *RingConfig) ToLifecyclerConfig(logger log.Logger) (ring.BasicLifecycl return ring.BasicLifecyclerConfig{ ID: cfg.InstanceID, Addr: fmt.Sprintf("%s:%d", instanceAddr, instancePort), + Zone: cfg.InstanceZone, HeartbeatPeriod: cfg.HeartbeatPeriod, TokensObservePeriod: 0, NumTokens: cfg.NumTokens, @@ -107,9 +114,10 @@ func (cfg *RingConfig) ToRingConfig() ring.Config { rc.KVStore = cfg.KVStore rc.HeartbeatTimeout = cfg.HeartbeatTimeout rc.SubringCacheDisabled = true + rc.ZoneAwarenessEnabled = cfg.ZoneAwarenessEnabled - // Each rule group is loaded to *exactly* one ruler. - rc.ReplicationFactor = 1 + // Each rule group is evaluated by *exactly* one ruler, but it can be loaded by multiple rulers for API HA + rc.ReplicationFactor = cfg.ReplicationFactor return rc } diff --git a/pkg/ruler/ruler_test.go b/pkg/ruler/ruler_test.go index f7187df1ec8..7978ad6e28d 100644 --- a/pkg/ruler/ruler_test.go +++ b/pkg/ruler/ruler_test.go @@ -75,6 +75,7 @@ func defaultRulerConfig(t testing.TB) Config { cfg.Ring.InstanceAddr = "localhost" cfg.Ring.InstanceID = "localhost" cfg.Ring.FinalSleep = 0 + cfg.Ring.ReplicationFactor = 1 cfg.EnableQueryStats = false return cfg @@ -619,6 +620,7 @@ func TestGetRules(t *testing.T) { KVStore: kv.Config{ Mock: kvStore, }, + ReplicationFactor: 1, } r, _ := buildRuler(t, cfg, nil, store, rulerAddrMap) @@ -1124,7 +1126,8 @@ func TestSharding(t *testing.T) { KVStore: kv.Config{ Mock: kvStore, }, - HeartbeatTimeout: 1 * time.Minute, + HeartbeatTimeout: 1 * time.Minute, + ReplicationFactor: 1, }, FlushCheckPeriod: 0, EnabledTenants: tc.enabledUsers, @@ -1261,7 +1264,8 @@ func Test_LoadPartialGroups(t *testing.T) { KVStore: kv.Config{ Mock: kvStore, }, - HeartbeatTimeout: 1 * time.Minute, + HeartbeatTimeout: 1 * time.Minute, + ReplicationFactor: 1, }, FlushCheckPeriod: 0, } @@ -1784,7 +1788,8 @@ func TestRulerDisablesRuleGroups(t *testing.T) { KVStore: kv.Config{ Mock: kvStore, }, - HeartbeatTimeout: 1 * time.Minute, + HeartbeatTimeout: 1 * time.Minute, + ReplicationFactor: 1, }, FlushCheckPeriod: 0, } From bcc7af4a34bced94170598ac7c93fecc140ac5f1 Mon Sep 17 00:00:00 2001 From: Emmanuel Lodovice Date: Tue, 27 Feb 2024 13:41:13 -0800 Subject: [PATCH 02/12] Allow rules to be loaded to rulers as backup for List rules API HA Signed-off-by: Emmanuel Lodovice --- CHANGELOG.md | 1 + docs/configuration/config-file-reference.md | 15 + pkg/ruler/api.go | 3 + pkg/ruler/manager.go | 14 +- pkg/ruler/manager_test.go | 49 +++ pkg/ruler/merger.go | 34 ++ pkg/ruler/merger_test.go | 114 ++++++ pkg/ruler/rule_backup_manager.go | 156 ++++++++ pkg/ruler/rule_backup_manager_test.go | 228 +++++++++++ pkg/ruler/ruler.go | 222 +++++++++-- pkg/ruler/ruler_test.go | 414 ++++++++++++++++---- 11 files changed, 1126 insertions(+), 124 deletions(-) create mode 100644 pkg/ruler/merger.go create mode 100644 pkg/ruler/merger_test.go create mode 100644 pkg/ruler/rule_backup_manager.go create mode 100644 pkg/ruler/rule_backup_manager_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 15bbbd83515..e319d2d1105 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ * [FEATURE] Ruler: Add `ruler.concurrent-evals-enabled` flag to enable concurrent evaluation within a single rule group for independent rules. Maximum concurrency can be configured via `ruler.max-concurrent-evals`. #5766 * [FEATURE] Distributor Queryable: Experimental: Add config `zone_results_quorum_metadata`. When querying ingesters using metadata APIs such as label names and values, only results from quorum number of zones will be included and merged. #5779 * [FEATURE] Storage Cache Clients: Add config `set_async_circuit_breaker_config` to utilize the circuit breaker pattern for dynamically thresholding asynchronous set operations. Implemented in both memcached and redis cache clients. #5789 +* [FEATURE] Ruler: Add `ruler.api-deduplicate-rules` flag to remove duplicate rule groups from the Prometheus compatible rules API endpoint. Add `ruler.ring.replication-factor` and `ruler.ring.zone-awareness-enabled` flags to configure rule group replication, but only the first ruler in the replicaset evaluates the rule group, the rest will just hold a copy as backup. Add `ruler.api-enable-rules-backup` flag to configure rulers to send the rule group backups stored in the replicaset to handle events when a ruler is down during an API request to list rules. #5782 * [ENHANCEMENT] Store Gateway: Added `-store-gateway.enabled-tenants` and `-store-gateway.disabled-tenants` to explicitly enable or disable store-gateway for specific tenants. #5638 * [ENHANCEMENT] Compactor: Add new compactor metric `cortex_compactor_start_duration_seconds`. #5683 * [ENHANCEMENT] Upgraded Docker base images to `alpine:3.18`. #5684 diff --git a/docs/configuration/config-file-reference.md b/docs/configuration/config-file-reference.md index 52be35d17e1..93e327fd50c 100644 --- a/docs/configuration/config-file-reference.md +++ b/docs/configuration/config-file-reference.md @@ -4263,6 +4263,21 @@ ring: # CLI flag: -experimental.ruler.enable-api [enable_api: | default = false] +# Enable rulers to store a copy of rules owned by other rulers with default +# state (state before any evaluation) and send this copy in list API requests as +# backup in case the ruler who owns the rule fails to send its rules. This +# allows the rules API to handle ruler outage by returning rules with default +# state. Ring replication-factor needs to be set to 3 or more for this to be +# useful. +# CLI flag: -ruler.api-enable-rules-backup +[api_enable_rules_backup: | default = false] + +# Remove duplicate rules in the prometheus rules and alerts API response. If +# there are duplicate rules the rule with the latest evaluation timestamp will +# be kept. +# CLI flag: -ruler.api-deduplicate-rules +[api_deduplicate_rules: | default = false] + # Comma separated list of tenants whose rules this ruler can evaluate. If # specified, only these tenants will be handled by ruler, otherwise this ruler # can process rules from all tenants. Subject to sharding. diff --git a/pkg/ruler/api.go b/pkg/ruler/api.go index bcb3df70dde..225f65683f2 100644 --- a/pkg/ruler/api.go +++ b/pkg/ruler/api.go @@ -258,6 +258,9 @@ func (a *API) PrometheusRules(w http.ResponseWriter, req *http.Request) { // keep data.groups are in order sort.Slice(groups, func(i, j int) bool { + if groups[i].File == groups[j].File { + return groups[i].Name < groups[j].Name + } return groups[i].File < groups[j].File }) diff --git a/pkg/ruler/manager.go b/pkg/ruler/manager.go index 95bf7c43ccb..be3784ab628 100644 --- a/pkg/ruler/manager.go +++ b/pkg/ruler/manager.go @@ -44,6 +44,9 @@ type DefaultMultiTenantManager struct { notifiers map[string]*rulerNotifier notifiersDiscoveryMetrics map[string]discovery.DiscovererMetrics + // rules backup + rulesBackupManager *rulesBackupManager + managersTotal prometheus.Gauge lastReloadSuccessful *prometheus.GaugeVec lastReloadSuccessfulTimestamp *prometheus.GaugeVec @@ -90,6 +93,7 @@ func NewDefaultMultiTenantManager(cfg Config, managerFactory ManagerFactory, eva userManagers: map[string]RulesManager{}, userManagerMetrics: userManagerMetrics, ruleCache: map[string][]*promRules.Group{}, + rulesBackupManager: newRulesBackupManager(cfg, logger, reg), managersTotal: promauto.With(reg).NewGauge(prometheus.GaugeOpts{ Namespace: "cortex", Name: "ruler_managers_total", @@ -161,8 +165,12 @@ func (r *DefaultMultiTenantManager) deleteRuleCache(user string) { delete(r.ruleCache, user) } +func (r *DefaultMultiTenantManager) BackUpRuleGroups(ctx context.Context, ruleGroups map[string]rulespb.RuleGroupList) { + r.rulesBackupManager.backUpRuleGroups(ctx, ruleGroups) +} + // syncRulesToManager maps the rule files to disk, detects any changes and will create/update the -// the users Prometheus Rules Manager. +// users Prometheus Rules Manager. func (r *DefaultMultiTenantManager) syncRulesToManager(ctx context.Context, user string, groups rulespb.RuleGroupList) { // Map the files to disk and return the file names to be passed to the users manager if they // have been updated @@ -333,6 +341,10 @@ func (r *DefaultMultiTenantManager) GetRules(userID string) []*promRules.Group { return groups } +func (r *DefaultMultiTenantManager) GetBackupRules(userID string) []*promRules.Group { + return r.rulesBackupManager.getRuleGroups(userID) +} + func (r *DefaultMultiTenantManager) Stop() { r.notifiersMtx.Lock() for _, n := range r.notifiers { diff --git a/pkg/ruler/manager_test.go b/pkg/ruler/manager_test.go index ec95205a14c..65b3b8c1152 100644 --- a/pkg/ruler/manager_test.go +++ b/pkg/ruler/manager_test.go @@ -253,6 +253,55 @@ func TestSyncRuleGroupsCleanUpPerUserMetrics(t *testing.T) { require.NotContains(t, mfm["cortex_ruler_config_last_reload_successful"].String(), "value:\""+user+"\"") } +func TestBackupRules(t *testing.T) { + dir := t.TempDir() + reg := prometheus.NewPedanticRegistry() + evalMetrics := NewRuleEvalMetrics(Config{RulePath: dir, EnableQueryStats: true}, reg) + m, err := NewDefaultMultiTenantManager(Config{RulePath: dir}, factory, evalMetrics, reg, log.NewNopLogger()) + require.NoError(t, err) + + const user1 = "testUser" + const user2 = "testUser2" + + require.Equal(t, 0, len(m.GetBackupRules(user1))) + require.Equal(t, 0, len(m.GetBackupRules(user2))) + + userRules := map[string]rulespb.RuleGroupList{ + user1: { + &rulespb.RuleGroupDesc{ + Name: "group1", + Namespace: "ns", + Interval: 1 * time.Minute, + User: user1, + }, + }, + user2: { + &rulespb.RuleGroupDesc{ + Name: "group2", + Namespace: "ns", + Interval: 1 * time.Minute, + User: user1, + }, + }, + } + m.BackUpRuleGroups(context.TODO(), userRules) + managerOptions := &promRules.ManagerOptions{} + g1 := promRules.NewGroup(promRules.GroupOptions{ + Name: userRules[user1][0].Name, + File: userRules[user1][0].Namespace, + Interval: userRules[user1][0].Interval, + Opts: managerOptions, + }) + g2 := promRules.NewGroup(promRules.GroupOptions{ + Name: userRules[user2][0].Name, + File: userRules[user2][0].Namespace, + Interval: userRules[user2][0].Interval, + Opts: managerOptions, + }) + requireGroupsEqual(t, m.GetBackupRules(user1), []*promRules.Group{g1}) + requireGroupsEqual(t, m.GetBackupRules(user2), []*promRules.Group{g2}) +} + func getManager(m *DefaultMultiTenantManager, user string) RulesManager { m.userManagerMtx.RLock() defer m.userManagerMtx.RUnlock() diff --git a/pkg/ruler/merger.go b/pkg/ruler/merger.go new file mode 100644 index 00000000000..7ae7e69317b --- /dev/null +++ b/pkg/ruler/merger.go @@ -0,0 +1,34 @@ +package ruler + +import ( + "time" + + promRules "github.com/prometheus/prometheus/rules" +) + +// mergeGroupStateDesc removes duplicates from the provided []*GroupStateDesc by keeping the GroupStateDesc with the +// latest information. It uses the EvaluationTimestamp of the GroupStateDesc and the EvaluationTimestamp of the +// ActiveRules in a GroupStateDesc to determine the which GroupStateDesc has the latest information. +func mergeGroupStateDesc(in []*GroupStateDesc) []*GroupStateDesc { + states := make(map[string]*GroupStateDesc) + rgTime := make(map[string]time.Time) + for _, state := range in { + latestTs := state.EvaluationTimestamp + for _, r := range state.ActiveRules { + if latestTs.Before(r.EvaluationTimestamp) { + latestTs = r.EvaluationTimestamp + } + } + key := promRules.GroupKey(state.Group.Namespace, state.Group.Name) + ts, ok := rgTime[key] + if !ok || ts.Before(latestTs) { + states[key] = state + rgTime[key] = latestTs + } + } + groups := make([]*GroupStateDesc, 0, len(states)) + for _, state := range states { + groups = append(groups, state) + } + return groups +} diff --git a/pkg/ruler/merger_test.go b/pkg/ruler/merger_test.go new file mode 100644 index 00000000000..d4bae6d008e --- /dev/null +++ b/pkg/ruler/merger_test.go @@ -0,0 +1,114 @@ +package ruler + +import ( + "reflect" + "slices" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/cortexproject/cortex/pkg/ruler/rulespb" +) + +func TestMergeGroupStateDesc(t *testing.T) { + curTime := time.Now() + r := rulespb.RuleDesc{ + Expr: "1 > 1", + } + g1 := rulespb.RuleGroupDesc{ + Name: "g1", + Namespace: "ns1", + } + g2 := rulespb.RuleGroupDesc{ + Name: "g2", + Namespace: "ns1", + } + rs1 := RuleStateDesc{ + Rule: &r, + EvaluationTimestamp: curTime, + } + rs1NotRun := RuleStateDesc{ + Rule: &r, + } + rs2 := RuleStateDesc{ + Rule: &r, + EvaluationTimestamp: curTime, + } + rs2NotRun := RuleStateDesc{ + Rule: &r, + } + rs3 := RuleStateDesc{ + Rule: &r, + EvaluationTimestamp: curTime.Add(10 * time.Second), + } + + gs1 := GroupStateDesc{ + Group: &g1, + ActiveRules: []*RuleStateDesc{&rs1, &rs2}, + EvaluationTimestamp: curTime, + } + gs1NotRun := GroupStateDesc{ + Group: &g1, + ActiveRules: []*RuleStateDesc{&rs1NotRun, &rs2NotRun}, + } + gs2 := GroupStateDesc{ + Group: &g2, + ActiveRules: []*RuleStateDesc{&rs1, &rs2}, + EvaluationTimestamp: curTime, + } + gs2NotRun := GroupStateDesc{ + Group: &g2, + ActiveRules: []*RuleStateDesc{&rs1NotRun, &rs2NotRun}, + } + gs3 := GroupStateDesc{ + Group: &g2, + ActiveRules: []*RuleStateDesc{&rs1, &rs3}, + EvaluationTimestamp: curTime, + } + + type testCase struct { + input []*GroupStateDesc + expectedOutput []*GroupStateDesc + } + + testCases := map[string]testCase{ + "No duplicate": { + input: []*GroupStateDesc{&gs1, &gs2}, + expectedOutput: []*GroupStateDesc{&gs1, &gs2}, + }, + "No duplicate but not evaluated": { + input: []*GroupStateDesc{&gs1NotRun, &gs2NotRun}, + expectedOutput: []*GroupStateDesc{&gs1NotRun, &gs2NotRun}, + }, + "With exact duplicate": { + input: []*GroupStateDesc{&gs1, &gs2NotRun, &gs1, &gs2NotRun}, + expectedOutput: []*GroupStateDesc{&gs1, &gs2NotRun}, + }, + "With duplicates that are not evaluated": { + input: []*GroupStateDesc{&gs1, &gs2, &gs1NotRun, &gs2NotRun}, + expectedOutput: []*GroupStateDesc{&gs1, &gs2}, + }, + "With duplicate with a new newer rule evaluation": { + input: []*GroupStateDesc{&gs3, &gs1, &gs2, &gs1NotRun}, + expectedOutput: []*GroupStateDesc{&gs1, &gs3}, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + out := mergeGroupStateDesc(tc.input) + slices.SortFunc(out, func(a, b *GroupStateDesc) int { + fileCompare := strings.Compare(a.Group.Namespace, b.Group.Namespace) + if fileCompare != 0 { + return fileCompare + } + return strings.Compare(a.Group.Name, b.Group.Name) + }) + require.Equal(t, len(tc.expectedOutput), len(out)) + require.True(t, reflect.DeepEqual(tc.expectedOutput, out)) + }) + } + +} diff --git a/pkg/ruler/rule_backup_manager.go b/pkg/ruler/rule_backup_manager.go new file mode 100644 index 00000000000..ec09b23363e --- /dev/null +++ b/pkg/ruler/rule_backup_manager.go @@ -0,0 +1,156 @@ +package ruler + +import ( + "context" + "errors" + "sync" + + "github.com/go-kit/log" + "github.com/go-kit/log/level" + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/promauto" + "github.com/prometheus/prometheus/model/rulefmt" + "github.com/prometheus/prometheus/promql/parser" + promRules "github.com/prometheus/prometheus/rules" + + "github.com/cortexproject/cortex/pkg/ruler/rulespb" +) + +// Implements GroupLoader interface but instead of reading from a file when Load is called, it returns the +// rulefmt.RuleGroup it has stored +type loader struct { + ruleGroups map[string][]rulefmt.RuleGroup +} + +func (r *loader) Load(identifier string) (*rulefmt.RuleGroups, []error) { + return &rulefmt.RuleGroups{ + Groups: r.ruleGroups[identifier], + }, nil +} + +func (r *loader) Parse(query string) (parser.Expr, error) { + return parser.ParseExpr(query) +} + +// rulesBackupManager is an in-memory store that holds []promRules.Group of multiple users. It only stores the Groups, +// it doesn't evaluate them. +type rulesBackupManager struct { + backupRuleGroupsMtx sync.RWMutex + backupRuleGroups map[string][]*promRules.Group + cfg Config + + logger log.Logger + + backupGroupRulesCount *prometheus.GaugeVec + lastBackupReloadSuccessful *prometheus.GaugeVec +} + +func newRulesBackupManager(cfg Config, logger log.Logger, reg prometheus.Registerer) *rulesBackupManager { + return &rulesBackupManager{ + backupRuleGroups: make(map[string][]*promRules.Group), + cfg: cfg, + logger: logger, + + backupGroupRulesCount: promauto.With(reg).NewGaugeVec(prometheus.GaugeOpts{ + Namespace: "cortex", + Name: "ruler_backup_rule_group_rules", + Help: "The number of backed up rules", + }, []string{"user", "rule_group"}), + lastBackupReloadSuccessful: promauto.With(reg).NewGaugeVec(prometheus.GaugeOpts{ + Namespace: "cortex", + Name: "ruler_backup_last_reload_successful", + Help: "Boolean set to 1 whenever the last configuration reload attempt was successful.", + }, []string{"user"}), + } +} + +// backUpRuleGroups updates the map[string][]*promRules.Group that the rulesBackupManager stores in memory. +func (r *rulesBackupManager) backUpRuleGroups(_ context.Context, ruleGroups map[string]rulespb.RuleGroupList) { + backupRuleGroups := make(map[string][]*promRules.Group) + for user, groups := range ruleGroups { + promGroups, err := r.ruleGroupListToPromGroups(user, groups) + if err != nil { + r.lastBackupReloadSuccessful.WithLabelValues(user).Set(0) + level.Error(r.logger).Log("msg", "unable to back up rules", "user", user, "err", err) + continue + } + backupRuleGroups[user] = promGroups + r.lastBackupReloadSuccessful.WithLabelValues(user).Set(1) + } + r.backupRuleGroupsMtx.Lock() + defer r.backupRuleGroupsMtx.Unlock() + r.updateMetrics(backupRuleGroups) + r.backupRuleGroups = backupRuleGroups +} + +// ruleGroupListToPromGroups converts rulespb.RuleGroupList to []*promRules.Group by creating a single use +// promRules.Manager and calling its LoadGroups method. +func (r *rulesBackupManager) ruleGroupListToPromGroups(user string, ruleGroups rulespb.RuleGroupList) ([]*promRules.Group, error) { + rgs := ruleGroups.Formatted() + + loader := &loader{ + ruleGroups: rgs, + } + promManager := promRules.NewManager(&promRules.ManagerOptions{ + ExternalURL: r.cfg.ExternalURL.URL, + GroupLoader: loader, + }) + + namespaces := make([]string, 0, len(rgs)) + for k := range rgs { + namespaces = append(namespaces, k) + } + loadedGroups, errs := promManager.LoadGroups(r.cfg.EvaluationInterval, r.cfg.ExternalLabels, r.cfg.ExternalURL.String(), nil, namespaces...) + if errs != nil { + for _, e := range errs { + level.Error(r.logger).Log("msg", "loading groups to backup failed", "user", user, "namespaces", namespaces, "err", e) + } + return nil, errors.New("error loading rules to backup") + } + + groups := make([]*promRules.Group, 0, len(loadedGroups)) + for _, g := range loadedGroups { + groups = append(groups, g) + } + return groups, nil +} + +// getRuleGroups returns the []*promRules.Group that rulesBackupManager stores for a given user +func (r *rulesBackupManager) getRuleGroups(userID string) []*promRules.Group { + var result []*promRules.Group + r.backupRuleGroupsMtx.RLock() + defer r.backupRuleGroupsMtx.RUnlock() + if groups, exists := r.backupRuleGroups[userID]; exists { + result = groups + } + return result +} + +func (r *rulesBackupManager) updateMetrics(newBackupGroups map[string][]*promRules.Group) { + for user, groups := range newBackupGroups { + keptGroups := make(map[string][]interface{}) + for _, g := range groups { + key := promRules.GroupKey(g.File(), g.Name()) + r.backupGroupRulesCount.WithLabelValues(user, key).Set(float64(len(g.Rules()))) + keptGroups[key] = nil + } + oldGroups := r.backupRuleGroups[user] + for _, g := range oldGroups { + key := promRules.GroupKey(g.File(), g.Name()) + if _, exists := keptGroups[key]; !exists { + r.backupGroupRulesCount.DeleteLabelValues(user, key) + } + } + } + + for user, groups := range r.backupRuleGroups { + if _, exists := newBackupGroups[user]; exists { + continue + } + for _, g := range groups { + key := promRules.GroupKey(g.File(), g.Name()) + r.backupGroupRulesCount.DeleteLabelValues(user, key) + } + r.lastBackupReloadSuccessful.DeleteLabelValues(user) + } +} diff --git a/pkg/ruler/rule_backup_manager_test.go b/pkg/ruler/rule_backup_manager_test.go new file mode 100644 index 00000000000..6503cd9d85c --- /dev/null +++ b/pkg/ruler/rule_backup_manager_test.go @@ -0,0 +1,228 @@ +package ruler + +import ( + "context" + "strings" + "testing" + + "github.com/go-kit/log" + "github.com/prometheus/client_golang/prometheus" + io_prometheus_client "github.com/prometheus/client_model/go" + "github.com/prometheus/prometheus/model/labels" + "github.com/prometheus/prometheus/promql/parser" + promRules "github.com/prometheus/prometheus/rules" + "github.com/stretchr/testify/require" + "golang.org/x/exp/slices" + + "github.com/cortexproject/cortex/pkg/ruler/rulespb" + "github.com/cortexproject/cortex/pkg/util" +) + +func TestBackUpRuleGroups(t *testing.T) { + r := rulespb.RuleDesc{ + Expr: "1 > bool 1", + Record: "test", + } + parsedExpr, _ := parser.ParseExpr(r.Expr) + g1 := rulespb.RuleGroupDesc{ + Name: "g1", + Namespace: "ns1", + Rules: []*rulespb.RuleDesc{&r}, + } + g2 := rulespb.RuleGroupDesc{ + Name: "g2", + Namespace: "ns1", + Rules: []*rulespb.RuleDesc{&r}, + } + g3 := rulespb.RuleGroupDesc{ + Name: "g3", + Namespace: "ns2", + Rules: []*rulespb.RuleDesc{&r}, + } + + rInvalid := rulespb.RuleDesc{ + Expr: "1 > 1", // invalid expression + } + gInvalid := rulespb.RuleGroupDesc{ + Name: "g1", + Namespace: "ns1", + Rules: []*rulespb.RuleDesc{&rInvalid}, + } + cfg := defaultRulerConfig(t) + managerOptions := &promRules.ManagerOptions{} + manager := newRulesBackupManager(cfg, log.NewNopLogger(), nil) + g1Option := promRules.GroupOptions{ + Name: g1.Name, + File: g1.Namespace, + Interval: cfg.EvaluationInterval, + Rules: []promRules.Rule{ + promRules.NewRecordingRule(r.Record, parsedExpr, labels.Labels{}), + }, + } + g2Option := promRules.GroupOptions{ + Name: g2.Name, + File: g2.Namespace, + Interval: cfg.EvaluationInterval, + Rules: []promRules.Rule{ + promRules.NewRecordingRule(r.Record, parsedExpr, labels.Labels{}), + }, + } + g3Option := promRules.GroupOptions{ + Name: g3.Name, + File: g3.Namespace, + Interval: cfg.EvaluationInterval, + Rules: []promRules.Rule{ + promRules.NewRecordingRule(r.Record, parsedExpr, labels.Labels{}), + }, + } + + type testCase struct { + input map[string]rulespb.RuleGroupList + expectedOutput map[string][]*promRules.GroupOptions + } + + testCases := map[string]testCase{ + "Empty input": { + input: make(map[string]rulespb.RuleGroupList), + expectedOutput: make(map[string][]*promRules.GroupOptions), + }, + "With invalid rules": { + input: map[string]rulespb.RuleGroupList{ + "user1": {&gInvalid}, + }, + expectedOutput: make(map[string][]*promRules.GroupOptions), + }, + "With partial invalid rules": { + input: map[string]rulespb.RuleGroupList{ + "user1": {&gInvalid, &g3}, + "user2": {&g1, &g2}, + }, + expectedOutput: map[string][]*promRules.GroupOptions{ + "user2": {&g1Option, &g2Option}, + }, + }, + "With groups from multiple users": { + input: map[string]rulespb.RuleGroupList{ + "user1": {&g1, &g2, &g3}, + "user2": {&g1, &g2}, + }, + expectedOutput: map[string][]*promRules.GroupOptions{ + "user1": {&g1Option, &g2Option, &g3Option}, + "user2": {&g1Option, &g2Option}, + }, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + manager.backUpRuleGroups(context.TODO(), tc.input) + require.Equal(t, len(tc.expectedOutput), len(manager.backupRuleGroups)) + for user, expectedGroupOptions := range tc.expectedOutput { + loadedGroups := manager.getRuleGroups(user) + expectedGroups := make([]*promRules.Group, 0, len(expectedGroupOptions)) + for _, o := range expectedGroupOptions { + o.Opts = managerOptions + expectedGroups = append(expectedGroups, promRules.NewGroup(*o)) + } + requireGroupsEqual(t, expectedGroups, loadedGroups) + } + }) + } +} + +func TestBackUpRuleGroupsMetrics(t *testing.T) { + r := rulespb.RuleDesc{ + Expr: "1 > bool 1", + Record: "test", + } + g1 := rulespb.RuleGroupDesc{ + Name: "g1", + Namespace: "ns1", + Rules: []*rulespb.RuleDesc{&r}, + } + g2 := rulespb.RuleGroupDesc{ + Name: "g2", + Namespace: "ns1", + Rules: []*rulespb.RuleDesc{&r}, + } + g2Updated := rulespb.RuleGroupDesc{ + Name: "g2", + Namespace: "ns1", + Rules: []*rulespb.RuleDesc{&r, &r}, + } + + cfg := defaultRulerConfig(t) + reg := prometheus.NewPedanticRegistry() + manager := newRulesBackupManager(cfg, log.NewNopLogger(), reg) + + manager.backUpRuleGroups(context.TODO(), map[string]rulespb.RuleGroupList{ + "user1": {&g1, &g2}, + "user2": {&g1}, + }) + gm, err := reg.Gather() + require.NoError(t, err) + mfm, err := util.NewMetricFamilyMap(gm) + require.NoError(t, err) + require.Equal(t, 2, len(mfm["cortex_ruler_backup_last_reload_successful"].Metric)) + requireMetricEqual(t, mfm["cortex_ruler_backup_last_reload_successful"].Metric[0], map[string]string{ + "user": "user1", + }, float64(1)) + requireMetricEqual(t, mfm["cortex_ruler_backup_last_reload_successful"].Metric[1], map[string]string{ + "user": "user2", + }, float64(1)) + require.Equal(t, 3, len(mfm["cortex_ruler_backup_rule_group_rules"].Metric)) + requireMetricEqual(t, mfm["cortex_ruler_backup_rule_group_rules"].Metric[0], map[string]string{ + "user": "user1", + "rule_group": "ns1;g1", + }, float64(1)) + requireMetricEqual(t, mfm["cortex_ruler_backup_rule_group_rules"].Metric[1], map[string]string{ + "user": "user2", + "rule_group": "ns1;g1", + }, float64(1)) + requireMetricEqual(t, mfm["cortex_ruler_backup_rule_group_rules"].Metric[2], map[string]string{ + "user": "user1", + "rule_group": "ns1;g2", + }, float64(1)) + + manager.backUpRuleGroups(context.TODO(), map[string]rulespb.RuleGroupList{ + "user1": {&g2Updated}, + }) + gm, err = reg.Gather() + require.NoError(t, err) + mfm, err = util.NewMetricFamilyMap(gm) + require.NoError(t, err) + require.Equal(t, 1, len(mfm["cortex_ruler_backup_last_reload_successful"].Metric)) + requireMetricEqual(t, mfm["cortex_ruler_backup_last_reload_successful"].Metric[0], map[string]string{ + "user": "user1", + }, float64(1)) + require.Equal(t, 1, len(mfm["cortex_ruler_backup_rule_group_rules"].Metric)) + requireMetricEqual(t, mfm["cortex_ruler_backup_rule_group_rules"].Metric[0], map[string]string{ + "user": "user1", + "rule_group": "ns1;g2", + }, float64(2)) +} + +func requireGroupsEqual(t *testing.T, a []*promRules.Group, b []*promRules.Group) { + require.Equal(t, len(a), len(b)) + sortFunc := func(g1, g2 *promRules.Group) int { + fileCompare := strings.Compare(g1.File(), g2.File()) + if fileCompare != 0 { + return fileCompare + } + return strings.Compare(g1.Name(), g2.Name()) + } + slices.SortFunc(a, sortFunc) + slices.SortFunc(b, sortFunc) + for i, gA := range a { + gB := b[i] + require.True(t, gA.Equals(gB), "group1", gA.Name(), "group2", gB.Name()) + } +} + +func requireMetricEqual(t *testing.T, m *io_prometheus_client.Metric, labels map[string]string, value float64) { + l := m.GetLabel() + require.Equal(t, len(labels), len(l)) + for _, pair := range l { + require.Equal(t, labels[*pair.Name], *pair.Value) + } + require.Equal(t, value, *m.Gauge.Value) +} diff --git a/pkg/ruler/ruler.go b/pkg/ruler/ruler.go index ba3c698e36c..dccc4300e1c 100644 --- a/pkg/ruler/ruler.go +++ b/pkg/ruler/ruler.go @@ -127,7 +127,9 @@ type Config struct { Ring RingConfig `yaml:"ring"` FlushCheckPeriod time.Duration `yaml:"flush_period"` - EnableAPI bool `yaml:"enable_api"` + EnableAPI bool `yaml:"enable_api"` + APIEnableRulesBackup bool `yaml:"api_enable_rules_backup"` + APIDeduplicateRules bool `yaml:"api_deduplicate_rules"` EnabledTenants flagext.StringSliceCSV `yaml:"enabled_tenants"` DisabledTenants flagext.StringSliceCSV `yaml:"disabled_tenants"` @@ -195,6 +197,8 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { f.DurationVar(&cfg.FlushCheckPeriod, "ruler.flush-period", 1*time.Minute, "Period with which to attempt to flush rule groups.") f.StringVar(&cfg.RulePath, "ruler.rule-path", "/rules", "file path to store temporary rule files for the prometheus rule managers") f.BoolVar(&cfg.EnableAPI, "experimental.ruler.enable-api", false, "Enable the ruler api") + f.BoolVar(&cfg.APIEnableRulesBackup, "ruler.api-enable-rules-backup", false, "Enable rulers to store a copy of rules owned by other rulers with default state (state before any evaluation) and send this copy in list API requests as backup in case the ruler who owns the rule fails to send its rules. This allows the rules API to handle ruler outage by returning rules with default state. Ring replication-factor needs to be set to 3 or more for this to be useful.") + f.BoolVar(&cfg.APIDeduplicateRules, "ruler.api-deduplicate-rules", false, "Remove duplicate rules in the prometheus rules and alerts API response. If there are duplicate rules the rule with the latest evaluation timestamp will be kept.") f.DurationVar(&cfg.OutageTolerance, "ruler.for-outage-tolerance", time.Hour, `Max time to tolerate outage for restoring "for" state of alert.`) f.DurationVar(&cfg.ForGracePeriod, "ruler.for-grace-period", 10*time.Minute, `Minimum duration between alert and restored "for" state. This is maintained only for alerts with configured "for" time greater than grace period.`) f.DurationVar(&cfg.ResendDelay, "ruler.resend-delay", time.Minute, `Minimum amount of time to wait before resending an alert to Alertmanager.`) @@ -215,8 +219,12 @@ type MultiTenantManager interface { // SyncRuleGroups is used to sync the Manager with rules from the RuleStore. // If existing user is missing in the ruleGroups map, its ruler manager will be stopped. SyncRuleGroups(ctx context.Context, ruleGroups map[string]rulespb.RuleGroupList) + // BackUpRuleGroups is used to store backups of rule groups owned by a different ruler instance. + BackUpRuleGroups(ctx context.Context, ruleGroups map[string]rulespb.RuleGroupList) // GetRules fetches rules for a particular tenant (userID). GetRules(userID string) []*promRules.Group + // GetBackupRules fetches rules for a particular tenant (userID) that the ruler stores for backup purposes + GetBackupRules(userID string) []*promRules.Group // Stop stops all Manager components. Stop() // ValidateRuleGroup validates a rulegroup @@ -270,6 +278,7 @@ type Ruler struct { rulerSync *prometheus.CounterVec ruleGroupStoreLoadDuration prometheus.Gauge ruleGroupSyncDuration prometheus.Gauge + rulerGetRulesFailures *prometheus.CounterVec allowedTenants *util.AllowedTenants @@ -312,6 +321,11 @@ func newRuler(cfg Config, manager MultiTenantManager, reg prometheus.Registerer, Name: "cortex_ruler_rule_group_sync_duration_seconds", Help: "The duration in seconds required to sync and load rule groups from storage.", }), + + rulerGetRulesFailures: promauto.With(reg).NewCounterVec(prometheus.CounterOpts{ + Name: "cortex_ruler_get_rules_failure_total", + Help: "The total number of failed rules request sent to rulers.", + }, []string{"ruler"}), } if len(cfg.EnabledTenants) > 0 { @@ -467,6 +481,7 @@ func instanceOwnsRuleGroup(r ring.ReadRing, g *rulespb.RuleGroupDesc, disabledRu return false, errors.Wrap(err, "error reading ring to verify rule group ownership") } + // Even if the replication factor is set to a number bigger than 1, only the first ruler evaluates the rule group ownsRuleGroup := rlrs.Instances[0].Addr == instanceAddr if ownsRuleGroup && ruleGroupDisabled(g, disabledRuleGroups) { return false, &DisabledRuleGroupErr{Message: fmt.Sprintf("rule group %s, namespace %s, user %s is disabled", g.Name, g.Namespace, g.User)} @@ -475,6 +490,29 @@ func instanceOwnsRuleGroup(r ring.ReadRing, g *rulespb.RuleGroupDesc, disabledRu return ownsRuleGroup, nil } +func instanceBacksUpRuleGroup(r ring.ReadRing, g *rulespb.RuleGroupDesc, disabledRuleGroups validation.DisabledRuleGroups, instanceAddr string) (bool, error) { + hash := tokenForGroup(g) + + rlrs, err := r.Get(hash, RingOp, nil, nil, nil) + if err != nil { + return false, errors.Wrap(err, "error reading ring to verify rule group backup ownership") + } + + var backupRuleGroup bool + // Only the second up to the last replica is used a backup + for i := 1; i < len(rlrs.Instances); i++ { + if rlrs.Instances[i].Addr == instanceAddr { + backupRuleGroup = true + break + } + } + + if backupRuleGroup && ruleGroupDisabled(g, disabledRuleGroups) { + return false, &DisabledRuleGroupErr{Message: fmt.Sprintf("rule group %s, namespace %s, user %s is disabled", g.Name, g.Namespace, g.User)} + } + return backupRuleGroup, nil +} + func (r *Ruler) ServeHTTP(w http.ResponseWriter, req *http.Request) { if r.cfg.EnableSharding { r.ring.ServeHTTP(w, req) @@ -543,16 +581,20 @@ func (r *Ruler) syncRules(ctx context.Context, reason string) { r.ruleGroupSyncDuration.Set(ruleGroupSyncDuration) }() - loadedConfigs, err := r.loadRuleGroups(ctx) + loadedConfigs, backupConfigs, err := r.loadRuleGroups(ctx) if err != nil { return } // This will also delete local group files for users that are no longer in 'configs' map. r.manager.SyncRuleGroups(ctx, loadedConfigs) + + if r.cfg.APIEnableRulesBackup { + r.manager.BackUpRuleGroups(ctx, backupConfigs) + } } -func (r *Ruler) loadRuleGroups(ctx context.Context) (map[string]rulespb.RuleGroupList, error) { +func (r *Ruler) loadRuleGroups(ctx context.Context) (map[string]rulespb.RuleGroupList, map[string]rulespb.RuleGroupList, error) { timer := prometheus.NewTimer(nil) defer func() { @@ -560,51 +602,65 @@ func (r *Ruler) loadRuleGroups(ctx context.Context) (map[string]rulespb.RuleGrou r.ruleGroupStoreLoadDuration.Set(storeLoadSeconds) }() - configs, err := r.listRules(ctx) + ownedConfigs, backupConfigs, err := r.listRules(ctx) if err != nil { level.Error(r.logger).Log("msg", "unable to list rules", "err", err) - return nil, err + return nil, nil, err } - loadedConfigs, err := r.store.LoadRuleGroups(ctx, configs) + loadedOwnedConfigs, err := r.store.LoadRuleGroups(ctx, ownedConfigs) if err != nil { - level.Warn(r.logger).Log("msg", "failed to load some rules owned by this ruler", "count", len(configs)-len(loadedConfigs), "err", err) + level.Warn(r.logger).Log("msg", "failed to load some rules owned by this ruler", "count", len(ownedConfigs)-len(loadedOwnedConfigs), "err", err) } - return loadedConfigs, nil + if r.cfg.APIEnableRulesBackup { + loadedBackupConfigs, err := r.store.LoadRuleGroups(ctx, backupConfigs) + if err != nil { + level.Warn(r.logger).Log("msg", "failed to load some rules backed up by this ruler", "count", len(backupConfigs)-len(loadedBackupConfigs), "err", err) + } + return loadedOwnedConfigs, loadedBackupConfigs, nil + } + return loadedOwnedConfigs, nil, nil } -func (r *Ruler) listRules(ctx context.Context) (result map[string]rulespb.RuleGroupList, err error) { +func (r *Ruler) listRules(ctx context.Context) (owned map[string]rulespb.RuleGroupList, backedUp map[string]rulespb.RuleGroupList, err error) { switch { case !r.cfg.EnableSharding: - result, err = r.listRulesNoSharding(ctx) + owned, backedUp, err = r.listRulesNoSharding(ctx) case r.cfg.ShardingStrategy == util.ShardingStrategyDefault: - result, err = r.listRulesShardingDefault(ctx) + owned, backedUp, err = r.listRulesShardingDefault(ctx) case r.cfg.ShardingStrategy == util.ShardingStrategyShuffle: - result, err = r.listRulesShuffleSharding(ctx) + owned, backedUp, err = r.listRulesShuffleSharding(ctx) default: - return nil, errors.New("invalid sharding configuration") + return nil, nil, errors.New("invalid sharding configuration") } if err != nil { return } - for userID := range result { + for userID := range owned { if !r.allowedTenants.IsAllowed(userID) { level.Debug(r.logger).Log("msg", "ignoring rule groups for user, not allowed", "user", userID) - delete(result, userID) + delete(owned, userID) + } + } + + for userID := range backedUp { + if !r.allowedTenants.IsAllowed(userID) { + level.Debug(r.logger).Log("msg", "ignoring rule groups for user, not allowed", "user", userID) + delete(backedUp, userID) } } return } -func (r *Ruler) listRulesNoSharding(ctx context.Context) (map[string]rulespb.RuleGroupList, error) { +func (r *Ruler) listRulesNoSharding(ctx context.Context) (map[string]rulespb.RuleGroupList, map[string]rulespb.RuleGroupList, error) { allRuleGroups, err := r.store.ListAllRuleGroups(ctx) if err != nil { - return nil, err + return nil, nil, err } for userID, groups := range allRuleGroups { disabledRuleGroupsForUser := r.limits.DisabledRuleGroups(userID) @@ -621,29 +677,36 @@ func (r *Ruler) listRulesNoSharding(ctx context.Context) (map[string]rulespb.Rul } allRuleGroups[userID] = filteredGroupsForUser } - return allRuleGroups, nil + return allRuleGroups, nil, nil } -func (r *Ruler) listRulesShardingDefault(ctx context.Context) (map[string]rulespb.RuleGroupList, error) { +func (r *Ruler) listRulesShardingDefault(ctx context.Context) (map[string]rulespb.RuleGroupList, map[string]rulespb.RuleGroupList, error) { configs, err := r.store.ListAllRuleGroups(ctx) if err != nil { - return nil, err + return nil, nil, err } - filteredConfigs := make(map[string]rulespb.RuleGroupList) + ownedConfigs := make(map[string]rulespb.RuleGroupList) + backedUpConfigs := make(map[string]rulespb.RuleGroupList) for userID, groups := range configs { - filtered := filterRuleGroups(userID, groups, r.limits.DisabledRuleGroups(userID), r.ring, r.lifecycler.GetInstanceAddr(), r.logger, r.ringCheckErrors) - if len(filtered) > 0 { - filteredConfigs[userID] = filtered + owned := filterRuleGroups(userID, groups, r.limits.DisabledRuleGroups(userID), r.ring, r.lifecycler.GetInstanceAddr(), r.logger, r.ringCheckErrors) + if len(owned) > 0 { + ownedConfigs[userID] = owned + } + if r.cfg.APIEnableRulesBackup { + backup := filterBackupRuleGroups(userID, groups, r.limits.DisabledRuleGroups(userID), r.ring, r.lifecycler.GetInstanceAddr(), r.logger, r.ringCheckErrors) + if len(backup) > 0 { + backedUpConfigs[userID] = backup + } } } - return filteredConfigs, nil + return ownedConfigs, backedUpConfigs, nil } -func (r *Ruler) listRulesShuffleSharding(ctx context.Context) (map[string]rulespb.RuleGroupList, error) { +func (r *Ruler) listRulesShuffleSharding(ctx context.Context) (map[string]rulespb.RuleGroupList, map[string]rulespb.RuleGroupList, error) { users, err := r.store.ListAllUsers(ctx) if err != nil { - return nil, errors.Wrap(err, "unable to list users of ruler") + return nil, nil, errors.Wrap(err, "unable to list users of ruler") } // Only users in userRings will be used in the to load the rules. @@ -664,7 +727,7 @@ func (r *Ruler) listRulesShuffleSharding(ctx context.Context) (map[string]rulesp } if len(userRings) == 0 { - return nil, nil + return nil, nil, nil } userCh := make(chan string, len(userRings)) @@ -674,7 +737,8 @@ func (r *Ruler) listRulesShuffleSharding(ctx context.Context) (map[string]rulesp close(userCh) mu := sync.Mutex{} - result := map[string]rulespb.RuleGroupList{} + owned := map[string]rulespb.RuleGroupList{} + backedUp := map[string]rulespb.RuleGroupList{} concurrency := loadRulesConcurrency if len(userRings) < concurrency { @@ -690,13 +754,21 @@ func (r *Ruler) listRulesShuffleSharding(ctx context.Context) (map[string]rulesp return errors.Wrapf(err, "failed to fetch rule groups for user %s", userID) } - filtered := filterRuleGroups(userID, groups, r.limits.DisabledRuleGroups(userID), userRings[userID], r.lifecycler.GetInstanceAddr(), r.logger, r.ringCheckErrors) - if len(filtered) == 0 { + filterOwned := filterRuleGroups(userID, groups, r.limits.DisabledRuleGroups(userID), userRings[userID], r.lifecycler.GetInstanceAddr(), r.logger, r.ringCheckErrors) + var filterBackup []*rulespb.RuleGroupDesc + if r.cfg.APIEnableRulesBackup { + filterBackup = filterBackupRuleGroups(userID, groups, r.limits.DisabledRuleGroups(userID), userRings[userID], r.lifecycler.GetInstanceAddr(), r.logger, r.ringCheckErrors) + } + if len(filterOwned) == 0 && len(filterBackup) == 0 { continue } - mu.Lock() - result[userID] = filtered + if len(filterOwned) > 0 { + owned[userID] = filterOwned + } + if len(filterBackup) > 0 { + backedUp[userID] = filterBackup + } mu.Unlock() } return nil @@ -704,7 +776,7 @@ func (r *Ruler) listRulesShuffleSharding(ctx context.Context) (map[string]rulesp } err = g.Wait() - return result, err + return owned, backedUp, err } // filterRuleGroups returns map of rule groups that given instance "owns" based on supplied ring. @@ -740,6 +812,38 @@ func filterRuleGroups(userID string, ruleGroups []*rulespb.RuleGroupDesc, disabl return result } +// filterBackupRuleGroups returns map of rule groups that given instance backs up based on supplied ring. +// This function only uses User, Namespace, and Name fields of individual RuleGroups. +// +// Reason why this function is not a method on Ruler is to make sure we don't accidentally use r.ring, +// but only ring passed as parameter. +func filterBackupRuleGroups(userID string, ruleGroups []*rulespb.RuleGroupDesc, disabledRuleGroups validation.DisabledRuleGroups, ring ring.ReadRing, instanceAddr string, log log.Logger, ringCheckErrors prometheus.Counter) []*rulespb.RuleGroupDesc { + var result []*rulespb.RuleGroupDesc + for _, g := range ruleGroups { + backup, err := instanceBacksUpRuleGroup(ring, g, disabledRuleGroups, instanceAddr) + if err != nil { + switch e := err.(type) { + case *DisabledRuleGroupErr: + level.Info(log).Log("msg", e.Message) + continue + default: + ringCheckErrors.Inc() + level.Error(log).Log("msg", "failed to check if the ruler replica backs up the rule group", "user", userID, "namespace", g.Namespace, "group", g.Name, "err", err) + continue + } + } + + if backup { + level.Debug(log).Log("msg", "rule group backed up", "user", g.User, "namespace", g.Namespace, "name", g.Name) + result = append(result, g) + } else { + level.Debug(log).Log("msg", "rule group not backed up, ignoring", "user", g.User, "namespace", g.Namespace, "name", g.Name) + } + } + + return result +} + // GetRules retrieves the running rules from this ruler and all running rulers in the ring if // sharding is enabled func (r *Ruler) GetRules(ctx context.Context, rulesRequest RulesRequest) ([]*GroupStateDesc, error) { @@ -752,12 +856,17 @@ func (r *Ruler) GetRules(ctx context.Context, rulesRequest RulesRequest) ([]*Gro return r.getShardedRules(ctx, userID, rulesRequest) } - return r.getLocalRules(userID, rulesRequest) + return r.getLocalRules(userID, rulesRequest, false) } -func (r *Ruler) getLocalRules(userID string, rulesRequest RulesRequest) ([]*GroupStateDesc, error) { +func (r *Ruler) getLocalRules(userID string, rulesRequest RulesRequest, includeBackups bool) ([]*GroupStateDesc, error) { groups := r.manager.GetRules(userID) + if includeBackups { + backupGroups := r.manager.GetBackupRules(userID) + groups = append(groups, backupGroups...) + } + groupDescs := make([]*GroupStateDesc, 0, len(groups)) prefix := filepath.Join(r.cfg.RulePath, userID) + "/" @@ -902,12 +1011,19 @@ func (r *Ruler) getShardedRules(ctx context.Context, userID string, rulesRequest } var ( - mergedMx sync.Mutex - merged []*GroupStateDesc + mtx sync.Mutex + merged []*GroupStateDesc + errs []error ) + failedZones := make(map[string]interface{}) - // Concurrently fetch rules from all rulers. Since rules are not replicated, - // we need all requests to succeed. + zoneByAddress := make(map[string]string) + if r.cfg.APIEnableRulesBackup { + for _, ruler := range rulers.Instances { + zoneByAddress[ruler.Addr] = ruler.Zone + } + } + // Concurrently fetch rules from all rulers. jobs := concurrency.CreateJobsFromStrings(rulers.GetAddresses()) err = concurrency.ForEach(ctx, jobs, len(jobs), func(ctx context.Context, job interface{}) error { addr := job.(string) @@ -923,28 +1039,48 @@ func (r *Ruler) getShardedRules(ctx context.Context, userID string, rulesRequest Files: rulesRequest.GetFiles(), Type: rulesRequest.GetType(), }) + if err != nil { + level.Error(r.logger).Log("msg", "unable to retrieve rules from ruler", "addr", addr, "err", err) + r.rulerGetRulesFailures.WithLabelValues(addr).Inc() + // If APIEnableRulesBackup is enabled and there are enough rulers replicating the rules, we should + // be able to handle failures. + if r.cfg.APIEnableRulesBackup && len(jobs) >= r.cfg.Ring.ReplicationFactor { + mtx.Lock() + failedZones[zoneByAddress[addr]] = nil + errs = append(errs, err) + failed := (rulers.MaxUnavailableZones > 0 && len(failedZones) > rulers.MaxUnavailableZones) || (rulers.MaxUnavailableZones <= 0 && len(errs) > rulers.MaxErrors) + mtx.Unlock() + if !failed { + return nil + } + } return errors.Wrapf(err, "unable to retrieve rules from ruler %s", addr) } - mergedMx.Lock() + mtx.Lock() merged = append(merged, newGrps.Groups...) - mergedMx.Unlock() + mtx.Unlock() return nil }) + if err == nil && (r.cfg.APIEnableRulesBackup || r.cfg.APIDeduplicateRules) { + merged = mergeGroupStateDesc(merged) + } + return merged, err } // Rules implements the rules service func (r *Ruler) Rules(ctx context.Context, in *RulesRequest) (*RulesResponse, error) { userID, err := tenant.TenantID(ctx) + if err != nil { return nil, fmt.Errorf("no user id found in context") } - groupDescs, err := r.getLocalRules(userID, *in) + groupDescs, err := r.getLocalRules(userID, *in, r.cfg.APIEnableRulesBackup) if err != nil { return nil, err } diff --git a/pkg/ruler/ruler_test.go b/pkg/ruler/ruler_test.go index 7978ad6e28d..9282284352f 100644 --- a/pkg/ruler/ruler_test.go +++ b/pkg/ruler/ruler_test.go @@ -340,13 +340,17 @@ func TestGetRules(t *testing.T) { type rulesMap map[string][]*rulespb.RuleDesc type testCase struct { - sharding bool - shardingStrategy string - shuffleShardSize int - rulesRequest RulesRequest - expectedCount map[string]int - rulerStateMap map[string]ring.InstanceState - expectedError error + sharding bool + shardingStrategy string + shuffleShardSize int + rulesRequest RulesRequest + expectedCount map[string]int + expectedClientCallCount int + rulerStateMap map[string]ring.InstanceState + rulerAZMap map[string]string + expectedError error + enableAPIRulesBackup bool + enableZoneAwareReplication bool } ruleMap := rulesMap{ @@ -458,6 +462,18 @@ func TestGetRules(t *testing.T) { "ruler3": ring.ACTIVE, } + rulerStateMapTwoPending := map[string]ring.InstanceState{ + "ruler1": ring.PENDING, + "ruler2": ring.PENDING, + "ruler3": ring.ACTIVE, + } + + rulerAZEvenSpread := map[string]string{ + "ruler1": "a", + "ruler2": "b", + "ruler3": "c", + } + expectedRules := expectedRulesMap{ "ruler1": map[string]rulespb.RuleGroupList{ "user1": { @@ -503,6 +519,7 @@ func TestGetRules(t *testing.T) { "user2": 4, "user3": 2, }, + expectedClientCallCount: len(expectedRules), }, "Default Sharding with No Filter": { sharding: true, @@ -513,6 +530,19 @@ func TestGetRules(t *testing.T) { "user2": 9, "user3": 3, }, + expectedClientCallCount: len(expectedRules), + }, + "Default Sharding with No Filter but with API Rules backup enabled": { + sharding: true, + shardingStrategy: util.ShardingStrategyDefault, + rulerStateMap: rulerStateMapAllActive, + expectedCount: map[string]int{ + "user1": 5, + "user2": 9, + "user3": 3, + }, + enableAPIRulesBackup: true, + expectedClientCallCount: len(expectedRules), }, "Shuffle Sharding and ShardSize = 2 with Rule Type Filter": { sharding: true, @@ -527,6 +557,7 @@ func TestGetRules(t *testing.T) { "user2": 5, "user3": 1, }, + expectedClientCallCount: 2, }, "Shuffle Sharding and ShardSize = 2 and Rule Group Name Filter": { sharding: true, @@ -541,6 +572,7 @@ func TestGetRules(t *testing.T) { "user2": 1, "user3": 2, }, + expectedClientCallCount: 2, }, "Shuffle Sharding and ShardSize = 2 and Rule Group Name and Rule Type Filter": { sharding: true, @@ -556,6 +588,7 @@ func TestGetRules(t *testing.T) { "user2": 2, "user3": 1, }, + expectedClientCallCount: 2, }, "Shuffle Sharding and ShardSize = 2 with Rule Type and Namespace Filters": { sharding: true, @@ -571,6 +604,7 @@ func TestGetRules(t *testing.T) { "user2": 0, "user3": 1, }, + expectedClientCallCount: 2, }, "Shuffle Sharding and ShardSize = 2 with Rule Type Filter and one ruler is in LEAVING state": { sharding: true, @@ -585,6 +619,7 @@ func TestGetRules(t *testing.T) { "user2": 5, "user3": 1, }, + expectedClientCallCount: 2, }, "Shuffle Sharding and ShardSize = 2 with Rule Type Filter and one ruler is in Pending state": { sharding: true, @@ -594,6 +629,99 @@ func TestGetRules(t *testing.T) { rulesRequest: RulesRequest{ Type: recordingRuleFilter, }, + expectedError: ring.ErrTooManyUnhealthyInstances, + expectedClientCallCount: 0, + }, + "Shuffle Sharding and ShardSize = 3 with API Rules backup enabled": { + sharding: true, + shuffleShardSize: 3, + shardingStrategy: util.ShardingStrategyShuffle, + rulerStateMap: rulerStateMapAllActive, + enableAPIRulesBackup: true, + rulesRequest: RulesRequest{ + Type: recordingRuleFilter, + }, + expectedCount: map[string]int{ + "user1": 3, + "user2": 5, + "user3": 1, + }, + expectedClientCallCount: 3, + }, + "Shuffle Sharding and ShardSize = 3 with API Rules backup enabled and one ruler is in Pending state": { + sharding: true, + shuffleShardSize: 3, + shardingStrategy: util.ShardingStrategyShuffle, + rulerStateMap: rulerStateMapOnePending, + enableAPIRulesBackup: true, + rulesRequest: RulesRequest{ + Type: recordingRuleFilter, + }, + expectedCount: map[string]int{ + "user1": 3, + "user2": 5, + "user3": 1, + }, + expectedClientCallCount: 2, // one of the ruler is pending, so we don't expect that ruler to be called + }, + "Shuffle Sharding and ShardSize = 3 with API Rules backup enabled and two ruler is in Pending state": { + sharding: true, + shuffleShardSize: 3, + shardingStrategy: util.ShardingStrategyShuffle, + rulerStateMap: rulerStateMapTwoPending, + enableAPIRulesBackup: true, + rulesRequest: RulesRequest{ + Type: recordingRuleFilter, + }, + expectedError: ring.ErrTooManyUnhealthyInstances, + }, + "Shuffle Sharding and ShardSize = 3 and AZ replication with API Rules backup enabled": { + sharding: true, + shuffleShardSize: 3, + shardingStrategy: util.ShardingStrategyShuffle, + enableZoneAwareReplication: true, + rulerStateMap: rulerStateMapAllActive, + rulerAZMap: rulerAZEvenSpread, + enableAPIRulesBackup: true, + rulesRequest: RulesRequest{ + Type: recordingRuleFilter, + }, + expectedCount: map[string]int{ + "user1": 3, + "user2": 5, + "user3": 1, + }, + expectedClientCallCount: 3, + }, + "Shuffle Sharding and ShardSize = 3 and AZ replication with API Rules backup enabled and one ruler in pending state": { + sharding: true, + shuffleShardSize: 3, + shardingStrategy: util.ShardingStrategyShuffle, + enableZoneAwareReplication: true, + rulerStateMap: rulerStateMapOnePending, + rulerAZMap: rulerAZEvenSpread, + enableAPIRulesBackup: true, + rulesRequest: RulesRequest{ + Type: recordingRuleFilter, + }, + expectedCount: map[string]int{ + "user1": 3, + "user2": 5, + "user3": 1, + }, + expectedClientCallCount: 2, // one of the ruler is pending, so we don't expect that ruler to be called + }, + "Shuffle Sharding and ShardSize = 3 and AZ replication with API Rules backup enabled and two ruler in pending state": { + sharding: true, + shuffleShardSize: 3, + shardingStrategy: util.ShardingStrategyShuffle, + enableZoneAwareReplication: true, + rulerStateMap: rulerStateMapTwoPending, + rulerAZMap: rulerAZEvenSpread, + enableAPIRulesBackup: true, + rulesRequest: RulesRequest{ + Type: recordingRuleFilter, + }, expectedError: ring.ErrTooManyUnhealthyInstances, }, } @@ -613,6 +741,7 @@ func TestGetRules(t *testing.T) { cfg.ShardingStrategy = tc.shardingStrategy cfg.EnableSharding = tc.sharding + cfg.APIEnableRulesBackup = tc.enableAPIRulesBackup cfg.Ring = RingConfig{ InstanceID: id, @@ -622,6 +751,13 @@ func TestGetRules(t *testing.T) { }, ReplicationFactor: 1, } + if tc.enableAPIRulesBackup { + cfg.Ring.ReplicationFactor = 3 + cfg.Ring.ZoneAwarenessEnabled = tc.enableZoneAwareReplication + } + if tc.enableZoneAwareReplication { + cfg.Ring.InstanceZone = tc.rulerAZMap[id] + } r, _ := buildRuler(t, cfg, nil, store, rulerAddrMap) r.limits = ruleLimits{evalDelay: 0, tenantShard: tc.shuffleShardSize} @@ -649,7 +785,7 @@ func TestGetRules(t *testing.T) { d = ring.NewDesc() } for rID, tokens := range allTokensByRuler { - d.AddIngester(rID, rulerAddrMap[rID].lifecycler.GetInstanceAddr(), "", tokens, tc.rulerStateMap[rID], time.Now()) + d.AddIngester(rID, rulerAddrMap[rID].lifecycler.GetInstanceAddr(), rulerAddrMap[rID].lifecycler.GetInstanceZone(), tokens, ring.ACTIVE, time.Now()) } return d, true, nil }) @@ -668,6 +804,24 @@ func TestGetRules(t *testing.T) { forEachRuler(func(_ string, r *Ruler) { r.syncRules(context.Background(), rulerSyncReasonInitial) }) + + if tc.sharding { + // update the State of the rulers in the ring based on tc.rulerStateMap + err := kvStore.CAS(context.Background(), ringKey, func(in interface{}) (out interface{}, retry bool, err error) { + d, _ := in.(*ring.Desc) + if d == nil { + d = ring.NewDesc() + } + for rID, tokens := range allTokensByRuler { + d.AddIngester(rID, rulerAddrMap[rID].lifecycler.GetInstanceAddr(), rulerAddrMap[rID].lifecycler.GetInstanceZone(), tokens, tc.rulerStateMap[rID], time.Now()) + } + return d, true, nil + }) + require.NoError(t, err) + // Wait a bit to make sure ruler's ring is updated. + time.Sleep(100 * time.Millisecond) + } + for u := range allRulesByUser { ctx := user.InjectOrgID(context.Background(), u) forEachRuler(func(_ string, r *Ruler) { @@ -687,9 +841,9 @@ func TestGetRules(t *testing.T) { mockPoolClient := r.clientsPool.(*mockRulerClientsPool) if tc.shardingStrategy == util.ShardingStrategyShuffle { - require.Equal(t, int32(tc.shuffleShardSize), mockPoolClient.numberOfCalls.Load()) + require.Equal(t, int32(tc.expectedClientCallCount), mockPoolClient.numberOfCalls.Load()) } else { - require.Equal(t, int32(len(rulerAddrMap)), mockPoolClient.numberOfCalls.Load()) + require.Equal(t, int32(tc.expectedClientCallCount), mockPoolClient.numberOfCalls.Load()) } mockPoolClient.numberOfCalls.Store(0) } @@ -698,13 +852,21 @@ func TestGetRules(t *testing.T) { totalLoadedRules := 0 totalConfiguredRules := 0 + ruleBackupCount := make(map[string]int) forEachRuler(func(rID string, r *Ruler) { - localRules, err := r.listRules(context.Background()) + localRules, localBackupRules, err := r.listRules(context.Background()) require.NoError(t, err) for _, rules := range localRules { totalLoadedRules += len(rules) } + for user, rules := range localBackupRules { + for _, rule := range rules { + key := user + rule.Namespace + rule.Name + c := ruleBackupCount[key] + ruleBackupCount[key] = c + 1 + } + } totalConfiguredRules += len(allRulesByRuler[rID]) }) @@ -715,6 +877,28 @@ func TestGetRules(t *testing.T) { numberOfRulers := len(rulerAddrMap) require.Equal(t, totalConfiguredRules*numberOfRulers, totalLoadedRules) } + if tc.enableAPIRulesBackup && tc.sharding && tc.expectedError == nil { + // all rules should be backed up + require.Equal(t, totalConfiguredRules, len(ruleBackupCount)) + var hasUnhealthyRuler bool + for _, state := range tc.rulerStateMap { + if state != ring.ACTIVE && state != ring.LEAVING { + hasUnhealthyRuler = true + break + } + } + for _, v := range ruleBackupCount { + if !hasUnhealthyRuler { + // with replication factor set to 3, each rule is backed up by 2 rulers + require.Equal(t, 2, v) + } else { + require.GreaterOrEqual(t, v, 1) + } + } + } else { + // If APIEnableRulesBackup is disabled, rulers should not back up any rules + require.Equal(t, 0, len(ruleBackupCount)) + } }) } } @@ -748,14 +932,16 @@ func TestSharding(t *testing.T) { type expectedRulesMap map[string]map[string]rulespb.RuleGroupList type testCase struct { - sharding bool - shardingStrategy string - shuffleShardSize int - setupRing func(*ring.Desc) - enabledUsers []string - disabledUsers []string - - expectedRules expectedRulesMap + sharding bool + shardingStrategy string + enableAPIRulesBackup bool + replicationFactor int + shuffleShardSize int + setupRing func(*ring.Desc) + enabledUsers []string + disabledUsers []string + expectedRules expectedRulesMap + expectedBackupRules expectedRulesMap } const ( @@ -777,21 +963,24 @@ func TestSharding(t *testing.T) { testCases := map[string]testCase{ "no sharding": { - sharding: false, - expectedRules: expectedRulesMap{ruler1: allRules}, + sharding: false, + replicationFactor: 1, + expectedRules: expectedRulesMap{ruler1: allRules}, }, "no sharding, single user allowed": { - sharding: false, - enabledUsers: []string{user1}, + sharding: false, + replicationFactor: 1, + enabledUsers: []string{user1}, expectedRules: expectedRulesMap{ruler1: map[string]rulespb.RuleGroupList{ user1: {user1Group1, user1Group2}, }}, }, "no sharding, single user disabled": { - sharding: false, - disabledUsers: []string{user1}, + sharding: false, + replicationFactor: 1, + disabledUsers: []string{user1}, expectedRules: expectedRulesMap{ruler1: map[string]rulespb.RuleGroupList{ user2: {user2Group1}, user3: {user3Group1}, @@ -799,8 +988,9 @@ func TestSharding(t *testing.T) { }, "default sharding, single ruler": { - sharding: true, - shardingStrategy: util.ShardingStrategyDefault, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyDefault, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", []uint32{0}, ring.ACTIVE, time.Now()) }, @@ -808,9 +998,10 @@ func TestSharding(t *testing.T) { }, "default sharding, single ruler, single enabled user": { - sharding: true, - shardingStrategy: util.ShardingStrategyDefault, - enabledUsers: []string{user1}, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyDefault, + enabledUsers: []string{user1}, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", []uint32{0}, ring.ACTIVE, time.Now()) }, @@ -820,9 +1011,10 @@ func TestSharding(t *testing.T) { }, "default sharding, single ruler, single disabled user": { - sharding: true, - shardingStrategy: util.ShardingStrategyDefault, - disabledUsers: []string{user1}, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyDefault, + disabledUsers: []string{user1}, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", []uint32{0}, ring.ACTIVE, time.Now()) }, @@ -833,8 +1025,9 @@ func TestSharding(t *testing.T) { }, "default sharding, multiple ACTIVE rulers": { - sharding: true, - shardingStrategy: util.ShardingStrategyDefault, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyDefault, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", sortTokens([]uint32{user1Group1Token + 1, user2Group1Token + 1}), ring.ACTIVE, time.Now()) desc.AddIngester(ruler2, ruler2Addr, "", sortTokens([]uint32{user1Group2Token + 1, user3Group1Token + 1}), ring.ACTIVE, time.Now()) @@ -854,9 +1047,10 @@ func TestSharding(t *testing.T) { }, "default sharding, multiple ACTIVE rulers, single enabled user": { - sharding: true, - shardingStrategy: util.ShardingStrategyDefault, - enabledUsers: []string{user1}, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyDefault, + enabledUsers: []string{user1}, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", sortTokens([]uint32{user1Group1Token + 1, user2Group1Token + 1}), ring.ACTIVE, time.Now()) desc.AddIngester(ruler2, ruler2Addr, "", sortTokens([]uint32{user1Group2Token + 1, user3Group1Token + 1}), ring.ACTIVE, time.Now()) @@ -874,9 +1068,10 @@ func TestSharding(t *testing.T) { }, "default sharding, multiple ACTIVE rulers, single disabled user": { - sharding: true, - shardingStrategy: util.ShardingStrategyDefault, - disabledUsers: []string{user1}, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyDefault, + disabledUsers: []string{user1}, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", sortTokens([]uint32{user1Group1Token + 1, user2Group1Token + 1}), ring.ACTIVE, time.Now()) desc.AddIngester(ruler2, ruler2Addr, "", sortTokens([]uint32{user1Group2Token + 1, user3Group1Token + 1}), ring.ACTIVE, time.Now()) @@ -894,8 +1089,9 @@ func TestSharding(t *testing.T) { }, "default sharding, unhealthy ACTIVE ruler": { - sharding: true, - shardingStrategy: util.ShardingStrategyDefault, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyDefault, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", sortTokens([]uint32{user1Group1Token + 1, user2Group1Token + 1}), ring.ACTIVE, time.Now()) @@ -918,8 +1114,9 @@ func TestSharding(t *testing.T) { }, "default sharding, LEAVING ruler": { - sharding: true, - shardingStrategy: util.ShardingStrategyDefault, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyDefault, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", sortTokens([]uint32{user1Group1Token + 1, user2Group1Token + 1}), ring.LEAVING, time.Now()) @@ -934,8 +1131,9 @@ func TestSharding(t *testing.T) { }, "default sharding, JOINING ruler": { - sharding: true, - shardingStrategy: util.ShardingStrategyDefault, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyDefault, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", sortTokens([]uint32{user1Group1Token + 1, user2Group1Token + 1}), ring.JOINING, time.Now()) @@ -950,8 +1148,9 @@ func TestSharding(t *testing.T) { }, "shuffle sharding, single ruler": { - sharding: true, - shardingStrategy: util.ShardingStrategyShuffle, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyShuffle, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", sortTokens([]uint32{0}), ring.ACTIVE, time.Now()) @@ -963,9 +1162,10 @@ func TestSharding(t *testing.T) { }, "shuffle sharding, multiple rulers, shard size 1": { - sharding: true, - shardingStrategy: util.ShardingStrategyShuffle, - shuffleShardSize: 1, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyShuffle, + shuffleShardSize: 1, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", sortTokens([]uint32{userToken(user1, 0) + 1, userToken(user2, 0) + 1, userToken(user3, 0) + 1}), ring.ACTIVE, time.Now()) @@ -980,9 +1180,10 @@ func TestSharding(t *testing.T) { // Same test as previous one, but with shard size=2. Second ruler gets all the rules. "shuffle sharding, two rulers, shard size 2": { - sharding: true, - shardingStrategy: util.ShardingStrategyShuffle, - shuffleShardSize: 2, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyShuffle, + shuffleShardSize: 2, setupRing: func(desc *ring.Desc) { // Exact same tokens setup as previous test. @@ -997,9 +1198,10 @@ func TestSharding(t *testing.T) { }, "shuffle sharding, two rulers, shard size 1, distributed users": { - sharding: true, - shardingStrategy: util.ShardingStrategyShuffle, - shuffleShardSize: 1, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyShuffle, + shuffleShardSize: 1, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", sortTokens([]uint32{userToken(user1, 0) + 1}), ring.ACTIVE, time.Now()) @@ -1017,9 +1219,10 @@ func TestSharding(t *testing.T) { }, }, "shuffle sharding, three rulers, shard size 2": { - sharding: true, - shardingStrategy: util.ShardingStrategyShuffle, - shuffleShardSize: 2, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyShuffle, + shuffleShardSize: 2, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", sortTokens([]uint32{userToken(user1, 0) + 1, user1Group1Token + 1}), ring.ACTIVE, time.Now()) @@ -1041,9 +1244,10 @@ func TestSharding(t *testing.T) { }, }, "shuffle sharding, three rulers, shard size 2, ruler2 has no users": { - sharding: true, - shardingStrategy: util.ShardingStrategyShuffle, - shuffleShardSize: 2, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyShuffle, + shuffleShardSize: 2, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", sortTokens([]uint32{userToken(user1, 0) + 1, userToken(user2, 1) + 1, user1Group1Token + 1, user1Group2Token + 1}), ring.ACTIVE, time.Now()) @@ -1064,10 +1268,11 @@ func TestSharding(t *testing.T) { }, "shuffle sharding, three rulers, shard size 2, single enabled user": { - sharding: true, - shardingStrategy: util.ShardingStrategyShuffle, - shuffleShardSize: 2, - enabledUsers: []string{user1}, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyShuffle, + shuffleShardSize: 2, + enabledUsers: []string{user1}, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", sortTokens([]uint32{userToken(user1, 0) + 1, user1Group1Token + 1}), ring.ACTIVE, time.Now()) @@ -1087,10 +1292,11 @@ func TestSharding(t *testing.T) { }, "shuffle sharding, three rulers, shard size 2, single disabled user": { - sharding: true, - shardingStrategy: util.ShardingStrategyShuffle, - shuffleShardSize: 2, - disabledUsers: []string{user1}, + sharding: true, + replicationFactor: 1, + shardingStrategy: util.ShardingStrategyShuffle, + shuffleShardSize: 2, + disabledUsers: []string{user1}, setupRing: func(desc *ring.Desc) { desc.AddIngester(ruler1, ruler1Addr, "", sortTokens([]uint32{userToken(user1, 0) + 1, user1Group1Token + 1}), ring.ACTIVE, time.Now()) @@ -1107,6 +1313,40 @@ func TestSharding(t *testing.T) { }, }, }, + + "shuffle sharding, three rulers, shard size 2, enable api backup": { + sharding: true, + replicationFactor: 2, + shardingStrategy: util.ShardingStrategyShuffle, + enableAPIRulesBackup: true, + shuffleShardSize: 2, + enabledUsers: []string{user1}, + + setupRing: func(desc *ring.Desc) { + desc.AddIngester(ruler1, ruler1Addr, "", sortTokens([]uint32{userToken(user1, 0) + 1, user1Group1Token + 1}), ring.ACTIVE, time.Now()) + desc.AddIngester(ruler2, ruler2Addr, "", sortTokens([]uint32{userToken(user1, 1) + 1, user1Group2Token + 1, userToken(user2, 1) + 1, userToken(user3, 1) + 1}), ring.ACTIVE, time.Now()) + desc.AddIngester(ruler3, ruler3Addr, "", sortTokens([]uint32{userToken(user2, 0) + 1, userToken(user3, 0) + 1, user2Group1Token + 1, user3Group1Token + 1}), ring.ACTIVE, time.Now()) + }, + + expectedRules: expectedRulesMap{ + ruler1: map[string]rulespb.RuleGroupList{ + user1: {user1Group1}, + }, + ruler2: map[string]rulespb.RuleGroupList{ + user1: {user1Group2}, + }, + ruler3: map[string]rulespb.RuleGroupList{}, + }, + expectedBackupRules: expectedRulesMap{ + ruler1: map[string]rulespb.RuleGroupList{ + user1: {user1Group2}, + }, + ruler2: map[string]rulespb.RuleGroupList{ + user1: {user1Group1}, + }, + ruler3: map[string]rulespb.RuleGroupList{}, + }, + }, } for name, tc := range testCases { @@ -1117,8 +1357,9 @@ func TestSharding(t *testing.T) { setupRuler := func(id string, host string, port int, forceRing *ring.Ring) *Ruler { store := newMockRuleStore(allRules, nil) cfg := Config{ - EnableSharding: tc.sharding, - ShardingStrategy: tc.shardingStrategy, + EnableSharding: tc.sharding, + APIEnableRulesBackup: tc.enableAPIRulesBackup, + ShardingStrategy: tc.shardingStrategy, Ring: RingConfig{ InstanceID: id, InstanceAddr: host, @@ -1127,7 +1368,7 @@ func TestSharding(t *testing.T) { Mock: kvStore, }, HeartbeatTimeout: 1 * time.Minute, - ReplicationFactor: 1, + ReplicationFactor: tc.replicationFactor, }, FlushCheckPeriod: 0, EnabledTenants: tc.enabledUsers, @@ -1177,23 +1418,28 @@ func TestSharding(t *testing.T) { } // Always add ruler1 to expected rulers, even if there is no ring (no sharding). - loadedRules1, err := r1.listRules(context.Background()) + loadedRules1, backupRules1, err := r1.listRules(context.Background()) require.NoError(t, err) expected := expectedRulesMap{ ruler1: loadedRules1, } + expectedBackup := expectedRulesMap{ + ruler1: backupRules1, + } + addToExpected := func(id string, r *Ruler) { // Only expect rules from other rulers when using ring, and they are present in the ring. if r != nil && rulerRing != nil && rulerRing.HasInstance(id) { - loaded, err := r.listRules(context.Background()) + loaded, backup, err := r.listRules(context.Background()) require.NoError(t, err) // Normalize nil map to empty one. if loaded == nil { loaded = map[string]rulespb.RuleGroupList{} } expected[id] = loaded + expectedBackup[id] = backup } } @@ -1201,6 +1447,14 @@ func TestSharding(t *testing.T) { addToExpected(ruler3, r3) require.Equal(t, tc.expectedRules, expected) + + if !tc.enableAPIRulesBackup { + require.Equal(t, 0, len(expectedBackup[ruler1])) + require.Equal(t, 0, len(expectedBackup[ruler2])) + require.Equal(t, 0, len(expectedBackup[ruler3])) + } else { + require.Equal(t, tc.expectedBackupRules, expectedBackup) + } }) } } @@ -1292,7 +1546,7 @@ func Test_LoadPartialGroups(t *testing.T) { len(r1.manager.GetRules(user3)) > 0 }) - returned, err := r1.listRules(context.Background()) + returned, _, err := r1.listRules(context.Background()) require.NoError(t, err) require.Equal(t, returned, allRules) require.Equal(t, 2, len(manager.userManagers)) @@ -1837,7 +2091,7 @@ func TestRulerDisablesRuleGroups(t *testing.T) { } actualRules := map[string]rulespb.RuleGroupList{} - loadedRules, err := r1.listRules(context.Background()) + loadedRules, _, err := r1.listRules(context.Background()) require.NoError(t, err) for k, v := range loadedRules { if len(v) > 0 { @@ -1848,7 +2102,7 @@ func TestRulerDisablesRuleGroups(t *testing.T) { fetchRules := func(id string, r *Ruler) { // Only expect rules from other rulers when using ring, and they are present in the ring. if r != nil && rulerRing != nil && rulerRing.HasInstance(id) { - loaded, err := r.listRules(context.Background()) + loaded, _, err := r.listRules(context.Background()) require.NoError(t, err) // Normalize nil map to empty one. From 0e2c26095d96cb0592790663bf7c7bdd4914a75d Mon Sep 17 00:00:00 2001 From: Emmanuel Lodovice Date: Sun, 10 Mar 2024 23:58:59 -0700 Subject: [PATCH 03/12] Add integration test for rulers API with backup enabled Signed-off-by: Emmanuel Lodovice --- integration/ruler_test.go | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/integration/ruler_test.go b/integration/ruler_test.go index 8cc8da26ad2..626f308fe2b 100644 --- a/integration/ruler_test.go +++ b/integration/ruler_test.go @@ -395,6 +395,14 @@ func TestRulerSharding(t *testing.T) { } func TestRulerAPISharding(t *testing.T) { + testRulerAPIWithSharding(t, false) +} + +func TestRulerAPIShardingWithAPIRulesBackupEnabled(t *testing.T) { + testRulerAPIWithSharding(t, true) +} + +func testRulerAPIWithSharding(t *testing.T, enableAPIRulesBackup bool) { const numRulesGroups = 100 random := rand.New(rand.NewSource(time.Now().UnixNano())) @@ -444,24 +452,30 @@ func TestRulerAPISharding(t *testing.T) { require.NoError(t, s.StartAndWaitReady(consul, minio)) // Configure the ruler. + overrides := map[string]string{ + // Since we're not going to run any rule, we don't need the + // store-gateway to be configured to a valid address. + "-querier.store-gateway-addresses": "localhost:12345", + // Enable the bucket index so we can skip the initial bucket scan. + "-blocks-storage.bucket-store.bucket-index.enabled": "true", + } + if enableAPIRulesBackup { + overrides["-ruler.ring.replication-factor"] = "3" + overrides["-ruler.api-enable-rules-backup"] = "true" + } rulerFlags := mergeFlags( BlocksStorageFlags(), RulerFlags(), RulerShardingFlags(consul.NetworkHTTPEndpoint()), - map[string]string{ - // Since we're not going to run any rule, we don't need the - // store-gateway to be configured to a valid address. - "-querier.store-gateway-addresses": "localhost:12345", - // Enable the bucket index so we can skip the initial bucket scan. - "-blocks-storage.bucket-store.bucket-index.enabled": "true", - }, + overrides, ) // Start rulers. ruler1 := e2ecortex.NewRuler("ruler-1", consul.NetworkHTTPEndpoint(), rulerFlags, "") ruler2 := e2ecortex.NewRuler("ruler-2", consul.NetworkHTTPEndpoint(), rulerFlags, "") - rulers := e2ecortex.NewCompositeCortexService(ruler1, ruler2) - require.NoError(t, s.StartAndWaitReady(ruler1, ruler2)) + ruler3 := e2ecortex.NewRuler("ruler-3", consul.NetworkHTTPEndpoint(), rulerFlags, "") + rulers := e2ecortex.NewCompositeCortexService(ruler1, ruler2, ruler3) + require.NoError(t, s.StartAndWaitReady(ruler1, ruler2, ruler3)) // Upload rule groups to one of the rulers. c, err := e2ecortex.NewClient("", "", "", ruler1.HTTPEndpoint(), "user-1") @@ -542,6 +556,10 @@ func TestRulerAPISharding(t *testing.T) { }, } // For each test case, fetch the rules with configured filters, and ensure the results match. + if enableAPIRulesBackup { + err := ruler2.Kill() // if api-enable-rules-backup is enabled the APIs should be able to handle a ruler going down + require.NoError(t, err) + } for name, tc := range testCases { t.Run(name, func(t *testing.T) { actualGroups, err := c.GetPrometheusRules(tc.filter) From 97644863e439e07e8e5849fe9f46ac4512f6bd9f Mon Sep 17 00:00:00 2001 From: Emmanuel Lodovice Date: Wed, 20 Mar 2024 23:12:01 -0700 Subject: [PATCH 04/12] Mark the entire feature as experimental and improve variable names Signed-off-by: Emmanuel Lodovice --- CHANGELOG.md | 2 +- docs/configuration/config-file-reference.md | 29 +++++++++++---------- integration/ruler_test.go | 2 +- pkg/ruler/rule_backup_manager.go | 20 +++++++------- pkg/ruler/rule_backup_manager_test.go | 2 +- pkg/ruler/ruler.go | 4 +-- pkg/ruler/ruler_ring.go | 4 +-- 7 files changed, 32 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e319d2d1105..b9d7c2e01e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +19,7 @@ * [FEATURE] Ruler: Add `ruler.concurrent-evals-enabled` flag to enable concurrent evaluation within a single rule group for independent rules. Maximum concurrency can be configured via `ruler.max-concurrent-evals`. #5766 * [FEATURE] Distributor Queryable: Experimental: Add config `zone_results_quorum_metadata`. When querying ingesters using metadata APIs such as label names and values, only results from quorum number of zones will be included and merged. #5779 * [FEATURE] Storage Cache Clients: Add config `set_async_circuit_breaker_config` to utilize the circuit breaker pattern for dynamically thresholding asynchronous set operations. Implemented in both memcached and redis cache clients. #5789 -* [FEATURE] Ruler: Add `ruler.api-deduplicate-rules` flag to remove duplicate rule groups from the Prometheus compatible rules API endpoint. Add `ruler.ring.replication-factor` and `ruler.ring.zone-awareness-enabled` flags to configure rule group replication, but only the first ruler in the replicaset evaluates the rule group, the rest will just hold a copy as backup. Add `ruler.api-enable-rules-backup` flag to configure rulers to send the rule group backups stored in the replicaset to handle events when a ruler is down during an API request to list rules. #5782 +* [FEATURE] Ruler: Add experimental `experimental.ruler.api-deduplicate-rules` flag to remove duplicate rule groups from the Prometheus compatible rules API endpoint. Add experimental `ruler.ring.replication-factor` and `ruler.ring.zone-awareness-enabled` flags to configure rule group replication, but only the first ruler in the replicaset evaluates the rule group, the rest will just hold a copy as backup. Add experimental `experimental.ruler.api-enable-rules-backup` flag to configure rulers to send the rule group backups stored in the replicaset to handle events when a ruler is down during an API request to list rules. #5782 * [ENHANCEMENT] Store Gateway: Added `-store-gateway.enabled-tenants` and `-store-gateway.disabled-tenants` to explicitly enable or disable store-gateway for specific tenants. #5638 * [ENHANCEMENT] Compactor: Add new compactor metric `cortex_compactor_start_duration_seconds`. #5683 * [ENHANCEMENT] Upgraded Docker base images to `alpine:3.18`. #5684 diff --git a/docs/configuration/config-file-reference.md b/docs/configuration/config-file-reference.md index 93e327fd50c..677648471cc 100644 --- a/docs/configuration/config-file-reference.md +++ b/docs/configuration/config-file-reference.md @@ -4233,12 +4233,13 @@ ring: # CLI flag: -ruler.ring.heartbeat-timeout [heartbeat_timeout: | default = 1m] - # The replication factor to use when loading rule groups for API HA. + # EXPERIMENTAL: The replication factor to use when loading rule groups for API + # HA. # CLI flag: -ruler.ring.replication-factor [replication_factor: | default = 1] - # True to enable zone-awareness and load rule groups across different - # availability zones for API HA. + # EXPERIMENTAL: True to enable zone-awareness and load rule groups across + # different availability zones for API HA. # CLI flag: -ruler.ring.zone-awareness-enabled [zone_awareness_enabled: | default = false] @@ -4263,19 +4264,19 @@ ring: # CLI flag: -experimental.ruler.enable-api [enable_api: | default = false] -# Enable rulers to store a copy of rules owned by other rulers with default -# state (state before any evaluation) and send this copy in list API requests as -# backup in case the ruler who owns the rule fails to send its rules. This -# allows the rules API to handle ruler outage by returning rules with default -# state. Ring replication-factor needs to be set to 3 or more for this to be -# useful. -# CLI flag: -ruler.api-enable-rules-backup +# EXPERIMENTAL: Enable rulers to store a copy of rules owned by other rulers +# with default state (state before any evaluation) and send this copy in list +# API requests as backup in case the ruler who owns the rule fails to send its +# rules. This allows the rules API to handle ruler outage by returning rules +# with default state. Ring replication-factor needs to be set to 3 or more for +# this to be useful. +# CLI flag: -experimental.ruler.api-enable-rules-backup [api_enable_rules_backup: | default = false] -# Remove duplicate rules in the prometheus rules and alerts API response. If -# there are duplicate rules the rule with the latest evaluation timestamp will -# be kept. -# CLI flag: -ruler.api-deduplicate-rules +# EXPERIMENTAL: Remove duplicate rules in the prometheus rules and alerts API +# response. If there are duplicate rules the rule with the latest evaluation +# timestamp will be kept. +# CLI flag: -experimental.ruler.api-deduplicate-rules [api_deduplicate_rules: | default = false] # Comma separated list of tenants whose rules this ruler can evaluate. If diff --git a/integration/ruler_test.go b/integration/ruler_test.go index 626f308fe2b..81e2440c76e 100644 --- a/integration/ruler_test.go +++ b/integration/ruler_test.go @@ -461,7 +461,7 @@ func testRulerAPIWithSharding(t *testing.T, enableAPIRulesBackup bool) { } if enableAPIRulesBackup { overrides["-ruler.ring.replication-factor"] = "3" - overrides["-ruler.api-enable-rules-backup"] = "true" + overrides["-experimental.ruler.api-enable-rules-backup"] = "true" } rulerFlags := mergeFlags( BlocksStorageFlags(), diff --git a/pkg/ruler/rule_backup_manager.go b/pkg/ruler/rule_backup_manager.go index ec09b23363e..1cd325bb7b9 100644 --- a/pkg/ruler/rule_backup_manager.go +++ b/pkg/ruler/rule_backup_manager.go @@ -35,9 +35,9 @@ func (r *loader) Parse(query string) (parser.Expr, error) { // rulesBackupManager is an in-memory store that holds []promRules.Group of multiple users. It only stores the Groups, // it doesn't evaluate them. type rulesBackupManager struct { - backupRuleGroupsMtx sync.RWMutex - backupRuleGroups map[string][]*promRules.Group - cfg Config + backupRuleGroupsMtx sync.RWMutex + inMemoryRuleGroupsBackup map[string][]*promRules.Group + cfg Config logger log.Logger @@ -47,9 +47,9 @@ type rulesBackupManager struct { func newRulesBackupManager(cfg Config, logger log.Logger, reg prometheus.Registerer) *rulesBackupManager { return &rulesBackupManager{ - backupRuleGroups: make(map[string][]*promRules.Group), - cfg: cfg, - logger: logger, + inMemoryRuleGroupsBackup: make(map[string][]*promRules.Group), + cfg: cfg, + logger: logger, backupGroupRulesCount: promauto.With(reg).NewGaugeVec(prometheus.GaugeOpts{ Namespace: "cortex", @@ -80,7 +80,7 @@ func (r *rulesBackupManager) backUpRuleGroups(_ context.Context, ruleGroups map[ r.backupRuleGroupsMtx.Lock() defer r.backupRuleGroupsMtx.Unlock() r.updateMetrics(backupRuleGroups) - r.backupRuleGroups = backupRuleGroups + r.inMemoryRuleGroupsBackup = backupRuleGroups } // ruleGroupListToPromGroups converts rulespb.RuleGroupList to []*promRules.Group by creating a single use @@ -120,7 +120,7 @@ func (r *rulesBackupManager) getRuleGroups(userID string) []*promRules.Group { var result []*promRules.Group r.backupRuleGroupsMtx.RLock() defer r.backupRuleGroupsMtx.RUnlock() - if groups, exists := r.backupRuleGroups[userID]; exists { + if groups, exists := r.inMemoryRuleGroupsBackup[userID]; exists { result = groups } return result @@ -134,7 +134,7 @@ func (r *rulesBackupManager) updateMetrics(newBackupGroups map[string][]*promRul r.backupGroupRulesCount.WithLabelValues(user, key).Set(float64(len(g.Rules()))) keptGroups[key] = nil } - oldGroups := r.backupRuleGroups[user] + oldGroups := r.inMemoryRuleGroupsBackup[user] for _, g := range oldGroups { key := promRules.GroupKey(g.File(), g.Name()) if _, exists := keptGroups[key]; !exists { @@ -143,7 +143,7 @@ func (r *rulesBackupManager) updateMetrics(newBackupGroups map[string][]*promRul } } - for user, groups := range r.backupRuleGroups { + for user, groups := range r.inMemoryRuleGroupsBackup { if _, exists := newBackupGroups[user]; exists { continue } diff --git a/pkg/ruler/rule_backup_manager_test.go b/pkg/ruler/rule_backup_manager_test.go index 6503cd9d85c..6410168848b 100644 --- a/pkg/ruler/rule_backup_manager_test.go +++ b/pkg/ruler/rule_backup_manager_test.go @@ -115,7 +115,7 @@ func TestBackUpRuleGroups(t *testing.T) { for name, tc := range testCases { t.Run(name, func(t *testing.T) { manager.backUpRuleGroups(context.TODO(), tc.input) - require.Equal(t, len(tc.expectedOutput), len(manager.backupRuleGroups)) + require.Equal(t, len(tc.expectedOutput), len(manager.inMemoryRuleGroupsBackup)) for user, expectedGroupOptions := range tc.expectedOutput { loadedGroups := manager.getRuleGroups(user) expectedGroups := make([]*promRules.Group, 0, len(expectedGroupOptions)) diff --git a/pkg/ruler/ruler.go b/pkg/ruler/ruler.go index dccc4300e1c..e377c6a19fe 100644 --- a/pkg/ruler/ruler.go +++ b/pkg/ruler/ruler.go @@ -197,8 +197,8 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { f.DurationVar(&cfg.FlushCheckPeriod, "ruler.flush-period", 1*time.Minute, "Period with which to attempt to flush rule groups.") f.StringVar(&cfg.RulePath, "ruler.rule-path", "/rules", "file path to store temporary rule files for the prometheus rule managers") f.BoolVar(&cfg.EnableAPI, "experimental.ruler.enable-api", false, "Enable the ruler api") - f.BoolVar(&cfg.APIEnableRulesBackup, "ruler.api-enable-rules-backup", false, "Enable rulers to store a copy of rules owned by other rulers with default state (state before any evaluation) and send this copy in list API requests as backup in case the ruler who owns the rule fails to send its rules. This allows the rules API to handle ruler outage by returning rules with default state. Ring replication-factor needs to be set to 3 or more for this to be useful.") - f.BoolVar(&cfg.APIDeduplicateRules, "ruler.api-deduplicate-rules", false, "Remove duplicate rules in the prometheus rules and alerts API response. If there are duplicate rules the rule with the latest evaluation timestamp will be kept.") + f.BoolVar(&cfg.APIEnableRulesBackup, "experimental.ruler.api-enable-rules-backup", false, "EXPERIMENTAL: Enable rulers to store a copy of rules owned by other rulers with default state (state before any evaluation) and send this copy in list API requests as backup in case the ruler who owns the rule fails to send its rules. This allows the rules API to handle ruler outage by returning rules with default state. Ring replication-factor needs to be set to 3 or more for this to be useful.") + f.BoolVar(&cfg.APIDeduplicateRules, "experimental.ruler.api-deduplicate-rules", false, "EXPERIMENTAL: Remove duplicate rules in the prometheus rules and alerts API response. If there are duplicate rules the rule with the latest evaluation timestamp will be kept.") f.DurationVar(&cfg.OutageTolerance, "ruler.for-outage-tolerance", time.Hour, `Max time to tolerate outage for restoring "for" state of alert.`) f.DurationVar(&cfg.ForGracePeriod, "ruler.for-grace-period", 10*time.Minute, `Minimum duration between alert and restored "for" state. This is maintained only for alerts with configured "for" time greater than grace period.`) f.DurationVar(&cfg.ResendDelay, "ruler.resend-delay", time.Minute, `Minimum amount of time to wait before resending an alert to Alertmanager.`) diff --git a/pkg/ruler/ruler_ring.go b/pkg/ruler/ruler_ring.go index 7e7a5279601..4dea1381a0e 100644 --- a/pkg/ruler/ruler_ring.go +++ b/pkg/ruler/ruler_ring.go @@ -73,8 +73,8 @@ func (cfg *RingConfig) RegisterFlags(f *flag.FlagSet) { f.DurationVar(&cfg.HeartbeatPeriod, "ruler.ring.heartbeat-period", 5*time.Second, "Period at which to heartbeat to the ring. 0 = disabled.") f.DurationVar(&cfg.HeartbeatTimeout, "ruler.ring.heartbeat-timeout", time.Minute, "The heartbeat timeout after which rulers are considered unhealthy within the ring. 0 = never (timeout disabled).") f.DurationVar(&cfg.FinalSleep, "ruler.ring.final-sleep", 0*time.Second, "The sleep seconds when ruler is shutting down. Need to be close to or larger than KV Store information propagation delay") - f.IntVar(&cfg.ReplicationFactor, "ruler.ring.replication-factor", 1, "The replication factor to use when loading rule groups for API HA.") - f.BoolVar(&cfg.ZoneAwarenessEnabled, "ruler.ring.zone-awareness-enabled", false, "True to enable zone-awareness and load rule groups across different availability zones for API HA.") + f.IntVar(&cfg.ReplicationFactor, "ruler.ring.replication-factor", 1, "EXPERIMENTAL: The replication factor to use when loading rule groups for API HA.") + f.BoolVar(&cfg.ZoneAwarenessEnabled, "ruler.ring.zone-awareness-enabled", false, "EXPERIMENTAL: True to enable zone-awareness and load rule groups across different availability zones for API HA.") // Instance flags cfg.InstanceInterfaceNames = []string{"eth0", "en0"} From a5725590a574b8c679d7fa8721c83d11427e8a5d Mon Sep 17 00:00:00 2001 From: Emmanuel Lodovice Date: Thu, 21 Mar 2024 22:20:00 -0700 Subject: [PATCH 05/12] Rename backUpRuleGroups to setRuleGroups to make it code less confusing Signed-off-by: Emmanuel Lodovice --- pkg/ruler/manager.go | 2 +- pkg/ruler/rule_backup_manager.go | 4 ++-- pkg/ruler/rule_backup_manager_test.go | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/ruler/manager.go b/pkg/ruler/manager.go index be3784ab628..9faeca5779f 100644 --- a/pkg/ruler/manager.go +++ b/pkg/ruler/manager.go @@ -166,7 +166,7 @@ func (r *DefaultMultiTenantManager) deleteRuleCache(user string) { } func (r *DefaultMultiTenantManager) BackUpRuleGroups(ctx context.Context, ruleGroups map[string]rulespb.RuleGroupList) { - r.rulesBackupManager.backUpRuleGroups(ctx, ruleGroups) + r.rulesBackupManager.setRuleGroups(ctx, ruleGroups) } // syncRulesToManager maps the rule files to disk, detects any changes and will create/update the diff --git a/pkg/ruler/rule_backup_manager.go b/pkg/ruler/rule_backup_manager.go index 1cd325bb7b9..b7ce5aed7d0 100644 --- a/pkg/ruler/rule_backup_manager.go +++ b/pkg/ruler/rule_backup_manager.go @@ -64,8 +64,8 @@ func newRulesBackupManager(cfg Config, logger log.Logger, reg prometheus.Registe } } -// backUpRuleGroups updates the map[string][]*promRules.Group that the rulesBackupManager stores in memory. -func (r *rulesBackupManager) backUpRuleGroups(_ context.Context, ruleGroups map[string]rulespb.RuleGroupList) { +// setRuleGroups updates the map[string][]*promRules.Group that the rulesBackupManager stores in memory. +func (r *rulesBackupManager) setRuleGroups(_ context.Context, ruleGroups map[string]rulespb.RuleGroupList) { backupRuleGroups := make(map[string][]*promRules.Group) for user, groups := range ruleGroups { promGroups, err := r.ruleGroupListToPromGroups(user, groups) diff --git a/pkg/ruler/rule_backup_manager_test.go b/pkg/ruler/rule_backup_manager_test.go index 6410168848b..aca13f161f7 100644 --- a/pkg/ruler/rule_backup_manager_test.go +++ b/pkg/ruler/rule_backup_manager_test.go @@ -114,7 +114,7 @@ func TestBackUpRuleGroups(t *testing.T) { } for name, tc := range testCases { t.Run(name, func(t *testing.T) { - manager.backUpRuleGroups(context.TODO(), tc.input) + manager.setRuleGroups(context.TODO(), tc.input) require.Equal(t, len(tc.expectedOutput), len(manager.inMemoryRuleGroupsBackup)) for user, expectedGroupOptions := range tc.expectedOutput { loadedGroups := manager.getRuleGroups(user) @@ -154,7 +154,7 @@ func TestBackUpRuleGroupsMetrics(t *testing.T) { reg := prometheus.NewPedanticRegistry() manager := newRulesBackupManager(cfg, log.NewNopLogger(), reg) - manager.backUpRuleGroups(context.TODO(), map[string]rulespb.RuleGroupList{ + manager.setRuleGroups(context.TODO(), map[string]rulespb.RuleGroupList{ "user1": {&g1, &g2}, "user2": {&g1}, }) @@ -183,7 +183,7 @@ func TestBackUpRuleGroupsMetrics(t *testing.T) { "rule_group": "ns1;g2", }, float64(1)) - manager.backUpRuleGroups(context.TODO(), map[string]rulespb.RuleGroupList{ + manager.setRuleGroups(context.TODO(), map[string]rulespb.RuleGroupList{ "user1": {&g2Updated}, }) gm, err = reg.Gather() From 2d32a1e883fc6eba03d68db94a10a26a3fc445f1 Mon Sep 17 00:00:00 2001 From: Emmanuel Lodovice Date: Mon, 25 Mar 2024 13:26:33 -0700 Subject: [PATCH 06/12] Remove backup manager lock because its not needed Signed-off-by: Emmanuel Lodovice --- pkg/ruler/rule_backup_manager.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/pkg/ruler/rule_backup_manager.go b/pkg/ruler/rule_backup_manager.go index b7ce5aed7d0..58fc3894bc4 100644 --- a/pkg/ruler/rule_backup_manager.go +++ b/pkg/ruler/rule_backup_manager.go @@ -3,7 +3,6 @@ package ruler import ( "context" "errors" - "sync" "github.com/go-kit/log" "github.com/go-kit/log/level" @@ -35,7 +34,6 @@ func (r *loader) Parse(query string) (parser.Expr, error) { // rulesBackupManager is an in-memory store that holds []promRules.Group of multiple users. It only stores the Groups, // it doesn't evaluate them. type rulesBackupManager struct { - backupRuleGroupsMtx sync.RWMutex inMemoryRuleGroupsBackup map[string][]*promRules.Group cfg Config @@ -77,8 +75,6 @@ func (r *rulesBackupManager) setRuleGroups(_ context.Context, ruleGroups map[str backupRuleGroups[user] = promGroups r.lastBackupReloadSuccessful.WithLabelValues(user).Set(1) } - r.backupRuleGroupsMtx.Lock() - defer r.backupRuleGroupsMtx.Unlock() r.updateMetrics(backupRuleGroups) r.inMemoryRuleGroupsBackup = backupRuleGroups } @@ -118,8 +114,6 @@ func (r *rulesBackupManager) ruleGroupListToPromGroups(user string, ruleGroups r // getRuleGroups returns the []*promRules.Group that rulesBackupManager stores for a given user func (r *rulesBackupManager) getRuleGroups(userID string) []*promRules.Group { var result []*promRules.Group - r.backupRuleGroupsMtx.RLock() - defer r.backupRuleGroupsMtx.RUnlock() if groups, exists := r.inMemoryRuleGroupsBackup[userID]; exists { result = groups } From f273e9c06971c5b424af0ad5284acbc3b7732b67 Mon Sep 17 00:00:00 2001 From: Emmanuel Lodovice Date: Mon, 1 Apr 2024 09:46:46 -0700 Subject: [PATCH 07/12] Improve code quality - Remove duplicate code and use better data structures - Make backup rule_group label match the prometheus rule_group label - Skip initialization when feature is not enabled Signed-off-by: Emmanuel Lodovice --- pkg/ruler/manager.go | 18 +++++--- pkg/ruler/manager_test.go | 2 +- pkg/ruler/rule_backup_manager.go | 33 ++++++++++----- pkg/ruler/rule_backup_manager_test.go | 35 +++++++++------ pkg/ruler/ruler.go | 61 +++++++++++---------------- 5 files changed, 84 insertions(+), 65 deletions(-) diff --git a/pkg/ruler/manager.go b/pkg/ruler/manager.go index 9faeca5779f..e9994dadded 100644 --- a/pkg/ruler/manager.go +++ b/pkg/ruler/manager.go @@ -82,7 +82,7 @@ func NewDefaultMultiTenantManager(cfg Config, managerFactory ManagerFactory, eva os.Exit(1) } - return &DefaultMultiTenantManager{ + m := &DefaultMultiTenantManager{ cfg: cfg, notifierCfg: ncfg, managerFactory: managerFactory, @@ -93,7 +93,6 @@ func NewDefaultMultiTenantManager(cfg Config, managerFactory ManagerFactory, eva userManagers: map[string]RulesManager{}, userManagerMetrics: userManagerMetrics, ruleCache: map[string][]*promRules.Group{}, - rulesBackupManager: newRulesBackupManager(cfg, logger, reg), managersTotal: promauto.With(reg).NewGauge(prometheus.GaugeOpts{ Namespace: "cortex", Name: "ruler_managers_total", @@ -116,7 +115,11 @@ func NewDefaultMultiTenantManager(cfg Config, managerFactory ManagerFactory, eva }, []string{"user"}), registry: reg, logger: logger, - }, nil + } + if cfg.APIEnableRulesBackup { + m.rulesBackupManager = newRulesBackupManager(cfg, logger, reg) + } + return m, nil } func (r *DefaultMultiTenantManager) SyncRuleGroups(ctx context.Context, ruleGroups map[string]rulespb.RuleGroupList) { @@ -166,7 +169,9 @@ func (r *DefaultMultiTenantManager) deleteRuleCache(user string) { } func (r *DefaultMultiTenantManager) BackUpRuleGroups(ctx context.Context, ruleGroups map[string]rulespb.RuleGroupList) { - r.rulesBackupManager.setRuleGroups(ctx, ruleGroups) + if r.rulesBackupManager != nil { + r.rulesBackupManager.setRuleGroups(ctx, ruleGroups) + } } // syncRulesToManager maps the rule files to disk, detects any changes and will create/update the @@ -342,7 +347,10 @@ func (r *DefaultMultiTenantManager) GetRules(userID string) []*promRules.Group { } func (r *DefaultMultiTenantManager) GetBackupRules(userID string) []*promRules.Group { - return r.rulesBackupManager.getRuleGroups(userID) + if r.rulesBackupManager != nil { + return r.rulesBackupManager.getRuleGroups(userID) + } + return nil } func (r *DefaultMultiTenantManager) Stop() { diff --git a/pkg/ruler/manager_test.go b/pkg/ruler/manager_test.go index 65b3b8c1152..b6969e63d59 100644 --- a/pkg/ruler/manager_test.go +++ b/pkg/ruler/manager_test.go @@ -257,7 +257,7 @@ func TestBackupRules(t *testing.T) { dir := t.TempDir() reg := prometheus.NewPedanticRegistry() evalMetrics := NewRuleEvalMetrics(Config{RulePath: dir, EnableQueryStats: true}, reg) - m, err := NewDefaultMultiTenantManager(Config{RulePath: dir}, factory, evalMetrics, reg, log.NewNopLogger()) + m, err := NewDefaultMultiTenantManager(Config{RulePath: dir, APIEnableRulesBackup: true}, factory, evalMetrics, reg, log.NewNopLogger()) require.NoError(t, err) const user1 = "testUser" diff --git a/pkg/ruler/rule_backup_manager.go b/pkg/ruler/rule_backup_manager.go index 58fc3894bc4..2e9e6e0a6ea 100644 --- a/pkg/ruler/rule_backup_manager.go +++ b/pkg/ruler/rule_backup_manager.go @@ -3,6 +3,8 @@ package ruler import ( "context" "errors" + "net/url" + "path/filepath" "github.com/go-kit/log" "github.com/go-kit/log/level" @@ -39,7 +41,7 @@ type rulesBackupManager struct { logger log.Logger - backupGroupRulesCount *prometheus.GaugeVec + backupRuleGroup *prometheus.GaugeVec lastBackupReloadSuccessful *prometheus.GaugeVec } @@ -49,10 +51,10 @@ func newRulesBackupManager(cfg Config, logger log.Logger, reg prometheus.Registe cfg: cfg, logger: logger, - backupGroupRulesCount: promauto.With(reg).NewGaugeVec(prometheus.GaugeOpts{ + backupRuleGroup: promauto.With(reg).NewGaugeVec(prometheus.GaugeOpts{ Namespace: "cortex", - Name: "ruler_backup_rule_group_rules", - Help: "The number of backed up rules", + Name: "ruler_backup_rule_group", + Help: "Boolean set to 1 indicating the ruler stores the rule group as backup.", }, []string{"user", "rule_group"}), lastBackupReloadSuccessful: promauto.With(reg).NewGaugeVec(prometheus.GaugeOpts{ Namespace: "cortex", @@ -124,15 +126,17 @@ func (r *rulesBackupManager) updateMetrics(newBackupGroups map[string][]*promRul for user, groups := range newBackupGroups { keptGroups := make(map[string][]interface{}) for _, g := range groups { - key := promRules.GroupKey(g.File(), g.Name()) - r.backupGroupRulesCount.WithLabelValues(user, key).Set(float64(len(g.Rules()))) + fullFileName := r.getFilePathForGroup(g, user) + key := promRules.GroupKey(fullFileName, g.Name()) + r.backupRuleGroup.WithLabelValues(user, key).Set(1) keptGroups[key] = nil } oldGroups := r.inMemoryRuleGroupsBackup[user] for _, g := range oldGroups { - key := promRules.GroupKey(g.File(), g.Name()) + fullFileName := r.getFilePathForGroup(g, user) + key := promRules.GroupKey(fullFileName, g.Name()) if _, exists := keptGroups[key]; !exists { - r.backupGroupRulesCount.DeleteLabelValues(user, key) + r.backupRuleGroup.DeleteLabelValues(user, key) } } } @@ -142,9 +146,18 @@ func (r *rulesBackupManager) updateMetrics(newBackupGroups map[string][]*promRul continue } for _, g := range groups { - key := promRules.GroupKey(g.File(), g.Name()) - r.backupGroupRulesCount.DeleteLabelValues(user, key) + fullFileName := r.getFilePathForGroup(g, user) + key := promRules.GroupKey(fullFileName, g.Name()) + r.backupRuleGroup.DeleteLabelValues(user, key) } r.lastBackupReloadSuccessful.DeleteLabelValues(user) } } + +// getFilePathForGroup returns the supposed file path of the group if it was being evaluated. +// This is based on how mapper.go generates file paths. +func (r *rulesBackupManager) getFilePathForGroup(g *promRules.Group, user string) string { + dirPath := filepath.Join(r.cfg.RulePath, user) + encodedFileName := url.PathEscape(g.File()) + return filepath.Join(dirPath, encodedFileName) +} diff --git a/pkg/ruler/rule_backup_manager_test.go b/pkg/ruler/rule_backup_manager_test.go index aca13f161f7..1b10c741bd9 100644 --- a/pkg/ruler/rule_backup_manager_test.go +++ b/pkg/ruler/rule_backup_manager_test.go @@ -2,6 +2,8 @@ package ruler import ( "context" + "net/url" + "path/filepath" "strings" "testing" @@ -154,6 +156,13 @@ func TestBackUpRuleGroupsMetrics(t *testing.T) { reg := prometheus.NewPedanticRegistry() manager := newRulesBackupManager(cfg, log.NewNopLogger(), reg) + getGroupName := func(g *rulespb.RuleGroupDesc, user string) string { + dirPath := filepath.Join(cfg.RulePath, user) + encodedFileName := url.PathEscape(g.Namespace) + path := filepath.Join(dirPath, encodedFileName) + return promRules.GroupKey(path, g.Name) + } + manager.setRuleGroups(context.TODO(), map[string]rulespb.RuleGroupList{ "user1": {&g1, &g2}, "user2": {&g1}, @@ -169,18 +178,18 @@ func TestBackUpRuleGroupsMetrics(t *testing.T) { requireMetricEqual(t, mfm["cortex_ruler_backup_last_reload_successful"].Metric[1], map[string]string{ "user": "user2", }, float64(1)) - require.Equal(t, 3, len(mfm["cortex_ruler_backup_rule_group_rules"].Metric)) - requireMetricEqual(t, mfm["cortex_ruler_backup_rule_group_rules"].Metric[0], map[string]string{ + require.Equal(t, 3, len(mfm["cortex_ruler_backup_rule_group"].Metric)) + requireMetricEqual(t, mfm["cortex_ruler_backup_rule_group"].Metric[0], map[string]string{ "user": "user1", - "rule_group": "ns1;g1", - }, float64(1)) - requireMetricEqual(t, mfm["cortex_ruler_backup_rule_group_rules"].Metric[1], map[string]string{ - "user": "user2", - "rule_group": "ns1;g1", + "rule_group": getGroupName(&g1, "user1"), }, float64(1)) - requireMetricEqual(t, mfm["cortex_ruler_backup_rule_group_rules"].Metric[2], map[string]string{ + requireMetricEqual(t, mfm["cortex_ruler_backup_rule_group"].Metric[1], map[string]string{ "user": "user1", - "rule_group": "ns1;g2", + "rule_group": getGroupName(&g2, "user1"), + }, float64(1)) + requireMetricEqual(t, mfm["cortex_ruler_backup_rule_group"].Metric[2], map[string]string{ + "user": "user2", + "rule_group": getGroupName(&g1, "user2"), }, float64(1)) manager.setRuleGroups(context.TODO(), map[string]rulespb.RuleGroupList{ @@ -194,11 +203,11 @@ func TestBackUpRuleGroupsMetrics(t *testing.T) { requireMetricEqual(t, mfm["cortex_ruler_backup_last_reload_successful"].Metric[0], map[string]string{ "user": "user1", }, float64(1)) - require.Equal(t, 1, len(mfm["cortex_ruler_backup_rule_group_rules"].Metric)) - requireMetricEqual(t, mfm["cortex_ruler_backup_rule_group_rules"].Metric[0], map[string]string{ + require.Equal(t, 1, len(mfm["cortex_ruler_backup_rule_group"].Metric)) + requireMetricEqual(t, mfm["cortex_ruler_backup_rule_group"].Metric[0], map[string]string{ "user": "user1", - "rule_group": "ns1;g2", - }, float64(2)) + "rule_group": getGroupName(&g2, "user1"), + }, float64(1)) } func requireGroupsEqual(t *testing.T, a []*promRules.Group, b []*promRules.Group) { diff --git a/pkg/ruler/ruler.go b/pkg/ruler/ruler.go index e377c6a19fe..df160d29073 100644 --- a/pkg/ruler/ruler.go +++ b/pkg/ruler/ruler.go @@ -324,7 +324,7 @@ func newRuler(cfg Config, manager MultiTenantManager, reg prometheus.Registerer, rulerGetRulesFailures: promauto.With(reg).NewCounterVec(prometheus.CounterOpts{ Name: "cortex_ruler_get_rules_failure_total", - Help: "The total number of failed rules request sent to rulers.", + Help: "The total number of failed rules request sent to rulers in getShardedRules.", }, []string{"ruler"}), } @@ -472,7 +472,7 @@ func tokenForGroup(g *rulespb.RuleGroupDesc) uint32 { return ringHasher.Sum32() } -func instanceOwnsRuleGroup(r ring.ReadRing, g *rulespb.RuleGroupDesc, disabledRuleGroups validation.DisabledRuleGroups, instanceAddr string) (bool, error) { +func instanceOwnsRuleGroup(r ring.ReadRing, g *rulespb.RuleGroupDesc, disabledRuleGroups validation.DisabledRuleGroups, instanceAddr string, forBackup bool) (bool, error) { hash := tokenForGroup(g) @@ -481,36 +481,25 @@ func instanceOwnsRuleGroup(r ring.ReadRing, g *rulespb.RuleGroupDesc, disabledRu return false, errors.Wrap(err, "error reading ring to verify rule group ownership") } - // Even if the replication factor is set to a number bigger than 1, only the first ruler evaluates the rule group - ownsRuleGroup := rlrs.Instances[0].Addr == instanceAddr - if ownsRuleGroup && ruleGroupDisabled(g, disabledRuleGroups) { - return false, &DisabledRuleGroupErr{Message: fmt.Sprintf("rule group %s, namespace %s, user %s is disabled", g.Name, g.Namespace, g.User)} - } - - return ownsRuleGroup, nil -} - -func instanceBacksUpRuleGroup(r ring.ReadRing, g *rulespb.RuleGroupDesc, disabledRuleGroups validation.DisabledRuleGroups, instanceAddr string) (bool, error) { - hash := tokenForGroup(g) - - rlrs, err := r.Get(hash, RingOp, nil, nil, nil) - if err != nil { - return false, errors.Wrap(err, "error reading ring to verify rule group backup ownership") - } - - var backupRuleGroup bool - // Only the second up to the last replica is used a backup - for i := 1; i < len(rlrs.Instances); i++ { - if rlrs.Instances[i].Addr == instanceAddr { - backupRuleGroup = true - break + var ownsRuleGroup bool + if forBackup { + // Only the second up to the last replica are used as backup + for i := 1; i < len(rlrs.Instances); i++ { + if rlrs.Instances[i].Addr == instanceAddr { + ownsRuleGroup = true + break + } } + } else { + // Even if the replication factor is set to a number bigger than 1, only the first ruler evaluates the rule group + ownsRuleGroup = rlrs.Instances[0].Addr == instanceAddr } - if backupRuleGroup && ruleGroupDisabled(g, disabledRuleGroups) { + if ownsRuleGroup && ruleGroupDisabled(g, disabledRuleGroups) { return false, &DisabledRuleGroupErr{Message: fmt.Sprintf("rule group %s, namespace %s, user %s is disabled", g.Name, g.Namespace, g.User)} } - return backupRuleGroup, nil + + return ownsRuleGroup, nil } func (r *Ruler) ServeHTTP(w http.ResponseWriter, req *http.Request) { @@ -788,7 +777,7 @@ func filterRuleGroups(userID string, ruleGroups []*rulespb.RuleGroupDesc, disabl // Prune the rule group to only contain rules that this ruler is responsible for, based on ring. var result []*rulespb.RuleGroupDesc for _, g := range ruleGroups { - owned, err := instanceOwnsRuleGroup(ring, g, disabledRuleGroups, instanceAddr) + owned, err := instanceOwnsRuleGroup(ring, g, disabledRuleGroups, instanceAddr, false) if err != nil { switch e := err.(type) { case *DisabledRuleGroupErr: @@ -820,7 +809,7 @@ func filterRuleGroups(userID string, ruleGroups []*rulespb.RuleGroupDesc, disabl func filterBackupRuleGroups(userID string, ruleGroups []*rulespb.RuleGroupDesc, disabledRuleGroups validation.DisabledRuleGroups, ring ring.ReadRing, instanceAddr string, log log.Logger, ringCheckErrors prometheus.Counter) []*rulespb.RuleGroupDesc { var result []*rulespb.RuleGroupDesc for _, g := range ruleGroups { - backup, err := instanceBacksUpRuleGroup(ring, g, disabledRuleGroups, instanceAddr) + backup, err := instanceOwnsRuleGroup(ring, g, disabledRuleGroups, instanceAddr, true) if err != nil { switch e := err.(type) { case *DisabledRuleGroupErr: @@ -1011,11 +1000,11 @@ func (r *Ruler) getShardedRules(ctx context.Context, userID string, rulesRequest } var ( - mtx sync.Mutex - merged []*GroupStateDesc - errs []error + mtx sync.Mutex + merged []*GroupStateDesc + errCount int ) - failedZones := make(map[string]interface{}) + failedZones := make(map[string]struct{}) zoneByAddress := make(map[string]string) if r.cfg.APIEnableRulesBackup { @@ -1047,9 +1036,9 @@ func (r *Ruler) getShardedRules(ctx context.Context, userID string, rulesRequest // be able to handle failures. if r.cfg.APIEnableRulesBackup && len(jobs) >= r.cfg.Ring.ReplicationFactor { mtx.Lock() - failedZones[zoneByAddress[addr]] = nil - errs = append(errs, err) - failed := (rulers.MaxUnavailableZones > 0 && len(failedZones) > rulers.MaxUnavailableZones) || (rulers.MaxUnavailableZones <= 0 && len(errs) > rulers.MaxErrors) + failedZones[zoneByAddress[addr]] = struct{}{} + errCount += 1 + failed := (rulers.MaxUnavailableZones > 0 && len(failedZones) > rulers.MaxUnavailableZones) || (rulers.MaxUnavailableZones <= 0 && errCount > rulers.MaxErrors) mtx.Unlock() if !failed { return nil From 1bac62e38ca4306013ce22d14610dd6f6196f668 Mon Sep 17 00:00:00 2001 From: Emmanuel Lodovice Date: Wed, 3 Apr 2024 01:17:22 -0700 Subject: [PATCH 08/12] Store rulepb.RuleGroupList in rules backup instead of promRules.Group Signed-off-by: Emmanuel Lodovice --- pkg/ruler/manager.go | 2 +- pkg/ruler/manager_test.go | 24 +-- pkg/ruler/rule_backup_manager.go | 113 +++----------- pkg/ruler/rule_backup_manager_test.go | 101 +------------ pkg/ruler/ruler.go | 103 ++++++++++++- pkg/ruler/ruler_test.go | 209 ++++++++++++++++++++++++++ 6 files changed, 345 insertions(+), 207 deletions(-) diff --git a/pkg/ruler/manager.go b/pkg/ruler/manager.go index e9994dadded..57d2d2907fe 100644 --- a/pkg/ruler/manager.go +++ b/pkg/ruler/manager.go @@ -346,7 +346,7 @@ func (r *DefaultMultiTenantManager) GetRules(userID string) []*promRules.Group { return groups } -func (r *DefaultMultiTenantManager) GetBackupRules(userID string) []*promRules.Group { +func (r *DefaultMultiTenantManager) GetBackupRules(userID string) rulespb.RuleGroupList { if r.rulesBackupManager != nil { return r.rulesBackupManager.getRuleGroups(userID) } diff --git a/pkg/ruler/manager_test.go b/pkg/ruler/manager_test.go index b6969e63d59..d88d47e6613 100644 --- a/pkg/ruler/manager_test.go +++ b/pkg/ruler/manager_test.go @@ -257,7 +257,12 @@ func TestBackupRules(t *testing.T) { dir := t.TempDir() reg := prometheus.NewPedanticRegistry() evalMetrics := NewRuleEvalMetrics(Config{RulePath: dir, EnableQueryStats: true}, reg) - m, err := NewDefaultMultiTenantManager(Config{RulePath: dir, APIEnableRulesBackup: true}, factory, evalMetrics, reg, log.NewNopLogger()) + waitDurations := []time.Duration{ + 1 * time.Millisecond, + 1 * time.Millisecond, + } + ruleManagerFactory := RuleManagerFactory(nil, waitDurations) + m, err := NewDefaultMultiTenantManager(Config{RulePath: dir, APIEnableRulesBackup: true}, ruleManagerFactory, evalMetrics, reg, log.NewNopLogger()) require.NoError(t, err) const user1 = "testUser" @@ -285,21 +290,8 @@ func TestBackupRules(t *testing.T) { }, } m.BackUpRuleGroups(context.TODO(), userRules) - managerOptions := &promRules.ManagerOptions{} - g1 := promRules.NewGroup(promRules.GroupOptions{ - Name: userRules[user1][0].Name, - File: userRules[user1][0].Namespace, - Interval: userRules[user1][0].Interval, - Opts: managerOptions, - }) - g2 := promRules.NewGroup(promRules.GroupOptions{ - Name: userRules[user2][0].Name, - File: userRules[user2][0].Namespace, - Interval: userRules[user2][0].Interval, - Opts: managerOptions, - }) - requireGroupsEqual(t, m.GetBackupRules(user1), []*promRules.Group{g1}) - requireGroupsEqual(t, m.GetBackupRules(user2), []*promRules.Group{g2}) + require.Equal(t, userRules[user1], m.GetBackupRules(user1)) + require.Equal(t, userRules[user2], m.GetBackupRules(user2)) } func getManager(m *DefaultMultiTenantManager, user string) RulesManager { diff --git a/pkg/ruler/rule_backup_manager.go b/pkg/ruler/rule_backup_manager.go index 2e9e6e0a6ea..4ce791aea2a 100644 --- a/pkg/ruler/rule_backup_manager.go +++ b/pkg/ruler/rule_backup_manager.go @@ -2,52 +2,31 @@ package ruler import ( "context" - "errors" "net/url" "path/filepath" "github.com/go-kit/log" - "github.com/go-kit/log/level" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" - "github.com/prometheus/prometheus/model/rulefmt" - "github.com/prometheus/prometheus/promql/parser" promRules "github.com/prometheus/prometheus/rules" "github.com/cortexproject/cortex/pkg/ruler/rulespb" ) -// Implements GroupLoader interface but instead of reading from a file when Load is called, it returns the -// rulefmt.RuleGroup it has stored -type loader struct { - ruleGroups map[string][]rulefmt.RuleGroup -} - -func (r *loader) Load(identifier string) (*rulefmt.RuleGroups, []error) { - return &rulefmt.RuleGroups{ - Groups: r.ruleGroups[identifier], - }, nil -} - -func (r *loader) Parse(query string) (parser.Expr, error) { - return parser.ParseExpr(query) -} - -// rulesBackupManager is an in-memory store that holds []promRules.Group of multiple users. It only stores the Groups, -// it doesn't evaluate them. +// rulesBackupManager is an in-memory store that holds rulespb.RuleGroupList of multiple users. It only stores the +// data, it DOESN'T evaluate. type rulesBackupManager struct { - inMemoryRuleGroupsBackup map[string][]*promRules.Group + inMemoryRuleGroupsBackup map[string]rulespb.RuleGroupList cfg Config logger log.Logger - backupRuleGroup *prometheus.GaugeVec - lastBackupReloadSuccessful *prometheus.GaugeVec + backupRuleGroup *prometheus.GaugeVec } func newRulesBackupManager(cfg Config, logger log.Logger, reg prometheus.Registerer) *rulesBackupManager { return &rulesBackupManager{ - inMemoryRuleGroupsBackup: make(map[string][]*promRules.Group), + inMemoryRuleGroupsBackup: make(map[string]rulespb.RuleGroupList), cfg: cfg, logger: logger, @@ -56,85 +35,39 @@ func newRulesBackupManager(cfg Config, logger log.Logger, reg prometheus.Registe Name: "ruler_backup_rule_group", Help: "Boolean set to 1 indicating the ruler stores the rule group as backup.", }, []string{"user", "rule_group"}), - lastBackupReloadSuccessful: promauto.With(reg).NewGaugeVec(prometheus.GaugeOpts{ - Namespace: "cortex", - Name: "ruler_backup_last_reload_successful", - Help: "Boolean set to 1 whenever the last configuration reload attempt was successful.", - }, []string{"user"}), } } -// setRuleGroups updates the map[string][]*promRules.Group that the rulesBackupManager stores in memory. +// setRuleGroups updates the map[string]rulespb.RuleGroupList that the rulesBackupManager stores in memory. func (r *rulesBackupManager) setRuleGroups(_ context.Context, ruleGroups map[string]rulespb.RuleGroupList) { - backupRuleGroups := make(map[string][]*promRules.Group) - for user, groups := range ruleGroups { - promGroups, err := r.ruleGroupListToPromGroups(user, groups) - if err != nil { - r.lastBackupReloadSuccessful.WithLabelValues(user).Set(0) - level.Error(r.logger).Log("msg", "unable to back up rules", "user", user, "err", err) - continue - } - backupRuleGroups[user] = promGroups - r.lastBackupReloadSuccessful.WithLabelValues(user).Set(1) - } - r.updateMetrics(backupRuleGroups) - r.inMemoryRuleGroupsBackup = backupRuleGroups -} - -// ruleGroupListToPromGroups converts rulespb.RuleGroupList to []*promRules.Group by creating a single use -// promRules.Manager and calling its LoadGroups method. -func (r *rulesBackupManager) ruleGroupListToPromGroups(user string, ruleGroups rulespb.RuleGroupList) ([]*promRules.Group, error) { - rgs := ruleGroups.Formatted() - - loader := &loader{ - ruleGroups: rgs, - } - promManager := promRules.NewManager(&promRules.ManagerOptions{ - ExternalURL: r.cfg.ExternalURL.URL, - GroupLoader: loader, - }) - - namespaces := make([]string, 0, len(rgs)) - for k := range rgs { - namespaces = append(namespaces, k) - } - loadedGroups, errs := promManager.LoadGroups(r.cfg.EvaluationInterval, r.cfg.ExternalLabels, r.cfg.ExternalURL.String(), nil, namespaces...) - if errs != nil { - for _, e := range errs { - level.Error(r.logger).Log("msg", "loading groups to backup failed", "user", user, "namespaces", namespaces, "err", e) - } - return nil, errors.New("error loading rules to backup") - } - - groups := make([]*promRules.Group, 0, len(loadedGroups)) - for _, g := range loadedGroups { - groups = append(groups, g) - } - return groups, nil + r.updateMetrics(ruleGroups) + r.inMemoryRuleGroupsBackup = ruleGroups } -// getRuleGroups returns the []*promRules.Group that rulesBackupManager stores for a given user -func (r *rulesBackupManager) getRuleGroups(userID string) []*promRules.Group { - var result []*promRules.Group +// getRuleGroups returns the rulespb.RuleGroupList that rulesBackupManager stores for a given user +func (r *rulesBackupManager) getRuleGroups(userID string) rulespb.RuleGroupList { + var result rulespb.RuleGroupList if groups, exists := r.inMemoryRuleGroupsBackup[userID]; exists { result = groups } return result } -func (r *rulesBackupManager) updateMetrics(newBackupGroups map[string][]*promRules.Group) { +// getRuleGroups updates the ruler_backup_rule_group metric by adding new groups that were backed up and removing +// those that are removed from the backup. +func (r *rulesBackupManager) updateMetrics(newBackupGroups map[string]rulespb.RuleGroupList) { for user, groups := range newBackupGroups { - keptGroups := make(map[string][]interface{}) + keptGroups := make(map[string]struct{}) for _, g := range groups { fullFileName := r.getFilePathForGroup(g, user) - key := promRules.GroupKey(fullFileName, g.Name()) + key := promRules.GroupKey(fullFileName, g.GetName()) r.backupRuleGroup.WithLabelValues(user, key).Set(1) - keptGroups[key] = nil + keptGroups[key] = struct{}{} } oldGroups := r.inMemoryRuleGroupsBackup[user] for _, g := range oldGroups { fullFileName := r.getFilePathForGroup(g, user) - key := promRules.GroupKey(fullFileName, g.Name()) + key := promRules.GroupKey(fullFileName, g.GetName()) if _, exists := keptGroups[key]; !exists { r.backupRuleGroup.DeleteLabelValues(user, key) } @@ -147,17 +80,17 @@ func (r *rulesBackupManager) updateMetrics(newBackupGroups map[string][]*promRul } for _, g := range groups { fullFileName := r.getFilePathForGroup(g, user) - key := promRules.GroupKey(fullFileName, g.Name()) + key := promRules.GroupKey(fullFileName, g.GetName()) r.backupRuleGroup.DeleteLabelValues(user, key) } - r.lastBackupReloadSuccessful.DeleteLabelValues(user) } } // getFilePathForGroup returns the supposed file path of the group if it was being evaluated. -// This is based on how mapper.go generates file paths. -func (r *rulesBackupManager) getFilePathForGroup(g *promRules.Group, user string) string { +// This is based on how mapper.go generates file paths. This can be used to generate value similar to the one returned +// by prometheus Group.File() method. +func (r *rulesBackupManager) getFilePathForGroup(g *rulespb.RuleGroupDesc, user string) string { dirPath := filepath.Join(r.cfg.RulePath, user) - encodedFileName := url.PathEscape(g.File()) + encodedFileName := url.PathEscape(g.GetNamespace()) return filepath.Join(dirPath, encodedFileName) } diff --git a/pkg/ruler/rule_backup_manager_test.go b/pkg/ruler/rule_backup_manager_test.go index 1b10c741bd9..224b164be3c 100644 --- a/pkg/ruler/rule_backup_manager_test.go +++ b/pkg/ruler/rule_backup_manager_test.go @@ -4,17 +4,13 @@ import ( "context" "net/url" "path/filepath" - "strings" "testing" "github.com/go-kit/log" "github.com/prometheus/client_golang/prometheus" io_prometheus_client "github.com/prometheus/client_model/go" - "github.com/prometheus/prometheus/model/labels" - "github.com/prometheus/prometheus/promql/parser" promRules "github.com/prometheus/prometheus/rules" "github.com/stretchr/testify/require" - "golang.org/x/exp/slices" "github.com/cortexproject/cortex/pkg/ruler/rulespb" "github.com/cortexproject/cortex/pkg/util" @@ -25,7 +21,6 @@ func TestBackUpRuleGroups(t *testing.T) { Expr: "1 > bool 1", Record: "test", } - parsedExpr, _ := parser.ParseExpr(r.Expr) g1 := rulespb.RuleGroupDesc{ Name: "g1", Namespace: "ns1", @@ -42,90 +37,36 @@ func TestBackUpRuleGroups(t *testing.T) { Rules: []*rulespb.RuleDesc{&r}, } - rInvalid := rulespb.RuleDesc{ - Expr: "1 > 1", // invalid expression - } - gInvalid := rulespb.RuleGroupDesc{ - Name: "g1", - Namespace: "ns1", - Rules: []*rulespb.RuleDesc{&rInvalid}, - } cfg := defaultRulerConfig(t) - managerOptions := &promRules.ManagerOptions{} manager := newRulesBackupManager(cfg, log.NewNopLogger(), nil) - g1Option := promRules.GroupOptions{ - Name: g1.Name, - File: g1.Namespace, - Interval: cfg.EvaluationInterval, - Rules: []promRules.Rule{ - promRules.NewRecordingRule(r.Record, parsedExpr, labels.Labels{}), - }, - } - g2Option := promRules.GroupOptions{ - Name: g2.Name, - File: g2.Namespace, - Interval: cfg.EvaluationInterval, - Rules: []promRules.Rule{ - promRules.NewRecordingRule(r.Record, parsedExpr, labels.Labels{}), - }, - } - g3Option := promRules.GroupOptions{ - Name: g3.Name, - File: g3.Namespace, - Interval: cfg.EvaluationInterval, - Rules: []promRules.Rule{ - promRules.NewRecordingRule(r.Record, parsedExpr, labels.Labels{}), - }, - } type testCase struct { - input map[string]rulespb.RuleGroupList - expectedOutput map[string][]*promRules.GroupOptions + input map[string]rulespb.RuleGroupList } testCases := map[string]testCase{ "Empty input": { - input: make(map[string]rulespb.RuleGroupList), - expectedOutput: make(map[string][]*promRules.GroupOptions), - }, - "With invalid rules": { - input: map[string]rulespb.RuleGroupList{ - "user1": {&gInvalid}, - }, - expectedOutput: make(map[string][]*promRules.GroupOptions), + input: make(map[string]rulespb.RuleGroupList), }, - "With partial invalid rules": { + "With groups from single users": { input: map[string]rulespb.RuleGroupList{ - "user1": {&gInvalid, &g3}, "user2": {&g1, &g2}, }, - expectedOutput: map[string][]*promRules.GroupOptions{ - "user2": {&g1Option, &g2Option}, - }, }, "With groups from multiple users": { input: map[string]rulespb.RuleGroupList{ - "user1": {&g1, &g2, &g3}, + "user1": {&g1, &g3}, "user2": {&g1, &g2}, }, - expectedOutput: map[string][]*promRules.GroupOptions{ - "user1": {&g1Option, &g2Option, &g3Option}, - "user2": {&g1Option, &g2Option}, - }, }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { manager.setRuleGroups(context.TODO(), tc.input) - require.Equal(t, len(tc.expectedOutput), len(manager.inMemoryRuleGroupsBackup)) - for user, expectedGroupOptions := range tc.expectedOutput { + require.Equal(t, len(tc.input), len(manager.inMemoryRuleGroupsBackup)) + for user, groups := range tc.input { loadedGroups := manager.getRuleGroups(user) - expectedGroups := make([]*promRules.Group, 0, len(expectedGroupOptions)) - for _, o := range expectedGroupOptions { - o.Opts = managerOptions - expectedGroups = append(expectedGroups, promRules.NewGroup(*o)) - } - requireGroupsEqual(t, expectedGroups, loadedGroups) + require.Equal(t, groups, loadedGroups) } }) } @@ -171,13 +112,6 @@ func TestBackUpRuleGroupsMetrics(t *testing.T) { require.NoError(t, err) mfm, err := util.NewMetricFamilyMap(gm) require.NoError(t, err) - require.Equal(t, 2, len(mfm["cortex_ruler_backup_last_reload_successful"].Metric)) - requireMetricEqual(t, mfm["cortex_ruler_backup_last_reload_successful"].Metric[0], map[string]string{ - "user": "user1", - }, float64(1)) - requireMetricEqual(t, mfm["cortex_ruler_backup_last_reload_successful"].Metric[1], map[string]string{ - "user": "user2", - }, float64(1)) require.Equal(t, 3, len(mfm["cortex_ruler_backup_rule_group"].Metric)) requireMetricEqual(t, mfm["cortex_ruler_backup_rule_group"].Metric[0], map[string]string{ "user": "user1", @@ -199,10 +133,6 @@ func TestBackUpRuleGroupsMetrics(t *testing.T) { require.NoError(t, err) mfm, err = util.NewMetricFamilyMap(gm) require.NoError(t, err) - require.Equal(t, 1, len(mfm["cortex_ruler_backup_last_reload_successful"].Metric)) - requireMetricEqual(t, mfm["cortex_ruler_backup_last_reload_successful"].Metric[0], map[string]string{ - "user": "user1", - }, float64(1)) require.Equal(t, 1, len(mfm["cortex_ruler_backup_rule_group"].Metric)) requireMetricEqual(t, mfm["cortex_ruler_backup_rule_group"].Metric[0], map[string]string{ "user": "user1", @@ -210,23 +140,6 @@ func TestBackUpRuleGroupsMetrics(t *testing.T) { }, float64(1)) } -func requireGroupsEqual(t *testing.T, a []*promRules.Group, b []*promRules.Group) { - require.Equal(t, len(a), len(b)) - sortFunc := func(g1, g2 *promRules.Group) int { - fileCompare := strings.Compare(g1.File(), g2.File()) - if fileCompare != 0 { - return fileCompare - } - return strings.Compare(g1.Name(), g2.Name()) - } - slices.SortFunc(a, sortFunc) - slices.SortFunc(b, sortFunc) - for i, gA := range a { - gB := b[i] - require.True(t, gA.Equals(gB), "group1", gA.Name(), "group2", gB.Name()) - } -} - func requireMetricEqual(t *testing.T, m *io_prometheus_client.Metric, labels map[string]string, value float64) { l := m.GetLabel() require.Equal(t, len(labels), len(l)) diff --git a/pkg/ruler/ruler.go b/pkg/ruler/ruler.go index df160d29073..0dda94be573 100644 --- a/pkg/ruler/ruler.go +++ b/pkg/ruler/ruler.go @@ -20,6 +20,7 @@ import ( "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/model/rulefmt" "github.com/prometheus/prometheus/notifier" + "github.com/prometheus/prometheus/promql/parser" promRules "github.com/prometheus/prometheus/rules" "github.com/prometheus/prometheus/util/strutil" "github.com/weaveworks/common/user" @@ -224,7 +225,7 @@ type MultiTenantManager interface { // GetRules fetches rules for a particular tenant (userID). GetRules(userID string) []*promRules.Group // GetBackupRules fetches rules for a particular tenant (userID) that the ruler stores for backup purposes - GetBackupRules(userID string) []*promRules.Group + GetBackupRules(userID string) rulespb.RuleGroupList // Stop stops all Manager components. Stop() // ValidateRuleGroup validates a rulegroup @@ -851,11 +852,6 @@ func (r *Ruler) GetRules(ctx context.Context, rulesRequest RulesRequest) ([]*Gro func (r *Ruler) getLocalRules(userID string, rulesRequest RulesRequest, includeBackups bool) ([]*GroupStateDesc, error) { groups := r.manager.GetRules(userID) - if includeBackups { - backupGroups := r.manager.GetBackupRules(userID) - groups = append(groups, backupGroups...) - } - groupDescs := make([]*GroupStateDesc, 0, len(groups)) prefix := filepath.Join(r.cfg.RulePath, userID) + "/" @@ -979,6 +975,101 @@ func (r *Ruler) getLocalRules(userID string, rulesRequest RulesRequest, includeB } } + if !includeBackups { + return groupDescs, nil + } + + backupGroups := r.manager.GetBackupRules(userID) + for _, group := range backupGroups { + if len(fileSet) > 0 { + if _, OK := fileSet[group.GetNamespace()]; !OK { + continue + } + } + + if len(ruleGroupNameSet) > 0 { + if _, OK := ruleGroupNameSet[group.GetName()]; !OK { + continue + } + } + interval := r.cfg.EvaluationInterval + if group.Interval != 0 { + interval = group.Interval + } + + groupDesc := &GroupStateDesc{ + Group: &rulespb.RuleGroupDesc{ + Name: group.GetName(), + Namespace: group.GetNamespace(), + Interval: interval, + User: userID, + Limit: group.Limit, + }, + // We are keeping default value for EvaluationTimestamp and EvaluationDuration since the backup is not evaluating + } + for _, r := range group.GetRules() { + name := r.GetRecord() + isAlertingRule := false + if name == "" { + name = r.GetAlert() + isAlertingRule = true + } + if len(ruleNameSet) > 0 { + if _, OK := ruleNameSet[name]; !OK { + continue + } + } + lastError := "" + + var ruleDesc *RuleStateDesc + query, err := parser.ParseExpr(r.GetExpr()) + if err != nil { + return nil, errors.Errorf("failed to parse rule query '%v'", r.GetExpr()) + } + if isAlertingRule { + if !returnAlerts { + continue + } + alerts := []*AlertStateDesc{} // backup rules are not evaluated so there will be no active alerts + ruleDesc = &RuleStateDesc{ + Rule: &rulespb.RuleDesc{ + Expr: query.String(), + Alert: name, + For: r.GetFor(), + KeepFiringFor: r.GetKeepFiringFor(), + Labels: r.Labels, + Annotations: r.Annotations, + }, + State: promRules.StateInactive.String(), // backup rules are not evaluated so they are inactive + Health: string(promRules.HealthUnknown), + LastError: lastError, + Alerts: alerts, + EvaluationTimestamp: time.Time{}, + EvaluationDuration: time.Duration(0), + } + } else { + if !returnRecording { + continue + } + ruleDesc = &RuleStateDesc{ + Rule: &rulespb.RuleDesc{ + Record: name, + Expr: query.String(), + Labels: r.Labels, + }, + Health: string(promRules.HealthUnknown), + LastError: lastError, + EvaluationTimestamp: time.Time{}, + EvaluationDuration: time.Duration(0), + } + } + groupDesc.ActiveRules = append(groupDesc.ActiveRules, ruleDesc) + } + if len(groupDesc.ActiveRules) > 0 { + groupDescs = append(groupDescs, groupDesc) + } + } + return groupDescs, nil } diff --git a/pkg/ruler/ruler_test.go b/pkg/ruler/ruler_test.go index 9282284352f..22d52aa6495 100644 --- a/pkg/ruler/ruler_test.go +++ b/pkg/ruler/ruler_test.go @@ -903,6 +903,215 @@ func TestGetRules(t *testing.T) { } } +func TestGetRulesFromBackup(t *testing.T) { + // ruler ID -> (user ID -> list of groups). + type expectedRulesMap map[string]map[string]rulespb.RuleGroupList + + rule := []*rulespb.RuleDesc{ + { + Record: "rtest_user1_1", + Expr: "sum(rate(node_cpu_seconds_total[3h:10m]))", + }, + { + Alert: "atest_user1_1", + Expr: "sum(rate(node_cpu_seconds_total[3h:10m]))", + }, + { + Record: "rtest_user1_2", + Expr: "sum(rate(node_cpu_seconds_total[3h:10m]))", + Labels: []cortexpb.LabelAdapter{ + {Name: "key", Value: "val"}, + }, + }, + { + Alert: "atest_user1_2", + Expr: "sum(rate(node_cpu_seconds_total[3h:10m]))", + Labels: []cortexpb.LabelAdapter{ + {Name: "key", Value: "val"}, + }, + Annotations: []cortexpb.LabelAdapter{ + {Name: "aKey", Value: "aVal"}, + }, + For: 10 * time.Second, + KeepFiringFor: 20 * time.Second, + }, + } + + tenantId := "user1" + + rulerStateMapOnePending := map[string]ring.InstanceState{ + "ruler1": ring.ACTIVE, + "ruler2": ring.PENDING, + "ruler3": ring.ACTIVE, + } + + rulerAZEvenSpread := map[string]string{ + "ruler1": "a", + "ruler2": "b", + "ruler3": "c", + } + + expectedRules := expectedRulesMap{ + "ruler1": map[string]rulespb.RuleGroupList{ + tenantId: { + &rulespb.RuleGroupDesc{User: "user1", Namespace: "namespace", Name: "l1", Interval: 10 * time.Minute, Limit: 10, Rules: rule}, + &rulespb.RuleGroupDesc{User: "user1", Namespace: "namespace", Name: "l2", Interval: 0, Rules: rule}, + }, + }, + "ruler2": map[string]rulespb.RuleGroupList{ + tenantId: { + &rulespb.RuleGroupDesc{User: "user1", Namespace: "namespace", Name: "b1", Interval: 10 * time.Minute, Limit: 10, Rules: rule}, + &rulespb.RuleGroupDesc{User: "user1", Namespace: "namespace", Name: "b2", Interval: 0, Rules: rule}, + }, + }, + "ruler3": map[string]rulespb.RuleGroupList{ + tenantId: { + &rulespb.RuleGroupDesc{User: "user1", Namespace: "namespace2", Name: "b3", Interval: 0, Rules: rule}, + }, + }, + } + + kvStore, cleanUp := consul.NewInMemoryClient(ring.GetCodec(), log.NewNopLogger(), nil) + t.Cleanup(func() { assert.NoError(t, cleanUp.Close()) }) + allRulesByUser := map[string]rulespb.RuleGroupList{} + allTokensByRuler := map[string][]uint32{} + rulerAddrMap := map[string]*Ruler{} + + createRuler := func(id string) *Ruler { + store := newMockRuleStore(allRulesByUser, nil) + cfg := defaultRulerConfig(t) + + cfg.ShardingStrategy = util.ShardingStrategyShuffle + cfg.EnableSharding = true + cfg.APIEnableRulesBackup = true + cfg.EvaluationInterval = 5 * time.Minute + + cfg.Ring = RingConfig{ + InstanceID: id, + InstanceAddr: id, + KVStore: kv.Config{ + Mock: kvStore, + }, + ReplicationFactor: 3, + ZoneAwarenessEnabled: true, + InstanceZone: rulerAZEvenSpread[id], + } + + r, _ := buildRuler(t, cfg, nil, store, rulerAddrMap) + r.limits = ruleLimits{evalDelay: 0, tenantShard: 3} + rulerAddrMap[id] = r + if r.ring != nil { + require.NoError(t, services.StartAndAwaitRunning(context.Background(), r.ring)) + t.Cleanup(r.ring.StopAsync) + } + return r + } + + for rID, r := range expectedRules { + createRuler(rID) + for u, rules := range r { + allRulesByUser[u] = append(allRulesByUser[u], rules...) + allTokensByRuler[rID] = generateTokenForGroups(rules, 1) + } + } + + err := kvStore.CAS(context.Background(), ringKey, func(in interface{}) (out interface{}, retry bool, err error) { + d, _ := in.(*ring.Desc) + if d == nil { + d = ring.NewDesc() + } + for rID, tokens := range allTokensByRuler { + d.AddIngester(rID, rulerAddrMap[rID].lifecycler.GetInstanceAddr(), rulerAddrMap[rID].lifecycler.GetInstanceZone(), tokens, ring.ACTIVE, time.Now()) + } + return d, true, nil + }) + require.NoError(t, err) + // Wait a bit to make sure ruler's ring is updated. + time.Sleep(100 * time.Millisecond) + + forEachRuler := func(f func(rID string, r *Ruler)) { + for rID, r := range rulerAddrMap { + f(rID, r) + } + } + + // Sync Rules + forEachRuler(func(_ string, r *Ruler) { + r.syncRules(context.Background(), rulerSyncReasonInitial) + }) + + // update the State of the rulers in the ring based on tc.rulerStateMap + err = kvStore.CAS(context.Background(), ringKey, func(in interface{}) (out interface{}, retry bool, err error) { + d, _ := in.(*ring.Desc) + if d == nil { + d = ring.NewDesc() + } + for rID, tokens := range allTokensByRuler { + d.AddIngester(rID, rulerAddrMap[rID].lifecycler.GetInstanceAddr(), rulerAddrMap[rID].lifecycler.GetInstanceZone(), tokens, rulerStateMapOnePending[rID], time.Now()) + } + return d, true, nil + }) + require.NoError(t, err) + // Wait a bit to make sure ruler's ring is updated. + time.Sleep(100 * time.Millisecond) + + requireGroupStateEqual := func(a *GroupStateDesc, b *GroupStateDesc) { + require.Equal(t, a.Group.Interval, b.Group.Interval) + require.Equal(t, a.Group.User, b.Group.User) + require.Equal(t, a.Group.Limit, b.Group.Limit) + require.Equal(t, a.EvaluationTimestamp, b.EvaluationTimestamp) + require.Equal(t, a.EvaluationDuration, b.EvaluationDuration) + require.Equal(t, len(a.ActiveRules), len(b.ActiveRules)) + for i, aRule := range a.ActiveRules { + bRule := b.ActiveRules[i] + require.Equal(t, aRule.EvaluationTimestamp, bRule.EvaluationTimestamp) + require.Equal(t, aRule.EvaluationDuration, bRule.EvaluationDuration) + require.Equal(t, aRule.Health, bRule.Health) + require.Equal(t, aRule.LastError, bRule.LastError) + require.Equal(t, aRule.Rule.Expr, bRule.Rule.Expr) + require.Equal(t, len(aRule.Rule.Labels), len(bRule.Rule.Labels)) + require.Equal(t, fmt.Sprintf("%+v", aRule.Rule.Labels), fmt.Sprintf("%+v", aRule.Rule.Labels)) + if aRule.Rule.Alert != "" { + require.Equal(t, fmt.Sprintf("%+v", aRule.Rule.Annotations), fmt.Sprintf("%+v", bRule.Rule.Annotations)) + require.Equal(t, aRule.Rule.Alert, bRule.Rule.Alert) + require.Equal(t, aRule.Rule.For, bRule.Rule.For) + require.Equal(t, aRule.Rule.KeepFiringFor, bRule.Rule.KeepFiringFor) + require.Equal(t, aRule.State, bRule.State) + require.Equal(t, aRule.Alerts, bRule.Alerts) + } else { + require.Equal(t, aRule.Rule.Record, bRule.Rule.Record) + } + } + } + ctx := user.InjectOrgID(context.Background(), tenantId) + ruleStateDescriptions, err := rulerAddrMap["ruler1"].GetRules(ctx, RulesRequest{}) + require.NoError(t, err) + require.Equal(t, 5, len(ruleStateDescriptions)) + stateByKey := map[string]*GroupStateDesc{} + for _, state := range ruleStateDescriptions { + stateByKey[state.Group.Namespace+";"+state.Group.Name] = state + } + // Rule Group Name that starts will b are from the backup and those that start with l are evaluating, the details of + // the group other than the Name should be equal to the group that starts with l as the config is the same. This test + // confirms that the way we convert rulepb.RuleGroupList to GroupStateDesc is consistent to how we convert + // promRules.Group to GroupStateDesc + requireGroupStateEqual(stateByKey["namespace;l1"], stateByKey["namespace;b1"]) + requireGroupStateEqual(stateByKey["namespace;l2"], stateByKey["namespace;b2"]) + + // Validate backup rules respect the filters + ruleStateDescriptions, err = rulerAddrMap["ruler1"].GetRules(ctx, RulesRequest{ + RuleNames: []string{"rtest_user1_1", "atest_user1_1"}, + Files: []string{"namespace"}, + RuleGroupNames: []string{"b1"}, + Type: recordingRuleFilter, + }) + require.NoError(t, err) + require.Equal(t, 1, len(ruleStateDescriptions)) + require.Equal(t, "b1", ruleStateDescriptions[0].Group.Name) + require.Equal(t, 1, len(ruleStateDescriptions[0].ActiveRules)) + require.Equal(t, "rtest_user1_1", ruleStateDescriptions[0].ActiveRules[0].Rule.Record) +} + func TestSharding(t *testing.T) { const ( user1 = "user1" From 4aef787a5094adb8131dca77a10a27ef51975e00 Mon Sep 17 00:00:00 2001 From: Emmanuel Lodovice Date: Thu, 4 Apr 2024 01:14:42 -0700 Subject: [PATCH 09/12] Add GetReplicationSetForOperationWithNoQuorum ring method and use it in getShardedRules Signed-off-by: Emmanuel Lodovice --- .../shuffle_sharding_grouper_test.go | 5 ++ pkg/ring/ring.go | 62 +++++++++++----- pkg/ring/ring_test.go | 73 +++++++++++++++++-- pkg/ring/util_test.go | 5 ++ pkg/ruler/ruler.go | 3 +- 5 files changed, 121 insertions(+), 27 deletions(-) diff --git a/pkg/compactor/shuffle_sharding_grouper_test.go b/pkg/compactor/shuffle_sharding_grouper_test.go index e237a43a921..26d6248cad5 100644 --- a/pkg/compactor/shuffle_sharding_grouper_test.go +++ b/pkg/compactor/shuffle_sharding_grouper_test.go @@ -771,6 +771,11 @@ func (r *RingMock) GetReplicationSetForOperation(op ring.Operation) (ring.Replic return args.Get(0).(ring.ReplicationSet), args.Error(1) } +func (r *RingMock) GetReplicationSetForOperationWithNoQuorum(op ring.Operation) (ring.ReplicationSet, map[string]struct{}, error) { + args := r.Called(op) + return args.Get(0).(ring.ReplicationSet), make(map[string]struct{}), args.Error(1) +} + func (r *RingMock) ReplicationFactor() int { return 0 } diff --git a/pkg/ring/ring.go b/pkg/ring/ring.go index aad67332ef5..40b0b43c82a 100644 --- a/pkg/ring/ring.go +++ b/pkg/ring/ring.go @@ -59,6 +59,13 @@ type ReadRing interface { // the input operation. GetReplicationSetForOperation(op Operation) (ReplicationSet, error) + // GetReplicationSetForOperationWithNoQuorum returns all instances where the input operation should be executed. + // The resulting ReplicationSet contains all healthy instances in the ring, but the computation for MaxErrors + // does not require quorum so only 1 replica is needed to complete the operation. For MaxUnavailableZones, it is + // not automatically reduced when there are unhealthy instances in a zone because healthy instances in the zone + // are still returned, but the information about zones with unhealthy instances is returned. + GetReplicationSetForOperationWithNoQuorum(op Operation) (ReplicationSet, map[string]struct{}, error) + ReplicationFactor() int // InstancesCount returns the number of instances in the ring. @@ -484,13 +491,12 @@ func (r *Ring) GetInstanceDescsForOperation(op Operation) (map[string]InstanceDe return instanceDescs, nil } -// GetReplicationSetForOperation implements ReadRing. -func (r *Ring) GetReplicationSetForOperation(op Operation) (ReplicationSet, error) { +func (r *Ring) getReplicationSetForOperation(op Operation, requireQuorum bool) (ReplicationSet, map[string]struct{}, error) { r.mtx.RLock() defer r.mtx.RUnlock() if r.ringDesc == nil || len(r.ringTokens) == 0 { - return ReplicationSet{}, ErrEmptyRing + return ReplicationSet{}, make(map[string]struct{}), ErrEmptyRing } // Build the initial replication set, excluding unhealthy instances. @@ -512,18 +518,24 @@ func (r *Ring) GetReplicationSetForOperation(op Operation) (ReplicationSet, erro maxUnavailableZones := 0 if r.cfg.ZoneAwarenessEnabled { - // Given data is replicated to RF different zones, we can tolerate a number of - // RF/2 failing zones. However, we need to protect from the case the ring currently - // contains instances in a number of zones < RF. numReplicatedZones := utilmath.Min(len(r.ringZones), r.cfg.ReplicationFactor) - minSuccessZones := (numReplicatedZones / 2) + 1 - maxUnavailableZones = minSuccessZones - 1 + if requireQuorum { + // Given data is replicated to RF different zones, we can tolerate a number of + // RF/2 failing zones. However, we need to protect from the case the ring currently + // contains instances in a number of zones < RF. + minSuccessZones := (numReplicatedZones / 2) + 1 + maxUnavailableZones = minSuccessZones - 1 + } else { + // Given that quorum is not required, we only need at least one of the zone to be healthy to succeed. But we + // also need to handle case when RF < number of zones. + maxUnavailableZones = numReplicatedZones - 1 + } if len(zoneFailures) > maxUnavailableZones { - return ReplicationSet{}, ErrTooManyUnhealthyInstances + return ReplicationSet{}, zoneFailures, ErrTooManyUnhealthyInstances } - if len(zoneFailures) > 0 { + if requireQuorum && len(zoneFailures) > 0 { // We remove all instances (even healthy ones) from zones with at least // 1 failing instance. Due to how replication works when zone-awareness is // enabled (data is replicated to RF different zones), there's no benefit in @@ -537,11 +549,11 @@ func (r *Ring) GetReplicationSetForOperation(op Operation) (ReplicationSet, erro } healthyInstances = filteredInstances - } - // Since we removed all instances from zones containing at least 1 failing - // instance, we have to decrease the max unavailable zones accordingly. - maxUnavailableZones -= len(zoneFailures) + // Since we removed all instances from zones containing at least 1 failing + // instance, we have to decrease the max unavailable zones accordingly. + maxUnavailableZones -= len(zoneFailures) + } } else { // Calculate the number of required instances; // ensure we always require at least RF-1 when RF=3. @@ -550,10 +562,15 @@ func (r *Ring) GetReplicationSetForOperation(op Operation) (ReplicationSet, erro numRequired = r.cfg.ReplicationFactor } // We can tolerate this many failures - numRequired -= r.cfg.ReplicationFactor / 2 + if requireQuorum { + numRequired -= r.cfg.ReplicationFactor / 2 + } else { + // if quorum is not required then 1 replica is enough to handle the request + numRequired -= r.cfg.ReplicationFactor - 1 + } if len(healthyInstances) < numRequired { - return ReplicationSet{}, ErrTooManyUnhealthyInstances + return ReplicationSet{}, zoneFailures, ErrTooManyUnhealthyInstances } maxErrors = len(healthyInstances) - numRequired @@ -563,7 +580,18 @@ func (r *Ring) GetReplicationSetForOperation(op Operation) (ReplicationSet, erro Instances: healthyInstances, MaxErrors: maxErrors, MaxUnavailableZones: maxUnavailableZones, - }, nil + }, zoneFailures, nil +} + +// GetReplicationSetForOperation implements ReadRing. +func (r *Ring) GetReplicationSetForOperation(op Operation) (ReplicationSet, error) { + replicationSet, _, err := r.getReplicationSetForOperation(op, true) + return replicationSet, err +} + +// GetReplicationSetForOperationWithNoQuorum implements ReadRing. +func (r *Ring) GetReplicationSetForOperationWithNoQuorum(op Operation) (ReplicationSet, map[string]struct{}, error) { + return r.getReplicationSetForOperation(op, false) } // countTokens returns the number of tokens and tokens within the range for each instance. diff --git a/pkg/ring/ring_test.go b/pkg/ring/ring_test.go index cf8582d6449..38f8aa7054b 100644 --- a/pkg/ring/ring_test.go +++ b/pkg/ring/ring_test.go @@ -959,11 +959,11 @@ func TestRing_GetInstanceDescsForOperation(t *testing.T) { }, instanceDescs) } -func TestRing_GetReplicationSetForOperation(t *testing.T) { +func validateGetReplicationSetForOperation(t *testing.T, requireQuorum bool) { now := time.Now() g := NewRandomTokenGenerator() - tests := map[string]struct { + type testCase struct { ringInstances map[string]InstanceDesc ringHeartbeatTimeout time.Duration ringReplicationFactor int @@ -973,7 +973,9 @@ func TestRing_GetReplicationSetForOperation(t *testing.T) { expectedSetForWrite []string expectedErrForReporting error expectedSetForReporting []string - }{ + } + + tests := map[string]testCase{ "should return error on empty ring": { ringInstances: nil, ringHeartbeatTimeout: time.Minute, @@ -1038,7 +1040,10 @@ func TestRing_GetReplicationSetForOperation(t *testing.T) { expectedSetForWrite: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3", "127.0.0.4"}, expectedSetForReporting: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3", "127.0.0.4"}, }, - "should fail on 2 unhealthy instances and RF=3": { + } + + if requireQuorum { + tests["should fail on 2 unhealthy instances and RF=3"] = testCase{ ringInstances: map[string]InstanceDesc{ "instance-1": {Addr: "127.0.0.1", State: ACTIVE, Timestamp: now.Unix(), Tokens: g.GenerateTokens(NewDesc(), "instance-1", "", 128, true)}, "instance-2": {Addr: "127.0.0.2", State: ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: g.GenerateTokens(NewDesc(), "instance-2", "", 128, true)}, @@ -1051,7 +1056,36 @@ func TestRing_GetReplicationSetForOperation(t *testing.T) { expectedErrForRead: ErrTooManyUnhealthyInstances, expectedErrForWrite: ErrTooManyUnhealthyInstances, expectedErrForReporting: ErrTooManyUnhealthyInstances, - }, + } + } else { + tests["should pass on 2 unhealthy instances and RF=3"] = testCase{ + ringInstances: map[string]InstanceDesc{ + "instance-1": {Addr: "127.0.0.1", State: ACTIVE, Timestamp: now.Unix(), Tokens: GenerateTokens(128, nil)}, + "instance-2": {Addr: "127.0.0.2", State: ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: GenerateTokens(128, nil)}, + "instance-3": {Addr: "127.0.0.3", State: ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: GenerateTokens(128, nil)}, + "instance-4": {Addr: "127.0.0.4", State: ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: GenerateTokens(128, nil)}, + "instance-5": {Addr: "127.0.0.5", State: ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: GenerateTokens(128, nil)}, + }, + ringHeartbeatTimeout: time.Minute, + ringReplicationFactor: 3, + expectedSetForRead: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3"}, + expectedSetForWrite: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3"}, + expectedSetForReporting: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3"}, + } + tests["should fail on 3 unhealthy instances and RF=3"] = testCase{ + ringInstances: map[string]InstanceDesc{ + "instance-1": {Addr: "127.0.0.1", State: ACTIVE, Timestamp: now.Unix(), Tokens: GenerateTokens(128, nil)}, + "instance-2": {Addr: "127.0.0.2", State: ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: GenerateTokens(128, nil)}, + "instance-3": {Addr: "127.0.0.3", State: ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: GenerateTokens(128, nil)}, + "instance-4": {Addr: "127.0.0.4", State: ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: GenerateTokens(128, nil)}, + "instance-5": {Addr: "127.0.0.5", State: ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: GenerateTokens(128, nil)}, + }, + ringHeartbeatTimeout: time.Minute, + ringReplicationFactor: 3, + expectedErrForRead: ErrTooManyUnhealthyInstances, + expectedErrForWrite: ErrTooManyUnhealthyInstances, + expectedErrForReporting: ErrTooManyUnhealthyInstances, + } } for testName, testData := range tests { @@ -1076,21 +1110,44 @@ func TestRing_GetReplicationSetForOperation(t *testing.T) { KVClient: &MockClient{}, } - set, err := ring.GetReplicationSetForOperation(Read) + var set ReplicationSet + var err error + + if requireQuorum { + set, err = ring.GetReplicationSetForOperation(Read) + } else { + set, _, err = ring.GetReplicationSetForOperationWithNoQuorum(Read) + } require.Equal(t, testData.expectedErrForRead, err) assert.ElementsMatch(t, testData.expectedSetForRead, set.GetAddresses()) - set, err = ring.GetReplicationSetForOperation(Write) + if requireQuorum { + set, err = ring.GetReplicationSetForOperation(Write) + } else { + set, _, err = ring.GetReplicationSetForOperationWithNoQuorum(Write) + } require.Equal(t, testData.expectedErrForWrite, err) assert.ElementsMatch(t, testData.expectedSetForWrite, set.GetAddresses()) - set, err = ring.GetReplicationSetForOperation(Reporting) + if requireQuorum { + set, err = ring.GetReplicationSetForOperation(Reporting) + } else { + set, _, err = ring.GetReplicationSetForOperationWithNoQuorum(Reporting) + } require.Equal(t, testData.expectedErrForReporting, err) assert.ElementsMatch(t, testData.expectedSetForReporting, set.GetAddresses()) }) } } +func TestRing_GetReplicationSetForOperation(t *testing.T) { + validateGetReplicationSetForOperation(t, true) +} + +func TestRing_GetReplicationSetForOperationWithNoQuorum(t *testing.T) { + validateGetReplicationSetForOperation(t, false) +} + func TestRing_GetReplicationSetForOperation_WithZoneAwarenessEnabled(t *testing.T) { g := NewRandomTokenGenerator() tests := map[string]struct { diff --git a/pkg/ring/util_test.go b/pkg/ring/util_test.go index 0264f54f864..d7fa83ccd62 100644 --- a/pkg/ring/util_test.go +++ b/pkg/ring/util_test.go @@ -41,6 +41,11 @@ func (r *RingMock) GetReplicationSetForOperation(op Operation) (ReplicationSet, return args.Get(0).(ReplicationSet), args.Error(1) } +func (r *RingMock) GetReplicationSetForOperationWithNoQuorum(op Operation) (ReplicationSet, map[string]struct{}, error) { + args := r.Called(op) + return args.Get(0).(ReplicationSet), make(map[string]struct{}), args.Error(1) +} + func (r *RingMock) ReplicationFactor() int { return 0 } diff --git a/pkg/ruler/ruler.go b/pkg/ruler/ruler.go index 0dda94be573..23822082ada 100644 --- a/pkg/ruler/ruler.go +++ b/pkg/ruler/ruler.go @@ -1080,7 +1080,7 @@ func (r *Ruler) getShardedRules(ctx context.Context, userID string, rulesRequest ring = r.ring.ShuffleShard(userID, shardSize) } - rulers, err := ring.GetReplicationSetForOperation(ListRuleRingOp) + rulers, failedZones, err := ring.GetReplicationSetForOperationWithNoQuorum(ListRuleRingOp) if err != nil { return nil, err } @@ -1095,7 +1095,6 @@ func (r *Ruler) getShardedRules(ctx context.Context, userID string, rulesRequest merged []*GroupStateDesc errCount int ) - failedZones := make(map[string]struct{}) zoneByAddress := make(map[string]string) if r.cfg.APIEnableRulesBackup { From 00c188e28a82afc5681e5690b030f7d26e77ae65 Mon Sep 17 00:00:00 2001 From: Emmanuel Lodovice Date: Wed, 10 Apr 2024 01:23:40 -0700 Subject: [PATCH 10/12] Refactor getLocalRules to make the method shorter Signed-off-by: Emmanuel Lodovice --- pkg/ruler/ruler.go | 46 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/pkg/ruler/ruler.go b/pkg/ruler/ruler.go index 23822082ada..010a4aa84be 100644 --- a/pkg/ruler/ruler.go +++ b/pkg/ruler/ruler.go @@ -980,15 +980,41 @@ func (r *Ruler) getLocalRules(userID string, rulesRequest RulesRequest, includeB } backupGroups := r.manager.GetBackupRules(userID) + backupGroupDescs, err := r.ruleGroupListToGroupStateDesc(userID, backupGroups, groupListFilter{ + ruleNameSet, + ruleGroupNameSet, + fileSet, + returnAlerts, + returnRecording, + }) + if err != nil { + return nil, err + } + + return append(groupDescs, backupGroupDescs...), nil +} + +type groupListFilter struct { + ruleNameSet map[string]struct{} + ruleGroupNameSet map[string]struct{} + fileSet map[string]struct{} + returnAlerts bool + returnRecording bool +} + +// ruleGroupListToGroupStateDesc converts rulespb.RuleGroupList to []*GroupStateDesc while accepting filters to control what goes to the +// resulting []*GroupStateDesc +func (r *Ruler) ruleGroupListToGroupStateDesc(userID string, backupGroups rulespb.RuleGroupList, filters groupListFilter) ([]*GroupStateDesc, error) { + groupDescs := make([]*GroupStateDesc, 0, len(backupGroups)) for _, group := range backupGroups { - if len(fileSet) > 0 { - if _, OK := fileSet[group.GetNamespace()]; !OK { + if len(filters.fileSet) > 0 { + if _, OK := filters.fileSet[group.GetNamespace()]; !OK { continue } } - if len(ruleGroupNameSet) > 0 { - if _, OK := ruleGroupNameSet[group.GetName()]; !OK { + if len(filters.ruleGroupNameSet) > 0 { + if _, OK := filters.ruleGroupNameSet[group.GetName()]; !OK { continue } } @@ -1014,12 +1040,11 @@ func (r *Ruler) getLocalRules(userID string, rulesRequest RulesRequest, includeB name = r.GetAlert() isAlertingRule = true } - if len(ruleNameSet) > 0 { - if _, OK := ruleNameSet[name]; !OK { + if len(filters.ruleNameSet) > 0 { + if _, OK := filters.ruleNameSet[name]; !OK { continue } } - lastError := "" var ruleDesc *RuleStateDesc query, err := parser.ParseExpr(r.GetExpr()) @@ -1027,7 +1052,7 @@ func (r *Ruler) getLocalRules(userID string, rulesRequest RulesRequest, includeB return nil, errors.Errorf("failed to parse rule query '%v'", r.GetExpr()) } if isAlertingRule { - if !returnAlerts { + if !filters.returnAlerts { continue } alerts := []*AlertStateDesc{} // backup rules are not evaluated so there will be no active alerts @@ -1042,13 +1067,12 @@ func (r *Ruler) getLocalRules(userID string, rulesRequest RulesRequest, includeB }, State: promRules.StateInactive.String(), // backup rules are not evaluated so they are inactive Health: string(promRules.HealthUnknown), - LastError: lastError, Alerts: alerts, EvaluationTimestamp: time.Time{}, EvaluationDuration: time.Duration(0), } } else { - if !returnRecording { + if !filters.returnRecording { continue } ruleDesc = &RuleStateDesc{ @@ -1058,7 +1082,6 @@ func (r *Ruler) getLocalRules(userID string, rulesRequest RulesRequest, includeB Labels: r.Labels, }, Health: string(promRules.HealthUnknown), - LastError: lastError, EvaluationTimestamp: time.Time{}, EvaluationDuration: time.Duration(0), } @@ -1069,7 +1092,6 @@ func (r *Ruler) getLocalRules(userID string, rulesRequest RulesRequest, includeB groupDescs = append(groupDescs, groupDesc) } } - return groupDescs, nil } From daf1f83d387f84197e51598f8cf17c338c2ce7d0 Mon Sep 17 00:00:00 2001 From: Emmanuel Lodovice Date: Wed, 10 Apr 2024 15:58:55 -0700 Subject: [PATCH 11/12] Add new ring method to get all instances and created a new method in ruler to get Replicaset without requiring quorum Signed-off-by: Emmanuel Lodovice --- docs/configuration/config-file-reference.md | 2 +- .../shuffle_sharding_grouper_test.go | 8 +- pkg/ring/ring.go | 87 ++++--- pkg/ring/ring_test.go | 107 ++++---- pkg/ring/util_test.go | 8 +- pkg/ruler/rule_backup_manager.go | 2 +- pkg/ruler/ruler.go | 4 +- pkg/ruler/ruler_ring.go | 51 ++++ pkg/ruler/ruler_ring_test.go | 237 ++++++++++++++++++ 9 files changed, 384 insertions(+), 122 deletions(-) create mode 100644 pkg/ruler/ruler_ring_test.go diff --git a/docs/configuration/config-file-reference.md b/docs/configuration/config-file-reference.md index 677648471cc..39a19734af1 100644 --- a/docs/configuration/config-file-reference.md +++ b/docs/configuration/config-file-reference.md @@ -4268,7 +4268,7 @@ ring: # with default state (state before any evaluation) and send this copy in list # API requests as backup in case the ruler who owns the rule fails to send its # rules. This allows the rules API to handle ruler outage by returning rules -# with default state. Ring replication-factor needs to be set to 3 or more for +# with default state. Ring replication-factor needs to be set to 2 or more for # this to be useful. # CLI flag: -experimental.ruler.api-enable-rules-backup [api_enable_rules_backup: | default = false] diff --git a/pkg/compactor/shuffle_sharding_grouper_test.go b/pkg/compactor/shuffle_sharding_grouper_test.go index 26d6248cad5..ab19ece87be 100644 --- a/pkg/compactor/shuffle_sharding_grouper_test.go +++ b/pkg/compactor/shuffle_sharding_grouper_test.go @@ -766,14 +766,14 @@ func (r *RingMock) GetInstanceDescsForOperation(op ring.Operation) (map[string]r return args.Get(0).(map[string]ring.InstanceDesc), args.Error(1) } -func (r *RingMock) GetReplicationSetForOperation(op ring.Operation) (ring.ReplicationSet, error) { +func (r *RingMock) GetAllInstanceDescs(op ring.Operation) ([]ring.InstanceDesc, []ring.InstanceDesc, error) { args := r.Called(op) - return args.Get(0).(ring.ReplicationSet), args.Error(1) + return args.Get(0).([]ring.InstanceDesc), make([]ring.InstanceDesc, 0), args.Error(1) } -func (r *RingMock) GetReplicationSetForOperationWithNoQuorum(op ring.Operation) (ring.ReplicationSet, map[string]struct{}, error) { +func (r *RingMock) GetReplicationSetForOperation(op ring.Operation) (ring.ReplicationSet, error) { args := r.Called(op) - return args.Get(0).(ring.ReplicationSet), make(map[string]struct{}), args.Error(1) + return args.Get(0).(ring.ReplicationSet), args.Error(1) } func (r *RingMock) ReplicationFactor() int { diff --git a/pkg/ring/ring.go b/pkg/ring/ring.go index 40b0b43c82a..bc588ba3cfc 100644 --- a/pkg/ring/ring.go +++ b/pkg/ring/ring.go @@ -50,6 +50,9 @@ type ReadRing interface { // of unhealthy instances is greater than the tolerated max unavailable. GetAllHealthy(op Operation) (ReplicationSet, error) + // GetAllInstanceDescs returns a slice of healthy and unhealthy InstanceDesc. + GetAllInstanceDescs(op Operation) ([]InstanceDesc, []InstanceDesc, error) + // GetInstanceDescsForOperation returns map of InstanceDesc with instance ID as the keys. GetInstanceDescsForOperation(op Operation) (map[string]InstanceDesc, error) @@ -59,13 +62,6 @@ type ReadRing interface { // the input operation. GetReplicationSetForOperation(op Operation) (ReplicationSet, error) - // GetReplicationSetForOperationWithNoQuorum returns all instances where the input operation should be executed. - // The resulting ReplicationSet contains all healthy instances in the ring, but the computation for MaxErrors - // does not require quorum so only 1 replica is needed to complete the operation. For MaxUnavailableZones, it is - // not automatically reduced when there are unhealthy instances in a zone because healthy instances in the zone - // are still returned, but the information about zones with unhealthy instances is returned. - GetReplicationSetForOperationWithNoQuorum(op Operation) (ReplicationSet, map[string]struct{}, error) - ReplicationFactor() int // InstancesCount returns the number of instances in the ring. @@ -471,6 +467,28 @@ func (r *Ring) GetAllHealthy(op Operation) (ReplicationSet, error) { }, nil } +// GetAllInstanceDescs implements ReadRing. +func (r *Ring) GetAllInstanceDescs(op Operation) ([]InstanceDesc, []InstanceDesc, error) { + r.mtx.RLock() + defer r.mtx.RUnlock() + + if r.ringDesc == nil || len(r.ringDesc.Ingesters) == 0 { + return nil, nil, ErrEmptyRing + } + healthyInstances := make([]InstanceDesc, 0, len(r.ringDesc.Ingesters)) + unhealthyInstances := make([]InstanceDesc, 0, len(r.ringDesc.Ingesters)) + storageLastUpdate := r.KVClient.LastUpdateTime(r.key) + for _, instance := range r.ringDesc.Ingesters { + if r.IsHealthy(&instance, op, storageLastUpdate) { + healthyInstances = append(healthyInstances, instance) + } else { + unhealthyInstances = append(unhealthyInstances, instance) + } + } + + return healthyInstances, unhealthyInstances, nil +} + // GetInstanceDescsForOperation implements ReadRing. func (r *Ring) GetInstanceDescsForOperation(op Operation) (map[string]InstanceDesc, error) { r.mtx.RLock() @@ -491,12 +509,13 @@ func (r *Ring) GetInstanceDescsForOperation(op Operation) (map[string]InstanceDe return instanceDescs, nil } -func (r *Ring) getReplicationSetForOperation(op Operation, requireQuorum bool) (ReplicationSet, map[string]struct{}, error) { +// GetReplicationSetForOperation implements ReadRing. +func (r *Ring) GetReplicationSetForOperation(op Operation) (ReplicationSet, error) { r.mtx.RLock() defer r.mtx.RUnlock() if r.ringDesc == nil || len(r.ringTokens) == 0 { - return ReplicationSet{}, make(map[string]struct{}), ErrEmptyRing + return ReplicationSet{}, ErrEmptyRing } // Build the initial replication set, excluding unhealthy instances. @@ -518,24 +537,18 @@ func (r *Ring) getReplicationSetForOperation(op Operation, requireQuorum bool) ( maxUnavailableZones := 0 if r.cfg.ZoneAwarenessEnabled { + // Given data is replicated to RF different zones, we can tolerate a number of + // RF/2 failing zones. However, we need to protect from the case the ring currently + // contains instances in a number of zones < RF. numReplicatedZones := utilmath.Min(len(r.ringZones), r.cfg.ReplicationFactor) - if requireQuorum { - // Given data is replicated to RF different zones, we can tolerate a number of - // RF/2 failing zones. However, we need to protect from the case the ring currently - // contains instances in a number of zones < RF. - minSuccessZones := (numReplicatedZones / 2) + 1 - maxUnavailableZones = minSuccessZones - 1 - } else { - // Given that quorum is not required, we only need at least one of the zone to be healthy to succeed. But we - // also need to handle case when RF < number of zones. - maxUnavailableZones = numReplicatedZones - 1 - } + minSuccessZones := (numReplicatedZones / 2) + 1 + maxUnavailableZones = minSuccessZones - 1 if len(zoneFailures) > maxUnavailableZones { - return ReplicationSet{}, zoneFailures, ErrTooManyUnhealthyInstances + return ReplicationSet{}, ErrTooManyUnhealthyInstances } - if requireQuorum && len(zoneFailures) > 0 { + if len(zoneFailures) > 0 { // We remove all instances (even healthy ones) from zones with at least // 1 failing instance. Due to how replication works when zone-awareness is // enabled (data is replicated to RF different zones), there's no benefit in @@ -549,11 +562,11 @@ func (r *Ring) getReplicationSetForOperation(op Operation, requireQuorum bool) ( } healthyInstances = filteredInstances - - // Since we removed all instances from zones containing at least 1 failing - // instance, we have to decrease the max unavailable zones accordingly. - maxUnavailableZones -= len(zoneFailures) } + + // Since we removed all instances from zones containing at least 1 failing + // instance, we have to decrease the max unavailable zones accordingly. + maxUnavailableZones -= len(zoneFailures) } else { // Calculate the number of required instances; // ensure we always require at least RF-1 when RF=3. @@ -562,15 +575,10 @@ func (r *Ring) getReplicationSetForOperation(op Operation, requireQuorum bool) ( numRequired = r.cfg.ReplicationFactor } // We can tolerate this many failures - if requireQuorum { - numRequired -= r.cfg.ReplicationFactor / 2 - } else { - // if quorum is not required then 1 replica is enough to handle the request - numRequired -= r.cfg.ReplicationFactor - 1 - } + numRequired -= r.cfg.ReplicationFactor / 2 if len(healthyInstances) < numRequired { - return ReplicationSet{}, zoneFailures, ErrTooManyUnhealthyInstances + return ReplicationSet{}, ErrTooManyUnhealthyInstances } maxErrors = len(healthyInstances) - numRequired @@ -580,18 +588,7 @@ func (r *Ring) getReplicationSetForOperation(op Operation, requireQuorum bool) ( Instances: healthyInstances, MaxErrors: maxErrors, MaxUnavailableZones: maxUnavailableZones, - }, zoneFailures, nil -} - -// GetReplicationSetForOperation implements ReadRing. -func (r *Ring) GetReplicationSetForOperation(op Operation) (ReplicationSet, error) { - replicationSet, _, err := r.getReplicationSetForOperation(op, true) - return replicationSet, err -} - -// GetReplicationSetForOperationWithNoQuorum implements ReadRing. -func (r *Ring) GetReplicationSetForOperationWithNoQuorum(op Operation) (ReplicationSet, map[string]struct{}, error) { - return r.getReplicationSetForOperation(op, false) + }, nil } // countTokens returns the number of tokens and tokens within the range for each instance. diff --git a/pkg/ring/ring_test.go b/pkg/ring/ring_test.go index 38f8aa7054b..a4bdaf130b6 100644 --- a/pkg/ring/ring_test.go +++ b/pkg/ring/ring_test.go @@ -959,11 +959,45 @@ func TestRing_GetInstanceDescsForOperation(t *testing.T) { }, instanceDescs) } -func validateGetReplicationSetForOperation(t *testing.T, requireQuorum bool) { +func TestRing_GetAllInstanceDescs(t *testing.T) { + now := time.Now().Unix() + twoMinutesAgo := time.Now().Add(-2 * time.Minute).Unix() + + ringDesc := &Desc{Ingesters: map[string]InstanceDesc{ + "instance-1": {Addr: "127.0.0.1", Tokens: []uint32{1}, State: ACTIVE, Timestamp: now}, + "instance-2": {Addr: "127.0.0.2", Tokens: []uint32{2}, State: LEAVING, Timestamp: now}, // not healthy state + "instance-3": {Addr: "127.0.0.3", Tokens: []uint32{3}, State: ACTIVE, Timestamp: twoMinutesAgo}, // heartbeat timed out + }} + + ring := Ring{ + cfg: Config{HeartbeatTimeout: time.Minute}, + ringDesc: ringDesc, + ringTokens: ringDesc.GetTokens(), + ringTokensByZone: ringDesc.getTokensByZone(), + ringInstanceByToken: ringDesc.getTokensInfo(), + ringZones: getZones(ringDesc.getTokensByZone()), + strategy: NewDefaultReplicationStrategy(), + KVClient: &MockClient{}, + } + + testOp := NewOp([]InstanceState{ACTIVE}, nil) + + healthyInstanceDescs, unhealthyInstanceDescs, err := ring.GetAllInstanceDescs(testOp) + require.NoError(t, err) + require.EqualValues(t, []InstanceDesc{ + {Addr: "127.0.0.1", Tokens: []uint32{1}, State: ACTIVE, Timestamp: now}, + }, healthyInstanceDescs) + require.EqualValues(t, []InstanceDesc{ + {Addr: "127.0.0.2", Tokens: []uint32{2}, State: LEAVING, Timestamp: now}, + {Addr: "127.0.0.3", Tokens: []uint32{3}, State: ACTIVE, Timestamp: twoMinutesAgo}, + }, unhealthyInstanceDescs) +} + +func TestRing_GetReplicationSetForOperation(t *testing.T) { now := time.Now() g := NewRandomTokenGenerator() - type testCase struct { + tests := map[string]struct { ringInstances map[string]InstanceDesc ringHeartbeatTimeout time.Duration ringReplicationFactor int @@ -973,9 +1007,7 @@ func validateGetReplicationSetForOperation(t *testing.T, requireQuorum bool) { expectedSetForWrite []string expectedErrForReporting error expectedSetForReporting []string - } - - tests := map[string]testCase{ + }{ "should return error on empty ring": { ringInstances: nil, ringHeartbeatTimeout: time.Minute, @@ -1040,10 +1072,7 @@ func validateGetReplicationSetForOperation(t *testing.T, requireQuorum bool) { expectedSetForWrite: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3", "127.0.0.4"}, expectedSetForReporting: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3", "127.0.0.4"}, }, - } - - if requireQuorum { - tests["should fail on 2 unhealthy instances and RF=3"] = testCase{ + "should fail on 2 unhealthy instances and RF=3": { ringInstances: map[string]InstanceDesc{ "instance-1": {Addr: "127.0.0.1", State: ACTIVE, Timestamp: now.Unix(), Tokens: g.GenerateTokens(NewDesc(), "instance-1", "", 128, true)}, "instance-2": {Addr: "127.0.0.2", State: ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: g.GenerateTokens(NewDesc(), "instance-2", "", 128, true)}, @@ -1056,36 +1085,7 @@ func validateGetReplicationSetForOperation(t *testing.T, requireQuorum bool) { expectedErrForRead: ErrTooManyUnhealthyInstances, expectedErrForWrite: ErrTooManyUnhealthyInstances, expectedErrForReporting: ErrTooManyUnhealthyInstances, - } - } else { - tests["should pass on 2 unhealthy instances and RF=3"] = testCase{ - ringInstances: map[string]InstanceDesc{ - "instance-1": {Addr: "127.0.0.1", State: ACTIVE, Timestamp: now.Unix(), Tokens: GenerateTokens(128, nil)}, - "instance-2": {Addr: "127.0.0.2", State: ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: GenerateTokens(128, nil)}, - "instance-3": {Addr: "127.0.0.3", State: ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: GenerateTokens(128, nil)}, - "instance-4": {Addr: "127.0.0.4", State: ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: GenerateTokens(128, nil)}, - "instance-5": {Addr: "127.0.0.5", State: ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: GenerateTokens(128, nil)}, - }, - ringHeartbeatTimeout: time.Minute, - ringReplicationFactor: 3, - expectedSetForRead: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3"}, - expectedSetForWrite: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3"}, - expectedSetForReporting: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3"}, - } - tests["should fail on 3 unhealthy instances and RF=3"] = testCase{ - ringInstances: map[string]InstanceDesc{ - "instance-1": {Addr: "127.0.0.1", State: ACTIVE, Timestamp: now.Unix(), Tokens: GenerateTokens(128, nil)}, - "instance-2": {Addr: "127.0.0.2", State: ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: GenerateTokens(128, nil)}, - "instance-3": {Addr: "127.0.0.3", State: ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: GenerateTokens(128, nil)}, - "instance-4": {Addr: "127.0.0.4", State: ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: GenerateTokens(128, nil)}, - "instance-5": {Addr: "127.0.0.5", State: ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: GenerateTokens(128, nil)}, - }, - ringHeartbeatTimeout: time.Minute, - ringReplicationFactor: 3, - expectedErrForRead: ErrTooManyUnhealthyInstances, - expectedErrForWrite: ErrTooManyUnhealthyInstances, - expectedErrForReporting: ErrTooManyUnhealthyInstances, - } + }, } for testName, testData := range tests { @@ -1110,44 +1110,21 @@ func validateGetReplicationSetForOperation(t *testing.T, requireQuorum bool) { KVClient: &MockClient{}, } - var set ReplicationSet - var err error - - if requireQuorum { - set, err = ring.GetReplicationSetForOperation(Read) - } else { - set, _, err = ring.GetReplicationSetForOperationWithNoQuorum(Read) - } + set, err := ring.GetReplicationSetForOperation(Read) require.Equal(t, testData.expectedErrForRead, err) assert.ElementsMatch(t, testData.expectedSetForRead, set.GetAddresses()) - if requireQuorum { - set, err = ring.GetReplicationSetForOperation(Write) - } else { - set, _, err = ring.GetReplicationSetForOperationWithNoQuorum(Write) - } + set, err = ring.GetReplicationSetForOperation(Write) require.Equal(t, testData.expectedErrForWrite, err) assert.ElementsMatch(t, testData.expectedSetForWrite, set.GetAddresses()) - if requireQuorum { - set, err = ring.GetReplicationSetForOperation(Reporting) - } else { - set, _, err = ring.GetReplicationSetForOperationWithNoQuorum(Reporting) - } + set, err = ring.GetReplicationSetForOperation(Reporting) require.Equal(t, testData.expectedErrForReporting, err) assert.ElementsMatch(t, testData.expectedSetForReporting, set.GetAddresses()) }) } } -func TestRing_GetReplicationSetForOperation(t *testing.T) { - validateGetReplicationSetForOperation(t, true) -} - -func TestRing_GetReplicationSetForOperationWithNoQuorum(t *testing.T) { - validateGetReplicationSetForOperation(t, false) -} - func TestRing_GetReplicationSetForOperation_WithZoneAwarenessEnabled(t *testing.T) { g := NewRandomTokenGenerator() tests := map[string]struct { diff --git a/pkg/ring/util_test.go b/pkg/ring/util_test.go index d7fa83ccd62..29fac1f2bf3 100644 --- a/pkg/ring/util_test.go +++ b/pkg/ring/util_test.go @@ -36,14 +36,14 @@ func (r *RingMock) GetInstanceDescsForOperation(op Operation) (map[string]Instan return args.Get(0).(map[string]InstanceDesc), args.Error(1) } -func (r *RingMock) GetReplicationSetForOperation(op Operation) (ReplicationSet, error) { +func (r *RingMock) GetAllInstanceDescs(op Operation) ([]InstanceDesc, []InstanceDesc, error) { args := r.Called(op) - return args.Get(0).(ReplicationSet), args.Error(1) + return args.Get(0).([]InstanceDesc), make([]InstanceDesc, 0), args.Error(1) } -func (r *RingMock) GetReplicationSetForOperationWithNoQuorum(op Operation) (ReplicationSet, map[string]struct{}, error) { +func (r *RingMock) GetReplicationSetForOperation(op Operation) (ReplicationSet, error) { args := r.Called(op) - return args.Get(0).(ReplicationSet), make(map[string]struct{}), args.Error(1) + return args.Get(0).(ReplicationSet), args.Error(1) } func (r *RingMock) ReplicationFactor() int { diff --git a/pkg/ruler/rule_backup_manager.go b/pkg/ruler/rule_backup_manager.go index 4ce791aea2a..ce4cef2c9b4 100644 --- a/pkg/ruler/rule_backup_manager.go +++ b/pkg/ruler/rule_backup_manager.go @@ -53,7 +53,7 @@ func (r *rulesBackupManager) getRuleGroups(userID string) rulespb.RuleGroupList return result } -// getRuleGroups updates the ruler_backup_rule_group metric by adding new groups that were backed up and removing +// updateMetrics updates the ruler_backup_rule_group metric by adding new groups that were backed up and removing // those that are removed from the backup. func (r *rulesBackupManager) updateMetrics(newBackupGroups map[string]rulespb.RuleGroupList) { for user, groups := range newBackupGroups { diff --git a/pkg/ruler/ruler.go b/pkg/ruler/ruler.go index 010a4aa84be..23de3ab908d 100644 --- a/pkg/ruler/ruler.go +++ b/pkg/ruler/ruler.go @@ -198,7 +198,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { f.DurationVar(&cfg.FlushCheckPeriod, "ruler.flush-period", 1*time.Minute, "Period with which to attempt to flush rule groups.") f.StringVar(&cfg.RulePath, "ruler.rule-path", "/rules", "file path to store temporary rule files for the prometheus rule managers") f.BoolVar(&cfg.EnableAPI, "experimental.ruler.enable-api", false, "Enable the ruler api") - f.BoolVar(&cfg.APIEnableRulesBackup, "experimental.ruler.api-enable-rules-backup", false, "EXPERIMENTAL: Enable rulers to store a copy of rules owned by other rulers with default state (state before any evaluation) and send this copy in list API requests as backup in case the ruler who owns the rule fails to send its rules. This allows the rules API to handle ruler outage by returning rules with default state. Ring replication-factor needs to be set to 3 or more for this to be useful.") + f.BoolVar(&cfg.APIEnableRulesBackup, "experimental.ruler.api-enable-rules-backup", false, "EXPERIMENTAL: Enable rulers to store a copy of rules owned by other rulers with default state (state before any evaluation) and send this copy in list API requests as backup in case the ruler who owns the rule fails to send its rules. This allows the rules API to handle ruler outage by returning rules with default state. Ring replication-factor needs to be set to 2 or more for this to be useful.") f.BoolVar(&cfg.APIDeduplicateRules, "experimental.ruler.api-deduplicate-rules", false, "EXPERIMENTAL: Remove duplicate rules in the prometheus rules and alerts API response. If there are duplicate rules the rule with the latest evaluation timestamp will be kept.") f.DurationVar(&cfg.OutageTolerance, "ruler.for-outage-tolerance", time.Hour, `Max time to tolerate outage for restoring "for" state of alert.`) f.DurationVar(&cfg.ForGracePeriod, "ruler.for-grace-period", 10*time.Minute, `Minimum duration between alert and restored "for" state. This is maintained only for alerts with configured "for" time greater than grace period.`) @@ -1102,7 +1102,7 @@ func (r *Ruler) getShardedRules(ctx context.Context, userID string, rulesRequest ring = r.ring.ShuffleShard(userID, shardSize) } - rulers, failedZones, err := ring.GetReplicationSetForOperationWithNoQuorum(ListRuleRingOp) + rulers, failedZones, err := GetReplicationSetForListRule(ring, &r.cfg.Ring) if err != nil { return nil, err } diff --git a/pkg/ruler/ruler_ring.go b/pkg/ruler/ruler_ring.go index 4dea1381a0e..7ab8f4c0306 100644 --- a/pkg/ruler/ruler_ring.go +++ b/pkg/ruler/ruler_ring.go @@ -11,6 +11,7 @@ import ( "github.com/cortexproject/cortex/pkg/ring" "github.com/cortexproject/cortex/pkg/ring/kv" "github.com/cortexproject/cortex/pkg/util/flagext" + utilmath "github.com/cortexproject/cortex/pkg/util/math" ) const ( @@ -121,3 +122,53 @@ func (cfg *RingConfig) ToRingConfig() ring.Config { return rc } + +// GetReplicationSetForListRule is similar to ring.GetReplicationSetForOperation but does NOT require quorum. Because +// it does not require quorum it returns healthy instance in the AZ with failed instances unlike +// GetReplicationSetForOperation. This is important for ruler because healthy instances in the AZ with failed +// instance could be evaluating some rule groups. +func GetReplicationSetForListRule(r ring.ReadRing, cfg *RingConfig) (ring.ReplicationSet, map[string]struct{}, error) { + healthy, unhealthy, err := r.GetAllInstanceDescs(ListRuleRingOp) + if err != nil { + return ring.ReplicationSet{}, make(map[string]struct{}), err + } + ringZones := make(map[string]struct{}) + zoneFailures := make(map[string]struct{}) + for _, instance := range healthy { + ringZones[instance.Zone] = struct{}{} + } + for _, instance := range unhealthy { + ringZones[instance.Zone] = struct{}{} + zoneFailures[instance.Zone] = struct{}{} + } + // Max errors and max unavailable zones are mutually exclusive. We initialise both + // to 0, and then we update them whether zone-awareness is enabled or not. + maxErrors := 0 + maxUnavailableZones := 0 + if cfg.ZoneAwarenessEnabled { + numReplicatedZones := utilmath.Min(len(ringZones), r.ReplicationFactor()) + // Given that quorum is not required, we only need at least one of the zone to be healthy to succeed. But we + // also need to handle case when RF < number of zones. + maxUnavailableZones = numReplicatedZones - 1 + if len(zoneFailures) > maxUnavailableZones { + return ring.ReplicationSet{}, zoneFailures, ring.ErrTooManyUnhealthyInstances + } + } else { + numRequired := len(healthy) + len(unhealthy) + if numRequired < r.ReplicationFactor() { + numRequired = r.ReplicationFactor() + } + // quorum is not required so 1 replica is enough to handle the request + numRequired -= r.ReplicationFactor() - 1 + if len(healthy) < numRequired { + return ring.ReplicationSet{}, zoneFailures, ring.ErrTooManyUnhealthyInstances + } + + maxErrors = len(healthy) - numRequired + } + return ring.ReplicationSet{ + Instances: healthy, + MaxErrors: maxErrors, + MaxUnavailableZones: maxUnavailableZones, + }, zoneFailures, nil +} diff --git a/pkg/ruler/ruler_ring_test.go b/pkg/ruler/ruler_ring_test.go new file mode 100644 index 00000000000..d9190beb6e5 --- /dev/null +++ b/pkg/ruler/ruler_ring_test.go @@ -0,0 +1,237 @@ +package ruler + +import ( + "context" + "testing" + "time" + + "github.com/go-kit/log" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/cortexproject/cortex/pkg/ring" + "github.com/cortexproject/cortex/pkg/ring/kv" + "github.com/cortexproject/cortex/pkg/ring/kv/consul" + "github.com/cortexproject/cortex/pkg/util" + "github.com/cortexproject/cortex/pkg/util/services" +) + +func TestGetReplicationSetForListRule(t *testing.T) { + now := time.Now() + + tests := map[string]struct { + ringInstances map[string]ring.InstanceDesc + ringHeartbeatTimeout time.Duration + ringReplicationFactor int + enableAZReplication bool + expectedErr error + expectedSet []string + expectedMaxError int + expectedMaxUnavailableZones int + expectedFailedZones map[string]struct{} + }{ + "should return error on empty ring": { + ringInstances: nil, + ringHeartbeatTimeout: time.Minute, + ringReplicationFactor: 1, + expectedErr: ring.ErrEmptyRing, + }, + "should succeed on all healthy instances and RF=1": { + ringInstances: map[string]ring.InstanceDesc{ + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + }, + ringHeartbeatTimeout: time.Minute, + ringReplicationFactor: 1, + expectedSet: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3"}, + }, + "should fail on 1 unhealthy instance and RF=1": { + ringInstances: map[string]ring.InstanceDesc{ + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + }, + ringHeartbeatTimeout: time.Minute, + ringReplicationFactor: 1, + expectedErr: ring.ErrTooManyUnhealthyInstances, + }, + "should succeed on 1 unhealthy instances and RF=3": { + ringInstances: map[string]ring.InstanceDesc{ + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + }, + ringHeartbeatTimeout: time.Minute, + ringReplicationFactor: 3, + expectedSet: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3"}, + expectedMaxError: 1, + }, + "should succeed on 2 unhealthy instances and RF=3": { + ringInstances: map[string]ring.InstanceDesc{ + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-5": {Addr: "127.0.0.5", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + }, + ringHeartbeatTimeout: time.Minute, + ringReplicationFactor: 3, + expectedSet: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3"}, + expectedMaxError: 0, + }, + "should fail on 3 unhealthy instances and RF=3": { + ringInstances: map[string]ring.InstanceDesc{ + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-5": {Addr: "127.0.0.5", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + }, + ringHeartbeatTimeout: time.Minute, + ringReplicationFactor: 3, + expectedErr: ring.ErrTooManyUnhealthyInstances, + }, + "should succeed on 0 unhealthy instances and RF=3 zone replication enabled": { + ringInstances: map[string]ring.InstanceDesc{ + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z3"}, + "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-30 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, + }, + ringHeartbeatTimeout: time.Minute, + ringReplicationFactor: 3, + expectedSet: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3", "127.0.0.4"}, + enableAZReplication: true, + expectedFailedZones: make(map[string]struct{}), + expectedMaxUnavailableZones: 2, + }, + "should succeed on 3 unhealthy instances in 2 zones and RF=3 zone replication enabled": { + ringInstances: map[string]ring.InstanceDesc{ + "instance-1": {Addr: "127.0.0.1", State: ring.PENDING, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z3"}, + "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, + "instance-5": {Addr: "127.0.0.5", State: ring.PENDING, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, + }, + ringHeartbeatTimeout: time.Minute, + ringReplicationFactor: 3, + expectedSet: []string{"127.0.0.2", "127.0.0.3"}, + enableAZReplication: true, + expectedFailedZones: map[string]struct{}{ + "z1": {}, + "z2": {}, + }, + expectedMaxUnavailableZones: 2, + }, + "should succeed on 1 unhealthy instances in 1 zone and RF=3 zone replication enabled": { + ringInstances: map[string]ring.InstanceDesc{ + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z3"}, + "instance-4": {Addr: "127.0.0.4", State: ring.PENDING, Timestamp: now.Add(-30 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, + }, + ringHeartbeatTimeout: time.Minute, + ringReplicationFactor: 3, + expectedSet: []string{"127.0.0.1", "127.0.0.2", "127.0.0.3"}, + enableAZReplication: true, + expectedFailedZones: map[string]struct{}{ + "z1": {}, + }, + expectedMaxUnavailableZones: 2, + }, + "should fail on 3 unhealthy instances in 3 zonez and RF=3 zone replication enabled": { + ringInstances: map[string]ring.InstanceDesc{ + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z3"}, + "instance-4": {Addr: "127.0.0.4", State: ring.PENDING, Timestamp: now.Add(-30 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, + "instance-5": {Addr: "127.0.0.5", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, + "instance-6": {Addr: "127.0.0.6", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z3"}, + }, + ringHeartbeatTimeout: time.Minute, + ringReplicationFactor: 3, + enableAZReplication: true, + expectedErr: ring.ErrTooManyUnhealthyInstances, + expectedFailedZones: map[string]struct{}{ + "z1": {}, + "z2": {}, + "z3": {}, + }, + }, + "should fail on 2 unhealthy instances in 2 zones and RF=2 zone replication enabled": { + ringInstances: map[string]ring.InstanceDesc{ + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z3"}, + "instance-4": {Addr: "127.0.0.4", State: ring.PENDING, Timestamp: now.Add(-30 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, + "instance-5": {Addr: "127.0.0.5", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, + }, + ringHeartbeatTimeout: time.Minute, + ringReplicationFactor: 2, + enableAZReplication: true, + expectedErr: ring.ErrTooManyUnhealthyInstances, + expectedFailedZones: map[string]struct{}{ + "z1": {}, + "z2": {}, + }, + }, + } + + for testName, testData := range tests { + t.Run(testName, func(t *testing.T) { + kvStore, closer := consul.NewInMemoryClient(ring.GetCodec(), log.NewNopLogger(), nil) + t.Cleanup(func() { assert.NoError(t, closer.Close()) }) + + store := newMockRuleStore(nil, nil) + cfg := Config{ + EnableSharding: true, + ShardingStrategy: util.ShardingStrategyDefault, + Ring: RingConfig{ + InstanceID: "instance-1", + InstanceAddr: "127.0.0.1", + InstancePort: 9999, + KVStore: kv.Config{ + Mock: kvStore, + }, + HeartbeatTimeout: 1 * time.Minute, + ReplicationFactor: testData.ringReplicationFactor, + ZoneAwarenessEnabled: testData.enableAZReplication, + }, + FlushCheckPeriod: 0, + } + + r, _ := buildRuler(t, cfg, nil, store, nil) + r.limits = ruleLimits{evalDelay: 0} + + rulerRing := r.ring + // We start ruler's ring, but nothing else (not even lifecycler). + require.NoError(t, services.StartAndAwaitRunning(context.Background(), rulerRing)) + t.Cleanup(rulerRing.StopAsync) + + err := kvStore.CAS(context.Background(), ringKey, func(in interface{}) (out interface{}, retry bool, err error) { + d, _ := in.(*ring.Desc) + if d == nil { + d = ring.NewDesc() + } + d.Ingesters = testData.ringInstances + return d, true, nil + }) + require.NoError(t, err) + // Wait a bit to make sure ruler's ring is updated. + time.Sleep(100 * time.Millisecond) + + set, failedZones, err := GetReplicationSetForListRule(rulerRing, &cfg.Ring) + require.Equal(t, testData.expectedErr, err) + assert.ElementsMatch(t, testData.expectedSet, set.GetAddresses()) + require.Equal(t, testData.expectedMaxError, set.MaxErrors) + require.Equal(t, testData.expectedMaxUnavailableZones, set.MaxUnavailableZones) + if testData.enableAZReplication { + require.Equal(t, len(testData.expectedFailedZones), len(failedZones)) + require.EqualValues(t, testData.expectedFailedZones, failedZones) + } + }) + } +} From 2681f23bc6a3204f8e24aab15d65d8559062af1d Mon Sep 17 00:00:00 2001 From: Emmanuel Lodovice Date: Thu, 11 Apr 2024 17:49:39 -0700 Subject: [PATCH 12/12] Fix flaky test due to sorting issue Signed-off-by: Emmanuel Lodovice --- pkg/ring/ring_test.go | 1 + pkg/ruler/ruler_ring_test.go | 92 ++++++++++++++++++------------------ 2 files changed, 48 insertions(+), 45 deletions(-) diff --git a/pkg/ring/ring_test.go b/pkg/ring/ring_test.go index a4bdaf130b6..82bb096d020 100644 --- a/pkg/ring/ring_test.go +++ b/pkg/ring/ring_test.go @@ -987,6 +987,7 @@ func TestRing_GetAllInstanceDescs(t *testing.T) { require.EqualValues(t, []InstanceDesc{ {Addr: "127.0.0.1", Tokens: []uint32{1}, State: ACTIVE, Timestamp: now}, }, healthyInstanceDescs) + sort.Slice(unhealthyInstanceDescs, func(i, j int) bool { return unhealthyInstanceDescs[i].Addr < unhealthyInstanceDescs[j].Addr }) require.EqualValues(t, []InstanceDesc{ {Addr: "127.0.0.2", Tokens: []uint32{2}, State: LEAVING, Timestamp: now}, {Addr: "127.0.0.3", Tokens: []uint32{3}, State: ACTIVE, Timestamp: twoMinutesAgo}, diff --git a/pkg/ruler/ruler_ring_test.go b/pkg/ruler/ruler_ring_test.go index d9190beb6e5..f9bfaeb03eb 100644 --- a/pkg/ruler/ruler_ring_test.go +++ b/pkg/ruler/ruler_ring_test.go @@ -19,6 +19,8 @@ import ( func TestGetReplicationSetForListRule(t *testing.T) { now := time.Now() + g := ring.NewRandomTokenGenerator() + tests := map[string]struct { ringInstances map[string]ring.InstanceDesc ringHeartbeatTimeout time.Duration @@ -38,9 +40,9 @@ func TestGetReplicationSetForListRule(t *testing.T) { }, "should succeed on all healthy instances and RF=1": { ringInstances: map[string]ring.InstanceDesc{ - "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-1", "", 128, true)}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-2", "", 128, true)}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-3", "", 128, true)}, }, ringHeartbeatTimeout: time.Minute, ringReplicationFactor: 1, @@ -48,10 +50,10 @@ func TestGetReplicationSetForListRule(t *testing.T) { }, "should fail on 1 unhealthy instance and RF=1": { ringInstances: map[string]ring.InstanceDesc{ - "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-1", "", 128, true)}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-2", "", 128, true)}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-3", "", 128, true)}, + "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-4", "", 128, true)}, }, ringHeartbeatTimeout: time.Minute, ringReplicationFactor: 1, @@ -59,10 +61,10 @@ func TestGetReplicationSetForListRule(t *testing.T) { }, "should succeed on 1 unhealthy instances and RF=3": { ringInstances: map[string]ring.InstanceDesc{ - "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-1", "", 128, true)}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-2", "", 128, true)}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-3", "", 128, true)}, + "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-4", "", 128, true)}, }, ringHeartbeatTimeout: time.Minute, ringReplicationFactor: 3, @@ -71,11 +73,11 @@ func TestGetReplicationSetForListRule(t *testing.T) { }, "should succeed on 2 unhealthy instances and RF=3": { ringInstances: map[string]ring.InstanceDesc{ - "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-5": {Addr: "127.0.0.5", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-1", "", 128, true)}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-2", "", 128, true)}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-3", "", 128, true)}, + "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-4", "", 128, true)}, + "instance-5": {Addr: "127.0.0.5", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-5", "", 128, true)}, }, ringHeartbeatTimeout: time.Minute, ringReplicationFactor: 3, @@ -84,11 +86,11 @@ func TestGetReplicationSetForListRule(t *testing.T) { }, "should fail on 3 unhealthy instances and RF=3": { ringInstances: map[string]ring.InstanceDesc{ - "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil)}, - "instance-5": {Addr: "127.0.0.5", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil)}, + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-1", "", 128, true)}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-2", "", 128, true)}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-3", "", 128, true)}, + "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-4", "", 128, true)}, + "instance-5": {Addr: "127.0.0.5", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-5", "", 128, true)}, }, ringHeartbeatTimeout: time.Minute, ringReplicationFactor: 3, @@ -96,10 +98,10 @@ func TestGetReplicationSetForListRule(t *testing.T) { }, "should succeed on 0 unhealthy instances and RF=3 zone replication enabled": { ringInstances: map[string]ring.InstanceDesc{ - "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, - "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, - "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z3"}, - "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-30 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-1", "z1", 128, true), Zone: "z1"}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-2", "z2", 128, true), Zone: "z2"}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-3", "z3", 128, true), Zone: "z3"}, + "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-30 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-4", "z1", 128, true), Zone: "z1"}, }, ringHeartbeatTimeout: time.Minute, ringReplicationFactor: 3, @@ -110,11 +112,11 @@ func TestGetReplicationSetForListRule(t *testing.T) { }, "should succeed on 3 unhealthy instances in 2 zones and RF=3 zone replication enabled": { ringInstances: map[string]ring.InstanceDesc{ - "instance-1": {Addr: "127.0.0.1", State: ring.PENDING, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, - "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, - "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z3"}, - "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, - "instance-5": {Addr: "127.0.0.5", State: ring.PENDING, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, + "instance-1": {Addr: "127.0.0.1", State: ring.PENDING, Timestamp: now.Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-1", "z1", 128, true), Zone: "z1"}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-2", "z2", 128, true), Zone: "z2"}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-3", "z3", 128, true), Zone: "z3"}, + "instance-4": {Addr: "127.0.0.4", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-4", "z1", 128, true), Zone: "z1"}, + "instance-5": {Addr: "127.0.0.5", State: ring.PENDING, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-5", "z2", 128, true), Zone: "z2"}, }, ringHeartbeatTimeout: time.Minute, ringReplicationFactor: 3, @@ -128,10 +130,10 @@ func TestGetReplicationSetForListRule(t *testing.T) { }, "should succeed on 1 unhealthy instances in 1 zone and RF=3 zone replication enabled": { ringInstances: map[string]ring.InstanceDesc{ - "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, - "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, - "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z3"}, - "instance-4": {Addr: "127.0.0.4", State: ring.PENDING, Timestamp: now.Add(-30 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-1", "z1", 128, true), Zone: "z1"}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-2", "z2", 128, true), Zone: "z2"}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-3", "z3", 128, true), Zone: "z3"}, + "instance-4": {Addr: "127.0.0.4", State: ring.PENDING, Timestamp: now.Add(-30 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-4", "z1", 128, true), Zone: "z1"}, }, ringHeartbeatTimeout: time.Minute, ringReplicationFactor: 3, @@ -144,12 +146,12 @@ func TestGetReplicationSetForListRule(t *testing.T) { }, "should fail on 3 unhealthy instances in 3 zonez and RF=3 zone replication enabled": { ringInstances: map[string]ring.InstanceDesc{ - "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, - "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, - "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z3"}, - "instance-4": {Addr: "127.0.0.4", State: ring.PENDING, Timestamp: now.Add(-30 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, - "instance-5": {Addr: "127.0.0.5", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, - "instance-6": {Addr: "127.0.0.6", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z3"}, + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-1", "z1", 128, true), Zone: "z1"}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-2", "z2", 128, true), Zone: "z2"}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-3", "z3", 128, true), Zone: "z3"}, + "instance-4": {Addr: "127.0.0.4", State: ring.PENDING, Timestamp: now.Add(-30 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-4", "z1", 128, true), Zone: "z1"}, + "instance-5": {Addr: "127.0.0.5", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-5", "z2", 128, true), Zone: "z2"}, + "instance-6": {Addr: "127.0.0.6", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-6", "z3", 128, true), Zone: "z3"}, }, ringHeartbeatTimeout: time.Minute, ringReplicationFactor: 3, @@ -163,11 +165,11 @@ func TestGetReplicationSetForListRule(t *testing.T) { }, "should fail on 2 unhealthy instances in 2 zones and RF=2 zone replication enabled": { ringInstances: map[string]ring.InstanceDesc{ - "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, - "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, - "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z3"}, - "instance-4": {Addr: "127.0.0.4", State: ring.PENDING, Timestamp: now.Add(-30 * time.Second).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z1"}, - "instance-5": {Addr: "127.0.0.5", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: ring.GenerateTokens(128, nil), Zone: "z2"}, + "instance-1": {Addr: "127.0.0.1", State: ring.ACTIVE, Timestamp: now.Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-1", "z1", 128, true), Zone: "z1"}, + "instance-2": {Addr: "127.0.0.2", State: ring.ACTIVE, Timestamp: now.Add(-10 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-2", "z2", 128, true), Zone: "z2"}, + "instance-3": {Addr: "127.0.0.3", State: ring.ACTIVE, Timestamp: now.Add(-20 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-3", "z3", 128, true), Zone: "z3"}, + "instance-4": {Addr: "127.0.0.4", State: ring.PENDING, Timestamp: now.Add(-30 * time.Second).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-4", "z1", 128, true), Zone: "z1"}, + "instance-5": {Addr: "127.0.0.5", State: ring.ACTIVE, Timestamp: now.Add(-2 * time.Minute).Unix(), Tokens: g.GenerateTokens(ring.NewDesc(), "instance-5", "z2", 128, true), Zone: "z2"}, }, ringHeartbeatTimeout: time.Minute, ringReplicationFactor: 2,