Repository navigation
Conversation
…s and never claim legacy tranches The AWS provider reports LayerState.ExistingUSDPerHour and ExpiringUSDPerHour in commitment dollars (SavingsPlan.Commitment for the SP layers, amortized cost for the RI layer), but Allocate compares them with an on-demand-equivalent target. The planner passed them through unconverted, so existing coverage was understated by the commitment rate and the gap overstated: a 2.5 commitment $/hr EC2 Instance SP at rate 0.5 was netted as 2.5 OD instead of 5. executeLadderRun now converts each layer with od = commitment / rate, using the GetCommitmentRates rate the run already fetched (option 2 on the issue). Existing rounds up and expiring down, so net coverage is never understated. A layer with non-zero existing or expiring commitment and no rate fails the run instead of being netted 1:1. ladder_runs.existing_usd_hr and the plan JSON existing are now on-demand-equivalent. Per the maintainer's note on the issue, ClaimDueLadderTranches now claims only amount_basis = 'commitment' rows. Legacy NULL-basis and on_demand_equivalent rows stay netted until the overdue sweep cancels them, so an executor can never buy their OD amount as commitment dollars. Closes LeanerCloud#946 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
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
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
The ladder compared an on-demand-equivalent (OD) target with existing Savings Plan coverage measured in commitment dollars. This PR converts existing and expiring coverage to OD dollars before
Allocate. Following the maintainer's note on the issue, it also makes legacy NULL-basis andon_demand_equivalenttranches impossible to claim.Closes #946
Units
LayerState.ExistingUSDPerHourandExpiringUSDPerHourare in this unit:SavingsPlan.Commitmentfor the SP layers and amortized RI cost for the RI layer.ris in (0, 1] and means commitment dollars per OD dollar.od = commitment / randcommitment = od * r. This is the same rule go'sallocate_units.gouses for tranches.Root cause
executeLadderRunpassedGetLayerStatesstraight toAllocateandassembleLadderPlan. go'sLayerStatedoc says callers must convert these values to OD first. Existing coverage was therefore understated by a factor ofrand the gap overstated. Example: a 2.5 commitmentFix
internal/server/ladder_existing_units.go(new):ladderLayerStatesOnDemand(states, rates)returns a converted copy of the states and never mutates the provider's map.executeLadderRunalready fetched fromGetCommitmentRatesfor the config's term and payment option. This is option 2 on the issue. DescribeSavingsPlans does not expose each plan's own rate, so plans bought at a different discount are approximated by today's rate.big.Rat). When the quotient is not a float64, existing rounds up and expiring rounds down. Net coverage (existing - expiring) is therefore never understated, which errs toward under-buying.Allocatestill rejects it.executeLadderRuncalls it right after the rate fetch. The converted states feedAllocateand the plan, soladder_runs.existing_usd_hrand plan JSONexistingare now OD. That matches go'sLadderPlan.ExistingUSDPerHourdoc and the amend ceiling (LEAST(target, baseline*coverage) - existing), which already assumed OD. The ceiling therefore gets stricter, never looser.ClaimDueLadderTranches(selectDueLadderTranchesTx) now selects onlyamount_basis = 'commitment'rows. The validatedladder_tranche_amount_provenanceconstraint guarantees those rows have acommitment_rate.on_demand_equivalentrows are never claimed. They stay netted while withinLadderFireGrace. After that, the existing overdue sweep cancels them and the next run re-plans their gap in commitment dollars ("cancelled or re-planned", with no migration).LIMIT ... FOR UPDATE SKIP LOCKED. Older legacy rows therefore cannot use up a claim's limit and starve a due commitment row.Visible behavior changes
existing coverage: layer compute-sp: no commitment rate ...). Today the AWS provider supplies a rate only for EC2 Instance SP (fix(ladder): bump cloud-commitments-go and plan tranches in commitment dollars #976 already holds those layers for new purchases). Before this PR, such runs silently produced over-sized plans; now they produce no plan. Once go's provider returns Compute SP and RI rates, or option 3 (Cost Explorer covered OD cost) lands, these runs plan again. A softer alternative would be a persisted Hold-only run with this reason. I kept fail-loud to match fix(ladder): bump cloud-commitments-go and plan tranches in commitment dollars #976's "no capability → Errored" precedent, and I'm happy to switch if you prefer.existing_usd_hron new runs is OD. Historicalladder_runsrows keep their commitment-dollar snapshot because no rate was stored to backfill from. If the latest run predates this change, the amend ceiling uses that older, smaller existing value until the next run writes an OD one.Verification
All evidence comes from unit, fake-capability and local-PostgreSQL tests. Nothing ran against a real AWS account. Tests ran with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/nullbecause this machine's default AWS profile is invalid.Fail-before (on
origin/mainc8bdd46, with the new tests and no fix):go test ./internal/server/ -run 'LadderLayerStatesOnDemand|TestExecuteLadderRun_(Existing|Expiring)':TestExecuteLadderRun_ExistingSavingsPlanCoverageIsOnDemandEquivalent: existing 2.5 (want 5), gap 7.5 OD (want 5), total_hourly_commit 3.75 (want 2.5), plan JSON existing 2.5 (want 5).TestExecuteLadderRun_ExpiringCoverageIsOnDemandEquivalent: gap 8 (want 6), commit 4 (want 3).TestExecuteLadderRun_ExistingCoverageWithoutRateIsErrored: no error (the run was netted 1:1).go test -tags integration ./internal/config/ -run TestClaimDueLadderTranches_NeverClaimsOnDemandOrUnknownBasisRows: the limit-1 claim took the older NULL row, 3 execution rows were created (want 1), and the NULL and OD rows werefired(wantscheduled, thencancelledafter grace).Pass-after (commit 6af6413):
TestLadderLayerStatesOnDemand_PinsCommitmentToOnDemandConversioncovers 5 @ 0.5 → 10, 3 @ 0.25 → 12, 7 @ 1 → 7, and expiring by the same rate...._RoundsTowardUnderBuyingchecks that at 1/0.3, existing is the nearest float at or above the exact value and expiring the nearest at or below...._DoesNotMutateInput,..._EmptyLayersNeedNoRateand..._RejectsUnconvertibleCoverage(8 cases) cover the rest, plus the threeexecuteLadderRuntests above.go build ./...,go vet ./...,go vet -tags integrationon config/purchase/api/server, andmake tidy-checkare clean.go test -count=1passes for./internal/server/ ./internal/config/ ./internal/purchase/ ./internal/api/ ./internal/mocks/, andgo test -race -run Ladder ./internal/server/passes.go test -tags integration -count=1 ./internal/config/(whole package) passes, as do-run Ladder ./internal/purchase/and-run Ladder ./internal/api/. These ran on the repo's testcontainers PostgreSQL via Docker, with no skips.--allow-parallel-runners, with and without--build-tags integration) reports 0 issues.gocyclo -over 10 -ignore "_test\.go"on the changed non-test files is silent.scripts/gosec-hook.shon the changed packages and markdownlint-cli 0.47.0 on the known_issues file are clean.scripts/test-git-secrets-allowlist.shreports 137 PASS, 0 FAIL.Test fixture changes: the existing claim tests now seed commitment-basis rows at rate
1through a newclaimableTranchehelper (or explicit basis fields in the purchase and api rigs), so their netting values are unchanged.TestLadderTrancheProvenance_InsertClaimAndReconcilenow asserts that only the commitment row is claimed and fires the NULL and OD rows directly, so the fired-row reader still covers every basis.TestLadderAmendCeiling_CancelsOverdueBeforeCountingmarks its overdue row as commitment, so the cancel, not the basis filter, is what keeps it unclaimed.Out of scope / follow-ups
sumSPHourlyCostsums EC2 Instance SPs from every region, while the EC2 Instance SP rate is regional. This is pre-existing and noted here only.ladder_runs.existing_usd_hrrows are not backfilled.Frontend follow-up for the UI owner
No change is required. The timeline chart already treats
existing_usd_hras the OD base. New runs now match that label, so only runs from before this PR show a commitment-dollar base. Optionally, the run snapshot text ("existing $X") could say "on-demand-equivalent".Maintainer: please mirror the issue's triage labels on this PR:
priority/p2,severity/medium,urgency/this-quarter,impact/few,effort/m,type/bug,triaged.🤖 Generated with Claude Code