fix(gcp): derive CUD commitment type from machine family - #1650
Conversation
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-<vCPUs>-<MB>" 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
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
|
Pushed
Not done, deliberately: reading the commitment's own Pre-fix failure output for the new commitment-only regression is in the PR description. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files ==='
git diff --name-only HEAD^ HEAD
echo '=== Diff stat ==='
git diff --stat HEAD^ HEAD
echo '=== Relevant symbol locations ==='
rg -n -C 3 \
'func (extractResourceTypeFromOperations|machineTypeFromResourcePath|commitmentTypeForMachineType|buildInsertRequest|GroupCommitments)\b|extractResourceTypeFromOperations\s*\(|buildInsertRequest\s*\(' \
providers/gcp 2>/dev/null || true
echo '=== Go file outlines in computeengine ==='
fd -e go . providers/gcp/services/computeengine -x ast-grep outline {} --items all --type function --match 'extractResourceTypeFromOperations|machineTypeFromResourcePath|commitmentTypeForMachineType|buildInsertRequest|GroupCommitments' --view expanded 2>/dev/null || true
echo '=== Updated patch ==='
git diff --find-renames HEAD^ HEAD -- providers/gcpLength of output: 34365 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Commitment mapping and request construction ==='
sed -n '40,220p' providers/gcp/services/computeengine/client.go
sed -n '720,840p' providers/gcp/services/computeengine/client.go
echo '=== Recommendation conversion and path extraction ==='
sed -n '1040,1255p' providers/gcp/services/computeengine/client.go
echo '=== All extractor and selector call sites ==='
rg -n -C 5 \
'\bextractResourceTypeFromRecommendation\s*\(|\bmachineTypeFromResourcePath\s*\(|\.ResourceType\b' \
providers/gcp --glob '*.go'
echo '=== Recommender operation fixtures and resource paths ==='
rg -n -C 5 \
'OperationGroups|machineTypes|commitments/|ResourceType:\s*"compute.googleapis.com/Commitment"' \
providers/gcp --glob '*_test.go'
echo '=== Purchase mocks and relevant regression-test range ==='
rg -n -C 4 \
'type MockCommitmentsService|func \(.*MockCommitmentsService.*Insert|insertReqs|lastInsertReq' \
providers/gcp/services/computeengine --glob '*_test.go'
sed -n '1580,1970p' providers/gcp/services/computeengine/client_test.goLength of output: 139120
✅ Action performedReview finished.
|
What was bought vs. what was requested
buildInsertRequestpinned the GCP commitmentTypeto the string literal"GENERAL_PURPOSE"on every purchase.GENERAL_PURPOSEis the N1-onlymember of
computepb.Commitment_Type(the SDK's own enum doc: "TypeGENERAL_PURPOSE specifies a commitment that applies only to eligible resources
of general purpose N1 machine series"), so requested and purchased diverged on
every non-N1 recommendation:
rec.ResourceType)Type(before)Type(after)n2-standard-16GENERAL_PURPOSEGENERAL_PURPOSE_N2c3-highcpu-22GENERAL_PURPOSECOMPUTE_OPTIMIZED_C3m3-ultramem-32GENERAL_PURPOSEMEMORY_OPTIMIZED_M3a3-megagpu-8gGENERAL_PURPOSEACCELERATOR_OPTIMIZED_A3_MEGACommitment.Typeselects which machine series the CUD discounts. An N1commitment 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 reported as realized savings. The recommended machine type was
already on
rec.ResourceTypebut was only read into theDescriptionstring.The fix
commitmentTypeForMachineTypederives theTypefrom the machine-familysegment of
rec.ResourceTypeand maps it onto the SDK's owncomputepb.Commitment_Typeconstants, so a wrong member is a compile errorrather than a wire-level surprise (repo rule against bare enum string
literals; cf. fix(providers/azure): use SDK enum constant for appliedScopeType in purchase bodies #1256, Sweep sibling enum-as-string literals in Azure purchase bodies (appliedScopeType, term) -- companion to ARCH-01 #1317, and the
"MEMORY_MB"incident in fix(gcp): converter never sets Count/SavingsPct/Cost — CUD purchase requests 0 vCPU; scorer drops all GCP recs #1022).highgpu/megagpu/ultragpu), whichis encoded in the machine-type name.
custom-<vCPUs>-<MB>names, which carry no family segment, map toGENERAL_PURPOSE; family-prefixed custom types (n2-custom-…,e2-custom-…) resolve on their own family.Why a silent default is unacceptable here
An unmappable family returns an explicit error naming the machine type and the
purchase is refused before any Insert reaches GCP. It does not fall back
to
GENERAL_PURPOSE. On a money path a silent default spends real money on acommitment that discounts nothing and cannot be cancelled — strictly worse than
refusing and surfacing the gap.
Deliberately unmapped (fail loud):
m4andx4, whose commitment types splitinto size buckets (
MEMORY_OPTIMIZED_M4vs_M4_6TB;_X4_480_6T,_X4_960_12T,_X4_1440_24T, …) that are not derivable from the machine-typename;
t2a; and any series GCP has not published aCommitment_Typefor.Also in this diff
The sibling
ResourceCommitmentVCPU/MEMORYwire values now come fromcomputepb.ResourceCommitmentenum constants instead of string literals,keeping the #1022
"MEMORY_MB"regression compiler-checked.termPlanalreadysourced
TWELVE_MONTH/THIRTY_SIX_MONTHfrom the SDK enum and fails loud, soit needed no change.
Regression coverage
Asserts on the
Typecarried by theInsertRegionCommitmentRequestactuallyhanded to the SDK client (captured by the mock), not on an intermediate struct
that could be right while the wire value is wrong:
TestComputeEngineClient_PurchaseCommitment_TypeMatchesMachineFamilyTestComputeEngineClient_PurchaseCommitment_UnmappableFamilyRefuses— assertsan error and that
insertReqsstays empty.TestComputeEngineClient_PurchaseCommitment_ResourceTypesUseSDKEnumsTestCommitmentTypeForMachineType_MapsFamilyToSDKEnum/_UnmappableFailsLoudConfirmed failing pre-fix. Reverting only the
Typederivation:n1-standard-4passing pre-fix is the control:GENERAL_PURPOSEwas alreadycorrect for N1, which is exactly why the bug was invisible.
Gates
providers/gcpis a separate Go module, so its commands were run from insideproviders/gcp/.go build ./...(root)go vet ./...(root)go build ./...(providers/gcp)go vet ./...(providers/gcp)go test ./...(providers/gcp)go test -race ./services/computeengine/gocyclo -over 10 providers/gcp/gofmt -l providers/gcp/services/computeengine/golangci-lint run ./...(v2.10.1, CI-pinned, wholeproviders/gcpmodule)golangci-lint run ./services/computeengine/(v2.10.1)The
golangci-lintexit 1 is pre-existing debt in this module, which is notcovered by CI (#1478).
Corrected measurement (an earlier revision of this body quoted 684, which was
the raw output line count -- each finding prints with ~3 lines of source
context -- not a finding count; scope was also unstated):
providers/gcp/...before this PRproviders/gcp/...after this PRproviders/gcp/services/computeengineafter this PRZero new findings: the finding sets with line numbers stripped diff to empty in
the new-findings direction. The two-finding drop is incidental -- both are
pre-existing
godot"comment should end in a period" findings on doc commentsthis PR rewrote. Baseline was produced by re-running the same pinned binary with
this diff reverted in place.
Closes #1538
Review follow-up (commit
f5e645c6f)Input side: the machine-type operation is now selected explicitly
extractResourceTypeFromOperationstook the last/segment of the firstoperation with a non-empty
Resource, with no check that the path named amachine type. A CUD recommendation's operations reference the commitment
resource itself, so a commitment-only payload set
rec.ResourceType = "cud-001"(the commitment name), which the newderivation then rejected as machine family
cud. The refusal was correct; thestated cause was invented.
Now: only a resource path containing
/machineTypes/yields a machine type.When none exists, the extractor returns an error naming the segment it looked
for and the resource paths it actually found, and
ResourceTypeis leftempty rather than filled from an unrelated path segment. The purchase then
refuses with
no machine type on the recommendation.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.
This surfaces a pre-existing bug; it does not create one
The commitment-name-as-machine-type defect is already live on
main, notintroduced here. The pre-fix run below emits, from the old extractor:
That is
enrichRecWithPricingcallinggetComputePricing(rec.ResourceType, ...)with the commitment name. The extractor has been returning a commitment name
as a machine type all along, and the pricing lookup has been silently failing on
it. Nothing consumed
ResourceTypestrictly enough to make that visible.So the change is not "a risky new refusal". It adds the first consumer that
cannot tolerate a non-machine-type value, which is what makes the pre-existing
data-shape bug observable, and then fixes the extractor that produced it.
Pre-fix proof, restoring only the old "first non-empty Resource, last segment"
extractor while keeping the new tests:
Accelerator / graphics / storage families now refuse
buildInsertRequestsends onlyVCPUandMEMORY. GCP requires anACCELERATORresource in anACCELERATOR_OPTIMIZED*/GRAPHICS_OPTIMIZED*commitment and a
LOCAL_SSDresource inSTORAGE_OPTIMIZED_Z3, so the firstrevision newly declared types whose required resources were not being sent.
Best case GCP rejects the insert; worst case it creates a commitment covering
the cheap vCPU/RAM and none of the GPUs or local SSDs that dominate the spend.
a2,a3,a4,g2,g4,z3now take the same "not derivable, do notguess" refusal as
m4/x4, with an error naming the missing resource kind.The
a3sub-series map is gone with them.f1/g1Legacy shared-core members of the N1 lineage, correct under
GENERAL_PURPOSEbefore this mapping existed. Mapped alongside
custom-*so the mapping does notturn a working purchase into a refusal.
GroupCommitmentsand one vacuous assertionGroupCommitmentsnow takesVCPU/MEMORYfrom the SDK enum. It has nonon-test caller today, but it is exactly where a future caller would
reintroduce the invalid
"MEMORY_MB"spelling of fix(gcp): converter never sets Count/SavingsPct/Cost — CUD purchase requests 0 vCPU; scorer drops all GCP recs #1022.TestComputeEngineClient_PurchaseCommitment_ResourceTypesUseSDKEnumscomparedan 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".Deliberately not done
The commitment recommender's operation value for path
/carries thecommitment's own
type, which would remove the family-mapping guess entirely.Carrying it to
buildInsertRequestrequires a new field onpkg/common.Recommendation-- a separate Go module shared by all threeproviders, with persistence implications. Out of scope here; filed as #1663.
Also filed: #1664 —
GroupCommitmentsgroups by account+region+term only,ignoring
ResourceType, and itsCommitmentRequesthas noTypefield at all,so a future caller would sum
n2andc3vCPUs into one untyped commitment. Nonon-test caller today, which is exactly when it is cheap to fix.
Gates after the follow-up
go build ./...(root)go vet ./...(root)go build ./...(providers/gcp)go vet ./...(providers/gcp)go test ./...(providers/gcp)go test -race ./services/computeengine/gocyclo -over 10 providers/gcp/gofmt -l providers/gcp/services/computeengine/golangci-lint run ./...(v2.10.1, providers/gcp)