Redo the semantic domain import until it finishes - #4355
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4355 +/- ##
=======================================
Coverage 75.96% 75.96%
=======================================
Files 305 305
Lines 11384 11384
Branches 1411 1411
=======================================
Hits 8648 8648
Misses 2332 2332
Partials 404 404
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Delivery note — publish the release before building an installer
In that pairing this breaks hard rather than degrading.
Two ways out:
Option 2 is a one-line change to this PR and removes the ordering constraint entirely, which is the way I would lean. Update: Option 2 applied. |
The database pod's postStart hook imported the semantic domains when either collection was empty, so an import killed part way through left them non-empty but incomplete and was never redone. Record a finished import instead, and only when it finishes. Add a readiness probe on a marker the hook writes, so the pod stays out of the database Service until the replica set advertises the current pod IP, which changes on every restart and which the backend needs since it connects with ?replicaSet=rs0. The hook now appends to its log rather than truncating it, and stamps each start, so that earlier starts survive a restart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The completion record was written by the postStart hook, so the documented manual rerun of update-semantic-domains.sh left it untouched and the next pod start redid the whole import. Write it from the script instead, after both imports succeed, so both paths agree. Also trim /data/db/postStart.log to the most recent starts. It is appended to on every container start and lives on the database's persistent volume, with nothing rotating it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The readiness marker was written before the semantic domain import so that the probe could pass while the import ran, but the kubelet does not probe a container until its postStart hook returns, and the import runs inside that hook. The pod could not join the database Service any sooner, so write the marker last, where it also means that everything before it succeeded. Report a failed import in the postStart log and fail the hook explicitly, so the container restarts and retries: The Combine cannot be used without the semantic domains, and a database that serves without them looks healthy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- The completion record outlives the pod that wrote it, so a release that ships updated semantic domain data needs the manual import too. - The postStart log is trimmed to 200 lines rather than to whole entries, so its oldest entry can begin part way through. Comments now use one space after a period rather than two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The postStart hook captured mongosh's output and imported only when it was exactly "needed", so a stray line on stdout left the pod ready with no semantic domains, and a failed query aborted the hook through set -e with nothing in the log to say why. Branch on mongosh's exit status instead: anything short of a completed import runs the import, which is a merge and safe to repeat, and only stdout is discarded so an error still reaches the postStart log. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The completion record is in the database's persistent volume, so it outlives the pod that wrote it. An installation that imported before the record existed has none, so its first pod start on this chart imports once more and stays out of the database Service until it is done. A timeout given to the installer therefore has to cover that import, since the database is not available until its postStart hook has finished it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The record is written by update-semantic-domains.sh, which is in the database image, while the hook that reads it is in the chart, which ships in the installer. An installer built from master before the next release pairs this chart with the previous release's image, whose script does not write the record, so nothing ever would: the import would be redone on every start, and the installer's wait for the record would never pass. Record it from the hook too, after the script succeeds. Both writes are the same upsert, and each covers a case the other does not: the script's is what makes a manual rerun count, the hook's is what makes an older image work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a025d47 to
7191cb2
Compare
Split out of #4352, part 2 of 4 — see
#4352 (comment) for the split and how it was verified.
The database pod's
postStarthook imported the semantic domains whenever either collection was empty. A count cannottell a finished import from one killed part way through, so an interrupted import left the collections non-empty but
incomplete and was never redone.
CombineDatabase.SemanticDomainImportStatus, written only when theimport finishes. The record is read by exit status, so anything short of a completed import imports again — a merge,
and safe to repeat.
update-semantic-domains.shrather than from the hook, so that the documented manual reruncounts too and does not leave the next pod start redoing the whole import.
semantic domains, and a database that serves without them looks healthy.
databaseService until thereplica set advertises the current pod IP — which changes on every restart, and which the backend needs since it
connects with
?replicaSet=rs0./data/db/postStart.loginstead of truncating it, stamp each start, and trim to the last 200 lines, sincethe log is on the database's persistent volume with nothing rotating it.
Scope
Four files, none of them touched by parts 1 or 4:
database/init/update-semantic-domains.shdeploy/helm/thecombine/charts/database/templates/database.yamldocs/deploy/README.mdtimeoutrow ofinstaller/README.mdThat last one is a deliberate move from where the split analysis put it. It documents that the installer's timeout has
to cover the semantic domain import, which is a consequence of the readiness probe here, so it belongs with the change
that causes it rather than with part 4. #4353 changes the
updaterow of the same table; the two are verifiedconflict-free, as is this branch against #4353 and part 1 as a whole.
QA and prod
Affected, and this is the part to review with prod in mind. The chart template has no target conditionals and
updateStrategyisRecreateon every profile, so on the first upgrade to this chart the database pod is replaced andthen held out of the
databaseService by the new readiness probe until itspostStarthook finishes. An existing QAor prod installation has no completion record, so that hook imports the semantic domains once more: several minutes
with no
databaseendpoint.QA and prod deploys run
setup_combine.pythrough.github/actions/combine-deploy-updatewithout--wait, so helmitself does not block on the import — but the endpoint gap is real and worth planning around. The manual rerun,
kubectl -n thecombine exec deployment/database -- /opt/thecombine/update-semantic-domains.sh, now also writes thecompletion record.
Testing
Not yet exercised on hardware. Worth covering both a fresh install, where the import runs inside the hook, and an
upgrade of an installation that already has the domains but no record, since that is the case QA and prod will hit
first.
🤖 Generated with Claude Code
This change is