Skip to content

Update win-dumpconfigurator.ps1#120

Open
anmocanu wants to merge 5 commits into
Azure:mainfrom
anmocanu:patch-4
Open

Update win-dumpconfigurator.ps1#120
anmocanu wants to merge 5 commits into
Azure:mainfrom
anmocanu:patch-4

Conversation

@anmocanu

Copy link
Copy Markdown
Contributor

.VERSION
    v1.2: [May 2026] - Updated script (current)
                       - Filtered non-actionable kdbgctrl noise from user-facing output.
                       - Added explicit before/after human-readable dump configuration logging.
                       - Added strict post-apply verification and status failure on validation mismatch.
    v1.1: [May 2026] - Updated script
                       - Added intelligent dump placement for Azure temporary storage scenarios.
                       - Added optional pagefile relocation from D: to C: for dump reliability.
                       - Added WMI-based live pagefile auditing and no-reboot workflow.
    v1.0: Initial commit. First working version of the script.
@anmocanu

Copy link
Copy Markdown
Contributor Author

This PR also addresses #121 apart from the Tooling WI.

anmocanu added 3 commits May 27, 2026 11:31
Comment out DumpType and MovePagefile variables for testing.
    v1.2: [May 2026] - Updated script (current)
                       - Added Michael.Smith@microsoft.com as co-author (v1.0 creator)
                       - Changed log file location to $env:PUBLIC\Desktop for uniformity with other scripts
                       - Made automatic reboot configuration optional via -ConfigureAutomaticReboot parameter
                       - Filtered non-actionable kdbgctrl noise from user-facing output.
                       - Added explicit before/after human-readable dump configuration logging.
                       - Added strict post-apply verification and status failure on validation mismatch.
    v1.1: [May 2026] - Updated script
                       - Added intelligent dump placement for Azure temporary storage scenarios.
                       - Added optional pagefile relocation from D: to C: for dump reliability.
                       - Added WMI-based live pagefile auditing and no-reboot workflow.
    v1.0: Initial commit. First working version of the script.
Script now fails if dump type is invalid. DDF location is now changed correctly.
@EdwinBernal1 EdwinBernal1 self-requested a review June 16, 2026 13:22

@EdwinBernal1 EdwinBernal1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Issues Found

🔴 Bug: [switch]$OneDump in Param block conflicts with string comparison

Param(
    [switch]$OneDump,
    ...
)

Later:

if ($OneDump -eq 'true') { ... }

A [switch] parameter is a [bool] — its values are $true/$false, not the string 'true'. When passed via az vm repair run --run-on-repair --parameters OneDump=true, the parameter binding may or may not work correctly depending on how Custom Script Extension passes parameters. The PR comment mentions this was changed for CLI compatibility, but the Param block still declares it as [switch].

Fix: Change [switch]$OneDump to [string]$OneDump = 'false' to match the string comparison pattern. Or use [bool] with proper PowerShell $true/$false handling.

🟡 Concern: Get-WmiObject deprecated in PowerShell 7+

$PagefileSettings = Get-WmiObject -Class Win32_PageFileSetting
$PagefileUsages = Get-WmiObject -Class Win32_PageFileUsage

Get-WmiObject was removed in PowerShell 7.x. While Azure repair VMs currently run Windows PowerShell 5.1, future Azure images may ship with PowerShell 7. Use Get-CimInstance as a drop-in replacement:

$PagefileSettings = Get-CimInstance -ClassName Win32_PageFileSetting

🟡 Concern: Hardcoded D: as temp drive

if (Test-Path "D:\") {
    $TempDriveExists = $true
    $DumpFile = "D:\DedicatedDump.sys"
}

Azure temp drive is usually D: but not always. On some SKUs or configurations (especially VMs with data disks), the temp drive may be E: or another letter. The script should detect the Azure temp drive dynamically, e.g., by checking for the INTELPOD volume label or reading the ResourceDisk.MountPoint from waagent.conf. However, this is a pre-existing limitation and common in the repo's scripts, so it's not a blocker.

🟡 Concern: Pagefile relocation is destructive and not reversible

if ($MovePagefile -eq 'true') {
    $pagefile = Get-WmiObject Win32_PageFileSetting -Filter "Name LIKE '%D:%'"
    if ($pagefile) {
        $pagefile.Delete()
        # Creates new pagefile on C:
    }
}

Moving the pagefile from D: to C: is potentially destructive:

  1. If C: runs out of space, the VM may crash
  2. Restoring pagefile to D: requires another script run or manual intervention
  3. No validation of C: free space before relocating

The -MovePagefile parameter is opt-in (good), but should log a warning about C: space requirements and reversibility.

🟡 Concern: Step numbering jumps from 8 to 10

The output steps jump from Step 8 to Step 10, skipping Step 9. This is cosmetic but will confuse support engineers referencing log output.

🟡 Concern: Debug variables left in source

# $TargetDumpType = 1
# $OneDump = 'true'
# $MovePagefile = 'true'
# $ConfigureAutomaticReboot = 'true'

Commented-out debug variables should be removed before merge. They add noise and risk being accidentally uncommitted.

🟡 Concern: Typo in output

Procceding with pagefile relocation

Should be "Proceeding with pagefile relocation".

🟡 Concern: $DumpFile may be unset

If Test-Path "D:\" returns false and no temp drive is found, $DumpFile is set to "" (empty string). Later:

kdbgctrl -sd $TargetDumpType -df $DumpFile

Passing an empty -df value to kdbgctrl may produce unexpected results. Add a guard:

if ([string]::IsNullOrEmpty($DumpFile)) {
    Log-Warning "No valid dump file path could be determined"
    $DumpFile = "$env:SystemRoot\MEMORY.DMP"  # Windows default
}

🟢 Minor: Scope creep — win-ignoreAllFailures.ps1

The PR modifies win-ignoreAllFailures.ps1 (version bump to v1.2). This is unrelated to dump configuration. While harmless, it makes the PR harder to review and should ideally be in a separate commit or PR.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants