Skip to content
This repository was archived by the owner on Oct 29, 2023. It is now read-only.

Add pidfile support for TiDB#64

Merged
datacharmer merged 4 commits intodatacharmer:capabilitiesfrom
morgo:pid-file
Feb 23, 2019
Merged

Add pidfile support for TiDB#64
datacharmer merged 4 commits intodatacharmer:capabilitiesfrom
morgo:pid-file

Conversation

@morgo
Copy link
Contributor

@morgo morgo commented Feb 20, 2019

I checked out how this is implemented in MySQL, and it is not a feature of mysqld, but rather mysqld_safe. TiDB does not have a comparable to mysqld_safe, and since many modern oses use something like systemd for this instead, it does not make sense to implement one (it has been a source of many security vulnerabilities too).

So instead I modified the start script to generate a pid file, and modified the stop and status scripts to use it. It seems to work fine (only tested on Linux, but I tried not to use any specific features).

The stop and status scripts check for the pid file, in addition to the start script checking for stale pid files.

@morgo morgo mentioned this pull request Feb 20, 2019
5 tasks
Copy link
Owner

@datacharmer datacharmer left a comment

Choose a reason for hiding this comment

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

The goal is to have the a consistent output from the sandbox scripts, as some of the text returned by script execution is evaluated in the functional tests.

  • status should return "on" and "off"
  • start should check for the server health and write the result accordingly. Currently, it exits with zero regardless. Could we perhaps check for the socket existence in a loop, since the PID is being generated in the script? If we do that, we can remove the fixed sleep instruction.
  • For consistency, start should not advertise "starting" but say what happened afterwards ("sandbox server started" or "sandbox server not started yet".

BTW, why do master downloads return different versions for MacOS and Linux? (#62 (comment)) I get 2.1.0 on MacOS and 3.0.0 on Linux.

@morgo
Copy link
Contributor Author

morgo commented Feb 21, 2019

BTW, why do master downloads return different versions for MacOS and Linux? (#62 (comment)) I get 2.1.0 on MacOS and 3.0.0 on Linux.

I suspect that MacOS builds might not be regenerating daily. Master should not be returning 3.0.0. I will ask the build team to look into it, but in my mind, we should only be targeting 3.0+ since it has support for tcp+unix socket. It will also now advertise socket/port/datadir correctly.

Thank you for the feedback, I will incorporate.

Changed use and status to use regular template
Fixed other templates to return compatible error codes and messages
Don't say starting until after attempted
@morgo
Copy link
Contributor Author

morgo commented Feb 22, 2019

I've updated some temporary packages to:
https://s3.us-east-2.amazonaws.com/files.tocker.ca/tidb-master-darwin-amd64.tar.gz
https://s3.us-east-2.amazonaws.com/files.tocker.ca/tidb-master-linux-amd64.tar.gz

I've verified they work on Mac OS and Linux, but they were manually created.. so no guarantees.

I have incorporated the changes requested in your review. In addition, the use and status commands now use the base templates.

@datacharmer
Copy link
Owner

Thanks for the update.
The good news is that test_sb passes. The bad ones is that the new start script is now mock-unfriendly.
Could you suggest a new template (similar to mysqld_safe_mock_template) that can emulate tidb-server so that the start script passes?

@morgo
Copy link
Contributor Author

morgo commented Feb 22, 2019

Sure, I'll take a look.

At quick glance it looks like the mock template writes a pid file (which in the case of TiDB the start script does), so conceptually it only needs to check that a -config argument was passed. i.e.:

#!/bin/sh
# mock-tidb-server

options=$1

# check for -config

if echo $options | grep -F 'config' > /dev/null 2>&1
then
 echo "contains config"
 exit 0
else
 echo "no config!"
 exit 1
fi

Do I need to add a sleep in there so it is a "running server"? I assume I don't need to patch start to accept a custom tidb-server, but rather a custom package is generated and unpacked?

@datacharmer
Copy link
Owner

datacharmer commented Feb 22, 2019

Correct. We should not change start to pass a mock test.
However, a no-op mock-script for tidb-server would not be enough, since start waits for a socket to appear.

The mock server is generated with a procedure similar to what we do in set-mock.sh.

The test that is failing is in sandbox_test.go

Run as

cd sandboxes
go test -v -run TestCreateSandbox/mocktidb .

Copy link
Owner

@datacharmer datacharmer left a comment

Choose a reason for hiding this comment

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

I can make the whole thing work (adding a new mock template for tidb-server that generates the right "almost" socket file) but only if we change the restrictive -S in start to a more relaxed -e.
Suggestions?


lsof -Pi :{{.Port}} -sTCP:LISTEN -t >/dev/null
exit $?`
if [ -S {{.GlobalTmpDir}}/mysql_sandbox{{.Port}}.sock ]
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if [ -S {{.GlobalTmpDir}}/mysql_sandbox{{.Port}}.sock ]
if [ -e {{.GlobalTmpDir}}/mysql_sandbox{{.Port}}.sock ]

See above comment

Co-Authored-By: morgo <tocker@gmail.com>
@datacharmer datacharmer merged commit 3509e00 into datacharmer:capabilities Feb 23, 2019
datacharmer added a commit that referenced this pull request Feb 23, 2019
* Implement flavor and capabilities support

Implement Issue #49 Add support for --client-from=X
Implement Issue #50 Change evaluation of version to include a flavor
Fix Issue #51 unpack command fails when tarball name doesn't include a version
Implement Issue #52 unpack command should create a FLAVOR file for the extracted tarball
Add command "dbdeployer admin capabilities [flavor [version]]"
Ensure tiDB binaries are recognized as valid.
Look for mysql_install_db only when flavor + version need it.
Fix Go native tests to use flavor and client basedir.
Code formatting.
Add tiDB templates
code formatting and annotations
update .gitignore and improve main test script
Add use template, fixed bug in CONTRIBUTING
fix bug in add_option filename
Add fix to kill script
Add check for template name uniqueness
Add check for tidb template names to use a known prefix
Add check to prevent replication with tidb flavor
Add flavor detection based on files in expanded tarball
Fix unpack panic when top directory not in tarball
Add tests for TiDB, flavor detection, capabilities
Fix Issue #48 MySQLx and group replication
Fix Issue #48 "can't enable mysqlx and group replication at the same time"
Add new script and better flavor support
Change "remote get" docs to reflect changed names
Fix Issue #62 TiDB fails on MacOS
Add script "after_start" to sandboxes (does nothing by default, but it is used by TiDB to clean-up unwanted scripts)
Add pidfile support for TiDB (#64)
Chang tiDB use and status to use regular template
Fix other templates to return compatible error codes and messages
Improve Travis test execution flow
Fix mock tests for TiDB
Add functional tests for TiDB
@morgo morgo deleted the pid-file branch February 26, 2019 19:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants