diff --git a/CHANGELOG.md b/CHANGELOG.md index 38b84ff0..272894f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -309,6 +309,10 @@ 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). +- 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/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= 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()) 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) {