-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat(dev): Use arm64 ClickHouse image on Apple arm64 machines #29081
Conversation
715b1da
to
4b0dd7c
Compare
1550afb
to
ef8b77e
Compare
ef8b77e
to
d796462
Compare
On an Apple M1 machine this can be tested with |
src/sentry/conf/server.py
Outdated
# altinity provides clickhouse support to other companies | ||
# Official support: https://github.com/ClickHouse/ClickHouse/issues/22222 | ||
# This image is build with this script https://gist.github.com/filimonov/5f9732909ff66d5d0a65b8283382590d | ||
"image": "altinity/clickhouse-server:21.6.1.6734-testing-arm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the earliest ClickHouse version that works with ARM? That's a big jump from 20.3 to 21.6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version they have are 20.10.1.4844-testing-arm
, 21.4.1.6307-testing-arm
and 21.6.1.6734-testing-arm
. I've downgraded to the 20.x series.
"pull": True, | ||
"ports": {"9000/tcp": 9000, "9009/tcp": 9009, "8123/tcp": 8123}, | ||
"ulimits": [{"name": "nofile", "soft": 262144, "hard": 262144}], | ||
"environment": {"MAX_MEMORY_USAGE_RATIO": "0.3"}, | ||
# The arm image does not properly load the MAX_MEMORY_USAGE_RATIO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not certain yet. I tried it manually with -e
, however, I believe the image does not accept the variable (i.e. MAX_MEMORY_USAGE_RATIO is not set as an ENV in the Docker image).
You can see in this screenshot that it is not defined in the image:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(i.e. MAX_MEMORY_USAGE_RATIO is not set as an ENV in the Docker image).
This is not relevant as you can pass anything via the -e
option and it will be set inside at runtime. I'd say we should investigate this (not in this PR tho, as a follow up)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did try as well manually with -e. Added it to my backlog.
@BYK @lynnagara Billy is out this week. Would you mind reviewing this? Thanks |
ClickHouse, currently, [does not provide an `arm64` Docker image for the clickhouse-server][issue]. Until they do, we can use the [experimental image that Altinity produces][image]. I believe this image is built by this [script][script] on an `aarch64` host. Fixes #28564 [issue]: ClickHouse/ClickHouse#22222 [image]: https://hub.docker.com/layers/altinity/clickhouse-server/21.6.1.6734-testing-arm/images/sha256-9a4516444fef9e0f11ee6b2de716d3b97b36bf05d9cc2d44c4596cfb0584dea6?context=explore [script]: https://gist.github.com/filimonov/5f9732909ff66d5d0a65b8283382590d
2fd876b
to
91b95d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly rejecting for the version mismatch.
src/sentry/conf/server.py
Outdated
"volumes": { | ||
"clickhouse_dist" | ||
if settings.SENTRY_DISTRIBUTED_CLICKHOUSE_TABLES | ||
else "clickhouse": {"bind": "/var/lib/clickhouse"}, | ||
"clickhouse_logs": {"bind": "/var/log/clickhouse-server"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this, especially as a bind mount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because I needed to extract the logs. I guess I could have attached to the container. I don't need it. I will remove it.
Co-authored-by: Burak Yigit Kaya <byk@sentry.io>
# altinity provides clickhouse support to other companies | ||
# Official support: https://github.com/ClickHouse/ClickHouse/issues/22222 | ||
# This image is build with this script https://gist.github.com/filimonov/5f9732909ff66d5d0a65b8283382590d | ||
"image": "yandex/clickhouse-server:20.3.9.70" | ||
if not APPLE_ARM64 | ||
else "altinity/clickhouse-server:20.10.1.4844-testing-arm", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to move the comment to line 1809 to avoid confusion:
# altinity provides clickhouse support to other companies | |
# Official support: https://github.com/ClickHouse/ClickHouse/issues/22222 | |
# This image is build with this script https://gist.github.com/filimonov/5f9732909ff66d5d0a65b8283382590d | |
"image": "yandex/clickhouse-server:20.3.9.70" | |
if not APPLE_ARM64 | |
else "altinity/clickhouse-server:20.10.1.4844-testing-arm", | |
"image": "yandex/clickhouse-server:20.3.9.70" | |
if not APPLE_ARM64 | |
# altinity provides clickhouse support to other companies | |
# Official support: https://github.com/ClickHouse/ClickHouse/issues/22222 | |
# This image is build with this script https://gist.github.com/filimonov/5f9732909ff66d5d0a65b8283382590d | |
else "altinity/clickhouse-server:20.10.1.4844-testing-arm", |
In #29081, we changed the arm ClickHouse container at the last moment without realizing that the older version has this `libunwind: Unsupported .eh_frame_hdr version` [issue][issue]. We upgrade to a newer version that does not have profiling enabled. [issue]: ClickHouse/ClickHouse#15638
In #29081, we changed the arm ClickHouse container at the last moment without realizing that the older version has this `libunwind: Unsupported .eh_frame_hdr version` [issue][issue]. We upgrade to a newer version that does not have profiling enabled. [issue]: ClickHouse/ClickHouse#15638
ClickHouse, currently, does not provide an
arm64
Docker image for the clickhouse-server.Until they do, we can use the experimental image that Altinity produces.
I believe this image is built by this script on an
aarch64
host.Fixes #28564