Skip to content

feat!: add bitnami clickhouse #1760

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

feat!: add bitnami clickhouse #1760

wants to merge 2 commits into from

Conversation

TartanLeGrand
Copy link
Contributor

@TartanLeGrand TartanLeGrand commented May 13, 2025 •
edited
Loading

This pull request refactors the ClickHouse integration in the Helm chart for Sentry, simplifying configuration and improving maintainability. Key changes include replacing embedded ClickHouse configuration with externalized settings, updating dependencies, and enabling additional Sentry features.

ClickHouse Integration Refactor:

  • Replaced embedded ClickHouse configuration with externalized settings, removing complex nested configurations like replicas, users, and Zookeeper dependencies (charts/sentry/values.yaml, [1] [2].
  • Updated helper templates to use externalized ClickHouse settings, such as service.ports and auth.username, instead of older nested configurations (charts/sentry/templates/_helper.tpl, [1] [2] [3] [4].

Dependency Updates:

  • Updated the ClickHouse chart dependency to a newer version and removed Zookeeper as a dependency (charts/sentry/Chart.yaml, charts/sentry/Chart.yamlL22-R24).

Sentry Feature Enhancements:

Configuration Simplifications:

Miscellaneous Improvements:

@TartanLeGrand TartanLeGrand changed the title feat: add bitnami clickhouse feat!: add bitnami clickhouse May 13, 2025
@TartanLeGrand TartanLeGrand force-pushed the feat/add-bitnami-clickhouse branch 8 times, most recently from 76a89cf to 5029c2a Compare May 15, 2025 14:50
@TartanLeGrand TartanLeGrand force-pushed the feat/add-bitnami-clickhouse branch from 0cd5122 to 4e00952 Compare May 22, 2025 09:47
@manishrawat1992
Copy link

@TartanLeGrand If we want to change clickhouse image, better to go with altinity clickhouse operator.
It is what sentry uses on their prod environment and it makes managing clickhouse a charm

@TartanLeGrand
Copy link
Contributor Author

@manishrawat1992 Ok, do you have the source ? 😄

@Mokto Mokto added the conflicts label Jun 2, 2025
@Mokto
Copy link
Contributor

Mokto commented Jun 2, 2025

👋 Hi, @TartanLeGrand,
I detected conflicts against the base branch 🙊
You'll want to sync 🔄 your branch with upstream!

@TartanLeGrand
Copy link
Contributor Author

Hello @Mokto, are you ok with this on bitnami or you are more ok with @manishrawat1992. 🤔🤔

@manishrawat1992
Copy link

@TartanLeGrand I install altinity clickhouse operator as a separate chart and use external clickhouse config to reference it.
It is better to go with bitnami clickhouse for this chart

@Mokto
Copy link
Contributor

Mokto commented Jun 3, 2025

I agree.

I do think using the clickhouse operator is much, much better but outside the scope of this repository.
I'm even wondering if we should keep supporting the external databases.

I've always wanted the chart to be a one click install, but let's be honest, it's far from far and needs a lot of tweaking.

@manishrawat1992
Copy link

manishrawat1992 commented Jun 3, 2025 •
edited
Loading

@Mokto External Datastores have been the best part of this chart, honestly managing all the dependencies on K8 would be a massive effort.

Providing a option for this will be extremely helpful.
#1773

It is not very hard to implement

@Mokto
Copy link
Contributor

Mokto commented Jun 3, 2025

It's hard to maintain though and we have the responsibility to make updates to database chart versions without breaking anything. It's really hard.

@manishrawat1992
Copy link

It's hard to maintain though and we have the responsibility to make updates to database chart versions without breaking anything. It's really hard.

Tried adding clickhouse operator. I fully support your argument. It has too many issues to justify usage here.
I feel bitnami/clickhouse is the way to go for this chart

@Mokto
Copy link
Contributor

Mokto commented Jun 12, 2025

Well my argument was also for bitnami/clickhouse

@TartanLeGrand
Copy link
Contributor Author

Ok @Mokto good for me too. I need more changes and we can migrate on this.

@manishrawat1992
Copy link

@TartanLeGrand
Can we limit this PR to clickhouse changes. It would simplify the review.

"Removed support for distributed ClickHouse configurations, simplifying the deployment to single-node mode."
This is a breaking change. Can I help fix this.

@TartanLeGrand
Copy link
Contributor Author

Hello 👋 @manishrawat1992 I rebase this an cleanup all changes.

@TartanLeGrand TartanLeGrand force-pushed the feat/add-bitnami-clickhouse branch from 4e00952 to daef296 Compare June 17, 2025 09:47
@Mokto Mokto removed the conflicts label Jun 17, 2025
@TartanLeGrand TartanLeGrand force-pushed the feat/add-bitnami-clickhouse branch from 07b7faf to 4b7bc0e Compare June 17, 2025 11:21
manishrawat1992
manishrawat1992 reviewed Jun 18, 2025
View reviewed changes
charts/sentry/templates/snuba/_helper-snuba.tpl Outdated
@@ -43,6 +43,8 @@ settings.py: |
"password": env("CLICKHOUSE_PASSWORD", ""),
"max_connections": int(os.environ.get("CLICKHOUSE_MAX_CONNECTIONS", 100)),
"database": env("CLICKHOUSE_DATABASE", "default"),
"cluster_name": env("CLICKHOUSE_CLUSTER_NAME", "default"),
Copy link

Choose a reason for hiding this comment

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

This is problematic, and breaks external clickhouse in case it is distributed. Please look at tail end of this map

manishrawat1992
manishrawat1992 reviewed Jun 18, 2025
View reviewed changes
charts/sentry/values.yaml Outdated
@@ -2134,49 +2134,50 @@ config:
disableWriteException: true

clickhouse:
image:
tag: 23.8.16-debian-12-r0
Copy link

Choose a reason for hiding this comment

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

Better use 24.8 now. Sentry has upgraded clickhouse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/getsentry/self-hosted/blob/master/docker-compose.yml#L182

@TartanLeGrand TartanLeGrand force-pushed the feat/add-bitnami-clickhouse branch from 7339b5d to 973b937 Compare July 13, 2025 22:46
@Mokto
Copy link
Contributor

Mokto commented Jul 21, 2025

👋 Hi, @TartanLeGrand,
I detected conflicts against the base branch 🙊
You'll want to sync 🔄 your branch with upstream!

@TartanLeGrand TartanLeGrand force-pushed the feat/add-bitnami-clickhouse branch from 973b937 to d13fade Compare July 24, 2025 09:04
@Mokto Mokto removed the conflicts label Jul 25, 2025
@Mokto
Copy link
Contributor

Mokto commented Jul 28, 2025

👋 Hi, @TartanLeGrand,
I detected conflicts against the base branch 🙊
You'll want to sync 🔄 your branch with upstream!

@TartanLeGrand TartanLeGrand force-pushed the feat/add-bitnami-clickhouse branch from 29e922e to 036efa0 Compare July 29, 2025 23:33
@Mokto Mokto removed the conflicts label Jul 30, 2025
@TartanLeGrand TartanLeGrand force-pushed the feat/add-bitnami-clickhouse branch from 8692fd8 to 2f4360b Compare July 31, 2025 11:40
@TartanLeGrand TartanLeGrand force-pushed the feat/add-bitnami-clickhouse branch from 46e7dfc to 8368102 Compare August 13, 2025 07:13
@TartanLeGrand TartanLeGrand marked this pull request as ready for review August 13, 2025 07:57
@TartanLeGrand
Copy link
Contributor Author

@Mokto and @manishrawat1992 it's ready to review. I want to add an doc for upgrade from old clickhouse based on :

https://github.com/Altinity/clickhouse-backup/blob/master/Examples.md#how-to-move-data-to-another-clickhouse-server

@Mokto
Copy link
Contributor

Mokto commented Aug 21, 2025

👋 Hi, @TartanLeGrand,
I detected conflicts against the base branch 🙊
You'll want to sync 🔄 your branch with upstream!

@Mokto Mokto added conflicts and removed conflicts labels Aug 28, 2025
@Mokto
Copy link
Contributor

Mokto commented Sep 1, 2025

👋 Hi, @TartanLeGrand,
I detected conflicts against the base branch 🙊
You'll want to sync 🔄 your branch with upstream!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@manishrawat1992 manishrawat1992 manishrawat1992 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
conflicts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TartanLeGrand @manishrawat1992 @Mokto