SqlServerDsc: Resources work with automatic seeding

[Sidoran]

Pull Request (PR) description

• SqlAg
  • Added optional parameter SeedingMode that will set the SeedingMode for the SQL
    Server 2016 and higher. This parameter can only be used together with the
    module SqlServer installed (tested v21.0.17099). The parameter will be
    ignored if SQLPS module will be used.
• SqlAgReplica
  • Added optional parameter SeedingMode that will set the SeedingMode for the SQL
    Server 2016 and higher. This parameter can only be used together with the
    module SqlServer installed (tested v21.0.17099). The parameter will be
    ignored if SQLPS module will be used.

This Pull Request (PR) fixes the following issues

• Fixes SqlAGReplica: Support automatic seeding #487

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 #1915 (b78a8b2) into main (be2f562) will decrease coverage by 1%.
The diff coverage is 55%.

@@         Coverage Diff          @@
##           main   #1915   +/-   ##
====================================
- Coverage    92%     91%   -1%     
====================================
  Files        91      91           
  Lines      7761    7788   +27     
====================================
+ Hits       7149    7164   +15     
- Misses      612     624   +12     

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

Impacted Files	Coverage Δ
source/DSCResources/DSC_SqlAG/DSC_SqlAG.psm1	0% <0%> (ø)
...SCResources/DSC_SqlAGReplica/DSC_SqlAGReplica.psm1	99% <93%> (-1%)	⬇️

[Sidoran]

Hi @johlju, can you please look at PR? Can you please also rerun the verification pipeline because two agents stopped to respond, and the whole pipeline is marked as failed?

[johlju]

I just made.a quick review and found some thing you can start with. Need to do more review. I only commented on the first code file, but the same comments are for the other as well.

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

---

CHANGELOG.md line 54 at r3 (raw file):

    (tested v21.0.17099). The parameter will be ignored if SQLPS module will be
    used.
>>>>>>> upstream/main

This seems to have been leftover.

Code quote:

>>>>>>> upstream/main

---

source/DSCResources/DSC_SqlAG/DSC_SqlAG.psm1 line 99 at r3 (raw file):

            $alwaysOnAvailabilityGroupResource.Add('DatabaseHealthTrigger', $availabilityGroup.DatabaseHealthTrigger)
            $alwaysOnAvailabilityGroupResource.Add('DtcSupportEnabled', $availabilityGroup.DtcSupportEnabled)
            if ( $sqlModuleName -eq 'SQLServer' )

We should always return this property, either with a value or $null.

Code quote:

            if ( $sqlModuleName -eq 'SQLServer' )
            {
                $alwaysOnAvailabilityGroupResource.Add('SeedingMode', $availabilityGroup.AvailabilityReplicas[$serverObject.DomainInstanceName].SeedingMode)
            }

---

source/DSCResources/DSC_SqlAG/DSC_SqlAG.psm1 line 359 at r3 (raw file):

                }

                if ( ( $sqlMajorVersion -ge 13 ) -and ( $sqlModuleName -eq 'SQLServer' ) )

If there is a chance that the command not always has the parameter then it would be better to check if the actual command supports the parameter. So here we can do this instead:

(Get-Command -Name 'New-SqlAvailabilityReplica').Parameters.ContainsKey('SeedingMode')

Code quote:

if ( ( $sqlMajorVersion -ge 13 ) -and ( $sqlModuleName -eq 'SQLServer' ) )

---

source/DSCResources/DSC_SqlAG/DSC_SqlAG.psm1 line 524 at r3 (raw file):

                }

                if ( ( $submittedParameters -contains 'SeedingMode' ) -and ( $sqlMajorVersion -ge 13 ) -and ( $SeedingMode -ne $availabilityGroup.AvailabilityReplicas[$serverObject.DomainInstanceName].SeedingMode ) -and ( $sqlModuleName -eq 'SQLServer' ) )

Could we split the if-statement into at least two to make it easier to read? For example first check if seeding mode should be enforce, then if it is correct version and the property exist.

Code quote:

if ( ( $submittedParameters -contains 'SeedingMode' ) -and ( $sqlMajorVersion -ge 13 ) -and ( $SeedingMode -ne $availabilityGroup.AvailabilityReplicas[$serverObject.DomainInstanceName].SeedingMode ) -and ( $sqlModuleName -eq 'SQLServer' ) )

---

source/DSCResources/DSC_SqlAG/DSC_SqlAG.psm1 line 524 at r3 (raw file):

                }

                if ( ( $submittedParameters -contains 'SeedingMode' ) -and ( $sqlMajorVersion -ge 13 ) -and ( $SeedingMode -ne $availabilityGroup.AvailabilityReplicas[$serverObject.DomainInstanceName].SeedingMode ) -and ( $sqlModuleName -eq 'SQLServer' ) )

If there is a chance that the type does not contain the property, we should check for that instead of looking for the module name. Like:

($availabilityGroup.AvailabilityReplicas[$serverObject.DomainInstanceName] | Get-Member  -MemberType Property).Name -contains 'SeedingMode'

Code quote:

$sqlModuleName -eq 'SQLServer'

[Sidoran]

@johlju, I made the changes according to your comments.
The only thing confusing me is the code responsible for the Get, Update (inside Set), and Test using Microsoft.SqlServer.Management.Smo.Server object which supports SeedingMode for all SQL 2016 and newer. The only issue I have is the New-SQLAvailabilityReplica command in SQLPS module does not support SeedingMode parameters. And that's why I have to use the if ( (Get-Command -Name 'New-SqlAvailabilityReplica').Parameters.ContainsKey('SeedingMode') ) in all places. If you will have some idea how to get rid of this check I would be happy to rewrite this part of the code.

[johlju]

If you will have some idea how to get rid of this check I would be happy to rewrite this part of the code.

Only way is to connect to the replica (e.g. Connect-SqlDscDatabaseEngine to get the Server object. The set the SeedingMode property on the right object. But that might be a bigger (too big?) change?

[johlju]

Reviewed 1 of 5 files at r1, 1 of 1 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Sidoran)

[johlju]

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @Sidoran)

---

source/DSCResources/DSC_SqlAG/DSC_SqlAG.psm1 line 359 at r5 (raw file):

                }

                if ( ( $sqlMajorVersion -ge 13 ) -and (Get-Command -Name 'New-SqlAvailabilityReplica').Parameters.ContainsKey('SeedingMode') )

Instead of having this check, I thing we should have this check one time at the beginning of the Set-method. If the user passes SeedingMode and version is lower than 13 then it should throw an exception saying that "using SeedingMode for verison lower than 13 is not supported".

Code quote:

$sqlMajorVersion -ge 13

[johlju]

Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @Sidoran)

a discussion (no related file):
Is it possible for you to add unit tests to cover the changed (added) code paths?

---

[Sidoran]

Previously, johlju (Johan Ljunggren) wrote…

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @Sidoran)

source/DSCResources/DSC_SqlAG/DSC_SqlAG.psm1 line 359 at r5 (raw file):

                }

                if ( ( $sqlMajorVersion -ge 13 ) -and (Get-Command -Name 'New-SqlAvailabilityReplica').Parameters.ContainsKey('SeedingMode') )

Instead of having this check, I thing we should have this check one time at the beginning of the Set-method. If the user passes SeedingMode and version is lower than 13 then it should throw an exception saying that "using SeedingMode for verison lower than 13 is not supported".

Code quote:

$sqlMajorVersion -ge 13

The Major version check at the beginning will break the current behavior for the BasicAvailabilityGroup, DatabaseHealthTrigger and DtcSupportEnabled availability group options.

Previously, johlju (Johan Ljunggren) wrote…

> Is it possible for you to add unit tests to cover the changed (added) code paths?

I've already made some unit test modifications for the SqlAGReplica resource. The SqlAg, as I see, is uncovered by pester because of migration to Pester 5.x

Previously, johlju (Johan Ljunggren) wrote…

> Only way is to connect to the replica (e.g. `Connect-SqlDscDatabaseEngine` to get the `Server` object. The set the `SeedingMode` property on the right object. But that might be a bigger (too big?) change?

Sorry, I didn't get your Idea. The Connect-SqlDscDatabaseEngine will work with the SeedingMode because it uses the Connect-SQL, which uses the SMO object.
IMHO, the problem is in the New-SqlAvailabilityReplica command that has the SeedingMode parameter in the SQLServer module and doesn't have it in the case of SQLPS(which is the default).

[johlju]

The Major version check at the beginning will break the current behavior for the BasicAvailabilityGroup, DatabaseHealthTrigger and DtcSupportEnabled availability group options.

I haven’t look at the code so not sure, but I meant: “if major version is less than 13 and PSBoundParameters contain SeedingMode then throw an exception telling the user to not specify SeedingMode as it is not supported”. I might be wrong but I thought you had that check because it was supported in lower major versions?

[johlju]

Sorry, I didn't get your Idea. The Connect-SqlDscDatabaseEngine will work with the SeedingMode because it uses the Connect-SQL, which uses the SMO object.
IMHO, the problem is in the New-SqlAvailabilityReplica command that has the SeedingMode parameter in the SQLServer module and doesn't have it in the case of SQLPS(which is the default).

I meant set all the settings using the SMO objects directly (if possible) and not use any commands from either module. It was the only way I could see around that the Microsoft SQL Server Team has not added the parameter to the command in the module SQLPS.

But I’m good with the current way of checking for the parameter you added. Though maybe we should have outputted a warning message telling the user that they should use the module SqlServer if they pass the parameter SeedingMode. 🤔

[johlju]

I've already made some unit test modifications for the SqlAGReplica resource. The SqlAg, as I see, is uncovered by pester because of migration to Pester 5.x

Yes, no need to add unit tests for SqlAg if it hasn’t been converted. It probably need to be heavily refactored then anyway.

[Sidoran]

I haven’t look at the code so not sure, but I meant: “if major version is less than 13 and PSBoundParameters contain SeedingMode then throw an exception telling the user to not specify SeedingMode as it is not supported”. I might be wrong but I thought you had that check because it was supported in lower major versions?

Auto seeding was introduced in SQL 2016. When I did changes, I was based on existing behavior in a code, as you may see here

SqlServerDsc/source/DSCResources/DSC_SqlAG/DSC_SqlAG.psm1

Lines 367 to 372 in a86039c

	if ( $sqlMajorVersion -ge 13 )
	{
	$newAvailabilityGroupParams.Add('BasicAvailabilityGroup', $BasicAvailabilityGroup)
	$newAvailabilityGroupParams.Add('DatabaseHealthTrigger', $DatabaseHealthTrigger)
	$newAvailabilityGroupParams.Add('DtcSupportEnabled', $DtcSupportEnabled)
	}

. As far as I understand, this was a conscious decision not to throw an exception in case you try to use a property from SQL 2016 in an older version.

[johlju]

When I did changes, I was based on existing behavior in a code

Ah, I see. Then we continue on the same path.

[johlju]

Reviewed 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Sidoran)

[johlju]

@Sidoran this is ready to merge but got an issue with the pipeline unrelated to this PR. Trying to fix it in PR #1926 so I can merge this.

[johlju]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Sidoran)