SqlDatabaseRecoveryModel: Database name can now be a pattern

[dcrreynolds]

• Change details added to Unreleased section of CHANGELOG.md?
• Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
• Examples appropriately updated?
• New/changed code adheres to Style Guidelines?
• Unit and (optional) Integration tests created/updated where possible?

---

This change is 

[codecov-io]

Codecov Report

Merging #1052 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##            dev   #1052    +/-   ##
=====================================
+ Coverage    97%     97%   +<1%     
=====================================
  Files        33      33            
  Lines      4044    4057    +13     
=====================================
+ Hits       3957    3970    +13     
  Misses       87      87

[johlju]

@dcrreynolds thanks for sending in this PR! I think this has potential, but I have some thoughts around this change. Do you have a thought on how to solve the below?

Since the parameter Name is a Key. Then if I add two resources like below that would be allowed (configuration would compile), but this would yield a ping-pong behavior where both will always be false.

Configuration Example
{
    Import-DscResource -ModuleName SqlServerDsc

    node localhost
    {
        SqlDatabaseRecoveryModel MyDbWildcardToFull
        {
            Name                 = 'MyDb*'
            RecoveryModel        = 'Full'
            ServerName           = 'sqltest.company.local'
            InstanceName         = 'DSC'
        }

        SqlDatabaseRecoveryModel MyDbTestToSimple
        {
            Name                 = 'MyDbTest'
            RecoveryModel        = 'Simple'
            ServerName           = 'sqltest.company.local'
            InstanceName         = 'DSC'
        }
    }
}

[dcrreynolds]

I feel like that is a systemic issue of DSC, lots of DSC resources can ping pong in between themselves. We also see the same behavior with DSC and GPO or other methods of configuration.

We could have the resource log somewhere which recovery models have been configured, but something about that ideas feels like it could be problematic.

I have never seen codecov before. What is the failure trying to tell me?

[johlju]

@randomnote1 and others in the community, please can I get your input on this one? How do you see this change in relation to SqlAGDatabase?

Since SqlAGDatabase uses wildcards for the database names added to the AG I'm thinking that this change would help a lot. But at the same time I don't like the idea of introducing a potential ping-pong behavior. But maybe the pros are higher than the cons?

@dcrreynolds I think we cannot take in account changes made by other tools (like GPO), we have to see such change as a manual change, if the configuration is not in desired state then the configuration will be updated to be in desired state. But we can try make sure the configurations will not contradict each other.
Using a log as you say don't seem to be the best approach.

[johlju]

@dcrreynolds Codecov is complaining because there are not high enough code coverage with this change. Project has 97% coverage, this PR has 38% coverage of the code. You see in the #1052 (comment) that 16 rows less are hit and missed rows increased with 23 rows. Also, the tests are failing which might result in some of these hits and misses.
To solve this you need to make sure the tests passes and add unit tests to cover this change. I did not mention that before because I wanted to discuss the design first. 😄

[randomnote1]

I think that the SqlDatabaseRecoveryModel resource differs fundamentally from SqlAGDatabase because it defines the database(s) that are to be a member of a specified Availability Group. SqlDatabaseRecoveryModel is defining a property on a database.

I do see @johlju's point about ping-pong behavior. But, this behavior could potentially be seen in a resource like SqlDatabaseRecoveryModel because of the wildcards used in defining the database membership. If a database matches a wildcard in two or more resources, it will throw an error at runtime when attempting to add the database to the second AG.

With all that said, I do like the idea of being able to specify a property on a database via wildcards or RegEx as I see it being immensely valuable in my own work. I think the risk of the potential ping-pong behaviour is acceptable, however we need to ensure the feedback to the end user is extremely clear. Otherwise troubleshooting the ping-pong behavior will be a nightmare.

Could we possible surface an informational message in the SQL log when the change is made? Maybe something like The recovery model on database '<database name>' has been changed from '<old recovery model>' to '<new recovery model>' by DSC based on the pattern '<pattern>'.. This would add some work, but might make it much more friendly to troubleshoot.

[johlju]

@randomnote1 Thank you for that insight!

I agree logging this would be the best. Maybe we could log an event using xp_logevent? Is there another (better) way?
Using xp_logevent would probably require a certain permission level on the account running the resource, that permission must be documented. Also, if the required permission is high, then maybe we should only log to SQL Server log when a wildcard/regex is assigned to Name, making it possible to use lower permission when not using wildcard/regex (permission to make changes to only the particular database).

So far pros over weigh the cons - so unless someone else objects we can continue doing this change, with the suggested changes.

[johlju]

Maybe we should add a optional parameter UseWildcard or UseRegularExpression that we use to switch the behavior of the resource?

[randomnote1]

We had discussed adding flags to the SqlAgDatabase resource and decided that it was unnecessary to add the overhead of the additional parameters. Theoretically it is possible to analyze the string coming in to determine if it is a wildcard or regex pattern. I'll have to defer to folks smarter than myself to discuss the merits of that.

[randomnote1]

For logging, xp_logevent may require too high of a permissions level. However, it is highly likely that the PsDscRunAsCredential is already a member of sysadmin.

Maybe we could utilize RAISEERROR to work around the permissions requirement.

[dcrreynolds]

Great conversation. I'll work to roll in the discussed changes and fix the codecov/test issues. Thanks!

[johlju]

RAISEERROR will update both SQL Server log and the Event Log, so I think we should only use RAISEERROR if it does not generate an Error entry in the Event Log. I see that we should preferably generate an information entry in the Event Log. A warning entry might work to, but en error entry could kick of errors in monitoring tools, and we don't want that.
If RAISEERROR can generate information or warning entries then I agree to use RAISEERROR.

@dcrreynolds I agree, great conversation! When we in the community work together we get the best result! All opinions matter, wish more people had time to comment on stuff.

[johlju]

@dcrreynolds the method that you will use to write to the SQL Server log, please add that as an helper function in the helper module SqlServerDscHelper. I see that this could potentially be used more widely to write log information by other resources too.

[dcrreynolds]

After some testing, it seems that RAISEERROR can generate information log entries, but does require the user to be a member of the sysadmin fixed server role or a user with ALTER TRACE permissions.

I also learned that the SQL is generating a similar message in the SQL and app longs when it makes the change. "Setting database option RECOVERY to FULL for database 'SP_Search'." The only difference is it doesn't say why the change was made.

Does the built in logging provide enough info? Are we ok with the requirements for RAISEERROR?

[johlju]

@dcrreynolds What happens if the user does not have that permission? No information log entry in the Application Event Log, but in the SQL Server log? Or no log information at all in either Application Event Log or the SQL Server log?

The point that @randomnote1 made was that it do write why it changed or whom changed it, so it would be easier to determine what causing the change.

Depending on the answer to the questions above, I would be OK with the permission requirement for the RAISEERROR, if that is a requirement only when using the resource with a wildcard. When not using a wildcard I don't see the need for logging, since the resource could be run with a user with less privileges. I can be swayed to write a log even without a wildcard, but that would mean a breaking change.

[johlju]

Labeling this PR as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on the PR is taken up again.

[johlju]

Reopening this PR as it was wrongly closed by GitHub All Stale due to miss configuration. It still labeled as abandoned. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on the PR is taken up again.

[dcrreynolds]

Shoot, you are right. The screen shots are from an earlier test.

Awesome! I'll get to work fixing up the test. Hopefully it doesn't take me so long that this gets labeled abandoned again :)

[stale]

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

[dcrreynolds]

good to go!

[johlju]

Reviewed 1 of 4 files at r3, 3 of 4 files at r4.
Reviewable status: 4 of 5 files reviewed, 17 unresolved discussions (waiting on @dcrreynolds)

---

CHANGELOG.md, line 40 at r4 (raw file):

    AsSvcStartupType, IsSvcStartupType and RsSvcStartupType ([issue #1165](https://github.com/PowerShell/SqlServerDsc/issues/1165).
    [Maxime Daniou (@mdaniou)](https://github.com/mdaniou)
- Changes to SqlDatabaseRecoveryModel

We need to move this back up to the unreleased section (git reabse always make this merge mistake when we have done a new release).

---

DSCResources/MSFT_SqlDatabaseRecoveryModel/MSFT_SqlDatabaseRecoveryModel.psm1, line 5 at r4 (raw file):

              -Force

$logMessage = "RAISERROR('PowerShell Desired State Configuration Updated database {0} to recovery model {1} because it matched pattern {2}',1,1,1) with log"

Can we make this a helper function, together with the necessary Invoke-Query call?
Would it possible to make this a generic helper function that logs a message, optional passing in a, for example an array, of values that will be used to build the string?

If so, that helper function could be used all over SqlServerDsc to log information.

This is is non-blocking. But would love to see this as a helper function.

---

DSCResources/MSFT_SqlDatabaseRecoveryModel/MSFT_SqlDatabaseRecoveryModel.psm1, line 55 at r4 (raw file):

-like "$Name"

We shouldn't use -match to use regular expression instead? It might be more powerful than wildcard filtering. What do you think?
If so, throughout.

---

DSCResources/MSFT_SqlDatabaseRecoveryModel/MSFT_SqlDatabaseRecoveryModel.psm1, line 64 at r4 (raw file):

""

We should use single quotes (') where we can.

---

DSCResources/MSFT_SqlDatabaseRecoveryModel/MSFT_SqlDatabaseRecoveryModel.psm1, line 67 at r4 (raw file):

"tempdb"

We should use single quotes (') where we can.

---

DSCResources/MSFT_SqlDatabaseRecoveryModel/MSFT_SqlDatabaseRecoveryModel.psm1, line 69 at r4 (raw file):

            if($sqlDatabaseObject.Name -eq "tempdb")
            {
                New-VerboseMessage -Message "Skipping 'tempdb', recovery model for this DB cannot be changed"

Can we end each sentence with full stop (.). Throughout all verbose messages.

---

DSCResources/MSFT_SqlDatabaseRecoveryModel/MSFT_SqlDatabaseRecoveryModel.psm1, line 71 at r4 (raw file):

Continue

Statements should use lower-case (continue).

---

DSCResources/MSFT_SqlDatabaseRecoveryModel/MSFT_SqlDatabaseRecoveryModel.psm1, line 78 at r4 (raw file):

$sqlDatabaseRecoveryModel += ",$($sqlDatabaseObject.RecoveryModel)"

I think we must introduce a read-only property that returns and array of an embedded instances (CIM subclass), that contains an array of DatabaseName and RecoveryModel.
See example
https://github.com/PowerShell/xWebAdministration/blob/7c3efd3fb3549c39df161798db77b20fead64ea8/DSCResources/MSFT_xWebsite/MSFT_xWebsite.schema.mof#L24-L30

This way it is possible to see what database has what recovery model. And the property RecoveryModel returns the expected recovery model (that the user wanted in the configuration).

Would that work?

---

DSCResources/MSFT_SqlDatabaseRecoveryModel/MSFT_SqlDatabaseRecoveryModel.psm1, line 145 at r4 (raw file):

    if ($sqlServerObject)
    {
        $databases = $sqlServerObject.Databases.Where{$_.Name -like "$Name"}

See previous comment about using regular expression.

---

DSCResources/MSFT_SqlDatabaseRecoveryModel/MSFT_SqlDatabaseRecoveryModel.psm1, line 156 at r4 (raw file):

"tempdb"

We should use single quotes (') where we can.

---

DSCResources/MSFT_SqlDatabaseRecoveryModel/MSFT_SqlDatabaseRecoveryModel.psm1, line 160 at r4 (raw file):

Continue

Statements should use lower-case (continue).

---

DSCResources/MSFT_SqlDatabaseRecoveryModel/MSFT_SqlDatabaseRecoveryModel.psm1, line 234 at r4 (raw file):

    Write-Verbose -Message "Testing RecoveryModel of database '$Name'"

    $currentValues = Get-TargetResource @PSBoundParameters

This doesn't seem it can determine if one of the database does not have the correct recovery model?
Maybe this should evaluate the new read-only parameter suggested in a previous comment?

---

Examples/Resources/SqlDatabaseRecoveryModel/2-SetDatabaseRecoveryModel.ps1, line 47 at r4 (raw file):

SqlDatabaseRecoveryModel

Maybe add SqlDatabaseRecoveryModel instance for the 'Adventureworks' database, with another recovery module, too?

---

Tests/Unit/MSFT_SqlDatabaseRecoveryModel.Tests.ps1, line 51 at r4 (raw file):

Class

class (lower-case 'c'). Throughout.

---

Tests/Unit/MSFT_SqlDatabaseRecoveryModel.Tests.ps1, line 53 at r4 (raw file):

[string]$Name

We should have space between the type and variable name [string] $Name. Throughout.

---

Tests/Unit/MSFT_SqlDatabaseRecoveryModel.Tests.ps1, line 117 at r4 (raw file):

                        "on SQL server 'localhost\MSSQLSERVER'.")

                   # Mock -CommandName Connect-SQL -MockWith { throw $throwInvalidOperation }

Should the commented row be removed?

---

Tests/Unit/MSFT_SqlDatabaseRecoveryModel.Tests.ps1, line 205 at r4 (raw file):

Should -Be ""

We should use single quotes where we can.

[stale]

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

[johlju]

@dcrreynolds just pinging to see if you have time to work on this?

[dcrreynolds]

Hi! Yes I can finish this up. I didn't realize this had gotten to be over a year old!

[johlju]

@dcrreynolds Time flies when one are having fun 😄 Would be great to get this merged. I hope it is easy to rebase this one after this long.

[dcrreynolds]

Hi! I haven't forgotten about this, spent some time merging everything in tonight. For the review above, I have made most changes.

I do think keeping the logic using the like competitor is better than using match. Using match will break a lot of backwards compatibility since it will likely accidentally start applying to a lot of new DBs.

I've added a read-only property named DatabaseInfo that is a Hashtable of DBName and RecoveryMode. This is currently configured to only include matching DB names to avoid confusion. The RecoveryModel parameter was left unchanged since returning the desired model as opposed to the actual would lead to some confusion. A comma separated list is informative without dropping data, allows for a quick status comparison, and the new hashtable supports better access programatically.

Let me know if all of this works for you.

[johlju]

@dcrreynolds will be great to get this to the finish line. I read you comments. Since it was a while ago, would you mind go into Reviewable and write 'Done' on the review comments that is fixed, and add the respective comments you made in the previous comment inside the Reviewable so I understand the context. 🙂

Also, You said you merged changes into your working branch, but I don't see any new commits here yet. Did you (force) push the changes? I guess that you also resolved the conflicts since you merged the recent changes.

[johlju]

Closing this PR due to inactivity. If there are someone who wants to work on this issue or PR please open a new PR.