New-TestResources: Give deployment retries a unique name to avoid self-collision - #17155
Connie Yau (conniey) wants to merge 2 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 68 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Retry names can exceed Azure’s 64-character deployment-name limit.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds unique names for resource-deployment retries to prevent collisions with active deployments.
Changes:
- Appends a random retry suffix to deployment names.
- Logs retry and original deployment names.
| File | Description |
|---|---|
eng/common/TestResources/New-TestResources.ps1 |
Generates unique names for retry deployments. |
|
The following pipelines have been queued for testing: |
…ar ARM limit Addresses review feedback on PR Azure#17155: ARM deployment names are capped at 64 characters. The '-retry-<8 hex chars>' suffix adds 15 characters, so a BaseName already near/at 64 chars would push the retry name over the limit and fail name validation on every retry attempt, defeating the fix. Truncate only the base portion so the unique suffix is always preserved within the 64-character limit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce6c37d3-fb5b-4100-8c45-fc8ee31b4170
|
The following pipelines have been queued for testing: |
…nt one on failure Addresses review feedback on PR Azure#17155. The previous fix (retry under a unique name) only avoided the Code=DeploymentActive error message; it didn't stop a second, fully redundant deployment from running concurrently against the same resources as a still-active original, which can itself cause resource-level conflicts and leaves an orphaned duplicate deployment record behind. New flow in Resolve-DeploymentAfterFailure, called when the initial New-AzResourceGroupDeployment throws or returns non-'Succeeded': - Query ARM for the existing deployment by name. - If it's still running, poll it to completion (bounded by a 60 minute timeout) instead of submitting another deployment. - If it already reached a terminal state (Succeeded/Failed/Canceled), return it as-is so normal failure handling reports the real error instead of masking it with a retry. - Only submit a brand new deployment (under a unique name, still respecting the 64-char ARM limit) if no deployment record exists at all, meaning nothing is actually in flight server-side. Also reset to at the start of each template-file iteration so a stale value from a previous template can't leak through if the try block throws without assigning a new one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ce6c37d3-fb5b-4100-8c45-fc8ee31b4170
da363b4 to
262e8b2
Compare
|
The following pipelines have been queued for testing: |
977b9ea to
c9b6b59
Compare
…ng on failure
New-AzResourceGroupDeployment retries reused the same -Name as the
original deployment. If the original was still running server-side
(e.g. a long-running template), the retry collided with it
(Code=DeploymentActive) and every retry failed the same way.
Resolve-DeploymentAfterFailure now asks ARM for the existing
deployment's actual state before deciding what to do:
- still running -> poll it to completion (bounded, 60 min timeout)
instead of starting a redundant concurrent deployment, which
could otherwise cause resource-level conflicts and leave an
orphaned duplicate deployment record behind
- already succeeded/failed/canceled -> return that result as-is so
normal failure handling reports the real error instead of masking
it with a retry
- not found at all -> safe to submit a new deployment under a
unique name, truncating the base name so the result stays within
ARM's 64-character deployment-name limit
Also resets $deployment to $null at the start of each
template-file iteration so a stale value from a previous template
can't leak through if the try block throws without assigning a new
one.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ce6c37d3-fb5b-4100-8c45-fc8ee31b4170
c9b6b59 to
b994f59
Compare
|
The following pipelines have been queued for testing: |
| [int] $PollTimeoutMinutes = 60 | ||
| ) { | ||
| $terminalStates = @('Succeeded', 'Failed', 'Canceled') | ||
| $existing = Get-AzResourceGroupDeployment -ResourceGroupName $ResourceGroupName -Name $DeploymentName -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
A null result could mean:
- deployment genuinely does not exist
- authentication expired
- authorization failed
- ARM was temporarily unavailable
- request was throttled
- network request failed
Should we verify that the result is actually DeploymentNotFound
|
The following pipelines have been queued for testing: |
Deployment retries reused the same
-Name $BaseNameas the original deployment. If the original was still running server-side (e.g. a long-running template), the retry collided with it (Code=DeploymentActive) and every retry failed the same way.Updated fix (see review discussion): instead of blindly starting another deployment on failure,
Resolve-DeploymentAfterFailurenow asks ARM for the existing deployment's actual state first:This avoids concurrent deployments racing each other against the same resources and avoids leaving orphaned duplicate deployment records. Also resets $deployment per template-file iteration so a stale value can't leak through.
Found via an MCP Postgres live-test failure (build 6893002), but applies to any tool with long-running deploys.