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

tidb-in-kubernetes: add tidb and pump affinity configuration for prod best practice #2083

Merged
merged 25 commits into from
Dec 16, 2019

Conversation

Yisaer
Copy link
Contributor

@Yisaer Yisaer commented Nov 28, 2019

What is changed, added or deleted?

Add tidb and pump affinity configuration for prod best practice.

What is the related PR or file link(s)?

#2073

Which version does your change affect?

I will add it into v3.0 and v3.1 if I get 2 LGTM

  • dev

@Yisaer Yisaer requested a review from tennix November 28, 2019 08:31

默认情况下, TiDB 的 affinity 亲和性设置为 `{}`,当启用 TiDB Binlog 时,由于目前 Pump 组件与 TiDB 组件默认并不是一一对应的,如果当 Pump 与 TiDB 组件分开部署并出现网络隔离时并且 TiDB Binlog 还开启了 `ignore-error` 时,将会导致 TiDB 丢失 Binlog 。推荐通过亲和性特性将 TiDB 与 Pump 部署在同一台 Node 上,同时通过反亲和性特性将 Pump 分散在不同的 Node 上,每台 Node 上至多仅需一个 Pump 实例即可。

* 将 `tidb.affinity` 按照如下设置:
Copy link
Member

Choose a reason for hiding this comment

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

The affinity rules need to be restricted within a single tidb cluster.

Copy link
Contributor Author

@Yisaer Yisaer Nov 28, 2019

Choose a reason for hiding this comment

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

Updated.

Add app.kubernetes.io/instance key to restricted into a single tidb cluster

@Yisaer Yisaer requested review from weekface and tennix November 28, 2019 11:06
@lilin90 lilin90 added translation/doing This PR’s assignee is translating this PR. tidb-in-k8s labels Nov 28, 2019

* 将 `tidb.affinity` 按照如下设置:

```yaml
Copy link
Member

Choose a reason for hiding this comment

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

Indent this code block with 4 more spaces to make it display well under the list item.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


* 将 `binlog.pump.affinity` 按照如下设置:

```yaml
Copy link
Member

Choose a reason for hiding this comment

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

Indent this code block with 4 more spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


```yaml
binlog:
pump:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest configuring affinity with tidb too, in case that pump topology changed after the rolling update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tidb affinity already configured in the request.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean podaffinity for pump to colocate with tidb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.


* 将 `tidb.affinity` 按照如下设置:

```yaml
Copy link
Member

Choose a reason for hiding this comment

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

Please indent this code block with 8 spaces ahead. Otherwise, it will cause display chaos at the PingCAP website (although now it displays well on GitHub). Thanks!

Suggested change
```yaml
```yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


* 将 `binlog.pump.affinity` 按照如下设置:

```yaml
Copy link
Member

Choose a reason for hiding this comment

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

Indent this code block with 8 spaces from the beginning of each line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@Yisaer Yisaer requested a review from lilin90 November 29, 2019 03:59
Copy link
Member

@lilin90 lilin90 left a comment

Choose a reason for hiding this comment

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

LGTM

@Yisaer Yisaer requested a review from lilin90 November 29, 2019 04:57
@lilin90
Copy link
Member

lilin90 commented Nov 29, 2019

@DanielZhangQD PTAL

- key: "app.kubernetes.io/instance"
operator: In
values:
- <release-name>
Copy link
Member

Choose a reason for hiding this comment

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

Add a document for what is this? Because users may just copy and paste this yaml snippet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest changing this to cluster-name, many DBAs take it as something like v1.0.4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently <release-name> is used in many places in tidb-in-kubernetes document. I think we should keep it same globally. Maybe we could add a note to explain what's <release-name> ? @tennix @DanielZhangQD

Copy link
Contributor

@DanielZhangQD DanielZhangQD left a comment

Choose a reason for hiding this comment

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

LGTM

@Yisaer
Copy link
Contributor Author

Yisaer commented Dec 5, 2019

@lilin90 PATL

Copy link
Member

@lilin90 lilin90 left a comment

Choose a reason for hiding this comment

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

Rest LGTM


> **注意:**
>
> <release-name> 需要替换为目标 `tidb-cluster`的 helm release name
Copy link
Member

@lilin90 lilin90 Dec 9, 2019

Choose a reason for hiding this comment

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

Suggested change
> <release-name> 需要替换为目标 `tidb-cluster`的 helm release name
> `<release-name>` 需要替换为目标 `tidb-cluster` 的 Helm release name


> **注意:**
>
> <release-name> 需要替换为目标 `tidb-cluster`的 helm release name
Copy link
Member

@lilin90 lilin90 Dec 9, 2019

Choose a reason for hiding this comment

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

Suggested change
> <release-name> 需要替换为目标 `tidb-cluster`的 helm release name
> `<release-name>` 需要替换为目标 `tidb-cluster` 的 Helm release name


> **注意:**
>
> <release-name> 需要替换为目标 `tidb-cluster`的 helm release name
Copy link
Member

@lilin90 lilin90 Dec 9, 2019

Choose a reason for hiding this comment

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

Suggested change
> <release-name> 需要替换为目标 `tidb-cluster`的 helm release name
> `<release-name>` 需要替换为目标 `tidb-cluster` 的 Helm release name

Copy link
Member

@lilin90 lilin90 left a comment

Choose a reason for hiding this comment

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

LGTM

@lilin90 lilin90 merged commit db6a7dd into pingcap:master Dec 16, 2019
@lilin90 lilin90 added the size/medium Changes of a medium size. label Dec 16, 2019
@junlan-zhang junlan-zhang added translation/done This PR has been translated from English into Chinese and updated to pingcap/docs-cn in a PR. and removed translation/doing This PR’s assignee is translating this PR. labels Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/medium Changes of a medium size. translation/done This PR has been translated from English into Chinese and updated to pingcap/docs-cn in a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants