From 4110d1d9c851d3e4286167345cb8d20e547f2584 Mon Sep 17 00:00:00 2001 From: Isabella Siu Date: Wed, 12 Feb 2025 16:30:10 -0500 Subject: [PATCH 1/3] WIP: Implementing TTL for cached datasource instances --- backend/instancemgmt/instance_manager.go | 46 ++++++++++-------- backend/instancemgmt/instance_manager_test.go | 47 +++++++++++++++++++ go.mod | 1 + go.sum | 2 + 4 files changed, 77 insertions(+), 19 deletions(-) diff --git a/backend/instancemgmt/instance_manager.go b/backend/instancemgmt/instance_manager.go index 4ca761d81..d271c0799 100644 --- a/backend/instancemgmt/instance_manager.go +++ b/backend/instancemgmt/instance_manager.go @@ -2,14 +2,15 @@ package instancemgmt import ( "context" + "fmt" "reflect" - "sync" "time" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" "github.com/grafana/grafana-plugin-sdk-go/backend" + gocache "github.com/patrickmn/go-cache" ) var ( @@ -19,6 +20,9 @@ var ( Help: "The number of active plugin instances", }) disposeTTL = 5 * time.Second + + instanceTTL = 1 * time.Hour + instanceCleanup = 2 * time.Hour ) // Instance is a marker interface for an instance. @@ -76,10 +80,20 @@ func New(provider InstanceProvider) InstanceManager { if provider == nil { panic("provider cannot be nil") } + cache := gocache.New(instanceTTL, instanceCleanup) + cache.OnEvicted(func(_ string, i interface{}) { + ci := i.(CachedInstance) + if disposer, valid := ci.instance.(InstanceDisposer); valid { + time.AfterFunc(disposeTTL, func() { + disposer.Dispose() + }) + } + activeInstances.Dec() + }) return &instanceManager{ provider: provider, - cache: sync.Map{}, + cache: cache, locker: newLocker(), } } @@ -87,7 +101,7 @@ func New(provider InstanceProvider) InstanceManager { type instanceManager struct { locker *locker provider InstanceProvider - cache sync.Map + cache *gocache.Cache } func (im *instanceManager) Get(ctx context.Context, pluginContext backend.PluginContext) (Instance, error) { @@ -96,9 +110,10 @@ func (im *instanceManager) Get(ctx context.Context, pluginContext backend.Plugin return nil, err } // Double-checked locking for update/create criteria - im.locker.RLock(cacheKey) - item, ok := im.cache.Load(cacheKey) - im.locker.RUnlock(cacheKey) + strKey := fmt.Sprintf("%v", cacheKey) + im.locker.RLock(strKey) + item, ok := im.cache.Get(strKey) + im.locker.RUnlock(strKey) if ok { ci := item.(CachedInstance) @@ -109,10 +124,10 @@ func (im *instanceManager) Get(ctx context.Context, pluginContext backend.Plugin } } - im.locker.Lock(cacheKey) - defer im.locker.Unlock(cacheKey) + im.locker.Lock(strKey) + defer im.locker.Unlock(strKey) - if item, ok := im.cache.Load(cacheKey); ok { + if item, ok := im.cache.Get(strKey); ok { ci := item.(CachedInstance) needsUpdate := im.provider.NeedsUpdate(ctx, pluginContext, ci) @@ -120,24 +135,17 @@ func (im *instanceManager) Get(ctx context.Context, pluginContext backend.Plugin return ci.instance, nil } - if disposer, valid := ci.instance.(InstanceDisposer); valid { - time.AfterFunc(disposeTTL, func() { - disposer.Dispose() - activeInstances.Dec() - }) - } else { - activeInstances.Dec() - } + im.cache.Delete(strKey) } instance, err := im.provider.NewInstance(ctx, pluginContext) if err != nil { return nil, err } - im.cache.Store(cacheKey, CachedInstance{ + im.cache.Set(strKey, CachedInstance{ PluginContext: pluginContext, instance: instance, - }) + }, gocache.DefaultExpiration) activeInstances.Inc() return instance, nil diff --git a/backend/instancemgmt/instance_manager_test.go b/backend/instancemgmt/instance_manager_test.go index 8351fe240..3cbd82cf9 100644 --- a/backend/instancemgmt/instance_manager_test.go +++ b/backend/instancemgmt/instance_manager_test.go @@ -70,6 +70,53 @@ func TestInstanceManager(t *testing.T) { }) } +func TestInstanceManagerExpiration(t *testing.T) { + ctx := context.Background() + pCtx := backend.PluginContext{ + OrgID: 1, + AppInstanceSettings: &backend.AppInstanceSettings{ + Updated: time.Now(), + }, + } + + origInstanceTTL := instanceTTL + instanceTTL = time.Millisecond + origInstanceCleanup := instanceCleanup + instanceCleanup = 2 * time.Millisecond + t.Cleanup(func() { + instanceTTL = origInstanceTTL + instanceCleanup = origInstanceCleanup + }) + + tip := &testInstanceProvider{} + im := New(tip) + + instance, err := im.Get(ctx, pCtx) + require.NoError(t, err) + require.NotNil(t, instance) + require.Equal(t, pCtx.OrgID, instance.(*testInstance).orgID) + require.Equal(t, pCtx.AppInstanceSettings.Updated, instance.(*testInstance).updated) + + t.Run("After expiration", func(t *testing.T) { + instance.(*testInstance).wg.Wait() + require.True(t, instance.(*testInstance).disposed.Load()) + require.Equal(t, int64(1), instance.(*testInstance).disposedTimes.Load()) + + newInstance, err := im.Get(ctx, pCtx) + + t.Run("New instance should be created", func(t *testing.T) { + require.NoError(t, err) + require.NotNil(t, newInstance) + require.Equal(t, pCtx.OrgID, newInstance.(*testInstance).orgID) + require.Equal(t, pCtx.AppInstanceSettings.Updated, newInstance.(*testInstance).updated) + }) + + t.Run("New instance should not be the same as old instance", func(t *testing.T) { + require.NotSame(t, instance, newInstance) + }) + }) +} + func TestInstanceManagerConcurrency(t *testing.T) { t.Run("Check possible race condition issues when initially creating instance", func(t *testing.T) { ctx := context.Background() diff --git a/go.mod b/go.mod index d742c2097..3fb2fcff4 100644 --- a/go.mod +++ b/go.mod @@ -96,6 +96,7 @@ require ( github.com/oasdiff/yaml3 v0.0.0-20250309153720-d2182401db90 // indirect github.com/oklog/run v1.0.0 // indirect github.com/oklog/ulid v1.3.1 // indirect + github.com/patrickmn/go-cache v2.1.0+incompatible github.com/perimeterx/marshmallow v1.1.5 // indirect github.com/pierrec/lz4/v4 v4.1.22 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect diff --git a/go.sum b/go.sum index 1163e4ba8..d261c4ec5 100644 --- a/go.sum +++ b/go.sum @@ -180,6 +180,8 @@ github.com/oklog/ulid v1.3.1 h1:EGfNDEx6MqHz8B3uNV6QAib1UR2Lm97sHi3ocA6ESJ4= github.com/oklog/ulid v1.3.1/go.mod h1:CirwcVhetQ6Lv90oh/F+FBtV6XMibvdAFo93nm5qn4U= github.com/olekukonko/tablewriter v0.0.5 h1:P2Ga83D34wi1o9J6Wh1mRuqd4mF/x/lgBS7N7AbDhec= github.com/olekukonko/tablewriter v0.0.5/go.mod h1:hPp6KlRPjbx+hW8ykQs1w3UBbZlj6HuIJcUGPhkA7kY= +github.com/patrickmn/go-cache v2.1.0+incompatible h1:HRMgzkcYKYpi3C8ajMPV8OFXaaRUnok+kx1WdO15EQc= +github.com/patrickmn/go-cache v2.1.0+incompatible/go.mod h1:3Qf8kWWT7OJRJbdiICTKqZju1ZixQ/KpMGzzAfe6+WQ= github.com/perimeterx/marshmallow v1.1.5 h1:a2LALqQ1BlHM8PZblsDdidgv1mWi1DgC2UmX50IvK2s= github.com/perimeterx/marshmallow v1.1.5/go.mod h1:dsXbUu8CRzfYP5a87xpp0xq9S3u0Vchtcl8we9tYaXw= github.com/pierrec/lz4/v4 v4.1.22 h1:cKFw6uJDK+/gfw5BcDL0JL5aBsAFdsIT18eRtLj7VIU= From 2ccc384cba498a39b3dcc37cb36fd2428a1b8ffa Mon Sep 17 00:00:00 2001 From: Isabella Siu Date: Tue, 15 Apr 2025 17:08:45 -0400 Subject: [PATCH 2/3] increase expiration timeout --- backend/instancemgmt/instance_manager.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/instancemgmt/instance_manager.go b/backend/instancemgmt/instance_manager.go index d271c0799..50f2b4525 100644 --- a/backend/instancemgmt/instance_manager.go +++ b/backend/instancemgmt/instance_manager.go @@ -21,8 +21,8 @@ var ( }) disposeTTL = 5 * time.Second - instanceTTL = 1 * time.Hour - instanceCleanup = 2 * time.Hour + instanceTTL = 24 * time.Hour + instanceCleanup = 48 * time.Hour ) // Instance is a marker interface for an instance. From a3ed029cb1c9d4f99275711b9b5161d76c594647 Mon Sep 17 00:00:00 2001 From: Isabella Siu Date: Thu, 17 Apr 2025 14:06:50 -0400 Subject: [PATCH 3/3] fix race condition in test --- backend/instancemgmt/instance_manager.go | 10 ++++--- backend/instancemgmt/instance_manager_test.go | 26 +++---------------- 2 files changed, 10 insertions(+), 26 deletions(-) diff --git a/backend/instancemgmt/instance_manager.go b/backend/instancemgmt/instance_manager.go index 50f2b4525..88824f8a1 100644 --- a/backend/instancemgmt/instance_manager.go +++ b/backend/instancemgmt/instance_manager.go @@ -19,10 +19,10 @@ var ( Name: "active_instances", Help: "The number of active plugin instances", }) - disposeTTL = 5 * time.Second + defaultDisposeTTL = 5 * time.Second - instanceTTL = 24 * time.Hour - instanceCleanup = 48 * time.Hour + defaultInstanceTTL = 24 * time.Hour + defaultInstanceCleanup = 48 * time.Hour ) // Instance is a marker interface for an instance. @@ -77,6 +77,10 @@ type InstanceProvider interface { // New create a new instance manager. func New(provider InstanceProvider) InstanceManager { + return NewWithOptions(provider, defaultInstanceTTL, defaultInstanceCleanup, defaultDisposeTTL) +} + +func NewWithOptions(provider InstanceProvider, instanceTTL, instanceCleanup, disposeTTL time.Duration) InstanceManager { if provider == nil { panic("provider cannot be nil") } diff --git a/backend/instancemgmt/instance_manager_test.go b/backend/instancemgmt/instance_manager_test.go index 3cbd82cf9..b0968d929 100644 --- a/backend/instancemgmt/instance_manager_test.go +++ b/backend/instancemgmt/instance_manager_test.go @@ -21,7 +21,7 @@ func TestInstanceManager(t *testing.T) { } tip := &testInstanceProvider{} - im := New(tip) + im := NewWithOptions(tip, defaultInstanceTTL, defaultInstanceCleanup, time.Millisecond) t.Run("When getting instance should create a new instance", func(t *testing.T) { instance, err := im.Get(ctx, pCtx) @@ -43,11 +43,6 @@ func TestInstanceManager(t *testing.T) { Updated: time.Now(), }, } - origDisposeTTL := disposeTTL - disposeTTL = time.Millisecond - t.Cleanup(func() { - disposeTTL = origDisposeTTL - }) newInstance, err := im.Get(ctx, pCtxUpdated) t.Run("New instance should be created", func(t *testing.T) { @@ -79,17 +74,8 @@ func TestInstanceManagerExpiration(t *testing.T) { }, } - origInstanceTTL := instanceTTL - instanceTTL = time.Millisecond - origInstanceCleanup := instanceCleanup - instanceCleanup = 2 * time.Millisecond - t.Cleanup(func() { - instanceTTL = origInstanceTTL - instanceCleanup = origInstanceCleanup - }) - tip := &testInstanceProvider{} - im := New(tip) + im := NewWithOptions(tip, time.Millisecond, 2*time.Millisecond, defaultDisposeTTL) instance, err := im.Get(ctx, pCtx) require.NoError(t, err) @@ -157,12 +143,6 @@ func TestInstanceManagerConcurrency(t *testing.T) { }) t.Run("Check possible race condition issues when re-creating instance on settings update", func(t *testing.T) { - origDisposeTTL := disposeTTL - disposeTTL = time.Millisecond - t.Cleanup(func() { - disposeTTL = origDisposeTTL - }) - ctx := context.Background() initialPCtx := backend.PluginContext{ OrgID: 1, @@ -171,7 +151,7 @@ func TestInstanceManagerConcurrency(t *testing.T) { }, } tip := &testInstanceProvider{} - im := New(tip) + im := NewWithOptions(tip, defaultInstanceTTL, defaultInstanceCleanup, time.Millisecond) // Creating initial instance with old contexts instanceToDispose, _ := im.Get(ctx, initialPCtx)