diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index fa3d85900..ac4c6e5b0 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -56,6 +56,145 @@ 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. +// +// 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 +// (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. +// - 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. + "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, + + // NOTE: the accelerator- (a2/a3/a4), graphics- (g2/g4) and storage-optimized + // (z3) families are deliberately NOT here; see familyRequiredResource. +} + +// 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, +} + +// 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). +// +// 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: 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)") + } + + 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)", + family, machineType, required.String(), + computepb.ResourceCommitment_VCPU.String(), computepb.ResourceCommitment_MEMORY.String(), + required.String()) + } + + 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 { @@ -453,10 +592,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++ @@ -624,6 +765,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 +802,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), }, }, @@ -959,7 +1111,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) @@ -1013,32 +1175,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/" - for _, opGroup := range gcpRec.Content.OperationGroups { - if resourceType := extractResourceTypeFromOperations(opGroup.Operations); resourceType != "" { - rec.ResourceType = resourceType - return +// 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 + +// 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 b736ee4cc..64eab863b 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", }, @@ -1599,3 +1599,362 @@ 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}, + // 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"}, + {"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 { + 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") + }) + } +} + +// 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 +// 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}, + {"custom-4-8192", computepb.Commitment_GENERAL_PURPOSE}, + } + + 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", "a3-megagpu-8g", "z3-highmem-88"} { + 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 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") + + 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, "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-001", + "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) + } +}