AWS resolve config on start and add more options#637
Open
6543 wants to merge 14 commits into
Open
Conversation
10 tasks
Port of woodpecker-ci#637 onto current main: - aws-instance-type becomes a string slice, entries in 'type' or 'type:region' form, tried in order as deploy fallbacks - AMI and instance types are resolved and validated at startup (arch match, region offering) - deprecated aws-user-data handling was already removed on main (woodpecker-ci#633), the port keeps the new RenderUserDataTemplate signature
Match the provider layout used by hetznercloud (woodpecker-ci#605) and scaleway (woodpecker-ci#636).
AMI IDs and subnet IDs only exist in one region. A deploy candidate pointing at a foreign region would pass startup (only the instance type offering was checked) and then every RunInstances call in that region fails with InvalidAMIID/InvalidSubnetID. Those errors are not InsufficientInstanceCapacity, so the whole fallback chain aborts instead of moving on. Check DescribeImages and DescribeSubnets against each distinct candidate region during resolveDeployCandidates so misconfiguration surfaces at startup with an actionable error.
The wait loop after RunInstances discarded the ListDeployedAgentNames error and returned a static message, hiding the actual API failure. Wrap the underlying error instead.
Agents launched through a fallback candidate in another region were invisible to getAgent and ListDeployedAgentNames, which only queried the client's default region. The deploy wait loop then reported the agent as missing and the instance kept running unmanaged, billing forever with no way to remove it. Resolve the list of deploy regions once at startup and reuse two small helpers everywhere a region matters: - regionOpt(region) targets a region, no-op for the default one; also replaces the inline closures in the startup checks and the deploy loop - instancesByTag(region, tag, value) wraps DescribeInstances and flattens the reservation layer, shared by getAgent and ListDeployedAgentNames getAgent additionally returns the region the instance was found in so RemoveAgent terminates it against the right endpoint.
A deploy candidate now carries every region-scoped resource it deploys with (AMI, subnets) instead of reusing the provider-global values, which are only valid in the default region. All parts of the tuple are resolved and validated at startup for the candidate's own region, so a tuple that can never deploy fails fast instead of aborting the fallback chain at deploy time. This also validates the configured subnets for default-region candidates, which was previously skipped. The tuple shape prepares for a later capability match, where candidates are selected per agent by platform (os/arch) and backend.
Security group IDs are region-scoped like AMIs and subnets. Resolve them per candidate region and deploy each candidate with its own set, so a bad security group surfaces as a startup error instead of a non-capacity RunInstances error that aborts the fallback chain.
InsufficientInstanceCapacity is not the only code AWS returns when the requested capacity is unavailable: unsupported type in the picked zone and the spot-specific limits surface as different codes. With the old exact match the fallback chain aborted on errors it exists to handle, especially with spot instances. Also make the error after the last candidate say that the whole chain was exhausted instead of repeating the plain RunInstances error.
Instance type details were always fetched from the default region and the architecture was compared against the default-region AMI. Resolve the type in the candidate's own region and compare against the candidate's own AMI, matching the tuple semantics.
DescribeInstances also returns terminated instances, whose tags linger for a while. A freshly terminated instance with the same Name tag as a new one tripped getAgent's single-instance guard and made RemoveAgent fail spuriously. Filter by instance state server-side in instancesByTag so every lookup behaves like ListDeployedAgentNames already did, and drop the now redundant client-side state check there.
Introduce a small ec2api.Client interface covering the EC2 calls the provider makes, so the AWS provider is testable with a mockery mock like the hetznercloud provider already is. Cover the deploy candidate tuple resolution and its startup errors, the capacity error classification, the deploy fallback chain and the region-aware, state-filtered agent lookup.
Member
Author
|
claude review told me about "cross-region fallback fundamentally broken" ... so i did some back and forth and told it to do a tuple ... |
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.
finish provider refactoring with this