From e669581d2e019c4575bb71bec2c909ffd3cd7e5f Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 04:39:37 +0000 Subject: [PATCH] feat(ladder): report the net existing commitment Allocate sized the gap against Allocate sizes the purchase gap against net existing commitment (existing minus expiring), but the result did not expose that figure, so callers recomputed a gross one from the layer states and a plan's existing figure did not reconcile with its gap. AllocateResult now carries NetExistingUSDPerHour on every successful result, including the hold-only early exits. Net existing is computed right after validation, before the baseline-unavailable hold, so an inconsistent snapshot (expiring above existing) is now an error on that path too instead of a hold. Also correct the BufferReshapeConfig cap docs: since #605 a nil cap is rejected, not treated as "no cap". Refs #83 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 7 ++ pkg/ladder/allocate.go | 40 ++++++-- pkg/ladder/allocate_net_existing_test.go | 120 +++++++++++++++++++++++ pkg/ladder/capability.go | 7 +- 4 files changed, 163 insertions(+), 11 deletions(-) create mode 100644 pkg/ladder/allocate_net_existing_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 93b1a002..f29383e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Added +- **`ladder.AllocateResult.NetExistingUSDPerHour`:** the net existing + commitment (existing minus expiring, summed over layers) that `Allocate` + sized the gap against, set on every successful result including the + hold-only exits. Callers can report it instead of recomputing a gross + figure from the layer states, so a plan's existing figure reconciles with + its gap. `Allocate` now checks the snapshot before the baseline-unavailable + hold, so expiring above existing is an error there too (#83). - **`exchange.WithOfferedInstanceTypes` / `RunAutoExchangeParams.OfferedInstanceTypes`:** restrict RI reshape targets to the instance types the region offers. `AnalyzeReshaping` and `AnalyzeReshapingWithRecs` take it as a variadic diff --git a/pkg/ladder/allocate.go b/pkg/ladder/allocate.go index e3ec56a6..9bd26c25 100644 --- a/pkg/ladder/allocate.go +++ b/pkg/ladder/allocate.go @@ -103,10 +103,18 @@ type Allocation struct { // no-op if and only if Allocations and Reshapes are both empty (see IsNoOp); // in that case Holds carries the explanation (baseline unavailable, gap below // the minimum threshold, or every split amount below the minimum). +// +// NetExistingUSDPerHour is the net existing commitment E that Allocate sized +// the gap against: the sum over layers of ExistingUSDPerHour minus +// ExpiringUSDPerHour, in the same on-demand-equivalent USD/hr unit as the +// inputs. It is set on every successful result, including the hold-only +// early exits, so callers report the figure the engine used instead of +// recomputing a gross one from the layer states. type AllocateResult struct { - Allocations []Allocation - Reshapes []PlannedAction - Holds []PlannedAction + NetExistingUSDPerHour *big.Rat + Allocations []Allocation + Reshapes []PlannedAction + Holds []PlannedAction } // IsNoOp reports whether the result proposes no state-changing actions: @@ -121,7 +129,10 @@ func (r *AllocateResult) IsNoOp() bool { // arithmetic; float64 inputs are converted at the boundary via ratFromFloat. // // Steps (see inline comments): -// 1. Validate config, layers (enums + topology), and layer states. +// 1. Validate config, layers (enums + topology), and layer states, then +// compute net existing commitment (reported on every result as +// NetExistingUSDPerHour; an inconsistent snapshot is an error even when +// the baseline is unavailable). // 2. Detect under-utilized buffer layers (reshape, or informational hold // when utilization is unknown on a non-empty buffer layer). This runs // INDEPENDENTLY of the purchase gap: an over-committed account is exactly @@ -148,6 +159,23 @@ func Allocate(in *AllocationInput) (*AllocateResult, error) { if err := validateAllocateInput(in); err != nil { return nil, err } + nets, existingTotal, err := computeNetExisting(in.Layers, in.LayerStates) + if err != nil { + return nil, err + } + result, err := allocate(in, nets, existingTotal) + if err != nil { + return nil, err + } + result.NetExistingUSDPerHour = new(big.Rat).Set(existingTotal) + return result, nil +} + +// allocate runs steps 2-8 of Allocate on validated input. nets and +// existingTotal are the per-layer and total net existing commitment from +// computeNetExisting, computed by Allocate before any early exit so every +// result can report the E the gap is sized against. +func allocate(in *AllocationInput, nets map[LayerType]*big.Rat, existingTotal *big.Rat) (*AllocateResult, error) { rates, err := parseCommitmentRates(in.CommitmentRates) if err != nil { return nil, err @@ -160,10 +188,6 @@ func Allocate(in *AllocationInput) (*AllocateResult, error) { if in.Baseline.LowWaterUSDPerHour == nil { return withMaintenance(baselineUnavailableHold(in), reshapes, maintHolds), nil } - nets, existingTotal, err := computeNetExisting(in.Layers, in.LayerStates) - if err != nil { - return nil, err - } lowWater, target, err := computeBaselineAndTarget(in) if err != nil { return nil, err diff --git a/pkg/ladder/allocate_net_existing_test.go b/pkg/ladder/allocate_net_existing_test.go new file mode 100644 index 00000000..4e60520d --- /dev/null +++ b/pkg/ladder/allocate_net_existing_test.go @@ -0,0 +1,120 @@ +package ladder + +import ( + "math/big" + "strings" + "testing" +) + +// netExistingStates returns AWS layer states with $10/hr existing on the +// ComputeSP layer, $4/hr of which expires this cadence window, plus $2/hr on +// the EC2InstanceSP layer with nothing expiring. Gross existing is $12/hr and +// net existing (what Allocate subtracts from the target) is $8/hr. +func netExistingStates(layers []LayerSpec) map[LayerType]LayerState { + states := zeroStates(layers) + states = withExisting(states, LayerComputeSP, 10.0) + states = withExpiring(states, LayerComputeSP, 4.0) + return withExisting(states, LayerEC2InstanceSP, 2.0) +} + +// TestAllocate_ReportsNetExisting verifies the result carries the net +// existing commitment (existing - expiring, summed over layers) that Allocate +// used to size the gap, on every successful return path, so callers can +// report the same figure instead of recomputing a gross one (go#83). +func TestAllocate_ReportsNetExisting(t *testing.T) { + t.Parallel() + cases := []struct { + lowWater *float64 + name string + wantAlloc bool + }{ + // target $20 > net $8: purchases are planned. + {name: "allocation", lowWater: ptr(20.0), wantAlloc: true}, + // target $8 == net $8: target-met hold, nothing bought. + {name: "target met hold", lowWater: ptr(8.0)}, + // no baseline: baseline-unavailable hold. + {name: "baseline unavailable hold", lowWater: nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + layers := awsLayers() + result, err := Allocate(&AllocationInput{ + Config: validConfigAWS(), + Layers: layers, + Baseline: UsageBaseline{LowWaterUSDPerHour: tc.lowWater}, + LayerStates: netExistingStates(layers), + Now: nowFixed(), + InFlight: noInFlight(), + CommitmentRates: unitRates(), + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.NetExistingUSDPerHour == nil { + t.Fatal("NetExistingUSDPerHour is nil; want the net existing commitment Allocate used") + } + ratEq(t, "NetExistingUSDPerHour", result.NetExistingUSDPerHour, 8, 1) + if got := len(result.Allocations) > 0; got != tc.wantAlloc { + t.Fatalf("allocations present = %v, want %v (holds: %+v)", got, tc.wantAlloc, result.Holds) + } + }) + } +} + +// TestAllocate_NetExistingReconcilesWithGap verifies that the reported net +// existing figure is the E the gap was sized against: with unit rates and +// 100% coverage, target - NetExistingUSDPerHour equals the sum of the +// planned on-demand-equivalent gaps. A gross figure ($12/hr here) would not +// reconcile, which is the approval-email inconsistency go#83 describes. +func TestAllocate_NetExistingReconcilesWithGap(t *testing.T) { + t.Parallel() + layers := awsLayers() + result, err := Allocate(&AllocationInput{ + Config: validConfigAWS(), + Layers: layers, + Baseline: UsageBaseline{LowWaterUSDPerHour: ptr(20.0)}, + LayerStates: netExistingStates(layers), + Now: nowFixed(), + InFlight: noInFlight(), + CommitmentRates: unitRates(), + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.NetExistingUSDPerHour == nil { + t.Fatal("NetExistingUSDPerHour is nil") + } + planned := new(big.Rat) + for _, a := range result.Allocations { + planned.Add(planned, a.OnDemandEquivalentUSDPerHour) + } + reconciled := new(big.Rat).Add(planned, result.NetExistingUSDPerHour) + ratEq(t, "planned gap + NetExistingUSDPerHour", reconciled, 20, 1) +} + +// TestAllocate_NilBaselineInconsistentSnapshot_Error verifies that net +// existing is computed (and an inconsistent snapshot rejected) before the +// baseline-unavailable hold, so the hold never reports an existing figure +// derived from expiring > existing. +func TestAllocate_NilBaselineInconsistentSnapshot_Error(t *testing.T) { + t.Parallel() + layers := awsLayers() + states := withExisting(zeroStates(layers), LayerConvertibleRI, 2.0) + states = withExpiring(states, LayerConvertibleRI, 5.0) + _, err := Allocate(&AllocationInput{ + Config: validConfigAWS(), + Layers: layers, + Baseline: UsageBaseline{}, + LayerStates: states, + Now: nowFixed(), + InFlight: noInFlight(), + CommitmentRates: unitRates(), + }) + if err == nil { + t.Fatal("expected an error for expiring > existing with a nil baseline, got nil") + } + if !strings.Contains(err.Error(), "inconsistent provider snapshot") { + t.Errorf("error missing %q: %v", "inconsistent provider snapshot", err) + } +} diff --git a/pkg/ladder/capability.go b/pkg/ladder/capability.go index e4a4b769..07f762b4 100644 --- a/pkg/ladder/capability.go +++ b/pkg/ladder/capability.go @@ -86,11 +86,12 @@ type LadderCapability interface { // is needed; NaN/Inf/non-positive values are rejected wherever these caps // are validated. type BufferReshapeConfig struct { //nolint:govet // fieldalignment: file owned by open PR #441 - // MaxPaymentPerExchangeUSD caps the payment for a single exchange. nil - // means no per-exchange cap is applied. + // MaxPaymentPerExchangeUSD caps the payment for a single exchange. It is + // required: nil is an error, never "no cap" (reshape executes exchanges + // immediately, so an omitted cap must not mean unlimited spend). MaxPaymentPerExchangeUSD *float64 // MaxPaymentDailyUSD caps the total exchange payments for the current day. - // nil means no daily cap is applied. + // It is required on the same terms as MaxPaymentPerExchangeUSD. MaxPaymentDailyUSD *float64 // UtilizationThresholdPct triggers a reshape when commitment utilization // drops below this percentage.