diff --git a/CHANGELOG.md b/CHANGELOG.md index 4caacb67..a7149720 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -177,6 +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`. 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, @@ -498,6 +499,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` diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index 52268474..0b06abb0 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. // @@ -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 } @@ -753,7 +756,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_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/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") } diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 15650282..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,44 +15,23 @@ 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" ) -// maxRecsPages caps Consumption API recommendation pagination. -const maxRecsPages = 10 - // maxReservationsPages caps reservation-detail pagination. 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 @@ -68,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. @@ -123,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 @@ -151,65 +105,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. @@ -548,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 { @@ -597,122 +489,3 @@ func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYe Currency: selected.CurrencyCode, }, 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; -// 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 29941057..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 @@ -487,57 +467,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() @@ -840,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) @@ -869,179 +793,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 -// 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/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/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/slot_wiring_test.go b/providers/azure/services/cache/slot_wiring_test.go index 9520b704..8609af9d 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,18 +44,12 @@ 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) }}, {"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) }}, diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index e8081698..c6298322 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -145,13 +145,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 { @@ -224,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 = Engine commitments = append(commitments, *commitment) } } @@ -455,6 +452,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 { @@ -499,6 +505,15 @@ func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYe }, nil } +// 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). func (c *Client) convertRecommendation(ctx context.Context, rec armconsumption.ReservationRecommendationClassification) *common.Recommendation { @@ -523,5 +538,6 @@ func (c *Client) convertRecommendation(ctx context.Context, rec armconsumption.R Term: f.Term, PaymentOption: "upfront", Timestamp: time.Now(), + Details: Details(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)) +} diff --git a/providers/azure/services/cache/reservation_unit_price_test.go b/providers/azure/services/managedredis/reservation_unit_price_test.go similarity index 53% rename from providers/azure/services/cache/reservation_unit_price_test.go rename to providers/azure/services/managedredis/reservation_unit_price_test.go index f268e62f..0b5a40d1 100644 --- a/providers/azure/services/cache/reservation_unit_price_test.go +++ b/providers/azure/services/managedredis/reservation_unit_price_test.go @@ -1,4 +1,4 @@ -package cache +package managedredis import ( "context" @@ -22,3 +22,25 @@ func TestClient_ReservationUnitPrice(t *testing.T) { 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()) +}