From 61f631abccee48b643608565ec5e357e3424224f Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Tue, 27 Oct 2026 22:03:51 +0100 Subject: [PATCH 1/2] fix(linstor-migrate): skip a definition whose replicas disagree on the stack A definition with no stack of its own whose replicas were built two ways was converted with the stack unset, and the controller then pinned the default stack over every replica, LUKS ones included; a replica built otherwise than the stack its definition names was converted all the same. Either way the satellite would bring a replica up under a stack it was not built with, DRBD metadata written over a LUKS volume's ciphertext for one, and the operator got a warning only. Such a definition is now skipped with its replicas and the report names the disagreement. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/linstor-migration.md | 1 + pkg/linstormigrate/convert.go | 73 +++++++++++++++++++------- pkg/linstormigrate/convert_test.go | 2 +- pkg/linstormigrate/layer_stack_test.go | 61 +++++++++++---------- 4 files changed, 91 insertions(+), 46 deletions(-) diff --git a/docs/linstor-migration.md b/docs/linstor-migration.md index d482de36..fbf94104 100644 --- a/docs/linstor-migration.md +++ b/docs/linstor-migration.md @@ -11,6 +11,7 @@ Migrated: nodes, storage pools, resource groups, resource definitions (+ volume **NOT migrated — read the report on stderr for the exact list per cluster:** - **LUKS passphrases.** LUKS-encrypted volumes convert with their layer stack intact (an RD whose `layer_stack` column is empty takes the stack its replicas were built with, read from `LAYER_RESOURCE_IDS`), but the per-volume passphrase (encrypted with the LINSTOR master key) is not decrypted. You must provision `spec.encryption` for those RDs by hand before the encrypted volumes can be opened. Each affected RD is reported. +- **Definitions whose replicas disagree on the layer stack** are skipped with their replicas and reported: replicas built two ways under a definition with no stack of its own, or a replica built otherwise than the stack the definition names. Adopted as they are, such a replica would be brought up under a stack it was not built with, DRBD metadata over a LUKS volume's ciphertext for one. Settle the stack in LINSTOR and convert again. - **Backup-shipping remotes**, unknown/runtime flag bits, and DELETE'd / FAILED_DEPLOYMENT rows are skipped and reported. None affect live data availability. - **DRBD TCP ports** are absent from most LINSTOR 1.33 dumps — see "Capture live DRBD ports" below. diff --git a/pkg/linstormigrate/convert.go b/pkg/linstormigrate/convert.go index 3e24060e..38338c11 100644 --- a/pkg/linstormigrate/convert.go +++ b/pkg/linstormigrate/convert.go @@ -447,13 +447,48 @@ func (c *converter) indexDiskStacks() { } } +// stackDisagreement says why the RD's replicas cannot be adopted under one +// stack, or "" when they can: replicas built under different stacks when +// the RD row names none, or a replica built under another stack than the +// one the row names. It reads the row's stack without reporting a parse +// failure; rdLayerStack reports that once. +func (c *converter) stackDisagreement(row *ResourceDefinitionRow) string { + var stack []string + if !emptyList(row.LayerStack) { + _ = json.Unmarshal([]byte(row.LayerStack), &stack) + } + + keys := make([]string, 0, len(c.diskStacks[row.ResourceName])) + for key := range c.diskStacks[row.ResourceName] { + keys = append(keys, key) + } + + slices.Sort(keys) + + if len(stack) == 0 { + if len(keys) > 1 { + return "no layer stack of its own, and its replicas were built as " + strings.Join(keys, " and ") + } + + return "" + } + + for _, key := range keys { + if key != strings.Join(stack, ",") { + return "layer stack " + strings.Join(stack, ",") + ", but a replica was built as " + key + } + } + + return "" +} + // rdLayerStack is the layer stack a converted RD carries. An RD row // that names one keeps it; an empty one takes the stack its replicas // were built with, so the satellite adopts each volume as it is rather // than as the default, and the group's stack, which blockstor would -// otherwise pin, is never laid over data built without it. Replicas -// that disagree, or a stack the replicas were not built with, are -// reported for the operator to settle before adopting. +// otherwise pin, is never laid over data built without it. An RD whose +// replicas disagree, or whose stack its replicas were not built with, +// never gets here: rdConvertible skips it (stackDisagreement). func (c *converter) rdLayerStack(row *ResourceDefinitionRow, dsp string) []string { stack := c.parseList(row.LayerStack, "resource definition "+dsp+" layer_stack") onDisk := c.diskStacks[row.ResourceName] @@ -466,25 +501,14 @@ func (c *converter) rdLayerStack(row *ResourceDefinitionRow, dsp string) []strin slices.Sort(keys) if len(stack) > 0 { - for _, key := range keys { - if key != strings.Join(stack, ",") { - c.warnf("resource definition %s: layer stack %s, but a replica was built as %s — that replica will not come up as it is", dsp, strings.Join(stack, ","), key) - } - } - return stack } - switch len(keys) { - case 0: - return nil - case 1: + if len(keys) == 1 { return slices.Clone(onDisk[keys[0]]) - default: - c.warnf("resource definition %s: no layer stack of its own, and its replicas were built as %s — left unset; set spec.layerStack before adopting", dsp, strings.Join(keys, " and ")) - - return nil } + + return nil } func (c *converter) convertNodes() []crdv1alpha1.Node { @@ -1081,6 +1105,14 @@ func (c *converter) rdConvertible(row *ResourceDefinitionRow, dsp string) bool { c.warnf("resource definition %s: unhandled flags bitmask %d dropped", dsp, row.ResourceFlags) } + if reason := c.stackDisagreement(row); reason != "" { + c.warnf("resource definition %s: %s — skipped with its replicas; adopted as it is, a replica would be "+ + "brought up under a stack it was not built with, DRBD metadata written over a LUKS volume's "+ + "ciphertext say; settle the stack in LINSTOR and convert again", dsp, reason) + + return false + } + if reason := c.volumelessReason(row.ResourceName); reason != "" { c.warnf("resource definition %s: %s — skipped; there is no volume to adopt, and migrating it leaves the controller placing replicas the satellite can never bring up", dsp, reason) @@ -1668,7 +1700,7 @@ func (c *converter) snapshotNodesFor(rdName, snapName, snapDsp string) []string // (`"[\"DRBD\",\"STORAGE\"]"`). Empty input, `[]` and `null` all yield // nil; a malformed value is reported and yields nil. func (c *converter) parseList(raw, context string) []string { - if raw == "" || raw == "[]" || raw == "null" { + if emptyList(raw) { return nil } @@ -1688,6 +1720,11 @@ func (c *converter) parseList(raw, context string) []string { return out } +// emptyList reports a list column that holds no list: empty, `[]` or `null`. +func emptyList(raw string) bool { + return raw == "" || raw == "[]" || raw == "null" +} + func (c *converter) displayNode(name string) string { return displayName(c.nodeDsp[name], name) } diff --git a/pkg/linstormigrate/convert_test.go b/pkg/linstormigrate/convert_test.go index 030b6d4d..18f5ee73 100644 --- a/pkg/linstormigrate/convert_test.go +++ b/pkg/linstormigrate/convert_test.go @@ -1763,7 +1763,7 @@ func TestBothLatchReasonsAreReported(t *testing.T) { {NodeName: "NODE-A", PoolName: "P2", DriverName: "ZFS", UUID: "sp-2"}, }, ResourceDefinitions: []ResourceDefinitionRow{ - {ResourceName: "PVC-BOTH", ResourceDspName: "pvc-both", LayerStack: `["DRBD","STORAGE"]`, UUID: "rd-b"}, + {ResourceName: "PVC-BOTH", ResourceDspName: "pvc-both", LayerStack: `["STORAGE"]`, UUID: "rd-b"}, }, VolumeDefinitions: []VolumeDefinitionRow{ {ResourceName: "PVC-BOTH", VlmNr: 0, VlmSize: 1048576, UUID: "vd-b"}, diff --git a/pkg/linstormigrate/layer_stack_test.go b/pkg/linstormigrate/layer_stack_test.go index 908005db..c78eccc7 100644 --- a/pkg/linstormigrate/layer_stack_test.go +++ b/pkg/linstormigrate/layer_stack_test.go @@ -153,39 +153,46 @@ func TestADisklessReplicaDoesNotShapeTheStack(t *testing.T) { } } -// Replicas built two ways leave nothing to read: the stack stays unset -// and the operator is told which ways. -func TestReplicasThatDisagreeLeaveTheStackUnset(t *testing.T) { +// A definition whose replicas cannot be adopted under one stack is not +// converted, nor are its replicas: adopted as it is, a replica would come up +// under a stack it was not built with, DRBD metadata over a LUKS volume's +// ciphertext among them. Both shapes are refused and named: replicas built +// two ways under a row with no stack, and a replica built otherwise than the +// stack the row names. +func TestADefinitionWhoseReplicasDisagreeOnTheStackIsNotConverted(t *testing.T) { t.Parallel() - got, res := convertedStack(t, stackDump("[]", - layerChain(1, "NODE-A", "PVC-S", "DRBD", "LUKS", "STORAGE"), layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE"))) - if len(got) != 0 { - t.Fatalf("stack %v, want unset", got) - } - - if !hasWarning(res, "built as DRBD,LUKS,STORAGE and DRBD,STORAGE") { - t.Errorf("disagreement not reported; warnings %v", res.Warnings) - } -} + for name, tc := range map[string]struct { + rowStack, reason string + }{ + "replicas built two ways": {rowStack: "[]", reason: "built as DRBD,LUKS,STORAGE and DRBD,STORAGE"}, + "a replica built otherwise": { + rowStack: `["DRBD","LUKS","STORAGE"]`, reason: "a replica was built as DRBD,STORAGE", + }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() -// A stack the RD row names is kept, and a replica built otherwise is -// reported: the satellite would try to rebuild it. -func TestAReplicaBuiltOtherwiseThanItsRDIsReported(t *testing.T) { - t.Parallel() + res, err := Convert(stackDump(tc.rowStack, + layerChain(1, "NODE-A", "PVC-S", "DRBD", "LUKS", "STORAGE"), layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE"))) + if err != nil { + t.Fatalf("Convert: %v", err) + } - got, res := convertedStack(t, stackDump(`["DRBD","LUKS","STORAGE"]`, - layerChain(1, "NODE-A", "PVC-S", "DRBD", "LUKS", "STORAGE"), layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE"))) - if !slices.Equal(got, []string{"DRBD", "LUKS", "STORAGE"}) { - t.Fatalf("stack %v, want the RD row's", got) - } + for i := range res.ResourceDefinitions { + if res.ResourceDefinitions[i].Name == "pvc-s" { + t.Fatalf("pvc-s converted with stack %v", res.ResourceDefinitions[i].Spec.LayerStack) + } + } - if !hasWarning(res, "a replica was built as DRBD,STORAGE") { - t.Errorf("mismatch not reported; warnings %v", res.Warnings) - } + if len(res.Resources) != 0 { + t.Errorf("%d replicas of a skipped definition converted", len(res.Resources)) + } - if hasWarning(res, "a replica was built as DRBD,LUKS,STORAGE") { - t.Errorf("a matching replica was reported; warnings %v", res.Warnings) + if !hasWarning(res, tc.reason) || !hasWarning(res, "skipped with its replicas") { + t.Errorf("the refusal does not name %q; warnings %v", tc.reason, res.Warnings) + } + }) } } From ddd65ec1d243bcca68a1cd339f9ccd78226aa0dd Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 2 Nov 2026 11:15:21 +0100 Subject: [PATCH 2/2] fix(linstor-migrate): refuse a replica whose stack the dump does not show Under a definition with no stack of its own, a diskful replica the dump carries no layer chain for was taken as agreeing with the rest, so a definition whose replicas showed no chain at all converted with the stack unset and no warning, and one with a single chain converted under that stack for every replica. Such a definition is now refused with its replicas too. The tests cover every refused shape, layer order included, against replicas that would otherwise convert. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/linstor-migration.md | 2 +- pkg/linstormigrate/convert.go | 44 ++++++++++++++++++++++-- pkg/linstormigrate/layer_stack_test.go | 46 +++++++++++++++++++++----- 3 files changed, 81 insertions(+), 11 deletions(-) diff --git a/docs/linstor-migration.md b/docs/linstor-migration.md index fbf94104..f9aead48 100644 --- a/docs/linstor-migration.md +++ b/docs/linstor-migration.md @@ -11,7 +11,7 @@ Migrated: nodes, storage pools, resource groups, resource definitions (+ volume **NOT migrated — read the report on stderr for the exact list per cluster:** - **LUKS passphrases.** LUKS-encrypted volumes convert with their layer stack intact (an RD whose `layer_stack` column is empty takes the stack its replicas were built with, read from `LAYER_RESOURCE_IDS`), but the per-volume passphrase (encrypted with the LINSTOR master key) is not decrypted. You must provision `spec.encryption` for those RDs by hand before the encrypted volumes can be opened. Each affected RD is reported. -- **Definitions whose replicas disagree on the layer stack** are skipped with their replicas and reported: replicas built two ways under a definition with no stack of its own, or a replica built otherwise than the stack the definition names. Adopted as they are, such a replica would be brought up under a stack it was not built with, DRBD metadata over a LUKS volume's ciphertext for one. Settle the stack in LINSTOR and convert again. +- **Definitions whose replicas disagree on the layer stack** are skipped with their replicas and reported: replicas built two ways, or with their layers in another order, under a definition with no stack of its own; a diskful replica the dump shows no layer chain for, under such a definition; or a replica built otherwise than the stack the definition names. Adopted as they are, such a replica would be brought up under a stack it was not built with, DRBD metadata over a LUKS volume's ciphertext for one. Settle the stack in LINSTOR and convert again. - **Backup-shipping remotes**, unknown/runtime flag bits, and DELETE'd / FAILED_DEPLOYMENT rows are skipped and reported. None affect live data availability. - **DRBD TCP ports** are absent from most LINSTOR 1.33 dumps — see "Capture live DRBD ports" below. diff --git a/pkg/linstormigrate/convert.go b/pkg/linstormigrate/convert.go index 38338c11..8bedf5e9 100644 --- a/pkg/linstormigrate/convert.go +++ b/pkg/linstormigrate/convert.go @@ -133,6 +133,11 @@ type converter struct { // resource_name, then by the joined stack, to the stack itself. diskStacks map[string]map[string][]string + // chained is every live replica indexDiskStacks found a layer chain + // for. A diskful replica without one was built under a stack the + // dump does not show. + chained map[replicaKey]bool + // referential-integrity sets, populated as each kind converts so // later kinds can drop rows that would dangle (a replica of a // skipped/absent RD, a replica on an unknown node, ...). @@ -214,6 +219,7 @@ func ConvertWithOptions(dump *Dump, opts Options) (*Result, error) { storVol: map[volumeReplicaKey]LayerStorageVolumeRow{}, luksVol: map[volumeReplicaKey]LayerLuksVolumeRow{}, diskStacks: map[string]map[string][]string{}, + chained: map[replicaKey]bool{}, convertedRD: map[string]bool{}, convertedNode: map[string]bool{}, convertedRG: map[string]bool{}, @@ -439,6 +445,8 @@ func (c *converter) indexDiskStacks() { stack = append(stack, cur.LayerResourceKind) } + c.chained[replicaKey{node: row.NodeName, rd: row.ResourceName}] = true + if c.diskStacks[row.ResourceName] == nil { c.diskStacks[row.ResourceName] = map[string][]string{} } @@ -458,6 +466,18 @@ func (c *converter) stackDisagreement(row *ResourceDefinitionRow) string { _ = json.Unmarshal([]byte(row.LayerStack), &stack) } + // With no stack of its own, the RD takes the one its replicas were + // built with, so every diskful replica has to show it; one the dump + // carries no chain for leaves that stack unknown. A stack the row names + // is what LINSTOR built the replicas under, so a missing chain there + // says nothing new. + if len(stack) == 0 { + if node := c.unchainedReplica(row.ResourceName); node != "" { + return "no layer stack of its own, and the replica on " + c.displayNode(node) + + " has no layer chain in the dump, so the stack it was built with is unknown" + } + } + keys := make([]string, 0, len(c.diskStacks[row.ResourceName])) for key := range c.diskStacks[row.ResourceName] { keys = append(keys, key) @@ -482,6 +502,26 @@ func (c *converter) stackDisagreement(row *ResourceDefinitionRow) string { return "" } +// unchainedReplica names the node of a live diskful replica of rdName that +// the dump carries no layer chain for, or "" when every one has its chain. +// Such a replica was built under a stack nobody can read, and taking it as +// agreeing with the others is how a LUKS volume ends up adopted as plain. +// The filter is indexDiskStacks's: no snapshot, not diskless. +func (c *converter) unchainedReplica(rdName string) string { + for i := range c.dump.Resources { + res := &c.dump.Resources[i] + if res.ResourceName != rdName || res.SnapshotName != "" || res.ResourceFlags&resourceFlagDiskless != 0 { + continue + } + + if !c.chained[replicaKey{node: res.NodeName, rd: res.ResourceName}] { + return res.NodeName + } + } + + return "" +} + // rdLayerStack is the layer stack a converted RD carries. An RD row // that names one keeps it; an empty one takes the stack its replicas // were built with, so the satellite adopts each volume as it is rather @@ -1107,8 +1147,8 @@ func (c *converter) rdConvertible(row *ResourceDefinitionRow, dsp string) bool { if reason := c.stackDisagreement(row); reason != "" { c.warnf("resource definition %s: %s — skipped with its replicas; adopted as it is, a replica would be "+ - "brought up under a stack it was not built with, DRBD metadata written over a LUKS volume's "+ - "ciphertext say; settle the stack in LINSTOR and convert again", dsp, reason) + "brought up under a stack it was not built with, which for a LUKS volume writes DRBD metadata "+ + "over its ciphertext; settle the stack in LINSTOR and convert again", dsp, reason) return false } diff --git a/pkg/linstormigrate/layer_stack_test.go b/pkg/linstormigrate/layer_stack_test.go index c78eccc7..5e8eff4b 100644 --- a/pkg/linstormigrate/layer_stack_test.go +++ b/pkg/linstormigrate/layer_stack_test.go @@ -153,28 +153,56 @@ func TestADisklessReplicaDoesNotShapeTheStack(t *testing.T) { } } +// withReplicas adds a diskful replica of PVC-S on both nodes, so a test can +// see whether the replicas of a refused definition still convert. +func withReplicas(dump *Dump) *Dump { + dump.Resources = append(dump.Resources, + ResourceRow{NodeName: "NODE-A", ResourceName: "PVC-S", UUID: "r-a"}, + ResourceRow{NodeName: "NODE-B", ResourceName: "PVC-S", UUID: "r-b"}, + ) + + return dump +} + // A definition whose replicas cannot be adopted under one stack is not // converted, nor are its replicas: adopted as it is, a replica would come up // under a stack it was not built with, DRBD metadata over a LUKS volume's -// ciphertext among them. Both shapes are refused and named: replicas built -// two ways under a row with no stack, and a replica built otherwise than the -// stack the row names. +// ciphertext among them. Every shape is refused and named: replicas built +// two ways, or with their layers in another order, under a row with no +// stack; a replica built otherwise than the stack the row names; and, under +// a row with no stack, a diskful replica the dump shows no chain for, alone +// or next to one that has one. func TestADefinitionWhoseReplicasDisagreeOnTheStackIsNotConverted(t *testing.T) { t.Parallel() + luks := layerChain(1, "NODE-A", "PVC-S", "DRBD", "LUKS", "STORAGE") + for name, tc := range map[string]struct { rowStack, reason string + chains [][]LayerResourceIDRow }{ - "replicas built two ways": {rowStack: "[]", reason: "built as DRBD,LUKS,STORAGE and DRBD,STORAGE"}, + "replicas built two ways": { + rowStack: "[]", reason: "built as DRBD,LUKS,STORAGE and DRBD,STORAGE", + chains: [][]LayerResourceIDRow{luks, layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE")}, + }, + "layers in another order": { + rowStack: "[]", reason: "built as DRBD,LUKS,STORAGE and LUKS,DRBD,STORAGE", + chains: [][]LayerResourceIDRow{luks, layerChain(10, "NODE-B", "PVC-S", "LUKS", "DRBD", "STORAGE")}, + }, "a replica built otherwise": { rowStack: `["DRBD","LUKS","STORAGE"]`, reason: "a replica was built as DRBD,STORAGE", + chains: [][]LayerResourceIDRow{luks, layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE")}, + }, + "no chain at all": {rowStack: "[]", reason: "has no layer chain in the dump"}, + "one chain missing": { + rowStack: "[]", reason: "the replica on node-b has no layer chain in the dump", + chains: [][]LayerResourceIDRow{luks}, }, } { t.Run(name, func(t *testing.T) { t.Parallel() - res, err := Convert(stackDump(tc.rowStack, - layerChain(1, "NODE-A", "PVC-S", "DRBD", "LUKS", "STORAGE"), layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE"))) + res, err := Convert(withReplicas(stackDump(tc.rowStack, tc.chains...))) if err != nil { t.Fatalf("Convert: %v", err) } @@ -185,8 +213,10 @@ func TestADefinitionWhoseReplicasDisagreeOnTheStackIsNotConverted(t *testing.T) } } - if len(res.Resources) != 0 { - t.Errorf("%d replicas of a skipped definition converted", len(res.Resources)) + for i := range res.Resources { + if res.Resources[i].Spec.ResourceDefinitionName == "pvc-s" { + t.Errorf("replica %s of a skipped definition converted", res.Resources[i].Name) + } } if !hasWarning(res, tc.reason) || !hasWarning(res, "skipped with its replicas") {