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

op-guide: add TiDB binary deployment doc #944

Merged
merged 10 commits into from
Mar 28, 2019

Conversation

liubo0127
Copy link
Contributor

@lilin90 PTAL

@lilin90 lilin90 changed the title Add binary deployment op-guide: add TiDB binary deployment doc Mar 11, 2019
@lilin90 lilin90 requested a review from morgo March 11, 2019 11:44
Copy link
Contributor

@morgo morgo left a comment

Choose a reason for hiding this comment

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

I have made a few suggestions.

op-guide/binary-deployment.md Outdated Show resolved Hide resolved
op-guide/binary-deployment.md Outdated Show resolved Hide resolved
| vm.swappiness | Set `vm.swappiness = 0` |


> **Note**: To adjust the operating system parameters, contact your system administrator.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be more specific than this. As I understand, the only required change is to file limits. We should show an example of how to change it.

Copy link
Member

@zz-jason zz-jason Mar 13, 2019

Choose a reason for hiding this comment

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

use sudo echo 100000 > /proc/sys/fs/file-max to change User Open Files Limit to 100000

like this?

Copy link
Member

Choose a reason for hiding this comment

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

@morgo Please see Jason's reply. If this is not what you mean, is it possible that you help add an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

@zz-jason afaik the sudo only applies to the echo there. We could denote it is a root command with '#'

Copy link
Member

Choose a reason for hiding this comment

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

@morgo @zz-jason The echo approach only applies to current session. When the system get rebooted, this setting will get lost. To make the setting persistent, users have to modify /etc/security/limits.conf like what tidb-ansible does https://github.com/pingcap/tidb-ansible/blob/master/roles/bootstrap/tasks/root_tasks.yml#L23-L31

      {{ deploy_user }}        soft        nofile        1000000
      {{ deploy_user }}        hard        nofile        1000000
      {{ deploy_user }}        soft        stack         10240

The {{ deploy_user }} is the user which runs tidb.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a fair amount of confidence that only the fs.file-max limit needs to be changed. Context: tikv/tikv#3387

@ethercflow I like the idea of leaving the others at default and linking to the guide with some suggestions of "what to look at". @tennix how do you feel about only suggesting fs.file-max ?

Copy link
Member

Choose a reason for hiding this comment

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

TiKV checks some other kernel parameters and reports warnings in the logs. I think it's better to tune these parameters to make TiKV happy. And I believe that's what TiDB Ansible does.

Copy link
Contributor Author

@liubo0127 liubo0127 Mar 27, 2019

Choose a reason for hiding this comment

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

This document does not actually involve parameter tuning. I think these parameters can be added, some parameters are consistent with tidb-ansible, and some parameters are system defaults. Setting these parameters is good for performance, and no setting has no affect on deployment.

Copy link
Member

Choose a reason for hiding this comment

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

For further improvement, we can do it later. As long as it does not affect the deployment and testing, I hope this PR can be merged soon. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

and no setting has no affect on deployment.
@liubo0127 if they don't change fs.file-max it is possible tikv won't start in a really weird way. Context: tikv/tikv#3387

I am happy with it for now, but I would like to see them described as optional (except fs.file-max).

op-guide/binary-deployment.md Outdated Show resolved Hide resolved
op-guide/binary-deployment.md Outdated Show resolved Hide resolved
op-guide/binary-deployment.md Outdated Show resolved Hide resolved
op-guide/binary-deployment.md Outdated Show resolved Hide resolved
op-guide/binary-deployment.md Outdated Show resolved Hide resolved
op-guide/binary-deployment.md Outdated Show resolved Hide resolved
op-guide/binary-deployment.md Outdated Show resolved Hide resolved
@c4pt0r
Copy link
Member

c4pt0r commented Mar 11, 2019

  1. Node export is optional, in the deployment of Prometheus.

  2. Every main binary component supports '--metrics-addr' parameter. I have a docker-compose repo for launch a full monitor stack for TiDB platform, which is very handy.
    https://github.com/c4pt0r/tidb-dashboard-docker-compose

@morgo PTAL.

@morgo
Copy link
Contributor

morgo commented Mar 11, 2019

@c4pt0r LGTM. I suggest that we move monitoring from this document to a sub-document where we can offer the binary install, or docker compose.

@lilin90 lilin90 requested a review from zz-jason March 12, 2019 06:50
@lilin90
Copy link
Member

lilin90 commented Mar 13, 2019

@morgo We can move the monitoring service in another PR. Is it a good place here?

@liubo0127 and I updated some sections. PTAL again~ Thanks!

@morgo
Copy link
Contributor

morgo commented Mar 13, 2019

@morgo We can move the monitoring service in another PR. Is it a good place here?

Yes, looks like a good place here.

@liubo0127
Copy link
Contributor Author

LGTM

@lilin90
Copy link
Member

lilin90 commented Mar 27, 2019

Since the monitoring section has been added to another file, I removed it from this one. Ref: #965

| CPU Frequency Scaling | It is recommended to turn on CPU overclocking |
| Transparent Hugepages | For Red Hat 7+ and CentOS 7+ systems, it is required to set the Transparent Hugepages to `always` |
| I/O Scheduler | Set the I/O Scheduler of data disks to the `deadline` mode |
| vm.swappiness | Set `vm.swappiness = 0` |
Copy link
Contributor Author

@liubo0127 liubo0127 Mar 27, 2019

Choose a reason for hiding this comment

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

In order to be consistent with the parameters in tidb-ansible, it is recommended to add the following two parameters.

| net.core.somaxconn | Set net.core.somaxconn = 32768 in sysctl.conf |
| net.ipv4.tcp_syncookies | Set net.ipv4.tcp_syncookies = 0 in sysctl.conf |

@morgo
Copy link
Contributor

morgo commented Mar 27, 2019

LGTM

1 similar comment
@ceohockey60
Copy link
Contributor

LGTM

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 0572c7f into pingcap:master Mar 28, 2019
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.

8 participants