fix(aws): GetCapacityBlockOfferings/Reservations/Blocks drop per-region errors - #152
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…on errors GetCapacityBlockOfferings buffered per-region errors, printed them only under --verbose, and always returned (results, nil) — the exact pattern already fixed in #63/#109 for other discovery paths, but this function was added afterward (#67) and reintroduced it. GetCapacityReservations and GetCapacityBlocks had the identical shape and were never fixed at all. Since "no capacity" is a common, legitimate answer for all three calls, a total failure (expired credentials, an SCP denial, throttling) was indistinguishable from genuinely empty inventory. All three now apply the same #63 contract as SearchInstanceTypes: an error when every queried region fails, an unconditional stderr warning for a partial failure so a degraded result stays visible. Also adds aws.ErrCapacityBlockIneligible: DescribeCapacityBlockOfferings returns InvalidParameterValue with a distinct, stable message ("... is not supported for Capacity Blocks ...") when an instance type simply isn't a Capacity Block type at all — a different, permanent answer from a transient query failure, and previously indistinguishable from one. Callers can check for it via errors.Is; when every queried region reports ineligibility (not a mix of ineligibility and real failures), the aggregate error names that specifically rather than the generic "all N region queries failed". Verified live against account 942542972736: c7i.4xlarge, m5.large, g7e.4xlarge, and trn2.48xlarge all return the exact ineligibility message classified here; p5.48xlarge hits a genuine service-limit error instead, which is correctly NOT classified as ineligible. TestGetCapacityReservations_Empty/_MultiRegion and TestGetCapacityBlocks_Empty asserted the bug: they only passed because Substrate doesn't implement DescribeCapacityReservations (answers InvalidAction) and the old code discarded that error. Replaced with _AllRegionsFailed/_MultiRegionAllFailed variants asserting the #63 contract instead, matching TestGetCapacityBlockOfferings_AllRegionsFailed from #109's fix. Fixes #110
scttfrdmn
force-pushed
the
fix/110-capacity-blocks-region-errors
branch
from
August 19, 2026 04:54
a1df5e3 to
a40cab6
Compare
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
GetCapacityBlockOfferingsbuffered per-region errors, printed them only under--verbose, and always returned(results, nil)— the exact pattern already fixed in [HIGH] SearchInstanceTypes/GetSpotPricing return success even when every region query fails #63/Capacity Blocks for ML: instance-count gating, CB-only types, eligibility allowlist, 24h floor, utilization break-even #109 for other discovery paths, but this function was added afterward (Capacity Blocks for ML: wire the dead --blocks flag + add offerings discovery (DescribeCapacityBlockOfferings) #67) and reintroduced it.GetCapacityReservationsandGetCapacityBlockshad the identical shape and were never fixed at all (investigation turned these up — the issue only reported the offerings path directly but asked to audit the others).SearchInstanceTypes: an error when every queried region fails, an unconditional stderr warning for a partial failure.aws.ErrCapacityBlockIneligible:DescribeCapacityBlockOfferingsreturnsInvalidParameterValuewith a distinct, stable message ("... is not supported for Capacity Blocks ...") when an instance type simply isn't a Capacity Block type at all — a different, permanent answer from a transient query failure, previously indistinguishable from one. Callers can check viaerrors.Is; when every queried region reports ineligibility (not a mix of ineligibility and real failures), the aggregate error names that specifically instead of the generic "all N region queries failed."Verification against live AWS
Account 942542972736,
us-west-2:c7i.4xlarge,m5.large,g7e.4xlarge,trn2.48xlarge→ all return the exactInvalidParameterValue: ... is not supported for Capacity Blocks ...message the classifier matches.p5.48xlarge→ hits a genuineCapacityBlockDescribeLimitExceededservice-limit error, correctly not classified as ineligible (verifiedisCapacityBlockIneligiblereturnsfalseand the CLI surfaces the real AWS message unchanged).Test plan
TestGetCapacityReservations_Empty/_MultiRegionandTestGetCapacityBlocks_Emptyasserted the bug — they only passed because Substrate doesn't implementDescribeCapacityReservations(answersInvalidAction) and the old code discarded that error. Replaced with_AllRegionsFailed/_MultiRegionAllFailedvariants asserting the [HIGH] SearchInstanceTypes/GetSpotPricing return success even when every region query fails #63 contract, matching the precedent inTestGetCapacityBlockOfferings_AllRegionsFailed(Capacity Blocks for ML: instance-count gating, CB-only types, eligibility allowlist, 24h floor, utilization break-even #109).pkg/aws/client_ineligible_test.go: classifier unit tests (ineligible vs. service-limit vs. access-denied vs. throttled vs. non-API error),allIneligibleaggregation, andnewUnreachableClient-backed all-regions-fail tests forGetCapacityReservations/GetCapacityBlocks.GetCapacityReservations/GetCapacityBlocksto the pre-fix implementation makes the new_AllRegionsFailtests fail with "expected an error ... got nil" — verified explicitly before restoring the fix.go build ./...,go vet ./...,go test ./...all greengolangci-lint run ./pkg/aws/...— 0 issuesmake check-docsclean## [Unreleased]Fixes #110