From 8a02b620010712ec2b51b7805ad95b49122d0bb5 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 2 Nov 2026 23:28:17 +0100 Subject: [PATCH 1/5] fix(linstor-migrate): refuse replicas with external DRBD metadata A LINSTOR replica whose DRBD metadata lives on a separate volume is built with a ".meta" STORAGE child under its DRBD layer, and each of its DRBD volumes records the meta pool. The converter dropped that child and adopted the replica as if its metadata were internal, so the satellite brought it up with create-md on the tail of the data device. blockstor cannot adopt that layout: the satellite looks for a metadata volume under its own name and does not provision one. Skip such a definition with its replicas and say which replica keeps its metadata outside. A replica the dump has no layer chain for is judged by StorPoolNameDrbdMeta, resolved per volume through every scope LINSTOR reads it from: volume definition, volume group, replica, definition, resource group and node. ".internal" is read trimmed and in any case, as LINSTOR reads it. The property over chains without a ".meta" child does not mean external metadata: LINSTOR applies it only to replicas created after it is set, and ".internal" asks for internal metadata. Such a definition converts. The property is left off the converted definition and its replicas, the two scopes the dispatcher reads it from, and the report says the metadata stays on the data device. Rows that do not convert do not decide: a diskless replica, a replica marked DELETE, a snapshot's rows, and a definition without a DRBD layer. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/linstor-migration.md | 2 + pkg/linstormigrate/convert.go | 280 ++++++++++++ pkg/linstormigrate/external_metadata_test.go | 441 +++++++++++++++++++ pkg/linstormigrate/layer_stack_test.go | 10 +- pkg/linstormigrate/model.go | 5 + pkg/linstormigrate/props.go | 5 + site/content/en/docs/migration.md | 2 + 7 files changed, 741 insertions(+), 4 deletions(-) create mode 100644 pkg/linstormigrate/external_metadata_test.go diff --git a/docs/linstor-migration.md b/docs/linstor-migration.md index 88c0f5cc..800625c8 100644 --- a/docs/linstor-migration.md +++ b/docs/linstor-migration.md @@ -11,6 +11,8 @@ 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 key (encrypted with the LINSTOR master key) is not decrypted. blockstor opens every LUKS volume with the cluster passphrase kept in its Secret, so add that passphrase to each such volume as a key slot before adopting it: pre-flight step 7 says how. Each affected RD is reported. +- **Definitions with external DRBD metadata** are skipped with their replicas and reported: a replica with a `.meta` layer child or a meta pool on its DRBD volumes, or a replica the dump has no layer chain for while `StorPoolNameDrbdMeta` names a pool in any scope LINSTOR reads it from (volume definition, volume group, replica, definition, resource group, node). blockstor keeps DRBD metadata on the data device and does not adopt a separate metadata volume, so such a replica would be brought up with internal metadata written over the end of its data. Changing the property does not move existing metadata, so copy the data to a volume with internal metadata in LINSTOR and convert that one. For a replica without a chain, check in LINSTOR where its metadata is first: if it is on the data device, remove the property and convert again. +- **`StorPoolNameDrbdMeta` over internal metadata** does not stop the definition: when every replica's chain has no `.meta` child, the metadata is on the data device (LINSTOR applies the property only to replicas created after it was set, and `.internal` asks for internal metadata). The definition converts, and the property is not carried over to the definition or its replicas, the two places blockstor reads it from, so the satellite does not look for a separate metadata volume. The report names the scope the property was set on. - **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. A `layer_stack` column the converter cannot read counts as no stack, and the warning names its value. 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 d88ac8ee..0e9b93e5 100644 --- a/pkg/linstormigrate/convert.go +++ b/pkg/linstormigrate/convert.go @@ -21,6 +21,7 @@ package linstormigrate import ( "encoding/json" "fmt" + "maps" "slices" "sort" "strconv" @@ -138,6 +139,12 @@ type converter struct { // dump does not show. chained map[replicaKey]bool + // metaPoolDropped names the definitions whose StorPoolNameDrbdMeta is + // left off the converted definition and its replicas: their replicas + // keep their metadata on the data device, and the dispatcher would + // otherwise read the property and look for a metadata volume. + metaPoolDropped map[string]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, ...). @@ -220,6 +227,7 @@ func ConvertWithOptions(dump *Dump, opts Options) (*Result, error) { luksVol: map[volumeReplicaKey]LayerLuksVolumeRow{}, diskStacks: map[string]map[string][]string{}, chained: map[replicaKey]bool{}, + metaPoolDropped: map[string]bool{}, convertedRD: map[string]bool{}, convertedNode: map[string]bool{}, convertedRG: map[string]bool{}, @@ -1114,6 +1122,7 @@ func (c *converter) convertResourceDefinitions() []crdv1alpha1.ResourceDefinitio } typed, residual, extra := k8sstore.SplitProps(c.props.ResourceDefinition(row.ResourceName)) + residual = c.withoutDroppedMetaPool(row.ResourceName, residual) latch := c.rdInitializedLatch(row.ResourceName) @@ -1186,15 +1195,285 @@ func (c *converter) rdConvertible(row *ResourceDefinitionRow, dsp string) bool { return false } + reason, remedy, internal := c.externalMetadataReason(row) + if reason != "" { + c.warnf("resource definition %s: %s — skipped with its replicas; %s", dsp, reason, remedy) + + 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) return false } + if internal != "" { + c.metaPoolDropped[row.ResourceName] = true + c.warnf("resource definition %s: %s", dsp, internal) + } + return true } +// externalMetadataReason answers why the replicas of row cannot be adopted +// for where LINSTOR keeps their DRBD metadata, and what the operator does +// about it, both "" when nothing stands in the way. The third answer is a +// note for a definition that converts although a meta pool is set for it. +// +// LINSTOR builds a replica with external metadata with a ".meta" STORAGE +// child under its DRBD layer and records the meta pool on each DRBD volume. +// blockstor cannot adopt that layout: the satellite looks for the metadata +// under its own volume name and does not provision one, so a replica +// converted without it comes up with internal metadata and create-md lands +// on the tail of the data. +// +// StorPoolNameDrbdMeta alone is not that evidence: LINSTOR applies it to +// replicas created after it is set, and decides from the ".meta" child when +// it loads its database. A replica the dump has a chain for and no ".meta" +// keeps its metadata on the data device and converts; the property is left +// off the converted definition and replicas, the two scopes the dispatcher +// reads it from, and the other scopes convert onto objects nothing reads it +// from. A replica the dump has no chain for is decided by the property +// alone, read through every scope LINSTOR takes it from, per volume. +func (c *converter) externalMetadataReason(row *ResourceDefinitionRow) (string, string, string) { + rdName := row.ResourceName + + if node := c.externalMetadataReplica(rdName); node != "" { + return "the replica on " + c.displayNode(node) + " keeps its DRBD metadata on a separate volume", + "blockstor keeps DRBD metadata on the data device, so adopted as it is the satellite would write " + + "new metadata over the end of the volume's data; copy its data to a volume with internal metadata " + + "in LINSTOR and convert that one", "" + } + + if !c.runsDRBD(row) { + return "", "", "" + } + + var internal string + + for i := range c.dump.Resources { + res := &c.dump.Resources[i] + if res.ResourceName != rdName || res.SnapshotName != "" || + res.ResourceFlags&(resourceFlagDiskless|resourceFlagDelete) != 0 { + continue + } + + pools := c.drbdMetaPools(row, res.NodeName) + if len(pools) == 0 { + continue + } + + if !c.chained[replicaKey{node: res.NodeName, rd: rdName}] { + for _, mp := range pools { + if isExternalMetaPool(mp.pool) { + return "the dump has no layer chain for the replica on " + c.displayNode(res.NodeName) + ", and " + + storPoolNameDrbdMeta + " " + mp.pool + " on " + mp.scope + + " would put its DRBD metadata on a separate volume", + "check in LINSTOR where that replica keeps its metadata: if on a separate volume, copy its data " + + "to a volume with internal metadata and convert that one; if on the data device, remove " + + "the property and convert again", "" + } + } + } + + if internal == "" { + internal = "its replicas keep their DRBD metadata on the data device, though " + storPoolNameDrbdMeta + + " " + pools[0].pool + " is set on " + pools[0].scope + "; the property is not carried over to the " + + "definition or its replicas, so blockstor does not look for a separate metadata volume" + } + } + + return "", "", internal +} + +// runsDRBD says whether the replicas of row come up with a DRBD layer, and +// so whether a meta pool property means anything for them. A stack the row +// names decides; an empty one is the stack the replicas were built with, +// which stackDisagreement has already found to be one; with neither, the +// controller pins DRBD over STORAGE. +func (c *converter) runsDRBD(row *ResourceDefinitionRow) bool { + var stack []string + if !emptyList(row.LayerStack) { + _ = json.Unmarshal([]byte(row.LayerStack), &stack) + } + + if len(stack) == 0 { + for _, onDisk := range c.diskStacks[row.ResourceName] { + stack = onDisk + } + } + + return len(stack) == 0 || slices.ContainsFunc(stack, func(kind string) bool { return strings.EqualFold(kind, "DRBD") }) +} + +// externalMetadataReplica names the node of a replica of rdName that keeps +// its DRBD metadata on a separate volume, by its ".meta" layer child or by +// the meta pool on one of its DRBD volumes, or "" when none does. A replica +// marked DELETE does not convert, so its layout does not count. +func (c *converter) externalMetadataReplica(rdName string) string { + deleted := map[string]bool{} + + for i := range c.dump.Resources { + res := &c.dump.Resources[i] + if res.ResourceName == rdName && res.SnapshotName == "" && res.ResourceFlags&resourceFlagDelete != 0 { + deleted[res.NodeName] = true + } + } + + byID := map[int32]*LayerResourceIDRow{} + + for i := range c.dump.LayerResourceIDs { + lri := &c.dump.LayerResourceIDs[i] + if lri.ResourceName != rdName || lri.SnapshotName != "" || deleted[lri.NodeName] { + continue + } + + if lri.LayerResourceSuffix == drbdMetaSuffix { + return lri.NodeName + } + + byID[lri.LayerResourceID] = lri + } + + for i := range c.dump.LayerDrbdVolumes { + vol := &c.dump.LayerDrbdVolumes[i] + if lri, ok := byID[vol.LayerResourceID]; ok && isExternalMetaPool(vol.PoolName) { + return lri.NodeName + } + } + + return "" +} + +// metaPool is the StorPoolNameDrbdMeta one volume resolves to, and the +// scope it came from. +type metaPool struct { + pool string + scope string +} + +// drbdMetaPools is the StorPoolNameDrbdMeta LINSTOR resolves for each volume +// of the replica of row on node, for the volumes that resolve one. Each +// volume is resolved on its own, most specific scope first: volume +// definition, volume group, resource, resource definition, resource group, +// node (getExtMetaDataStorPoolName in LINSTOR's RscDrbdLayerHelper). +func (c *converter) drbdMetaPools(row *ResourceDefinitionRow, node string) []metaPool { + rd, rg := row.ResourceName, row.ResourceGroupName + + shared := func() (metaPool, bool) { + for _, scope := range []struct { + props map[string]string + name string + }{ + {c.props.Resource(node, rd), "the replica on " + c.displayNode(node)}, + {c.props.ResourceDefinition(rd), "the definition"}, + {c.resourceGroupProps(rg), "resource group " + rg}, + {c.props.Node(node), "node " + c.displayNode(node)}, + } { + if v := scope.props[storPoolNameDrbdMeta]; v != "" { + return metaPool{pool: v, scope: scope.name}, true + } + } + + return metaPool{}, false + } + + var ( + out []metaPool + vols int + ) + + for i := range c.dump.VolumeDefinitions { + vd := &c.dump.VolumeDefinitions[i] + if vd.ResourceName != rd || vd.SnapshotName != "" { + continue + } + + vols++ + + vlm := strconv.Itoa(int(vd.VlmNr)) + if v := c.props.VolumeDefinition(rd, vlm)[storPoolNameDrbdMeta]; v != "" { + out = append(out, metaPool{pool: v, scope: "volume definition " + c.displayRD(rd) + "/" + vlm}) + + continue + } + + if v := c.resourceGroupVolumeProps(rg, vlm)[storPoolNameDrbdMeta]; v != "" { + out = append(out, metaPool{pool: v, scope: "volume group " + rg + "/" + vlm}) + + continue + } + + if mp, ok := shared(); ok { + out = append(out, mp) + } + } + + if vols == 0 { + if mp, ok := shared(); ok { + out = append(out, mp) + } + } + + return out +} + +func (c *converter) resourceGroupProps(rg string) map[string]string { + if rg == "" { + return nil + } + + return c.props.ResourceGroup(rg) +} + +func (c *converter) resourceGroupVolumeProps(rg, vlm string) map[string]string { + if rg == "" { + return nil + } + + return c.props.VolumeGroup(rg, vlm) +} + +// withoutDroppedMetaPool takes StorPoolNameDrbdMeta out of the props of a +// definition named in metaPoolDropped, or of one of its replicas. +func (c *converter) withoutDroppedMetaPool(rdName string, props map[string]string) map[string]string { + if !c.metaPoolDropped[rdName] { + return props + } + + if _, ok := props[storPoolNameDrbdMeta]; !ok { + return props + } + + out := maps.Clone(props) + delete(out, storPoolNameDrbdMeta) + + if len(out) == 0 { + return nil + } + + return out +} + +// isExternalMetaPool says whether a meta pool value names a separate pool: +// set, and not ".internal", read as LINSTOR reads it (trimmed, any case). +func isExternalMetaPool(pool string) bool { + pool = strings.TrimSpace(pool) + + return pool != "" && !strings.EqualFold(pool, drbdMetaInternal) +} + +// The layer suffix and the property LINSTOR marks external DRBD metadata +// with. +const ( + drbdMetaSuffix = ".meta" + storPoolNameDrbdMeta = "StorPoolNameDrbdMeta" + // drbdMetaInternal is the StorPoolNameDrbdMeta value that asks for + // internal metadata. + drbdMetaInternal = ".internal" +) + // rdHasVolumeDefinitions reports whether this resource definition owns a // volume that will convert. // @@ -1563,6 +1842,7 @@ func (c *converter) buildResource(row *ResourceRow, rdDsp, nodeDsp, replicaName c.reportReplicaVolumeFlags(row, replicaName) typed, residual, extra := k8sstore.SplitProps(c.props.Resource(row.NodeName, row.ResourceName)) + residual = c.withoutDroppedMetaPool(row.ResourceName, residual) resource := crdv1alpha1.Resource{ TypeMeta: typeMeta("Resource"), diff --git a/pkg/linstormigrate/external_metadata_test.go b/pkg/linstormigrate/external_metadata_test.go new file mode 100644 index 00000000..00f5949d --- /dev/null +++ b/pkg/linstormigrate/external_metadata_test.go @@ -0,0 +1,441 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package linstormigrate + +import ( + "os" + "path/filepath" + "slices" + "testing" +) + +// A replica whose DRBD metadata lives on a separate volume is never adopted: +// blockstor would bring it up with internal metadata and write it over the +// end of the data. The definition and all its replicas stay behind, and the +// report says which replica and why. The evidence is a ".meta" layer child, +// a meta pool on a DRBD volume, or, for a replica the dump has no chain +// for, StorPoolNameDrbdMeta in any scope LINSTOR reads it from. +func TestADefinitionWithExternalMetadataIsSkipped(t *testing.T) { + t.Parallel() + + meta := LayerResourceIDRow{ + LayerResourceID: 20, LayerResourceKind: "STORAGE", LayerResourceParentID: ptr(int32(1)), + LayerResourceSuffix: ".meta", NodeName: "NODE-A", ResourceName: "PVC-S", + } + chained := func() *Dump { + return stackDump("[]", layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE"), layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE")) + } + // The definition names its own stack, so #223 lets replicas with no + // chain through, and only the property can tell where their metadata is. + unchained := func(instance string) *Dump { + return withProps(stackDump(`["DRBD","STORAGE"]`), + PropsContainerRow{PropsInstance: instance, PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}) + } + + for name, tc := range map[string]struct { + dump *Dump + want string + }{ + "meta layer": { + dump: stackDump("[]", append(layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE"), meta), layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE")), + want: "the replica on node-a keeps its DRBD metadata on a separate volume", + }, + "meta pool on a DRBD volume": { + dump: func() *Dump { + d := chained() + d.LayerDrbdVolumes = []LayerDrbdVolumeRow{{LayerResourceID: 10, VlmNr: 0, NodeName: "NODE-B", PoolName: "meta"}} + + return d + }(), + want: "the replica on node-b keeps its DRBD metadata on a separate volume", + }, + "no chain, pool on the volume definition": {dump: unchained("/VLM_DFNS/PVC-S/0"), want: "on volume definition pvc-s/0"}, + "no chain, pool on the volume group": {dump: unchained("/VLM_GRPS/RG/0"), want: "on volume group RG/0"}, + "no chain, pool on the replica": {dump: unchained("/RSCS/NODE-A/PVC-S"), want: "on the replica on node-a"}, + "no chain, pool on the definition": {dump: unchained("/RSC_DFNS/PVC-S"), want: "on the definition"}, + "no chain, pool on the resource group": {dump: unchained("/RSC_GRPS/RG"), want: "on resource group RG"}, + "no chain, pool on the node": {dump: unchained("/NODES/NODE-B"), want: "on node node-b"}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + res := convertTwoReplicas(t, tc.dump) + + if len(res.ResourceDefinitions) != 0 || len(res.Resources) != 0 { + t.Fatalf("converted %d definitions and %d replicas, want none; warnings %v", + len(res.ResourceDefinitions), len(res.Resources), res.Warnings) + } + + if !hasWarning(res, tc.want) { + t.Errorf("the skip does not say %q; warnings %v", tc.want, res.Warnings) + } + + if !hasWarning(res, "copy its data to a volume with internal metadata") { + t.Errorf("the skip does not say how to move the data; warnings %v", res.Warnings) + } + }) + } +} + +// StorPoolNameDrbdMeta on replicas whose chains have no ".meta" child does +// not move their metadata: LINSTOR applies it to replicas created later, and +// ".internal" asks for internal metadata anyway. The definition converts, +// the property is left off the definition and its replicas, the scopes the +// dispatcher reads it from, and the report says the metadata stays where it +// is. +func TestAMetaPoolOverInternalMetadataConvertsWithoutIt(t *testing.T) { + t.Parallel() + + for name, prop := range map[string]PropsContainerRow{ + "pool on the volume definition": {PropsInstance: "/VLM_DFNS/PVC-S/0", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}, + "pool on the volume group": {PropsInstance: "/VLM_GRPS/RG/0", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}, + "pool on a replica": {PropsInstance: "/RSCS/NODE-B/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}, + "pool on the definition": {PropsInstance: "/RSC_DFNS/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}, + "pool on the resource group": {PropsInstance: "/RSC_GRPS/RG", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}, + "pool on a node": {PropsInstance: "/NODES/NODE-A", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}, + "internal on the definition": {PropsInstance: "/RSC_DFNS/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: ".internal"}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + res := convertTwoReplicas(t, withProps(stackDump("[]", + layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE"), layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE")), prop)) + + if len(res.ResourceDefinitions) != 1 || len(res.Resources) != 2 { + t.Fatalf("converted %d definitions and %d replicas, want the definition and both replicas; warnings %v", + len(res.ResourceDefinitions), len(res.Resources), res.Warnings) + } + + if v, ok := res.ResourceDefinitions[0].Spec.Props[storPoolNameDrbdMeta]; ok { + t.Errorf("the definition carries %s %q", storPoolNameDrbdMeta, v) + } + + for i := range res.Resources { + if v, ok := res.Resources[i].Spec.Props[storPoolNameDrbdMeta]; ok { + t.Errorf("replica %s carries %s %q", res.Resources[i].Name, storPoolNameDrbdMeta, v) + } + } + + if !hasWarning(res, "keep their DRBD metadata on the data device") || !hasWarning(res, "not carried over") { + t.Errorf("the report does not say the metadata stays internal; warnings %v", res.Warnings) + } + + if hasWarning(res, "separate volume") || hasWarning(res, "copy its data") || hasWarning(res, "skipped") { + t.Errorf("the report says the definition was skipped or the metadata is external; warnings %v", res.Warnings) + } + }) + } +} + +// LINSTOR resolves the meta pool per volume, and reads ".internal" trimmed +// and in any case. A replica without a chain is external when any of its +// volumes resolves to a separate pool, and internal when every one asks for +// internal metadata. +func TestAMetaPoolIsResolvedPerVolumeAsLINSTORReadsIt(t *testing.T) { + t.Parallel() + + twoVolumes := func(props ...PropsContainerRow) *Dump { + d := withProps(stackDump(`["DRBD","STORAGE"]`), props...) + d.VolumeDefinitions = append(d.VolumeDefinitions, VolumeDefinitionRow{ResourceName: "PVC-S", VlmNr: 1, VlmSize: 1048576, UUID: "vd-s1"}) + + return d + } + + t.Run("internal on volume 0, a pool on the definition", func(t *testing.T) { + t.Parallel() + + res := convertTwoReplicas(t, twoVolumes( + PropsContainerRow{PropsInstance: "/VLM_DFNS/PVC-S/0", PropKey: "StorPoolNameDrbdMeta", PropValue: ".internal"}, + PropsContainerRow{PropsInstance: "/RSC_DFNS/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"})) + + if len(res.ResourceDefinitions) != 0 { + t.Fatalf("converted %d definitions, want none: volume 1 keeps its metadata on meta; warnings %v", + len(res.ResourceDefinitions), res.Warnings) + } + + if !hasWarning(res, "meta on the definition would put its DRBD metadata on a separate volume") { + t.Errorf("the skip does not name the pool volume 1 resolves to; warnings %v", res.Warnings) + } + }) + + t.Run("internal in another case and with spaces", func(t *testing.T) { + t.Parallel() + + res := convertTwoReplicas(t, twoVolumes( + PropsContainerRow{PropsInstance: "/RSC_DFNS/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: " .INTERNAL "})) + + if len(res.ResourceDefinitions) != 1 || len(res.Resources) != 2 { + t.Fatalf("converted %d definitions and %d replicas, want the definition and both replicas; warnings %v", + len(res.ResourceDefinitions), len(res.Resources), res.Warnings) + } + }) +} + +// Rows that do not convert say nothing about where a live replica keeps its +// metadata: a diskless replica, a replica marked DELETE and a snapshot's +// rows, whatever their layout or property. +func TestRowsThatDoNotConvertDoNotDecideTheMetadata(t *testing.T) { + t.Parallel() + + meta := func(node string, id int32) LayerResourceIDRow { + return LayerResourceIDRow{ + LayerResourceID: id + 1, LayerResourceKind: "STORAGE", LayerResourceParentID: ptr(id), + LayerResourceSuffix: ".meta", NodeName: node, ResourceName: "PVC-S", + } + } + // The definition names its stack, so a replica without a chain is let + // through by #223 and only the metadata check can stop it. + named := func(chains ...[]LayerResourceIDRow) *Dump { return stackDump(`["DRBD","STORAGE"]`, chains...) } + + for name, tc := range map[string]struct { + dump *Dump + replicas []ResourceRow + }{ + "diskless replica with a pool and no chain": { + dump: withProps(named(layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE")), + PropsContainerRow{PropsInstance: "/RSCS/NODE-B/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}), + replicas: []ResourceRow{ + {NodeName: "NODE-A", ResourceName: "PVC-S", UUID: "r-a"}, + {NodeName: "NODE-B", ResourceName: "PVC-S", ResourceFlags: resourceFlagDiskless, UUID: "r-b"}, + }, + }, + "snapshot placement with a pool and no chain": { + dump: withProps(named(layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE")), + PropsContainerRow{PropsInstance: "/RSCS/NODE-B/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}), + replicas: []ResourceRow{ + {NodeName: "NODE-A", ResourceName: "PVC-S", UUID: "r-a"}, + {NodeName: "NODE-B", ResourceName: "PVC-S", SnapshotName: "SNAP", UUID: "s-b"}, + }, + }, + "snapshot volume definition with a pool": { + dump: func() *Dump { + d := withProps(named(layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE")), + PropsContainerRow{PropsInstance: "/VLM_DFNS/PVC-S/7", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}) + d.VolumeDefinitions = append(d.VolumeDefinitions, + VolumeDefinitionRow{ResourceName: "PVC-S", SnapshotName: "SNAP", VlmNr: 7, VlmSize: 1048576, UUID: "svd"}) + + return d + }(), + replicas: []ResourceRow{ + {NodeName: "NODE-A", ResourceName: "PVC-S", UUID: "r-a"}, + {NodeName: "NODE-B", ResourceName: "PVC-S", UUID: "r-b"}, + }, + }, + "snapshot layer row with a meta child": { + dump: func() *Dump { + d := named(layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE"), layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE")) + m := meta("NODE-A", 40) + m.SnapshotName = "SNAP" + d.LayerResourceIDs = append(d.LayerResourceIDs, m) + + return d + }(), + replicas: []ResourceRow{ + {NodeName: "NODE-A", ResourceName: "PVC-S", UUID: "r-a"}, + {NodeName: "NODE-B", ResourceName: "PVC-S", UUID: "r-b"}, + }, + }, + "meta child on a replica marked DELETE": { + dump: named(layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE"), + append(layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE"), meta("NODE-B", 10))), + replicas: []ResourceRow{ + {NodeName: "NODE-A", ResourceName: "PVC-S", UUID: "r-a"}, + {NodeName: "NODE-B", ResourceName: "PVC-S", ResourceFlags: resourceFlagDelete, UUID: "r-b"}, + }, + }, + "internal pool and no chain": { + dump: withProps(named(layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE")), + PropsContainerRow{PropsInstance: "/RSCS/NODE-B/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: ".internal"}), + replicas: []ResourceRow{ + {NodeName: "NODE-A", ResourceName: "PVC-S", UUID: "r-a"}, + {NodeName: "NODE-B", ResourceName: "PVC-S", UUID: "r-b"}, + }, + }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + tc.dump.Resources = tc.replicas + + res, err := Convert(tc.dump) + if err != nil { + t.Fatalf("Convert: %v", err) + } + + if len(res.ResourceDefinitions) != 1 { + t.Fatalf("converted %d definitions, want pvc-s; warnings %v", len(res.ResourceDefinitions), res.Warnings) + } + + if hasWarning(res, "separate volume") { + t.Errorf("the metadata was reported external; warnings %v", res.Warnings) + } + }) + } +} + +// The meta pool of a DRBD volume loads from the dump as kubectl prints it, +// a null pool included. +func TestTheDRBDVolumeMetaPoolLoadsFromTheDump(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + body := `{"items":[` + + `{"spec":{"layer_resource_id":10,"vlm_nr":0,"node_name":"NODE-B","pool_name":"meta"}},` + + `{"spec":{"layer_resource_id":11,"vlm_nr":0,"node_name":null,"pool_name":null}}]}` + + for table, data := range map[string]string{ + "layerdrbdvolumes": body, + "nodes": `{"items":[{"spec":{"node_name":"NODE-B","node_dsp_name":"node-b","node_type":2,"uuid":"n-b"}}]}`, + } { + err := os.WriteFile(filepath.Join(dir, table+dumpFileSuffix), []byte(data), 0o600) + if err != nil { + t.Fatal(err) + } + } + + dump, err := LoadDump(dir) + if err != nil { + t.Fatalf("LoadDump: %v", err) + } + + want := []LayerDrbdVolumeRow{{LayerResourceID: 10, NodeName: "NODE-B", PoolName: "meta"}, {LayerResourceID: 11}} + if !slices.Equal(dump.LayerDrbdVolumes, want) { + t.Fatalf("loaded %+v, want %+v", dump.LayerDrbdVolumes, want) + } +} + +// One definition's metadata says nothing about another's: a neighbour on +// other nodes with internal metadata still converts. +func TestExternalMetadataSkipsOnlyItsOwnDefinition(t *testing.T) { + t.Parallel() + + meta := LayerResourceIDRow{ + LayerResourceID: 20, LayerResourceKind: "STORAGE", LayerResourceParentID: ptr(int32(1)), + LayerResourceSuffix: ".meta", NodeName: "NODE-A", ResourceName: "PVC-S", + } + + for name, dump := range map[string]*Dump{ + "meta layer on the other definition": stackDump("[]", + append(layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE"), meta), layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE")), + // PVC-T has no replica on NODE-B, so the node's property is not its. + // PVC-S names its stack and has no chain on NODE-B, so the property + // decides for it. + "meta pool on a node the other definition is not on": withProps(stackDump(`["DRBD","STORAGE"]`, + layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE")), + PropsContainerRow{PropsInstance: "/NODES/NODE-B", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}), + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + dump.ResourceDefinitions = append(dump.ResourceDefinitions, + ResourceDefinitionRow{ResourceName: "PVC-T", ResourceDspName: "pvc-t", ResourceGroupName: "RG", LayerStack: "[]", UUID: "rd-t"}) + dump.VolumeDefinitions = append(dump.VolumeDefinitions, VolumeDefinitionRow{ResourceName: "PVC-T", VlmNr: 0, VlmSize: 1048576, UUID: "vd-t"}) + dump.LayerResourceIDs = append(dump.LayerResourceIDs, layerChain(30, "NODE-A", "PVC-T", "DRBD", "STORAGE")...) + dump.Resources = []ResourceRow{ + {NodeName: "NODE-A", ResourceName: "PVC-S", UUID: "r-a"}, + {NodeName: "NODE-B", ResourceName: "PVC-S", UUID: "r-b"}, + {NodeName: "NODE-A", ResourceName: "PVC-T", UUID: "r-t"}, + } + + res, err := Convert(dump) + if err != nil { + t.Fatalf("Convert: %v", err) + } + + if len(res.ResourceDefinitions) != 1 || res.ResourceDefinitions[0].Name != "pvc-t" || len(res.Resources) != 1 { + t.Fatalf("converted %d definitions and %d replicas, want pvc-t and its replica; warnings %v", + len(res.ResourceDefinitions), len(res.Resources), res.Warnings) + } + }) + } +} + +// convertTwoReplicas places a replica of PVC-S on each node of dump and +// converts it. +func convertTwoReplicas(t *testing.T, dump *Dump) *Result { + t.Helper() + + dump.Resources = []ResourceRow{ + {NodeName: "NODE-A", ResourceName: "PVC-S", UUID: "r-a"}, + {NodeName: "NODE-B", ResourceName: "PVC-S", UUID: "r-b"}, + } + + res, err := Convert(dump) + if err != nil { + t.Fatalf("Convert: %v", err) + } + + return res +} + +func withProps(dump *Dump, rows ...PropsContainerRow) *Dump { + dump.PropsContainers = append(dump.PropsContainers, rows...) + + return dump +} + +// A meta pool says nothing about a replica that is going away or a +// definition with no DRBD layer: neither keeps DRBD metadata, so the +// definition converts. +func TestAMetaPoolWithoutDRBDMetadataBehindItConverts(t *testing.T) { + t.Parallel() + + for name, tc := range map[string]struct { + dump *Dump + replicas []ResourceRow + }{ + "on a replica marked DELETE": { + // NODE-B has no chain, so without the DELETE filter its property + // would decide alone. + dump: withProps(stackDump(`["DRBD","STORAGE"]`, layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE")), + PropsContainerRow{PropsInstance: "/RSCS/NODE-B/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}), + replicas: []ResourceRow{ + {NodeName: "NODE-A", ResourceName: "PVC-S", UUID: "r-a"}, + {NodeName: "NODE-B", ResourceName: "PVC-S", ResourceFlags: resourceFlagDelete, UUID: "r-b"}, + }, + }, + "on a definition without DRBD": { + dump: withProps(stackDump(`["STORAGE"]`), + PropsContainerRow{PropsInstance: "/RSC_GRPS/RG", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}), + replicas: []ResourceRow{ + {NodeName: "NODE-A", ResourceName: "PVC-S", UUID: "r-a"}, + }, + }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + tc.dump.Resources = tc.replicas + + res, err := Convert(tc.dump) + if err != nil { + t.Fatalf("Convert: %v", err) + } + + if len(res.ResourceDefinitions) != 1 || len(res.Resources) != 1 { + t.Fatalf("converted %d definitions and %d replicas, want the definition and its live replica; warnings %v", + len(res.ResourceDefinitions), len(res.Resources), res.Warnings) + } + + if hasWarning(res, storPoolNameDrbdMeta) { + t.Errorf("the meta pool was reported; warnings %v", res.Warnings) + } + }) + } +} diff --git a/pkg/linstormigrate/layer_stack_test.go b/pkg/linstormigrate/layer_stack_test.go index 5e8eff4b..b7552539 100644 --- a/pkg/linstormigrate/layer_stack_test.go +++ b/pkg/linstormigrate/layer_stack_test.go @@ -110,15 +110,17 @@ func TestAnEmptyRDStackIsReadOffItsReplicas(t *testing.T) { } } -// Snapshot instances and suffixed children (DRBD external metadata) -// are not the replica's data path and do not shape its stack. +// Snapshot instances and suffixed children (a cache's sub-devices) are +// not the replica's data path and do not shape its stack. External DRBD +// metadata, the ".meta" child, refuses the definition instead +// (TestADefinitionWithExternalMetadataIsSkipped). func TestSnapshotAndSuffixedLayersDoNotShapeTheStack(t *testing.T) { t.Parallel() - meta := LayerResourceIDRow{LayerResourceID: 20, LayerResourceKind: "STORAGE", LayerResourceParentID: ptr(int32(1)), LayerResourceSuffix: ".meta", NodeName: "NODE-A", ResourceName: "PVC-S"} + cache := LayerResourceIDRow{LayerResourceID: 20, LayerResourceKind: "STORAGE", LayerResourceParentID: ptr(int32(1)), LayerResourceSuffix: ".dcache", NodeName: "NODE-A", ResourceName: "PVC-S"} snap := LayerResourceIDRow{LayerResourceID: 21, LayerResourceKind: "STORAGE", NodeName: "NODE-A", ResourceName: "PVC-S", SnapshotName: "SNAP"} - got, res := convertedStack(t, stackDump("[]", layerChain(1, "NODE-A", "PVC-S", "DRBD", "LUKS", "STORAGE"), []LayerResourceIDRow{meta, snap})) + got, res := convertedStack(t, stackDump("[]", layerChain(1, "NODE-A", "PVC-S", "DRBD", "LUKS", "STORAGE"), []LayerResourceIDRow{cache, snap})) if !slices.Equal(got, []string{"DRBD", "LUKS", "STORAGE"}) { t.Fatalf("stack %v; warnings %v", got, res.Warnings) } diff --git a/pkg/linstormigrate/model.go b/pkg/linstormigrate/model.go index 6ecfe97e..fbc48506 100644 --- a/pkg/linstormigrate/model.go +++ b/pkg/linstormigrate/model.go @@ -206,6 +206,11 @@ type LayerDrbdVolumeDefinitionRow struct { type LayerDrbdVolumeRow struct { LayerResourceID int32 `json:"layer_resource_id"` VlmNr int32 `json:"vlm_nr"` + // NodeName and PoolName name the storage pool that holds the + // volume's external DRBD metadata; both are empty for internal + // metadata. + NodeName string `json:"node_name,omitempty"` + PoolName string `json:"pool_name,omitempty"` } // LayerStorageVolumeRow is one row of LAYER_STORAGE_VOLUMES — which diff --git a/pkg/linstormigrate/props.go b/pkg/linstormigrate/props.go index 004f2600..5ad31546 100644 --- a/pkg/linstormigrate/props.go +++ b/pkg/linstormigrate/props.go @@ -116,6 +116,11 @@ func (idx *PropsIndex) VolumeDefinition(rd, vlmNr string) map[string]string { return idx.get("VLM_DFNS", rd, vlmNr) } +// VolumeGroup returns the /VLM_GRPS// props. +func (idx *PropsIndex) VolumeGroup(rg, vlmNr string) map[string]string { + return idx.get("VLM_GRPS", rg, vlmNr) +} + // Resource returns the /RSCS// props. func (idx *PropsIndex) Resource(node, rd string) map[string]string { return idx.get("RSCS", node, rd) diff --git a/site/content/en/docs/migration.md b/site/content/en/docs/migration.md index 6d2ae096..5f489f64 100644 --- a/site/content/en/docs/migration.md +++ b/site/content/en/docs/migration.md @@ -78,6 +78,8 @@ linstor-migrate -in ./dump -drbd-ports ./drbd-ports.txt -out ./blockstor-manifes Read the whole report it prints. Each `warning:` line is something the tool refused to guess. Resolve every one of these before you go on: - `LUKS passphrase NOT migrated`. +- `keeps its DRBD metadata on a separate volume`, or `would put its DRBD metadata on a separate volume` for a replica the dump has no layer chain for. The definition is skipped with its replicas: blockstor keeps DRBD metadata on the data device, and adopted as it is the replica would get new metadata over the end of its data. Changing the property does not move existing metadata. For a replica without a chain, check in LINSTOR where its metadata is first. If it is on the data device, remove the property and convert again. Otherwise, copy the data to a volume with internal metadata in LINSTOR and convert that one. +- `keep their DRBD metadata on the data device, though StorPoolNameDrbdMeta ... is set`. Nothing to do: the metadata is internal, because LINSTOR applies the property only to replicas created after it was set, and `.internal` asks for internal metadata. The definition converts, and the property is not carried over to the definition or its replicas, so blockstor does not look for a separate metadata volume. - `no DRBD port`. - `skipped with its replicas`. The definition's replicas cannot be adopted under one layer stack: they were built two ways, a diskful replica has no layer chain in the dump, or a replica was built otherwise than the stack the definition names. The reason in the line says which. An unreadable `layer_stack` column counts as no stack, and the line names its value. Adopted as it is, such a replica would come 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. - `unreadable layer_stack`. The definition's `layer_stack` column is not a list the converter can read. When its replicas agree, it takes the stack they were built with and the line names it; check that this is the stack you expect. With no diskful replica the stack is left unset and the controller pins one; see [How the stack is pinned](../layer-stack/#how-the-stack-is-pinned). From a4be104f78b2b0f75a31aa5e2947d3a1284cf4ae Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Wed, 4 Nov 2026 16:42:10 +0100 Subject: [PATCH 2/5] fix(linstor-migrate): keep a dropped meta pool out of the overflow props SplitProps routes a non-DRBD key to the residual props and to the overflow map both, and only the residual copy was taken out. The store merges the overflow back into the props on every write, so the first ordinary property update brought StorPoolNameDrbdMeta back and sent the adopted replica down the external-metadata path. Take it out before splitting. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- pkg/linstormigrate/convert.go | 11 +- .../meta_pool_round_trip_test.go | 110 ++++++++++++++++++ 2 files changed, 116 insertions(+), 5 deletions(-) create mode 100644 pkg/linstormigrate/meta_pool_round_trip_test.go diff --git a/pkg/linstormigrate/convert.go b/pkg/linstormigrate/convert.go index 0e9b93e5..d4ee902a 100644 --- a/pkg/linstormigrate/convert.go +++ b/pkg/linstormigrate/convert.go @@ -1121,8 +1121,7 @@ func (c *converter) convertResourceDefinitions() []crdv1alpha1.ResourceDefinitio rgRef = "" } - typed, residual, extra := k8sstore.SplitProps(c.props.ResourceDefinition(row.ResourceName)) - residual = c.withoutDroppedMetaPool(row.ResourceName, residual) + typed, residual, extra := k8sstore.SplitProps(c.withoutDroppedMetaPool(row.ResourceName, c.props.ResourceDefinition(row.ResourceName))) latch := c.rdInitializedLatch(row.ResourceName) @@ -1436,7 +1435,10 @@ func (c *converter) resourceGroupVolumeProps(rg, vlm string) map[string]string { } // withoutDroppedMetaPool takes StorPoolNameDrbdMeta out of the props of a -// definition named in metaPoolDropped, or of one of its replicas. +// definition named in metaPoolDropped, or of one of its replicas. It runs +// before SplitProps, which routes a non-DRBD key to the residual props and to +// the overflow map both: the store merges the overflow back into the props on +// every write, so a copy left there returns with the next property update. func (c *converter) withoutDroppedMetaPool(rdName string, props map[string]string) map[string]string { if !c.metaPoolDropped[rdName] { return props @@ -1841,8 +1843,7 @@ func (c *converter) buildResource(row *ResourceRow, rdDsp, nodeDsp, replicaName c.reportReplicaVolumeFlags(row, replicaName) - typed, residual, extra := k8sstore.SplitProps(c.props.Resource(row.NodeName, row.ResourceName)) - residual = c.withoutDroppedMetaPool(row.ResourceName, residual) + typed, residual, extra := k8sstore.SplitProps(c.withoutDroppedMetaPool(row.ResourceName, c.props.Resource(row.NodeName, row.ResourceName))) resource := crdv1alpha1.Resource{ TypeMeta: typeMeta("Resource"), diff --git a/pkg/linstormigrate/meta_pool_round_trip_test.go b/pkg/linstormigrate/meta_pool_round_trip_test.go new file mode 100644 index 00000000..df5eb80e --- /dev/null +++ b/pkg/linstormigrate/meta_pool_round_trip_test.go @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package linstormigrate + +import ( + "testing" + + "k8s.io/apimachinery/pkg/runtime" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + crdv1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + k8sstore "github.com/cozystack/blockstor/pkg/store/k8s" +) + +// A meta pool left off a converted definition and its replicas stays off +// after an ordinary property update through the store: the store merges the +// overflow props back into the props on every write, so a copy left there +// would return and send the adopted replica down the external-metadata path. +func TestADroppedMetaPoolStaysOffThroughAStoreUpdate(t *testing.T) { + t.Parallel() + + res := convertTwoReplicas(t, withProps(stackDump("[]", + layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE"), layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE")), + PropsContainerRow{PropsInstance: "/RSC_DFNS/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}, + PropsContainerRow{PropsInstance: "/RSCS/NODE-A/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"})) + + if len(res.ResourceDefinitions) != 1 || len(res.Resources) != 2 { + t.Fatalf("converted %d definitions and %d replicas; warnings %v", len(res.ResourceDefinitions), len(res.Resources), res.Warnings) + } + + scheme := runtime.NewScheme() + if err := crdv1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme: %v", err) + } + + objs := make([]ctrlclient.Object, 0, 1+len(res.Resources)) + objs = append(objs, &res.ResourceDefinitions[0]) + for i := range res.Resources { + objs = append(objs, &res.Resources[i]) + } + + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build() + st := k8sstore.New(cli) + ctx := t.Context() + + addAux := func(props map[string]string) map[string]string { + if props == nil { + props = map[string]string{} + } + + props["Aux/example"] = "value" + + return props + } + + if err := st.ResourceDefinitions().PatchResourceDefinitionSpec(ctx, "pvc-s", func(rd *apiv1.ResourceDefinition) error { + rd.Props = addAux(rd.Props) + + return nil + }); err != nil { + t.Fatalf("update the definition: %v", err) + } + + if err := st.Resources().PatchResourceSpec(ctx, "pvc-s", "node-a", func(r *apiv1.Resource) error { + r.Props = addAux(r.Props) + + return nil + }); err != nil { + t.Fatalf("update the replica: %v", err) + } + + // The dispatcher takes the meta pool from these two Props maps. + var rd crdv1alpha1.ResourceDefinition + if err := cli.Get(ctx, ctrlclient.ObjectKeyFromObject(&res.ResourceDefinitions[0]), &rd); err != nil { + t.Fatalf("read the definition back: %v", err) + } + + if v, ok := rd.Spec.Props[storPoolNameDrbdMeta]; ok { + t.Errorf("the definition carries %s %q after an update", storPoolNameDrbdMeta, v) + } + + for i := range res.Resources { + var r crdv1alpha1.Resource + if err := cli.Get(ctx, ctrlclient.ObjectKeyFromObject(&res.Resources[i]), &r); err != nil { + t.Fatalf("read replica %s back: %v", res.Resources[i].Name, err) + } + + if v, ok := r.Spec.Props[storPoolNameDrbdMeta]; ok { + t.Errorf("replica %s carries %s %q after an update", r.Name, storPoolNameDrbdMeta, v) + } + } +} From d2af90cd10b61aa49940408bb8f90c4197043f9d Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Wed, 4 Nov 2026 16:42:11 +0100 Subject: [PATCH 3/5] fix(linstor-migrate): match ".internal" untrimmed, as LINSTOR does LINSTOR trims a meta pool value only to tell whether it is set, and compares it with ".internal" as it is, ignoring case. A value with spaces around ".internal" is a pool name there, so it is one here too. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- pkg/linstormigrate/convert.go | 9 +++--- pkg/linstormigrate/external_metadata_test.go | 34 +++++++++++++++++--- 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/pkg/linstormigrate/convert.go b/pkg/linstormigrate/convert.go index d4ee902a..9dfa85c1 100644 --- a/pkg/linstormigrate/convert.go +++ b/pkg/linstormigrate/convert.go @@ -1458,12 +1458,11 @@ func (c *converter) withoutDroppedMetaPool(rdName string, props map[string]strin return out } -// isExternalMetaPool says whether a meta pool value names a separate pool: -// set, and not ".internal", read as LINSTOR reads it (trimmed, any case). +// isExternalMetaPool says whether a meta pool value names a separate pool, +// read as LINSTOR v1.33.0 reads it: blank is unset, and ".internal" is matched +// in any case but untrimmed, so a value with spaces around it is a pool name. func isExternalMetaPool(pool string) bool { - pool = strings.TrimSpace(pool) - - return pool != "" && !strings.EqualFold(pool, drbdMetaInternal) + return strings.TrimSpace(pool) != "" && !strings.EqualFold(pool, drbdMetaInternal) } // The layer suffix and the property LINSTOR marks external DRBD metadata diff --git a/pkg/linstormigrate/external_metadata_test.go b/pkg/linstormigrate/external_metadata_test.go index 00f5949d..c7da5bce 100644 --- a/pkg/linstormigrate/external_metadata_test.go +++ b/pkg/linstormigrate/external_metadata_test.go @@ -143,10 +143,10 @@ func TestAMetaPoolOverInternalMetadataConvertsWithoutIt(t *testing.T) { } } -// LINSTOR resolves the meta pool per volume, and reads ".internal" trimmed -// and in any case. A replica without a chain is external when any of its -// volumes resolves to a separate pool, and internal when every one asks for -// internal metadata. +// LINSTOR resolves the meta pool per volume, and matches ".internal" in any +// case but untrimmed: spaces around it make it a pool name. A replica without +// a chain is external when any of its volumes resolves to a separate pool, +// and internal when every one asks for internal metadata. func TestAMetaPoolIsResolvedPerVolumeAsLINSTORReadsIt(t *testing.T) { t.Parallel() @@ -174,12 +174,36 @@ func TestAMetaPoolIsResolvedPerVolumeAsLINSTORReadsIt(t *testing.T) { } }) - t.Run("internal in another case and with spaces", func(t *testing.T) { + t.Run("internal in another case", func(t *testing.T) { + t.Parallel() + + res := convertTwoReplicas(t, twoVolumes( + PropsContainerRow{PropsInstance: "/RSC_DFNS/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: ".INTERNAL"})) + + if len(res.ResourceDefinitions) != 1 || len(res.Resources) != 2 { + t.Fatalf("converted %d definitions and %d replicas, want the definition and both replicas; warnings %v", + len(res.ResourceDefinitions), len(res.Resources), res.Warnings) + } + }) + + t.Run("internal with spaces around it is a pool name", func(t *testing.T) { t.Parallel() res := convertTwoReplicas(t, twoVolumes( PropsContainerRow{PropsInstance: "/RSC_DFNS/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: " .INTERNAL "})) + if len(res.ResourceDefinitions) != 0 { + t.Fatalf("converted %d definitions, want none: LINSTOR reads %q as a pool; warnings %v", + len(res.ResourceDefinitions), " .INTERNAL ", res.Warnings) + } + }) + + t.Run("a blank value is unset", func(t *testing.T) { + t.Parallel() + + res := convertTwoReplicas(t, twoVolumes( + PropsContainerRow{PropsInstance: "/RSC_DFNS/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: " "})) + if len(res.ResourceDefinitions) != 1 || len(res.Resources) != 2 { t.Fatalf("converted %d definitions and %d replicas, want the definition and both replicas; warnings %v", len(res.ResourceDefinitions), len(res.Resources), res.Warnings) From d44ff561ebe2167afc4634b7c1941ef07693a668 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Wed, 4 Nov 2026 17:31:41 +0100 Subject: [PATCH 4/5] fix(linstor-migrate): judge the meta pool by what converts An unreadable layer_stack was decoded in part when deciding whether a definition runs DRBD, so the meta pool property survived on a definition with internal metadata. A volume marked DELETE, which is not converted, could keep its live neighbours from migrating. The stack is now read as everywhere else in the converter, and deleted volumes are left out. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- pkg/linstormigrate/convert.go | 11 +++-- pkg/linstormigrate/external_metadata_test.go | 48 ++++++++++++++++++++ 2 files changed, 54 insertions(+), 5 deletions(-) diff --git a/pkg/linstormigrate/convert.go b/pkg/linstormigrate/convert.go index 9dfa85c1..8b330178 100644 --- a/pkg/linstormigrate/convert.go +++ b/pkg/linstormigrate/convert.go @@ -1292,10 +1292,9 @@ func (c *converter) externalMetadataReason(row *ResourceDefinitionRow) (string, // which stackDisagreement has already found to be one; with neither, the // controller pins DRBD over STORAGE. func (c *converter) runsDRBD(row *ResourceDefinitionRow) bool { - var stack []string - if !emptyList(row.LayerStack) { - _ = json.Unmarshal([]byte(row.LayerStack), &stack) - } + // An unreadable column counts as no stack, as everywhere else in the + // converter: a partial decode of it says nothing about DRBD. + stack, _ := rowLayerStack(row) if len(stack) == 0 { for _, onDisk := range c.diskStacks[row.ResourceName] { @@ -1385,7 +1384,9 @@ func (c *converter) drbdMetaPools(row *ResourceDefinitionRow, node string) []met for i := range c.dump.VolumeDefinitions { vd := &c.dump.VolumeDefinitions[i] - if vd.ResourceName != rd || vd.SnapshotName != "" { + // A volume marked DELETE is not converted, so its metadata + // setting does not decide anything for the live ones. + if vd.ResourceName != rd || vd.SnapshotName != "" || vd.VlmFlags&resourceFlagDelete != 0 { continue } diff --git a/pkg/linstormigrate/external_metadata_test.go b/pkg/linstormigrate/external_metadata_test.go index c7da5bce..e1084ac7 100644 --- a/pkg/linstormigrate/external_metadata_test.go +++ b/pkg/linstormigrate/external_metadata_test.go @@ -463,3 +463,51 @@ func TestAMetaPoolWithoutDRBDMetadataBehindItConverts(t *testing.T) { }) } } + +// An unreadable layer_stack counts as no stack here too, so the replicas' +// chains decide: DRBD over STORAGE with internal metadata, and the meta pool +// property is dropped instead of surviving a partial decode of the column. +func TestAnUnreadableStackStillDropsTheMetaPool(t *testing.T) { + t.Parallel() + + res := convertTwoReplicas(t, withProps(stackDump(`["STORAGE",4]`, + layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE"), layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE")), + PropsContainerRow{PropsInstance: "/RSC_DFNS/PVC-S", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"})) + + if len(res.ResourceDefinitions) != 1 || len(res.Resources) != 2 { + t.Fatalf("converted %d definitions and %d replicas, want the definition and both replicas; warnings %v", + len(res.ResourceDefinitions), len(res.Resources), res.Warnings) + } + + rd := res.ResourceDefinitions[0] + if _, ok := rd.Spec.Props[storPoolNameDrbdMeta]; ok { + t.Errorf("the definition's props carry %s", storPoolNameDrbdMeta) + } + + if _, ok := rd.Spec.ExtraProps[storPoolNameDrbdMeta]; ok { + t.Errorf("the definition's extra props carry %s", storPoolNameDrbdMeta) + } +} + +// A volume marked DELETE is not converted, so its meta pool does not stop the +// live volumes: the definition converts with the live volume alone. +func TestADeletedVolumesMetaPoolDoesNotStopTheLiveOnes(t *testing.T) { + t.Parallel() + + dump := withProps(stackDump(`["DRBD","STORAGE"]`), + PropsContainerRow{PropsInstance: "/VLM_DFNS/PVC-S/0", PropKey: "StorPoolNameDrbdMeta", PropValue: "meta"}, + PropsContainerRow{PropsInstance: "/VLM_DFNS/PVC-S/1", PropKey: "StorPoolNameDrbdMeta", PropValue: ".internal"}) + dump.VolumeDefinitions[0].VlmFlags = resourceFlagDelete + dump.VolumeDefinitions = append(dump.VolumeDefinitions, VolumeDefinitionRow{ResourceName: "PVC-S", VlmNr: 1, VlmSize: 1048576, UUID: "vd-s1"}) + + res := convertTwoReplicas(t, dump) + if len(res.ResourceDefinitions) != 1 || len(res.Resources) != 2 { + t.Fatalf("converted %d definitions and %d replicas, want the definition and both replicas; warnings %v", + len(res.ResourceDefinitions), len(res.Resources), res.Warnings) + } + + vds := res.ResourceDefinitions[0].Spec.VolumeDefinitions + if len(vds) != 1 || vds[0].VolumeNumber != 1 { + t.Errorf("volumes %+v, want volume 1 alone", vds) + } +} From f3f5e238b9716586235cc13d5e806900f9d7898b Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Wed, 4 Nov 2026 18:32:42 +0100 Subject: [PATCH 5/5] fix(linstor-migrate): judge a replica's metadata layout by live volumes A meta pool on a deleted volume's DRBD row, or a ".meta" child holding only that volume, still kept the live volumes of the definition from migrating. Both layout checks now look at the volumes that convert. A ".meta" child the dump shows no volume for still counts, since nothing says which volumes it holds. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- pkg/linstormigrate/convert.go | 86 +++++++++++++++++--- pkg/linstormigrate/external_metadata_test.go | 78 ++++++++++++++++++ 2 files changed, 152 insertions(+), 12 deletions(-) diff --git a/pkg/linstormigrate/convert.go b/pkg/linstormigrate/convert.go index 8b330178..9db8bd61 100644 --- a/pkg/linstormigrate/convert.go +++ b/pkg/linstormigrate/convert.go @@ -1307,19 +1307,15 @@ func (c *converter) runsDRBD(row *ResourceDefinitionRow) bool { // externalMetadataReplica names the node of a replica of rdName that keeps // its DRBD metadata on a separate volume, by its ".meta" layer child or by -// the meta pool on one of its DRBD volumes, or "" when none does. A replica -// marked DELETE does not convert, so its layout does not count. +// the meta pool on one of its DRBD volumes, or "" when none does. Only what +// converts counts: a replica marked DELETE, and a volume marked DELETE, do +// not. A ".meta" child the dump has no storage rows for still counts, since +// nothing says which volumes it holds. func (c *converter) externalMetadataReplica(rdName string) string { - deleted := map[string]bool{} - - for i := range c.dump.Resources { - res := &c.dump.Resources[i] - if res.ResourceName == rdName && res.SnapshotName == "" && res.ResourceFlags&resourceFlagDelete != 0 { - deleted[res.NodeName] = true - } - } + deleted, live := c.deletedReplicaNodes(rdName), c.liveVolumes(rdName) byID := map[int32]*LayerResourceIDRow{} + metaChildren := map[int32]*LayerResourceIDRow{} for i := range c.dump.LayerResourceIDs { lri := &c.dump.LayerResourceIDs[i] @@ -1328,15 +1324,21 @@ func (c *converter) externalMetadataReplica(rdName string) string { } if lri.LayerResourceSuffix == drbdMetaSuffix { - return lri.NodeName + metaChildren[lri.LayerResourceID] = lri + + continue } byID[lri.LayerResourceID] = lri } + if node := c.liveMetaChild(metaChildren, live); node != "" { + return node + } + for i := range c.dump.LayerDrbdVolumes { vol := &c.dump.LayerDrbdVolumes[i] - if lri, ok := byID[vol.LayerResourceID]; ok && isExternalMetaPool(vol.PoolName) { + if lri, ok := byID[vol.LayerResourceID]; ok && live[vol.VlmNr] && isExternalMetaPool(vol.PoolName) { return lri.NodeName } } @@ -1344,6 +1346,66 @@ func (c *converter) externalMetadataReplica(rdName string) string { return "" } +// deletedReplicaNodes is the nodes whose replica of rdName is marked DELETE. +func (c *converter) deletedReplicaNodes(rdName string) map[string]bool { + deleted := map[string]bool{} + + for i := range c.dump.Resources { + res := &c.dump.Resources[i] + if res.ResourceName == rdName && res.SnapshotName == "" && res.ResourceFlags&resourceFlagDelete != 0 { + deleted[res.NodeName] = true + } + } + + return deleted +} + +// liveVolumes is the volume numbers of rdName that are not marked DELETE. +func (c *converter) liveVolumes(rdName string) map[int32]bool { + live := map[int32]bool{} + + for i := range c.dump.VolumeDefinitions { + vd := &c.dump.VolumeDefinitions[i] + if vd.ResourceName == rdName && vd.SnapshotName == "" && vd.VlmFlags&resourceFlagDelete == 0 { + live[vd.VlmNr] = true + } + } + + return live +} + +// liveMetaChild names the node of a ".meta" child that holds a volume in +// live, or that the dump shows no volume for. +func (c *converter) liveMetaChild(children map[int32]*LayerResourceIDRow, live map[int32]bool) string { + if len(children) == 0 { + return "" + } + + vols := map[int32][]int32{} + + for i := range c.dump.LayerStorageVolumes { + row := &c.dump.LayerStorageVolumes[i] + if _, ok := children[row.LayerResourceID]; ok { + vols[row.LayerResourceID] = append(vols[row.LayerResourceID], row.VlmNr) + } + } + + ids := make([]int32, 0, len(children)) + for id := range children { + ids = append(ids, id) + } + + slices.Sort(ids) + + for _, id := range ids { + if len(vols[id]) == 0 || slices.ContainsFunc(vols[id], func(nr int32) bool { return live[nr] }) { + return children[id].NodeName + } + } + + return "" +} + // metaPool is the StorPoolNameDrbdMeta one volume resolves to, and the // scope it came from. type metaPool struct { diff --git a/pkg/linstormigrate/external_metadata_test.go b/pkg/linstormigrate/external_metadata_test.go index e1084ac7..c3d1326e 100644 --- a/pkg/linstormigrate/external_metadata_test.go +++ b/pkg/linstormigrate/external_metadata_test.go @@ -511,3 +511,81 @@ func TestADeletedVolumesMetaPoolDoesNotStopTheLiveOnes(t *testing.T) { t.Errorf("volumes %+v, want volume 1 alone", vds) } } + +// The layout of a volume marked DELETE does not stop its live neighbour +// either: neither a meta pool on its DRBD volume nor a ".meta" child that +// holds it alone. +func TestADeletedVolumesLayoutDoesNotStopTheLiveOnes(t *testing.T) { + t.Parallel() + + twoVolumes := func(d *Dump) *Dump { + d.VolumeDefinitions[0].VlmFlags = resourceFlagDelete + d.VolumeDefinitions = append(d.VolumeDefinitions, VolumeDefinitionRow{ResourceName: "PVC-S", VlmNr: 1, VlmSize: 1048576, UUID: "vd-s1"}) + + return d + } + + for name, dump := range map[string]*Dump{ + "meta pool on the deleted volume": func() *Dump { + d := twoVolumes(stackDump(`["DRBD","STORAGE"]`, + layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE"), layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE"))) + d.LayerDrbdVolumes = []LayerDrbdVolumeRow{ + {LayerResourceID: 1, VlmNr: 0, NodeName: "NODE-A", PoolName: "meta"}, + {LayerResourceID: 1, VlmNr: 1}, + {LayerResourceID: 10, VlmNr: 0, NodeName: "NODE-B", PoolName: "meta"}, + {LayerResourceID: 10, VlmNr: 1}, + } + + return d + }(), + "meta child holding only the deleted volume": func() *Dump { + d := twoVolumes(stackDump(`["DRBD","STORAGE"]`, + layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE"), layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE"))) + d.LayerResourceIDs = append(d.LayerResourceIDs, LayerResourceIDRow{ + LayerResourceID: 30, LayerResourceKind: "STORAGE", LayerResourceParentID: ptr(int32(1)), + LayerResourceSuffix: ".meta", NodeName: "NODE-A", ResourceName: "PVC-S", + }) + d.LayerStorageVolumes = append(d.LayerStorageVolumes, LayerStorageVolumeRow{LayerResourceID: 30, VlmNr: 0, NodeName: "NODE-A"}) + + return d + }(), + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + res := convertTwoReplicas(t, dump) + if len(res.ResourceDefinitions) != 1 || len(res.Resources) != 2 { + t.Fatalf("converted %d definitions and %d replicas, want the definition and both replicas; warnings %v", + len(res.ResourceDefinitions), len(res.Resources), res.Warnings) + } + + vds := res.ResourceDefinitions[0].Spec.VolumeDefinitions + if len(vds) != 1 || vds[0].VolumeNumber != 1 { + t.Errorf("volumes %+v, want volume 1 alone", vds) + } + }) + } +} + +// A ".meta" child that holds a live volume still stops the definition. +func TestAMetaChildHoldingALiveVolumeStillSkips(t *testing.T) { + t.Parallel() + + d := stackDump(`["DRBD","STORAGE"]`, + layerChain(1, "NODE-A", "PVC-S", "DRBD", "STORAGE"), layerChain(10, "NODE-B", "PVC-S", "DRBD", "STORAGE")) + d.VolumeDefinitions[0].VlmFlags = resourceFlagDelete + d.VolumeDefinitions = append(d.VolumeDefinitions, VolumeDefinitionRow{ResourceName: "PVC-S", VlmNr: 1, VlmSize: 1048576, UUID: "vd-s1"}) + d.LayerResourceIDs = append(d.LayerResourceIDs, LayerResourceIDRow{ + LayerResourceID: 30, LayerResourceKind: "STORAGE", LayerResourceParentID: ptr(int32(1)), + LayerResourceSuffix: ".meta", NodeName: "NODE-A", ResourceName: "PVC-S", + }) + d.LayerStorageVolumes = append(d.LayerStorageVolumes, + LayerStorageVolumeRow{LayerResourceID: 30, VlmNr: 0, NodeName: "NODE-A"}, + LayerStorageVolumeRow{LayerResourceID: 30, VlmNr: 1, NodeName: "NODE-A"}) + + res := convertTwoReplicas(t, d) + if len(res.ResourceDefinitions) != 0 { + t.Fatalf("converted %d definitions, want none: volume 1 keeps its metadata on a separate volume; warnings %v", + len(res.ResourceDefinitions), res.Warnings) + } +}