From ea5433b2a56552b024c06485ee70dd350e261118 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 2 Nov 2026 22:31:21 +0100 Subject: [PATCH] fix(linstor-migrate): report an unreadable layer stack The stack check read the RD row's layer_stack without looking at the parse error, so an unreadable column counted as no stack. When the replicas then refused the definition, the warning said it had "no layer stack of its own" and the value was never shown; when they agreed, the only trace was a generic "unparsable list dropped". The refusal now names the unreadable value, and a definition that converts says which stack it took from its replicas, or that it has none to take. Either way it is reported once. The site's migration page still described the warnings from before the refusal; it now lists the refusal and this warning. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/linstor-migration.md | 2 +- pkg/linstormigrate/convert.go | 49 ++++++-- .../unreadable_layer_stack_test.go | 107 ++++++++++++++++++ site/content/en/docs/migration.md | 4 +- 4 files changed, 151 insertions(+), 11 deletions(-) create mode 100644 pkg/linstormigrate/unreadable_layer_stack_test.go diff --git a/docs/linstor-migration.md b/docs/linstor-migration.md index 321ef459..88c0f5cc 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 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 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. +- **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 ab295e03..d88ac8ee 100644 --- a/pkg/linstormigrate/convert.go +++ b/pkg/linstormigrate/convert.go @@ -458,12 +458,17 @@ 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. +// one the row names. A row whose stack cannot be read counts as naming +// none, and the refusal names the value; rdLayerStack reports it when the +// definition converts, so it is reported once either way. func (c *converter) stackDisagreement(row *ResourceDefinitionRow) string { - var stack []string - if !emptyList(row.LayerStack) { - _ = json.Unmarshal([]byte(row.LayerStack), &stack) + stack, readable := rowLayerStack(row) + + // An unreadable column is taken as no stack, the replicas settling it, + // and a refusal says which it was. + ownStack := "no layer stack of its own" + if !readable { + ownStack = "an unreadable layer_stack " + strconv.Quote(row.LayerStack) } // With no stack of its own, the RD takes the one its replicas were @@ -473,7 +478,7 @@ func (c *converter) stackDisagreement(row *ResourceDefinitionRow) string { // 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) + + return ownStack + ", and the replica on " + c.displayNode(node) + " has no layer chain in the dump, so the stack it was built with is unknown" } } @@ -487,7 +492,7 @@ func (c *converter) stackDisagreement(row *ResourceDefinitionRow) string { 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 ownStack + ", and its replicas were built as " + strings.Join(keys, " and ") } return "" @@ -530,7 +535,7 @@ func (c *converter) unchainedReplica(rdName string) string { // 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") + stack, readable := rowLayerStack(row) onDisk := c.diskStacks[row.ResourceName] keys := make([]string, 0, len(onDisk)) @@ -545,12 +550,40 @@ func (c *converter) rdLayerStack(row *ResourceDefinitionRow, dsp string) []strin } if len(keys) == 1 { + if !readable { + c.warnf("resource definition %s: unreadable layer_stack %q; took the stack its replicas were built with, %s", + dsp, row.LayerStack, keys[0]) + } + return slices.Clone(onDisk[keys[0]]) } + if !readable { + c.warnf("resource definition %s: unreadable layer_stack %q, and no diskful replica to read one from; left unset", + dsp, row.LayerStack) + } + return nil } +// rowLayerStack reads the RD row's layer_stack column. readable is false +// for a value that is not a JSON list; the stack is then nil, as for an +// empty column, and the caller says so. +func rowLayerStack(row *ResourceDefinitionRow) ([]string, bool) { + if emptyList(row.LayerStack) { + return nil, true + } + + var stack []string + + err := json.Unmarshal([]byte(row.LayerStack), &stack) + if err != nil { + return nil, false + } + + return stack, true +} + func (c *converter) convertNodes() []crdv1alpha1.Node { nodes := make([]crdv1alpha1.Node, 0, len(c.dump.Nodes)) diff --git a/pkg/linstormigrate/unreadable_layer_stack_test.go b/pkg/linstormigrate/unreadable_layer_stack_test.go new file mode 100644 index 00000000..99daac3e --- /dev/null +++ b/pkg/linstormigrate/unreadable_layer_stack_test.go @@ -0,0 +1,107 @@ +// 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 ( + "slices" + "strconv" + "strings" + "testing" + + crdv1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" +) + +const unreadableStack = `["DRBD",` + +// countWarnings counts the warnings that carry substr. +func countWarnings(res *Result, substr string) int { + n := 0 + + for _, w := range res.Warnings { + if strings.Contains(w, substr) { + n++ + } + } + + return n +} + +// A layer_stack column the converter cannot read is reported once, with its +// value, whatever happens to the definition. When the replicas cannot settle +// the stack the definition is refused, and the refusal says the column was +// unreadable rather than that the definition has no stack of its own. +func TestAnUnreadableRDStackIsReportedOnce(t *testing.T) { + t.Parallel() + + luks := func(first int32, node string) []LayerResourceIDRow { + return layerChain(first, node, "PVC-S", "DRBD", "LUKS", "STORAGE") + } + plain := func(first int32, node string) []LayerResourceIDRow { + return layerChain(first, node, "PVC-S", "DRBD", "STORAGE") + } + + for name, tc := range map[string]struct { + chains [][]LayerResourceIDRow + converted bool + stack []string + }{ + "replicas agree": { + chains: [][]LayerResourceIDRow{luks(1, "NODE-A"), luks(10, "NODE-B")}, converted: true, + stack: []string{"DRBD", "LUKS", "STORAGE"}, + }, + "replicas disagree": {chains: [][]LayerResourceIDRow{luks(1, "NODE-A"), plain(10, "NODE-B")}}, + "a replica has no chain": {chains: [][]LayerResourceIDRow{luks(1, "NODE-A")}}, + "no replicas": {converted: true}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + dump := stackDump(unreadableStack, tc.chains...) + if len(tc.chains) > 0 { + dump = withReplicas(dump) + } + + res, err := Convert(dump) + if err != nil { + t.Fatalf("Convert: %v", err) + } + + if got := countWarnings(res, strconv.Quote(unreadableStack)); got != 1 { + t.Errorf("the unreadable layer_stack %q is in %d warnings, want 1; warnings %v", unreadableStack, got, res.Warnings) + } + + if tc.stack != nil && !hasWarning(res, "took the stack its replicas were built with") { + t.Errorf("the warning does not say which stack was taken; warnings %v", res.Warnings) + } + + if hasWarning(res, "no layer stack of its own") { + t.Errorf("an unreadable layer_stack was reported as no stack; warnings %v", res.Warnings) + } + + idx := slices.IndexFunc(res.ResourceDefinitions, func(rd crdv1alpha1.ResourceDefinition) bool { return rd.Name == "pvc-s" }) + if converted := idx >= 0; converted != tc.converted { + t.Fatalf("converted = %v, want %v; warnings %v", converted, tc.converted, res.Warnings) + } + + if tc.converted && !slices.Equal(res.ResourceDefinitions[idx].Spec.LayerStack, tc.stack) { + t.Errorf("stack %v, want %v", res.ResourceDefinitions[idx].Spec.LayerStack, tc.stack) + } + }) + } +} diff --git a/site/content/en/docs/migration.md b/site/content/en/docs/migration.md index 503a8a77..6d2ae096 100644 --- a/site/content/en/docs/migration.md +++ b/site/content/en/docs/migration.md @@ -79,8 +79,8 @@ Read the whole report it prints. Each `warning:` line is something the tool refu - `LUKS passphrase NOT migrated`. - `no DRBD port`. -- `no layer stack of its own, and its replicas were built as ... left unset; set spec.layerStack before adopting`. The definition names no layer stack, and its replicas were built with different stacks. Set `spec.layerStack` on the definition in the converted manifests to the stack its data was built with. The converter sets `spec.initialized` on a definition whose replicas it adopts, so one left without a stack runs as `DRBD`, `STORAGE`, however its replicas were built: if its group sets another stack, the controller pins the default one; see [How the stack is pinned](../layer-stack/#how-the-stack-is-pinned). -- `but a replica was built as ... that replica will not come up as it is`. The definition's layer stack differs from the one a replica was built with. Settle which stack the data has before you adopt it. +- `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). A definition with no layer stack whose replicas all agree takes their stack without a warning.