Repository navigation
fix(gcp): derive CUD commitment type from machine family - #1650
Conversation
cristim
added
effort/m
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 30 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)
📝 WalkthroughWalkthroughGCP Compute Engine commitment purchases now derive commitment types from recommended machine families. Recommendation parsing accepts only machine type paths. Unsupported families and recommendations without machine types fail before insertion. Resource types use Compute SDK enum values. ChangesGCP commitment type and machine resource validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Recommender
participant ComputeEngineClient
participant buildInsertRequest
participant ComputeEngineAPI
Recommender->>ComputeEngineClient: provide recommendation resource paths
ComputeEngineClient->>ComputeEngineClient: extract machine type
ComputeEngineClient->>buildInsertRequest: provide machine family and resources
buildInsertRequest->>buildInsertRequest: derive SDK commitment type
buildInsertRequest->>ComputeEngineAPI: insert validated commitment
ComputeEngineAPI-->>ComputeEngineClient: return purchase result
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
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.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
providers/gcp/services/computeengine/client.go (1)
59-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the family mapping into its own file.
client.gois well over the 500-line limit, and this change adds about 120 more lines. The mapping tables andcommitmentTypeForMachineTypeform a self-contained unit. Move them to a sibling file such asproviders/gcp/services/computeengine/commitment_type.goin the same package. No behavior changes.As per coding guidelines, "keep files under 500 lines".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/gcp/services/computeengine/client.go` around lines 59 - 145, Move machineFamilyCommitmentType, familyRequiredResource, and commitmentTypeForMachineType into a new sibling commitment_type.go file in the same package. Preserve their existing declarations, comments, visibility, and behavior without changing lookup or refusal logic; leave client.go focused on client functionality and under the 500-line limit.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/gcp/services/computeengine/client_test.go`:
- Around line 1932-1933: Update the assertion in the relevant test to check that
err.Error() does not contain the exact commitment name “cud-001” rather than the
generic substring “cud”, preserving the existing intent and failure message.
---
Nitpick comments:
In `@providers/gcp/services/computeengine/client.go`:
- Around line 59-145: Move machineFamilyCommitmentType, familyRequiredResource,
and commitmentTypeForMachineType into a new sibling commitment_type.go file in
the same package. Preserve their existing declarations, comments, visibility,
and behavior without changing lookup or refusal logic; leave client.go focused
on client functionality and under the 500-line limit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9646a39f-7312-46a1-8c50-511c6e64bd3e
📒 Files selected for processing (2)
providers/gcp/services/computeengine/client.goproviders/gcp/services/computeengine/client_test.go
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
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
CodeRabbit flagged the commitment-name guard in TestPurchaseCommitment_CommitmentOnlyRecommendationRefuses as asserting on the generic substring "cud" instead of the actual commitment name "cud-001", which a future error-text change containing the lowercase word "cud" could pass for the wrong reason. Assert on the exact name. Also drop f1 and g1 (f1-micro, g1-small) from machineFamilyCommitmentType. The prior comment justified mapping them to GENERAL_PURPOSE by claiming they "resolved correctly before this mapping existed" -- but GCP excludes shared-core machine types from committed use discounts entirely, so no commitment product exists for either family. Mapping them to GENERAL_PURPOSE would buy a real N1 commitment that discounts nothing, reintroducing the exact issue #1538 failure shape (pay the full multi-year commitment, discount nothing) inside the PR meant to eliminate it, and violating this file's own "not derivable, do not guess" principle. f1/g1 now take a dedicated fail-loud path (familyNoCommitmentAvailable) with an error naming the reason, and the two test cases that pinned GENERAL_PURPOSE for them now assert refusal instead.
a725cc7 to
928baaf
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 30 minutes. |
Adversarial review record (merged)Independent reviewer, distinct from the author. Recording it here because CodeRabbit's status on this PR read No silent fallback — the thing that most mattered. Exhaustive against the real SDK, not from memory. Enumerated all 39 Threaded to the wire, not computed-and-dropped — traced into the real The regression test genuinely bites. The pre-fix wiring was rebuilt in isolation and the tests FAIL with It also closes an old hole: sibling Finding fixed before merge
Verification of the lint claimgolangci-lint at the CI-pinned v2.10.1 (not the newer local build, which has a different bundled ruleset): 222 issues before and after, identical per-rule breakdown — zero new findings, and no gate-suppression used. Could not verify
|
1 participant
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 cloud-commitments-go#14, Sweep sibling enum-as-string literals in Azure purchase bodies (appliedScopeType, term) -- companion to ARCH-01 cloud-commitments-go#22, 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 LeanerCloud/cloud-commitments-go#60.
Also filed: LeanerCloud/cloud-commitments-go#61 —
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)Summary by CodeRabbit