diff --git a/CHANGELOG.md b/CHANGELOG.md index 54390e21..4caacb67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -284,6 +284,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- **Breaking (source):** `managedredis.ReservationsDetailsPager` and `(*managedredis.Client).SetReservationsPager` are removed; tests inject reservation inventory with `SetInventoryFactories` instead (#699). +- **`azure` Redis:** `managedredis.GetExistingCommitments` now reads held reservations from the Reservations API (purchase date, quantity, location, state), so a Redis reservation bought in the last day reduces the matching recommendation instead of being invisible to the duplicate guard (#699). An incomplete live reservation row (missing SKU, location, quantity or purchase date, an unattributable scope, or an expiry falling today) now fails the lookup with an error instead of being skipped, as `azure` compute already does. - **`aws` RI recommendations:** a fractional Cost Explorer quantity now rounds down instead of to the nearest integer, so a purchase never exceeds CE's quantity. The upfront, recurring monthly cost, monthly savings and savings diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index 8d1b5700..e8081698 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -46,12 +46,6 @@ type RecommendationsPager interface { NextPage(ctx context.Context) (armconsumption.ReservationRecommendationsClientListResponse, error) } -// ReservationsDetailsPager interface for reservations details pager (enables mocking). -type ReservationsDetailsPager interface { - More() bool - NextPage(ctx context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) -} - // RedisCachesPager interface for Redis caches pager (enables mocking). type RedisCachesPager interface { More() bool @@ -65,7 +59,7 @@ type Client struct { cred azcore.TokenCredential httpClient HTTPClient recommendationsPager RecommendationsPager - reservationsPager ReservationsDetailsPager + inventoryFactories *reservations.InventoryFactories redisCachesPager RedisCachesPager subscriptionID string region string @@ -116,9 +110,18 @@ func (c *Client) SetRecommendationsPager(pager RecommendationsPager) { c.recommendationsPager = pager } -// SetReservationsPager sets the reservations pager (for testing). -func (c *Client) SetReservationsPager(pager ReservationsDetailsPager) { - c.reservationsPager = pager +// SetInventoryFactories injects reservation-inventory factories (for testing). +func (c *Client) SetInventoryFactories(f *reservations.InventoryFactories) { + c.inventoryFactories = f +} + +// factories returns the injected inventory factories, or the real SDK-backed +// ones when none were injected (production only calls NewClient). +func (c *Client) factories() reservations.InventoryFactories { + if c.inventoryFactories != nil { + return *c.inventoryFactories + } + return reservations.DefaultInventoryFactories(c.cred) } // SetRedisCachesPager sets the Redis caches pager (for testing). @@ -201,78 +204,30 @@ func (c *Client) appendPage(ctx context.Context, pricer *recommendations.Pricer, return recs, nil } -// GetExistingCommitments retrieves existing Azure Cache for Redis reserved capacity. +// GetExistingCommitments retrieves the Azure Cache for Redis reservations that +// apply to this subscription from the Reservations API, which carries the +// purchase date, quantity, location and lifecycle state the duplicate-purchase +// guard needs (issue #699). Like compute, any failure or incomplete live row +// is returned as an error rather than as an empty or partial inventory. func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { - commitments := make([]common.Commitment, 0) - - pager, err := c.reservationDetailsPager() + responses, err := reservations.ListAppliedReservations(ctx, c.subscriptionID, c.factories(), maxReservationsPages) if err != nil { - return nil, fmt.Errorf("failed to initialize reservations details pager: %w", err) + return nil, fmt.Errorf("managedredis: list applied reservations: %w", err) } - for pageIdx := 0; pager.More(); pageIdx++ { - if err := ctx.Err(); err != nil { - return nil, fmt.Errorf("context canceled during pagination: %w", err) - } - if pageIdx >= maxReservationsPages { - return nil, fmt.Errorf("managedredis: GetExistingCommitments pagination cap (%d pages) reached", maxReservationsPages) - } - page, err := slot.NextPage(ctx, pager) + now := time.Now() + commitments := make([]common.Commitment, 0, len(responses)) + for _, r := range responses { + commitment, err := reservations.StrictCommitment(r, c.subscriptionID, common.ServiceMemoryDB, + armreservations.ReservedResourceTypeRedisCache, reservations.StrictLabels{Service: "managedredis", Resource: "Redis"}, now) if err != nil { - return nil, fmt.Errorf("failed to fetch reservations details page: %w", err) + return nil, err } - for _, detail := range page.Value { - if cm := c.reservationDetailToCommitment(detail); cm != nil { - commitments = append(commitments, *cm) - } + if commitment != nil { + commitments = append(commitments, *commitment) } } - - // ReservationsDetails is a daily usage API: one row per reservation per - // usage day. Without this, a single reservation held for N days would - // surface as N commitments sharing one CommitmentID (issue #73). - return reservations.DedupeCommitmentsByID(commitments), nil -} - -// reservationDetailsPager returns the pager to use for reservation details, -// preferring the injected mock over a real SDK pager. -// The subscription-scope NewListPager is used so that all reservation orders -// are queried rather than a single hardcoded order ID. -func (c *Client) reservationDetailsPager() (ReservationsDetailsPager, error) { - if c.reservationsPager != nil { - return c.reservationsPager, nil - } - client, err := armconsumption.NewReservationsDetailsClient(c.cred, nil) - if err != nil { - return nil, err - } - scope := fmt.Sprintf("subscriptions/%s", c.subscriptionID) - return client.NewListPager(scope, &armconsumption.ReservationsDetailsClientListOptions{}), nil -} - -// reservationDetailToCommitment converts a single reservation detail to a Commitment. -// Returns nil when the detail should be skipped (nil properties, non-Redis SKU). -func (c *Client) reservationDetailToCommitment(detail *armconsumption.ReservationDetail) *common.Commitment { - if detail == nil || detail.Properties == nil { - return nil - } - props := detail.Properties - if props.SKUName == nil || !strings.Contains(strings.ToLower(*props.SKUName), "redis") { - return nil - } - cm := &common.Commitment{ - Provider: common.ProviderAzure, - Account: c.subscriptionID, - CommitmentType: common.CommitmentReservedInstance, - Service: common.ServiceMemoryDB, - Region: c.region, - State: common.CommitmentStateActive, - } - if props.ReservationID != nil { - cm.CommitmentID = *props.ReservationID - } - cm.ResourceType = *props.SKUName - return cm + return commitments, nil } // PurchaseCommitment purchases Azure Cache for Redis reserved capacity using the two-step diff --git a/providers/azure/services/managedredis/client_test.go b/providers/azure/services/managedredis/client_test.go index b5bff28a..d3039712 100644 --- a/providers/azure/services/managedredis/client_test.go +++ b/providers/azure/services/managedredis/client_test.go @@ -14,12 +14,14 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" ) // -- mock helpers -- @@ -39,23 +41,56 @@ func (m *mockRecommendationsPager) NextPage(_ context.Context) (armconsumption.R return p, nil } -type mockReservationsPager struct { - pages []armconsumption.ReservationsDetailsClientListResponse - index int - err error +// fakeAppliedLister returns a fixed set of reservation order IDs. +type fakeAppliedLister struct { + orderIDs []*string + err error } -func (m *mockReservationsPager) More() bool { return m.index < len(m.pages) } -func (m *mockReservationsPager) NextPage(_ context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) { - if m.err != nil { - return armconsumption.ReservationsDetailsClientListResponse{}, m.err +func (f *fakeAppliedLister) GetAppliedReservationList(context.Context, string, *armreservations.AzureReservationAPIClientGetAppliedReservationListOptions) (armreservations.AzureReservationAPIClientGetAppliedReservationListResponse, error) { + if f.err != nil { + return armreservations.AzureReservationAPIClientGetAppliedReservationListResponse{}, f.err } - if m.index >= len(m.pages) { - return armconsumption.ReservationsDetailsClientListResponse{}, errors.New("no more pages") + return armreservations.AzureReservationAPIClientGetAppliedReservationListResponse{ + AppliedReservations: armreservations.AppliedReservations{ + Properties: &armreservations.AppliedReservationsProperties{ + ReservationOrderIDs: &armreservations.AppliedReservationList{Value: f.orderIDs}, + }, + }, + }, nil +} + +// fakeOrderPager is a single-page reservations.OrderReservationsPager. +type fakeOrderPager struct { + rows []*armreservations.ReservationResponse + err error + done bool +} + +func (p *fakeOrderPager) More() bool { return !p.done } +func (p *fakeOrderPager) NextPage(context.Context) (armreservations.ReservationClientListResponse, error) { + p.done = true + if p.err != nil { + return armreservations.ReservationClientListResponse{}, p.err } - p := m.pages[m.index] - m.index++ - return p, nil + return armreservations.ReservationClientListResponse{ReservationList: armreservations.ReservationList{Value: p.rows}}, nil +} + +const testOrderPath = "/providers/Microsoft.Capacity/reservationOrders/order-1" + +// factoriesFor injects one reservation order whose pager is newPager(). +func factoriesFor(newPager func() reservations.OrderReservationsPager) *reservations.InventoryFactories { + return &reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { + return &fakeAppliedLister{orderIDs: []*string{mocks.StringPtr(testOrderPath)}}, nil + }, + NewOrderPager: func(string) reservations.OrderReservationsPager { return newPager() }, + } +} + +// factoriesRows injects one order holding rows (and err on the page fetch). +func factoriesRows(err error, rows ...*armreservations.ReservationResponse) *reservations.InventoryFactories { + return factoriesFor(func() reservations.OrderReservationsPager { return &fakeOrderPager{rows: rows, err: err} }) } type mockRedisPager struct { @@ -406,108 +441,75 @@ func TestGetRecommendations_MultiplePages(t *testing.T) { func TestGetExistingCommitments_Empty(t *testing.T) { t.Parallel() c := NewClient(nil, "sub", "eastus") - c.SetReservationsPager(&mockReservationsPager{}) + c.SetInventoryFactories(&reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { return &fakeAppliedLister{}, nil }, + NewOrderPager: func(string) reservations.OrderReservationsPager { return &fakeOrderPager{} }, + }) commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) assert.Empty(t, commitments) } -func TestGetExistingCommitments_RedisSKUIncluded(t *testing.T) { +func TestGetExistingCommitments_RedisRowConverted(t *testing.T) { t.Parallel() c := NewClient(nil, "sub", "eastus") - resID := "res-123" - sku := "redis-premium-p1" - c.SetReservationsPager(&mockReservationsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - {ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - {Properties: &armconsumption.ReservationDetailProperties{ReservationID: &resID, SKUName: &sku}}, - }, - }}, - }, - }) + c.SetInventoryFactories(factoriesRows(nil, redisRow("res-123", "Premium_P1", "westus2", 3))) commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) require.Len(t, commitments, 1) - assert.Equal(t, resID, commitments[0].CommitmentID) - assert.Equal(t, sku, commitments[0].ResourceType) + assert.Equal(t, testOrderPath+"/reservations/res-123", commitments[0].CommitmentID) + assert.Equal(t, "Premium_P1", commitments[0].ResourceType) + assert.Equal(t, "westus2", commitments[0].Region, "reservation location must win over the client region") + assert.Equal(t, 3, commitments[0].Count) + assert.Equal(t, common.CommitmentStateActive, commitments[0].State) assert.Equal(t, common.ServiceMemoryDB, commitments[0].Service) assert.Equal(t, common.ProviderAzure, commitments[0].Provider) } -// TestGetExistingCommitments_DedupesDailyUsageRows is the regression test -// for #73: armconsumption.ReservationsDetails is a daily usage API, one row -// per reservation per usage day. A pager returning three rows for the same -// ReservationID must surface as a single commitment. -func TestGetExistingCommitments_DedupesDailyUsageRows(t *testing.T) { +func TestGetExistingCommitments_NonRedisFiltered(t *testing.T) { t.Parallel() c := NewClient(nil, "sub", "eastus") - resID := "res-123" - sku := "redis-premium-p1" - dailyRow := &armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{ReservationID: &resID, SKUName: &sku}, - } - c.SetReservationsPager(&mockReservationsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - {ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{dailyRow, dailyRow, dailyRow}, - }}, - }, - }) + sqlRow := redisRow("res-1", "Standard_S0", "eastus", 1) + sqlType := armreservations.ReservedResourceTypeSQLDatabases + sqlRow.Properties.ReservedResourceType = &sqlType + c.SetInventoryFactories(factoriesRows(nil, sqlRow, redisRow("res-2", "Premium_P2", "eastus", 1))) commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) - require.Len(t, commitments, 1, "three daily rows for one reservation must collapse to one commitment (issue #73)") - assert.Equal(t, resID, commitments[0].CommitmentID) + require.Len(t, commitments, 1) + assert.Equal(t, testOrderPath+"/reservations/res-2", commitments[0].CommitmentID) } -func TestGetExistingCommitments_NonRedisFiltered(t *testing.T) { +// A row the guard cannot classify may be the reservation bought an hour ago, +// so it fails closed instead of being dropped. +func TestGetExistingCommitments_NilPropertiesFailsClosed(t *testing.T) { t.Parallel() c := NewClient(nil, "sub", "eastus") - sqlSKU := "sql-standard-s1" - redisSKU := "redis-premium-p2" - id1 := "res-1" - id2 := "res-2" - c.SetReservationsPager(&mockReservationsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - {ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - {Properties: &armconsumption.ReservationDetailProperties{ReservationID: &id1, SKUName: &sqlSKU}}, - {Properties: &armconsumption.ReservationDetailProperties{ReservationID: &id2, SKUName: &redisSKU}}, - }, - }}, - }, - }) - commitments, err := c.GetExistingCommitments(context.Background()) - require.NoError(t, err) - require.Len(t, commitments, 1) - assert.Equal(t, id2, commitments[0].CommitmentID) + c.SetInventoryFactories(factoriesRows(nil, &armreservations.ReservationResponse{})) + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err) } -func TestGetExistingCommitments_NilProperties(t *testing.T) { +func TestGetExistingCommitments_PagerError(t *testing.T) { t.Parallel() c := NewClient(nil, "sub", "eastus") - c.SetReservationsPager(&mockReservationsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - {ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{{Properties: nil}}, - }}, - }, - }) - commitments, err := c.GetExistingCommitments(context.Background()) - require.NoError(t, err) - assert.Empty(t, commitments) + c.SetInventoryFactories(factoriesRows(errors.New("api error"))) + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "api error") } -func TestGetExistingCommitments_PagerError(t *testing.T) { +func TestGetExistingCommitments_ListerError(t *testing.T) { t.Parallel() c := NewClient(nil, "sub", "eastus") - c.SetReservationsPager(&mockReservationsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{{}}, - err: errors.New("api error"), + c.SetInventoryFactories(&reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { + return &fakeAppliedLister{err: errors.New("boom")}, nil + }, + NewOrderPager: func(string) reservations.OrderReservationsPager { return &fakeOrderPager{} }, }) _, err := c.GetExistingCommitments(context.Background()) - require.Error(t, err) - assert.Contains(t, err.Error(), "failed to fetch reservations details page") + require.Error(t, err, "a failed inventory lookup must not masquerade as an empty list") + assert.Contains(t, err.Error(), "managedredis: list applied reservations") } // -- GetOfferingDetails -- @@ -1008,9 +1010,9 @@ func TestSetterMethods(t *testing.T) { c.SetRecommendationsPager(recPager) assert.Equal(t, recPager, c.recommendationsPager) - resPager := &mockReservationsPager{} - c.SetReservationsPager(resPager) - assert.Equal(t, resPager, c.reservationsPager) + factories := &reservations.InventoryFactories{} + c.SetInventoryFactories(factories) + assert.Equal(t, factories, c.inventoryFactories) redisPager := &mockRedisPager{} c.SetRedisCachesPager(redisPager) diff --git a/providers/azure/services/managedredis/held_reservation_dedupe_test.go b/providers/azure/services/managedredis/held_reservation_dedupe_test.go new file mode 100644 index 00000000..4a6a37f3 --- /dev/null +++ b/providers/azure/services/managedredis/held_reservation_dedupe_test.go @@ -0,0 +1,249 @@ +package managedredis + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/fake" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/recfilter" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/recommendations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" +) + +// Issue #699: a Redis reservation CUDly (or anyone) already holds must reduce +// the matching recommendation. Fixture dates are UTC calendar dates computed +// once, because the Reservations API returns date-only values. +var ( + fixtureToday = time.Now().UTC().Truncate(24 * time.Hour) + fixtureYesterday = fixtureToday.AddDate(0, 0, -1) + fixtureThreeDays = fixtureToday.AddDate(0, 0, -3) + fixtureFuture = fixtureToday.AddDate(1, 0, 0) + fixturePast = fixtureToday.AddDate(0, 0, -30) +) + +// redisRow is a complete live Redis reservation: Shared scope billed to the +// client's own subscription ("sub", the production purchase path), Succeeded, +// purchased today, expiring next year. +func redisRow(id, sku, location string, qty int32) *armreservations.ReservationResponse { + redis := armreservations.ReservedResourceTypeRedisCache + shared := armreservations.AppliedScopeTypeShared + succeeded := armreservations.ProvisioningStateSucceeded + purchased, expiry := fixtureToday, fixtureFuture + return &armreservations.ReservationResponse{ + ID: mocks.StringPtr(testOrderPath + "/reservations/" + id), + Location: mocks.StringPtr(location), + SKU: &armreservations.SKUName{Name: mocks.StringPtr(sku)}, + Properties: &armreservations.Properties{ + ReservedResourceType: &redis, + AppliedScopeType: &shared, + BillingScopeID: mocks.StringPtr("/subscriptions/sub"), + ProvisioningState: &succeeded, + Quantity: &qty, + PurchaseDate: &purchased, + ExpiryDate: &expiry, + }, + } +} + +func testPricer(unit float64) *recommendations.Pricer { + return recommendations.NewPricer("managedredis", func(context.Context, string, string, int) (recommendations.ReservationPrice, error) { + return recommendations.ReservationPrice{Total: unit, Currency: "USD"}, nil + }) +} + +func legacyRedisRec(term string) armconsumption.ReservationRecommendationClassification { + return mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), mocks.WithSKU("Premium_P1"), mocks.WithTerm(term), + mocks.WithQuantity(2), mocks.WithCosts(1000, 700, 300)) +} + +func modernRedisRec() armconsumption.ReservationRecommendationClassification { + return mocks.BuildModernReservationRecommendation( + mocks.WithModernRegion("eastus"), mocks.WithModernSKUName("Premium_P1"), + mocks.WithModernQuantity(2), mocks.WithModernCosts(1000, 700, 300)) +} + +// variantsOf returns what GetRecommendations emits for the page: an upfront +// and a monthly variant per recommendation, in input order. +func variantsOf(t *testing.T, c *Client, page ...armconsumption.ReservationRecommendationClassification) []common.Recommendation { + t.Helper() + recs, err := c.appendPage(context.Background(), testPricer(1000), nil, page) + require.NoError(t, err) + return recs +} + +func adjust(t *testing.T, c *Client, recs []common.Recommendation) (passed []common.Recommendation, err error) { + t.Helper() + passed, _, err = recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, c) + return passed, err +} + +// purchasedReservation buys rec through a capturing transport and builds the +// held reservation from the request body CUDly actually sent, so the test +// proves that what CUDly buys is what the dedupe matches. +func purchasedReservation(t *testing.T, rec common.Recommendation, qty int32) *armreservations.ReservationResponse { + t.Helper() + capture := &reservationstest.Capture{} + buyer := NewClientWithHTTP(&fake.TokenCredential{}, "sub", "eastus", capture) + _, err := buyer.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.NoError(t, err) + var body struct { + Location string `json:"location"` + SKU struct { + Name string `json:"name"` + } `json:"sku"` + } + require.NoError(t, json.Unmarshal(capture.BodyBytes, &body)) + require.Equal(t, rec.Region, body.Location, "the purchase must land in the region the recommendation is keyed on") + return redisRow("held", body.SKU.Name, body.Location, qty) +} + +func TestHeldReservation_RoundTripReducesRecommendation(t *testing.T) { + t.Parallel() + for name, raw := range map[string]armconsumption.ReservationRecommendationClassification{ + "legacy SKUProperties": legacyRedisRec("P1Y"), + "modern SKUName": modernRedisRec(), + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + recs := variantsOf(t, c, raw) + require.Len(t, recs, 2) + upfront := recs[0] + require.Equal(t, 2, upfront.Count) + require.Equal(t, "upfront", upfront.PaymentOption) + + c.SetInventoryFactories(factoriesRows(nil, purchasedReservation(t, upfront, 1))) + passed, err := adjust(t, c, []common.Recommendation{upfront}) + require.NoError(t, err) + require.Len(t, passed, 1) + assert.Equal(t, 1, passed[0].Count) + assert.InDelta(t, upfront.CommitmentCost/2, passed[0].CommitmentCost, 0.001, "money fields scale with the count") + }) + } +} + +func TestHeldReservation_FullyCoveredIsFiltered(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + upfront := variantsOf(t, c, legacyRedisRec("P1Y"))[0] + c.SetInventoryFactories(factoriesRows(nil, purchasedReservation(t, upfront, 2))) + passed, err := adjust(t, c, []common.Recommendation{upfront}) + require.NoError(t, err) + assert.Empty(t, passed) +} + +func TestHeldReservation_Table(t *testing.T) { + t.Parallel() + state := func(s armreservations.ProvisioningState) func(*armreservations.ReservationResponse) { + return func(r *armreservations.ReservationResponse) { r.Properties.ProvisioningState = &s } + } + purchased := func(d time.Time) func(*armreservations.ReservationResponse) { + return func(r *armreservations.ReservationResponse) { r.Properties.PurchaseDate = &d } + } + expiry := func(d time.Time) func(*armreservations.ReservationResponse) { + return func(r *armreservations.ReservationResponse) { r.Properties.ExpiryDate = &d } + } + cases := []struct { + name string + mutate func(*armreservations.ReservationResponse) + want int + wantErr string // non-empty: the lookup must fail with this text + }{ + {"shared, this subscription", func(*armreservations.ReservationResponse) {}, 1, ""}, + {"single scope, this subscription", func(r *armreservations.ReservationResponse) { + single := armreservations.AppliedScopeTypeSingle + r.Properties.AppliedScopeType = &single + r.Properties.AppliedScopes = []*string{mocks.StringPtr("/subscriptions/sub")} + }, 1, ""}, + {"creating", state(armreservations.ProvisioningStateCreating), 1, ""}, + {"pending billing", state(armreservations.ProvisioningStatePendingBilling), 1, ""}, + {"purchased yesterday", purchased(fixtureYesterday), 1, ""}, + {"shared, billed to another subscription", func(r *armreservations.ReservationResponse) { + r.Properties.BillingScopeID = mocks.StringPtr("/subscriptions/other") + }, 2, ""}, + {"canceled", state(armreservations.ProvisioningStateCancelled), 2, ""}, + {"expired state", state(armreservations.ProvisioningStateExpired), 2, ""}, + {"succeeded past expiry", expiry(fixturePast), 2, ""}, + {"other region", func(r *armreservations.ReservationResponse) { r.Location = mocks.StringPtr("westus") }, 2, ""}, + {"other sku", func(r *armreservations.ReservationResponse) { r.SKU.Name = mocks.StringPtr("Premium_P2") }, 2, ""}, + {"purchased three days ago", purchased(fixtureThreeDays), 2, ""}, + {"missing quantity", func(r *armreservations.ReservationResponse) { r.Properties.Quantity = nil }, 0, "quantity"}, + {"expires today", expiry(fixtureToday), 0, "expiryDate"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + upfront := variantsOf(t, c, legacyRedisRec("P1Y"))[0] + row := redisRow("held", "Premium_P1", "eastus", 1) + tc.mutate(row) + c.SetInventoryFactories(factoriesRows(nil, row)) + passed, err := adjust(t, c, []common.Recommendation{upfront}) + if tc.wantErr != "" { + require.ErrorContains(t, err, tc.wantErr) + return + } + require.NoError(t, err) + require.Len(t, passed, 1) + assert.Equal(t, tc.want, passed[0].Count) + }) + } +} + +// TestHeldReservation_VariantPin pins today's shared-budget behavior of the +// recent-purchase guard on GetRecommendations-shaped output. The held +// quantity is consumed by the first matching variant in input order, so the P1Y +// upfront variant takes it and every other variant is untouched; the P1Y rec +// comes before the P3Y rec in the input and the pin depends on that order. +// The key has no term or payment option, so this is a known over-buy that this +// test pins and does not endorse; tracked in go#703. +func TestHeldReservation_VariantPin(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + recs := variantsOf(t, c, legacyRedisRec("P1Y"), legacyRedisRec("P3Y")) + require.Len(t, recs, 4) + c.SetInventoryFactories(factoriesRows(nil, redisRow("held", "Premium_P1", "eastus", 1))) + + passed, err := adjust(t, c, recs) + require.NoError(t, err) + type variant struct{ term, payment string } + got := map[variant]int{} + for _, r := range passed { + got[variant{r.Term, r.PaymentOption}] = r.Count + } + assert.Equal(t, map[variant]int{ + {"1yr", "upfront"}: 1, {"1yr", "monthly"}: 2, + {"3yr", "upfront"}: 2, {"3yr", "monthly"}: 2, + }, got) +} + +func TestNewClient_DefaultInventoryFactories(t *testing.T) { + t.Parallel() + for name, c := range map[string]*Client{ + "NewClient": NewClient(nil, "sub", "eastus"), + "NewClientWithHTTP": NewClientWithHTTP(nil, "sub", "eastus", nil), + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + require.Nil(t, c.inventoryFactories, "production wiring injects nothing") + f := c.factories() + assert.NotNil(t, f.NewAppliedLister) + assert.NotNil(t, f.NewOrderPager) + }) + } + injected := &reservations.InventoryFactories{} + c := NewClient(nil, "sub", "eastus") + c.SetInventoryFactories(injected) + assert.Equal(t, *injected, c.factories()) +} diff --git a/providers/azure/services/managedredis/pagination_cap_test.go b/providers/azure/services/managedredis/pagination_cap_test.go index a5dde4c5..6346d5d5 100644 --- a/providers/azure/services/managedredis/pagination_cap_test.go +++ b/providers/azure/services/managedredis/pagination_cap_test.go @@ -4,9 +4,11 @@ import ( "context" "testing" - "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" ) // countingReservationsPager reports More() until limit pages were requested, @@ -16,16 +18,16 @@ type countingReservationsPager struct { } func (p *countingReservationsPager) More() bool { return p.calls < p.limit } -func (p *countingReservationsPager) NextPage(_ context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) { +func (p *countingReservationsPager) NextPage(_ context.Context) (armreservations.ReservationClientListResponse, error) { p.calls++ - return armconsumption.ReservationsDetailsClientListResponse{}, nil + return armreservations.ReservationClientListResponse{}, nil } func TestGetExistingCommitments_PageCapReturnsError(t *testing.T) { t.Parallel() pager := &countingReservationsPager{limit: 1000} client := NewClient(nil, "test-subscription", "eastus") - client.SetReservationsPager(pager) + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) _, err := client.GetExistingCommitments(context.Background()) require.Error(t, err) @@ -39,7 +41,7 @@ func TestGetExistingCommitments_CancelledContextReturnsContextError(t *testing.T cancel() pager := &countingReservationsPager{limit: 1000} client := NewClient(nil, "test-subscription", "eastus") - client.SetReservationsPager(pager) + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) _, err := client.GetExistingCommitments(ctx) require.ErrorIs(t, err, context.Canceled) @@ -50,14 +52,14 @@ func TestGetExistingCommitments_CapIsExact(t *testing.T) { t.Parallel() atCap := &countingReservationsPager{limit: maxReservationsPages} client := NewClient(nil, "test-subscription", "eastus") - client.SetReservationsPager(atCap) + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return atCap })) _, err := client.GetExistingCommitments(context.Background()) require.NoError(t, err, "exactly cap pages must succeed") assert.Equal(t, maxReservationsPages, atCap.calls) overCap := &countingReservationsPager{limit: maxReservationsPages + 1} client = NewClient(nil, "test-subscription", "eastus") - client.SetReservationsPager(overCap) + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return overCap })) _, err = client.GetExistingCommitments(context.Background()) require.Error(t, err) assert.Equal(t, maxReservationsPages, overCap.calls) @@ -70,12 +72,12 @@ type cancelOnSecondCallPager struct { } func (p *cancelOnSecondCallPager) More() bool { return true } -func (p *cancelOnSecondCallPager) NextPage(_ context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) { +func (p *cancelOnSecondCallPager) NextPage(_ context.Context) (armreservations.ReservationClientListResponse, error) { p.calls++ if p.calls == 2 { p.cancel() } - return armconsumption.ReservationsDetailsClientListResponse{}, nil + return armreservations.ReservationClientListResponse{}, nil } func TestGetExistingCommitments_CancelMidWalkReturnsContextError(t *testing.T) { @@ -84,7 +86,7 @@ func TestGetExistingCommitments_CancelMidWalkReturnsContextError(t *testing.T) { defer cancel() pager := &cancelOnSecondCallPager{cancel: cancel} client := NewClient(nil, "test-subscription", "eastus") - client.SetReservationsPager(pager) + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) _, err := client.GetExistingCommitments(ctx) require.ErrorIs(t, err, context.Canceled) diff --git a/providers/azure/services/managedredis/slot_wiring_test.go b/providers/azure/services/managedredis/slot_wiring_test.go index 7d8e3db0..28708f5c 100644 --- a/providers/azure/services/managedredis/slot_wiring_test.go +++ b/providers/azure/services/managedredis/slot_wiring_test.go @@ -9,8 +9,10 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/slot/slottest" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" ) // slotPager is a one-page fake pager whose NextPage reports whether the shared @@ -28,6 +30,21 @@ func (s *slotPager[R]) NextPage(context.Context) (R, error) { return z, nil } +// slotLister reports one order so the order pager is exercised too. +type slotLister struct{ p *slottest.Probe } + +func (l slotLister) GetAppliedReservationList(context.Context, string, *armreservations.AzureReservationAPIClientGetAppliedReservationListOptions) (armreservations.AzureReservationAPIClientGetAppliedReservationListResponse, error) { + l.p.Check() + order := "order-1" + return armreservations.AzureReservationAPIClientGetAppliedReservationListResponse{ + AppliedReservations: armreservations.AppliedReservations{ + Properties: &armreservations.AppliedReservationsProperties{ + ReservationOrderIDs: &armreservations.AppliedReservationList{Value: []*string{&order}}, + }, + }, + }, nil +} + type slotHTTP struct{ p *slottest.Probe } func (h slotHTTP) Do(*http.Request) (*http.Response, error) { @@ -40,8 +57,13 @@ func TestSlotHeldDuringCalls(t *testing.T) { recs := func(p *slottest.Probe) *slotPager[armconsumption.ReservationRecommendationsClientListResponse] { return &slotPager[armconsumption.ReservationRecommendationsClientListResponse]{p: p} } - resv := func(p *slottest.Probe) *slotPager[armconsumption.ReservationsDetailsClientListResponse] { - return &slotPager[armconsumption.ReservationsDetailsClientListResponse]{p: p} + resv := func(p *slottest.Probe) *reservations.InventoryFactories { + return &reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { return slotLister{p}, nil }, + NewOrderPager: func(string) reservations.OrderReservationsPager { + return &slotPager[armreservations.ReservationClientListResponse]{p: p} + }, + } } cases := []struct { name string @@ -51,7 +73,7 @@ func TestSlotHeldDuringCalls(t *testing.T) { _, _ = (&Client{recommendationsPager: recs(p)}).GetRecommendations(ctx, nil) }}, {"reservations", func(ctx context.Context, p *slottest.Probe) { - _, _ = (&Client{reservationsPager: resv(p)}).GetExistingCommitments(ctx) + _, _ = (&Client{inventoryFactories: resv(p)}).GetExistingCommitments(ctx) }}, {"valid resource types", func(ctx context.Context, p *slottest.Probe) { _, _ = (&Client{redisCachesPager: &slotPager[armredis.ClientListBySubscriptionResponse]{p: p}}).GetValidResourceTypes(ctx)