Skip to content

Contained a gs#249

Merged
lowlydba merged 46 commits intolowlydba:mainfrom
DorBreger:containedAGs
Apr 6, 2025
Merged

Contained a gs#249
lowlydba merged 46 commits intolowlydba:mainfrom
DorBreger:containedAGs

Conversation

@DorBreger
Copy link
Copy Markdown
Contributor

@DorBreger DorBreger commented May 22, 2024

Description

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue) - Fixes #
  • New feature (non-breaking change which adds functionality)

Checklist:

@DorBreger
Copy link
Copy Markdown
Contributor Author

WIP. needs the latest dbatools.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 22, 2024

Docs Build 📝

Thank you for contribution!✨

The docs for this PR have been published here:
https://lowlydba.github.io/lowlydba.sqlserver/pr/249

You can compare to the docs for the main branch here:
https://lowlydba.github.io/lowlydba.sqlserver/branch/main

The docsite for this PR is also available for download as an artifact from this run:
https://github.com/lowlydba/lowlydba.sqlserver/actions/runs/9518565030

File changes:

Click to see the diff comparison.

NOTE: only file modifications are shown here. New and deleted files are excluded.
See the file list and check the published docs to see those files.

diff --git a/home/runner/work/lowlydba.sqlserver/lowlydba.sqlserver/docsbuild/base/collections/lowlydba/sqlserver/availability_group_module.html b/home/runner/work/lowlydba.sqlserver/lowlydba.sqlserver/docsbuild/head/collections/lowlydba/sqlserver/availability_group_module.html
index 0fe25ed..59fd612 100644
--- a/home/runner/work/lowlydba.sqlserver/lowlydba.sqlserver/docsbuild/base/collections/lowlydba/sqlserver/availability_group_module.html
+++ b/home/runner/work/lowlydba.sqlserver/lowlydba.sqlserver/docsbuild/head/collections/lowlydba/sqlserver/availability_group_module.html
@@ -256,6 +256,18 @@ see <a class="reference internal" href="#ansible-collections-lowlydba-sqlserver-
 </div></td>
 </tr>
 <tr class="row-odd"><td><div class="ansible-option-cell">
+<div class="ansibleOptionAnchor" id="parameter-contained_availability_group"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-contained-availability-group"><strong>contained_availability_group</strong></p>
+<a class="ansibleOptionLink" href="#parameter-contained_availability_group" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
+</div></td>
+<td><div class="ansible-option-cell"><p>Indicates whether the availability group is Contained. Requires DBATools &gt;= 2.1.15</p>
+<p class="ansible-option-line"><strong class="ansible-option-choices">Choices:</strong></p>
+<ul class="simple">
+<li><p><code class="ansible-option-choices-entry docutils literal notranslate"><span class="pre">false</span></code></p></li>
+<li><p><code class="ansible-option-choices-entry docutils literal notranslate"><span class="pre">true</span></code></p></li>
+</ul>
+</div></td>
+</tr>
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-database"></div>
 <div class="ansibleOptionAnchor" id="parameter-database_name"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-database-name"><span id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-database"></span><strong>database</strong></p>
 <a class="ansibleOptionLink" href="#parameter-database" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-aliases">aliases: database_name</span></p>
@@ -264,7 +276,7 @@ see <a class="reference internal" href="#ansible-collections-lowlydba-sqlserver-
 <td><div class="ansible-option-cell"><p>Name of the database to create the Availability Group for.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-database_health_trigger"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-database-health-trigger"><strong>database_health_trigger</strong></p>
 <a class="ansibleOptionLink" href="#parameter-database_health_trigger" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
 </div></td>
@@ -276,7 +288,7 @@ see <a class="reference internal" href="#ansible-collections-lowlydba-sqlserver-
 </ul>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-dtc_support_enabled"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-dtc-support-enabled"><strong>dtc_support_enabled</strong></p>
 <a class="ansibleOptionLink" href="#parameter-dtc_support_enabled" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
 </div></td>
@@ -288,7 +300,7 @@ see <a class="reference internal" href="#ansible-collections-lowlydba-sqlserver-
 </ul>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-failover_mode"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-failover-mode"><strong>failover_mode</strong></p>
 <a class="ansibleOptionLink" href="#parameter-failover_mode" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
@@ -300,7 +312,7 @@ see <a class="reference internal" href="#ansible-collections-lowlydba-sqlserver-
 </ul>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-failure_condition_level"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-failure-condition-level"><strong>failure_condition_level</strong></p>
 <a class="ansibleOptionLink" href="#parameter-failure_condition_level" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
@@ -315,7 +327,7 @@ see <a class="reference internal" href="#ansible-collections-lowlydba-sqlserver-
 </ul>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-force"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-force"><strong>force</strong></p>
 <a class="ansibleOptionLink" href="#parameter-force" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
 </div></td>
@@ -327,7 +339,7 @@ see <a class="reference internal" href="#ansible-collections-lowlydba-sqlserver-
 </ul>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-healthcheck_timeout"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-healthcheck-timeout"><strong>healthcheck_timeout</strong></p>
 <a class="ansibleOptionLink" href="#parameter-healthcheck_timeout" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">integer</span></p>
 </div></td>
@@ -335,7 +347,7 @@ see <a class="reference internal" href="#ansible-collections-lowlydba-sqlserver-
 <p>Changes that are made to the timeout settings are effective immediately and do not require a restart of the SQL Server resource.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-is_distributed_ag"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-is-distributed-ag"><strong>is_distributed_ag</strong></p>
 <a class="ansibleOptionLink" href="#parameter-is_distributed_ag" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
 </div></td>
@@ -347,7 +359,7 @@ see <a class="reference internal" href="#ansible-collections-lowlydba-sqlserver-
 </ul>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-seeding_mode"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-seeding-mode"><strong>seeding_mode</strong></p>
 <a class="ansibleOptionLink" href="#parameter-seeding_mode" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
@@ -359,56 +371,56 @@ see <a class="reference internal" href="#ansible-collections-lowlydba-sqlserver-
 </ul>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-shared_path"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-shared-path"><strong>shared_path</strong></p>
 <a class="ansibleOptionLink" href="#parameter-shared_path" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>The network share where the backups will be backed up and restored from.</p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-sql_instance"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-sql-instance"><strong>sql_instance</strong></p>
 <a class="ansibleOptionLink" href="#parameter-sql_instance" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span> / <span class="ansible-option-required">required</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>The SQL Server instance to modify.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-sql_instance_secondary"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-sql-instance-secondary"><strong>sql_instance_secondary</strong></p>
 <a class="ansibleOptionLink" href="#parameter-sql_instance_secondary" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>The secondary SQL Server instance for the new Availability Group.</p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-sql_password"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-sql-password"><strong>sql_password</strong></p>
 <a class="ansibleOptionLink" href="#parameter-sql_password" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>Password for SQL Authentication.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-sql_password_secondary"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-sql-password-secondary"><strong>sql_password_secondary</strong></p>
 <a class="ansibleOptionLink" href="#parameter-sql_password_secondary" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>Password for SQL Authentication for the secondary replica.</p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-sql_username"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-sql-username"><strong>sql_username</strong></p>
 <a class="ansibleOptionLink" href="#parameter-sql_username" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>Username for SQL Authentication.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-sql_username_secondary"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-sql-username-secondary"><strong>sql_username_secondary</strong></p>
 <a class="ansibleOptionLink" href="#parameter-sql_username_secondary" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>Username for SQL Authentication for the secondary replica.</p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-state"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-state"><strong>state</strong></p>
 <a class="ansibleOptionLink" href="#parameter-state" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
@@ -420,7 +432,7 @@ see <a class="reference internal" href="#ansible-collections-lowlydba-sqlserver-
 </ul>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-use_last_backup"></div><p class="ansible-option-title" id="ansible-collections-lowlydba-sqlserver-availability-group-module-parameter-use-last-backup"><strong>use_last_backup</strong></p>
 <a class="ansibleOptionLink" href="#parameter-use_last_backup" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
 </div></td>

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 22, 2024

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.37%. Comparing base (d3d84e6) to head (5d3289c).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #249      +/-   ##
==========================================
- Coverage   88.83%   85.37%   -3.47%     
==========================================
  Files          70      107      +37     
  Lines        2598     2954     +356     
  Branches       39       78      +39     
==========================================
+ Hits         2308     2522     +214     
- Misses        275      402     +127     
- Partials       15       30      +15     
Flag Coverage Δ
integration 40.83% <ø> (?)
sanity 100.00% <ø> (?)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@DorBreger
Copy link
Copy Markdown
Contributor Author

Alright @lowlydba I think I'm ready for you to have a look at the PR

use_last_backup: false
healthcheck_timeout: 15000
basic_availability_group: false
contained_availability_group: false
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a test where this is true?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New test in tests/inegration? I added some tasks in the win_availability_group that run when: contained_availability_group And test functionality.

Comment thread plugins/modules/availability_group.py Outdated
Copy link
Copy Markdown
Owner

@lowlydba lowlydba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks great overall. The suggested edit should clear up the failing sanity check, and if you can add a test that covers actually using a contained AG, it will be good to go!

@lowlydba
Copy link
Copy Markdown
Owner

I just updated the repo permissions so you should be able to trigger the CI tests with your own commits now as well.

Co-authored-by: John McCall <john@lowlydba.com>
@DorBreger
Copy link
Copy Markdown
Contributor Author

Hi! Just in case you missed it, what kind of tests did you mean?

@lowlydba
Copy link
Copy Markdown
Owner

lowlydba commented Jun 6, 2024

Hi! Just in case you missed it, what kind of tests did you mean?

Right below the comment thread here, you'll see a series of workflows that run integration and unit tests for the collection. Right now the "CI / Sanity" test is failing for some minor issues - all of the workflows will need to pass to make this mergeable.

@DorBreger
Copy link
Copy Markdown
Contributor Author

I still don't really understand how to add tests that cover contained AGs, I edited tests/integration/ and added an option to change contained_availability_group, but where can I create a test where it is set to true?

@DorBreger
Copy link
Copy Markdown
Contributor Author

I still don't really understand how to add tests that cover contained AGs, I edited tests/integration/ and added an option to change contained_availability_group, but where can I create a test where it is set to true?

Because what I would really like to do is to run the win_availability_group test with contained_availability_group: true and I was wondering if that's possible without creating a new test.

@lowlydba
Copy link
Copy Markdown
Owner

lowlydba commented Jun 8, 2024

I still don't really understand how to add tests that cover contained AGs, I edited tests/integration/ and added an option to change contained_availability_group, but where can I create a test where it is set to true?

Because what I would really like to do is to run the win_availability_group test with contained_availability_group: true and I was wondering if that's possible without creating a new test.

Yeah, the issue is right now its always set to false, so it won't get tested. You'd want to add a new test like:

- name: Create contained availability group
   lowlydba.sqlserver.availability_group:
      ag_name: "contained_AG"
      contained_availability_group: true
   register: result
- assert:
   that:
      - result.data.ComputerName != None
      - result.data.InstanceName != None
      - result.data.SqlInstance != None
      - result.data.AvailabilityGroup == "contained_AG"
      - result.data.ClusterType == "{{ cluster_type }}"
      - result.data.DtcSupportEnabled is false
      - result.data.AvailabilityReplicas != None
      - result is changed

The when conditionals now won't get use as written, so those can probably be removed.

@DorBreger
Copy link
Copy Markdown
Contributor Author

Seems like adding a test form contained AGs didn't help the codecov test? Would really like your input on that @lowlydba. Thank you!

@DorBreger
Copy link
Copy Markdown
Contributor Author

Also, I noticed I don't handle the case where an availability group that is not contained exists but the module specifies it should be contained. It is currently not possible to have an availability group be converted to a contained availability group, should I just error out in this case? and if force is specified only then drop it and recreate?

@DorBreger
Copy link
Copy Markdown
Contributor Author

@lowlydba I am completely baffled as to why this does not work. The error seems to be Configuring login failed: The network path was not found When pointing login to the listener, as should be done with contained availability groups, but I am unable to recreate this in my own setup. It works on mine.

@DorBreger
Copy link
Copy Markdown
Contributor Author

What active directory environment do the tests run it? Could it be a DC replication lag issue?

@lowlydba
Copy link
Copy Markdown
Owner

lowlydba commented Nov 2, 2024

What active directory environment do the tests run it? Could it be a DC replication lag issue?

You can check the GitHub test workflow for info, but there isn't anything beyond a local AD since we're using a single test runner.

I still don't have a ton of time to dedicate to this project right now, so I apologize that I can't be of more help at this time.

@DorBreger
Copy link
Copy Markdown
Contributor Author

Hi @lowlydba was wondering if you take a look at this I'm unable to replicate this issue locally

@lowlydba
Copy link
Copy Markdown
Owner

Hi @lowlydba was wondering if you take a look at this I'm unable to replicate this issue locally

I don't have anything setup locally right now for AG testing, so I am not sure honestly. I've also never personally used contained AG before (and don't use any of this collection at my current job).

I can take a look when I find some free time, but no promises.

@lowlydba
Copy link
Copy Markdown
Owner

@DorBreger In an effort to continue to be able to support this collection for users, I've introduced GitHub Sponsors for this project (see announcement). This feature request would be a "small feature" in that context - if you have any further questions please reply in that discussion thread and we can discuss.

Thanks!

@DorBreger
Copy link
Copy Markdown
Contributor Author

Looking at the output of grp1 it seems like ANY connection to ANY listener created in the context of those tests will fail with an error message of "the network path was not found". Must be the creation of the AGs with cluster_type: None (Because then who would create the listener CNO?) But I am not exactly sure as I am not familiar at all with any AG stack besides windows+WSFC+AD. I am removing any tests trying to connect to any listener and pushing One last time.

@DorBreger
Copy link
Copy Markdown
Contributor Author

This is the best this PR will get I'm afraid. Can't really say anything about the codecov and the sp_whoisactive tests failing. Feel free to merge this

@DorBreger
Copy link
Copy Markdown
Contributor Author

@lowlydba would love to get your response when you have the time :)

@lowlydba
Copy link
Copy Markdown
Owner

lowlydba commented Apr 2, 2025

The codecov is for sure blocking, but the results do look weird for this PR. I am re-running the tests to see what a fresh run's coverage comes back as.

@lowlydba
Copy link
Copy Markdown
Owner

lowlydba commented Apr 2, 2025

@DorBreger I think the codecov is off since its still comparing to the very very old commit (844484b) which I am guessing its getting from your repo fork? Can you try updating the fork and/or rebasing your branch to make sure it has the right head commit to compare to?

85.37% (-14.63%) compared to 844484b

@DorBreger
Copy link
Copy Markdown
Contributor Author

@lowlydba just did, sorry for the delay. I have been busy

@lowlydba
Copy link
Copy Markdown
Owner

lowlydba commented Apr 6, 2025

Thanks for trying that! It still looks off, it seems like it may have silently broken at some point on the default branch. I'm going to keep looking into this.

@DorBreger
Copy link
Copy Markdown
Contributor Author

Seems like all checks are successful @lowlydba

@lowlydba lowlydba merged commit ba96b6b into lowlydba:main Apr 6, 2025
17 checks passed
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.

3 participants