feat: SeiNode import existing PVC + ensure-data-pvc create-path fix#113
Open
feat: SeiNode import existing PVC + ensure-data-pvc create-path fix#113
Conversation
Implements the direction + LLD merged in #106 and #112. New spec field spec.dataVolume.import.pvcName (optional, immutable, DNS1123-validated) lets a SeiNode adopt a pre-existing PVC instead of provisioning one. The controller validates the referenced PVC against seven requirements (exists, not terminating, Bound, RWO, capacity >= mode default, underlying PV capacity consistent, PV not Failed) and never mutates it. Transient failures loop at the executor's TaskPollInterval; unrecoverable failures mark the plan Failed with a specific Reason. Tracked via the new ImportPVCReady Condition on SeiNode status. Reason strings are treated as a public alerting contract — renames are breaking changes. ensure-data-pvc now uses Get-then-Create on the create path. If a PVC with the expected name exists and is NOT controlled by this SeiNode, the task fails terminally (this is the #104 fix). If it IS controlled, treat as Complete (crash-recovery idempotence). Finalizer (deleteNodeDataPVC) skips PVC deletion when import is set — operator owns the storage lifecycle for imported volumes. RBAC adds get/list/watch on persistentvolumes for the PV consistency checks; no write verbs. Tests: 18 unit tests in internal/task/ensure_pvc_test.go covering create-path happy/crash-recovery/non-controlled-conflict and each of the seven import-path validation requirements in both transient and terminal classification. 5 integration tests in internal/controller/node/import_pvc_test.go covering end-to-end import reaches-Running, late-PVC-converges, terminal-marks-Failed, deletion-preserves-imported-PVC, and a regression guard for the non-import deletion path. Deferred to a follow-up: metrics and events from LLD §8. The feature is functionally complete without them; separate PR to keep this one focused. Closes #105. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Closes #105. Fixes #104 as part of the create-path refactor.
Implements the merged design:
Summary
New optional spec field `spec.dataVolume.import.pvcName` lets a SeiNode adopt a pre-existing PVC rather than provisioning one. The controller validates the referenced PVC against the seven requirements from the LLD and never mutates it. Transient failures loop at the executor's existing poll interval; unrecoverable failures mark the plan Failed with a specific `Reason`. Status surfaced via a new `ImportPVCReady` Condition on SeiNode.
The `ensure-data-pvc` create path is refactored to Get-then-Create, treating an existing non-controlled PVC as a terminal error rather than silently binding to it (fixes #104).
What changed
CRD / API types (`api/v1alpha1/seinode_types.go`, +generated)
Task (`internal/task/ensure_pvc.go`)
Planner (`internal/planner/planner.go`)
Finalizer (`internal/controller/node/controller.go`)
RBAC
Tests
Deliberately deferred
Deviations from the LLD (flagged)
Two small implementation decisions worth naming:
`statusDirty` flip for condition-only mutations. The reconciler's existing `statusDirty` flag gates the status flush. To ensure a condition-only change gets persisted, added a 3-line compare helper (`conditionsSnapshot` + `conditionsEqual`) that flips `statusDirty` when the condition list mutated. This is implementation plumbing not called out in the LLD but follows directly from "condition is set before flush — must trigger flush when it changes."
Transient API errors on Get. When `Get` returns an unexpected non-`NotFound` error (networking blip), classified as transient with `PVCNotFound` / `UnderlyingPVMissing` reason. The reason is slightly imprecise but behavior is correct (retry). Alternative would be to return the error and let the executor retry at a higher layer. Easy to flip if reviewers prefer.
Test plan
Manual migration context
This PR is what would let us do today's pacific-1 archive import as a first-class feature rather than as a workaround. The manual trick we used yesterday (pre-create PVC with matching name, rely on the `AlreadyExists` bug) becomes `spec.dataVolume.import.pvcName: data-archive-0-0` and the controller handles it properly.
🤖 Generated with Claude Code