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