Repository navigation
fix(aws): create subnets in an availability zone that offers the instance types - #878
abrarshivani wants to merge 1 commit into
Conversation
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.
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.
Coverage Report for CI Build 38002756987Coverage increased (+1.4%) to 51.849%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. Falling back to AWS zone placement on UnauthorizedOperation keeps create and dryrun working for identities that lack the two new permissions, while a pinned availabilityZone still fails loudly.
The real-AWS smoke job now passes through the fallback (its log carries the new warning for g4dn.xlarge), so the zone-aware path is exercised only by the fake until cnt-ci gets ec2:DescribeAvailabilityZones and ec2:DescribeInstanceTypeOfferings. Granting those is a follow-up on our side.
b834516 to
bc5023e
Compare
|
Thanks. Sounds good. |
…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. 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. An identity without the ec2:DescribeAvailabilityZones or ec2:DescribeInstanceTypeOfferings permission gets UnauthorizedOperation from the zone discovery calls. In that case, and only when no availabilityZone is set, the pre-flight logs a warning naming both permissions and leaves the zone unselected, so the subnets are created without AvailabilityZone and AWS chooses it as before. A pinned availabilityZone fails instead, because it cannot be validated, and any other error from the discovery calls still fails the pre-flight. The subnet functions 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. The selected zone is stored as an availability-zone property in .status.properties for single-node environments and clusters, and holodeck describe shows it in the provider section and in its JSON and YAML output as provider.availabilityZone. Existing property names are unchanged, and a cache file written before this change has no zone, which describe omits. To test this, the EC2Client interface, the fake and the mock client gain DescribeAvailabilityZones and DescribeInstanceTypeOfferings. The fake seeds us-west-2a through us-west-2d, can restrict an instance type to some zones, add Local Zones or change a zone's state, honours the zone-type, state and instance-type filters, and pages offerings two at a time so callers have to follow NextToken. Tests that run the pre-flight against the fake use us-west-2, the only region whose zones it knows, and the mock e2e suite sets that region after loading its shared configs. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
bc5023e to
33c4768
Compare
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.