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

SqlServerDsc: Should parameter ServerName be of type qualifier Key, Require or Write? #319

Closed
johlju opened this issue Jan 18, 2017 · 5 comments · Fixed by #1503
Closed
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request.

Comments

@johlju
Copy link
Member

johlju commented Jan 18, 2017

Hello all!

For a while I have questioned myself if the parameter ServerName should be mandatory (Key or Required),
or that it instead should be a non-mandatory parameter (Write) with a default value of env:COMPUTERNAME.

The type qualifier, [Key], on a property indicates that this property will uniquely identify the resource instance. At least one [Key] property is required.
The [Required] qualifier indicates that the property is required (a value must be specified in any configuration script that uses this resource).
The [Write] qualifier indicates that this property is optional when using the custom resource in a configuration script.

For this discussion I will assume that "InstanceName" parameter is always Key.

Scenario 1
When SQL Server is installed on a single server the ServerName parameter could be left out of the configuration so that the default value of env:COMPUTERNAME could be used. So for that scenario the parameter could have the type qualifier Write.

Scenario 2
But what if SQL Server is installed in cluster, either as a resource in a failover cluster or as AlwaysOn Availability Groups? Let's say we set up AlwaysOn with two Availability Groups, and we want to create one database in each Availability Group. This configuration will be run on the node acting as the primary replica, let's call it node 1. A second replica is already setup at this point, let's call it node 2.
Each availability group listener has a DNS-record pointing to its IP address; AG1 and AG2.

The databases was created on node 1 using the following configuration (just a mock example).

Configuration Example
{
    node node1
    {
        SqlDatabase 'Database1-In-AG1'
        {
            Name = 'Database1'
            InstanceName = 'TESTDSC'
        }

        SqlDatabase 'Database2-In-AG2'
        {
            Name = 'Database2'
            InstanceName = 'TESTDSC'
        }
    }
}

The above example would work because the parameter ServerName is default set to env:COMPUTERNAME and both availability groups had node 1 as the primary replica.

But now let say that the first availability group AG1 is primary on node 1, and let's say that the second availability group AG2 is primary on node 2. In this scenario the above example would fail because node 1 is no longer primary replica for both availability groups.

To solve that we need to add the parameter ServerName to the configuration, to tell which DNS-record each availability group listens on.

Configuration Example
{
    node node1
    {
        SqlDatabase 'Database1-In-AG1'
        {
            Name = 'Database1'
            ServerName = 'AG1'
            InstanceName = 'TESTDSC'
        }

        SqlDatabase 'Database2-In-AG2'
        {
            Name = 'Database2'
            ServerName = 'AG2'
            InstanceName = 'TESTDSC'
        }
    }
}

But the parameter ServerName still does not need to have the type qualifier Required or Key. It is enough for it to have type qualifier Write with a default value of env:COMPUTERNAME.

I have not thought of any scenario yet that needs to have the parameter ServerName as type qualifier Key.

Can anyone else see a scenario where it would be required?

Thanks,
Johan

@randomnote1
Copy link
Contributor

randomnote1 commented Oct 10, 2017

After thinking about this extensively, I believe the "ServerName" parameter can be dropped completely.

To connect to the SQL instance (clustered or standalone), the instance name can be looked up in the registry. Doing this will allow Connect-SQL to dynamically detect the hostname. It is impossible to have more than one instance per node with the same instance name (I tested to double-check).

When connecting to or managing a database that is a member of an Availability Group, it is important to remember that use of a listener is not required. In fact, there are many cases where AGs are used as a 1:1 replacement for database mirroring. Because of this, I do not believe that supplying the hostname for an AG listener makes sense.

However, it might be a good idea for the Database resources to determine if the database is a member of an AG and make a decision about what to do based on that information. Options could include:

  • Connect to the primary replica and get/set/test the settings
  • Silently stop processing the resource
  • Look for a parameter telling the resource what to do. Maybe ApplyOnlyWhenPrimary = $true will silently stop if the database is not currently on the primary replica and ApplyOnlyWhenPrimary = $false will always connect to the AG replica and update the database settings.

@nabrond
Copy link
Contributor

nabrond commented Oct 15, 2017

I agree completely that these parameters must be standardized across all resources. However, I disagree with @randomnote1 on the topic of removing the parameter pointing at the appropriate host name for the instance. I think automatically determining this information at runtime will prove to be a costly operation that will undoubtedly introduce performance and maintenance issues down the road.

I do like the idea of adding an additional common parameter to only test / set when on the primary node/replica of a cluster or AG. It is interesting when reviewing SQL Server logs and seeing two different nodes making max memory changes! We could implement that as a common function Test-ActiveNode or Get-ActiveNode that each resource could call upfront to determine whether to continue.

@randomnote1
Copy link
Contributor

I need some parameter name help. I'm trying to determine a concise name that tells the end user that the resource will or will not process/apply when the current node is not actively hosting the SQL Instance. Here are some of my ideas so far:

  • ProcessWhenNotActiveNode
  • ProcessOnPassiveNode - I don't like this one because I believe it makes it sound like it is offloading processing to a secondary node.
  • ProcessOnlyOnActiveNode - This would keep the current behavior of applying the resource no matter what, but would override the behavior when true.

I'm leaning towards using "Process" because it think it describes the Get/Set/Test instructions where I think "Apply" indicates the "Set" instructions.

@johlju johlju changed the title Discussion: Should parameter SQLServer be of type qualifier Key, Require or Write? Discussion: Should parameter ServerName be of type qualifier Key, Require or Write? Jan 10, 2018
@stale
Copy link

stale bot commented Jun 6, 2018

This issue has been automatically marked as needs more information because it has not had activity from the community in the last 30 days. It will be closed if no further activity occurs within 10 days. If the issue is label with any of the work labels (e.g bug, enhancement, documentation, or tests) the issue will not auto-close.

@stale stale bot added the needs more information The issue needs more information from the author or the community. label Jun 6, 2018
@johlju johlju added enhancement The issue is an enhancement request. help wanted The issue is up for grabs for anyone in the community. and removed discussion The issue is a discussion. labels Jun 6, 2018
@stale stale bot removed the needs more information The issue needs more information from the author or the community. label Jun 6, 2018
@johlju johlju added the breaking change When used on an issue, the issue has been determined to be a breaking change. label Jun 6, 2018
@Rob-S
Copy link
Contributor

Rob-S commented Mar 30, 2020

Since which node is active / primary vs. passive / secondary is dependent on the context of a specific availability group (possibly determined indirectly by a DatabaseName property), I'm leaning toward the terminology that SQL Server itself uses, namely "availability replica", "primary replica", "secondary replica" (Always On Availability Groups). I agree with @randomnote1 that something like ProcessOnPrimaryReplica makes it sound like the processing will occur on the primary node when the configuration is being applied to the secondary, which is undesirable. The requirement could be accomplished with a single property maybe:

    WhenReplicaIs = 'any'
    WhenReplicaIs = 'primary'
    WhenReplicaIs = 'secondary'

WhenReplicaIs = 'primary' might be a better default than the current behavior (WhenReplicaIs = 'any'), since many alter-type functions fail when attempted on a secondary replica. The ProcessOn properties could also be used to determine where the processing actually occurs, with or without the WhenReplicaIs property as in:

    WhenReplicaIs = 'any'
    ProcessOnPrimaryReplica = $true

to perform the processing on the primary replica when the configuration is being applied to any replica, or for things like copy-only backups, which can be done on a secondary:

    WhenReplicaIs = 'primary'
    ProcessOnSecondaryReplica = $true

to process on the secondary replica only when the configuration is being applied to the primary replica.

@johlju johlju changed the title Discussion: Should parameter ServerName be of type qualifier Key, Require or Write? SqlServerDsc: Should parameter ServerName be of type qualifier Key, Require or Write? Apr 15, 2020
@johlju johlju added in progress The issue is being actively worked on by someone. and removed help wanted The issue is up for grabs for anyone in the community. labels Apr 16, 2020
johlju added a commit that referenced this issue Apr 26, 2020
#1503)

- SqlAgentAlert
  - The parameter `ServerName` now throws when passing an empty string or
    null value (part of issue #319).
- SqlAgentFailsafe
  - The parameter `ServerName` now throws when passing an empty string or
    null value (part of issue #319).
- SqlAgentOperator
  - The parameter `ServerName` now throws when passing an empty string or
    null value (part of issue #319).
- SqlAlias
  - BREAKING CHANGE: The parameter `ServerName` is now non-mandatory to
    prevent ping-pong behavior ([issue #1502](#1502)).
    The `ServerName` is not returned as an empty string when the protocol is
    Named Pipes.
- SqlAlwaysOnService
  - BREAKING CHANGE: The parameter `ServerName` is now non-mandatory and
    defaults to `$env:COMPUTERNAME` (issue #319).
  - Normalize parameter descriptive text for default values.
- SqlDatabase
  - BREAKING CHANGE: The parameter `ServerName` is now non-mandatory and
    defaults to `$env:COMPUTERNAME` (issue #319).
  - Normalize parameter descriptive text for default values.
- SqlDatabaseDefaultLocation
  - BREAKING CHANGE: The parameter `ServerName` is now non-mandatory and
    defaults to `$env:COMPUTERNAME` (issue #319).
  - Normalize parameter descriptive text for default values.
- SqlDatabasePermission
  - BREAKING CHANGE: The parameter `ServerName` is now non-mandatory and
    defaults to `$env:COMPUTERNAME` (issue #319).
  - Normalize parameter descriptive text for default values.
- SqlDatabaseRecoveryModel
  - BREAKING CHANGE: The parameter `ServerName` is now non-mandatory and
    defaults to `$env:COMPUTERNAME` (issue #319).
  - Normalize parameter descriptive text for default values.
- SqlDatabaseRole
  - BREAKING CHANGE: The parameter `ServerName` is now non-mandatory and
    defaults to `$env:COMPUTERNAME` (issue #319).
  - Normalize parameter descriptive text for default values.
- SqlDatabaseUser
  - BREAKING CHANGE: The parameter `ServerName` is now non-mandatory and
    defaults to `$env:COMPUTERNAME` (issue #319).
  - Normalize parameter descriptive text for default values.
- SqlRs
  - Fix typo in the schema parameter `SuppressRestart` description
    and in the parameter description in the `README.md`.
- SqlServerConfiguration
  - BREAKING CHANGE: The parameter `ServerName` is now non-mandatory and
    defaults to `$env:COMPUTERNAME` (issue #319).
  - Normalize parameter descriptive text for default values.
- SqlServerDatabaseMail
  - Normalize parameter descriptive text for default values.
  - The parameter `ServerName` now throws when passing an empty string or
    null value (part of issue #319).
- SqlServerEndpoint
  - Normalize parameter descriptive text for default values.
  - The parameter `ServerName` now throws when passing an empty string or
    null value (part of issue #319).
- SqlServerEndpointPermission
  - BREAKING CHANGE: The parameter `ServerName` is now non-mandatory and
    defaults to `$env:COMPUTERNAME` (issue #319).
  - Normalize parameter descriptive text for default values.
- SqlServerEndpointState
  - The parameter `ServerName` now throws when passing an empty string or
    null value (part of issue #319).
- SqlServerLogin
  - BREAKING CHANGE: The parameter `ServerName` is now non-mandatory and
    defaults to `$env:COMPUTERNAME` (issue #319).
  - Normalize parameter descriptive text for default values.
- SqlServerPermission
  - The parameter `ServerName` now throws when passing an empty string or
    null value (part of issue #319).
- SqlServerRole
  - BREAKING CHANGE: The parameter `ServerName` is now non-mandatory and
    defaults to `$env:COMPUTERNAME` (issue #319).
  - Normalize parameter descriptive text for default values.
- SqlServiceAccount
  - BREAKING CHANGE: The parameter `ServerName` is now non-mandatory and
    defaults to `$env:COMPUTERNAME` (issue #319).
  - Normalize parameter descriptive text for default values.
- SqlSetup
  - Update integration tests to correctly detect sysadmins because of changes
    to the build worker.
@johlju johlju removed the in progress The issue is being actively worked on by someone. label Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change When used on an issue, the issue has been determined to be a breaking change. enhancement The issue is an enhancement request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants