Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions internal/handlers/v1alpha1/sizer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ func (m *MockStore) Statistics(ctx context.Context) (model.InventoryStats, error
return model.InventoryStats{}, nil
}

func (m *MockStore) RequestMetricsCacheRefresh() {}

func (m *MockStore) Accounts() store.Accounts {
panic("Accounts() not implemented in MockStore for this test")
}
Expand Down
1 change: 1 addition & 0 deletions internal/rvtools/jobs/worker.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ func (w *RVToolsWorker) Work(ctx context.Context, job *river.Job[RVToolsJobArgs]
}
return w.failJob(ctx, logger, job.ID, "create_assessment", err, errMsg)
}
w.store.RequestMetricsCacheRefresh()

updates := store.NewRelationshipBuilder().
With(model.NewAssessmentResource(assessment.ID.String()), model.OwnerRelation, model.NewUserSubject(job.Args.Username)).
Expand Down
5 changes: 5 additions & 0 deletions internal/service/assessment.go
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,8 @@ func (as *AssessmentService) CreateAssessment(ctx context.Context, createForm ma
return nil, err
}

as.store.RequestMetricsCacheRefresh()

tracer.Success().
WithUUID("assessment_id", createdAssessment.ID).
WithString("assessment_name", createdAssessment.Name).
Expand Down Expand Up @@ -249,6 +251,8 @@ func (as *AssessmentService) UpdateAssessment(ctx context.Context, id uuid.UUID,
return nil, err
}

as.store.RequestMetricsCacheRefresh()

tracer.Success().WithString("update_type", "with_new_snapshot").Log()
return as.GetAssessment(ctx, id)
}
Expand Down Expand Up @@ -291,6 +295,7 @@ func (as *AssessmentService) DeleteAssessment(ctx context.Context, id uuid.UUID)
if err := as.store.Assessment().Delete(ctx, id); err != nil {
return fmt.Errorf("failed to delete assessment: %w", err)
}
as.store.RequestMetricsCacheRefresh()

tracer.Success().WithString("deleted_assessment_name", assessment.Name).Log()
return nil
Expand Down
2 changes: 2 additions & 0 deletions internal/service/sizer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ func (m *MockStore) Statistics(ctx context.Context) (model.InventoryStats, error
return model.InventoryStats{}, nil
}

func (m *MockStore) RequestMetricsCacheRefresh() {}

func (m *MockStore) Accounts() store.Accounts {
return nil
}
Expand Down
88 changes: 88 additions & 0 deletions internal/store/metric_cache.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package store

import (
"context"
"fmt"
"sync/atomic"
"time"

"github.com/kubev2v/migration-planner/internal/store/model"
"golang.org/x/sync/singleflight"
)

const (
minCooldownPeriod = 5 * time.Minute
maxCooldownPeriod = 3 * time.Hour
)

// MetricsCache manages cached inventory statistics
type MetricsCache struct {
assessmentStore Assessment
needsUpdate atomic.Bool // Set when the server modifies data requiring cache refresh

stats atomic.Pointer[model.InventoryStats]
lastRefresh atomic.Int64

group singleflight.Group
}

// NewMetricsCache creates a new metrics cache
func NewMetricsCache(s Assessment) *MetricsCache {
return &MetricsCache{
assessmentStore: s,
}
}

// GetStats returns cached stats, refreshing only if cooldown expired.
func (mc *MetricsCache) GetStats(ctx context.Context) (model.InventoryStats, error) {
ptr := mc.stats.Load()

if ptr != nil && !mc.shouldRefresh() {
return *ptr, nil
}

v, err, _ := mc.group.Do("refresh_stats", func() (any, error) {

assessments, err := mc.assessmentStore.List(ctx, NewAssessmentQueryFilter())
if err != nil {
return nil, err
}

stats := model.NewInventoryStats(assessments)

mc.stats.Store(&stats)
mc.lastRefresh.Store(time.Now().UnixNano())
mc.needsUpdate.Store(false)

return stats, nil
})

if err != nil {
return model.InventoryStats{}, fmt.Errorf("refresh cache failed: %w", err)
}

return v.(model.InventoryStats), nil
Comment thread
AvielSegev marked this conversation as resolved.
}
Comment thread
AvielSegev marked this conversation as resolved.

func (mc *MetricsCache) RequestMetricsCacheRefresh() {
mc.needsUpdate.Store(true)
}

// shouldRefresh checks if cooldown period has passed
func (mc *MetricsCache) shouldRefresh() bool {
last := mc.lastRefresh.Load()
if last == 0 {
return true
}

// Potential change by other pods
if time.Since(time.Unix(0, last)) > maxCooldownPeriod {
return true
}
Comment on lines +78 to +81
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify distributed cache invalidation limitations.

The comment "Potential change by other pods" is misleading. This implementation has no distributed cache invalidation—each pod maintains independent cache state. When pod A modifies an assessment, pod B's cache won't refresh until maxCooldownPeriod (3 hours) expires, potentially serving stale data.

If this trade-off is intentional (freshness vs. performance), consider clarifying the comment and documenting the staleness window in multi-pod deployments.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/store/metric_cache.go` around lines 78 - 81, The existing comment
"Potential change by other pods" is misleading because this code uses no
distributed invalidation; update the comment around the time.Since(time.Unix(0,
last)) > maxCooldownPeriod check (and any nearby mention of last and
maxCooldownPeriod) to state explicitly that each pod holds independent cache
state, that entries may remain stale for up to maxCooldownPeriod (currently 3
hours), and that this is a deliberate freshness vs. performance trade-off (or
note next steps such as exposing maxCooldownPeriod as a config or implementing a
distributed invalidation mechanism if you want stronger consistency).


if !mc.needsUpdate.Load() {
return false
}

return time.Since(time.Unix(0, last)) > minCooldownPeriod
}
17 changes: 11 additions & 6 deletions internal/store/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type Store interface {
PartnerCustomer() PartnerCustomer
Statistics(ctx context.Context) (model.InventoryStats, error)
Close() error
RequestMetricsCacheRefresh()
}

type DataStore struct {
Expand All @@ -37,21 +38,25 @@ type DataStore struct {
job Job
accounts Accounts
partnerCustomer PartnerCustomer
metricCache *MetricsCache
}

func NewStore(db *gorm.DB) Store {
assessment := NewAssessmentStore(db)

return &DataStore{
agent: NewAgentSource(db),
source: NewSource(db),
imageInfra: NewImageInfraStore(db),
privateKey: NewCacheKeyStore(NewPrivateKey(db)),
label: NewLabelStore(db),
assessment: NewAssessmentStore(db),
assessment: assessment,
cluster: NewClusterSizingInputStore(db),
job: NewJobStore(db),
authz: NewAuthzStore(db),
accounts: NewAccountsStore(db),
partnerCustomer: NewPartnerCustomerStore(db),
metricCache: NewMetricsCache(assessment),
db: db,
}
}
Expand Down Expand Up @@ -105,11 +110,11 @@ func (s *DataStore) PartnerCustomer() PartnerCustomer {
}

func (s *DataStore) Statistics(ctx context.Context) (model.InventoryStats, error) {
assessments, err := s.Assessment().List(ctx, NewAssessmentQueryFilter())
if err != nil {
return model.InventoryStats{}, err
}
return model.NewInventoryStats(assessments), nil
return s.metricCache.GetStats(ctx)
}
Comment on lines 112 to +114
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff

Consider fallback when cache refresh fails.

When MetricCache.GetStats(ctx) fails (e.g., database temporarily unavailable during refresh), Statistics() returns an error. Should there be a fallback to return stale cached data with a warning, or is failing fast the intended behavior?

This depends on your availability vs. freshness requirements. If stale stats are acceptable during transient failures, consider:

  • Returning cached stats with a logged warning when refresh fails
  • Only failing when no cached stats exist

If failing fast is correct (e.g., stats must be fresh), document this behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/store/store.go` around lines 112 - 114, The current
DataStore.Statistics simply returns s.MetricCache.GetStats(ctx) which fails hard
when a refresh error occurs; change it to attempt a cached fallback: call
GetStats and if it returns an error, try to retrieve stale cached stats (e.g.,
via a new or existing method like MetricCache.GetCachedStats(ctx) or
MetricCache.LastStats()) and return those with a logged warning, but only
propagate the original error when no cached stats exist; implement logging
inside DataStore.Statistics to record the refresh failure and that stale data is
being served.


func (s *DataStore) RequestMetricsCacheRefresh() {
s.metricCache.RequestMetricsCacheRefresh()
}

func (s *DataStore) Close() error {
Expand Down
Loading