Skip to content

Sync eng/common directory with azure-sdk-tools for PR 14973#48731

Open
azure-sdk wants to merge 2 commits intomainfrom
sync-eng/common-storage/supportSoftDeletedBlobsDuringResourceCleanup-14973
Open

Sync eng/common directory with azure-sdk-tools for PR 14973#48731
azure-sdk wants to merge 2 commits intomainfrom
sync-eng/common-storage/supportSoftDeletedBlobsDuringResourceCleanup-14973

Conversation

@azure-sdk
Copy link
Copy Markdown
Collaborator

Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#14973 See eng/common workflow

ibrandes and others added 2 commits April 8, 2026 18:27
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@azure-sdk azure-sdk requested a review from a team as a code owner April 8, 2026 18:40
@azure-sdk azure-sdk requested review from Copilot and ibrandes April 8, 2026 18:40
@azure-sdk azure-sdk added EngSys This issue is impacting the engineering system. Central-EngSys This issue is owned by the Engineering System team. labels Apr 8, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Syncs eng/common with updates from azure-sdk-tools PR 14973, adding special-case cleanup for VLW (Versioned-Level WORM) containers to ensure storage container/account deletion succeeds when versioning + soft-delete leave behind non-current versions.

Changes:

  • Add VLW-aware blob cleanup path during storage account removal.
  • Introduce Remove-VlwContainerBlobs to enumerate and delete blob versions / soft-deleted blobs and clear holds/policies.

Comment on lines +439 to +441
if (($container | Get-Member 'BlobContainerProperties') -and $container.BlobContainerProperties.HasImmutableStorageWithVersioning) {
Remove-VlwContainerBlobs -Container $container -StorageAccountName $Account.StorageAccountName -ResourceGroupName $Account.ResourceGroupName
} else {
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get-Member is relatively expensive and is executed once per container in the deletion loop. Prefer a property existence check via PSObject.Properties (or equivalent) instead of Get-Member so this scales better when many containers are present.

Copilot uses AI. Check for mistakes.

for ($round = 0; $round -lt 5; $round++) {
$found = $false
$blobs = @($Container | Get-AzStorageBlob -IncludeVersion -IncludeDeleted -ErrorAction SilentlyContinue)
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using -ErrorAction SilentlyContinue here can mask real failures (auth/permission issues, transient storage errors). If listing fails, $blobs becomes empty and the function will silently stop, likely leaving the container undeletable. Recommend handling errors explicitly (e.g., capture and emit a warning/error) so the caller has actionable diagnostics when cleanup cannot proceed.

Suggested change
$blobs = @($Container | Get-AzStorageBlob -IncludeVersion -IncludeDeleted -ErrorAction SilentlyContinue)
try {
$blobs = @($Container | Get-AzStorageBlob -IncludeVersion -IncludeDeleted -ErrorAction Stop)
} catch {
Write-Error "Failed to list blobs for VLW container '$($Container.Name)' in storage account '$StorageAccountName', resource group '$ResourceGroupName'. Cleanup cannot continue. $($_.Exception.Message)"
throw
}

Copilot uses AI. Check for mistakes.
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++) {
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hard-coded max pass count (5) is a magic number that’s difficult to tune if behavior changes. Consider making it a parameter with a default (or at least a named constant) and, if the limit is reached while still finding blobs, return/throw a clear failure so the caller knows cleanup may be incomplete.

Copilot uses AI. Check for mistakes.
# immutability policies / legal holds and delete each version individually. Multiple passes handle
# 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"
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write-Host is difficult to control in CI logs (can’t be suppressed via standard preference variables and can be noisy). Prefer Write-Verbose/Write-Information (or whatever this script uses elsewhere) so callers can opt into detailed logging without always emitting host output.

Suggested change
Write-Host "Cleaning VLW container '$($Container.Name)' versions and soft-deleted blobs in account '$StorageAccountName', group: $ResourceGroupName"
Write-Verbose "Cleaning VLW container '$($Container.Name)' versions and soft-deleted blobs in account '$StorageAccountName', group: $ResourceGroupName"

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Central-EngSys This issue is owned by the Engineering System team. EngSys This issue is impacting the engineering system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants