Fix/laptop install first boot - #4352
Conversation
The database gained a postStart hook that initializes the rs0 replica set and imports the semantic domains, so it is now the slowest deployment to come up. Nothing in the install path waited for it, and the installer then stopped k3s, which SIGKILLs pods because the k3s unit is patched to KillMode=mixed. wait-for-combine only ever read one deployment's READY count. Collapsing the kubectl table with an unquoted expansion put every row on one line, so grep matched the whole line for each name and the greedy sed captured the last N/1 in it -- the alphabetically last deployment, maintenance. Replace the parsing with kubectl wait, and check the database first. Guard the semantic domain import with a completion record instead of a document count. A count cannot tell a finished import from one killed part way through, so an interrupted import was never redone and the domains stayed incomplete. The installer now also waits for that record before shutting the cluster down. Add a readiness probe so the database stays out of its Service endpoints 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. Drain the deployments in combinectl stop, restoring their replica counts in combinectl start, so pods terminate instead of being killed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nginx resolves the hostnames in its proxy_pass directives when it loads its configuration and exits if any of them cannot be resolved: [emerg] host not found in upstream "backend" in /etc/nginx/conf.d/default.conf:48 On a cold start the frontend container can be running before the cluster DNS can answer for the backend service, which leaves the pod crash looping until the kubelet's restart backoff happens to line up with DNS being ready. Wait for the name in a docker-entrypoint.d script, and start nginx anyway after two minutes so that a real misconfiguration is still reported rather than leaving a container that runs but never serves. Add a readiness probe to the frontend, since with the wait in place a running container is no longer necessarily a serving one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
documentation
|
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 #4352 +/- ##
=======================================
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:
|
Every other step that records a follow-on state checks SINGLE_STEP before continuing, but Wait-for-combine fell straight through into Shutdown-combine, so single-step could not be used to inspect a running cluster before the installer stopped it. Setting STATE directly rather than calling next-state leaves the recorded state in place, so the next run resumes at Shutdown-combine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"The Combine is Running." was printed whenever the k3s unit was active, which covers several states that are not running. Distinguish them: - the k3s unit is active but the Kubernetes API is not serving yet - the cluster is up but the namespace has no deployments - some of The Combine's deployments are missing - the deployments exist but are scaled down - the deployments are scaled up but not all available yet The scaled down case is the one combinectl stop now creates, and it would otherwise have been reported as running with every deployment at 0/0. Restoring the replica counts also no longer depends on the saved counts being present. k3s can be started without combinectl -- directly with systemctl, or by the unit still being enabled on a server install -- which would otherwise leave the deployments scaled to zero with nothing to bring them back, so fall back to one replica each. Guard the wait for the pods to exit with a check that there are pods to wait for; a selector-based "kubectl wait" fails immediately when nothing matches it, which reported a clean stop as a two-minute timeout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"kubectl wait" returns at once, rather than blocking for its timeout, when the API server is unreachable or the deployment is gone, so its retry loop had nothing but an echo in it and could spin, pegging a core and flooding the terminal. Pace the retries. Neither wait had an upper bound either, so a deployment that was never going to come up produced no error, ever. Give each stage a deadline, default one hour and overridable with WAIT_TIMEOUT_SECONDS, and print the pods in the namespace before exiting so there is somewhere to start looking. Rerunning the installer resumes the wait. Check the semantic domain import every 30 seconds rather than every 10; each check starts a mongosh inside the database container, competing with the import it is waiting on. 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>
start-combine-deployments replayed the saved counts as they were, so a count for a deployment that no longer exists, for example one renamed by a chart update, failed to scale on every start. That failure is what keeps the file, so the stale entry, and any deployment the file omits, were retried forever. Take the deployments that are scaled down from the cluster and the counts from the file, falling back to one replica for a deployment the file does not mention, and discard the file once nothing is scaled down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
combine-deployments discards kubectl's errors, so a cluster that is not serving the API yet looks exactly like an empty namespace. stop-combine-deployments read that as "nothing to stop" and returned without saving any replica counts or scaling anything down, and combine-stop went straight on to stop k3s, which with KillMode=mixed SIGKILLs the containers. A stop issued while The Combine was still coming up therefore killed the database part way through its startup work, which is what forces the semantic domain import to be redone. Wait for the API in the stop path as the start path already does, and leave k3s running when the deployments could not be stopped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A deployment scaled to zero has every replica it asked for, so it did not land in the pending list, and it is in the namespace, so it was not missing either. With the other deployments still asking for replicas the total was non-zero, and the status came out as "The Combine is Running" even though one of its services was switched off. Count the deployments that ask for no replicas separately and name them, so that a service left behind by a partial start is visible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One deadline covers the wait for all four deployments, so "Timed out after 3600s waiting for deployment/maintenance" claimed a wait that had mostly elapsed on the database. Say what the wait was still waiting for instead, and describe the timeout in the header comment as the budget for the deployments and then for the import, rather than one per deployment. The semantic domain import runs from the database pod's postStart hook, so rerunning the installer does not start an import that never ran; give that wait a hint that names the manual import instead of the default advice. 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>
combinectl start returned the status of an echo when the cluster never became ready, so it exited 0 after starting nothing, and a deployment that could not be scaled up was reported the same way. Return non-zero from both, as stop-combine-deployments already does, and make the other exits explicit. Correct what the comments and docs claim: - The database is not available until its postStart hook has imported the semantic domains, so the import elapses inside the wait for the deployments, not inside the wait that follows it. That second wait is a guard for a database pod that an install left running, which therefore never ran the hook. - 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. - A new cluster-internal nginx upstream has to be added to the DNS wait, which only knows about "backend". Comments now use one space after a period rather than two, and some blocks are rewrapped to the width of the file around them. 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, as the installer already does: 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>
wait-for-semantic-domains had the same hour-long budget as the wait for the deployments, but readiness is gated on the same record, so it either passes on its first check or is in the case where no pod will ever write the record. That spent an hour polling before printing the manual-import hint that is the actual remedy; give it a short budget of its own. check-wait-deadline now reports the budget of the wait that set it, so the two cannot drift. WAIT_TIMEOUT_SECONDS went into arithmetic unchecked, where bash reads a non-number as 0 and a leading zero as octal: "abc" became a deadline that had already passed, "007" became seven seconds, and "3600s" aborted with a bare arithmetic error. Require a whole number greater than zero, as the timeout option already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
stop-combine-deployments claimed to return non-zero whenever the deployments were not stopped, but a pod still terminating after two minutes warns and returns 0 so that one stuck pod cannot leave The Combine with no way to stop. Say what each path does. The database is not ready until its postStart hook has imported the semantic domains, which has two consequences worth writing down: a timeout given to the installer makes helm wait for that import, so it has to be long enough to cover it; and an installation that imported before the completion record existed has none, so its first pod start imports once more and stays out of the database Service until it is done. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WAIT_TIMEOUT_SECONDS was checked at the top of the script, so a bad value left in the environment stopped an uninstall, which never waits for anything. Check it with the other install-only setup, where the state is known, as the version number already is. Also note that the support software the installer's update option skips includes combinectl itself, so a change to that tool arrives with a full installation and not with an update. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Splitting this PR: four stand-alone PRs, no conflictsI built the split locally and tested it rather than eyeballing it: all 6 pairwise merges are clean, four-way Everything is file-disjoint except two small overlaps, in 1. Wait for cluster DNS before starting nginx
From QA & prod: affected. Both changes live in the frontend image and the shared frontend chart, with no target 2. Database first boot: import record, postStart hook, readiness probe
From the database parts of QA & prod: affected, and this is the one to review with prod in mind. The chart template has no target 3. combinectl start/stop/status
From the combinectl parts of QA & prod: no effect on the server clusters. The 4. Installer waits
From the wait rewrite in QA & prod: none. Ordering (functional, not textual)
So 1 and 3 can go in any order at any time; 2 before 4. Cost of splitting
|
|
Superseded by four stand-alone PRs, each verified to merge without conflict in any order:
Split rationale and verification: #4352 (comment) |
1 participant
Devin: https://app.devin.ai/review/sillsdev/TheCombine/pull/4352
This change is