Skip to content

fix(gcp): read the CUD commitment Type from the recommender payload instead of deriving it from the machine family #60

Description

@cristim

Context

PR LeanerCloud/cloud-commitments-cli#1650 (issue LeanerCloud/cloud-commitments-cli#1538) fixed GCP CUDs being purchased as GENERAL_PURPOSE
regardless of the recommended machine family. It derives the commitment Type
from the machine-family segment of rec.ResourceType via a hand-maintained
machineFamilyCommitmentType map in
providers/gcp/services/computeengine/client.go.

That map is a derivation, and therefore a guess that has to be kept in sync
with GCP:

  • New families need a map entry or they hit the fail-loud path and their
    purchases are refused (already true today for m4, x4, t2a).
  • Families whose commitment type splits into size buckets not encoded in the
    machine-type name (MEMORY_OPTIMIZED_M4 vs _M4_6TB; the seven
    MEMORY_OPTIMIZED_X4_* buckets) are not derivable at all from the machine
    type, so they can never be supported this way.

What to do instead

The Commitment Recommender's own operation payload carries the commitment
resource it wants created, including its type field. GCP is telling us which
Commitment_Type to buy; we do not have to infer it.

Read the commitment type from the recommender operation value (the operation
whose path targets the commitment resource root), validate it against
computepb.Commitment_Type_value so an unknown member fails loud rather than
being passed through as a bare string, and use the machine-family map only as a
fallback for payloads that omit it.

This removes the guess entirely and makes the size-bucketed M4/X4 families
purchasable.

Why it was not done in LeanerCloud/cloud-commitments-cli#1650

buildInsertRequest receives a common.Recommendation, not the raw
recommenderpb.Recommendation. Carrying the commitment type from the converter
to the purchase path requires a new field on pkg/common.Recommendation (or on
common.ComputeDetails), which lives in the separate pkg Go module shared
by the AWS, Azure and GCP providers
, and which is persisted. That is a design
change touching all three providers plus storage, not a bug fix, and folding it
into a p0 money-path PR would have made that PR unreviewable.

Acceptance

  • The purchased Type comes from the recommender payload when present.
  • An unrecognized type value fails loud (no fallback to a guessed family type,
    no bare string literal on the wire).
  • The machine-family map remains as the documented fallback.
  • Regression coverage asserts on the Type carried by the
    InsertRegionCommitmentRequest handed to the SDK client, not on an
    intermediate struct.
  • At least one M4 or X4 payload, currently refused, becomes purchasable.

Related: LeanerCloud/cloud-commitments-cli#1538, LeanerCloud/cloud-commitments-cli#1650, #14, #22.

Findings from the 2026-09-02 codebase audit

Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.

A08b-029 (high)

Sibling defect on the read side of the same field, worth folding into whatever lands here. convertCommitment at providers/gcp/services/computeengine/client.go:524 sets com.ResourceType = commitment.Resources[0].Type, which the repo's own doc at :535-538 says is the VCPU/MEMORY/LOCAL_SSD enum, so every existing commitment reports ResourceType: "VCPU". Recommendations set ResourceType to a machine type via extractResourceTypeFromRecommendation (:1197-1222), so no existing commitment can ever match a recommendation by resource type and dedupe/coverage see zero overlap, re-recommending capacity the project already owns. commitment.Type (e.g. GENERAL_PURPOSE_N2), the field that does identify the covered family, is never read in the converter. If this issue lands a recommender-supplied Type, the same value should be carried here as the inverse of machineFamilyCommitmentType. Audit finding A08b-029.

Activity

  1. cristim commented on Oct 7, 2026

    @cristim
    MemberAuthor

    Claimed by worker 4 (shard rank 28). Implementing: read the CUD commitment Type from the recommender operation payload (validated against computepb.Commitment_Type_value, fail-loud on unknown), carried via a new field on common.Recommendation/ComputeDetails, with the machine-family map kept as documented fallback. Acceptance per the issue.

  2. cristim commented on Oct 7, 2026

    @cristim
    MemberAuthor

    Draft PR #275 adds only the shared ComputeDetails.GCPCommitmentType persistence prerequisite. Issue #60 remains open for reading and validating the recommender commitment type in the GCP provider and replacing machine-family derivation. No provider code or dependency pins change in this prerequisite. #275

  3. cristim commented on Oct 7, 2026

    @cristim
    MemberAuthor

    Shared persistence prerequisite PR #275 merged normally at 07d8b1f. The remote shared types and codec tests match the verified files exactly; pkg resolves as v0.0.0-20261007231611-07d8b1fb3e3c. Issue #60 remains open for the recommender payload/provider validation implementation, pending its plan review. No provider changes or module pins were made, and no live GCP scenario or purchase was performed. #275

  4. cristim commented on Oct 8, 2026

    @cristim
    MemberAuthor

    Issue #60 is closed by PR #281, merged normally as 5053efa7c1674cf7145eca4cc35a1c6cc2db3c3a, following shared prerequisite #275 and persisted-type consumer #280.

    Acceptance verification covers the public local path from root Recommender operations through conversion, persisted codec details, duplicate filtering, and the InsertRegionCommitmentRequest passed to the SDK boundary:

    • Supplied M4 and X4 commitment types override machine-family derivation and reach the captured Insert request with exact VCPU and MEMORY quantities.
    • Unknown, non-concrete, malformed and unsupported supplied types refuse with zero Insert; absence preserves the documented legacy machine-family fallback.
    • Direct values and codec-decoded pointers behave consistently. Matching existing commitment buckets suppress purchase, while different buckets remain eligible. Existing inventory carries commitment Type rather than its first VCPU or MEMORY resource kind, satisfying the related read-side audit requirement.
    • Independent original-parent overlays fail all twelve public regression cases. Wrong-Type and compound-mutation probes fail; restored final source passes.

    Fresh canonical fetch verifies the merged tree is byte-identical to reviewed 25519609df1bb97a1a6e91920b37fe57880a4264, including all five changed files. Exact CI lint, all-five-module build, full standalone GCP race-short suite, normal hooks and independent full-PR review passed. Both actual merge-commit workflows completed successfully, and all watchers are reaped.

    Evidence uses schema-based fixtures, local SDK/gRPC scenarios, codec roundtrips and mock typed requests. No captured production response, live GCP acceptance or cloud purchase was performed. Post-merge evidence records the actual CI runs and owner's CodeRabbit quota waiver. The retrospective review remains tracked when quota returns.

    Follow-up issues filed: none after the exit audit. Existing #87 and #107 retain separate AWS inventory and Redshift tagging work.

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