apply ClickHouse gold-view migrations via post-install Hook Job - #1480
Conversation
|
Warning Review limit reached
More reviews will be available in 46 minutes and 47 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 1c3a14ff94e3e591ae959c29ab18e8b1a825ce9d and da378de. 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds a Helm hook Job and helper scripts to run ClickHouse gold-view migrations over HTTP, removes ClickHouse setup from the ingestion bootstrap scripts, updates ingestion design docs, and adds local chart validation in the GitOps Makefile. ChangesClickHouse migration hook flow
Sequence Diagram(s)sequenceDiagram
participant Helm
participant clickhouse-migrate-job
participant apply-ch-migrations.sh
participant lib/ch-exec.sh
participant ClickHouse
Helm->>clickhouse-migrate-job: render post-install,post-upgrade hook
clickhouse-migrate-job->>apply-ch-migrations.sh: run bash /ingestion/scripts/apply-ch-migrations.sh
apply-ch-migrations.sh->>lib/ch-exec.sh: call run_ch for SQL statements
lib/ch-exec.sh->>ClickHouse: POST SQL over HTTP
ClickHouse-->>lib/ch-exec.sh: query result
apply-ch-migrations.sh->>ClickHouse: create databases and apply migrations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ook Job Since constructorfabric#1428 dropped the bundled ClickHouse StatefulSet (CH is now always external L2), nothing applied the gold-view migrations: the host init.sh path `kubectl exec`'d into a pod that no longer exists, and the clickhouse-init-svcdbs Hook only runs CREATE DATABASE. Both clusters lost their migrations. Add a `clickhouse-migrate` Helm Hook Job (post-install,post-upgrade) that runs apply-ch-migrations.sh in the toolbox image (which already bundles the SQL) and dials the external ClickHouse over its HTTP interface — no new image/CI needed. It creates the staging/silver/app databases + ADR-0007 placeholders, then applies scripts/migrations/*.sql in glob order. A failed migration fails (and, under gitops --rollback-on-failure, rolls back) the release — intentional; backoffLimit is low since SQL errors are deterministic. - lib/ch-exec.sh: HTTP-only run_ch/ch_table_exists (password via the X-ClickHouse-Key header, never on the command line). - create-bronze-placeholders.sh: sourced from the lib; DDL untouched. - init.sh / run-init.sh: strip the now-dead ClickHouse migration section (the kubectl-exec-into-bundled-CH path) — it targeted a retired StatefulSet. - values.yaml: clickhouse.runMigrations gate (default true). - DESIGN.md: realign the schema-migrations + DB-provisioning + password rotation docs to the external-L2 / Hook-Job model. - e2e migration_applier.py: repoint stale comments (parser logic unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
1c3a14f to
da378de
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/insight/templates/clickhouse-migrate-job.yaml`:
- Around line 60-63: The app.kubernetes.io/name label in the clickhouse-migrate
job can overflow Kubernetes’ 63-character limit because the existing use of
include "insight.fullname" is already truncated before appending the
-clickhouse-migrate suffix. Update the metadata.labels in the clickhouse-migrate
job template to derive the name from .Chart.Name or insight.labels, or apply
truncation after adding the suffix, while keeping the
app.kubernetes.io/component label unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66adba38-173e-40f3-a2e2-f696763868ed
📥 Commits
Reviewing files that changed from the base of the PR and between d031d8f and 1c3a14ff94e3e591ae959c29ab18e8b1a825ce9d.
📒 Files selected for processing (10)
charts/insight/templates/clickhouse-migrate-job.yamlcharts/insight/values.yamldeploy/gitops/Makefiledocs/domain/ingestion/specs/DESIGN.mdsrc/ingestion/run-init.shsrc/ingestion/scripts/apply-ch-migrations.shsrc/ingestion/scripts/create-bronze-placeholders.shsrc/ingestion/scripts/init.shsrc/ingestion/scripts/lib/ch-exec.shsrc/ingestion/tests/e2e/e2e_lib/migration_applier.py
2 participants
Problem
After #1428 dropped the bundled L2 subcharts (ClickHouse is now always external), the ClickHouse gold-view migrations (
src/ingestion/scripts/migrations/*.sql) stopped being applied on the gitops path:init.shpath applied them bykubectl exec-ing into the bundledinsight-clickhouseStatefulSet — which chart(insight): drop L2 subchart bundling (clickhouse/mariadb/redis/redpanda) #1428 deleted. It also had no automated trigger (its orchestrator was removed in cleanup: remove dev-up.sh path, helmfile/, and orphan install scripts #1431).clickhouse-init-svcdbsHook only runsCREATE DATABASE, not the migrations.Net result: both clusters lost their gold views, and nothing owned re-applying them on
helm install/upgrade.Fix
Add a
clickhouse-migrateHelm Hook Job (post-install,post-upgrade) that applies the migrations against the external ClickHouse — no new image or CI pipeline needed, because the existingtoolboximage already bundles the SQL.The Job runs
apply-ch-migrations.sh, which mirrors the oldinit.shcontract:CREATE DATABASEforstaging/silver/ app dbcreate-bronze-placeholders.sh— ADR-0007 placeholders so gold-viewCREATE VIEWtype-checks on a fresh clustermigrations/*.sqlin lexicographic orderAll over ClickHouse's HTTP interface (no pod to
kubectl execinto), via a smalllib/ch-exec.sh.Key decisions
post-install,post-upgrade: gold-view consumers (analytics-api) resolve view source tables lazily at query time, so views materialising shortly after pod startup is fine.--rollback-on-failuredefault, rolls back — the release.backoffLimitis low (2) since SQL errors are deterministic, not transient.CREATE OR REPLACE/IF NOT EXISTS— same idempotent contract as the legacy path.X-ClickHouse-Keyheader, notcurl -u.clickhouse.runMigrations(defaulttrue).Changes
charts/insight/templates/clickhouse-migrate-job.yaml— the Hook Job.src/ingestion/scripts/lib/ch-exec.sh— HTTPrun_ch/ch_table_exists.src/ingestion/scripts/apply-ch-migrations.sh— Job entrypoint.create-bronze-placeholders.sh— sourced from the lib; 700 lines of DDL untouched.init.sh/run-init.sh— strip the now-dead ClickHouse section (it targeted the retired StatefulSet).values.yaml—clickhouse.runMigrationsgate.docs/domain/ingestion/specs/DESIGN.md— realign schema-migration, DB-provisioning, and password-rotation docs to the external-L2 / Hook-Job model (incl. the parallel MariaDBmariadb-init-svcdbsprovisioning).migration_applier.py— repoint stale comments (parser logic unchanged).Validation
bash -non all scripts; offline unit-test of the HTTP statement parser (comments stripped, 59 statements parsed fromgold-views.sql).make validate-insight ENV=local→ 15/15 resources valid (kubeconform-strict); Job renders with correct hook annotations, env, andbackoffLimit: 2.ingestion.toolboxImagemust point at a toolbox build that includes the newscripts/lib/ch-exec.sh+apply-ch-migrations.sh. The CI toolbox rebuild handles that on merge; until then the Job would run an image without these files.Out of scope
run-init.shis otherwise orphaned (docs-only) and line 675 of the ingestion DESIGN still references the removedairbyte-toolkit/build-connector.sh— separate pre-existing drift, left for a connector-docs pass.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes