Connect-SQL: Add Wait for Online SMO Status

[Kreby]

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

• Fixes SqlSetup: Working SqlServerDsc SQL 2019 configuration initially fails on SQL 2022 then passes #1816

Task list

• [ X] Added an entry to the change log under the Unreleased section of the
  file CHANGELOG.md. Entry should say what was changed and how that
  affects users (if applicable), and reference the issue being resolved
  (if applicable).
• Resource documentation updated in the resource's README.md.
• Resource parameter descriptions updated in schema.mof.
• Comment-based help updated, including parameter descriptions.
• [ X] Localization strings updated.
• Examples updated.
• [ X] Unit tests updated. See DSC Community Testing Guidelines.
• Integration tests updated (where possible). See DSC Community Testing Guidelines.
• [ X] Code changes adheres to DSC Community Style Guidelines.

---

This change is 

[Kreby]

@johlju I had this working, passing all the tests from the pipeline, but then when I ran my actual test deployments I was seeing weird issues. I don't think they are related to this change however. I've submitted as a draft right now until I can do some further testing tomorrow. At least you can take a look at it in the mean time.

Would you be able to provide me the non English localization of the string I added?

I'm not sure if any additional or modified unit/integration tests are needed based on what was added. Let me know if you think otherwise.

[codecov]

Codecov Report

Merging #1916 (d822269) into main (43a080e) will decrease coverage by 1%.
The diff coverage is 100%.

@@         Coverage Diff          @@
##           main   #1916   +/-   ##
====================================
- Coverage    92%     92%   -1%     
====================================
  Files        91      91           
  Lines      7732    7741    +9     
====================================
  Hits       7127    7127           
- Misses      605     614    +9     

Flag	Coverage Δ
unit	92% <100%> (-1%)	⬇️

Impacted Files	Coverage Δ
...dules/SqlServerDsc.Common/SqlServerDsc.Common.psm1	96% <100%> (-2%)	⬇️

[johlju]

Would you be able to provide me the non English localization of the string I added?

Keep them as english, it can be translated in another PR.

[johlju]

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Kreby)

---

source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 line 584 at r1 (raw file):

            Write-Verbose -Message (
                $script:localizedData.WaitForDatabaseEngineInstanceStatus -f $instanceStatus, $onlineStatus

Maybe we should output this once after like 20 seconds? Other wise it could theoretically a lot of them during the $StatementTimeout.

Code quote:

            Write-Verbose -Message (
                $script:localizedData.WaitForDatabaseEngineInstanceStatus -f $instanceStatus, $onlineStatus
            ) -Verbose

---

source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 line 591 at r1 (raw file):

        } while ($connectTimer.Elapsed.TotalSeconds -lt $StatementTimeout)

        $connectTimer.Stop()

Think we should stop this down in finally so that we don't keep counting if an error occurs.

Code quote:

$connectTimer.Stop()

---

source/Modules/SqlServerDsc.Common/en-US/SqlServerDsc.Common.strings.psd1 line 47 at r1 (raw file):

    FailedToObtainServerInstance = Failed to obtain a SQL Server instance with name '{0}' on server '{1}'. Ensure the SQL Server instance exists on the server and that the 'SQLServer' module references a version of the 'Microsoft.SqlServer.Management.Smo.Wmi' library that supports the version of the SQL Server instance. (SQLCOMMON0070)
    DatabaseEngineInstanceNotOnline = The SQL instance '{0}' was expected to have the status 'Online', but had status '{1}'. (SQLCOMMON0071)
    WaitForDatabaseEngineInstanceStatus = The SQL instance status is '{0}' waiting for '{1}'.  (SQLCOMMON0072)"

Suggest we also mention that we are "...waiting for x number of seconds". See also other comment of just output it once after like 20 seconds.

Code quote:

WaitForDatabaseEngineInstanceStatus = The SQL instance status is '{0}' waiting for '{1}'.  (SQLCOMMON0072)"

---

source/Modules/SqlServerDsc.Common/sv-SE/SqlServerDsc.Common.strings.psd1 line 53 at r1 (raw file):

    FailedToObtainServerInstance = Failed to obtain a SQL Server instance with name '{0}' on server '{1}'. Ensure the SQL Server instance exists on the server and that the 'SQLServer' module references a version of the 'Microsoft.SqlServer.Management.Smo.Wmi' library that supports the version of the SQL Server instance. (SQLCOMMON0070)
    DatabaseEngineInstanceNotOnline = The SQL instance '{0}' was expected to have the status 'Online', but had status '{1}'. (SQLCOMMON0071)
    WaitForDatabaseEngineInstanceStatus = The SQL instance status is '{0}' waiting for '{1}'.  (SQLCOMMON0072)"

Same change here as previous comment.

Code quote:

WaitForDatabaseEngineInstanceStatus = The SQL instance status is '{0}' waiting for '{1}'.  (SQLCOMMON0072)"

[johlju]

I'm not sure if any additional or modified unit/integration tests are needed based on what was added. Let me know if you think otherwise.

It is possible but complicated to write unit test to test this change. Not sure it is worth the effort so did not comment on it. 🙂 Just a few other comments.

[Kreby]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @johlju)

---

source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 line 584 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe we should output this once after like 20 seconds? Other wise it could theoretically a lot of them during the $StatementTimeout.

Theoretically yes, however the loop only executes after the blocking Connect() was successful. So this loop could run multiple times but from all of my testing will likely only run once or twice, if at all. While the Connect() is blocking nothing will be output so this is really only ever going to happen once the connection has been established and while waiting for the SMO Status property to go from $null to Online.

I'm not opposed to a change but this is just output for the SMO Status initialization not the connection to the SQL Server itself. Does that make sense?

---

source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 line 591 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Think we should stop this down in finally so that we don't keep counting if an error occurs.

Sure, I'll make this change.

---

source/Modules/SqlServerDsc.Common/en-US/SqlServerDsc.Common.strings.psd1 line 47 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Suggest we also mention that we are "...waiting for x number of seconds". See also other comment of just output it once after like 20 seconds.

I'll make an update eventually based on the outcome of the above conversation around the waiting.

---

source/Modules/SqlServerDsc.Common/sv-SE/SqlServerDsc.Common.strings.psd1 line 53 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same change here as previous comment.

I'll make an update eventually based on the outcome of the above conversation around the waiting.

[johlju]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Kreby)

---

source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 line 584 at r1 (raw file):

Previously, Kreby wrote…

Theoretically yes, however the loop only executes after the blocking Connect() was successful. So this loop could run multiple times but from all of my testing will likely only run once or twice, if at all. While the Connect() is blocking nothing will be output so this is really only ever going to happen once the connection has been established and while waiting for the SMO Status property to go from $null to Online.

I'm not opposed to a change but this is just output for the SMO Status initialization not the connection to the SQL Server itself. Does that make sense?

My thought is that we don't need to output the verbose message at all normally (just unnecessary verbose messages). But if it for some reason takes longer then we output the verbose message, or could be a warning too, that it taking longer than expected. Or, we could change it to a Write-Debug instead and keep the logic as-is.

[Kreby]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @johlju)

---

source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 line 584 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

My thought is that we don't need to output the verbose message at all normally (just unnecessary verbose messages). But if it for some reason takes longer then we output the verbose message, or could be a warning too, that it taking longer than expected. Or, we could change it to a Write-Debug instead and keep the logic as-is.

I'm good with using Write-Debug instead since it is a bit more of a debug message than a verbose one if you are okay with it.

[johlju]

source/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 line 584 at r1 (raw file):

Previously, Kreby wrote…

I'm good with using Write-Debug instead since it is a bit more of a debug message than a verbose one if you are okay with it.

I’m good with it.

[johlju]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Kreby)

[johlju]

I don't think they are related to this change however. I've submitted as a draft right now until I can do some further testing tomorrow.

Guessing the issue you were seeing was unrelated. The change should only make it better, didn't see anything that would make anything fail (unless there is an extreme edge case). 🙂 I merge this and if there is any issues we can do an additional PR.

[Kreby]

@johlju Yes, whatever I was seeing was just some transient Azure issues. I didn't have time to review after the builds finished yesterday after the most recent push. I was going to update it this morning, but it looks like you beat me to it. Thanks for merging and your help.