diff --git a/public/Import-DbaSpConfigure.ps1 b/public/Import-DbaSpConfigure.ps1 index c3dbeec86da..d7fe2d6c286 100644 --- a/public/Import-DbaSpConfigure.ps1 +++ b/public/Import-DbaSpConfigure.ps1 @@ -68,15 +68,11 @@ function Import-DbaSpConfigure { None You cannot pipe objects to Import-DbaSpConfigure .OUTPUTS - System.Boolean + None - Returns $true if the sp_configure settings were successfully applied to the destination instance, or $false if the operation failed. + This command writes no objects to the pipeline. Progress is reported as messages: one per configuration option that was changed, and a final message when the migration is finished. - When using the -ServerCopy parameter set, settings are migrated from the source instance to the destination instance and the command returns a boolean indicating success or failure of the overall migration process. - - When using the -FromFile parameter set, sp_configure settings from a SQL file are executed against the target instance and the command returns a boolean indicating success or failure of the configuration import. - - Note: The function may also display warning messages about configuration options that require SQL Server restart, but these do not affect the boolean return value. + A warning is written when an option could not be set on the destination, and when an option that was changed only takes effect after a restart of SQL Server. .EXAMPLE PS C:\> Import-DbaSpConfigure -Source sqlserver -Destination sqlcluster @@ -119,34 +115,55 @@ function Import-DbaSpConfigure { [switch]$EnableException ) begin { + # Connect-DbaInstance tells us whether it opened a connection for us. We must only close what we opened + # ourselves, because closing a connection of the caller takes their session with it. See #10554. + $isNewSourceConnection = $false + $isNewDestinationConnection = $false + $isNewServerConnection = $false + if (-not $PSBoundParameters.Path -and $PSBoundParameters.Source) { try { - $sourceserver = Connect-DbaInstance -SqlInstance $Source -SqlCredential $SourceSqlCredential + $splatConnectSource = @{ + SqlInstance = $Source + SqlCredential = $SourceSqlCredential + IsNewConnectionReference = [ref]$isNewSourceConnection + } + $sourceserver = Connect-DbaInstance @splatConnectSource } catch { Stop-Function -Message "Failure" -Category ConnectionError -ErrorRecord $_ -Target $Source return } if (-not (Test-SqlSa -SqlInstance $sourceserver -SqlCredential $SourceSqlCredential)) { - Stop-Function -Message "Not a sysadmin on $sourceserver. Quitting." -Category PermissionDenied -Target $server -Continue + Stop-Function -Message "Not a sysadmin on $sourceserver. Quitting." -Category PermissionDenied -Target $sourceserver -Continue } try { - $destserver = Connect-DbaInstance -SqlInstance $Destination -SqlCredential $DestinationSqlCredential + $splatConnectDestination = @{ + SqlInstance = $Destination + SqlCredential = $DestinationSqlCredential + IsNewConnectionReference = [ref]$isNewDestinationConnection + } + $destserver = Connect-DbaInstance @splatConnectDestination } catch { Stop-Function -Message "Failure" -Category ConnectionError -ErrorRecord $_ -Target $Destination return } if (-not (Test-SqlSa -SqlInstance $destserver -SqlCredential $DestinationSqlCredential)) { - Stop-Function -Message "Not a sysadmin on $destserver. Quitting." -Category PermissionDenied -Target $server -Continue + Stop-Function -Message "Not a sysadmin on $destserver. Quitting." -Category PermissionDenied -Target $destserver -Continue } $source = $sourceserver.DomainInstanceName $destination = $destserver.DomainInstanceName } else { try { - $server = Connect-DbaInstance -SqlInstance $SqlInstance -SqlCredential $SqlCredential + $splatConnectServer = @{ + SqlInstance = $SqlInstance + SqlCredential = $SqlCredential + IsNewConnectionReference = [ref]$isNewServerConnection + } + $server = Connect-DbaInstance @splatConnectServer } catch { Stop-Function -Message "Failure" -Category ConnectionError -ErrorRecord $_ -Target $SqlInstance return @@ -176,37 +193,75 @@ function Import-DbaSpConfigure { } If ($Pscmdlet.ShouldProcess($destination, "Execute sp_configure")) { - $sourceserver.Configuration.ShowAdvancedOptions.ConfigValue = $true - $sourceserver.Configuration.Alter($true) - $destserver.Configuration.ShowAdvancedOptions.ConfigValue = $true - $sourceserver.Configuration.Alter($true) + # 'show advanced options' has to be on to read and to set the advanced options. It used to be + # switched on and then off again, which turned it off on instances that had it on. Both instances + # are now put back the way they were, in the finally block below, so that an option that cannot + # be set does not leave them switched on either. + $showAdvancedOptionsNumber = $sourceserver.Configuration.ShowAdvancedOptions.Number + $sourceShowAdvancedOptions = $sourceserver.Configuration.ShowAdvancedOptions.ConfigValue + $destShowAdvancedOptions = $destserver.Configuration.ShowAdvancedOptions.ConfigValue + + if ($sourceShowAdvancedOptions -eq 0) { + $sourceserver.Configuration.ShowAdvancedOptions.ConfigValue = $true + $sourceserver.Configuration.Alter($true) + } + if ($destShowAdvancedOptions -eq 0) { + # This used to alter the source a second time, so the option never reached the destination. + $destserver.Configuration.ShowAdvancedOptions.ConfigValue = $true + $destserver.Configuration.Alter($true) + } + $needsrestart = $false $destprops = $destserver.Configuration.Properties - foreach ($sourceprop in $sourceserver.Configuration.Properties) { - $displayname = $sourceprop.DisplayName + try { + foreach ($sourceprop in $sourceserver.Configuration.Properties) { + $displayname = $sourceprop.DisplayName + + # 'show advanced options' is the means to do the migration, not part of it. + if ($sourceprop.Number -eq $showAdvancedOptionsNumber) { + continue + } + + $destprop = $destprops | Where-Object { $_.Displayname -eq $displayname } + if ($null -eq $destprop) { + continue + } + + # Only options that really differ are touched. Assigning a value marks the property as + # changed even when it is the value that is already set, and Configuration.Alter() then + # sends every option of the instance in one batch, which fails as a whole as soon as one + # of them is not supported by the edition. That made the migration fail on SQL Server + # 2022 and newer even when both instances were already identical. + if ($destprop.ConfigValue -eq $sourceprop.ConfigValue) { + continue + } - $destprop = $destprops | Where-Object { $_.Displayname -eq $displayname } - if ($null -ne $destprop) { try { $destprop.configvalue = $sourceprop.configvalue - $null = $destserver.Query("RECONFIGURE WITH OVERRIDE") + $destserver.Configuration.Alter($true) + if (-not $destprop.IsDynamic) { + $needsrestart = $true + } Write-Message -Level Output -Message "updated $($destprop.displayname) to $($sourceprop.configvalue)." } catch { - Stop-Function -Message "Could not set $($destprop.displayname) to $($sourceprop.configvalue). Feature may not be supported." -ErrorRecord $_ -Continue + # An option that could not be set stays pending and would fail every following + # Alter() together with it, so the pending change is discarded before going on. + $destserver.Configuration.Refresh() + $destprops = $destserver.Configuration.Properties + Stop-Function -Message "Could not set $displayname to $($sourceprop.configvalue). Feature may not be supported." -ErrorRecord $_ -Continue } } + } finally { + if ($destserver.Configuration.ShowAdvancedOptions.ConfigValue -ne $destShowAdvancedOptions) { + $destserver.Configuration.ShowAdvancedOptions.ConfigValue = $destShowAdvancedOptions + $destserver.Configuration.Alter($true) + } + if ($sourceserver.Configuration.ShowAdvancedOptions.ConfigValue -ne $sourceShowAdvancedOptions) { + $sourceserver.Configuration.ShowAdvancedOptions.ConfigValue = $sourceShowAdvancedOptions + $sourceserver.Configuration.Alter($true) + } } - try { - $destserver.Configuration.Alter() - } catch { - $needsrestart = $true - } - - $sourceserver.Configuration.ShowAdvancedOptions.ConfigValue = $false - $sourceserver.Configuration.Alter($true) - $destserver.Configuration.ShowAdvancedOptions.ConfigValue = $false - $destserver.Configuration.Alter($true) if ($needsrestart -eq $true) { Write-Message -Level Warning -Message "Some configuration options will be updated once SQL Server is restarted." @@ -221,7 +276,10 @@ function Import-DbaSpConfigure { } else { if ($Pscmdlet.ShouldProcess($destination, "Importing sp_configure from $Path")) { - $server.Configuration.ShowAdvancedOptions.ConfigValue = $true + # 'show advanced options' is not touched here. It used to be set on the Configuration collection + # without ever calling Alter(), so it never reached the instance - but it did leave a pending + # change on the server object of the caller, which their next Alter() would have applied. + # The file written by Export-DbaSpConfigure sets the option itself, first to 1 and then back to 0. $sql = Get-Content $Path foreach ($line in $sql) { try { @@ -231,7 +289,6 @@ function Import-DbaSpConfigure { Stop-Function -Message "$line failed. Feature may not be supported." -ErrorRecord $_ -Continue } } - $server.Configuration.ShowAdvancedOptions.ConfigValue = $false Write-Message -Level Warning -Message "Some configuration options will be updated once SQL Server is restarted." } } @@ -239,10 +296,14 @@ function Import-DbaSpConfigure { end { if (Test-FunctionInterrupt) { return } - if ($PSBoundParameters.Path) { + # Only close the connections that were opened here. See #10554. + if ($isNewServerConnection) { $server.ConnectionContext.Disconnect() - } else { + } + if ($isNewSourceConnection) { $sourceserver.ConnectionContext.Disconnect() + } + if ($isNewDestinationConnection) { $destserver.ConnectionContext.Disconnect() } diff --git a/tests/Import-DbaSpConfigure.Tests.ps1 b/tests/Import-DbaSpConfigure.Tests.ps1 index eda14cbcd3e..ba4ffc68b5c 100644 --- a/tests/Import-DbaSpConfigure.Tests.ps1 +++ b/tests/Import-DbaSpConfigure.Tests.ps1 @@ -25,8 +25,225 @@ Describe $CommandName -Tag UnitTests { } } } -<# - Integration test should appear below and are custom to the command you are writing. - Read https://github.com/dataplat/dbatools/blob/development/contributing.md#tests - for more guidence. -#> \ No newline at end of file + +Describe $CommandName -Tag IntegrationTests { + BeforeAll { + # We want to run all commands in the BeforeAll block with EnableException to ensure that the test fails if the setup fails. + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + # The file is exported from the same instance it is imported into again, so that no test changes the + # configuration of the lab. + $exportPath = "$($TestConfig.Temp)\$CommandName-$(Get-Random)" + $null = New-Item -Path $exportPath -ItemType Directory + $configFile = Export-DbaSpConfigure -SqlInstance $TestConfig.InstanceSingle -Path $exportPath + + # We want to run all commands outside of the BeforeAll block without EnableException to be able to test for specific warnings. + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + AfterAll { + # We want to run all commands in the AfterAll block with EnableException to ensure that the test fails if the cleanup fails. + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + Remove-Item -Path $exportPath -Recurse -ErrorAction SilentlyContinue + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + Context "The connection of the caller is left alone when importing from a file (#10554)" { + BeforeAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + # Only a non-pooled connection can show this. SMO silently reopens a pooled connection, so the test + # would pass even with the disconnect this is about. + $callerServer = Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle -NonPooledConnection + $null = $callerServer.ConnectionContext.ExecuteNonQuery("CREATE TABLE #dbatoolsci_marker (id INT)") + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + + # The command always warns about a possible restart, so the warning is silenced and asserted below. + $splatImport = @{ + SqlInstance = $callerServer + Path = $configFile.FullName + WarningAction = "SilentlyContinue" + } + $null = Import-DbaSpConfigure @splatImport + } + + AfterAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $null = $callerServer | Disconnect-DbaInstance + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + It "leaves the connection open" { + $callerServer.ConnectionContext.IsOpen | Should -BeTrue + } + + It "leaves the connection open, so the session survives" { + { $callerServer.ConnectionContext.ExecuteScalar("SELECT COUNT(*) FROM #dbatoolsci_marker") } | Should -Not -Throw + } + + It "warns that a restart may be needed" { + # The warning about the restart is the last one. On instances where the edition does not allow one of + # the options in the file to be set, the command warns about those lines first. + $WarnVar[-1] | Should -Match "Some configuration options will be updated once SQL Server is restarted" + } + } + + Context "No pending configuration change is left on the server object of the caller (#10554)" { + BeforeAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + # The command used to set show advanced options on the Configuration collection of the caller without + # ever calling Alter(). That only shows while the option is enabled on the instance, because the + # pending value left behind was 0 while the running value was 1, and the next Alter() of the caller + # would have applied it. + $setupServer = Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle + $originalShowAdvancedOptions = $setupServer.Configuration.ShowAdvancedOptions.ConfigValue + $setupServer.Configuration.ShowAdvancedOptions.ConfigValue = $true + $setupServer.Configuration.Alter($true) + + $advancedServer = Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle -NonPooledConnection + $showAdvancedOptionsBefore = $advancedServer.Configuration.ShowAdvancedOptions.ConfigValue + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + + $splatImportAdvanced = @{ + SqlInstance = $advancedServer + Path = $configFile.FullName + WarningAction = "SilentlyContinue" + } + $null = Import-DbaSpConfigure @splatImportAdvanced + } + + AfterAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $null = $advancedServer | Disconnect-DbaInstance + + $setupServer.Configuration.ShowAdvancedOptions.ConfigValue = $originalShowAdvancedOptions + $setupServer.Configuration.Alter($true) + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + It "reads the option as enabled before the import" { + $showAdvancedOptionsBefore | Should -Be 1 + } + + It "leaves the option on the server object of the caller as it was" { + $advancedServer.Configuration.ShowAdvancedOptions.ConfigValue | Should -Be $showAdvancedOptionsBefore + } + } + + Context "The copy applies what differs and leaves both callers alone (#10554)" { + BeforeAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $setupServer = Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle + + # The command has to put this back the way it found it, so the test runs with it switched on. + $originalShowAdvancedOptions = $setupServer.Configuration.ShowAdvancedOptions.ConfigValue + $setupServer.Configuration.ShowAdvancedOptions.ConfigValue = $true + $setupServer.Configuration.Alter($true) + + # Source and destination are the same instance, so nothing but the one option below can change and + # the test does not need a second instance. The two server objects are made to disagree the way two + # instances would: the source is connected while the option still has the value that is to be + # copied, the instance is then changed, and only then is the destination connected. SMO reads the + # configuration once per server object, so each of them keeps the value it saw. + $originalCostThreshold = $setupServer.Configuration.CostThresholdForParallelism.RunValue + $changedCostThreshold = $originalCostThreshold + 7 + + $sourceServer = Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle -NonPooledConnection + $null = $sourceServer.Configuration.Properties.Count + $null = $sourceServer.ConnectionContext.ExecuteNonQuery("CREATE TABLE #dbatoolsci_source_marker (id INT)") + + $setupServer.Configuration.CostThresholdForParallelism.ConfigValue = $changedCostThreshold + $setupServer.Configuration.Alter($true) + + $destinationServer = Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle -NonPooledConnection + $null = $destinationServer.Configuration.Properties.Count + $null = $destinationServer.ConnectionContext.ExecuteNonQuery("CREATE TABLE #dbatoolsci_destination_marker (id INT)") + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + + $null = Import-DbaSpConfigure -Source $sourceServer -Destination $destinationServer + + # Every other command below writes to $WarnVar as well, so it has to be kept here. + $copyWarnings = $WarnVar + + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $configurationQuery = @" +SELECT name, value, value_in_use FROM sys.configurations WHERE name IN ('cost threshold for parallelism', 'show advanced options') +"@ + $configurationAfter = Invoke-DbaQuery -SqlInstance $TestConfig.InstanceSingle -Query $configurationQuery + $costThresholdAfter = ($configurationAfter | Where-Object name -eq "cost threshold for parallelism").value_in_use + $showAdvancedOptionsAfter = ($configurationAfter | Where-Object name -eq "show advanced options").value_in_use + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + AfterAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $null = $sourceServer, $destinationServer | Disconnect-DbaInstance + + $setupServer.Configuration.Refresh() + $setupServer.Configuration.CostThresholdForParallelism.ConfigValue = $originalCostThreshold + $setupServer.Configuration.ShowAdvancedOptions.ConfigValue = $originalShowAdvancedOptions + $setupServer.Configuration.Alter($true) + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + It "copies the option the two instances disagree about" { + $costThresholdAfter | Should -Be $originalCostThreshold + } + + It "leaves show advanced options the way it found it" { + $showAdvancedOptionsAfter | Should -Be 1 + } + + It "does not warn, because the option that changed takes effect without a restart" { + $copyWarnings | Should -BeNullOrEmpty + } + + It "leaves the source connection open" { + $sourceServer.ConnectionContext.IsOpen | Should -BeTrue + } + + It "leaves the destination connection open" { + $destinationServer.ConnectionContext.IsOpen | Should -BeTrue + } + + It "leaves the source connection open, so the session survives" { + { $sourceServer.ConnectionContext.ExecuteScalar("SELECT COUNT(*) FROM #dbatoolsci_source_marker") } | Should -Not -Throw + } + + It "leaves the destination connection open, so the session survives" { + { $destinationServer.ConnectionContext.ExecuteScalar("SELECT COUNT(*) FROM #dbatoolsci_destination_marker") } | Should -Not -Throw + } + } + + Context "The command still closes the connection it opens itself" { + BeforeAll { + # Passing the name instead of a server object is the other side of the guard: the command opens the + # connection here, so it is the one that has to close it again. + $splatImportByName = @{ + SqlInstance = $TestConfig.InstanceSingle + Path = $configFile.FullName + WarningAction = "SilentlyContinue" + } + $null = Import-DbaSpConfigure @splatImportByName + } + + It "runs to the end and warns that a restart may be needed" { + $WarnVar[-1] | Should -Match "Some configuration options will be updated once SQL Server is restarted" + } + } +}