diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cb5536e..82724b42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -155,6 +155,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Changed +- GCP Cloud Storage pricing lists the SKU catalog once per client, like Cloud + SQL and Memorystore. - Azure multi-subscription collection keeps skipped recommendations when a subscription also fails. They are returned by the new `PartialSubscriptionFailureError.Skips()` (a `*PartialResultError`, or nil) diff --git a/providers/gcp/internal/skumatch/savings.go b/providers/gcp/internal/skumatch/savings.go index efcc4add..0113c9fb 100644 --- a/providers/gcp/internal/skumatch/savings.go +++ b/providers/gcp/internal/skumatch/savings.go @@ -1,5 +1,11 @@ package skumatch +import ( + "fmt" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + // SavingsPercent returns the percentage a commitment saves against on-demand // over a term. commitmentTotal must already be a term total (per-hour price * // hoursInTerm), on the same basis as onDemandPrice * hoursInTerm. A zero @@ -9,3 +15,59 @@ func SavingsPercent(onDemandPrice, hoursInTerm, commitmentTotal float64) float64 onDemandTotal := onDemandPrice * hoursInTerm return ((onDemandTotal - commitmentTotal) / onDemandTotal) * 100 } + +// TermPrices holds per-hour catalog prices scaled to a commitment term. +type TermPrices struct { + HourlyRate float64 + CommitmentPrice float64 + OnDemandPrice float64 + SavingsPercentage float64 +} + +// ScaleToTerm rejects missing catalog prices and scales both per-hour prices +// to term totals, so the savings percentage compares like with like (a +// per-hour commitment against a term-total on-demand would read near 100%). +func ScaleToTerm(what, region string, onDemandHourly, commitmentHourly float64, termYears int) (TermPrices, error) { + if onDemandHourly <= 0 { + return TermPrices{}, fmt.Errorf("no pricing found for %s", what) + } + if commitmentHourly <= 0 { + return TermPrices{}, fmt.Errorf("no commitment pricing found for %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", what, region) + } + hoursInTerm := common.HoursPerYear * float64(termYears) + commitmentTotal := commitmentHourly * hoursInTerm + return TermPrices{ + HourlyRate: commitmentHourly, + CommitmentPrice: commitmentTotal, + OnDemandPrice: onDemandHourly * hoursInTerm, + SavingsPercentage: SavingsPercent(onDemandHourly, hoursInTerm, commitmentTotal), + }, nil +} + +// OfferingDetails builds the offering for a commitment total, paying it up +// front or spreading it monthly by payment option. An unknown option pays the +// total up front. +func OfferingDetails(rec common.Recommendation, offeringID string, termYears int, commitmentTotal, hourlyRate float64, currency string) *common.OfferingDetails { + var upfrontCost, recurringCost float64 + + switch rec.PaymentOption { + case "all-upfront", "upfront": + upfrontCost = commitmentTotal + case "monthly", "no-upfront": + recurringCost = commitmentTotal / (float64(termYears) * 12) + default: + upfrontCost = commitmentTotal + } + + return &common.OfferingDetails{ + OfferingID: offeringID, + ResourceType: rec.ResourceType, + Term: rec.Term, + PaymentOption: rec.PaymentOption, + UpfrontCost: upfrontCost, + RecurringCost: recurringCost, + TotalCost: commitmentTotal, + EffectiveHourlyRate: hourlyRate, + Currency: currency, + } +} diff --git a/providers/gcp/internal/skumatch/savings_test.go b/providers/gcp/internal/skumatch/savings_test.go index 07500bee..800b3811 100644 --- a/providers/gcp/internal/skumatch/savings_test.go +++ b/providers/gcp/internal/skumatch/savings_test.go @@ -1,9 +1,11 @@ package skumatch import ( + "fmt" "math" "testing" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/stretchr/testify/assert" "google.golang.org/api/cloudbilling/v1" ) @@ -69,3 +71,76 @@ func TestMatchesTier(t *testing.T) { }) } } + +func TestScaleToTerm(t *testing.T) { + t.Parallel() + tests := []struct { + name string + onDemand float64 + commitment float64 + years int + want TermPrices + wantErrText string + }{ + {name: "zero on-demand", onDemand: 0, commitment: 0.05, years: 1, wantErrText: "no pricing found for Thing x"}, + {name: "negative on-demand", onDemand: -1, commitment: 0.05, years: 1, wantErrText: "no pricing found for Thing x"}, + {name: "zero commitment", onDemand: 0.1, commitment: 0, years: 1, wantErrText: "no commitment pricing found for Thing x in region r1: catalog has no CUD SKU; cannot compute savings percentage"}, + {name: "negative commitment", onDemand: 0.1, commitment: -0.05, years: 3, wantErrText: "no commitment pricing found for Thing x in region r1: catalog has no CUD SKU; cannot compute savings percentage"}, + {name: "1 year", onDemand: 0.1, commitment: 0.05, years: 1, want: TermPrices{HourlyRate: 0.05, CommitmentPrice: 0.05 * 8760, OnDemandPrice: 0.1 * 8760, SavingsPercentage: 50}}, + {name: "3 year", onDemand: 0.2, commitment: 0.1, years: 3, want: TermPrices{HourlyRate: 0.1, CommitmentPrice: 0.1 * 26280, OnDemandPrice: 0.2 * 26280, SavingsPercentage: 50}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := ScaleToTerm("Thing x", "r1", tt.onDemand, tt.commitment, tt.years) + if tt.wantErrText != "" { + assert.EqualError(t, err, tt.wantErrText) + return + } + assert.NoError(t, err) + assert.InDelta(t, tt.want.HourlyRate, got.HourlyRate, 1e-9) + assert.InDelta(t, tt.want.CommitmentPrice, got.CommitmentPrice, 1e-6) + assert.InDelta(t, tt.want.OnDemandPrice, got.OnDemandPrice, 1e-6) + assert.InDelta(t, tt.want.SavingsPercentage, got.SavingsPercentage, 1e-9) + }) + } +} + +func TestOfferingDetails(t *testing.T) { + t.Parallel() + tests := []struct { + option string + years int + wantUpfront float64 + wantRecurring float64 + }{ + {"all-upfront", 1, 1200, 0}, + {"upfront", 3, 1200, 0}, + {"monthly", 1, 0, 100}, + {"monthly", 3, 0, 1200.0 / 36}, + {"no-upfront", 1, 0, 100}, + {"no-upfront", 3, 0, 1200.0 / 36}, + {"bogus", 1, 1200, 0}, + {"bogus", 3, 1200, 0}, + {"", 1, 1200, 0}, + {"", 3, 1200, 0}, + } + for _, tt := range tests { + t.Run(fmt.Sprintf("%q/%dyr", tt.option, tt.years), func(t *testing.T) { + t.Parallel() + rec := common.Recommendation{ResourceType: "rt", Term: "term-x", PaymentOption: tt.option} + got := OfferingDetails(rec, "id-1", tt.years, 1200, 0.5, "EUR") + assert.Equal(t, &common.OfferingDetails{ + OfferingID: "id-1", + ResourceType: "rt", + Term: "term-x", + PaymentOption: tt.option, + UpfrontCost: tt.wantUpfront, + RecurringCost: tt.wantRecurring, + TotalCost: 1200, + EffectiveHourlyRate: 0.5, + Currency: "EUR", + }, got) + }) + } +} diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index 48042b07..4da2ce5d 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -4,6 +4,7 @@ package cloudsql import ( "context" "fmt" + "slices" "strings" "time" @@ -156,10 +157,8 @@ func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation return fmt.Errorf("failed to get valid tiers: %w", err) } - for _, tier := range validTiers { - if tier == rec.ResourceType { - return nil - } + if slices.Contains(validTiers, rec.ResourceType) { + return nil } return fmt.Errorf("invalid Cloud SQL tier: %s", rec.ResourceType) @@ -177,31 +176,8 @@ func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendati return nil, fmt.Errorf("failed to get pricing: %w", err) } - var upfrontCost, recurringCost float64 - totalCost := pricing.CommitmentPrice - - switch rec.PaymentOption { - case "all-upfront", "upfront": - upfrontCost = totalCost - recurringCost = 0 - case "monthly", "no-upfront": - upfrontCost = 0 - recurringCost = totalCost / (float64(termYears) * 12) - default: - upfrontCost = totalCost - } - - return &common.OfferingDetails{ - OfferingID: fmt.Sprintf("gcp-cloudsql-%s-%s-%s", rec.ResourceType, c.region, rec.Term), - ResourceType: rec.ResourceType, - Term: rec.Term, - PaymentOption: rec.PaymentOption, - UpfrontCost: upfrontCost, - RecurringCost: recurringCost, - TotalCost: totalCost, - EffectiveHourlyRate: pricing.HourlyRate, - Currency: pricing.Currency, - }, nil + offeringID := fmt.Sprintf("gcp-cloudsql-%s-%s-%s", rec.ResourceType, c.region, rec.Term) + return skumatch.OfferingDetails(rec, offeringID, termYears, pricing.CommitmentPrice, pricing.HourlyRate, pricing.Currency), nil } // GetValidResourceTypes returns valid Cloud SQL tiers. @@ -266,26 +242,17 @@ func (c *Client) getSQLPricing(ctx context.Context, tier, region string, termYea if err != nil { return nil, err } - 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) + prices, err := skumatch.ScaleToTerm("Cloud SQL tier "+tier, region, onDemandPrice, commitmentPrice, termYears) + if err != nil { + return nil, err } - hoursInTerm := common.HoursPerYear * 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 := skumatch.SavingsPercent(onDemandPrice, hoursInTerm, commitmentPriceTerm) - return &SQLPricing{ - HourlyRate: commitmentPrice, - CommitmentPrice: commitmentPriceTerm, - OnDemandPrice: onDemandPrice * hoursInTerm, + HourlyRate: prices.HourlyRate, + CommitmentPrice: prices.CommitmentPrice, + OnDemandPrice: prices.OnDemandPrice, Currency: currency, - SavingsPercentage: savingsPercentage, + SavingsPercentage: prices.SavingsPercentage, }, nil } diff --git a/providers/gcp/services/cloudsql/scale_errors_test.go b/providers/gcp/services/cloudsql/scale_errors_test.go new file mode 100644 index 00000000..15d8dda4 --- /dev/null +++ b/providers/gcp/services/cloudsql/scale_errors_test.go @@ -0,0 +1,40 @@ +package cloudsql + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +// Pins the service label and region that ScaleToTerm puts in its errors. +func TestGetSQLPricing_ScaleErrorsNameLabelAndRegion(t *testing.T) { + t.Parallel() + const region = "europe-west4" + tests := []struct { + name string + onDemand, commit int64 + wantErr string + }{ + {"no on-demand", -50000000, 42000000, "no pricing found for Cloud SQL tier db-n1-standard-1"}, + {"no commitment", 50000000, -42000000, "no commitment pricing found for Cloud SQL tier db-n1-standard-1 in region europe-west4: catalog has no CUD SKU; cannot compute savings percentage"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + client, err := NewClient(ctx, "test-project", region) + require.NoError(t, err) + skus := negPriceSkus(tt.onDemand, tt.commit) + for _, sku := range skus { + sku.ServiceRegions = []string{region} + } + client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: skus}}) + + _, err = client.getSQLPricing(ctx, "db-n1-standard-1", region, 1) + assert.EqualError(t, err, tt.wantErr) + }) + } +} diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index 93cea1f5..5dd46e13 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -4,6 +4,7 @@ package cloudstorage import ( "context" "fmt" + "slices" "strings" "time" @@ -54,6 +55,7 @@ type RecommenderIterator interface { // Client handles GCP Cloud Storage commitments. type Client struct { + skus skulist.Cache ctx context.Context projectID string region string @@ -138,10 +140,8 @@ func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation return fmt.Errorf("failed to get valid storage classes: %w", err) } - for _, class := range validClasses { - if class == rec.ResourceType { - return nil - } + if slices.Contains(validClasses, rec.ResourceType) { + return nil } return fmt.Errorf("invalid Cloud Storage class: %s", rec.ResourceType) @@ -159,31 +159,8 @@ func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendati return nil, fmt.Errorf("failed to get pricing: %w", err) } - var upfrontCost, recurringCost float64 - totalCost := pricing.CommitmentPrice - - switch rec.PaymentOption { - case "all-upfront", "upfront": - upfrontCost = totalCost - recurringCost = 0 - case "monthly", "no-upfront": - upfrontCost = 0 - recurringCost = totalCost / (float64(termYears) * 12) - default: - upfrontCost = totalCost - } - - return &common.OfferingDetails{ - OfferingID: fmt.Sprintf("gcp-storage-%s-%s-%s", rec.ResourceType, c.region, rec.Term), - ResourceType: rec.ResourceType, - Term: rec.Term, - PaymentOption: rec.PaymentOption, - UpfrontCost: upfrontCost, - RecurringCost: recurringCost, - TotalCost: totalCost, - EffectiveHourlyRate: pricing.HourlyRate, - Currency: pricing.Currency, - }, nil + offeringID := fmt.Sprintf("gcp-storage-%s-%s-%s", rec.ResourceType, c.region, rec.Term) + return skumatch.OfferingDetails(rec, offeringID, termYears, pricing.CommitmentPrice, pricing.HourlyRate, pricing.Currency), nil } // GetValidResourceTypes returns valid Cloud Storage classes. @@ -217,7 +194,7 @@ func (c *Client) getStoragePricing(ctx context.Context, storageClass, region str return nil, err } - skus, err := svc.ListSKUs(cloudStorageServiceID) + skus, err := c.listSKUs(svc) if err != nil { return nil, fmt.Errorf("failed to list SKUs: %w", err) } @@ -226,23 +203,17 @@ func (c *Client) getStoragePricing(ctx context.Context, storageClass, region str if err != nil { return nil, err } - if onDemandPrice <= 0 { - return nil, fmt.Errorf("no pricing found for Cloud Storage class %s", storageClass) - } - if commitmentPrice <= 0 { - 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) + prices, err := skumatch.ScaleToTerm("Cloud Storage class "+storageClass, region, onDemandPrice, commitmentPrice, termYears) + if err != nil { + return nil, err } - hoursInTerm := common.HoursPerYear * float64(termYears) - commitmentPriceTerm := commitmentPrice * hoursInTerm - savingsPercentage := skumatch.SavingsPercent(onDemandPrice, hoursInTerm, commitmentPriceTerm) - return &StoragePricing{ - HourlyRate: commitmentPrice, - CommitmentPrice: commitmentPriceTerm, - OnDemandPrice: onDemandPrice * hoursInTerm, + HourlyRate: prices.HourlyRate, + CommitmentPrice: prices.CommitmentPrice, + OnDemandPrice: prices.OnDemandPrice, Currency: currency, - SavingsPercentage: savingsPercentage, + SavingsPercentage: prices.SavingsPercentage, }, nil } @@ -282,5 +253,11 @@ func skuMatchesStorageClass(sku *cloudbilling.Sku, storageClass, region string) return skumatch.InRegion(sku, region) } +// listSKUs returns the service's SKU catalog, listing it once per client so +// that pricing N recommendations costs one catalog read instead of N. +func (c *Client) listSKUs(svc BillingService) (*cloudbilling.ListSkusResponse, error) { + return c.skus.Get(svc, cloudStorageServiceID) +} + // cloudStorageServiceID is the Cloud Billing service for Cloud Storage. const cloudStorageServiceID = "services/95FF-2EF5-5EA1" diff --git a/providers/gcp/services/cloudstorage/pagination_test.go b/providers/gcp/services/cloudstorage/pagination_test.go new file mode 100644 index 00000000..5f5e6296 --- /dev/null +++ b/providers/gcp/services/cloudstorage/pagination_test.go @@ -0,0 +1,51 @@ +package cloudstorage + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +// Pricing N recommendations lists the SKU catalog once. +type countingBilling struct { + BillingService + calls int +} + +func (b *countingBilling) ListSKUs(id string) (*cloudbilling.ListSkusResponse, error) { + b.calls++ + return b.BillingService.ListSKUs(id) +} + +func TestListSKUs_CachedPerClient(t *testing.T) { + t.Parallel() + b := &countingBilling{BillingService: &MockBillingService{skus: &cloudbilling.ListSkusResponse{}}} + c, _ := NewClient(context.Background(), "p", "us-central1") + for i := 0; i < 3; i++ { + _, err := c.listSKUs(b) + require.NoError(t, err) + } + assert.Equal(t, 1, b.calls) +} + +type idRecordingBilling struct { + BillingService + id string +} + +func (b *idRecordingBilling) ListSKUs(id string) (*cloudbilling.ListSkusResponse, error) { + b.id = id + return b.BillingService.ListSKUs(id) +} + +func TestListSKUs_UsesCloudStorageBillingService(t *testing.T) { + t.Parallel() + b := &idRecordingBilling{BillingService: &MockBillingService{skus: &cloudbilling.ListSkusResponse{}}} + c, _ := NewClient(context.Background(), "p", "us-central1") + _, err := c.listSKUs(b) + require.NoError(t, err) + assert.Equal(t, "services/95FF-2EF5-5EA1", b.id) +} diff --git a/providers/gcp/services/cloudstorage/scale_errors_test.go b/providers/gcp/services/cloudstorage/scale_errors_test.go new file mode 100644 index 00000000..f2ea3b18 --- /dev/null +++ b/providers/gcp/services/cloudstorage/scale_errors_test.go @@ -0,0 +1,40 @@ +package cloudstorage + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +// Pins the service label and region that ScaleToTerm puts in its errors. +func TestGetStoragePricing_ScaleErrorsNameLabelAndRegion(t *testing.T) { + t.Parallel() + const region = "europe-west4" + tests := []struct { + name string + onDemand, commit int64 + wantErr string + }{ + {"no on-demand", -50000000, 42000000, "no pricing found for Cloud Storage class STANDARD"}, + {"no commitment", 50000000, -42000000, "no commitment pricing found for Cloud Storage class STANDARD in region europe-west4: catalog has no CUD SKU; cannot compute savings percentage"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + client, err := NewClient(ctx, "test-project", region) + require.NoError(t, err) + skus := storageMockSkus("STANDARD", tt.onDemand, tt.commit) + for _, sku := range skus { + sku.ServiceRegions = []string{region} + } + client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: skus}}) + + _, err = client.getStoragePricing(ctx, "STANDARD", region, 1) + assert.EqualError(t, err, tt.wantErr) + }) + } +} diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index dfa459c2..0b846965 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -901,31 +901,8 @@ func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendati return nil, fmt.Errorf("failed to get pricing: %w", err) } - var upfrontCost, recurringCost float64 - totalCost := pricing.CommitmentPrice - - switch rec.PaymentOption { - case "all-upfront", "upfront": - upfrontCost = totalCost - recurringCost = 0 - case "monthly", "no-upfront": - upfrontCost = 0 - recurringCost = totalCost / (float64(termYears) * 12) - default: - upfrontCost = totalCost - } - - return &common.OfferingDetails{ - OfferingID: fmt.Sprintf("gcp-compute-%s-%s-%s", rec.ResourceType, c.region, rec.Term), - ResourceType: rec.ResourceType, - Term: rec.Term, - PaymentOption: rec.PaymentOption, - UpfrontCost: upfrontCost, - RecurringCost: recurringCost, - TotalCost: totalCost, - EffectiveHourlyRate: pricing.HourlyRate, - Currency: pricing.Currency, - }, nil + offeringID := fmt.Sprintf("gcp-compute-%s-%s-%s", rec.ResourceType, c.region, rec.Term) + return skumatch.OfferingDetails(rec, offeringID, termYears, pricing.CommitmentPrice, pricing.HourlyRate, pricing.Currency), nil } // GetValidResourceTypes returns valid machine types from GCP Compute API. diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index 7424c241..969625d2 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -4,6 +4,7 @@ package memorystore import ( "context" "fmt" + "slices" "time" "cloud.google.com/go/recommender/apiv1/recommenderpb" @@ -143,10 +144,8 @@ func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation return fmt.Errorf("failed to get valid tiers: %w", err) } - for _, tier := range validTiers { - if tier == rec.ResourceType { - return nil - } + if slices.Contains(validTiers, rec.ResourceType) { + return nil } return fmt.Errorf("invalid Memorystore tier: %s", rec.ResourceType) @@ -164,31 +163,8 @@ func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendati return nil, fmt.Errorf("failed to get pricing: %w", err) } - var upfrontCost, recurringCost float64 - totalCost := pricing.CommitmentPrice - - switch rec.PaymentOption { - case "all-upfront", "upfront": - upfrontCost = totalCost - recurringCost = 0 - case "monthly", "no-upfront": - upfrontCost = 0 - recurringCost = totalCost / (float64(termYears) * 12) - default: - upfrontCost = totalCost - } - - return &common.OfferingDetails{ - OfferingID: fmt.Sprintf("gcp-memorystore-%s-%s-%s", rec.ResourceType, c.region, rec.Term), - ResourceType: rec.ResourceType, - Term: rec.Term, - PaymentOption: rec.PaymentOption, - UpfrontCost: upfrontCost, - RecurringCost: recurringCost, - TotalCost: totalCost, - EffectiveHourlyRate: pricing.HourlyRate, - Currency: pricing.Currency, - }, nil + offeringID := fmt.Sprintf("gcp-memorystore-%s-%s-%s", rec.ResourceType, c.region, rec.Term) + return skumatch.OfferingDetails(rec, offeringID, termYears, pricing.CommitmentPrice, pricing.HourlyRate, pricing.Currency), nil } // GetValidResourceTypes returns valid Memorystore tiers. @@ -229,26 +205,17 @@ func (c *Client) getRedisPricing(ctx context.Context, tier, region string, termY if err != nil { return nil, err } - if onDemandPrice <= 0 { - return nil, fmt.Errorf("no pricing found for Memorystore tier %s", tier) - } - if commitmentPrice <= 0 { - 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) + prices, err := skumatch.ScaleToTerm("Memorystore tier "+tier, region, onDemandPrice, commitmentPrice, termYears) + if err != nil { + return nil, err } - hoursInTerm := common.HoursPerYear * 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 := skumatch.SavingsPercent(onDemandPrice, hoursInTerm, commitmentPriceTerm) - return &RedisPricing{ - HourlyRate: commitmentPrice, - CommitmentPrice: commitmentPriceTerm, - OnDemandPrice: onDemandPrice * hoursInTerm, + HourlyRate: prices.HourlyRate, + CommitmentPrice: prices.CommitmentPrice, + OnDemandPrice: prices.OnDemandPrice, Currency: currency, - SavingsPercentage: savingsPercentage, + SavingsPercentage: prices.SavingsPercentage, }, nil } diff --git a/providers/gcp/services/memorystore/scale_errors_test.go b/providers/gcp/services/memorystore/scale_errors_test.go new file mode 100644 index 00000000..e1ea6ee6 --- /dev/null +++ b/providers/gcp/services/memorystore/scale_errors_test.go @@ -0,0 +1,40 @@ +package memorystore + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +// Pins the service label and region that ScaleToTerm puts in its errors. +func TestGetRedisPricing_ScaleErrorsNameLabelAndRegion(t *testing.T) { + t.Parallel() + const region = "europe-west4" + tests := []struct { + name string + onDemand, commit int64 + wantErr string + }{ + {"no on-demand", -50000000, 42000000, "no pricing found for Memorystore tier STANDARD_HA"}, + {"no commitment", 50000000, -42000000, "no commitment pricing found for Memorystore tier STANDARD_HA in region europe-west4: catalog has no CUD SKU; cannot compute savings percentage"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + client, err := NewClient(ctx, "test-project", region) + require.NoError(t, err) + skus := negPriceSkus(tt.onDemand, tt.commit) + for _, sku := range skus { + sku.ServiceRegions = []string{region} + } + client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: skus}}) + + _, err = client.getRedisPricing(ctx, "STANDARD_HA", region, 1) + assert.EqualError(t, err, tt.wantErr) + }) + } +}