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

Add docker-entrypoint-initdb.d support #3695

Merged
merged 2 commits into from
Dec 7, 2018
Merged

Add docker-entrypoint-initdb.d support #3695

merged 2 commits into from
Dec 7, 2018

Conversation

xzkostyan
Copy link
Contributor

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

This done in postgres entrypoint in the same way: https://github.com/docker-library/postgres/tree/master/11.

The most tricky part is starting clickhouse-server in detached mode and sleeping 1 second after it

@xzkostyan
Copy link
Contributor Author

Solves #3319.

@xzkostyan
Copy link
Contributor Author

As alternative port readiness check for the client can be performed in something like this:

while true; do
    timeout 1 bash -c 'cat < /dev/null > /dev/tcp/localhost/9000' 2>1 /dev/null;
    rv=$?
    if [[ $rv = 0 ]]; then
        break
    fi
done

I don't know which approach is better sleep or this cycle.

#!/bin/bash
set -e

clickhouse client -n <<-EOSQL
Copy link
Contributor

Choose a reason for hiding this comment

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

Better use clickhouse-client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no clickhouse-client binary inside the clickhouse-server image.

Copy link
Contributor

Choose a reason for hiding this comment

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

And that is very confusing. IMHO client should be added to server container. It costs nothing (just a symlink) but makes usage of clickhouse-client much more logical.

pid="$!"
sleep 1

clickhouseclient=( clickhouse client --multiquery )
Copy link
Member

Choose a reason for hiding this comment

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

Don't understand, why we need array here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. In the postgres/mysql entrypoints password and database added later in script. This can be useful for future *_PASSWORD and *_USER variables support.

Array is redundant right now. I can change it to simple variable.

@alexey-milovidov alexey-milovidov merged commit 42969e9 into ClickHouse:master Dec 7, 2018
@alexey-milovidov
Copy link
Member

alexey-milovidov commented Dec 7, 2018

I don't know which approach is better sleep or this cycle.

With current approach we have race condition.

while true; do
    timeout 1 bash -c 'cat < /dev/null > /dev/tcp/localhost/9000' 2>1 /dev/null;
    rv=$?
    if [[ $rv = 0 ]]; then
        break
    fi
done

With this approach we have possibility of infinite loop without any diagnostics.
Better to set some limit... for example, 10 retries.

@xzkostyan
Copy link
Contributor Author

I can make another PR with port readiness probe within 10 retries. @alexey-milovidov should I?

@filimonov
Copy link
Contributor

filimonov commented Dec 10, 2018

As alternative port readiness check for the client can be performed in something like this:

cat < /dev/null > /dev/tcp/localhost/9000

That will leave "warning" in the log file, something like that:

2018.12.10 12:08:49.548198 [ 25 ] {} <Trace> TCPHandlerFactory: TCP Request. Address: [::1]:58626
2018.12.10 12:08:49.549074 [ 25 ] {} <Warning> TCPHandler: Client has not sent any data.
2018.12.10 12:08:49.549131 [ 25 ] {} <Information> TCPHandler: Done processing connection.

Some users are quite nervous about such warnings... :\

In some scanrios (for example, if docker uses real network instead of NAT) it can give false match, or will never succeed. So port should not be hardcoded there.

Possible alternatives:

  1. wait for a line "Ready for connections." in serverlog (something like that: https://superuser.com/a/449307 )
  2. parametrize the port by something like ${CLICKHOUSE_PORT:-9000} (still will create a warning).
  3. do the http ping wget --server-response --spider --quiet "http://localhost:8123" or echo 'HEAD /ping HTTP/1.0' > /dev/tcp/localhost/8123 (port still should be parametrized)

Also instead of parametrizing the port, you can extract it from config, like that:

clickhouse-extract-from-config --config-file /etc/clickhouse-server/config.xml  --key=tcp_port 
clickhouse-extract-from-config --config-file /etc/clickhouse-server/config.xml  --key=http_port 

summarize

For me that option looks the most straight ahead (easy to understand, no warnings, use wget possibilities to do retries with max number of retries, wget is avaliable in most of containers)

CLICKHOUSE_HTTP_PORT=$(clickhouse-extract-from-config --config-file /etc/clickhouse-server/config.xml  --key=http_port)
wget --server-response --spider --quiet --tries=20 --waitretry=1 --retry-connrefused "http://localhost:$CLICKHOUSE_HTTP_PORT/ping" && echo 'ok' || echo "ups"

@filimonov
Copy link
Contributor

And also, while we are here - it's better to change those lines:
https://github.com/yandex/ClickHouse/blob/0466f6beafc7d1c0f20b4309e420ab3ab8b98a0f/docker/server/entrypoint.sh#L9-L15
to use clickhouse-extract-from-config

filimonov added a commit to filimonov/ClickHouse that referenced this pull request Dec 10, 2018
alexey-milovidov added a commit that referenced this pull request Dec 10, 2018
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.

4 participants