Repository navigation
fix(aws): create subnets in an availability zone that offers the instance types - #878
abrarshivani wants to merge 4 commits into
Conversation
Add DescribeInstanceTypeOfferings and DescribeAvailabilityZones to the EC2Client interface so the AWS provider can ask which zones of a region offer an instance type, and implement them in the fake and in the mock client. The fake seeds us-west-2a through us-west-2d as standard zones and, by default, offers every known instance type in all of them. Tests can restrict a type to some zones, add Local Zones, or change a zone's state. DescribeAvailabilityZones honours the zone-type and state filters, and DescribeInstanceTypeOfferings honours the instance-type filter and pages its results two at a time, so callers have to follow NextToken as they must against the real API. An invalid NextToken, including a negative one, returns InvalidNextToken instead of panicking. CreateSubnet now keeps the requested AvailabilityZone on the stored subnet. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
…ance types Holodeck created its subnets without an Availability Zone, so AWS picked one. Not every zone in a region offers every instance type, and the pre-flight check only validated instance types at the region level. The gpu-driver-container precompiled CI hits this with g5g.xlarge in us-west-2: when the subnet lands in us-west-2d, RunInstances fails with Unsupported: Your requested instance type (g5g.xlarge) is not supported in your requested Availability Zone (us-west-2d). after the VPC and its networking already exist, and the rollback that follows can fail with DependencyViolation and leak the VPC. The pre-flight now asks DescribeInstanceTypeOfferings which zones offer the instance types the environment needs. For clusters that is the control-plane type plus the worker type, but only when there are workers to launch, so a control-plane-only cluster is not rejected over a worker type it never uses. Only the region's standard zones in the available state are considered, since Local and Wavelength Zones sort first and support only a subset of services, and the first matching zone in sorted order is chosen. If no zone offers every type, Create(), CreateCluster() and DryRun() fail before anything is created. Every subnet Holodeck creates, including both subnets of a cluster, is placed in the chosen zone, and createSubnet and createPublicSubnet return an error rather than let AWS choose when no zone was selected. A new optional availabilityZone field on the instance and cluster specs pins the zone. The pre-flight rejects it if that zone does not offer the requested types and lists the zones that do. The pre-flight now also needs the ec2:DescribeAvailabilityZones and ec2:DescribeInstanceTypeOfferings IAM permissions, which the prerequisites page now lists. Provider tests that run the pre-flight against the fake use us-west-2, the only region whose zones it knows. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
The zone chosen by the pre-flight only appeared in the create log. Store it as an availability-zone property in .status.properties, next to the VPC and subnet IDs, for both single-node environments and clusters, and read it back when the cache is loaded. holodeck describe shows it in the provider section and in its JSON and YAML output as provider.availabilityZone. The change is additive: existing property names are unchanged, and a cache file written before this change simply has no zone, which describe omits. Delete does not use the property. The mock e2e test now checks that the recorded zone is the one every subnet was created in. Its configs are shared with the real-AWS runs, so rather than edit them the mock suite sets the region to us-west-2, the only region whose zones the fake knows, after loading each config. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The status update loop mutates copied properties and cannot reliably persist or add the selected zone.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds AWS Availability Zone selection based on requested instance-type availability, preventing unsupported instance launches and leaked networking resources.
Changes:
- Selects or validates an Availability Zone during pre-flight checks.
- Pins subnets to the selected zone and persists it in status.
- Extends AWS fakes, tests, CLI output, API types, and documentation.
| File | Description |
|---|---|
tests/e2e_mock_test.go |
Verifies subnet and cached zone consistency. |
pkg/testutil/mocks/aws.go |
Mocks new EC2 discovery operations. |
pkg/provider/aws/status.go |
Persists the selected zone in status. |
pkg/provider/aws/image.go |
Implements zone discovery and selection. |
pkg/provider/aws/image_test.go |
Aligns tests with fake region data. |
pkg/provider/aws/create.go |
Pins created subnets to the selected zone. |
pkg/provider/aws/create_test.go |
Tests single-node zone behavior. |
pkg/provider/aws/cluster.go |
Carries the selected zone into cluster creation. |
pkg/provider/aws/cluster_test.go |
Tests cluster zone intersections and constraints. |
pkg/provider/aws/cache_test.go |
Tests zone cache compatibility. |
pkg/provider/aws/aws.go |
Adds zone state and cache handling. |
pkg/provider/aws/aws_test.go |
Aligns provider tests with seeded zones. |
pkg/provider/aws/aws_ginkgo_test.go |
Extends dry-run and cache assertions. |
internal/aws/ec2_client.go |
Adds EC2 zone-discovery methods. |
internal/aws/awsfake/store.go |
Adds fake zone and offering data. |
internal/aws/awsfake/ec2.go |
Implements fake discovery and subnet recording. |
internal/aws/awsfake/awsfake_test.go |
Tests fake offerings, filtering, and pagination. |
docs/prerequisites.md |
Documents required IAM permissions. |
docs/guides/multinode-clusters.md |
Documents cluster zone configuration. |
docs/commands/create.md |
Documents automatic and pinned selection. |
cmd/cli/describe/describe.go |
Displays the selected zone. |
cmd/cli/describe/describe_test.go |
Tests zone display data. |
api/holodeck/v1alpha1/types.go |
Adds optional zone fields. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| case AvailabilityZone: | ||
| if properties.Value != cache.AvailabilityZone { | ||
| properties.Value = cache.AvailabilityZone | ||
| modified = true | ||
| } |
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Thanks for this. Choosing the subnet zone from instance type offerings fixes a real failure mode where RunInstances would return "Unsupported" after all the VPC networking already existed.
The checkInstanceTypes pre-flight now intersects DescribeInstanceTypeOfferings with the region's available standard zones (Local and Wavelength Zones are skipped) and puts both subnets in that one zone. If no zone offers every requested type, or a pinned availabilityZone doesn't offer them, holodeck create now fails before the VPC is created and lists the zones that would work. The zone is also recorded in status, so holodeck describe shows it.
Approved before CI was checked. E2E Real Smoke fails at the new zone pre-flight with a 403 on ec2:DescribeAvailabilityZones for the CI IAM user, so this is not ready to merge yet. Follow-up review to come.
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
The zone selection itself looks right, but it adds two IAM requirements to create and dryrun, and the real-AWS smoke job shows what that does to an identity without them: E2E Real Smoke fails in under a second with a 403 on DescribeAvailabilityZones for the cnt-ci user. The gpu-operator and device-plugin e2e jobs run under their own IAM identities, so they would hit the same wall the day this merges.
| // Wavelength Zones are excluded: they sort before the region's own zones and | ||
| // support only a subset of AWS services. | ||
| func (p *Provider) zonesOfferingAllInstanceTypes(instanceTypes []string) ([]string, error) { | ||
| zonesOutput, err := p.ec2.DescribeAvailabilityZones(context.TODO(), &ec2.DescribeAvailabilityZonesInput{ |
There was a problem hiding this comment.
An UnauthorizedOperation here (or from DescribeInstanceTypeOfferings below) aborts the whole pre-flight, so every caller lacking the two new permissions loses create entirely. Please fall back to the old behaviour on that error: log a warning, leave selectedAvailabilityZone empty, let createSubnet omit AvailabilityZone, and reject a pinned availabilityZone with a message naming the missing permissions. If you would rather keep it strict, mark the PR as breaking and add an upgrade note, and we will grant the permissions to cnt-ci so the smoke job can go green before merge.
There was a problem hiding this comment.
Agreed. I will keep this non-breaking and fall back to the previous behavior only when the AZ discovery calls fail due to missing permissions. If availabilityZone is explicitly set, we should fail since we can't validate the requested zone. Other discovery errors should still fail the pre-flight. I would still recommend granting the new permissions to cnt-ci so the smoke job exercises the AZ aware path.
…issions The availability zone pre-flight calls DescribeAvailabilityZones and DescribeInstanceTypeOfferings. An identity without the ec2:DescribeAvailabilityZones or ec2:DescribeInstanceTypeOfferings permission gets UnauthorizedOperation from those calls, which aborted the pre-flight, so callers that could create environments before the zone selection change could no longer create anything. The real-AWS CI user is one of them. When either call fails with UnauthorizedOperation and no availabilityZone is set, the pre-flight now logs a warning naming both permissions and leaves the zone unselected, and createSubnet and createPublicSubnet omit AvailabilityZone so that AWS chooses it as before. The region-level instance type check still runs, and dryrun behaves the same way. If availabilityZone is set, the pre-flight fails instead, because the requested zone cannot be validated without those permissions. Any other error from the zone discovery calls still fails the pre-flight. The subnet functions still refuse an empty zone unless the pre-flight recorded this fallback, so a path that skips the pre-flight cannot silently let AWS pick the zone. The error code is read through the ErrorCode method that smithy.APIError defines, which keeps smithy-go an indirect dependency. The prerequisites page lists the two permissions as recommended and describes what happens without them. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
Coverage Report for CI Build 37519463798Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.03%) to 51.849%Details
Uncovered Changes
Coverage Regressions9 previously-covered lines in 7 files lost coverage.
Coverage Stats
💛 - Coveralls |
Description
Create subnets in an Availability Zone that offers the requested instance types, and fail the pre-flight when no zone offers them or the pinned zone doesn't.
Motivation
g5g.xlargein us-west-2, which us-west-2d doesn't offer.Unsupported.DependencyViolationand leaks the VPC.Type of Change
Changes Made
Create,CreateClusterandDryRunbefore creating anything when no zone qualifies.availabilityZonetoinstanceandcluster.holodeck describe.NextTokenvalues.Testing
Unit tests added/updated
E2E tests added/updated
Manual testing performed
Cover partial, missing and pinned zone offerings.
Cover Local, unavailable and constrained zones.
Cover cluster type intersection and zero-worker clusters.
Cover the cache round trip and
describe.Cover zone lookup failures, the missing-permission fallback, and subnet creation without a selected zone.
Use us-west-2, the fake's zone region, in every zone-selection test.
Confirm each new test fails without its fix.
Test Commands Run
Checklist
git commit -s)Additional Notes
ec2:DescribeAvailabilityZonesandec2:DescribeInstanceTypeOfferingsare recommended. Without them Holodeck falls back to AWS choosing the zone, so the real-AWS smoke job exercises zone selection only oncecnt-cihas them.InsufficientInstanceCapacitycan still occur. Retrying another zone could be a follow-up.describeshows the spec region, which can differ from the zone's region underAWS_REGION.make generatedoesn't run on Go 1.26. The new string fields need no deepcopy changes.