Skip to content

fix(gcp): remove GroupCommitments, which merged machine families into one untyped commitment #446

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 1 commit into from
Oct 16, 2026
Merged

fix(gcp): remove GroupCommitments, which merged machine families into one untyped commitment #446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 0 additions & 75 deletions providers/gcp/services/computeengine/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
100 changes: 0 additions & 100 deletions providers/gcp/services/computeengine/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading