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

Add DNS Caching Phase 1 #594

Merged
merged 10 commits into from
Jun 16, 2020
Merged

Conversation

karinazhou
Copy link
Member

@karinazhou karinazhou commented Jun 6, 2020

This PR adds support for the upcoming DNS Caching feature on the server-side. There are two phases for this feature and this is the first phase support.

When using TCP protocol, the SqlClient driver will record the DNS information such as IP address and port number in an in-memory cache if the server sends back the corresponding TDS token for this caching. The DNS information will stay in the cache during the application lifetime unless the server tells the driver to clean the cache.

In phase 1, the IP and port number are from the DNS resolution during pre-login handshake. If the DNS server fails to resolve the server name, the driver will connect with its cached IP and port if they existed. Otherwise, the connection will fail.

  • Add support for both .NET Framework and .NET Core.
  • This change requires the corresponding change in native SNI.
  • The caching behavior is only for TCP connections.
  • Need to fail the DNS Server to test this behavior.

image

@cheenamalhotra cheenamalhotra added this to the 2.0.0 milestone Jun 9, 2020
@Wraith2
Copy link
Contributor

Wraith2 commented Jun 9, 2020

I can't currently reply inline, you said.

#594 (comment)

Currently, there is no control of clearing all items in the cache.

That's a leak then. A cache is normally bounded, is this one? And more importantly if users can't clear it they will have to fall back to the nuclear option of killing the process to get rid of it and that's a fairly annoying thing to need to do.

As @cheenamalhotra said I think there should be an api for users to control this feature.

Can you point me at the part of the spec that describes this?

@karinazhou
Copy link
Member Author

karinazhou commented Jun 9, 2020

@Wraith2 There is no public spec for this since the server-side hasn't released this feature yet. From the current internal design docs, this cache is hidden from the client and used by the driver internally. From the behavior on the server-side, it will send back the IsSupported flag in the feature ext tds to tell the driver whether to remove the item from the cache or not. If the driver receives this flag, it will remove its corresponding item from the cache. But the cache itself will not be cleared up because it is shared by all the connections when the application is running.

@Wraith2
Copy link
Contributor

Wraith2 commented Jun 9, 2020

If you can't connect to the server because of an invalid cache entry you can't get an eviction notification so the cache is permanently poisoned for the process. There needs to be some eviction mechanism even if it isn't available to users.

@karinazhou
Copy link
Member Author

karinazhou commented Jun 10, 2020

@Wraith2 I understand your concern. Basically, we will always first use the information from the connection string to connect even though we have items in the cache. The server name will be resolved by DNS server as before. Only when the DNS resolution fails, we will try again with the IP in the cache.

Currently, the SQL Servers without DNS Caching feature won't have IsSupported flag in their feature ext token. This means that we won't put anything in the DNS cache for these servers. For other servers on Azure which may have this DNS Caching feature supported, we will cache the IPs for them as required or clean the IP in the cache as they want.

This PR is not the complete version for the entire DNS caching support implementation. There will be a continuing phase 2 for both server and driver side. We will cache the actual information sent from the server every time we reach the server endpoint. Of course, we will take your advice and try to see how we can clean up the whole cache if needed.

Removed temporary comments, added some comments for logic explanation, updated formatting and changed initial capacity for DNS Cache.
@Wraith2
Copy link
Contributor

Wraith2 commented Jun 10, 2020

I think it might be a good idea to call this a fallback dns cache. Typically caches are used to prevent the need for an operation by memorization of a result which in the case of this feature could cause reliability problems. What this feature is really doing is being a backup in case the operation fails, so we fall back to it to increase reliability. I understand that the name of the defines from the sql server team can't be changed but in this library we have free reign to use descriptive naming.

@karinazhou
Copy link
Member Author

@Wraith2 Sure. It makes sense to me. I will change the name accordingly.

Remove Azure-prefix from variable names and rename DNS cache
…CachingPhase1Whole

# Conflicts Resolved:
#	src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
@karinazhou karinazhou merged commit ead806e into dotnet:master Jun 16, 2020
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.

5 participants