Add support to stop TiDB gracefully #2810

weekface · 2020-06-24T13:21:52Z

What problem does this PR solve?

This pr add terminationGracePeriodSeconds and lifecycle to TidbCluster spec.tidb attribute.

User can use these two attributes to stop TiDB server gracefully:

set spec.tidb.terminationGracePeriodSeconds to a proper value, for exmaple: 600 (10 minutes).
set spec.tidb.lifecycle to: kill -QUIT 1;

# IT IS NOT SUITABLE FOR PRODUCTION USE.
# This YAML describes a basic TiDB cluster with minimum resource requirements,
# which should be able to run in any Kubernetes cluster with storage support.
apiVersion: pingcap.com/v1alpha1
kind: TidbCluster
metadata:
  name: basic
spec:
  version: v4.0.0
  timezone: UTC
  pvReclaimPolicy: Delete
  discovery: {}
  pd:
    baseImage: pingcap/pd
    replicas: 1
    # if storageClassName is not set, the default Storage Class of the Kubernetes cluster will be used
    # storageClassName: local-storage
    requests:
      storage: "1Gi"
    config: {}
  tikv:
    baseImage: pingcap/tikv
    replicas: 1
    # if storageClassName is not set, the default Storage Class of the Kubernetes cluster will be used
    # storageClassName: local-storage
    requests:
      storage: "1Gi"
    config: {}
  tidb:
    baseImage: pingcap/tidb
    replicas: 1
    service:
      type: ClusterIP
    config: {}
    terminationGracePeriodSeconds: 600
    lifecycle:
      preStop:
        exec:
          command: ["kill", "-QUIT", "1"]

The TiDB server will go graceful shutting down to wait for all clients to close the connection until k8s force kill the TiDB container(10 minutes later).

fixes: #2597

What is changed and how does it work?

Check List

Tests

Unit test
E2E test
Stability test
Manual test (add detailed scripts or steps below)
No code

Code changes

Has Go code change
Has CI related scripts change
Has Terraform scripts change

Side effects

Breaking backward compatibility

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Does this PR introduce a user-facing change?:

Support configuring container lifecycle hooks and `terminationGracePeriodSeconds` in TiDB spec

tennix · 2020-06-29T07:04:06Z

Can we make sure that TiDB still receives connection before the LB removes the backend?

tennix · 2020-06-29T07:19:19Z

In #2597, we want the tidb-server to continue to receive connections before it's removed from LB backend. For example, fail the readiness probe first but don't send KILL signal to tidb-server, so tidb-server can continue to receive connections, after 30s, send KILL signal to tidb-server and wait tidb-server gracefully terminate the existing connections.

weekface · 2020-06-29T07:49:52Z

Can we make sure that TiDB still receives connection before the LB removes the backend?

I think so.

When a pod is going to be deleted (DeletionTimestamp != nil), its IP is ignored in endpoints controller and will be completely removed from endpoints. ref: #2597 (comment)

So we don't need readiness probe.

Before removed from endpoints, TiDB can receive new connections. After removed from endpoints, TiDB can't receive new connections, but the old connections work well as expected.

The kill -QUIT 1 and a longer terminationGracePeriodSeconds will wait for all these old connections closed by client.

DanielZhangQD · 2020-06-30T02:38:09Z

Can we make sure that TiDB still receives connection before the LB removes the backend?

I think so.

When a pod is going to be deleted (DeletionTimestamp != nil), its IP is ignored in endpoints controller and will be completely removed from endpoints. ref: #2597 (comment)

So we don't need readiness probe.

Before removed from endpoints, TiDB can receive new connections. After removed from endpoints, TiDB can't receive new connections, but the old connections work well as expected.

The kill -QUIT 1 and a longer terminationGracePeriodSeconds will wait for all these old connections closed by client.

I think the LB that @tennix mentioned is the real load balancer, e.g. the NLB on AWS, it has its own probe for the target backend, which is also for port 4000 with our current configurations, however, with this PR, I think the new connections will still be sent to the TiDB Pod that is being terminated, which will cause request failure because the NLB will not remove it from its backend at that time.

I think the key point is:

The LB removes the Pod from its backend first
- Before that, the Pod can still handle new connections
- After that, no new connections will come to the Pod
The Pod can be terminated with SIGQUIT and the existing connections are handled gracefully

Is my understanding correct? @tennix

weekface · 2020-06-30T03:05:20Z

however, with this PR, I think the new connections will still be sent to the TiDB Pod that is being terminated, which will cause request failure because the NLB will not remove it from its backend at that time.

I don't know why this request failed? TiDB will not close the connection until the user closes the connection himself.

DanielZhangQD · 2020-06-30T06:30:30Z

however, with this PR, I think the new connections will still be sent to the TiDB Pod that is being terminated, which will cause request failure because the NLB will not remove it from its backend at that time.

I don't know why this request failed? TiDB will not close the connection until the user closes the connection himself.

For this description, Before removed from endpoints, TiDB can receive new connections. After removed from endpoints, TiDB can't receive new connections, but the old connections work well as expected., please verify when the pod is removed from the endpoint, will it also be removed from the load balancer backend?
If yes, I think this PR is good.
If no, the LB will still send new connection request to this Pod, but this Pod does not handle new connections at that time.

weekface · 2020-06-30T07:14:10Z

please verify when the pod is removed from the endpoint, will it also be removed from the load balancer backend?

About endpoints remove please this comment: #2597 (comment)

We can't control the LB behavior in TiDB Operator, all we can do is wait the connections to close gracefully by client.

If no, the LB will still send new connection request to this Pod, but this Pod does not handle new connections at that time.

The request flow from LB to Pod is: LB -> NodePort -> Service -> Pod. After the pod was removed from Service, no new connection can be sent to the Pod anymore.

tennix · 2020-06-30T07:48:36Z

The event happens in the following order:

Users or controller delete the pod
kube-apiserver adds a deletion marker on the pod object
kube-proxy removes the pod from endpoints and kubelet send KILL to tidb-server process (these two events happen in random order I guess)
tidb-server stops receiving new connections after it got KILL signal but still handles old connections, so if a new connection comes in, it will fail.

The node will be removed from the external LB if using Local external traffic policy. This is not guaranteed to happen before tidb-server stop receiving new connections. So the client will get timeout errors if there is no retry logic (for example some PHP frameworks).

cofyc · 2020-06-30T08:10:34Z

as the tidb-server closes the listener immediately after receiving termination signals (including graceful QUIT), does it help to wait a few seconds before sending signals to the tidb-server process? this gives the endpoints controller (also the LB operator) time to react.

weekface · 2020-06-30T09:04:06Z

The node will be removed from the external LB if using Local external traffic policy. This is not guaranteed to happen before tidb-server stop receiving new connections. So the client will get timeout errors if there is no retry logic (for example some PHP frameworks).

For service that is LoadBalacer and Local external traffic policy, k8s will start a ServiceHealth http server for each Service. The LB will health check this server to remove or add node to the external LB backends. This server will return a 503 if the endpoints are empty on this node. Then the external LB can remove this node from LB backends after retrying a few times.

This is not guaranteed to happen before stopping tidb-server. But we can sleep a few seconds before send QUIT signal to tidb-server as @cofyc said to wait the external LB remove this node from its backends.

Therefore, almost no connection will fail during the rolling update.

cofyc · 2020-07-01T02:33:50Z

pkg/apis/pingcap/v1alpha1/types.go

+	// events. For the PostStart and PreStop lifecycle handlers, management of the container blocks
+	// until the action is complete, unless the container process fails, in which case the handler is aborted.
+	// +optional
+	Lifecycle *corev1.Lifecycle `json:"lifecycle,omitempty"`


how about moving these two fields to ComponentSpec struct? thought it may not normally be necessary to configure these fields for other components, it would be strange for users that only TiDB has these pod fields.

…-operator into graceful-stop-tidb

cofyc · 2020-07-01T04:28:35Z

pkg/manager/member/tidb_member_manager.go

+
+	if tc.Spec.TiDB.TerminationGracePeriodSeconds != nil {
+		tidbSet.Spec.Template.Spec.TerminationGracePeriodSeconds = tc.Spec.TiDB.TerminationGracePeriodSeconds
+	}


didn't update other components' controller logic
implement in a separate PR?

cofyc · 2020-07-01T04:42:24Z

I suggest changing the release note to a more user faced one: Support configuring container lifecycle hooks and `terminationGracePeriodSeconds` in TiDB spec

stopping TiDB gracefully is the main goal, but it's not the actual change in this PR. any action can be injected to achieve different purposes.

cofyc

LGTM

DanielZhangQD

LGTM

DanielZhangQD · 2020-07-01T07:16:50Z

/merge

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2020-07-01T08:56:34Z

cherry pick to release-1.1 in PR #2848

* graceful stop tidb * address comment

* add v1.1.2 release nots * Apply suggestions from code review Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com> Co-authored-by: Ran <huangran@pingcap.com> * Apply suggestions from code review Co-authored-by: Ran <huangran@pingcap.com> * add release notes of #2810 Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com> Co-authored-by: Ran <huangran@pingcap.com>

graceful stop tidb

75b6274

weekface added the needs-cherry-pick-1.1 label Jun 24, 2020

weekface requested review from cofyc, tennix and DanielZhangQD June 24, 2020 13:21

weekface mentioned this pull request Jun 24, 2020

Add document for stop TiDB gracefully #2811

Closed

Yisaer previously approved these changes Jun 28, 2020

View reviewed changes

cofyc previously approved these changes Jun 30, 2020

View reviewed changes

Merge branch 'master' into graceful-stop-tidb

71452e7

cofyc reviewed Jul 1, 2020

View reviewed changes

weekface added 2 commits July 1, 2020 11:38

address comment

89c0a37

Merge branch 'graceful-stop-tidb' of https://github.com/weekface/tidb…

65fc736

…-operator into graceful-stop-tidb

weekface dismissed stale reviews from cofyc and Yisaer via 65fc736 July 1, 2020 03:38

weekface requested review from Yisaer and cofyc July 1, 2020 03:39

cofyc reviewed Jul 1, 2020

View reviewed changes

weekface mentioned this pull request Jul 1, 2020

set terminationGracePeriodSeconds and lifecycle to all controllers #2847

Open

weekface requested a review from cofyc July 1, 2020 06:29

cofyc approved these changes Jul 1, 2020

View reviewed changes

cofyc mentioned this pull request Jul 1, 2020

release v1.1.2 #2839

Closed

6 tasks

DanielZhangQD approved these changes Jul 1, 2020

View reviewed changes

Merge branch 'master' into graceful-stop-tidb

9210558

cofyc merged commit 8b93073 into pingcap:master Jul 1, 2020

ti-srebot pushed a commit to ti-srebot/tidb-operator that referenced this pull request Jul 1, 2020

cherry pick pingcap#2810 to release-1.1

baf4ace

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Jul 1, 2020

Add support to stop TiDB gracefully (#2810) #2848

Closed

weekface deleted the graceful-stop-tidb branch July 1, 2020 09:23

DanielZhangQD pushed a commit that referenced this pull request Jul 1, 2020

Automated cherry pick of #2810 upstream release 1.1 (#2849)

16123b2

* graceful stop tidb * address comment

cofyc added a commit to cofyc/tidb-operator that referenced this pull request Jul 1, 2020

add release notes of pingcap#2810

78bdc33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to stop TiDB gracefully #2810

Add support to stop TiDB gracefully #2810

weekface commented Jun 24, 2020 •

edited

Loading

tennix commented Jun 29, 2020

tennix commented Jun 29, 2020

weekface commented Jun 29, 2020

DanielZhangQD commented Jun 30, 2020 •

edited

Loading

weekface commented Jun 30, 2020

DanielZhangQD commented Jun 30, 2020

weekface commented Jun 30, 2020

tennix commented Jun 30, 2020

cofyc commented Jun 30, 2020 •

edited

Loading

weekface commented Jun 30, 2020 •

edited

Loading

cofyc Jul 1, 2020

weekface Jul 1, 2020

cofyc Jul 1, 2020

weekface Jul 1, 2020

cofyc commented Jul 1, 2020 •

edited

Loading

cofyc left a comment

DanielZhangQD left a comment

DanielZhangQD commented Jul 1, 2020

ti-srebot commented Jul 1, 2020

Add support to stop TiDB gracefully #2810

Add support to stop TiDB gracefully #2810

Conversation

weekface commented Jun 24, 2020 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Does this PR introduce a user-facing change?:

tennix commented Jun 29, 2020

tennix commented Jun 29, 2020

weekface commented Jun 29, 2020

DanielZhangQD commented Jun 30, 2020 • edited Loading

weekface commented Jun 30, 2020

DanielZhangQD commented Jun 30, 2020

weekface commented Jun 30, 2020

tennix commented Jun 30, 2020

cofyc commented Jun 30, 2020 • edited Loading

weekface commented Jun 30, 2020 • edited Loading

cofyc Jul 1, 2020

Choose a reason for hiding this comment

weekface Jul 1, 2020

Choose a reason for hiding this comment

cofyc Jul 1, 2020

Choose a reason for hiding this comment

weekface Jul 1, 2020

Choose a reason for hiding this comment

cofyc commented Jul 1, 2020 • edited Loading

cofyc left a comment

Choose a reason for hiding this comment

DanielZhangQD left a comment

Choose a reason for hiding this comment

DanielZhangQD commented Jul 1, 2020

ti-srebot commented Jul 1, 2020

weekface commented Jun 24, 2020 •

edited

Loading

DanielZhangQD commented Jun 30, 2020 •

edited

Loading

cofyc commented Jun 30, 2020 •

edited

Loading

weekface commented Jun 30, 2020 •

edited

Loading

cofyc commented Jul 1, 2020 •

edited

Loading