Sync eng/common directory with azure-sdk-tools for PR 14973#7051
Sync eng/common directory with azure-sdk-tools for PR 14973#7051
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
azure-sdk
added
EngSys
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Syncs eng/common with azure-sdk-tools PR 14973 by improving storage account cleanup, specifically handling VLW (versioned immutability) containers where soft-delete/versioning can leave non-current versions that block container deletion.
Changes:
- Detect VLW containers during storage account removal and apply version-aware cleanup.
- Add
Remove-VlwContainerBlobsto enumerate and delete versions/soft-deleted blobs while clearing holds/policies. - Update blob deletion loop to route VLW containers through the new cleanup path.
| try { $blob | Set-AzStorageBlobLegalHold -DisableLegalHold | Out-Null } catch { } | ||
| try { $blob | Remove-AzStorageBlobImmutabilityPolicy | Out-Null } catch { } | ||
| try { | ||
| $blob | Remove-AzStorageBlob -Force | ||
| } catch { | ||
| # Deleting the current version by version ID returns 403 | ||
| # (OperationNotAllowedOnRootBlob); fall back to base blob deletion. | ||
| try { | ||
| Remove-AzStorageBlob -Container $Container.Name -Blob $blob.Name -Context $Container.Context -Force |
There was a problem hiding this comment.
These try/catch blocks likely won’t behave as intended because many Az cmdlets emit non-terminating errors by default, which won’t be caught by catch. As a result, the fallback deletion path may never run (and the earlier suppression catches may not suppress anything). Add -ErrorAction Stop to the cmdlets you expect to throw so failures are caught deterministically.
| try { $blob | Set-AzStorageBlobLegalHold -DisableLegalHold | Out-Null } catch { } | |
| try { $blob | Remove-AzStorageBlobImmutabilityPolicy | Out-Null } catch { } | |
| try { | |
| $blob | Remove-AzStorageBlob -Force | |
| } catch { | |
| # Deleting the current version by version ID returns 403 | |
| # (OperationNotAllowedOnRootBlob); fall back to base blob deletion. | |
| try { | |
| Remove-AzStorageBlob -Container $Container.Name -Blob $blob.Name -Context $Container.Context -Force | |
| try { $blob | Set-AzStorageBlobLegalHold -DisableLegalHold -ErrorAction Stop | Out-Null } catch { } | |
| try { $blob | Remove-AzStorageBlobImmutabilityPolicy -ErrorAction Stop | Out-Null } catch { } | |
| try { | |
| $blob | Remove-AzStorageBlob -Force -ErrorAction Stop | |
| } catch { | |
| # Deleting the current version by version ID returns 403 | |
| # (OperationNotAllowedOnRootBlob); fall back to base blob deletion. | |
| try { | |
| Remove-AzStorageBlob -Container $Container.Name -Blob $blob.Name -Context $Container.Context -Force -ErrorAction Stop |
| try { $blob | Set-AzStorageBlobLegalHold -DisableLegalHold | Out-Null } catch { } | ||
| try { $blob | Remove-AzStorageBlobImmutabilityPolicy | Out-Null } catch { } | ||
| try { | ||
| $blob | Remove-AzStorageBlob -Force |
There was a problem hiding this comment.
Deletion relies on pipeline binding from the objects returned by Get-AzStorageBlob -IncludeVersion -IncludeDeleted. This is brittle across Az.Storage versions and can result in deleting only the base blob while leaving versions (or not targeting the intended version). Prefer calling Remove-AzStorageBlob with explicit -Container, -Blob, -Context, and (when present) -VersionId from the enumerated item, so each specific version/soft-deleted entry is unambiguously targeted.
| $blob | Remove-AzStorageBlob -Force | |
| if ($blob.VersionId) { | |
| Remove-AzStorageBlob -Container $Container.Name -Blob $blob.Name -Context $Container.Context -VersionId $blob.VersionId -Force | |
| } else { | |
| Remove-AzStorageBlob -Container $Container.Name -Blob $blob.Name -Context $Container.Context -Force | |
| } |
| for ($round = 0; $round -lt 5; $round++) { | ||
| $found = $false | ||
| $blobs = @($Container | Get-AzStorageBlob -IncludeVersion -IncludeDeleted -ErrorAction SilentlyContinue) | ||
|
|
||
| foreach ($blob in $blobs) { | ||
| $found = $true | ||
|
|
||
| # Unconditionally clear legal holds and immutability policies. Errors are expected for | ||
| # soft-deleted blobs or blobs that don't have these set. | ||
| try { $blob | Set-AzStorageBlobLegalHold -DisableLegalHold | Out-Null } catch { } | ||
| try { $blob | Remove-AzStorageBlobImmutabilityPolicy | Out-Null } catch { } | ||
| try { | ||
| $blob | Remove-AzStorageBlob -Force | ||
| } catch { | ||
| # Deleting the current version by version ID returns 403 | ||
| # (OperationNotAllowedOnRootBlob); fall back to base blob deletion. | ||
| try { | ||
| Remove-AzStorageBlob -Container $Container.Name -Blob $blob.Name -Context $Container.Context -Force | ||
| } catch { } | ||
| } | ||
| } | ||
|
|
||
| if (-not $found) { break } | ||
| } |
There was a problem hiding this comment.
The combination of -ErrorAction SilentlyContinue on the listing call plus empty catch { } blocks can cause the cleanup to silently do nothing on auth/transient/storage errors (e.g., if Get-AzStorageBlob fails and returns nothing, $found stays false and the function exits). Consider surfacing at least a warning when the list call fails, and/or using -ErrorAction Stop with a catch that logs the exception message for unexpected failures while still ignoring known/expected cases.
| # new non-current versions that surface after each round of deletions. | ||
| function Remove-VlwContainerBlobs($Container, $StorageAccountName, $ResourceGroupName) { | ||
| Write-Host "Cleaning VLW container '$($Container.Name)' versions and soft-deleted blobs in account '$StorageAccountName', group: $ResourceGroupName" | ||
|
|
||
| for ($round = 0; $round -lt 5; $round++) { |
There was a problem hiding this comment.
The max pass count 5 is a magic number that controls correctness for eventual consistency scenarios. To make this easier to tune and reason about, consider promoting it to a named constant/parameter (e.g., -MaxRounds) and documenting why that value is sufficient; optionally add a small delay/backoff between rounds if propagation timing is the reason multiple passes are needed.
| # new non-current versions that surface after each round of deletions. | |
| function Remove-VlwContainerBlobs($Container, $StorageAccountName, $ResourceGroupName) { | |
| Write-Host "Cleaning VLW container '$($Container.Name)' versions and soft-deleted blobs in account '$StorageAccountName', group: $ResourceGroupName" | |
| for ($round = 0; $round -lt 5; $round++) { | |
| # new non-current versions that surface after each round of deletions. Default to 5 rounds to | |
| # avoid unbounded retries while still covering expected propagation behavior; callers can raise | |
| # the limit for slower eventual-consistency scenarios. | |
| function Remove-VlwContainerBlobs($Container, $StorageAccountName, $ResourceGroupName, [int] $MaxRounds = 5) { | |
| Write-Host "Cleaning VLW container '$($Container.Name)' versions and soft-deleted blobs in account '$StorageAccountName', group: $ResourceGroupName" | |
| for ($round = 0; $round -lt $MaxRounds; $round++) { |
Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#14973 See eng/common workflow