search

lowlydba / lowlydba.sqlserver

:spoon: A cross-platform Ansible collection using PowerShell to configure and maintain SQL Server.
https://galaxy.ansible.com/ui/repo/published/lowlydba/sqlserver
GNU General Public License v3.0
19 stars 12 forks source link

Contained a gs #249

Open DorBreger opened 3 months ago

DorBreger commented 3 months ago

Description

How Has This Been Tested?

Types of changes

Checklist:

DorBreger commented 3 months ago

WIP. needs the latest dbatools.

github-actions[bot] commented 3 months ago

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 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
+

contained_availability_group

+

boolean

+
+

Indicates whether the availability group is Contained. Requires DBATools >= 2.1.15

+

Choices:

+
    +

  • false

    • +

  • true

    • +
+
+ +

database

aliases: database_name

@@ -264,7 +276,7 @@ see

Name of the database to create the Availability Group for.

-
+

database_health_trigger

boolean

@@ -276,7 +288,7 @@ see
+

dtc_support_enabled

boolean

@@ -288,7 +300,7 @@ see
+

failover_mode

string

@@ -300,7 +312,7 @@ see
+

failure_condition_level

string

@@ -315,7 +327,7 @@ see
+

force

boolean

@@ -327,7 +339,7 @@ see
+

healthcheck_timeout

integer

@@ -335,7 +347,7 @@ see
+

is_distributed_ag

boolean

@@ -347,7 +359,7 @@ see
+

seeding_mode

string

@@ -359,56 +371,56 @@ see
+

shared_path

string

The network share where the backups will be backed up and restored from.

-
+

sql_instance

string / required

The SQL Server instance to modify.

-
+

sql_instance_secondary

string

The secondary SQL Server instance for the new Availability Group.

-
+

sql_password

string

Password for SQL Authentication.

-
+

sql_password_secondary

string

Password for SQL Authentication for the secondary replica.

-
+

sql_username

string

Username for SQL Authentication.

-
+

sql_username_secondary

string

Username for SQL Authentication for the secondary replica.

-
+

state

string

@@ -420,7 +432,7 @@ see
+

use_last_backup

boolean

```
codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (655394d) to head (2ede116).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #249 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 33 33 Lines 106 106 ========================================= Hits 106 106 ``` | [Flag](https://app.codecov.io/gh/lowlydba/lowlydba.sqlserver/pull/249/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=John+McCall) | Coverage Δ | | |---|---|---| | [sanity](https://app.codecov.io/gh/lowlydba/lowlydba.sqlserver/pull/249/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=John+McCall) | `100.00% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=John+McCall#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

DorBreger commented 3 months ago

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

lowlydba commented 3 months ago

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

DorBreger commented 2 months ago

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

lowlydba commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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?

lowlydba commented 2 months ago

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?

It should probably error out in both cases, using force to drop and re-create feels a bit too risky as behavior in general.

lowlydba commented 2 months ago

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

The codecov tests appear to be passing (they just look for code coverage) but the actual tests are failing.

I am not sure why the original availability group test is failing now though:

TASK [win_availability_group : Create availability group] ** ... The full traceback is: Database "ag-test-db" might contain bulk logged changes that have not been backed up. Take a log backup on the principal database or primary database. Then restore this backup either on the mirror database to enable database mirroring or on every secondary database to enable you to join it to the availability group.

DorBreger commented 1 month ago

I can't seem to figure out why the test is failing? doesn't seem to have anything to do with my changes either. I also have a hard time replicating this bug on my local setup.

lowlydba commented 1 month ago

I can't seem to figure out why the test is failing? doesn't seem to have anything to do with my changes either. I also have a hard time replicating this bug on my local setup.

It may be helpful to try having the new "Create Contained availability group" test use all its own variables (i.e. not the same database as the AG test above it) to rule out any issues from re-using parts of the tests earlier in the file. This will help narrow down what the issue is. I think using a separate database may resolve the backup warnings you're seeing.

For the dlevel tests, I am pushing a fix for those via https://github.com/lowlydba/lowlydba.sqlserver/pull/257/files

DorBreger commented 3 weeks ago

I can't seem to figure out why the test is failing? doesn't seem to have anything to do with my changes either. I also have a hard time replicating this bug on my local setup.

It may be helpful to try having the new "Create Contained availability group" test use all its own variables (i.e. not the same database as the AG test above it) to rule out any issues from re-using parts of the tests earlier in the file. This will help narrow down what the issue is. I think using a separate database may resolve the backup warnings you're seeing.

For the dlevel tests, I am pushing a fix for those via https://github.com/lowlydba/lowlydba.sqlserver/pull/257/files

Sounds good. How would you like me to implement this syntactically in the same file as the regular tests?

lowlydba commented 1 week ago

Sounds good. How would you like me to implement this syntactically in the same file as the regular tests?

I think just using the inputs needed inline for each assertion is fine, no need to declare a ton of them at the top like the current ones.