fix(windows-cse): check exit codes of external commands to honour try/catch#8133
Open
timmy-wright wants to merge 1 commit into
Open
fix(windows-cse): check exit codes of external commands to honour try/catch#8133timmy-wright wants to merge 1 commit into
timmy-wright wants to merge 1 commit into
Conversation
timmy-wright
requested review from
AbelHu,
Devinwong,
SriHarsha001,
awesomenix,
cameronmeissner,
djsly,
ganeshkumarashok,
junjiezhang1997,
lilypan26,
phealy,
r2k1,
sulixu,
surajssd,
yagmurbaydogan,
yewmsft and
zachary-bailey
as code owners
timmy-wright
requested review from
calvin197,
mxj220 and
pdamianov-dev
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves Windows CSE reliability by ensuring failures from native executables (which only set $LASTEXITCODE and don’t throw) are surfaced as exceptions so existing try/catch blocks can handle them, and it updates the generated CustomData snapshots accordingly.
Changes:
- Add
$LASTEXITCODEchecks after varioussc.exe,reg.exe,icacls,netsh, andnssm.exeinvocations andthrowon non-zero exit codes. - Ensure
reg.exe importfailures inside an existingtry/catchbecome catchable. - Regenerate/update
pkg/agent/testdata/**/CustomDatasnapshots to reflect script changes.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| staging/cse/windows/windowsciliumnetworkingfunc.ps1 | Throw on non-zero exit code after invoking the Windows Cilium install script. |
| staging/cse/windows/kubeletfunc.ps1 | Add exit-code checks for nssm.exe install/configure and DependOnService operations. |
| staging/cse/windows/containerdfunc.ps1 | Add exit-code checks for sc.exe delete and nssm.exe install/configure for containerd. |
| staging/cse/windows/configfunc.ps1 | Add exit-code checks for sc.exe failure, reg.exe import, icacls, and nssm.exe service creation/configuration (csi-proxy, hosts-config-agent). |
| parts/windows/kuberneteswindowssetup.ps1 | Add exit-code checks for icacls.exe ACL updates and netsh advfirewall disable. |
| pkg/agent/testdata/AKSWindows23H2Gen2+NextGenNetworkingNoConfig/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows23H2Gen2+NextGenNetworkingDisabled/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows23H2Gen2+NextGenNetworking/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+ootcredentialprovider/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+SecurityProfile/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+ManagedIdentity/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+KubeletServingCertificateRotation/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+KubeletClientTLSBootstrapping/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+K8S119/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+K8S119+FIPS/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+K8S119+CSI/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+K8S118/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+K8S117/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+K8S116/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+EnablePrivateClusterHostsConfigAgent/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+CustomVnet/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+CustomCloud/CustomData | Snapshot update to include new exit-code checks. |
| pkg/agent/testdata/AKSWindows2019+CustomCloud+ootcredentialprovider/CustomData | Snapshot update to include new exit-code checks. |
timmy-wright
force-pushed
the
timmy/windows-cse-scripts-error-handling
branch
from
db957f9 to
c9b61f0
Compare
Contributor
Test Results 3 files 10 suites 56s ⏱️ For more details on these failures, see this check. Results for commit d1e233a. ♻️ This comment has been updated with latest results. |
Contributor
Author
🎬 Squad Review — PR #8133Reviewers: Hockney (Windows), Verbal (Reliability), Kujan (Testing), Keaton (Maintainability) Overall Verdict: Approve with required changes ✅
|
| Issue | Reviewer(s) | Details |
|---|---|---|
$ErrorActionPreference = "Stop" outside try block |
Verbal, Kujan | ~200 lines execute with Stop mode but no catch handler (between line ~38 and the new try at ~250). If dot-sourcing windowscsehelper.ps1 hits a non-terminating error, it becomes terminating with no catch — CSE result file never gets written, making the failure appear as a timeout. Consider moving it inside the try block. |
pwsh vs powershell in CI |
Kujan | Tests now run on PowerShell Core 7.x (pwsh) but production uses Windows PowerShell 5.1. The Out-File-Ascii wrapper acknowledges this gap, but other 5.1-specific behaviors may slip through. At minimum add a comment; ideally add one 5.1 compatibility test step. |
No unit tests for Invoke-Nssm |
Kujan | This is the core abstraction of the PR but has zero dedicated tests. Add at least: success case, non-zero exit code throws, error message includes args and exit code. |
Out-File-Ascii naming |
Keaton | Hyphen in noun violates PowerShell naming conventions. Consider Out-FileAscii or Write-AsciiFile. |
Get-Root-RegistryPath naming |
Keaton | "RegistryPath" in a Windows context implies the Windows Registry (HKLM:\...). This returns a filesystem path to containerd's certificate store. Consider Get-ContainerdCertsRoot. |
| v2 test context implicit directory dependency | Kujan | The v2 Pester context relies on a directory created by v1's BeforeEach. If test ordering changes, v2 breaks. Add Create-Directory in the v2 BeforeAll. |
🟢 Praised
| Aspect | Reviewer(s) | Notes |
|---|---|---|
| Error message quality | Verbal, Hockney | Messages consistently include WHAT command, WHAT target, WHAT exit code — actionable for on-call |
Upload-GuestVMLogs guard |
All 4 | Get-Command check is strictly better than the old error-code-specific check — handles any scenario where the function isn't available |
| Try block expansion | Keaton, Verbal | Wider scope + capability-based guard is more robust than the old narrow scope |
| Diagnostic-only exclusions | Verbal | Correctly left unchecked: log-only icacls, CollectGuestLogs.exe (best-effort in finally block) |
| CI pipeline improvements | Kujan, Hockney | PesterConfiguration with JUnit output, Cobertura coverage, if: (!cancelled()) step ordering — solid infrastructure |
| Bidirectional VHD/CSE compatibility | Hockney | No compatibility risk. All changes are self-contained within CSE scripts — Invoke-Nssm is defined in the same files that call it, RemoveNulls is in windowscsehelper.ps1 (always loaded first) |
Invoke-Nssm wrapper design |
Hockney | ValueFromRemainingArguments + splatting is correct and clean |
| Comment quality | Keaton | Every new comment explains WHY, not WHAT — above average for the codebase |
📊 Reviewer Verdicts
| Reviewer | Domain | Verdict |
|---|---|---|
| 🏗️ Hockney | Windows AKS | Approve with suggestions — fix $? bug, consolidate Invoke-Nssm |
| 🔒 Verbal | Reliability | Approve with minor issues — fix $?, consider ErrorActionPreference placement |
| 🧪 Kujan | Testing | Request changes — fix $? bug, add Invoke-Nssm tests, assess pwsh gap |
| 🏗️ Keaton | Maintainability | Request changes — consolidate Invoke-Nssm, fix $?, naming issues |
Review performed by Squad v0.9.1 — Hockney, Verbal, Kujan, Keaton
timmy-wright
commented
Apr 13, 2026
Contributor
Author
timmy-wright
left a comment
timmy-wright
left a comment
There was a problem hiding this comment.
Squad Inline Review — see individual comments on the diff for details.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 11 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
staging/cse/windows/containerdfunc.tests.ps1:143
- In the
containerd template v2context, the test writesconfig.tomlinto$PSScriptRoot\containerdfunc.tests.suites, but this directory is no longer checked in (and is now gitignored). Unlike the v1 context, this v2 setup doesn’t create the directory, so these tests can fail when run in isolation. Please create the target directory in this context’s setup as well (or move directory creation to the outerBeforeEach).
Context 'containerd template v2' {
BeforeAll {
$containerdDir = "$PSScriptRoot\containerdfunc.tests.suites"
$cniBinDir = 'C:/cni/bin'
$cniConfDir = 'C:/cni/conf'
$pauseImage = 'mcr.microsoft.com/oss/v2/kubernetes/pause:3.6'
$global:KubeClusterConfigPath = [Io.path]::Combine("", "kubeclusterconfig.json")
$global:ContainerdInstallLocation = $containerdDir
$global:WindowsDataDir = $PSScriptRoot
$configPath = Join-Path $global:ContainerdInstallLocation "config.toml"
}
Comment on lines
+3
to
+14
| function Invoke-Nssm | ||
| { | ||
| param( | ||
| [Parameter(Mandatory = $true)][string]$KubeDir, | ||
| [Parameter(Mandatory = $true, ValueFromRemainingArguments = $true)][string[]]$NssmArguments | ||
| ) | ||
| & "$KubeDir\nssm.exe" @NssmArguments | RemoveNulls | ||
| if ($LASTEXITCODE -ne 0) | ||
| { | ||
| throw "nssm.exe $( $NssmArguments -join ' ' ) failed (exit code $LASTEXITCODE)" | ||
| } | ||
| } |
There was a problem hiding this comment.
helpers.ps1 is added under staging/, but the Windows CSE packaging script copies only staging/cse/windows/* into the published CSE zip. As a result, this file is unlikely to land at C:\AzureData\windows\helpers.ps1 at runtime, even though multiple scripts now dot-source it. Please either move this helper into staging/cse/windows/ (so it’s included in the CSE zip) or update the packaging/extraction so it is reliably present at C:\AzureData\windows\helpers.ps1 before any dependent scripts are dot-sourced.
| @@ -1,4 +1,5 @@ | |||
|
|
|||
| . c:\AzureData\windows\helpers.ps1 | |||
There was a problem hiding this comment.
This script now unconditionally dot-sources c:\AzureData\windows\helpers.ps1. If that file is missing (e.g., older cached CSE zip, or packaging didn’t include it), dot-sourcing will terminate the whole CSE flow before any error handling can run. Please either ensure helpers.ps1 is always present in the CSE package, or guard this import with Test-Path and throw a clear error (or fall back to local implementations) when it’s absent.
Suggested change
| . c:\AzureData\windows\helpers.ps1 | |
| $helpersScriptPath = 'c:\AzureData\windows\helpers.ps1' | |
| if (-not (Test-Path -LiteralPath $helpersScriptPath)) { | |
| throw "Required Windows CSE helper script not found at '$helpersScriptPath'. configfunc.ps1 depends on this file. This likely indicates an incomplete or incompatible CSE package." | |
| } | |
| . $helpersScriptPath |
| @@ -1,9 +1,31 @@ | |||
| . c:\AzureData\windows\helpers.ps1 | |||
There was a problem hiding this comment.
This script now unconditionally dot-sources c:\AzureData\windows\helpers.ps1, but the PR doesn’t show helpers.ps1 being included in the published Windows CSE zip. If it isn’t present at runtime, this import will fail immediately and prevent containerd setup. Please ensure helpers.ps1 is packaged to C:\AzureData\windows\helpers.ps1 (preferred), or make this import conditional with a clear failure message.
Suggested change
| . c:\AzureData\windows\helpers.ps1 | |
| $helpersScriptPath = 'C:\AzureData\windows\helpers.ps1' | |
| if (-not (Test-Path -LiteralPath $helpersScriptPath)) { | |
| throw "Required helper script not found at '$helpersScriptPath'. Ensure helpers.ps1 is packaged to C:\AzureData\windows\helpers.ps1 before running containerd setup." | |
| } | |
| . $helpersScriptPath |
| @@ -1,3 +1,5 @@ | |||
| . c:\AzureData\windows\helpers.ps1 | |||
There was a problem hiding this comment.
This script now unconditionally dot-sources c:\AzureData\windows\helpers.ps1. If that file isn’t present on the node (e.g., older cached CSE package, or packaging doesn’t include it), CSE will fail before kubelet/kubeproxy service registration. Please ensure helpers.ps1 is always packaged to this location, or guard the dot-source with Test-Path and provide a deterministic error/fallback path.
Suggested change
| . c:\AzureData\windows\helpers.ps1 | |
| $helpersScriptPath = 'c:\AzureData\windows\helpers.ps1' | |
| if (Test-Path -Path $helpersScriptPath -PathType Leaf) { | |
| . $helpersScriptPath | |
| } else { | |
| throw "Required dependency '$helpersScriptPath' was not found. kubeletfunc.ps1 cannot continue without helpers.ps1. Ensure the CSE package includes this file at the expected path." | |
| } |
Comment on lines
+514
to
+603
| Describe 'RegisterContainerDService' { | ||
| BeforeEach { | ||
| Mock Assert-FileExists | ||
| Mock Invoke-Nssm | ||
| Mock Start-Service | ||
|
|
||
| $script:capturedFilePath = $null | ||
| $script:capturedContent = $null | ||
| Mock Out-FileAscii -MockWith { | ||
| param($Content,$FilePath) | ||
| $script:capturedFilePath = $FilePath | ||
| $script:capturedContent = $Content | ||
| } | ||
| } | ||
|
|
||
| Context 'when containerd service does not exist' { | ||
| BeforeEach { | ||
| $script:GetServiceCallCount = 0 | ||
| $mockRunningSvc = [PSCustomObject]@{Name = 'containerd'; Status = 'Running'} | ||
| Mock Get-Service -MockWith { | ||
| $script:GetServiceCallCount++ | ||
| if ($script:GetServiceCallCount -eq 1) { return $null } | ||
| return $mockRunningSvc | ||
| } | ||
| Mock sc.exe | ||
| } | ||
|
|
||
| It 'does not call sc.exe when service does not exist' { | ||
| RegisterContainerDService -kubedir 'C:\k' | ||
|
|
||
| Assert-MockCalled sc.exe -Exactly -Times 0 | ||
| } | ||
| } | ||
|
|
||
| Context 'when containerd service already exists' { | ||
| BeforeEach { | ||
| $script:GetServiceCallCount = 0 | ||
| $mockExistingSvc = [PSCustomObject]@{Name = 'containerd'; Status = 'Stopped'} | ||
| $mockRunningSvc = [PSCustomObject]@{Name = 'containerd'; Status = 'Running'} | ||
| Mock Get-Service -MockWith { | ||
| $script:GetServiceCallCount++ | ||
| if ($script:GetServiceCallCount -eq 1) { return $mockExistingSvc } | ||
| return $mockRunningSvc | ||
| } | ||
| } | ||
|
|
||
| It 'calls sc.exe delete to remove the existing service' { | ||
| Mock sc.exe -MockWith { $global:LASTEXITCODE = 0 } | ||
|
|
||
| RegisterContainerDService -kubedir 'C:\k' | ||
|
|
||
| Assert-MockCalled sc.exe -Exactly -Times 1 | ||
| } | ||
|
|
||
| It 'does not throw when sc.exe delete succeeds' { | ||
| Mock sc.exe -MockWith { $global:LASTEXITCODE = 0 } | ||
|
|
||
| { RegisterContainerDService -kubedir 'C:\k' } | Should -Not -Throw | ||
| } | ||
|
|
||
| It 'throws when sc.exe delete fails' { | ||
| Mock sc.exe -MockWith { $global:LASTEXITCODE = 1 } | ||
|
|
||
| { RegisterContainerDService -kubedir 'C:\k' } | Should -Throw '*sc.exe failed to delete existing containerd service*' | ||
| } | ||
|
|
||
| It 'includes the exit code in the error message when sc.exe fails' { | ||
| Mock sc.exe -MockWith { $global:LASTEXITCODE = 5 } | ||
|
|
||
| { RegisterContainerDService -kubedir 'C:\k' } | Should -Throw '*exit code 5*' | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Describe 'RegisterContainerDService' { | ||
| BeforeEach { | ||
| Mock Assert-FileExists | ||
| Mock Invoke-Nssm | ||
| Mock Start-Service | ||
|
|
||
| $script:capturedFilePath = $null | ||
| $script:capturedContent = $null | ||
| Mock Out-FileAscii -MockWith { | ||
| param($Content,$FilePath) | ||
| $script:capturedFilePath = $FilePath | ||
| $script:capturedContent = $Content | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Describe 'RegisterContainerDService' is duplicated back-to-back, containing the same mocks and assertions twice. This will run the same tests twice, increasing runtime and making maintenance harder. Please remove one of the duplicated Describe blocks (keep a single set of tests).
Comment on lines
+15
to
+16
| issues: write | ||
| pull-requests: write |
There was a problem hiding this comment.
The workflow grants issues: write and pull-requests: write, but the job appears to only need to publish check/test results. On pull_request workflows, avoid unnecessary write scopes to reduce the impact of a compromised dependency/action. Please scope permissions down to the minimum required (likely contents: read + checks: write, and only add pull-requests: write if a step actually comments/updates PRs).
Suggested change
| issues: write | |
| pull-requests: write |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 12 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
parts/windows/kuberneteswindowssetup.ps1:75
- The Go template markers have been altered from
{{ ... }}to{ { ... } }in multiple places (e.g., SSHKeys).{ {is not valid Go template syntax, so these placeholders will not be rendered and the resulting PowerShell will be invalid at runtime. Please restore the exact{{ ... }}template delimiters.
## SSH public keys to add to authorized_keys
$global:SSHKeys = @( {{ GetSshPublicKeysPowerShell }} )
Comment on lines
+43
to
+47
| $MasterIP = "{{ GetKubernetesEndpoint }}" | ||
| $KubeDnsServiceIp = "{{ GetParameter "kubeDNSServiceIP" }}" | ||
| $MasterFQDNPrefix = "{{ GetParameter "masterEndpointDNSNamePrefix" }}" | ||
| $Location = "{{ GetVariable "location" }}" | ||
| {{ if UserAssignedIDEnabled }} |
Comment on lines
95
to
99
| $global:WindowsTelemetryGUID = "{{GetParameter "windowsTelemetryGUID"}}" | ||
| {{if eq GetIdentitySystem "adfs"}} | ||
| {{ if eq GetIdentitySystem "adfs" }} | ||
| $global:TenantId = "adfs" | ||
| {{else}} | ||
| {{ else }} | ||
| $global:TenantId = "{{GetVariable "tenantID"}}" |
| Write-Log "Install-SecureTLSBootstrapClient is not a recognized function, will skip installation of the secure TLS bootstrap client" | ||
| } | ||
|
|
||
| Install-CredentialProvider -KubeDir $global:KubeDir -CustomCloudContainerRegistryDNSSuffix {{if IsAKSCustomCloud}}"{{ AKSCustomCloudContainerRegistryDNSSuffix }}"{{else}}""{{end}} | ||
| Install-CredentialProvider -KubeDir $global:KubeDir -CustomCloudContainerRegistryDNSSuffix {{ if IsAKSCustomCloud }}"{{ AKSCustomCloudContainerRegistryDNSSuffix }}" {{ else }}"" {{ end }} |
Comment on lines
+326
to
+330
| if (Test-Path -Path 'c:\AzureData\windows\helpers.ps1') { | ||
| . c:\AzureData\windows\helpers.ps1 | ||
| } | ||
| else { | ||
| Write-Log "Helpers function script not found, skipping dot-source" |
Comment on lines
+1
to
+5
| # common helper functions | ||
|
|
||
| function Invoke-Nssm | ||
| { | ||
| param( |
Comment on lines
+69
to
73
| BeforeEach { | ||
| $containerdDir = "$PSScriptRoot\containerdfunc.tests.suites" | ||
| $registryPath = "$containerdDir" | ||
| $cniBinDir = 'C:/cni/bin' | ||
| $cniConfDir = 'C:/cni/conf' |
Comment on lines
+589
to
+593
| Describe 'RegisterContainerDService' { | ||
| BeforeEach { | ||
| Mock Assert-FileExists | ||
| Mock Invoke-Nssm | ||
| Mock Start-Service |
Comment on lines
+156
to
+159
| $global:IsDualStackEnabled = {{ if IsIPv6DualStackFeatureEnabled }}$true {{ else }}$false {{ end }} | ||
| $global:IsAzureCNIOverlayEnabled = {{ if IsAzureCNIOverlayFeatureEnabled }}$true {{ else }}$false {{ end }} | ||
| $global:CiliumDataplaneEnabled = {{ if CiliumDataplaneEnabled }}$true {{ else }}$false {{ end }} | ||
| $global:IsIMDSRestrictionEnabled = {{ if EnableIMDSRestriction }}$true {{ else }}$false {{ end }} |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 18 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
parts/windows/kuberneteswindowssetup.ps1.template:49
- The template markers in this block were changed from Go-template syntax (
{{ ... }}) to{ { ... } }, which will not be recognized by the template generator and will end up as literal text in the rendered script (breaking CustomData execution). Please revert to valid template delimiters here.
parts/windows/kuberneteswindowssetup.ps1.template:63 - These variables ($KubeDnsServiceIp/$MasterFQDNPrefix/$Location/$UserAssignedClientID/$TargetEnvironment/$ArmResourceEndpoint/$AADClientId/$NetworkAPIVersion) are assigned twice in a row, which is likely an accidental duplication and makes the rendered script harder to reason about. Please remove the duplicate block and keep a single set of assignments.
parts/windows/kuberneteswindowssetup.ps1.template:331 Invoke-Nssmis required by several CSE scripts (e.g., kubeletfunc/containerdfunc/configfunc), buthelpers.ps1is only dot-sourced if it exists underC:\AzureData\windows\helpers.ps1. As added in this PR, the helper is located atstaging/helpers.ps1and won’t be included in the Windows CSE zip (the packaging script copies onlystaging/cse/windows/*), so this will skip dot-sourcing and later calls toInvoke-Nssmwill fail. Please ensure the helpers file is packaged toC:\AzureData\windows\helpers.ps1(or moveInvoke-Nssminto an always-loaded script like windowscsehelper.ps1).
| VnetCNIWindowsPluginsDownloadURL: "https://packages.aks.azure.com/azure-cni/v1.6.21/binaries/azure-vnet-cni-windows-amd64-v1.6.21.zip", | ||
| WindowsPauseImageURL: "mcr.microsoft.com/oss/v2/kubernetes/pause:3.10.1", | ||
| WindowsProvisioningScriptsPackageURL: "https://packages.aks.azure.com/aks/windows/cse/aks-windows-cse-scripts-v0.0.52.zip", | ||
| WindowsProvisioningScriptsPackageURL: "", |
Comment on lines
+514
to
+593
| Describe 'RegisterContainerDService' { | ||
| BeforeEach { | ||
| Mock Assert-FileExists | ||
| Mock Invoke-Nssm | ||
| Mock Start-Service | ||
|
|
||
| $script:capturedFilePath = $null | ||
| $script:capturedContent = $null | ||
| Mock Out-FileAscii -MockWith { | ||
| param($Content,$FilePath) | ||
| $script:capturedFilePath = $FilePath | ||
| $script:capturedContent = $Content | ||
| } | ||
| } | ||
|
|
||
| Context 'when containerd service does not exist' { | ||
| BeforeEach { | ||
| $script:GetServiceCallCount = 0 | ||
| $mockRunningSvc = [PSCustomObject]@{Name = 'containerd'; Status = 'Running'} | ||
| Mock Get-Service -MockWith { | ||
| $script:GetServiceCallCount++ | ||
| if ($script:GetServiceCallCount -eq 1) { return $null } | ||
| return $mockRunningSvc | ||
| } | ||
| Mock sc.exe | ||
| } | ||
|
|
||
| It 'does not call sc.exe when service does not exist' { | ||
| RegisterContainerDService -kubedir 'C:\k' | ||
|
|
||
| Assert-MockCalled sc.exe -Exactly -Times 0 | ||
| } | ||
| } | ||
|
|
||
| Context 'when containerd service already exists' { | ||
| BeforeEach { | ||
| $script:GetServiceCallCount = 0 | ||
| $mockExistingSvc = [PSCustomObject]@{Name = 'containerd'; Status = 'Stopped'} | ||
| $mockRunningSvc = [PSCustomObject]@{Name = 'containerd'; Status = 'Running'} | ||
| Mock Get-Service -MockWith { | ||
| $script:GetServiceCallCount++ | ||
| if ($script:GetServiceCallCount -eq 1) { return $mockExistingSvc } | ||
| return $mockRunningSvc | ||
| } | ||
| } | ||
|
|
||
| It 'calls sc.exe delete to remove the existing service' { | ||
| Mock sc.exe -MockWith { $global:LASTEXITCODE = 0 } | ||
|
|
||
| RegisterContainerDService -kubedir 'C:\k' | ||
|
|
||
| Assert-MockCalled sc.exe -Exactly -Times 1 | ||
| } | ||
|
|
||
| It 'does not throw when sc.exe delete succeeds' { | ||
| Mock sc.exe -MockWith { $global:LASTEXITCODE = 0 } | ||
|
|
||
| { RegisterContainerDService -kubedir 'C:\k' } | Should -Not -Throw | ||
| } | ||
|
|
||
| It 'throws when sc.exe delete fails' { | ||
| Mock sc.exe -MockWith { $global:LASTEXITCODE = 1 } | ||
|
|
||
| { RegisterContainerDService -kubedir 'C:\k' } | Should -Throw '*sc.exe failed to delete existing containerd service*' | ||
| } | ||
|
|
||
| It 'includes the exit code in the error message when sc.exe fails' { | ||
| Mock sc.exe -MockWith { $global:LASTEXITCODE = 5 } | ||
|
|
||
| { RegisterContainerDService -kubedir 'C:\k' } | Should -Throw '*exit code 5*' | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Describe 'RegisterContainerDService' { | ||
| BeforeEach { | ||
| Mock Assert-FileExists | ||
| Mock Invoke-Nssm | ||
| Mock Start-Service |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Native executables in PowerShell set
$LASTEXITCODEon failure but do not throw exceptions, sotry/catchblocks silently swallow failures from commands likeicacls.exe,sc.exe,nssm.exe,netsh, andreg.exe.This PR adds
if ($LASTEXITCODE -ne 0) { throw "..." }checks after each unchecked external command call so that failures propagate as exceptions through the call stack.Changes
parts/windows/kuberneteswindowssetup.ps1icacls.exe(x4),netsh advfirewallstaging/cse/windows/configfunc.ps1sc.exe failure(x3),reg.exe importin try/catch that did not catch it,icacls(x4),nssm.exe install+configurefor csi-proxy and hosts-config-agentstaging/cse/windows/kubeletfunc.ps1nssm.exe install+configurefor Kubelet and Kubeproxy,Invoke-Expressionnssm DependOnServicestaging/cse/windows/containerdfunc.ps1sc.exe delete,nssm.exe install+configurefor containerdstaging/cse/windows/windowsciliumnetworkingfunc.ps1Notes
reg.exe importcall inconfigfunc.ps1was already inside atry/catchbut the catch was unreachable for native command failures. The$LASTEXITCODEcheck now makes it catchable.nssm.exeblocks a check is added afterinstalland after the finalsetcall.Relates to IcM 613775405