Skip to content

fix(cnstore): require instance-bound lock drain before CN exit - #624

Open
VioletQwQ-0 wants to merge 15 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/cn-safe-drain-operator-20260923
Open

VioletQwQ-0 wants to merge 15 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/cn-safe-drain-operator-20260923

Conversation

@VioletQwQ-0

@VioletQwQ-0 VioletQwQ-0 commented Sep 23, 2026 •

Copy link
Copy Markdown

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

Related: #623 (no automatic closure). Companion MatrixOne PR: matrixorigin/matrixone#29291.

What this PR does / why we need it:

A CN may have no client sessions while other CNs still depend on its lock service. The old UUID/boolean restart check cannot prove that the response belongs to the current process instance. This PR queries the running CN for its full lock-service ID, uses the instance/attempt/allocator-epoch BeginDrain/QueryDrain protocol, and binds completion to Pod UID, CN UUID, main-container ID/start time, lifecycle, and upgrade revision. The Prepared → Requesting → Requested → CompletionAuthorized attempt is durable and validated with fresh API reads; resourceVersion conflicts fail closed. Direct DELETE uses UID/resourceVersion preconditions. Deletion failures and controller restarts retain 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, or instance change leaves protection in place. A drain deadline keeps protection but does not stop observing the same attempt. There is no legacy UUID boolean fallback or automatic force-delete. CN identity discovery uses the companion MO IdentityOnly response rather than enumerating locks.

The pinned MatrixOne dependency is github.com/VioletQwQ-0/matrixone v0.7.1-0.20260930031329-b350c0c82e83, the immutable companion commit b350c0c82e83188abbed6df6c321f3113f4f4982 (including allocator-loss recovery and the main merge). Merge matrixorigin/matrixone#29291 first, replace this fork pin with the merged upstream commit, then rerun affected tests before merging this PR. The drain methods require MORPC v102; v101 remains the unrelated upstream JSON capability.

Historical CR update (058e81a652fa6dae4189c84add799901202ee895):

  • Timeout recovery and identity-only lookup from the previous update remain unchanged. The new commit fixes a controller regression test expectation: an unsafe QueryDrain also calls RemainTxnCount for diagnostics, so the test now asserts set → can → remain → can while retaining the same attempt and deletion protection until the later successful query.
  • On macOS arm64 with Go 1.26.4, GOWORK=off go test ./pkg/controllers/cnstore -count=1 -timeout=5m passed using headers and libraries from the local MatrixOne checkout; the focused CGO_ENABLED=1 go test -race case also passed. go test -list selected the case and git diff --check passed. Native-library provenance was not rebuilt or independently certified in this turn; these are local package results, not Kubernetes or mixed-binary validation.
  • The prior checks run passed package tests but failed the clean-worktree gate because go mod tidy removed two obsolete checksums for the previous MO fork version. Commit 058e81a records that tidy result; GOWORK=off go mod verify passed. Exact-head checks now passes. Exact-head e2e stopped for the third consecutive run while loading openkruise/kruise-helm-hook:v0.1.0 into kind with Docker unauthorized, before Operator lifecycle assertions ran. Treat e2e as BLOCKED_ENVIRONMENT, not a pass or a demonstrated lifecycle regression.
  • Allocator epoch/instance loss remains fail-closed. Recovery requires an operator to establish the current instance and a fresh compatible drain attempt; do not clear the annotation, force-delete, or re-admit the drained process to bypass the guard. Roll out compatible MO CN/TN binaries first, then this Operator; unsupported old combinations cannot safely pass normal eviction.
  • Real Operator/Kruise eviction, unit-agent/cloud node release, mixed-version binaries, shared-dev deployment, and QA remain NOT_RUN.

Current dependency / e2e repair (8f5126e446095939e65f5a59e2f889296724315f):

  • Pinned the current companion MO commit after merging main, including its allocator-epoch recovery and unique v102 gate. No absolute-path replacement or workspace is required; main and API module validation used GOWORK=off.
  • kind's old load docker-image invokes an unbounded docker save. The previous run stopped at that path after pulling the multi-platform Kruise hook. The helper now pulls/exports the node platform explicitly when Docker supports platform export and loads the resulting archive. Legacy single-platform Docker remains supported. Pull/export/load errors propagate without retry or success masking; a shell regression covering cached/uncached images, legacy Docker and each failure boundary runs in checks.
  • Local and Docker builds use the pinned MO source's top-level make cgo, restoring the executable permissions discarded by module ZIP extraction. Native inputs are not borrowed from another checkout. Local native build PASS; libmo.a SHA-256: 8be49b06f9ff4dc3eb6632cf0bb89e20826b0844909c09724bd336e6d64b5c01.
  • Local PASS: manager build and --help; selected tests nonempty; complete cnstore/cnclaim package tests and race (with real envtest assets); TestDrainRealAPIConcurrency and TestUpgradeRecoveryRealAPIConflicts executed in ordinary and race runs, not skipped; separate API module ordinary/race; relevant vet; script syntax and shellcheck; go mod tidy -diff; complete GOWORK=off make verify including generated/chart checks and clean-worktree gate.
  • Raw outputs are retained under evidence/cn-safe-drain-pr-sync-20260930.PJrMbp/ in the local task workspace. New-head checks/e2e must finish before claiming CI PASS. The CI e2e still uses MO 1.2.3 and is baseline environment/controller coverage, not an instance-bound drain acceptance test; real candidate-binary Kruise eviction, mixed versions, unit-agent/cloud paths and QA remain separate NOT_RUN gates.

Special notes for your reviewer:

Earlier candidate validation on mo-55 used an isolated directory, Go 1.26.6, matching MO native libraries, and Operator source commit 7b8e6142b6b484cb45ae1929777be86246848868; it has not all been rerun at the current head:

  • PASS: GOWORK=off CGO_ENABLED=1 go test -race -count=1 ./pkg/controllers/cnstore ./pkg/controllers/cnclaim ./pkg/mocli ./pkg/querycli (pinned-fork dependency; evidence/operator-pinned-race.log).
  • PASS: race envtest for TestDrainRealAPIConcurrency and TestUpgradeRecoveryRealAPIConflicts against a real Kubernetes API server (evidence/operator-pinned-envtest.log).
  • PASS: separate API-module race test, known-race detector control, relevant go vet, and git diff --check.
  • PASS: full Docker image build from the pinned module and isolated --network none --help launch (evidence/operator-docker-build-v3.log).

The previous head 02fbe0e38702bf8af304ec4ad721571cc73eb0b6 repaired the Requested/Requesting fixture validity and proof-mismatch coverage reported against bdde018. The production helper rejects mismatched service/attempt IDs and missing allocator identity/epoch before persisting Requested. The MatrixOne two-CN process test exercises the lock protocol, not this Operator/Kruise eviction lifecycle. QA required: yes.

Additional documentation (e.g. design docs, usage docs, etc.):

Protocol and merge order: matrixorigin/matrixone#29291. Historical issue: #623. No historical FULLTEXT2 run is resumed or combined with this evidence.

@VioletQwQ-0
VioletQwQ-0 force-pushed the codex/cn-safe-drain-operator-20260923 branch from 7afa077 to 962ddb4 Compare September 23, 2026 11:42
@VioletQwQ-0
VioletQwQ-0 marked this pull request as ready for review September 24, 2026 06:22
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@VioletQwQ-0

Copy link
Copy Markdown
Author

@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.

@VioletQwQ-0

Copy link
Copy Markdown
Author

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.

@VioletQwQ-0

Copy link
Copy Markdown
Author

Exact head 058e81a: checks is PASS after the test expectation and go.sum tidy fixes. e2e remains BLOCKED_ENVIRONMENT: three consecutive runs (jobs 108772913054, 108814042049, 108816252542) stop at kind load docker-image for openkruise/kruise-helm-hook:v0.1.0 with Docker unauthorized, after the host pull succeeds and before any Operator lifecycle assertions. Please have the CI/image-store owner inspect this kind/Docker load path; I have not weakened the test or claimed e2e passed. The requested code re-review can proceed independently, but merge/deployment validation still needs a real e2e run.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants