Concurrent metrics update in cache

Co-authored-by: Chris Reeder <[email protected]>

Author: clintonk

core/common.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/netapp/trident/config"
+	"github.com/netapp/trident/core/metrics"
 	. "github.com/netapp/trident/logging"
 	"github.com/netapp/trident/storage"
 	"github.com/netapp/trident/utils/models"
@@ -28,7 +29,7 @@ func recordTiming(operation string, err *error) func() {
 		if *err != nil {
 			success = "false"
 		}
-		operationDurationInMsSummary.WithLabelValues(operation, success).Observe(endTimeMS)
+		metrics.OperationDurationInMsSummary.WithLabelValues(operation, success).Observe(endTimeMS)
 	}
 }
 
@@ -47,7 +48,7 @@ func recordTransactionTiming(txn *storage.VolumeTransaction, err *error) {
 	if *err != nil {
 		success = "false"
 	}
-	operationDurationInMsSummary.WithLabelValues(operation, success).Observe(endTimeMS)
+	metrics.OperationDurationInMsSummary.WithLabelValues(operation, success).Observe(endTimeMS)
 }
 
 // getProtocol returns the appropriate protocol based on a specified volume mode, access mode and protocol, or

core/concurrent_cache/backend.go

@@ -102,15 +102,25 @@ func UpsertBackend(id, name, newName string) Subquery {
 		newKey: newName,
 		setResults: func(s *Subquery, r *Result) error {
 			backends.rlock()
-			if i, ok := backends.data[s.id]; ok {
-				r.Backend.Read = i.SmartCopy().(storage.Backend)
+			oldBackendData, ok := backends.data[s.id]
+			if ok {
+				r.Backend.Read = oldBackendData.SmartCopy().(storage.Backend)
 			}
 			backends.runlock()
+
 			r.Backend.Upsert = func(b storage.Backend) {
-				backends.lock()
 				if s.id == "" {
 					s.id = b.BackendUUID()
 				}
+
+				// Update metrics
+				if oldBackendData != nil {
+					deleteBackendFromMetrics(oldBackendData.(storage.Backend))
+				}
+				addBackendToMetrics(b)
+
+				backends.lock()
+
 				// This is the canonical method for upserting with a unique key.
 				// There are 4 cases: key and newKey are equal, key is empty, newKey is empty,
 				// and key and newKey are different. In all cases expect #3,
@@ -131,6 +141,7 @@ func UpsertBackend(id, name, newName string) Subquery {
 					delete(backends.key.data, s.key)
 					backends.key.data[s.newKey] = s.id
 				}
+
 				backends.data[s.id] = b
 				backends.unlock()
 			}
@@ -146,11 +157,18 @@ func DeleteBackend(id string) Subquery {
 		id:  id,
 		setResults: func(s *Subquery, r *Result) error {
 			backends.rlock()
-			if i, ok := backends.data[s.id]; ok {
-				r.Backend.Read = i.SmartCopy().(storage.Backend)
+			backendData, ok := backends.data[s.id]
+			if ok {
+				r.Backend.Read = backendData.SmartCopy().(storage.Backend)
 			}
 			backends.runlock()
+
 			r.Backend.Delete = func() {
+				// Update metrics
+				if backendData != nil {
+					deleteBackendFromMetrics(backendData.(storage.Backend))
+				}
+
 				backends.lock()
 				delete(backends.data, s.id)
 				// delete from the uniqueKey cache as well

core/concurrent_cache/backend_test.go

@@ -0,0 +1,203 @@
+package concurrent_cache
+
+import (
+	"testing"
+
+	"github.com/prometheus/client_golang/prometheus/testutil"
+	"github.com/stretchr/testify/assert"
+	"go.uber.org/mock/gomock"
+
+	"github.com/netapp/trident/core/metrics"
+	"github.com/netapp/trident/storage"
+)
+
+func TestUpsertBackend_Metrics(t *testing.T) {
+	tests := []struct {
+		name           string
+		backendExists  bool
+		initialBackend storage.Backend
+		upsertBackend  storage.Backend
+	}{
+		{
+			name:          "insert new backend",
+			backendExists: false,
+		},
+		{
+			name:          "update existing backend",
+			backendExists: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockCtrl := gomock.NewController(t)
+			defer mockCtrl.Finish()
+
+			// Reset metrics before each test
+			metrics.BackendsGauge.Reset()
+			metrics.TridentBackendInfo.Reset()
+
+			// Set up initial state if backend exists
+			if tt.backendExists {
+				tt.initialBackend = getMockBackendWithMap(mockCtrl, map[string]string{
+					"name":       "existing-backend",
+					"driverName": "test-driver",
+					"state":      string(storage.Online),
+					"uuid":       "test-backend-uuid",
+				})
+				backends.lock()
+				backends.data["test-backend-uuid"] = tt.initialBackend
+				backends.unlock()
+				// Add the existing backend to metrics to simulate realistic state
+				addBackendToMetrics(tt.initialBackend)
+			}
+
+			// Get initial metric values
+			initialBackendGauge := testutil.ToFloat64(metrics.BackendsGauge.WithLabelValues("test-driver", string(storage.Online)))
+			initialTridentBackendInfo := testutil.ToFloat64(metrics.TridentBackendInfo.WithLabelValues("test-driver", "existing-backend", "test-backend-uuid"))
+
+			// Create upsert backend
+			tt.upsertBackend = getMockBackendWithMap(mockCtrl, map[string]string{
+				"name":       "updated-backend",
+				"driverName": "test-driver",
+				"state":      string(storage.Online),
+				"uuid":       "test-backend-uuid",
+			})
+
+			// Execute upsert operation
+			subquery := UpsertBackend("test-backend-uuid", "test-backend", "updated-backend")
+			result := &Result{}
+			err := subquery.setResults(&subquery, result)
+			assert.NoError(t, err, "UpsertBackend setResults should not error")
+
+			// Verify the upsert function was created
+			assert.NotNil(t, result.Backend.Upsert, "Upsert function should be created")
+
+			// Call the upsert function
+			result.Backend.Upsert(tt.upsertBackend)
+
+			// Verify metrics were updated correctly
+			afterUpsertBackendGauge := testutil.ToFloat64(metrics.BackendsGauge.WithLabelValues("test-driver", string(storage.Online)))
+			afterUpsertTridentBackendInfo := testutil.ToFloat64(metrics.TridentBackendInfo.WithLabelValues("test-driver", "updated-backend", "test-backend-uuid"))
+
+			if tt.backendExists {
+				// For existing backend: delete old (dec) + add new (inc) = no net change
+				assert.Equal(t, initialBackendGauge, afterUpsertBackendGauge, "BackendGauge should remain unchanged when updating existing backend")
+				// TridentBackendInfo should be set to 1 for the new backend name/labels
+				assert.Equal(t, initialTridentBackendInfo, afterUpsertTridentBackendInfo, "TridentBackendInfo should be set to 1 for updated backend")
+
+				// Verify the old TridentBackendInfo metric is removed
+				oldTridentBackendInfo := testutil.ToFloat64(metrics.TridentBackendInfo.WithLabelValues("test-driver", "existing-backend", "test-backend-uuid"))
+
+				assert.Equal(t, float64(0), oldTridentBackendInfo, "Old TridentBackendInfo should be removed")
+			} else {
+				// For new backend: only add (inc) = increment by 1
+				assert.Equal(t, initialBackendGauge+1, afterUpsertBackendGauge, "BackendGauge should be incremented by 1 when adding new backend")
+				assert.Equal(t, initialTridentBackendInfo+1, afterUpsertTridentBackendInfo, "TridentBackendInfo should be set to 1 for new backend")
+			}
+
+			// Verify the backend was actually stored
+			backends.rlock()
+			storedBackend, exists := backends.data["test-backend-uuid"]
+			backends.runlock()
+			assert.True(t, exists, "Backend should exist in storage after upsert")
+			assert.Equal(t, tt.upsertBackend, storedBackend, "Stored backend should match upserted backend")
+
+			// Clean up
+			backends.lock()
+			delete(backends.data, "test-backend-uuid")
+			delete(backends.key.data, "test-backend")
+			delete(backends.key.data, "updated-backend")
+			backends.unlock()
+		})
+	}
+}
+
+func TestDeleteBackend_Metrics(t *testing.T) {
+	tests := []struct {
+		name            string
+		backendExists   bool
+		backendToDelete storage.Backend
+	}{
+		{
+			name:          "delete existing backend",
+			backendExists: true,
+		},
+		{
+			name:          "delete non-existing backend",
+			backendExists: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockCtrl := gomock.NewController(t)
+			defer mockCtrl.Finish()
+
+			// Reset metrics before each test
+			metrics.BackendsGauge.Reset()
+			metrics.TridentBackendInfo.Reset()
+
+			// Set up initial state if backend exists
+			if tt.backendExists {
+				tt.backendToDelete = getMockBackendWithMap(mockCtrl, map[string]string{
+					"name":       "existing-backend",
+					"driverName": "test-driver",
+					"state":      string(storage.Online),
+					"uuid":       "test-backend-uuid",
+				})
+				backends.lock()
+				backends.data["test-backend-uuid"] = tt.backendToDelete
+				backends.unlock()
+				// Add the existing backend to metrics to simulate realistic state
+				addBackendToMetrics(tt.backendToDelete)
+			}
+
+			// Get initial metric value
+			initialBackendGauge := testutil.ToFloat64(metrics.BackendsGauge.WithLabelValues("test-driver", string(storage.Online)))
+			initialTridentBackendInfo := testutil.ToFloat64(metrics.TridentBackendInfo.WithLabelValues("test-driver", "existing-backend", "test-backend-uuid"))
+
+			// Execute delete operation
+			subquery := DeleteBackend("test-backend-uuid")
+			result := &Result{}
+			err := subquery.setResults(&subquery, result)
+			assert.NoError(t, err, "DeleteBackend setResults should not error")
+
+			if tt.backendExists {
+				// Verify the delete function was created and the backend was read
+				assert.NotNil(t, result.Backend.Delete, "Delete function should be created")
+				assert.Equal(t, tt.backendToDelete, result.Backend.Read, "Read backend should match the backend that exists")
+
+				// Call the delete function
+				result.Backend.Delete()
+
+				// Verify metrics were updated correctly (decremented by 1)
+				afterDeleteBackendGauge := testutil.ToFloat64(metrics.BackendsGauge.WithLabelValues("test-driver", string(storage.Online)))
+				afterDeleteTridentBackendInfo := testutil.ToFloat64(metrics.TridentBackendInfo.WithLabelValues("test-driver", "existing-backend", "test-backend-uuid"))
+
+				assert.Equal(t, initialBackendGauge-1, afterDeleteBackendGauge, "BackendGauge should be decremented by 1 when deleting existing backend")
+				assert.Equal(t, initialTridentBackendInfo-1, afterDeleteTridentBackendInfo, "TridentBackendInfo should be set to 0 when deleting existing backend")
+
+				// Verify the backend was actually removed from storage
+				backends.rlock()
+				_, exists := backends.data["test-backend-uuid"]
+				backends.runlock()
+				assert.False(t, exists, "Backend should not exist in storage after delete")
+			} else {
+				// For non-existing backend, delete function should still be created but Read should be nil
+				assert.NotNil(t, result.Backend.Delete, "Delete function should be created even for non-existing backend")
+				assert.Nil(t, result.Backend.Read, "Read backend should be nil for non-existing backend")
+
+				// Call the delete function
+				result.Backend.Delete()
+
+				// Verify metrics were NOT updated (no change since backend didn't exist)
+				afterDeleteBackendGauge := testutil.ToFloat64(metrics.BackendsGauge.WithLabelValues("test-driver", string(storage.Online)))
+				afterDeleteTridentBackendInfo := testutil.ToFloat64(metrics.TridentBackendInfo.WithLabelValues("test-driver", "existing-backend", "test-backend-uuid"))
+
+				assert.Equal(t, initialBackendGauge, afterDeleteBackendGauge, "BackendGauge should remain unchanged when deleting non-existing backend")
+				assert.Equal(t, initialTridentBackendInfo, afterDeleteTridentBackendInfo, "TridentBackendInfo should remain unchanged when deleting non-existing backend")
+			}
+		})
+	}
+}

core/concurrent_cache/concurrent_cache.go

@@ -232,11 +232,30 @@ func assembleQueries(queries [][]Subquery, roots [][]int, cachesPresent map[reso
 }
 
 func lockCachesAndFillInIDs(queries [][]Subquery, roots [][]int, cachesPresent map[resource]struct{}) error {
-	sortedResources := make([]resource, 0, len(cachesPresent))
+	resources := make([]resource, 0, len(cachesPresent))
 	for r := range cachesPresent {
-		sortedResources = append(sortedResources, r)
+		resources = append(resources, r)
 	}
-	slices.SortFunc(sortedResources, func(i, j resource) int {
+	unlocker, err := rLockCaches(resources)
+	defer unlocker()
+	if err != nil {
+		return err
+	}
+
+	for i := range queries {
+		for _, r := range roots[i] {
+			if err := fillInIDs(r, queries[i]); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+// rLockCaches sorts and acquires read locks on all caches for the given resources. It returns a function that releases
+// the locks, and error if a cache does not exist.
+func rLockCaches(resources []resource) (func(), error) {
+	slices.SortFunc(resources, func(i, j resource) int {
 		switch {
 		case resourceRanks[i] > resourceRanks[j]:
 			return -1
@@ -253,24 +272,18 @@ func lockCachesAndFillInIDs(queries [][]Subquery, roots [][]int, cachesPresent m
 
 		return 0
 	})
-
-	for _, r := range sortedResources {
+	unlocks := make([]func(), 0, len(caches))
+	for _, r := range resources {
 		c, ok := caches[r]
 		if !ok {
-			return fmt.Errorf("no cache found for %s", r)
+			return func() {}, fmt.Errorf("no cache found for resource %s", r)
 		}
 		c.rlock()
-		defer c.runlock()
+		unlocks = append(unlocks, c.runlock)
 	}
-
-	for i := range queries {
-		for _, r := range roots[i] {
-			if err := fillInIDs(r, queries[i]); err != nil {
-				return err
-			}
-		}
-	}
-	return nil
+	return func() {
+		doReverse(unlocks)
+	}, nil
 }
 
 func checkError(queries []Subquery) error {