Add ordered instance_types with insufficient-capacity fall-through - #702
Draft
cchristous wants to merge 2 commits into
Draft
Add ordered instance_types with insufficient-capacity fall-through#702cchristous wants to merge 2 commits into
cchristous wants to merge 2 commits into
Conversation
Add an optional ordered `instance_types` list to the shared run config. When set (in place of `instance_type`), Packer launches the first type and falls through to the next on an insufficient-capacity error (InsufficientInstanceCapacity / InsufficientHostCapacity / InsufficientReservedInstanceCapacity), succeeding on the first that launches and failing with the last error if all are exhausted. Any non-capacity error aborts immediately so misconfiguration is not masked. All other placement config (tenancy, host resource group, subnet/AZ, license specs, block device mappings) is type-independent and preserved across attempts. `instance_types` is mutually exclusive with `instance_type` and `spot_instance_types`, and rejects empty and burstable (T-family) entries. The capacity classifier is centralized in the awserrors package. Implemented in both the v1 (builder/common) and v2 (common) packages, covering the amazon-ebs, ebsvolume, ebssurrogate, and instance builders. HCL2 spec and docs regenerated.
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Have you signed the CLA already but the status is still pending? Recheck it. |
…ests Applies findings from the multi-agent review of hashicorp#702 (both v1 builder/common and v2 common packages, kept in lockstep): - Fix the burstable-type regexp: (:? -> (?: (non-capturing group). As written it matched an optional literal colon; now a proper validation gate. - Rename burstableInstanceTypeRe -> reBurstableInstanceType to match the adjacent reShutdownBehavior naming precedent. - Generalize the InstanceTypes doc: drop the hardcoded capacity-code list and placement-field enumeration (code-owned, rot-prone) and the hardware narrative; regenerate docs-partials and .web-docs. - runInstanceWithFallback: on multi-type exhaustion, wrap the last error with the list of types tried so the failure record shows the fallback ran. The single-type path returns the raw error unchanged (backward compatible). - Remove a redundant 'what' comment above the instance_type fallback default. - Tests: assert the exhaustion error names the types tried; add empty-list guard test; add instance_type+spot_instance_types and all-three-selector exclusivity cases; assert the zero-selector error message.
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.
Closes #701
What
Adds an optional, ordered
instance_types([]string) to the shared runconfig. When set, it replaces the single
instance_type: Packer launches thefirst type and, on an insufficient-capacity error, falls through to the next
type in the list, succeeding on the first that launches. If every type is
capacity-starved, the build fails with the last error.
Rationale — why this is the right change
A build pinned to one
instance_typefails hard withInsufficientInstanceCapacity/InsufficientHostCapacitywhen that type iscapacity-starved, even when an equivalent type would launch. The driving case:
macOS
arm64_macAMIs build onmac2.metal(M1) viatenancy=hostinto aLicense Manager host resource group; M1 Dedicated Host capacity is
intermittently exhausted, but the same AMI is cross-compatible with
mac2-m2.metal(M2) andmac-m4.metal(M4). An ordered list lets the buildsucceed on the first type with capacity.
Design choices:
instance_types []string, replacinginstance_type— chosen over aninstance_type_fallbackssupplement because it reads as one ordered prioritylist and matches the existing precedent of
spot_instance_typeson the Spotpath. It's mutually exclusive with
instance_typeandspot_instance_types,reusing the existing exclusivity rule;
instance_typealone is untouched andfully backward compatible.
awserrors.IsCapacityError) —fall-through happens only on
InsufficientInstanceCapacity,InsufficientHostCapacity,InsufficientReservedInstanceCapacity. Everyother error (config/permission/quota —
AuthFailure,InstanceLimitExceeded,Unsupported,InvalidParameterValue, …) fails the build immediately, soreal misconfigurations are never masked by silently trying another type.
RunInstancesInputisbuilt once; only
InstanceTypechanges per attempt, so tenancy,host_resource_group_arn, subnet/AZ, license specifications, and block devicemappings are preserved.
instance_types— the creditspecification (forced
standardfor burstable families to avoid AWS'sunlimiteddefault) is derived frominstance_type, which is empty wheninstance_typesis used. Rather than silently launch a t3/t4g entry withsurprise
unlimitedcredits,Preparerejects burstable entries with a clearerror; the feature targets non-burstable capacity-constrained families anyway.
(If burstable support is ever wanted, the correct fix is per-candidate credit
derivation inside the fallback loop — not relaxing this guard.)
builder/common(aws-sdk-go v1,amazon-instance) andcommon(aws-sdk-go v2, the EBS builders) in lockstep;the field, validation, and fallback loop are added to both so all four
builders behave consistently and the generated docs (driven off the v1
partial) stay correct.
Alternatives considered:
instance_type_fallbacksas a supplement (rejected —two knobs for one concept); Spot support (out of scope —
spot_instance_typesalready exists); a broader capacity-code set (rejected — kept to unambiguous
insufficient-capacity codes to avoid masking config errors).
Risk
StepRunSourceInstance.Runlaunch path used by all on-demand builders. Behavior is unchanged unless the
new
instance_typesfield is set.wraps the existing
RunInstancesretry in an ordered loop without alteringthe single-type path; the classifier is conservative (capacity codes only).
instance_typesunset thecandidate list is
[instance_type], so the loop runs exactly one iterationissuing the same
RunInstancescall with the same per-attemptiamInstanceProfileretry as before — byte-for-byte identical behavior forexisting templates. The only new behavior is gated entirely on the new field.
The classifier is allow-list based, so only the three insufficient-capacity
codes trigger fall-through; anything else returns immediately, preserving
today's fail-fast semantics. Unit tests cover the classifier (capacity vs
non-capacity codes, wrapped errors, nil) and the fallback loop (first success,
fall-through, all-exhausted returns the last error, non-capacity aborts
immediately, single-type path) with a mocked EC2 client; the loop is extracted
behind the
ec2iface.EC2API(v1) /clients.Ec2Client(v2) interface forisolated testing. HCL2 spec, struct-markdown partials, and compiled web-docs
were regenerated via
make generate.Testing
make test(unit,-race) — green.awserrorscapacity classifier;runInstanceWithFallbackordering/selection;
instance_typesvalidation — in bothbuilder/common(v1) and
common(v2).InsufficientInstanceCapacityagainst real AWS isn't practical; thefall-through logic is fully covered by unit tests with a mocked client.
Pre-Submission Review
Two automated review passes were run.
First pass (PR-review + simplify, before opening):
instance_typesto prevent a surpriseunlimited-credits launch (credit spec isinstance_type-derived).runInstanceWithFallbackreturns a clear error instead of(nil, nil)on anempty candidate list (defensive).
IsCapacityErrorswitched toerrors.Asfor parity with v2 and torecognize wrapped capacity errors.
"returns the last error" assertion; wrapped-capacity-error classifier cases;
error-message content assertions on the validation tests; burstable rejection.
(matching the existing
reShutdownBehaviorprecedent); aligned the duplicatedv1/v2
IsCapacityErrordoc comment.Second pass (full multi-aspect review — correctness, tests, silent-failure,
comments, simplification). No critical/important correctness issues were found
(v1/v2 confirmed in lockstep, backward-compat byte-for-byte). Findings addressed:
(:?→(?:(non-capturing group); it now servesas a proper validation gate rather than matching an optional literal colon.
reBurstableInstanceTypeto match the adjacentreShutdownBehaviornaming precedent.runInstanceWithFallbacknow wraps the last errorwith the list of types tried (via
%w) so the persisted failure shows thefallback ran and was exhausted; the single-type path returns the raw error
unchanged (preserving byte-for-byte backward compatibility).
placement fields) and the hardware narrative from the
InstanceTypesfielddoc; regenerated partials and web-docs.
instance_type+spot_instance_typesandall-three-selector exclusivity; zero-selector error-message assertion; and an
assertion that the exhaustion error names the types tried.
Instancesguard on thesuccess path (pre-existing, no real AWS path), per-entry nitro/credit
validation for list types (outside the metal/GPU use case), and a full
Run()-level wiring test (heavy state bag; the selection helper is covered).Scoping note:
instance_typestill carriesrequired:"true"in its struct tag.It is already not strictly required (the pre-existing
spot_instance_typesis analternative), and
Prepareenforces the real "exactly one of" rule; flippingthe tag would move
instance_typebetween the required/not-required generateddoc partials for every builder, so it's left out of this PR's scope — happy to
change it if preferred.