From 9372ae632bcb60df2e20a35d5b643588c8a0f619 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 17:38:00 +0200 Subject: [PATCH] fix(gcp): remove GroupCommitments, which merged machine families into one untyped commitment GroupCommitments keyed requests on account, region and term only, and CommitmentRequest carried no commitment type, so it summed vCPU and memory from incompatible machine families into one request with no type to buy it under. It had no caller outside its own tests in this repo or in the platform, CLI and MCP consumers, so it is deleted with its two types rather than kept as an exported entry point to a wrong purchase. Closes #61 Co-Authored-By: Claude Opus 5.5 --- .../gcp/services/computeengine/client.go | 75 ------------- .../gcp/services/computeengine/client_test.go | 100 ------------------ 2 files changed, 175 deletions(-) diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 9d49812e..7aae6db9 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -600,81 +600,6 @@ func (c *Client) convertGCPCommitmentToCommon(commitment *computepb.Commitment) }, nil } -// ResourceCommitment represents a single resource within a GCP commitment. -type ResourceCommitment struct { - Amount int64 // number of vCPUs or memory in MB - Type string // GCP ResourceCommitment.Type enum: "VCPU" or "MEMORY" -} - -// CommitmentRequest represents a single GCP commitment to create. -type CommitmentRequest struct { - Name string // unique per region+project - Plan string // "TWELVE_MONTH" or "THIRTY_SIX_MONTH" - Region string - Resources []ResourceCommitment -} - -// GroupCommitments groups recommendations by project+region+term into CommitmentRequests. -// GCP requires both a VCPU and a MEMORY resource (memory Amount in MB) in a -// single commitments.insert call. -// Each recommendation's Count is treated as vCPU count; the memory Amount is -// read from ComputeDetails.MemoryGB (populated by extractMemoryMBFromRecommendation). -// Recommendations with an unrecognized term or missing memory are skipped with a -// log warning so a bad rec never contaminates an otherwise valid group. -func GroupCommitments(recs []common.Recommendation) []CommitmentRequest { - type key struct{ account, region, term string } - type agg struct { - vcpus int64 - memoryMB int64 - plan string - } - groups := make(map[key]*agg) - - for i := range recs { - rec := &recs[i] - if rec.Service != common.ServiceCompute || rec.Provider != common.ProviderGCP { - continue - } - plan, err := termPlan(rec.Term) - if err != nil { - log.Printf("GroupCommitments: skipping recommendation with unrecognized term %q: %v", rec.Term, err) - continue - } - recMemMB, err := memoryMBFromDetails(*rec) - if err != nil { - log.Printf("GroupCommitments: skipping recommendation missing memory amount: %v", err) - continue - } - k := key{account: rec.Account, region: rec.Region, term: rec.Term} - if _, ok := groups[k]; !ok { - groups[k] = &agg{plan: plan} - } - groups[k].vcpus += int64(rec.Count) - groups[k].memoryMB += recMemMB - } - - result := make([]CommitmentRequest, 0, len(groups)) - ts := time.Now().UnixNano() - counter := 0 - for k, a := range groups { - result = append(result, CommitmentRequest{ - Name: fmt.Sprintf("cud-%s-%d-%d", k.region, ts, counter), - Plan: a.plan, - Region: k.region, - Resources: []ResourceCommitment{ - {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++ - } - return result -} - // isResourceExhausted reports whether the error represents a RESOURCE_EXHAUSTED // (quota / 429) response. Uses typed checks first: errors.As for REST API errors // (*googleapi.Error with Code 429) and status.FromError for gRPC errors diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index 3ac6bd9c..1fb69143 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -1466,106 +1466,6 @@ func TestConvertGCPRecommendation_ParamPaymentOptionRespected(t *testing.T) { assert.Equal(t, "monthly", rec.PaymentOption) } -// TestGroupCommitments_UsesValidMemoryEnum is a regression test for issue #1022: -// GroupCommitments must emit the canonical GCP ResourceCommitment.Type enum -// member "MEMORY" (Amount in MB), not the invalid "MEMORY_MB" string that GCP -// rejects on commitments.insert. Fails on the pre-fix code that emitted -// "MEMORY_MB". -// The memory amount is read from ComputeDetails.MemoryGB (set by the converter); -// recommendations without Details are skipped. -func TestGroupCommitments_UsesValidMemoryEnum(t *testing.T) { - // 4 vCPU, 24 GB memory (non-4096-ratio to confirm the value is read from - // Details, not computed from the old ratio). - const wantMemoryGB = 24.0 - const wantMemoryMB = int64(wantMemoryGB * 1024) - - recs := []common.Recommendation{ - { - Provider: common.ProviderGCP, - Service: common.ServiceCompute, - Account: "test-project", - Region: "us-central1", - Term: "1yr", - Count: 4, - Details: common.ComputeDetails{MemoryGB: wantMemoryGB}, - }, - } - - groups := GroupCommitments(recs) - require.Len(t, groups, 1, "one project+region+term group expected") - - var sawVCPU, sawMemory bool - for _, r := range groups[0].Resources { - switch r.Type { - case "VCPU": - sawVCPU = true - assert.Equal(t, int64(4), r.Amount, "VCPU amount must equal the summed Count") - case "MEMORY": - sawMemory = true - assert.Equal(t, wantMemoryMB, r.Amount, "MEMORY amount must equal the payload-sourced MB (issue #1022)") - default: - t.Fatalf("invalid ResourceCommitment.Type %q (issue #1022): must be VCPU/MEMORY/LOCAL_SSD/ACCELERATOR", r.Type) - } - } - assert.True(t, sawVCPU, "GroupCommitments must include a VCPU resource") - assert.True(t, sawMemory, "GroupCommitments must include a MEMORY resource (issue #1022)") -} - -// TestGroupCommitments_UsesValidMemoryEnum_PointerDetails is the -// GroupCommitments regression test for #71: recommendations loaded via the -// scheduler/web execution path carry Details as *ComputeDetails, not -// ComputeDetails. Pre-fix, memoryMBFromDetails only recognized the value -// form, so GroupCommitments silently `continue`d past every such -// recommendation and never grouped it. -func TestGroupCommitments_UsesValidMemoryEnum_PointerDetails(t *testing.T) { - const wantMemoryGB = 24.0 - const wantMemoryMB = int64(wantMemoryGB * 1024) - - recs := []common.Recommendation{ - { - Provider: common.ProviderGCP, - Service: common.ServiceCompute, - Account: "test-project", - Region: "us-central1", - Term: "1yr", - Count: 4, - Details: &common.ComputeDetails{MemoryGB: wantMemoryGB}, - }, - } - - groups := GroupCommitments(recs) - require.Len(t, groups, 1, "one project+region+term group expected; pre-fix this recommendation was silently skipped") - - var sawMemory bool - for _, r := range groups[0].Resources { - if r.Type == "MEMORY" { - sawMemory = true - assert.Equal(t, wantMemoryMB, r.Amount) - } - } - assert.True(t, sawMemory, "GroupCommitments must include a MEMORY resource for pointer-form Details") -} - -// TestGroupCommitments_SkipsRecsWithoutMemory verifies that GroupCommitments skips -// recommendations that have no Details.MemoryGB (payload omitted memory) rather -// than producing a zero-memory commitment that GCP would reject. -func TestGroupCommitments_SkipsRecsWithoutMemory(t *testing.T) { - recs := []common.Recommendation{ - { - Provider: common.ProviderGCP, - Service: common.ServiceCompute, - Account: "test-project", - Region: "us-central1", - Term: "1yr", - Count: 4, - // No Details: memory is absent from the payload. - }, - } - - groups := GroupCommitments(recs) - assert.Empty(t, groups, "GroupCommitments must skip recs without payload memory, not produce a zero-memory commitment") -} - // TestConvertGCPRecommendation_NonMonthlyPaymentOptionForcedToMonthly is a // regression test for the CR finding that non-monthly values (e.g. "upfront") // are silently propagated to the recommendation, which is invalid for GCP CUDs