diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index c857d51c..1bc55bc2 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -12,6 +12,7 @@ import ( "regexp" "strconv" "strings" + "sync" "time" compute "cloud.google.com/go/compute/apiv1" @@ -30,7 +31,6 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/retry" "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skulist" - "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" ) // maxRecsPages caps GCP Recommender API iteration to avoid burning a Lambda @@ -268,6 +268,9 @@ type Client struct { machineTypesService MachineTypesService billingService BillingService recommenderClient RecommenderClient + + skuMu sync.Mutex + skuCache []*cloudbilling.Sku } // NewClient creates a new GCP Compute Engine client. @@ -293,6 +296,9 @@ func (c *Client) SetMachineTypesService(svc MachineTypesService) { // SetBillingService sets the billing service (for testing). func (c *Client) SetBillingService(svc BillingService) { c.billingService = svc + c.skuMu.Lock() + c.skuCache = nil + c.skuMu.Unlock() } // SetRecommenderClient sets the recommender client (for testing). @@ -971,7 +977,11 @@ func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendati termYears = 3 } - pricing, err := c.getComputePricing(ctx, rec.ResourceType, c.region, termYears) + res, err := resourcesFromRecommendation(rec) + if err != nil { + return nil, fmt.Errorf("failed to get pricing: %w", err) + } + pricing, err := c.getComputePricing(ctx, rec.ResourceType, c.region, termYears, res) if err != nil { return nil, fmt.Errorf("failed to get pricing: %w", err) } @@ -1053,52 +1063,6 @@ func (c *Client) GetValidResourceTypes(ctx context.Context) ([]string, error) { return machineTypes, nil } -// ComputePricing contains pricing information for Compute Engine. -type ComputePricing struct { - HourlyRate float64 - CommitmentPrice float64 - OnDemandPrice float64 - Currency string - SavingsPercentage float64 -} - -// 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 *Client) getComputePricing(ctx context.Context, machineType, region string, termYears int) (*ComputePricing, error) { - svc, err := c.getOrCreateBillingService(ctx) - if err != nil { - return nil, err - } - - skus, err := svc.ListSKUs("services/6F81-5844-456A") - if err != nil { - return nil, fmt.Errorf("failed to list SKUs: %w", err) - } - - onDemandPrice, commitmentPrice, currency, err := extractComputePricingFromSKUs(skus.Skus, machineType, region) - if err != nil { - return nil, err - } - if onDemandPrice == 0 { - return nil, fmt.Errorf("no on-demand pricing found for machine type %s", machineType) - } - if commitmentPrice == 0 { - 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{ - HourlyRate: commitmentPrice / hoursInTerm, - CommitmentPrice: commitmentPrice, - OnDemandPrice: onDemandPrice * hoursInTerm, - Currency: currency, - SavingsPercentage: savingsPercentage, - }, nil -} - // getOrCreateBillingService returns the billing service, creating it if needed. func (c *Client) getOrCreateBillingService(ctx context.Context) (BillingService, error) { if c.billingService != nil { @@ -1113,73 +1077,9 @@ func (c *Client) getOrCreateBillingService(ctx context.Context) (BillingService, return &realBillingService{ctx: ctx, service: service}, nil } -// extractComputePricingFromSKUs extracts on-demand and commitment pricing from SKU list. -func extractComputePricingFromSKUs(skus []*cloudbilling.Sku, machineType, region string) (onDemand, commitment float64, currency string, err error) { - currency = "USD" - - for _, sku := range skus { - if !skuMatchesMachineType(sku, machineType, region) { - continue - } - - price, curr := extractComputePriceFromSKU(sku) - if price == 0 { - continue - } - - if curr != "" { - currency = curr - } - - slot, slotErr := skumatch.Slot(sku) - if slotErr != nil { - return 0, 0, "", slotErr - } - switch slot { - case skumatch.SlotCommitment: - commitment = price - case skumatch.SlotOnDemand: - onDemand = price - } - } - - return onDemand, commitment, currency, nil -} - -// extractComputePriceFromSKU extracts the unit price from a SKU. -func extractComputePriceFromSKU(sku *cloudbilling.Sku) (price float64, currency string) { - if len(sku.PricingInfo) == 0 { - return 0, "" - } - - pricingInfo := sku.PricingInfo[0] - if pricingInfo.PricingExpression == nil || len(pricingInfo.PricingExpression.TieredRates) == 0 { - return 0, "" - } - - rate := pricingInfo.PricingExpression.TieredRates[0] - if rate.UnitPrice == nil { - return 0, "" - } - - price = float64(rate.UnitPrice.Units) + float64(rate.UnitPrice.Nanos)/1e9 - return price, rate.UnitPrice.CurrencyCode -} - -// calculateComputeSavingsPercentage calculates the savings percentage. -func calculateComputeSavingsPercentage(onDemandPrice, hoursInTerm, commitmentPrice float64) float64 { - onDemandTotal := onDemandPrice * hoursInTerm - return ((onDemandTotal - commitmentPrice) / onDemandTotal) * 100 -} - -// skuMatchesMachineType checks if a SKU matches the machine type and region. -func skuMatchesMachineType(sku *cloudbilling.Sku, machineType, region string) bool { - return skumatch.HasToken(sku.Description, machineType) && skumatch.InRegion(sku, region) -} - // 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 +// It also calls getComputePricing to fill RecurringMonthlyCost/OnDemandCost/SavingsPercentage +// 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 unrecognized so the caller skips an @@ -1229,51 +1129,38 @@ func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommend c.enrichRecWithPricing(ctx, rec) - // GCP Compute CUDs are monthly-payment commitments (PaymentOption is forced - // to "monthly" above), so the per-month charge is CommitmentCost / - // termMonths. enrichRecWithPricing performs the single billing lookup and - // populates CommitmentCost; reuse it here rather than issuing a second call. - // When the billing lookup failed CommitmentCost stays 0 and we leave - // RecurringMonthlyCost nil (unknown) so the frontend renders "—" rather than - // an incorrect explicit "$0" (nil means unavailable, 0 means known-zero fee). - if rec.CommitmentCost > 0 { - termYears := termYearsFromTerm(rec.Term) - monthly := rec.CommitmentCost / float64(termYears*12) - rec.RecurringMonthlyCost = &monthly - } - return rec, nil } -// 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). +// enrichRecWithPricing fills OnDemandCost, SavingsPercentage and +// RecurringMonthlyCost on rec from the billing catalog. The recommendation is a +// monthly CUD with no upfront payment, so CommitmentCost (the upfront amount) +// stays 0 and the committed price is the monthly recurring cost. OnDemandCost is +// the monthly on-demand price of the same resources, like the AWS and Azure +// providers and the platform baseline. A missing or invalid pricing entry is logged +// and leaves RecurringMonthlyCost nil (unknown, not zero) without discarding the +// recommendation: the Recommender EstimatedSavings is still the authoritative +// savings signal (issue #1022 C2). func (c *Client) enrichRecWithPricing(ctx context.Context, rec *common.Recommendation) { if rec.ResourceType == "" { return } termYears := termYearsFromTerm(rec.Term) - pricing, err := c.getComputePricing(ctx, rec.ResourceType, c.region, termYears) + res, err := resourcesFromRecommendation(*rec) + if err != nil { + log.Printf("computeengine: pricing unavailable for %s in %s: %v", rec.ResourceType, c.region, err) + return + } + pricing, err := c.getComputePricing(ctx, rec.ResourceType, c.region, termYears, res) 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 + termMonths := float64(termYears * 12) + monthly := pricing.CommitmentPrice / termMonths + rec.RecurringMonthlyCost = &monthly + rec.OnDemandCost = pricing.OnDemandPrice / termMonths 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 - } - } } // machineTypePathSegment precedes the machine type in a GCP resource path, e.g. diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index 304274a2..53d5b8ef 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -345,90 +345,6 @@ func TestComputeEngineClient_GetRegion(t *testing.T) { } } -func TestSkuMatchesMachineType(t *testing.T) { - tests := []struct { - name string - sku *cloudbilling.Sku - machineType string - region string - expected bool - }{ - { - name: "SKU matches machine type and region", - sku: &cloudbilling.Sku{ - Description: "n1-standard-1 VM running in Americas", - Category: &cloudbilling.Category{UsageType: "OnDemand"}, - ServiceRegions: []string{"us-central1"}, - }, - machineType: "n1-standard-1", - region: "us-central1", - expected: true, - }, - { - name: "SKU matches machine type but not region", - sku: &cloudbilling.Sku{ - Description: "n1-standard-1 VM running in Europe", - Category: &cloudbilling.Category{UsageType: "OnDemand"}, - ServiceRegions: []string{"europe-west1"}, - }, - machineType: "n1-standard-1", - region: "us-central1", - expected: false, - }, - { - name: "SKU does not match machine type", - sku: &cloudbilling.Sku{ - Description: "n2-highmem-4 VM running in Americas", - Category: &cloudbilling.Category{UsageType: "OnDemand"}, - ServiceRegions: []string{"us-central1"}, - }, - machineType: "n1-standard-1", - region: "us-central1", - expected: false, - }, - { - name: "SKU with nil service regions matches no region", - sku: &cloudbilling.Sku{ - Description: "n1-standard-1 VM", - Category: &cloudbilling.Category{UsageType: "OnDemand"}, - ServiceRegions: nil, - }, - machineType: "n1-standard-1", - region: "us-central1", - expected: false, - }, - { - name: "SKU for a longer machine type that starts with the same text is a decoy", - sku: &cloudbilling.Sku{ - Description: "n1-standard-16 VM running in Americas", - Category: &cloudbilling.Category{UsageType: "OnDemand"}, - ServiceRegions: []string{"us-central1"}, - }, - machineType: "n1-standard-1", - region: "us-central1", - expected: false, - }, - { - name: "Case insensitive machine type match", - sku: &cloudbilling.Sku{ - Description: "N1-STANDARD-1 VM running in Americas", - Category: &cloudbilling.Category{UsageType: "OnDemand"}, - ServiceRegions: []string{"us-central1"}, - }, - machineType: "n1-standard-1", - region: "us-central1", - expected: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := skuMatchesMachineType(tt.sku, tt.machineType, tt.region) - assert.Equal(t, tt.expected, result) - }) - } -} - func TestComputePricingStructure(t *testing.T) { pricing := ComputePricing{ HourlyRate: 0.10, @@ -1076,69 +992,24 @@ func TestComputeEngineClient_PurchaseCommitment_WaitError(t *testing.T) { 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{ - { - Description: "n1-standard-1 VM running in Americas", - Category: &cloudbilling.Category{UsageType: "OnDemand"}, - ServiceRegions: []string{"us-central1"}, - PricingInfo: []*cloudbilling.PricingInfo{ - { - PricingExpression: &cloudbilling.PricingExpression{ - TieredRates: []*cloudbilling.TierRate{ - { - UnitPrice: &cloudbilling.Money{ - Units: 0, - Nanos: 50000000, - CurrencyCode: "USD", - }, - }, - }, - }, - }, - }, - }, - { - Description: "n1-standard-1 commitment 1yr in Americas", - Category: &cloudbilling.Category{UsageType: "Commit1Yr"}, - ServiceRegions: []string{"us-central1"}, - PricingInfo: []*cloudbilling.PricingInfo{ - { - PricingExpression: &cloudbilling.PricingExpression{ - TieredRates: []*cloudbilling.TierRate{ - { - UnitPrice: &cloudbilling.Money{ - Units: 0, - Nanos: 32000000, - CurrencyCode: "USD", - }, - }, - }, - }, - }, - }, - }, - }, - }, - } - client.SetBillingService(mockService) + client.SetBillingService(mockBillingWithCommitment()) rec := common.Recommendation{ - ResourceType: "n1-standard-1", + ResourceType: "n1-standard-4", Term: "1yr", - PaymentOption: "upfront", + PaymentOption: "monthly", + Count: 4, + Details: common.ComputeDetails{MemoryGB: 15}, } details, err := client.GetOfferingDetails(ctx, rec) require.NoError(t, err) - assert.Equal(t, "n1-standard-1", details.ResourceType) + assert.Equal(t, "n1-standard-4", details.ResourceType) assert.Equal(t, "1yr", details.Term) assert.Equal(t, "USD", details.Currency) - assert.Greater(t, details.TotalCost, float64(0)) + // (4 vCPU * 0.019915 + 15 GiB * 0.002669) per hour over 8760 h. + assert.InDelta(t, (4*0.019915+15*0.002669)*8760, details.TotalCost, 1e-6) + assert.InDelta(t, details.TotalCost/12, details.RecurringCost, 1e-6) } func TestComputeEngineClient_GetOfferingDetails_NoPricing(t *testing.T) { @@ -1152,11 +1023,11 @@ func TestComputeEngineClient_GetOfferingDetails_NoPricing(t *testing.T) { } client.SetBillingService(mockService) - rec := common.Recommendation{ResourceType: "n1-standard-1"} + rec := common.Recommendation{ResourceType: "n1-standard-1", Count: 1, Details: common.ComputeDetails{MemoryGB: 3.75}} _, err := client.GetOfferingDetails(ctx, rec) assert.Error(t, err) - assert.Contains(t, err.Error(), "no on-demand pricing found") + assert.Contains(t, err.Error(), "no on-demand core and RAM pricing found") } func TestComputeEngineClient_GetRecommendations_WithMock(t *testing.T) { @@ -1308,8 +1179,8 @@ func TestComputeEngineClient_ConvertGCPRecommendation(t *testing.T) { assert.Equal(t, 50.5, rec.EstimatedSavings) assert.Equal(t, "monthly", rec.PaymentOption, `GCP CUDs are billed monthly; PaymentOption must match ValidPaymentOptionsByProvider["gcp"]`) - // GCP Compute CUDs are monthly-billed and RecurringMonthlyCost is derived - // from CommitmentCost. No billing service is injected here, so the pricing + // GCP Compute CUDs are monthly-billed and RecurringMonthlyCost is priced from + // the billing catalog. No billing service is injected here, so the pricing // lookup fails and the field stays nil (unknown) rather than an incorrect // explicit 0 (nil means unavailable, 0 means a known-zero recurring fee). assert.Nil(t, rec.RecurringMonthlyCost) @@ -1392,55 +1263,11 @@ func realisticCUDRecommendation() *recommenderpb.Recommendation { } } -// mockBillingWithCommitment returns a MockBillingService that has both an -// on-demand SKU and a commitment SKU for n1-standard machines in us-central1. +// mockBillingWithCommitment returns the live-catalog-shaped N1 core and RAM SKUs +// (on demand and 1-year commitment, us-central1) that the n1-standard-4 fixtures +// are priced against. func mockBillingWithCommitment() *MockBillingService { - return &MockBillingService{ - skus: &cloudbilling.ListSkusResponse{ - Skus: []*cloudbilling.Sku{ - { - Description: "n1-standard-4 VM running in Americas", - Category: &cloudbilling.Category{UsageType: "OnDemand"}, - 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", - Category: &cloudbilling.Category{UsageType: "Commit1Yr"}, - ServiceRegions: []string{"us-central1"}, - PricingInfo: []*cloudbilling.PricingInfo{ - { - PricingExpression: &cloudbilling.PricingExpression{ - TieredRates: []*cloudbilling.TierRate{ - { - UnitPrice: &cloudbilling.Money{ - Units: 0, - Nanos: 120000000, - CurrencyCode: "USD", - }, - }, - }, - }, - }, - }, - }, - }, - }, - } + return &MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: n1Americas1YrSKUs()}} } // TestConverterToInsert_CountNonZero_VCPUAmountSet is the primary regression test @@ -1521,7 +1348,11 @@ func TestConverterFillsPricingForScorer(t *testing.T) { // 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.Zero(t, rec.CommitmentCost, "a monthly CUD has no upfront payment, so CommitmentCost is 0 (platform computes upfront + monthly x term)") + require.NotNil(t, rec.RecurringMonthlyCost) + // 4 vCPU and 6 GiB at the 1-year N1 commitment rates, per month. + assert.InDelta(t, (4*0.019915+6*0.002669)*8760/12, *rec.RecurringMonthlyCost, 1e-6) + assert.Zero(t, rec.BreakEvenMonths, "no upfront payment, so there is nothing to break even on") assert.Greater(t, rec.OnDemandCost, float64(0), "OnDemandCost must be > 0") // Apply a non-trivial MinSavingsPct filter and confirm the rec passes. @@ -1567,35 +1398,12 @@ func TestGetComputePricing_NoCommitmentSKUReturnsError(t *testing.T) { ctx := context.Background() client, _ := NewClient(ctx, "test-project", "us-central1") - // Only on-demand SKU -- no commitment SKU. + // Only the on-demand SKUs -- no commitment SKU. client.SetBillingService(&MockBillingService{ - skus: &cloudbilling.ListSkusResponse{ - Skus: []*cloudbilling.Sku{ - { - Description: "n1-standard-4 VM running in Americas", - Category: &cloudbilling.Category{UsageType: "OnDemand"}, - ServiceRegions: []string{"us-central1"}, - PricingInfo: []*cloudbilling.PricingInfo{ - { - PricingExpression: &cloudbilling.PricingExpression{ - TieredRates: []*cloudbilling.TierRate{ - { - UnitPrice: &cloudbilling.Money{ - Units: 0, - Nanos: 190000000, - CurrencyCode: "USD", - }, - }, - }, - }, - }, - }, - }, - }, - }, + skus: &cloudbilling.ListSkusResponse{Skus: n1Americas1YrSKUs()[:2]}, }) - pricing, err := client.getComputePricing(ctx, "n1-standard-4", "us-central1", 1) + pricing, err := client.getComputePricing(ctx, "n1-standard-4", "us-central1", 1, computeResources{vcpus: 4, memoryGiB: 15}) 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") diff --git a/providers/gcp/services/computeengine/pricing.go b/providers/gcp/services/computeengine/pricing.go new file mode 100644 index 00000000..1c7fe255 --- /dev/null +++ b/providers/gcp/services/computeengine/pricing.go @@ -0,0 +1,318 @@ +package computeengine + +import ( + "context" + "fmt" + "regexp" + "strings" + + "google.golang.org/api/cloudbilling/v1" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" +) + +const ( + // computeServiceID is the Cloud Billing catalog service for Compute Engine. + computeServiceID = "services/6F81-5844-456A" + + hoursPerYear = 8760.0 + + usageUnitHour = "h" + usageUnitGiBHour = "GiBy.h" + + usageTypeCommit1Yr = "Commit1Yr" + usageTypeCommit3Yr = "Commit3Yr" +) + +// ComputePricing is the price of a commitment's resources, for one term. +type ComputePricing struct { + // HourlyRate is the committed price per hour for the whole resource set. + HourlyRate float64 + // CommitmentPrice is the committed price over the whole term. + CommitmentPrice float64 + // OnDemandPrice is the on-demand price of the same resources over the term. + OnDemandPrice float64 + Currency string + SavingsPercentage float64 +} + +// computeResources is the amount a commitment covers. +type computeResources struct { + vcpus int + memoryGiB float64 +} + +// resourcesFromRecommendation reads the vCPU count and memory a recommendation +// covers; both are required to price it. +func resourcesFromRecommendation(rec common.Recommendation) (computeResources, error) { + if rec.Count <= 0 { + return computeResources{}, fmt.Errorf("recommendation for %s has no vCPU count; cannot price it", rec.ResourceType) + } + memMB, err := memoryMBFromDetails(rec) + if err != nil { + return computeResources{}, err + } + return computeResources{vcpus: rec.Count, memoryGiB: float64(memMB) / 1024.0}, nil +} + +// skuFamilyLabels holds how the catalog names a machine family. The labels are +// not derivable from the machine type prefix for every family. +type skuFamilyLabels struct { + onDemand string // "