Repository navigation
fix(cnstore): require instance-bound lock drain before CN exit - #624
VioletQwQ-0 wants to merge 16 commits into
Conversation
7afa077 to
962ddb4
Compare
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Design and recovery review at head 02fbe0e38702bf8af304ec4ad721571cc73eb0b6.
[P1] A drain that exceeds StoreDrainTimeout cannot complete automatically, even after its dependencies disappear. OnPreparingStop reloads the persisted start timestamp and returns at controller.go:632-635 before handleConnectionDraining or handleLockMigration. For an attempt still in Requested when a remote transaction passes the deadline, every subsequent reconcile repeats that return. The transaction can later commit and QueryDrain can become safe, but the controller never asks again; the timeout test even asserts that this path makes no lock RPC. Retaining deletion protection is correct. Keep observing completion after the deadline while alerting on the overdue attempt, or define an explicit recoverable transition. Add a deterministic test: held remote transaction -> deadline -> commit -> same Pod/attempt eventually authorizes completion without unsafe deletion.
[P2] GetLockServiceIdentity uses GetLockInfo solely to read the exact service ID (pkg/querycli/query.go:82-95). The CN handler always enumerates all local locks and copies their keys, holders, and waiters (matrixone/pkg/cnservice/server_query.go:390-425); IterLocks holds each local lock-table read lock while invoking that callback (matrixone/pkg/lockservice/service_observability.go:193-224). Each drain identity probe therefore costs work proportional to current locks and can contend with lock mutations. Please add a lightweight identity-only response path, preserving existing GetLockInfo behavior, and validate that it avoids lock enumeration.
The instance/attempt/allocator proof and persisted phase are sound safety boundaries. The recovery and deployment contract still needs to be explicit: a Requested attempt keeps its old allocator epoch after allocator replacement, so repeated QueryDrain cannot complete; the new Operator also requires a LockServiceID unavailable from old CN binaries. Document either a safe re-handshake or an operational recovery procedure, plus a mixed-version rollout order that does not rely on the legacy UUID boolean check. The current PR evidence does not cover real Operator/Kruise eviction or that version matrix. These are not reasons to relax the fail-closed deletion rule.
|
@XuPeng-SH CR update at 5d33fe3: StoreDrainTimeout now keeps deletion protection while continuing to observe the same drain attempt; a multi-round Observe regression covers a held remote transaction that commits after the deadline. GetLockServiceIdentity requests the new MO IdentityOnly response so CN identity lookup does not enumerate locks. The PR body now states fail-closed allocator-epoch recovery and MO-first mixed-version rollout; no UUID fallback or automatic force-delete was added. The dependency is pinned to companion MO 06bcdb8b2742. Local controller compilation is blocked by missing matching native headers; CI is running. Please review the code and the explicit remaining integration gates. |
|
Follow-up at 96aa8c6: the overdue-drain controller regression now accounts for the diagnostic RemainTxnCount call on the unsafe QueryDrain round, then verifies the same attempt reaches completion with a second QueryDrain after the held transaction releases. This changes only the test expectation; deletion protection and request order remain asserted. GOWORK=off cnstore package tests and the focused CGO_ENABLED=1 race test pass locally. New CI is running; the prior e2e job stopped while loading the OpenKruise hook image (unauthorized), before lifecycle tests. XuPeng-SH: the timeout and identity-only changes from the previous update remain ready for re-review; controlled cloud eviction and mixed-version rollout remain separate gates. |
|
Exact head 058e81a: |
## What type of PR is this? - [x] API-change - [x] BUG - [ ] Improvement - [x] Documentation - [ ] Feature - [x] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Kernel-side work for #29477; keep the issue open for Operator/cloud acceptance. Related: matrixorigin/matrixone-operator#623. Companion: [matrixone-operator#624](matrixorigin/matrixone-operator#624). ## What this PR does / why we need it: A CN with no client sessions can still serve locks held by transactions on another CN. An allocator lookup miss, a UUID matching a different incarnation, or a stale restart response must not authorize eviction. BeginDrain/QueryDrain bind completion to the exact lock-service incarnation, drain attempt, and allocator identity/epoch. Missing or mismatched proof fails closed; completion requires the CN's existing local drain checks and terminal allocator observation. The protocol preserves existing admission and cleanup ownership. Invalid allocator bindings are rejected before publication, allocation waiters are released on failure, and retries cannot reopen drain admission. After allocator state loss, a pending non-admitting binding requires a matching-epoch heartbeat; an already-draining CN can recover without returning to Enable. Terminal heartbeats containing active transactions remain rejected. Retired proofs are bounded and retries are idempotent. Transient timeout validation failures no longer leave an idle draining CN stuck behind negative retirement: BeginDrain reuses the pending non-admitting handshake, clears only obsolete retirement metadata, and preserves inactive-service and commit fences. Positive retirement proofs remain idempotent. IdentityOnly fetches the exact identity without enumerating locks; existing requests retain their lock-list behavior. ### Rebase and deployment contract Rebased onto main `f1137a6cac72852be2ab2e5078b684b98f23d96c`. The nine ordinary commits were replayed separately; merge-only recovery documentation and protocol guards were preserved explicitly. Drain RPC methods remain 23/24 and now require MORPC **v105**, preserving main's v102 vector-cache control, v103 distributed IVF PRE, and v104 typed JSON behavior. Generated bindings preserve upstream query cache fields, LockOptions.KeepRows, and the removed Result trace fields' reserved numbers 6–9. Generation uses the repository's lock gogofast/query gogofaster modes and is reproducible byte for byte. Acceptance is a **coordinated downtime upgrade**: stop CN/TN services, deploy compatible binaries, restart them, then enable the matching Operator client. Mixed-version and rolling upgrades are outside this release's acceptance scope. Version rejection and fail-closed proof handling still apply. This PR does not update the companion Operator's dependency pin; integration must use the rebased kernel revision. ### Prior rebase validation at `3856a3dae1` - Fresh native `make cgo` build passed. - Full normal tests passed for lockservice (90.934s), cnservice (9.776s), lockop (5.334s), pb/query, and defines, with `-short -tags matrixone_test -count=1 -vet=all`. - pb/lock has no package tests; it compiled successfully and its schema behavior passed consumer tests and the external wire QA. - Focused real RPC/instance-bound proof, allocator-loss recovery, stale admission, and remote-holder tests passed; capability tests reject versions 99–104 and accept v105. - Six-package go vet and repository-config golangci-lint, `make err-check`, and `git diff --check` passed. - External wire QA passed: existing vector-cache request fields coexist with IdentityOnly; old trace tags decode safely; reserved fields and KeepRows survive generation. - Full lockservice race tests passed (97.786s). Both changed capability tests also passed 100 repetitions each under race. - Separately configured **gpt-6.1-sol / xhigh** design and final overall review passed. Final reviewed revision: `3856a3dae1eaf7ff4d644d48d7ed5cdfc632b3d2`; no unresolved material blocker. Review reconciled complete ownership/state/wire/test closures and independently checked original-history and upstream preservation. ### Negative-retirement recovery at `b559459d73` The original allocator implementation fails the new real two-CN negative-retirement regression; allocator-loss is the passing control. The correction changes only BeginDrain recovery and adds no normal lock-path work, worker, or retry loop. Existing recovery tests now cover both failure modes, including held remote locks, stale heartbeats, non-admission, old-generation cleanup, retained named/persistent commit fences, and terminal positive-proof retries. A duplicate phase test and ordinary SQL atomicity cases that never invoked drain were removed; actual drain coverage remains in real RPC/two-CN tests. - Fresh native build and full lockservice normal tests passed (89.977s); final strengthened recovery matrix passed (0.275s). - Full lockservice race passed (90.581s). Recovery handshake and real remote-holder recovery each passed 100 exact race repetitions (7.823s / 22.807s). - Final go vet, repository-config golangci-lint v2.6.2, make err-check, and diff checks passed. - Separately configured gpt-6.1-sol / xhigh design and final overall review passed at `b559459d734c0a0e2c38fb2ed5b7dbc2ae399014`; no unresolved material blocker. ### CI upgrade-test fixture correction at `713f4bd3de` The previous CI run passed SCA, shared build, and multi-CN BVT, but failed `TestV408LoginRejectsAccountDroppedAfterAuthentication`: DROP ACCOUNT legitimately closed the borrowed authenticated session, and the test called MaybeUpgradeTenant after its owner had destroyed it. The test and frontend code were unchanged from the frozen PR base. This correction retains real AuthenticateUser, captures authenticated scalar values before DROP, and checks compensation twice through its stable real CN owner, CheckTenantUpgrade. Existing wrapper UT covers version gating/error propagation. Both ErrNotFound results, no successful-cache publication, and independent CN SQL liveness remain required. Query-registry removal before tenant mutation remains; account routine cleanup is unchanged. No production nil guard, retry, sleep, hook, or normal-query overhead is added. - The three related V408 integration tests passed normally (10.622s); the previously failing account-drop test passed under race (22.308s). - Incremental upgrade-package vet/lint (0 issues), make err-check, and diff checks passed. - Separately configured gpt-6.1-sol / xhigh design and final overall review passed at `713f4bd3de8f58eb24cbd6172a2b28b37906ba9c`. Drain implementation and its prior acceptance evidence remain unchanged. - Initial local startup attempts encountered an overlong Unix socket path; only the external test wrapper TMPDIR was shortened before the successful source-bound runs. No production change was made for that environment error. The current evidence is local Linux kernel validation. Live SQL distributed cases, real process-restart deployment, Operator/Kruise eviction, and cloud control-plane acceptance were not rerun for this rebase. They remain separate integration gates; earlier experiments on previous heads are not presented as current-head proof. GitHub CI will run on the pushed revision and is not claimed as passed. --------- Co-authored-by: XuPeng-SH <xupeng3112@163.com>
This branch has not been deployed
2 participants
What type of PR is this?
Which issue(s) this PR fixes:
Related: #623 (no automatic closure). Companion MatrixOne PR: matrixorigin/matrixone#29291, merged at
5e965dece9ea15fa5d73c9e9615c59fbde651aaa.What this PR does / why we need it:
A CN with no client sessions may still host locks needed by another CN. A UUID/boolean restart check cannot establish that its response belongs to the current process. This PR queries the running CN for its full lock-service ID, uses BeginDrain/QueryDrain with instance, attempt and allocator epoch, and binds completion to Pod UID, CN UUID, main-container ID/start time, lifecycle and upgrade revision.
The durable
Prepared → Requesting → Requested → CompletionAuthorizedattempt is validated using fresh API reads and optimistic concurrency. Direct DELETE carries UID/resourceVersion preconditions; delete failures and controller restarts preserve authorization only for the same instance. Upgrade recovery requires the expected revision, new main container and fresh health observation before business admission resumes. Missing identity, unsupported protocol, cancellation and instance changes retain protection. There is no legacy UUID boolean fallback or automatic force-delete.The drain deadline does not stop observing the same attempt. Identity discovery uses the MO IdentityOnly response, not lock enumeration. Allocator epoch loss remains fail-closed: establish a current compatible instance and a fresh valid attempt through the supported recovery path; do not clear annotations, force-delete or re-admit a drained process to bypass the guard.
Current dependency and setup update (
3d8a27f8fdae971fa66c8c8d6a8186b186635c80):github.com/matrixorigin/matrixone v0.7.1-0.20261004020908-5e965dece9ea. The personal-fork replacement has been removed. This is the immutable merged companion commit, not moving main. Native libraries are rebuilt from that exact module. MORPC drain methods require v102; upstream v101 remains the JSON capability.unauthorized, before lifecycle assertions. Its logs did not identify the failing command. Independent anonymous manifest reads of the configured Quay MinIO tag fail locally and on mo-55.RELEASE.2023-11-01T01-57-10ZMinIO source at official commit55e713db0a367f6cccee00af49f00c269e6ca619, instead of requiring that inaccessible registry image or silently upgrading MinIO. The build includes the upstream license. Only the kind fixture uses the local image; standalone examples remain unchanged.Validation at the current candidate:
./pkg/...tests; full cnstore/cnclaim ordinary andCGO_ENABLED=1 -race -count=1tests; mocli/querycli compilation.TestDrainRealAPIConcurrencyandTestUpgradeRecoveryRealAPIConflictsin ordinary and race runs, not skipped (Kubernetes 1.24.1 API server).GOWORK=off; relevant vet; manager build and--help; shell regression, syntax and ShellCheck; tidy/diff checks; fullGOWORK=off make verify, including generated/chart and clean-tree checks.cgo/libmo.aSHA-256:d60c29ef4ca45d850f5786cca55e090f85650150e0395b2b97fa6025eac7e695.evidence/operator-upstream-20261008.vRrnpW/in the task workspace. A real MinIO image build on mo-55 was stopped after roughly eight minutes of stalled public base-image downloads, before MinIO compilation (BLOCKED_ENVIRONMENT, not Docker/kind PASS). Only the task-owned build process was stopped; shared containers and the daemon were not changed. Fresh-head checks/e2e must complete before claiming CI PASS.Special notes for your reviewer:
QA required: yes. MO merge is complete, but merge and package tests do not establish rollout, eviction acceptance or QA. Deploy compatible CN/TN binaries before this Operator; old combinations without instance-level proof must block normal eviction.
Baseline CI e2e still runs MO 1.2.3. It is environment/controller coverage, not acceptance of this new drain protocol. Candidate-binary Kubernetes/Kruise controlled eviction, mixed-version binaries, unit-agent/cloud node release, shared-dev deployment and QA remain separate
NOT_RUNgates. A live Pod object alone does not prove the protected main container remains running. Direct external DELETE/Eviction is not converted into safe drain by OnDeleted.Earlier fixture/proof regressions are retained: valid Requested/Requesting records and unchanged-instance positive controls precede identity mutation; mismatched BeginDrain identity cannot persist Requested or authorize later QueryDrain. Previous package, process-level two-CN and pinned-fork results remain historical evidence, not exact-current integration results.
Additional documentation (e.g. design docs, usage docs, etc.):
Protocol: matrixorigin/matrixone#29291. Historical failure and cross-layer boundary: #623. Failed FULLTEXT2 runs and their databases are retained; none is resumed or combined with these results.