feat(ladder): integrate stranded phase-2 AWS capability onto main - #1361
Merged
Conversation
…dule Split each Allocation into a buy-now purchase for the immediate ramp step and future Tranche rows per delayed step, so commitment terms expire staggered. Step amounts use exact big.Rat arithmetic with a last-step remainder and an overshoot clamp, so totals always reconstruct the gap exactly. Tranche rows are fully self-describing (Layer, Term, PaymentOption, amount) and clock plus ID generation are injected. Part of #1334.
…ring Reject empty or repeated IDs from the injected NewID across all tranches of a run, since SaveTranches upserts by ID and a collision would silently drop a scheduled purchase. Render step fractions with %.4g so small percentages no longer show as 0% on positive purchases, require a non-empty rationale on every input allocation, and align the tranche amount validation docs with what big.Rat SetString actually enforces.
Switch TrancheInput and Tranche Term/PaymentOption fields from raw strings to the ladder.Term and ladder.PaymentOption enums introduced on feat/ladder-types, validate them via the enum Validate methods instead of non-empty checks, and move all test fixtures to the typed constants. Unknown enum values are now rejected at the tranche-building boundary in addition to empty ones.
AWSLadder implements the pkg/ladder.LadderCapability read methods: commitment listing merging convertible RIs and savings plans with reservation-total costing (InstanceCount, UsagePrice, amortized upfront); layer states honoring the explicit-zeros contract; nearest-rank percentile usage baseline with strict series validation. Write side stubbed for PR 6. Part of #1335.
PurchaseLayer dispatches to the savingsplans/ec2 purchase clients with mandatory idempotency tokens and per-target boundary validation. ReshapeBuffer is a thin RunAutoExchange delegation with fail-loud dry-run rejection pending #1348 and a documented store-wide pending-cancellation warning. Also: SDK-derived plan-type constants and coverage interface split. Part of #1335.
Require non-empty Platform/Tenancy/Scope on EC2 RI purchase recs (the ec2 client silently defaults empty Tenancy to "default" and Scope to "Regional", which could buy a default-tenancy RI from a dedicated-tenancy rec). Correct the ReshapeBuffer godoc: daily-cap stops surface as Failed plus an error, not Skipped (upstream reclassification tracked in #1348). Add tests for the new rejections, +Inf HourlyCommitment, and the inverse plan-type mismatch.
Add direct coverage of the write-side-not-wired guard (errors.Is on both write methods of a New()-built ladder). Fix the stale read-dependency count in the AWSLadder doc comment (four -> five after the coverage interface split) and the stale "coverage source" reference in the short-series error. Denominate the partial-failure message by actual attempts (completed + failed) instead of Analyzed, which includes never-attempted items.
mockCostExplorerClient did not implement GetSavingsPlansCoverage or GetSavingsPlansUtilization, which were added to recommendations.CostExplorerAPI when the CE savings-plans queries landed (637c570). The gap left the root providers/aws test package failing to build (service_client_test.go:41), which only surfaces on `go test`/`go vet`, not `go build`. Bringing in the AWSLadder read side, which exercises those CE queries, makes the stale mock worth fixing now rather than leaving the package un-testable. Add the two missing methods as empty-output stubs, matching the existing mock methods, so mockCostExplorerClient satisfies the full interface again.
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds deterministic provider-neutral tranche construction with exact ramp allocation and self-describing tranche validation, plus an AWS ladder implementation covering commitments, layer states, usage baselines, purchases, and RI reshaping. ChangesCore tranche construction
AWS ladder capability
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AWSLadder
participant AWSReadSources
participant CostExplorer
Caller->>AWSLadder: request commitments, states, or baseline
AWSLadder->>AWSReadSources: fetch RIs, Savings Plans, or usage series
AWSLadder->>CostExplorer: fetch coverage and utilization
CostExplorer-->>AWSLadder: metric summaries
AWSLadder-->>Caller: ladder data
sequenceDiagram
participant Caller
participant BuildTranches
participant PlannedAction
participant Tranche
Caller->>BuildTranches: provide allocations and ramp schedule
BuildTranches->>PlannedAction: create immediate purchase actions
BuildTranches->>Tranche: create delayed scheduled rows
Tranche-->>Caller: return validated tranche result
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
providers/aws/ladder/layer_states.go (1)
38-72: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider parallelizing the independent read calls.
GetLayerStatesissues up to seven sequential blocking calls (RI list, SP list, RI coverage, RI utilization, SP coverage, and SP utilization x2 for the two SP layers), none of which depend on each other's results. For a scheduled task that may run per-account/per-region, this adds up to several seconds of avoidable latency per invocation.Consider fanning these out with
errgroup(each goroutine populating its own result/err, joined before buildingstates), while preserving the existing degrade-to-nil-on-error semantics per source. Note: verify AWS Cost Explorer per-account rate limits accommodate concurrent calls, since CE has historically had low default TPS quotas.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/aws/ladder/layer_states.go` around lines 38 - 72, The independent AWS reads in GetLayerStates currently run sequentially; fan them out with an errgroup and join all work before constructing states. Run RI listing, SP listing, coverage, utilization, and the single shared SP coverage fetch concurrently, while preserving each source’s existing error handling and degrade-to-nil behavior. Keep SP layer state construction dependent on the shared result and verify the concurrency is compatible with Cost Explorer account rate limits.providers/aws/ladder/reshape.go (1)
79-108: 🩺 Stability & Availability | 🔵 TrivialCross-task coordination hazard is documented but not enforced.
The WARNING above (lines 31-37) correctly calls out that the underlying
RunAutoExchangecancels ALL pending exchange records store-wide, including ones from the unrelated standaloneri_exchange_reshapescheduled task. Since Phase 3 scheduling isn't part of this PR, there's currently no code-level guard against a Phase 3ladder_runinvocation racing with that standalone task against the same store.Worth confirming before
ReshapeBufferis wired into production scheduling: what mechanism (distributed lock, mutex flag, disabling the standalone scheduler, etc.) will enforce mutual exclusion between the two callers of the same exchange store.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/aws/ladder/reshape.go` around lines 79 - 108, Add an explicit mutual-exclusion guard around AWSLadder.ReshapeBuffer and the standalone ri_exchange_reshape caller so only one can operate on the shared exchange store at a time. Use the repository’s established distributed-lock, mutex, or scheduler-disable mechanism, and ensure the guard is acquired before RunAutoExchange and released on every exit path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@providers/aws/ladder/layer_states.go`:
- Around line 38-72: The independent AWS reads in GetLayerStates currently run
sequentially; fan them out with an errgroup and join all work before
constructing states. Run RI listing, SP listing, coverage, utilization, and the
single shared SP coverage fetch concurrently, while preserving each source’s
existing error handling and degrade-to-nil behavior. Keep SP layer state
construction dependent on the shared result and verify the concurrency is
compatible with Cost Explorer account rate limits.
In `@providers/aws/ladder/reshape.go`:
- Around line 79-108: Add an explicit mutual-exclusion guard around
AWSLadder.ReshapeBuffer and the standalone ri_exchange_reshape caller so only
one can operate on the shared exchange store at a time. Use the repository’s
established distributed-lock, mutex, or scheduler-disable mechanism, and ensure
the guard is acquired before RunAutoExchange and released on every exit path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffbd449d-4137-474c-aed0-df6902e18f27
📒 Files selected for processing (15)
pkg/ladder/ladder.gopkg/ladder/ladder_test.gopkg/ladder/store.gopkg/ladder/store_test.goproviders/aws/ladder/baseline.goproviders/aws/ladder/commitments.goproviders/aws/ladder/interfaces.goproviders/aws/ladder/ladder.goproviders/aws/ladder/ladder_test.goproviders/aws/ladder/layer_states.goproviders/aws/ladder/purchase.goproviders/aws/ladder/purchase_test.goproviders/aws/ladder/reshape.goproviders/aws/ladder/reshape_test.goproviders/aws/service_client_test.go
cristim
added a commit
that referenced
this pull request
Jul 21, 2026
The prior commit's code comments cited "#1461" as the tracking issue for the RI utilization filter bug. #1461 is an unrelated PR (NaN/Inf rejection in SP money parsing). The bug this fix addresses was introduced by PR #1361, per the review that surfaced it; correct all comment references accordingly.
7 tasks
cristim
added a commit
that referenced
this pull request
Jul 22, 2026
… decisions GetRIUtilization built GetReservationUtilization with no Filter at all, so it blended utilization across every reserved-resource type in the account (RDS, ElastiCache, OpenSearch, Redshift, standard EC2 RIs) and every region into one SUBSCRIPTION_ID-grouped number. The ladder ConvertibleRI (buffer) layer reads this as if it were EC2-convertible utilization for one account/region, then uses it to decide whether to trigger a real RI reshape/exchange -- an underutilized RI in an unrelated service or region could trigger a reshape, or mask a genuinely poorly-utilized convertible RI. Add a SERVICE+REGION Filter to the CE call, mirroring the same pattern already used by GetRICoverageMap in coverage.go. Also intersect the (now EC2+region-scoped) response against the account's own convertible RI IDs before aggregating in GetLayerStates, since a standard (non-convertible) EC2 RI in the same account/region would otherwise still pass the SERVICE+REGION filter and blend into the layer's UtilizationPct. Threads a region parameter through GetRIUtilization's callers (providers/aws/ladder, providers/aws/service_client.go, internal/server, internal/api). Follow-up to #1361.
cristim
added a commit
that referenced
this pull request
Jul 22, 2026
The prior commit's code comments cited "#1461" as the tracking issue for the RI utilization filter bug. #1461 is an unrelated PR (NaN/Inf rejection in SP money parsing). The bug this fix addresses was introduced by PR #1361, per the review that surfaced it; correct all comment references accordingly.
cristim
added a commit
that referenced
this pull request
Jul 22, 2026
… decisions (follow-up to #1361) (#1479) * fix(aws/ladder): scope RI utilization query to EC2+region for reshape decisions GetRIUtilization built GetReservationUtilization with no Filter at all, so it blended utilization across every reserved-resource type in the account (RDS, ElastiCache, OpenSearch, Redshift, standard EC2 RIs) and every region into one SUBSCRIPTION_ID-grouped number. The ladder ConvertibleRI (buffer) layer reads this as if it were EC2-convertible utilization for one account/region, then uses it to decide whether to trigger a real RI reshape/exchange -- an underutilized RI in an unrelated service or region could trigger a reshape, or mask a genuinely poorly-utilized convertible RI. Add a SERVICE+REGION Filter to the CE call, mirroring the same pattern already used by GetRICoverageMap in coverage.go. Also intersect the (now EC2+region-scoped) response against the account's own convertible RI IDs before aggregating in GetLayerStates, since a standard (non-convertible) EC2 RI in the same account/region would otherwise still pass the SERVICE+REGION filter and blend into the layer's UtilizationPct. Threads a region parameter through GetRIUtilization's callers (providers/aws/ladder, providers/aws/service_client.go, internal/server, internal/api). Follow-up to #1361. * fix(aws/ladder): correct comment reference from #1461 to #1361 The prior commit's code comments cited "#1461" as the tracking issue for the RI utilization filter bug. #1461 is an unrelated PR (NaN/Inf rejection in SP money parsing). The bug this fix addresses was introduced by PR #1361, per the review that surfaced it; correct all comment references accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Re-integrates the Phase-2 AWS commitment-laddering capability onto
main. This code was written and CodeRabbit-reviewed in PRs #1341, #1347 and #1349, but those PRs were stacked (each based on the previous feature branch, not onmain) and merged bottom-to-top within the stack. The top-of-stack PR intomainwas never opened, so the content shows as "merged" on GitHub yet never landed onmain.providers/aws/ladder/andpkg/ladder/ladder.goare absent frommaintoday, andLadderCapabilityhas no implementation.This PR cherry-picks the seven original commits (already reviewed) onto a fresh branch off current
origin/main, restoring:pkg/ladder/ladder.go): staggered tranches from allocations via a ramp schedule, duplicate-ID detection, typed term/payment enums (feat(ladder): staggered tranche building from allocations #1341).providers/aws/ladder/): commitments, layer states, baseline (feat(aws): AWSLadder capability read side #1347).PurchaseLayer,ReshapeBufferdelegation, plus the phase-2 review fixes (feat(aws): AWSLadder capability write side #1349).Why now
Phase-3 PR-2 (the
ladder_runscheduled task, issue #1336) is designed to sit on this AWS capability. Without it onmain, the handler half will not compile. Integrating the stranded stack first restores the intended foundation.Also fixed (pre-existing
maindebt)The last commit repairs a stale test mock:
mockCostExplorerClientnever implementedGetSavingsPlansCoverage/GetSavingsPlansUtilizationafter those methods were added torecommendations.CostExplorerAPI(637c570). The gap left the rootproviders/awstest package failing to build (service_client_test.go:41), which only surfaces undergo test/go vet, notgo build. The read side exercises those CE queries, so the mock is fixed here rather than left un-testable.Verification (all exit 0)
pkg:go build ./...,go vet ./...clean;go test ./ladder/...246 pass.providers/aws:go build ./...,go vet ./...clean;go test ./...allok(root aws pkg 105, ladder 113).go build ./...clean;go mod tidyno-op.Scope
No conflicts on cherry-pick:
allocate.go/capability.go/plan.goare identical between the stranded branch andmain; the sharedstore.go/types.godiffs are the tranche additions; the twelve other files are net-new (mainlacks them). No execution, email, approval, or scheduling paths are touched; everything stays behind the existing default-off flags.Closes #1335. Part of #1333.
Summary by CodeRabbit
New Features
Bug Fixes