Wait, don't fail, when a tenant's password file hasn't been mounted yet - #1135
Merged
Conversation
Enabling Trino for an org projects its password onto the tenant Secret and creates the catalog in the same reconcile tick. The Trino pods read that Secret through a mounted volume, so for up to a kubelet sync period the file the catalog points at genuinely does not exist yet, and the coordinator rejects CREATE CATALOG with a configuration error. That window was reported as a failure. The org was stamped Failed with failed_at set and a Trino configuration error in status_message, and the tick logged "Trino provisioner reconcile failed" — for an org that goes Ready on its own a minute later, unaided. Observed in production while enabling an org by hand; it now matters more, because PostHog is about to opt every newly onboarded org into a cell, so this becomes the normal onboarding path rather than a rare manual one. Classify that one case as Pending with a reason, which is what the reconcile loop already does for every other "not ready yet" input. Getting the classification wrong in the other direction would silently downgrade a real failure to "still waiting", so the match is narrow. Trino's statement error is now a typed *TrinoStatementError carrying errorName and message rather than a flattened string, and the classifier matches the single exact sentence Trino emits, built from our own property name and our own computed path for THIS org. Catalog properties carry tenant-influenced values that Trino echoes back in configuration errors, so a looser match could be tripped by an org's own data — the trap isInstanceFatalError already documents for DuckDB's echoed query text. If Trino rewords the message the match stops firing and the org fails loudly again, which is today's behavior. TrinoStatementError.Error() reproduces the previous text verbatim, so logs and status messages are otherwise unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qyribmtBS2uxYrrM3pACb
Test Impact PlanDeterministic summary of how this PR changes tests, CI runners, and coverage-risk signals. Summary
Signals
Coverage risk: neutral or increased No coverage-reduction warnings detected. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Enabling Trino for an org projects its password onto the tenant Secret and creates the catalog in the same reconcile tick. The Trino pods read that Secret through a mounted volume, so for up to a kubelet sync period the file the catalog points at genuinely does not exist yet, and the coordinator rejects
CREATE CATALOG:That window was reported as a failure: the org was stamped
Failedwithfailed_atset and a Trino configuration error parked instatus_message, and the tick loggedTrino provisioner reconcile failed— for an org that goesReadyon its own about a minute later, unaided.This classifies that one case as
Pendingwith a reason, which is what the reconcile loop already does for every other "not ready yet" input (org has no managed warehouse row yet,waiting for the duckling to publish a metadata-store credential).Why now
Observed in production while enabling an org by hand. It matters more from here on: PostHog/posthog#91098 opts every newly onboarded org into a cell, so this stops being a rare manual path and becomes what onboarding normally does. Without this, routine onboarding writes a spurious
Failedstate and a WARN every time.Why the match is narrow
Getting this wrong in the other direction would silently downgrade a real failure to "still waiting", so:
*TrinoStatementErrorcarryingerrorName/errorType/message, instead of being flattened into a string at the point of creation. The classifier useserrors.As, so the type is authoritative — the same principleisInstanceFatalErrorfollows for DuckDB.That last point is deliberate. Catalog properties carry tenant-influenced values (bucket names, endpoints) that Trino echoes back in configuration errors, so a loose match could be tripped by an org's own data — the trap CLAUDE.md already documents for DuckDB echoing query text back into
INTERNAL Errormessages. There's a test for exactly that shape.If Trino ever rewords the message, the match stops firing and the org fails loudly again — today's behavior, not a new silent state.
TrinoStatementError.Error()reproduces the previous flattened text verbatim, so logs and status messages are otherwise unchanged.Tests
go test -tags kubernetes ./controlplane/provisioner/...— green.TestReconcile_TenantSecretMountLagIsPendingNotFailed— reproduces the production error verbatim. Asserts the tick returns no error at all (so the loop doesn't log a failure), the org isProvisioningwith the mount-lag reason,failed_atis not stamped, and it converges toReadyon the next tick once the file appears.TestTenantSecretNotMountedYetOnlyMatchesThisOrgsMissingPasswordFile— the guard rail. Another org's password file, a different property missing a different file, tenant data echoed back inside an unrelated config error, the same text as an untyped error, an unrelated 503, and nil all stay failures.No
tests/e2e-mw-dev/change: this touches Trino catalog reconcile classification, none of the activation-pipeline paths CLAUDE.md lists as obligating harness updates, and adds no columns.Pre-existing on
origin/mainand unrelated to this change: threecontrolplane/adminPostgres tests fail locally against a stale local schema (column "max_hot_idle_workers" ... does not exist). Verified by stashing this branch and re-running on a clean tree.