SqlAGDatabase: Fix for issue #1492

[Fiander]

Pull Request (PR) description

Added support for automatic seeding as per issue #1492.
Added tests for this addition.

This Pull Request (PR) fixes the following issues

Fixes #1492

Task list

• 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.
• Localization strings updated.
• Examples updated.
• Unit tests updated. See DSC Community Testing Guidelines.
• Integration tests updated (where possible). See DSC Community Testing Guidelines.
• Code changes adheres to DSC Community Style Guidelines.

---

This change is 

[codecov]

Codecov Report

Merging #1642 (eaeaade) into master (f5724c7) will decrease coverage by 0%.
The diff coverage is 98%.

@@          Coverage Diff           @@
##           master   #1642   +/-   ##
======================================
- Coverage      98%     98%   -1%     
======================================
  Files          38      38           
  Lines        6177    6192   +15     
======================================
+ Hits         6085    6099   +14     
- Misses         92      93    +1     

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

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../DSCResources/DSC_SqlProtocol/DSC_SqlProtocol.psm1	100% <ø> (ø)
...Resources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1	99% <98%> (-1%)	⬇️
...urce/DSCResources/DSC_SqlMaxDop/DSC_SqlMaxDop.psm1	100% <100%> (ø)

[johlju]

Started review on this one, but need to continue another day, just did a quick read-through for now.

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @Fiander)

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 442 at r2 (raw file):

$primaryServerObject.Databases[$databaseToAddToAvailabilityGroup].LastBackupDate.ToFileTimeUtc()  | Out-Null

Is it possible to use evaluate $primaryServerObject.Databases[$databaseToAddToAvailabilityGroup].LastBackupDate using if-statement instead of having try-catch-statement for this logic. Personally it feels wrong to throw an error to verify a false state. Just checking if there isn't a better way- 🙂

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 477 at r2 (raw file):

                    catch
                    {
                        # Log the failure

If the backup is needed and fails we should try to run Add-SqlAvailabilityDatabase on it further down? Feels like this need to be moved together with the other code?

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 498 at r2 (raw file):

                    catch
                    {
                        # Log the failure

If the backup is needed and fails we should try to run Add-SqlAvailabilityDatabase on it further down? Feels like this need to be moved together with the other code?

[johlju]

In the comment above I meant to say "If the backup is needed and fails should we try to run Add-SqlAvailabilityDatabase on it further down?". AG databases are not my strong suit so need your expertise here. 🙂

[Fiander]

no problem :-)
about your comment of row 477. I did not change that code except for adding 4 spaces in front to align it with the added if ( $backupNeeded) statement. i don't know how the original code works when a backup fails, but in the unit tests there is in row 1247 a test for failed backups. so i think that should be handled correctly.

Databases that can not be added to an availability group are reported by the resource ( row 642 - 651 )

When the seedingMode = 'Manual', then a backup is made, that is restored on all replica servers with norecovery.
then when that is done, the secondary's are added to the availability group, and the changes since the last log backup are transported trough the HADR port. When that synchronization is done ( the remote LSN matches the local LSN), the database is in sync and ready.

When the seedingMode = 'Automatic', then you can add a database to an availibility group, and trough the HADR port the complete database is synchronized. But that is only possible when the LSNs of the database make sense. and therefor at least one full backup is needed. the BAK file is not needed, just a full backup. The first full backup of a database set the database_lsn to zero, and from there the synchronization takes place.

this is also the reason i added two variables: backupneeded, and restoreneeded. with automatic seeding i never need a restore, but sometimes a backup. that backup may even be to "disk = nul".

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @johlju)

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 442 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$primaryServerObject.Databases[$databaseToAddToAvailabilityGroup].LastBackupDate.ToFileTimeUtc()  | Out-Null

Is it possible to use evaluate $primaryServerObject.Databases[$databaseToAddToAvailabilityGroup].LastBackupDate using if-statement instead of having try-catch-statement for this logic. Personally it feels wrong to throw an error to verify a false state. Just checking if there isn't a better way- 🙂

Done. i didn't like that code either. now i found a way that is simpler.

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 477 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

If the backup is needed and fails we should try to run Add-SqlAvailabilityDatabase on it further down? Feels like this need to be moved together with the other code?

i dont know. all i did is put an if ( $backupNeeded) around that code.

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 498 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

If the backup is needed and fails we should try to run Add-SqlAvailabilityDatabase on it further down? Feels like this need to be moved together with the other code?

i dont know. all i did is put an if ( $backupNeeded) around that code.

[johlju]

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 477 at r2 (raw file):

Previously, Fiander wrote…

i dont know. all i did is put an if ( $backupNeeded) around that code.

Ah, I missed that it was inly whitespace diff, I thought this was all new code. My mistake,

[johlju]

Awesome work! 😃 Thanks for the explanation above. Just tiny changes needed.

Reviewed 3 of 6 files at r1, 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Fiander)

---

CHANGELOG.md, line 10 at r3 (raw file):

Resource

Tiny typo. lower-case 'r' in "resource"

---

CHANGELOG.md, line 10 at r3 (raw file):

ste-TargetResource

Tiny typo. 'Set' in "Set-TargetResource"

---

CHANGELOG.md, line 11 at r3 (raw file):

availibility

Tiny typo. "availability"

---

CHANGELOG.md, line 11 at r3 (raw file):

wil

Tiny typo. "will"

---

CHANGELOG.md, line 12 at r3 (raw file):

Quoted 6 lines of code…

- AGDatabase
  - Fix for issue ([issue #1492](https://github.com/dsccommunity/SqlServerDsc/issues/1492))
    added AutomaticSeeding for this Resource. In ste-TargetResource added logic that looks
    at all replicas of an availibility group. When automatic seedig is found, it wil use that.
  - Lots of extra tests to check AutomaticSeeding.
  - Backuppath is still needed just in case a database never has been backuped before.

Need to rebase (resolve conflicts) when you have time, and the move this back under unreleased section since it was a release done after this PR. Normale git does see that automatically and move it to the wrong place (well fotr git it is the correct place 🙂 ).

---

CHANGELOG.md, line 13 at r3 (raw file):

Backuppath

I think we should have a space between: 'backup path'?

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 237 at r3 (raw file):

Removed

Nitpick: lower-case 'r' here.

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 425 at r3 (raw file):

#Determine...

We should add space after # (# Determine...

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 425 at r3 (raw file):

an backup is not needed

should say "backup and restore is not needed"?

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 428 at r3 (raw file):

foreach ( $availabilityGroupReplica in $secondaryReplicas )

Nitpick: Can we add a blank row before this row to get some air in the code?

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 439 at r3 (raw file):

                # Because an availibility group can only be made when the database has at least one backup,
                # if there is no backup, create one. This backup is not needed for restore, only the initial setup
                # of the LSN in the database is needed

Multi line comments should use <# #> and indent the comment, like:

<#
    Because an availability group can only be made when the database has at least one backup,
    if there is no backup, create one. This backup is not needed for restore, only the initial setup
    of the LSN in the database is needed.
#>

There was a tiny typo that I changed too in availability

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 607 at r3 (raw file):

#Wanneer er een backup gemaakt is, moet die hier verwijderd worden.

Can we get this one in english please 😉

[Fiander]

Reviewable status: 0 of 5 files reviewed, 12 unresolved discussions (waiting on @johlju)

---

CHANGELOG.md, line 10 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Resource

Tiny typo. lower-case 'r' in "resource"

Done.

---

CHANGELOG.md, line 10 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

ste-TargetResource

Tiny typo. 'Set' in "Set-TargetResource"

Done.

---

CHANGELOG.md, line 11 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

availibility

Tiny typo. "availability"

Done.

---

CHANGELOG.md, line 11 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

wil

Tiny typo. "will"

Done.

---

CHANGELOG.md, line 12 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

- AGDatabase
  - Fix for issue ([issue #1492](https://github.com/dsccommunity/SqlServerDsc/issues/1492))
    added AutomaticSeeding for this Resource. In ste-TargetResource added logic that looks
    at all replicas of an availibility group. When automatic seedig is found, it wil use that.
  - Lots of extra tests to check AutomaticSeeding.
  - Backuppath is still needed just in case a database never has been backuped before.

Need to rebase (resolve conflicts) when you have time, and the move this back under unreleased section since it was a release done after this PR. Normale git does see that automatically and move it to the wrong place (well fotr git it is the correct place 🙂 ).

Done.

---

CHANGELOG.md, line 13 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Backuppath

I think we should have a space between: 'backup path'?

No. but i have changed it to $BackupPath to make clear we stil need that variable despite automaticSeeding.

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 237 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Removed

Nitpick: lower-case 'r' here.

Done.

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 425 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

#Determine...

We should add space after # (# Determine...

Done.

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 425 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

an backup is not needed

should say "backup and restore is not needed"?

Restore not needed, backup..... maybe...

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 428 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

foreach ( $availabilityGroupReplica in $secondaryReplicas )

Nitpick: Can we add a blank row before this row to get some air in the code?

Done.

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 439 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

                # Because an availibility group can only be made when the database has at least one backup,
                # if there is no backup, create one. This backup is not needed for restore, only the initial setup
                # of the LSN in the database is needed

Multi line comments should use <# #> and indent the comment, like:

<#
    Because an availability group can only be made when the database has at least one backup,
    if there is no backup, create one. This backup is not needed for restore, only the initial setup
    of the LSN in the database is needed.
#>

There was a tiny typo that I changed too in availability

Done.

---

source/DSCResources/DSC_SqlAGDatabase/DSC_SqlAGDatabase.psm1, line 607 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

#Wanneer er een backup gemaakt is, moet die hier verwijderd worden.

Can we get this one in english please 😉

oops... part of the pseudoCode ive started with... forgot to create real comment.

[johlju]

Reviewed 5 of 5 files at r9.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Fiander)

---

CHANGELOG.md, line 10 at r9 (raw file):

AGDatabase

Should be SqlAGDatabase

---

CHANGELOG.md, line 12 at r9 (raw file):

set

Upper-case 'S' in 'Set'.

---

CHANGELOG.md, line 15 at r9 (raw file):

$BackupPath

Maybe: The parameter `BackupPath` is still...
That would make it more clear fo users what is meant?

[Fiander]

Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on @johlju)

---

CHANGELOG.md, line 10 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

AGDatabase

Should be SqlAGDatabase

Done.

---

CHANGELOG.md, line 12 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

set

Upper-case 'S' in 'Set'.

Done.

---

CHANGELOG.md, line 15 at r9 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$BackupPath

Maybe: The parameter `BackupPath` is still...
That would make it more clear fo users what is meant?

Done.

[johlju]

Reviewed 1 of 1 files at r10.
Reviewable status: complete! all files reviewed, all discussions resolved

[johlju]

Reviewed 1 of 1 files at r11.
Reviewable status: complete! all files reviewed, all discussions resolved