From 3d6e8c59eb6e8276b6fbf614025073435d8ef917 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 18:38:25 +0100 Subject: [PATCH] fix(gcp): price m4-ultramem-224 from its own catalog rows m4-ultramem-224 was priced from the plain M4 rows, understating RAM cost by about 25%. familyLabels now maps it to M4Ultramem224 so it matches its own SKUs; RAM quoted in decimal GBy.h converts via the existing #722 factor. m4n-* and unknown M4 ultramem sizes return errUnknownCatalogFamily; m1, m2 and m3 ultramem shapes still resolve through skuFamilyOverrides. The commitment SKU text is assumed by analogy and unverified; a wrong text fails closed with "no commitment pricing found". Closes #719 Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 8 ++ .../gcp/services/computeengine/pricing.go | 22 ++++- .../services/computeengine/pricing_test.go | 96 +++++++++++++++++++ 3 files changed, 125 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 272894f9..9c209782 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -317,6 +317,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed - **Duplicate check:** a recent purchase now reduces every term and payment variant of a recommendation, not only the first one in input order, for EC2 as well as every other service. Each `term|payment` group starts from the full held quantity and consumes it in input order, so one held unit against four variants of 2 yields 1, 1, 1, 1 (was 1, 2, 2, 2). Providers pick this up on their next `pkg` pin bump. Fixes #703. +- **`providers/gcp`:** `m4-ultramem-224` now matches its own `M4Ultramem224` + catalog rows instead of the plain `M4` rates, which understated RAM cost by + about 25%. The commitment SKU text (`Commitment v1: M4Ultramem224 Cpu/Ram in + ...`) is assumed by analogy with other families and not verified against the + live catalog; a wrong text fails closed with "no commitment pricing found". + Other M4 ultramem shapes (`m4n-*` and unknown sizes) are refused with an + unknown-catalog-family error. The `m1`, `m2` and `m3` ultramem shapes are + unchanged. Fixes #719. - **`common`:** a `Recommendation`, and a `PurchaseResult` carrying one (with no `Error`), now decodes from JSON when `details` holds an object. `details` is decoded into the typed pointer for the recommendation's service through diff --git a/providers/gcp/services/computeengine/pricing.go b/providers/gcp/services/computeengine/pricing.go index 92309f04..e87c0f23 100644 --- a/providers/gcp/services/computeengine/pricing.go +++ b/providers/gcp/services/computeengine/pricing.go @@ -2,6 +2,7 @@ package computeengine import ( "context" + "errors" "fmt" "regexp" "strings" @@ -82,11 +83,30 @@ var skuFamilyOverrides = map[string]skuFamilyLabels{ "m3": {onDemand: "M3 Memory-optimized", commit: []string{"M3 Memory-optimized"}}, } +// errUnknownCatalogFamily marks a machine type whose catalog SKU family cannot +// be derived exactly; pricing it would risk taking another family's rates. +var errUnknownCatalogFamily = errors.New("unknown catalog family") + +// m4UltramemSizes lists the M4 Ultramem sizes the code accepts. The label +// "M4Ultramem224" (no space) is from the live catalog's on-demand rows; the +// commitment text "Commitment v1: M4Ultramem224 Cpu/Ram" is assumed by analogy, +// and a wrong text fails closed with "no commitment pricing found". +var m4UltramemSizes = map[string]bool{"224": true} + func familyLabels(machineType string) (skuFamilyLabels, error) { - family, _, _ := strings.Cut(strings.ToLower(strings.TrimSpace(machineType)), "-") + mt := strings.ToLower(strings.TrimSpace(machineType)) + family, rest, _ := strings.Cut(mt, "-") if family == "" { return skuFamilyLabels{}, fmt.Errorf("no machine family in machine type %q", machineType) } + if (family == "m4" || family == "m4n") && strings.HasPrefix(rest, "ultramem") { + size, ok := strings.CutPrefix(rest, "ultramem-") + if family != "m4" || !ok || !m4UltramemSizes[size] { + return skuFamilyLabels{}, fmt.Errorf("%w: cannot derive the catalog label for machine type %q", errUnknownCatalogFamily, machineType) + } + label := "M4Ultramem" + size + return skuFamilyLabels{onDemand: label, commit: []string{label}}, nil + } if labels, ok := skuFamilyOverrides[family]; ok { return labels, nil } diff --git a/providers/gcp/services/computeengine/pricing_test.go b/providers/gcp/services/computeengine/pricing_test.go index b57db99f..88974986 100644 --- a/providers/gcp/services/computeengine/pricing_test.go +++ b/providers/gcp/services/computeengine/pricing_test.go @@ -2,6 +2,7 @@ package computeengine import ( "context" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -387,3 +388,98 @@ func TestGetComputePricing_C4DGBytePricedRAM(t *testing.T) { assert.InDelta(t, (8*0.0147166+30*0.001572996*gibPerGB)*8760*3, three.CommitmentPrice, 1e-6) assert.Greater(t, three.SavingsPercentage, one.SavingsPercentage) } + +// m4AndUltramemSKUs carries plain M4 rows next to M4Ultramem224 rows. The +// catalog writes the Ultramem family with no space after "M4". RAM is GiBy.h +// here; TestFindFamilyRates_M4UltramemGBHRamPrices covers the live GBy.h unit. +func m4AndUltramemSKUs() []*cloudbilling.Sku { + return []*cloudbilling.Sku{ + catalogSKU("m4-od-core", "M4 Instance Core running in Americas", "CPU", "OnDemand", "h", 4000000, americas), + catalogSKU("m4-od-ram", "M4 Instance Ram running in Americas", "RAM", "OnDemand", "GiBy.h", 4570000, americas), + catalogSKU("m4-c1-core", "Commitment v1: M4 Cpu in Americas for 1 Year", "CPU", "Commit1Yr", "h", 2500000, americas), + catalogSKU("m4-c1-ram", "Commitment v1: M4 Ram in Americas for 1 Year", "RAM", "Commit1Yr", "GiBy.h", 2900000, americas), + catalogSKU("mu-od-core", "M4Ultramem224 Instance Core running in Americas", "CPU", "OnDemand", "h", 8000000, americas), + catalogSKU("mu-od-ram", "M4Ultramem224 Instance Ram running in Americas", "RAM", "OnDemand", "GiBy.h", 5675880, americas), + catalogSKU("mu-c1-core", "Commitment v1: M4Ultramem224 Cpu in Americas for 1 Year", "CPU", "Commit1Yr", "h", 5000000, americas), + catalogSKU("mu-c1-ram", "Commitment v1: M4Ultramem224 Ram in Americas for 1 Year", "RAM", "Commit1Yr", "GiBy.h", 3500000, americas), + } +} + +func TestFamilyLabels_M4Ultramem(t *testing.T) { + t.Parallel() + labels, err := familyLabels("m4-ultramem-224") + require.NoError(t, err) + assert.Equal(t, "M4Ultramem224", labels.onDemand) + assert.Equal(t, []string{"M4Ultramem224"}, labels.commit) + + for _, mt := range []string{"m4-ultramem-999", "m4-ultramem", "m4-ultramem-", "m4n-ultramem-224"} { + _, err := familyLabels(mt) + require.ErrorIs(t, err, errUnknownCatalogFamily, mt) + } +} + +func TestClassify_M4UltramemNeverMatchesPlainM4AndViceVersa(t *testing.T) { + t.Parallel() + ultra, err := familyLabels("m4-ultramem-224") + require.NoError(t, err) + plain, err := familyLabels("m4-megamem-28") + require.NoError(t, err) + + for _, sku := range m4AndUltramemSKUs() { + isUltraSKU := strings.Contains(sku.Description, "M4Ultramem224") + _, _, gotUltra, err := newRateMatcher(ultra, "us-central1", 1).classify(sku) + require.NoError(t, err) + assert.Equal(t, isUltraSKU, gotUltra, "ultramem shape vs %q", sku.Description) + _, _, gotPlain, err := newRateMatcher(plain, "us-central1", 1).classify(sku) + require.NoError(t, err) + assert.Equal(t, !isUltraSKU, gotPlain, "plain m4 shape vs %q", sku.Description) + } +} + +func TestFindFamilyRates_M4UltramemUsesOwnRows(t *testing.T) { + t.Parallel() + skus := m4AndUltramemSKUs() + ultra, err := findFamilyRates(skus, "m4-ultramem-224", "us-central1", 1) + require.NoError(t, err) + assert.InDelta(t, 0.008, ultra.onDemand.cpu, 1e-9) + assert.InDelta(t, 0.005, ultra.commitment.cpu, 1e-9) + + plain, err := findFamilyRates(skus, "m4-megamem-28", "us-central1", 1) + require.NoError(t, err) + assert.InDelta(t, 0.004, plain.onDemand.cpu, 1e-9) + assert.InDelta(t, 0.0025, plain.commitment.cpu, 1e-9) + + _, err = findFamilyRates(skus, "m4-ultramem-999", "us-central1", 1) + require.ErrorIs(t, err, errUnknownCatalogFamily) +} + +func TestFamilyLabels_NonM4UltramemKeepOverrides(t *testing.T) { + t.Parallel() + cases := map[string]string{ + "m1-ultramem-40": "Memory-optimized", + "m2-ultramem-208": "Memory-optimized", + "m3-ultramem-32": "M3 Memory-optimized", + } + for mt, want := range cases { + labels, err := familyLabels(mt) + require.NoError(t, err, mt) + assert.Equal(t, want, labels.onDemand, mt) + assert.Equal(t, []string{want}, labels.commit, mt) + } +} + +func TestFindFamilyRates_M4UltramemGBHRamPrices(t *testing.T) { + t.Parallel() + skus := m4AndUltramemSKUs() + for _, sku := range skus { + if sku.SkuId == "mu-od-ram" || sku.SkuId == "mu-c1-ram" { + sku.PricingInfo[0].PricingExpression.UsageUnit = "GBy.h" + } + } + got, err := findFamilyRates(skus, "m4-ultramem-224", "us-central1", 1) + require.NoError(t, err) + assert.InDelta(t, 0.008, got.onDemand.cpu, 1e-9) + assert.InDelta(t, 0.005, got.commitment.cpu, 1e-9) + assert.InDelta(t, 0.00567588*gibPerGB, got.onDemand.ram, 1e-12) + assert.InDelta(t, 0.0035*gibPerGB, got.commitment.ram, 1e-12) +}