diff --git a/pkg/store/k8s/drbd_minor_carry_bug433_test.go b/pkg/store/k8s/drbd_minor_carry_bug433_test.go new file mode 100644 index 00000000..6b456169 --- /dev/null +++ b/pkg/store/k8s/drbd_minor_carry_bug433_test.go @@ -0,0 +1,207 @@ +// 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 k8s_test + +import ( + "context" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "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" + "github.com/cozystack/blockstor/pkg/store/k8s" +) + +// Bug 433: the per-volume DRBDMinor (RD.Spec.VolumeDefinitions[].DRBDMinor) +// is the /dev/drbd device identity. Per the CRD contract +// (api/v1alpha1/resourcedefinition_types.go) "a non-nil value is +// authoritative and is NEVER overwritten … the store-side +// VolumeDefinitions carry-across preserves the value through a REST +// modify". DRBDMinor has NO counterpart on the wire shape +// apiv1.VolumeDefinition (which carries only VolumeNumber/SizeKib/Props/ +// Flags), so a VD-scoped modify that round-trips the entry through +// wireToCRDVD — volumeDefinitions.Update and PatchVolumeDefinitionSpec — +// silently zeroes the minor. +// +// This is the same wire-rebuild-drops-operator-only-field class as the +// carry-across family (Bug 206 RD.VolumeDefinitions, Bug 208 Node ranges, +// Bug 209 RD.Encryption). Only the VD-scoped element rebuild dropped it: +// the RD-scoped path preserves the whole VolumeDefinitions slice +// wholesale, so the minor rode along for free. +// +// Fix: both VD-scoped write paths route their write-back through +// wireToCRDVDPreserving, which carries DRBDMinor across the rebuild. +// +// These tests FAIL on the pre-fix tree (the minor comes back nil) and +// PASS with the carry-across in place. + +const ( + bug433Minor int32 = 20000 + bug433OneGiB int64 = 1 * 1024 * 1024 + bug433TwoGiB int64 = 2 * 1024 * 1024 +) + +// bug433SeedStore builds a k8s store fronting a fake client that holds a +// single RD with one volume carrying a non-nil DRBDMinor (the device +// identity we assert survives every routine modify). +func bug433SeedStore(t *testing.T, rdName string) (*k8s.Store, client.Client) { + t.Helper() + + scheme := runtime.NewScheme() + if err := crdv1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme: %v", err) + } + + minor := bug433Minor + seed := crdv1alpha1.ResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: rdName}, + Spec: crdv1alpha1.ResourceDefinitionSpec{ + VolumeDefinitions: []crdv1alpha1.ResourceDefinitionVolume{ + { + VolumeNumber: 0, + SizeKib: bug433OneGiB, + DRBDMinor: &minor, + }, + }, + }, + } + + cli := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(&seed). + WithStatusSubresource(&crdv1alpha1.ResourceDefinition{}). + Build() + + return k8s.New(cli), cli +} + +// bug433DRBDMinor reads the durable DRBDMinor pointer for (rd, vol0) +// straight off the RD CRD (nil = wiped). +func bug433DRBDMinor(t *testing.T, cli client.Client, rdName string) *int32 { + t.Helper() + + var got crdv1alpha1.ResourceDefinition + if err := cli.Get(context.Background(), client.ObjectKey{Name: rdName}, &got); err != nil { + t.Fatalf("get RD %q: %v", rdName, err) + } + + for i := range got.Spec.VolumeDefinitions { + if got.Spec.VolumeDefinitions[i].VolumeNumber == 0 { + return got.Spec.VolumeDefinitions[i].DRBDMinor + } + } + + t.Fatalf("RD %q: volume 0 vanished", rdName) + + return nil +} + +// assertMinorPreserved fails unless (rd, vol0) still carries the seeded +// device identity. +func bug433AssertMinorPreserved(t *testing.T, cli client.Client, rdName, via string) { + t.Helper() + + m := bug433DRBDMinor(t, cli, rdName) + if m == nil { + t.Fatalf("%s WIPED the volume's DRBDMinor to nil (Bug 433). The per-volume DRBDMinor is "+ + "the /dev/drbd device identity; the CRD contract says a non-nil minor is NEVER "+ + "overwritten and is preserved across a REST modify. wireToCRDVD drops it — the "+ + "VD-scoped write must route through wireToCRDVDPreserving.", via) + } + + if *m != bug433Minor { + t.Fatalf("%s CHANGED the volume's DRBDMinor: got %d want %d — the device identity of a "+ + "live volume must not move on a routine modify (Bug 433).", via, *m, bug433Minor) + } +} + +// TestBug433_PatchVDSpecResizePreservesDRBDMinor — a legal in-bounds +// `vd set-size` grow must not touch the volume's DRBD device minor. +func TestBug433_PatchVDSpecResizePreservesDRBDMinor(t *testing.T) { + t.Parallel() + + const rd = "bug433-resize" + + s, cli := bug433SeedStore(t, rd) + + err := s.VolumeDefinitions().PatchVolumeDefinitionSpec(context.Background(), rd, 0, + func(vd *apiv1.VolumeDefinition) error { + vd.SizeKib = bug433TwoGiB + + return nil + }) + if err != nil { + t.Fatalf("patch (resize): %v", err) + } + + bug433AssertMinorPreserved(t, cli, rd, "`vd set-size` grow (PatchVolumeDefinitionSpec)") +} + +// TestBug433_PatchVDSpecPropModifyPreservesDRBDMinor — a pure +// `vd set-property` edit (no size change) must not touch the device +// identity. +func TestBug433_PatchVDSpecPropModifyPreservesDRBDMinor(t *testing.T) { + t.Parallel() + + const rd = "bug433-props" + + s, cli := bug433SeedStore(t, rd) + + err := s.VolumeDefinitions().PatchVolumeDefinitionSpec(context.Background(), rd, 0, + func(vd *apiv1.VolumeDefinition) error { + if vd.Props == nil { + vd.Props = map[string]string{} + } + vd.Props["Aux/bug433-probe"] = "x" + + return nil + }) + if err != nil { + t.Fatalf("patch (props): %v", err) + } + + bug433AssertMinorPreserved(t, cli, rd, "`vd set-property` (PatchVolumeDefinitionSpec)") +} + +// TestBug433_UpdatePreservesDRBDMinor — the sibling wholesale +// volumeDefinitions.Update path has the identical drop; a REST modify +// routed through it must likewise keep the device identity. +func TestBug433_UpdatePreservesDRBDMinor(t *testing.T) { + t.Parallel() + + const rd = "bug433-update" + + s, cli := bug433SeedStore(t, rd) + + // The wire shape carries no DRBDMinor, so an operator Update supplies + // only VolumeNumber/SizeKib/Props/Flags — the store must carry the + // device identity across from the live CRD entry. + err := s.VolumeDefinitions().Update(context.Background(), rd, &apiv1.VolumeDefinition{ + VolumeNumber: 0, + SizeKib: bug433TwoGiB, + }) + if err != nil { + t.Fatalf("update: %v", err) + } + + bug433AssertMinorPreserved(t, cli, rd, "volumeDefinitions.Update") +} diff --git a/pkg/store/k8s/typed_field_carry_across_test.go b/pkg/store/k8s/typed_field_carry_across_test.go index f573a745..b749e204 100644 --- a/pkg/store/k8s/typed_field_carry_across_test.go +++ b/pkg/store/k8s/typed_field_carry_across_test.go @@ -247,6 +247,31 @@ var classifications = map[string]specClassification{ //nolint:gochecknoglobals / }, mustCarryAcross: map[string]bool{}, }, + "ResourceDefinitionVolume": { + // Not a Spec: the inline element-shape of + // ResourceDefinitionSpec.VolumeDefinitions, rebuilt per-element by + // wireToCRDVD (wire shape apiv1.VolumeDefinition) on the VD-scoped + // Update / PatchVolumeDefinitionSpec paths. + kind: "ResourceDefinitionVolume element (pkg/store/k8s/volume_definitions.go::wireToCRDVD / wireToCRDVDPreserving; wire shape apiv1.VolumeDefinition)", + wireDerived: map[string]bool{ + "VolumeNumber": true, + "SizeKib": true, + "Props": true, + "Flags": true, + }, + mustCarryAcross: map[string]bool{ + // Bug 433: the per-volume DRBDMinor is the /dev/drbd + // device identity and has NO counterpart on + // apiv1.VolumeDefinition. The RD-scoped write preserves the + // whole VolumeDefinitions slice wholesale (so the minor rode + // along for free — see ResourceDefinitionSpec above), but the + // VD-scoped element rebuild dropped it until the write-back + // was routed through wireToCRDVDPreserving. Same + // wire-rebuild-drops-operator-only-field class as Bug 206 / + // 208 / 209. + "DRBDMinor": true, + }, + }, } // specsUnderTest lists every CRD Spec whose Update / Patch path @@ -263,10 +288,13 @@ var classifications = map[string]specClassification{ //nolint:gochecknoglobals / // not apply. // // ResourceDefinitionVolume is the inline element-shape of -// ResourceDefinitionSpec.VolumeDefinitions and is itself rebuilt by -// `wireToCRDVD`, but every field there has a wire counterpart on -// apiv1.VolumeDefinition (volumeNumber, sizeKib, props, flags) — no -// operator-only fields to carry across. +// ResourceDefinitionSpec.VolumeDefinitions, itself rebuilt per-element by +// `wireToCRDVD` on the VD-scoped Update / PatchVolumeDefinitionSpec +// paths. It carries one operator-only field with NO wire counterpart — +// DRBDMinor, the /dev/drbd device identity (Bug 433) — so it IS under +// test here (this is what the pre-433 "every field has a wire +// counterpart" assumption missed). The store-side write-back routes +// through wireToCRDVDPreserving to carry the minor across the rebuild. func specsUnderTest() []reflect.Type { return []reflect.Type{ reflect.TypeOf(crdv1alpha1.NodeSpec{}), @@ -276,6 +304,7 @@ func specsUnderTest() []reflect.Type { reflect.TypeOf(crdv1alpha1.StoragePoolSpec{}), reflect.TypeOf(crdv1alpha1.SnapshotSpec{}), reflect.TypeOf(crdv1alpha1.PhysicalDeviceSpec{}), + reflect.TypeOf(crdv1alpha1.ResourceDefinitionVolume{}), } } diff --git a/pkg/store/k8s/volume_definitions.go b/pkg/store/k8s/volume_definitions.go index 9b3ef29a..c1aa40aa 100644 --- a/pkg/store/k8s/volume_definitions.go +++ b/pkg/store/k8s/volume_definitions.go @@ -271,7 +271,9 @@ func (s *volumeDefinitions) Update(ctx context.Context, rdName string, vd *apiv1 return errors.Wrapf(store.ErrNotFound, "volume %d on resource definition %q", vd.VolumeNumber, rdName) } - rd.Spec.VolumeDefinitions[idx] = wireToCRDVD(vd) + // Bug 433: carry the operator-only DRBDMinor across the wire + // rebuild so a routine REST modify can't wipe the device identity. + rd.Spec.VolumeDefinitions[idx] = wireToCRDVDPreserving(&rd.Spec.VolumeDefinitions[idx], vd) return s.c.Update(ctx, rd) }), "update RD %q for volume %d", rdName, vd.VolumeNumber) @@ -321,8 +323,10 @@ func (s *volumeDefinitions) PatchVolumeDefinitionSpec(ctx context.Context, rdNam } // Re-derive the inline CRD entry from the mutated wire value - // and write it back into the parent RD's slice. - rd.Spec.VolumeDefinitions[idx] = wireToCRDVD(&wire) + // and write it back into the parent RD's slice. Bug 433: the + // wire shape has no DRBDMinor, so carry the operator-only device + // identity across the rebuild instead of zeroing it. + rd.Spec.VolumeDefinitions[idx] = wireToCRDVDPreserving(&rd.Spec.VolumeDefinitions[idx], &wire) return s.c.Patch(ctx, rd, ctrlclient.MergeFromWithOptions(base, ctrlclient.MergeFromWithOptimisticLock{})) }), "patch RD %q for volume %d", rdName, volumeNumber) @@ -456,6 +460,34 @@ func wireToCRDVD(vd *apiv1.VolumeDefinition) crdv1alpha1.ResourceDefinitionVolum } } +// wireToCRDVDPreserving re-derives the inline CRD VolumeDefinition entry +// from a mutated wire value while carrying across the CRD-only typed +// fields that have NO counterpart on the wire shape. +// +// Today that is the per-volume DRBDMinor — the /dev/drbd device +// identity, which apiv1.VolumeDefinition does not carry (it transcodes +// only VolumeNumber/SizeKib/Props/Flags). A naive wireToCRDVD round-trip +// therefore zeroes the minor. Per the CRD contract a non-nil minor is +// authoritative and "is NEVER overwritten … the store-side +// VolumeDefinitions carry-across preserves the value through a REST +// modify" (api/v1alpha1/resourcedefinition_types.go). Both VD-scoped +// write paths (volumeDefinitions.Update, PatchVolumeDefinitionSpec) MUST +// route their write-back through this helper so a legal `vd set-size` / +// `vd set-property` cannot silently change the DRBD device identity of a +// live volume. +// +// Bug 433 — the same wire-rebuild-drops-operator-only-field class as the +// carry-across family (Bug 206 RD.VolumeDefinitions, Bug 208 Node ranges, +// Bug 209 RD.Encryption). The RD-scoped path preserves the whole +// VolumeDefinitions slice wholesale (so DRBDMinor rode along for free), +// which is why only the VD-scoped element rebuild dropped it. +func wireToCRDVDPreserving(prev *crdv1alpha1.ResourceDefinitionVolume, wire *apiv1.VolumeDefinition) crdv1alpha1.ResourceDefinitionVolume { + out := wireToCRDVD(wire) + out.DRBDMinor = prev.DRBDMinor + + return out +} + // vdByNumber returns the embedded volume-definition with the given // VolumeNumber, or nil when the RD spec does not carry it. func vdByNumber(rd *crdv1alpha1.ResourceDefinition, volumeNumber int32) *crdv1alpha1.ResourceDefinitionVolume { diff --git a/tests/e2e/cli-matrix/README.md b/tests/e2e/cli-matrix/README.md index 687e407e..b07682b8 100644 --- a/tests/e2e/cli-matrix/README.md +++ b/tests/e2e/cli-matrix/README.md @@ -43,6 +43,7 @@ A cell counts as **FAIL** if either leg times out, the `linstor` CLI exits non-z | `snap-restore-snapshotless-node-rejected.sh` | 397 | P0 DATA INTEGRITY. `snapshot resource restore` onto a node NOT holding the snapshot is rejected (no silent empty replica, no orphan RD); restoring onto the snapshot's own nodes converges UpToDate AND every replica holds the real snapshot bytes (marker read per-replica), never a silently-empty UpToDate copy. | | `rd-clone-vd-data-plane.sh` | 020 | `linstor rd clone ` on a VD-bearing source (plain CLI body, no `use_zfs_clone`) AND the raw-REST `use_zfs_clone=true` body linstor-csi sends both materialise a real clone: 2 replicas UpToDate, marker bytes from the source present on EVERY clone replica (promote each in turn), clone status COMPLETE, internal `clone-` snapshot visible on the source. Pre-fix: 400 on `use_zfs_clone`, 501 on VD-bearing sources (Bug 114 gate). | | `encryption-passphrase-luks-rd.sh` | 023 | Secret-only LUKS flow: `linstor encryption create-passphrase` alone (legacy `DrbdOptions/EncryptPassphrase` controller prop asserted ABSENT throughout) unlocks `rd c -l drbd,luks,storage` + autoplace to UpToDate, and the Secret-backed passphrase actually opens the LUKS header on each replica's backing device. Requires the Bug-023 fix (PR #143); pre-fix the rd-create is rejected with "LUKS layer requires DrbdOptions/EncryptPassphrase to be set first". | +| `vd-modify-preserves-drbd-minor.sh` | 433 | A legal in-bounds VD-scoped modify (`vd set-size` grow / `vd set-property`) must NOT change the per-volume DRBDMinor — the /dev/drbd device identity. Creates rd-a (lowest minor Ma) + rd-b (next minor Mb), frees Ma by deleting rd-a, then modifies rd-b: its DRBDMinor AND device path must stay Mb over a settle window. Pre-fix `wireToCRDVD` dropped the minor on the VD-scoped write-back and the allocator re-stamped the freed lower Ma — a permanent device-identity change on a live resized volume. | ## Running diff --git a/tests/e2e/cli-matrix/vd-modify-preserves-drbd-minor.sh b/tests/e2e/cli-matrix/vd-modify-preserves-drbd-minor.sh new file mode 100755 index 00000000..ff188f11 --- /dev/null +++ b/tests/e2e/cli-matrix/vd-modify-preserves-drbd-minor.sh @@ -0,0 +1,192 @@ +#!/usr/bin/env bash +# +# usage: vd-modify-preserves-drbd-minor.sh WORK_DIR +# +# L6 cli-matrix cell — Bug 433 (P2 correctness+availability): a legal, +# in-bounds VD-scoped modify (`vd set-size` grow or `vd set-property`) +# must NOT change the per-volume DRBDMinor — the /dev/drbd device +# identity. +# +# The pre-fix store round-trips the inline VolumeDefinition through +# wireToCRDVD on the VD-scoped write paths (Update / +# PatchVolumeDefinitionSpec); the wire shape carries no DRBDMinor, so the +# round-trip zeroed it. In isolation the allocator re-picks the same +# value (self-heals), but once a LOWER minor has been freed by routine RD +# churn the allocator hands the resized volume that lower minor instead — +# a permanent device-identity change on a live volume, driven purely by a +# modify. +# +# Scenario (minor allocation is lowest-free + cluster-scoped, so this is +# robust to the stand's starting minor without hard-coding values): +# +# 1. rd-a c + vd c 1G + r c (grabs the lowest free minor Ma). +# 2. rd-b c + vd c 1G + r c (grabs the next minor Mb > Ma — rd-b's +# stable device identity). Capture Mb and rd-b's /dev/drbdN. +# 3. rd-a d — frees the LOWER minor Ma, which the pre-fix reheal would +# pull rd-b down to. +# 4. linstor vd s rd-b 0 2G → assert rd-b's DRBDMinor AND device path +# stay == Mb over a settle window (pre-fix: flips to Ma). +# 5. linstor vd set-property rd-b 0 Aux/... x → assert the same. +# 6. Cleanup; assert_no_orphans. +# +# If Bug 433 regressed, step 4 or 5 catches the minor moving off Mb (to +# the freed lower Ma, or transiently to nil) within the settle window. + +set -euo pipefail + +WORK_DIR=${1:?work_dir required} +export KUBECONFIG="$WORK_DIR/kubeconfig" + +SCRIPT_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=lib.sh +source "$SCRIPT_DIR/lib.sh" + +require_workers 1 + +linstor_cli_setup + +POOL="${POOL:-lvm-thin}" +RD_A="cli-matrix-minor-a" +RD_B="cli-matrix-minor-b" +SIZE_2G_KIB=2097152 + +cleanup() { + [[ -n "${RD_B:-}" ]] && delete_rd "$RD_B" 2>/dev/null || true + [[ -n "${RD_A:-}" ]] && delete_rd "$RD_A" 2>/dev/null || true + [[ -n "${RD_B:-}" ]] && assert_no_orphans "$RD_B" + [[ -n "${RD_A:-}" ]] && assert_no_orphans "$RD_A" +} +trap 'cleanup; linstor_cli_teardown' EXIT + +# rd_vol0_minor reads the authoritative per-volume DRBDMinor off the RD +# spec (nil/unset reads as ""). +rd_vol0_minor() { + local rd=$1 + kubectl get resourcedefinition "$rd" \ + -o jsonpath='{.spec.volumeDefinitions[0].drbdMinor}' 2>/dev/null || echo "" +} + +# wait_rd_vol0_minor blocks until the controller has allocated a non-empty +# minor for rd vol0, then echoes it. +wait_rd_vol0_minor() { + local rd=$1 t=${2:-120} m deadline + deadline=$(( $(date +%s) + t )) + while (( $(date +%s) < deadline )); do + m=$(rd_vol0_minor "$rd") + if [[ -n "$m" ]]; then echo "$m"; return 0; fi + sleep 2 + done + echo "FAIL: $rd vol0 DRBDMinor never allocated within ${t}s" >&2 + exit 1 +} + +# resource_vol0_device resolves rd@node's volume-0 /dev/drbdN identity +# from the Resource CRD status (devicePath, falling back to drbdMinor). +resource_vol0_device() { + local rd=$1 node=$2 dev minor + dev=$(kubectl get resource "${rd}.${node}" \ + -o jsonpath='{.status.volumes[0].devicePath}' 2>/dev/null || echo "") + if [[ -z "$dev" ]]; then + minor=$(kubectl get resource "${rd}.${node}" \ + -o jsonpath='{.status.drbdMinor}' 2>/dev/null || echo "") + [[ -n "$minor" ]] && dev="/dev/drbd${minor}" + fi + echo "$dev" +} + +# assert_minor_stable holds for a settle window and FAILs the instant the +# minor or device path drifts off the captured identity — catching both a +# transient wipe-to-nil and a reheal-to-a-different-minor. +assert_minor_stable() { + local rd=$1 node=$2 want_minor=$3 want_dev=$4 label=$5 deadline m d + deadline=$(( $(date +%s) + 12 )) + while (( $(date +%s) < deadline )); do + m=$(rd_vol0_minor "$rd") + if [[ "$m" != "$want_minor" ]]; then + echo "FAIL (Bug 433): $label CHANGED $rd vol0 DRBDMinor: '$want_minor' -> '$m'" >&2 + echo " The per-volume DRBDMinor is the /dev/drbd device identity; a legal" >&2 + echo " VD-scoped modify must never move it. wireToCRDVD dropped it on the" >&2 + echo " write-back — the fix carries it across via wireToCRDVDPreserving." >&2 + exit 1 + fi + d=$(resource_vol0_device "$rd" "$node") + if [[ -n "$want_dev" && -n "$d" && "$d" != "$want_dev" ]]; then + echo "FAIL (Bug 433): $label CHANGED $rd vol0 device identity: '$want_dev' -> '$d'" >&2 + exit 1 + fi + sleep 2 + done + echo " $label: $rd vol0 minor stable at '$want_minor' (dev '$want_dev') OK" +} + +echo "============================================================" +echo ">> vd-modify-preserves-drbd-minor (Bug 433) :: POOL=$POOL" +echo "============================================================" + +echo ">> pre-flight: $POOL on >=1 node" +sp_json=$("${LCTL[@]}" --machine-readable storage-pool list --storage-pools "$POOL" 2>/dev/null || echo "[]") +ok_nodes=$(jq -r '[.[]? | .[]? | select(.provider_kind != null) | .node_name] | unique | length' <<<"$sp_json" 2>/dev/null || echo 0) +if (( ok_nodes < 1 )); then + echo "SKIP ($POOL): pool not on any node (got $ok_nodes)" + exit 0 +fi +N1=$(jq -r '[.[]? | .[]? | select(.provider_kind != null) | .node_name] | unique | first // ""' <<<"$sp_json" 2>/dev/null || echo "") +if [[ -z "$N1" ]]; then + echo "SKIP ($POOL): could not resolve a diskful node" + exit 0 +fi +echo ">> target node: $N1" + +# ---- rd-a grabs the lowest free minor Ma ---------------------------------- +echo ">> rd-a: rd c + vd c 1G + r c $N1" +_out=$("${LCTL[@]}" resource-definition create "$RD_A" 2>&1) \ + || { echo "FAIL: rd c $RD_A: $_out" >&2; exit 1; } +_out=$("${LCTL[@]}" volume-definition create "$RD_A" 1G 2>&1) \ + || { echo "FAIL: vd c $RD_A 1G: $_out" >&2; exit 1; } +_out=$("${LCTL[@]}" resource create "$N1" "$RD_A" --storage-pool="$POOL" 2>&1) \ + || { echo "FAIL: r c $N1 $RD_A: $_out" >&2; exit 1; } +wait_disk_state "$RD_A" "$N1" "UpToDate" 240 0 +MINOR_A=$(wait_rd_vol0_minor "$RD_A") +echo ">> rd-a vol0 minor Ma=$MINOR_A" + +# ---- rd-b grabs the next minor Mb (> Ma) — its stable identity ------------ +echo ">> rd-b: rd c + vd c 1G + r c $N1" +_out=$("${LCTL[@]}" resource-definition create "$RD_B" 2>&1) \ + || { echo "FAIL: rd c $RD_B: $_out" >&2; exit 1; } +_out=$("${LCTL[@]}" volume-definition create "$RD_B" 1G 2>&1) \ + || { echo "FAIL: vd c $RD_B 1G: $_out" >&2; exit 1; } +_out=$("${LCTL[@]}" resource create "$N1" "$RD_B" --storage-pool="$POOL" 2>&1) \ + || { echo "FAIL: r c $N1 $RD_B: $_out" >&2; exit 1; } +wait_disk_state "$RD_B" "$N1" "UpToDate" 240 0 +MINOR_B=$(wait_rd_vol0_minor "$RD_B") +DEV_B=$(resource_vol0_device "$RD_B" "$N1") +echo ">> rd-b vol0 minor Mb=$MINOR_B dev=$DEV_B (stable device identity)" + +# Sanity: we need Ma < Mb so freeing Ma creates a lower-free minor the +# pre-fix reheal would pull rd-b down to. If not (dirty range), the +# scenario can't reproduce the divergence — skip rather than false-pass. +if ! [[ "$MINOR_A" =~ ^[0-9]+$ && "$MINOR_B" =~ ^[0-9]+$ ]] || (( MINOR_A >= MINOR_B )); then + echo "SKIP: need Ma < Mb to free a lower minor (got Ma=$MINOR_A Mb=$MINOR_B); dirty minor range" + exit 0 +fi + +# ---- free the lower minor Ma via rd-a delete ------------------------------ +echo ">> rd-a delete — frees the lower minor Ma=$MINOR_A" +delete_rd "$RD_A" +RD_A="" # deleted; keep cleanup from re-deleting/orphan-checking it + +# ---- legal grow on rd-b vol0: minor MUST stay Mb -------------------------- +echo ">> linstor vd s $RD_B 0 2G" +"${LCTL[@]}" volume-definition set-size "$RD_B" 0 2G >/dev/null +wait_vd_size "$RD_B" 0 "$SIZE_2G_KIB" 60 +assert_minor_stable "$RD_B" "$N1" "$MINOR_B" "$DEV_B" "\`vd set-size\` grow" + +# ---- legal set-property on rd-b vol0: minor MUST stay Mb ------------------ +echo ">> linstor vd set-property $RD_B 0 Aux/bug433-probe x" +"${LCTL[@]}" volume-definition set-property "$RD_B" 0 Aux/bug433-probe x >/dev/null +assert_minor_stable "$RD_B" "$N1" "$MINOR_B" "$DEV_B" "\`vd set-property\`" + +echo ">> vd-modify-preserves-drbd-minor (Bug 433) OK" +cleanup +trap 'linstor_cli_teardown' EXIT +echo ">> vd-modify-preserves-drbd-minor COMPLETE" diff --git a/tests/integration/group_blue_drbd_minor_bug433_test.go b/tests/integration/group_blue_drbd_minor_bug433_test.go new file mode 100644 index 00000000..a4540b2c --- /dev/null +++ b/tests/integration/group_blue_drbd_minor_bug433_test.go @@ -0,0 +1,294 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build integration + +/* +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. +*/ + +// Bug 433 — end-to-end regression for the per-volume DRBDMinor drop on a +// VD-scoped modify. +// +// RD.Spec.VolumeDefinitions[].DRBDMinor is the /dev/drbd device +// identity, "identical on every node that hosts a replica" and — per the +// CRD contract — "a non-nil value is authoritative and is NEVER +// overwritten … the store-side VolumeDefinitions carry-across preserves +// the value through a REST modify" (api/v1alpha1/resourcedefinition_types.go). +// +// A VD-scoped modify (`vd set-size` / `vd set-property`) round-trips the +// entry through wireToCRDVD, which transcodes only VolumeNumber/SizeKib/ +// Props/Flags and dropped DRBDMinor, so PatchVolumeDefinitionSpec wrote +// back an entry with a nil minor. In isolation the allocator re-picks the +// same value (self-heals); but once a lower minor has been freed (routine +// RD churn), the controller's allocate-if-nil pass hands the resized +// volume a DIFFERENT minor — a permanent device-identity change on a live +// volume, driven purely by a legal in-bounds modify. +// +// These exercise the WHOLE path (REST handler → store → controller +// allocator), unlike the store-layer L1 unit (pkg/store/k8s/ +// drbd_minor_carry_bug433_test.go): they FAIL on the pre-fix tree with a +// diverged minor and PASS once wireToCRDVDPreserving carries it across. +package integration + +import ( + "bytes" + "context" + "encoding/json" + "io" + "net/http" + "testing" + "time" + + "k8s.io/apimachinery/pkg/types" + + blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" + "github.com/cozystack/blockstor/tests/integration/harness" +) + +// b433Put issues a PUT with a JSON body and returns (status, body). +func b433Put(t *testing.T, url string, payload any) (int, []byte) { + t.Helper() + + raw, err := json.Marshal(payload) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), groupGTimeout) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, http.MethodPut, url, bytes.NewReader(raw)) + if err != nil { + t.Fatalf("build PUT: %v", err) + } + + req.Header.Set("Content-Type", "application/json") + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("PUT %s: %v", url, err) + } + defer func() { _ = resp.Body.Close() }() + + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + + return resp.StatusCode, body +} + +// b433VDMinor returns the durable DRBDMinor pointer for (rd, vn) read +// straight off the RD CRD (nil = unset). +func b433VDMinor(t *testing.T, stack *harness.Stack, rd string, vn int32) *int32 { + t.Helper() + + var rdObj blockstoriov1alpha1.ResourceDefinition + if err := stack.Env.Client.Get(context.Background(), types.NamespacedName{Name: rd}, &rdObj); err != nil { + t.Fatalf("get RD %q: %v", rd, err) + } + + for i := range rdObj.Spec.VolumeDefinitions { + if rdObj.Spec.VolumeDefinitions[i].VolumeNumber == vn { + return rdObj.Spec.VolumeDefinitions[i].DRBDMinor + } + } + + return nil +} + +// b433WaitMinor blocks until (rd, vn) has a non-nil DRBDMinor. +func b433WaitMinor(t *testing.T, stack *harness.Stack, rd string, vn int32) int32 { + t.Helper() + + deadline := time.Now().Add(20 * time.Second) + for time.Now().Before(deadline) { + if m := b433VDMinor(t, stack, rd, vn); m != nil { + return *m + } + + time.Sleep(100 * time.Millisecond) + } + + t.Fatalf("RD %q vol %d: DRBDMinor never allocated", rd, vn) + + return 0 +} + +// b433WaitMinorSettled waits until (rd, vn) has a non-nil DRBDMinor that +// stays constant for a full settle window, then returns it. This lets a +// racing allocator finish re-stamping before the assertion reads the +// final value (so a transient nil→re-heal can't be mistaken for stability). +func b433WaitMinorSettled(t *testing.T, stack *harness.Stack, rd string, vn int32) int32 { + t.Helper() + + const settle = 2 * time.Second + + deadline := time.Now().Add(20 * time.Second) + + var ( + last int32 + haveLast bool + stableAt time.Time + ) + + for time.Now().Before(deadline) { + m := b433VDMinor(t, stack, rd, vn) + switch { + case m == nil: + haveLast = false + case !haveLast || *m != last: + last = *m + haveLast = true + stableAt = time.Now() + default: + if time.Since(stableAt) >= settle { + return last + } + } + + time.Sleep(100 * time.Millisecond) + } + + if haveLast { + return last + } + + t.Fatalf("RD %q vol %d: DRBDMinor never settled to a non-nil value", rd, vn) + + return 0 +} + +// b433DeleteRDResources deletes every fixture-node Resource of an RD via +// the k8s client so a subsequent RD delete isn't blocked by child refs. +func b433DeleteRDResources(t *testing.T, stack *harness.Stack, rd string) { + t.Helper() + + ctx := context.Background() + for _, node := range harness.FixtureNodes() { + r := &blockstoriov1alpha1.Resource{} + if err := stack.Env.Client.Get(ctx, types.NamespacedName{Name: rd + "." + node}, r); err != nil { + continue + } + + _ = stack.Env.Client.Delete(ctx, r) + } +} + +// b433WaitRDGone blocks until the RD object is fully absent from the API +// (so takenMinorsCluster no longer counts its minors). +func b433WaitRDGone(t *testing.T, stack *harness.Stack, rd string) { + t.Helper() + + deadline := time.Now().Add(20 * time.Second) + for time.Now().Before(deadline) { + var rdObj blockstoriov1alpha1.ResourceDefinition + if err := stack.Env.Client.Get(context.Background(), types.NamespacedName{Name: rd}, &rdObj); err != nil { + return + } + + time.Sleep(100 * time.Millisecond) + } + + t.Fatalf("RD %q did not disappear within budget", rd) +} + +// TestBug433VDResizePreservesDRBDMinor — FAIL-on-bug regression. +// +// A legal in-bounds `vd set-size` grow MUST NOT change the volume's DRBD +// device minor. On the pre-fix tree the resize wipes the minor +// (wireToCRDVD drops it) and, with a lower minor freed by a prior RD +// delete, the allocator re-stamps a DIFFERENT minor — a permanent +// device-identity change on a live volume. Passes only once +// PatchVolumeDefinitionSpec carries DRBDMinor across the wire round-trip. +func TestBug433VDResizePreservesDRBDMinor(t *testing.T) { + stack := harness.StartStack(t) + harness.SeedThreeNodeCluster(t, stack) + + // RD-A grabs the lowest minor; RD-B the next one up. + rdA := seedRDWithVolume(t, stack, "b433-minor-a") + minorA := b433WaitMinor(t, stack, rdA, 0) + + rdB := seedRDWithVolume(t, stack, "b433-minor-b") + minorB := b433WaitMinor(t, stack, rdB, 0) + + t.Logf("RD-A minor=%d, RD-B minor=%d (RD-B's stable device identity)", minorA, minorB) + + // Free RD-A's (lower) minor so the allocator would hand it to RD-B on + // any re-allocation. + b433DeleteRDResources(t, stack, rdA) + deleteRDViaREST(context.Background(), t, stack.RestURL, rdA) + b433WaitRDGone(t, stack, rdA) + + // Legal in-bounds grow on RD-B vol0 (1 GiB -> 2 GiB). + url := stack.RestURL + "/v1/resource-definitions/" + rdB + "/volume-definitions/0" + status, body := b433Put(t, url, map[string]any{"size_kib": int64(2 * 1024 * 1024)}) + if status < 200 || status >= 300 { + t.Fatalf("legal in-bounds resize was rejected: status=%d body=%s", status, string(body)) + } + + final := b433WaitMinorSettled(t, stack, rdB, 0) + if final != minorB { + t.Fatalf("`vd set-size` CHANGED the live volume's DRBD minor: %d -> %d. "+ + "The per-volume DRBDMinor is the /dev/drbd device identity and the CRD "+ + "contract says a non-nil minor is NEVER overwritten and is preserved across a "+ + "REST modify (resourcedefinition_types.go). PatchVolumeDefinitionSpec round-trips "+ + "through wireToCRDVD, which drops DRBDMinor; the fix routes the write-back through "+ + "wireToCRDVDPreserving. On the stand this re-creates the DRBD device at a new minor "+ + "on a resized live volume.", + minorB, final) + } +} + +// TestBug433VDPropModifyPreservesDRBDMinor — FAIL-on-bug regression. +// +// Same drop via a pure props modify (`vd set-property`): no size change, +// yet the entry is rebuilt through wireToCRDVD and loses its minor. With +// a lower minor freed, the resulting re-allocation diverges. A property +// edit must never touch the device identity. +func TestBug433VDPropModifyPreservesDRBDMinor(t *testing.T) { + stack := harness.StartStack(t) + harness.SeedThreeNodeCluster(t, stack) + + rdA := seedRDWithVolume(t, stack, "b433-minorp-a") + minorA := b433WaitMinor(t, stack, rdA, 0) + + rdB := seedRDWithVolume(t, stack, "b433-minorp-b") + minorB := b433WaitMinor(t, stack, rdB, 0) + + t.Logf("RD-A minor=%d, RD-B minor=%d", minorA, minorB) + + b433DeleteRDResources(t, stack, rdA) + deleteRDViaREST(context.Background(), t, stack.RestURL, rdA) + b433WaitRDGone(t, stack, rdA) + + // Legal props-only modify on RD-B vol0. + url := stack.RestURL + "/v1/resource-definitions/" + rdB + "/volume-definitions/0" + status, body := b433Put(t, url, map[string]any{ + "override_props": map[string]string{"Aux/bug433-probe": "x"}, + }) + if status < 200 || status >= 300 { + t.Fatalf("legal props modify was rejected: status=%d body=%s", status, string(body)) + } + + final := b433WaitMinorSettled(t, stack, rdB, 0) + if final != minorB { + t.Fatalf("`vd set-property` CHANGED the live volume's DRBD minor: %d -> %d. "+ + "A property edit must not touch the device identity; wireToCRDVD drops DRBDMinor "+ + "on every VD-scoped modify — the fix routes the write-back through "+ + "wireToCRDVDPreserving.", + minorB, final) + } +} diff --git a/tests/operator-harness/lib.sh b/tests/operator-harness/lib.sh index 33e0e649..7f89e50a 100755 --- a/tests/operator-harness/lib.sh +++ b/tests/operator-harness/lib.sh @@ -603,6 +603,35 @@ except Exception: print(0)" "$vol" 2>/dev/null || echo 0) [[ "$actual" == "$expected" ]] ;; + drbd_minor) + # Bug 433: assert the per-volume DRBDMinor — the /dev/drbd + # device identity — on RD.Spec.VolumeDefinitions[] equals + # `expected`. A VD-scoped modify (`vd set-size` / `vd + # set-property`) must NOT change it; the pre-fix wire round-trip + # dropped the minor and, once a lower minor was freed by routine + # RD churn, the allocator re-stamped a DIFFERENT one — a + # permanent device-identity change on a live volume. Pair with + # hold_s so a transient nil→re-heal can't be mistaken for + # stability; an unset minor reads as "". + local rd vol expected actual + rd=$(substitute "$(python3 -c "import json,sys; print(json.loads(sys.argv[1]).get('rd',''))" "$spec")") + vol=$(python3 -c "import json,sys; print(json.loads(sys.argv[1]).get('vol',0))" "$spec") + expected=$(python3 -c "import json,sys; print(json.loads(sys.argv[1]).get('expected',''))" "$spec") + actual=$(kubectl get resourcedefinition "$rd" -o json 2>/dev/null \ + | python3 -c "import json,sys +try: + target=int(sys.argv[1]) + d=json.load(sys.stdin) + for v in (d.get('spec', {}).get('volumeDefinitions') or []): + if v.get('volumeNumber', -1) == target: + m=v.get('drbdMinor') + print('' if m is None else m) + sys.exit(0) + print('') +except Exception: + print('')" "$vol" 2>/dev/null || echo "") + [[ "$actual" == "$expected" ]] + ;; vd_count) # BUG-048: assert the RD carries EXACTLY `expected` # VolumeDefinitions. A concurrent-auto-assign lost-update diff --git a/tests/operator-harness/replay-runner.sh b/tests/operator-harness/replay-runner.sh index f1f8ddca..186020b9 100755 --- a/tests/operator-harness/replay-runner.sh +++ b/tests/operator-harness/replay-runner.sh @@ -72,6 +72,10 @@ # - resource_absent wait until r d takes effect on a node # - rd_absent wait until rd is gone everywhere # - vd_size_kib VolumeDefinition.size_kib equals expected_kib +# - drbd_minor RD.Spec.VolumeDefinitions[vol].drbdMinor equals +# `expected` — the /dev/drbd device identity is +# stable across a VD-scoped modify (Bug 433); pair +# with hold_s (an unset minor reads as "") # - vd_count RD carries EXACTLY `expected` VolumeDefinitions # (BUG-048 concurrent-add lost-update catcher) # - pvc_capacity PVC.Status.Capacity.storage matches expected diff --git a/tests/operator-harness/replay/vd-modify-preserves-drbd-minor.yaml b/tests/operator-harness/replay/vd-modify-preserves-drbd-minor.yaml new file mode 100644 index 00000000..864f0206 --- /dev/null +++ b/tests/operator-harness/replay/vd-modify-preserves-drbd-minor.yaml @@ -0,0 +1,158 @@ +name: vd-modify-preserves-drbd-minor +description: | + Bug 433 (P2 correctness+availability) — a LEGAL, in-bounds VD-scoped + modify (`vd set-size` grow or `vd set-property`) must NOT change the + per-volume DRBDMinor, the /dev/drbd device identity. + + The pre-fix store round-trips the inline VolumeDefinition through + wireToCRDVD on the VD-scoped write paths (Update / + PatchVolumeDefinitionSpec); the wire shape apiv1.VolumeDefinition has no + DRBDMinor, so the round-trip zeroed it. In isolation the allocator + re-picks the same value (self-heals), but once a LOWER minor has been + freed by routine RD churn the controller's allocate-if-nil pass hands + the resized volume that lower minor instead — a permanent + device-identity change on a live volume, driven purely by a resize. + + This replay reproduces the divergence at the operator-CLI level: + - rd-a grabs the lowest minor (20000), rd-b the next (20001); + - rd-a is deleted, freeing 20000; + - a legal grow AND a legal set-property are issued on rd-b vol0; + - rd-b's DRBDMinor must stay 20001 throughout (pre-fix it flips to + 20000). The drbd_minor await with hold_s is the catcher. + + NOTE: the expected minors (20000 / 20001) assume a stand whose DRBD + minor range floor ([20000, 65535]) is free at the start. On a dirty + stand, adjust the two `expected` values to the minors rd-a / rd-b + actually receive (steps confirm-rd-a-minor / confirm-rd-b-minor read + them). The invariant under test — rd-b's minor is UNCHANGED across the + modify — holds regardless of the absolute value. + +prerequisites: + min_nodes: 1 + storage_pool: stand + +vars: + sp: stand + +steps: + # ---- rd-a grabs the lowest minor (20000) ---- + - name: create-rd-a + cmd: ["resource-definition", "create", "{{rd}}-a"] + expect_exit: 0 + - name: create-vd-a-1g + cmd: ["volume-definition", "create", "{{rd}}-a", "1G"] + expect_exit: 0 + await: + kind: vd_size_kib + rd: "{{rd}}-a" + vol: 0 + expected_kib: 1048576 + timeout_s: 30 + - name: place-rd-a + cmd: ["resource", "create", "{{node1}}", "{{rd}}-a", "--storage-pool", "{{sp}}"] + expect_exit: 0 + await: + kind: disk_state + rd: "{{rd}}-a" + node: "{{node1}}" + expected: UpToDate + timeout_s: 240 + - name: confirm-rd-a-minor + cmd: ["volume-definition", "list", "--resource-definitions", "{{rd}}-a"] + expect_exit: 0 + await: + kind: drbd_minor + rd: "{{rd}}-a" + vol: 0 + expected: 20000 + timeout_s: 60 + + # ---- rd-b grabs the next minor (20001) — its stable identity ---- + - name: create-rd-b + cmd: ["resource-definition", "create", "{{rd}}-b"] + expect_exit: 0 + - name: create-vd-b-1g + cmd: ["volume-definition", "create", "{{rd}}-b", "1G"] + expect_exit: 0 + await: + kind: vd_size_kib + rd: "{{rd}}-b" + vol: 0 + expected_kib: 1048576 + timeout_s: 30 + - name: place-rd-b + cmd: ["resource", "create", "{{node1}}", "{{rd}}-b", "--storage-pool", "{{sp}}"] + expect_exit: 0 + await: + kind: disk_state + rd: "{{rd}}-b" + node: "{{node1}}" + expected: UpToDate + timeout_s: 240 + - name: confirm-rd-b-minor + cmd: ["volume-definition", "list", "--resource-definitions", "{{rd}}-b"] + expect_exit: 0 + await: + kind: drbd_minor + rd: "{{rd}}-b" + vol: 0 + expected: 20001 + timeout_s: 60 + + # ---- free the lower minor (20000) via rd-a delete ---- + - name: delete-rd-a-frees-lower-minor + cmd: ["resource-definition", "delete", "{{rd}}-a"] + expect_exit: 0 + await: + kind: rd_absent + rd: "{{rd}}-a" + timeout_s: 60 + + # ---- legal grow on rd-b vol0: minor MUST stay 20001 ---- + - name: grow-rd-b-2g + cmd: ["volume-definition", "set-size", "{{rd}}-b", "0", "2G"] + expect_exit: 0 + await: + kind: vd_size_kib + rd: "{{rd}}-b" + vol: 0 + expected_kib: 2097152 + timeout_s: 60 + - name: minor-stable-after-grow + cmd: ["volume-definition", "list", "--resource-definitions", "{{rd}}-b"] + expect_exit: 0 + await: + kind: drbd_minor + rd: "{{rd}}-b" + vol: 0 + expected: 20001 + hold_s: 6 + timeout_s: 60 + + # ---- legal set-property on rd-b vol0: minor MUST stay 20001 ---- + - name: set-property-rd-b + cmd: ["volume-definition", "set-property", "{{rd}}-b", "0", "Aux/bug433-probe", "x"] + expect_exit: 0 + - name: minor-stable-after-set-property + cmd: ["volume-definition", "list", "--resource-definitions", "{{rd}}-b"] + expect_exit: 0 + await: + kind: drbd_minor + rd: "{{rd}}-b" + vol: 0 + expected: 20001 + hold_s: 6 + timeout_s: 60 + +teardown: + - cmd: ["resource-definition", "delete", "{{rd}}-b"] + expect_exit: 0 + await: + kind: rd_absent + rd: "{{rd}}-b" + timeout_s: 60 + - cmd: ["resource-definition", "delete", "{{rd}}-a"] + expect_exit: [0, 1, 10] + +invariants: + - no_orphans