From b0a144ad02d1232cc3c3e7862e67188122b896a1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 08:42:55 +0100 Subject: [PATCH 1/2] fix(ladder): hold the buffer purchase when its layer has a pending reshape A run could emit a buffer-layer purchase and a RESHAPE recommendation for the same layer. buildResult now converts a buffer-only layer's allocation into an informational Hold when that layer has a reshape, so the run buys less instead of adding to an under-utilized layer. Layers that also carry the base or flex role keep their allocation. Closes #30 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + pkg/ladder/allocate.go | 42 ++++++++++++++- pkg/ladder/allocate_test.go | 101 +++++++++++++++++++++++++++++++++--- 3 files changed, 136 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d80102e9..1c39579b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -223,6 +223,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed - **`gcp`:** the Cloud SQL, Memorystore and Cloud Storage price extractors keep 1-year and 3-year commitment SKUs in separate slots and return the price for the requested term, so a catalog that lists both terms no longer fails as ambiguous (#254). Two SKUs of the same term at different prices still fail closed, and a recommendation term other than 1yr or 3yr (for example "2yr") now errors instead of being priced as one year. +- **`ladder`:** a run no longer emits a buffer-layer purchase for a layer it also recommends reshaping. The buffer allocation becomes an informational Hold naming the pending reshape; the reshape and all other allocations are unchanged. A layer that also carries the base or flex role (merged Azure reservation) is not held (#30). - **`azure`:** recommendations from the Consumption path (compute, database, cache, cosmosdb, managedredis, synapse) and the Savings Plan benefit path now set `CommitmentCurrency` to `USD`, because their commitment amounts come from sources already required to be USD. The Advisor and legacy paths carry no checked commitment amounts and stay empty (#623). - **AWS/OpenSearch:** the STS account-ID lookup used for post-purchase tagging caches only a successful result, so one throttled or timed-out `GetCallerIdentity` no longer disables tagging for the client's lifetime (#53). - **AWS/recommendations:** `normalizeRegionName` maps the current `Europe (Ireland|Frankfurt|London|Paris|Stockholm)` Cost Explorer display names to region codes, so coverage lookups keyed by region code match those recommendations (#53). diff --git a/pkg/ladder/allocate.go b/pkg/ladder/allocate.go index cbc28678..727c3268 100644 --- a/pkg/ladder/allocate.go +++ b/pkg/ladder/allocate.go @@ -541,6 +541,7 @@ func buildResult(in *AllocationInput, lowWater, target, existingTotal, gap *big. if err != nil { return nil, err } + allocs, reshapeHolds := holdReshapedBufferAllocations(allocs, reshapes, in.Layers, in.DataSources) allocs, rateHolds := convertAllocations(allocs, rates, in.DataSources) allocs, err = applyCapGuardrail(allocs, in.Config.MaxHourlyCommitPerRun) if err != nil { @@ -548,8 +549,9 @@ func buildResult(in *AllocationInput, lowWater, target, existingTotal, gap *big. } allocs, skipHolds := dropSubMinimumAllocations(allocs, in.DataSources) allocs, reshapes, truncHolds := truncateToMaxActions(allocs, reshapes, in.Config.MaxActionsPerRun, in.LayerStates, in.DataSources) - holds := make([]PlannedAction, 0, len(maintHolds)+len(rateHolds)+len(skipHolds)+len(truncHolds)) + holds := make([]PlannedAction, 0, len(maintHolds)+len(reshapeHolds)+len(rateHolds)+len(skipHolds)+len(truncHolds)) holds = append(holds, maintHolds...) + holds = append(holds, reshapeHolds...) holds = append(holds, rateHolds...) holds = append(holds, skipHolds...) holds = append(holds, truncHolds...) @@ -560,6 +562,44 @@ func buildResult(in *AllocationInput, lowWater, target, existingTotal, gap *big. }, nil } +// holdReshapedBufferAllocations turns the purchase for a buffer layer into an +// informational Hold when the same run recommends reshaping that layer: +// buying more of an under-utilized layer contradicts the reshape. Only +// buffer-only layers are held; a layer that also carries the base or flex role +// (the merged Azure reservation) keeps its allocation because it is not purely +// a buffer purchase. Must run before convertAllocations so GapUSDPerHour is +// only OnDemandEquivalentUSDPerHour is populated. +func holdReshapedBufferAllocations(allocs []Allocation, reshapes []PlannedAction, layers []LayerSpec, dataSources []string) ([]Allocation, []PlannedAction) { + if len(reshapes) == 0 { + return allocs, nil + } + bufferOnly := make(map[LayerType]bool, len(layers)) + for _, ls := range layers { + bufferOnly[ls.Type] = slices.Contains(ls.Roles, RoleBuffer) && + !slices.Contains(ls.Roles, RoleBase) && !slices.Contains(ls.Roles, RoleFlex) + } + reshaped := make(map[LayerType]bool, len(reshapes)) + for _, r := range reshapes { + reshaped[r.Layer] = bufferOnly[r.Layer] + } + var kept []Allocation + var holds []PlannedAction + for _, a := range allocs { + if !reshaped[a.Layer] { + kept = append(kept, a) + continue + } + holds = append(holds, PlannedAction{ + Action: ActionHold, + Layer: a.Layer, + Rationale: fmt.Sprintf("buffer purchase of %s on layer %s withheld: the layer has a pending reshape recommendation, so buying more of an under-utilized layer is deferred", + fmtRatUSD(a.OnDemandEquivalentUSDPerHour), a.Layer), + DataSources: dataSources, + }) + } + return kept, holds +} + // dropSubMinimumAllocations removes allocations whose amount fell below the // minimum allocatable threshold after splitting and cap scaling, emitting an // informational Hold for each so the skipped amount is never silently lost. diff --git a/pkg/ladder/allocate_test.go b/pkg/ladder/allocate_test.go index df0a2302..803765b1 100644 --- a/pkg/ladder/allocate_test.go +++ b/pkg/ladder/allocate_test.go @@ -750,9 +750,10 @@ func TestAllocate_MaxActions_ReshapeRetained(t *testing.T) { t.Errorf("surviving purchase must be the largest-gap layer EC2InstanceSP, got %s", result.Allocations[0].Layer) } - // Exactly the two smaller purchases were dropped with holds. + // ComputeSP was dropped by truncation; the ConvertibleRI buffer purchase + // never reached truncation because its layer has the pending reshape. held := truncatedHoldLayers(result.Holds) - wantHeld := map[LayerType]bool{LayerComputeSP: true, LayerConvertibleRI: true} + wantHeld := map[LayerType]bool{LayerComputeSP: true} if !maps.Equal(held, wantHeld) { t.Errorf("dropped-purchase hold layers = %v, want %v", held, wantHeld) } @@ -765,8 +766,9 @@ func TestAllocate_MaxActions_ReshapeRetained(t *testing.T) { // its layer + amount in a max_actions_per_run hold. // // Fixture matches TestAllocate_MaxActions_ReshapeRetained but with cap=1: the -// single reshape (ConvertibleRI) consumes the only slot, so all three purchases -// (EC2InstanceSP $4/hr, ComputeSP $0.5/hr, ConvertibleRI $0.5/hr) are dropped. +// single reshape (ConvertibleRI) consumes the only slot, so the remaining +// purchases (EC2InstanceSP $4/hr, ComputeSP $0.5/hr) are dropped; the +// ConvertibleRI buffer purchase is held earlier because of the reshape. func TestAllocate_MaxActions_ReshapesFillCap(t *testing.T) { t.Parallel() cfg := validConfigAWS() @@ -805,7 +807,6 @@ func TestAllocate_MaxActions_ReshapesFillCap(t *testing.T) { wantHeld := map[LayerType]bool{ LayerEC2InstanceSP: true, LayerComputeSP: true, - LayerConvertibleRI: true, } if !maps.Equal(held, wantHeld) { t.Errorf("dropped-purchase hold layers = %v, want %v", held, wantHeld) @@ -827,8 +828,9 @@ func TestAllocate_MaxActions_ReshapesFillCap(t *testing.T) { t.Errorf("hold rationale must name the dropped amount: %q", h.Rationale) } } - if truncHolds != 3 { - t.Errorf("want 3 truncation holds (one per dropped purchase), got %d", truncHolds) + // The buffer purchase is held earlier (pending reshape), not truncated. + if truncHolds != 2 { + t.Errorf("want 2 truncation holds (one per dropped purchase), got %d", truncHolds) } } @@ -1917,3 +1919,88 @@ func TestAllocate_NilInFlightRejected(t *testing.T) { t.Errorf("error %q does not name InFlight", err) } } + +func reshapeHoldInput(bufferUtil float64) *AllocationInput { + layers := awsLayers() + states := withBufferUtilization(withExisting(zeroStates(layers), LayerConvertibleRI, 2.0), bufferUtil) + return &AllocationInput{ + Config: validConfigAWS(), + Layers: layers, + Baseline: UsageBaseline{LowWaterUSDPerHour: ptr(10.0), StableUSDPerHour: ptr(4.0)}, + LayerStates: states, + Now: nowFixed(), + InFlight: noInFlight(), CommitmentRates: unitRates(), + } +} + +func hasAllocOn(r *AllocateResult, l LayerType) bool { + for _, a := range r.Allocations { + if a.Layer == l { + return true + } + } + return false +} + +func TestAllocate_BufferHeldWhenLayerHasReshape(t *testing.T) { + t.Parallel() + result, err := Allocate(reshapeHoldInput(70.0)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if hasAllocOn(result, LayerConvertibleRI) { + t.Errorf("buffer purchase must be held, got %+v", result.Allocations) + } + if !hasAllocOn(result, LayerComputeSP) || !hasAllocOn(result, LayerEC2InstanceSP) { + t.Errorf("non-buffer allocations must be unchanged, got %+v", result.Allocations) + } + if len(result.Reshapes) != 1 || result.Reshapes[0].Layer != LayerConvertibleRI { + t.Fatalf("reshape must still appear, got %+v", result.Reshapes) + } + var held bool + for _, h := range result.Holds { + if h.Action == ActionHold && h.Layer == LayerConvertibleRI && strings.Contains(h.Rationale, "pending reshape") { + held = true + } + } + if !held { + t.Errorf("want a buffer Hold naming the pending reshape, got %+v", result.Holds) + } +} + +func TestAllocate_BufferBoughtWhenNoReshape(t *testing.T) { + t.Parallel() + result, err := Allocate(reshapeHoldInput(95.0)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(result.Reshapes) != 0 { + t.Fatalf("want no reshape, got %+v", result.Reshapes) + } + if !hasAllocOn(result, LayerConvertibleRI) { + t.Errorf("buffer must still be bought, got %+v", result.Allocations) + } +} + +// A reshape on another layer must not suppress the buffer purchase. Valid +// topologies allow one buffer layer, so this exercises the helper directly. +func TestHoldReshapedBufferAllocations_OtherLayerUnaffected(t *testing.T) { + t.Parallel() + allocs := []Allocation{{Layer: LayerConvertibleRI, OnDemandEquivalentUSDPerHour: big.NewRat(1, 1)}} + reshapes := []PlannedAction{{Action: ActionReshape, Layer: LayerComputeSP}} + kept, holds := holdReshapedBufferAllocations(allocs, reshapes, awsLayers(), nil) + if len(kept) != 1 || len(holds) != 0 { + t.Errorf("want buffer kept and no holds, got kept=%+v holds=%+v", kept, holds) + } +} + +// The merged Azure reservation also carries the base role; it is not held. +func TestHoldReshapedBufferAllocations_MergedBaseBufferKept(t *testing.T) { + t.Parallel() + allocs := []Allocation{{Layer: LayerAzureReservation, OnDemandEquivalentUSDPerHour: big.NewRat(1, 1)}} + reshapes := []PlannedAction{{Action: ActionReshape, Layer: LayerAzureReservation}} + kept, holds := holdReshapedBufferAllocations(allocs, reshapes, azureLayers(), nil) + if len(kept) != 1 || len(holds) != 0 { + t.Errorf("want merged allocation kept, got kept=%+v holds=%+v", kept, holds) + } +} From d8f389f7f777d95d955e45bf7dc2da044b82037b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 08:47:48 +0100 Subject: [PATCH 2/2] docs(ladder): tidy buffer-hold docstring, drop dead flex check Rewrite the garbled docstring, remove the unreachable flex-role clause (validation rejects flex+buffer) and make the changelog entry accurate about cap and max-actions side effects. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + pkg/ladder/allocate.go | 9 ++++----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c39579b..bde5b9b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -224,6 +224,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - **`gcp`:** the Cloud SQL, Memorystore and Cloud Storage price extractors keep 1-year and 3-year commitment SKUs in separate slots and return the price for the requested term, so a catalog that lists both terms no longer fails as ambiguous (#254). Two SKUs of the same term at different prices still fail closed, and a recommendation term other than 1yr or 3yr (for example "2yr") now errors instead of being priced as one year. - **`ladder`:** a run no longer emits a buffer-layer purchase for a layer it also recommends reshaping. The buffer allocation becomes an informational Hold naming the pending reshape; the reshape and all other allocations are unchanged. A layer that also carries the base or flex role (merged Azure reservation) is not held (#30). +- **`ladder`:** a run no longer emits a buffer-layer purchase for a layer it also recommends reshaping. The buffer allocation becomes an informational Hold naming the pending reshape; the reshape stays. Other allocations are not edited, but with the held buffer removed a binding `MaxHourlyCommitPerRun` can give base/flex a larger share of the cap (total spend never higher), and a freed max-actions slot can let a deferred purchase through. A layer that also carries the base role (merged Azure reservation) is not held (#30). - **`azure`:** recommendations from the Consumption path (compute, database, cache, cosmosdb, managedredis, synapse) and the Savings Plan benefit path now set `CommitmentCurrency` to `USD`, because their commitment amounts come from sources already required to be USD. The Advisor and legacy paths carry no checked commitment amounts and stay empty (#623). - **AWS/OpenSearch:** the STS account-ID lookup used for post-purchase tagging caches only a successful result, so one throttled or timed-out `GetCallerIdentity` no longer disables tagging for the client's lifetime (#53). - **AWS/recommendations:** `normalizeRegionName` maps the current `Europe (Ireland|Frankfurt|London|Paris|Stockholm)` Cost Explorer display names to region codes, so coverage lookups keyed by region code match those recommendations (#53). diff --git a/pkg/ladder/allocate.go b/pkg/ladder/allocate.go index 727c3268..76e5a20c 100644 --- a/pkg/ladder/allocate.go +++ b/pkg/ladder/allocate.go @@ -565,18 +565,17 @@ func buildResult(in *AllocationInput, lowWater, target, existingTotal, gap *big. // holdReshapedBufferAllocations turns the purchase for a buffer layer into an // informational Hold when the same run recommends reshaping that layer: // buying more of an under-utilized layer contradicts the reshape. Only -// buffer-only layers are held; a layer that also carries the base or flex role +// buffer-only layers are held; a layer that also carries the base role // (the merged Azure reservation) keeps its allocation because it is not purely -// a buffer purchase. Must run before convertAllocations so GapUSDPerHour is -// only OnDemandEquivalentUSDPerHour is populated. +// a buffer purchase. Must run before convertAllocations, while only +// OnDemandEquivalentUSDPerHour is populated. func holdReshapedBufferAllocations(allocs []Allocation, reshapes []PlannedAction, layers []LayerSpec, dataSources []string) ([]Allocation, []PlannedAction) { if len(reshapes) == 0 { return allocs, nil } bufferOnly := make(map[LayerType]bool, len(layers)) for _, ls := range layers { - bufferOnly[ls.Type] = slices.Contains(ls.Roles, RoleBuffer) && - !slices.Contains(ls.Roles, RoleBase) && !slices.Contains(ls.Roles, RoleFlex) + bufferOnly[ls.Type] = slices.Contains(ls.Roles, RoleBuffer) && !slices.Contains(ls.Roles, RoleBase) } reshaped := make(map[LayerType]bool, len(reshapes)) for _, r := range reshapes {