fix(aws/ladder): scope RI utilization query to EC2+region for reshape decisions (follow-up to #1361) - #1479
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Filed the deferred computeEC2CoveragePct finding as a separate follow-up: #1482 (lower priority -- no engine decision currently consumes CoveragePct, so it's a wrong reported metric only, not a live money-affecting decision like the fix in this PR). @coderabbitai review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
… 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.
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.
d4a8a6a to
b60f2a9
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
GetRIUtilization(providers/aws/recommendations/utilization.go) builtGetReservationUtilizationInputwith noFilterat all, grouping only bySUBSCRIPTION_ID. This blends utilization across every reserved-resource type in the account (RDS, ElastiCache, OpenSearch, Redshift, standard EC2 RIs) and every region into a single number.ConvertibleRI(buffer) layer (providers/aws/ladder/layer_states.go) reads this as "this account/region's EC2 convertible-RI utilization" and uses it to decide whether to trigger a real RI reshape/exchange (pkg/ladder/allocate.go,ActionReshape). An underutilized RI in an unrelated service or region could trigger a reshape, or a genuinely poorly-utilized convertible RI could be masked by healthy unrelated reservations blended into the same aggregate. Real-money-decision correctness bug, follow-up to feat(ladder): integrate stranded phase-2 AWS capability onto main #1361 (merged).FilterscopingSERVICE=Amazon Elastic Compute Cloud - ComputeandREGION=<region>to the CE call (mirrors the existingserviceRegionFilterpattern incoverage.go). Threaded aregionparameter throughGetRIUtilizationand its callers.ListConvertibleReservedInstances) before aggregating inGetLayerStates, 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'sUtilizationPct. This was a clean, contained addition sinceGetLayerStatesalready has both the RI list and the utilization slice in scope, so it's included rather than deferred as a follow-up.Test plan
go build ./...-- exit 0go vet ./...-- exit 0go test ./providers/aws/... ./internal/api/... ./internal/server/...-- exit 0go test ./...(full suite) -- exit 0TestGetRIUtilization_ScopesToEC2AndRegion,TestGetRIUtilization_EmptyRegionOmitsRegionFilter(providers/aws/recommendations/utilization_test.go),TestGetLayerStates_RILayer_UtilizationExcludesUnrelatedReservations(providers/aws/ladder/ladder_test.go) -- all confirmed to FAIL against the pre-fix code (verified manually by reverting the fix locally) and PASS aftergolangci-lint runon touched packages -- 0 new issues introduced by this diff (pre-existing repo debt in touched files left untouched, out of scope); note the CI-pinned v2.10.1 binary could not be installed in this sandbox (external-script-execution policy), so this was verified with the locally available v2.11.4 insteadgocyclo -over 10on touched files -- exit 0, no outputOut of scope / documented follow-up
computeEC2CoveragePct(layer_states.go) -- the one-colon pool-key heuristic also matches ElastiCache/OpenSearch/Redshift/MemoryDB pool keys, not just EC2 -- was not fixed here. No engine decision currently consumesCoveragePct(confirmed by an earlier review), so today's impact is a wrong reported metric only, not a money-affecting decision. Filing as a separate follow-up issue.