From c44948e943ae540a9f1d74f125e6b041be87ea0a Mon Sep 17 00:00:00 2001 From: Ganeshkumar Ashokavardhanan Date: Wed, 22 Apr 2026 09:54:33 -0700 Subject: [PATCH 1/3] fix(e2e): wait on RegionalReplicationStatus.State, not just TargetRegions E2E was reporting "Replication took: 12m" via ensureReplication then immediately failing on VMSS create with `404 GalleryImageNotFound` for ~10 more minutes. Root cause: replicatedToCurrentRegion only checks PublishingProfile.TargetRegions (intent), and the parent LRO returns Succeeded before per-region replicas are actually serving traffic. The vmss.go retry loop (10 x 5s) was far too short to absorb the gap and surfaced the infra failure as `failed to create VMSS after 10 retries`, indistinguishable from a real test failure. Fix: - ensureReplication now polls Get with Expand=ReplicationStatus until RegionalReplicationStatus.State == Completed (20 min timeout, 15s interval). Fails fast on terminal Failed. - CreateVMSSWithRetry no longer burns fixed retries on 404 GalleryImageNotFound. It calls the replication poller, which either returns quickly when the region is Completed (fabric-eventual- consistency race -> retry once), blocks until Completed, or fails fast on Failed with the real cause attached. - Add a public WaitForImageVersionReplicatedToRegion entrypoint and unit tests for the region-name normalization in findRegionalReplicationStatus. This should eliminate the GalleryImageNotFound class of false E2E failures across Linux VHD, Windows VHD, GPU, and base AgentBaker E2E pipelines. --- e2e/config/azure.go | 140 ++++++++++++++++++++++++++- e2e/config/azure_replication_test.go | 64 ++++++++++++ e2e/vmss.go | 43 +++++--- 3 files changed, 231 insertions(+), 16 deletions(-) create mode 100644 e2e/config/azure_replication_test.go diff --git a/e2e/config/azure.go b/e2e/config/azure.go index 2db59bcb75a..da56cd25959 100644 --- a/e2e/config/azure.go +++ b/e2e/config/azure.go @@ -659,8 +659,13 @@ func (a *AzureClient) ensureReplication(ctx context.Context, image *Image, versi } if replicatedToCurrentRegion(version, location) { - toolkit.Logf(ctx, "Image version %s is already in region %s", *version.ID, location) - return nil + // Intent-to-replicate is registered in PublishingProfile.TargetRegions, but that + // does NOT mean the regional replica is actually serving traffic. The regional + // replication state can still be "Replicating" while the parent ProvisioningState + // is "Succeeded" — this is the source of GalleryImageNotFound 404s on VMSS create. + // Wait for the regional state itself before declaring success. + toolkit.Logf(ctx, "Image version %s is already a target of region %s; verifying regional replication state", *version.ID, location) + return a.waitForRegionalReplicationCompleted(ctx, image, version, location) } regions := make([]string, 0, len(version.Properties.PublishingProfile.TargetRegions)) for _, targetRegion := range version.Properties.PublishingProfile.TargetRegions { @@ -670,12 +675,114 @@ func (a *AzureClient) ensureReplication(ctx context.Context, image *Image, versi toolkit.Logf(ctx, "##vso[task.logissue type=warning;]Replicating to region %s", location) start := time.Now() // Record the start time - err := a.replicateImageVersionToCurrentRegion(ctx, image, version, location) + if err := a.replicateImageVersionToCurrentRegion(ctx, image, version, location); err != nil { + return err + } + + // The replicate LRO above completes when the parent resource is Succeeded, but the + // regional replica may still be in "Replicating" state for several more minutes. + // Block until that regional state hits Completed; otherwise downstream VMSS create + // will see GalleryImageNotFound and the test will appear to fail for a non-test reason. + if err := a.waitForRegionalReplicationCompleted(ctx, image, version, location); err != nil { + return err + } elapsed := time.Since(start) // Calculate the elapsed time toolkit.LogDuration(ctx, elapsed, 3*time.Minute, fmt.Sprintf("Replication took: %s (%s)", elapsed, *version.ID)) - return err + return nil +} + +// waitForRegionalReplicationCompleted polls the gallery image version with the +// ReplicationStatus expand option until the named region's RegionalReplicationStatus.State +// is Completed. Returns an error if the regional replication enters a terminal Failed state +// or the wait times out. Treats Unknown as transient (Azure occasionally emits Unknown +// briefly during state transitions). +// +// This closes a documented gap in the SIG API: the parent provisioning LRO can succeed +// before per-region replicas are actually serving traffic. Without this wait, callers see +// GalleryImageNotFound 404s on VMSS creation that look like bugs but are infra eventual +// consistency. +func (a *AzureClient) waitForRegionalReplicationCompleted(ctx context.Context, image *Image, version *armcompute.GalleryImageVersion, location string) error { + imgVersionClient, err := armcompute.NewGalleryImageVersionsClient(image.Gallery.SubscriptionID, a.Credential, a.ArmOptions) + if err != nil { + return fmt.Errorf("create image version client for replication wait: %w", err) + } + + getOpts := &armcompute.GalleryImageVersionsClientGetOptions{ + Expand: to.Ptr(armcompute.ReplicationStatusTypesReplicationStatus), + } + + var lastLoggedState armcompute.ReplicationState + const ( + pollInterval = 15 * time.Second + pollTimeout = 20 * time.Minute + ) + + pollErr := wait.PollUntilContextTimeout(ctx, pollInterval, pollTimeout, true, func(ctx context.Context) (bool, error) { + resp, err := imgVersionClient.Get(ctx, image.Gallery.ResourceGroupName, image.Gallery.Name, image.Name, *version.Name, getOpts) + if err != nil { + // Transient API errors should not abort the wait; the SDK already retries throttling. + toolkit.Logf(ctx, "transient error getting image version replication status (will retry): %v", err) + return false, nil + } + + regional := findRegionalReplicationStatus(resp.GalleryImageVersion.Properties.ReplicationStatus, location) + if regional == nil || regional.State == nil { + // Region not yet present in the status summary; keep waiting. + return false, nil + } + + state := *regional.State + if state != lastLoggedState { + progress := int32(0) + if regional.Progress != nil { + progress = *regional.Progress + } + toolkit.Logf(ctx, "Image version %s regional replication state in %s: %s (progress: %d%%)", *version.ID, location, state, progress) + lastLoggedState = state + } + + switch state { + case armcompute.ReplicationStateCompleted: + return true, nil + case armcompute.ReplicationStateFailed: + details := "" + if regional.Details != nil { + details = *regional.Details + } + return false, fmt.Errorf("regional replication of %s to %s failed: %s", *version.ID, location, details) + case armcompute.ReplicationStateReplicating, armcompute.ReplicationStateUnknown: + return false, nil + default: + // Forward-compat: unknown future states treated as in-progress. + return false, nil + } + }) + + if pollErr != nil { + return fmt.Errorf("waiting for regional replication of %s to %s to complete: %w", *version.ID, location, pollErr) + } + toolkit.Logf(ctx, "Image version %s regional replication to %s confirmed Completed", *version.ID, location) + return nil +} + +// findRegionalReplicationStatus finds the RegionalReplicationStatus for the given location +// (case-insensitive, ignoring spaces) within the version-wide ReplicationStatus. +func findRegionalReplicationStatus(status *armcompute.ReplicationStatus, location string) *armcompute.RegionalReplicationStatus { + if status == nil { + return nil + } + normalized := strings.ToLower(strings.ReplaceAll(location, " ", "")) + for _, regional := range status.Summary { + if regional == nil || regional.Region == nil { + continue + } + if strings.ToLower(strings.ReplaceAll(*regional.Region, " ", "")) == normalized { + return regional + } + } + return nil } func (a *AzureClient) waitForVersionOperationCompletion(ctx context.Context, image *Image, version *armcompute.GalleryImageVersion) error { @@ -783,6 +890,31 @@ func (a *AzureClient) EnsureSIGImageVersion(ctx context.Context, image *Image, l return VHDResourceID(*resp.ID), nil } +// WaitForImageVersionReplicatedToRegion is the public entrypoint for callers (e.g. the +// VMSS create retry loop) that need to confirm a previously-resolved image is actually +// serving traffic in a region before retrying a request that just failed with +// GalleryImageNotFound. It fetches the live image version with ReplicationStatus +// expanded and blocks until the regional state is Completed (or fails fast on terminal +// Failed). It does not initiate replication; callers should have already gone through +// EnsureSIGImageVersion / LatestSIGImageVersionByTag. +func (a *AzureClient) WaitForImageVersionReplicatedToRegion(ctx context.Context, image *Image, location string) error { + if image == nil { + return fmt.Errorf("nil image") + } + if image.Version == "" { + return fmt.Errorf("image %s has no resolved version; cannot check replication state", image.Name) + } + imgVersionClient, err := armcompute.NewGalleryImageVersionsClient(image.Gallery.SubscriptionID, a.Credential, a.ArmOptions) + if err != nil { + return fmt.Errorf("create image version client: %w", err) + } + resp, err := imgVersionClient.Get(ctx, image.Gallery.ResourceGroupName, image.Gallery.Name, image.Name, image.Version, nil) + if err != nil { + return fmt.Errorf("get image version %s/%s for replication-wait: %w", image.Name, image.Version, err) + } + return a.waitForRegionalReplicationCompleted(ctx, image, &resp.GalleryImageVersion, location) +} + func DefaultRetryOpts() policy.RetryOptions { return policy.RetryOptions{ // Use generous retry settings to survive Azure Compute Gallery throttling. diff --git a/e2e/config/azure_replication_test.go b/e2e/config/azure_replication_test.go new file mode 100644 index 00000000000..618d448c469 --- /dev/null +++ b/e2e/config/azure_replication_test.go @@ -0,0 +1,64 @@ +package config + +import ( + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v7" +) + +// TestFindRegionalReplicationStatus exercises the location-matching logic used to +// extract the per-region replication state from a SIG image version's ReplicationStatus +// summary. Region names from ARM may be in either the "WestUS 2" form or the "westus2" +// form, and may include arbitrary casing; the lookup must normalize both sides. +func TestFindRegionalReplicationStatus(t *testing.T) { + completed := armcompute.ReplicationStateCompleted + replicating := armcompute.ReplicationStateReplicating + + status := &armcompute.ReplicationStatus{ + Summary: []*armcompute.RegionalReplicationStatus{ + {Region: to.Ptr("East US"), State: &completed, Progress: to.Ptr(int32(100))}, + {Region: to.Ptr("West US 2"), State: &replicating, Progress: to.Ptr(int32(40))}, + {Region: to.Ptr("uaenorth"), State: &completed, Progress: to.Ptr(int32(100))}, + nil, // tolerate nil entries + {Region: nil, State: &completed}, + }, + } + + tests := []struct { + name string + status *armcompute.ReplicationStatus + location string + wantFound bool + wantState armcompute.ReplicationState + wantRegion string + }{ + {name: "nil status", status: nil, location: "westus2"}, + {name: "exact lowercase normalized match (with space in summary)", status: status, location: "westus2", wantFound: true, wantState: replicating, wantRegion: "West US 2"}, + {name: "uppercase input match (with space in summary)", status: status, location: "EASTUS", wantFound: true, wantState: completed, wantRegion: "East US"}, + {name: "input with embedded spaces", status: status, location: "east us", wantFound: true, wantState: completed, wantRegion: "East US"}, + {name: "summary already normalized", status: status, location: "uaenorth", wantFound: true, wantState: completed, wantRegion: "uaenorth"}, + {name: "missing region returns nil", status: status, location: "centralus"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := findRegionalReplicationStatus(tt.status, tt.location) + if !tt.wantFound { + if got != nil { + t.Fatalf("expected nil, got region %q state %v", *got.Region, *got.State) + } + return + } + if got == nil { + t.Fatalf("expected to find region %q, got nil", tt.wantRegion) + } + if got.Region == nil || *got.Region != tt.wantRegion { + t.Errorf("region mismatch: want %q got %v", tt.wantRegion, got.Region) + } + if got.State == nil || *got.State != tt.wantState { + t.Errorf("state mismatch: want %v got %v", tt.wantState, got.State) + } + }) + } +} diff --git a/e2e/vmss.go b/e2e/vmss.go index 786bf7799ae..d6e03d526b6 100644 --- a/e2e/vmss.go +++ b/e2e/vmss.go @@ -317,24 +317,26 @@ func enableScriptlessCompilation(s *Scenario) bool { func CreateVMSSWithRetry(ctx context.Context, s *Scenario) (*ScenarioVM, error) { delay := 5 * time.Second - retryOn := func(err error) bool { + classify := func(err error) (retry bool, isGalleryImageMissing bool) { var respErr *azcore.ResponseError // only retry on Azure API errors with specific error codes if !errors.As(err, &respErr) { - return false + return false, false } - // AllocationFailed sometimes happens for exotic SKUs (new GPUs) with limited availability, sometimes retrying helps - // It's not a quota issue + // AllocationFailed sometimes happens for exotic SKUs (new GPUs) with limited availability, sometimes retrying helps. + // It's not a quota issue. if respErr.StatusCode == 200 && respErr.ErrorCode == "AllocationFailed" { - return true + return true, false } - // GalleryImageNotFound can happen transiently after image replication completes - // due to Azure eventual consistency - the gallery API reports success but the - // compute fabric in the target region hasn't fully propagated the image yet + // GalleryImageNotFound: gallery image version exists and the parent provisioningState is Succeeded, + // but the regional replica is not yet serving traffic. ensureReplication SHOULD have caught this before + // we got here; if we're seeing it on VMSS create, it's either fabric-level eventual consistency (very + // short window) or a regression in the wait. Either way, classify it specially so we can fail fast on + // terminal Failed states instead of burning 10 retries. if respErr.StatusCode == 404 && respErr.ErrorCode == "GalleryImageNotFound" { - return true + return true, true } - return false + return false, false } maxAttempts := 10 @@ -347,11 +349,28 @@ func CreateVMSSWithRetry(ctx context.Context, s *Scenario) (*ScenarioVM, error) return vm, nil } - // not a retryable error - if !retryOn(err) { + retry, isGalleryImageMissing := classify(err) + if !retry { return vm, err } + // For GalleryImageNotFound, defer to the canonical replication-state poller in + // the config package. This either: + // - returns quickly when the regional state is already Completed (fabric-level race; + // fall through to a normal retry), or + // - blocks until the region transitions to Completed (then retry once on success), or + // - fails fast with a clear error if the region is in a terminal Failed state. + if isGalleryImageMissing && s.VHD != nil && s.Runtime != nil && s.Runtime.Cluster != nil && s.Runtime.Cluster.Model != nil && s.Runtime.Cluster.Model.Location != nil { + location := *s.Runtime.Cluster.Model.Location + toolkit.Logf(ctx, "GalleryImageNotFound on VMSS create in %s; consulting regional replication status before retrying", location) + if waitErr := config.Azure.WaitForImageVersionReplicatedToRegion(ctx, s.VHD, location); waitErr != nil { + // Surface the precise infra cause instead of a generic "failed after N retries". + return vm, fmt.Errorf("VMSS create returned GalleryImageNotFound and regional replication is not usable: %w (original error: %v)", waitErr, err) + } + toolkit.Logf(ctx, "Regional replication confirmed Completed; retrying VMSS create immediately") + continue + } + if attempt >= maxAttempts { return vm, fmt.Errorf("failed to create VMSS after %d retries: %w", maxAttempts, err) } From 880777bd09c319f22054bc79ff1526d7056a7c78 Mon Sep 17 00:00:00 2001 From: Ganeshkumar Ashokavardhanan Date: Wed, 29 Jul 2026 14:21:12 -0700 Subject: [PATCH 2/3] fix(e2e): replicate gallery images to a fixed region set so concurrent writers converge publishingProfile.targetRegions is full desired state, not an atomic append. Each E2E writer previously did GET -> append only its own region -> CreateOrUpdate, so writers computed different desired states from possibly stale reads and a concurrent write could silently drop another writer's region. The result was GalleryImageNotFound on VMSS create. PR #8823 addressed this with a sync.Mutex, which only serialises goroutines inside one process and cannot coordinate the separate E2E pipelines that race on the same image version. A lost update is only harmful when writers disagree. Replicating to a fixed set of E2E regions makes every writer - across goroutines, processes and pipelines - submit an identical desired state, so whichever write lands last is still correct and no locking or cross-process coordination is needed. Scenarios now reference the region constants instead of writing literals, so the list is the definition rather than a copy that can drift, and a scenario pinned to an unlisted region fails fast instead of timing out waiting for replication. Replication failures are only fatal when the caller's own region is missing, so contention or a quota on a region a test never uses cannot fail that test. Image versions captured at runtime for a single test stay single-region: they have exactly one writer and are deleted on cleanup. Note this only affects the standalone E2E pipelines that resolve images by tag. The VHD builder pipelines already consume published VHD metadata (#8893) and take a path that never writes to the gallery. Also retains the regional readiness wait from #8374: a version is listed in targetRegions before its regional replica actually serves traffic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 01d66448-4349-475a-975b-5c4ec42f92f3 --- CHANGELOG.md | 59 ++++++ e2e/config/azure.go | 211 +++++++++++++------- e2e/config/azure_replication_test.go | 77 +++++++ e2e/config/regions.go | 75 +++++++ e2e/config/vhd.go | 3 + e2e/scenario_gpu_managed_experience_test.go | 6 +- e2e/scenario_test.go | 24 +-- e2e/test_helpers.go | 7 + 8 files changed, 380 insertions(+), 82 deletions(-) create mode 100644 CHANGELOG.md create mode 100644 e2e/config/regions.go diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 00000000000..4a9ae979d4b --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,59 @@ +# Changelog + +## E2E gallery image replication: convergent target regions + +**Goal**: stop concurrent E2E runs from clobbering each other's gallery image +`publishingProfile.targetRegions`, which surfaces as `GalleryImageNotFound` on VMSS create. + +**Findings** +- `targetRegions` is full desired state, not an atomic append. The previous + GET -> append-my-region -> CreateOrUpdate loop made every writer compute a *different* + desired state, so a stale write silently dropped another writer's region. + `vhdbuilder/packer/replicate-captured-sig-image-version.sh` documents the same + constraint: "SIG API requires specifying a complete set of replication targets". +- A lost update is only harmful when writers disagree. If every writer submits the same + superset, concurrent writes are identical and interleaving stops mattering. +- The region set is not dynamic: every scenario `Location` is a compile-time literal and + `E2E_LOCATION` is never overridden in any pipeline. The whole set is six regions. +- **#8893 already added a write-free path.** The VHD build pipeline publishes + `{resourceId, version, regions}` metadata, and when `E2E_VHD_METADATA_FILE` is set + `GetVHDResourceID` returns early and E2E never writes to the gallery at all. Both VHD + builder pipelines enable it via `useVhdMetadataArtifacts: true`. +- The race therefore only survives in the six standalone E2E pipelines (`e2e.yaml`, + `e2e-gpu.yaml`, `e2e-gpu-azurelinux.yaml`, `e2e-windows.yaml`, `e2e-tme.yaml`, + `e2e-rcv1p-not-opted-in.yaml`), which resolve images by tag and take the legacy path. + This change is a safety net for that path, not the strategic fix. +- Being listed in `targetRegions` does not mean the replica serves traffic; + `RegionalReplicationStatus.State` must be awaited (kept from #8374). + +**Failed attempts (discarded)** +- A two-pass design: a discovery-only `go test` pass computed the exact regions a run + needed, wrote a metadata file, and the real pass consumed it read-only. It worked, but + computed at runtime what is a compile-time constant and forced unrelated changes (VHD + cloning that broke pointer-identity FIPS detection, a lazy RCV1P CSE build to avoid Azure + side effects during discovery, seeding randomized VHD selection across processes). + 668 lines for the same Azure-side result as ~160. +- A narrowing retry (`canNarrowToRequiredRegion`) plus a self-healing repair inside + `WaitForImageVersionReplicatedToRegion`. Both existed only to protect newly added code: + review showed the narrowing reintroduced the very snapshot-derived write this change + removes, and the repair could spend the VMSS context budget before the wait it guarded. + Replaced by a single "did my own region make it?" check. + +**Files changed** +- `e2e/config/regions.go` (new): region constants, `E2EReplicationRegions`, `IsE2ERegion`, + `NormalizeRegion`, `(*Image).replicationRegions`. +- `e2e/config/azure.go`: `ensureReplication` now batches the whole desired set through + `ensureTargetRegions` with a 409/412 re-read-and-merge retry; failure is only fatal when + the caller's own region is missing. Removed `replicateImageVersionToCurrentRegion` and + `replicatedToCurrentRegion`. +- `e2e/config/vhd.go`, `e2e/test_helpers.go`: `Image.Ephemeral` keeps runtime-captured + per-test image versions single-region — they have one writer and are deleted on cleanup. +- `e2e/test_helpers.go`: fail fast when a scenario region is outside the E2E set. +- `e2e/scenario_test.go`, `e2e/scenario_gpu_managed_experience_test.go`: region literals now + reference the constants so the list cannot drift. +- `e2e/config/azure_replication_test.go`: merge, convergence, ephemeral, and region tests. + +**Next step**: confirm whether the `REPLICATIONS` pipeline variable already covers the six +E2E regions. If it does, images arrive pre-replicated, `missing` is always empty, and this +code never writes — which is the outcome to aim for. Longer term, move the six standalone +pipelines onto the #8893 metadata path so they stop writing entirely. diff --git a/e2e/config/azure.go b/e2e/config/azure.go index da56cd25959..972abae1028 100644 --- a/e2e/config/azure.go +++ b/e2e/config/azure.go @@ -652,45 +652,145 @@ func (a *AzureClient) LatestSIGImageVersionByTag(ctx context.Context, image *Ima return VHDResourceID(*latestVersion.ID), nil } +// ensureReplication makes an image version usable in location. For shared images it +// replicates to the whole E2E region set rather than just location: TargetRegions is full +// desired state, so writers that each add only their own region can clobber one another, +// while writers that all submit the same superset converge no matter how their reads and +// writes interleave. func (a *AzureClient) ensureReplication(ctx context.Context, image *Image, version *armcompute.GalleryImageVersion, location string) error { - // Wait for any ongoing update operations to complete first - if err := a.waitForVersionOperationCompletion(ctx, image, version); err != nil { - return fmt.Errorf("waiting for version operation completion: %w", err) + start := time.Now() + + if err := a.ensureTargetRegions(ctx, image, version, image.replicationRegions(location)); err != nil { + // The extra regions only prevent future contention; this test needs one region. + // Fail only if that region did not make it, so a quota, a region disabled on the + // subscription, or contention over regions this test never touches cannot fail it. + live, getErr := a.getImageVersion(ctx, image, *version.Name) + if getErr != nil || !hasTargetRegion(live.Properties.PublishingProfile.TargetRegions, location) { + return err + } + toolkit.Logf(ctx, "Replicating %s to the full E2E region set failed, but %s is a target so continuing: %v", *version.ID, location, err) + *version = *live } - if replicatedToCurrentRegion(version, location) { - // Intent-to-replicate is registered in PublishingProfile.TargetRegions, but that - // does NOT mean the regional replica is actually serving traffic. The regional - // replication state can still be "Replicating" while the parent ProvisioningState - // is "Succeeded" — this is the source of GalleryImageNotFound 404s on VMSS create. - // Wait for the regional state itself before declaring success. - toolkit.Logf(ctx, "Image version %s is already a target of region %s; verifying regional replication state", *version.ID, location) - return a.waitForRegionalReplicationCompleted(ctx, image, version, location) + // Being listed in TargetRegions only records intent. The regional replica can still be + // "Replicating" while the parent ProvisioningState is already "Succeeded" — that gap is + // the source of GalleryImageNotFound 404s on VMSS create, so wait for the region itself. + if err := a.waitForRegionalReplicationCompleted(ctx, image, version, location); err != nil { + return err } - regions := make([]string, 0, len(version.Properties.PublishingProfile.TargetRegions)) - for _, targetRegion := range version.Properties.PublishingProfile.TargetRegions { - regions = append(regions, *targetRegion.Name) + + elapsed := time.Since(start) + toolkit.LogDuration(ctx, elapsed, 3*time.Minute, fmt.Sprintf("Replication took: %s (%s)", elapsed, *version.ID)) + return nil +} + +// ensureTargetRegions adds every missing region in a single update. On conflict it re-reads +// and merges again, because a concurrent writer may have updated the version in between. +func (a *AzureClient) ensureTargetRegions(ctx context.Context, image *Image, version *armcompute.GalleryImageVersion, desiredRegions []string) error { + const maxAttempts = 4 + for attempt := 1; ; attempt++ { + if err := a.waitForVersionOperationCompletion(ctx, image, version); err != nil { + return fmt.Errorf("waiting for version operation completion: %w", err) + } + + targetRegions, missing := mergeTargetRegions(version.Properties.PublishingProfile.TargetRegions, desiredRegions) + if len(missing) == 0 { + return nil + } + + toolkit.Logf(ctx, "Replicating image version %s to missing regions: %s", *version.ID, strings.Join(missing, ", ")) + toolkit.Logf(ctx, "##vso[task.logissue type=warning;]Replicating to regions %s", strings.Join(missing, ", ")) + + previous := version.Properties.PublishingProfile.TargetRegions + version.Properties.PublishingProfile.TargetRegions = targetRegions + err := a.updateImageVersion(ctx, image, version) + if err == nil { + return nil + } + version.Properties.PublishingProfile.TargetRegions = previous + if !isGalleryUpdateConflict(err) || attempt >= maxAttempts { + return err + } + + toolkit.Logf(ctx, "Concurrent update of image version %s; re-reading and merging again", *version.ID) + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(5 * time.Second): + } + + live, err := a.getImageVersion(ctx, image, *version.Name) + if err != nil { + return err + } + *version = *live } - toolkit.Logf(ctx, "Replicating to region %s, available regions: %s, image version %s", location, strings.Join(regions, ", "), *version.ID) - toolkit.Logf(ctx, "##vso[task.logissue type=warning;]Replicating to region %s", location) +} - start := time.Now() // Record the start time - if err := a.replicateImageVersionToCurrentRegion(ctx, image, version, location); err != nil { - return err +func (a *AzureClient) getImageVersion(ctx context.Context, image *Image, version string) (*armcompute.GalleryImageVersion, error) { + client, err := armcompute.NewGalleryImageVersionsClient(image.Gallery.SubscriptionID, a.Credential, a.ArmOptions) + if err != nil { + return nil, fmt.Errorf("create image version client: %w", err) } + resp, err := client.Get(ctx, image.Gallery.ResourceGroupName, image.Gallery.Name, image.Name, version, nil) + if err != nil { + return nil, fmt.Errorf("get image version %s/%s: %w", image.Name, version, err) + } + return &resp.GalleryImageVersion, nil +} - // The replicate LRO above completes when the parent resource is Succeeded, but the - // regional replica may still be in "Replicating" state for several more minutes. - // Block until that regional state hits Completed; otherwise downstream VMSS create - // will see GalleryImageNotFound and the test will appear to fail for a non-test reason. - if err := a.waitForRegionalReplicationCompleted(ctx, image, version, location); err != nil { - return err +func (a *AzureClient) updateImageVersion(ctx context.Context, image *Image, version *armcompute.GalleryImageVersion) error { + client, err := armcompute.NewGalleryImageVersionsClient(image.Gallery.SubscriptionID, a.Credential, a.ArmOptions) + if err != nil { + return fmt.Errorf("create a new images client: %v", err) + } + op, err := client.BeginCreateOrUpdate(ctx, image.Gallery.ResourceGroupName, image.Gallery.Name, image.Name, *version.Name, *version, nil) + if err != nil { + return fmt.Errorf("begin updating image version target regions: %w", err) } - elapsed := time.Since(start) // Calculate the elapsed time + if _, err := op.PollUntilDone(ctx, DefaultPollUntilDoneOptions); err != nil { + return fmt.Errorf("updating image version target regions: %w", err) + } + return nil +} - toolkit.LogDuration(ctx, elapsed, 3*time.Minute, fmt.Sprintf("Replication took: %s (%s)", elapsed, *version.ID)) +// mergeTargetRegions returns the target regions with any missing desired region added, along +// with the names that were missing. Existing entries are preserved as-is so per-region replica +// counts and storage account types configured elsewhere are never rewritten. +func mergeTargetRegions(existing []*armcompute.TargetRegion, desiredRegions []string) ([]*armcompute.TargetRegion, []string) { + merged := append([]*armcompute.TargetRegion(nil), existing...) + known := make(map[string]struct{}, len(existing)) + for _, region := range existing { + if region != nil && region.Name != nil { + known[NormalizeRegion(*region.Name)] = struct{}{} + } + } - return nil + var missing []string + for _, location := range desiredRegions { + normalized := NormalizeRegion(location) + if normalized == "" { + continue + } + if _, exists := known[normalized]; exists { + continue + } + known[normalized] = struct{}{} + missing = append(missing, normalized) + merged = append(merged, &armcompute.TargetRegion{ + Name: to.Ptr(normalized), + RegionalReplicaCount: to.Ptr[int32](1), + StorageAccountType: to.Ptr(armcompute.StorageAccountTypeStandardLRS), + }) + } + slices.Sort(missing) + return merged, missing +} + +func isGalleryUpdateConflict(err error) bool { + var respErr *azcore.ResponseError + return errors.As(err, &respErr) && + (respErr.StatusCode == http.StatusConflict || respErr.StatusCode == http.StatusPreconditionFailed) } // waitForRegionalReplicationCompleted polls the gallery image version with the @@ -773,12 +873,12 @@ func findRegionalReplicationStatus(status *armcompute.ReplicationStatus, locatio if status == nil { return nil } - normalized := strings.ToLower(strings.ReplaceAll(location, " ", "")) + normalized := NormalizeRegion(location) for _, regional := range status.Summary { if regional == nil || regional.Region == nil { continue } - if strings.ToLower(strings.ReplaceAll(*regional.Region, " ", "")) == normalized { + if NormalizeRegion(*regional.Region) == normalized { return regional } } @@ -839,28 +939,6 @@ func (a *AzureClient) waitForVersionOperationCompletion(ctx context.Context, ima return nil } -func (a *AzureClient) replicateImageVersionToCurrentRegion(ctx context.Context, image *Image, version *armcompute.GalleryImageVersion, location string) error { - galleryImageVersion, err := armcompute.NewGalleryImageVersionsClient(image.Gallery.SubscriptionID, a.Credential, a.ArmOptions) - if err != nil { - return fmt.Errorf("create a new images client: %v", err) - } - version.Properties.PublishingProfile.TargetRegions = append(version.Properties.PublishingProfile.TargetRegions, &armcompute.TargetRegion{ - Name: &location, - RegionalReplicaCount: to.Ptr[int32](1), - StorageAccountType: to.Ptr(armcompute.StorageAccountTypeStandardLRS), - }) - - resp, err := galleryImageVersion.BeginCreateOrUpdate(ctx, image.Gallery.ResourceGroupName, image.Gallery.Name, image.Name, *version.Name, *version, nil) - if err != nil { - return fmt.Errorf("begin updating image version target regions: %w", err) - } - if _, err := resp.PollUntilDone(ctx, DefaultPollUntilDoneOptions); err != nil { - return fmt.Errorf("updating image version target regions: %w", err) - } - - return nil -} - func (a *AzureClient) EnsureSIGImageVersion(ctx context.Context, image *Image, location string) (VHDResourceID, error) { galleryImageVersion, err := armcompute.NewGalleryImageVersionsClient(image.Gallery.SubscriptionID, a.Credential, a.ArmOptions) if err != nil { @@ -904,15 +982,23 @@ func (a *AzureClient) WaitForImageVersionReplicatedToRegion(ctx context.Context, if image.Version == "" { return fmt.Errorf("image %s has no resolved version; cannot check replication state", image.Name) } - imgVersionClient, err := armcompute.NewGalleryImageVersionsClient(image.Gallery.SubscriptionID, a.Credential, a.ArmOptions) - if err != nil { - return fmt.Errorf("create image version client: %w", err) - } - resp, err := imgVersionClient.Get(ctx, image.Gallery.ResourceGroupName, image.Gallery.Name, image.Name, image.Version, nil) + live, err := a.getImageVersion(ctx, image, image.Version) if err != nil { return fmt.Errorf("get image version %s/%s for replication-wait: %w", image.Name, image.Version, err) } - return a.waitForRegionalReplicationCompleted(ctx, image, &resp.GalleryImageVersion, location) + return a.waitForRegionalReplicationCompleted(ctx, image, live, location) +} + +// hasTargetRegion reports whether location is already a replication target, comparing region +// names case- and space-insensitively because ARM returns both "West US 2" and "westus2". +func hasTargetRegion(regions []*armcompute.TargetRegion, location string) bool { + normalized := NormalizeRegion(location) + for _, region := range regions { + if region != nil && region.Name != nil && NormalizeRegion(*region.Name) == normalized { + return true + } + } + return false } func DefaultRetryOpts() policy.RetryOptions { @@ -938,15 +1024,6 @@ func DefaultRetryOpts() policy.RetryOptions { } } -func replicatedToCurrentRegion(version *armcompute.GalleryImageVersion, location string) bool { - for _, targetRegion := range version.Properties.PublishingProfile.TargetRegions { - if strings.EqualFold(strings.ReplaceAll(*targetRegion.Name, " ", ""), location) { - return true - } - } - return false -} - // DeleteSIGImageVersion deletes a SIG image version func (a *AzureClient) DeleteSIGImageVersion(ctx context.Context, galleryResourceGroup, galleryName, imageName, version string) { // Ignore errors, don't need to wait for the deletion to complete diff --git a/e2e/config/azure_replication_test.go b/e2e/config/azure_replication_test.go index 618d448c469..d1980f7fc57 100644 --- a/e2e/config/azure_replication_test.go +++ b/e2e/config/azure_replication_test.go @@ -1,10 +1,12 @@ package config import ( + "slices" "testing" "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v7" + "github.com/stretchr/testify/require" ) // TestFindRegionalReplicationStatus exercises the location-matching logic used to @@ -62,3 +64,78 @@ func TestFindRegionalReplicationStatus(t *testing.T) { }) } } + +func regionNames(regions []*armcompute.TargetRegion) []string { + names := make([]string, 0, len(regions)) + for _, region := range regions { + names = append(names, NormalizeRegion(*region.Name)) + } + slices.Sort(names) + return names +} + +func TestMergeTargetRegions(t *testing.T) { + existing := []*armcompute.TargetRegion{ + {Name: to.Ptr("West US 2")}, + {Name: to.Ptr("eastus")}, + } + + merged, missing := mergeTargetRegions(existing, []string{"westus2", "EastUS", "uaenorth", " southeastasia ", ""}) + + require.Equal(t, []string{"southeastasia", "uaenorth"}, missing) + require.Equal(t, []string{"eastus", "southeastasia", "uaenorth", "westus2"}, regionNames(merged)) + // Existing entries must be passed through untouched so replica counts and storage + // account types configured outside E2E are not rewritten. + require.Equal(t, "West US 2", *merged[0].Name) + require.Len(t, existing, 2, "input slice must not be mutated") + + _, missing = mergeTargetRegions(merged, []string{"westus2", "uaenorth"}) + require.Empty(t, missing, "merge must be idempotent once all regions are present") +} + +// TestMergeTargetRegionsConverges is the property the whole design rests on: TargetRegions is +// full desired state, so a lost update is only harmful if writers disagree. Two writers that +// start from different stale snapshots but merge the same desired set produce the same result, +// so whichever write lands last is still correct. +func TestMergeTargetRegionsConverges(t *testing.T) { + desired := []string{"eastus", "westus2", "uaenorth"} + + // Writer A read the version before anyone had replicated. + a, _ := mergeTargetRegions(nil, desired) + // Writer B read it after some other writer had already added westus2. + b, _ := mergeTargetRegions([]*armcompute.TargetRegion{{Name: to.Ptr("westus2")}}, desired) + + require.Equal(t, regionNames(a), regionNames(b)) +} + +func TestE2EReplicationRegionsIncludesDefaultLocation(t *testing.T) { + original := Config.DefaultLocation + t.Cleanup(func() { Config.DefaultLocation = original }) + + Config.DefaultLocation = RegionWestUS3 + require.Equal(t, e2eRegions, E2EReplicationRegions(), "a default location already in the set must not be duplicated") + + Config.DefaultLocation = "North Europe" + require.Contains(t, E2EReplicationRegions(), "northeurope") + require.True(t, IsE2ERegion("northeurope")) + require.False(t, IsE2ERegion("centralindia")) +} + +// TestReplicationRegionsForEphemeralImage guards the cost/blast-radius carve-out: image +// versions captured at runtime for a single test have exactly one writer and are deleted on +// cleanup, so they must not be fanned out to the shared E2E region set. +func TestReplicationRegionsForEphemeralImage(t *testing.T) { + shared := &Image{} + require.Equal(t, append(E2EReplicationRegions(), RegionWestUS2), shared.replicationRegions(RegionWestUS2)) + + ephemeral := &Image{Ephemeral: true} + require.Equal(t, []string{RegionWestUS2}, ephemeral.replicationRegions(RegionWestUS2)) +} + +func TestHasTargetRegion(t *testing.T) { + regions := []*armcompute.TargetRegion{{Name: to.Ptr("West US 2")}, nil, {}} + require.True(t, hasTargetRegion(regions, "westus2")) + require.True(t, hasTargetRegion(regions, "West US 2")) + require.False(t, hasTargetRegion(regions, "eastus")) + require.False(t, hasTargetRegion(nil, "westus2")) +} diff --git a/e2e/config/regions.go b/e2e/config/regions.go new file mode 100644 index 00000000000..fa318c8bbb4 --- /dev/null +++ b/e2e/config/regions.go @@ -0,0 +1,75 @@ +package config + +import "strings" + +// E2E scenarios pin themselves to one of these regions. Reference these constants from +// scenarios rather than writing region literals, so this list stays the single definition +// of where E2E runs instead of a copy that can drift. +const ( + RegionEastUS = "eastus" + RegionSouthCentralUS = "southcentralus" + RegionSouthEastAsia = "southeastasia" + RegionUAENorth = "uaenorth" + RegionWestUS2 = "westus2" + RegionWestUS3 = "westus3" +) + +// e2eRegions is the set of regions every E2E gallery image version is replicated to. +// +// PublishingProfile.TargetRegions is full desired state: an update replaces the region +// list rather than appending to it. Writers that each add only their own region compute +// different desired states from possibly stale reads, so a concurrent write can silently +// drop another writer's region. Replicating to this fixed set instead means every writer - +// across goroutines, test processes, and pipelines - submits an identical desired state, +// so a lost update loses nothing and no locking or cross-process coordination is needed. +var e2eRegions = []string{ + RegionEastUS, + RegionSouthCentralUS, + RegionSouthEastAsia, + RegionUAENorth, + RegionWestUS2, + RegionWestUS3, +} + +// E2EReplicationRegions returns the regions E2E images are replicated to. DefaultLocation is +// included so a local run with a custom E2E_LOCATION still works; CI never overrides it, so +// every pipeline computes the same set. +func E2EReplicationRegions() []string { + regions := make([]string, 0, len(e2eRegions)+1) + regions = append(regions, e2eRegions...) + if location := NormalizeRegion(Config.DefaultLocation); location != "" && !containsRegion(e2eRegions, location) { + regions = append(regions, location) + } + return regions +} + +// IsE2ERegion reports whether scenarios may run in the given region. +func IsE2ERegion(location string) bool { + return containsRegion(E2EReplicationRegions(), NormalizeRegion(location)) +} + +// replicationRegions returns the regions this image version should be replicated to. +func (i *Image) replicationRegions(location string) []string { + if i.Ephemeral { + // Created and deleted by a single test, so it has exactly one writer: there is no + // lost update to defend against and no reason to pay to replicate a throwaway + // captured OS image to regions nothing will read it from. + return []string{location} + } + return append(E2EReplicationRegions(), location) +} + +func containsRegion(regions []string, normalized string) bool { + for _, region := range regions { + if region == normalized { + return true + } + } + return false +} + +// NormalizeRegion converts a region name to the compact lowercase form ARM uses in +// resource IDs, so "West US 2", "westus2" and "WestUS2" compare equal. +func NormalizeRegion(location string) string { + return strings.ToLower(strings.ReplaceAll(strings.TrimSpace(location), " ", "")) +} diff --git a/e2e/config/vhd.go b/e2e/config/vhd.go index 38a1f40df20..b0f8ceeae96 100644 --- a/e2e/config/vhd.go +++ b/e2e/config/vhd.go @@ -321,6 +321,9 @@ type Image struct { IgnoreFailedCgroupTelemetryServices bool Flatcar bool SkipOldVHDValidations bool + // Ephemeral marks an image version created at runtime for a single test and deleted + // on cleanup. It has exactly one writer, so it is replicated only where it is used. + Ephemeral bool // OSDiskSizeGB overrides the default OS disk size (50 GB) when set. OSDiskSizeGB int32 } diff --git a/e2e/scenario_gpu_managed_experience_test.go b/e2e/scenario_gpu_managed_experience_test.go index 371cd37b917..2f82b9958fc 100644 --- a/e2e/scenario_gpu_managed_experience_test.go +++ b/e2e/scenario_gpu_managed_experience_test.go @@ -461,7 +461,7 @@ func Test_Ubuntu2204_NvidiaDevicePluginRunning(t *testing.T) { func Test_AzureLinux3_NvidiaDevicePluginRunning(t *testing.T) { RunScenario(t, &Scenario{ Description: "Tests that NVIDIA device plugin and DCGM Exporter are running & functional on Azure Linux v3 GPU nodes", - Location: "westus2", + Location: config.RegionWestUS2, Tags: Tags{ GPU: true, }, @@ -536,7 +536,7 @@ func Test_AzureLinux3_NvidiaDevicePluginRunning(t *testing.T) { func Test_Ubuntu2404_NvidiaDevicePluginRunning_MIG(t *testing.T) { RunScenario(t, &Scenario{ Description: "Tests that NVIDIA device plugin and DCGM Exporter work with MIG enabled on Ubuntu 24.04 GPU nodes", - Location: "westus2", + Location: config.RegionWestUS2, Tags: Tags{ GPU: true, }, @@ -777,7 +777,7 @@ func Test_CreateVMExtensionLinuxAKSNode_Timing(t *testing.T) { func Test_Ubuntu2404_NvidiaDevicePluginRunning_MIG_Mixed(t *testing.T) { RunScenario(t, &Scenario{ Description: "Tests that NVIDIA device plugin work with MIG Mixed mode on Ubuntu 24.04 GPU nodes", - Location: "westus2", + Location: config.RegionWestUS2, Tags: Tags{ GPU: true, }, diff --git a/e2e/scenario_test.go b/e2e/scenario_test.go index fae5e69ff34..1086612f165 100644 --- a/e2e/scenario_test.go +++ b/e2e/scenario_test.go @@ -263,11 +263,11 @@ func Test_ACL_DisableSSH(t *testing.T) { } func Test_ACL_GPUNC(t *testing.T) { - runScenarioACLGPU(t, "Standard_NC4as_T4_v3", "westus2") + runScenarioACLGPU(t, "Standard_NC4as_T4_v3", config.RegionWestUS2) } func Test_ACL_GPUA100(t *testing.T) { - runScenarioACLGPU(t, "Standard_NC24ads_A100_v4", "westus2") + runScenarioACLGPU(t, "Standard_NC24ads_A100_v4", config.RegionWestUS2) } func Test_ACL_GPUA10(t *testing.T) { @@ -1290,11 +1290,11 @@ func Test_Ubuntu2204_CustomSysctls(t *testing.T) { } func Test_Ubuntu2204_GPUNC(t *testing.T) { - runScenarioUbuntu2204GPU(t, "Standard_NC4as_T4_v3", "westus2") + runScenarioUbuntu2204GPU(t, "Standard_NC4as_T4_v3", config.RegionWestUS2) } func Test_Ubuntu2204_GPUA100(t *testing.T) { - runScenarioUbuntu2204GPU(t, "Standard_NC24ads_A100_v4", "westus2") + runScenarioUbuntu2204GPU(t, "Standard_NC24ads_A100_v4", config.RegionWestUS2) } func Test_Ubuntu2204_GPUA10(t *testing.T) { @@ -1392,7 +1392,7 @@ func Test_Ubuntu2204_GPUGridDriver(t *testing.T) { func Test_Ubuntu2204_GPUNoDriver(t *testing.T) { RunScenario(t, &Scenario{ Description: "Tests that a GPU-enabled node using the Ubuntu 2204 VHD opting for skipping gpu driver installation can be properly bootstrapped", - Location: "westus2", + Location: config.RegionWestUS2, Tags: Tags{ GPU: true, }, @@ -1601,7 +1601,7 @@ func Test_AzureLinuxV3_MA35D(t *testing.T) { }, }, // No MA35D GPU capacity in West US, so using East US - Location: "eastus", + Location: config.RegionEastUS, K8sSystemPoolSKU: "Standard_D2s_v3", }) } @@ -1799,7 +1799,7 @@ func Test_AzureLinuxV3_KubeletCustomConfig(t *testing.T) { func Test_AzureLinuxV3_GPU(t *testing.T) { RunScenario(t, &Scenario{ Description: "Tests that a GPU-enabled node using a AzureLinuxV3 (CgroupV2) VHD can be properly bootstrapped", - Location: "westus2", + Location: config.RegionWestUS2, Tags: Tags{ GPU: true, }, @@ -1857,7 +1857,7 @@ func Test_AzureLinuxV3_GPUA10(t *testing.T) { func Test_AzureLinuxV3_GPUAzureCNI(t *testing.T) { RunScenario(t, &Scenario{ Description: "AzureLinux V3 (CgroupV2) gpu scenario on cluster configured with Azure CNI", - Location: "westus2", + Location: config.RegionWestUS2, Tags: Tags{ GPU: true, }, @@ -2827,7 +2827,7 @@ func Test_Ubuntu2404_SecureTLSBootstrapping_BootstrapToken_Fallback(t *testing.T func Test_Ubuntu2404Gen2_GPUNoDriver(t *testing.T) { RunScenario(t, &Scenario{ Description: "Tests that a GPU-enabled node using the Ubuntu 2404 VHD opting for skipping gpu driver installation can be properly bootstrapped", - Location: "westus2", + Location: config.RegionWestUS2, Tags: Tags{ GPU: true, }, @@ -2941,7 +2941,7 @@ func Test_Ubuntu2404_GPUA10(t *testing.T) { func Test_Ubuntu2404_GPU_RTXPro6000_GridV20(t *testing.T) { RunScenario(t, &Scenario{ Description: "Tests that an RTX PRO 6000 BSE v6 (grid-v20) GPU node on Ubuntu 2404 bootstraps with the aks-gpu-grid-v20 (595.x) driver", - Location: "southeastasia", + Location: config.RegionSouthEastAsia, K8sSystemPoolSKU: "Standard_D2s_v3", Tags: Tags{ GPU: true, @@ -2991,11 +2991,11 @@ func Test_Ubuntu2404_NPD_Basic(t *testing.T) { } func Test_Ubuntu2404_GPU_H100(t *testing.T) { - RunScenario(t, runScenarioUbuntu2404GPUNPD(t, "Standard_ND96isr_H100_v5", "uaenorth", "")) + RunScenario(t, runScenarioUbuntu2404GPUNPD(t, "Standard_ND96isr_H100_v5", config.RegionUAENorth, "")) } func Test_Ubuntu2404_GPU_A100(t *testing.T) { - RunScenario(t, runScenarioUbuntu2404GPUNPD(t, "Standard_ND96asr_v4", "southcentralus", "Standard_D2s_v3")) + RunScenario(t, runScenarioUbuntu2404GPUNPD(t, "Standard_ND96asr_v4", config.RegionSouthCentralUS, "Standard_D2s_v3")) } func Test_AzureLinux3_PMC_Install(t *testing.T) { diff --git a/e2e/test_helpers.go b/e2e/test_helpers.go index edf75233bf4..13b5af7e78b 100644 --- a/e2e/test_helpers.go +++ b/e2e/test_helpers.go @@ -398,6 +398,12 @@ func maybeSkipScenario(ctx context.Context, t testing.TB, s *Scenario) { } } + // Images are replicated to config.E2EReplicationRegions(), so a scenario in any other + // region would never have an image. Fail loudly rather than time out on replication. + if !config.IsE2ERegion(s.Location) { + t.Fatalf("scenario %q runs in region %q, which is not an E2E region; add it to e2eRegions in e2e/config/regions.go", t.Name(), s.Location) + } + _, err := CachedPrepareVHD(ctx, GetVHDRequest{ Image: *s.VHD, Location: s.Location, @@ -938,6 +944,7 @@ func CreateSIGImageVersionFromDisk(ctx context.Context, s *Scenario, version str Name: *gallery.Name, } customVHD.Version = version + customVHD.Ephemeral = true return &customVHD } From e0720e18f419a87c01ba2c456b8a559eaad7f851 Mon Sep 17 00:00:00 2001 From: Ganeshkumar Ashokavardhanan Date: Mon, 3 Aug 2026 12:18:13 -0700 Subject: [PATCH 3/3] fix(e2e): scope the replication region set per image OS No Windows scenario pins a location, so every Windows test runs in the default region. Replicating Windows image versions - the largest in the gallery - to the other five E2E regions cost storage for regions no Windows test can reach. Derive the region set from the image instead of using one global list. Convergence is unaffected: what matters is that every writer of a given image version computes the same desired state, not that all images share one set. The same function now backs both replication and scenario validation, so a scenario pinned to a region its image is not replicated to fails fast naming the exact list to add it to, rather than the two definitions drifting apart. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 01d66448-4349-475a-975b-5c4ec42f92f3 --- CHANGELOG.md | 59 ------------------------- e2e/config/azure_replication_test.go | 29 ++++++++---- e2e/config/regions.go | 66 +++++++++++++++++++--------- e2e/test_helpers.go | 9 ++-- 4 files changed, 70 insertions(+), 93 deletions(-) delete mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md deleted file mode 100644 index 4a9ae979d4b..00000000000 --- a/CHANGELOG.md +++ /dev/null @@ -1,59 +0,0 @@ -# Changelog - -## E2E gallery image replication: convergent target regions - -**Goal**: stop concurrent E2E runs from clobbering each other's gallery image -`publishingProfile.targetRegions`, which surfaces as `GalleryImageNotFound` on VMSS create. - -**Findings** -- `targetRegions` is full desired state, not an atomic append. The previous - GET -> append-my-region -> CreateOrUpdate loop made every writer compute a *different* - desired state, so a stale write silently dropped another writer's region. - `vhdbuilder/packer/replicate-captured-sig-image-version.sh` documents the same - constraint: "SIG API requires specifying a complete set of replication targets". -- A lost update is only harmful when writers disagree. If every writer submits the same - superset, concurrent writes are identical and interleaving stops mattering. -- The region set is not dynamic: every scenario `Location` is a compile-time literal and - `E2E_LOCATION` is never overridden in any pipeline. The whole set is six regions. -- **#8893 already added a write-free path.** The VHD build pipeline publishes - `{resourceId, version, regions}` metadata, and when `E2E_VHD_METADATA_FILE` is set - `GetVHDResourceID` returns early and E2E never writes to the gallery at all. Both VHD - builder pipelines enable it via `useVhdMetadataArtifacts: true`. -- The race therefore only survives in the six standalone E2E pipelines (`e2e.yaml`, - `e2e-gpu.yaml`, `e2e-gpu-azurelinux.yaml`, `e2e-windows.yaml`, `e2e-tme.yaml`, - `e2e-rcv1p-not-opted-in.yaml`), which resolve images by tag and take the legacy path. - This change is a safety net for that path, not the strategic fix. -- Being listed in `targetRegions` does not mean the replica serves traffic; - `RegionalReplicationStatus.State` must be awaited (kept from #8374). - -**Failed attempts (discarded)** -- A two-pass design: a discovery-only `go test` pass computed the exact regions a run - needed, wrote a metadata file, and the real pass consumed it read-only. It worked, but - computed at runtime what is a compile-time constant and forced unrelated changes (VHD - cloning that broke pointer-identity FIPS detection, a lazy RCV1P CSE build to avoid Azure - side effects during discovery, seeding randomized VHD selection across processes). - 668 lines for the same Azure-side result as ~160. -- A narrowing retry (`canNarrowToRequiredRegion`) plus a self-healing repair inside - `WaitForImageVersionReplicatedToRegion`. Both existed only to protect newly added code: - review showed the narrowing reintroduced the very snapshot-derived write this change - removes, and the repair could spend the VMSS context budget before the wait it guarded. - Replaced by a single "did my own region make it?" check. - -**Files changed** -- `e2e/config/regions.go` (new): region constants, `E2EReplicationRegions`, `IsE2ERegion`, - `NormalizeRegion`, `(*Image).replicationRegions`. -- `e2e/config/azure.go`: `ensureReplication` now batches the whole desired set through - `ensureTargetRegions` with a 409/412 re-read-and-merge retry; failure is only fatal when - the caller's own region is missing. Removed `replicateImageVersionToCurrentRegion` and - `replicatedToCurrentRegion`. -- `e2e/config/vhd.go`, `e2e/test_helpers.go`: `Image.Ephemeral` keeps runtime-captured - per-test image versions single-region — they have one writer and are deleted on cleanup. -- `e2e/test_helpers.go`: fail fast when a scenario region is outside the E2E set. -- `e2e/scenario_test.go`, `e2e/scenario_gpu_managed_experience_test.go`: region literals now - reference the constants so the list cannot drift. -- `e2e/config/azure_replication_test.go`: merge, convergence, ephemeral, and region tests. - -**Next step**: confirm whether the `REPLICATIONS` pipeline variable already covers the six -E2E regions. If it does, images arrive pre-replicated, `missing` is always empty, and this -code never writes — which is the outcome to aim for. Longer term, move the six standalone -pipelines onto the #8893 metadata path so they stop writing entirely. diff --git a/e2e/config/azure_replication_test.go b/e2e/config/azure_replication_test.go index d1980f7fc57..2851efd535c 100644 --- a/e2e/config/azure_replication_test.go +++ b/e2e/config/azure_replication_test.go @@ -108,27 +108,38 @@ func TestMergeTargetRegionsConverges(t *testing.T) { require.Equal(t, regionNames(a), regionNames(b)) } -func TestE2EReplicationRegionsIncludesDefaultLocation(t *testing.T) { +func TestE2ERegionsPerImageOS(t *testing.T) { original := Config.DefaultLocation t.Cleanup(func() { Config.DefaultLocation = original }) - Config.DefaultLocation = RegionWestUS3 - require.Equal(t, e2eRegions, E2EReplicationRegions(), "a default location already in the set must not be duplicated") + linux := &Image{OS: OSUbuntu} + require.Equal(t, linuxE2ERegions, linux.E2ERegions()) + require.True(t, linux.SupportsE2ERegion(RegionUAENorth)) + + // No Windows scenario pins a location, so Windows images must not be fanned out to + // regions no Windows test uses - they are the largest images in the gallery. + windows := &Image{OS: OSWindows} + require.Equal(t, []string{RegionWestUS3}, windows.E2ERegions()) + require.False(t, windows.SupportsE2ERegion(RegionUAENorth)) + require.True(t, windows.SupportsE2ERegion("West US 3")) + + // A custom local E2E_LOCATION must still work, without duplicating a known region. Config.DefaultLocation = "North Europe" - require.Contains(t, E2EReplicationRegions(), "northeurope") - require.True(t, IsE2ERegion("northeurope")) - require.False(t, IsE2ERegion("centralindia")) + require.Contains(t, windows.E2ERegions(), "northeurope") + require.True(t, windows.SupportsE2ERegion("northeurope")) + Config.DefaultLocation = RegionWestUS3 + require.Equal(t, windowsE2ERegions, windows.E2ERegions()) } // TestReplicationRegionsForEphemeralImage guards the cost/blast-radius carve-out: image // versions captured at runtime for a single test have exactly one writer and are deleted on // cleanup, so they must not be fanned out to the shared E2E region set. func TestReplicationRegionsForEphemeralImage(t *testing.T) { - shared := &Image{} - require.Equal(t, append(E2EReplicationRegions(), RegionWestUS2), shared.replicationRegions(RegionWestUS2)) + shared := &Image{OS: OSUbuntu} + require.Equal(t, append(shared.E2ERegions(), RegionWestUS2), shared.replicationRegions(RegionWestUS2)) - ephemeral := &Image{Ephemeral: true} + ephemeral := &Image{OS: OSUbuntu, Ephemeral: true} require.Equal(t, []string{RegionWestUS2}, ephemeral.replicationRegions(RegionWestUS2)) } diff --git a/e2e/config/regions.go b/e2e/config/regions.go index fa318c8bbb4..9600d9f4de1 100644 --- a/e2e/config/regions.go +++ b/e2e/config/regions.go @@ -14,15 +14,16 @@ const ( RegionWestUS3 = "westus3" ) -// e2eRegions is the set of regions every E2E gallery image version is replicated to. +// linuxE2ERegions is the set of regions every shared Linux gallery image version is +// replicated to. // -// PublishingProfile.TargetRegions is full desired state: an update replaces the region -// list rather than appending to it. Writers that each add only their own region compute -// different desired states from possibly stale reads, so a concurrent write can silently -// drop another writer's region. Replicating to this fixed set instead means every writer - -// across goroutines, test processes, and pipelines - submits an identical desired state, -// so a lost update loses nothing and no locking or cross-process coordination is needed. -var e2eRegions = []string{ +// PublishingProfile.TargetRegions is full desired state: an update replaces the region list +// rather than appending to it. Writers that each add only their own region compute different +// desired states from possibly stale reads, so a concurrent write can silently drop another +// writer's region. Replicating to a fixed set instead means every writer - across goroutines, +// test processes, and pipelines - submits an identical desired state, so a lost update loses +// nothing and no locking or cross-process coordination is needed. +var linuxE2ERegions = []string{ RegionEastUS, RegionSouthCentralUS, RegionSouthEastAsia, @@ -31,23 +32,33 @@ var e2eRegions = []string{ RegionWestUS3, } -// E2EReplicationRegions returns the regions E2E images are replicated to. DefaultLocation is -// included so a local run with a custom E2E_LOCATION still works; CI never overrides it, so -// every pipeline computes the same set. -func E2EReplicationRegions() []string { - regions := make([]string, 0, len(e2eRegions)+1) - regions = append(regions, e2eRegions...) - if location := NormalizeRegion(Config.DefaultLocation); location != "" && !containsRegion(e2eRegions, location) { +// windowsE2ERegions is deliberately smaller: no Windows scenario pins a location, so they all +// run in the default region. Windows images are by far the largest, and replicating them to +// regions no Windows test uses would cost storage for nothing. Convergence still holds because +// every writer of a Windows image computes this same set. +var windowsE2ERegions = []string{ + RegionWestUS3, +} + +// E2ERegions returns the regions scenarios using this image may run in, and therefore the +// regions the image is replicated to. It backs both the replication set and scenario +// validation, so the two cannot disagree. +func (i *Image) E2ERegions() []string { + base := linuxE2ERegions + if i.OS == OSWindows { + base = windowsE2ERegions + } + + regions := make([]string, 0, len(base)+1) + regions = append(regions, base...) + // A local run may point E2E at a region no scenario names. CI never overrides it, so + // every pipeline still computes the same set. + if location := NormalizeRegion(Config.DefaultLocation); location != "" && !containsRegion(base, location) { regions = append(regions, location) } return regions } -// IsE2ERegion reports whether scenarios may run in the given region. -func IsE2ERegion(location string) bool { - return containsRegion(E2EReplicationRegions(), NormalizeRegion(location)) -} - // replicationRegions returns the regions this image version should be replicated to. func (i *Image) replicationRegions(location string) []string { if i.Ephemeral { @@ -56,7 +67,20 @@ func (i *Image) replicationRegions(location string) []string { // captured OS image to regions nothing will read it from. return []string{location} } - return append(E2EReplicationRegions(), location) + return append(i.E2ERegions(), location) +} + +// SupportsE2ERegion reports whether scenarios using this image may run in the given region. +func (i *Image) SupportsE2ERegion(location string) bool { + return containsRegion(i.E2ERegions(), NormalizeRegion(location)) +} + +// E2ERegionsVarName names the list a new region must be added to, for error messages. +func (i *Image) E2ERegionsVarName() string { + if i.OS == OSWindows { + return "windowsE2ERegions" + } + return "linuxE2ERegions" } func containsRegion(regions []string, normalized string) bool { diff --git a/e2e/test_helpers.go b/e2e/test_helpers.go index 13b5af7e78b..efd87839477 100644 --- a/e2e/test_helpers.go +++ b/e2e/test_helpers.go @@ -398,10 +398,11 @@ func maybeSkipScenario(ctx context.Context, t testing.TB, s *Scenario) { } } - // Images are replicated to config.E2EReplicationRegions(), so a scenario in any other - // region would never have an image. Fail loudly rather than time out on replication. - if !config.IsE2ERegion(s.Location) { - t.Fatalf("scenario %q runs in region %q, which is not an E2E region; add it to e2eRegions in e2e/config/regions.go", t.Name(), s.Location) + // Images are replicated to the regions their scenarios may run in, so a scenario in any + // other region would never have an image. Fail loudly rather than time out on replication. + if !s.VHD.SupportsE2ERegion(s.Location) { + t.Fatalf("scenario %q runs in region %q, which is not an E2E region for %s images; add it to %s in e2e/config/regions.go", + t.Name(), s.Location, s.VHD.OS, s.VHD.E2ERegionsVarName()) } _, err := CachedPrepareVHD(ctx, GetVHDRequest{