Skip to content
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

Merged
merged 14 commits into from
May 16, 2024

Conversation

serenefiresiren
Copy link
Contributor

@serenefiresiren serenefiresiren commented May 11, 2024

Type of Change

  • Bug fix (non-breaking change, fixes # )
  • New feature (non-breaking change, adds functionality, fixes #8713 )
  • Breaking change (affects multiple commands or functionality, fixes # )
  • Ran manual Pester test and has passed (.\tests\manual.pester.ps1)
  • Adding code coverage to existing functionality
  • Pester test is included
  • If new file reference added for test, has is been added to github.com/dataplat/appveyor-lab ?
  • Unit test is included
  • Documentation
  • Build system

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

Copy link
Contributor

@andreasjordan andreasjordan left a 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 {
Copy link
Contributor

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.

.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
Copy link
Contributor

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.

}

if ($ReuseSystemDatabases -and $IsContained -eq $false) {
Write-Message -Level Warning -Message "Reuse system databases is only applicable in contained availability groups test "
Copy link
Contributor

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.

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.
Copy link
Contributor

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."

.EXAMPLE
PS C:\> New-DbaAvailabilityGroup -Primary sql2022n01 -Secondary sql2022n02 -Name AgContained -IsContained

Creates a contained availability group named AgContained on sql2022
Copy link
Contributor

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".

@@ -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
Copy link
Contributor

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'
Copy link
Contributor

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.

@serenefiresiren
Copy link
Contributor Author

Thanks for the feedback! I'll get these fixed Monday.

@potatoqualitee
Copy link
Member

Thank you both 🙏🏼 Will fix encoding issue once changes are complete, just prior to merge.

@DorBreger
Copy link
Contributor

serenefiresiren#2

Copy link
Contributor

@andreasjordan andreasjordan left a 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"
Copy link
Contributor

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.

}

if ($ReuseSystemDatabases -and $IsContained -eq $false) {
Write-Message -Level Warning -Message "Reuse system databases is only applicable in contained availability groups"
Copy link
Contributor

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.

@andreasjordan
Copy link
Contributor

Sorry, still not correct. Please have a look at the other parameter tests.

@andreasjordan
Copy link
Contributor

andreasjordan commented May 15, 2024

#9352 has all the needed changes, so we should close this one.

@potatoqualitee
Copy link
Member

thank you all 🙏🏼 will merge for credit and fix all issues in next PR

@potatoqualitee potatoqualitee merged commit b51ae6d into dataplat:development May 16, 2024
2 of 3 checks passed
@serenefiresiren serenefiresiren deleted the NewContainedAGs branch May 16, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants