Skip to content

fix(gcp): GroupCommitments merges different machine families into one untyped commitment #61

Description

@cristim

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

  1. Delete it. It has no caller and buildInsertRequest already builds the
    real request. Simplest, and removes the hazard outright.
  2. 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions