From 417ecf014e676db86c5bdfb42071cb42bc587f54 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:04:11 +0100 Subject: [PATCH 01/21] refactor(azure): share strict reservation inventory listing Add reservations.StrictCommitments (list applied reservations, then StrictCommitment each row) and use it in compute and managedredis. Add reservationstest fixtures for live rows, the applied lister and the order pager. Co-Authored-By: Claude Opus 5.5 --- providers/azure/services/compute/client.go | 20 +---- .../reservationstest/inventory.go | 72 ++++++++++++++++++ .../services/internal/reservations/strict.go | 23 ++++++ .../reservations/strict_commitments_test.go | 76 +++++++++++++++++++ .../azure/services/managedredis/client.go | 24 ++---- 5 files changed, 180 insertions(+), 35 deletions(-) create mode 100644 providers/azure/services/internal/reservations/reservationstest/inventory.go create mode 100644 providers/azure/services/internal/reservations/strict_commitments_test.go diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index 78e290b9..932a8627 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -294,24 +294,8 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen factories = *c.inventoryFactories } - responses, err := reservations.ListAppliedReservations(ctx, c.subscriptionID, factories, maxReservationsPages) - if err != nil { - return nil, fmt.Errorf("compute: list applied reservations: %w", err) - } - - now := time.Now() - commitments := make([]common.Commitment, 0, len(responses)) - for _, r := range responses { - commitment, err := reservations.StrictCommitment(r, c.subscriptionID, common.ServiceCompute, - armreservations.ReservedResourceTypeVirtualMachines, reservations.StrictLabels{Service: "compute", Resource: "VM"}, now) - if err != nil { - return nil, err - } - if commitment != nil { - commitments = append(commitments, *commitment) - } - } - return commitments, nil + return reservations.StrictCommitments(ctx, c.subscriptionID, factories, maxReservationsPages, common.ServiceCompute, + armreservations.ReservedResourceTypeVirtualMachines, reservations.StrictLabels{Service: "compute", Resource: "VM"}) } // providerRegistrationState is the JSON shape returned by the ARM providers API. diff --git a/providers/azure/services/internal/reservations/reservationstest/inventory.go b/providers/azure/services/internal/reservations/reservationstest/inventory.go new file mode 100644 index 00000000..626ba26f --- /dev/null +++ b/providers/azure/services/internal/reservations/reservationstest/inventory.go @@ -0,0 +1,72 @@ +package reservationstest + +import ( + "context" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" +) + +// OrderPath is the reservation order ID the fakes below serve. +const OrderPath = "/providers/Microsoft.Capacity/reservationOrders/order-1" + +// LiveRow is a complete live reservation of resourceType: Shared scope billed +// to subscriptionID, Succeeded, purchased today and expiring next year. +func LiveRow(resourceType armreservations.ReservedResourceType, subscriptionID, id, sku, location string, qty int32) *armreservations.ReservationResponse { + shared := armreservations.AppliedScopeTypeShared + succeeded := armreservations.ProvisioningStateSucceeded + purchased := time.Now().UTC().Truncate(24 * time.Hour) + expiry := purchased.AddDate(1, 0, 0) + billing := "/subscriptions/" + subscriptionID + path := OrderPath + "/reservations/" + id + return &armreservations.ReservationResponse{ + ID: &path, + Location: &location, + SKU: &armreservations.SKUName{Name: &sku}, + Properties: &armreservations.Properties{ + ReservedResourceType: &resourceType, + AppliedScopeType: &shared, + BillingScopeID: &billing, + ProvisioningState: &succeeded, + Quantity: &qty, + PurchaseDate: &purchased, + ExpiryDate: &expiry, + }, + } +} + +// AppliedLister serves OrderPath as the only applied order, or Err. +type AppliedLister struct { + Err error +} + +func (l AppliedLister) GetAppliedReservationList(context.Context, string, *armreservations.AzureReservationAPIClientGetAppliedReservationListOptions) (armreservations.AzureReservationAPIClientGetAppliedReservationListResponse, error) { + if l.Err != nil { + return armreservations.AzureReservationAPIClientGetAppliedReservationListResponse{}, l.Err + } + order := OrderPath + return armreservations.AzureReservationAPIClientGetAppliedReservationListResponse{ + AppliedReservations: armreservations.AppliedReservations{ + Properties: &armreservations.AppliedReservationsProperties{ + ReservationOrderIDs: &armreservations.AppliedReservationList{Value: []*string{&order}}, + }, + }, + }, nil +} + +// OrderPager is a single-page pager over Rows, or Err on the page fetch. +type OrderPager struct { + Rows []*armreservations.ReservationResponse + Err error + done bool +} + +func (p *OrderPager) More() bool { return !p.done } + +func (p *OrderPager) NextPage(context.Context) (armreservations.ReservationClientListResponse, error) { + p.done = true + if p.Err != nil { + return armreservations.ReservationClientListResponse{}, p.Err + } + return armreservations.ReservationClientListResponse{ReservationList: armreservations.ReservationList{Value: p.Rows}}, nil +} diff --git a/providers/azure/services/internal/reservations/strict.go b/providers/azure/services/internal/reservations/strict.go index bb178b9f..d9ad349f 100644 --- a/providers/azure/services/internal/reservations/strict.go +++ b/providers/azure/services/internal/reservations/strict.go @@ -1,6 +1,7 @@ package reservations import ( + "context" "fmt" "log" "strings" @@ -19,6 +20,28 @@ type StrictLabels struct { Resource string } +// StrictCommitments lists the reservations applied to subscriptionID and +// converts each with StrictCommitment, so any lister failure or incomplete +// live row fails the whole inventory instead of yielding a partial one. +func StrictCommitments(ctx context.Context, subscriptionID string, f InventoryFactories, maxPages int, service common.ServiceType, wantType armreservations.ReservedResourceType, labels StrictLabels) ([]common.Commitment, error) { + responses, err := ListAppliedReservations(ctx, subscriptionID, f, maxPages) + if err != nil { + return nil, fmt.Errorf("%s: list applied reservations: %w", labels.Service, err) + } + now := time.Now() + commitments := make([]common.Commitment, 0, len(responses)) + for _, r := range responses { + commitment, err := StrictCommitment(r, subscriptionID, service, wantType, labels, now) + if err != nil { + return nil, err + } + if commitment != nil { + commitments = append(commitments, *commitment) + } + } + return commitments, nil +} + // StrictCommitment validates and converts one reservation, failing closed // instead of dropping or zero-counting incomplete inventory. It returns a nil // commitment for reservations of another type. diff --git a/providers/azure/services/internal/reservations/strict_commitments_test.go b/providers/azure/services/internal/reservations/strict_commitments_test.go new file mode 100644 index 00000000..ea0f3d04 --- /dev/null +++ b/providers/azure/services/internal/reservations/strict_commitments_test.go @@ -0,0 +1,76 @@ +package reservations_test + +import ( + "context" + "errors" + "testing" + + "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/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" +) + +const sub = "sub-a" + +var labels = reservations.StrictLabels{Service: "svc", Resource: "Redis"} + +func factories(listErr error, rows ...*armreservations.ReservationResponse) reservations.InventoryFactories { + return reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { + return reservationstest.AppliedLister{Err: listErr}, nil + }, + NewOrderPager: func(string) reservations.OrderReservationsPager { + return &reservationstest.OrderPager{Rows: rows} + }, + } +} + +func list(f reservations.InventoryFactories) ([]common.Commitment, error) { + return reservations.StrictCommitments(context.Background(), sub, f, 5, common.ServiceMemoryDB, + armreservations.ReservedResourceTypeRedisCache, labels) +} + +func TestStrictCommitments_ListErrorKeepsPrefixAndCause(t *testing.T) { + t.Parallel() + cause := errors.New("boom") + _, err := list(factories(cause)) + require.ErrorIs(t, err, cause) + assert.Contains(t, err.Error(), "svc: list applied reservations: ") +} + +func TestStrictCommitments_IgnoresOtherTypes(t *testing.T) { + t.Parallel() + got, err := list(factories(nil, + reservationstest.LiveRow(armreservations.ReservedResourceTypeSQLDatabases, sub, "sql", "S0", "eastus", 1), + reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, sub, "redis", "P1", "eastus", 1))) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, reservationstest.OrderPath+"/reservations/redis", got[0].CommitmentID) +} + +func TestStrictCommitments_IncompleteLiveRowFailsClosed(t *testing.T) { + t.Parallel() + row := reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, sub, "redis", "P1", "eastus", 1) + row.Properties.Quantity = nil + _, err := list(factories(nil, row)) + require.ErrorContains(t, err, "quantity") +} + +func TestStrictCommitments_ReturnsConvertedFields(t *testing.T) { + t.Parallel() + got, err := list(factories(nil, + reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, sub, "a", "P1", "westus2", 3), + reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, sub, "b", "P2", "eastus", 1))) + require.NoError(t, err) + require.Len(t, got, 2) + assert.Equal(t, "P1", got[0].ResourceType) + assert.Equal(t, "westus2", got[0].Region) + assert.Equal(t, 3, got[0].Count) + assert.Equal(t, common.ServiceMemoryDB, got[0].Service) + assert.Equal(t, common.CommitmentStateActive, got[0].State) + assert.Equal(t, "P2", got[1].ResourceType) +} diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index c6298322..545a0bce 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -204,25 +204,15 @@ func (c *Client) appendPage(ctx context.Context, pricer *recommendations.Pricer, // 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) { - responses, err := reservations.ListAppliedReservations(ctx, c.subscriptionID, c.factories(), maxReservationsPages) + commitments, err := reservations.StrictCommitments(ctx, c.subscriptionID, c.factories(), maxReservationsPages, common.ServiceMemoryDB, + armreservations.ReservedResourceTypeRedisCache, reservations.StrictLabels{Service: "managedredis", Resource: "Redis"}) if err != nil { - return nil, fmt.Errorf("managedredis: list applied reservations: %w", err) + return nil, err } - - 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, err - } - if commitment != nil { - // The dedupe key includes the engine and recommendations carry - // CacheDetails.Engine "redis", so held reservations must too. - commitment.Engine = Engine - commitments = append(commitments, *commitment) - } + // The dedupe key includes the engine and recommendations carry + // CacheDetails.Engine "redis", so held reservations must too. + for i := range commitments { + commitments[i].Engine = Engine } return commitments, nil } From 7e1911298bfce0f02cd9276cf987ae8de1a13275 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:13:24 +0100 Subject: [PATCH 02/21] fix(azure): read Cosmos DB held reservations from the Reservations API GetExistingCommitments now lists applied reservations through the shared StrictCommitments path instead of the Consumption daily-usage details, so the duplicate-purchase guard sees purchase date, quantity, location and state, and fails closed on incomplete rows. Removes the Consumption pager, collector and converter. Part of #705 Co-Authored-By: Claude Opus 5.5 --- providers/azure/services/cosmosdb/client.go | 114 ++------ .../azure/services/cosmosdb/client_test.go | 254 ++++++------------ .../services/cosmosdb/slot_wiring_test.go | 18 +- 3 files changed, 126 insertions(+), 260 deletions(-) diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index 66eb07e7..377374b3 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -53,12 +53,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) -} - // CosmosAccountsPager interface for Cosmos DB accounts pager (enables mocking). type CosmosAccountsPager interface { More() bool @@ -70,7 +64,7 @@ type Client struct { cred azcore.TokenCredential httpClient HTTPClient recommendationsPager RecommendationsPager - reservationsPager ReservationsDetailsPager + inventoryFactories *reservations.InventoryFactories cosmosAccountsPager CosmosAccountsPager subscriptionID string region string @@ -118,9 +112,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) } // SetCosmosAccountsPager sets the Cosmos DB accounts pager (for testing). @@ -212,91 +215,14 @@ func (c *Client) appendPage(ctx context.Context, pricer *azrecs.Pricer, recs []c return recs, nil } -// GetExistingCommitments retrieves existing Cosmos DB reserved capacity using Azure Resource Graph. +// GetExistingCommitments retrieves the Cosmos DB 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 #705). 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) { - pager, err := c.createReservationsPager() - if err != nil { - return nil, fmt.Errorf("cosmosdb: create reservations pager: %w", err) - } - - return c.collectCosmosReservations(ctx, pager) -} - -// createReservationsPager creates a pager for listing reservations. -func (c *Client) createReservationsPager() (ReservationsDetailsPager, error) { - // Use injected pager if available (for testing) - 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 -} - -// collectCosmosReservations collects Cosmos DB reservations from the pager. -// Returns an error on first pagination failure so callers can't silently act -// on a partial list — see the compute client for the full rationale. -func (c *Client) collectCosmosReservations(ctx context.Context, pager ReservationsDetailsPager) ([]common.Commitment, error) { - commitments := make([]common.Commitment, 0) - - 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("cosmosdb: GetExistingCommitments pagination cap (%d pages) reached", maxReservationsPages) - } - page, err := slot.NextPage(ctx, pager) - if err != nil { - return nil, fmt.Errorf("cosmosdb: list reservations: %w", err) - } - - for _, detail := range page.Value { - if commitment := c.convertCosmosReservation(detail); 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 -} - -// convertCosmosReservation converts a reservation detail to a commitment if it's a Cosmos DB reservation. -func (c *Client) convertCosmosReservation(detail *armconsumption.ReservationDetail) *common.Commitment { - if detail.Properties == nil { - return nil - } - - props := detail.Properties - if props.SKUName == nil || !strings.Contains(strings.ToLower(*props.SKUName), "cosmos") { - return nil - } - - commitment := &common.Commitment{ - Provider: common.ProviderAzure, - Account: c.subscriptionID, - CommitmentType: common.CommitmentReservedInstance, - Service: common.ServiceNoSQL, - Region: c.region, - State: common.CommitmentStateActive, - } - - if props.ReservationID != nil { - commitment.CommitmentID = *props.ReservationID - } - if props.SKUName != nil { - commitment.ResourceType = *props.SKUName - } - - return commitment + return reservations.StrictCommitments(ctx, c.subscriptionID, c.factories(), maxReservationsPages, common.ServiceNoSQL, + armreservations.ReservedResourceTypeCosmosDb, reservations.StrictLabels{Service: "cosmosdb", Resource: "Cosmos DB"}) } // PurchaseCommitment purchases Cosmos DB reserved capacity using the two-step diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index 06a846bf..981afdc9 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -15,12 +15,15 @@ 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/cosmos/armcosmos/v2" + "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" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // MockRecommendationsPager mocks the RecommendationsPager interface. @@ -49,32 +52,6 @@ func (m *MockRecommendationsPager) NextPage(ctx context.Context) (armconsumption return page, nil } -// MockReservationsDetailsPager mocks the ReservationsDetailsPager interface. -type MockReservationsDetailsPager struct { - pages []armconsumption.ReservationsDetailsClientListResponse - index int - err error -} - -func (m *MockReservationsDetailsPager) More() bool { - if m.err != nil { - return true // Return true to allow error to be returned on NextPage - } - return m.index < len(m.pages) -} - -func (m *MockReservationsDetailsPager) NextPage(ctx context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) { - if m.err != nil { - return armconsumption.ReservationsDetailsClientListResponse{}, m.err - } - if m.index >= len(m.pages) { - return armconsumption.ReservationsDetailsClientListResponse{}, errors.New("no more pages") - } - page := m.pages[m.index] - m.index++ - return page, nil -} - // MockCosmosAccountsPager mocks the CosmosAccountsPager interface. type MockCosmosAccountsPager struct { pages []armcosmos.DatabaseAccountsClientListResponse @@ -569,19 +546,6 @@ func TestCosmosDBClient_GetOfferingDetails_RequestsSKUFilter(t *testing.T) { mockHTTP.AssertExpectations(t) } -func TestCosmosDBClient_GetExistingCommitments_Empty(t *testing.T) { - t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - // Mock pager returns no pages — the empty-subscription case. - client.SetReservationsPager(&MockReservationsDetailsPager{}) - - commitments, err := client.GetExistingCommitments(ctx) - require.NoError(t, err) - assert.Empty(t, commitments) -} - func TestCosmosPricingStructure(t *testing.T) { t.Parallel() pricing := CosmosPricing{ @@ -665,153 +629,113 @@ func TestCosmosDBClient_GetRecommendations_MultiplePages(t *testing.T) { assert.Equal(t, 2, mockPager.index) // Verify both pages were consumed } -func TestCosmosDBClient_GetExistingCommitments_WithMockPager(t *testing.T) { - t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - reservationID := "test-reservation-123" - skuName := "sql-db-standard" // Does NOT contain "cosmos" so won't be included +// 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 &reservationstest.OrderPager{Rows: rows, Err: err} }) +} - // Create mock pager with test data - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: &armconsumption.ReservationDetailProperties{ - ReservationID: &reservationID, - SKUName: &skuName, - }, - }, - }, - }, - }, - }, +// 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 reservationstest.AppliedLister{}, nil }, + NewOrderPager: func(string) reservations.OrderReservationsPager { return newPager() }, } - client.SetReservationsPager(mockPager) +} + +func cosmosRow(id, sku, location string, qty int32) *armreservations.ReservationResponse { + return reservationstest.LiveRow(armreservations.ReservedResourceTypeCosmosDb, "sub", id, sku, location, qty) +} - commitments, err := client.GetExistingCommitments(ctx) +func TestGetExistingCommitments_Empty(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + c.SetInventoryFactories(factoriesRows(nil)) + commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) - assert.NotNil(t, commitments) - // The SKU name doesn't contain "cosmos" so won't be included assert.Empty(t, commitments) } -func TestCosmosDBClient_GetExistingCommitments_CosmosCommitments(t *testing.T) { +func TestGetExistingCommitments_CosmosRowConverted(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - reservationID := "test-reservation-123" - skuName := "cosmos-db-standard" // Contains "cosmos" - - // Create mock pager with cosmos commitment - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: &armconsumption.ReservationDetailProperties{ - ReservationID: &reservationID, - SKUName: &skuName, - }, - }, - }, - }, - }, - }, - } - client.SetReservationsPager(mockPager) - - commitments, err := client.GetExistingCommitments(ctx) + c := NewClient(nil, "sub", "eastus") + c.SetInventoryFactories(factoriesRows(nil, cosmosRow("res-123", "Cosmos_DB_Standard", "westus2", 3))) + commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) require.Len(t, commitments, 1) - assert.Equal(t, reservationID, commitments[0].CommitmentID) - assert.Equal(t, skuName, commitments[0].ResourceType) + assert.Equal(t, reservationstest.OrderPath+"/reservations/res-123", commitments[0].CommitmentID) + assert.Equal(t, "Cosmos_DB_Standard", 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.ServiceNoSQL, commitments[0].Service) + assert.Empty(t, commitments[0].Engine, "NoSQLDetails keys carry no engine") + assert.Empty(t, commitments[0].Deployment, "NoSQLDetails keys carry no deployment") } -// TestCosmosDBClient_GetExistingCommitments_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 TestCosmosDBClient_GetExistingCommitments_DedupesDailyUsageRows(t *testing.T) { +func TestGetExistingCommitments_NonCosmosFiltered(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - reservationID := "test-reservation-123" - skuName := "cosmos-db-standard" - - dailyRow := &armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{ - ReservationID: &reservationID, - SKUName: &skuName, - }, - } - - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{dailyRow, dailyRow, dailyRow}, - }, - }, - }, - } - client.SetReservationsPager(mockPager) - - commitments, err := client.GetExistingCommitments(ctx) + c := NewClient(nil, "sub", "eastus") + sqlRow := cosmosRow("res-1", "Standard_S0", "eastus", 1) + sqlType := armreservations.ReservedResourceTypeSQLDatabases + sqlRow.Properties.ReservedResourceType = &sqlType + c.SetInventoryFactories(factoriesRows(nil, sqlRow, cosmosRow("res-2", "Cosmos_DB_Standard", "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, reservationID, commitments[0].CommitmentID) + require.Len(t, commitments, 1) + assert.Equal(t, reservationstest.OrderPath+"/reservations/res-2", commitments[0].CommitmentID) } -func TestCosmosDBClient_GetExistingCommitments_PagerError(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() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - mockPager := &MockReservationsDetailsPager{ - err: errors.New("API error"), - } - client.SetReservationsPager(mockPager) - - // Pagination errors must propagate — partial lists are unsafe for the - // purchase flow (duplicate-purchase risk). See the compute client. - commitments, err := client.GetExistingCommitments(ctx) + c := NewClient(nil, "sub", "eastus") + c.SetInventoryFactories(factoriesRows(nil, &armreservations.ReservationResponse{})) + _, err := c.GetExistingCommitments(context.Background()) require.Error(t, err) - assert.Contains(t, err.Error(), "cosmosdb: list reservations") - assert.Nil(t, commitments) } -func TestCosmosDBClient_GetExistingCommitments_NilProperties(t *testing.T) { +func TestGetExistingCommitments_PagerError(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") + c := NewClient(nil, "sub", "eastus") + c.SetInventoryFactories(factoriesRows(errors.New("api error"))) + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "api error") +} - // Create mock pager with nil properties - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: nil, // Nil properties should be skipped - }, - }, - }, - }, +func TestGetExistingCommitments_ListerError(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + c.SetInventoryFactories(&reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { + return reservationstest.AppliedLister{Err: errors.New("boom")}, nil }, - } - client.SetReservationsPager(mockPager) + NewOrderPager: func(string) reservations.OrderReservationsPager { return &reservationstest.OrderPager{} }, + }) + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err, "a failed inventory lookup must not masquerade as an empty list") + assert.Contains(t, err.Error(), "cosmosdb: list applied reservations") +} - commitments, err := client.GetExistingCommitments(ctx) - require.NoError(t, err) - assert.Empty(t, commitments) +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()) } func TestCosmosDBClient_GetValidResourceTypes_WithMockPager(t *testing.T) { @@ -1021,10 +945,10 @@ func TestCosmosDBClient_SetterMethods(t *testing.T) { client.SetRecommendationsPager(mockRecPager) assert.Equal(t, mockRecPager, client.recommendationsPager) - // Test SetReservationsPager - mockResPager := &MockReservationsDetailsPager{} - client.SetReservationsPager(mockResPager) - assert.Equal(t, mockResPager, client.reservationsPager) + // Test SetInventoryFactories + factories := &reservations.InventoryFactories{} + client.SetInventoryFactories(factories) + assert.Same(t, factories, client.inventoryFactories) // Test SetCosmosAccountsPager mockAccountsPager := &MockCosmosAccountsPager{} diff --git a/providers/azure/services/cosmosdb/slot_wiring_test.go b/providers/azure/services/cosmosdb/slot_wiring_test.go index 2ce02a41..a4cc506d 100644 --- a/providers/azure/services/cosmosdb/slot_wiring_test.go +++ b/providers/azure/services/cosmosdb/slot_wiring_test.go @@ -9,8 +9,11 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cosmos/armcosmos/v2" + "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" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // onePager yields a single empty page and asserts the slot is held while @@ -28,6 +31,14 @@ func (p *onePager[T]) NextPage(context.Context) (T, error) { return zero, nil } +// slotLister reports one order so the order pager is exercised too. +type slotLister struct{ p *slottest.Probe } + +func (l slotLister) GetAppliedReservationList(ctx context.Context, sub string, o *armreservations.AzureReservationAPIClientGetAppliedReservationListOptions) (armreservations.AzureReservationAPIClientGetAppliedReservationListResponse, error) { + l.p.Check() + return reservationstest.AppliedLister{}.GetAppliedReservationList(ctx, sub, o) +} + type probeHTTP struct{ probe *slottest.Probe } func (h probeHTTP) Do(*http.Request) (*http.Response, error) { @@ -43,7 +54,12 @@ func TestClientCallsHoldSharedSlot(t *testing.T) { _, _ = c.GetRecommendations(ctx, nil) }, "GetExistingCommitments": func(ctx context.Context, p *slottest.Probe, c *Client) { - c.SetReservationsPager(&onePager[armconsumption.ReservationsDetailsClientListResponse]{probe: p}) + c.SetInventoryFactories(&reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { return slotLister{p}, nil }, + NewOrderPager: func(string) reservations.OrderReservationsPager { + return &onePager[armreservations.ReservationClientListResponse]{probe: p} + }, + }) _, _ = c.GetExistingCommitments(ctx) }, "GetValidResourceTypes": func(ctx context.Context, p *slottest.Probe, c *Client) { From bfb0594ac3ec3f5bf978f3b4a4859c535f970ae3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:14:23 +0100 Subject: [PATCH 03/21] test(azure): cap Cosmos DB reservation inventory pagination Cover the reservation inventory page cap, exact-cap success, cancelled context and mid-walk cancellation for GetExistingCommitments. Part of #705 Co-Authored-By: Claude Opus 5.5 --- .../services/cosmosdb/pagination_cap_test.go | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/providers/azure/services/cosmosdb/pagination_cap_test.go b/providers/azure/services/cosmosdb/pagination_cap_test.go index a8a31f78..c7d55b5f 100644 --- a/providers/azure/services/cosmosdb/pagination_cap_test.go +++ b/providers/azure/services/cosmosdb/pagination_cap_test.go @@ -5,7 +5,11 @@ import ( "testing" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cosmos/armcosmos/v2" + "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" ) // countingAccountsPager reports More() until limit pages were requested, so a @@ -71,3 +75,85 @@ func TestFetchDominantAPIType_ExactlyCapPagesWithPartialLastPageSucceeds(t *test assert.Equal(t, "mongodb", client.fetchDominantAPIType(context.Background())) assert.Equal(t, maxAccountsPages, pager.calls) } + +// countingReservationsPager reports More() until limit pages were requested, +// so a missing cap shows up as calls == limit instead of a hung test. +type countingReservationsPager struct { + calls, limit int +} + +func (p *countingReservationsPager) More() bool { return p.calls < p.limit } +func (p *countingReservationsPager) NextPage(_ context.Context) (armreservations.ReservationClientListResponse, error) { + p.calls++ + return armreservations.ReservationClientListResponse{}, nil +} + +func TestGetExistingCommitments_PageCapReturnsError(t *testing.T) { + t.Parallel() + pager := &countingReservationsPager{limit: 1000} + client := NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "pagination cap") + assert.Equal(t, maxReservationsPages, pager.calls) +} + +func TestGetExistingCommitments_CancelledContextReturnsContextError(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + pager := &countingReservationsPager{limit: 1000} + client := NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Zero(t, pager.calls) +} + +func TestGetExistingCommitments_CapIsExact(t *testing.T) { + t.Parallel() + atCap := &countingReservationsPager{limit: maxReservationsPages} + client := NewClient(nil, "test-subscription", "eastus") + 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.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return overCap })) + _, err = client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Equal(t, maxReservationsPages, overCap.calls) +} + +// cancelOnSecondCallPager cancels the context while serving its second page. +type cancelOnSecondCallPager struct { + calls int + cancel context.CancelFunc +} + +func (p *cancelOnSecondCallPager) More() bool { return true } +func (p *cancelOnSecondCallPager) NextPage(_ context.Context) (armreservations.ReservationClientListResponse, error) { + p.calls++ + if p.calls == 2 { + p.cancel() + } + return armreservations.ReservationClientListResponse{}, nil +} + +func TestGetExistingCommitments_CancelMidWalkReturnsContextError(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + pager := &cancelOnSecondCallPager{cancel: cancel} + client := NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Equal(t, 2, pager.calls) +} From 23fc4b4e3adf29732e3a4872ec88c2fef188c437 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:15:20 +0100 Subject: [PATCH 04/21] test(azure): tighten strict inventory helper tests Assert a nil result on the error paths, prefix the generic test helper names, and document the inventory fakes in the reservationstest package. Co-Authored-By: Claude Opus 5.5 --- .../reservations/reservationstest/capture.go | 3 +- .../reservations/strict_commitments_test.go | 32 ++++++++++--------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/providers/azure/services/internal/reservations/reservationstest/capture.go b/providers/azure/services/internal/reservations/reservationstest/capture.go index 43f671bf..7cb3c147 100644 --- a/providers/azure/services/internal/reservations/reservationstest/capture.go +++ b/providers/azure/services/internal/reservations/reservationstest/capture.go @@ -1,5 +1,6 @@ // Package reservationstest provides a fake ARM transport for asserting the -// purchase request bodies the Azure service clients send. +// purchase request bodies the Azure service clients send, and fakes for the +// reservation inventory path (live rows, applied lister, order pager). package reservationstest import ( diff --git a/providers/azure/services/internal/reservations/strict_commitments_test.go b/providers/azure/services/internal/reservations/strict_commitments_test.go index ea0f3d04..320aa968 100644 --- a/providers/azure/services/internal/reservations/strict_commitments_test.go +++ b/providers/azure/services/internal/reservations/strict_commitments_test.go @@ -14,11 +14,11 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) -const sub = "sub-a" +const strictSub = "sub-a" -var labels = reservations.StrictLabels{Service: "svc", Resource: "Redis"} +var strictLabels = reservations.StrictLabels{Service: "svc", Resource: "Redis"} -func factories(listErr error, rows ...*armreservations.ReservationResponse) reservations.InventoryFactories { +func strictFactories(listErr error, rows ...*armreservations.ReservationResponse) reservations.InventoryFactories { return reservations.InventoryFactories{ NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { return reservationstest.AppliedLister{Err: listErr}, nil @@ -29,24 +29,25 @@ func factories(listErr error, rows ...*armreservations.ReservationResponse) rese } } -func list(f reservations.InventoryFactories) ([]common.Commitment, error) { - return reservations.StrictCommitments(context.Background(), sub, f, 5, common.ServiceMemoryDB, - armreservations.ReservedResourceTypeRedisCache, labels) +func strictList(f reservations.InventoryFactories) ([]common.Commitment, error) { + return reservations.StrictCommitments(context.Background(), strictSub, f, 5, common.ServiceMemoryDB, + armreservations.ReservedResourceTypeRedisCache, strictLabels) } func TestStrictCommitments_ListErrorKeepsPrefixAndCause(t *testing.T) { t.Parallel() cause := errors.New("boom") - _, err := list(factories(cause)) + got, err := strictList(strictFactories(cause)) + assert.Nil(t, got) require.ErrorIs(t, err, cause) assert.Contains(t, err.Error(), "svc: list applied reservations: ") } func TestStrictCommitments_IgnoresOtherTypes(t *testing.T) { t.Parallel() - got, err := list(factories(nil, - reservationstest.LiveRow(armreservations.ReservedResourceTypeSQLDatabases, sub, "sql", "S0", "eastus", 1), - reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, sub, "redis", "P1", "eastus", 1))) + got, err := strictList(strictFactories(nil, + reservationstest.LiveRow(armreservations.ReservedResourceTypeSQLDatabases, strictSub, "sql", "S0", "eastus", 1), + reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, strictSub, "redis", "P1", "eastus", 1))) require.NoError(t, err) require.Len(t, got, 1) assert.Equal(t, reservationstest.OrderPath+"/reservations/redis", got[0].CommitmentID) @@ -54,17 +55,18 @@ func TestStrictCommitments_IgnoresOtherTypes(t *testing.T) { func TestStrictCommitments_IncompleteLiveRowFailsClosed(t *testing.T) { t.Parallel() - row := reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, sub, "redis", "P1", "eastus", 1) + row := reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, strictSub, "redis", "P1", "eastus", 1) row.Properties.Quantity = nil - _, err := list(factories(nil, row)) + got, err := strictList(strictFactories(nil, row)) + assert.Nil(t, got) require.ErrorContains(t, err, "quantity") } func TestStrictCommitments_ReturnsConvertedFields(t *testing.T) { t.Parallel() - got, err := list(factories(nil, - reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, sub, "a", "P1", "westus2", 3), - reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, sub, "b", "P2", "eastus", 1))) + got, err := strictList(strictFactories(nil, + reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, strictSub, "a", "P1", "westus2", 3), + reservationstest.LiveRow(armreservations.ReservedResourceTypeRedisCache, strictSub, "b", "P2", "eastus", 1))) require.NoError(t, err) require.Len(t, got, 2) assert.Equal(t, "P1", got[0].ResourceType) From c2238a22da967e437f0dd13bcc08899dcf2d5545 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:19:47 +0100 Subject: [PATCH 05/21] test(azure): prove held Cosmos DB reservations dedupe recommendations Buy a converter-built recommendation through PurchaseCommitment, build the held reservation from the captured purchase body, and run both through the duplicate checker: partial and full holds, non-counting rows, and fail-closed rows. Asserts the NoSQL key carries no engine or deployment. Part of #705 Co-Authored-By: Claude Opus 5.5 --- .../cosmosdb/held_reservation_dedupe_test.go | 159 ++++++++++++++++++ 1 file changed, 159 insertions(+) create mode 100644 providers/azure/services/cosmosdb/held_reservation_dedupe_test.go diff --git a/providers/azure/services/cosmosdb/held_reservation_dedupe_test.go b/providers/azure/services/cosmosdb/held_reservation_dedupe_test.go new file mode 100644 index 00000000..d7dded61 --- /dev/null +++ b/providers/azure/services/cosmosdb/held_reservation_dedupe_test.go @@ -0,0 +1,159 @@ +package cosmosdb + +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/reservationstest" +) + +// Issue #705: a Cosmos DB reservation CUDly (or anyone) already holds must +// reduce the matching recommendation. Fixture dates are UTC calendar dates, +// because the Reservations API returns date-only values. +var ( + fixtureToday = time.Now().UTC().Truncate(24 * time.Hour) + fixturePast = fixtureToday.AddDate(0, 0, -30) +) + +const cosmosSKU = "CosmosDB_RU_1000" + +func testPricer() *recommendations.Pricer { + return recommendations.NewPricer("cosmosdb", func(context.Context, string, string, int) (recommendations.ReservationPrice, error) { + return recommendations.ReservationPrice{Total: 1000, Currency: "USD"}, nil + }) +} + +// upfrontRec runs the real converter and variant expansion for a count-2 +// P1Y recommendation and returns its upfront variant. +func upfrontRec(t *testing.T, c *Client) common.Recommendation { + t.Helper() + raw := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), mocks.WithTerm("P1Y"), mocks.WithQuantity(2), + mocks.WithNormalizedSize(cosmosSKU), mocks.WithCosts(1000, 700, 300)) + recs, err := c.appendPage(context.Background(), testPricer(), nil, + []armconsumption.ReservationRecommendationClassification{raw}) + require.NoError(t, err) + require.Len(t, recs, 2) + upfront := recs[0] + require.Equal(t, "upfront", upfront.PaymentOption) + require.Equal(t, 2, upfront.Count) + // The dedupe key reads engine and deployment from the details; NoSQL + // recommendations carry neither, so held commitments must carry neither. + require.Empty(t, common.EngineFromDetails(upfront.Details)) + require.Empty(t, common.DeploymentFromDetails(upfront.Details)) + return upfront +} + +// purchasedRow buys rec through a capturing transport and builds the held +// reservation from the request body CUDly actually sent. +func purchasedRow(t *testing.T, rec common.Recommendation, qty int32) *armreservations.ReservationResponse { + t.Helper() + capture := &reservationstest.Capture{} + buyer := NewClientWithHTTP(&fake.TokenCredential{}, "sub", rec.Region, 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"` + Properties struct { + ReservedResourceType string `json:"reservedResourceType"` + Quantity int32 `json:"quantity"` + } `json:"properties"` + } + require.NoError(t, json.Unmarshal(capture.BodyBytes, &body)) + require.Equal(t, rec.Region, body.Location) + require.Equal(t, rec.Count, int(body.Properties.Quantity)) + return reservationstest.LiveRow(armreservations.ReservedResourceType(body.Properties.ReservedResourceType), + "sub", "held", body.SKU.Name, body.Location, qty) +} + +func adjust(t *testing.T, c *Client, recs ...common.Recommendation) ([]common.Recommendation, error) { + t.Helper() + passed, _, err := recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, c) + return passed, err +} + +func TestHeldReservation_PartialHoldReducesRecommendation(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + upfront := upfrontRec(t, c) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, upfront, 1))) + + passed, err := adjust(t, c, 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_FullHoldFiltersRecommendation(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + upfront := upfrontRec(t, c) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, upfront, 2))) + + passed, err := adjust(t, c, 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 } + } + 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, ""}, + {"creating", state(armreservations.ProvisioningStateCreating), 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("CosmosDB_RU_5000") }, 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 := upfrontRec(t, c) + row := purchasedRow(t, upfront, 1) + tc.mutate(row) + c.SetInventoryFactories(factoriesRows(nil, row)) + passed, err := adjust(t, c, 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) + }) + } +} From 65c99db923455de86d7868e8f942b1128e9cfbe7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:28:08 +0100 Subject: [PATCH 06/21] fix(azure): read Synapse held reservations from the Reservations API GetExistingCommitments now lists applied reservations through the shared StrictCommitments path instead of the Consumption daily-usage details, so the duplicate-purchase guard sees purchase date, quantity, location and state, and fails closed on incomplete rows. Removes the Consumption pager and collector, and fixes the stale SqlDW reserved resource type comment. Part of #705 Co-Authored-By: Claude Opus 5.5 --- providers/azure/provider_test.go | 29 +++ providers/azure/services/synapse/client.go | 79 +++----- .../azure/services/synapse/client_test.go | 178 ++++++++---------- .../services/synapse/pagination_cap_test.go | 73 ------- .../services/synapse/slot_wiring_test.go | 22 ++- 5 files changed, 146 insertions(+), 235 deletions(-) diff --git a/providers/azure/provider_test.go b/providers/azure/provider_test.go index 7bb13c37..17e8c857 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -13,6 +13,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud" "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/reservations/armreservations" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -485,6 +486,34 @@ func TestAzureClients_GetExistingCommitments_PagerConstructionError(t *testing.T } } +// Held reservations come from the Reservations API, so a client-construction +// failure of the default inventory factories surfaces under the shared +// ": list applied reservations" prefix. +func TestAzureClients_ReservationInventory_DefaultFactoryError(t *testing.T) { + // Keep this test sequential: the SDK reads this process-wide configuration. + originalCloud := cloud.AzurePublic + t.Cleanup(func() { cloud.AzurePublic = originalCloud }) + cloud.AzurePublic = cloud.Configuration{} + + _, sdkErr := armreservations.NewAzureReservationAPIClient(nil, nil) + require.Error(t, sdkErr) + + for _, tc := range []struct { + name string + client provider.ServiceClient + }{ + {"synapse", NewSynapseClient(nil, "test-subscription", "eastus")}, + } { + t.Run(tc.name, func(t *testing.T) { + commitments, err := tc.client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Nil(t, commitments) + assert.Contains(t, err.Error(), tc.name+": list applied reservations: ") + assert.Contains(t, err.Error(), sdkErr.Error()) + }) + } +} + func TestAzureProvider_GetRecommendationsClient_NotConfigured(t *testing.T) { t.Parallel() // Test GetRecommendationsClient when Azure is not configured. Inject a diff --git a/providers/azure/services/synapse/client.go b/providers/azure/services/synapse/client.go index 0d79c14f..b47358b8 100644 --- a/providers/azure/services/synapse/client.go +++ b/providers/azure/services/synapse/client.go @@ -49,18 +49,12 @@ 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) -} - // Client handles Azure Synapse Analytics Reserved Capacity. type Client struct { cred azcore.TokenCredential httpClient HTTPClient recommendationsPager RecommendationsPager - reservationsPager ReservationsDetailsPager + inventoryFactories *reservations.InventoryFactories subscriptionID string region string } @@ -97,9 +91,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) } // GetServiceType returns the service type. @@ -194,54 +197,14 @@ func (c *Client) appendRegionRecommendations(ctx context.Context, pricer *recomm return recs, nil } -// GetExistingCommitments retrieves existing Synapse reserved capacity -// commitments from the Azure Consumption API. +// GetExistingCommitments retrieves the Synapse 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 #705). 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) { - pager, err := c.createReservationsPager() - if err != nil { - return nil, fmt.Errorf("synapse: create reservations pager: %w", err) - } - - return c.collectSynapseReservations(ctx, pager) -} - -func (c *Client) createReservationsPager() (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 -} - -func (c *Client) collectSynapseReservations(ctx context.Context, pager ReservationsDetailsPager) ([]common.Commitment, error) { - commitments := make([]common.Commitment, 0) - - 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("synapse: GetExistingCommitments pagination cap (%d pages) reached", maxReservationsPages) - } - page, err := slot.NextPage(ctx, pager) - if err != nil { - return nil, fmt.Errorf("synapse: list reservations: %w", err) - } - for _, detail := range page.Value { - if commitment := c.convertSynapseReservation(detail); 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 + return reservations.StrictCommitments(ctx, c.subscriptionID, c.factories(), maxReservationsPages, common.ServiceDataWarehouse, + armreservations.ReservedResourceTypeSQLDataWarehouse, reservations.StrictLabels{Service: "synapse", Resource: "Synapse"}) } // convertSynapseReservation converts a reservation detail to a Commitment if @@ -280,7 +243,7 @@ func (c *Client) convertSynapseReservation(detail *armconsumption.ReservationDet // PurchaseCommitment purchases Synapse reserved capacity via the Azure // Reservations API two-step flow (calculatePrice -> purchase). The reserved -// resource type is "SqlDW" which covers Dedicated SQL Pool DWU reservations. +// resource type is "SqlDataWarehouse" which covers Dedicated SQL Pool DWU reservations. func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, diff --git a/providers/azure/services/synapse/client_test.go b/providers/azure/services/synapse/client_test.go index 9b1c85bc..80cf361c 100644 --- a/providers/azure/services/synapse/client_test.go +++ b/providers/azure/services/synapse/client_test.go @@ -23,6 +23,7 @@ import ( "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" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // ---- credential mock ------------------------------------------------------- @@ -78,28 +79,6 @@ func (e *errorRecommendationsPager) NextPage(_ context.Context) (armconsumption. return armconsumption.ReservationRecommendationsClientListResponse{}, errors.New("API error") } -type fakeReservationsPager struct { - pages []armconsumption.ReservationsDetailsClientListResponse - index int - err error -} - -func (m *fakeReservationsPager) More() bool { - return m.index < len(m.pages) -} - -func (m *fakeReservationsPager) NextPage(_ context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) { - if m.err != nil { - return armconsumption.ReservationsDetailsClientListResponse{}, m.err - } - if m.index >= len(m.pages) { - return armconsumption.ReservationsDetailsClientListResponse{}, errors.New("no more pages") - } - page := m.pages[m.index] - m.index++ - return page, nil -} - // ---- helpers --------------------------------------------------------------- func newTestClient() *Client { @@ -328,117 +307,114 @@ func TestGetRecommendations_modernShape(t *testing.T) { // ---- GetExistingCommitments ----------------------------------------------- +// 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 &reservationstest.OrderPager{Rows: rows, Err: err} }) +} + +// 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 reservationstest.AppliedLister{}, nil }, + NewOrderPager: func(string) reservations.OrderReservationsPager { return newPager() }, + } +} + +func synapseRow(id, sku, location string, qty int32) *armreservations.ReservationResponse { + return reservationstest.LiveRow(armreservations.ReservedResourceTypeSQLDataWarehouse, "sub-123", id, sku, location, qty) +} + func TestGetExistingCommitments_empty(t *testing.T) { t.Parallel() c := newTestClient() - c.SetReservationsPager(&fakeReservationsPager{}) - + c.SetInventoryFactories(factoriesRows(nil)) commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) assert.Empty(t, commitments) } -func TestGetExistingCommitments_synapseSKU(t *testing.T) { +func TestGetExistingCommitments_synapseRowConverted(t *testing.T) { t.Parallel() c := newTestClient() - - skuName := "DW1000c" - reservationID := "synapse-res-123" - c.SetReservationsPager(&fakeReservationsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: &armconsumption.ReservationDetailProperties{ - SKUName: &skuName, - ReservationID: &reservationID, - }, - }, - }, - }, - }, - }, - }) - + c.SetInventoryFactories(factoriesRows(nil, synapseRow("res-123", "DW1000c", "westus2", 3))) commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) require.Len(t, commitments, 1) - assert.Equal(t, common.ServiceDataWarehouse, commitments[0].Service) + assert.Equal(t, reservationstest.OrderPath+"/reservations/res-123", commitments[0].CommitmentID) assert.Equal(t, "DW1000c", commitments[0].ResourceType) - assert.Equal(t, "synapse-res-123", commitments[0].CommitmentID) - assert.Equal(t, common.CommitmentReservedInstance, commitments[0].CommitmentType) + 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.ServiceDataWarehouse, commitments[0].Service) assert.Equal(t, common.ProviderAzure, commitments[0].Provider) + assert.Empty(t, commitments[0].Engine, "DataWarehouseDetails keys carry no engine") + assert.Empty(t, commitments[0].Deployment, "DataWarehouseDetails keys carry no deployment") } -// 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_nonSynapseFiltered(t *testing.T) { t.Parallel() c := newTestClient() - - skuName := "DW1000c" - reservationID := "synapse-res-123" - dailyRow := &armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{ - SKUName: &skuName, - ReservationID: &reservationID, - }, - } - c.SetReservationsPager(&fakeReservationsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{dailyRow, dailyRow, dailyRow}, - }, - }, - }, - }) - + sqlRow := synapseRow("res-1", "Standard_S0", "eastus", 1) + sqlType := armreservations.ReservedResourceTypeSQLDatabases + sqlRow.Properties.ReservedResourceType = &sqlType + c.SetInventoryFactories(factoriesRows(nil, sqlRow, synapseRow("res-2", "DW1000c", "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, "synapse-res-123", commitments[0].CommitmentID) + require.Len(t, commitments, 1) + assert.Equal(t, reservationstest.OrderPath+"/reservations/res-2", commitments[0].CommitmentID) } -func TestGetExistingCommitments_filterNonSynapse(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 := newTestClient() - - vmSKU := "Standard_D2s_v3" - c.SetReservationsPager(&fakeReservationsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: &armconsumption.ReservationDetailProperties{ - SKUName: &vmSKU, - }, - }, - }, - }, - }, - }, - }) - - commitments, err := c.GetExistingCommitments(context.Background()) - require.NoError(t, err) - assert.Empty(t, commitments, "non-Synapse SKU should be filtered out") + c.SetInventoryFactories(factoriesRows(nil, &armreservations.ReservationResponse{})) + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err) } func TestGetExistingCommitments_pagerError(t *testing.T) { t.Parallel() c := newTestClient() - c.SetReservationsPager(&fakeReservationsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{{}}, - err: errors.New("pager error"), - }) + 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_listerError(t *testing.T) { + t.Parallel() + c := newTestClient() + c.SetInventoryFactories(&reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { + return reservationstest.AppliedLister{Err: errors.New("boom")}, nil + }, + NewOrderPager: func(string) reservations.OrderReservationsPager { return &reservationstest.OrderPager{} }, + }) _, err := c.GetExistingCommitments(context.Background()) - assert.Error(t, err) + require.Error(t, err, "a failed inventory lookup must not masquerade as an empty list") + assert.Contains(t, err.Error(), "synapse: list applied reservations") +} + +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()) } // ---- ValidateOffering ----------------------------------------------------- diff --git a/providers/azure/services/synapse/pagination_cap_test.go b/providers/azure/services/synapse/pagination_cap_test.go index 9ed15fa3..cabffa6b 100644 --- a/providers/azure/services/synapse/pagination_cap_test.go +++ b/providers/azure/services/synapse/pagination_cap_test.go @@ -2,7 +2,6 @@ package synapse import ( "context" - "fmt" "testing" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" @@ -24,16 +23,6 @@ func (p *countingRecommendationsPager) NextPage(_ context.Context) (armconsumpti return armconsumption.ReservationRecommendationsClientListResponse{}, nil } -type countingReservationsPager struct { - calls, limit int -} - -func (p *countingReservationsPager) More() bool { return p.calls < p.limit } -func (p *countingReservationsPager) NextPage(_ context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) { - p.calls++ - return armconsumption.ReservationsDetailsClientListResponse{}, nil -} - func TestGetRecommendations_PageCapReturnsError(t *testing.T) { t.Parallel() pager := &countingRecommendationsPager{limit: 1000} @@ -59,31 +48,6 @@ func TestGetRecommendations_CancelledContextReturnsContextError(t *testing.T) { assert.Zero(t, pager.calls) } -func TestGetExistingCommitments_PageCapReturnsError(t *testing.T) { - t.Parallel() - pager := &countingReservationsPager{limit: 1000} - client := NewClient(nil, "test-subscription", "eastus") - client.SetReservationsPager(pager) - - _, err := client.GetExistingCommitments(context.Background()) - require.Error(t, err) - assert.Contains(t, err.Error(), "pagination cap") - assert.Equal(t, maxReservationsPages, pager.calls) -} - -func TestGetExistingCommitments_CancelledContextReturnsContextError(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithCancel(context.Background()) - cancel() - pager := &countingReservationsPager{limit: 1000} - client := NewClient(nil, "test-subscription", "eastus") - client.SetReservationsPager(pager) - - _, err := client.GetExistingCommitments(ctx) - require.ErrorIs(t, err, context.Canceled) - assert.Zero(t, pager.calls) -} - // finalPartialRecommendationsPager serves exactly pages pages: full pages of // two recommendations, then a last page of one with no next link. type finalPartialRecommendationsPager struct { @@ -111,31 +75,6 @@ func (p *finalPartialRecommendationsPager) NextPage(_ context.Context) (armconsu }, nil } -// finalPartialReservationsPager serves exactly pages pages: full pages of two -// reservations, then a last page of one with no next link. -type finalPartialReservationsPager struct { - calls, pages int -} - -func (p *finalPartialReservationsPager) More() bool { return p.calls < p.pages } -func (p *finalPartialReservationsPager) NextPage(_ context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) { - n := 2 - if p.calls == p.pages-1 { - n = 1 - } - var details []*armconsumption.ReservationDetail - for i := 0; i < n; i++ { - sku, id := "DW1000c", fmt.Sprintf("res-%d-%d", p.calls, i) - details = append(details, &armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{SKUName: &sku, ReservationID: &id}, - }) - } - p.calls++ - return armconsumption.ReservationsDetailsClientListResponse{ - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{Value: details}, - }, nil -} - func TestGetRecommendations_ExactlyCapPagesWithPartialLastPageSucceeds(t *testing.T) { t.Parallel() pager := &finalPartialRecommendationsPager{pages: maxRecsPages} @@ -148,15 +87,3 @@ func TestGetRecommendations_ExactlyCapPagesWithPartialLastPageSucceeds(t *testin assert.Len(t, recs, 2*(2*(maxRecsPages-1)+1)) assert.Equal(t, maxRecsPages, pager.calls) } - -func TestGetExistingCommitments_ExactlyCapPagesWithPartialLastPageSucceeds(t *testing.T) { - t.Parallel() - pager := &finalPartialReservationsPager{pages: maxReservationsPages} - client := NewClient(nil, "test-subscription", "eastus") - client.SetReservationsPager(pager) - - got, err := client.GetExistingCommitments(context.Background()) - require.NoError(t, err) - assert.Len(t, got, 2*(maxReservationsPages-1)+1) - assert.Equal(t, maxReservationsPages, pager.calls) -} diff --git a/providers/azure/services/synapse/slot_wiring_test.go b/providers/azure/services/synapse/slot_wiring_test.go index 77d93e08..235f10a3 100644 --- a/providers/azure/services/synapse/slot_wiring_test.go +++ b/providers/azure/services/synapse/slot_wiring_test.go @@ -8,8 +8,11 @@ import ( "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/LeanerCloud/cloud-commitments-go/providers/azure/internal/slot/slottest" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // slotPager is a one-page fake pager whose NextPage reports whether the shared @@ -27,6 +30,14 @@ 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(ctx context.Context, sub string, o *armreservations.AzureReservationAPIClientGetAppliedReservationListOptions) (armreservations.AzureReservationAPIClientGetAppliedReservationListResponse, error) { + l.p.Check() + return reservationstest.AppliedLister{}.GetAppliedReservationList(ctx, sub, o) +} + type slotHTTP struct{ p *slottest.Probe } func (h slotHTTP) Do(*http.Request) (*http.Response, error) { @@ -39,8 +50,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 @@ -50,7 +66,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) }}, {"retail pricing", func(ctx context.Context, p *slottest.Probe) { _, _ = (&Client{httpClient: slotHTTP{p}}).getSynapsePricing(ctx, "DW100c", "eastus", 1) From b3121a7397ca3e7e55af800b41bfa9e9eb063779 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:29:10 +0100 Subject: [PATCH 07/21] refactor(azure): drop the unused Synapse Consumption reservation converter Nothing calls convertSynapseReservation since GetExistingCommitments reads the Reservations API; remove it with its tests. Part of #705 Co-Authored-By: Claude Opus 5.5 --- providers/azure/services/synapse/client.go | 34 --------- .../azure/services/synapse/client_test.go | 76 ------------------- 2 files changed, 110 deletions(-) diff --git a/providers/azure/services/synapse/client.go b/providers/azure/services/synapse/client.go index b47358b8..3f422a77 100644 --- a/providers/azure/services/synapse/client.go +++ b/providers/azure/services/synapse/client.go @@ -207,40 +207,6 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen armreservations.ReservedResourceTypeSQLDataWarehouse, reservations.StrictLabels{Service: "synapse", Resource: "Synapse"}) } -// convertSynapseReservation converts a reservation detail to a Commitment if -// it is a Synapse SQL Pool or Spark reservation. Identification relies on the -// SKU name containing a Synapse-specific prefix ("DW" for Dedicated SQL Pools -// or "SCU" for Spark Compute Units). -func (c *Client) convertSynapseReservation(detail *armconsumption.ReservationDetail) *common.Commitment { - if detail == nil || detail.Properties == nil { - return nil - } - props := detail.Properties - if props.SKUName == nil { - return nil - } - skuLower := strings.ToLower(*props.SKUName) - if !strings.HasPrefix(skuLower, "dw") && - !strings.HasPrefix(skuLower, "scu") && - !strings.Contains(skuLower, "synapse") { - return nil - } - - commitment := &common.Commitment{ - Provider: common.ProviderAzure, - Account: c.subscriptionID, - CommitmentType: common.CommitmentReservedInstance, - Service: common.ServiceDataWarehouse, - Region: c.region, - State: common.CommitmentStateActive, - } - if props.ReservationID != nil { - commitment.CommitmentID = *props.ReservationID - } - commitment.ResourceType = *props.SKUName - return commitment -} - // PurchaseCommitment purchases Synapse reserved capacity via the Azure // Reservations API two-step flow (calculatePrice -> purchase). The reserved // resource type is "SqlDataWarehouse" which covers Dedicated SQL Pool DWU reservations. diff --git a/providers/azure/services/synapse/client_test.go b/providers/azure/services/synapse/client_test.go index 80cf361c..0804e14e 100644 --- a/providers/azure/services/synapse/client_test.go +++ b/providers/azure/services/synapse/client_test.go @@ -798,82 +798,6 @@ func TestPurchaseCommitment_tokenError(t *testing.T) { assert.False(t, result.Success) } -// ---- convertSynapseReservation -------------------------------------------- - -func TestConvertSynapseReservation_nil(t *testing.T) { - t.Parallel() - c := newTestClient() - assert.Nil(t, c.convertSynapseReservation(nil)) -} - -func TestConvertSynapseReservation_nilProperties(t *testing.T) { - t.Parallel() - c := newTestClient() - assert.Nil(t, c.convertSynapseReservation(&armconsumption.ReservationDetail{})) -} - -func TestConvertSynapseReservation_nonSynapseSKU(t *testing.T) { - t.Parallel() - c := newTestClient() - sku := "Premium_P1" - assert.Nil(t, c.convertSynapseReservation(&armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{SKUName: &sku}, - })) -} - -func TestConvertSynapseReservation_dwSKU(t *testing.T) { - t.Parallel() - c := newTestClient() - sku := "DW3000c" - resID := "res-abc" - commitment := c.convertSynapseReservation(&armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{ - SKUName: &sku, - ReservationID: &resID, - }, - }) - require.NotNil(t, commitment) - assert.Equal(t, "DW3000c", commitment.ResourceType) - assert.Equal(t, "res-abc", commitment.CommitmentID) - assert.Equal(t, common.ServiceDataWarehouse, commitment.Service) -} - -func TestConvertSynapseReservation_scuPrefix(t *testing.T) { - t.Parallel() - c := newTestClient() - // SCU prefix (Spark Compute Units) should be classified as Synapse. - sku := "SCU_Standard" - resID := "res-scu" - commitment := c.convertSynapseReservation(&armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{ - SKUName: &sku, - ReservationID: &resID, - }, - }) - require.NotNil(t, commitment) - assert.Equal(t, "SCU_Standard", commitment.ResourceType) -} - -func TestConvertSynapseReservation_scuSubstringNotMatched(t *testing.T) { - t.Parallel() - c := newTestClient() - // Non-Synapse SKU that merely contains "scu" as a substring must not - // be misclassified as a Synapse reservation. Guards against the prior - // substring-match false-positive. - sku := "rescue_premium" - assert.Nil(t, c.convertSynapseReservation(&armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{SKUName: &sku}, - })) -} - -func TestConvertSynapseReservation_nilSKUName(t *testing.T) { - t.Parallel() - c := newTestClient() - assert.Nil(t, c.convertSynapseReservation(&armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{}, - })) -} - // ---- public reservation selection ----------------------------------------- func TestGetOfferingDetails_ReservationSelection(t *testing.T) { From 752628adfe7199b99c0b7a4cbc577da37f90816f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:30:43 +0100 Subject: [PATCH 08/21] test(azure): cap Synapse reservation inventory pagination Cover the reservation inventory page cap, exact-cap success with a partial last page, cancelled context and mid-walk cancellation for GetExistingCommitments. Part of #705 Co-Authored-By: Claude Opus 5.5 --- .../services/synapse/pagination_cap_test.go | 117 ++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/providers/azure/services/synapse/pagination_cap_test.go b/providers/azure/services/synapse/pagination_cap_test.go index cabffa6b..7828971b 100644 --- a/providers/azure/services/synapse/pagination_cap_test.go +++ b/providers/azure/services/synapse/pagination_cap_test.go @@ -2,11 +2,14 @@ package synapse import ( "context" + "fmt" "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/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" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -87,3 +90,117 @@ func TestGetRecommendations_ExactlyCapPagesWithPartialLastPageSucceeds(t *testin assert.Len(t, recs, 2*(2*(maxRecsPages-1)+1)) assert.Equal(t, maxRecsPages, pager.calls) } + +// countingReservationsPager reports More() until limit pages were requested, +// so a missing cap shows up as calls == limit instead of a hung test. +type countingReservationsPager struct { + calls, limit int +} + +func (p *countingReservationsPager) More() bool { return p.calls < p.limit } +func (p *countingReservationsPager) NextPage(_ context.Context) (armreservations.ReservationClientListResponse, error) { + p.calls++ + return armreservations.ReservationClientListResponse{}, nil +} + +func TestGetExistingCommitments_PageCapReturnsError(t *testing.T) { + t.Parallel() + pager := &countingReservationsPager{limit: 1000} + client := NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "pagination cap") + assert.Equal(t, maxReservationsPages, pager.calls) +} + +func TestGetExistingCommitments_CancelledContextReturnsContextError(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + pager := &countingReservationsPager{limit: 1000} + client := NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Zero(t, pager.calls) +} + +func TestGetExistingCommitments_CapIsExact(t *testing.T) { + t.Parallel() + atCap := &countingReservationsPager{limit: maxReservationsPages} + client := NewClient(nil, "test-subscription", "eastus") + 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.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return overCap })) + _, err = client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Equal(t, maxReservationsPages, overCap.calls) +} + +// cancelOnSecondCallPager cancels the context while serving its second page. +type cancelOnSecondCallPager struct { + calls int + cancel context.CancelFunc +} + +func (p *cancelOnSecondCallPager) More() bool { return true } +func (p *cancelOnSecondCallPager) NextPage(_ context.Context) (armreservations.ReservationClientListResponse, error) { + p.calls++ + if p.calls == 2 { + p.cancel() + } + return armreservations.ReservationClientListResponse{}, nil +} + +func TestGetExistingCommitments_CancelMidWalkReturnsContextError(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + pager := &cancelOnSecondCallPager{cancel: cancel} + client := NewClient(nil, "test-subscription", "eastus") + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Equal(t, 2, pager.calls) +} + +// finalPartialReservationsPager serves exactly pages pages: full pages of two +// reservations, then a last page of one. +type finalPartialReservationsPager struct { + calls, pages int +} + +func (p *finalPartialReservationsPager) More() bool { return p.calls < p.pages } +func (p *finalPartialReservationsPager) NextPage(context.Context) (armreservations.ReservationClientListResponse, error) { + n := 2 + if p.calls == p.pages-1 { + n = 1 + } + var rows []*armreservations.ReservationResponse + for i := 0; i < n; i++ { + rows = append(rows, synapseRow(fmt.Sprintf("res-%d-%d", p.calls, i), "DW1000c", "eastus", 1)) + } + p.calls++ + return armreservations.ReservationClientListResponse{ReservationList: armreservations.ReservationList{Value: rows}}, nil +} + +func TestGetExistingCommitments_ExactlyCapPagesWithPartialLastPageSucceeds(t *testing.T) { + t.Parallel() + pager := &finalPartialReservationsPager{pages: maxReservationsPages} + client := newTestClient() + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + got, err := client.GetExistingCommitments(context.Background()) + require.NoError(t, err) + assert.Len(t, got, 2*(maxReservationsPages-1)+1) + assert.Equal(t, maxReservationsPages, pager.calls) +} From 02018629eaddc4b6cbade5723b31df4e5415d611 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:37:43 +0100 Subject: [PATCH 09/21] test(azure): prove held Synapse reservations dedupe recommendations Buy a converter-built recommendation through PurchaseCommitment, build the held reservation from the captured purchase body, and run both through the duplicate checker: partial and full holds, non-counting rows, and fail-closed rows. Asserts the data-warehouse key carries no engine or deployment. Part of #705 Co-Authored-By: Claude Opus 5.5 --- .../synapse/held_reservation_dedupe_test.go | 159 ++++++++++++++++++ 1 file changed, 159 insertions(+) create mode 100644 providers/azure/services/synapse/held_reservation_dedupe_test.go diff --git a/providers/azure/services/synapse/held_reservation_dedupe_test.go b/providers/azure/services/synapse/held_reservation_dedupe_test.go new file mode 100644 index 00000000..9e93b97a --- /dev/null +++ b/providers/azure/services/synapse/held_reservation_dedupe_test.go @@ -0,0 +1,159 @@ +package synapse + +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/reservationstest" +) + +// Issue #705: a Synapse reservation CUDly (or anyone) already holds must +// reduce the matching recommendation. Fixture dates are UTC calendar dates, +// because the Reservations API returns date-only values. +var ( + fixtureToday = time.Now().UTC().Truncate(24 * time.Hour) + fixturePast = fixtureToday.AddDate(0, 0, -30) +) + +const synapseSKU = "DW1000c" + +func testPricer() *recommendations.Pricer { + return recommendations.NewPricer("synapse", func(context.Context, string, string, int) (recommendations.ReservationPrice, error) { + return recommendations.ReservationPrice{Total: 1000, Currency: "USD"}, nil + }) +} + +// upfrontRec runs the real converter and variant expansion for a count-2 +// P1Y recommendation and returns its upfront variant. +func upfrontRec(t *testing.T, c *Client) common.Recommendation { + t.Helper() + raw := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), mocks.WithTerm("P1Y"), mocks.WithQuantity(2), + mocks.WithNormalizedSize(synapseSKU), mocks.WithCosts(1000, 700, 300)) + recs, err := c.appendRegionRecommendations(context.Background(), testPricer(), nil, + []armconsumption.ReservationRecommendationClassification{raw}) + require.NoError(t, err) + require.Len(t, recs, 2) + upfront := recs[0] + require.Equal(t, "upfront", upfront.PaymentOption) + require.Equal(t, 2, upfront.Count) + // The dedupe key reads engine and deployment from the details; data-warehouse + // recommendations carry neither, so held commitments must carry neither. + require.Empty(t, common.EngineFromDetails(upfront.Details)) + require.Empty(t, common.DeploymentFromDetails(upfront.Details)) + return upfront +} + +// purchasedRow buys rec through a capturing transport and builds the held +// reservation from the request body CUDly actually sent. +func purchasedRow(t *testing.T, rec common.Recommendation, qty int32) *armreservations.ReservationResponse { + t.Helper() + capture := &reservationstest.Capture{} + buyer := NewClientWithHTTP(&fake.TokenCredential{}, "sub", rec.Region, 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"` + Properties struct { + ReservedResourceType string `json:"reservedResourceType"` + Quantity int32 `json:"quantity"` + } `json:"properties"` + } + require.NoError(t, json.Unmarshal(capture.BodyBytes, &body)) + require.Equal(t, rec.Region, body.Location) + require.Equal(t, rec.Count, int(body.Properties.Quantity)) + return reservationstest.LiveRow(armreservations.ReservedResourceType(body.Properties.ReservedResourceType), + "sub", "held", body.SKU.Name, body.Location, qty) +} + +func adjust(t *testing.T, c *Client, recs ...common.Recommendation) ([]common.Recommendation, error) { + t.Helper() + passed, _, err := recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, c) + return passed, err +} + +func TestHeldReservation_PartialHoldReducesRecommendation(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + upfront := upfrontRec(t, c) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, upfront, 1))) + + passed, err := adjust(t, c, 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_FullHoldFiltersRecommendation(t *testing.T) { + t.Parallel() + c := NewClient(nil, "sub", "eastus") + upfront := upfrontRec(t, c) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, upfront, 2))) + + passed, err := adjust(t, c, 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 } + } + 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, ""}, + {"creating", state(armreservations.ProvisioningStateCreating), 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("DW2000c") }, 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 := upfrontRec(t, c) + row := purchasedRow(t, upfront, 1) + tc.mutate(row) + c.SetInventoryFactories(factoriesRows(nil, row)) + passed, err := adjust(t, c, 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) + }) + } +} From 42b14137ddc5ee9768158c719e777fc29fa327ce Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:41:07 +0100 Subject: [PATCH 10/21] fix(azure): update Cosmos DB inventory error expectations and changelog Move the cosmosdb row from the Consumption-pager construction-error table to the Reservations API default-factory error test, and record the cosmosdb and synapse inventory changes in the changelog. Part of #705 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 ++++ providers/azure/provider_test.go | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 866981b9..c00bcf35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -309,6 +309,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - **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. +- **Breaking (source):** `cosmosdb.ReservationsDetailsPager` and `(*cosmosdb.Client).SetReservationsPager` are removed; tests inject reservation inventory with `SetInventoryFactories` instead (#705). +- **`azure` Cosmos DB:** `cosmosdb.GetExistingCommitments` now reads held reservations from the Reservations API (purchase date, quantity, location, state), so a Cosmos DB reservation bought in the last day reduces the matching recommendation instead of being invisible to the duplicate guard. `Region` is now the reservation location, not the client region. An incomplete live reservation row fails the lookup with an error instead of being skipped (#705). +- **Breaking (source):** `synapse.ReservationsDetailsPager` and `(*synapse.Client).SetReservationsPager` are removed; tests inject reservation inventory with `SetInventoryFactories` instead (#705). +- **`azure` Synapse:** `synapse.GetExistingCommitments` now reads held reservations from the Reservations API (purchase date, quantity, location, state), so a Synapse reservation bought in the last day reduces the matching recommendation instead of being invisible to the duplicate guard. `Region` is now the reservation location, not the client region. An incomplete live reservation row fails the lookup with an error instead of being skipped (#705). - **`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/provider_test.go b/providers/azure/provider_test.go index 17e8c857..ad45e8a7 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -470,7 +470,6 @@ func TestAzureClients_GetExistingCommitments_PagerConstructionError(t *testing.T client provider.ServiceClient }{ {"cache", NewCacheClient(nil, "test-subscription", "eastus")}, - {"cosmosdb", NewCosmosDBClient(nil, "test-subscription", "eastus")}, {"database", NewDatabaseClient(nil, "test-subscription", "eastus")}, {"search", NewSearchClient(nil, "test-subscription", "eastus")}, } { @@ -502,6 +501,7 @@ func TestAzureClients_ReservationInventory_DefaultFactoryError(t *testing.T) { name string client provider.ServiceClient }{ + {"cosmosdb", NewCosmosDBClient(nil, "test-subscription", "eastus")}, {"synapse", NewSynapseClient(nil, "test-subscription", "eastus")}, } { t.Run(tc.name, func(t *testing.T) { From 584b987acd3f6fa8afc395ea547ff6c42b5ff584 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:46:09 +0100 Subject: [PATCH 11/21] fix(azure): read SQL Database held reservations from the Reservations API GetExistingCommitments now lists applied reservations through the shared StrictCommitments path instead of the Consumption daily-usage details, so the duplicate-purchase guard sees purchase date, quantity, location and state, and fails closed on incomplete rows. Held reservations are stamped with engine sqlserver and the cached dominant AZConfig, as recommendations are, and the server walk is skipped when nothing is held. Removes the Consumption pager and collector. Part of #705 Co-Authored-By: Claude Opus 5.5 --- providers/azure/provider_test.go | 2 +- providers/azure/services/database/client.go | 100 +++---- .../azure/services/database/client_test.go | 271 ++++++------------ .../services/database/slot_wiring_test.go | 20 +- 4 files changed, 150 insertions(+), 243 deletions(-) diff --git a/providers/azure/provider_test.go b/providers/azure/provider_test.go index ad45e8a7..ab1d7f6b 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -470,7 +470,6 @@ func TestAzureClients_GetExistingCommitments_PagerConstructionError(t *testing.T client provider.ServiceClient }{ {"cache", NewCacheClient(nil, "test-subscription", "eastus")}, - {"database", NewDatabaseClient(nil, "test-subscription", "eastus")}, {"search", NewSearchClient(nil, "test-subscription", "eastus")}, } { t.Run(tc.name, func(t *testing.T) { @@ -502,6 +501,7 @@ func TestAzureClients_ReservationInventory_DefaultFactoryError(t *testing.T) { client provider.ServiceClient }{ {"cosmosdb", NewCosmosDBClient(nil, "test-subscription", "eastus")}, + {"database", NewDatabaseClient(nil, "test-subscription", "eastus")}, {"synapse", NewSynapseClient(nil, "test-subscription", "eastus")}, } { t.Run(tc.name, func(t *testing.T) { diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index da0b56c1..bc89211a 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -34,6 +34,10 @@ const reservationResourceTypeSQLDB = "SQLDatabases" // maxRecsPages caps Consumption API recommendation pagination. const maxRecsPages = 10 +// sqlServerEngine is the engine every Azure SQL Database recommendation and +// held reservation carries. +const sqlServerEngine = "sqlserver" + // maxReservationsPages caps reservation-detail pagination. const maxReservationsPages = 50 @@ -60,12 +64,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) -} - // CapabilitiesClient interface for SQL capabilities (enables mocking). type CapabilitiesClient interface { ListByLocation(ctx context.Context, locationName string, options *armsql.CapabilitiesClientListByLocationOptions) (armsql.CapabilitiesClientListByLocationResponse, error) @@ -96,7 +94,7 @@ type Client struct { cred azcore.TokenCredential httpClient HTTPClient recommendationsPager RecommendationsPager - reservationsPager ReservationsDetailsPager + inventoryFactories *reservations.InventoryFactories capabilitiesClient CapabilitiesClient serversPager SQLServersPager managedInstancesPager SQLManagedInstancesPager @@ -174,9 +172,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) } // SetCapabilitiesClient sets the capabilities client (for testing). @@ -268,61 +275,28 @@ func (c *Client) appendPage(ctx context.Context, pricer *azrecs.Pricer, recs []c return recs, nil } -// GetExistingCommitments retrieves existing SQL Database reserved capacity using Azure Resource Graph. +// GetExistingCommitments retrieves the SQL Database 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 #705). Any failure or incomplete live row is returned as an +// error rather than as an empty or partial inventory. +// +// The dedupe key includes engine and deployment, and recommendations carry +// Engine "sqlserver" and the subscription's dominant AZConfig, so held +// reservations are stamped with the same values. The server walk is skipped +// when nothing is held. func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { - pager, err := c.createReservationsPager() - if err != nil { - return nil, fmt.Errorf("database: create reservations pager: %w", err) - } - - return c.collectSQLReservations(ctx, pager) -} - -// createReservationsPager creates a pager for listing reservations. -func (c *Client) createReservationsPager() (ReservationsDetailsPager, error) { - // Use injected pager if available (for testing) - if c.reservationsPager != nil { - return c.reservationsPager, nil - } - - client, err := armconsumption.NewReservationsDetailsClient(c.cred, nil) - if err != nil { - return nil, err + commitments, err := reservations.StrictCommitments(ctx, c.subscriptionID, c.factories(), maxReservationsPages, common.ServiceRelationalDB, + armreservations.ReservedResourceTypeSQLDatabases, reservations.StrictLabels{Service: "database", Resource: "SQL"}) + if err != nil || len(commitments) == 0 { + return commitments, err } - - scope := fmt.Sprintf("subscriptions/%s", c.subscriptionID) - return client.NewListPager(scope, &armconsumption.ReservationsDetailsClientListOptions{}), nil -} - -// collectSQLReservations collects SQL Database reservations from the pager. -// Returns an error on first pagination failure so callers can't silently act -// on a partial list — see the compute client for the full rationale. -func (c *Client) collectSQLReservations(ctx context.Context, pager ReservationsDetailsPager) ([]common.Commitment, error) { - commitments := make([]common.Commitment, 0) - - 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("database: GetExistingCommitments pagination cap (%d pages) reached", maxReservationsPages) - } - page, err := slot.NextPage(ctx, pager) - if err != nil { - return nil, fmt.Errorf("database: list reservations: %w", err) - } - - for _, detail := range page.Value { - if commitment := c.convertSQLReservation(detail); commitment != nil { - commitments = append(commitments, *commitment) - } - } + az := c.cachedDominantAZConfig(ctx) + for i := range commitments { + commitments[i].Engine = sqlServerEngine + commitments[i].Deployment = az } - - // 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 + return commitments, nil } // convertSQLReservation converts a reservation detail to a commitment if it's a SQL reservation. @@ -995,7 +969,7 @@ func (c *Client) createServersPager() (SQLServersPager, error) { func detailsFromSQLSKU(sku string) *common.DatabaseDetails { // Engine is always SQL Server for an Azure SQL Database reservation. return &common.DatabaseDetails{ - Engine: "sqlserver", + Engine: sqlServerEngine, InstanceClass: sku, } } diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index bbf7ee93..b274d4f9 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -22,6 +22,8 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" azpricing "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/pricing" "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" ) // MockRecommendationsPager mocks the RecommendationsPager interface. @@ -43,29 +45,6 @@ func (m *MockRecommendationsPager) NextPage(ctx context.Context) (armconsumption return page, nil } -// MockReservationsDetailsPager mocks the ReservationsDetailsPager interface. -type MockReservationsDetailsPager struct { - pages []armconsumption.ReservationsDetailsClientListResponse - index int - err error -} - -func (m *MockReservationsDetailsPager) More() bool { - return m.index < len(m.pages) -} - -func (m *MockReservationsDetailsPager) NextPage(ctx context.Context) (armconsumption.ReservationsDetailsClientListResponse, error) { - if m.err != nil { - return armconsumption.ReservationsDetailsClientListResponse{}, m.err - } - if m.index >= len(m.pages) { - return armconsumption.ReservationsDetailsClientListResponse{}, errors.New("no more pages") - } - page := m.pages[m.index] - m.index++ - return page, nil -} - // MockCapabilitiesClient mocks the CapabilitiesClient interface. // CallCount tracks how many times ListByLocation was invoked — used by // the cachedSKULookup-fetched-once test to pin the perf invariant. @@ -618,20 +597,6 @@ func TestDatabaseClient_GetOfferingDetails_ReservationAcrossPages(t *testing.T) } } -func TestDatabaseClient_GetExistingCommitments_Empty(t *testing.T) { - t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - // Mock pager returns no pages — the empty-subscription case. See the - // cache client for the full rationale. - client.SetReservationsPager(&MockReservationsDetailsPager{}) - - commitments, err := client.GetExistingCommitments(ctx) - require.NoError(t, err) - assert.Empty(t, commitments) -} - func TestDatabaseClient_GetRecommendations_WithMockPager(t *testing.T) { t.Parallel() ctx := context.Background() @@ -683,167 +648,117 @@ func TestDatabaseClient_GetRecommendations_MultiplePages(t *testing.T) { assert.Empty(t, recs) } -func TestDatabaseClient_GetExistingCommitments_WithMockPager(t *testing.T) { - t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - reservationID := "test-reservation-123" - skuName := "sql-standard-s0" +// 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 &reservationstest.OrderPager{Rows: rows, Err: err} }) +} - // Create mock pager with SQL commitment - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: &armconsumption.ReservationDetailProperties{ - ReservationID: &reservationID, - SKUName: &skuName, - }, - }, - }, - }, - }, - }, +// 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 reservationstest.AppliedLister{}, nil }, + NewOrderPager: func(string) reservations.OrderReservationsPager { return newPager() }, } +} - client.SetReservationsPager(mockPager) +func sqlRow(id, sku, location string, qty int32) *armreservations.ReservationResponse { + return reservationstest.LiveRow(armreservations.ReservedResourceTypeSQLDatabases, "sub", id, sku, location, qty) +} - commitments, err := client.GetExistingCommitments(ctx) - require.NoError(t, err) - require.Len(t, commitments, 1) - assert.Equal(t, reservationID, commitments[0].CommitmentID) - assert.Equal(t, skuName, commitments[0].ResourceType) - assert.Equal(t, common.ServiceRelationalDB, commitments[0].Service) +// inventoryClient returns a client with empty server pagers injected, so no +// inventory test can reach the network, and the pagers for call counting. +func inventoryClient(rows ...*armreservations.ReservationResponse) (*Client, *MockSQLManagedInstancesPager, *MockSQLServersPager) { + c := NewClient(nil, "sub", "eastus") + mi, servers := &MockSQLManagedInstancesPager{}, &MockSQLServersPager{} + c.SetManagedInstancesPager(mi) + c.SetServersPager(servers) + c.SetInventoryFactories(factoriesRows(nil, rows...)) + return c, mi, servers } -// TestDatabaseClient_GetExistingCommitments_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 TestDatabaseClient_GetExistingCommitments_DedupesDailyUsageRows(t *testing.T) { +// An empty inventory must not walk the subscription's SQL servers. +func TestGetExistingCommitments_EmptySkipsServerWalk(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - reservationID := "test-reservation-123" - skuName := "sql-standard-s0" - - dailyRow := &armconsumption.ReservationDetail{ - Properties: &armconsumption.ReservationDetailProperties{ - ReservationID: &reservationID, - SKUName: &skuName, - }, - } - - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{dailyRow, dailyRow, dailyRow}, - }, - }, - }, - } - - client.SetReservationsPager(mockPager) - - commitments, err := client.GetExistingCommitments(ctx) + c, mi, servers := inventoryClient() + mi.pages = []armsql.ManagedInstancesClientListResponse{buildMIPage(true)} + servers.pages = []armsql.ServersClientListResponse{buildServerPage(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, reservationID, commitments[0].CommitmentID) + assert.Empty(t, commitments) + assert.Zero(t, mi.CallCount) + assert.Zero(t, servers.CallCount) } -func TestDatabaseClient_GetExistingCommitments_FilterNonSQL(t *testing.T) { +func TestGetExistingCommitments_SQLRowConverted(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - // Test that non-SQL SKUs are filtered out - nonSQLSKU := "redis-premium-p1" - sqlSKU := "sql-premium-p2" - reservationID1 := "test-reservation-1" - reservationID2 := "test-reservation-2" - - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: &armconsumption.ReservationDetailProperties{ - ReservationID: &reservationID1, - SKUName: &nonSQLSKU, - }, - }, - { - Properties: &armconsumption.ReservationDetailProperties{ - ReservationID: &reservationID2, - SKUName: &sqlSKU, - }, - }, - }, - }, - }, - }, - } - - client.SetReservationsPager(mockPager) - - commitments, err := client.GetExistingCommitments(ctx) + c, _, _ := inventoryClient(sqlRow("res-123", "GP_Gen5_2", "westus2", 3)) + commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) require.Len(t, commitments, 1) - assert.Equal(t, reservationID2, commitments[0].CommitmentID) + assert.Equal(t, reservationstest.OrderPath+"/reservations/res-123", commitments[0].CommitmentID) + assert.Equal(t, "GP_Gen5_2", 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.ServiceRelationalDB, commitments[0].Service) + assert.Equal(t, "sqlserver", commitments[0].Engine) } -func TestDatabaseClient_GetExistingCommitments_NilProperties(t *testing.T) { +func TestGetExistingCommitments_DeploymentStamp(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - // Test that nil properties are handled gracefully - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{ - { - ReservationDetailsListResult: armconsumption.ReservationDetailsListResult{ - Value: []*armconsumption.ReservationDetail{ - { - Properties: nil, - }, - }, - }, - }, - }, + for name, tc := range map[string]struct { + pages []armsql.ManagedInstancesClientListResponse + want string + }{ + "no managed instances": {nil, ""}, + "all zone-redundant": {[]armsql.ManagedInstancesClientListResponse{buildMIPage(true, true)}, "zoneRedundant"}, + "none zone-redundant": {[]armsql.ManagedInstancesClientListResponse{buildMIPage(false)}, "none"}, + "mixed fleet is unknown": {[]armsql.ManagedInstancesClientListResponse{buildMIPage(true, false)}, ""}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + c, mi, _ := inventoryClient(sqlRow("a", "GP_Gen5_2", "eastus", 1), sqlRow("b", "GP_Gen5_4", "eastus", 1)) + mi.pages = tc.pages + commitments, err := c.GetExistingCommitments(context.Background()) + require.NoError(t, err) + require.Len(t, commitments, 2) + for _, commitment := range commitments { + assert.Equal(t, tc.want, commitment.Deployment) + } + }) } +} - client.SetReservationsPager(mockPager) - - commitments, err := client.GetExistingCommitments(ctx) +func TestGetExistingCommitments_NonSQLFiltered(t *testing.T) { + t.Parallel() + redisRow := sqlRow("res-1", "Premium_P1", "eastus", 1) + redisType := armreservations.ReservedResourceTypeRedisCache + redisRow.Properties.ReservedResourceType = &redisType + c, _, _ := inventoryClient(redisRow, sqlRow("res-2", "GP_Gen5_2", "eastus", 1)) + commitments, err := c.GetExistingCommitments(context.Background()) require.NoError(t, err) - assert.Empty(t, commitments) + require.Len(t, commitments, 1) + assert.Equal(t, reservationstest.OrderPath+"/reservations/res-2", commitments[0].CommitmentID) } -func TestDatabaseClient_GetExistingCommitments_PagerError(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, without walking servers. +func TestGetExistingCommitments_NilPropertiesFailsClosed(t *testing.T) { t.Parallel() - ctx := context.Background() - client := NewClient(nil, "test-subscription", "eastus") - - // Pagination errors must propagate — returning a partial commitment list - // silently is unsafe for the purchase flow (it could trigger duplicate - // purchases for reservations we didn't see). - mockPager := &MockReservationsDetailsPager{ - pages: []armconsumption.ReservationsDetailsClientListResponse{{}}, - err: errors.New("API error"), - } - - client.SetReservationsPager(mockPager) + c, mi, servers := inventoryClient(&armreservations.ReservationResponse{}) + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Zero(t, mi.CallCount) + assert.Zero(t, servers.CallCount) +} - commitments, err := client.GetExistingCommitments(ctx) +func TestGetExistingCommitments_PagerError(t *testing.T) { + t.Parallel() + c, _, _ := inventoryClient() + c.SetInventoryFactories(factoriesRows(errors.New("api error"))) + _, err := c.GetExistingCommitments(context.Background()) require.Error(t, err) - assert.Contains(t, err.Error(), "database: list reservations") - assert.Nil(t, commitments) + assert.Contains(t, err.Error(), "api error") } func TestDatabaseClient_GetValidResourceTypes_WithMock(t *testing.T) { @@ -959,10 +874,10 @@ func TestDatabaseClient_SetterMethods(t *testing.T) { client.SetRecommendationsPager(mockRecPager) assert.Equal(t, mockRecPager, client.recommendationsPager) - // Test SetReservationsPager - mockResPager := &MockReservationsDetailsPager{} - client.SetReservationsPager(mockResPager) - assert.Equal(t, mockResPager, client.reservationsPager) + // Test SetInventoryFactories + factories := &reservations.InventoryFactories{} + client.SetInventoryFactories(factories) + assert.Same(t, factories, client.inventoryFactories) // Test SetCapabilitiesClient mockCapClient := &MockCapabilitiesClient{} diff --git a/providers/azure/services/database/slot_wiring_test.go b/providers/azure/services/database/slot_wiring_test.go index e197b808..775a8c62 100644 --- a/providers/azure/services/database/slot_wiring_test.go +++ b/providers/azure/services/database/slot_wiring_test.go @@ -8,9 +8,12 @@ import ( "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/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/slot/slottest" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // onePager yields a single empty page and asserts the slot is held while @@ -28,6 +31,14 @@ func (p *onePager[T]) NextPage(context.Context) (T, error) { return zero, nil } +// slotLister reports one order so the order pager is exercised too. +type slotLister struct{ probe *slottest.Probe } + +func (l slotLister) GetAppliedReservationList(ctx context.Context, sub string, o *armreservations.AzureReservationAPIClientGetAppliedReservationListOptions) (armreservations.AzureReservationAPIClientGetAppliedReservationListResponse, error) { + l.probe.Check() + return reservationstest.AppliedLister{}.GetAppliedReservationList(ctx, sub, o) +} + type probeCaps struct{ probe *slottest.Probe } func (c probeCaps) ListByLocation(context.Context, string, *armsql.CapabilitiesClientListByLocationOptions) (armsql.CapabilitiesClientListByLocationResponse, error) { @@ -50,7 +61,14 @@ func TestClientCallsHoldSharedSlot(t *testing.T) { _, _ = c.GetRecommendations(ctx, nil) }, "GetExistingCommitments": func(ctx context.Context, p *slottest.Probe, c *Client) { - c.SetReservationsPager(&onePager[armconsumption.ReservationsDetailsClientListResponse]{probe: p}) + c.SetManagedInstancesPager(&MockSQLManagedInstancesPager{}) + c.SetServersPager(&MockSQLServersPager{}) + c.SetInventoryFactories(&reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { return slotLister{p}, nil }, + NewOrderPager: func(string) reservations.OrderReservationsPager { + return &onePager[armreservations.ReservationClientListResponse]{probe: p} + }, + }) _, _ = c.GetExistingCommitments(ctx) }, "GetValidResourceTypes": func(ctx context.Context, p *slottest.Probe, c *Client) { From 7bb1eaf71eb963506023d9a84ac9b6403b41e7fb Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:46:56 +0100 Subject: [PATCH 12/21] refactor(azure): drop the unused SQL Database Consumption reservation converter Nothing calls convertSQLReservation since GetExistingCommitments reads the Reservations API; remove it. Part of #705 Co-Authored-By: Claude Opus 5.5 --- providers/azure/services/database/client.go | 30 --------------------- 1 file changed, 30 deletions(-) diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index bc89211a..e777723d 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -299,36 +299,6 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen return commitments, nil } -// convertSQLReservation converts a reservation detail to a commitment if it's a SQL reservation. -func (c *Client) convertSQLReservation(detail *armconsumption.ReservationDetail) *common.Commitment { - if detail.Properties == nil { - return nil - } - - props := detail.Properties - if props.SKUName == nil || !strings.Contains(strings.ToLower(*props.SKUName), "sql") { - return nil - } - - commitment := &common.Commitment{ - Provider: common.ProviderAzure, - Account: c.subscriptionID, - CommitmentType: common.CommitmentReservedInstance, - Service: common.ServiceRelationalDB, - Region: c.region, - State: common.CommitmentStateActive, - } - - if props.ReservationID != nil { - commitment.CommitmentID = *props.ReservationID - } - if props.SKUName != nil { - commitment.ResourceType = *props.SKUName - } - - return commitment -} - // PurchaseCommitment purchases SQL Database reserved capacity using the two-step // calculatePrice->purchase flow required by Azure's Reservations API (issue #677). func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { From 4805d904b293e77c426de3a8c65b44c37b3c9f45 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:47:43 +0100 Subject: [PATCH 13/21] test(azure): cover SQL Database reservation inventory errors and pagination Add the lister-error and default-factory tests, the reservation page cap tests on shared reservationstest pagers, and the changelog entries. Part of #705 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 + .../azure/services/database/client_test.go | 34 +++++++++++ .../services/database/pagination_cap_test.go | 59 +++++++++++++++++++ .../reservationstest/inventory.go | 30 ++++++++++ 4 files changed, 125 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c00bcf35..3ec3aaf3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -313,6 +313,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - **`azure` Cosmos DB:** `cosmosdb.GetExistingCommitments` now reads held reservations from the Reservations API (purchase date, quantity, location, state), so a Cosmos DB reservation bought in the last day reduces the matching recommendation instead of being invisible to the duplicate guard. `Region` is now the reservation location, not the client region. An incomplete live reservation row fails the lookup with an error instead of being skipped (#705). - **Breaking (source):** `synapse.ReservationsDetailsPager` and `(*synapse.Client).SetReservationsPager` are removed; tests inject reservation inventory with `SetInventoryFactories` instead (#705). - **`azure` Synapse:** `synapse.GetExistingCommitments` now reads held reservations from the Reservations API (purchase date, quantity, location, state), so a Synapse reservation bought in the last day reduces the matching recommendation instead of being invisible to the duplicate guard. `Region` is now the reservation location, not the client region. An incomplete live reservation row fails the lookup with an error instead of being skipped (#705). +- **Breaking (source):** `database.ReservationsDetailsPager` and `(*database.Client).SetReservationsPager` are removed; tests inject reservation inventory with `SetInventoryFactories` instead (#705). +- **`azure` SQL Database:** `database.GetExistingCommitments` now reads held reservations from the Reservations API (purchase date, quantity, location, state), so a SQL Database reservation bought in the last day reduces the matching recommendation instead of being invisible to the duplicate guard. Held reservations carry engine `sqlserver` and the subscription's dominant zone-redundancy setting, as recommendations do. `Region` is now the reservation location, not the client region. An incomplete live reservation row fails the lookup with an error instead of being skipped (#705). - **`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/database/client_test.go b/providers/azure/services/database/client_test.go index b274d4f9..b711ebb2 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -761,6 +761,40 @@ func TestGetExistingCommitments_PagerError(t *testing.T) { assert.Contains(t, err.Error(), "api error") } +func TestGetExistingCommitments_ListerError(t *testing.T) { + t.Parallel() + c, _, _ := inventoryClient() + c.SetInventoryFactories(&reservations.InventoryFactories{ + NewAppliedLister: func() (reservations.AppliedReservationsLister, error) { + return reservationstest.AppliedLister{Err: errors.New("boom")}, nil + }, + NewOrderPager: func(string) reservations.OrderReservationsPager { return &reservationstest.OrderPager{} }, + }) + _, err := c.GetExistingCommitments(context.Background()) + require.Error(t, err, "a failed inventory lookup must not masquerade as an empty list") + assert.Contains(t, err.Error(), "database: list applied reservations") +} + +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()) +} + func TestDatabaseClient_GetValidResourceTypes_WithMock(t *testing.T) { t.Parallel() ctx := context.Background() diff --git a/providers/azure/services/database/pagination_cap_test.go b/providers/azure/services/database/pagination_cap_test.go index 9f8e28a9..3949fcc9 100644 --- a/providers/azure/services/database/pagination_cap_test.go +++ b/providers/azure/services/database/pagination_cap_test.go @@ -6,6 +6,10 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // countingMIPager and countingServersPager report More() until limit pages @@ -178,3 +182,58 @@ func TestHasRegularServers_ExactlyCapPagesWithPartialLastPageSucceeds(t *testing assert.True(t, complete) assert.Equal(t, maxSQLListPages, pager.calls) } + +func TestGetExistingCommitments_PageCapReturnsError(t *testing.T) { + t.Parallel() + pager := &reservationstest.CountingPager{Limit: 1000} + client, _, _ := inventoryClient() + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "pagination cap") + assert.Equal(t, maxReservationsPages, pager.Calls) +} + +func TestGetExistingCommitments_CancelledContextReturnsContextError(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + pager := &reservationstest.CountingPager{Limit: 1000} + client, _, _ := inventoryClient() + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Zero(t, pager.Calls) +} + +func TestGetExistingCommitments_CapIsExact(t *testing.T) { + t.Parallel() + atCap := &reservationstest.CountingPager{Limit: maxReservationsPages} + client, _, _ := inventoryClient() + 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 := &reservationstest.CountingPager{Limit: maxReservationsPages + 1} + client, _, _ = inventoryClient() + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return overCap })) + _, err = client.GetExistingCommitments(context.Background()) + require.Error(t, err) + assert.Equal(t, maxReservationsPages, overCap.Calls) +} + +func TestGetExistingCommitments_CancelMidWalkReturnsContextError(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + pager := &reservationstest.CancelOnSecondCallPager{Cancel: cancel} + client, _, _ := inventoryClient() + client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) + + _, err := client.GetExistingCommitments(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Equal(t, 2, pager.Calls) +} diff --git a/providers/azure/services/internal/reservations/reservationstest/inventory.go b/providers/azure/services/internal/reservations/reservationstest/inventory.go index 626ba26f..2123bc34 100644 --- a/providers/azure/services/internal/reservations/reservationstest/inventory.go +++ b/providers/azure/services/internal/reservations/reservationstest/inventory.go @@ -70,3 +70,33 @@ func (p *OrderPager) NextPage(context.Context) (armreservations.ReservationClien } return armreservations.ReservationClientListResponse{ReservationList: armreservations.ReservationList{Value: p.Rows}}, nil } + +// CountingPager reports More until Limit pages were requested, so a missing +// pagination cap shows up as Calls == Limit instead of a hung test. +type CountingPager struct { + Calls, Limit int +} + +func (p *CountingPager) More() bool { return p.Calls < p.Limit } + +func (p *CountingPager) NextPage(context.Context) (armreservations.ReservationClientListResponse, error) { + p.Calls++ + return armreservations.ReservationClientListResponse{}, nil +} + +// CancelOnSecondCallPager never ends and calls Cancel while serving its +// second page. +type CancelOnSecondCallPager struct { + Calls int + Cancel context.CancelFunc +} + +func (p *CancelOnSecondCallPager) More() bool { return true } + +func (p *CancelOnSecondCallPager) NextPage(context.Context) (armreservations.ReservationClientListResponse, error) { + p.Calls++ + if p.Calls == 2 { + p.Cancel() + } + return armreservations.ReservationClientListResponse{}, nil +} From c55bacfe218eca9e4d154cef39bea4e1fe0005c6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:49:20 +0100 Subject: [PATCH 14/21] test(azure): prove held SQL Database reservations dedupe recommendations Buy a converter-built recommendation (with the AZConfig enrichment) through PurchaseCommitment, build the held reservation from the captured purchase body, and run both through the duplicate checker: partial and full holds, non-counting rows, fail-closed rows, and the no, zone-redundant, non and mixed managed-instance fleets. Part of #705 Co-Authored-By: Claude Opus 5.5 --- .../database/held_reservation_dedupe_test.go | 206 ++++++++++++++++++ 1 file changed, 206 insertions(+) create mode 100644 providers/azure/services/database/held_reservation_dedupe_test.go diff --git a/providers/azure/services/database/held_reservation_dedupe_test.go b/providers/azure/services/database/held_reservation_dedupe_test.go new file mode 100644 index 00000000..43aff434 --- /dev/null +++ b/providers/azure/services/database/held_reservation_dedupe_test.go @@ -0,0 +1,206 @@ +package database + +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/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql" + "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/reservationstest" +) + +// Issue #705: a SQL Database reservation CUDly (or anyone) already holds must +// reduce the matching recommendation. Fixture dates are UTC calendar dates, +// because the Reservations API returns date-only values. +var ( + fixtureToday = time.Now().UTC().Truncate(24 * time.Hour) + fixturePast = fixtureToday.AddDate(0, 0, -30) +) + +const sqlSKU = "GP_Gen5_2" + +func testPricer() *recommendations.Pricer { + return recommendations.NewPricer("database", func(context.Context, string, string, int) (recommendations.ReservationPrice, error) { + return recommendations.ReservationPrice{Total: 1000, Currency: "USD"}, nil + }) +} + +// fleetClient returns a client whose subscription has the given managed +// instances (one ZoneRedundant flag each); the same client serves the +// recommendation and the held-reservation lookup, as in production. +func fleetClient(zoneRedundant ...bool) *Client { + c := NewClient(nil, "sub", "eastus") + c.SetCapabilitiesClient(&MockCapabilitiesClient{}) + mi := &MockSQLManagedInstancesPager{} + if len(zoneRedundant) > 0 { + mi.pages = []armsql.ManagedInstancesClientListResponse{buildMIPage(zoneRedundant...)} + } + c.SetManagedInstancesPager(mi) + c.SetServersPager(&MockSQLServersPager{}) + return c +} + +// upfrontRec runs the real converter (including the AZConfig enrichment) and +// variant expansion for a count-2 P1Y recommendation and returns its upfront +// variant. +func upfrontRec(t *testing.T, c *Client) common.Recommendation { + t.Helper() + raw := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), mocks.WithTerm("P1Y"), mocks.WithQuantity(2), + mocks.WithNormalizedSize(sqlSKU), mocks.WithCosts(1000, 700, 300)) + recs, err := c.appendPage(context.Background(), testPricer(), nil, + []armconsumption.ReservationRecommendationClassification{raw}) + require.NoError(t, err) + require.Len(t, recs, 2) + upfront := recs[0] + require.Equal(t, "upfront", upfront.PaymentOption) + require.Equal(t, 2, upfront.Count) + require.Equal(t, "sqlserver", common.EngineFromDetails(upfront.Details)) + return upfront +} + +// purchasedRow buys rec through a capturing transport and builds the held +// reservation from the request body CUDly actually sent. +func purchasedRow(t *testing.T, region string, rec common.Recommendation, qty int32) *armreservations.ReservationResponse { + t.Helper() + capture := &reservationstest.Capture{} + buyer := NewClientWithHTTP(&fake.TokenCredential{}, "sub", region, 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"` + Properties struct { + ReservedResourceType string `json:"reservedResourceType"` + Quantity int32 `json:"quantity"` + } `json:"properties"` + } + require.NoError(t, json.Unmarshal(capture.BodyBytes, &body)) + require.Equal(t, rec.Region, body.Location) + require.Equal(t, rec.Count, int(body.Properties.Quantity)) + return reservationstest.LiveRow(armreservations.ReservedResourceType(body.Properties.ReservedResourceType), + "sub", "held", body.SKU.Name, body.Location, qty) +} + +func adjust(t *testing.T, c *Client, recs ...common.Recommendation) ([]common.Recommendation, error) { + t.Helper() + passed, _, err := recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, c) + return passed, err +} + +func TestHeldReservation_PartialHoldReducesRecommendation(t *testing.T) { + t.Parallel() + c := fleetClient() + upfront := upfrontRec(t, c) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 1))) + + passed, err := adjust(t, c, 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_FullHoldFiltersRecommendation(t *testing.T) { + t.Parallel() + c := fleetClient() + upfront := upfrontRec(t, c) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 2))) + + passed, err := adjust(t, c, 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 } + } + 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, ""}, + {"creating", state(armreservations.ProvisioningStateCreating), 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("GP_Gen5_4") }, 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 := fleetClient() + upfront := upfrontRec(t, c) + row := purchasedRow(t, c.region, upfront, 1) + tc.mutate(row) + c.SetInventoryFactories(factoriesRows(nil, row)) + passed, err := adjust(t, c, 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) + }) + } +} + +// The key carries the subscription's dominant zone-redundancy, which the held +// reservation must match exactly or the guard fails open. +func TestHeldReservation_FleetShapes(t *testing.T) { + t.Parallel() + for name, tc := range map[string]struct { + fleet []bool + az string + }{ + "no managed instances": {nil, ""}, + "all zone-redundant": {[]bool{true, true}, "zoneRedundant"}, + "none zone-redundant": {[]bool{false}, "none"}, + "mixed fleet": {[]bool{true, false}, ""}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + c := fleetClient(tc.fleet...) + upfront := upfrontRec(t, c) + require.Equal(t, tc.az, common.DeploymentFromDetails(upfront.Details), "the rec key's deployment") + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 1))) + + held, err := c.GetExistingCommitments(context.Background()) + require.NoError(t, err) + require.Len(t, held, 1) + assert.Equal(t, tc.az, held[0].Deployment) + + passed, err := adjust(t, c, upfront) + require.NoError(t, err) + require.Len(t, passed, 1) + assert.Equal(t, 1, passed[0].Count, "the hold must match the key whatever the fleet shape") + }) + } +} From c5160206930aa357f8032087dd7a3607753538c3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:51:05 +0100 Subject: [PATCH 15/21] test(azure): share reservation cap-test pagers and pin purchase region Move the counting and cancel-on-second-call reservation pagers into reservationstest for cosmosdb and synapse, and build the held-reservation buyer with the recommending client region so the location assertion can fail. Part of #705 Co-Authored-By: Claude Opus 5.5 --- .../cosmosdb/held_reservation_dedupe_test.go | 13 ++--- .../services/cosmosdb/pagination_cap_test.go | 49 +++++-------------- .../synapse/held_reservation_dedupe_test.go | 13 ++--- .../services/synapse/pagination_cap_test.go | 48 +++++------------- 4 files changed, 36 insertions(+), 87 deletions(-) diff --git a/providers/azure/services/cosmosdb/held_reservation_dedupe_test.go b/providers/azure/services/cosmosdb/held_reservation_dedupe_test.go index d7dded61..60031660 100644 --- a/providers/azure/services/cosmosdb/held_reservation_dedupe_test.go +++ b/providers/azure/services/cosmosdb/held_reservation_dedupe_test.go @@ -57,11 +57,12 @@ func upfrontRec(t *testing.T, c *Client) common.Recommendation { } // purchasedRow buys rec through a capturing transport and builds the held -// reservation from the request body CUDly actually sent. -func purchasedRow(t *testing.T, rec common.Recommendation, qty int32) *armreservations.ReservationResponse { +// reservation from the request body CUDly actually sent. region is the +// recommending client's, so the location assertion can fail. +func purchasedRow(t *testing.T, region string, rec common.Recommendation, qty int32) *armreservations.ReservationResponse { t.Helper() capture := &reservationstest.Capture{} - buyer := NewClientWithHTTP(&fake.TokenCredential{}, "sub", rec.Region, capture) + buyer := NewClientWithHTTP(&fake.TokenCredential{}, "sub", region, capture) _, err := buyer.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) var body struct { @@ -91,7 +92,7 @@ func TestHeldReservation_PartialHoldReducesRecommendation(t *testing.T) { t.Parallel() c := NewClient(nil, "sub", "eastus") upfront := upfrontRec(t, c) - c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, upfront, 1))) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 1))) passed, err := adjust(t, c, upfront) require.NoError(t, err) @@ -104,7 +105,7 @@ func TestHeldReservation_FullHoldFiltersRecommendation(t *testing.T) { t.Parallel() c := NewClient(nil, "sub", "eastus") upfront := upfrontRec(t, c) - c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, upfront, 2))) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 2))) passed, err := adjust(t, c, upfront) require.NoError(t, err) @@ -143,7 +144,7 @@ func TestHeldReservation_Table(t *testing.T) { t.Parallel() c := NewClient(nil, "sub", "eastus") upfront := upfrontRec(t, c) - row := purchasedRow(t, upfront, 1) + row := purchasedRow(t, c.region, upfront, 1) tc.mutate(row) c.SetInventoryFactories(factoriesRows(nil, row)) passed, err := adjust(t, c, upfront) diff --git a/providers/azure/services/cosmosdb/pagination_cap_test.go b/providers/azure/services/cosmosdb/pagination_cap_test.go index c7d55b5f..c4011849 100644 --- a/providers/azure/services/cosmosdb/pagination_cap_test.go +++ b/providers/azure/services/cosmosdb/pagination_cap_test.go @@ -5,11 +5,11 @@ import ( "testing" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cosmos/armcosmos/v2" - "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" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" ) // countingAccountsPager reports More() until limit pages were requested, so a @@ -76,84 +76,57 @@ func TestFetchDominantAPIType_ExactlyCapPagesWithPartialLastPageSucceeds(t *test assert.Equal(t, maxAccountsPages, pager.calls) } -// countingReservationsPager reports More() until limit pages were requested, -// so a missing cap shows up as calls == limit instead of a hung test. -type countingReservationsPager struct { - calls, limit int -} - -func (p *countingReservationsPager) More() bool { return p.calls < p.limit } -func (p *countingReservationsPager) NextPage(_ context.Context) (armreservations.ReservationClientListResponse, error) { - p.calls++ - return armreservations.ReservationClientListResponse{}, nil -} - func TestGetExistingCommitments_PageCapReturnsError(t *testing.T) { t.Parallel() - pager := &countingReservationsPager{limit: 1000} + pager := &reservationstest.CountingPager{Limit: 1000} client := NewClient(nil, "test-subscription", "eastus") client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) _, err := client.GetExistingCommitments(context.Background()) require.Error(t, err) assert.Contains(t, err.Error(), "pagination cap") - assert.Equal(t, maxReservationsPages, pager.calls) + assert.Equal(t, maxReservationsPages, pager.Calls) } func TestGetExistingCommitments_CancelledContextReturnsContextError(t *testing.T) { t.Parallel() ctx, cancel := context.WithCancel(context.Background()) cancel() - pager := &countingReservationsPager{limit: 1000} + pager := &reservationstest.CountingPager{Limit: 1000} client := NewClient(nil, "test-subscription", "eastus") client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) _, err := client.GetExistingCommitments(ctx) require.ErrorIs(t, err, context.Canceled) - assert.Zero(t, pager.calls) + assert.Zero(t, pager.Calls) } func TestGetExistingCommitments_CapIsExact(t *testing.T) { t.Parallel() - atCap := &countingReservationsPager{limit: maxReservationsPages} + atCap := &reservationstest.CountingPager{Limit: maxReservationsPages} client := NewClient(nil, "test-subscription", "eastus") 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) + assert.Equal(t, maxReservationsPages, atCap.Calls) - overCap := &countingReservationsPager{limit: maxReservationsPages + 1} + overCap := &reservationstest.CountingPager{Limit: maxReservationsPages + 1} client = NewClient(nil, "test-subscription", "eastus") client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return overCap })) _, err = client.GetExistingCommitments(context.Background()) require.Error(t, err) - assert.Equal(t, maxReservationsPages, overCap.calls) -} - -// cancelOnSecondCallPager cancels the context while serving its second page. -type cancelOnSecondCallPager struct { - calls int - cancel context.CancelFunc -} - -func (p *cancelOnSecondCallPager) More() bool { return true } -func (p *cancelOnSecondCallPager) NextPage(_ context.Context) (armreservations.ReservationClientListResponse, error) { - p.calls++ - if p.calls == 2 { - p.cancel() - } - return armreservations.ReservationClientListResponse{}, nil + assert.Equal(t, maxReservationsPages, overCap.Calls) } func TestGetExistingCommitments_CancelMidWalkReturnsContextError(t *testing.T) { t.Parallel() ctx, cancel := context.WithCancel(context.Background()) defer cancel() - pager := &cancelOnSecondCallPager{cancel: cancel} + pager := &reservationstest.CancelOnSecondCallPager{Cancel: cancel} client := NewClient(nil, "test-subscription", "eastus") client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) _, err := client.GetExistingCommitments(ctx) require.ErrorIs(t, err, context.Canceled) - assert.Equal(t, 2, pager.calls) + assert.Equal(t, 2, pager.Calls) } diff --git a/providers/azure/services/synapse/held_reservation_dedupe_test.go b/providers/azure/services/synapse/held_reservation_dedupe_test.go index 9e93b97a..27c84914 100644 --- a/providers/azure/services/synapse/held_reservation_dedupe_test.go +++ b/providers/azure/services/synapse/held_reservation_dedupe_test.go @@ -57,11 +57,12 @@ func upfrontRec(t *testing.T, c *Client) common.Recommendation { } // purchasedRow buys rec through a capturing transport and builds the held -// reservation from the request body CUDly actually sent. -func purchasedRow(t *testing.T, rec common.Recommendation, qty int32) *armreservations.ReservationResponse { +// reservation from the request body CUDly actually sent. region is the +// recommending client's, so the location assertion can fail. +func purchasedRow(t *testing.T, region string, rec common.Recommendation, qty int32) *armreservations.ReservationResponse { t.Helper() capture := &reservationstest.Capture{} - buyer := NewClientWithHTTP(&fake.TokenCredential{}, "sub", rec.Region, capture) + buyer := NewClientWithHTTP(&fake.TokenCredential{}, "sub", region, capture) _, err := buyer.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) var body struct { @@ -91,7 +92,7 @@ func TestHeldReservation_PartialHoldReducesRecommendation(t *testing.T) { t.Parallel() c := NewClient(nil, "sub", "eastus") upfront := upfrontRec(t, c) - c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, upfront, 1))) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 1))) passed, err := adjust(t, c, upfront) require.NoError(t, err) @@ -104,7 +105,7 @@ func TestHeldReservation_FullHoldFiltersRecommendation(t *testing.T) { t.Parallel() c := NewClient(nil, "sub", "eastus") upfront := upfrontRec(t, c) - c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, upfront, 2))) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 2))) passed, err := adjust(t, c, upfront) require.NoError(t, err) @@ -143,7 +144,7 @@ func TestHeldReservation_Table(t *testing.T) { t.Parallel() c := NewClient(nil, "sub", "eastus") upfront := upfrontRec(t, c) - row := purchasedRow(t, upfront, 1) + row := purchasedRow(t, c.region, upfront, 1) tc.mutate(row) c.SetInventoryFactories(factoriesRows(nil, row)) passed, err := adjust(t, c, upfront) diff --git a/providers/azure/services/synapse/pagination_cap_test.go b/providers/azure/services/synapse/pagination_cap_test.go index 7828971b..128d1171 100644 --- a/providers/azure/services/synapse/pagination_cap_test.go +++ b/providers/azure/services/synapse/pagination_cap_test.go @@ -10,6 +10,7 @@ import ( "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" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations/reservationstest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -91,86 +92,59 @@ func TestGetRecommendations_ExactlyCapPagesWithPartialLastPageSucceeds(t *testin assert.Equal(t, maxRecsPages, pager.calls) } -// countingReservationsPager reports More() until limit pages were requested, -// so a missing cap shows up as calls == limit instead of a hung test. -type countingReservationsPager struct { - calls, limit int -} - -func (p *countingReservationsPager) More() bool { return p.calls < p.limit } -func (p *countingReservationsPager) NextPage(_ context.Context) (armreservations.ReservationClientListResponse, error) { - p.calls++ - return armreservations.ReservationClientListResponse{}, nil -} - func TestGetExistingCommitments_PageCapReturnsError(t *testing.T) { t.Parallel() - pager := &countingReservationsPager{limit: 1000} + pager := &reservationstest.CountingPager{Limit: 1000} client := NewClient(nil, "test-subscription", "eastus") client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) _, err := client.GetExistingCommitments(context.Background()) require.Error(t, err) assert.Contains(t, err.Error(), "pagination cap") - assert.Equal(t, maxReservationsPages, pager.calls) + assert.Equal(t, maxReservationsPages, pager.Calls) } func TestGetExistingCommitments_CancelledContextReturnsContextError(t *testing.T) { t.Parallel() ctx, cancel := context.WithCancel(context.Background()) cancel() - pager := &countingReservationsPager{limit: 1000} + pager := &reservationstest.CountingPager{Limit: 1000} client := NewClient(nil, "test-subscription", "eastus") client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) _, err := client.GetExistingCommitments(ctx) require.ErrorIs(t, err, context.Canceled) - assert.Zero(t, pager.calls) + assert.Zero(t, pager.Calls) } func TestGetExistingCommitments_CapIsExact(t *testing.T) { t.Parallel() - atCap := &countingReservationsPager{limit: maxReservationsPages} + atCap := &reservationstest.CountingPager{Limit: maxReservationsPages} client := NewClient(nil, "test-subscription", "eastus") 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) + assert.Equal(t, maxReservationsPages, atCap.Calls) - overCap := &countingReservationsPager{limit: maxReservationsPages + 1} + overCap := &reservationstest.CountingPager{Limit: maxReservationsPages + 1} client = NewClient(nil, "test-subscription", "eastus") client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return overCap })) _, err = client.GetExistingCommitments(context.Background()) require.Error(t, err) - assert.Equal(t, maxReservationsPages, overCap.calls) -} - -// cancelOnSecondCallPager cancels the context while serving its second page. -type cancelOnSecondCallPager struct { - calls int - cancel context.CancelFunc -} - -func (p *cancelOnSecondCallPager) More() bool { return true } -func (p *cancelOnSecondCallPager) NextPage(_ context.Context) (armreservations.ReservationClientListResponse, error) { - p.calls++ - if p.calls == 2 { - p.cancel() - } - return armreservations.ReservationClientListResponse{}, nil + assert.Equal(t, maxReservationsPages, overCap.Calls) } func TestGetExistingCommitments_CancelMidWalkReturnsContextError(t *testing.T) { t.Parallel() ctx, cancel := context.WithCancel(context.Background()) defer cancel() - pager := &cancelOnSecondCallPager{cancel: cancel} + pager := &reservationstest.CancelOnSecondCallPager{Cancel: cancel} client := NewClient(nil, "test-subscription", "eastus") client.SetInventoryFactories(factoriesFor(func() reservations.OrderReservationsPager { return pager })) _, err := client.GetExistingCommitments(ctx) require.ErrorIs(t, err, context.Canceled) - assert.Equal(t, 2, pager.calls) + assert.Equal(t, 2, pager.Calls) } // finalPartialReservationsPager serves exactly pages pages: full pages of two From cfc64fc57d679931b2f655e0155a0f0fc14f7660 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 06:02:33 +0100 Subject: [PATCH 16/21] test(azure): tighten SQL Database inventory tests and Deployment doc Give the nil-properties test real server and managed-instance pages so its no-walk assertions can fail, add a managed-instance walk failure row to the held-reservation fleet shapes, and document Commitment.Deployment for Azure SQL. Part of #705 Co-Authored-By: Claude Opus 5.5 --- pkg/common/types.go | 14 +++++++------- providers/azure/services/database/client_test.go | 2 ++ .../database/held_reservation_dedupe_test.go | 14 ++++++++++---- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/pkg/common/types.go b/pkg/common/types.go index 942f2f60..d6e53ad5 100644 --- a/pkg/common/types.go +++ b/pkg/common/types.go @@ -527,12 +527,12 @@ const ( // Commitment represents an existing commitment (RI/SP/CUD/etc). // -// Deployment is RDS-specific (Multi-AZ vs Single-AZ); it stays empty for -// services that don't have a deployment dimension (EC2, ElastiCache, -// etc). When populated, it carries the same vocabulary as -// DatabaseDetails.AZConfig on Recommendation ("single-az" / "multi-az") -// so pool-key matching can collapse both sides via normaliseDeployment -// in the recommendations package. Without this field, RDS expiry +// Deployment is set for AWS RDS and Azure SQL Database and stays empty for +// services without a deployment dimension (EC2, ElastiCache, etc). When +// populated, it carries the same vocabulary as DatabaseDetails.AZConfig on +// Recommendation ("single-az" / "multi-az" on AWS, "zoneRedundant" / "none" +// on Azure) so pool-key matching can collapse both sides via +// normaliseDeployment in the recommendations package. Without this field, RDS expiry // adjustments would silently miss because Recommendation lookup keys // are deployment-aware while commitment keys defaulted to empty. type Commitment struct { @@ -546,7 +546,7 @@ type Commitment struct { Region string `json:"region"` ResourceType string `json:"resource_type"` Engine string `json:"engine,omitempty"` // Database engine for RDS/ElastiCache (e.g., "mysql", "aurora-postgresql") - Deployment string `json:"deployment,omitempty"` // RDS Multi-AZ vs Single-AZ; empty for non-RDS + Deployment string `json:"deployment,omitempty"` // RDS Multi-AZ vs Single-AZ, Azure SQL zone redundancy; empty otherwise State CommitmentState `json:"state"` // EC2 reserved-instance dimensions. Empty means unknown or a wildcard. diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index b711ebb2..2cbf5f3b 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -746,6 +746,8 @@ func TestGetExistingCommitments_NonSQLFiltered(t *testing.T) { func TestGetExistingCommitments_NilPropertiesFailsClosed(t *testing.T) { t.Parallel() c, mi, servers := inventoryClient(&armreservations.ReservationResponse{}) + mi.pages = []armsql.ManagedInstancesClientListResponse{buildMIPage(true)} + servers.pages = []armsql.ServersClientListResponse{buildServerPage(1)} _, err := c.GetExistingCommitments(context.Background()) require.Error(t, err) assert.Zero(t, mi.CallCount) diff --git a/providers/azure/services/database/held_reservation_dedupe_test.go b/providers/azure/services/database/held_reservation_dedupe_test.go index 43aff434..ae392e92 100644 --- a/providers/azure/services/database/held_reservation_dedupe_test.go +++ b/providers/azure/services/database/held_reservation_dedupe_test.go @@ -3,6 +3,7 @@ package database import ( "context" "encoding/json" + "errors" "testing" "time" @@ -178,16 +179,21 @@ func TestHeldReservation_FleetShapes(t *testing.T) { t.Parallel() for name, tc := range map[string]struct { fleet []bool + miErr error az string }{ - "no managed instances": {nil, ""}, - "all zone-redundant": {[]bool{true, true}, "zoneRedundant"}, - "none zone-redundant": {[]bool{false}, "none"}, - "mixed fleet": {[]bool{true, false}, ""}, + "no managed instances": {nil, nil, ""}, + "all zone-redundant": {[]bool{true, true}, nil, "zoneRedundant"}, + "none zone-redundant": {[]bool{false}, nil, "none"}, + "mixed fleet": {[]bool{true, false}, nil, ""}, + "managed-instance walk fails": {nil, errors.New("mi list failed"), ""}, } { t.Run(name, func(t *testing.T) { t.Parallel() c := fleetClient(tc.fleet...) + if tc.miErr != nil { + c.SetManagedInstancesPager(&MockSQLManagedInstancesPager{err: tc.miErr}) + } upfront := upfrontRec(t, c) require.Equal(t, tc.az, common.DeploymentFromDetails(upfront.Details), "the rec key's deployment") c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 1))) From 093bce11835b06a6ff41169a2de1e8e19a69fa55 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 06:04:59 +0100 Subject: [PATCH 17/21] test(azure): make the SQL fleet walk-failure case actually fail the walk The failing managed-instance pager had no pages, so NextPage never ran and the row duplicated "no managed instances". Give it a page and assert it was called. Part of #705 Co-Authored-By: Claude Opus 5.5 --- .../services/database/held_reservation_dedupe_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/providers/azure/services/database/held_reservation_dedupe_test.go b/providers/azure/services/database/held_reservation_dedupe_test.go index ae392e92..310f5349 100644 --- a/providers/azure/services/database/held_reservation_dedupe_test.go +++ b/providers/azure/services/database/held_reservation_dedupe_test.go @@ -191,8 +191,10 @@ func TestHeldReservation_FleetShapes(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() c := fleetClient(tc.fleet...) + var failing *MockSQLManagedInstancesPager if tc.miErr != nil { - c.SetManagedInstancesPager(&MockSQLManagedInstancesPager{err: tc.miErr}) + failing = &MockSQLManagedInstancesPager{err: tc.miErr, pages: []armsql.ManagedInstancesClientListResponse{buildMIPage(true)}} + c.SetManagedInstancesPager(failing) } upfront := upfrontRec(t, c) require.Equal(t, tc.az, common.DeploymentFromDetails(upfront.Details), "the rec key's deployment") @@ -207,6 +209,9 @@ func TestHeldReservation_FleetShapes(t *testing.T) { require.NoError(t, err) require.Len(t, passed, 1) assert.Equal(t, 1, passed[0].Count, "the hold must match the key whatever the fleet shape") + if failing != nil { + assert.Positive(t, failing.CallCount, "the failing walk must actually run") + } }) } } From 125cbb01d1fdac080ac7917ead005b38690230b8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 06:17:43 +0100 Subject: [PATCH 18/21] style(azure): align reservation test fixture structs Reorder OrderPager and CancelOnSecondCallPager fields to satisfy govet fieldalignment. Part of #705 Co-Authored-By: Claude Opus 5.5 --- .../internal/reservations/reservationstest/inventory.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/providers/azure/services/internal/reservations/reservationstest/inventory.go b/providers/azure/services/internal/reservations/reservationstest/inventory.go index 2123bc34..352a3cb6 100644 --- a/providers/azure/services/internal/reservations/reservationstest/inventory.go +++ b/providers/azure/services/internal/reservations/reservationstest/inventory.go @@ -56,8 +56,8 @@ func (l AppliedLister) GetAppliedReservationList(context.Context, string, *armre // OrderPager is a single-page pager over Rows, or Err on the page fetch. type OrderPager struct { - Rows []*armreservations.ReservationResponse Err error + Rows []*armreservations.ReservationResponse done bool } @@ -87,8 +87,8 @@ func (p *CountingPager) NextPage(context.Context) (armreservations.ReservationCl // CancelOnSecondCallPager never ends and calls Cancel while serving its // second page. type CancelOnSecondCallPager struct { - Calls int Cancel context.CancelFunc + Calls int } func (p *CancelOnSecondCallPager) More() bool { return true } From 25d083ba2ffc6961f6dc3e06b576aec40b51d01a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 06:30:22 +0100 Subject: [PATCH 19/21] fix(azure): return cancellation from SQL Database inventory enrichment A context canceled during the managed-instance or server walk returned the held reservations stamped with a blank Deployment and a nil error. Return the cancellation so the duplicate checker sees the failure. Part of #705 Co-Authored-By: Claude Opus 5.5 --- providers/azure/services/database/client.go | 3 +++ .../azure/services/database/client_test.go | 17 +++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index e777723d..fb84b9c3 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -292,6 +292,9 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen return commitments, err } az := c.cachedDominantAZConfig(ctx) + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("database: enrich held reservations: %w", err) + } for i := range commitments { commitments[i].Engine = sqlServerEngine commitments[i].Deployment = az diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index 2cbf5f3b..a384dc20 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -754,6 +754,23 @@ func TestGetExistingCommitments_NilPropertiesFailsClosed(t *testing.T) { assert.Zero(t, servers.CallCount) } +// A cancellation during the enrichment walk must surface, not return +// inventory stamped with a blank deployment. +func TestGetExistingCommitments_CancelDuringEnrichmentWalk(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + c, _, servers := inventoryClient(sqlRow("res-1", "GP_Gen5_2", "eastus", 1)) + c.SetManagedInstancesPager(&cancelAfterPagesMIPager{ + MockSQLManagedInstancesPager: MockSQLManagedInstancesPager{pages: []armsql.ManagedInstancesClientListResponse{buildMIPage(true)}}, + cancel: cancel, + }) + servers.pages = []armsql.ServersClientListResponse{buildServerPage(1)} + commitments, err := c.GetExistingCommitments(ctx) + require.ErrorIs(t, err, context.Canceled) + assert.Nil(t, commitments) +} + func TestGetExistingCommitments_PagerError(t *testing.T) { t.Parallel() c, _, _ := inventoryClient() From c7d5ac52096ab10b56dd0fa733d21da3b66c2334 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 06:31:53 +0100 Subject: [PATCH 20/21] fix(azure): stamp SQL Database held reservations with the recommendations' zone-redundancy snapshot If the walk failed while keying recommendations and succeeded later for the inventory, held reservations got a different Deployment than the keys and the duplicate guard failed open. Record the AZConfig the recommendation path stamped and reuse it for held reservations on the same client. Part of #705 Co-Authored-By: Claude Opus 5.5 --- providers/azure/services/database/client.go | 27 +++++++++++++++++-- .../database/held_reservation_dedupe_test.go | 24 +++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index fb84b9c3..a6f0c472 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -134,6 +134,12 @@ type Client struct { // persistently failing subscription is not re-walked per recommendation. serverInfoMu sync.Mutex serverInfoLoaded bool + + // AZConfig the recommendation path last stamped on a recommendation, + // so held reservations are keyed identically even if a later walk + // answers differently. Guarded by serverInfoMu. + recAZConfig string + recAZConfigSet bool } // serverInfoRetryDelayDefault is the minimum wait before re-walking the @@ -291,7 +297,10 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen if err != nil || len(commitments) == 0 { return commitments, err } - az := c.cachedDominantAZConfig(ctx) + az, ok := c.recommendationAZConfig() + if !ok { + az = c.cachedDominantAZConfig(ctx) + } if err := ctx.Err(); err != nil { return nil, fmt.Errorf("database: enrich held reservations: %w", err) } @@ -621,7 +630,9 @@ func (c *Client) convertAzureSQLRecommendation(ctx context.Context, azureRec arm if entry, ok := c.cachedSKULookup(ctx, f.ResourceType); ok && entry.engineVersion != "" { details.EngineVersion = entry.engineVersion } - if az := c.cachedDominantAZConfig(ctx); az != "" { + az := c.cachedDominantAZConfig(ctx) + c.recordRecommendationAZConfig(az) + if az != "" { details.AZConfig = az } if dep := c.cachedDominantDeployment(ctx); dep != "" { @@ -730,6 +741,18 @@ func (c *Client) cachedDominantAZConfig(ctx context.Context) string { return az } +func (c *Client) recordRecommendationAZConfig(az string) { + c.serverInfoMu.Lock() + defer c.serverInfoMu.Unlock() + c.recAZConfig, c.recAZConfigSet = az, true +} + +func (c *Client) recommendationAZConfig() (string, bool) { + c.serverInfoMu.Lock() + defer c.serverInfoMu.Unlock() + return c.recAZConfig, c.recAZConfigSet +} + // cachedDominantDeployment returns the dominant Deployment observed // across the subscription's SQL resources, or "" when ambiguous or // unavailable. Values: "managed" / "single". Shares the single diff --git a/providers/azure/services/database/held_reservation_dedupe_test.go b/providers/azure/services/database/held_reservation_dedupe_test.go index 310f5349..6a109c3d 100644 --- a/providers/azure/services/database/held_reservation_dedupe_test.go +++ b/providers/azure/services/database/held_reservation_dedupe_test.go @@ -215,3 +215,27 @@ func TestHeldReservation_FleetShapes(t *testing.T) { }) } } + +// A walk that fails on the recommendation call and succeeds on the inventory +// call must not give the held reservation a different deployment than the key +// the recommendation already carries. +func TestHeldReservation_UsesRecommendationsAZSnapshot(t *testing.T) { + t.Parallel() + c := fleetClient() + c.SetManagedInstancesPager(&flakyMIPager{failures: 1}) + c.serverInfoRetryDelay = 10 * time.Millisecond + upfront := upfrontRec(t, c) + require.Empty(t, common.DeploymentFromDetails(upfront.Details), "the rec was keyed while the walk failed") + time.Sleep(30 * time.Millisecond) + c.SetInventoryFactories(factoriesRows(nil, purchasedRow(t, c.region, upfront, 1))) + + held, err := c.GetExistingCommitments(context.Background()) + require.NoError(t, err) + require.Len(t, held, 1) + assert.Empty(t, held[0].Deployment, "the held reservation must reuse the rec's snapshot") + + passed, err := adjust(t, c, upfront) + require.NoError(t, err) + require.Len(t, passed, 1) + assert.Equal(t, 1, passed[0].Count) +} From 6a34025bac0ccbe41cf030f33528b9be57f8e591 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 06:32:43 +0100 Subject: [PATCH 21/21] style(azure): align SQL Database client struct fields Satisfy govet fieldalignment after adding the recommendation AZConfig snapshot fields. Part of #705 Co-Authored-By: Claude Opus 5.5 --- providers/azure/services/database/client.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index a6f0c472..346772c3 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -115,8 +115,12 @@ type Client struct { // Only a complete walk is cached (serverInfoLoaded); a failed or // canceled walk is retried, but not before serverInfoRetryAfter so a // persistently failing subscription is not re-walked per recommendation. - azConfig string - deployment string + azConfig string + deployment string + // recAZConfig is the AZConfig the recommendation path last stamped, so + // held reservations are keyed identically even if a later walk answers + // differently. Guarded by serverInfoMu. + recAZConfig string serverInfoRetryDelay time.Duration // Lazy SKU catalog cache. Populated once on the first @@ -134,12 +138,7 @@ type Client struct { // persistently failing subscription is not re-walked per recommendation. serverInfoMu sync.Mutex serverInfoLoaded bool - - // AZConfig the recommendation path last stamped on a recommendation, - // so held reservations are keyed identically even if a later walk - // answers differently. Guarded by serverInfoMu. - recAZConfig string - recAZConfigSet bool + recAZConfigSet bool } // serverInfoRetryDelayDefault is the minimum wait before re-walking the