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

Upgrade clickhouse to 23.8 #3009

Merged
merged 10 commits into from
May 6, 2024
8 changes: 2 additions & 6 deletions clickhouse/config.xml
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
<yandex>
<max_server_memory_usage_to_ram_ratio>
<!-- This include is important!
It is required for the version of Clickhouse
used on ARM to read the environment variable. -->
<include from_env="MAX_MEMORY_USAGE_RATIO"/>
</max_server_memory_usage_to_ram_ratio>
<!-- This include is important! It is required for the version of Clickhouse used on ARM to read the environment variable. -->
Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason, in more recent versions of clickhouse the newlines here are causing parsing issues, so I just removed them

<max_server_memory_usage_to_ram_ratio><include from_env="MAX_MEMORY_USAGE_RATIO"/></max_server_memory_usage_to_ram_ratio>
<logger>
<level>warning</level>
<console>true</console>
Expand Down
2 changes: 1 addition & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ services:
build:
context: ./clickhouse
args:
BASE_IMAGE: "${CLICKHOUSE_IMAGE:-}"
BASE_IMAGE: "altinity/clickhouse-server:23.8.11.29.altinitystable"
ulimits:
nofile:
soft: 262144
Expand Down
1 change: 1 addition & 0 deletions install.sh
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ source install/check-latest-commit.sh
source install/check-minimum-requirements.sh

# Let's go! Start impacting things.
source install/upgrade-clickhouse.sh
Copy link
Member Author

Choose a reason for hiding this comment

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

The reason this is placed before turn-things-off is because turn things off script will remove the clickhouse image, which we need to verify the version of clickhouse before proceeding

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add this as a comment in the code, since whoever edits this in the future will probably want to know this fact as well.

source install/turn-things-off.sh
source install/update-docker-volume-permissions.sh
source install/create-docker-volumes.sh
Expand Down
2 changes: 0 additions & 2 deletions install/detect-platform.sh
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,8 @@ fi
export DOCKER_ARCH=$(docker info --format '{{.Architecture}}')
if [[ "$DOCKER_ARCH" = "x86_64" ]]; then
export DOCKER_PLATFORM="linux/amd64"
export CLICKHOUSE_IMAGE="altinity/clickhouse-server:21.8.13.1.altinitystable"
Copy link
Member Author

Choose a reason for hiding this comment

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

not needed, since more recent clickhouse images are multi platform

elif [[ "$DOCKER_ARCH" = "aarch64" ]]; then
export DOCKER_PLATFORM="linux/arm64"
export CLICKHOUSE_IMAGE="altinity/clickhouse-server:21.8.12.29.altinitydev.arm"
else
echo "FAIL: Unsupported docker architecture $DOCKER_ARCH."
exit 1
Expand Down
26 changes: 26 additions & 0 deletions install/upgrade-clickhouse.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
echo "${_group}Upgrading Clickhouse ..."

# First check to see if user is upgrading by checking for existing clickhouse volume
if [[ -n "$(docker volume ls -q --filter name=sentry-clickhouse)" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if it's worth adding a test for this? Should (hopefully) be pretty straightforward with the new python-based testing regime?

Copy link
Member Author

Choose a reason for hiding this comment

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

Editing the install script is not easily tested using the new python tests. Those are effective after self-hosted sentry is installed and up and running. The new self-hosted Sentry upgrade tests I added sufficiently covers this IMO, it skips over this portion of the code on initial install and hits this on the upgrade

# Start clickhouse if it is not already running
$dc up -d clickhouse

# Wait for clickhouse
RETRIES=30
until $dc ps clickhouse | grep 'healthy' || [ $RETRIES -eq 0 ]; do
echo "Waiting for clickhouse server, $((RETRIES--)) remaining attempts..."
sleep 1
done

# In order to get to 23.8, we need to first upgrade go from 21.8 -> 22.8 -> 23.3 -> 23.8
version=$($dc exec clickhouse clickhouse-client -q 'SELECT version()')
if [[ "$version" == "22.8.15.25.altinitystable" || "$version" == "21.8.12.29.altinitydev.arm" ]]; then
$dc down clickhouse
$dcb --build-arg BASE_IMAGE=altinity/clickhouse-server:22.8.15.25.altinitystable clickhouse
$dc up -d clickhouse
$dc down clickhouse
$dcb --build-arg BASE_IMAGE=altinity/clickhouse-server:23.3.19.33.altinitystable clickhouse
fi
$dc down clickhouse
fi
echo "${_endgroup}"