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

Connect-DbaInstance - Use DAC with new switch DedicatedAdminConnection #7644

Merged
merged 4 commits into from
Jul 29, 2021

Conversation

andreasjordan
Copy link
Contributor

Type of Change

  • Bug fix (non-breaking change, fixes # )
  • New feature (non-breaking change, adds functionality, fixes Connect-DbaInstance - Support making DAC connection #6831 )
  • Breaking change (effects 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/sqlcollaborative/appveyor-lab ?
  • Unit test is included
  • Documentation
  • Build system

Just first ideas.

Problem: Non-pooled connections do not get closed when variable with Server SMO is removed or goes out of scope. But this is not related to DAC but a general problem we should address before thinking about merging this.

It currently only works with input type RegisteredServer, ConnectionString and String, not with Server or SqlConnection. Server could be easily handled with ConnectionContext.Copy(), just haven't coded it yet.

@potatoqualitee
Copy link
Member

sweet!

@andreasjordan
Copy link
Contributor Author

If there is already a DAC connection, this erro is raised: The specified network name is no longer available.

Connects using "ADMIN:" to create a dedicated admin connection (DAC).
If the instance is on a remote server, the remote access has to be enabled via "sp_configure 'remote admin connections', 1".
The parameter NonPooledConnection will be set to request a non-pooled connection.
The connection will not be closed if the variable holding the Server SMO is going ot of scope, so it is very important to call .ConnectionContext.Disconnect() to close the connection.
Copy link
Member

Choose a reason for hiding this comment

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

out of scope

Copy link
Member

Choose a reason for hiding this comment

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

Also, can we do this/detect idle in runspaces? Can we create a Disconnect-DbaInstance command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disconnect-DbaInstance is a good idea. But for that we would have to build a cache (like for TEPP) where Connect-DbaInstance saves (registers) all connections it opens. Then Disconnect-DbaInstance -SqlInstance XYZ could get all connections from cache and close them. Disconnect-DbaInstance -SqlInstance XYZ -Force could connect to the instance and get all connections based on the client name and kill them.

functions/Connect-DbaInstance.ps1 Outdated Show resolved Hide resolved
@potatoqualitee
Copy link
Member

Perhaps we can catch that -- if DedicatedAdminConnection and errmsg -match "The specified network name is no longer available." then errmsg = "Only one DAC connection is allowed etc"

@niphlod
Copy link
Contributor

niphlod commented Jul 29, 2021

error messages are translated .... either we force english on powershell or parsing the error message text is going to be hard

@andreasjordan
Copy link
Contributor Author

Added a general message on failure with DAC.

@potatoqualitee
Copy link
Member

good point, niph! i like the solution @andreasjordan !

@andreasjordan
Copy link
Contributor Author

If we want to build a "cache" to be able to implement Disconnect-DbaInstance - who can help?

@potatoqualitee
Copy link
Member

potatoqualitee commented Jul 29, 2021

you and me, probably ;) we can also use a good ol $script:connectionhash = @{ } like i do with other modules if our super advanced caching is an issue

$script:connectionhash = @{ } in dbatools.psm1

$script:connectionhhash[$($instance.InputObject)] = $server in Connect-DbaInstance

@andreasjordan
Copy link
Contributor Author

you and me, probably ;) we can also use a good ol $script:connectionhash = @{ } like i do with other modules if our super advanced caching is an issue

$script:connectionhash = @{ } in dbatools.psm1

$script:connectionhhash[$($instance.InputObject)] = $server in Connect-DbaInstance

Ok, I'll play with that and see what I can do...

@andreasjordan
Copy link
Contributor Author

I think $script:connectionhash is not a solution.

We have a general problem with the Server SMO or the way we work with them: It does not have a destructor we can use to disconnect. This is not a problem with pooled connections, there is even [Microsoft.Data.SqlClient.SqlConnection]::ClearAllPools() to clear all pools. It is a problem for non-pooled connections (and the dedicated admin connection as I think this should always be a non-pooled connection).

$server = Connect-DbaInstance -SqlInstance srv1 -NonPooledConnection creates a non-pooled connection. To close this connection, we need to issue $server.ConnectionContext.Disconnect().

So this works:

$server = Connect-DbaInstance -SqlInstance srv1 -NonPooledConnection
$null = Install-DbaFirstResponderKit -SqlInstance $server -OnlyScript sp_Blitz.sql  
$server.ConnectionContext.Disconnect()

The call to Connect-DbaInstance inside of Install-DbaFirstResponderKit just returns the passed in Server SMO in $server, so no new Server SMO is build and no other connection is opened.

This does not work so well:

$null = Install-DbaFirstResponderKit -SqlInstance srv1 -OnlyScript sp_Blitz.sql

The call to Connect-DbaInstance inside of Install-DbaFirstResponderKit creates a new Server SMO and opens the connection. But the Server SMO is not returned and so I don't have a chance to call .ConnectionContext.Disconnect(). And I don't see a way to find the object in my powershell session. And even as there is no variable anymore holding the Server SMO, the connection still lives.

But if we change Install-DbaFirstResponderKit to close the connection at the end - we have a problem with the first example.

We would have to check inside of Install-DbaFirstResponderKit, if the returned $server is the same object as we received and close only if the object was build inside of the command.

I think the basic problem is that a $server = $null does not do the disconnect. So we would need a custom destructor for our Server SMO that tests for non-pooled connection and in that case issues the disconnect. Is there a way to do that?

@potatoqualitee
Copy link
Member

since you are working with $Script: variable, you will need to do everything inside of the module. Build Disconnect-DbaInstance and before you disconnect, make it output everything in $script:script:connectionhash then you can see how you'd be able to work with those.

@potatoqualitee
Copy link
Member

we'll probably want a Get-DbaConnectedInstance too

@andreasjordan
Copy link
Contributor Author

The problem is that we don't get it to be in sync. The $Script: variable can hold a bunch on connections, but we don't know which of them are still bound to a variable in the user scope. And this would only be needed for non-pooled connections. And we only have four commands that request a non-pooled connection:

  • Get-DbaDbExtentDiff
  • Install-DbaFirstResponderKit
  • Install-DbaMaintenanceSolution
  • Write-DbaDbTableData

I would like to address this in a seperate pull request and just add the switch for DAC here.

@potatoqualitee
Copy link
Member

oh totally. i'll do a POC in another branch after i create tests for the compression commands

@andreasjordan
Copy link
Contributor Author

Just found out: It is not problem to close a non-pooled connection. It will be reopened if needed. So we could close the connection inside of the commands that request a non-pooled connection.
image

@andreasjordan andreasjordan marked this pull request as ready for review July 29, 2021 17:37
@potatoqualitee potatoqualitee merged commit 899563e into development Jul 29, 2021
@potatoqualitee potatoqualitee deleted the ConnectDbaInstance_DAC branch July 29, 2021 17:58
@potatoqualitee
Copy link
Member

fabulous 🤩 !

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.

Connect-DbaInstance - Support making DAC connection
3 participants