From 2b0a6de87bb872f55aa94942cc4de3fdcdfd5106 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 13:01:55 +0100 Subject: [PATCH 1/3] build(aws): pin pkg to a52dfb9 for PurchaseResult.OfferingClass (#211) Re-pin providers/aws to the pkg commit on main that added PurchaseResult.OfferingClass (#697). go.mod changes: only the pkg pseudo-version moved, v0.0.0-20261106235222-20c9dacb335f => v0.0.0-20261108091534-a52dfb9143cc. No transitive require bumps; go mod tidy changed nothing else. Co-Authored-By: Claude Opus 5.5 --- providers/aws/go.mod | 2 +- providers/aws/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/providers/aws/go.mod b/providers/aws/go.mod index e56a301b..b697a578 100644 --- a/providers/aws/go.mod +++ b/providers/aws/go.mod @@ -3,7 +3,7 @@ module github.com/LeanerCloud/cloud-commitments-go/providers/aws go 1.26.10 require ( - github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261106235222-20c9dacb335f + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261108091534-a52dfb9143cc github.com/aws/aws-sdk-go-v2 v1.47.5 github.com/aws/aws-sdk-go-v2/config v1.33.13 github.com/aws/aws-sdk-go-v2/credentials v1.20.13 diff --git a/providers/aws/go.sum b/providers/aws/go.sum index 8809cfad..9096cb38 100644 --- a/providers/aws/go.sum +++ b/providers/aws/go.sum @@ -1,5 +1,5 @@ -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261106235222-20c9dacb335f h1:+GzNJalL4vfbk1Lwcu2jIGvJxc7DKScfR2N94+uGbXk= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261106235222-20c9dacb335f/go.mod h1:l9V+ecJCfAigYr6XiK9kpIUemF9bnXOKHJN1s4TPsM0= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261108091534-a52dfb9143cc h1:4Aw4TpF9X1Jw/eCQTvBp3bHmFhS8GIWdl5L48d6v3Zs= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261108091534-a52dfb9143cc/go.mod h1:l9V+ecJCfAigYr6XiK9kpIUemF9bnXOKHJN1s4TPsM0= github.com/aws/aws-sdk-go-v2 v1.47.5 h1:5yA+A+BiXbDq1ZLbYwPViwhEGDMdR0MEhL8Jh2LnLS4= github.com/aws/aws-sdk-go-v2 v1.47.5/go.mod h1:iNIkogrRvFwm+1OQIUOP2lPBtlXZboOIUWiCa+pHEMw= github.com/aws/aws-sdk-go-v2/config v1.33.13 h1:u52x6ISQOx172tpwM0VRxhJzEsTJYpMcEYUGM2A1wrw= From 4f9642d195a656423979297e80d88378a9d71adb Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 13:17:13 +0100 Subject: [PATCH 2/3] feat(aws): report the adopted RI's offering class on EC2 re-drives (#211) findRIByIdempotencyToken now returns the matching RI (nil when none), and the adoption branch sets PurchaseResult.OfferingClass from the RI itself instead of leaving the caller to assume the requested class. Cost stays nil on adoption; an empty class from AWS is logged. The fresh path is unchanged. Tests cover both class mismatches, the nil-ID skip, a nil-ID-only lookup taking the fresh path, and empty class plus non-nil Cost on a fresh buy. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 + providers/aws/services/ec2/client.go | 38 ++++++---- providers/aws/services/ec2/client_test.go | 73 +++++++++++++++++--- providers/aws/services/ec2/reconcile_test.go | 4 +- 4 files changed, 92 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 974b897c..35f063ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -309,6 +309,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). compiles; keyed literals are unaffected. On an adopted re-drive it carries the offering class AWS reports for the existing RI (the EC2 provider sets it in a follow-up change); empty otherwise (#211). +- **`aws`/EC2:** an adopted re-drive now reports the existing RI's offering + class in `PurchaseResult.OfferingClass` (#211). ### Fixed diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 2a1e09b9..bca12331 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -154,7 +154,7 @@ func (c *Client) purchase(ctx context.Context, rec common.Recommendation, count // already succeeded, so short-circuit and return the existing RI rather // than buying a second one. if opts.IdempotencyToken != "" { - existingID, found, lookupErr := c.findRIByIdempotencyToken(ctx, opts.IdempotencyToken) + existingRI, lookupErr := c.findRIByIdempotencyToken(ctx, opts.IdempotencyToken) if lookupErr != nil { // A failed lookup must NOT fall through to a purchase: doing so // would defeat the guard and risk a double-buy on a re-drive. Fail @@ -163,11 +163,8 @@ func (c *Client) purchase(ctx context.Context, rec common.Recommendation, count result.Error = fmt.Errorf("idempotency lookup failed before EC2 RI purchase (refusing to purchase to avoid a possible double-buy): %w", lookupErr) return result, result.Error } - if found { - log.Printf("EC2 RI for idempotency token %s already exists (%s); skipping purchase (issue #636 re-drive)", common.MaskToken(opts.IdempotencyToken), existingID) - result.Success = true - result.ExistingCommitment = true - result.CommitmentID = existingID + if existingRI != nil { + adoptExistingRI(&result, existingRI, opts.IdempotencyToken) return result, nil } } @@ -262,13 +259,30 @@ func checkApprovedUpfront(rec common.Recommendation, offering *types.ReservedIns return nil } +// adoptExistingRI records a re-driven purchase that found the RI an earlier +// attempt bought (issue #636). The offering class comes from the RI itself, not +// the request, so a caller never mislabels an adopted RI (issue #211). Cost +// stays nil: no charge was made by this call. +func adoptExistingRI(result *common.PurchaseResult, ri *types.ReservedInstances, token string) { + id := aws.ToString(ri.ReservedInstancesId) + log.Printf("EC2 RI for idempotency token %s already exists (%s); skipping purchase (issue #636 re-drive)", common.MaskToken(token), id) + result.Success = true + result.ExistingCommitment = true + result.CommitmentID = id + result.OfferingClass = string(ri.OfferingClass) + if result.OfferingClass == "" { + log.Printf("EC2 RI %s adopted for idempotency token %s reports no offering class", id, common.MaskToken(token)) + } +} + // findRIByIdempotencyToken looks for an active or payment-pending Reserved // Instance tagged with the given idempotency token (issue #636). It returns the -// RI ID and true when exactly such an RI exists, so a re-driven purchase can -// short-circuit instead of buying a second commitment. Retired/canceled RIs are +// first such RI, or nil when none exists, so a re-driven purchase can +// short-circuit instead of buying a second commitment and report the adopted +// RI's real offering class (issue #211). Retired/canceled RIs are // excluded (they carry the same state filter as GetExistingCommitments) so a // returned or expired commitment does not suppress a legitimate fresh purchase. -func (c *Client) findRIByIdempotencyToken(ctx context.Context, token string) (riID string, found bool, err error) { +func (c *Client) findRIByIdempotencyToken(ctx context.Context, token string) (*types.ReservedInstances, error) { input := &ec2.DescribeReservedInstancesInput{ Filters: []types.Filter{ { @@ -284,15 +298,15 @@ func (c *Client) findRIByIdempotencyToken(ctx context.Context, token string) (ri response, err := c.client.DescribeReservedInstances(ctx, input) if err != nil { - return "", false, fmt.Errorf("failed to describe reserved instances for idempotency check: %w", err) + return nil, fmt.Errorf("failed to describe reserved instances for idempotency check: %w", err) } for i := range response.ReservedInstances { ri := &response.ReservedInstances[i] if ri.ReservedInstancesId != nil { - return aws.ToString(ri.ReservedInstancesId), true, nil + return ri, nil } } - return "", false, nil + return nil, nil } // tagReservedInstance applies the standard CUDly tag set (including the diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 6099ae1e..3e35352d 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -1711,6 +1711,8 @@ func TestPurchaseCommitment_ExistingCommitmentFlag(t *testing.T) { assert.True(t, result.Success) assert.Equal(t, "ri-fresh-211", result.CommitmentID) assert.False(t, result.ExistingCommitment) + assert.Empty(t, result.OfferingClass) + assert.NotNil(t, result.Cost) m.AssertExpectations(t) }) @@ -1731,28 +1733,77 @@ func TestPurchaseCommitment_ExistingCommitmentFlag(t *testing.T) { assert.False(t, result.ExistingCommitment) }) - // The wrong-class incident: the RI an earlier attempt bought is Standard but - // the operator has since configured Convertible. The re-drive still adopts it - // (no offering lookup, no second purchase), and the flag is what lets the - // caller avoid recording the configured class for it. - t.Run("re-drive with a different configured class is flagged", func(t *testing.T) { + // The wrong-class incident: the RI an earlier attempt bought differs from the + // class now requested. The re-drive still adopts it (no offering lookup, no + // second purchase) and reports the RI's real class, not the requested one. + for _, tc := range []struct { + name string + riClass types.OfferingClassType + requested string + }{ + {"standard-ri-convertible-requested", types.OfferingClassTypeStandard, "convertible"}, + {"convertible-ri-standard-requested", types.OfferingClassTypeConvertible, "standard"}, + } { + t.Run("re-drive adopts the RI's class/"+tc.name, func(t *testing.T) { + t.Parallel() + m := &MockEC2Client{} + client := &Client{client: m, region: "us-east-1"} + m.On("DescribeReservedInstances", mock.Anything, tokenLookup). + Return(&ec2.DescribeReservedInstancesOutput{ReservedInstances: []types.ReservedInstances{ + {ReservedInstancesId: aws.String("ri-adopted-211"), OfferingClass: tc.riClass}, + }}, nil).Once() + + result, err := client.PurchaseCommitment(context.Background(), rec, + common.PurchaseOptions{IdempotencyToken: token, OfferingClass: tc.requested}) + require.NoError(t, err) + assert.True(t, result.Success) + assert.Equal(t, "ri-adopted-211", result.CommitmentID) + assert.True(t, result.ExistingCommitment) + assert.Equal(t, string(tc.riClass), result.OfferingClass) + assert.Nil(t, result.Cost) + m.AssertExpectations(t) + m.AssertNotCalled(t, "PurchaseReservedInstancesOffering", mock.Anything, mock.Anything) + }) + } + + t.Run("nil-ID entry is skipped", func(t *testing.T) { t.Parallel() m := &MockEC2Client{} client := &Client{client: m, region: "us-east-1"} m.On("DescribeReservedInstances", mock.Anything, tokenLookup). Return(&ec2.DescribeReservedInstancesOutput{ReservedInstances: []types.ReservedInstances{ - {ReservedInstancesId: aws.String("ri-standard-211"), OfferingClass: types.OfferingClassTypeStandard}, + {ReservedInstancesId: nil, OfferingClass: types.OfferingClassTypeStandard}, + {ReservedInstancesId: aws.String("ri-valid"), OfferingClass: types.OfferingClassTypeConvertible}, }}, nil).Once() - result, err := client.PurchaseCommitment(context.Background(), rec, - common.PurchaseOptions{IdempotencyToken: token, OfferingClass: "convertible"}) + result, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{IdempotencyToken: token}) require.NoError(t, err) - assert.True(t, result.Success) - assert.Equal(t, "ri-standard-211", result.CommitmentID) assert.True(t, result.ExistingCommitment) - m.AssertExpectations(t) + assert.Equal(t, "ri-valid", result.CommitmentID) + assert.Equal(t, "convertible", result.OfferingClass) m.AssertNotCalled(t, "PurchaseReservedInstancesOffering", mock.Anything, mock.Anything) }) + + t.Run("nil-ID-only lookup takes the fresh path", func(t *testing.T) { + t.Parallel() + m := &MockEC2Client{} + client := &Client{client: m, region: "us-east-1"} + m.On("DescribeReservedInstances", mock.Anything, tokenLookup). + Return(&ec2.DescribeReservedInstancesOutput{ReservedInstances: []types.ReservedInstances{ + {ReservedInstancesId: nil, OfferingClass: types.OfferingClassTypeStandard}, + }}, nil).Once() + m.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything).Return(offerings, nil) + m.On("PurchaseReservedInstancesOffering", mock.Anything, mock.Anything). + Return(&ec2.PurchaseReservedInstancesOfferingOutput{ReservedInstancesId: aws.String("ri-fresh-211")}, nil) + m.On("CreateTags", mock.Anything, mock.Anything).Return(&ec2.CreateTagsOutput{}, nil) + + result, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{IdempotencyToken: token}) + require.NoError(t, err) + assert.False(t, result.ExistingCommitment) + assert.Equal(t, "ri-fresh-211", result.CommitmentID) + assert.Empty(t, result.OfferingClass) + m.AssertExpectations(t) + }) } func zonalRec(az string) common.Recommendation { diff --git a/providers/aws/services/ec2/reconcile_test.go b/providers/aws/services/ec2/reconcile_test.go index da01df3c..bc223f6c 100644 --- a/providers/aws/services/ec2/reconcile_test.go +++ b/providers/aws/services/ec2/reconcile_test.go @@ -187,9 +187,9 @@ func TestReconcilePurchase_LostResponseUntaggedRI(t *testing.T) { _, err := c.PurchaseCommitment(context.Background(), req.Recommendation, req.Options) require.ErrorIs(t, err, common.ErrOutcomeUnknown) - _, found, err := c.findRIByIdempotencyToken(context.Background(), req.Options.IdempotencyToken) + found, err := c.findRIByIdempotencyToken(context.Background(), req.Options.IdempotencyToken) require.NoError(t, err) - assert.False(t, found, "the token lookup alone misses an untagged RI") + assert.Nil(t, found, "the token lookup alone misses an untagged RI") got := c.ReconcilePurchase(context.Background(), req) assert.True(t, got.IsFound()) From 14b6b37e3ac400542201739909ad0430c5318c42 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 13:18:01 +0100 Subject: [PATCH 3/3] fix(gcp): return a nil Cost on an adopted Compute Engine CUD (#211) An adopted re-drive made no charge in this call, so Cost is now nil (unknown) instead of a pointer to 0. A fresh purchase still reports a zero upfront cost, with or without an idempotency token. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 ++ providers/gcp/services/computeengine/client.go | 9 +++++++-- providers/gcp/services/computeengine/client_test.go | 6 ++++-- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35f063ad..8f18523b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -311,6 +311,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). follow-up change); empty otherwise (#211). - **`aws`/EC2:** an adopted re-drive now reports the existing RI's offering class in `PurchaseResult.OfferingClass` (#211). +- GCP Compute Engine adoption now returns a nil `Cost` instead of a pointer to 0; + a fresh purchase is unchanged (#211). ### Fixed diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index f03aae93..050cc3c8 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -768,8 +768,13 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati result.Success = true result.CommitmentID = commitmentName - upfrontCost := 0.0 // Compute Engine CUDs are billed monthly, with no upfront charge. - result.Cost = &upfrontCost + if !result.ExistingCommitment { + // Compute Engine CUDs are billed monthly, with no upfront charge. An + // adopted commitment made no charge in this call, so its Cost stays nil + // (unknown) rather than a claimed zero (issue #211). + upfrontCost := 0.0 + result.Cost = &upfrontCost + } return result, nil } diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index 12d09093..0e8e5271 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -926,8 +926,7 @@ func TestComputeEngineClient_PurchaseCommitment_ReDriveAlreadyExists(t *testing. assert.True(t, result.Success) assert.Equal(t, name, result.CommitmentID) assert.True(t, result.ExistingCommitment, "an adopted commitment must be flagged") - require.NotNil(t, result.Cost) - assert.Zero(t, *result.Cost) + assert.Nil(t, result.Cost, "an adopted commitment made no charge in this call") return } require.Error(t, err) @@ -971,6 +970,7 @@ func TestComputeEngineClient_PurchaseCommitment_ReDriveAlreadyExistsOnWait(t *te assert.True(t, result.Success) assert.Equal(t, name, result.CommitmentID) assert.True(t, result.ExistingCommitment) + assert.Nil(t, result.Cost) } // A first purchase that succeeds outright is not an adoption, even with a token. @@ -987,6 +987,8 @@ func TestComputeEngineClient_PurchaseCommitment_FreshPurchaseNotExisting(t *test require.NoError(t, err) assert.True(t, result.Success) assert.False(t, result.ExistingCommitment) + require.NotNil(t, result.Cost) + assert.Zero(t, *result.Cost) } func TestComputeEngineClient_PurchaseCommitment_3Year(t *testing.T) {