Where
providers/gcp/services/computeengine/client.go — GroupCommitments and the
CommitmentRequest / ResourceCommitment types it returns.
What
GroupCommitments aggregates recommendations into commitment requests using
type key struct{ account, region, term string }
ResourceType is not part of the key, and CommitmentRequest has no
Type field at all. So the function will happily sum the vCPU and memory of
an n2-standard-16 recommendation and a c3-highcpu-22 recommendation into a
single request that names no commitment type.
That is the same defect class as LeanerCloud/cloud-commitments-cli#1538 (a CUD bought under a type that does not
discount the machines it was recommended for), except worse: LeanerCloud/cloud-commitments-cli#1538 bought one
wrong type, this would buy one commitment for two mutually-incompatible
families and carry no type to buy it under.
Why now, given it is dead code
GroupCommitments has no non-test caller anywhere in the repo (verified by
grep across all modules; only client_test.go references it). Nothing is
mis-purchasing today.
That is exactly the point. It is an exported function whose signature invites a
future caller to wire it into the purchase path, at which point the bug ships.
It is cheap to fix while nothing depends on it, and nobody fixes dead code once
it has a caller. PR LeanerCloud/cloud-commitments-cli#1650 already converted its {Type: "VCPU"} / {Type: "MEMORY"} bare string literals to the SDK enum for the same reason.
Options
- Delete it. It has no caller and
buildInsertRequest already builds the
real request. Simplest, and removes the hazard outright.
- Fix it: add
ResourceType (or the derived commitment type) to the
grouping key, add a Type field to CommitmentRequest populated via
commitmentTypeForMachineType, and fail loud on an unmappable family — the
same contract buildInsertRequest now has.
Option 1 is preferred unless a caller is actually planned. If it is kept, it
must not be possible to produce a CommitmentRequest without a commitment type.
Acceptance
- Either
GroupCommitments and its types are gone, or a request can only be
produced with a commitment Type derived from a single machine family.
- If kept: a test asserts that recommendations from two different machine
families do not merge into one request.
Related: LeanerCloud/cloud-commitments-cli#1538, LeanerCloud/cloud-commitments-cli#1650, LeanerCloud/cloud-commitments-cli#1022.
Where
providers/gcp/services/computeengine/client.go—GroupCommitmentsand theCommitmentRequest/ResourceCommitmenttypes it returns.What
GroupCommitmentsaggregates recommendations into commitment requests usingResourceTypeis not part of the key, andCommitmentRequesthas noTypefield at all. So the function will happily sum the vCPU and memory ofan
n2-standard-16recommendation and ac3-highcpu-22recommendation into asingle request that names no commitment type.
That is the same defect class as LeanerCloud/cloud-commitments-cli#1538 (a CUD bought under a type that does not
discount the machines it was recommended for), except worse: LeanerCloud/cloud-commitments-cli#1538 bought one
wrong type, this would buy one commitment for two mutually-incompatible
families and carry no type to buy it under.
Why now, given it is dead code
GroupCommitmentshas no non-test caller anywhere in the repo (verified bygrep across all modules; only
client_test.goreferences it). Nothing ismis-purchasing today.
That is exactly the point. It is an exported function whose signature invites a
future caller to wire it into the purchase path, at which point the bug ships.
It is cheap to fix while nothing depends on it, and nobody fixes dead code once
it has a caller. PR LeanerCloud/cloud-commitments-cli#1650 already converted its
{Type: "VCPU"}/{Type: "MEMORY"}bare string literals to the SDK enum for the same reason.Options
buildInsertRequestalready builds thereal request. Simplest, and removes the hazard outright.
ResourceType(or the derived commitment type) to thegrouping key, add a
Typefield toCommitmentRequestpopulated viacommitmentTypeForMachineType, and fail loud on an unmappable family — thesame contract
buildInsertRequestnow has.Option 1 is preferred unless a caller is actually planned. If it is kept, it
must not be possible to produce a
CommitmentRequestwithout a commitment type.Acceptance
GroupCommitmentsand its types are gone, or a request can only beproduced with a commitment
Typederived from a single machine family.families do not merge into one request.
Related: LeanerCloud/cloud-commitments-cli#1538, LeanerCloud/cloud-commitments-cli#1650, LeanerCloud/cloud-commitments-cli#1022.