diff --git a/providers/gcp/provider.go b/providers/gcp/provider.go index ad3f77ca9..56d4d75c0 100644 --- a/providers/gcp/provider.go +++ b/providers/gcp/provider.go @@ -333,16 +333,10 @@ func (p *GCPProvider) GetAccounts(ctx context.Context) ([]common.Account, error) } } - // If no projects found, return at least the default project - if len(accounts) == 0 { - accounts = append(accounts, common.Account{ - Provider: common.ProviderGCP, - ID: p.projectID, - Name: p.projectID, - IsDefault: true, - }) - } - + // Return empty if no ACTIVE projects are visible to the credentials (10-M7). + // Synthesising a fallback account from p.projectID can silently succeed + // when the account has no accessible projects, hiding auth / permission + // errors from callers. Return empty so the caller can surface the gap. return accounts, nil } diff --git a/providers/gcp/provider_test.go b/providers/gcp/provider_test.go index d6b4f90d4..254bf47c4 100644 --- a/providers/gcp/provider_test.go +++ b/providers/gcp/provider_test.go @@ -562,6 +562,10 @@ func TestGCPProvider_GetAccounts_WithMock(t *testing.T) { assert.Equal(t, "project-2", accounts[1].ID) } +// TestGCPProvider_GetAccounts_Empty asserts that GetAccounts returns an empty +// slice (not a synthesised fallback project) when no ACTIVE projects are +// visible to the credentials (10-M7). A synthesised account would hide +// permission / auth errors from callers. func TestGCPProvider_GetAccounts_Empty(t *testing.T) { ctx := context.Background() p := NewProviderWithProject(ctx, "default-project") @@ -573,9 +577,7 @@ func TestGCPProvider_GetAccounts_Empty(t *testing.T) { accounts, err := p.GetAccounts(ctx) require.NoError(t, err) - // Should return the default project when no projects found - assert.Len(t, accounts, 1) - assert.Equal(t, "default-project", accounts[0].ID) + assert.Empty(t, accounts, "GetAccounts must return empty when no ACTIVE projects are visible (10-M7)") } func TestGCPProvider_GetAccounts_Error(t *testing.T) { diff --git a/providers/gcp/recommendations.go b/providers/gcp/recommendations.go index b33027d8f..2a8c1cecc 100644 --- a/providers/gcp/recommendations.go +++ b/providers/gcp/recommendations.go @@ -18,7 +18,9 @@ import ( "github.com/LeanerCloud/CUDly/pkg/concurrency" "github.com/LeanerCloud/CUDly/pkg/logging" "github.com/LeanerCloud/CUDly/providers/gcp/services/cloudsql" + "github.com/LeanerCloud/CUDly/providers/gcp/services/cloudstorage" "github.com/LeanerCloud/CUDly/providers/gcp/services/computeengine" + "github.com/LeanerCloud/CUDly/providers/gcp/services/memorystore" ) // defaultGCPRegionConcurrency caps the parallel per-region goroutines inside @@ -42,13 +44,21 @@ func gcpRegionConcurrency() int { return defaultGCPRegionConcurrency } -// regionResult bundles the Compute Engine and Cloud SQL recommendation slices -// returned for a single GCP region. The merge in GetRecommendations walks -// regions in sorted order and appends compute then sql per region so output -// is deterministic independent of goroutine completion order. +// regionResult bundles per-service recommendation slices returned for a single +// GCP region. The merge in GetRecommendations walks regions in sorted order +// and appends compute, sql, cache, storage per region so output is +// deterministic independent of goroutine completion order. +// +// All four GCP service clients (computeengine, cloudsql, memorystore, +// cloudstorage) implement GetRecommendations and are fanned out concurrently +// when shouldIncludeService permits. Note that cache and storage purchase paths +// are advisory-only (no-op PurchaseCommitment); their recommendations are still +// surfaced so operators can see spend-optimisation signals. type regionResult struct { compute []common.Recommendation sql []common.Recommendation + cache []common.Recommendation + storage []common.Recommendation } // RecommendationsClientAdapter aggregates GCP CUD and commitment recommendations across all services @@ -65,9 +75,10 @@ type RecommendationsClientAdapter struct { // - Outer: errgroup over regions, capped at gcpRegionConcurrency() // (CUDLY_GCP_REGION_PARALLELISM, default 10) to stay polite to the // project-scoped Recommender API quota. -// - Inner: within each region's goroutine, the (compute, cloud-sql) calls -// run as two further goroutines under a per-region sub-errgroup, so the -// per-region cost is max(compute, sql) rather than compute + sql. +// - Inner: within each region's goroutine, the four service calls +// (compute, cloud-sql, memorystore, cloudstorage) run as concurrent +// goroutines under a per-region sub-errgroup, so the per-region cost is +// max(service latencies) rather than their sum. // // Behaviour change vs the previous nested for-loops: per-(region, service) // errors that were previously silently swallowed (`if err == nil { ... }` @@ -132,9 +143,9 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p return nil, err } - // Deterministic merge: walk regions in sorted order, append compute then - // sql per region. Output is stable regardless of GCP API region-list - // ordering or goroutine completion order. + // Deterministic merge: walk regions in sorted order, append compute, sql, + // cache, storage per region. Output is stable regardless of GCP API + // region-list ordering or goroutine completion order. sortedRegions := make([]string, 0, len(results)) for region := range results { sortedRegions = append(sortedRegions, region) @@ -146,64 +157,109 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p res := results[region] allRecommendations = append(allRecommendations, res.compute...) allRecommendations = append(allRecommendations, res.sql...) + allRecommendations = append(allRecommendations, res.cache...) + allRecommendations = append(allRecommendations, res.storage...) } return allRecommendations, nil } -// collectRegion fetches Compute Engine and Cloud SQL recommendations for a -// single region concurrently. Per-service errors are logged at WARN with the -// region+service tag and never propagate — the previous silent-skip-on-err -// shape is preserved (so a misconfigured project doesn't error out the whole -// recommendations refresh) but errors are now observable in logs. Extracted -// from GetRecommendations to keep that function under the gocyclo gate -// (.golangci.yml min-complexity: 15) after the post-Wait ctx.Err() block was -// added. +// collectComputeRecs fetches Compute Engine CUD recommendations for one region. +// Handles semaphore acquire/release and client construction so these branches +// are not counted toward collectRegion's cyclomatic complexity. +func (r *RecommendationsClientAdapter) collectComputeRecs(ctx context.Context, params common.RecommendationParams, region string) ([]common.Recommendation, error) { + if err := concurrency.Acquire(ctx); err != nil { + return nil, err + } + defer concurrency.Release(ctx) + client, err := computeengine.NewClient(ctx, r.projectID, region, r.clientOpts...) + if err != nil { + return nil, err + } + return client.GetRecommendations(ctx, params) +} + +// collectSQLRecs fetches Cloud SQL CUD recommendations for one region. +func (r *RecommendationsClientAdapter) collectSQLRecs(ctx context.Context, params common.RecommendationParams, region string) ([]common.Recommendation, error) { + if err := concurrency.Acquire(ctx); err != nil { + return nil, err + } + defer concurrency.Release(ctx) + client, err := cloudsql.NewClient(ctx, r.projectID, region, r.clientOpts...) + if err != nil { + return nil, err + } + return client.GetRecommendations(ctx, params) +} + +// collectCacheRecs fetches Memorystore recommendations for one region. +func (r *RecommendationsClientAdapter) collectCacheRecs(ctx context.Context, params common.RecommendationParams, region string) ([]common.Recommendation, error) { + if err := concurrency.Acquire(ctx); err != nil { + return nil, err + } + defer concurrency.Release(ctx) + client, err := memorystore.NewClient(ctx, r.projectID, region, r.clientOpts...) + if err != nil { + return nil, err + } + return client.GetRecommendations(ctx, params) +} + +// collectStorageRecs fetches Cloud Storage recommendations for one region. +func (r *RecommendationsClientAdapter) collectStorageRecs(ctx context.Context, params common.RecommendationParams, region string) ([]common.Recommendation, error) { + if err := concurrency.Acquire(ctx); err != nil { + return nil, err + } + defer concurrency.Release(ctx) + client, err := cloudstorage.NewClient(ctx, r.projectID, region, r.clientOpts...) + if err != nil { + return nil, err + } + return client.GetRecommendations(ctx, params) +} + +// collectRegion fetches recommendations for all four GCP services +// (Compute Engine, Cloud SQL, Memorystore, Cloud Storage) for a single region +// concurrently. Per-service errors are logged at WARN with the region+service +// tag and never propagate -- the previous silent-skip-on-err shape is preserved +// (so a misconfigured project doesn't error out the whole recommendations +// refresh) but errors are now observable in logs. The per-service fetch logic +// (semaphore, client construction, GetRecommendations call) is delegated to +// dedicated helpers (collectComputeRecs, collectSQLRecs, collectCacheRecs, +// collectStorageRecs) to keep this function's cyclomatic complexity under the +// gocyclo gate. +// +// Note: memorystore and cloudstorage PurchaseCommitment paths are advisory-only +// (no programmatic purchase API exists for either); their recommendations are +// surfaced so operators can see spend-optimisation signals (H-2 fix). func (r *RecommendationsClientAdapter) collectRegion(ctx context.Context, params common.RecommendationParams, region string) regionResult { var ( - computeRecs, sqlRecs []common.Recommendation - computeErr, sqlErr error + computeRecs, sqlRecs, cacheRecs, storageRecs []common.Recommendation + computeErr, sqlErr, cacheErr, storageErr error ) g, gctx := errgroup.WithContext(ctx) - // Per-(region, service) goroutines are leaves — they issue the actual - // Recommender API call. Acquire bounds aggregate concurrent IO across - // the whole recommendations-collection fan-out tree at the shared - // semaphore's cap (CUDLY_MAX_PARALLELISM, default 20); Release returns - // the slot. Without this bound the per-region fan-out (cap 10) × - // per-service sub-fan-out (2) × accounts × providers can produce - // hundreds of concurrent gRPC clients that exhaust Lambda memory. If - // no semaphore is on ctx (CLI tools, unit tests), Acquire/Release are - // no-ops. See pkg/concurrency. if shouldIncludeService(params, common.ServiceCompute) { g.Go(func() error { - if err := concurrency.Acquire(gctx); err != nil { - computeErr = err - return nil - } - defer concurrency.Release(gctx) - client, err := computeengine.NewClient(gctx, r.projectID, region, r.clientOpts...) - if err != nil { - computeErr = err - return nil - } - computeRecs, computeErr = client.GetRecommendations(gctx, params) + computeRecs, computeErr = r.collectComputeRecs(gctx, params, region) return nil }) } if shouldIncludeService(params, common.ServiceRelationalDB) { g.Go(func() error { - if err := concurrency.Acquire(gctx); err != nil { - sqlErr = err - return nil - } - defer concurrency.Release(gctx) - client, err := cloudsql.NewClient(gctx, r.projectID, region, r.clientOpts...) - if err != nil { - sqlErr = err - return nil - } - sqlRecs, sqlErr = client.GetRecommendations(gctx, params) + sqlRecs, sqlErr = r.collectSQLRecs(gctx, params, region) + return nil + }) + } + if shouldIncludeService(params, common.ServiceCache) { + g.Go(func() error { + cacheRecs, cacheErr = r.collectCacheRecs(gctx, params, region) + return nil + }) + } + if shouldIncludeService(params, common.ServiceStorage) { + g.Go(func() error { + storageRecs, storageErr = r.collectStorageRecs(gctx, params, region) return nil }) } @@ -215,8 +271,14 @@ func (r *RecommendationsClientAdapter) collectRegion(ctx context.Context, params if sqlErr != nil { logging.Warnf("GCP %s cloudsql recommendations: %v", region, sqlErr) } + if cacheErr != nil { + logging.Warnf("GCP %s memorystore recommendations: %v", region, cacheErr) + } + if storageErr != nil { + logging.Warnf("GCP %s cloudstorage recommendations: %v", region, storageErr) + } - return regionResult{compute: computeRecs, sql: sqlRecs} + return regionResult{compute: computeRecs, sql: sqlRecs, cache: cacheRecs, storage: storageRecs} } // GetRecommendationsForService retrieves GCP commitment recommendations for a specific service @@ -235,10 +297,11 @@ func (r *RecommendationsClientAdapter) GetAllRecommendations(ctx context.Context // getRegions retrieves available GCP regions for the project func (r *RecommendationsClientAdapter) getRegions(ctx context.Context) ([]string, error) { - // Create a temporary provider to get regions - provider := NewProviderWithProject(ctx, r.projectID, r.clientOpts...) + // Create a temporary provider to get regions. The local variable is named + // p (not provider) to avoid shadowing the imported provider package (10-N3). + p := NewProviderWithProject(ctx, r.projectID, r.clientOpts...) - regions, err := provider.GetRegions(ctx) + regions, err := p.GetRegions(ctx) if err != nil { return nil, err } diff --git a/providers/gcp/recommendations_test.go b/providers/gcp/recommendations_test.go index a32eb8073..d0fe486b6 100644 --- a/providers/gcp/recommendations_test.go +++ b/providers/gcp/recommendations_test.go @@ -160,6 +160,81 @@ func TestRecommendationsClientAdapter_GetRecommendations_PropagatesContextCancel "GetRecommendations must propagate the parent ctx error") } +// TestRegionResult_HasCacheAndStorageFields is a compile-time regression test +// for H-2 (GCP broad audit): regionResult must carry cache and storage slices +// so collectRegion can fan out to memorystore and cloudstorage in addition to +// compute and cloudsql. If this test stops compiling, the wiring was reverted. +func TestRegionResult_HasCacheAndStorageFields(t *testing.T) { + recs := []common.Recommendation{{Provider: common.ProviderGCP}} + + // Field access verifies that regionResult has the full four-service shape. + result := regionResult{ + compute: recs, + sql: recs, + cache: recs, + storage: recs, + } + + assert.Len(t, result.compute, 1, "compute field must be present on regionResult") + assert.Len(t, result.sql, 1, "sql field must be present on regionResult") + assert.Len(t, result.cache, 1, "cache field must be present on regionResult (H-2)") + assert.Len(t, result.storage, 1, "storage field must be present on regionResult (H-2)") +} + +// TestShouldIncludeService_Cache_Storage verifies that shouldIncludeService +// correctly routes ServiceCache and ServiceStorage requests, which is a +// prerequisite for H-2 (wiring them into collectRegion). +func TestShouldIncludeService_Cache_Storage(t *testing.T) { + tests := []struct { + name string + params common.RecommendationParams + service common.ServiceType + expected bool + }{ + { + name: "all services includes Cache", + params: common.RecommendationParams{}, + service: common.ServiceCache, + expected: true, + }, + { + name: "all services includes Storage", + params: common.RecommendationParams{}, + service: common.ServiceStorage, + expected: true, + }, + { + name: "Cache-scoped request includes Cache", + params: common.RecommendationParams{Service: common.ServiceCache}, + service: common.ServiceCache, + expected: true, + }, + { + name: "Cache-scoped request excludes Storage", + params: common.RecommendationParams{Service: common.ServiceCache}, + service: common.ServiceStorage, + expected: false, + }, + { + name: "Storage-scoped request includes Storage", + params: common.RecommendationParams{Service: common.ServiceStorage}, + service: common.ServiceStorage, + expected: true, + }, + { + name: "Storage-scoped request excludes Compute", + params: common.RecommendationParams{Service: common.ServiceStorage}, + service: common.ServiceCompute, + expected: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, shouldIncludeService(tt.params, tt.service)) + }) + } +} + // TestGCPRegionConcurrency pins the env-knob parsing for // CUDLY_GCP_REGION_PARALLELISM. func TestGCPRegionConcurrency(t *testing.T) { diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index b164f0310..8f443e3eb 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -4,6 +4,7 @@ package cloudsql import ( "context" "fmt" + "log" "strings" "time" @@ -181,7 +182,13 @@ func (c *CloudSQLClient) GetRecommendations(ctx context.Context, params common.R return nil, fmt.Errorf("cloudsql: iterate recommendations: %w", err) } - converted := c.convertGCPRecommendation(ctx, rec) + // Skip non-ACTIVE recommendations (CLAIMED/SUCCEEDED/FAILED/DISMISSED). + // See computeengine.GetRecommendations for the full rationale. + if rec.GetStateInfo().GetState() != recommenderpb.RecommendationStateInfo_ACTIVE { + continue + } + + converted := c.convertGCPRecommendation(ctx, rec, params) if converted != nil { recommendations = append(recommendations, *converted) } @@ -190,56 +197,15 @@ func (c *CloudSQLClient) GetRecommendations(ctx context.Context, params common.R return recommendations, nil } -// GetExistingCommitments retrieves existing Cloud SQL commitments -func (c *CloudSQLClient) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { - commitments := make([]common.Commitment, 0) - - // Use injected service if available (for testing) - var svc SQLAdminService - if c.sqlAdminService != nil { - svc = c.sqlAdminService - } else { - service, err := sqladmin.NewService(ctx, c.clientOpts...) - if err != nil { - return nil, fmt.Errorf("failed to create SQL admin service: %w", err) - } - svc = &realSQLAdminService{service: service} - } - - // List all SQL instances in the project - instances, err := svc.ListInstances(c.projectID) - if err != nil { - return nil, fmt.Errorf("failed to list SQL instances: %w", err) - } - - if instances == nil { - return commitments, nil - } - - for _, instance := range instances.Items { - // Check if instance has a commitment (long-term pricing plan) - if instance.Settings != nil && instance.Settings.PricingPlan == "PACKAGE" { - commitment := common.Commitment{ - Provider: common.ProviderGCP, - Account: c.projectID, - CommitmentType: common.CommitmentCUD, - Service: common.ServiceRelationalDB, - Region: instance.Region, - CommitmentID: instance.Name, - State: strings.ToLower(instance.State), - ResourceType: instance.DatabaseVersion, - } - - // Extract tier (machine type) - if instance.Settings.Tier != "" { - commitment.ResourceType = instance.Settings.Tier - } - - commitments = append(commitments, commitment) - } - } - - return commitments, nil +// GetExistingCommitments returns an empty slice for Cloud SQL. Cloud SQL +// spend-based CUDs are purchased via the Cloud Billing console / Cloud Commerce +// Consumer Procurement API, not exposed through the sqladmin API. The legacy +// PricingPlan "PACKAGE" is a per-instance billing mode (non-commitment, +// deprecated) -- treating it as a commitment caused double-counting against +// real spend-based CUDs (10-L3). Return empty until a proper commitment- +// detection path is available. +func (c *CloudSQLClient) GetExistingCommitments(_ context.Context) ([]common.Commitment, error) { + return nil, nil } // PurchaseCommitment is intentionally a no-op for Cloud SQL: GCP exposes no @@ -362,7 +328,9 @@ type SQLPricing struct { SavingsPercentage float64 } -// getSQLPricing gets pricing from GCP Cloud Billing Catalog API +// getSQLPricing gets pricing from GCP Cloud Billing Catalog API. +// It returns an error when commitment pricing is absent from the catalog rather +// than fabricating a price from a hardcoded discount factor (issue #1020). func (c *CloudSQLClient) getSQLPricing(ctx context.Context, tier, region string, termYears int) (*SQLPricing, error) { svc, err := c.getOrCreateBillingService(ctx) if err != nil { @@ -374,18 +342,24 @@ func (c *CloudSQLClient) getSQLPricing(ctx context.Context, tier, region string, return nil, fmt.Errorf("failed to list SKUs: %w", err) } - onDemandPrice, currency := extractSQLPricingFromSKUs(skus.Skus, tier, region) + onDemandPrice, commitmentPrice, currency := extractSQLPricingFromSKUs(skus.Skus, tier, region) if onDemandPrice == 0 { return nil, fmt.Errorf("no pricing found for Cloud SQL tier %s", tier) } + if commitmentPrice == 0 { + return nil, fmt.Errorf("no commitment pricing found for Cloud SQL tier %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", tier, region) + } hoursInTerm := 8760.0 * float64(termYears) - commitmentPrice := estimateSQLCommitmentPrice(onDemandPrice, hoursInTerm, termYears) - savingsPercentage := calculateSQLSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice) + // Scale the per-hour commitment price to a term total so it is on the + // same basis as onDemandPrice * hoursInTerm. Without this, the savings + // percentage would be nearly 100% (per-hour price vs term total). + commitmentPriceTerm := commitmentPrice * hoursInTerm + savingsPercentage := calculateSQLSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPriceTerm) return &SQLPricing{ - HourlyRate: commitmentPrice / hoursInTerm, - CommitmentPrice: commitmentPrice, + HourlyRate: commitmentPrice, + CommitmentPrice: commitmentPriceTerm, OnDemandPrice: onDemandPrice * hoursInTerm, Currency: currency, SavingsPercentage: savingsPercentage, @@ -406,8 +380,9 @@ func (c *CloudSQLClient) getOrCreateBillingService(ctx context.Context) (Billing return &realBillingService{service: service}, nil } -// extractSQLPricingFromSKUs extracts on-demand pricing from SKU list -func extractSQLPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string) (onDemand float64, currency string) { +// extractSQLPricingFromSKUs extracts on-demand and commitment pricing from the SKU list. +// Cloud SQL committed-use discounts are surfaced as "commitment" SKUs in the billing catalog. +func extractSQLPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string) (onDemand, commitment float64, currency string) { currency = "USD" for _, sku := range skus { @@ -424,11 +399,14 @@ func extractSQLPricingFromSKUs(skus []*cloudbilling.Sku, tier, region string) (o currency = curr } - // Cloud SQL doesn't have separate commitment pricing in the API - onDemand = price + if strings.Contains(strings.ToLower(sku.Description), "commitment") { + commitment = price + } else { + onDemand = price + } } - return onDemand, currency + return onDemand, commitment, currency } // extractSQLPriceFromSKU extracts the unit price from a SKU @@ -451,15 +429,6 @@ func extractSQLPriceFromSKU(sku *cloudbilling.Sku) (float64, string) { return price, rate.UnitPrice.CurrencyCode } -// estimateSQLCommitmentPrice estimates commitment price based on GCP SQL package savings -func estimateSQLCommitmentPrice(onDemandPrice, hoursInTerm float64, termYears int) float64 { - discount := 0.85 // 15% savings for 1 year - if termYears == 3 { - discount = 0.80 // 20% savings for 3 years - } - return onDemandPrice * hoursInTerm * discount -} - // calculateSQLSavingsPercentage calculates the savings percentage func calculateSQLSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice float64) float64 { onDemandTotal := onDemandPrice * hoursInTerm @@ -486,8 +455,76 @@ func skuMatchesTier(sku *cloudbilling.Sku, tier, region string) bool { return true } -// convertGCPRecommendation converts a GCP Recommender recommendation to common format -func (c *CloudSQLClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation) *common.Recommendation { +// extractResourceTypeFromContent extracts the last path segment of the first +// non-empty Operation.Resource across all operation groups. Used by all four +// GCP service converters to set rec.ResourceType from Recommender payloads. +func extractResourceTypeFromContent(content *recommenderpb.RecommendationContent) string { + if content == nil || content.OperationGroups == nil { + return "" + } + for _, opGroup := range content.OperationGroups { + for _, op := range opGroup.Operations { + if op.Resource == "" { + continue + } + parts := strings.Split(op.Resource, "/") + if len(parts) > 0 { + return parts[len(parts)-1] + } + } + } + return "" +} + +// extractEstimatedSavings returns the negative of the PrimaryImpact cost +// projection (GCP encodes savings as a negative cost delta). +func extractEstimatedSavings(gcpRec *recommenderpb.Recommendation) float64 { + if gcpRec.PrimaryImpact == nil { + return 0 + } + costProj := gcpRec.PrimaryImpact.GetCostProjection() + if costProj == nil || costProj.Cost == nil { + return 0 + } + cost := costProj.Cost + return -(float64(cost.Units) + float64(cost.Nanos)/1e9) +} + +// fillSQLPricing calls getSQLPricing and, on success, writes CommitmentCost, +// OnDemandCost, SavingsPercentage, and BreakEvenMonths into rec. Pricing +// failures are logged and do not discard the recommendation. +func (c *CloudSQLClient) fillSQLPricing(ctx context.Context, rec *common.Recommendation, termYears int) { + pricing, err := c.getSQLPricing(ctx, rec.ResourceType, c.region, termYears) + if err != nil { + log.Printf("cloudsql: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err) + return + } + rec.CommitmentCost = pricing.CommitmentPrice + rec.OnDemandCost = pricing.OnDemandPrice + rec.SavingsPercentage = pricing.SavingsPercentage + if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 { + monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12) + if monthlySavings > 0 { + rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings + } + } +} + +// convertGCPRecommendation converts a GCP Recommender recommendation to common format. +// It also calls getSQLPricing to fill CommitmentCost/OnDemandCost/SavingsPercentage/ +// BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly +// (issue #1022 C2). Pricing failures are logged but do not discard the recommendation. +func (c *CloudSQLClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation { + paymentOption := params.PaymentOption + if paymentOption == "" { + paymentOption = "monthly" + } + + term := params.Term + if term == "" { + term = "1yr" + } + rec := &common.Recommendation{ Provider: common.ProviderGCP, Service: common.ServiceRelationalDB, @@ -495,34 +532,21 @@ func (c *CloudSQLClient) convertGCPRecommendation(ctx context.Context, gcpRec *r Region: c.region, CommitmentType: common.CommitmentCUD, Timestamp: time.Now(), - Term: "1yr", - PaymentOption: "monthly", - } - - // Extract resource type from recommendation content - if gcpRec.Content != nil { - if gcpRec.Content.OperationGroups != nil { - for _, opGroup := range gcpRec.Content.OperationGroups { - for _, op := range opGroup.Operations { - if op.Resource != "" { - parts := strings.Split(op.Resource, "/") - if len(parts) > 0 { - rec.ResourceType = parts[len(parts)-1] - } - } - } - } - } + Term: term, + PaymentOption: paymentOption, } - // Extract cost impact - if gcpRec.PrimaryImpact != nil { - // Use GetCostProjection() method to access the cost projection - if costProj := gcpRec.PrimaryImpact.GetCostProjection(); costProj != nil && costProj.Cost != nil { - cost := costProj.Cost - savings := -(float64(cost.Units) + float64(cost.Nanos)/1e9) - rec.EstimatedSavings = savings + rec.ResourceType = extractResourceTypeFromContent(gcpRec.Content) + rec.EstimatedSavings = extractEstimatedSavings(gcpRec) + + // Thread pricing into the converter so the scorer can rank/filter GCP recs + // correctly (issue #1022 C2). + if rec.ResourceType != "" { + termYears := 1 + if rec.Term == "3yr" || rec.Term == "3" { + termYears = 3 } + c.fillSQLPricing(ctx, rec, termYears) } return rec diff --git a/providers/gcp/services/cloudsql/client_test.go b/providers/gcp/services/cloudsql/client_test.go index 1a6bff726..6a93b81ae 100644 --- a/providers/gcp/services/cloudsql/client_test.go +++ b/providers/gcp/services/cloudsql/client_test.go @@ -300,74 +300,18 @@ func TestCloudSQLClient_ValidateOffering_TierListError(t *testing.T) { assert.Contains(t, err.Error(), "failed to list SQL tiers") } -func TestCloudSQLClient_GetExistingCommitments_WithMock(t *testing.T) { +// TestCloudSQLClient_GetExistingCommitments_ReturnsEmpty asserts that +// GetExistingCommitments always returns an empty slice regardless of the +// sqladmin instance list. Cloud SQL spend-based CUDs are not detectable via +// the sqladmin API; PricingPlan "PACKAGE" is a legacy billing mode, not a +// commitment indicator (10-L3). +func TestCloudSQLClient_GetExistingCommitments_ReturnsEmpty(t *testing.T) { ctx := context.Background() client, _ := NewClient(ctx, "test-project", "us-central1") - mockService := &MockSQLAdminService{ - instances: &sqladmin.InstancesListResponse{ - Items: []*sqladmin.DatabaseInstance{ - { - Name: "instance-1", - Region: "us-central1", - State: "RUNNABLE", - DatabaseVersion: "MYSQL_8_0", - Settings: &sqladmin.Settings{ - Tier: "db-n1-standard-1", - PricingPlan: "PACKAGE", - }, - }, - { - Name: "instance-2", - Region: "us-central1", - State: "RUNNABLE", - DatabaseVersion: "POSTGRES_14", - Settings: &sqladmin.Settings{ - Tier: "db-n1-standard-2", - PricingPlan: "PER_USE", // Not a commitment - }, - }, - }, - }, - } - client.SetSQLAdminService(mockService) - commitments, err := client.GetExistingCommitments(ctx) require.NoError(t, err) - require.Len(t, commitments, 1) - assert.Equal(t, "instance-1", commitments[0].CommitmentID) - assert.Equal(t, "db-n1-standard-1", commitments[0].ResourceType) - assert.Equal(t, common.ServiceRelationalDB, commitments[0].Service) -} - -func TestCloudSQLClient_GetExistingCommitments_Error(t *testing.T) { - ctx := context.Background() - client, _ := NewClient(ctx, "test-project", "us-central1") - - mockService := &MockSQLAdminService{ - err: errors.New("API error"), - } - client.SetSQLAdminService(mockService) - - _, err := client.GetExistingCommitments(ctx) - assert.Error(t, err) - assert.Contains(t, err.Error(), "failed to list SQL instances") -} - -func TestCloudSQLClient_GetExistingCommitments_Empty(t *testing.T) { - ctx := context.Background() - client, _ := NewClient(ctx, "test-project", "us-central1") - - mockService := &MockSQLAdminService{ - instances: &sqladmin.InstancesListResponse{ - Items: []*sqladmin.DatabaseInstance{}, - }, - } - client.SetSQLAdminService(mockService) - - commitments, err := client.GetExistingCommitments(ctx) - require.NoError(t, err) - assert.Empty(t, commitments) + assert.Empty(t, commitments, "Cloud SQL GetExistingCommitments must return empty (10-L3)") } func TestCloudSQLClient_GetValidResourceTypes_WithMock(t *testing.T) { @@ -500,28 +444,42 @@ func TestCloudSQLClient_PurchaseCommitment_NotSupported(t *testing.T) { assert.False(t, mockService.insertCalled, "PurchaseCommitment must not call InsertInstance") } -func TestCloudSQLClient_GetOfferingDetails_WithMock(t *testing.T) { - ctx := context.Background() - client, _ := NewClient(ctx, "test-project", "us-central1") - - mockService := &MockBillingService{ - skus: &cloudbilling.ListSkusResponse{ - Skus: []*cloudbilling.Sku{ - { - Description: "db-n1-standard-1 Cloud SQL", - ServiceRegions: []string{"us-central1"}, - PricingInfo: []*cloudbilling.PricingInfo{ +// sqlMockSkus returns a slice with both an on-demand and a commitment SKU for +// the given tier and region. Required by tests that exercise GetOfferingDetails +// after the issue #1020 fix (fabricated commitment prices are no longer allowed; +// both SKUs must be present for getSQLPricing to succeed). +func sqlMockSkus(tier, region string) []*cloudbilling.Sku { + onDemandSKU := &cloudbilling.Sku{ + Description: tier + " Cloud SQL", + ServiceRegions: []string{region}, + PricingInfo: []*cloudbilling.PricingInfo{ + { + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ { - PricingExpression: &cloudbilling.PricingExpression{ - TieredRates: []*cloudbilling.TierRate{ - { - UnitPrice: &cloudbilling.Money{ - Units: 0, - Nanos: 50000000, // 0.05 per hour - CurrencyCode: "USD", - }, - }, - }, + UnitPrice: &cloudbilling.Money{ + Units: 0, + Nanos: 50000000, + CurrencyCode: "USD", + }, + }, + }, + }, + }, + }, + } + commitmentSKU := &cloudbilling.Sku{ + Description: tier + " Cloud SQL commitment 1yr", + ServiceRegions: []string{region}, + PricingInfo: []*cloudbilling.PricingInfo{ + { + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ + { + UnitPrice: &cloudbilling.Money{ + Units: 0, + Nanos: 42000000, + CurrencyCode: "USD", }, }, }, @@ -529,6 +487,17 @@ func TestCloudSQLClient_GetOfferingDetails_WithMock(t *testing.T) { }, }, } + return []*cloudbilling.Sku{onDemandSKU, commitmentSKU} +} + +func TestCloudSQLClient_GetOfferingDetails_WithMock(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + // Both on-demand and commitment SKUs are required after the issue #1020 fix. + mockService := &MockBillingService{ + skus: &cloudbilling.ListSkusResponse{Skus: sqlMockSkus("db-n1-standard-1", "us-central1")}, + } client.SetBillingService(mockService) rec := common.Recommendation{ @@ -549,30 +518,9 @@ func TestCloudSQLClient_GetOfferingDetails_3Year(t *testing.T) { ctx := context.Background() client, _ := NewClient(ctx, "test-project", "us-central1") + // Both on-demand and commitment SKUs are required after the issue #1020 fix. mockService := &MockBillingService{ - skus: &cloudbilling.ListSkusResponse{ - Skus: []*cloudbilling.Sku{ - { - Description: "db-n1-standard-1 Cloud SQL", - ServiceRegions: []string{"us-central1"}, - PricingInfo: []*cloudbilling.PricingInfo{ - { - PricingExpression: &cloudbilling.PricingExpression{ - TieredRates: []*cloudbilling.TierRate{ - { - UnitPrice: &cloudbilling.Money{ - Units: 0, - Nanos: 50000000, - CurrencyCode: "USD", - }, - }, - }, - }, - }, - }, - }, - }, - }, + skus: &cloudbilling.ListSkusResponse{Skus: sqlMockSkus("db-n1-standard-1", "us-central1")}, } client.SetBillingService(mockService) @@ -635,7 +583,8 @@ func TestCloudSQLClient_GetRecommendations_WithMock(t *testing.T) { mockIterator := &MockRecommenderIterator{ recommendations: []*recommenderpb.Recommendation{ { - Name: "recommendation-1", + Name: "recommendation-1", + StateInfo: &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE}, PrimaryImpact: &recommenderpb.Impact{ Category: recommenderpb.Impact_COST, Projection: &recommenderpb.Impact_CostProjection{ @@ -762,7 +711,7 @@ func TestCloudSQLClient_ConvertGCPRecommendation(t *testing.T) { }, } - rec := client.convertGCPRecommendation(ctx, gcpRec) + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) require.NotNil(t, rec) assert.Equal(t, common.ProviderGCP, rec.Provider) assert.Equal(t, common.ServiceRelationalDB, rec.Service) @@ -781,7 +730,114 @@ func TestCloudSQLClient_ConvertGCPRecommendation_NilContent(t *testing.T) { Content: nil, } - rec := client.convertGCPRecommendation(ctx, gcpRec) + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) require.NotNil(t, rec) assert.Equal(t, common.ProviderGCP, rec.Provider) } + +// TestCloudSQLConvertGCPRecommendation_PropagatesTermFromParams is a regression +// test for the finding that convertGCPRecommendation hardcoded rec.Term = "1yr", +// ignoring params.Term. A caller requesting "3yr" must get "3yr" in the output. +// +// This test FAILS on the pre-fix code that always set Term = "1yr". +func TestCloudSQLConvertGCPRecommendation_PropagatesTermFromParams(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + gcpRec := &recommenderpb.Recommendation{Name: "test-rec"} + + tests := []struct { + inputTerm string + wantTerm string + }{ + {"3yr", "3yr"}, + {"1yr", "1yr"}, + {"", "1yr"}, // empty defaults to "1yr" + } + + for _, tt := range tests { + params := common.RecommendationParams{Term: tt.inputTerm} + rec := client.convertGCPRecommendation(ctx, gcpRec, params) + require.NotNil(t, rec) + assert.Equal(t, tt.wantTerm, rec.Term, + "params.Term %q must produce rec.Term %q", tt.inputTerm, tt.wantTerm) + } +} + +// TestGetSQLPricing_CommitmentPriceIsTermTotal is a regression test for the +// unit-mismatch bug where commitmentPrice (per-hour from SKU) was passed +// directly to calculateSQLSavingsPercentage which expects a term total, +// producing a ~99.99% savings percentage. After the fix, CommitmentPrice must +// equal hourlyRate * hoursInTerm and SavingsPercentage must be realistic. +// +// This test FAILS on the pre-fix code where CommitmentPrice == 0.042 (per-hour). +func TestGetSQLPricing_CommitmentPriceIsTermTotal(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + // onDemand = $0.05/hr, commitment = $0.042/hr + // hoursInTerm (1yr) = 8760 + // Expected: CommitmentPrice = 0.042*8760 = 367.92, OnDemandPrice = 0.05*8760 = 438 + // Expected savings ~= (438-367.92)/438*100 ~= 16% + mockService := &MockBillingService{ + skus: &cloudbilling.ListSkusResponse{ + Skus: []*cloudbilling.Sku{ + { + Description: "db-n1-standard-1 Cloud SQL", + ServiceRegions: []string{"us-central1"}, + PricingInfo: []*cloudbilling.PricingInfo{ + { + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ + { + UnitPrice: &cloudbilling.Money{ + Units: 0, + Nanos: 50000000, // $0.05/hr + CurrencyCode: "USD", + }, + }, + }, + }, + }, + }, + }, + { + Description: "db-n1-standard-1 Cloud SQL commitment", + ServiceRegions: []string{"us-central1"}, + PricingInfo: []*cloudbilling.PricingInfo{ + { + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ + { + UnitPrice: &cloudbilling.Money{ + Units: 0, + Nanos: 42000000, // $0.042/hr + CurrencyCode: "USD", + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + client.SetBillingService(mockService) + + pricing, err := client.getSQLPricing(ctx, "db-n1-standard-1", "us-central1", 1) + require.NoError(t, err) + + const hoursInYear = 8760.0 + // CommitmentPrice must be a term total, not the raw per-hour SKU rate. + assert.InDelta(t, 0.042*hoursInYear, pricing.CommitmentPrice, 0.01, + "CommitmentPrice must be commitment SKU hourly rate * hoursInTerm") + // HourlyRate must be the per-hour commitment rate. + assert.InDelta(t, 0.042, pricing.HourlyRate, 0.0001, + "HourlyRate must be the per-hour commitment SKU rate") + // SavingsPercentage must be realistic (not ~99.99%). + assert.Greater(t, pricing.SavingsPercentage, float64(0), + "SavingsPercentage must be positive") + assert.Less(t, pricing.SavingsPercentage, float64(50), + "SavingsPercentage must be realistic (not ~100%)") +} diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index 0248a6c4f..bcae9ae1a 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -4,6 +4,7 @@ package cloudstorage import ( "context" "fmt" + "log" "strings" "time" @@ -17,6 +18,10 @@ import ( "github.com/LeanerCloud/CUDly/pkg/common" ) +// maxRecsPages caps GCP Recommender API iteration to avoid looping forever on a +// stalled or unexpectedly large result set. +const maxRecsPages = 20 + // StorageService interface for storage operations (enables mocking) type StorageService interface { Buckets(ctx context.Context, projectID string) BucketIterator @@ -179,16 +184,31 @@ func (c *CloudStorageClient) GetRecommendations(ctx context.Context, params comm } it := recClient.ListRecommendations(ctx, req) - for { + for pageIdx := 0; ; pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("context cancelled during pagination: %w", err) + } + if pageIdx >= maxRecsPages { + return nil, fmt.Errorf("cloudstorage: GetRecommendations iteration cap (%d items) reached", maxRecsPages) + } rec, err := it.Next() if err == iterator.Done { break } if err != nil { - break + // Iterator errors must propagate so callers don't silently act on a + // partial recommendation list -- see the computeengine client for the + // full rationale (issue #1022 H2 fix). + return nil, fmt.Errorf("cloudstorage: iterate recommendations: %w", err) + } + + // Skip non-ACTIVE recommendations (CLAIMED/SUCCEEDED/FAILED/DISMISSED). + // See computeengine.GetRecommendations for the full rationale. + if rec.GetStateInfo().GetState() != recommenderpb.RecommendationStateInfo_ACTIVE { + continue } - converted := c.convertGCPRecommendation(ctx, rec) + converted := c.convertGCPRecommendation(ctx, rec, params) if converted != nil { recommendations = append(recommendations, *converted) } @@ -197,52 +217,13 @@ func (c *CloudStorageClient) GetRecommendations(ctx context.Context, params comm return recommendations, nil } -// GetExistingCommitments retrieves existing Cloud Storage commitments -func (c *CloudStorageClient) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { - commitments := make([]common.Commitment, 0) - - // Use injected service if available (for testing) - var svc StorageService - if c.storageService != nil { - svc = c.storageService - } else { - client, err := storage.NewClient(ctx, c.clientOpts...) - if err != nil { - return nil, fmt.Errorf("failed to create storage client: %w", err) - } - svc = &realStorageService{client: client} - } - defer svc.Close() - - // List all buckets in the project - it := svc.Buckets(ctx, c.projectID) - for { - bucket, err := it.Next() - if err == iterator.Done { - break - } - if err != nil { - return nil, fmt.Errorf("failed to list buckets: %w", err) - } - - // Check if bucket has committed storage - if bucket.Location == c.region { - commitment := common.Commitment{ - Provider: common.ProviderGCP, - Account: c.projectID, - CommitmentType: common.CommitmentReservedCapacity, - Service: common.ServiceStorage, - Region: c.region, - CommitmentID: bucket.Name, - State: "active", - ResourceType: bucket.StorageClass, - } - - commitments = append(commitments, commitment) - } - } - - return commitments, nil +// GetExistingCommitments returns an empty slice for Cloud Storage. GCP Cloud +// Storage has no commitment API: there is no committed-use discount purchase +// for GCS, and enumerating regional buckets does not represent a commitment -- +// it caused every bucket in a region to appear as a "commitment" in the UI +// (10-L2). Return empty until a proper commitment-detection path is available. +func (c *CloudStorageClient) GetExistingCommitments(_ context.Context) ([]common.Commitment, error) { + return nil, nil } // PurchaseCommitment is intentionally a no-op for Cloud Storage: GCP has no CUD @@ -340,7 +321,9 @@ type StoragePricing struct { SavingsPercentage float64 } -// getStoragePricing gets pricing from GCP Cloud Billing Catalog API +// getStoragePricing gets pricing from GCP Cloud Billing Catalog API. +// It returns an error when commitment pricing is absent from the catalog rather +// than fabricating a price from a hardcoded discount factor (issue #1020). func (c *CloudStorageClient) getStoragePricing(ctx context.Context, storageClass, region string, termYears int) (*StoragePricing, error) { svc, err := c.getOrCreateBillingService(ctx) if err != nil { @@ -356,17 +339,20 @@ func (c *CloudStorageClient) getStoragePricing(ctx context.Context, storageClass if onDemandPrice == 0 { return nil, fmt.Errorf("no pricing found for Cloud Storage class %s", storageClass) } - - hoursInTerm := 8760.0 * float64(termYears) if commitmentPrice == 0 { - commitmentPrice = estimateStorageCommitmentPrice(onDemandPrice, hoursInTerm, termYears) + return nil, fmt.Errorf("no commitment pricing found for Cloud Storage class %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", storageClass, region) } - savingsPercentage := calculateStorageSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice) + hoursInTerm := 8760.0 * float64(termYears) + // Scale the per-unit commitment price to a term total so it is on the + // same basis as onDemandPrice * hoursInTerm. Without this, the savings + // percentage would be nearly 100% (per-unit price vs term total). + commitmentPriceTerm := commitmentPrice * hoursInTerm + savingsPercentage := calculateStorageSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPriceTerm) return &StoragePricing{ - HourlyRate: commitmentPrice / hoursInTerm, - CommitmentPrice: commitmentPrice, + HourlyRate: commitmentPrice, + CommitmentPrice: commitmentPriceTerm, OnDemandPrice: onDemandPrice * hoursInTerm, Currency: currency, SavingsPercentage: savingsPercentage, @@ -435,15 +421,6 @@ func extractStoragePriceFromSKU(sku *cloudbilling.Sku) (float64, string) { return price, rate.UnitPrice.CurrencyCode } -// estimateStorageCommitmentPrice estimates commitment price based on GCP storage savings -func estimateStorageCommitmentPrice(onDemandPrice, hoursInTerm float64, termYears int) float64 { - discount := 0.75 // 25% savings for 1 year - if termYears == 3 { - discount = 0.70 // 30% savings for 3 years - } - return onDemandPrice * hoursInTerm * discount -} - // calculateStorageSavingsPercentage calculates the savings percentage func calculateStorageSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice float64) float64 { onDemandTotal := onDemandPrice * hoursInTerm @@ -470,8 +447,76 @@ func skuMatchesStorageClass(sku *cloudbilling.Sku, storageClass, region string) return true } -// convertGCPRecommendation converts a GCP Recommender recommendation to common format -func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation) *common.Recommendation { +// extractResourceTypeFromContent extracts the last path segment of the first +// non-empty Operation.Resource across all operation groups. Used by all four +// GCP service converters to set rec.ResourceType from Recommender payloads. +func extractResourceTypeFromContent(content *recommenderpb.RecommendationContent) string { + if content == nil || content.OperationGroups == nil { + return "" + } + for _, opGroup := range content.OperationGroups { + for _, op := range opGroup.Operations { + if op.Resource == "" { + continue + } + parts := strings.Split(op.Resource, "/") + if len(parts) > 0 { + return parts[len(parts)-1] + } + } + } + return "" +} + +// extractEstimatedSavings returns the negative of the PrimaryImpact cost +// projection (GCP encodes savings as a negative cost delta). +func extractEstimatedSavings(gcpRec *recommenderpb.Recommendation) float64 { + if gcpRec.PrimaryImpact == nil { + return 0 + } + costProj := gcpRec.PrimaryImpact.GetCostProjection() + if costProj == nil || costProj.Cost == nil { + return 0 + } + cost := costProj.Cost + return -(float64(cost.Units) + float64(cost.Nanos)/1e9) +} + +// fillStoragePricing calls getStoragePricing and, on success, writes CommitmentCost, +// OnDemandCost, SavingsPercentage, and BreakEvenMonths into rec. Pricing +// failures are logged and do not discard the recommendation. +func (c *CloudStorageClient) fillStoragePricing(ctx context.Context, rec *common.Recommendation, termYears int) { + pricing, err := c.getStoragePricing(ctx, rec.ResourceType, c.region, termYears) + if err != nil { + log.Printf("cloudstorage: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err) + return + } + rec.CommitmentCost = pricing.CommitmentPrice + rec.OnDemandCost = pricing.OnDemandPrice + rec.SavingsPercentage = pricing.SavingsPercentage + if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 { + monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12) + if monthlySavings > 0 { + rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings + } + } +} + +// convertGCPRecommendation converts a GCP Recommender recommendation to common format. +// It also calls getStoragePricing to fill CommitmentCost/OnDemandCost/SavingsPercentage/ +// BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly +// (issue #1022 C2). Pricing failures are logged but do not discard the recommendation. +func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation { + paymentOption := params.PaymentOption + if paymentOption == "" { + paymentOption = "monthly" + } + + term := params.Term + if term == "" { + term = "1yr" + } + rec := &common.Recommendation{ Provider: common.ProviderGCP, Service: common.ServiceStorage, @@ -479,34 +524,21 @@ func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRe Region: c.region, CommitmentType: common.CommitmentReservedCapacity, Timestamp: time.Now(), - Term: "1yr", - PaymentOption: "monthly", + Term: term, + PaymentOption: paymentOption, } - // Extract resource type from recommendation content - if gcpRec.Content != nil { - if gcpRec.Content.OperationGroups != nil { - for _, opGroup := range gcpRec.Content.OperationGroups { - for _, op := range opGroup.Operations { - if op.Resource != "" { - parts := strings.Split(op.Resource, "/") - if len(parts) > 0 { - rec.ResourceType = parts[len(parts)-1] - } - } - } - } - } - } + rec.ResourceType = extractResourceTypeFromContent(gcpRec.Content) + rec.EstimatedSavings = extractEstimatedSavings(gcpRec) - // Extract cost impact - if gcpRec.PrimaryImpact != nil { - // Use GetCostProjection() method to access the cost projection - if costProj := gcpRec.PrimaryImpact.GetCostProjection(); costProj != nil && costProj.Cost != nil { - cost := costProj.Cost - savings := -(float64(cost.Units) + float64(cost.Nanos)/1e9) - rec.EstimatedSavings = savings + // Thread pricing into the converter so the scorer can rank/filter GCP recs + // correctly (issue #1022 C2). + if rec.ResourceType != "" { + termYears := 1 + if rec.Term == "3yr" || rec.Term == "3" { + termYears = 3 } + c.fillStoragePricing(ctx, rec, termYears) } return rec diff --git a/providers/gcp/services/cloudstorage/client_test.go b/providers/gcp/services/cloudstorage/client_test.go index 9ee3c648b..78b874d0b 100644 --- a/providers/gcp/services/cloudstorage/client_test.go +++ b/providers/gcp/services/cloudstorage/client_test.go @@ -301,69 +301,16 @@ func TestCloudStorageClient_SetterMethods(t *testing.T) { assert.Equal(t, mockBilling, client.billingService) } -func TestCloudStorageClient_GetExistingCommitments_WithMock(t *testing.T) { +// TestCloudStorageClient_GetExistingCommitments_ReturnsEmpty asserts that +// GetExistingCommitments always returns an empty slice. GCS has no commitment +// API; enumerating regional buckets does not represent a commitment (10-L2). +func TestCloudStorageClient_GetExistingCommitments_ReturnsEmpty(t *testing.T) { ctx := context.Background() client, _ := NewClient(ctx, "test-project", "us-central1") - mockService := &MockStorageService{ - buckets: []*storage.BucketAttrs{ - { - Name: "bucket-1", - Location: "us-central1", - StorageClass: "STANDARD", - }, - { - Name: "bucket-2", - Location: "us-central1", - StorageClass: "NEARLINE", - }, - { - Name: "bucket-other-region", - Location: "europe-west1", - StorageClass: "COLDLINE", - }, - }, - } - client.SetStorageService(mockService) - commitments, err := client.GetExistingCommitments(ctx) require.NoError(t, err) - // Only buckets in the matching region should be returned - assert.Len(t, commitments, 2) - assert.Equal(t, "bucket-1", commitments[0].CommitmentID) - assert.Equal(t, "STANDARD", commitments[0].ResourceType) - assert.Equal(t, common.ProviderGCP, commitments[0].Provider) - assert.Equal(t, common.ServiceStorage, commitments[0].Service) - assert.Equal(t, "bucket-2", commitments[1].CommitmentID) - assert.Equal(t, "NEARLINE", commitments[1].ResourceType) -} - -func TestCloudStorageClient_GetExistingCommitments_Error(t *testing.T) { - ctx := context.Background() - client, _ := NewClient(ctx, "test-project", "us-central1") - - mockService := &MockStorageService{ - listErr: errors.New("API error"), - } - client.SetStorageService(mockService) - - _, err := client.GetExistingCommitments(ctx) - assert.Error(t, err) - assert.Contains(t, err.Error(), "failed to list buckets") -} - -func TestCloudStorageClient_GetExistingCommitments_Empty(t *testing.T) { - ctx := context.Background() - client, _ := NewClient(ctx, "test-project", "us-central1") - - mockService := &MockStorageService{ - buckets: []*storage.BucketAttrs{}, - } - client.SetStorageService(mockService) - - commitments, err := client.GetExistingCommitments(ctx) - require.NoError(t, err) - assert.Empty(t, commitments) + assert.Empty(t, commitments, "Cloud Storage GetExistingCommitments must return empty (10-L2)") } // TestCloudStorageClient_PurchaseCommitment_NotSupported is the regression test for @@ -401,7 +348,8 @@ func TestCloudStorageClient_GetRecommendations_WithMock(t *testing.T) { mockClient := &MockRecommenderClient{ recommendations: []*recommenderpb.Recommendation{ { - Name: "recommendation-1", + Name: "recommendation-1", + StateInfo: &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE}, PrimaryImpact: &recommenderpb.Impact{ Category: recommenderpb.Impact_COST, Projection: &recommenderpb.Impact_CostProjection{ @@ -460,32 +408,51 @@ func TestCloudStorageClient_GetRecommendations_IteratorError(t *testing.T) { } client.SetRecommenderClient(mockClient) + // Iterator errors now propagate (issue #1022 H2 fix) -- they must not be + // silently swallowed, as that would mask auth/quota failures and cause callers + // to act on a partial (empty) recommendation list. recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) - require.NoError(t, err) // Error during iteration is handled gracefully - assert.Empty(t, recommendations) + require.Error(t, err) + assert.Contains(t, err.Error(), "cloudstorage: iterate recommendations") + assert.Nil(t, recommendations) } -func TestCloudStorageClient_GetOfferingDetails_WithMock(t *testing.T) { - ctx := context.Background() - client, _ := NewClient(ctx, "test-project", "us-central1") - - mockService := &MockBillingService{ - skus: &cloudbilling.ListSkusResponse{ - Skus: []*cloudbilling.Sku{ +// storageMockSkus returns a slice with both an on-demand and a commitment SKU for +// the given storage class and region. Required by tests that exercise GetOfferingDetails +// after the issue #1020 fix (fabricated commitment prices are no longer allowed). +func storageMockSkus(storageClass, region string, onDemandNanos, commitmentNanos int64) []*cloudbilling.Sku { + return []*cloudbilling.Sku{ + { + Description: storageClass + " Storage in " + region, + ServiceRegions: []string{region}, + PricingInfo: []*cloudbilling.PricingInfo{ { - Description: "Standard Storage in us-central1", - ServiceRegions: []string{"us-central1"}, - PricingInfo: []*cloudbilling.PricingInfo{ - { - PricingExpression: &cloudbilling.PricingExpression{ - TieredRates: []*cloudbilling.TierRate{ - { - UnitPrice: &cloudbilling.Money{ - Units: 0, - Nanos: 26000000, - CurrencyCode: "USD", - }, - }, + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ + { + UnitPrice: &cloudbilling.Money{ + Units: 0, + Nanos: onDemandNanos, + CurrencyCode: "USD", + }, + }, + }, + }, + }, + }, + }, + { + Description: storageClass + " Storage commitment in " + region, + ServiceRegions: []string{region}, + PricingInfo: []*cloudbilling.PricingInfo{ + { + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ + { + UnitPrice: &cloudbilling.Money{ + Units: 0, + Nanos: commitmentNanos, + CurrencyCode: "USD", }, }, }, @@ -494,6 +461,18 @@ func TestCloudStorageClient_GetOfferingDetails_WithMock(t *testing.T) { }, }, } +} + +func TestCloudStorageClient_GetOfferingDetails_WithMock(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + // Both on-demand and commitment SKUs required after the issue #1020 fix. + mockService := &MockBillingService{ + skus: &cloudbilling.ListSkusResponse{ + Skus: storageMockSkus("STANDARD", "us-central1", 26000000, 19500000), + }, + } client.SetBillingService(mockService) rec := common.Recommendation{ @@ -516,29 +495,10 @@ func TestCloudStorageClient_GetOfferingDetails_3yr(t *testing.T) { ctx := context.Background() client, _ := NewClient(ctx, "test-project", "us-central1") + // Both on-demand and commitment SKUs required after the issue #1020 fix. mockService := &MockBillingService{ skus: &cloudbilling.ListSkusResponse{ - Skus: []*cloudbilling.Sku{ - { - Description: "Nearline Storage in us-central1", - ServiceRegions: []string{"us-central1"}, - PricingInfo: []*cloudbilling.PricingInfo{ - { - PricingExpression: &cloudbilling.PricingExpression{ - TieredRates: []*cloudbilling.TierRate{ - { - UnitPrice: &cloudbilling.Money{ - Units: 0, - Nanos: 10000000, - CurrencyCode: "USD", - }, - }, - }, - }, - }, - }, - }, - }, + Skus: storageMockSkus("NEARLINE", "us-central1", 10000000, 7000000), }, } client.SetBillingService(mockService) @@ -599,29 +559,10 @@ func TestCloudStorageClient_GetOfferingDetails_DefaultPaymentOption(t *testing.T ctx := context.Background() client, _ := NewClient(ctx, "test-project", "us-central1") + // Both on-demand and commitment SKUs required after the issue #1020 fix. mockService := &MockBillingService{ skus: &cloudbilling.ListSkusResponse{ - Skus: []*cloudbilling.Sku{ - { - Description: "Standard Storage in us-central1", - ServiceRegions: []string{"us-central1"}, - PricingInfo: []*cloudbilling.PricingInfo{ - { - PricingExpression: &cloudbilling.PricingExpression{ - TieredRates: []*cloudbilling.TierRate{ - { - UnitPrice: &cloudbilling.Money{ - Units: 0, - Nanos: 26000000, - CurrencyCode: "USD", - }, - }, - }, - }, - }, - }, - }, - }, + Skus: storageMockSkus("STANDARD", "us-central1", 26000000, 19500000), }, } client.SetBillingService(mockService) @@ -666,7 +607,7 @@ func TestCloudStorageClient_ConvertGCPRecommendation(t *testing.T) { }, } - rec := client.convertGCPRecommendation(ctx, gcpRec) + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) require.NotNil(t, rec) assert.Equal(t, common.ProviderGCP, rec.Provider) assert.Equal(t, common.ServiceStorage, rec.Service) @@ -686,7 +627,7 @@ func TestCloudStorageClient_ConvertGCPRecommendation_NilContent(t *testing.T) { Content: nil, } - rec := client.convertGCPRecommendation(ctx, gcpRec) + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) require.NotNil(t, rec) assert.Equal(t, common.ProviderGCP, rec.Provider) assert.Empty(t, rec.ResourceType) @@ -710,7 +651,7 @@ func TestCloudStorageClient_ConvertGCPRecommendation_NilPrimaryImpact(t *testing }, } - rec := client.convertGCPRecommendation(ctx, gcpRec) + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) require.NotNil(t, rec) assert.Equal(t, float64(0), rec.EstimatedSavings) } @@ -769,14 +710,80 @@ func TestCloudStorageClient_GetStoragePricing_WithCommitmentPrice(t *testing.T) require.NoError(t, err) assert.Equal(t, "USD", pricing.Currency) assert.Greater(t, pricing.OnDemandPrice, float64(0)) - // When commitment price is found, it should be used - assert.Equal(t, float64(0.02), pricing.CommitmentPrice) + // CommitmentPrice is the term total (per-unit SKU price * hoursInTerm). + // onDemand unit = 0.026, commitment unit = 0.020, hoursInTerm = 8760. + assert.InDelta(t, 0.02*8760, pricing.CommitmentPrice, 0.01) + // HourlyRate is the per-unit commitment price (not divided by hoursInTerm). + assert.InDelta(t, 0.02, pricing.HourlyRate, 0.0001) + // SavingsPercentage should be positive and less than 100. + assert.Greater(t, pricing.SavingsPercentage, float64(0)) + assert.Less(t, pricing.SavingsPercentage, float64(100)) } func TestCloudStorageClient_GetStoragePricing_3Year(t *testing.T) { ctx := context.Background() client, _ := NewClient(ctx, "test-project", "us-central1") + // Both on-demand and commitment SKUs required after the issue #1020 fix: + // without a commitment SKU, getStoragePricing returns an error. + mockService := &MockBillingService{ + skus: &cloudbilling.ListSkusResponse{ + Skus: storageMockSkus("STANDARD", "us-central1", 26000000, 18200000), + }, + } + client.SetBillingService(mockService) + + pricing, err := client.getStoragePricing(ctx, "STANDARD", "us-central1", 3) + require.NoError(t, err) + assert.Greater(t, pricing.SavingsPercentage, float64(0)) + assert.Greater(t, pricing.OnDemandPrice, float64(0)) + assert.Greater(t, pricing.CommitmentPrice, float64(0)) +} + +// TestConvertGCPRecommendation_PropagatesTermFromParams is a regression test for +// the finding that convertGCPRecommendation hardcoded rec.Term = "1yr", +// ignoring params.Term. A caller requesting "3yr" must get "3yr" in the output. +// +// This test FAILS on the pre-fix code that always set Term = "1yr". +func TestCloudStorageConvertGCPRecommendation_PropagatesTermFromParams(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + gcpRec := &recommenderpb.Recommendation{Name: "test-rec"} + + tests := []struct { + inputTerm string + wantTerm string + }{ + {"3yr", "3yr"}, + {"1yr", "1yr"}, + {"", "1yr"}, // empty defaults to "1yr" + } + + for _, tt := range tests { + params := common.RecommendationParams{Term: tt.inputTerm} + rec := client.convertGCPRecommendation(ctx, gcpRec, params) + require.NotNil(t, rec) + assert.Equal(t, tt.wantTerm, rec.Term, + "params.Term %q must produce rec.Term %q", tt.inputTerm, tt.wantTerm) + } +} + +// TestGetStoragePricing_CommitmentPriceIsTermTotal is a regression test for the +// unit-mismatch bug where commitmentPrice (per-unit from SKU) was passed +// directly to calculateStorageSavingsPercentage which expects a term total, +// producing a ~99.99% savings percentage. After the fix, CommitmentPrice must +// equal unitRate * hoursInTerm and SavingsPercentage must be realistic. +// +// This test FAILS on the pre-fix code where CommitmentPrice == 0.02 (per-unit). +func TestGetStoragePricing_CommitmentPriceIsTermTotal(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + // onDemand = $0.026/unit, commitment = $0.020/unit + // hoursInTerm (1yr) = 8760 + // Expected: CommitmentPrice = 0.020*8760 = 175.2, OnDemandPrice = 0.026*8760 = 227.76 + // Expected savings ~= (227.76-175.2)/227.76*100 ~= 23% mockService := &MockBillingService{ skus: &cloudbilling.ListSkusResponse{ Skus: []*cloudbilling.Sku{ @@ -790,7 +797,26 @@ func TestCloudStorageClient_GetStoragePricing_3Year(t *testing.T) { { UnitPrice: &cloudbilling.Money{ Units: 0, - Nanos: 26000000, + Nanos: 26000000, // $0.026/unit + CurrencyCode: "USD", + }, + }, + }, + }, + }, + }, + }, + { + Description: "Standard Storage Commitment in us-central1", + ServiceRegions: []string{"us-central1"}, + PricingInfo: []*cloudbilling.PricingInfo{ + { + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ + { + UnitPrice: &cloudbilling.Money{ + Units: 0, + Nanos: 20000000, // $0.020/unit CurrencyCode: "USD", }, }, @@ -804,10 +830,23 @@ func TestCloudStorageClient_GetStoragePricing_3Year(t *testing.T) { } client.SetBillingService(mockService) - pricing, err := client.getStoragePricing(ctx, "STANDARD", "us-central1", 3) + pricing, err := client.getStoragePricing(ctx, "STANDARD", "us-central1", 1) require.NoError(t, err) - // 3-year should have 30% savings vs 25% for 1-year - assert.Greater(t, pricing.SavingsPercentage, float64(25)) + + const hoursInYear = 8760.0 + // CommitmentPrice must be a term total, not the raw per-unit SKU rate. + assert.InDelta(t, 0.02*hoursInYear, pricing.CommitmentPrice, 0.01, + "CommitmentPrice must be commitment SKU unit rate * hoursInTerm") + // HourlyRate must be the per-unit commitment rate. + assert.InDelta(t, 0.02, pricing.HourlyRate, 0.0001, + "HourlyRate must be the per-unit commitment SKU rate") + // OnDemandPrice stays a term total. + assert.InDelta(t, 0.026*hoursInYear, pricing.OnDemandPrice, 0.01) + // SavingsPercentage must be ~23%, not ~99.99%. + assert.Greater(t, pricing.SavingsPercentage, float64(1), + "SavingsPercentage must not be ~99.99% (unit-mismatch bug)") + assert.Less(t, pricing.SavingsPercentage, float64(60), + "SavingsPercentage must be a realistic storage commitment discount") } func TestSkuMatchesStorageClass_CaseInsensitive(t *testing.T) { diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 7e598af71..2af9cfa83 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -15,8 +15,12 @@ import ( "cloud.google.com/go/recommender/apiv1/recommenderpb" gax "github.com/googleapis/gax-go/v2" "google.golang.org/api/cloudbilling/v1" + "google.golang.org/api/googleapi" "google.golang.org/api/iterator" "google.golang.org/api/option" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "google.golang.org/protobuf/types/known/structpb" "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/retry" @@ -32,6 +36,26 @@ const maxCommitmentsPages = 50 // maxMachineTypeItems caps GCP machine types iteration (one item per Next() call). const maxMachineTypeItems = 20 +// termPlan converts a commitment term string to the canonical GCP Compute API +// commitment plan value derived from the SDK enum constants. +// +// Accepted forms: +// - 1-year: "1yr", "1", "12mo" +// - 3-year: "3yr", "3", "36mo" +// +// An empty or unrecognised term returns an error rather than silently defaulting +// to 12 months; a silent mis-default can purchase the wrong term and waste money. +func termPlan(term string) (string, error) { + switch strings.ToLower(strings.TrimSpace(term)) { + case "3yr", "3", "36mo": + return computepb.Commitment_THIRTY_SIX_MONTH.String(), nil + case "1yr", "1", "12mo": + return computepb.Commitment_TWELVE_MONTH.String(), nil + default: + return "", fmt.Errorf("termPlan: unrecognised commitment term %q (accepted: 1yr/1/12mo or 3yr/3/36mo)", term) + } +} + // CommitmentsService interface for commitments operations (enables mocking) type CommitmentsService interface { List(ctx context.Context, req *computepb.ListRegionCommitmentsRequest) CommitmentsIterator @@ -234,7 +258,15 @@ func (c *ComputeEngineClient) GetRecommendations(ctx context.Context, params com return nil, fmt.Errorf("computeengine: iterate recommendations: %w", err) } - converted := c.convertGCPRecommendation(ctx, rec) + // Skip non-ACTIVE recommendations (CLAIMED/SUCCEEDED/FAILED/DISMISSED). + // The GCP Recommender returns all states unless filtered at the API layer; + // acting on an already-CLAIMED or SUCCEEDED recommendation is a + // cross-run double-purchase vector and inflates actionable rec counts. + if rec.GetStateInfo().GetState() != recommenderpb.RecommendationStateInfo_ACTIVE { + continue + } + + converted := c.convertGCPRecommendation(ctx, rec, params) if converted != nil { recommendations = append(recommendations, *converted) } @@ -312,10 +344,11 @@ func (c *ComputeEngineClient) convertGCPCommitmentToCommon(commitment *computepb status = strings.ToLower(*commitment.Status) } + // All commitment types (GENERAL_PURPOSE, ACCELERATOR) map to CommitmentCUD + // for the purposes of the common layer. The if-branch checking for + // "GENERAL_PURPOSE" was a no-op (both arms assigned CommitmentCUD) and + // is removed (10-L1). commitmentType := common.CommitmentCUD - if commitment.Type != nil && *commitment.Type == "GENERAL_PURPOSE" { - commitmentType = common.CommitmentCUD - } com := common.Commitment{ Provider: common.ProviderGCP, @@ -341,7 +374,7 @@ func (c *ComputeEngineClient) convertGCPCommitmentToCommon(commitment *computepb // ResourceCommitment represents a single resource within a GCP commitment. type ResourceCommitment struct { Amount int64 // number of vCPUs or memory in MB - Type string // "VCPU" or "MEMORY_MB" + Type string // GCP ResourceCommitment.Type enum: "VCPU" or "MEMORY" } // CommitmentRequest represents a single GCP commitment to create. @@ -353,13 +386,18 @@ type CommitmentRequest struct { } // GroupCommitments groups recommendations by project+region+term into CommitmentRequests. -// GCP requires both VCPU and MEMORY_MB in a single commitments.insert call. -// Each recommendation's Count is treated as vCPU count; memory is estimated at 4 GB per vCPU. +// GCP requires both a VCPU and a MEMORY resource (memory Amount in MB) in a +// single commitments.insert call. +// Each recommendation's Count is treated as vCPU count; the memory Amount is +// read from ComputeDetails.MemoryGB (populated by extractMemoryMBFromRecommendation). +// Recommendations with an unrecognised term or missing memory are skipped with a +// log warning so a bad rec never contaminates an otherwise valid group. func GroupCommitments(recs []common.Recommendation) []CommitmentRequest { type key struct{ account, region, term string } type agg struct { - vcpus int64 - plan string + vcpus int64 + memoryMB int64 + plan string } groups := make(map[key]*agg) @@ -367,15 +405,22 @@ func GroupCommitments(recs []common.Recommendation) []CommitmentRequest { if rec.Service != common.ServiceCompute || rec.Provider != common.ProviderGCP { continue } + plan, err := termPlan(rec.Term) + if err != nil { + log.Printf("GroupCommitments: skipping recommendation with unrecognised term %q: %v", rec.Term, err) + continue + } + recMemMB, err := memoryMBFromDetails(rec) + if err != nil { + log.Printf("GroupCommitments: skipping recommendation missing memory amount: %v", err) + continue + } k := key{account: rec.Account, region: rec.Region, term: rec.Term} if _, ok := groups[k]; !ok { - plan := "TWELVE_MONTH" - if rec.Term == "3yr" || rec.Term == "3" { - plan = "THIRTY_SIX_MONTH" - } groups[k] = &agg{plan: plan} } groups[k].vcpus += int64(rec.Count) + groups[k].memoryMB += recMemMB } result := make([]CommitmentRequest, 0, len(groups)) @@ -388,7 +433,9 @@ func GroupCommitments(recs []common.Recommendation) []CommitmentRequest { Region: k.region, Resources: []ResourceCommitment{ {Type: "VCPU", Amount: a.vcpus}, - {Type: "MEMORY_MB", Amount: a.vcpus * 4096}, + // "MEMORY" is the GCP ResourceCommitment.Type enum member; the + // Amount is in MB (see buildInsertRequest, issue #1022). + {Type: "MEMORY", Amount: a.memoryMB}, }, }) counter++ @@ -396,11 +443,26 @@ func GroupCommitments(recs []common.Recommendation) []CommitmentRequest { return result } -// isResourceExhausted reports whether the error represents a RESOURCE_EXHAUSTED (429) response. +// isResourceExhausted reports whether the error represents a RESOURCE_EXHAUSTED +// (quota / 429) response. Uses typed checks first: errors.As for REST API errors +// (*googleapi.Error with Code 429) and status.FromError for gRPC errors +// (codes.ResourceExhausted). Falls back to string matching only for error types +// that neither unwraps as a *googleapi.Error nor as a gRPC status (10-M3). func isResourceExhausted(err error) bool { if err == nil { return false } + // REST path: *googleapi.Error wraps HTTP 429 Too Many Requests. + var gapiErr *googleapi.Error + if errors.As(err, &gapiErr) { + return gapiErr.Code == 429 + } + // gRPC path: RESOURCE_EXHAUSTED maps to quota-exceeded responses from + // Google Cloud APIs served over gRPC (e.g. quota for Recommender). + if st, ok := status.FromError(err); ok { + return st.Code() == codes.ResourceExhausted + } + // Fallback for wrapped/non-standard errors that carry the signal as text. s := err.Error() return strings.Contains(s, "ResourceExhausted") || strings.Contains(s, "RESOURCE_EXHAUSTED") || strings.Contains(s, "429") } @@ -464,7 +526,11 @@ func (c *ComputeEngineClient) PurchaseCommitment(ctx context.Context, rec common } defer svc.Close() - insertReq, commitmentName := c.buildInsertRequest(rec, opts) + insertReq, commitmentName, buildErr := c.buildInsertRequest(rec, opts) + if buildErr != nil { + result.Error = buildErr + return result, buildErr + } // Exponential backoff on RESOURCE_EXHAUSTED: BaseDelay 1s with 2× growth // capped at MaxDelay 4s gives the same 1s/2s/4s sequence the open-coded @@ -508,8 +574,9 @@ func (c *ComputeEngineClient) PurchaseCommitment(ctx context.Context, rec common // buildInsertRequest assembles the RegionCommitments.Insert request for a // purchase, threading opts.IdempotencyToken into both GCP idempotency levers so // a re-drive of the same execution cannot create a second CUD (issue #654, the -// financial double-buy). It returns the request and the commitment name (used as -// the resulting CommitmentID). +// financial double-buy). It returns the request, the commitment name (used as +// the resulting CommitmentID), and an error when rec.Count <= 0 (which would +// produce a zero-vCPU / zero-MB commitment that GCP rejects or silently wastes). // // - RequestId is GCP's native server-side idempotency key on Insert, which the // API documents as preventing clients from accidentally creating duplicate @@ -526,14 +593,18 @@ func (c *ComputeEngineClient) PurchaseCommitment(ctx context.Context, rec common // An empty token preserves the prior non-idempotent timestamp-based name (the // CLI path, which has no owning execution). The token is masked in logs via // common.MaskToken and never logged verbatim. -func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts common.PurchaseOptions) (*computepb.InsertRegionCommitmentRequest, string) { - plan := "TWELVE_MONTH" - if rec.Term == "3yr" || rec.Term == "3" { - plan = "THIRTY_SIX_MONTH" +func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts common.PurchaseOptions) (*computepb.InsertRegionCommitmentRequest, string, error) { + if rec.Count <= 0 { + return nil, "", fmt.Errorf("buildInsertRequest: rec.Count must be > 0 (got %d); a zero-vCPU commitment is invalid (issue #1022)", rec.Count) + } + + plan, err := termPlan(rec.Term) + if err != nil { + return nil, "", fmt.Errorf("buildInsertRequest: %w", err) } // GCP's computepb.Commitment has no Labels field, and the RegionCommitments - // client exposes no SetLabels call — CUDs cannot be tagged or labeled via + // client exposes no SetLabels call -- CUDs cannot be tagged or labeled via // the API. Encode the source into Description so customers can still filter // with `gcloud compute commitments list --filter="description:..."`. description := fmt.Sprintf("CUD for %s", rec.ResourceType) @@ -543,7 +614,19 @@ func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts commitmentName := idempotentCommitmentName(opts.IdempotencyToken) - // GCP requires both VCPU and MEMORY_MB in a single commitment insert. + // Derive the memory Amount from the Recommender payload (stored in + // ComputeDetails.MemoryGB by extractMemoryMBFromRecommendation). Using the + // real payload value is required for non-standard families (e.g. high-memory + // N2 uses 6 GB/vCPU rather than the GENERAL_PURPOSE 4 GB). We fail loud when + // the payload omits memory rather than silently falling back to a ratio that + // would produce an incorrect commitment for any non-standard family. + memMB, err := memoryMBFromDetails(rec) + if err != nil { + return nil, "", err + } + + // GCP requires both a VCPU and a MEMORY resource (memory Amount in MB) in a + // single commitment insert. commitment := &computepb.Commitment{ Name: stringPtr(commitmentName), Plan: stringPtr(plan), @@ -555,8 +638,12 @@ func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts Amount: int64Ptr(int64(rec.Count)), }, { - Type: stringPtr("MEMORY_MB"), - Amount: int64Ptr(int64(rec.Count) * 4096), + // GCP's ResourceCommitment.Type enum is VCPU/MEMORY/LOCAL_SSD/ + // ACCELERATOR; the memory member is "MEMORY" (the Amount is still in + // MB). Sending "MEMORY_MB" is an invalid enum value and GCP rejects + // the commitments.insert, failing the purchase (issue #1022). + Type: stringPtr("MEMORY"), + Amount: int64Ptr(memMB), }, }, } @@ -572,7 +659,7 @@ func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts common.MaskToken(opts.IdempotencyToken), commitmentName) } - return insertReq, commitmentName + return insertReq, commitmentName, nil } // ValidateOffering validates that a machine type exists @@ -689,7 +776,9 @@ type ComputePricing struct { SavingsPercentage float64 } -// getComputePricing gets pricing from GCP Cloud Billing Catalog API +// getComputePricing gets pricing from GCP Cloud Billing Catalog API. +// It returns an error when commitment pricing is absent from the catalog rather +// than fabricating a price from a hardcoded discount factor (issue #1020). func (c *ComputeEngineClient) getComputePricing(ctx context.Context, machineType, region string, termYears int) (*ComputePricing, error) { svc, err := c.getOrCreateBillingService(ctx) if err != nil { @@ -705,12 +794,11 @@ func (c *ComputeEngineClient) getComputePricing(ctx context.Context, machineType if onDemandPrice == 0 { return nil, fmt.Errorf("no on-demand pricing found for machine type %s", machineType) } - - hoursInTerm := 8760.0 * float64(termYears) if commitmentPrice == 0 { - commitmentPrice = estimateComputeCommitmentPrice(onDemandPrice, hoursInTerm, termYears) + return nil, fmt.Errorf("no commitment pricing found for machine type %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", machineType, region) } + hoursInTerm := 8760.0 * float64(termYears) savingsPercentage := calculateComputeSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice) return &ComputePricing{ @@ -784,15 +872,6 @@ func extractComputePriceFromSKU(sku *cloudbilling.Sku) (float64, string) { return price, rate.UnitPrice.CurrencyCode } -// estimateComputeCommitmentPrice estimates commitment price based on GCP CUD discounts -func estimateComputeCommitmentPrice(onDemandPrice, hoursInTerm float64, termYears int) float64 { - discount := 0.63 // 37% savings for 1 year - if termYears == 3 { - discount = 0.45 // 55% savings for 3 years - } - return onDemandPrice * hoursInTerm * discount -} - // calculateComputeSavingsPercentage calculates the savings percentage func calculateComputeSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice float64) float64 { onDemandTotal := onDemandPrice * hoursInTerm @@ -819,8 +898,35 @@ func skuMatchesMachineType(sku *cloudbilling.Sku, machineType, region string) bo return true } -// convertGCPRecommendation converts a GCP Recommender recommendation to common format -func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation) *common.Recommendation { +// convertGCPRecommendation converts a GCP Recommender recommendation to common format. +// It also calls getComputePricing to fill CommitmentCost/OnDemandCost/SavingsPercentage/ +// BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly +// (issue #1022 C2). Pricing failures are logged but do not discard the recommendation: +// EstimatedSavings from the Recommender payload is the authoritative savings signal. +// Returns nil when the params.Term is unrecognised so the caller skips an +// unroutable recommendation rather than queuing a purchase with an invalid plan. +func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation { + // GCP CUDs are billed monthly with no upfront option; force "monthly" + // unconditionally and log any non-monthly input so scheduler + // misconfiguration is visible. Supersedes the monthly stamp introduced + // in PR #829 (which stamped "monthly" in the purchase body but not in + // the converter default -- see PR #1047 for the supersession note). + if params.PaymentOption != "" && !strings.EqualFold(params.PaymentOption, "monthly") { + log.Printf("computeengine: unsupported GCP payment option %q; forcing monthly", params.PaymentOption) + } + paymentOption := "monthly" + + // H-3: propagate params.Term (default "1yr") and validate it so an + // unrecognised term fails loud here rather than reaching buildInsertRequest. + term := params.Term + if term == "" { + term = "1yr" + } + if _, err := termPlan(term); err != nil { + log.Printf("computeengine: skipping recommendation with unrecognised term %q: %v", term, err) + return nil + } + rec := &common.Recommendation{ Provider: common.ProviderGCP, Service: common.ServiceCompute, @@ -828,16 +934,54 @@ func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpR Region: c.region, CommitmentType: common.CommitmentCUD, Timestamp: time.Now(), - Term: "1yr", - PaymentOption: "upfront", + Term: term, + PaymentOption: paymentOption, } extractResourceTypeFromRecommendation(gcpRec, rec) extractCostImpactFromRecommendation(gcpRec, rec) + extractVCPUCountFromRecommendation(gcpRec, rec) + extractMemoryMBFromRecommendation(gcpRec, rec) + + c.enrichRecWithPricing(ctx, rec) return rec } +// enrichRecWithPricing fills CommitmentCost, OnDemandCost, SavingsPercentage, +// and BreakEvenMonths on rec by querying the billing catalog. Extracted from +// convertGCPRecommendation to keep that function's cyclomatic complexity under +// the gocyclo gate. A missing or invalid pricing entry is logged but does not +// discard the recommendation -- the Recommender-derived EstimatedSavings is +// still the authoritative savings signal (issue #1022 C2). +func (c *ComputeEngineClient) enrichRecWithPricing(ctx context.Context, rec *common.Recommendation) { + if rec.ResourceType == "" { + return + } + termYears := 1 + if rec.Term == "3yr" || rec.Term == "3" { + termYears = 3 + } + pricing, err := c.getComputePricing(ctx, rec.ResourceType, c.region, termYears) + if err != nil { + log.Printf("computeengine: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err) + return + } + rec.CommitmentCost = pricing.CommitmentPrice + rec.OnDemandCost = pricing.OnDemandPrice + rec.SavingsPercentage = pricing.SavingsPercentage + // BreakEvenMonths: months of accrued savings required to cover the + // commitment cost (conservative: treats the whole period cost as sunk, + // which is accurate for all-upfront and overestimates for monthly CUDs). + // monthlySavings = monthly spend difference between on-demand and CUD. + if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 { + monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12) + if monthlySavings > 0 { + rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings + } + } +} + // extractResourceTypeFromRecommendation extracts the resource type from a GCP recommendation func extractResourceTypeFromRecommendation(gcpRec *recommenderpb.Recommendation, rec *common.Recommendation) { if gcpRec.Content == nil || gcpRec.Content.OperationGroups == nil { @@ -889,6 +1033,141 @@ func extractCostImpactFromRecommendation(gcpRec *recommenderpb.Recommendation, r } } +// isMemoryAmountOp returns true when op's path_filters indicate a memory +// resource type. Used by extractVCPUCountFromRecommendation to skip the memory +// sibling of the VCPU operation in a GCP commitment resource operation group. +// Matches both "MEMORY" (the canonical ResourceCommitment.Type enum member) and +// the legacy "MEMORY_MB" spelling, since the Recommender's path_filter encoding +// is not contractually documented (issue #1022). +func isMemoryAmountOp(op *recommenderpb.Operation) bool { + for filterKey, filterVal := range op.GetPathFilters() { + if !strings.Contains(strings.ToLower(filterKey), "type") { + continue + } + if sv, ok := filterVal.GetKind().(*structpb.Value_StringValue); ok { + if strings.EqualFold(sv.StringValue, "MEMORY") || strings.EqualFold(sv.StringValue, "MEMORY_MB") { + return true + } + } + } + return false +} + +// vcpuCountFromOperationGroups walks the commitment operation groups and +// returns the VCPU count encoded in the operation's numeric value, or 0 if +// none is found. Extracted from extractVCPUCountFromRecommendation to keep +// cyclomatic complexity in check. +func vcpuCountFromOperationGroups(content *recommenderpb.RecommendationContent) int { + for _, opGroup := range content.GetOperationGroups() { + for _, op := range opGroup.GetOperations() { + if !strings.Contains(strings.ToLower(op.GetResourceType()), "commitment") { + continue + } + if !strings.Contains(strings.ToLower(op.GetPath()), "amount") { + continue + } + if isMemoryAmountOp(op) { + continue + } + if v := op.GetValue(); v != nil { + if nv, ok := v.GetKind().(*structpb.Value_NumberValue); ok && nv.NumberValue > 0 { + return int(nv.NumberValue) + } + } + } + } + return 0 +} + +// extractVCPUCountFromRecommendation extracts the recommended vCPU count from a +// GCP Commitment Recommender response (issue #1022 C1). +// +// The Recommender encodes the commitment resource amounts in two places: +// - Operation.Value: a structpb.Value whose numeric value is the amount, with +// Operation.Path indicating the resource type (e.g. "/resources/0/amount"). +// Operations with ResourceType "compute.googleapis.com/Commitment" and a path +// containing "amount" carry the VCPU count; the sibling MEMORY amount is +// extracted by extractMemoryMBFromRecommendation. +// - RecommendationContent.Overview: a JSON struct with a "numericValue" field. +// +// We prefer the operation-value path because it is structured and unambiguous. +// If no VCPU operation is found we fall back to the overview's numericValue. +func extractVCPUCountFromRecommendation(gcpRec *recommenderpb.Recommendation, rec *common.Recommendation) { + if gcpRec.Content == nil { + return + } + + if count := vcpuCountFromOperationGroups(gcpRec.Content); count > 0 { + rec.Count = count + return + } + + // Fallback: overview numericValue (used by older recommender versions). + if gcpRec.Content.GetOverview() != nil { + if nv := gcpRec.Content.GetOverview().GetFields()["numericValue"]; nv != nil { + if count := nv.GetNumberValue(); count > 0 { + rec.Count = int(count) + } + } + } +} + +// memoryMBFromOperationGroups walks the commitment operation groups and returns +// the MEMORY resource amount (in MB) encoded in the operation's numeric value, +// or 0 if none is found. It is the sibling of vcpuCountFromOperationGroups: +// both look at "compute.googleapis.com/Commitment" operations whose Path +// contains "amount", but this function selects only those where isMemoryAmountOp +// returns true (i.e. the path_filter type is MEMORY or MEMORY_MB). +func memoryMBFromOperationGroups(content *recommenderpb.RecommendationContent) int64 { + for _, opGroup := range content.GetOperationGroups() { + for _, op := range opGroup.GetOperations() { + if !strings.Contains(strings.ToLower(op.GetResourceType()), "commitment") { + continue + } + if !strings.Contains(strings.ToLower(op.GetPath()), "amount") { + continue + } + if !isMemoryAmountOp(op) { + continue + } + if v := op.GetValue(); v != nil { + if nv, ok := v.GetKind().(*structpb.Value_NumberValue); ok && nv.NumberValue > 0 { + return int64(nv.NumberValue) + } + } + } + } + return 0 +} + +// extractMemoryMBFromRecommendation extracts the MEMORY resource amount (in MB) +// from the Recommender payload and stores it in rec.Details as ComputeDetails.MemoryGB. +// This allows buildInsertRequest to use the payload-sourced amount rather than the +// general-purpose ratio approximation. No-op when the payload carries no MEMORY op. +func extractMemoryMBFromRecommendation(gcpRec *recommenderpb.Recommendation, rec *common.Recommendation) { + if gcpRec.Content == nil { + return + } + memMB := memoryMBFromOperationGroups(gcpRec.Content) + if memMB <= 0 { + return + } + // Store as ComputeDetails.MemoryGB (MB -> GB conversion). GCP memory amounts + // are always whole multiples of at least 256 MB, so the float64 roundtrip is + // exact: memMB / 1024 * 1024 == memMB for any value divisible by 1. + rec.Details = common.ComputeDetails{MemoryGB: float64(memMB) / 1024.0} +} + +// memoryMBFromDetails reads the MEMORY amount (in MB) from rec.Details when it +// was populated by extractMemoryMBFromRecommendation. Returns an error when the +// field is absent or zero -- callers must not silently fall back to a ratio. +func memoryMBFromDetails(rec common.Recommendation) (int64, error) { + if cd, ok := rec.Details.(common.ComputeDetails); ok && cd.MemoryGB > 0 { + return int64(cd.MemoryGB * 1024), nil + } + return 0, fmt.Errorf("memoryMBFromDetails: MEMORY resource amount absent from recommendation Details (no MEMORY op in Recommender payload); cannot build CUD insert without explicit memory") +} + // idempotentNameTokenLen is how many leading hex characters of the idempotency // token are folded into the derived commitment name. A GCP commitment name must // match RFC1035 (1-63 chars, lowercase [a-z]([-a-z0-9]*[a-z0-9])?); the "cud-" diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index 74ce48896..bb3e0d534 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -13,8 +13,10 @@ import ( "google.golang.org/api/cloudbilling/v1" "google.golang.org/api/iterator" "google.golang.org/genproto/googleapis/type/money" + "google.golang.org/protobuf/types/known/structpb" "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/pkg/scorer" ) // MockCommitmentsService mocks the CommitmentsService interface @@ -471,6 +473,7 @@ func TestComputeEngineClient_PurchaseCommitment_WithMock(t *testing.T) { Term: "1yr", CommitmentCost: 1000.0, Count: 5, + Details: common.ComputeDetails{MemoryGB: 20.0}, // 5 vCPU * 4 GB } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) @@ -487,7 +490,12 @@ func TestComputeEngineClient_PurchaseCommitment_EncodesSourceInDescription(t *te mockService := &MockCommitmentsService{operation: &MockOperation{err: nil}} client.SetCommitmentsService(mockService) - rec := common.Recommendation{ResourceType: "n1-standard-1", Term: "1yr", Count: 1} + rec := common.Recommendation{ + ResourceType: "n1-standard-1", + Term: "1yr", + Count: 1, + Details: common.ComputeDetails{MemoryGB: 4.0}, // 1 vCPU * 4 GB + } _, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceWeb}) require.NoError(t, err) @@ -504,7 +512,12 @@ func TestComputeEngineClient_PurchaseCommitment_OmitsTagWhenSourceEmpty(t *testi mockService := &MockCommitmentsService{operation: &MockOperation{err: nil}} client.SetCommitmentsService(mockService) - rec := common.Recommendation{ResourceType: "n1-standard-1", Term: "1yr", Count: 1} + rec := common.Recommendation{ + ResourceType: "n1-standard-1", + Term: "1yr", + Count: 1, + Details: common.ComputeDetails{MemoryGB: 4.0}, + } _, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) require.NoError(t, err) @@ -526,7 +539,12 @@ func TestComputeEngineClient_PurchaseCommitment_IdempotentReDrive(t *testing.T) mockService := &MockCommitmentsService{operation: &MockOperation{err: nil}} client.SetCommitmentsService(mockService) - rec := common.Recommendation{ResourceType: "n1-standard-1", Term: "1yr", Count: 5} + rec := common.Recommendation{ + ResourceType: "n1-standard-1", + Term: "1yr", + Count: 5, + Details: common.ComputeDetails{MemoryGB: 20.0}, // 5 vCPU * 4 GB + } token := common.DeriveIdempotencyToken("exec-654", 0) opts := common.PurchaseOptions{Source: common.PurchaseSourceWeb, IdempotencyToken: token} @@ -567,7 +585,12 @@ func TestComputeEngineClient_PurchaseCommitment_EmptyTokenNoRequestID(t *testing mockService := &MockCommitmentsService{operation: &MockOperation{err: nil}} client.SetCommitmentsService(mockService) - rec := common.Recommendation{ResourceType: "n1-standard-1", Term: "1yr", Count: 1} + rec := common.Recommendation{ + ResourceType: "n1-standard-1", + Term: "1yr", + Count: 1, + Details: common.ComputeDetails{MemoryGB: 4.0}, + } _, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) require.NoError(t, err) @@ -590,6 +613,8 @@ func TestComputeEngineClient_PurchaseCommitment_3Year(t *testing.T) { rec := common.Recommendation{ ResourceType: "n1-standard-1", Term: "3yr", + Count: 4, // must be > 0 after issue #1022 guard + Details: common.ComputeDetails{MemoryGB: 16.0}, // 4 vCPU * 4 GB } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) @@ -606,7 +631,14 @@ func TestComputeEngineClient_PurchaseCommitment_InsertError(t *testing.T) { } client.SetCommitmentsService(mockService) - rec := common.Recommendation{ResourceType: "n1-standard-1"} + // Count must be > 0, Term must be valid, and Details.MemoryGB must be set so + // all pre-insert guards pass and the insert error is exercised. + rec := common.Recommendation{ + ResourceType: "n1-standard-1", + Term: "1yr", + Count: 2, + Details: common.ComputeDetails{MemoryGB: 8.0}, + } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) assert.Error(t, err) @@ -623,7 +655,14 @@ func TestComputeEngineClient_PurchaseCommitment_WaitError(t *testing.T) { } client.SetCommitmentsService(mockService) - rec := common.Recommendation{ResourceType: "n1-standard-1"} + // Count must be > 0, Term must be valid, and Details.MemoryGB must be set so + // all pre-insert guards pass and the wait error is exercised. + rec := common.Recommendation{ + ResourceType: "n1-standard-1", + Term: "1yr", + Count: 2, + Details: common.ComputeDetails{MemoryGB: 8.0}, + } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) assert.Error(t, err) @@ -635,6 +674,8 @@ func TestComputeEngineClient_GetOfferingDetails_WithMock(t *testing.T) { ctx := context.Background() client, _ := NewClient(ctx, "test-project", "us-central1") + // Both on-demand and commitment SKUs are required; without a commitment SKU + // getComputePricing now returns an error (issue #1020 fix). mockService := &MockBillingService{ skus: &cloudbilling.ListSkusResponse{ Skus: []*cloudbilling.Sku{ @@ -657,6 +698,25 @@ func TestComputeEngineClient_GetOfferingDetails_WithMock(t *testing.T) { }, }, }, + { + Description: "n1-standard-1 commitment 1yr in Americas", + ServiceRegions: []string{"us-central1"}, + PricingInfo: []*cloudbilling.PricingInfo{ + { + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ + { + UnitPrice: &cloudbilling.Money{ + Units: 0, + Nanos: 32000000, + CurrencyCode: "USD", + }, + }, + }, + }, + }, + }, + }, }, }, } @@ -701,7 +761,8 @@ func TestComputeEngineClient_GetRecommendations_WithMock(t *testing.T) { mockIterator := &MockRecommenderIterator{ recommendations: []*recommenderpb.Recommendation{ { - Name: "recommendation-1", + Name: "recommendation-1", + StateInfo: &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE}, PrimaryImpact: &recommenderpb.Impact{ Category: recommenderpb.Impact_COST, Projection: &recommenderpb.Impact_CostProjection{ @@ -831,7 +892,7 @@ func TestComputeEngineClient_ConvertGCPRecommendation(t *testing.T) { }, } - rec := client.convertGCPRecommendation(ctx, gcpRec) + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) require.NotNil(t, rec) assert.Equal(t, common.ProviderGCP, rec.Provider) assert.Equal(t, common.ServiceCompute, rec.Service) @@ -883,3 +944,652 @@ func TestComputeEngineClient_GetRecommendations_PageCapFires(t *testing.T) { _, err = client.GetRecommendations(context.Background(), common.RecommendationParams{}) require.Error(t, err, "page cap must surface an error when the iterator never terminates") } + +// realisticCUDRecommendation builds a realistic GCP Commitment Recommender payload +// for a 4-vCPU n1-standard-4 commitment in us-central1. The operation groups have +// three ops: +// 1. A machine-type op whose resource path ends in the machine type (n1-standard-4) -- +// used by extractResourceTypeFromOperations to set rec.ResourceType. +// 2. A VCPU commitment resource op whose numeric Value is the VCPU count -- +// used by extractVCPUCountFromRecommendation to set rec.Count = 4. +// 3. A MEMORY commitment resource op whose numeric Value is 6144 MB -- +// used by extractMemoryMBFromRecommendation to set rec.Details.MemoryGB = 6. +// Using 6144 MB (1536 MB/vCPU) intentionally tests a non-4096-ratio case to +// confirm the value is read from the payload rather than computed from a ratio. +// +// This mirrors the GCP CUD Recommender format (issue #1022 C1 + memory fix). +func realisticCUDRecommendation() *recommenderpb.Recommendation { + vcpuVal, _ := structpb.NewValue(4.0) + memVal, _ := structpb.NewValue(6144.0) // 6144 MB = 6 GB; ratio is 1536 MB/vCPU, NOT 4096 + memTypeFilter, _ := structpb.NewValue("MEMORY") + return &recommenderpb.Recommendation{ + Name: "projects/test/locations/us-central1/recommenders/google.billing.CostInsight.commitmentRecommender/recommendations/rec-001", + PrimaryImpact: &recommenderpb.Impact{ + Category: recommenderpb.Impact_COST, + Projection: &recommenderpb.Impact_CostProjection{ + CostProjection: &recommenderpb.CostProjection{ + Cost: &money.Money{ + Units: -200, + Nanos: 0, + CurrencyCode: "USD", + }, + }, + }, + }, + Content: &recommenderpb.RecommendationContent{ + OperationGroups: []*recommenderpb.OperationGroup{ + { + Operations: []*recommenderpb.Operation{ + { + // Machine type op: resource path ends in the machine type name, + // so extractResourceTypeFromOperations sets rec.ResourceType = "n1-standard-4". + Action: "add", + Resource: "projects/test/zones/us-central1-a/machineTypes/n1-standard-4", + }, + }, + }, + { + Operations: []*recommenderpb.Operation{ + { + // VCPU op: carries the vCPU count as a numeric value. + // extractVCPUCountFromRecommendation reads this and sets rec.Count = 4. + Action: "add", + ResourceType: "compute.googleapis.com/Commitment", + Resource: "//compute.googleapis.com/projects/test/regions/us-central1/commitments/cud-001", + Path: "/resources/0/amount", + PathValue: &recommenderpb.Operation_Value{Value: vcpuVal}, + }, + { + // MEMORY op: carries the memory amount in MB as a numeric value. + // extractMemoryMBFromRecommendation reads this (path_filter type=MEMORY) + // and sets rec.Details = ComputeDetails{MemoryGB: 6.0}. + Action: "add", + ResourceType: "compute.googleapis.com/Commitment", + Resource: "//compute.googleapis.com/projects/test/regions/us-central1/commitments/cud-001", + Path: "/resources/1/amount", + PathValue: &recommenderpb.Operation_Value{Value: memVal}, + PathFilters: map[string]*structpb.Value{ + "/resources/1/type": memTypeFilter, + }, + }, + }, + }, + }, + }, + } +} + +// mockBillingWithCommitment returns a MockBillingService that has both an +// on-demand SKU and a commitment SKU for n1-standard machines in us-central1. +func mockBillingWithCommitment() *MockBillingService { + return &MockBillingService{ + skus: &cloudbilling.ListSkusResponse{ + Skus: []*cloudbilling.Sku{ + { + Description: "n1-standard-4 VM running in Americas", + ServiceRegions: []string{"us-central1"}, + PricingInfo: []*cloudbilling.PricingInfo{ + { + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ + { + UnitPrice: &cloudbilling.Money{ + Units: 0, + Nanos: 190000000, + CurrencyCode: "USD", + }, + }, + }, + }, + }, + }, + }, + { + Description: "n1-standard-4 commitment 1yr in Americas", + ServiceRegions: []string{"us-central1"}, + PricingInfo: []*cloudbilling.PricingInfo{ + { + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ + { + UnitPrice: &cloudbilling.Money{ + Units: 0, + Nanos: 120000000, + CurrencyCode: "USD", + }, + }, + }, + }, + }, + }, + }, + }, + }, + } +} + +// TestConverterToInsert_CountNonZero_VCPUAmountSet is the primary regression test +// for issue #1022 C1. It exercises the full converter->purchase path with a +// realistic Recommender payload and asserts: +// - converter sets Count > 0 (extracted from the operation's numeric value) +// - converter extracts the MEMORY amount from the payload (not a ratio) +// - buildInsertRequest produces the exact VCPU and MEMORY Amounts +// +// The payload uses 6144 MB for 4 vCPUs (1536 MB/vCPU, NOT 4096). An old ratio- +// based implementation would produce 4*4096 = 16384 MB; the correct value is +// 6144 MB. This assertion fails on pre-fix ratio-based code. +func TestConverterToInsert_CountNonZero_VCPUAmountSet(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + client.SetBillingService(mockBillingWithCommitment()) + + gcpRec := realisticCUDRecommendation() // 4 vCPU, 6144 MB (from payload) + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + require.NotNil(t, rec) + + // C1: Count must be > 0 so that buildInsertRequest produces non-zero VCPU Amount. + require.Greater(t, rec.Count, 0, "convertGCPRecommendation must set Count > 0 from the Recommender payload (issue #1022 C1)") + + // Memory must be extracted from the payload and stored in Details. + cd, ok := rec.Details.(common.ComputeDetails) + require.True(t, ok, "convertGCPRecommendation must populate ComputeDetails (memory extracted from payload)") + assert.Equal(t, 6.0, cd.MemoryGB, + "MEMORY amount from payload is 6144 MB = 6 GB; ratio-based code would produce 16 GB (4*4096 MB)") + + // Verify the insert request carries the correct VCPU and MEMORY amounts. + insertReq, _, buildErr := client.buildInsertRequest(*rec, common.PurchaseOptions{}) + require.NoError(t, buildErr, "buildInsertRequest must not error when Count > 0 and memory is present") + require.NotNil(t, insertReq) + require.NotNil(t, insertReq.CommitmentResource) + + // The resource Type strings MUST be valid GCP ResourceCommitment.Type enum + // members (VCPU/MEMORY/LOCAL_SSD/ACCELERATOR). "MEMORY_MB" is NOT a valid + // enum value and GCP rejects the commitments.insert, failing the purchase + // (issue #1022). This loop asserts on the canonical "MEMORY" spelling. + var vcpuAmount, memoryAmount int64 + var sawInvalidType string + for _, r := range insertReq.CommitmentResource.Resources { + switch r.GetType() { + case "VCPU": + vcpuAmount = r.GetAmount() + case "MEMORY": + memoryAmount = r.GetAmount() + default: + sawInvalidType = r.GetType() + } + } + assert.Empty(t, sawInvalidType, "insert request must use only valid ResourceCommitment.Type enum members; got %q (issue #1022)", sawInvalidType) + assert.Equal(t, int64(4), vcpuAmount, "VCPU Amount in the insert request must equal the extracted count") + // Exact memory check: payload says 6144 MB; old ratio (4*4096=16384 MB) would fail. + assert.Equal(t, int64(6144), memoryAmount, + "MEMORY Amount must match the payload's 6144 MB (not ratio-derived 4*4096=16384 MB)") +} + +// TestConverterFillsPricingForScorer is the regression test for issue #1022 C2. +// It exercises the full converter path with a realistic Recommender payload and a +// billing mock that returns both on-demand and commitment SKUs, and then asserts +// that a scorer with MinSavingsPct set does NOT drop the GCP recommendation. +// +// This test FAILS on the pre-fix code where convertGCPRecommendation never +// called getComputePricing, leaving SavingsPercentage at 0. +func TestConverterFillsPricingForScorer(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + client.SetBillingService(mockBillingWithCommitment()) + + gcpRec := realisticCUDRecommendation() + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + require.NotNil(t, rec) + + // C2: SavingsPercentage must be > 0 so a MinSavingsPct filter doesn't silently drop the rec. + require.Greater(t, rec.SavingsPercentage, float64(0), + "convertGCPRecommendation must set SavingsPercentage > 0 from billing pricing (issue #1022 C2)") + assert.Greater(t, rec.CommitmentCost, float64(0), "CommitmentCost must be > 0") + assert.Greater(t, rec.OnDemandCost, float64(0), "OnDemandCost must be > 0") + + // Apply a non-trivial MinSavingsPct filter and confirm the rec passes. + result := scorer.Score([]common.Recommendation{*rec}, scorer.Config{MinSavingsPct: 5.0}) + require.Len(t, result.Passed, 1, "GCP recommendation must pass a MinSavingsPct=5 scorer filter after pricing is wired (issue #1022 C2)") + assert.Empty(t, result.Filtered, "no GCP recommendations should be filtered by MinSavingsPct when pricing is set") +} + +// TestBuildInsertRequest_RefusesZeroCount is the regression test for the +// issue #1022 guard in buildInsertRequest. A recommendation with Count == 0 +// must return an error rather than sending a zero-vCPU commitment to GCP. +// +// This test FAILS on the pre-fix code (which had no such guard). +func TestBuildInsertRequest_RefusesZeroCount(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + rec := common.Recommendation{ + ResourceType: "n1-standard-4", + Term: "1yr", + Count: 0, // zero -- should be refused + } + + _, _, err := client.buildInsertRequest(rec, common.PurchaseOptions{}) + require.Error(t, err, "buildInsertRequest must refuse Count <= 0 (issue #1022 guard)") + assert.Contains(t, err.Error(), "rec.Count must be > 0") + + // PurchaseCommitment must also surface the error and not call Insert. + mockSvc := &MockCommitmentsService{operation: &MockOperation{}} + client.SetCommitmentsService(mockSvc) + result, purchaseErr := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + require.Error(t, purchaseErr) + assert.False(t, result.Success) + assert.Empty(t, mockSvc.insertReqs, "Insert must not be called when Count <= 0") +} + +// TestGetComputePricing_NoCommitmentSKUReturnsError is the regression test for +// issue #1020 (GCP). When the billing catalog has no commitment SKU, getComputePricing +// must return an error rather than fabricating a price from a hardcoded discount factor. +// +// This test FAILS on the pre-fix code where estimateComputeCommitmentPrice was called. +func TestGetComputePricing_NoCommitmentSKUReturnsError(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + // Only on-demand SKU -- no commitment SKU. + client.SetBillingService(&MockBillingService{ + skus: &cloudbilling.ListSkusResponse{ + Skus: []*cloudbilling.Sku{ + { + Description: "n1-standard-4 VM running in Americas", + ServiceRegions: []string{"us-central1"}, + PricingInfo: []*cloudbilling.PricingInfo{ + { + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ + { + UnitPrice: &cloudbilling.Money{ + Units: 0, + Nanos: 190000000, + CurrencyCode: "USD", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }) + + pricing, err := client.getComputePricing(ctx, "n1-standard-4", "us-central1", 1) + require.Error(t, err, "getComputePricing must return an error when no commitment SKU exists (issue #1020)") + assert.Contains(t, err.Error(), "no commitment pricing found") + assert.Nil(t, pricing, "no pricing struct must be returned when commitment SKU is missing") +} + +// TestConvertGCPRecommendation_EmptyParamsDefaultsToMonthly asserts that when +// RecommendationParams.PaymentOption is empty the converter defaults to "monthly". +// GCP CUDs have no upfront-payment option; defaulting to "upfront" caused incorrect +// purchase-body construction downstream. +// +// This test FAILS on the pre-fix code that defaulted to "upfront" (10-M5 / auditor finding). +func TestConvertGCPRecommendation_EmptyParamsDefaultsToMonthly(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + gcpRec := &recommenderpb.Recommendation{ + Name: "test-rec", + PrimaryImpact: &recommenderpb.Impact{ + Category: recommenderpb.Impact_COST, + Projection: &recommenderpb.Impact_CostProjection{ + CostProjection: &recommenderpb.CostProjection{ + Cost: &money.Money{Units: -100, CurrencyCode: "USD"}, + }, + }, + }, + } + + // Empty params: no caller-supplied PaymentOption. + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + require.NotNil(t, rec) + assert.Equal(t, "monthly", rec.PaymentOption, + "GCP CUDs have no upfront option; empty PaymentOption must default to \"monthly\" (10-M5)") +} + +// TestConvertGCPRecommendation_ParamPaymentOptionRespected asserts that an +// explicit PaymentOption in RecommendationParams is forwarded to the recommendation. +func TestConvertGCPRecommendation_ParamPaymentOptionRespected(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + gcpRec := &recommenderpb.Recommendation{Name: "test-rec"} + params := common.RecommendationParams{PaymentOption: "monthly"} + + rec := client.convertGCPRecommendation(ctx, gcpRec, params) + require.NotNil(t, rec) + assert.Equal(t, "monthly", rec.PaymentOption) +} + +// TestGroupCommitments_UsesValidMemoryEnum is a regression test for issue #1022: +// GroupCommitments must emit the canonical GCP ResourceCommitment.Type enum +// member "MEMORY" (Amount in MB), not the invalid "MEMORY_MB" string that GCP +// rejects on commitments.insert. Fails on the pre-fix code that emitted +// "MEMORY_MB". +// The memory amount is read from ComputeDetails.MemoryGB (set by the converter); +// recommendations without Details are skipped. +func TestGroupCommitments_UsesValidMemoryEnum(t *testing.T) { + // 4 vCPU, 24 GB memory (non-4096-ratio to confirm the value is read from + // Details, not computed from the old ratio). + const wantMemoryGB = 24.0 + const wantMemoryMB = int64(wantMemoryGB * 1024) + + recs := []common.Recommendation{ + { + Provider: common.ProviderGCP, + Service: common.ServiceCompute, + Account: "test-project", + Region: "us-central1", + Term: "1yr", + Count: 4, + Details: common.ComputeDetails{MemoryGB: wantMemoryGB}, + }, + } + + groups := GroupCommitments(recs) + require.Len(t, groups, 1, "one project+region+term group expected") + + var sawVCPU, sawMemory bool + for _, r := range groups[0].Resources { + switch r.Type { + case "VCPU": + sawVCPU = true + assert.Equal(t, int64(4), r.Amount, "VCPU amount must equal the summed Count") + case "MEMORY": + sawMemory = true + assert.Equal(t, wantMemoryMB, r.Amount, "MEMORY amount must equal the payload-sourced MB (issue #1022)") + default: + t.Fatalf("invalid ResourceCommitment.Type %q (issue #1022): must be VCPU/MEMORY/LOCAL_SSD/ACCELERATOR", r.Type) + } + } + assert.True(t, sawVCPU, "GroupCommitments must include a VCPU resource") + assert.True(t, sawMemory, "GroupCommitments must include a MEMORY resource (issue #1022)") +} + +// TestGroupCommitments_SkipsRecsWithoutMemory verifies that GroupCommitments skips +// recommendations that have no Details.MemoryGB (payload omitted memory) rather +// than producing a zero-memory commitment that GCP would reject. +func TestGroupCommitments_SkipsRecsWithoutMemory(t *testing.T) { + recs := []common.Recommendation{ + { + Provider: common.ProviderGCP, + Service: common.ServiceCompute, + Account: "test-project", + Region: "us-central1", + Term: "1yr", + Count: 4, + // No Details: memory is absent from the payload. + }, + } + + groups := GroupCommitments(recs) + assert.Empty(t, groups, "GroupCommitments must skip recs without payload memory, not produce a zero-memory commitment") +} + +// TestConvertGCPRecommendation_NonMonthlyPaymentOptionForcedToMonthly is a +// regression test for the CR finding that non-monthly values (e.g. "upfront") +// are silently propagated to the recommendation, which is invalid for GCP CUDs +// that only support monthly billing. The converter must always emit "monthly". +// +// This test FAILS on the pre-fix code that passed params.PaymentOption through +// unchanged when it was non-empty. +func TestConvertGCPRecommendation_NonMonthlyPaymentOptionForcedToMonthly(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + gcpRec := &recommenderpb.Recommendation{Name: "test-rec"} + + for _, input := range []string{"upfront", "all-upfront", "UPFRONT", "partial-upfront"} { + params := common.RecommendationParams{PaymentOption: input} + rec := client.convertGCPRecommendation(ctx, gcpRec, params) + require.NotNil(t, rec) + assert.Equal(t, "monthly", rec.PaymentOption, + "GCP CUDs are monthly-only; input %q must be forced to \"monthly\"", input) + } +} + +// TestIsMemoryAmountOp_MatchesBothSpellings asserts that the inbound Recommender +// memory-op detector skips both the canonical "MEMORY" and legacy "MEMORY_MB" +// path_filter spellings, so the VCPU extractor never mistakes the memory sibling +// for the vCPU amount (issue #1022). +func TestIsMemoryAmountOp_MatchesBothSpellings(t *testing.T) { + for _, typeVal := range []string{"MEMORY", "MEMORY_MB", "memory"} { + sv, _ := structpb.NewValue(typeVal) + op := &recommenderpb.Operation{ + PathFilters: map[string]*structpb.Value{ + "/resources/*/type": sv, + }, + } + assert.True(t, isMemoryAmountOp(op), "isMemoryAmountOp must skip memory op with type %q", typeVal) + } + + // A VCPU op (or an op with no type filter) must NOT be treated as memory. + svVCPU, _ := structpb.NewValue("VCPU") + vcpuOp := &recommenderpb.Operation{ + PathFilters: map[string]*structpb.Value{"/resources/*/type": svVCPU}, + } + assert.False(t, isMemoryAmountOp(vcpuOp), "isMemoryAmountOp must not skip a VCPU op") +} + +// TestBuildInsertRequest_RefusesMissingMemory is the regression test for the +// "fail loud" policy on missing memory: buildInsertRequest must return an error +// when rec.Details does not carry a ComputeDetails.MemoryGB value (i.e. the +// Recommender payload omitted the MEMORY op). A silent fallback to a fixed ratio +// would produce incorrect commitments for non-standard machine families. +// +// This test fails on pre-fix code that silently used memMBPerVCPU as a fallback. +func TestBuildInsertRequest_RefusesMissingMemory(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + rec := common.Recommendation{ + ResourceType: "n1-standard-4", + Term: "1yr", + Count: 4, + // No Details: memory absent from Recommender payload. + } + + _, _, err := client.buildInsertRequest(rec, common.PurchaseOptions{}) + require.Error(t, err, "buildInsertRequest must refuse when memory is absent from the payload") + assert.Contains(t, err.Error(), "MEMORY resource amount absent", + "error must name the cause so operators can diagnose the missing Recommender field") + + // PurchaseCommitment must surface the error and not call Insert. + mockSvc := &MockCommitmentsService{operation: &MockOperation{}} + client.SetCommitmentsService(mockSvc) + result, purchaseErr := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + require.Error(t, purchaseErr) + assert.False(t, result.Success) + assert.Empty(t, mockSvc.insertReqs, "Insert must not be called when memory is absent") +} + +// TestTermPlan_UsesSdkEnumConstants verifies that termPlan returns the exact string +// produced by the SDK enum's String() method (not a hand-written literal), so any +// future SDK rename propagates automatically. +func TestTermPlan_UsesSdkEnumConstants(t *testing.T) { + plan12, err := termPlan("1yr") + require.NoError(t, err) + assert.Equal(t, computepb.Commitment_TWELVE_MONTH.String(), plan12, + "1yr must map to the SDK TWELVE_MONTH constant, not a string literal") + + plan36, err := termPlan("3yr") + require.NoError(t, err) + assert.Equal(t, computepb.Commitment_THIRTY_SIX_MONTH.String(), plan36, + "3yr must map to the SDK THIRTY_SIX_MONTH constant, not a string literal") +} + +// TestTermPlan_RejectsUnknownTerm is the regression test for the "fail loud" policy: +// termPlan must return an error rather than silently defaulting to 12 months when +// given an unrecognised or empty term. A silent mis-default can purchase the wrong +// duration and waste money. +// +// This test fails on pre-fix code that silently returned TWELVE_MONTH for any +// unrecognised input. +func TestTermPlan_RejectsUnknownTerm(t *testing.T) { + for _, badTerm := range []string{"", "2yr", "invalid", "24mo", "forever"} { + _, err := termPlan(badTerm) + require.Error(t, err, "termPlan must reject unrecognised term %q (no silent 12-month default)", badTerm) + assert.Contains(t, err.Error(), "unrecognised commitment term", + "error for term %q must identify the bad input", badTerm) + } +} + +// TestTermPlan_AcceptsAllDocumentedForms asserts that termPlan accepts all +// documented 1-year and 3-year input forms. +func TestTermPlan_AcceptsAllDocumentedForms(t *testing.T) { + for _, form := range []string{"1yr", "1", "12mo"} { + got, err := termPlan(form) + require.NoError(t, err, "termPlan must accept 1-year form %q", form) + assert.Equal(t, computepb.Commitment_TWELVE_MONTH.String(), got, + "1-year form %q must map to TWELVE_MONTH", form) + } + for _, form := range []string{"3yr", "3", "36mo"} { + got, err := termPlan(form) + require.NoError(t, err, "termPlan must accept 3-year form %q", form) + assert.Equal(t, computepb.Commitment_THIRTY_SIX_MONTH.String(), got, + "3-year form %q must map to THIRTY_SIX_MONTH", form) + } +} + +// TestConvertGCPRecommendation_PropagatesParamsTerm asserts that convertGCPRecommendation +// propagates params.Term to rec.Term (H-3 audit finding). Pre-fix code hardcoded +// "1yr" regardless of params.Term, so callers requesting "3yr" silently received +// 1-year recommendations and 1-year pricing. +// +// This test FAILS on the pre-fix code that hardcoded Term: "1yr". +func TestConvertGCPRecommendation_PropagatesParamsTerm(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + gcpRec := &recommenderpb.Recommendation{Name: "test-rec"} + + // 3yr must be propagated. + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: "3yr"}) + require.NotNil(t, rec) + assert.Equal(t, "3yr", rec.Term, + "params.Term=3yr must be propagated to rec.Term (H-3 fix); pre-fix code hardcoded 1yr") + + // 1yr explicit must be propagated. + rec = client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: "1yr"}) + require.NotNil(t, rec) + assert.Equal(t, "1yr", rec.Term) + + // Empty term must default to "1yr". + rec = client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{}) + require.NotNil(t, rec) + assert.Equal(t, "1yr", rec.Term, + "empty params.Term must default to 1yr") +} + +// TestConvertGCPRecommendation_RejectsUnknownTerm asserts that convertGCPRecommendation +// returns nil when given an unrecognised params.Term (e.g. "5yr"). An unroutable +// recommendation must be dropped before it can reach buildInsertRequest and attempt +// to insert a commitment with an invalid plan. +// +// This test FAILS on the pre-fix code that either silently used "1yr" or panicked. +func TestConvertGCPRecommendation_RejectsUnknownTerm(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + gcpRec := &recommenderpb.Recommendation{Name: "test-rec"} + + rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: "5yr"}) + assert.Nil(t, rec, + "convertGCPRecommendation must return nil for unrecognised term (not silently default to 12 months)") +} + +// TestGetRecommendations_FiltersNonActiveStates is a regression test for H-1 +// (GCP broad audit): the Recommender API returns recommendations in all states +// (ACTIVE/CLAIMED/SUCCEEDED/FAILED/DISMISSED); only ACTIVE ones must be surfaced +// as actionable. Acting on an already-CLAIMED or SUCCEEDED recommendation is a +// cross-run double-purchase vector and inflates actionable rec counts. +// +// This test FAILS on the pre-fix code that converted all recommendations +// regardless of state. +func TestGetRecommendations_FiltersNonActiveStates(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + stateActive := recommenderpb.RecommendationStateInfo_ACTIVE + stateClaimed := recommenderpb.RecommendationStateInfo_CLAIMED + stateSucceeded := recommenderpb.RecommendationStateInfo_SUCCEEDED + stateFailed := recommenderpb.RecommendationStateInfo_FAILED + stateDismissed := recommenderpb.RecommendationStateInfo_DISMISSED + + recs := []*recommenderpb.Recommendation{ + { + Name: "active-rec", + StateInfo: &recommenderpb.RecommendationStateInfo{State: stateActive}, + PrimaryImpact: &recommenderpb.Impact{ + Category: recommenderpb.Impact_COST, + Projection: &recommenderpb.Impact_CostProjection{ + CostProjection: &recommenderpb.CostProjection{ + Cost: &money.Money{Units: -100, CurrencyCode: "USD"}, + }, + }, + }, + }, + { + Name: "claimed-rec", + StateInfo: &recommenderpb.RecommendationStateInfo{State: stateClaimed}, + }, + { + Name: "succeeded-rec", + StateInfo: &recommenderpb.RecommendationStateInfo{State: stateSucceeded}, + }, + { + Name: "failed-rec", + StateInfo: &recommenderpb.RecommendationStateInfo{State: stateFailed}, + }, + { + Name: "dismissed-rec", + StateInfo: &recommenderpb.RecommendationStateInfo{State: stateDismissed}, + }, + } + + mockIterator := &MockRecommenderIterator{recommendations: recs} + mockClient := &MockRecommenderClient{iterator: mockIterator} + client.SetRecommenderClient(mockClient) + + results, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + require.NoError(t, err) + require.Len(t, results, 1, + "only the ACTIVE recommendation must be returned; CLAIMED/SUCCEEDED/FAILED/DISMISSED must be filtered (H-1)") + assert.Equal(t, common.ProviderGCP, results[0].Provider) +} + +// TestGetRecommendations_ActiveRecIncluded is the positive-case complement of +// TestGetRecommendations_FiltersNonActiveStates: a recommendation explicitly +// marked ACTIVE must pass the state filter. +func TestGetRecommendations_ActiveRecIncluded(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + mockIterator := &MockRecommenderIterator{ + recommendations: []*recommenderpb.Recommendation{ + { + Name: "active-only", + StateInfo: &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE}, + PrimaryImpact: &recommenderpb.Impact{ + Category: recommenderpb.Impact_COST, + Projection: &recommenderpb.Impact_CostProjection{ + CostProjection: &recommenderpb.CostProjection{ + Cost: &money.Money{Units: -50, CurrencyCode: "USD"}, + }, + }, + }, + }, + }, + } + mockClient := &MockRecommenderClient{iterator: mockIterator} + client.SetRecommenderClient(mockClient) + + results, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + require.NoError(t, err) + require.Len(t, results, 1, "an ACTIVE recommendation must be included") +} diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index 2407b77c8..ba6a2946a 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -4,6 +4,7 @@ package memorystore import ( "context" "fmt" + "log" "strings" "time" @@ -117,13 +118,23 @@ func (r *realBillingService) ListSKUs(serviceID string) (*cloudbilling.ListSkusR return r.service.Services.Skus.List(serviceID).Do() } +// realRecommenderIterator wraps the real recommender iterator (10-L4: makes +// the memorystore client diffable against the other three service clients). +type realRecommenderIterator struct { + it *recommender.RecommendationIterator +} + +func (r *realRecommenderIterator) Next() (*recommenderpb.Recommendation, error) { + return r.it.Next() +} + // realRecommenderClient wraps the actual recommender client type realRecommenderClient struct { client *recommender.Client } func (r *realRecommenderClient) ListRecommendations(ctx context.Context, req *recommenderpb.ListRecommendationsRequest) RecommenderIterator { - return r.client.ListRecommendations(ctx, req) + return &realRecommenderIterator{it: r.client.ListRecommendations(ctx, req)} } func (r *realRecommenderClient) Close() error { @@ -181,7 +192,13 @@ func (c *MemorystoreClient) GetRecommendations(ctx context.Context, params commo return nil, fmt.Errorf("memorystore: iterate recommendations: %w", err) } - converted := c.convertGCPRecommendation(ctx, rec) + // Skip non-ACTIVE recommendations (CLAIMED/SUCCEEDED/FAILED/DISMISSED). + // See computeengine.GetRecommendations for the full rationale. + if rec.GetStateInfo().GetState() != recommenderpb.RecommendationStateInfo_ACTIVE { + continue + } + + converted := c.convertGCPRecommendation(ctx, rec, params) if converted != nil { recommendations = append(recommendations, *converted) } @@ -295,7 +312,9 @@ type RedisPricing struct { SavingsPercentage float64 } -// getRedisPricing gets pricing from GCP Cloud Billing Catalog API +// getRedisPricing gets pricing from GCP Cloud Billing Catalog API. +// It returns an error when commitment pricing is absent from the catalog rather +// than fabricating a price from a hardcoded discount factor (issue #1020). func (c *MemorystoreClient) getRedisPricing(ctx context.Context, tier, region string, termYears int) (*RedisPricing, error) { billingSvc, err := c.getOrCreateBillingService(ctx) if err != nil { @@ -311,17 +330,20 @@ func (c *MemorystoreClient) getRedisPricing(ctx context.Context, tier, region st if onDemandPrice == 0 { return nil, fmt.Errorf("no pricing found for Memorystore tier %s", tier) } - - hoursInTerm := 8760.0 * float64(termYears) if commitmentPrice == 0 { - commitmentPrice = estimateCommitmentPrice(onDemandPrice, hoursInTerm, termYears) + return nil, fmt.Errorf("no commitment pricing found for Memorystore tier %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", tier, region) } - savingsPercentage := calculateSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice) + hoursInTerm := 8760.0 * float64(termYears) + // Scale the per-hour commitment price to a term total so it is on the + // same basis as onDemandPrice * hoursInTerm. Without this, the savings + // percentage would be nearly 100% (per-hour price vs term total). + commitmentPriceTerm := commitmentPrice * hoursInTerm + savingsPercentage := calculateSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPriceTerm) return &RedisPricing{ - HourlyRate: commitmentPrice / hoursInTerm, - CommitmentPrice: commitmentPrice, + HourlyRate: commitmentPrice, + CommitmentPrice: commitmentPriceTerm, OnDemandPrice: onDemandPrice * hoursInTerm, Currency: currency, SavingsPercentage: savingsPercentage, @@ -390,15 +412,6 @@ func extractPriceFromSKU(sku *cloudbilling.Sku) (float64, string) { return price, rate.UnitPrice.CurrencyCode } -// estimateCommitmentPrice estimates commitment price based on typical GCP savings -func estimateCommitmentPrice(onDemandPrice, hoursInTerm float64, termYears int) float64 { - discount := 0.70 // 30% savings for 1 year - if termYears == 3 { - discount = 0.65 // 35% savings for 3 years - } - return onDemandPrice * hoursInTerm * discount -} - // calculateSavingsPercentage calculates the savings percentage func calculateSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice float64) float64 { onDemandTotal := onDemandPrice * hoursInTerm @@ -425,8 +438,76 @@ func skuMatchesTier(sku *cloudbilling.Sku, tier, region string) bool { return true } -// convertGCPRecommendation converts a GCP Recommender recommendation to common format -func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation) *common.Recommendation { +// extractResourceTypeFromContent extracts the last path segment of the first +// non-empty Operation.Resource across all operation groups. Used by all four +// GCP service converters to set rec.ResourceType from Recommender payloads. +func extractResourceTypeFromContent(content *recommenderpb.RecommendationContent) string { + if content == nil || content.OperationGroups == nil { + return "" + } + for _, opGroup := range content.OperationGroups { + for _, op := range opGroup.Operations { + if op.Resource == "" { + continue + } + parts := strings.Split(op.Resource, "/") + if len(parts) > 0 { + return parts[len(parts)-1] + } + } + } + return "" +} + +// extractEstimatedSavings returns the negative of the PrimaryImpact cost +// projection (GCP encodes savings as a negative cost delta). +func extractEstimatedSavings(gcpRec *recommenderpb.Recommendation) float64 { + if gcpRec.PrimaryImpact == nil { + return 0 + } + costProj := gcpRec.PrimaryImpact.GetCostProjection() + if costProj == nil || costProj.Cost == nil { + return 0 + } + cost := costProj.Cost + return -(float64(cost.Units) + float64(cost.Nanos)/1e9) +} + +// fillRedisPricing calls getRedisPricing and, on success, writes CommitmentCost, +// OnDemandCost, SavingsPercentage, and BreakEvenMonths into rec. Pricing +// failures are logged and do not discard the recommendation. +func (c *MemorystoreClient) fillRedisPricing(ctx context.Context, rec *common.Recommendation, termYears int) { + pricing, err := c.getRedisPricing(ctx, rec.ResourceType, c.region, termYears) + if err != nil { + log.Printf("memorystore: pricing unavailable for %s in %s (issue #1020): %v", rec.ResourceType, c.region, err) + return + } + rec.CommitmentCost = pricing.CommitmentPrice + rec.OnDemandCost = pricing.OnDemandPrice + rec.SavingsPercentage = pricing.SavingsPercentage + if pricing.OnDemandPrice > 0 && pricing.SavingsPercentage > 0 { + monthlySavings := pricing.OnDemandPrice * pricing.SavingsPercentage / 100.0 / float64(termYears*12) + if monthlySavings > 0 { + rec.BreakEvenMonths = pricing.CommitmentPrice / monthlySavings + } + } +} + +// convertGCPRecommendation converts a GCP Recommender recommendation to common format. +// It also calls getRedisPricing to fill CommitmentCost/OnDemandCost/SavingsPercentage/ +// BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly +// (issue #1022 C2). Pricing failures are logged but do not discard the recommendation. +func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation { + paymentOption := params.PaymentOption + if paymentOption == "" { + paymentOption = "monthly" + } + + term := params.Term + if term == "" { + term = "1yr" + } + rec := &common.Recommendation{ Provider: common.ProviderGCP, Service: common.ServiceCache, @@ -434,34 +515,21 @@ func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec Region: c.region, CommitmentType: common.CommitmentCUD, Timestamp: time.Now(), - Term: "1yr", - PaymentOption: "monthly", + Term: term, + PaymentOption: paymentOption, } - // Extract resource type from recommendation content - if gcpRec.Content != nil { - if gcpRec.Content.OperationGroups != nil { - for _, opGroup := range gcpRec.Content.OperationGroups { - for _, op := range opGroup.Operations { - if op.Resource != "" { - parts := strings.Split(op.Resource, "/") - if len(parts) > 0 { - rec.ResourceType = parts[len(parts)-1] - } - } - } - } - } - } + rec.ResourceType = extractResourceTypeFromContent(gcpRec.Content) + rec.EstimatedSavings = extractEstimatedSavings(gcpRec) - // Extract cost impact - if gcpRec.PrimaryImpact != nil { - // Use GetCostProjection() method to access the cost projection - if costProj := gcpRec.PrimaryImpact.GetCostProjection(); costProj != nil && costProj.Cost != nil { - cost := costProj.Cost - savings := -(float64(cost.Units) + float64(cost.Nanos)/1e9) - rec.EstimatedSavings = savings + // Thread pricing into the converter so the scorer can rank/filter GCP recs + // correctly (issue #1022 C2). + if rec.ResourceType != "" { + termYears := 1 + if rec.Term == "3yr" || rec.Term == "3" { + termYears = 3 } + c.fillRedisPricing(ctx, rec, termYears) } return rec diff --git a/providers/gcp/services/memorystore/client_test.go b/providers/gcp/services/memorystore/client_test.go index 4f543b865..81eff3f53 100644 --- a/providers/gcp/services/memorystore/client_test.go +++ b/providers/gcp/services/memorystore/client_test.go @@ -364,6 +364,8 @@ func TestMemorystoreClient_GetOfferingDetails_WithMockService(t *testing.T) { errContains string }{ { + // Both on-demand and commitment SKUs are required after the issue #1020 fix: + // getRedisPricing now returns an error when no commitment SKU is found. name: "successful 1yr offering details", rec: common.Recommendation{ ResourceType: "STANDARD_HA", @@ -391,11 +393,31 @@ func TestMemorystoreClient_GetOfferingDetails_WithMockService(t *testing.T) { }, }, }, + { + Description: "Memorystore Redis STANDARD_HA commitment 1yr", + ServiceRegions: []string{"us-central1"}, + PricingInfo: []*cloudbilling.PricingInfo{ + { + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ + { + UnitPrice: &cloudbilling.Money{ + CurrencyCode: "USD", + Units: 0, + Nanos: 35000000, // $0.035 per hour + }, + }, + }, + }, + }, + }, + }, }, }, wantErr: false, }, { + // Both on-demand and commitment SKUs are required after the issue #1020 fix. name: "successful 3yr offering details", rec: common.Recommendation{ ResourceType: "BASIC", @@ -423,6 +445,25 @@ func TestMemorystoreClient_GetOfferingDetails_WithMockService(t *testing.T) { }, }, }, + { + Description: "Memorystore Redis BASIC commitment 3yr", + ServiceRegions: []string{"us-central1"}, + PricingInfo: []*cloudbilling.PricingInfo{ + { + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ + { + UnitPrice: &cloudbilling.Money{ + CurrencyCode: "USD", + Units: 0, + Nanos: 19500000, // $0.0195 per hour + }, + }, + }, + }, + }, + }, + }, }, }, wantErr: false, @@ -490,7 +531,8 @@ func TestMemorystoreClient_GetRecommendations_WithMockClient(t *testing.T) { name: "returns recommendations successfully", recommendations: []*recommenderpb.Recommendation{ { - Name: "projects/test/locations/us-central1/recommenders/google.memorystore.redis.PerformanceRecommender/recommendations/rec-1", + Name: "projects/test/locations/us-central1/recommenders/google.memorystore.redis.PerformanceRecommender/recommendations/rec-1", + StateInfo: &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE}, Content: &recommenderpb.RecommendationContent{ OperationGroups: []*recommenderpb.OperationGroup{ { @@ -592,7 +634,7 @@ func TestMemorystoreClient_ConvertGCPRecommendation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := client.convertGCPRecommendation(ctx, tt.rec) + result := client.convertGCPRecommendation(ctx, tt.rec, common.RecommendationParams{}) require.NotNil(t, result) assert.Equal(t, common.ProviderGCP, result.Provider) @@ -605,6 +647,115 @@ func TestMemorystoreClient_ConvertGCPRecommendation(t *testing.T) { } } +// TestConvertGCPRecommendation_PropagatesTermFromParams is a regression test for +// the finding that convertGCPRecommendation hardcoded rec.Term = "1yr", +// ignoring params.Term. A caller requesting "3yr" must get "3yr" in the output. +// +// This test FAILS on the pre-fix code that always set Term = "1yr". +func TestConvertGCPRecommendation_PropagatesTermFromParams(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + gcpRec := &recommenderpb.Recommendation{Name: "test-rec"} + + tests := []struct { + inputTerm string + wantTerm string + }{ + {"3yr", "3yr"}, + {"1yr", "1yr"}, + {"", "1yr"}, // empty defaults to "1yr" + } + + for _, tt := range tests { + params := common.RecommendationParams{Term: tt.inputTerm} + rec := client.convertGCPRecommendation(ctx, gcpRec, params) + require.NotNil(t, rec) + assert.Equal(t, tt.wantTerm, rec.Term, + "params.Term %q must produce rec.Term %q", tt.inputTerm, tt.wantTerm) + } +} + +// TestGetRedisPricing_CommitmentPriceIsTermTotal is a regression test for the +// unit-mismatch bug where commitmentPrice (per-hour from SKU) was passed +// directly to calculateSavingsPercentage which expects a term total, producing +// a ~99.99% savings percentage. After the fix, CommitmentPrice must equal +// hourlyRate * hoursInTerm and SavingsPercentage must be a realistic fraction. +// +// This test FAILS on the pre-fix code where CommitmentPrice == 0.035 (per-hour). +func TestGetRedisPricing_CommitmentPriceIsTermTotal(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + // onDemand = $0.05/hr, commitment = $0.035/hr + // hoursInTerm (1yr) = 8760 + // Expected: CommitmentPrice = 0.035*8760 = 306.6, OnDemandPrice = 0.05*8760 = 438 + // Expected savings ~= (438-306.6)/438*100 ~= 30% + mockBilling := &MockBillingService{ + skus: &cloudbilling.ListSkusResponse{ + Skus: []*cloudbilling.Sku{ + { + Description: "Memorystore Redis STANDARD_HA instance", + ServiceRegions: []string{"us-central1"}, + PricingInfo: []*cloudbilling.PricingInfo{ + { + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ + { + UnitPrice: &cloudbilling.Money{ + CurrencyCode: "USD", + Units: 0, + Nanos: 50000000, // $0.05/hr + }, + }, + }, + }, + }, + }, + }, + { + Description: "Memorystore Redis STANDARD_HA commitment 1yr", + ServiceRegions: []string{"us-central1"}, + PricingInfo: []*cloudbilling.PricingInfo{ + { + PricingExpression: &cloudbilling.PricingExpression{ + TieredRates: []*cloudbilling.TierRate{ + { + UnitPrice: &cloudbilling.Money{ + CurrencyCode: "USD", + Units: 0, + Nanos: 35000000, // $0.035/hr + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + client.SetBillingService(mockBilling) + + pricing, err := client.getRedisPricing(ctx, "STANDARD_HA", "us-central1", 1) + require.NoError(t, err) + + const hoursInYear = 8760.0 + // CommitmentPrice must be a term total, not the raw per-hour SKU rate. + assert.InDelta(t, 0.035*hoursInYear, pricing.CommitmentPrice, 0.01, + "CommitmentPrice must be commitment SKU hourly rate * hoursInTerm") + // HourlyRate must be the per-hour commitment rate. + assert.InDelta(t, 0.035, pricing.HourlyRate, 0.0001, + "HourlyRate must be the per-hour commitment SKU rate") + // OnDemandPrice stays a term total. + assert.InDelta(t, 0.05*hoursInYear, pricing.OnDemandPrice, 0.01) + // SavingsPercentage must be ~30%, not ~99.99%. + assert.Greater(t, pricing.SavingsPercentage, float64(1), + "SavingsPercentage must not be ~99.99% (unit-mismatch bug)") + assert.Less(t, pricing.SavingsPercentage, float64(60), + "SavingsPercentage must be a realistic 1yr CUD discount") +} + func TestMemorystoreClient_SetterMethods(t *testing.T) { ctx := context.Background() client, _ := NewClient(ctx, "test-project", "us-central1")