fix(linstor-migrate): skip a definition whose replicas disagree on the stack

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

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{}
 		}
@@ -447,13 +455,80 @@ 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)
+	}
+
+	// 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)
+	}
+
+	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 ""
+}
+
+// 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
 // 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 +541,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 +1145,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, which for a LUKS volume writes DRBD metadata "+
+			"over its ciphertext; 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 +1740,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 +1760,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)
 }

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"},

pkg/linstormigrate/layer_stack_test.go

@@ -153,39 +153,76 @@ 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) {
-	t.Parallel()
+// 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"},
+	)
 
-	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)
-	}
+	return dump
 }
 
-// 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) {
+// 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. 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()
 
-	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)
-	}
+	luks := layerChain(1, "NODE-A", "PVC-S", "DRBD", "LUKS", "STORAGE")
 
-	if !hasWarning(res, "a replica was built as DRBD,STORAGE") {
-		t.Errorf("mismatch not reported; warnings %v", res.Warnings)
-	}
+	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",
+			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(withReplicas(stackDump(tc.rowStack, tc.chains...)))
+			if err != nil {
+				t.Fatalf("Convert: %v", err)
+			}
 
-	if hasWarning(res, "a replica was built as DRBD,LUKS,STORAGE") {
-		t.Errorf("a matching replica was reported; warnings %v", res.Warnings)
+			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)
+				}
+			}
+
+			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") {
+				t.Errorf("the refusal does not name %q; warnings %v", tc.reason, res.Warnings)
+			}
+		})
 	}
 }