fix: register reloadable config variables #108

fracasula · 2023-08-30T13:59:32Z

Description

Now that we're running warehouse tests with the -race flags (used in order to prevent races to go to production), we're seeing random failures in some tests (e.g. snowflake integrations) with hot-reloadable configs.

This PR tries to address the issue in a backwards compatible manner and by providing a succint alternative to use the new API without having to burden ourselves with locking and unlocking mutexes each time we have to deal with such variables.

Context

==================
WARNING: DATA RACE
Write at 0x000008f21110 by goroutine 3191:
  github.com/rudderlabs/rudder-go-kit/config.(*Config).RegisterDurationConfigVariable()
      /home/francesco/go/pkg/mod/github.com/rudderlabs/rudder-go-kit@v0.15.7/config/hotreloadable.go:159 +0x66f
  github.com/rudderlabs/rudder-go-kit/config.RegisterDurationConfigVariable()
      /home/francesco/go/pkg/mod/github.com/rudderlabs/rudder-go-kit@v0.15.7/config/hotreloadable.go:133 +0x467
  github.com/rudderlabs/rudder-server/services/pgnotifier.loadPGNotifierConfig()
      /home/francesco/Code/rudderstack/rudder-server/services/pgnotifier/pgnotifier.go:113 +0x3df
  github.com/rudderlabs/rudder-server/services/pgnotifier.Init()
      /home/francesco/Code/rudderstack/rudder-server/services/pgnotifier/pgnotifier.go:55 +0x29
  github.com/rudderlabs/rudder-server/runner.runAllInit()
      /home/francesco/Code/rudderstack/rudder-server/runner/runner.go:331 +0xb3
  github.com/rudderlabs/rudder-server/runner.(*Runner).Run()
      /home/francesco/Code/rudderstack/rudder-server/runner/runner.go:151 +0x100a
  github.com/rudderlabs/rudder-server/warehouse/integrations/snowflake_test.TestIntegration.func1.1()
      /home/francesco/Code/rudderstack/rudder-server/warehouse/integrations/snowflake/snowflake_test.go:189 +0xee

Previous read at 0x000008f21110 by goroutine 258:
  github.com/rudderlabs/rudder-server/services/pgnotifier.(*PGNotifier).Subscribe.func1()
      /home/francesco/Code/rudderstack/rudder-server/services/pgnotifier/pgnotifier.go:616 +0x1e9
  github.com/rudderlabs/rudder-server/rruntime.GoForWarehouse.func1()
      /home/francesco/Code/rudderstack/rudder-server/rruntime/goroutine-factory.go:36 +0x77

Goroutine 3191 (running) created at:
  github.com/rudderlabs/rudder-server/warehouse/integrations/snowflake_test.TestIntegration.func1()
      /home/francesco/Code/rudderstack/rudder-server/warehouse/integrations/snowflake/snowflake_test.go:187 +0x364
  github.com/rudderlabs/rudder-server/warehouse/integrations/snowflake_test.TestIntegration.func2.1()
      /home/francesco/Code/rudderstack/rudder-server/warehouse/integrations/snowflake/snowflake_test.go:332 +0xec
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1576 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1629 +0x47

Linear Ticket

< Linear Link >

Security

The code changed/added as part of this pull request won't create any security issues with how the software is being used.

codecov · 2023-08-30T14:23:05Z

Codecov Report

Patch coverage: 85.49% and project coverage change: +0.66% 🎉

Comparison is base (ff2dc63) 78.70% compared to head (6e01bd3) 79.37%.
Report is 1 commits behind head on main.

❗ Current head 6e01bd3 differs from pull request most recent head 562b3e7. Consider uploading reports for the commit 562b3e7 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #108      +/-   ##
==========================================
+ Coverage   78.70%   79.37%   +0.66%     
==========================================
  Files          68       68              
  Lines        4866     5087     +221     
==========================================
+ Hits         3830     4038     +208     
- Misses        830      849      +19     
+ Partials      206      200       -6

Files Changed	Coverage Δ
logger/config.go	`91.89% <ø> (ø)`
config/hotreloadable.go	`87.62% <81.88%> (+5.71%)`	⬆️
config/config.go	`82.60% <100.00%> (+2.86%)`	⬆️
config/load.go	`92.26% <100.00%> (+0.32%)`	⬆️
logger/factory.go	`80.37% <100.00%> (ø)`
logger/logger.go	`97.39% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

config/hotreloadable.go

Sidddddarth · 2023-08-31T10:29:14Z

I haven't implemented this for all methods yet, I'd like to receive some early feedback if possible.

LGTM

atzoum · 2023-08-31T18:43:01Z

One major issue we are having now with the RegisterXxx API is that the config package keeps pointers to fields of other structs, e.g. router, batchrouter, jobsdb handes etc. so these structs are never garbage collected even though they might no longer be needed by the system (see MT rudder server stop/start sequences). So we end up with a slow-burning memory leak.
Can we invert the control and let the config package return pointers to configuration variables instead?
Furthermore, asking for the same reloadable configuration property twice shouldn't result in multiple atomic structs to be created, the existing struct could be reused.
Finally, the Atomic struct could only export a Load method and keep Store unexported

fracasula · 2023-09-01T07:29:26Z

@atzoum I wasn't aware of the memory leaks, I'll have a look 👍

config/config.go

config/hotreloadable.go

atzoum

thank you @fracasula for taking the extra time to try and address important issues with our reloadable configs!

config/config.go

atzoum · 2023-09-05T07:40:31Z

config/hotreloadable.go

+// RegisterAtomicIntVar registers a hot-reloadable int config variable
+// Copy of RegisterIntConfigVariable, but with a way to avoid data races for hot reloadable config variables
+func (c *Config) RegisterAtomicIntVar(defaultValue, valueScale int, keys ...string) *Atomic[int] {
+	ptr := getOrCreatePointer(c.atomicVars, c.atomicVarsMisuses, &c.atomicVarsLock, defaultValue, keys...)


Whenever we are reusing an existing pointer we shouldn't register it again.
If getOrCreatePointer returned a second value indicating whether this is a new or existing pointer, we could use this information for avoiding registering it twice.

One thing we should definitely fix though is whenever we are calling appendVarToConfigMaps, we should pass as a key the full concatenated key instead of the first key from the slice.

One thing we should definitely fix though is whenever we are calling appendVarToConfigMaps, we should pass as a key the full concatenated key instead of the first key from the slice.

Nice, good catch 👍

Whenever we are reusing an existing pointer we shouldn't register it again. If getOrCreatePointer returned a second value indicating whether this is a new or existing pointer, we could use this information for avoiding registering it twice.

We could allow this though, it is now safe to do so and you'll get the same variable. I'm keen to let people do it at this point. If someone misuses the default values will get a panic as well. Perhaps we should settle on the discussion about the order of the keys first, I might be missing some use case.

atzoum · 2023-09-05T07:42:38Z

config/config.go

+	atomicVars              map[string]any
+	atomicVarsMisuses       map[string]string
+	atomicVarsLock          sync.RWMutex // used to protect both atomicVars maps


Minor: since all these fields are part of the hot-reloadable feature we could use hotReloadableConfigLock instead of atomicVarsLock, is there an actual need for a second lock?

They are used at different times and to protect different data so I'm keen to keep both actually to minimize lock contention. It seems like a small price to pay.

config/hotreloadable.go

$fracasula$

github-actions bot added the config label Aug 30, 2023

$@fracasula$ fracasula requested review from lvrach, atzoum, cisse21, BonapartePC, achettyiitr and Sidddddarth August 30, 2023 13:59

$@fracasula$ fracasula changed the title ~~fix: register atomic int~~ fix: register atomic variables Aug 30, 2023

$@fracasula$ fracasula changed the title ~~fix: register atomic variables~~ fix: register atomic config variables Aug 30, 2023

github-actions bot added the with tests label Aug 30, 2023

$fracasula$

fracasula commented Aug 30, 2023

View reviewed changes

config/hotreloadable.go Outdated Show resolved Hide resolved

$@fracasula$ fracasula mentioned this pull request Aug 30, 2023

chore: uploader can append rudderlabs/rudder-server#3805

Merged

1 task

Sidddddarth reviewed Aug 30, 2023

View reviewed changes

config/hotreloadable.go Outdated Show resolved Hide resolved

$@fracasula$ fracasula marked this pull request as ready for review August 31, 2023 16:59

$@fracasula$ fracasula force-pushed the fix.configHotReload branch from f829851 to b2eb639 Compare September 1, 2023 09:01

$@fracasula$ fracasula requested a review from Sidddddarth September 1, 2023 11:11

Sidddddarth reviewed Sep 1, 2023

View reviewed changes

config/config.go Show resolved Hide resolved

$fracasula$

fracasula commented Sep 3, 2023

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

atzoum reviewed Sep 5, 2023

View reviewed changes

config/hotreloadable.go Outdated Show resolved Hide resolved

atzoum reviewed Sep 5, 2023

View reviewed changes

$@fracasula$ fracasula requested review from atzoum and Sidddddarth September 11, 2023 14:37

atzoum reviewed Sep 12, 2023

View reviewed changes

config/hotreloadable.go Outdated Show resolved Hide resolved

atzoum reviewed Sep 12, 2023

View reviewed changes

config/hotreloadable.go Outdated Show resolved Hide resolved

atzoum reviewed Sep 12, 2023

View reviewed changes

config/hotreloadable.go Outdated Show resolved Hide resolved

atzoum reviewed Sep 12, 2023

View reviewed changes

config/hotreloadable.go Outdated Show resolved Hide resolved

fracasula added 23 commits September 12, 2023 17:44

$@fracasula$

chore: adding map[string]interface{} support

0e1c9df

$@fracasula$

chore: unexporting store

02ff5f6

$@fracasula$

chore: RegisterAtomicIntVar returns pointer

8fed589

$@fracasula$

chore: RegisterAtomicBoolVar returns pointer

b2760f0

$@fracasula$

chore: more pointers

25ca1ad

$@fracasula$

chore: more pointers

787068c

$@fracasula$

chore: getOrCreatePointer tests

ca60cd4

$@fracasula$

chore: updating panic message

955dc45

$@fracasula$

chore: making keys more readable

9a19508

$@fracasula$

chore: removing pointers to maps

3f7ac17

$@fracasula$

chore: atomic -> reloadable

8406f2e

$@fracasula$

chore: order of keys

063d7f8

$@fracasula$

chore: always return ptr from new API

3191fc8

$@fracasula$

chore: make fmt

e1478e5

$@fracasula$

chore: increasing test coverage

1b0bbc0

$@fracasula$

chore: spacing deprecated

64d707a

$@fracasula$

chore: beefing up warning

d7a6b97

$@fracasula$

chore: non-reloadable new API to return values

87eaad3

$@fracasula$

chore: updating swap comment

1598e4c

$@fracasula$

chore: updating logger factory

0010948

$@fracasula$

chore: sorting imports

a2f21f4

$@fracasula$

chore: warning in separate paragraph

755abbe

$@fracasula$

chore: get in favour of register for new api

6e01bd3

$@fracasula$ fracasula force-pushed the fix.configHotReload branch from bb0e5a4 to 6e01bd3 Compare September 12, 2023 15:44

$@fracasula$ fracasula changed the title ~~fix: register atomic config variables~~ fix: register reloadable config variables Sep 12, 2023

pChondros approved these changes Sep 13, 2023

View reviewed changes

$@fracasula$

Merge branch 'main' into fix.configHotReload

562b3e7

$@fracasula$ fracasula merged commit 2466840 into main Sep 13, 2023
8 checks passed

$@fracasula$ fracasula deleted the fix.configHotReload branch September 13, 2023 09:19

devops-github-rudderstack mentioned this pull request Sep 13, 2023

chore: release 0.15.9 #117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: register reloadable config variables #108

fix: register reloadable config variables #108

$@fracasula$ fracasula commented Aug 30, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading

Sidddddarth commented Aug 31, 2023

atzoum commented Aug 31, 2023 •

edited

Loading

fracasula commented Sep 1, 2023

atzoum left a comment

atzoum Sep 5, 2023

$@fracasula$ fracasula Sep 11, 2023 •

edited

Loading

atzoum Sep 5, 2023 •

edited

Loading

$@fracasula$ fracasula Sep 11, 2023

fix: register reloadable config variables #108

fix: register reloadable config variables #108

Conversation

fracasula commented Aug 30, 2023 • edited Loading

Description

Context

Linear Ticket

Security

codecov bot commented Aug 30, 2023 • edited Loading

Codecov Report

Sidddddarth commented Aug 31, 2023

atzoum commented Aug 31, 2023 • edited Loading

fracasula commented Sep 1, 2023

atzoum left a comment

Choose a reason for hiding this comment

atzoum Sep 5, 2023

Choose a reason for hiding this comment

fracasula Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

atzoum Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

fracasula Sep 11, 2023

Choose a reason for hiding this comment

$@fracasula$ fracasula commented Aug 30, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading

atzoum commented Aug 31, 2023 •

edited

Loading

$@fracasula$ fracasula Sep 11, 2023 •

edited

Loading

atzoum Sep 5, 2023 •

edited

Loading

$@fracasula$ fracasula Sep 11, 2023