From 5d87431d2854b7595ae161ced790a91a32e9b284 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 28 Jul 2026 21:12:25 +0200 Subject: [PATCH 1/3] fix(gcp): derive CUD commitment type from machine family buildInsertRequest pinned the GCP commitment Type to the string literal "GENERAL_PURPOSE" for every purchase. GENERAL_PURPOSE is the N1-only member of computepb.Commitment_Type, so what the customer requested and what CUDly bought diverged on every non-N1 recommendation: requested n2-standard-16 -> bought GENERAL_PURPOSE (N1) requested c3-highcpu-22 -> bought GENERAL_PURPOSE (N1) requested m3-ultramem-32 -> bought GENERAL_PURPOSE (N1) requested a3-megagpu-8g -> bought GENERAL_PURPOSE (N1) Commitment.Type selects which machine series the discount applies to. An N1 commitment discounts no N2/C3/M3/A3 instance, so the customer paid the full 1- or 3-year commitment on top of undimmed on-demand charges while the purchase was booked as realized savings. The recommended machine type was already available on rec.ResourceType but was only read into the Description string. The Type is now derived from the machine-family segment of rec.ResourceType and mapped onto the SDK's own computepb.Commitment_Type constants, so a wrong member is a compile error rather than a wire-level surprise. Legacy N1 "custom--" names, which carry no family segment, map to GENERAL_PURPOSE; family-prefixed custom types resolve on their own family. An unmappable family returns an explicit error naming the machine type and refuses the purchase. It does not fall back to GENERAL_PURPOSE: on a money path a silent default spends real money on a commitment that discounts nothing and cannot be cancelled, which is strictly worse than refusing and surfacing the gap. This deliberately covers m4 and x4, whose commitment types split into size buckets not derivable from the machine type name, t2a, and any series GCP has not published a Commitment_Type for. The sibling ResourceCommitment VCPU/MEMORY wire values now also come from computepb.ResourceCommitment enum constants rather than string literals, keeping the issue #1022 "MEMORY_MB" regression compiler-checked. Regression coverage asserts on the Type carried by the InsertRegionCommitmentRequest handed to the SDK client, not on an intermediate struct, and asserts that an unmappable family produces an error with no Insert reaching GCP at all. Closes #1538 --- .../gcp/services/computeengine/client.go | 134 +++++++++++- .../gcp/services/computeengine/client_test.go | 196 ++++++++++++++++++ 2 files changed, 326 insertions(+), 4 deletions(-) diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index fa3d85900..60b36c301 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -56,6 +56,121 @@ func termPlan(term string) (string, error) { } } +// machineFamilyCommitmentType maps a GCP machine series (the segment of a +// machine type before the first "-") onto the computepb.Commitment_Type enum +// member whose discount actually applies to that series. +// +// The mapping is not cosmetic: computepb.Commitment.Type selects which machine +// series the CUD discounts, and the SDK's own enum doc states that +// "Type GENERAL_PURPOSE specifies a commitment that applies only to eligible +// resources of general purpose N1 machine series". An N1 commitment therefore +// buys nothing for an N2/C3/M3 fleet. +// +// Deliberately absent (they fall through to the fail-loud path in +// commitmentTypeForMachineType): +// - m4 and x4, whose commitment types are split into size-specific buckets +// (MEMORY_OPTIMIZED_M4 vs MEMORY_OPTIMIZED_M4_6TB; +// MEMORY_OPTIMIZED_X4_480_6T, _X4_960_12T, _X4_1440_24T, ...). The bucket +// is not derivable from the machine-type name alone, and guessing one +// reintroduces the exact mis-purchase this mapping exists to prevent. +// - t2a and any series GCP has not published a Commitment_Type for. +var machineFamilyCommitmentType = map[string]computepb.Commitment_Type{ + // General purpose. GENERAL_PURPOSE is the N1-only member. + "n1": computepb.Commitment_GENERAL_PURPOSE, + // Legacy custom machine types are named "custom--" with no family + // segment and are N1; every other family prefixes its own custom types + // ("n2-custom-...", "e2-custom-..."), which the family lookup already + // handles. Without this entry a valid N1-custom recommendation would be + // refused rather than committed under the type that actually discounts it. + "custom": computepb.Commitment_GENERAL_PURPOSE, + + "n2": computepb.Commitment_GENERAL_PURPOSE_N2, + "n2d": computepb.Commitment_GENERAL_PURPOSE_N2D, + "n4": computepb.Commitment_GENERAL_PURPOSE_N4, + "n4d": computepb.Commitment_GENERAL_PURPOSE_N4D, + "e2": computepb.Commitment_GENERAL_PURPOSE_E2, + "t2d": computepb.Commitment_GENERAL_PURPOSE_T2D, + "c4": computepb.Commitment_GENERAL_PURPOSE_C4, + "c4a": computepb.Commitment_GENERAL_PURPOSE_C4A, + "c4d": computepb.Commitment_GENERAL_PURPOSE_C4D, + + // Compute optimized. COMPUTE_OPTIMIZED is the C2-only member. + "c2": computepb.Commitment_COMPUTE_OPTIMIZED, + "c2d": computepb.Commitment_COMPUTE_OPTIMIZED_C2D, + "c3": computepb.Commitment_COMPUTE_OPTIMIZED_C3, + "c3d": computepb.Commitment_COMPUTE_OPTIMIZED_C3D, + "h3": computepb.Commitment_COMPUTE_OPTIMIZED_H3, + "h4d": computepb.Commitment_COMPUTE_OPTIMIZED_H4D, + + // Memory optimized. Per the SDK enum doc, MEMORY_OPTIMIZED covers the M1 + // and M2 series; M3 has its own member. + "m1": computepb.Commitment_MEMORY_OPTIMIZED, + "m2": computepb.Commitment_MEMORY_OPTIMIZED, + "m3": computepb.Commitment_MEMORY_OPTIMIZED_M3, + + // Accelerator optimized. ACCELERATOR_OPTIMIZED is the A2-only member; A3 is + // resolved by sub-series below. + "a2": computepb.Commitment_ACCELERATOR_OPTIMIZED, + "a4": computepb.Commitment_ACCELERATOR_OPTIMIZED_A4, + + // Graphics optimized. + "g2": computepb.Commitment_GRAPHICS_OPTIMIZED, + "g4": computepb.Commitment_GRAPHICS_OPTIMIZED_G4, + + // Storage optimized. + "z3": computepb.Commitment_STORAGE_OPTIMIZED_Z3, +} + +// a3SubFamilyCommitmentType resolves the A3 series, whose commitment buckets +// differ by GPU tier. Unlike M4/X4 the tier IS encoded in the machine-type name +// (a3-highgpu-8g vs a3-megagpu-8g vs a3-ultragpu-8g), so it is derivable. Any +// other A3 sub-series falls through to the fail-loud path rather than being +// folded into ACCELERATOR_OPTIMIZED_A3. +var a3SubFamilyCommitmentType = map[string]computepb.Commitment_Type{ + "highgpu": computepb.Commitment_ACCELERATOR_OPTIMIZED_A3, + "megagpu": computepb.Commitment_ACCELERATOR_OPTIMIZED_A3_MEGA, + "ultragpu": computepb.Commitment_ACCELERATOR_OPTIMIZED_A3_ULTRA, +} + +// commitmentTypeForMachineType derives the commitment Type from the recommended +// machine type (e.g. "n2-highmem-8" -> GENERAL_PURPOSE_N2). +// +// An unmappable family returns an error rather than defaulting: a commitment +// bought under the wrong Type does not discount the recommended instances at +// all, so the customer pays the full 1- or 3-year commitment on top of undimmed +// on-demand charges while the purchase is reported as realized savings (issue +// #1538). Refusing the purchase is the strictly cheaper failure. +func commitmentTypeForMachineType(machineType string) (computepb.Commitment_Type, error) { + normalized := strings.ToLower(strings.TrimSpace(machineType)) + if normalized == "" { + return computepb.Commitment_UNDEFINED_TYPE, errors.New( + "commitmentTypeForMachineType: empty machine type; cannot derive the GCP commitment type (issue #1538)") + } + + segments := strings.Split(normalized, "-") + family := segments[0] + + if family == "a3" { + if len(segments) < 2 { + return computepb.Commitment_UNDEFINED_TYPE, fmt.Errorf( + "commitmentTypeForMachineType: machine type %q has no A3 sub-series segment; cannot tell ACCELERATOR_OPTIMIZED_A3 from _A3_MEGA/_A3_ULTRA (issue #1538)", machineType) + } + commitType, ok := a3SubFamilyCommitmentType[segments[1]] + if !ok { + return computepb.Commitment_UNDEFINED_TYPE, fmt.Errorf( + "commitmentTypeForMachineType: unsupported A3 sub-series %q in machine type %q; refusing to purchase rather than guessing an ACCELERATOR_OPTIMIZED_A3* bucket (issue #1538)", segments[1], machineType) + } + return commitType, nil + } + + commitType, ok := machineFamilyCommitmentType[family] + if !ok { + return computepb.Commitment_UNDEFINED_TYPE, fmt.Errorf( + "commitmentTypeForMachineType: no GCP commitment type is known for machine family %q (machine type %q); refusing to purchase rather than defaulting to GENERAL_PURPOSE, which only discounts the N1 series (issue #1538)", family, machineType) + } + return commitType, nil +} + // termYearsFromTerm returns the commitment length in years for a term label, // defaulting to 1 for any 1-year or unrecognized value and 3 for 3-year labels. func termYearsFromTerm(term string) int { @@ -624,6 +739,16 @@ func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts return nil, "", fmt.Errorf("buildInsertRequest: %w", err) } + // The commitment Type selects which machine series the CUD discounts. It was + // pinned to GENERAL_PURPOSE (N1-only) while the recommended machine type was + // read into Description alone, so an N2/C3/M3/... recommendation bought a + // commitment that applied to none of its instances: full commitment spend + // plus undimmed on-demand charges, booked as realized savings (issue #1538). + commitType, err := commitmentTypeForMachineType(rec.ResourceType) + 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 // the API. Encode the source into Description so customers can still filter @@ -651,19 +776,20 @@ func (c *ComputeEngineClient) buildInsertRequest(rec common.Recommendation, opts commitment := &computepb.Commitment{ Name: stringPtr(commitmentName), Plan: stringPtr(plan), - Type: stringPtr("GENERAL_PURPOSE"), + Type: stringPtr(commitType.String()), Description: stringPtr(description), Resources: []*computepb.ResourceCommitment{ { - Type: stringPtr("VCPU"), + Type: stringPtr(computepb.ResourceCommitment_VCPU.String()), Amount: int64Ptr(int64(rec.Count)), }, { // 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"), + // the commitments.insert, failing the purchase (issue #1022). Taking + // the wire value from the SDK enum keeps that compiler-checked. + Type: stringPtr(computepb.ResourceCommitment_MEMORY.String()), Amount: int64Ptr(memMB), }, }, diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index b736ee4cc..7c6f187b6 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -1599,3 +1599,199 @@ func TestGetRecommendations_ActiveRecIncluded(t *testing.T) { require.NoError(t, err) require.Len(t, results, 1, "an ACTIVE recommendation must be included") } + +// TestCommitmentTypeForMachineType_MapsFamilyToSDKEnum is the issue #1538 +// regression at unit level: the commitment Type must follow the recommended +// machine series, not a fixed GENERAL_PURPOSE (which only discounts N1). +func TestCommitmentTypeForMachineType_MapsFamilyToSDKEnum(t *testing.T) { + cases := []struct { + machineType string + want computepb.Commitment_Type + }{ + {"n1-standard-1", computepb.Commitment_GENERAL_PURPOSE}, + // Legacy N1 custom machine types carry no family segment. + {"custom-4-8192", computepb.Commitment_GENERAL_PURPOSE}, + // Family-prefixed custom types resolve on their own family, not on N1. + {"n2-custom-8-16384", computepb.Commitment_GENERAL_PURPOSE_N2}, + {"e2-custom-4-8192", computepb.Commitment_GENERAL_PURPOSE_E2}, + {"n2-standard-16", computepb.Commitment_GENERAL_PURPOSE_N2}, + {"n2-highmem-8", computepb.Commitment_GENERAL_PURPOSE_N2}, + {"n2d-standard-4", computepb.Commitment_GENERAL_PURPOSE_N2D}, + {"n4-standard-8", computepb.Commitment_GENERAL_PURPOSE_N4}, + {"n4d-standard-8", computepb.Commitment_GENERAL_PURPOSE_N4D}, + {"e2-medium", computepb.Commitment_GENERAL_PURPOSE_E2}, + {"t2d-standard-8", computepb.Commitment_GENERAL_PURPOSE_T2D}, + {"c4-standard-8", computepb.Commitment_GENERAL_PURPOSE_C4}, + {"c4a-standard-8", computepb.Commitment_GENERAL_PURPOSE_C4A}, + {"c4d-standard-8", computepb.Commitment_GENERAL_PURPOSE_C4D}, + {"c2-standard-16", computepb.Commitment_COMPUTE_OPTIMIZED}, + {"c2d-standard-16", computepb.Commitment_COMPUTE_OPTIMIZED_C2D}, + {"c3-highcpu-22", computepb.Commitment_COMPUTE_OPTIMIZED_C3}, + {"c3d-standard-8", computepb.Commitment_COMPUTE_OPTIMIZED_C3D}, + {"h3-standard-88", computepb.Commitment_COMPUTE_OPTIMIZED_H3}, + {"h4d-highmem-192", computepb.Commitment_COMPUTE_OPTIMIZED_H4D}, + {"m1-ultramem-40", computepb.Commitment_MEMORY_OPTIMIZED}, + {"m2-megamem-416", computepb.Commitment_MEMORY_OPTIMIZED}, + {"m3-ultramem-32", computepb.Commitment_MEMORY_OPTIMIZED_M3}, + {"a2-highgpu-1g", computepb.Commitment_ACCELERATOR_OPTIMIZED}, + {"a3-highgpu-8g", computepb.Commitment_ACCELERATOR_OPTIMIZED_A3}, + {"a3-megagpu-8g", computepb.Commitment_ACCELERATOR_OPTIMIZED_A3_MEGA}, + {"a3-ultragpu-8g", computepb.Commitment_ACCELERATOR_OPTIMIZED_A3_ULTRA}, + {"a4-highgpu-8g", computepb.Commitment_ACCELERATOR_OPTIMIZED_A4}, + {"g2-standard-4", computepb.Commitment_GRAPHICS_OPTIMIZED}, + {"g4-standard-48", computepb.Commitment_GRAPHICS_OPTIMIZED_G4}, + {"z3-highmem-88", computepb.Commitment_STORAGE_OPTIMIZED_Z3}, + // Case and surrounding whitespace must not change the outcome. + {" N2-STANDARD-16 ", computepb.Commitment_GENERAL_PURPOSE_N2}, + } + + for _, tc := range cases { + t.Run(tc.machineType, func(t *testing.T) { + got, err := commitmentTypeForMachineType(tc.machineType) + require.NoError(t, err) + assert.Equal(t, tc.want, got, + "machine type %q must commit under %s, not a fixed GENERAL_PURPOSE (issue #1538)", + tc.machineType, tc.want.String()) + }) + } +} + +// TestCommitmentTypeForMachineType_UnmappableFailsLoud asserts the money-path +// rule: an unknown or size-bucket-ambiguous family must return an explicit +// error, never a GENERAL_PURPOSE default. +func TestCommitmentTypeForMachineType_UnmappableFailsLoud(t *testing.T) { + cases := []struct { + name string + machineType string + }{ + {"empty", ""}, + {"whitespace only", " "}, + {"unknown family", "zz9-standard-4"}, + {"arm t2a has no commitment type", "t2a-standard-8"}, + {"m4 splits into size buckets", "m4-megamem-28"}, + {"x4 splits into size buckets", "x4-megamem-960-metal"}, + {"unknown a3 sub-series", "a3-edgegpu-8g"}, + {"a3 with no sub-series segment", "a3"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := commitmentTypeForMachineType(tc.machineType) + require.Error(t, err, "unmappable machine type must fail loud, not default") + assert.Equal(t, computepb.Commitment_UNDEFINED_TYPE, got) + assert.NotEqual(t, computepb.Commitment_GENERAL_PURPOSE, got, + "must never fall back to GENERAL_PURPOSE (issue #1538)") + assert.Contains(t, err.Error(), "commitmentTypeForMachineType", + "error must name the failing step") + }) + } +} + +// TestComputeEngineClient_PurchaseCommitment_TypeMatchesMachineFamily is the +// issue #1538 wire-level regression. It asserts on the Type actually carried by +// the InsertRegionCommitmentRequest handed to the SDK client, so a fix that only +// corrects an intermediate value cannot make it pass. +func TestComputeEngineClient_PurchaseCommitment_TypeMatchesMachineFamily(t *testing.T) { + cases := []struct { + machineType string + wantType computepb.Commitment_Type + }{ + {"n1-standard-4", computepb.Commitment_GENERAL_PURPOSE}, + {"n2-standard-16", computepb.Commitment_GENERAL_PURPOSE_N2}, + {"n2-highmem-8", computepb.Commitment_GENERAL_PURPOSE_N2}, + {"c3-highcpu-22", computepb.Commitment_COMPUTE_OPTIMIZED_C3}, + {"m3-ultramem-32", computepb.Commitment_MEMORY_OPTIMIZED_M3}, + {"a3-megagpu-8g", computepb.Commitment_ACCELERATOR_OPTIMIZED_A3_MEGA}, + } + + for _, tc := range cases { + t.Run(tc.machineType, func(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + mockService := &MockCommitmentsService{operation: &MockOperation{err: nil}} + client.SetCommitmentsService(mockService) + t.Cleanup(func() { + assert.NotNil(t, mockService.lastInsertReq, + "the purchase must actually reach the SDK Insert call") + }) + + rec := common.Recommendation{ + ResourceType: tc.machineType, + Term: "1yr", + Count: 8, + Details: common.ComputeDetails{MemoryGB: 64.0}, + } + + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + require.NoError(t, err) + require.True(t, result.Success) + + require.NotNil(t, mockService.lastInsertReq) + require.NotNil(t, mockService.lastInsertReq.CommitmentResource) + gotType := mockService.lastInsertReq.CommitmentResource.GetType() + assert.Equal(t, tc.wantType.String(), gotType, + "%s must be committed as %s; buying %s instead discounts nothing (issue #1538)", + tc.machineType, tc.wantType.String(), gotType) + }) + } +} + +// TestComputeEngineClient_PurchaseCommitment_UnmappableFamilyRefuses asserts the +// purchase is refused before any Insert reaches GCP when the machine family +// cannot be mapped, rather than spending real money on a GENERAL_PURPOSE CUD. +func TestComputeEngineClient_PurchaseCommitment_UnmappableFamilyRefuses(t *testing.T) { + for _, machineType := range []string{"", "t2a-standard-8", "x4-megamem-960-metal"} { + t.Run(machineType, func(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + mockService := &MockCommitmentsService{operation: &MockOperation{err: nil}} + client.SetCommitmentsService(mockService) + t.Cleanup(func() { + assert.Empty(t, mockService.insertReqs, + "no Insert may reach GCP for an unmappable machine family (issue #1538)") + }) + + rec := common.Recommendation{ + ResourceType: machineType, + Term: "1yr", + Count: 8, + Details: common.ComputeDetails{MemoryGB: 64.0}, + } + + result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + require.Error(t, err, "an unmappable machine family must fail the purchase") + assert.False(t, result.Success) + assert.Contains(t, err.Error(), "buildInsertRequest") + assert.Nil(t, mockService.lastInsertReq) + }) + } +} + +// TestComputeEngineClient_PurchaseCommitment_ResourceTypesUseSDKEnums pins the +// VCPU/MEMORY wire values to the computepb.ResourceCommitment enum members. The +// "MEMORY_MB" spelling that issue #1022 fixed is an invalid enum value GCP +// rejects; sourcing both from the SDK keeps that regression compiler-checked. +func TestComputeEngineClient_PurchaseCommitment_ResourceTypesUseSDKEnums(t *testing.T) { + ctx := context.Background() + client, _ := NewClient(ctx, "test-project", "us-central1") + + mockService := &MockCommitmentsService{operation: &MockOperation{err: nil}} + client.SetCommitmentsService(mockService) + + rec := common.Recommendation{ + ResourceType: "n2-standard-8", + Term: "1yr", + Count: 8, + Details: common.ComputeDetails{MemoryGB: 32.0}, + } + + _, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) + require.NoError(t, err) + require.NotNil(t, mockService.lastInsertReq) + resources := mockService.lastInsertReq.CommitmentResource.GetResources() + require.Len(t, resources, 2) + assert.Equal(t, computepb.ResourceCommitment_VCPU.String(), resources[0].GetType()) + assert.Equal(t, computepb.ResourceCommitment_MEMORY.String(), resources[1].GetType()) +} From 5714a3dc532086beef2344f8d116b70a74e9e43a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 28 Jul 2026 21:41:32 +0200 Subject: [PATCH 2/3] fix(gcp): select the machine-type op; refuse accelerator CUDs Review follow-up on the #1538 commitment-type derivation. Input side. extractResourceTypeFromOperations took the last "/" segment of the FIRST operation carrying a non-empty Resource, with no check that the path named a machine type. A CUD recommendation's operations reference the commitment resource itself ("//compute.googleapis.com/projects/p/regions/r/commitments/cud-001"), so on a commitment-only payload rec.ResourceType became the commitment NAME "cud-001", which the new derivation would then reject as machine family "cud". The purchase refusal was correct but the stated cause was invented. The machine-type operation is now selected explicitly: only a resource path containing "/machineTypes/" yields a machine type. When no such operation exists the extractor returns an error naming the segment it looked for and the resource paths it actually found. ResourceType is left EMPTY rather than filled from an unrelated path segment, the recommendation stays visible, and the purchase refuses with "no machine type on the recommendation" instead of a fabricated family. The recommendation is not dropped: six existing tests depend on operation-less payloads still converting, and a listed-but-unpurchasable recommendation surfaces the problem where a silently discarded one would not. Accelerator, graphics and storage families. buildInsertRequest sends only VCPU and MEMORY resources. GCP requires an ACCELERATOR resource in an ACCELERATOR_OPTIMIZED*/GRAPHICS_OPTIMIZED* commitment and a LOCAL_SSD resource in STORAGE_OPTIMIZED_Z3, so mapping a2/a3/a4/g2/g4/z3 newly DECLARED types whose required resources were not sent. Best case GCP rejects the insert; worst case it creates a commitment covering the cheap vCPU and RAM and none of the GPUs or local SSDs that dominate the spend. Those families now take the same "not derivable, do not guess" refusal as m4/x4, with an error naming the missing resource kind. f1-micro and g1-small are legacy shared-core members of the N1 lineage and resolved correctly before this mapping existed; they are mapped to GENERAL_PURPOSE alongside custom-* so the mapping does not turn a working purchase into a refusal. GroupCommitments now takes its VCPU/MEMORY values from the SDK enum too. It has no non-test caller, but it is exactly where a future caller would reintroduce the invalid "MEMORY_MB" spelling of issue #1022. TestComputeEngineClient_PurchaseCommitment_ResourceTypesUseSDKEnums compared an SDK-derived expected against an SDK-derived actual, so it passed on the pre-fix literal-string code and pinned nothing. It now asserts the literal wire strings "VCPU" and "MEMORY". Reading the commitment's own type field out of the recommender operation value would remove the family-mapping guess entirely, but carrying it to buildInsertRequest requires a new field on pkg/common.Recommendation, a separate module shared by all three providers. Left out of this fix deliberately. Refs #1538 --- .../gcp/services/computeengine/client.go | 170 ++++++++++----- .../gcp/services/computeengine/client_test.go | 202 ++++++++++++++++-- 2 files changed, 294 insertions(+), 78 deletions(-) diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 60b36c301..5d49ccdbf 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -66,6 +66,11 @@ func termPlan(term string) (string, error) { // resources of general purpose N1 machine series". An N1 commitment therefore // buys nothing for an N2/C3/M3 fleet. // +// This map is restricted to families whose commitment is fully expressible with +// the VCPU + MEMORY resources buildInsertRequest sends. Families whose +// commitment additionally requires ACCELERATOR or LOCAL_SSD resources live in +// familyRequiredResource and are refused, not mapped. +// // Deliberately absent (they fall through to the fail-loud path in // commitmentTypeForMachineType): // - m4 and x4, whose commitment types are split into size-specific buckets @@ -83,6 +88,12 @@ var machineFamilyCommitmentType = map[string]computepb.Commitment_Type{ // handles. Without this entry a valid N1-custom recommendation would be // refused rather than committed under the type that actually discounts it. "custom": computepb.Commitment_GENERAL_PURPOSE, + // f1-micro and g1-small are the legacy shared-core members of the N1 + // lineage, so GENERAL_PURPOSE is their commitment type. Listed for the same + // reason as "custom": they resolved correctly before this mapping existed, + // and omitting them would turn a working purchase into a refusal. + "f1": computepb.Commitment_GENERAL_PURPOSE, + "g1": computepb.Commitment_GENERAL_PURPOSE, "n2": computepb.Commitment_GENERAL_PURPOSE_N2, "n2d": computepb.Commitment_GENERAL_PURPOSE_N2D, @@ -108,28 +119,29 @@ var machineFamilyCommitmentType = map[string]computepb.Commitment_Type{ "m2": computepb.Commitment_MEMORY_OPTIMIZED, "m3": computepb.Commitment_MEMORY_OPTIMIZED_M3, - // Accelerator optimized. ACCELERATOR_OPTIMIZED is the A2-only member; A3 is - // resolved by sub-series below. - "a2": computepb.Commitment_ACCELERATOR_OPTIMIZED, - "a4": computepb.Commitment_ACCELERATOR_OPTIMIZED_A4, - - // Graphics optimized. - "g2": computepb.Commitment_GRAPHICS_OPTIMIZED, - "g4": computepb.Commitment_GRAPHICS_OPTIMIZED_G4, - - // Storage optimized. - "z3": computepb.Commitment_STORAGE_OPTIMIZED_Z3, + // NOTE: the accelerator- (a2/a3/a4), graphics- (g2/g4) and storage-optimized + // (z3) families are deliberately NOT here; see familyRequiredResource. } -// a3SubFamilyCommitmentType resolves the A3 series, whose commitment buckets -// differ by GPU tier. Unlike M4/X4 the tier IS encoded in the machine-type name -// (a3-highgpu-8g vs a3-megagpu-8g vs a3-ultragpu-8g), so it is derivable. Any -// other A3 sub-series falls through to the fail-loud path rather than being -// folded into ACCELERATOR_OPTIMIZED_A3. -var a3SubFamilyCommitmentType = map[string]computepb.Commitment_Type{ - "highgpu": computepb.Commitment_ACCELERATOR_OPTIMIZED_A3, - "megagpu": computepb.Commitment_ACCELERATOR_OPTIMIZED_A3_MEGA, - "ultragpu": computepb.Commitment_ACCELERATOR_OPTIMIZED_A3_ULTRA, +// familyRequiredResource lists machine families whose commitment type exists in +// the SDK but whose commitment is not expressible with the VCPU + MEMORY +// resources buildInsertRequest currently sends: GCP requires an ACCELERATOR +// resource in an ACCELERATOR_OPTIMIZED*/GRAPHICS_OPTIMIZED* commitment and a +// LOCAL_SSD resource in STORAGE_OPTIMIZED_Z3. +// +// Declaring one of those types with only vCPU and RAM is worse than the bug +// this file fixes. The best case is that GCP rejects commitments.insert; the +// worst case is a commitment that covers the cheap vCPU/RAM and none of the +// GPUs or local SSDs, which are where the spend actually is. Until the extra +// resource amounts are threaded through the recommendation, these families take +// the same "not derivable, do not guess" refusal as m4/x4. +var familyRequiredResource = map[string]computepb.ResourceCommitment_Type{ + "a2": computepb.ResourceCommitment_ACCELERATOR, + "a3": computepb.ResourceCommitment_ACCELERATOR, + "a4": computepb.ResourceCommitment_ACCELERATOR, + "g2": computepb.ResourceCommitment_ACCELERATOR, + "g4": computepb.ResourceCommitment_ACCELERATOR, + "z3": computepb.ResourceCommitment_LOCAL_SSD, } // commitmentTypeForMachineType derives the commitment Type from the recommended @@ -144,23 +156,17 @@ func commitmentTypeForMachineType(machineType string) (computepb.Commitment_Type normalized := strings.ToLower(strings.TrimSpace(machineType)) if normalized == "" { return computepb.Commitment_UNDEFINED_TYPE, errors.New( - "commitmentTypeForMachineType: empty machine type; cannot derive the GCP commitment type (issue #1538)") + "commitmentTypeForMachineType: no machine type on the recommendation (no operation resource path contained \"/machineTypes/\"); cannot derive which machine series the commitment would discount, so the purchase is refused (issue #1538)") } - segments := strings.Split(normalized, "-") - family := segments[0] + family := strings.Split(normalized, "-")[0] - if family == "a3" { - if len(segments) < 2 { - return computepb.Commitment_UNDEFINED_TYPE, fmt.Errorf( - "commitmentTypeForMachineType: machine type %q has no A3 sub-series segment; cannot tell ACCELERATOR_OPTIMIZED_A3 from _A3_MEGA/_A3_ULTRA (issue #1538)", machineType) - } - commitType, ok := a3SubFamilyCommitmentType[segments[1]] - if !ok { - return computepb.Commitment_UNDEFINED_TYPE, fmt.Errorf( - "commitmentTypeForMachineType: unsupported A3 sub-series %q in machine type %q; refusing to purchase rather than guessing an ACCELERATOR_OPTIMIZED_A3* bucket (issue #1538)", segments[1], machineType) - } - return commitType, nil + if required, ok := familyRequiredResource[family]; ok { + return computepb.Commitment_UNDEFINED_TYPE, fmt.Errorf( + "commitmentTypeForMachineType: machine family %q (machine type %q) commits under a type that also requires a %s resource, which this client does not send (it sends only %s and %s); refusing rather than buying a commitment that would cover the vCPU and RAM but none of the %s capacity that dominates its cost (issue #1538)", + family, machineType, required.String(), + computepb.ResourceCommitment_VCPU.String(), computepb.ResourceCommitment_MEMORY.String(), + required.String()) } commitType, ok := machineFamilyCommitmentType[family] @@ -568,10 +574,12 @@ func GroupCommitments(recs []common.Recommendation) []CommitmentRequest { Plan: a.plan, Region: k.region, Resources: []ResourceCommitment{ - {Type: "VCPU", Amount: a.vcpus}, - // "MEMORY" is the GCP ResourceCommitment.Type enum member; the - // Amount is in MB (see buildInsertRequest, issue #1022). - {Type: "MEMORY", Amount: a.memoryMB}, + {Type: computepb.ResourceCommitment_VCPU.String(), Amount: a.vcpus}, + // MEMORY is the GCP ResourceCommitment.Type enum member; the + // Amount is in MB (see buildInsertRequest, issue #1022). Sourced + // from the SDK enum so a future caller of GroupCommitments cannot + // reintroduce the "MEMORY_MB" spelling GCP rejects. + {Type: computepb.ResourceCommitment_MEMORY.String(), Amount: a.memoryMB}, }, }) counter++ @@ -1085,7 +1093,17 @@ func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpR PaymentOption: paymentOption, } - extractResourceTypeFromRecommendation(gcpRec, rec) + // ResourceType is left EMPTY (not guessed, and not filled from some other + // operation's last path segment) when no machine-type operation is present. + // Everything downstream reads it as a machine type, and the purchase path + // refuses an empty one, so the recommendation stays visible while being + // unpurchasable rather than silently buying a commitment derived from a + // commitment name (issue #1538). + if err := extractResourceTypeFromRecommendation(gcpRec, rec); err != nil { + log.Printf("computeengine: recommendation %q has no machine type and cannot be purchased: %v", + gcpRec.GetName(), err) + } + extractCostImpactFromRecommendation(gcpRec, rec) extractVCPUCountFromRecommendation(gcpRec, rec) extractMemoryMBFromRecommendation(gcpRec, rec) @@ -1139,32 +1157,66 @@ func (c *ComputeEngineClient) enrichRecWithPricing(ctx context.Context, rec *com } } -// 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 { - return - } +// machineTypePathSegment precedes the machine type in a GCP resource path, e.g. +// "projects/p/zones/us-central1-a/machineTypes/n2-standard-16". +const machineTypePathSegment = "/machineTypes/" + +// maxReportedResourcePaths caps how many operation resource paths are quoted +// back in the "no machine type" error so a large operation group cannot produce +// an unbounded log line. +const maxReportedResourcePaths = 5 - for _, opGroup := range gcpRec.Content.OperationGroups { - if resourceType := extractResourceTypeFromOperations(opGroup.Operations); resourceType != "" { - rec.ResourceType = resourceType - return +// extractResourceTypeFromRecommendation sets rec.ResourceType to the machine +// type named by the recommendation's operations. +// +// It returns an error rather than leaving rec.ResourceType empty or guessing, +// because this value is what commitmentTypeForMachineType derives the purchased +// commitment Type from. A commitment recommendation's operations also reference +// the commitment resource itself ("//compute.googleapis.com/projects/p/regions/ +// r/commitments/cud-001"); taking the last path segment of whichever operation +// came first would yield the commitment NAME ("cud-001") and then be read as a +// machine family, so the machine-type operation is selected explicitly. +func extractResourceTypeFromRecommendation(gcpRec *recommenderpb.Recommendation, rec *common.Recommendation) error { + var seen []string + for _, opGroup := range gcpRec.GetContent().GetOperationGroups() { + for _, op := range opGroup.GetOperations() { + resource := op.GetResource() + if resource == "" { + continue + } + if machineType, ok := machineTypeFromResourcePath(resource); ok { + rec.ResourceType = machineType + return nil + } + if len(seen) < maxReportedResourcePaths { + seen = append(seen, resource) + } } } + + found := "none" + if len(seen) > 0 { + found = strings.Join(seen, ", ") + } + return fmt.Errorf( + "extractResourceTypeFromRecommendation: no machine type in recommendation operations (no operation resource path contains %q); resource paths found: %s; refusing to read a non-machine-type path segment as a machine family (issue #1538)", + machineTypePathSegment, found) } -// extractResourceTypeFromOperations extracts resource type from operation list -func extractResourceTypeFromOperations(operations []*recommenderpb.Operation) string { - for _, op := range operations { - if op.Resource != "" { - // Extract machine type from resource path - parts := strings.Split(op.Resource, "/") - if len(parts) > 0 { - return parts[len(parts)-1] - } - } +// machineTypeFromResourcePath returns the machine type named by a GCP resource +// path, and whether the path names one at all. Only a "/machineTypes/" +// path qualifies; anything else (notably a "/commitments/" path) is +// rejected so its last segment cannot be mistaken for a machine type. +func machineTypeFromResourcePath(resource string) (string, bool) { + idx := strings.LastIndex(resource, machineTypePathSegment) + if idx < 0 { + return "", false + } + machineType := strings.Trim(resource[idx+len(machineTypePathSegment):], "/") + if machineType == "" || strings.Contains(machineType, "/") { + return "", false } - return "" + return machineType, true } // extractCostImpactFromRecommendation extracts the cost impact from a GCP recommendation diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index 7c6f187b6..9d89ef887 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -955,7 +955,7 @@ func TestComputeEngineClient_GetRecommendations_PageCapFires(t *testing.T) { // 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. +// used by extractResourceTypeFromRecommendation 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 -- @@ -988,7 +988,7 @@ func realisticCUDRecommendation() *recommenderpb.Recommendation { Operations: []*recommenderpb.Operation{ { // Machine type op: resource path ends in the machine type name, - // so extractResourceTypeFromOperations sets rec.ResourceType = "n1-standard-4". + // so extractResourceTypeFromRecommendation sets rec.ResourceType = "n1-standard-4". Action: "add", Resource: "projects/test/zones/us-central1-a/machineTypes/n1-standard-4", }, @@ -1611,6 +1611,9 @@ func TestCommitmentTypeForMachineType_MapsFamilyToSDKEnum(t *testing.T) { {"n1-standard-1", computepb.Commitment_GENERAL_PURPOSE}, // Legacy N1 custom machine types carry no family segment. {"custom-4-8192", computepb.Commitment_GENERAL_PURPOSE}, + // Legacy shared-core members of the N1 lineage. + {"f1-micro", computepb.Commitment_GENERAL_PURPOSE}, + {"g1-small", computepb.Commitment_GENERAL_PURPOSE}, // Family-prefixed custom types resolve on their own family, not on N1. {"n2-custom-8-16384", computepb.Commitment_GENERAL_PURPOSE_N2}, {"e2-custom-4-8192", computepb.Commitment_GENERAL_PURPOSE_E2}, @@ -1633,14 +1636,6 @@ func TestCommitmentTypeForMachineType_MapsFamilyToSDKEnum(t *testing.T) { {"m1-ultramem-40", computepb.Commitment_MEMORY_OPTIMIZED}, {"m2-megamem-416", computepb.Commitment_MEMORY_OPTIMIZED}, {"m3-ultramem-32", computepb.Commitment_MEMORY_OPTIMIZED_M3}, - {"a2-highgpu-1g", computepb.Commitment_ACCELERATOR_OPTIMIZED}, - {"a3-highgpu-8g", computepb.Commitment_ACCELERATOR_OPTIMIZED_A3}, - {"a3-megagpu-8g", computepb.Commitment_ACCELERATOR_OPTIMIZED_A3_MEGA}, - {"a3-ultragpu-8g", computepb.Commitment_ACCELERATOR_OPTIMIZED_A3_ULTRA}, - {"a4-highgpu-8g", computepb.Commitment_ACCELERATOR_OPTIMIZED_A4}, - {"g2-standard-4", computepb.Commitment_GRAPHICS_OPTIMIZED}, - {"g4-standard-48", computepb.Commitment_GRAPHICS_OPTIMIZED_G4}, - {"z3-highmem-88", computepb.Commitment_STORAGE_OPTIMIZED_Z3}, // Case and surrounding whitespace must not change the outcome. {" N2-STANDARD-16 ", computepb.Commitment_GENERAL_PURPOSE_N2}, } @@ -1670,8 +1665,6 @@ func TestCommitmentTypeForMachineType_UnmappableFailsLoud(t *testing.T) { {"arm t2a has no commitment type", "t2a-standard-8"}, {"m4 splits into size buckets", "m4-megamem-28"}, {"x4 splits into size buckets", "x4-megamem-960-metal"}, - {"unknown a3 sub-series", "a3-edgegpu-8g"}, - {"a3 with no sub-series segment", "a3"}, } for _, tc := range cases { @@ -1687,6 +1680,40 @@ func TestCommitmentTypeForMachineType_UnmappableFailsLoud(t *testing.T) { } } +// TestCommitmentTypeForMachineType_FamiliesNeedingExtraResourcesRefuse covers +// the families whose Commitment_Type exists in the SDK but whose commitment +// also requires an ACCELERATOR or LOCAL_SSD resource. buildInsertRequest sends +// only VCPU and MEMORY, so declaring one of those types would either be +// rejected by GCP or -- worse -- buy a commitment covering the cheap vCPU/RAM +// and none of the GPU or local-SSD capacity that dominates its cost. +func TestCommitmentTypeForMachineType_FamiliesNeedingExtraResourcesRefuse(t *testing.T) { + cases := []struct { + machineType string + wantResource computepb.ResourceCommitment_Type + }{ + {"a2-highgpu-1g", computepb.ResourceCommitment_ACCELERATOR}, + {"a3-highgpu-8g", computepb.ResourceCommitment_ACCELERATOR}, + {"a3-megagpu-8g", computepb.ResourceCommitment_ACCELERATOR}, + {"a3-ultragpu-8g", computepb.ResourceCommitment_ACCELERATOR}, + {"a4-highgpu-8g", computepb.ResourceCommitment_ACCELERATOR}, + {"g2-standard-4", computepb.ResourceCommitment_ACCELERATOR}, + {"g4-standard-48", computepb.ResourceCommitment_ACCELERATOR}, + {"z3-highmem-88", computepb.ResourceCommitment_LOCAL_SSD}, + } + + for _, tc := range cases { + t.Run(tc.machineType, func(t *testing.T) { + got, err := commitmentTypeForMachineType(tc.machineType) + require.Error(t, err, + "%s needs a %s resource this client does not send; it must refuse, not declare the type anyway", + tc.machineType, tc.wantResource.String()) + assert.Equal(t, computepb.Commitment_UNDEFINED_TYPE, got) + assert.Contains(t, err.Error(), tc.wantResource.String(), + "error must name the resource kind that is missing") + }) + } +} + // TestComputeEngineClient_PurchaseCommitment_TypeMatchesMachineFamily is the // issue #1538 wire-level regression. It asserts on the Type actually carried by // the InsertRegionCommitmentRequest handed to the SDK client, so a fix that only @@ -1701,7 +1728,7 @@ func TestComputeEngineClient_PurchaseCommitment_TypeMatchesMachineFamily(t *test {"n2-highmem-8", computepb.Commitment_GENERAL_PURPOSE_N2}, {"c3-highcpu-22", computepb.Commitment_COMPUTE_OPTIMIZED_C3}, {"m3-ultramem-32", computepb.Commitment_MEMORY_OPTIMIZED_M3}, - {"a3-megagpu-8g", computepb.Commitment_ACCELERATOR_OPTIMIZED_A3_MEGA}, + {"custom-4-8192", computepb.Commitment_GENERAL_PURPOSE}, } for _, tc := range cases { @@ -1741,7 +1768,7 @@ func TestComputeEngineClient_PurchaseCommitment_TypeMatchesMachineFamily(t *test // purchase is refused before any Insert reaches GCP when the machine family // cannot be mapped, rather than spending real money on a GENERAL_PURPOSE CUD. func TestComputeEngineClient_PurchaseCommitment_UnmappableFamilyRefuses(t *testing.T) { - for _, machineType := range []string{"", "t2a-standard-8", "x4-megamem-960-metal"} { + for _, machineType := range []string{"", "t2a-standard-8", "x4-megamem-960-metal", "a3-megagpu-8g", "z3-highmem-88"} { t.Run(machineType, func(t *testing.T) { ctx := context.Background() client, _ := NewClient(ctx, "test-project", "us-central1") @@ -1770,9 +1797,13 @@ func TestComputeEngineClient_PurchaseCommitment_UnmappableFamilyRefuses(t *testi } // TestComputeEngineClient_PurchaseCommitment_ResourceTypesUseSDKEnums pins the -// VCPU/MEMORY wire values to the computepb.ResourceCommitment enum members. The -// "MEMORY_MB" spelling that issue #1022 fixed is an invalid enum value GCP -// rejects; sourcing both from the SDK keeps that regression compiler-checked. +// VCPU/MEMORY wire values that reach GCP. The "MEMORY_MB" spelling issue #1022 +// fixed is an invalid enum value GCP rejects on commitments.insert. +// +// The expectations are deliberately literal strings, not +// computepb.ResourceCommitment_*.String(): comparing an SDK-derived expected +// against an SDK-derived actual passes no matter which member the code picks, +// so it would pin nothing. The literals are what GCP actually accepts. func TestComputeEngineClient_PurchaseCommitment_ResourceTypesUseSDKEnums(t *testing.T) { ctx := context.Background() client, _ := NewClient(ctx, "test-project", "us-central1") @@ -1792,6 +1823,139 @@ func TestComputeEngineClient_PurchaseCommitment_ResourceTypesUseSDKEnums(t *test require.NotNil(t, mockService.lastInsertReq) resources := mockService.lastInsertReq.CommitmentResource.GetResources() require.Len(t, resources, 2) - assert.Equal(t, computepb.ResourceCommitment_VCPU.String(), resources[0].GetType()) - assert.Equal(t, computepb.ResourceCommitment_MEMORY.String(), resources[1].GetType()) + assert.Equal(t, "VCPU", resources[0].GetType()) + assert.Equal(t, "MEMORY", resources[1].GetType(), + "the memory member is MEMORY, not MEMORY_MB (issue #1022)") +} + +// commitmentOnlyCUDRecommendation builds a CUD Recommender payload whose +// operations reference ONLY the commitment resource -- no machineTypes +// operation anywhere. This is the shape the commitment recommender is expected +// to emit, and it is the case where taking "the last path segment of the first +// operation with a non-empty Resource" yields the commitment NAME ("cud-001"), +// which would then be read as machine family "cud". +func commitmentOnlyCUDRecommendation() *recommenderpb.Recommendation { + vcpuVal, _ := structpb.NewValue(4.0) + memVal, _ := structpb.NewValue(6144.0) + memTypeFilter, _ := structpb.NewValue("MEMORY") + const commitmentResource = "//compute.googleapis.com/projects/test/regions/us-central1/commitments/cud-001" + + return &recommenderpb.Recommendation{ + Name: "projects/test/locations/us-central1/recommenders/google.compute.commitment.UsageCommitmentRecommender/recommendations/rec-002", + PrimaryImpact: &recommenderpb.Impact{ + Category: recommenderpb.Impact_COST, + Projection: &recommenderpb.Impact_CostProjection{ + CostProjection: &recommenderpb.CostProjection{ + Cost: &money.Money{Units: -200, CurrencyCode: "USD"}, + }, + }, + }, + Content: &recommenderpb.RecommendationContent{ + OperationGroups: []*recommenderpb.OperationGroup{ + { + Operations: []*recommenderpb.Operation{ + { + Action: "add", + ResourceType: "compute.googleapis.com/Commitment", + Resource: commitmentResource, + Path: "/resources/0/amount", + PathValue: &recommenderpb.Operation_Value{Value: vcpuVal}, + }, + { + Action: "add", + ResourceType: "compute.googleapis.com/Commitment", + Resource: commitmentResource, + Path: "/resources/1/amount", + PathValue: &recommenderpb.Operation_Value{Value: memVal}, + PathFilters: map[string]*structpb.Value{ + "/resources/1/type": memTypeFilter, + }, + }, + }, + }, + }, + }, + } +} + +// TestExtractResourceTypeFromRecommendation_CommitmentOnlyOpsFailLoud is the +// guard for the input side of issue #1538. The previous extractor returned the +// last "/" segment of the first operation with a non-empty Resource, so a +// commitment-only payload yielded the commitment name "cud-001", which +// commitmentTypeForMachineType would then reject as machine family "cud" -- a +// misleading cause for a payload that simply carries no machine type. +func TestExtractResourceTypeFromRecommendation_CommitmentOnlyOpsFailLoud(t *testing.T) { + rec := &common.Recommendation{} + + err := extractResourceTypeFromRecommendation(commitmentOnlyCUDRecommendation(), rec) + + require.Error(t, err, "a payload with no machineTypes operation must fail loud") + assert.Contains(t, err.Error(), "no machine type in recommendation operations") + assert.Contains(t, err.Error(), "/machineTypes/", + "error must name the path segment that was looked for") + assert.Contains(t, err.Error(), "commitments/cud-001", + "error must name the resource paths that were actually found") + assert.Empty(t, rec.ResourceType, + "ResourceType must stay empty, not be filled with the commitment name") + assert.NotEqual(t, "cud-001", rec.ResourceType) +} + +// TestPurchaseCommitment_CommitmentOnlyRecommendationRefuses ties the input-side +// guard to the money path: a recommendation converted from a commitment-only +// payload must refuse the purchase with a cause naming the missing machine +// type, and no Insert may reach GCP. +func TestPurchaseCommitment_CommitmentOnlyRecommendationRefuses(t *testing.T) { + ctx := context.Background() + client, err := NewClient(ctx, "test-project", "us-central1") + require.NoError(t, err) + + converted := client.convertGCPRecommendation(ctx, commitmentOnlyCUDRecommendation(), common.RecommendationParams{Term: "1yr"}) + require.NotNil(t, converted, "the recommendation stays visible; only the purchase is refused") + require.Empty(t, converted.ResourceType, + "a commitment-only payload must not yield a machine type (issue #1538)") + + mockService := &MockCommitmentsService{operation: &MockOperation{err: nil}} + client.SetCommitmentsService(mockService) + t.Cleanup(func() { + assert.Empty(t, mockService.insertReqs, + "no Insert may reach GCP for a recommendation with no machine type (issue #1538)") + }) + + converted.Count = 4 + converted.Details = common.ComputeDetails{MemoryGB: 6.0} + + result, err := client.PurchaseCommitment(ctx, *converted, common.PurchaseOptions{}) + require.Error(t, err) + assert.False(t, result.Success) + assert.Contains(t, err.Error(), "no machine type on the recommendation", + "the cause must name the missing machine type, not an invented machine family") + assert.NotContains(t, err.Error(), "cud", + "the commitment name must never surface as a machine family") +} + +// TestMachineTypeFromResourcePath_SelectsOnlyMachineTypePaths pins the selector +// itself: only a /machineTypes/ path yields a machine type. +func TestMachineTypeFromResourcePath_SelectsOnlyMachineTypePaths(t *testing.T) { + accepted := map[string]string{ + "projects/test/zones/us-central1-a/machineTypes/n2-standard-16": "n2-standard-16", + "projects/test/machineTypes/n1-standard-1": "n1-standard-1", + "//compute.googleapis.com/projects/p/zones/z/machineTypes/c3-highcpu-22": "c3-highcpu-22", + } + for path, want := range accepted { + got, ok := machineTypeFromResourcePath(path) + assert.True(t, ok, "%q names a machine type", path) + assert.Equal(t, want, got) + } + + rejected := []string{ + "//compute.googleapis.com/projects/test/regions/us-central1/commitments/cud-001", + "projects/test/zones/us-central1-a/instances/vm-1", + "projects/test/zones/us-central1-a/machineTypes/", + "", + } + for _, path := range rejected { + got, ok := machineTypeFromResourcePath(path) + assert.False(t, ok, "%q does not name a machine type", path) + assert.Empty(t, got) + } } From 928baaf0961b835f7d304a7dd2615652d6e29f8a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 3 Aug 2026 13:41:50 +0200 Subject: [PATCH 3/3] fix(gcp): refuse CUD purchase for shared-core f1/g1 machine families CodeRabbit flagged the commitment-name guard in TestPurchaseCommitment_CommitmentOnlyRecommendationRefuses as asserting on the generic substring "cud" instead of the actual commitment name "cud-001", which a future error-text change containing the lowercase word "cud" could pass for the wrong reason. Assert on the exact name. Also drop f1 and g1 (f1-micro, g1-small) from machineFamilyCommitmentType. The prior comment justified mapping them to GENERAL_PURPOSE by claiming they "resolved correctly before this mapping existed" -- but GCP excludes shared-core machine types from committed use discounts entirely, so no commitment product exists for either family. Mapping them to GENERAL_PURPOSE would buy a real N1 commitment that discounts nothing, reintroducing the exact issue #1538 failure shape (pay the full multi-year commitment, discount nothing) inside the PR meant to eliminate it, and violating this file's own "not derivable, do not guess" principle. f1/g1 now take a dedicated fail-loud path (familyNoCommitmentAvailable) with an error naming the reason, and the two test cases that pinned GENERAL_PURPOSE for them now assert refusal instead. --- .../gcp/services/computeengine/client.go | 30 +++++++++++++++---- .../gcp/services/computeengine/client_test.go | 7 ++--- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 5d49ccdbf..ac4c6e5b0 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -78,6 +78,9 @@ func termPlan(term string) (string, error) { // MEMORY_OPTIMIZED_X4_480_6T, _X4_960_12T, _X4_1440_24T, ...). The bucket // is not derivable from the machine-type name alone, and guessing one // reintroduces the exact mis-purchase this mapping exists to prevent. +// - f1 and g1 (f1-micro, g1-small), N1's shared-core members. GCP sells no +// commitment product for shared-core machine types at all; see +// familyNoCommitmentAvailable. // - t2a and any series GCP has not published a Commitment_Type for. var machineFamilyCommitmentType = map[string]computepb.Commitment_Type{ // General purpose. GENERAL_PURPOSE is the N1-only member. @@ -88,12 +91,6 @@ var machineFamilyCommitmentType = map[string]computepb.Commitment_Type{ // handles. Without this entry a valid N1-custom recommendation would be // refused rather than committed under the type that actually discounts it. "custom": computepb.Commitment_GENERAL_PURPOSE, - // f1-micro and g1-small are the legacy shared-core members of the N1 - // lineage, so GENERAL_PURPOSE is their commitment type. Listed for the same - // reason as "custom": they resolved correctly before this mapping existed, - // and omitting them would turn a working purchase into a refusal. - "f1": computepb.Commitment_GENERAL_PURPOSE, - "g1": computepb.Commitment_GENERAL_PURPOSE, "n2": computepb.Commitment_GENERAL_PURPOSE_N2, "n2d": computepb.Commitment_GENERAL_PURPOSE_N2D, @@ -144,6 +141,22 @@ var familyRequiredResource = map[string]computepb.ResourceCommitment_Type{ "z3": computepb.ResourceCommitment_LOCAL_SSD, } +// familyNoCommitmentAvailable lists machine families GCP sells no commitment +// product for at all, so no Commitment_Type mapping -- correct or otherwise -- +// exists to guess. Per the Compute Engine CUD documentation, shared-core +// machine types are excluded from committed use discounts entirely: +// https://docs.cloud.google.com/compute/docs/instances/committed-use-discounts-overview +// f1-micro and g1-small are N1's shared-core members and fall under that +// exclusion, so they take this dedicated refusal rather than being folded +// into machineFamilyCommitmentType under GENERAL_PURPOSE: that mapping would +// buy a real N1 commitment that discounts nothing, since GCP has nothing to +// apply it to for these families -- the exact issue #1538 failure shape +// reintroduced by this file's own mapping. +var familyNoCommitmentAvailable = map[string]bool{ + "f1": true, + "g1": true, +} + // commitmentTypeForMachineType derives the commitment Type from the recommended // machine type (e.g. "n2-highmem-8" -> GENERAL_PURPOSE_N2). // @@ -161,6 +174,11 @@ func commitmentTypeForMachineType(machineType string) (computepb.Commitment_Type family := strings.Split(normalized, "-")[0] + if familyNoCommitmentAvailable[family] { + return computepb.Commitment_UNDEFINED_TYPE, fmt.Errorf( + "commitmentTypeForMachineType: GCP sells no commitment product for shared-core machine family %q (machine type %q); shared-core machine types are excluded from committed use discounts entirely, so there is no Commitment_Type to purchase under (issue #1538)", family, machineType) + } + if required, ok := familyRequiredResource[family]; ok { return computepb.Commitment_UNDEFINED_TYPE, fmt.Errorf( "commitmentTypeForMachineType: machine family %q (machine type %q) commits under a type that also requires a %s resource, which this client does not send (it sends only %s and %s); refusing rather than buying a commitment that would cover the vCPU and RAM but none of the %s capacity that dominates its cost (issue #1538)", diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index 9d89ef887..64eab863b 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -1611,9 +1611,6 @@ func TestCommitmentTypeForMachineType_MapsFamilyToSDKEnum(t *testing.T) { {"n1-standard-1", computepb.Commitment_GENERAL_PURPOSE}, // Legacy N1 custom machine types carry no family segment. {"custom-4-8192", computepb.Commitment_GENERAL_PURPOSE}, - // Legacy shared-core members of the N1 lineage. - {"f1-micro", computepb.Commitment_GENERAL_PURPOSE}, - {"g1-small", computepb.Commitment_GENERAL_PURPOSE}, // Family-prefixed custom types resolve on their own family, not on N1. {"n2-custom-8-16384", computepb.Commitment_GENERAL_PURPOSE_N2}, {"e2-custom-4-8192", computepb.Commitment_GENERAL_PURPOSE_E2}, @@ -1665,6 +1662,8 @@ func TestCommitmentTypeForMachineType_UnmappableFailsLoud(t *testing.T) { {"arm t2a has no commitment type", "t2a-standard-8"}, {"m4 splits into size buckets", "m4-megamem-28"}, {"x4 splits into size buckets", "x4-megamem-960-metal"}, + {"f1-micro is shared-core, excluded from all CUDs", "f1-micro"}, + {"g1-small is shared-core, excluded from all CUDs", "g1-small"}, } for _, tc := range cases { @@ -1929,7 +1928,7 @@ func TestPurchaseCommitment_CommitmentOnlyRecommendationRefuses(t *testing.T) { assert.False(t, result.Success) assert.Contains(t, err.Error(), "no machine type on the recommendation", "the cause must name the missing machine type, not an invented machine family") - assert.NotContains(t, err.Error(), "cud", + assert.NotContains(t, err.Error(), "cud-001", "the commitment name must never surface as a machine family") }