fix(azure): dedupe Redis recommendations against held reservations

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

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

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)