Fix outdated information in node shutdown docs #19562
Conversation
In 2024, we made the decision to go back to setting the server.shutdown.connections.timeout value back to 0 (default) for Advanced clusters. This docs page was never updated to reflect that. See here for the record of this meeting: https://cockroachlabs.atlassian.net/wiki/spaces/MC/pages/3674636289/Re-Decision+connection+timeout+setting+rollout+for+CockroachDB+Cloud+clusters
✅ Deploy Preview for cockroachdb-interactivetutorials-docs canceled.
|
✅ Deploy Preview for cockroachdb-api-docs canceled.
|
✅ Netlify PreviewBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@taroface or @rmloveland this has been reviewed for technical accuracy. Could one of you help review it for grammar, clarity, style, etc? |
src/current/v23.2/node-shutdown.md
Outdated
|
|
||
| Client applications or application servers that connect to CockroachDB {{ site.data.products.advanced }} clusters should use connection pools that have a maximum lifetime that is shorter than the [`server.shutdown.connections.timeout`](#server-shutdown-connections-timeout) setting. | ||
| Client applications or application servers that connect to CockroachDB {{ site.data.products.advanced }} clusters may adjust the the [`server.shutdown.connections.timeout`](#server-shutdown-connections-timeout) setting and should make sure the connection pool maximum lifetime is shorter than that value, as per the guidance in the rest of this page. |
There was a problem hiding this comment.
This guidance seems counterintuitive to me, because the previous bullet says that the grace period isn't configurable on Advanced. Isn't that what server.shutdown.connections.timeout sets?
There was a problem hiding this comment.
"grace period" in this sentence is referring to the termination grace period in k8s: https://www.cockroachlabs.com/docs/stable/node-shutdown#termination-grace-period-on-kubernetes
the previous bullet says that the grace period isn't configurable on Advanced. Isn't that what server.shutdown.connections.timeout sets?
Before my edit, the sentence said "CockroachDB Advanced clusters have a server.shutdown.connections.timeout of 1800 seconds (30 minutes) and a termination grace period that is slightly longer." Although the "30 minutes" was incorrect, that older sentence hopefully makes it clear that these are two separate things.
I'm open to feedback on how to make sure this is still clear after my edit.
There was a problem hiding this comment.
I understand that the connection timeout and the grace period are separate things, but I just realized the source of my confusion: your new edit says a termination grace period that is slightly longer than 30 minutes, and this grace period is not configurable.. Should this actually be 30 seconds?
Also, can we state that the server.shutdown.connections.timeout default is 0 seconds, just to clarify that it is separate from the grace period? I'm happy to suggest edits but wanted to make sure I understood first.
There was a problem hiding this comment.
your new edit says "a termination grace period that is slightly longer than 30 minutes, and this grace period is not configurable." Should this actually be 30 seconds?
No, it is slightly longer than 30 minutes (just as it said before my edit). Could you say more about where 30 seconds came from? (If it's from the previous section, then let's clarify things there.)
Also, can we state that the server.shutdown.connections.timeout default is 0 seconds, just to clarify that it is separate from the grace period?
Sure, this makes sense. How's this:
Most of the guidance in this page is most relevant to manual deployments, although decommissioning and draining work the same way behind the scenes in a CockroachDB {{ site.data.products.advanced }} cluster. CockroachDB {{ site.data.products.advanced }} clusters are deployed with the
server.shutdown.connections.timeoutsetting at its default value of 0, and have a termination grace period that is slightly longer than 30 minutes. This grace period is not configurable.Client applications or application servers that connect to CockroachDB {{ site.data.products.advanced }} clusters may adjust the the
server.shutdown.connections.timeoutsetting. The connection pool maximum lifetime should be shorter thanserver.shutdown.connections.timeout, and the sum of all the shutdown-related timeouts should not exceed the termination grace period, as per the guidance in the rest of this page.
There was a problem hiding this comment.
The K8s section you linked me to says If undefined, Kubernetes sets terminationGracePeriodSeconds to 30 seconds. so I assumed that was also the case on Advanced. But I guess we have that set to 30 minutes?
I made suggestions to both bullets; LMK how they look and I can incorporate into the PR.
|
ack on the review request, it looks like Ryan beat me to it and he's way more familiar with the shutdown docs as well so i'll defer to his comments! |
src/current/v23.2/node-shutdown.md
Outdated
|
|
||
| Client applications or application servers that connect to CockroachDB {{ site.data.products.advanced }} clusters should use connection pools that have a maximum lifetime that is shorter than the [`server.shutdown.connections.timeout`](#server-shutdown-connections-timeout) setting. | ||
| Client applications or application servers that connect to CockroachDB {{ site.data.products.advanced }} clusters may adjust the the [`server.shutdown.connections.timeout`](#server-shutdown-connections-timeout) setting and should make sure the connection pool maximum lifetime is shorter than that value, as per the guidance in the rest of this page. |
There was a problem hiding this comment.
The K8s section you linked me to says If undefined, Kubernetes sets terminationGracePeriodSeconds to 30 seconds. so I assumed that was also the case on Advanced. But I guess we have that set to 30 minutes?
I made suggestions to both bullets; LMK how they look and I can incorporate into the PR.
@taroface the changes look good now. Is there something special I should do to incorporate them into the PR? |
taroface
force-pushed
the
fix-shutdown-docs
branch
from
ec6640b to
a8a7d7b
Compare
Thanks for the reminder. I went ahead and made the changes to all versions. Will enable auto-merge on this! |
In 2024, we made the decision to go back to setting the server.shutdown.connections.timeout value back to 0 (default) for Advanced clusters. This docs page was never updated to reflect that.
See here for the record of this meeting:
https://cockroachlabs.atlassian.net/wiki/spaces/MC/pages/3674636289/Re-Decision+connection+timeout+setting+rollout+for+CockroachDB+Cloud+clusters