From 4602416a69867c24c9788dd9afd99b27982ecacd Mon Sep 17 00:00:00 2001 From: Johan Ljunggren Date: Sun, 2 Apr 2023 18:53:47 +0200 Subject: [PATCH] `Restart-SqlService`: No longer silently ignores errors (#1895) - SqlServerDsc.Common - `Restart-SqlService` no longer silently ignores errors that prevents the instance to go online. If the instance has not gone online during the timeout period the error thrown will no contain the last error reported by `Connect-SQL` (issue #1891). --- CHANGELOG.md | 5 + .../SqlServerDsc.Common.psm1 | 22 +- tests/Unit/SqlServerDsc.Common.Tests.ps1 | 190 +++++++++++++----- 3 files changed, 163 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c4abc4802..4578c75cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Now imports the preferred module into the global scope so that MOF-based resources (that is in another module scope) can use the imported module. - Some code cleanup ([issue #1881](https://github.com/dsccommunity/SqlServerDsc/issues/1881)). +- SqlServerDsc.Common + - `Restart-SqlService` no longer silently ignores errors that prevents + the instance to go online. If the instance has not gone online during + the timeout period the error thrown will no contain the last error + reported by `Connect-SQL` ([issue #1891](https://github.com/dsccommunity/SqlServerDsc/issues/1891)). ### Fixed diff --git a/source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 b/source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 index d4cf3ddd4..33f42dbe5 100644 --- a/source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 +++ b/source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 @@ -1014,10 +1014,12 @@ function Restart-SqlService { $connectTimer = [System.Diagnostics.StopWatch]::StartNew() + $connectSqlError = $null + do { # This call, if it fails, will take between ~9-10 seconds to return. - $testConnectionServerObject = Connect-SQL -ServerName $ServerName -InstanceName $InstanceName -ErrorAction 'SilentlyContinue' + $testConnectionServerObject = Connect-SQL -ServerName $ServerName -InstanceName $InstanceName -ErrorAction 'SilentlyContinue' -ErrorVariable 'connectSqlError' # Make sure we have an SMO object to test Status if ($testConnectionServerObject) @@ -1037,8 +1039,22 @@ function Restart-SqlService # Was the timeout period reach before able to connect to the SQL Server instance? if (-not $testConnectionServerObject -or $testConnectionServerObject.Status -ne 'Online') { - $errorMessage = $script:localizedData.FailedToConnectToInstanceTimeout -f $ServerName, $InstanceName, $Timeout - New-InvalidOperationException -Message $errorMessage + $errorMessage = $script:localizedData.FailedToConnectToInstanceTimeout -f @( + $ServerName, + $InstanceName, + $Timeout + ) + + $newInvalidOperationExceptionParameters = @{ + Message = $errorMessage + } + + if ($connectSqlError) + { + $newInvalidOperationExceptionParameters.ErrorRecord = $connectSqlError[$connectSqlError.Count - 1] + } + + New-InvalidOperationException @newInvalidOperationExceptionParameters } } } diff --git a/tests/Unit/SqlServerDsc.Common.Tests.ps1 b/tests/Unit/SqlServerDsc.Common.Tests.ps1 index 215e7ac59..7eb4d85c7 100644 --- a/tests/Unit/SqlServerDsc.Common.Tests.ps1 +++ b/tests/Unit/SqlServerDsc.Common.Tests.ps1 @@ -801,6 +801,35 @@ Describe 'SqlServerDsc.Common\Start-SqlSetupProcess' -Tag 'StartSqlSetupProcess' } Describe 'SqlServerDsc.Common\Restart-SqlService' -Tag 'RestartSqlService' { + BeforeAll { + InModuleScope -ScriptBlock { + # Stubs for cross-platform testing. + function script:Get-Service + { + throw '{0}: StubNotImplemented' -f $MyInvocation.MyCommand + } + + function script:Restart-Service + { + throw '{0}: StubNotImplemented' -f $MyInvocation.MyCommand + } + + function script:Start-Service + { + throw '{0}: StubNotImplemented' -f $MyInvocation.MyCommand + } + } + } + + AfterAll { + InModuleScope -ScriptBlock { + # Remove stubs that was used for cross-platform testing. + Remove-Item -Path function:Get-Service + Remove-Item -Path function:Restart-Service + Remove-Item -Path function:Start-Service + } + } + Context 'Restart-SqlService standalone instance' { Context 'When the Windows services should be restarted' { BeforeAll { @@ -834,7 +863,7 @@ Describe 'SqlServerDsc.Common\Restart-SqlService' -Tag 'RestartSqlService' { } It 'Should restart SQL Service and running SQL Agent service' { - { Restart-SqlService -ServerName $env:COMPUTERNAME -InstanceName 'MSSQLSERVER' } | Should -Not -Throw + { Restart-SqlService -ServerName (Get-ComputerName) -InstanceName 'MSSQLSERVER' } | Should -Not -Throw Should -Invoke -CommandName Connect-SQL -ParameterFilter { <# @@ -854,7 +883,7 @@ Describe 'SqlServerDsc.Common\Restart-SqlService' -Tag 'RestartSqlService' { Context 'When skipping the cluster check' { It 'Should restart SQL Service and running SQL Agent service' { - { Restart-SqlService -ServerName $env:COMPUTERNAME -InstanceName 'MSSQLSERVER' -SkipClusterCheck } | Should -Not -Throw + { Restart-SqlService -ServerName (Get-ComputerName) -InstanceName 'MSSQLSERVER' -SkipClusterCheck } | Should -Not -Throw Should -Invoke -CommandName Connect-SQL -ParameterFilter { <# @@ -875,7 +904,7 @@ Describe 'SqlServerDsc.Common\Restart-SqlService' -Tag 'RestartSqlService' { Context 'When skipping the online check' { It 'Should restart SQL Service and running SQL Agent service and not wait for the SQL Server instance to come back online' { - { Restart-SqlService -ServerName $env:COMPUTERNAME -InstanceName 'MSSQLSERVER' -SkipWaitForOnline } | Should -Not -Throw + { Restart-SqlService -ServerName (Get-ComputerName) -InstanceName 'MSSQLSERVER' -SkipWaitForOnline } | Should -Not -Throw Should -Invoke -CommandName Connect-SQL -ParameterFilter { <# @@ -923,7 +952,7 @@ Describe 'SqlServerDsc.Common\Restart-SqlService' -Tag 'RestartSqlService' { } It 'Should just call Restart-SqlClusterService to restart the SQL Server cluster instance' { - { Restart-SqlService -ServerName $env:COMPUTERNAME -InstanceName 'MSSQLSERVER' } | Should -Not -Throw + { Restart-SqlService -ServerName (Get-ComputerName) -InstanceName 'MSSQLSERVER' } | Should -Not -Throw Should -Invoke -CommandName Connect-SQL -ParameterFilter { <# @@ -943,7 +972,7 @@ Describe 'SqlServerDsc.Common\Restart-SqlService' -Tag 'RestartSqlService' { Context 'When passing the Timeout value' { It 'Should just call Restart-SqlClusterService with the correct parameter' { - { Restart-SqlService -ServerName $env:COMPUTERNAME -InstanceName 'MSSQLSERVER' -Timeout 120 } | Should -Not -Throw + { Restart-SqlService -ServerName (Get-ComputerName) -InstanceName 'MSSQLSERVER' -Timeout 120 } | Should -Not -Throw Should -Invoke -CommandName Restart-SqlClusterService -ParameterFilter { <# @@ -957,7 +986,7 @@ Describe 'SqlServerDsc.Common\Restart-SqlService' -Tag 'RestartSqlService' { Context 'When passing the OwnerNode value' { It 'Should just call Restart-SqlClusterService with the correct parameter' { - { Restart-SqlService -ServerName $env:COMPUTERNAME -InstanceName 'MSSQLSERVER' -OwnerNode @('TestNode') } | Should -Not -Throw + { Restart-SqlService -ServerName (Get-ComputerName) -InstanceName 'MSSQLSERVER' -OwnerNode @('TestNode') } | Should -Not -Throw Should -Invoke -CommandName Restart-SqlClusterService -ParameterFilter { <# @@ -995,7 +1024,7 @@ Describe 'SqlServerDsc.Common\Restart-SqlService' -Tag 'RestartSqlService' { } It 'Should restart SQL Service and not try to restart missing SQL Agent service' { - { Restart-SqlService -ServerName $env:COMPUTERNAME -InstanceName 'NOAGENT' -SkipClusterCheck } | Should -Not -Throw + { Restart-SqlService -ServerName (Get-ComputerName) -InstanceName 'NOAGENT' -SkipClusterCheck } | Should -Not -Throw Should -Invoke -CommandName Get-Service -Scope It -Exactly -Times 1 Should -Invoke -CommandName Restart-Service -Scope It -Exactly -Times 1 @@ -1035,7 +1064,7 @@ Describe 'SqlServerDsc.Common\Restart-SqlService' -Tag 'RestartSqlService' { } It 'Should restart SQL Service and not try to restart stopped SQL Agent service' { - { Restart-SqlService -ServerName $env:COMPUTERNAME -InstanceName 'STOPPEDAGENT' -SkipClusterCheck } | Should -Not -Throw + { Restart-SqlService -ServerName (Get-ComputerName) -InstanceName 'STOPPEDAGENT' -SkipClusterCheck } | Should -Not -Throw Should -Invoke -CommandName Get-Service -Scope It -Exactly -Times 1 Should -Invoke -CommandName Restart-Service -Scope It -Exactly -Times 1 @@ -1044,63 +1073,122 @@ Describe 'SqlServerDsc.Common\Restart-SqlService' -Tag 'RestartSqlService' { } Context 'When it fails to connect to the instance within the timeout period' { - BeforeAll { - Mock -CommandName Connect-SQL -MockWith { - return @{ - Name = 'MSSQLSERVER' - InstanceName = '' - ServiceName = 'MSSQLSERVER' - Status = 'Offline' + Context 'When the connection throws an exception' { + BeforeAll { + Mock -CommandName Connect-SQL -MockWith { + # Using SilentlyContinue to not show the errors in the Pester output. + Write-Error -Message 'Mock connection error' -ErrorAction 'SilentlyContinue' } - } - Mock -CommandName Get-Service -MockWith { - return @{ - Name = 'MSSQLSERVER' - DisplayName = 'Microsoft SQL Server (MSSQLSERVER)' - DependentServices = @( - @{ - Name = 'SQLSERVERAGENT' - DisplayName = 'SQL Server Agent (MSSQLSERVER)' - Status = 'Running' - DependentServices = @() - } - ) + Mock -CommandName Get-Service -MockWith { + return @{ + Name = 'MSSQLSERVER' + DisplayName = 'Microsoft SQL Server (MSSQLSERVER)' + DependentServices = @( + @{ + Name = 'SQLSERVERAGENT' + DisplayName = 'SQL Server Agent (MSSQLSERVER)' + Status = 'Running' + DependentServices = @() + } + ) + } } + + Mock -CommandName Restart-Service + Mock -CommandName Start-Service } - Mock -CommandName Restart-Service - Mock -CommandName Start-Service + It 'Should wait for timeout before throwing error message' { + $mockLocalizedString = InModuleScope -ScriptBlock { + $localizedData.FailedToConnectToInstanceTimeout + } + + $mockErrorMessage = Get-InvalidOperationRecord -Message ( + ($mockLocalizedString -f (Get-ComputerName), 'MSSQLSERVER', 4) + '*Mock connection error*' + ) + + $mockErrorMessage.Exception.Message | Should -Not -BeNullOrEmpty + + { + Restart-SqlService -ServerName (Get-ComputerName) -InstanceName 'MSSQLSERVER' -Timeout 4 -SkipClusterCheck + } | Should -Throw -ExpectedMessage $mockErrorMessage + + <# + Not using -Exactly to handle when CI is slower, result is + that there are 3 calls to Connect-SQL. + #> + Should -Invoke -CommandName Connect-SQL -ParameterFilter { + <# + Make sure we assert the second call to Connect-SQL + + Due to issue https://github.com/pester/Pester/issues/1542 + we cannot use `$PSBoundParameters.ContainsKey('ErrorAction') -eq $true`. + #> + $ErrorAction -eq 'SilentlyContinue' + } -Scope It -Times 2 + } } - It 'Should wait for timeout before throwing error message' { - $mockLocalizedString = InModuleScope -ScriptBlock { - $localizedData.FailedToConnectToInstanceTimeout + Context 'When the Status returns offline' { + BeforeAll { + Mock -CommandName Connect-SQL -MockWith { + return @{ + Name = 'MSSQLSERVER' + InstanceName = '' + ServiceName = 'MSSQLSERVER' + Status = 'Offline' + } + } + + Mock -CommandName Get-Service -MockWith { + return @{ + Name = 'MSSQLSERVER' + DisplayName = 'Microsoft SQL Server (MSSQLSERVER)' + DependentServices = @( + @{ + Name = 'SQLSERVERAGENT' + DisplayName = 'SQL Server Agent (MSSQLSERVER)' + Status = 'Running' + DependentServices = @() + } + ) + } + } + + Mock -CommandName Restart-Service + Mock -CommandName Start-Service } - $mockErrorMessage = Get-InvalidOperationRecord -Message ( - $mockLocalizedString -f $env:ComputerName, 'MSSQLSERVER', 4 - ) + It 'Should wait for timeout before throwing error message' { + $mockLocalizedString = InModuleScope -ScriptBlock { + $localizedData.FailedToConnectToInstanceTimeout + } - $mockErrorMessage.Exception.Message | Should -Not -BeNullOrEmpty + $mockErrorMessage = Get-InvalidOperationRecord -Message ( + $mockLocalizedString -f (Get-ComputerName), 'MSSQLSERVER', 4 + ) - { - Restart-SqlService -ServerName $env:ComputerName -InstanceName 'MSSQLSERVER' -Timeout 4 -SkipClusterCheck - } | Should -Throw -ExpectedMessage $mockErrorMessage + $mockErrorMessage.Exception.Message | Should -Not -BeNullOrEmpty - <# - Not using -Exactly to handle when CI is slower, result is - that there are 3 calls to Connect-SQL. - #> - Should -Invoke -CommandName Connect-SQL -ParameterFilter { - <# - Make sure we assert the second call to Connect-SQL + { + Restart-SqlService -ServerName (Get-ComputerName) -InstanceName 'MSSQLSERVER' -Timeout 4 -SkipClusterCheck + } | Should -Throw -ExpectedMessage $mockErrorMessage - Due to issue https://github.com/pester/Pester/issues/1542 - we cannot use `$PSBoundParameters.ContainsKey('ErrorAction') -eq $true`. + <# + Not using -Exactly to handle when CI is slower, result is + that there are 3 calls to Connect-SQL. #> - $ErrorAction -eq 'SilentlyContinue' - } -Scope It -Times 2 + Should -Invoke -CommandName Connect-SQL -ParameterFilter { + <# + Make sure we assert the second call to Connect-SQL + + Due to issue https://github.com/pester/Pester/issues/1542 + we cannot use `$PSBoundParameters.ContainsKey('ErrorAction') -eq $true`. + #> + $ErrorAction -eq 'SilentlyContinue' + } -Scope It -Times 2 + } } } }