From 072e3779d0ad3022dea924e2040d575c2ee3d104 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 20:22:03 +0100 Subject: [PATCH 01/12] feat(azure/managedredis): export ReservationUnitPrice Advisor pricing looks up a reservationUnitPricer per service. managedredis only had the private getRedisPricing, so routing Redis rows through it would skip every row. Export the same wrapper the cache client has. Unknown prices and unsupported SKU identities return an error, never 0. Refs #692 Co-Authored-By: Claude Opus 5.5 --- .../azure/services/managedredis/client.go | 9 ++++ .../reservation_unit_price_test.go | 46 +++++++++++++++++++ 2 files changed, 55 insertions(+) create mode 100644 providers/azure/services/managedredis/reservation_unit_price_test.go diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index 8d1b5700..11d2f08c 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -500,6 +500,15 @@ type RedisPricing struct { ReservationPrice float64 } +// ReservationUnitPrice returns the Retail Prices reservation total and currency for one unit of sku in region over termYears. +func (c *Client) ReservationUnitPrice(ctx context.Context, sku, region string, termYears int) (recommendations.ReservationPrice, error) { + p, err := c.getRedisPricing(ctx, sku, region, termYears) + if err != nil { + return recommendations.ReservationPrice{}, err + } + return recommendations.ReservationPrice{Total: p.ReservationPrice, Currency: p.Currency}, nil +} + func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYears int) (*RedisPricing, error) { identity, err := pricing.ParseRedisIdentity(sku) if err != nil { diff --git a/providers/azure/services/managedredis/reservation_unit_price_test.go b/providers/azure/services/managedredis/reservation_unit_price_test.go new file mode 100644 index 00000000..0b5a40d1 --- /dev/null +++ b/providers/azure/services/managedredis/reservation_unit_price_test.go @@ -0,0 +1,46 @@ +package managedredis + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + azrecs "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/recommendations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" +) + +func TestClient_ReservationUnitPrice(t *testing.T) { + t.Parallel() + client := NewClientWithHTTP(nil, "sub", "eastus", &mocks.PricingHTTP{Items: []map[string]any{ + mocks.ReservationRow("Redis Cache", "Azure Redis Cache Premium", "eastus", "Azure_Redis_Cache_Premium_P1_Cache", "P1", "P1 Cache Instance", "1 Year", 500), + }}) + + got, err := client.ReservationUnitPrice(context.Background(), "Premium_P1", "eastus", 1) + + require.NoError(t, err) + assert.Equal(t, azrecs.ReservationPrice{Total: 500, Currency: "USD"}, got) +} + +func TestClient_ReservationUnitPrice_UnknownPriceFailsLoudly(t *testing.T) { + t.Parallel() + h := &mocks.PricingHTTP{} + client := NewClientWithHTTP(nil, "sub", "eastus", h) + + got, err := client.ReservationUnitPrice(context.Background(), "Premium_P1", "eastus", 1) + + require.Error(t, err) + assert.Equal(t, azrecs.ReservationPrice{}, got) +} + +func TestClient_ReservationUnitPrice_UnsupportedSKUErrorsBeforeRequest(t *testing.T) { + t.Parallel() + h := &mocks.PricingHTTP{} + client := NewClientWithHTTP(nil, "sub", "eastus", h) + + _, err := client.ReservationUnitPrice(context.Background(), "Azure_Managed_Redis_Balanced_B0", "eastus", 1) + + require.Error(t, err) + assert.Zero(t, h.Calls()) +} From 087e9a35dd8912b1487520c92c029ed52a66f23b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 07:30:32 +0100 Subject: [PATCH 02/12] fix(azure): collect and price Redis through managedredis (ServiceMemoryDB) Azure Redis recommendations were labeled ServiceCache, a service whose client is about to stop buying Redis. Swap the single Redis producer in the aggregate and Advisor paths from the cache client to managedredis, so every Redis recommendation carries ServiceMemoryDB: - the seam newCacheClientFn becomes newManagedRedisClientFn - the Redis service filter follows ServiceMemoryDB - the Advisor rediscache mapping resolves to memorydb, so coverage by Consumption rows keeps matching on the same service label - Advisor rows are priced through managedredis.ReservationUnitPrice A guard test asserts no returned recommendation has Service ServiceCache. Refs #692 Co-Authored-By: Claude Opus 5.5 --- providers/azure/recommendations.go | 38 ++--- .../azure/recommendations_advisor_pricing.go | 8 +- .../recommendations_advisor_pricing_test.go | 2 +- .../recommendations_redis_routing_test.go | 135 ++++++++++++++++++ providers/azure/recommendations_test.go | 40 +++--- 5 files changed, 179 insertions(+), 44 deletions(-) create mode 100644 providers/azure/recommendations_redis_routing_test.go diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index 52268474..441f9861 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -19,23 +19,23 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" azrecs "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/recommendations" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/slot" - "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/cache" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/cosmosdb" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/database" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/managedredis" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/savingsplans" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/synapse" ) // serviceRecsGetter is the narrow interface satisfied by each per-service -// client (compute, database, cache, cosmosdb). The interface exists solely to +// client (compute, database, managedredis, cosmosdb). The interface exists solely to // allow tests to substitute fake implementations; production code uses the // concrete types via the newXxxClientFn variables below. type serviceRecsGetter interface { GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) } -// newComputeClientFn, newDatabaseClientFn, newCacheClientFn, +// newComputeClientFn, newDatabaseClientFn, newManagedRedisClientFn, // newCosmosDBClientFn, and newSavingsPlansClientFn default to the real // constructors and are overridden in tests to inject fakes. The variables are // package-level (not fields on the adapter) so that the constructor signature @@ -53,8 +53,8 @@ var ( newDatabaseClientFn func(azcore.TokenCredential, string, string) serviceRecsGetter = func(cred azcore.TokenCredential, sub, region string) serviceRecsGetter { return database.NewClient(cred, sub, region) } - newCacheClientFn func(azcore.TokenCredential, string, string) serviceRecsGetter = func(cred azcore.TokenCredential, sub, region string) serviceRecsGetter { - return cache.NewClient(cred, sub, region) + newManagedRedisClientFn func(azcore.TokenCredential, string, string) serviceRecsGetter = func(cred azcore.TokenCredential, sub, region string) serviceRecsGetter { + return managedredis.NewClient(cred, sub, region) } newCosmosDBClientFn func(azcore.TokenCredential, string, string) serviceRecsGetter = func(cred azcore.TokenCredential, sub, region string) serviceRecsGetter { return cosmosdb.NewClient(cred, sub, region) @@ -113,14 +113,14 @@ func NewRecommendationsClientAdapter(cred azcore.TokenCredential, subscriptionID // All seven service calls run concurrently under errgroup. Each goroutine captures // its own error and returns nil to the group so that a single service failure // does not cancel sibling calls. Results are appended in a deterministic order -// (compute → database → cache → cosmosdb → synapse → savingsplans → advisor) after all goroutines finish. +// (compute → database → redis → cosmosdb → synapse → savingsplans → advisor) after all goroutines finish. func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { if params == nil { return nil, fmt.Errorf("params cannot be nil") } var ( - computeRecs, dbRecs, cacheRecs, cosmosRecs, advisorRecs, spRecs, synapseRecs []common.Recommendation - computeErr, dbErr, cacheErr, cosmosErr, advisorErr, spErr, synapseErr error + computeRecs, dbRecs, redisRecs, cosmosRecs, advisorRecs, spRecs, synapseRecs []common.Recommendation + computeErr, dbErr, redisErr, cosmosErr, advisorErr, spErr, synapseErr error ) ctx, skips := azrecs.WithSkipLog(ctx) @@ -144,7 +144,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // recommendations" when applying the all-attempted-failed guard. includeCompute := shouldIncludeService(*params, common.ServiceCompute) includeDB := shouldIncludeService(*params, common.ServiceRelationalDB) - includeCache := shouldIncludeService(*params, common.ServiceCache) + includeRedis := shouldIncludeService(*params, common.ServiceMemoryDB) includeCosmos := shouldIncludeService(*params, common.ServiceNoSQL) includeSP := shouldIncludeService(*params, common.ServiceSavingsPlansAll) includeSynapse := shouldIncludeService(*params, common.ServiceDataWarehouse) @@ -165,11 +165,11 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p }) } - // Cache (Redis) recommendations — subscription-wide. - if includeCache { + // Redis recommendations (labeled memorydb) — subscription-wide. + if includeRedis { goService(func() { - cacheClient := newCacheClientFn(r.cred, r.subscriptionID, "") - cacheRecs, cacheErr = cacheClient.GetRecommendations(gctx, params) + redisClient := newManagedRedisClientFn(r.cred, r.subscriptionID, "") + redisRecs, redisErr = redisClient.GetRecommendations(gctx, params) }) } @@ -181,8 +181,8 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p }) } - // Synapse (SqlDataWarehouse) recommendations — subscription-wide. Search and - // Managed Redis are deliberately not collected here: see #47. + // Synapse (SqlDataWarehouse) recommendations — subscription-wide. Search is + // deliberately not collected here: see #47. Redis is collected above. if includeSynapse { goService(func() { synapseRecs, synapseErr = newSynapseClientFn(r.cred, r.subscriptionID, "").GetRecommendations(gctx, params) @@ -224,7 +224,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p return nil, err } - consumptionRIs := slices.Concat(okRecs(computeRecs, computeErr), okRecs(dbRecs, dbErr), okRecs(cacheRecs, cacheErr)) + consumptionRIs := slices.Concat(okRecs(computeRecs, computeErr), okRecs(dbRecs, dbErr), okRecs(redisRecs, redisErr)) // Priced with the parent ctx: gctx is canceled once Wait returns. advisorRecs = r.priceAdvisor(ctx, dropCoveredAdvisorReservations(dropCoveredAdvisorSavingsPlans(advisorRecs, spRecs), consumptionRIs), advisorErr) if err := ctx.Err(); err != nil { @@ -232,7 +232,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p } merged, err := mergeServiceResults(serviceResult{err: computeErr, name: "compute", recs: computeRecs, attempted: includeCompute}, serviceResult{err: dbErr, name: "database", recs: dbRecs, attempted: includeDB}, - serviceResult{err: cacheErr, name: "cache", recs: cacheRecs, attempted: includeCache}, + serviceResult{err: redisErr, name: "managedredis", recs: redisRecs, attempted: includeRedis}, serviceResult{err: cosmosErr, name: "cosmosdb", recs: cosmosRecs, attempted: includeCosmos}, serviceResult{err: synapseErr, name: "synapse", recs: synapseRecs, attempted: includeSynapse}, // The savingsplans client is a stub that unconditionally returns @@ -400,7 +400,7 @@ type serviceResult struct { // mergeServiceResults logs per-service errors (matches the previous sequential // behavior where each error was logged inline via logging.Warnf) and appends // successful results in the order the slice is passed — callers must preserve -// the canonical compute → database → cache → cosmosdb → synapse → savingsplans → advisor +// the canonical compute → database → redis → cosmosdb → synapse → savingsplans → advisor // order so that order-sensitive consumers remain stable. The advisor entry's // error is logged via logging.Errorf to match the pre-parallelisation severity. // @@ -753,7 +753,7 @@ func serviceFromExtendedProperties(ext map[string]*string) string { case "sqldatabases": return string(common.ServiceRelationalDB) case "rediscache": - return string(common.ServiceCache) + return string(common.ServiceMemoryDB) } } if subcat, ok := ext["recommendationSubCategory"]; ok && subcat != nil { diff --git a/providers/azure/recommendations_advisor_pricing.go b/providers/azure/recommendations_advisor_pricing.go index 59cf990f..f0d89d4a 100644 --- a/providers/azure/recommendations_advisor_pricing.go +++ b/providers/azure/recommendations_advisor_pricing.go @@ -8,12 +8,12 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" azrecs "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/recommendations" - "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/cache" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/database" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/managedredis" ) -// reservationUnitPricer is the price lookup the compute, database and cache +// reservationUnitPricer is the price lookup the compute, database and Redis // clients export for pricing Advisor rows. type reservationUnitPricer interface { ReservationUnitPrice(ctx context.Context, sku, region string, termYears int) (azrecs.ReservationPrice, error) @@ -22,7 +22,7 @@ type reservationUnitPricer interface { var ( _ reservationUnitPricer = (*compute.Client)(nil) _ reservationUnitPricer = (*database.Client)(nil) - _ reservationUnitPricer = (*cache.Client)(nil) + _ reservationUnitPricer = (*managedredis.Client)(nil) ) // priceAdvisor prices the Advisor reservation rows with the service clients' @@ -36,7 +36,7 @@ func (r *RecommendationsClientAdapter) priceAdvisor(ctx context.Context, rows [] clients := map[common.ServiceType]serviceRecsGetter{ common.ServiceCompute: newComputeClientFn(r.cred, r.subscriptionID, ""), common.ServiceRelationalDB: newDatabaseClientFn(r.cred, r.subscriptionID, ""), - common.ServiceCache: newCacheClientFn(r.cred, r.subscriptionID, ""), + common.ServiceMemoryDB: newManagedRedisClientFn(r.cred, r.subscriptionID, ""), } lookups := make(map[common.ServiceType]azrecs.PriceFunc, len(clients)) for service, client := range clients { diff --git a/providers/azure/recommendations_advisor_pricing_test.go b/providers/azure/recommendations_advisor_pricing_test.go index fd58a150..bbbff218 100644 --- a/providers/azure/recommendations_advisor_pricing_test.go +++ b/providers/azure/recommendations_advisor_pricing_test.go @@ -97,7 +97,7 @@ func TestPriceAdvisorReservations_PassesNonReservationsThrough(t *testing.T) { func TestPriceAdvisorReservations_ServiceWithoutPricerIsSkipped(t *testing.T) { t.Parallel() ctx, skips := azrecs.WithSkipLog(context.Background()) - got := priceAdvisorReservations(ctx, []common.Recommendation{riRow(common.ServiceCache, "eastus", "1yr", "Premium_P1", 1, "")}, nil) + got := priceAdvisorReservations(ctx, []common.Recommendation{riRow(common.ServiceMemoryDB, "eastus", "1yr", "Premium_P1", 1, "")}, nil) assert.Empty(t, got) require.Len(t, skips.Entries(), 1) assert.Contains(t, skips.Entries()[0], "no reservation pricer for service") diff --git a/providers/azure/recommendations_redis_routing_test.go b/providers/azure/recommendations_redis_routing_test.go new file mode 100644 index 00000000..ce2f688d --- /dev/null +++ b/providers/azure/recommendations_redis_routing_test.go @@ -0,0 +1,135 @@ +package azure + +import ( + "context" + "slices" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/managedredis" +) + +func TestServiceFromExtendedProperties_RedisCacheIsMemoryDB(t *testing.T) { + t.Parallel() + got := serviceFromExtendedProperties(map[string]*string{"reservedResourceType": strPtr("RedisCache")}) + assert.Equal(t, string(common.ServiceMemoryDB), got) +} + +func redisRetailRows() []map[string]any { + row := func(region, term string, price float64) map[string]any { + return mocks.ReservationRow("Redis Cache", "Azure Redis Cache Premium", region, "Azure_Redis_Cache_Premium_P1_Cache", "P1", "P1 Cache Instance", term, price) + } + return []map[string]any{row("eastus", "1 Year", 1100), row("eastus", "3 Years", 2600), row("westus", "1 Year", 900)} +} + +// realRedisClient is the production managedredis client over a Consumption +// RedisCache payload and a stub Retail Prices catalog. +func realRedisClient() *managedredis.Client { + c := managedredis.NewClientWithHTTP(nil, "sub", "", &mocks.PricingHTTP{Items: redisRetailRows()}) + c.SetRecommendationsPager(&mocks.MockRecommendationsPager{Results: mocks.ConsumptionUnitFixtures("Premium_P1", "eastus"), HasMore: true}) + return c +} + +func redisAdapter(t *testing.T, redis serviceRecsGetter, advisor func(*RecommendationsClientAdapter) []common.Recommendation) *RecommendationsClientAdapter { + t.Helper() + overrideServiceClientFns(t, &fakeServiceClient{}, &fakeServiceClient{}, redis, &fakeServiceClient{}, &fakeServiceClient{}) + a := &RecommendationsClientAdapter{cred: &mockAzureTokenCredential{}, subscriptionID: "sub"} + a.getAdvisorRecsFn = func(context.Context, common.RecommendationParams) ([]common.Recommendation, error) { + if advisor == nil { + return nil, nil + } + return advisor(a), nil + } + return a +} + +func TestGetRecommendations_RedisCollectedViaManagedRedisOnly(t *testing.T) { + recs, err := redisAdapter(t, realRedisClient(), nil).GetRecommendations(context.Background(), &common.RecommendationParams{}) + + require.NoError(t, err) + require.Len(t, recs, 4, "one Consumption payload of two recommendations expands to four variants, listed once") + for _, rec := range recs { + assert.Equal(t, common.ServiceMemoryDB, rec.Service) + } +} + +func TestGetRecommendations_RedisFilterFollowsMemoryDB(t *testing.T) { + for _, tc := range []struct { + service common.ServiceType + calls int32 + }{{common.ServiceMemoryDB, 1}, {common.ServiceCache, 0}} { + redis := &countingFake{} + _, err := redisAdapter(t, redis, nil).GetRecommendations(context.Background(), &common.RecommendationParams{Service: tc.service}) + + require.NoError(t, err) + assert.Equal(t, tc.calls, redis.calls.Load(), "params.Service=%s", tc.service) + } +} + +type countingFake struct { + fakeServiceClient + calls atomic.Int32 +} + +func (f *countingFake) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + f.calls.Add(1) + return f.fakeServiceClient.GetRecommendations(ctx, p) +} + +func redisAdvisorRow(a *RecommendationsClientAdapter, region string) common.Recommendation { + ext := reservationExt(map[string]*string{"reservedResourceType": strPtr("RedisCache"), "sku": strPtr("Premium_P1"), "term": strPtr("P1Y"), "region": strPtr(region)}) + return *a.convertAdvisorRecommendation(context.Background(), advisorRec(subscriptionScope, ext)) +} + +func TestGetRecommendations_AdvisorRedisIsMemoryDBAndPricedThroughIt(t *testing.T) { + advisor := func(a *RecommendationsClientAdapter) []common.Recommendation { + return []common.Recommendation{redisAdvisorRow(a, "westus")} + } + // Only the Advisor row: the Consumption pager is empty, so nothing covers it. + redis := managedredis.NewClientWithHTTP(nil, "sub", "", &mocks.PricingHTTP{Items: redisRetailRows()}) + redis.SetRecommendationsPager(&mocks.MockRecommendationsPager{HasMore: true}) + + for _, params := range []common.RecommendationParams{{}, {Service: common.ServiceMemoryDB}} { + recs, err := redisAdapter(t, redis, advisor).GetRecommendations(context.Background(), ¶ms) + + require.NoError(t, err) + require.Len(t, recs, 2, "upfront and monthly legs of the priced row") + for _, rec := range recs { + assert.Equal(t, common.ServiceMemoryDB, rec.Service) + } + assert.True(t, slices.ContainsFunc(recs, func(r common.Recommendation) bool { return r.CommitmentCost == 1800 }), "2 units x 900 from the managedredis price lookup") + } +} + +func TestGetRecommendations_AdvisorRedisCoveredByConsumptionRedis(t *testing.T) { + // Same account, region and term as the Consumption payload (eastus, 1yr, 3yr). + advisor := func(a *RecommendationsClientAdapter) []common.Recommendation { + return []common.Recommendation{redisAdvisorRow(a, "eastus")} + } + + recs, err := redisAdapter(t, realRedisClient(), advisor).GetRecommendations(context.Background(), &common.RecommendationParams{}) + + require.NoError(t, err) + assert.Len(t, recs, 4, "the Advisor row is dropped: Consumption already covers that need") +} + +// Tripwire: no Redis source may relabel recommendations as ServiceCache, whose +// client is going to refuse Redis purchases. +func TestGetRecommendations_NoRecommendationIsLabelledServiceCache(t *testing.T) { + advisor := func(a *RecommendationsClientAdapter) []common.Recommendation { + return []common.Recommendation{redisAdvisorRow(a, "westus")} + } + + recs, err := redisAdapter(t, realRedisClient(), advisor).GetRecommendations(context.Background(), &common.RecommendationParams{}) + + require.NoError(t, err) + require.Len(t, recs, 6, "four Consumption variants plus the two legs of the uncovered westus Advisor row") + for _, rec := range recs { + assert.NotEqual(t, common.ServiceCache, rec.Service) + } +} diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index fbf9157e..391cfdc5 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -204,11 +204,11 @@ func TestRecommendationsClientAdapter_Fields(t *testing.T) { // given fakes and restores the originals on cleanup, so adapter tests never // build real ARM clients (which would issue live requests to // management.azure.com). -func overrideServiceClientFns(t *testing.T, compute, database, cache, cosmos, sp serviceRecsGetter) { +func overrideServiceClientFns(t *testing.T, compute, database, redis, cosmos, sp serviceRecsGetter) { t.Helper() origCompute := newComputeClientFn origDatabase := newDatabaseClientFn - origCache := newCacheClientFn + origRedis := newManagedRedisClientFn origCosmos := newCosmosDBClientFn origSP := newSavingsPlansClientFn origSynapse := newSynapseClientFn @@ -216,13 +216,13 @@ func overrideServiceClientFns(t *testing.T, compute, database, cache, cosmos, sp newSynapseClientFn = origSynapse newComputeClientFn = origCompute newDatabaseClientFn = origDatabase - newCacheClientFn = origCache + newManagedRedisClientFn = origRedis newCosmosDBClientFn = origCosmos newSavingsPlansClientFn = origSP }) newComputeClientFn = newFakeFn(compute) newDatabaseClientFn = newFakeFn(database) - newCacheClientFn = newFakeFn(cache) + newManagedRedisClientFn = newFakeFn(redis) newCosmosDBClientFn = newFakeFn(cosmos) newSavingsPlansClientFn = newFakeFn(sp) newSynapseClientFn = newFakeFn(&fakeServiceClient{}) @@ -253,7 +253,7 @@ func TestRecommendationsClientAdapter_GetAllRecommendations(t *testing.T) { overrideServiceClientFns(t, &fakeServiceClient{recs: []common.Recommendation{rec(common.ServiceCompute)}}, &fakeServiceClient{recs: []common.Recommendation{rec(common.ServiceRelationalDB)}}, - &fakeServiceClient{recs: []common.Recommendation{rec(common.ServiceCache)}}, + &fakeServiceClient{recs: []common.Recommendation{rec(common.ServiceMemoryDB)}}, &fakeServiceClient{recs: []common.Recommendation{rec(common.ServiceNoSQL)}}, &fakeServiceClient{}) @@ -268,7 +268,7 @@ func TestRecommendationsClientAdapter_GetAllRecommendations(t *testing.T) { require.NoError(t, err) assert.ElementsMatch(t, []common.Recommendation{ rec(common.ServiceCompute), rec(common.ServiceRelationalDB), - rec(common.ServiceCache), rec(common.ServiceNoSQL), + rec(common.ServiceMemoryDB), rec(common.ServiceNoSQL), }, recs) } @@ -419,7 +419,7 @@ func TestMergeServiceResults_OrderIsStable(t *testing.T) { computeRec := mkRec(common.ServiceCompute, "compute") dbRec := mkRec(common.ServiceRelationalDB, "database") - cacheRec := mkRec(common.ServiceCache, "cache") + redisRec := mkRec(common.ServiceMemoryDB, "redis") cosmosRec := mkRec(common.ServiceNoSQL, "cosmosdb") spRec := mkRec(common.ServiceSavingsPlansAll, "savingsplans") advisorRec := mkRec(common.ServiceCompute, "advisor") // Advisor produces Compute recs @@ -428,7 +428,7 @@ func TestMergeServiceResults_OrderIsStable(t *testing.T) { result, err := mergeServiceResults( serviceResult{err: nil, name: "compute", recs: []common.Recommendation{computeRec}, attempted: true}, serviceResult{err: nil, name: "database", recs: []common.Recommendation{dbRec}, attempted: true}, - serviceResult{err: nil, name: "cache", recs: []common.Recommendation{cacheRec}, attempted: true}, + serviceResult{err: nil, name: "managedredis", recs: []common.Recommendation{redisRec}, attempted: true}, serviceResult{err: nil, name: "cosmosdb", recs: []common.Recommendation{cosmosRec}, attempted: true}, serviceResult{err: nil, name: "savingsplans", recs: []common.Recommendation{spRec}, attempted: true}, serviceResult{err: nil, name: "advisor", recs: []common.Recommendation{advisorRec}, attempted: true}, @@ -438,7 +438,7 @@ func TestMergeServiceResults_OrderIsStable(t *testing.T) { require.Len(t, result, 6, "all six services must be represented") assert.Equal(t, "compute", result[0].ResourceType, "first must be compute") assert.Equal(t, "database", result[1].ResourceType, "second must be database") - assert.Equal(t, "cache", result[2].ResourceType, "third must be cache") + assert.Equal(t, "redis", result[2].ResourceType, "third must be redis") assert.Equal(t, "cosmosdb", result[3].ResourceType, "fourth must be cosmosdb") assert.Equal(t, "savingsplans", result[4].ResourceType, "fifth must be savingsplans") assert.Equal(t, "advisor", result[5].ResourceType, "sixth must be advisor (compute-type)") @@ -596,14 +596,14 @@ const fakeServiceSleep = 100 * time.Millisecond func TestGetRecommendations_Parallelism(t *testing.T) { origCompute := newComputeClientFn origDatabase := newDatabaseClientFn - origCache := newCacheClientFn + origRedis := newManagedRedisClientFn origCosmos := newCosmosDBClientFn origSynapse := newSynapseClientFn origSP := newSavingsPlansClientFn t.Cleanup(func() { newComputeClientFn = origCompute newDatabaseClientFn = origDatabase - newCacheClientFn = origCache + newManagedRedisClientFn = origRedis newCosmosDBClientFn = origCosmos newSavingsPlansClientFn = origSP newSynapseClientFn = origSynapse @@ -616,7 +616,7 @@ func TestGetRecommendations_Parallelism(t *testing.T) { noopFake := newFakeFn(&fakeServiceClient{}) newComputeClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{rec(common.ServiceCompute)}}) newDatabaseClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{rec(common.ServiceRelationalDB)}}) - newCacheClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{rec(common.ServiceCache)}}) + newManagedRedisClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{rec(common.ServiceMemoryDB)}}) newCosmosDBClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{rec(common.ServiceNoSQL)}}) newSavingsPlansClientFn = noopFake newSynapseClientFn = newFakeFn(&fakeServiceClient{}) @@ -650,14 +650,14 @@ func TestGetRecommendations_Parallelism(t *testing.T) { func TestGetRecommendations_OrderPreservation(t *testing.T) { origCompute := newComputeClientFn origDatabase := newDatabaseClientFn - origCache := newCacheClientFn + origRedis := newManagedRedisClientFn origCosmos := newCosmosDBClientFn origSynapse := newSynapseClientFn origSP := newSavingsPlansClientFn t.Cleanup(func() { newComputeClientFn = origCompute newDatabaseClientFn = origDatabase - newCacheClientFn = origCache + newManagedRedisClientFn = origRedis newCosmosDBClientFn = origCosmos newSavingsPlansClientFn = origSP newSynapseClientFn = origSynapse @@ -672,7 +672,7 @@ func TestGetRecommendations_OrderPreservation(t *testing.T) { // still reflect the canonical order. newComputeClientFn = newFakeFn(&fakeServiceClient{sleepDur: 40 * time.Millisecond, recs: []common.Recommendation{makeRec(common.ServiceCompute)}}) newDatabaseClientFn = newFakeFn(&fakeServiceClient{sleepDur: 30 * time.Millisecond, recs: []common.Recommendation{makeRec(common.ServiceRelationalDB)}}) - newCacheClientFn = newFakeFn(&fakeServiceClient{sleepDur: 20 * time.Millisecond, recs: []common.Recommendation{makeRec(common.ServiceCache)}}) + newManagedRedisClientFn = newFakeFn(&fakeServiceClient{sleepDur: 20 * time.Millisecond, recs: []common.Recommendation{makeRec(common.ServiceMemoryDB)}}) newCosmosDBClientFn = newFakeFn(&fakeServiceClient{sleepDur: 10 * time.Millisecond, recs: []common.Recommendation{makeRec(common.ServiceNoSQL)}}) newSavingsPlansClientFn = newFakeFn(&fakeServiceClient{}) newSynapseClientFn = newFakeFn(&fakeServiceClient{}) @@ -691,7 +691,7 @@ func TestGetRecommendations_OrderPreservation(t *testing.T) { require.Len(t, recs, 4, "expected one rec per injectable service") assert.Equal(t, common.ServiceCompute, recs[0].Service, "slot 0 must be compute") assert.Equal(t, common.ServiceRelationalDB, recs[1].Service, "slot 1 must be database") - assert.Equal(t, common.ServiceCache, recs[2].Service, "slot 2 must be cache") + assert.Equal(t, common.ServiceMemoryDB, recs[2].Service, "slot 2 must be redis") assert.Equal(t, common.ServiceNoSQL, recs[3].Service, "slot 3 must be cosmosdb") } @@ -704,14 +704,14 @@ func TestGetRecommendations_OrderPreservation(t *testing.T) { func TestGetRecommendations_ErrorIsolation(t *testing.T) { origCompute := newComputeClientFn origDatabase := newDatabaseClientFn - origCache := newCacheClientFn + origRedis := newManagedRedisClientFn origCosmos := newCosmosDBClientFn origSynapse := newSynapseClientFn origSP := newSavingsPlansClientFn t.Cleanup(func() { newComputeClientFn = origCompute newDatabaseClientFn = origDatabase - newCacheClientFn = origCache + newManagedRedisClientFn = origRedis newCosmosDBClientFn = origCosmos newSavingsPlansClientFn = origSP newSynapseClientFn = origSynapse @@ -724,7 +724,7 @@ func TestGetRecommendations_ErrorIsolation(t *testing.T) { // database returns an error; the other three must still contribute recs. newComputeClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{makeRec(common.ServiceCompute)}}) newDatabaseClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, err: errors.New("db unavailable")}) - newCacheClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{makeRec(common.ServiceCache)}}) + newManagedRedisClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{makeRec(common.ServiceMemoryDB)}}) newCosmosDBClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{makeRec(common.ServiceNoSQL)}}) newSavingsPlansClientFn = newFakeFn(&fakeServiceClient{}) newSynapseClientFn = newFakeFn(&fakeServiceClient{}) @@ -744,7 +744,7 @@ func TestGetRecommendations_ErrorIsolation(t *testing.T) { services[i] = r.Service } assert.Contains(t, services, common.ServiceCompute, "compute recs must be present despite db error") - assert.Contains(t, services, common.ServiceCache, "cache recs must be present despite db error") + assert.Contains(t, services, common.ServiceMemoryDB, "redis recs must be present despite db error") assert.Contains(t, services, common.ServiceNoSQL, "cosmosdb recs must be present despite db error") assert.NotContains(t, services, common.ServiceRelationalDB, "db recs must be absent when db errors") } From c3ff6d4510bf39ebf37d05092bd5666ef4fa285f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 14:33:47 +0100 Subject: [PATCH 03/12] refactor(azure/cache): stop emitting Redis recommendations (#692) managedredis is now the only producer of Azure Redis recommendations, so cache GetRecommendations becomes a documented no-op. The Consumption pager, converter, appendPage and their tests go with it. The catalogue and ReservationUnitPrice removal follows in the next commit. Co-Authored-By: Claude Opus 5.5 --- providers/azure/services/cache/client.go | 109 +---------- providers/azure/services/cache/client_test.go | 183 ------------------ .../services/cache/consumption_units_test.go | 50 ----- .../cache/noop_recommendations_test.go | 36 ++++ .../azure/services/cache/slot_wiring_test.go | 6 - 5 files changed, 42 insertions(+), 342 deletions(-) delete mode 100644 providers/azure/services/cache/consumption_units_test.go create mode 100644 providers/azure/services/cache/noop_recommendations_test.go diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 15650282..4ee4c62e 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -24,9 +24,6 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" ) -// maxRecsPages caps Consumption API recommendation pagination. -const maxRecsPages = 10 - // maxReservationsPages caps reservation-detail pagination. const maxReservationsPages = 50 @@ -151,65 +148,12 @@ func (c *Client) GetRegion() string { // AzureRetailPrice is the response envelope for the Azure Retail Prices API. type AzureRetailPrice = pricing.Page[pricing.RetailPriceItem] -// GetRecommendations gets Redis Cache reservation recommendations from Azure Consumption API. -func (c *Client) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { - recommendations := make([]common.Recommendation, 0) - pricer := azrecs.NewPricer("cache", c.ReservationUnitPrice) - - // Use injected pager if available (for testing) - var pager RecommendationsPager - if c.recommendationsPager != nil { - pager = c.recommendationsPager - } else { - client, err := armconsumption.NewReservationRecommendationsClient(c.cred, nil) - if err != nil { - return nil, fmt.Errorf("failed to create consumption client: %w", err) - } - // NewListPager's first argument is the billing scope, NOT the - // filter — see the parallel comment in compute/client.go for the - // failure mode that the wrong shape produced. - scope := fmt.Sprintf("/subscriptions/%s", c.subscriptionID) - filter := azrecs.ConsumptionFilter("RedisCache") - pager = client.NewListPager(scope, &armconsumption.ReservationRecommendationsClientListOptions{Filter: &filter}) - } - - for pageIdx := 0; pager.More(); pageIdx++ { - if err := ctx.Err(); err != nil { - return nil, fmt.Errorf("context canceled during pagination: %w", err) - } - if pageIdx >= maxRecsPages { - return nil, fmt.Errorf("cache: GetRecommendations pagination cap (%d pages) reached", maxRecsPages) - } - page, err := slot.NextPage(ctx, pager) - if err != nil { - return nil, fmt.Errorf("failed to get Redis Cache recommendations: %w", err) - } - - recommendations, err = c.appendPage(ctx, pricer, recommendations, page.Value) - if err != nil { - return recommendations, err - } - } - - return recommendations, nil -} - -// appendPage converts one page of recommendations and appends their priced -// variants. It is split out of GetRecommendations to keep that function under -// the cyclomatic limit. -func (c *Client) appendPage(ctx context.Context, pricer *azrecs.Pricer, recs []common.Recommendation, page []armconsumption.ReservationRecommendationClassification) ([]common.Recommendation, error) { - for _, rec := range page { - converted := c.convertAzureRedisRecommendation(ctx, rec) - if converted == nil { - continue - } - var err error - recs, err = azrecs.AppendConsumptionVariants(ctx, "cache", recs, *converted, pricer) - if err != nil { - return recs, err - } - } - return recs, nil +// GetRecommendations returns an empty slice: Azure Redis recommendations are +// collected, labeled ServiceMemoryDB, by the managedredis client. Collecting +// them here too would list every Redis recommendation twice, and a +// ServiceCache label routes the purchase to a client that must not buy Redis. +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { + return []common.Recommendation{}, nil } // GetExistingCommitments retrieves existing Redis Cache reserved capacity. @@ -598,47 +542,6 @@ func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYe }, nil } -// convertAzureRedisRecommendation converts Azure Redis Cache reservation recommendation to common format. -// See providers/azure/internal/recommendations.Extract for the shared -// SDK-to-struct ladder. Returns nil when the SDK payload is unusable. -// -// Details populated by parsing the SKU string into Engine ("redis") and -// NodeType, then enriched from the lazily-cached armredis catalog -// (cachedSKULookup). Shards is sourced from the catalog when an -// existing cache in the subscription matches the recommendation's -// Premium-tier SKU; otherwise stays 0 (zero means "unknown", not -// "definitely zero shards" — see the redisSKUEntry godoc). -func (c *Client) convertAzureRedisRecommendation(ctx context.Context, azureRec armconsumption.ReservationRecommendationClassification) *common.Recommendation { - f := azrecs.ExtractConsumptionOrSkip(ctx, "cache", azureRec) - if f == nil { - return nil - } - details := &common.CacheDetails{ - Engine: "redis", - NodeType: f.ResourceType, - } - if entry, ok := c.cachedSKULookup(ctx, f.ResourceType); ok && entry.shardCount > 0 { - details.Shards = entry.shardCount - } - return &common.Recommendation{ - Provider: common.ProviderAzure, - Service: common.ServiceCache, - Account: c.subscriptionID, - Region: f.Region, - ResourceType: f.ResourceType, - Count: f.Count, - OnDemandCost: f.OnDemandCost, - CommitmentCost: f.CommitmentCost, - EstimatedSavings: f.EstimatedSavings, - RecurringMonthlyCost: f.RecurringMonthlyCost, - CommitmentType: common.CommitmentReservedInstance, - Term: f.Term, - PaymentOption: "upfront", - Timestamp: time.Now(), - Details: details, - } -} - // cachedSKULookup returns the SKU catalog entry for skuName, fetching // the catalog lazily on first call. The catalog is fetched ONCE per // client lifetime via armredis.Client.NewListBySubscriptionPager; diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index 29941057..6ad26306 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -487,57 +487,6 @@ func TestCacheClient_ValidateOffering_CaseInsensitive(t *testing.T) { }) } -func TestCacheClient_GetRecommendations_WithMockPager(t *testing.T) { - t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - // Create mock pager with empty results - mockPager := &MockRecommendationsPager{ - pages: []armconsumption.ReservationRecommendationsClientListResponse{ - { - ReservationRecommendationsListResult: armconsumption.ReservationRecommendationsListResult{ - Value: []armconsumption.ReservationRecommendationClassification{}, - }, - }, - }, - } - - client.SetRecommendationsPager(mockPager) - - recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) - require.NoError(t, err) - assert.Empty(t, recs) -} - -func TestCacheClient_GetRecommendations_MultiplePages(t *testing.T) { - t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - // Create mock pager with multiple pages - mockPager := &MockRecommendationsPager{ - pages: []armconsumption.ReservationRecommendationsClientListResponse{ - { - ReservationRecommendationsListResult: armconsumption.ReservationRecommendationsListResult{ - Value: []armconsumption.ReservationRecommendationClassification{}, - }, - }, - { - ReservationRecommendationsListResult: armconsumption.ReservationRecommendationsListResult{ - Value: []armconsumption.ReservationRecommendationClassification{}, - }, - }, - }, - } - - client.SetRecommendationsPager(mockPager) - - recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) - require.NoError(t, err) - assert.Empty(t, recs) -} - func TestCacheClient_GetExistingCommitments_WithMockPager(t *testing.T) { t.Parallel() ctx := context.Background() @@ -869,138 +818,6 @@ func TestCacheClient_GetCommonSKUs(t *testing.T) { assert.Contains(t, skus, "Premium_P5") } -// TestCacheClient_ConvertAzureRedisRecommendation_NilGuards pins the new -// contract: unusable SDK payloads produce a nil *Recommendation. -func TestCacheClient_ConvertAzureRedisRecommendation_NilGuards(t *testing.T) { - t.Parallel() - client := NewClient(nil, "test-subscription", "eastus") - assert.Nil(t, client.convertAzureRedisRecommendation(context.Background(), nil)) -} - -// TestCacheClient_ConvertAzureRedisRecommendation_PopulatesAllFields asserts -// the converter forwards every helper-extracted field + applies the -// Cache-service-specific constants. An empty SKU catalog (no caches in -// the subscription) is the "no signal" baseline — Shards stays 0. -func TestCacheClient_ConvertAzureRedisRecommendation_PopulatesAllFields(t *testing.T) { - t.Parallel() - client := NewClient(nil, "test-subscription", "eastus") - // Inject empty mock pager so the cachedSKULookup hits the lazy-fetch - // codepath without making a real Azure API call. - client.SetRedisCachesPager(&MockRedisCachesPager{}) - - rec := mocks.BuildLegacyReservationRecommendation( - mocks.WithRegion("eastus"), - mocks.WithTerm("P3Y"), - mocks.WithQuantity(1), - mocks.WithNormalizedSize("Premium_P3"), - mocks.WithCosts(50, 35, 15), - ) - out := client.convertAzureRedisRecommendation(context.Background(), rec) - require.NotNil(t, out) - assert.Equal(t, common.ProviderAzure, out.Provider) - assert.Equal(t, common.ServiceCache, out.Service) - assert.Equal(t, "test-subscription", out.Account) - assert.Equal(t, "eastus", out.Region) - assert.Equal(t, "Premium_P3", out.ResourceType) - assert.Equal(t, 1, out.Count) - assert.InDelta(t, 50.0*30.4375/7, out.OnDemandCost, 1e-9) - assert.Zero(t, out.CommitmentCost, "commitment is priced from the retail row at variant expansion") - assert.InDelta(t, 15.0*30.4375/7, out.EstimatedSavings, 1e-9) - assert.Equal(t, common.CommitmentReservedInstance, out.CommitmentType) - assert.Equal(t, "3yr", out.Term) - assert.Equal(t, "upfront", out.PaymentOption) - - // Details carries Engine=redis + NodeType from the SKU string. Shards - // stays 0 when no cache instance matches in the subscription (the - // catalogue's only signal source for shard counts). - require.NotNil(t, out.Details) - details, ok := out.Details.(*common.CacheDetails) - require.True(t, ok, "Details must be a *common.CacheDetails pointer") - assert.Equal(t, "redis", details.Engine) - assert.Equal(t, "Premium_P3", details.NodeType) - assert.Equal(t, 0, details.Shards, "Shards is 0 when no matching cache exists in the subscription") -} - -// TestCacheClient_ConvertAzureRedisRecommendation_PopulatesShardsFromSKUCache -// asserts the new batched-SKU-catalog lookup populates CacheDetails.Shards -// when the subscription has a Premium-tier clustered cache with the same -// SKU as the recommendation. Single ListBySubscription call per client -// lifetime feeds many converter calls; this test pins the contract. -func TestCacheClient_ConvertAzureRedisRecommendation_PopulatesShardsFromSKUCache(t *testing.T) { - t.Parallel() - client := NewClient(nil, "test-subscription", "eastus") - - premiumName := armredis.SKUNamePremium - familyP := armredis.SKUFamilyP - capacity := int32(3) - shardCount := int32(5) - - // One Premium_P3 cache with 5 shards in the subscription. The - // converter will see f.ResourceType="Premium_P3" and look it up. - mockPager := &MockRedisCachesPager{ - pages: []armredis.ClientListBySubscriptionResponse{ - { - ListResult: armredis.ListResult{ - Value: []*armredis.ResourceInfo{ - { - Properties: &armredis.Properties{ - SKU: &armredis.SKU{ - Name: &premiumName, - Family: &familyP, - Capacity: &capacity, - }, - ShardCount: &shardCount, - }, - }, - }, - }, - }, - }, - } - client.SetRedisCachesPager(mockPager) - - rec := mocks.BuildLegacyReservationRecommendation( - mocks.WithRegion("eastus"), - mocks.WithNormalizedSize("Premium_P3"), - mocks.WithCosts(100, 70, 30), - ) - out := client.convertAzureRedisRecommendation(context.Background(), rec) - require.NotNil(t, out) - details, ok := out.Details.(*common.CacheDetails) - require.True(t, ok) - assert.Equal(t, "redis", details.Engine) - assert.Equal(t, "Premium_P3", details.NodeType) - assert.Equal(t, 5, details.Shards, "Shards must be enriched from the cached SKU catalog") -} - -// TestCacheClient_ConvertAzureRedisRecommendation_PagerErrorFallsBack -// asserts that a SKU-catalog fetch failure does NOT fail the -// conversion — Shards just stays at 0 and the rest of Details is -// populated from the recommendation payload as before. This is the -// graceful-degradation contract the issue asks for. -func TestCacheClient_ConvertAzureRedisRecommendation_PagerErrorFallsBack(t *testing.T) { - t.Parallel() - client := NewClient(nil, "test-subscription", "eastus") - mockPager := &MockRedisCachesPager{ - pages: []armredis.ClientListBySubscriptionResponse{{}}, - err: errors.New("transient Azure API error"), - } - client.SetRedisCachesPager(mockPager) - - rec := mocks.BuildLegacyReservationRecommendation( - mocks.WithRegion("eastus"), - mocks.WithNormalizedSize("Premium_P3"), - mocks.WithCosts(100, 70, 30), - ) - out := client.convertAzureRedisRecommendation(context.Background(), rec) - require.NotNil(t, out, "conversion must NOT fail on catalog-fetch error") - details, ok := out.Details.(*common.CacheDetails) - require.True(t, ok) - assert.Equal(t, "redis", details.Engine) - assert.Equal(t, "Premium_P3", details.NodeType) - assert.Equal(t, 0, details.Shards, "Shards left at 0 when catalog fetch fails") -} - // TestCacheClient_CachedSKULookup_FetchedOnce pins the perf invariant: // many converter calls in the same GetRecommendations run trigger // exactly ONE catalog fetch. The mock pager counts pages served; the diff --git a/providers/azure/services/cache/consumption_units_test.go b/providers/azure/services/cache/consumption_units_test.go deleted file mode 100644 index aecf201d..00000000 --- a/providers/azure/services/cache/consumption_units_test.go +++ /dev/null @@ -1,50 +0,0 @@ -package cache - -import ( - "context" - "errors" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/LeanerCloud/cloud-commitments-go/pkg/common" - "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/pricing" - "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" -) - -// TestGetRecommendations_ConsumptionUnits pins the units of the Consumption -// path: monthly on-demand and savings from the lookback-window amounts, and a -// commitment of Count x the term-total reservation retail price (synthetic -// rows, not live quotes), for a 1y legacy and a 3y modern recommendation. -func TestGetRecommendations_ConsumptionUnits(t *testing.T) { - t.Parallel() - client := NewClientWithHTTP(nil, "sub", "eastus", &mocks.PricingHTTP{Items: []map[string]any{ - mocks.ReservationRow("Redis Cache", "Azure Redis Cache Premium", "eastus", "Azure_Redis_Cache_Premium_P1_Cache", "P1", "P1 Cache Instance", "1 Year", 1100), - mocks.ReservationRow("Redis Cache", "Azure Redis Cache Premium", "eastus", "Azure_Redis_Cache_Premium_P1_Cache", "P1", "P1 Cache Instance", "3 Years", 2600), - }}) - client.SetRedisCachesPager(&MockRedisCachesPager{}) - client.SetRecommendationsPager(&mocks.MockRecommendationsPager{ - Results: mocks.ConsumptionUnitFixtures("Premium_P1", "eastus"), - HasMore: true, - }) - - recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) - require.NoError(t, err) - mocks.AssertConsumptionUnitVariants(t, recs, 1100, 2600) -} - -// A retail price outage must fail the collection, not return a partial list. -func TestGetRecommendations_ConsumptionPriceFetchFailureFailsCollection(t *testing.T) { - t.Parallel() - client := NewClientWithHTTP(nil, "sub", "eastus", &mocks.PricingHTTP{Err: errors.New("connection reset")}) - client.SetRedisCachesPager(&MockRedisCachesPager{}) - client.SetRecommendationsPager(&mocks.MockRecommendationsPager{ - Results: mocks.ConsumptionUnitFixtures("Premium_P1", "eastus"), - HasMore: true, - }) - - _, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) - require.Error(t, err) - assert.ErrorIs(t, err, pricing.ErrFetch) -} diff --git a/providers/azure/services/cache/noop_recommendations_test.go b/providers/azure/services/cache/noop_recommendations_test.go new file mode 100644 index 00000000..786c5e35 --- /dev/null +++ b/providers/azure/services/cache/noop_recommendations_test.go @@ -0,0 +1,36 @@ +package cache + +import ( + "context" + "errors" + "sync/atomic" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +type countingCred struct{ tokens atomic.Int32 } + +func (c *countingCred) GetToken(context.Context, policy.TokenRequestOptions) (azcore.AccessToken, error) { + c.tokens.Add(1) + return azcore.AccessToken{}, errors.New("GetRecommendations must not call Azure") +} + +// Azure Redis recommendations are collected by managedredis and labeled +// ServiceMemoryDB. A ServiceCache-labeled Redis recommendation would route its +// purchase to this client, which must not buy Redis. +func TestGetRecommendations_NoOpNeverEmitsServiceCache(t *testing.T) { + t.Parallel() + cred := &countingCred{} + + recs, err := NewClient(cred, "sub", "eastus").GetRecommendations(context.Background(), &common.RecommendationParams{}) + + require.NoError(t, err) + assert.Equal(t, []common.Recommendation{}, recs) + assert.Zero(t, cred.tokens.Load(), "no request is made") +} diff --git a/providers/azure/services/cache/slot_wiring_test.go b/providers/azure/services/cache/slot_wiring_test.go index 9520b704..3a22a6e9 100644 --- a/providers/azure/services/cache/slot_wiring_test.go +++ b/providers/azure/services/cache/slot_wiring_test.go @@ -37,9 +37,6 @@ func (h slotHTTP) Do(*http.Request) (*http.Response, error) { func TestSlotHeldDuringCalls(t *testing.T) { t.Parallel() - recs := func(p *slottest.Probe) *slotPager[armconsumption.ReservationRecommendationsClientListResponse] { - return &slotPager[armconsumption.ReservationRecommendationsClientListResponse]{p: p} - } resv := func(p *slottest.Probe) *slotPager[armconsumption.ReservationsDetailsClientListResponse] { return &slotPager[armconsumption.ReservationsDetailsClientListResponse]{p: p} } @@ -47,9 +44,6 @@ func TestSlotHeldDuringCalls(t *testing.T) { name string call func(ctx context.Context, p *slottest.Probe) }{ - {"recommendations", func(ctx context.Context, p *slottest.Probe) { - _, _ = (&Client{recommendationsPager: recs(p)}).GetRecommendations(ctx, nil) - }}, {"reservations", func(ctx context.Context, p *slottest.Probe) { _, _ = (&Client{reservationsPager: resv(p)}).GetExistingCommitments(ctx) }}, From f90a3bd1eb909bda35840add2e926eaa22749f1c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 15:25:09 +0100 Subject: [PATCH 04/12] refactor(azure/cache): drop the Redis SKU catalogue and unit pricer (#692) After the recommendations no-op, nothing reads the shard-count catalogue, ReservationUnitPrice (managedredis prices Advisor rows now) or the recommendations pager. Remove them with the tests that only exercised them. Co-Authored-By: Claude Opus 5.5 --- providers/azure/services/cache/client.go | 138 +----------------- providers/azure/services/cache/client_test.go | 66 --------- .../services/cache/pagination_cap_test.go | 35 ----- .../cache/reservation_unit_price_test.go | 24 --- .../azure/services/cache/slot_wiring_test.go | 3 - 5 files changed, 4 insertions(+), 262 deletions(-) delete mode 100644 providers/azure/services/cache/reservation_unit_price_test.go diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 4ee4c62e..67b98aa6 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -7,7 +7,6 @@ import ( "net/http" "net/url" "strings" - "sync" "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -16,10 +15,8 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" - "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/httpclient" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/pricing" - azrecs "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/recommendations" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/slot" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" ) @@ -30,27 +27,11 @@ const maxReservationsPages = 50 // maxCachesPages caps Redis cache list pagination. const maxCachesPages = 20 -// redisSKUEntry holds the SKU-catalog-derived fields the converter -// wants for each Redis SKU. Sourced from the cache's Properties: -// - shardCount: Properties.ShardCount (Premium-tier clustered caches). -// -// Non-clustered caches return shardCount=0; the converter treats 0 as -// "unknown" and leaves CacheDetails.Shards at its zero value. -type redisSKUEntry struct { - shardCount int -} - // HTTPClient interface for HTTP operations (enables mocking). type HTTPClient interface { Do(req *http.Request) (*http.Response, error) } -// RecommendationsPager interface for recommendations pager (enables mocking). -type RecommendationsPager interface { - More() bool - NextPage(ctx context.Context) (armconsumption.ReservationRecommendationsClientListResponse, error) -} - // ReservationsDetailsPager interface for reservations details pager (enables mocking). type ReservationsDetailsPager interface { More() bool @@ -65,32 +46,13 @@ type RedisCachesPager interface { // Client handles Azure Cache for Redis Reserved Capacity. type Client struct { - cred azcore.TokenCredential - httpClient HTTPClient - recommendationsPager RecommendationsPager - reservationsPager ReservationsDetailsPager - redisCachesPager RedisCachesPager - - // Lazy SKU catalog cache. armredis exposes no per-region "list all - // possible SKUs" surface, so we derive shard counts from the existing - // caches in the subscription (NewListBySubscriptionPager). Fetched - // ONCE per client lifetime; subsequent converter calls in the same - // GetRecommendations run hit the in-memory map. A failed fetch leaves - // skuCacheMap nil and converters fall back to Shards=0 with a WARN - // log — the conversion itself does NOT fail. - skuCacheMap map[string]redisSKUEntry + cred azcore.TokenCredential + httpClient HTTPClient + reservationsPager ReservationsDetailsPager + redisCachesPager RedisCachesPager subscriptionID string region string - - // Lazy SKU catalog cache. armredis exposes no per-region "list all - // possible SKUs" surface, so we derive shard counts from the existing - // caches in the subscription (NewListBySubscriptionPager). Fetched - // ONCE per client lifetime; subsequent converter calls in the same - // GetRecommendations run hit the in-memory map. A failed fetch leaves - // skuCacheMap nil and converters fall back to Shards=0 with a WARN - // log — the conversion itself does NOT fail. - skuCacheOnce sync.Once } // NewClient creates a new Azure Cache client. @@ -120,11 +82,6 @@ func NewClientWithHTTP(cred azcore.TokenCredential, subscriptionID, region strin } } -// SetRecommendationsPager sets the recommendations pager (for testing). -func (c *Client) SetRecommendationsPager(pager RecommendationsPager) { - c.recommendationsPager = pager -} - // SetReservationsPager sets the reservations pager (for testing). func (c *Client) SetReservationsPager(pager ReservationsDetailsPager) { c.reservationsPager = pager @@ -492,15 +449,6 @@ type RedisPricing struct { ReservationPrice float64 } -// ReservationUnitPrice returns the Retail Prices reservation total and currency for one unit of sku in region over termYears. -func (c *Client) ReservationUnitPrice(ctx context.Context, sku, region string, termYears int) (azrecs.ReservationPrice, error) { - p, err := c.getRedisPricing(ctx, sku, region, termYears) - if err != nil { - return azrecs.ReservationPrice{}, err - } - return azrecs.ReservationPrice{Total: p.ReservationPrice, Currency: p.Currency}, nil -} - func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYears int) (*RedisPricing, error) { identity, err := pricing.ParseRedisIdentity(sku) if err != nil { @@ -541,81 +489,3 @@ func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYe Currency: selected.CurrencyCode, }, nil } - -// cachedSKULookup returns the SKU catalog entry for skuName, fetching -// the catalog lazily on first call. The catalog is fetched ONCE per -// client lifetime via armredis.Client.NewListBySubscriptionPager; -// subsequent calls are O(1) map lookups. ok=false on cache miss OR -// catalog-fetch failure — the caller falls back to Shards=0 rather -// than failing the whole conversion. -// -// Why source from existing caches: armredis exposes no "list all -// possible SKUs" endpoint. The catalog surface that does exist -// (existing cache instances in the subscription) gives us authoritative -// shard counts for the SKUs the customer actually uses, which is the -// set the recommendation engine recommends from anyway. -func (c *Client) cachedSKULookup(ctx context.Context, skuName string) (redisSKUEntry, bool) { - c.skuCacheOnce.Do(func() { - c.skuCacheMap = c.fetchSKUCatalogue(ctx) - }) - if c.skuCacheMap == nil { - return redisSKUEntry{}, false - } - entry, ok := c.skuCacheMap[skuName] - return entry, ok -} - -// fetchSKUCatalogue performs the single ListBySubscription walk and -// reduces the response into a name->redisSKUEntry map keyed by the -// "_" SKU naming convention (e.g. "Premium_P1") -// that matches the recommendation engine's ResourceType output. -// -// Returns nil on error so the sync.Once-gated cache field stays nil and -// cachedSKULookup falls back to the empty-Details path. The fetch error -// is logged WARN once. -func (c *Client) fetchSKUCatalogue(ctx context.Context) map[string]redisSKUEntry { - pager, err := c.createRedisCachesPager() - if err != nil { - logging.Warnf("azure cache: SKU catalog pager create failed for region %s: %v — Details.Shards left at 0", c.region, err) - return nil - } - out := make(map[string]redisSKUEntry) - for pageIdx := 0; pager.More(); pageIdx++ { - if ctx.Err() != nil { - return nil - } - if pageIdx >= maxCachesPages { - logging.Warnf("azure cache: SKU catalog pagination cap (%d pages) reached for region %s; partial cache (%d entries) discarded, Details.Shards left at 0", maxCachesPages, c.region, len(out)) - return nil - } - page, err := slot.NextPage(ctx, pager) - if err != nil { - logging.Warnf("azure cache: SKU catalog page fetch failed for region %s: %v — partial cache (%d entries) discarded, Details.Shards left at 0", c.region, err, len(out)) - return nil - } - addSKUEntries(out, page.Value) - } - return out -} - -// addSKUEntries records each cache's SKU and shard count in out. -func addSKUEntries(out map[string]redisSKUEntry, caches []*armredis.ResourceInfo) { - for _, cache := range caches { - fullSKU := extractSKUFromCache(cache) - if fullSKU == "" { - continue - } - shards := 0 - if cache.Properties != nil && cache.Properties.ShardCount != nil { - shards = int(*cache.Properties.ShardCount) - } - // First-write-wins: if the same SKU appears on two caches - // with different ShardCounts, keep the first. Real Premium - // clusters configured to the same SKU typically share the - // shard count anyway — this just keeps the cache - // deterministic regardless of pager order. - if _, exists := out[fullSKU]; !exists { - out[fullSKU] = redisSKUEntry{shardCount: shards} - } - } -} diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index 6ad26306..e645c1da 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -24,26 +24,6 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" ) -// MockRecommendationsPager mocks the RecommendationsPager interface. -type MockRecommendationsPager struct { - mock.Mock - pages []armconsumption.ReservationRecommendationsClientListResponse - index int -} - -func (m *MockRecommendationsPager) More() bool { - return m.index < len(m.pages) -} - -func (m *MockRecommendationsPager) NextPage(ctx context.Context) (armconsumption.ReservationRecommendationsClientListResponse, error) { - if m.index >= len(m.pages) { - return armconsumption.ReservationRecommendationsClientListResponse{}, errors.New("no more pages") - } - page := m.pages[m.index] - m.index++ - return page, nil -} - // MockReservationsDetailsPager mocks the ReservationsDetailsPager interface. type MockReservationsDetailsPager struct { pages []armconsumption.ReservationsDetailsClientListResponse @@ -789,11 +769,6 @@ func TestCacheClient_SetterMethods(t *testing.T) { t.Parallel() client := NewClient(nil, "test-subscription", "eastus") - // Test SetRecommendationsPager - mockRecPager := &MockRecommendationsPager{} - client.SetRecommendationsPager(mockRecPager) - assert.Equal(t, mockRecPager, client.recommendationsPager) - // Test SetReservationsPager mockResPager := &MockReservationsDetailsPager{} client.SetReservationsPager(mockResPager) @@ -818,47 +793,6 @@ func TestCacheClient_GetCommonSKUs(t *testing.T) { assert.Contains(t, skus, "Premium_P5") } -// TestCacheClient_CachedSKULookup_FetchedOnce pins the perf invariant: -// many converter calls in the same GetRecommendations run trigger -// exactly ONE catalog fetch. The mock pager counts pages served; the -// assertion verifies the count stays at 1 after multiple lookups. -func TestCacheClient_CachedSKULookup_FetchedOnce(t *testing.T) { - t.Parallel() - client := NewClient(nil, "test-subscription", "eastus") - - premiumName := armredis.SKUNamePremium - familyP := armredis.SKUFamilyP - capacity := int32(1) - shardCount := int32(2) - - mockPager := &MockRedisCachesPager{ - pages: []armredis.ClientListBySubscriptionResponse{ - { - ListResult: armredis.ListResult{ - Value: []*armredis.ResourceInfo{ - { - Properties: &armredis.Properties{ - SKU: &armredis.SKU{ - Name: &premiumName, - Family: &familyP, - Capacity: &capacity, - }, - ShardCount: &shardCount, - }, - }, - }, - }, - }, - }, - } - client.SetRedisCachesPager(mockPager) - - for i := 0; i < 10; i++ { - _, _ = client.cachedSKULookup(context.Background(), "Premium_P1") - } - assert.Equal(t, 1, mockPager.index, "catalog must be fetched ONCE regardless of lookup count") -} - // Test the to package is properly imported (used in tests). var _ = to.Ptr("test") diff --git a/providers/azure/services/cache/pagination_cap_test.go b/providers/azure/services/cache/pagination_cap_test.go index 6d79df1d..cad00ed2 100644 --- a/providers/azure/services/cache/pagination_cap_test.go +++ b/providers/azure/services/cache/pagination_cap_test.go @@ -2,10 +2,8 @@ package cache import ( "context" - "testing" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3" - "github.com/stretchr/testify/assert" ) // countingRedisPager reports More() until limit pages were requested, so a @@ -20,28 +18,6 @@ func (p *countingRedisPager) NextPage(_ context.Context) (armredis.ClientListByS return armredis.ClientListBySubscriptionResponse{}, nil } -func TestFetchSKUCatalogue_PageCapStopsEndlessPager(t *testing.T) { - t.Parallel() - pager := &countingRedisPager{limit: 1000} - client := NewClient(nil, "test-subscription", "eastus") - client.SetRedisCachesPager(pager) - - assert.Nil(t, client.fetchSKUCatalogue(context.Background())) - assert.Equal(t, maxCachesPages, pager.calls) -} - -func TestFetchSKUCatalogue_CancelledContextStopsWalk(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithCancel(context.Background()) - cancel() - pager := &countingRedisPager{limit: 1000} - client := NewClient(nil, "test-subscription", "eastus") - client.SetRedisCachesPager(pager) - - assert.Nil(t, client.fetchSKUCatalogue(ctx)) - assert.Zero(t, pager.calls) -} - // finalPartialRedisPager serves exactly pages pages: each full page holds // perPage distinct SKUs, the last holds a single one with no next link. type finalPartialRedisPager struct { @@ -64,14 +40,3 @@ func (p *finalPartialRedisPager) NextPage(_ context.Context) (armredis.ClientLis p.calls++ return armredis.ClientListBySubscriptionResponse{ListResult: armredis.ListResult{Value: caches}}, nil } - -func TestFetchSKUCatalogue_ExactlyCapPagesWithPartialLastPageSucceeds(t *testing.T) { - t.Parallel() - pager := &finalPartialRedisPager{pages: maxCachesPages, perPage: 2} - client := NewClient(nil, "test-subscription", "eastus") - client.SetRedisCachesPager(pager) - - got := client.fetchSKUCatalogue(context.Background()) - assert.Len(t, got, 2*(maxCachesPages-1)+1) - assert.Equal(t, maxCachesPages, pager.calls) -} diff --git a/providers/azure/services/cache/reservation_unit_price_test.go b/providers/azure/services/cache/reservation_unit_price_test.go deleted file mode 100644 index f268e62f..00000000 --- a/providers/azure/services/cache/reservation_unit_price_test.go +++ /dev/null @@ -1,24 +0,0 @@ -package cache - -import ( - "context" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - azrecs "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/recommendations" - "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" -) - -func TestClient_ReservationUnitPrice(t *testing.T) { - t.Parallel() - client := NewClientWithHTTP(nil, "sub", "eastus", &mocks.PricingHTTP{Items: []map[string]any{ - mocks.ReservationRow("Redis Cache", "Azure Redis Cache Premium", "eastus", "Azure_Redis_Cache_Premium_P1_Cache", "P1", "P1 Cache Instance", "1 Year", 500), - }}) - - got, err := client.ReservationUnitPrice(context.Background(), "Premium_P1", "eastus", 1) - - require.NoError(t, err) - assert.Equal(t, azrecs.ReservationPrice{Total: 500, Currency: "USD"}, got) -} diff --git a/providers/azure/services/cache/slot_wiring_test.go b/providers/azure/services/cache/slot_wiring_test.go index 3a22a6e9..8609af9d 100644 --- a/providers/azure/services/cache/slot_wiring_test.go +++ b/providers/azure/services/cache/slot_wiring_test.go @@ -50,9 +50,6 @@ func TestSlotHeldDuringCalls(t *testing.T) { {"valid resource types", func(ctx context.Context, p *slottest.Probe) { _, _ = (&Client{redisCachesPager: &slotPager[armredis.ClientListBySubscriptionResponse]{p: p}}).GetValidResourceTypes(ctx) }}, - {"sku catalog", func(ctx context.Context, p *slottest.Probe) { - (&Client{region: "eastus", redisCachesPager: &slotPager[armredis.ClientListBySubscriptionResponse]{p: p}}).fetchSKUCatalogue(ctx) - }}, {"retail pricing", func(ctx context.Context, p *slottest.Probe) { _, _ = (&Client{httpClient: slotHTTP{p}}).getRedisPricing(ctx, "Premium_P1", "eastus", 1) }}, From bb94375bc056d5edd6a6e57b682d502489152c6e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 15:35:49 +0100 Subject: [PATCH 05/12] docs(changelog): note Azure Redis recommendations moved to managedredis (#692) Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54390e21..f3df9fb2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -177,6 +177,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Changed +- **`azure`:** Azure Redis recommendations now come only from managedredis and are labelled `memorydb` (`ServiceMemoryDB`), both from Consumption and from Advisor rows. The cache client's `GetRecommendations` returns none, so `GetRecommendationsForService(ServiceCache)` is empty for Azure, and consumers that stored Azure Redis plans or settings under `cache` must also match `memorydb`. Existing Redis reservations do not yet reduce recommendations (#699) (#692). - `ListTargetOfferings` now walks up to 50 pages (about 5000 offerings) instead of 10, and returns an error wrapping the new `ec2.ErrTargetOfferingsTruncated` (with no list) when that bound is hit, From 03e08536f1d49400805333a53aaceba893962303 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 15:48:03 +0100 Subject: [PATCH 06/12] docs(changelog): mark the Azure Redis moves as breaking (#692) Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3df9fb2..e4c78ae2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -177,7 +177,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Changed -- **`azure`:** Azure Redis recommendations now come only from managedredis and are labelled `memorydb` (`ServiceMemoryDB`), both from Consumption and from Advisor rows. The cache client's `GetRecommendations` returns none, so `GetRecommendationsForService(ServiceCache)` is empty for Azure, and consumers that stored Azure Redis plans or settings under `cache` must also match `memorydb`. Existing Redis reservations do not yet reduce recommendations (#699) (#692). +- **BREAKING `azure`:** Azure Redis recommendations now come only from managedredis and are labelled `memorydb` (`ServiceMemoryDB`), both from Consumption and from Advisor rows. The cache client's `GetRecommendations` returns none, so `GetRecommendationsForService(ServiceCache)` is empty for Azure, and consumers that stored Azure Redis plans or settings under `cache` must also match `memorydb`. Existing Redis reservations do not yet reduce recommendations (#699) (#692). - `ListTargetOfferings` now walks up to 50 pages (about 5000 offerings) instead of 10, and returns an error wrapping the new `ec2.ErrTargetOfferingsTruncated` (with no list) when that bound is hit, @@ -497,6 +497,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Removed +- **BREAKING:** `providers/azure/services/cache` no longer exports `RecommendationsPager`, `(*Client).SetRecommendationsPager` or `(*Client).ReservationUnitPrice`. The cache client no longer collects or prices recommendations; managedredis does. No cli, mcp or platform code references them. (#692) - **BREAKING:** `common.Matches` (`pkg/common`) is removed. It compared only Provider, Region, Service, ResourceType and engine and had no callers in this repo, the CLI, the MCP server or the platform; use `pkg/recfilter` From f0fc9f8f7a42758f995f22b7d2a6ac6f8e0c59a2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 15:49:24 +0100 Subject: [PATCH 07/12] refactor(azure/managedredis): price Consumption recs through ReservationUnitPrice (#692) Co-Authored-By: Claude Opus 5.5 --- providers/azure/nice | 0 providers/azure/services/managedredis/client.go | 8 +------- 2 files changed, 1 insertion(+), 7 deletions(-) create mode 100644 providers/azure/nice diff --git a/providers/azure/nice b/providers/azure/nice new file mode 100644 index 00000000..e69de29b diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index 11d2f08c..e7549d35 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -142,13 +142,7 @@ type AzureRetailPrice = pricing.Page[pricing.RetailPriceItem] // GetRecommendations gets Redis Cache reservation recommendations from the Azure Consumption API. func (c *Client) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { recs := make([]common.Recommendation, 0) - pricer := recommendations.NewPricer("managedredis", func(ctx context.Context, sku, region string, termYears int) (recommendations.ReservationPrice, error) { - p, err := c.getRedisPricing(ctx, sku, region, termYears) - if err != nil { - return recommendations.ReservationPrice{}, err - } - return recommendations.ReservationPrice{Total: p.ReservationPrice, Currency: p.Currency}, nil - }) + pricer := recommendations.NewPricer("managedredis", c.ReservationUnitPrice) var pager RecommendationsPager if c.recommendationsPager != nil { From d8e1a1153d4f3176163c1e3241cbd3002e4906b9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 19:28:30 +0100 Subject: [PATCH 08/12] chore(azure): drop a stray empty file added by mistake (#692) providers/azure/nice was created by a mis-parsed shell command in 913cb69 and is not part of the change. Co-Authored-By: Claude Opus 5.5 --- providers/azure/nice | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 providers/azure/nice diff --git a/providers/azure/nice b/providers/azure/nice deleted file mode 100644 index e69de29b..00000000 From 3f9554a277285a10eebcac9d43571f4614e2bb3a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 22:50:20 +0100 Subject: [PATCH 09/12] fix(azure): set CacheDetails on managedredis recommendations (#692) Engine filters and the CSV Engine column read CacheDetails, which the removed cache converter set. Restore Engine=redis and NodeType; Shards stays 0 (unknown). Co-Authored-By: Claude Opus 5.5 --- .../azure/services/managedredis/client.go | 2 + .../services/managedredis/details_test.go | 39 +++++++++++++++++++ 2 files changed, 41 insertions(+) create mode 100644 providers/azure/services/managedredis/details_test.go diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index cb95b024..a707c6c0 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -526,5 +526,7 @@ func (c *Client) convertRecommendation(ctx context.Context, rec armconsumption.R Term: f.Term, PaymentOption: "upfront", Timestamp: time.Now(), + // Shards is unknown here (it came from the removed SKU catalogue), so 0. + Details: &common.CacheDetails{Engine: "redis", NodeType: f.ResourceType}, } } diff --git a/providers/azure/services/managedredis/details_test.go b/providers/azure/services/managedredis/details_test.go new file mode 100644 index 00000000..dca5f6c3 --- /dev/null +++ b/providers/azure/services/managedredis/details_test.go @@ -0,0 +1,39 @@ +package managedredis + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/recfilter" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" +) + +// Engine filters and the CSV Engine column read CacheDetails, so the Redis +// recommendation must carry Engine and NodeType. Shards stays 0 (unknown). +func TestConvertRecommendation_CacheDetails(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub-abc", "westeurope") + azRec := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("westeurope"), + mocks.WithNormalizedSize("Premium_P3"), + mocks.WithQuantity(1), + mocks.WithCosts(1000.0, 700.0, 300.0), + ) + rec := c.convertRecommendation(context.Background(), azRec) + require.NotNil(t, rec) + + details, ok := rec.Details.(*common.CacheDetails) + require.True(t, ok, "Details must be a *common.CacheDetails pointer") + assert.Equal(t, "redis", details.Engine) + assert.Equal(t, "Premium_P3", details.NodeType) + assert.Equal(t, rec.ResourceType, details.NodeType) + assert.Zero(t, details.Shards, "shard count is unknown") + + assert.Equal(t, "redis", common.EngineFromDetails(rec.Details)) + assert.True(t, recfilter.Filters{IncludeEngines: []string{"redis"}}.IncludesEngine(rec)) + assert.False(t, recfilter.Filters{IncludeEngines: []string{"valkey"}}.IncludesEngine(rec)) +} From 9fbd08c92ac0c052daac1c1a1732fd036092788d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 22:50:33 +0100 Subject: [PATCH 10/12] docs(changelog): correct the Azure Redis routing entry (#692) Drop the stale held-reservation caveat (fixed by #706) and note that Shards is no longer reported. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cbae0636..a7149720 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -177,7 +177,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Changed -- **BREAKING `azure`:** Azure Redis recommendations now come only from managedredis and are labelled `memorydb` (`ServiceMemoryDB`), both from Consumption and from Advisor rows. The cache client's `GetRecommendations` returns none, so `GetRecommendationsForService(ServiceCache)` is empty for Azure, and consumers that stored Azure Redis plans or settings under `cache` must also match `memorydb`. Existing Redis reservations do not yet reduce recommendations (#699) (#692). +- **BREAKING `azure`:** Azure Redis recommendations now come only from managedredis and are labelled `memorydb` (`ServiceMemoryDB`), both from Consumption and from Advisor rows. The cache client's `GetRecommendations` returns none, so `GetRecommendationsForService(ServiceCache)` is empty for Azure, and consumers that stored Azure Redis plans or settings under `cache` must also match `memorydb`. Azure Redis recommendations keep `CacheDetails` `Engine` and `NodeType` but no longer report `Shards` (0, unknown) (#692). - `ListTargetOfferings` now walks up to 50 pages (about 5000 offerings) instead of 10, and returns an error wrapping the new `ec2.ErrTargetOfferingsTruncated` (with no list) when that bound is hit, From 3df2d9929857c096a2b95625a9ca9d7a690bf39a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 22:52:25 +0100 Subject: [PATCH 11/12] fix(azure): tag held Redis reservations with the redis engine (#692) The dedupe key includes the engine. Recommendations now carry CacheDetails.Engine "redis", so held reservations without an engine no longer reduced them (TestHeldReservation_RoundTripReducesRecommendation). Co-Authored-By: Claude Opus 5.5 --- providers/azure/services/managedredis/client.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index a707c6c0..928f6d6d 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -218,6 +218,9 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen return nil, err } if commitment != nil { + // The dedupe key includes the engine and recommendations carry + // CacheDetails.Engine "redis", so held reservations must too. + commitment.Engine = redisEngine commitments = append(commitments, *commitment) } } @@ -502,6 +505,9 @@ func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYe }, nil } +// redisEngine is the CacheDetails.Engine of every Azure Cache for Redis recommendation and reservation. +const redisEngine = "redis" + // convertRecommendation converts an Azure Consumption API recommendation to the common format. // Returns nil when the input is nil or cannot be parsed (e.g. an unsupported SDK Kind). func (c *Client) convertRecommendation(ctx context.Context, rec armconsumption.ReservationRecommendationClassification) *common.Recommendation { @@ -526,7 +532,7 @@ func (c *Client) convertRecommendation(ctx context.Context, rec armconsumption.R Term: f.Term, PaymentOption: "upfront", Timestamp: time.Now(), - // Shards is unknown here (it came from the removed SKU catalogue), so 0. - Details: &common.CacheDetails{Engine: "redis", NodeType: f.ResourceType}, + // Shards is unknown here (it came from the removed SKU catalog), so 0. + Details: &common.CacheDetails{Engine: redisEngine, NodeType: f.ResourceType}, } } From 9ac1ff80548365e370a30ae58b5ded2cba951972 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 22:57:57 +0100 Subject: [PATCH 12/12] fix(azure): give Advisor Redis recommendations the redis engine (#692) Held Redis reservations are keyed with engine "redis", so an Advisor-only Redis row with no Details stopped being reduced by the duplicate checker. Share one managedredis.Details helper between the Consumption and Advisor paths. Co-Authored-By: Claude Opus 5.5 --- providers/azure/recommendations.go | 3 +++ .../azure/recommendations_redis_held_test.go | 21 +++++++++++++++++++ .../azure/services/managedredis/client.go | 15 ++++++++----- 3 files changed, 34 insertions(+), 5 deletions(-) create mode 100644 providers/azure/recommendations_redis_held_test.go diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index 441f9861..0b06abb0 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -651,6 +651,9 @@ func (r *RecommendationsClientAdapter) convertAdvisorRecommendation(ctx context. azrecs.RecordSkip(ctx, "advisor", fmt.Errorf("%s recommendation in %q: %w", service, rec.Region, err)) return nil } + if rec.Service == common.ServiceMemoryDB && rec.Details == nil { + rec.Details = managedredis.Details(rec.ResourceType) + } return rec } diff --git a/providers/azure/recommendations_redis_held_test.go b/providers/azure/recommendations_redis_held_test.go new file mode 100644 index 00000000..a7bc0dab --- /dev/null +++ b/providers/azure/recommendations_redis_held_test.go @@ -0,0 +1,21 @@ +package azure + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/managedredis" +) + +// The dedupe key includes the engine, and held Redis reservations are keyed +// as "redis", so an Advisor-only Redis row must carry the same engine or a +// held reservation stops reducing it. +func TestAdvisorRedisRowCarriesRedisEngine(t *testing.T) { + adv := redisAdvisorRow(redisAdapter(t, realRedisClient(), nil), "westus") + require.Equal(t, common.ServiceMemoryDB, adv.Service) + assert.Equal(t, "redis", common.EngineFromDetails(adv.Details)) + assert.Equal(t, managedredis.Details(adv.ResourceType), adv.Details) +} diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index 928f6d6d..c6298322 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -220,7 +220,7 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen if commitment != nil { // The dedupe key includes the engine and recommendations carry // CacheDetails.Engine "redis", so held reservations must too. - commitment.Engine = redisEngine + commitment.Engine = Engine commitments = append(commitments, *commitment) } } @@ -505,8 +505,14 @@ func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYe }, nil } -// redisEngine is the CacheDetails.Engine of every Azure Cache for Redis recommendation and reservation. -const redisEngine = "redis" +// Engine is the CacheDetails.Engine of every Azure Cache for Redis recommendation and reservation. +const Engine = "redis" + +// Details is the CacheDetails every Azure Redis recommendation carries, whether +// it came from Consumption or Advisor. Shards is unknown, so 0. +func Details(resourceType string) *common.CacheDetails { + return &common.CacheDetails{Engine: Engine, NodeType: resourceType} +} // convertRecommendation converts an Azure Consumption API recommendation to the common format. // Returns nil when the input is nil or cannot be parsed (e.g. an unsupported SDK Kind). @@ -532,7 +538,6 @@ func (c *Client) convertRecommendation(ctx context.Context, rec armconsumption.R Term: f.Term, PaymentOption: "upfront", Timestamp: time.Now(), - // Shards is unknown here (it came from the removed SKU catalog), so 0. - Details: &common.CacheDetails{Engine: redisEngine, NodeType: f.ResourceType}, + Details: Details(f.ResourceType), } }