-
-
Notifications
You must be signed in to change notification settings - Fork 794
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Support for Contained Availability Groups #9345
New Support for Contained Availability Groups #9345
Conversation
added an extra check for major version to be consistent with source
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are on a good way and I don't see a risk of breaking functionallity. But I would like to have some things changed before we merge.
@@ -1,4 +1,4 @@ | |||
function New-DbaAvailabilityGroup { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a change here so there might be a problem with the encoding or the BOM. Please make sure that this line is not part of the diff.
public/New-DbaAvailabilityGroup.ps1
Outdated
.PARAMETER IsContained | ||
Builds the Availability Group as contained. Only supported in SQL Server 2022 or higher. | ||
|
||
https://learn.microsoft.com/en-us/sql/database-engine/availability-groups/windows/contained-availability-groups-overview?view=sql-server-ver16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would exclude the "?view=sql-server-ver16" to always point to the latest version.
public/New-DbaAvailabilityGroup.ps1
Outdated
} | ||
|
||
if ($ReuseSystemDatabases -and $IsContained -eq $false) { | ||
Write-Message -Level Warning -Message "Reuse system databases is only applicable in contained availability groups test " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "test" mean? And there is a space at the end of the string.
public/New-DbaAvailabilityGroup.ps1
Outdated
https://learn.microsoft.com/en-us/sql/database-engine/availability-groups/windows/contained-availability-groups-overview?view=sql-server-ver16 | ||
|
||
.PARAMETER ReuseSystemDatabases | ||
Used when rebuilding a cluster where system databases already exist for the contained availability group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not use the term "cluster" here. Quote from the documentation: "This option is only valid for contained AGs, and specifies that the newly created AG should reuse existing contained system databases for a previous contained AG of the same name. For example, if you had a contained AG with the name MyContainedAG, and wanted to drop and recreate it, you could use this option to reuse the contents of the original contained system databases."
public/New-DbaAvailabilityGroup.ps1
Outdated
.EXAMPLE | ||
PS C:\> New-DbaAvailabilityGroup -Primary sql2022n01 -Secondary sql2022n02 -Name AgContained -IsContained | ||
|
||
Creates a contained availability group named AgContained on sql2022 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of "sql2022" (which is your cluster name?) we should name the nodes: "sql2022n01 and sql2022n02".
public/New-DbaAvailabilityGroup.ps1
Outdated
@@ -488,6 +511,7 @@ function New-DbaAvailabilityGroup { | |||
$ag.AutomatedBackupPreference = [Microsoft.SqlServer.Management.Smo.AvailabilityGroupAutomatedBackupPreference]::$AutomatedBackupPreference | |||
$ag.FailureConditionLevel = [Microsoft.SqlServer.Management.Smo.AvailabilityGroupFailureConditionLevel]::$FailureConditionLevel | |||
$ag.HealthCheckTimeout = $HealthCheckTimeout | |||
$ag.ReuseSystemDatabases = $ReuseSystemDatabases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is saver (and better to read) if we move this line after the new line 527 ($ag.IsContained = $IsContained). This way, the two new parameters are used only on 2022 and newer. This way, the code does not change for older versions of sql server.
@@ -5,7 +5,7 @@ Write-Host -Object "Running $PSCommandpath" -ForegroundColor Cyan | |||
Describe "$commandname Unit Tests" -Tag 'UnitTests' { | |||
Context "Validate parameters" { | |||
[object[]]$params = (Get-Command $CommandName).Parameters.Keys | Where-Object { $_ -notin ('whatif', 'confirm') } | |||
[object[]]$knownParameters = 'Primary', 'PrimarySqlCredential', 'Secondary', 'SecondarySqlCredential', 'Name', 'DtcSupport', 'ClusterType', 'AutomatedBackupPreference', 'FailureConditionLevel', 'HealthCheckTimeout', 'Basic', 'DatabaseHealthTrigger', 'Passthru', 'Database', 'SharedPath', 'UseLastBackup', 'Force', 'AvailabilityMode', 'FailoverMode', 'BackupPriority', 'ConnectionModeInPrimaryRole', 'ConnectionModeInSecondaryRole', 'SeedingMode', 'Endpoint', 'EndpointUrl', 'Certificate', 'ConfigureXESession', 'IPAddress', 'SubnetMask', 'Port', 'Dhcp', 'EnableException' | |||
[object[]]$knownParameters = 'Primary', 'PrimarySqlCredential', 'Secondary', 'SecondarySqlCredential', 'Name', 'DtcSupport', 'ClusterType', 'AutomatedBackupPreference', 'FailureConditionLevel', 'HealthCheckTimeout', 'Basic', 'DatabaseHealthTrigger', 'Passthru', 'Database', 'SharedPath', 'UseLastBackup', 'Force', 'AvailabilityMode', 'FailoverMode', 'BackupPriority', 'ConnectionModeInPrimaryRole', 'ConnectionModeInSecondaryRole', 'SeedingMode', 'Endpoint', 'EndpointUrl', 'Certificate', 'ConfigureXESession', 'IPAddress', 'SubnetMask', 'Port', 'Dhcp', 'EnableException', 'ReuseSystemDatabases', 'IsContained' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to have the order of parameters in this list in the order of the param block. So after "name" in this case.
Thanks for the feedback! I'll get these fixed Monday. |
Thank you both 🙏🏼 Will fix encoding issue once changes are complete, just prior to merge. |
changes requested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I have not seen this in the first review. Please align the new parameter tests with the existing parameter tests.
@@ -366,6 +381,14 @@ function New-DbaAvailabilityGroup { | |||
return | |||
} | |||
|
|||
if ($IsContained -and $server.VersionMajor -lt 16) { | |||
Stop-Function -Level Warning -Message "Contained availability groups are only supported in SQL Server 2022 and above" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aa you can see some lines above, we need an additional "return" in case Stop-Function only issues a warning and not an exception (this depends on -EnableException). And please remove the -Level Warning
. But you should add the -Target $Primary
so that all the tests are written the same way.
public/New-DbaAvailabilityGroup.ps1
Outdated
} | ||
|
||
if ($ReuseSystemDatabases -and $IsContained -eq $false) { | ||
Write-Message -Level Warning -Message "Reuse system databases is only applicable in contained availability groups" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would handle this the same way like the other test: Stop-Function
and return
.
reimplemented some paramter tests in a way that's consitent with code
Sorry, still not correct. Please have a look at the other parameter tests. |
#9352 has all the needed changes, so we should close this one. |
thank you all 🙏🏼 will merge for credit and fix all issues in next PR |
Type of Change
.\tests\manual.pester.ps1
)Purpose
SQL Server 2022 added support for contained availability groups.
Approach
The New-DbaAvailabilityGroup leverages the built-in ReuseSystemDatabses and IsContained parameters to facilitate both the initial build and a recreation with previously provisioned contained databases.
Commands to test
Work in progress
Learning
What is a contained availability group?
New-SQLAvailabilityGroup
SMOObject