diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index 4bc68b03b..0a010c853 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -22,6 +22,43 @@ import ( "github.com/LeanerCloud/CUDly/providers/azure/services/savingsplans" ) +// serviceRecsGetter is the narrow interface satisfied by each per-service +// client (compute, database, cache, 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, +// 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 +// stays unchanged and the injection is limited to the test package that owns +// the test binary's address space. +// +// getAdvisorRecsFn wraps r.getAdvisorRecommendations; it is a field rather +// than a package-level var so that injection is scoped to the adapter +// instance and avoids shared-state issues when tests run the advisor path +// concurrently. Tests that do not need real ARM calls set it to noopAdvisorFn. +var ( + newComputeClientFn func(azcore.TokenCredential, string, string) serviceRecsGetter = func(cred azcore.TokenCredential, sub, region string) serviceRecsGetter { + return compute.NewClient(cred, sub, region) + } + 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) + } + newCosmosDBClientFn func(azcore.TokenCredential, string, string) serviceRecsGetter = func(cred azcore.TokenCredential, sub, region string) serviceRecsGetter { + return cosmosdb.NewClient(cred, sub, region) + } + newSavingsPlansClientFn func(azcore.TokenCredential, string, string) serviceRecsGetter = func(cred azcore.TokenCredential, sub, region string) serviceRecsGetter { + return savingsplans.NewClient(cred, sub, region) + } +) + // RecommendationsClientAdapter aggregates Azure reservation recommendations across all services. // // Invariant: subscriptionID must be non-empty. Downstream converters use it as @@ -31,9 +68,13 @@ import ( // path is NewRecommendationsClientAdapter; direct struct literals bypass the // invariant check and should be confined to tests that deliberately exercise // the unvalidated shape. +// +// getAdvisorRecsFn defaults to r.getAdvisorRecommendations and may be +// overridden per-instance in tests to avoid real ARM network calls. type RecommendationsClientAdapter struct { - cred azcore.TokenCredential - subscriptionID string + cred azcore.TokenCredential + subscriptionID string + getAdvisorRecsFn func(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) } // NewRecommendationsClientAdapter builds a RecommendationsClientAdapter with @@ -44,10 +85,12 @@ func NewRecommendationsClientAdapter(cred azcore.TokenCredential, subscriptionID if subscriptionID == "" { return nil, fmt.Errorf("azure recommendations: subscriptionID is required") } - return &RecommendationsClientAdapter{ + r := &RecommendationsClientAdapter{ cred: cred, subscriptionID: subscriptionID, - }, nil + } + r.getAdvisorRecsFn = r.getAdvisorRecommendations + return r, nil } // GetRecommendations retrieves all Azure reservation recommendations across services. @@ -109,7 +152,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // Compute (VM) recommendations — subscription-wide. if includeCompute { goService(&computeErr, func() { - computeClient := compute.NewClient(r.cred, r.subscriptionID, "") + computeClient := newComputeClientFn(r.cred, r.subscriptionID, "") computeRecs, computeErr = computeClient.GetRecommendations(gctx, params) }) } @@ -117,7 +160,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // Database (SQL) recommendations — subscription-wide. if includeDB { goService(&dbErr, func() { - dbClient := database.NewClient(r.cred, r.subscriptionID, "") + dbClient := newDatabaseClientFn(r.cred, r.subscriptionID, "") dbRecs, dbErr = dbClient.GetRecommendations(gctx, params) }) } @@ -125,7 +168,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // Cache (Redis) recommendations — subscription-wide. if includeCache { goService(&cacheErr, func() { - cacheClient := cache.NewClient(r.cred, r.subscriptionID, "") + cacheClient := newCacheClientFn(r.cred, r.subscriptionID, "") cacheRecs, cacheErr = cacheClient.GetRecommendations(gctx, params) }) } @@ -133,7 +176,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // CosmosDB (NoSQL) recommendations — subscription-wide. if includeCosmos { goService(&cosmosErr, func() { - cosmosClient := cosmosdb.NewClient(r.cred, r.subscriptionID, "") + cosmosClient := newCosmosDBClientFn(r.cred, r.subscriptionID, "") cosmosRecs, cosmosErr = cosmosClient.GetRecommendations(gctx, params) }) } @@ -145,7 +188,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // a scheduler change. if includeSP { goService(&spErr, func() { - spClient := savingsplans.NewClient(r.cred, r.subscriptionID, "") + spClient := newSavingsPlansClientFn(r.cred, r.subscriptionID, "") spRecs, spErr = spClient.GetRecommendations(gctx, params) }) } @@ -153,8 +196,15 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // Azure Advisor adds cross-cutting cost recommendations independent of the // per-service Reservation API. Failures here are non-fatal — the per-service // results above are still useful on their own. + // + // getAdvisorRecsFn defaults to r.getAdvisorRecommendations and may be + // replaced per-adapter in tests to avoid real ARM network calls. + advisorFn := r.getAdvisorRecsFn + if advisorFn == nil { + advisorFn = r.getAdvisorRecommendations + } goService(&advisorErr, func() { - advisorRecs, advisorErr = r.getAdvisorRecommendations(gctx, params) + advisorRecs, advisorErr = advisorFn(gctx, params) }) // Wait for all goroutines. g.Wait() always returns nil because every diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index 557e31f13..8a13718d8 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -5,6 +5,7 @@ import ( "errors" "strings" "testing" + "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" @@ -557,3 +558,192 @@ func TestMergeServiceResults_StubsDoNotMaskTotalFailure(t *testing.T) { assert.Contains(t, err.Error(), "all 4 Azure recommendation services failed") assert.Nil(t, recs) } + +// fakeServiceClient is a test double for serviceRecsGetter. It sleeps for +// sleepDur to simulate network latency and then returns the fixed recs slice +// (or err when non-nil). sleep is done inside GetRecommendations so that +// mock latency is isolated to the mock body — the test's assert path never +// sleeps (see memory feedback_no_sleep_in_tests). +type fakeServiceClient struct { + sleepDur time.Duration + recs []common.Recommendation + err error +} + +func (f *fakeServiceClient) GetRecommendations(ctx context.Context, _ common.RecommendationParams) ([]common.Recommendation, error) { + select { + case <-time.After(f.sleepDur): + case <-ctx.Done(): + return nil, ctx.Err() + } + return f.recs, f.err +} + +// newFakeFn returns a constructor compatible with the newXxxClientFn signature +// that ignores the credential/subscription/region and always returns fake. +func newFakeFn(fake serviceRecsGetter) func(azcore.TokenCredential, string, string) serviceRecsGetter { + return func(_ azcore.TokenCredential, _, _ string) serviceRecsGetter { return fake } +} + +// noopAdvisorFn is a getAdvisorRecsFn replacement that returns immediately +// with zero results, used in timing/isolation tests to keep all latency inside +// the injectable fakeServiceClient mocks. +func noopAdvisorFn(_ context.Context, _ common.RecommendationParams) ([]common.Recommendation, error) { + return nil, nil +} + +const fakeServiceSleep = 100 * time.Millisecond + +// TestGetRecommendations_Parallelism proves that all four service goroutines +// run concurrently: total wall-clock time must be well under 2x per-service +// sleep (i.e. less than 200ms) rather than near 4x (400ms sequential). +// +// savingsplans and advisor are injected as instant no-ops so that all latency +// comes from the four fakeServiceClient mocks; without this, both paths make +// real ARM network calls whose RTT dwarfs the 100ms threshold. +func TestGetRecommendations_Parallelism(t *testing.T) { + origCompute := newComputeClientFn + origDatabase := newDatabaseClientFn + origCache := newCacheClientFn + origCosmos := newCosmosDBClientFn + origSP := newSavingsPlansClientFn + t.Cleanup(func() { + newComputeClientFn = origCompute + newDatabaseClientFn = origDatabase + newCacheClientFn = origCache + newCosmosDBClientFn = origCosmos + newSavingsPlansClientFn = origSP + }) + + rec := func(svc common.ServiceType) common.Recommendation { + return common.Recommendation{Provider: common.ProviderAzure, Service: svc} + } + + 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)}}) + newCosmosDBClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{rec(common.ServiceNoSQL)}}) + newSavingsPlansClientFn = noopFake + + adapter := &RecommendationsClientAdapter{ + cred: &mockAzureTokenCredential{}, + subscriptionID: "sub-parallelism", + getAdvisorRecsFn: noopAdvisorFn, + } + + start := time.Now() + _, err := adapter.GetRecommendations(context.Background(), common.RecommendationParams{}) + elapsed := time.Since(start) + + require.NoError(t, err) + // With 4 services sleeping 100ms in parallel the wall-clock must be + // substantially less than 2x per-service sleep. We allow 190ms headroom + // for scheduler jitter; if the calls were serial this would take ~400ms. + assert.Less(t, elapsed, 2*fakeServiceSleep, + "expected parallel dispatch: elapsed %v >= 2x per-service sleep %v -- services may be running serially", + elapsed, fakeServiceSleep) +} + +// TestGetRecommendations_OrderPreservation verifies that the merged slice +// follows the canonical order compute -> database -> cache -> cosmosdb regardless +// of which fake goroutine returns first (staggered sleeps force an +// out-of-start-order completion). +// +// savingsplans and advisor are injected as instant no-ops so the test is +// fully hermetic: no real ARM calls, deterministic rec count. +func TestGetRecommendations_OrderPreservation(t *testing.T) { + origCompute := newComputeClientFn + origDatabase := newDatabaseClientFn + origCache := newCacheClientFn + origCosmos := newCosmosDBClientFn + origSP := newSavingsPlansClientFn + t.Cleanup(func() { + newComputeClientFn = origCompute + newDatabaseClientFn = origDatabase + newCacheClientFn = origCache + newCosmosDBClientFn = origCosmos + newSavingsPlansClientFn = origSP + }) + + makeRec := func(svc common.ServiceType) common.Recommendation { + return common.Recommendation{Provider: common.ProviderAzure, Service: svc, ResourceType: string(svc)} + } + + // Stagger sleeps so goroutines complete in reverse order: cosmosdb + // finishes first (~10ms), compute last (~40ms). The merged slice must + // 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)}}) + newCosmosDBClientFn = newFakeFn(&fakeServiceClient{sleepDur: 10 * time.Millisecond, recs: []common.Recommendation{makeRec(common.ServiceNoSQL)}}) + newSavingsPlansClientFn = newFakeFn(&fakeServiceClient{}) + + adapter := &RecommendationsClientAdapter{ + cred: &mockAzureTokenCredential{}, + subscriptionID: "sub-order", + getAdvisorRecsFn: noopAdvisorFn, + } + + recs, err := adapter.GetRecommendations(context.Background(), common.RecommendationParams{}) + require.NoError(t, err) + + // savingsplans and advisor are no-ops; we expect exactly 4 recs (one per + // injectable service) in canonical order. + 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.ServiceNoSQL, recs[3].Service, "slot 3 must be cosmosdb") +} + +// TestGetRecommendations_ErrorIsolation asserts that a single service error +// does not prevent the other services' results from appearing in the merged +// slice (no sibling cancellation). +// +// savingsplans and advisor are injected as instant no-ops so the rec count is +// fully deterministic regardless of network availability. +func TestGetRecommendations_ErrorIsolation(t *testing.T) { + origCompute := newComputeClientFn + origDatabase := newDatabaseClientFn + origCache := newCacheClientFn + origCosmos := newCosmosDBClientFn + origSP := newSavingsPlansClientFn + t.Cleanup(func() { + newComputeClientFn = origCompute + newDatabaseClientFn = origDatabase + newCacheClientFn = origCache + newCosmosDBClientFn = origCosmos + newSavingsPlansClientFn = origSP + }) + + makeRec := func(svc common.ServiceType) common.Recommendation { + return common.Recommendation{Provider: common.ProviderAzure, Service: svc} + } + + // 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)}}) + newCosmosDBClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{makeRec(common.ServiceNoSQL)}}) + newSavingsPlansClientFn = newFakeFn(&fakeServiceClient{}) + + adapter := &RecommendationsClientAdapter{ + cred: &mockAzureTokenCredential{}, + subscriptionID: "sub-isolation", + getAdvisorRecsFn: noopAdvisorFn, + } + + recs, err := adapter.GetRecommendations(context.Background(), common.RecommendationParams{}) + require.NoError(t, err, "a per-service error must not surface as a GetRecommendations error") + require.Len(t, recs, 3, "expected recs from the 3 healthy injectable services") + + services := make([]common.ServiceType, len(recs)) + for i, r := range recs { + 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.ServiceNoSQL, "cosmosdb recs must be present despite db error") + assert.NotContains(t, services, common.ServiceRelationalDB, "db recs must be absent when db errors") +}