Proposal: add multiple statefulsets support to TiDB Operator #240

weekface · 2018-12-18T11:50:51Z

fixes: #228

@tennix @onlymellb @xiaojingchen @gregwebs PTAL

gregwebs · 2018-12-18T14:54:15Z

docs/design-proposals/multiple-statefulsets.md

+  replicas: 3
+  image: pingcap/tikv:v2.1.0
+  …
+  extra:


This should be renamed since extra is a potentially common key name. sets would be better.

There are a default set and zero or more extra sets, so what about extraSets or otherSets

Yes, that is better. I wan't sure if we would support having just sets.

Yeah, that is the problem we just discussed. Removing the default statefulset would impact all the other sets that inherit the property from the default set.

Just sets +1

pd: - name: name-1 replicas: 3 ... - name: name-2 replicas: 2 ...

@tennix @onlymellb @xiaojingchen @gregwebs PTAL

xiaojingchen · 2018-12-19T03:47:32Z

docs/design-proposals/multiple-statefulsets.md

+}
+
+type TiKVStatus struct {
+StatefulSet     []apps.StatefulSetStatus     `json:"statefulSet,omitempty"`


StatefulSet -> StatefulSets

xiaojingchen · 2018-12-19T04:07:59Z

docs/design-proposals/multiple-statefulsets.md

+
+``` go
+type TidbClusterSpec struct {
+  TiKV            []TiKVSpec            `json:"tikv,omitempty"`


TiKV is a unreasonable name when its type is an array.

Any suggestion?

TiKVSets or TiKVDomains

@tennix @onlymellb @gregwebs Is this ok?

I prefer TiKVSets

TiKVSets is ok for me. Do we need rename TiKVSpec to TiKVSetSpec?

gregwebs · 2018-12-19T04:51:21Z

I am still very concerned that we are creating a highly-manual UX that doesn't actually match the user's workflow. To scale vertically, I just want to tell the operator to scale vertically. Actually change the yaml to create multiple stateful sets isn't what I want to do.

The advantage seems to be that

It makes it apparent to me that I need to increase the cluster capacity
I can write programs that can use this to automatically do what I want

tennix · 2018-12-19T02:40:14Z

docs/design-proposals/multiple-statefulsets.md

+
+## Background
+
+Now, TiDB Operator create only one statefulset for one PD/TiKV/TiDB component separately. This works very well in most cases.


s/create/creates/

tennix · 2018-12-19T02:42:36Z

docs/design-proposals/multiple-statefulsets.md

+* When using local volume, only horizontal scaling is guaranteed, vertical scaling may fail if there are not enough resources on the node. We need vertical scaling anyway
+* Users can't specify an exact number of pods in a given AZ. For example deploy 3 TiKV in AZ-1, and 2 TiKV in AZ-2
+* For on-premise deployment, k8s nodes may not have the same size. To make most of the resources, it's better to allow different profiles for a single cluster.
+* One TiKV peer use different resources(CPU/Memory/Storage) from other peers in the same TiDB cluster


This is the same case as above for on-premise deployment.

tennix · 2018-12-19T02:48:30Z

docs/design-proposals/multiple-statefulsets.md

+* For on-premise deployment, k8s nodes may not have the same size. To make most of the resources, it's better to allow different profiles for a single cluster.
+* One TiKV peer use different resources(CPU/Memory/Storage) from other peers in the same TiDB cluster
+* No isolation between different tidb-servers, a big AP query may affect TP query. The worst case is that AP query cause a tidb-server OOM, and this tidb-server is also serving TP request
+* No support offline the specific peers


No support for offline arbitrary peers due to ordinal limitations of statefulset.

tennix · 2018-12-19T03:48:53Z

docs/design-proposals/multiple-statefulsets.md

+
+Using local persistent volume, once deployed the PD and TiKV pod has to be on the node all its lifecycle, migrating it to other nodes with larger resources would be hard. However if we use multiple statefulsets, we can do the migration on the fly. This is useful when there is a hot region on a TiKV that can hardly split.
+
+So the TiDB Operator should support creating multiple statefulsets for one component to achieve all these requests:


However, we can achieve all the above issues by using multiple statefulsets:
...

So this proposal suggests extending one statefulset with multiple statefulsets for a single component.

tennix · 2018-12-19T03:49:38Z

docs/design-proposals/multiple-statefulsets.md

+* No support offline the specific peers
+* No support deploy TiDB Operator on multiple K8s clusters
+
+Using local persistent volume, once deployed the PD and TiKV pod has to be on the node all its lifecycle, migrating it to other nodes with larger resources would be hard. However if we use multiple statefulsets, we can do the migration on the fly. This is useful when there is a hot region on a TiKV that can hardly split.


This paragraph should be removed.

tennix · 2018-12-19T05:29:12Z

@gregwebs Yes, the vertical scaling requires many manual interventions. But I don't think it's proper to do this in tidb-operator. Vertical scaling may involve migrating all data to a new host, this may be time-consuming and there is no clear way to tell users the progress. We should add another proposal of how to implement it.

gregwebs · 2018-12-19T20:06:02Z

What would be very helpful to understand how this is designed to be used is to alter the user guide according to the proposal. Or otherwise list out all the different scenarios this is designed for and how it will be used in them.

…eekface/tidb-operator into multiple-statefulsets-proposal

weekface · 2018-12-21T10:10:35Z

I made some changes based on @tennix @onlymellb @xiaojingchen 's suggestions: e608b07

weekface · 2018-12-24T10:06:40Z

According to K8s api compatibility:

An API change is considered compatible if it:

adds new functionality that is not required for correct behavior (e.g., does not add a new required field)

does not change existing semantics, including:

the semantic meaning of default values and behavior

interpretation of existing API types, fields, and values

which fields are required and which are not

mutable fields do not become immutable

valid values do not become invalid

explicitly invalid values do not become valid

Put another way:

Any API call (e.g. a structure POSTed to a REST endpoint) that succeeded before your change must succeed after your change.

Any API call that does not use your change must behave the same as it did before your change.

Any API call that uses your change must not cause problems (e.g. crash or degrade behavior) when issued against an API servers that do not include your change.

It must be possible to round-trip your change (convert to different API versions and back) with no loss of information.

Existing clients need not be aware of your change in order for them to continue to function as they did previously, even when your change is in use.

It must be possible to rollback to a previous version of API server that does not include your change and have no impact on API objects which do not use your change. API objects that use your change will be impacted in case of a rollback.

If your change does not meet these criteria, it is not considered compatible, and may break older clients, or result in newer clients causing undefined behavior. Such changes are generally disallowed, though exceptions have been made in extreme cases (e.g. security or obvious bugs).

So add an extra field to existing may be a better solution. For example: changevalues.yaml from figure 1 to figure 2, and change the TidbCluster object from figure 3 to figure 4:

Figure 1:

tikv:
  replicas: 3
  image: pingcap/tikv:v2.1.0
  …

Figure 2:

tikv:
  replicas: 3
  image: pingcap/tikv:v2.1.0
  …

tikvs:
  - name: name-1
    replicas: 3
    image: pingcap/tikv:v2.1.0
    ...
  - name: name-2
    replicas: 2
    image: pingcap/tikv:v2.1.0
    ...

Figure 3:

type TidbClusterSpec struct {
  TiKV            TiKVSpec            `json:"tikv,omitempty"`
  …
}

Figure 4:

type TidbClusterSpec struct {
  TiKV            TiKVSpec            `json:"tikv,omitempty"`
  TiKVs           []TiKVSpec            `json:"tikvs,omitempty"`
  …
}

tennix · 2018-12-25T03:06:20Z

How do we handle the status of TidbCluster, should we merge all the statefulsets status? And how do we handle the order of all the statefulsets, the first PD acts as the seed and it's different from the rest.

weekface · 2018-12-25T03:19:15Z

How do we handle the status of TidbCluster, should we merge all the statefulsets status?

No. Add a new attribute SetStatuses.

And how do we handle the order of all the statefulsets, the first PD acts as the seed and it's different from the rest.

Yes, the pd is the first order. Others are ordered by their index.

…eekface/tidb-operator into multiple-statefulsets-proposal

weekface · 2018-12-25T08:27:56Z

I change the API according to K8s api compatibility and add some use cases including vertical scaling and multiple zones.

@tennix @onlymellb @xiaojingchen @gregwebs PTAL

tennix

LGTM

onlymellb · 2018-12-26T04:26:30Z

docs/design-proposals/multiple-statefulsets.md

+  TiKV            TiKVSpec            `json:"tikv,omitempty"`
+  …
+}
+


Add the definition of type TiKVSpec as shown in Figure 4.

Already there: https://github.com/pingcap/tidb-operator/pull/240/files/d794e4e469b9ba27a0b8231ede3de3a740497709#diff-9caf4dcfa00ba8640bab97c5a71a53bbR115

xiaojingchen · 2018-12-26T04:30:39Z

docs/design-proposals/multiple-statefulsets.md

+
+type TiKVStatus struct {
+  StatefulSet     *apps.StatefulSetStatus     `json:"statefulSet,omitempty"`
+  StatefulSets    []apps.StatefulSetStatus    `json:"statefulSets,omitempty"`


StatefulSets type change to map[string]apps.StatefulSetStatus

docs/design-proposals/multiple-statefulsets.md

onlymellb · 2018-12-26T04:42:40Z

docs/design-proposals/multiple-statefulsets.md

+```
+
+The operator does not guarantee the order of multiple statefulsets’ upgrade. If the user care about the order indeed. Just: modify one statefulset, wait it done, and then modify the other one.
+


I think we can have a default upgrade order, such as we can upgrade the main statefulset first, and then upgrade the statefulset in the order of statefulset slice?

onlymellb · 2018-12-26T04:44:55Z

docs/design-proposals/multiple-statefulsets.md

+```
+
+The operator does not guarantee the order of multiple statefulsets’ scale in/out. If the user care the order indeed. Just: modify one statefulset, wait it done, and then modify the other one.
+


onlymellb · 2018-12-26T04:55:14Z

docs/design-proposals/multiple-statefulsets.md

+To do vertical scaling, the user must:
+
+- Add a new statefulset in the `tikvs` section, using more `requests` or `limits` and wait it done. Add a new `name-3`, change from figure 9 to figure 10;
+- Scale down the old statefulset(`name-1`) in the `tikvs` section to zero and wait it done. See figure 11;


Do we need to wait for the new statefulset name-3 to start completely before scale down？

The user must ensure that.

docs/design-proposals/multiple-statefulsets.md

Co-Authored-By: weekface <weekface@gmail.com>

…eekface/tidb-operator into multiple-statefulsets-proposal

tennix · 2018-12-28T06:22:48Z

What if the name filed in tikvs are the same? How do we handle this case?
For API compatibility, do you mean we should use different apiVersion to access the two different form of resources
For the pd/tikv service, you mentioned there is no change. Do you mean that different statefulsets using the same headless and non-headless service as the main statefulset, or service per statefulset?

tennix · 2018-12-28T06:24:35Z

docs/design-proposals/multiple-statefulsets.md

+
+### Vertical scaling
+
+We implement this feature in two phases.


We can achieve vertical scaling in two phases.

We do not implement this in tidb-operator. But users can achieve this.

weekface · 2018-12-28T06:44:51Z

What if the name filed in tikvs are the same? How do we handle this case?

Validation will fail. Addressed.

For API compatibility, do you mean we should use different apiVersion to access the two different form of resources

I mean the API changes in this proposal are compatible.

For the pd/tikv service, you mentioned there is no change. Do you mean that different statefulsets using the same headless and non-headless service as the main statefulset, or service per statefulset?

No.

For PD: N+1 Services should be created, N headless services and 1 client service
For TiKV: N headless services should be created

Addressed.

…eekface/tidb-operator into multiple-statefulsets-proposal

tennix

LGTM

onlymellb

LGTM

weekface added 3 commits December 18, 2018 17:49

stash

8d0c123

Merge branch 'master' into multiple-statefulsets-proposal

7463cf7

tiny fix

09044f6

gregwebs reviewed Dec 18, 2018

View reviewed changes

xiaojingchen reviewed Dec 19, 2018

View reviewed changes

tennix reviewed Dec 19, 2018

View reviewed changes

weekface added 2 commits December 19, 2018 15:03

address comments

43cae6b

Merge branch 'master' into multiple-statefulsets-proposal

d4277c0

weekface added 3 commits December 21, 2018 11:29

address comment

065671d

Merge branch 'multiple-statefulsets-proposal' of https://github.com/w…

24c5653

…eekface/tidb-operator into multiple-statefulsets-proposal

address comment

e608b07

weekface force-pushed the multiple-statefulsets-proposal branch from f545a5c to e608b07 Compare December 21, 2018 10:07

Merge branch 'master' into multiple-statefulsets-proposal

68e2c25

Merge branch 'master' into multiple-statefulsets-proposal

08fdfa8

weekface added 3 commits December 25, 2018 11:54

keep api compatibility

10bbdfe

Merge branch 'multiple-statefulsets-proposal' of https://github.com/w…

6eec6d4

…eekface/tidb-operator into multiple-statefulsets-proposal

Merge branch 'master' into multiple-statefulsets-proposal

a8d4813

Merge branch 'master' into multiple-statefulsets-proposal

d794e4e

tennix previously approved these changes Dec 26, 2018

View reviewed changes

onlymellb reviewed Dec 26, 2018

View reviewed changes

xiaojingchen reviewed Dec 26, 2018

View reviewed changes

onlymellb reviewed Dec 26, 2018

View reviewed changes

docs/design-proposals/multiple-statefulsets.md Show resolved Hide resolved

onlymellb reviewed Dec 26, 2018

View reviewed changes

docs/design-proposals/multiple-statefulsets.md Outdated Show resolved Hide resolved

Update docs/design-proposals/multiple-statefulsets.md

e845c63

Co-Authored-By: weekface <weekface@gmail.com>

weekface dismissed stale reviews from tennix via e845c63 December 26, 2018 05:51

weekface added 5 commits December 26, 2018 14:36

address comment

0ef8781

Merge branch 'multiple-statefulsets-proposal' of https://github.com/w…

5379704

…eekface/tidb-operator into multiple-statefulsets-proposal

Merge branch 'master' into multiple-statefulsets-proposal

f3f95f0

address comment

886e934

Merge branch 'multiple-statefulsets-proposal' of https://github.com/w…

a481b98

…eekface/tidb-operator into multiple-statefulsets-proposal

tennix reviewed Dec 28, 2018

View reviewed changes

weekface added 2 commits December 28, 2018 14:39

address comment

1faf408

Merge branch 'master' into multiple-statefulsets-proposal

8d6934a

weekface added 3 commits December 28, 2018 15:38

use the same headless service in the same component

773bb1b

Merge branch 'multiple-statefulsets-proposal' of https://github.com/w…

bdace82

…eekface/tidb-operator into multiple-statefulsets-proposal

address comment

2cbc234

tennix approved these changes Dec 28, 2018

View reviewed changes

onlymellb approved these changes Dec 28, 2018

View reviewed changes

xiaojingchen approved these changes Dec 29, 2018

View reviewed changes

tennix merged commit 8392a81 into pingcap:master Dec 29, 2018

weekface deleted the multiple-statefulsets-proposal branch December 29, 2018 11:52

tennix mentioned this pull request Dec 30, 2018

Update roadmap #258

Merged

tennix mentioned this pull request Jan 10, 2019

Introduce a discovery service for PD to properly get bootstrapped #263

Closed


		## Background

		Now, TiDB Operator create only one statefulset for one PD/TiKV/TiDB component separately. This works very well in most cases.


		Using local persistent volume, once deployed the PD and TiKV pod has to be on the node all its lifecycle, migrating it to other nodes with larger resources would be hard. However if we use multiple statefulsets, we can do the migration on the fly. This is useful when there is a hot region on a TiKV that can hardly split.

		So the TiDB Operator should support creating multiple statefulsets for one component to achieve all these requests:

		```

		The operator does not guarantee the order of multiple statefulsets’ upgrade. If the user care about the order indeed. Just: modify one statefulset, wait it done, and then modify the other one.


		### Vertical scaling

		We implement this feature in two phases.

Proposal: add multiple statefulsets support to TiDB Operator #240

Proposal: add multiple statefulsets support to TiDB Operator #240

Conversation

weekface commented Dec 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregwebs commented Dec 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tennix commented Dec 19, 2018 • edited Loading

gregwebs commented Dec 19, 2018

weekface commented Dec 21, 2018

weekface commented Dec 24, 2018

tennix commented Dec 25, 2018

weekface commented Dec 25, 2018

weekface commented Dec 25, 2018

tennix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onlymellb Dec 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tennix commented Dec 28, 2018

Choose a reason for hiding this comment

weekface commented Dec 28, 2018

tennix left a comment

Choose a reason for hiding this comment

onlymellb left a comment

Choose a reason for hiding this comment

tennix commented Dec 19, 2018 •

edited

Loading

onlymellb Dec 26, 2018 •

edited

Loading