feat(migrate): LINSTOR→blockstor migration converter + adoption-safety fixes#180
feat(migrate): LINSTOR→blockstor migration converter + adoption-safety fixes#180Andrei Kvapil (kvaps) wants to merge 18 commits into
Conversation
pkg/linstormigrate reads the *.internal.linstor.linbit.com CRD dumps (LINSTOR's k8s-connector database tables) into typed row structs — the input side of the LINSTOR->blockstor migration converter. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Converts the loaded LINSTOR k8s-backend tables into blockstor v1alpha1 manifests: - Node/StoragePool/ResourceGroup/ResourceDefinition/Resource/Snapshot with names normalized through the store's Name() helper and the original spelling preserved in the blockstor.io/linstor-name annotation. - Props routed through the store's own three-way split (typed DRBDOptions / Props / ExtraProps) via a new exported SplitProps shim, so migrated objects read back exactly like REST-created ones. - Adoption safety: RD.Spec.Initialized=true and per-replica skipInitialSync=false so post-migration replicas always SyncTarget real data; per-volume DRBD minors, per-replica node-ids and the RD TCP port are carried verbatim (clusterIP-style presets). - The per-RD DRBD shared-secret is carried as the DrbdOptions/Net/shared-secret prop the satellite already renders. - Flag bitmasks are decoded via empirically calibrated bit values (dump vs live REST cross-reference); unknown bits and DELETE'd / FAILED_DEPLOYMENT rows are skipped and reported, never guessed. - Deterministic multi-doc YAML in apply order on stdout or -out, migration report on stderr. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The fixture models the table shapes observed in two production LINSTOR k8s-backend dumps (layer joins, JSON-array-in-string columns, flag bitmask populations, props instance paths) with fully fabricated identifiers, IPs, secrets and ciphertexts. Pinned behaviours, each proven fail-on-bug against the pre-fix converter where applicable: - TIE_BREAKER decode requires the DISKLESS bit: the witness keeps its flags even though LINSTOR writes no LAYER_STORAGE_VOLUMES row for diskless replicas (the original implementation lost TIE_BREAKER there), while a stale TIE_BREAKER bit on a diskful replica (an auto-diskful leftover seen in production) is dropped and reported. - Adoption safety latches: every RD converts Initialized=true, every replica skipInitialSync=false. - DRBD identity carried verbatim: per-volume minors, per-replica node-ids, the RD TCP port and the net shared-secret prop. - Never-guess policy: DELETE'd RDs and FAILED_DEPLOYMENT / in-flight snapshots are skipped with report lines. - Full manifest + report golden files pin the output stream. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
LAYER_LUKS_VOLUMES passphrases are encrypted with the LINSTOR master key; decrypting them needs the operator's master passphrase and LINSTOR's KDF. Until that lands, every LUKS-backed RD is converted with its layer stack intact and a loud per-RD report line telling the operator to provision spec.encryption manually before adopting. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
- ControllerConfig: cluster-wide /CTRL DrbdOptions/* (auto tiebreaker, verify-alg allow-list, auto-evict) now distill into the blockstor ControllerConfig singleton instead of being silently lost; LINSTOR runtime plumbing (NetCom ports, Cluster/LocalID) and master-key crypto material are excluded by construction, pinned by test. - Multi-volume RDs (observed in production with NON-contiguous DRBD minors) are covered by fixture + test: each volume slot carries its own minor verbatim. - Typed routing pinned for protocol A (resource group) and allow-two-primaries / RWX (RD). - Suffixed DRBD layer instances (external metadata) and operator-created linstor remotes now warn instead of silently dropping; the auto-generated self-remote stays silent. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
A Snapshot CRD imported by a migration tool arrives with SuspendIO=false/TakeSnapshot=false and an EMPTY Status. nextPhase's false/false branch read that as 'orchestration not started yet' and flipped SuspendIO=true: every diskful peer of the parent RD would freeze production I/O (drbdsetup suspend-io) just to re-take a snapshot that already exists on disk — one freeze per imported snapshot across the whole cluster. New blockstor.io/adopted annotation short-circuits Reconcile before any orchestration: the controller backfills the terminal per-node Ready=true state (CreateTimestamp from the optional blockstor.io/adopted-created-at ms-epoch annotation, so 'linstor s l' keeps the real snapshot age) and never touches the Spec flags. The satellite is a no-op on such objects by existing phase gates. linstor-migrate stamps both annotations on every converted snapshot, using the newest per-node create_timestamp from the dump. Proven fail-on-bug: with the gate disabled, the new envtest spec records 'advancing orchestration phase suspendIO=true' on an adopted Snapshot and fails; with the gate it passes 30/30. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
LINSTOR 1.33 stores port=-1 in its database for most RDs — the DRBD listen port is assigned at runtime and not persisted, so the dump's tcp_port is empty for nearly every resource while the kernel mesh is really listening on a concrete port (7000-701x on the validated cluster). If blockstor adopts with a DIFFERENT port, the satellite's drbdadm adjust changes the connection endpoint under live I/O: a reconnect blip, and on a marginal quorum a transient quorum loss. New -drbd-ports <file> (ParseDRBDPorts) takes a '<rd-name> <port>' map captured from the running kernel and presets it on every RD and replica, so adoption's adjust is a no-op on the endpoint. The live map wins over the dump; when a port is in NEITHER source the replicas get nil (blockstor allocates a fresh port) and the gap is reported once per RD so the operator can capture ports or accept the blip. Tests pin: live map overrides dump, missing port → nil + single report line, and the port-file parser incl. malformed-line rejection. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Cutover procedure the converter is only one step of: pre-flight (health check, DB dump, live DRBD-port capture, pool snapshot), convert, ordered control-plane swap (freeze CSI, stop linstor controller, deploy blockstor, apply manifests, converge, repoint CSI), per-object verification, control-plane-only rollback, and the explicit known-limitations table (LUKS, ports, dropped flag bits, placement). Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Boots envtest, installs blockstor's real CRDs and server-side-applies
every converted object, so the migrator's output is proven against
the actual OpenAPI schema AND every CEL XValidation rule (composite
<rd>.<node> / <pool>.<node> / <rd>.<snap> names, settable-once
drbdPort/drbdNodeID/initialized, enums) — a Go struct build cannot
catch a CEL violation, only the apiserver can.
The synthetic fixture always runs; the two production dumps are also
validated when present at /tmp/{infra,hidora} (or $LINSTOR_DUMP_DIRS),
skipped otherwise so their real secrets never enter the repo. Local
run: infra 254/254, hidora 2113/2113 objects applied with zero
validation errors.
Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
A replica whose parent RD was skipped (DELETE'd/absent) or whose host node did not convert would previously emit an orphan Resource: its <rd>.<node> CRD references an object that is never applied. Same for a snapshot whose parent RD didn't convert. Adopting such a dangling object risks the controller/satellite mishandling a resource with no definition. The converter now tracks the set of migrated RDs and nodes and skips any replica/snapshot that would dangle, reporting each drop. The two production dumps contain no such rows (verified), so their converted object counts are unchanged and still pass server-side CRD validation (254/254, 2113/2113). Fail-on-bug test: a fixture replica of a DELETE'd RD must not convert. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Symmetry with the replica/snapshot referential-integrity guard: a StoragePool on a node the converter skipped (e.g. a CONTROLLER-only node) is dead config; drop and report it. No such pools exist in the production dumps, so counts are unchanged. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
BLOCKER found by the production release gate. A real LINSTOR >=1.x database (and its k8s-backend dump) stores the zpool name under the generic StorDriver/StorPoolName, leaving StorDriver/ZPool and StorDriver/ZPoolThin blank. Verified on the live aenix-infra cluster: `linstor sp l` shows StorPoolName=data, ZPool/ZPoolThin empty, and the node's real zpool is named data. blockstor's newZFS only read the two ZPool keys, so a migrated ZFS StoragePool never registered a provider -> every diskful resource on it failed the Providers[pool] lookup -> NOTHING adopted. It was a loud, safe failure, but the migration could not succeed. The synthetic migrator fixture masked it by using StorDriver/ZPoolThin (a key blockstor already read), so the whole suite was green while prod was broken. Fix: newZFS accepts StorDriver/StorPoolName as the final fallback for both ZFS and ZFS_THIN. Regressions: - pkg/satellite/factory_test.go: the exact prod props register a provider (fails on the pre-fix factory). - the migrator fixture is reshaped to the real prod pool (ZFS thick + StorPoolName, no ZPool key), and a new migration-layer test runs the converted StoragePool props through the real NewProviderFromKind so the mask cannot return. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Pre-flight now requires verifying blockstor's computed zvol names (<zpool>/<rd-lowercased>_<vol%05d>) byte-match the on-disk datasets — critical for single-replica STORAGE-only volumes, which have no DRBD resync fallback if the name misses — plus a full staging rehearsal before any production cutover. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Adoption is name-based: blockstor addresses each zvol as <zpool>/<RD metadata.name>_<vol%05d> (zfs.go volumeDataset) and only adopts an existing dataset. Every production RD name across both dumps is a clean pvc-<uuid> (113/113 + 679/679), which lowercases to an RFC-1123-clean name that k8sstore.Name passes through unchanged — so the converted metadata.name byte-matches LINSTOR's on-disk <zpool>/pvc-<uuid>_00000. The test pins that verbatim-lowercase invariant (fixture RDs + a real uppercase UUID); a future change that hashed a clean name would make single-replica STORAGE-only volumes adopt an empty disk, and this catches it. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughAdds a LINSTOR-to-Blockstor migration package and CLI, adopted Snapshot reconciliation, ZFS pool-name fallback handling, migration documentation, and comprehensive fixture, golden, unit, and envtest validation. ChangesLINSTOR migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant linstor-migrate
participant linstormigrate
participant BlockstorAPI
Operator->>linstor-migrate: Provide dump and optional DRBD ports
linstor-migrate->>linstormigrate: Load and convert dump
linstormigrate-->>linstor-migrate: Return manifests and warnings
linstor-migrate->>BlockstorAPI: Apply generated manifests
BlockstorAPI-->>Operator: Report validation and adoption state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the linstor-migrate tool to convert LINSTOR database dumps into blockstor CRD manifests, enabling seamless adoption of existing on-disk data. It includes the core conversion logic, a command-line interface, a comprehensive runbook, and unit/integration tests. Additionally, the snapshot controller is updated to recognize adopted snapshots and bypass standard orchestration to prevent production I/O freezes, and a fallback is added to the ZFS provider factory to support StorDriver/StorPoolName for ZFS pool name resolution. Feedback points out a critical issue in the converter where reference fields are assigned raw, non-normalized names (often uppercase or mixed-case in LINSTOR) while the generated objects use normalized lowercase names, which would cause Kubernetes controllers to fail to resolve references.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func (c *converter) displayNode(name string) string { | ||
| return displayName(c.nodeDsp[name], name) | ||
| } | ||
|
|
||
| func (c *converter) displayRD(name string) string { | ||
| return displayName(c.rdDsp[name], name) | ||
| } | ||
|
|
||
| func (c *converter) displayRG(name string) string { | ||
| if name == "" { | ||
| return "" | ||
| } | ||
|
|
||
| return displayName(c.rgDsp[name], name) | ||
| } |
There was a problem hiding this comment.
In LINSTOR, resource, node, and resource group names are often uppercase or mixed-case (e.g., NODE-A, SC-REPLICATED, PVC-VOL1). While the converter normalizes the metadata.name of the generated objects to lowercase/RFC-1123 compliant names (via objectMeta which calls k8sstore.Name), the reference fields in other specs (such as StoragePoolSpec.NodeName, ResourceSpec.NodeName, ResourceSpec.ResourceDefinitionName, ResourceDefinitionSpec.ResourceGroupName, and SnapshotSpec.ResourceDefinitionName) are assigned the raw, non-normalized names returned by displayNode, displayRD, and displayRG.
This case mismatch will cause Kubernetes controllers to fail to resolve these references (e.g., looking for NODE-A instead of node-a), leading to NotFound errors and adoption failures in production environments where display names are not explicitly lowercase.
We can elegantly solve this across the entire converter by normalizing the output of displayNode, displayRD, and displayRG using k8sstore.Name.
| func (c *converter) displayNode(name string) string { | |
| return displayName(c.nodeDsp[name], name) | |
| } | |
| func (c *converter) displayRD(name string) string { | |
| return displayName(c.rdDsp[name], name) | |
| } | |
| func (c *converter) displayRG(name string) string { | |
| if name == "" { | |
| return "" | |
| } | |
| return displayName(c.rgDsp[name], name) | |
| } | |
| func (c *converter) displayNode(name string) string { | |
| return k8sstore.Name(displayName(c.nodeDsp[name], name)) | |
| } | |
| func (c *converter) displayRD(name string) string { | |
| return k8sstore.Name(displayName(c.rdDsp[name], name)) | |
| } | |
| func (c *converter) displayRG(name string) string { | |
| if name == "" { | |
| return "" | |
| } | |
| return k8sstore.Name(displayName(c.rgDsp[name], name)) | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/controller/snapshot_adopted_test.go (1)
61-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting unexpected errors and existing state in test setup.
Currently, if
k8sClient.Getreturns an unexpected error (e.g., connection failure) or if the object already exists (err == nil), the test silently skips creation. This could lead to confusing failures later in the test or flaky behavior from reusing stale state.Consider explicitly asserting the expected
NotFounderror to ensure a clean slate.💡 Proposed refactor
- err := k8sClient.Get(ctx, typeNamespacedName, existing) - if err != nil && errors.IsNotFound(err) { - resource := &blockstoriov1alpha1.Snapshot{ - ObjectMeta: metav1.ObjectMeta{ - Name: resourceName, - Namespace: "default", - Annotations: map[string]string{ - blockstoriov1alpha1.AnnotationSnapshotAdopted: "true", - blockstoriov1alpha1.AnnotationSnapshotAdoptedCreatedAt: "1770000000123", - }, - }, - Spec: blockstoriov1alpha1.SnapshotSpec{ - ResourceDefinitionName: "rd7", - SnapshotName: "adopted1", - Nodes: []string{"node-x", "node-y"}, - }, - } - Expect(k8sClient.Create(ctx, resource)).To(Succeed()) - } + err := k8sClient.Get(ctx, typeNamespacedName, existing) + Expect(err).To(HaveOccurred(), "Snapshot should not exist before test") + Expect(errors.IsNotFound(err)).To(BeTrue(), "Unexpected error getting Snapshot") + + resource := &blockstoriov1alpha1.Snapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: resourceName, + Namespace: "default", + Annotations: map[string]string{ + blockstoriov1alpha1.AnnotationSnapshotAdopted: "true", + blockstoriov1alpha1.AnnotationSnapshotAdoptedCreatedAt: "1770000000123", + }, + }, + Spec: blockstoriov1alpha1.SnapshotSpec{ + ResourceDefinitionName: "rd7", + SnapshotName: "adopted1", + Nodes: []string{"node-x", "node-y"}, + }, + } + Expect(k8sClient.Create(ctx, resource)).To(Succeed())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/snapshot_adopted_test.go` around lines 61 - 79, Update the setup around k8sClient.Get to explicitly require a NotFound result before creating the Snapshot resource. Assert that err is non-nil and errors.IsNotFound(err), and fail the test for unexpected errors or an already-existing object instead of silently continuing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/linstormigrate/convert.go`:
- Around line 533-543: Validate the non-empty resource group reference before
constructing the ResourceDefinition in the conversion flow around SplitProps and
c.displayRG. Confirm the referenced group exists in the converted result, and
reject or report the record when it does not; only emit ResourceGroupName for
valid references while preserving handling of empty names.
- Around line 848-859: Update snapshotNodesFor to include only nodes confirmed
by convertedNode, excluding CONTROLLER, unknown, and skipped nodes; warn when a
snapshot placement is excluded. Return no snapshot entries when no valid nodes
remain, while preserving sorting of retained display nodes.
---
Nitpick comments:
In `@internal/controller/snapshot_adopted_test.go`:
- Around line 61-79: Update the setup around k8sClient.Get to explicitly require
a NotFound result before creating the Snapshot resource. Assert that err is
non-nil and errors.IsNotFound(err), and fail the test for unexpected errors or
an already-existing object instead of silently continuing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d83e718-6146-4528-8514-edfaf7be4760
📒 Files selected for processing (39)
.golangci.ymlMakefileapi/v1alpha1/snapshot_types.gocmd/linstor-migrate/main.godocs/linstor-migration.mdinternal/controller/snapshot_adopted_test.gointernal/controller/snapshot_controller.gopkg/linstormigrate/convert.gopkg/linstormigrate/convert_test.gopkg/linstormigrate/dump.gopkg/linstormigrate/emit.gopkg/linstormigrate/flags.gopkg/linstormigrate/model.gopkg/linstormigrate/ports.gopkg/linstormigrate/props.gopkg/linstormigrate/testdata/dump/layerdrbdresourcedefinitions.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/layerdrbdresources.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/layerdrbdvolumedefinitions.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/layerluksvolumes.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/layerresourceids.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/layerstoragevolumes.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/linstorremotes.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/nodenetinterfaces.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/nodes.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/nodestorpool.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/propscontainers.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/resourcedefinitions.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/resourcegroups.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/resources.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/storpooldefinitions.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/volumedefinitions.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/volumegroups.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/dump/volumes.internal.linstor.linbit.com.jsonpkg/linstormigrate/testdata/golden/manifests.yamlpkg/linstormigrate/testdata/golden/report.txtpkg/linstormigrate/validate_envtest_test.gopkg/satellite/factory.gopkg/satellite/factory_test.gopkg/store/k8s/drbd_transcode_export.go
| // The fixture under testdata/dump is a synthetic LINSTOR k8s-backend | ||
| // dump. Its shape (tables, column sets, JSON-array-in-string columns, | ||
| // layer-id joins, props instance paths, flag bitmask populations) was | ||
| // modelled on two production cluster dumps; every identifier, IP, | ||
| // DRBD shared-secret and LUKS ciphertext in it is fabricated. | ||
| // | ||
| // Cases covered: | ||
| // - vol1: DRBD,STORAGE ×2 diskful + a TIE_BREAKER witness (flags 388) | ||
| // whose diskless replica has NO LAYER_STORAGE_VOLUMES row (LINSTOR | ||
| // writes none for diskless) — the pool falls back to the replica's | ||
| // StorPoolName prop; a STALE TIE_BREAKER bit (flags 128) on a | ||
| // diskful replica that must be dropped, mirroring auto-diskful | ||
| // leftovers observed in production; a preset TCP port + per-volume | ||
| // minor + per-replica node-ids; a SUCCESSFUL and a | ||
| // FAILED_DEPLOYMENT snapshot (only the former migrates). | ||
| // - vol2: STORAGE-only single-replica volume. | ||
| // - vol3: DRBD,LUKS,STORAGE with LAYER_LUKS_VOLUMES ciphertexts. | ||
| // - vol4: RD marked DELETE — skipped entirely. | ||
| // - vol5: allow-two-primaries (RWX) prop + an unknown flag bit 2048 | ||
| // replica that must convert with the bit reported, not guessed. | ||
|
|
||
| //nolint:gochecknoglobals // standard Go golden-test update flag | ||
| var update = flag.Bool("update", false, "rewrite the golden files under testdata/golden") | ||
|
|
||
| func convertFixture(t *testing.T) *Result { | ||
| t.Helper() | ||
|
|
||
| dump, err := LoadDump(filepath.Join("testdata", "dump")) | ||
| if err != nil { | ||
| t.Fatalf("LoadDump: %v", err) | ||
| } | ||
|
|
||
| res, err := Convert(dump) | ||
| if err != nil { | ||
| t.Fatalf("Convert: %v", err) | ||
| } | ||
|
|
||
| return res | ||
| } | ||
|
|
||
| // TestConvertGolden pins the full manifest stream and report so ANY | ||
| // behaviour change in the converter surfaces as a reviewable diff. | ||
| func TestConvertGolden(t *testing.T) { | ||
| res := convertFixture(t) | ||
|
|
||
| var manifests, report bytes.Buffer | ||
|
|
||
| err := WriteManifests(&manifests, res) | ||
| if err != nil { | ||
| t.Fatalf("WriteManifests: %v", err) | ||
| } | ||
|
|
||
| err = WriteReport(&report, res) | ||
| if err != nil { | ||
| t.Fatalf("WriteReport: %v", err) | ||
| } | ||
|
|
||
| compareGolden(t, filepath.Join("testdata", "golden", "manifests.yaml"), manifests.Bytes()) | ||
| compareGolden(t, filepath.Join("testdata", "golden", "report.txt"), report.Bytes()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Add the required recorded-response and server byte-diff golden test.
The suite uses fabricated dump rows and compares generated bytes only with local files, so it cannot detect divergence from real golinstor output or blockstor server normalization/validation.
pkg/linstormigrate/convert_test.go#L36-L95: feed recorded golinstor responses into the converter and byte-diff the result returned by the blockstor server.pkg/linstormigrate/testdata/dump/layerdrbdresourcedefinitions.internal.linstor.linbit.com.json#L1-L6: replace or supplement this synthetic table with recorded response data.pkg/linstormigrate/testdata/dump/layerdrbdresources.internal.linstor.linbit.com.json#L1-L10: use the matching recorded table response.pkg/linstormigrate/testdata/dump/layerdrbdvolumedefinitions.internal.linstor.linbit.com.json#L1-L48: use the matching recorded table response.pkg/linstormigrate/testdata/dump/resourcedefinitions.internal.linstor.linbit.com.json#L1-L10: use the matching recorded table response.pkg/linstormigrate/testdata/dump/resourcegroups.internal.linstor.linbit.com.json#L1-L6: use the matching recorded table response.pkg/linstormigrate/testdata/dump/resources.internal.linstor.linbit.com.json#L1-L15: use the matching recorded table response.
As per coding guidelines, “L2 contract / golden tests must use recorded golinstor responses → blockstor server byte-diff validation.”
📍 Affects 7 files
pkg/linstormigrate/convert_test.go#L36-L95(this comment)pkg/linstormigrate/testdata/dump/layerdrbdresourcedefinitions.internal.linstor.linbit.com.json#L1-L6pkg/linstormigrate/testdata/dump/layerdrbdresources.internal.linstor.linbit.com.json#L1-L10pkg/linstormigrate/testdata/dump/layerdrbdvolumedefinitions.internal.linstor.linbit.com.json#L1-L48pkg/linstormigrate/testdata/dump/resourcedefinitions.internal.linstor.linbit.com.json#L1-L10pkg/linstormigrate/testdata/dump/resourcegroups.internal.linstor.linbit.com.json#L1-L6pkg/linstormigrate/testdata/dump/resources.internal.linstor.linbit.com.json#L1-L15
Source: Coding guidelines
VerdictNOT LGTM The converter and the snapshot gate hold up under inspection, but the StorPoolName adoption fix covers only ZFS while LVM/LVM_THIN pools from the same database shape fail to register, and the converter silently collapses per-volume storage-pool placement to volume 0's pool, which contradicts the PR's own "never guesses, skips-and-reports" contract in a path whose worst case is a silently empty disk. Findings[MAJOR] The PR's own rationale for the ZFS fix ("a real LINSTOR database stores the pool name under the generic [MAJOR]
[MINOR]
[MINOR]
[MINOR] If [MINOR]
[MINOR] The PR body states all fixture identifiers are fabricated, yet both comments name the live production cluster the shape was taken from. Replace with a generic phrase ("a production ZFS-backed cluster"). Claim mismatches[PARTIAL] "It never guesses: rows it cannot faithfully represent ... are skipped and reported" — three silent paths remain: per-volume storage-pool divergence collapsed to volume 0's pool ( [PARTIAL] "ZFS pools register from [UNVERIFIABLE] "run locally against both production dumps, all objects apply with zero schema/CEL rejections" — the production dumps are not in the repo; what is verifiable is the in-repo fixture passing the envtest CRD+CEL harness ( Caveats
Recommended follow-ups
|
Addresses two referential-integrity gaps CodeRabbit flagged on #180, consistent with the existing replica/pool guards: - An RD naming a resource group that did not convert keeps the RD (a missing template does not make an adopted volume unusable) but clears the dangling reference and reports it, instead of emitting an RD that points at a non-existent RG. - A snapshot placement on a node that did not convert (CONTROLLER / unknown / skipped) is dropped from Spec.Nodes with a warning; a snapshot left with no migrated node is skipped — otherwise the adopted Snapshot would wait for Ready from a Node object that never exists. Also pins the cross-reference invariant Gemini flagged: every converted Resource/Snapshot/RD reference resolves to another converted object's metadata.name (assessed as a non-break for the production data — the references carry the LINSTOR display name, which is what blockstor stores on the wire and normalizes via Name() on lookup, and all prod names are clean lowercase pvc-<uuid>; this test makes that guarantee explicit). Both production dumps convert to identical object counts and still pass server-side CRD validation (254/254, 2113/2113). Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Human review (NOT LGTM) findings, in order of severity: [MAJOR] The StorDriver/StorPoolName fallback now covers LVM and LVM_THIN too, not just ZFS. A real LINSTOR database records the backing volume group (LVM) or <vg>/<thinpool> (LVM_THIN) in the same generic key while the kind-specific StorDriver/LvmVg and StorDriver/ThinPool stay blank, so an adopted LVM-backed pool hit the identical never-registers failure the ZFS fix addressed — and it would only surface at cutover, after the source controller is stopped. LVM_THIN splits the generic value; a value carrying only a VG still errors rather than guessing a thin-pool name. [MAJOR] A replica whose volumes span more than one storage pool is now skipped and reported instead of silently collapsing to volume 0's pool. LINSTOR allows a per-volume pool; blockstor's Resource holds one. The old behaviour sent blockstor looking for the other volumes' backing devices in the wrong pool — a fresh empty zvol beside the real data, silent for single-replica STORAGE-only volumes. That was a guess in a data-bearing field, against this tool's own never-guess contract. [MINOR] The adopted-snapshot gate is refused on a Snapshot already mid-orchestration (SuspendIO=true): short-circuiting there stranded drbdsetup suspend-io on every diskful peer with nothing left to resume it — frozen production I/O with no error. It now falls through to the normal orchestration, and the annotation godoc says so. [MINOR] linstor-migrate closes -out explicitly before reporting success, so an ENOSPC-style flush failure fails the run instead of leaving a truncated manifest behind with exit code 0. [MINOR] NODES.node_flags and VOLUMES.vlm_flags are no longer loaded and ignored: non-zero markers are reported (bit values deliberately not decoded — that would be a guess) so a node or volume the source cluster had flagged is never adopted silently. [MINOR] Replaced an internal cluster name that had leaked into two public code comments with a generic description. The snapshot-placement finding was already fixed in the preceding commit. Tests: LVM/LVM_THIN generic-key registration, divergent per-volume pools skipped, non-zero volume flags reported. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Human review (NOT LGTM) findings, in order of severity: [MAJOR] The StorDriver/StorPoolName fallback now covers LVM and LVM_THIN too, not just ZFS. A real LINSTOR database records the backing volume group (LVM) or <vg>/<thinpool> (LVM_THIN) in the same generic key while the kind-specific StorDriver/LvmVg and StorDriver/ThinPool stay blank, so an adopted LVM-backed pool hit the identical never-registers failure the ZFS fix addressed — and it would only surface at cutover, after the source controller is stopped. LVM_THIN splits the generic value; a value carrying only a VG still errors rather than guessing a thin-pool name. [MAJOR] A replica whose volumes span more than one storage pool is now skipped and reported instead of silently collapsing to volume 0's pool. LINSTOR allows a per-volume pool; blockstor's Resource holds one. The old behaviour sent blockstor looking for the other volumes' backing devices in the wrong pool — a fresh empty zvol beside the real data, silent for single-replica STORAGE-only volumes. That was a guess in a data-bearing field, against this tool's own never-guess contract. [MINOR] The adopted-snapshot gate is refused on a Snapshot already mid-orchestration (SuspendIO=true): short-circuiting there stranded drbdsetup suspend-io on every diskful peer with nothing left to resume it — frozen production I/O with no error. It now falls through to the normal orchestration, and the annotation godoc says so. [MINOR] linstor-migrate closes -out explicitly before reporting success, so an ENOSPC-style flush failure fails the run instead of leaving a truncated manifest behind with exit code 0. [MINOR] NODES.node_flags and VOLUMES.vlm_flags are no longer loaded and ignored: non-zero markers are reported (bit values deliberately not decoded — that would be a guess) so a node or volume the source cluster had flagged is never adopted silently. [MINOR] Replaced an internal cluster name that had leaked into two public code comments with a generic description. The snapshot-placement finding was already fixed in the preceding commit. Tests: LVM/LVM_THIN generic-key registration, divergent per-volume pools skipped, non-zero volume flags reported. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
47a0ebf to
888c9b8
Compare
Closes the reviewer's remaining follow-ups on #180: - A snapshot that captured more than one volume is now reported: the ZFS provider addresses snapshots as <pool>/<rd>_00000@<snap> (volume 0 only, zfs.go snapshotDataset), so the extra volume slots would otherwise imply restore coverage that does not exist on disk. - The runbook's pre-flight gains the mirror of the zvol-name check for snapshot datasets, so an adopted-snapshot name miss surfaces before cutover instead of at the first restore attempt. - The known-limitations table now names the volume-0-only snapshot scope, what an operator should do with an undecoded flag-bit warning, and the empirical node_type calibration (only SATELLITE confirmed against a live cluster). Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Thanks — this was a genuinely useful review. Both MAJORs were real; everything actionable is addressed at [MAJOR] LVM/LVM_THIN generic-key gap — fixed. You're right that the rationale generalises: [MAJOR] per-volume storage pool collapsed to volume 0 — fixed. A replica whose volumes span more than one pool is now skipped and reported rather than collapsed; you were right that this was a guess in a data-bearing field, and it contradicted the tool's own contract. Pinned by [MINOR] adopted gate on an in-flight Snapshot — fixed, and thank you for it: the failure mode you describe (suspend-io stranded on every diskful peer, no error anywhere) is worse than the bug the gate was added for. The gate is now refused when [MINOR] exit 0 on close failure — fixed. The output file is closed explicitly before success is reported, and the close error propagates into the exit code, so a truncated manifest can no longer pass a [MINOR] [MINOR] snapshot [MINOR] internal cluster name in public comments — fixed, replaced with a generic description in both files. Good catch. Follow-ups — both taken:
On the claim mismatches — all three are fair. The PR body's "never guesses" claim was overstated while the per-volume-pool collapse existed; that path is now a skip-and-report, and the two silent-load paths now warn. On the unverifiable one: correct, the production dumps are not in the repo and cannot be — what is reproducible here is the in-repo fixture through the envtest CRD+CEL harness, and I've stopped leaning on the private-dump numbers as evidence in review discussion. Not done, deliberately: the recorded-golinstor-response / server byte-diff golden. I agree it would be strictly stronger than fabricated rows, but it needs a recorded oracle corpus and a running server in the test path, which is a different piece of infrastructure from this PR. The current fixture is shaped from real dumps and every converted object is applied against the real CRDs (schema + CEL) in Also noted your caveat that the envtest harness self-skips without |
2 participants
What this PR does
Adds
linstor-migrate, an offline tool that converts a LINSTOR k8s-backend database dump into blockstorblockstor.cozystack.io/v1alpha1CRD manifests, so an existing LINSTOR deployment can be adopted by blockstor in place — no data is copied or resynced, blockstor takes over the same on-disk zvols/LVs and running DRBD devices. It also fixes three adoption-safety gaps in blockstor core that the migration surfaced.Why
The input is the set of
*.internal.linstor.linbit.comCRDs LINSTOR uses as its SQL tables when running with the k8s connector.cmd/linstor-migrate -in <dump-dir>emits a deterministic multi-document YAML stream (nodes → storage pools → resource groups → resource definitions → resources → snapshots) on stdout and a migration report on stderr; the manifests are then applied to a blockstor control plane that adopts the existing storage. The whole converter lives inpkg/linstormigrate/.The converter preserves every identity adoption depends on — per-volume DRBD minors, per-replica node-ids, the resource TCP port, the DRBD shared-secret, storage-pool backing config and object names — and stamps the adoption-safety latches (
RD.Spec.Initialized=true, per-replicaskipInitialSync=false) so any replica added after migration performs a real sync instead of coming up falsely UpToDate. It never guesses: rows it cannot faithfully represent (LUKS passphrases, unknown flag bits, DELETE'd / failed-deployment objects, orphan replicas) are skipped and reported rather than fabricated.Adoption-safety fixes in blockstor core
StorDriver/StorPoolName(pkg/satellite/factory.go). A real LINSTOR database stores the zpool name under the genericStorDriver/StorPoolName, leavingStorDriver/ZPool[Thin]blank.newZFSpreviously read only the ZPool keys, so an adopted ZFS pool never registered a provider and no diskful resource could come up. It now acceptsStorDriver/StorPoolNameas the final fallback.internal/controller/snapshot_controller.go,api/v1alpha1/snapshot_types.go). An imported Snapshot arrives with empty status; the reconciler's phase logic would otherwise read that as "not started" and drivesuspend-ioon every diskful peer to re-take a snapshot that already exists on disk. Ablockstor.io/adoptedannotation short-circuits orchestration and backfills the terminal per-node state instead.pkg/linstormigrate-drbd-ports). LINSTOR does not persist the DRBD listen port in its database, so adopting with a different port would move the connection endpoint under live I/O. The optional live-port map, captured from the running kernel, presets the ports sodrbdadm adjustis a no-op on the endpoint.Safety model
Data preservation rests on existing blockstor invariants, verified during review: the satellite gates
create-mdbehind a per-volumedrbdadm dump-mdprobe (never re-creates metadata over an adopted device),blkid-guardsmkfs(never reformats a populated filesystem), honours an explicit["STORAGE"]layer stack (no DRBD wrap on non-DRBD volumes), and the allocator is set-if-nil for the preset identities (CEL enforcesinitializedappend-only andskipInitialSync/drbdNodeIDsettable-once).Testing
NewProviderFromKind, andfactory_test.gopins theStorPoolNameregistration (proven fail-on-bug).suspend-iofires; the zvol-name adoption invariant (convertedmetadata.namebyte-matches the on-disk dataset for cleanpvc-<uuid>names) is pinned against a future hashing regression.Operator preconditions (see
docs/linstor-migration.md)The runbook is the required procedure; the converter is one step of it. Before any production cutover: capture live DRBD ports (report must show zero port warnings), gate on cluster health at dump time, cross-check on-disk zvol names, rehearse the full adoption on staging, provision
spec.encryptionby hand for LUKS-encrypted volumes, and migrate the smaller cluster first.Clean-room
The converter reads only the operator's own LINSTOR database dumps (data) and blockstor's own API types. It does not read, embed or derive from LINSTOR source code.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes