feat(aws): AWSLadder capability write side - #1349
Merged
Merged
Conversation
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.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR separates AWSLadder read-side coverage sources from on-demand series sources, introduces named Savings Plan plan-type constants, adds explicit write-side wiring, and implements ChangesAWS Ladder Read/Write Side Refactor and Implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant PurchaseLayer
participant riPurchase
participant spPurchase
participant riPurchaser
participant spPurchaser
Caller->>PurchaseLayer: layer, rec, opts
PurchaseLayer->>PurchaseLayer: validate wiring, layer type, idempotency token
alt convertible RI
PurchaseLayer->>riPurchase: purchaseRI(rec, opts)
riPurchase->>riPurchaser: PurchaseCommitment
else EC2Instance/Compute SP
PurchaseLayer->>spPurchase: purchaseSP(rec, opts, planType)
spPurchase->>spPurchaser: PurchaseCommitment
end
riPurchaser-->>Caller: PurchaseResult
spPurchaser-->>Caller: PurchaseResult
sequenceDiagram
participant Caller
participant ReshapeBuffer
participant buildRIExchangeConfig
participant exchangeRunner
participant summarizeExchangeResult
Caller->>ReshapeBuffer: scope, cfg
ReshapeBuffer->>ReshapeBuffer: validate wiring, scope, DryRun
ReshapeBuffer->>buildRIExchangeConfig: cfg
buildRIExchangeConfig-->>ReshapeBuffer: RIExchangeConfig
ReshapeBuffer->>exchangeRunner: RunAutoExchange(RIExchangeConfig)
exchangeRunner-->>ReshapeBuffer: AutoExchangeResult
ReshapeBuffer->>summarizeExchangeResult: AutoExchangeResult
summarizeExchangeResult-->>Caller: ReshapeSummary, error
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
providers/aws/ladder/purchase_test.go (1)
119-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the "write side not wired" guard.
PurchaseLayerreturnserrWriteNotWiredwhena.riPurchase/a.spPurchaseare nil (purchase.go Line 38-40), but no test in this file exercisesPurchaseLayeron a ladder that hasn't hadWithWriteSidecalled.TestWithWriteSide_NilArgsRejectedonly coversWithWriteSideitself rejecting nil args, notPurchaseLayer's own nil-dependency guard.✅ Suggested test addition
func TestPurchaseLayer_WriteSideNotWired_ErrorsWithoutCalling(t *testing.T) { a := newTestLadder(t, &fakeRILister{}, &fakeSPLister{}, &fakeCoverageSource{}, &fakeUtilizationSource{}) _, err := a.PurchaseLayer(context.Background(), ladder.LayerConvertibleRI, validRIRec(), validPurchaseOpts()) require.Error(t, err) assert.ErrorIs(t, err, errWriteNotWired) }🤖 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/purchase_test.go` around lines 119 - 301, `PurchaseLayer` is missing coverage for its own write-side wiring guard: add a test that calls `PurchaseLayer` on a ladder created without `WithWriteSide` so the nil `riPurchase`/`spPurchase` path is exercised. Use the existing `PurchaseLayer` and `newTestLadder` helpers, pass a valid recommendation/opts, and assert the call fails with `errWriteNotWired` without invoking any purchaser.providers/aws/ladder/ladder.go (1)
59-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale dependency count in doc comment.
All four read data-source dependencies are injected via narrow interfaces so that unit tests are hermetic (no real AWS calls needed). But
New's own docstring in this same file states New constructs an AWSLadder. The five required read-side interfaces must be non-nil; spCov and spUtil may be nil (wired later). SincecoverageSourcewas split intoriCoverageSourceandonDemandSeriesSource, the required count grew from 4 to 5; this struct-level comment wasn't updated to match.📝 Proposed fix
-// All four read data-source dependencies are injected via narrow interfaces so +// All five read data-source dependencies are injected via narrow interfaces so // that unit tests are hermetic (no real AWS calls needed). The caller wires the // concrete adapters (ec2svc.Client, savingsplans.Client, etc.) at startup.🤖 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/ladder.go` around lines 59 - 71, Update the AWSLadder documentation comment to reflect the current required read-side wiring: the split of coverageSource into riCoverageSource and onDemandSeriesSource means the constructor now needs five non-nil read dependencies, not four. Adjust the comment near AWSLadder/New to mention the five required read-side interfaces and keep the note that spCov and spUtil may still be nil until WithWriteSide wiring.providers/aws/ladder/baseline.go (1)
58-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "coverage source" reference in error message.
The fetch now goes through
a.onDemand.GetOnDemandSeries(renamed from the oldcoverageSource), but the adjacent length-check error still says GetUsageBaseline: series length %d is below minimum %d days; extend the lookback window or check the coverage source. Since there is no longer a "coverage source" involved in this path (it's the on-demand series source), this message will mislead an on-call engineer debugging a short series.📝 Proposed fix
if len(series) < minBaselineSeriesDays { return ladder.UsageBaseline{}, fmt.Errorf( - "GetUsageBaseline: series length %d is below minimum %d days; extend the lookback window or check the coverage source", + "GetUsageBaseline: series length %d is below minimum %d days; extend the lookback window or check the on-demand series source", len(series), minBaselineSeriesDays, ) }🤖 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/baseline.go` around lines 58 - 70, The length-check error message in GetUsageBaseline still refers to the old “coverage source,” which is misleading now that the data comes from a.onDemand.GetOnDemandSeries. Update the fmt.Errorf text in the UsageBaseline path to mention the on-demand series source instead of coverage source, while keeping the existing minBaselineSeriesDays and len(series) context so engineers can correctly identify the failing input.providers/aws/ladder/reshape.go (1)
188-192: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMisleading denominator in partial-failure message.
summary.Analyzedincludes Pending and Skipped items, which are not "exchange attempts." If a run has failures alongside skipped/pending items, the message ("N of Analyzed exchange attempt(s) failed") overstates the attempt count, which could confuse an on-call engineer reading the error during a money-path incident. Consider denominating bylen(result.Completed)+len(result.Failed)(actual attempts) instead.✏️ Proposed fix
if len(result.Failed) > 0 { + attempts := len(result.Completed) + len(result.Failed) return summary, fmt.Errorf( - "ReshapeBuffer: %d of %d exchange attempt(s) failed (first: %s: %s); see summary details for the full list", - len(result.Failed), summary.Analyzed, result.Failed[0].SourceRIID, result.Failed[0].Error) + "ReshapeBuffer: %d of %d exchange attempt(s) failed (first: %s: %s); see summary details for the full list", + len(result.Failed), attempts, result.Failed[0].SourceRIID, result.Failed[0].Error) }🤖 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 188 - 192, The partial-failure error in ReshapeBuffer uses summary.Analyzed as the total attempt count, which includes Pending and Skipped items. Update the failure message in reshape.go within the ReshapeBuffer flow to denominator by the actual attempted exchanges, using len(result.Completed)+len(result.Failed), while keeping the existing first-failure details from result.Failed[0].SourceRIID and result.Failed[0].Error.
🤖 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/baseline.go`:
- Around line 58-70: The length-check error message in GetUsageBaseline still
refers to the old “coverage source,” which is misleading now that the data comes
from a.onDemand.GetOnDemandSeries. Update the fmt.Errorf text in the
UsageBaseline path to mention the on-demand series source instead of coverage
source, while keeping the existing minBaselineSeriesDays and len(series) context
so engineers can correctly identify the failing input.
In `@providers/aws/ladder/ladder.go`:
- Around line 59-71: Update the AWSLadder documentation comment to reflect the
current required read-side wiring: the split of coverageSource into
riCoverageSource and onDemandSeriesSource means the constructor now needs five
non-nil read dependencies, not four. Adjust the comment near AWSLadder/New to
mention the five required read-side interfaces and keep the note that spCov and
spUtil may still be nil until WithWriteSide wiring.
In `@providers/aws/ladder/purchase_test.go`:
- Around line 119-301: `PurchaseLayer` is missing coverage for its own
write-side wiring guard: add a test that calls `PurchaseLayer` on a ladder
created without `WithWriteSide` so the nil `riPurchase`/`spPurchase` path is
exercised. Use the existing `PurchaseLayer` and `newTestLadder` helpers, pass a
valid recommendation/opts, and assert the call fails with `errWriteNotWired`
without invoking any purchaser.
In `@providers/aws/ladder/reshape.go`:
- Around line 188-192: The partial-failure error in ReshapeBuffer uses
summary.Analyzed as the total attempt count, which includes Pending and Skipped
items. Update the failure message in reshape.go within the ReshapeBuffer flow to
denominator by the actual attempted exchanges, using
len(result.Completed)+len(result.Failed), while keeping the existing
first-failure details from result.Failed[0].SourceRIID and
result.Failed[0].Error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02345d7c-8d21-4fea-a610-ed702075a2a3
📒 Files selected for processing (10)
providers/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.go
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.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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.
Summary
LadderCapability:PurchaseLayerandReshapeBufferreplace the PR 5 not-wired stubs. Write-side dependencies (riPurchaser,spPurchaser,exchangeRunner) are wired viaWithWriteSide; unwired instances keep failing loud, so read-only wiring stays free of purchase/exchange infrastructure.PurchaseLayerdispatches by layer:LayerConvertibleRI-> EC2 client (idempotency-tag dedupe guard),LayerEC2InstanceSP/LayerComputeSP-> Savings Plans client with the matching plan type. Boundary validation before any AWS call: supported-layer check, mandatory non-emptyopts.IdempotencyToken(re-driven executions can never double-buy), and per-target rec validation (*ComputeDetails+ Count/InstanceType for RIs;*SavingsPlanDetails+ plan-type match + positive finite HourlyCommitment for SPs; Term/PaymentOption for both). Client errors are wrapped with layer context via%w, preservingerrors.Is(err, common.ErrCommitmentPurchaseNotSupported).ReshapeBufferis a thin delegation toexchange.RunAutoExchangethrough a config-onlyexchangeRunnerseam (the concrete runner owns store/quote-execute client/offering lookup/inventory, mirroringinternal/server.executeRIExchangeReshape). Money caps: nil = no cap maps to an explicitunlimitedCapUSDconstant (never 0 — a zero cap would silently block every exchange and is rejected loudly;+Infis unusable becausebig.Rat.SetFloat64(+Inf)returns nil). Outcome maps toReshapeSummary; partial failures return the populated summary TOGETHER with a non-nil error.ExchangeRecordswith live approval tokens (actionable money instruments) and still triggers the store-wide pending cancellation — neither is a simulation. A true simulation mode inpkg/exchangeis tracked in feat(exchange): add true simulation mode to RunAutoExchange; make pending-cancellation coordination explicit #1348; until then the engine previews reshapes via ActionReshape rationales without callingReshapeBuffer.Coordination warning for the pipeline phase
exchange.RunAutoExchangebegins by unconditionally canceling ALL pending exchange records store-wide (CancelAllPendingExchanges) — a side effect shared with the standaloneri_exchange_reshapescheduled task. The pipeline phase that invokesReshapeBuffermust coordinate with (or disable) the standalone scheduler; the two must never run concurrently against the same store. Documented prominently in theReshapeBuffergodoc.CodeRabbit nitpick fixes carried from #1347
string(sptypes.SavingsPlanTypeEc2Instance)/...Compute), replacing the scattered"EC2Instance"/"Compute"literals across dispatch, filtering, and validation.coverageSourceinterface is split into single-methodriCoverageSourceandonDemandSeriesSource(interface segregation); method names unchanged, so one concrete adapter can still satisfy both.Stacked on #1347. Part of #1335 (phase 2 of #1333) - completes the AWS LadderCapability.
Test plan
ErrCommitmentPurchaseNotSupportedpassthrough, cap mapping incl. nil-vs-zero distinction, DryRun fail-loud with zero runner invocations, summary mapping incl. partial failures, runner-contract nil-result guard, plus the full PR 5 read-side suite.providers/aws/:go build ./...,go vet ./ladder/...,gofmt -l ladder/(empty),go test ./ladder/... -count=1,golangci-lint run --config ../../.golangci.yml ./ladder/...,gocyclo -over 10 ladder/(empty).Summary by CodeRabbit
New Features
WithWriteSidestep.PurchaseLayerfor executing RI and Savings Plan purchases with recommendation validation.ReshapeBufferto run automated buffer reshaping, including caps/threshold validation and audit summaries.Bug Fixes
Tests