From cd5a7b50209910954ed4dbe7d6fa0836511a6332 Mon Sep 17 00:00:00 2001 From: Raphael Silva Date: Fri, 28 Jun 2024 22:50:54 +0000 Subject: [PATCH 1/2] Make rules Manager Update method no-op after Close This has to be done because Close and Update methods are accessed concurrently. Signed-off-by: Raphael Silva --- rules/manager.go | 8 ++++++++ rules/manager_test.go | 17 +++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/rules/manager.go b/rules/manager.go index 063189e0a..92675e71d 100644 --- a/rules/manager.go +++ b/rules/manager.go @@ -190,10 +190,18 @@ func (m *Manager) Stop() { // Update the rule manager's state as the config requires. If // loading the new rules failed the old rule set is restored. +// This method will no-op in case the manager is already stopped func (m *Manager) Update(interval time.Duration, files []string, externalLabels labels.Labels, externalURL string, groupEvalIterationFunc GroupEvalIterationFunc) error { m.mtx.Lock() defer m.mtx.Unlock() + // We cannot update a stopped manager + select { + case <-m.done: + return nil + default: + } + groups, errs := m.LoadGroups(interval, externalLabels, externalURL, groupEvalIterationFunc, files...) if errs != nil { diff --git a/rules/manager_test.go b/rules/manager_test.go index 3bf5fac32..ec967df24 100644 --- a/rules/manager_test.go +++ b/rules/manager_test.go @@ -2099,6 +2099,23 @@ func TestBoundedRuleEvalConcurrency(t *testing.T) { require.EqualValues(t, maxInflight.Load(), int32(maxConcurrency)+int32(groupCount)) } +func TestUpdateWhenStopped(t *testing.T) { + files := []string{"fixtures/rules.yaml"} + ruleManager := NewManager(&ManagerOptions{ + Context: context.Background(), + Logger: log.NewNopLogger(), + }) + ruleManager.start() + err := ruleManager.Update(10*time.Second, files, labels.EmptyLabels(), "", nil) + require.NoError(t, err) + require.NotEmpty(t, ruleManager.groups) + + ruleManager.Stop() + // Updates following a stop are no-op + err = ruleManager.Update(10*time.Second, []string{}, labels.EmptyLabels(), "", nil) + require.NoError(t, err) +} + const artificialDelay = 250 * time.Millisecond func optsFactory(storage storage.Storage, maxInflight, inflightQueries *atomic.Int32, maxConcurrent int64) *ManagerOptions { From e0c9b2ee199b899891cc17b28a145db393ae23ec Mon Sep 17 00:00:00 2001 From: Raphael Silva Date: Fri, 28 Jun 2024 23:43:22 +0000 Subject: [PATCH 2/2] Fix linting errors Signed-off-by: Raphael Silva --- rules/manager.go | 2 +- rules/manager_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rules/manager.go b/rules/manager.go index 92675e71d..acc637e71 100644 --- a/rules/manager.go +++ b/rules/manager.go @@ -190,7 +190,7 @@ func (m *Manager) Stop() { // Update the rule manager's state as the config requires. If // loading the new rules failed the old rule set is restored. -// This method will no-op in case the manager is already stopped +// This method will no-op in case the manager is already stopped. func (m *Manager) Update(interval time.Duration, files []string, externalLabels labels.Labels, externalURL string, groupEvalIterationFunc GroupEvalIterationFunc) error { m.mtx.Lock() defer m.mtx.Unlock() diff --git a/rules/manager_test.go b/rules/manager_test.go index ec967df24..51239e6c9 100644 --- a/rules/manager_test.go +++ b/rules/manager_test.go @@ -2111,7 +2111,7 @@ func TestUpdateWhenStopped(t *testing.T) { require.NotEmpty(t, ruleManager.groups) ruleManager.Stop() - // Updates following a stop are no-op + // Updates following a stop are no-op. err = ruleManager.Update(10*time.Second, []string{}, labels.EmptyLabels(), "", nil) require.NoError(t, err) }