From 978db428a0a044b993fe755a23b7f34f8b4033b3 Mon Sep 17 00:00:00 2001 From: "tao.gan" Date: Thu, 14 May 2026 18:51:37 +0800 Subject: [PATCH] [storage]: decouple alive-chain membership from vmState in VolumeTree VolumeTree previously conflated two independent concepts in isOnline / resolveDirection: (a) is the snapshot on the volume's live backing chain (a structural property of the tree) (b) is the VM currently routed through the hypervisor (Running/Paused) Two consequences: 1. In VolumeSnapshotTreeBase.stepDelete, the multi-children "defer the alive-chain child to the final round" guard used isOnline, which returned false for every child when the VM was Stopped. The guard silently disappeared and child selection fell back to children.get(0), which could land on the volume's backing chain and corrupt the live chain on rebase failure. 2. resolveDirection's shouldUseCommitStrategy was likewise gated on vmState, so direction=Auto + Stopped degraded to Pull (writing N copies of the (target - parent) delta into each child file) instead of a single-file Commit. Split into two predicates: - isOnAliveChain(snapshotUuid): pure tree-structure query, used by stepDelete to identify the alive child regardless of vmState. - isHypervisorOperation(vmState): pure run-state predicate, used to pick libvirt vs primary-storage agent. isOnline is rewritten as the compound (treeIsCurrent && isHypervisorOperation(vmState) && both endpoints on alive chain), preserving its existing semantics for current callers. resolveDirection now derives shouldUseCommitStrategy purely from tree structure, so Auto + Stopped + alive-chain target now picks Commit. stepDelete renames onlineChild to aliveChild and queries isOnAliveChain, so the alive-child deferral protection now applies to Stopped VMs as well. Resolves: ZSV-10538 Change-Id: I6e63786d6d6b616f7a766b6c7463617572786969 --- .../snapshot/VolumeSnapshotTreeBase.java | 6 +++--- .../zstack/storage/snapshot/VolumeTree.java | 21 +++++++++++++------ 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.java b/storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.java index beb8b044d1a..f3195668f4b 100755 --- a/storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.java +++ b/storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.java @@ -884,8 +884,8 @@ private void stepDelete(Completion completion) { return; } - VolumeTree.VolumeSnapshotLeaf onlineChild = children.stream() - .filter(child -> volumeTree.isOnline(current, currentRoot.getUuid(), child.getUuid(), vmState)) + VolumeTree.VolumeSnapshotLeaf aliveChild = children.stream() + .filter(child -> volumeTree.isOnAliveChain(child.getUuid())) .findFirst().orElse(null); Completion comp = new Completion(completion) { @@ -910,7 +910,7 @@ public void fail(ErrorCode errorCode) { pull(child, volumeTree, online, comp); } } else { - if (onlineChild != null && Objects.equals(child.getUuid(), onlineChild.getUuid())) { + if (aliveChild != null && Objects.equals(child.getUuid(), aliveChild.getUuid())) { child = children.get(1); } boolean online = volumeTree.isOnline(current, currentRoot.getUuid(), child.getUuid(), vmState); diff --git a/storage/src/main/java/org/zstack/storage/snapshot/VolumeTree.java b/storage/src/main/java/org/zstack/storage/snapshot/VolumeTree.java index ea669fd6202..f7b9bcc0ba3 100644 --- a/storage/src/main/java/org/zstack/storage/snapshot/VolumeTree.java +++ b/storage/src/main/java/org/zstack/storage/snapshot/VolumeTree.java @@ -361,12 +361,19 @@ public List getAliveChainSnapshotUuids() { return aliveChain.stream().map(VolumeSnapshotInventory::getUuid).collect(Collectors.toList()); } + public boolean isOnAliveChain(String snapshotUuid) { + return current && getAliveChainSnapshotUuids().contains(snapshotUuid); + } + + public static boolean isHypervisorOperation(VmInstanceState vmState) { + return vmState == VmInstanceState.Running || vmState == VmInstanceState.Paused; + } + public DeleteVolumeSnapshotDirection resolveDirection(String targetSnapshotUuid, String childSnapshotUuid, String initialDirection, boolean targetSnapshotIsLatest, VmInstanceState vmState) { - boolean online = (vmState == VmInstanceState.Running || vmState == VmInstanceState.Paused) - && getAliveChainSnapshotUuids().contains(targetSnapshotUuid) && getAliveChainSnapshotUuids().contains(childSnapshotUuid); - - boolean shouldUseCommitStrategy = current && !targetSnapshotIsLatest && online; + boolean targetOnAliveChain = isOnAliveChain(targetSnapshotUuid); + boolean childOnAliveChain = isOnAliveChain(childSnapshotUuid); + boolean shouldUseCommitStrategy = current && !targetSnapshotIsLatest && targetOnAliveChain && childOnAliveChain; if (Objects.equals(initialDirection, DeleteVolumeSnapshotDirection.Pull.toString()) && shouldUseCommitStrategy) { throw new IllegalArgumentException("the snapshot will be deleted by block 'commit', but the direction is 'pull', " + @@ -387,8 +394,10 @@ public DeleteVolumeSnapshotDirection resolveDirection(String targetSnapshotUuid, } public boolean isOnline(boolean treeIsCurrent, String targetSnapshotUuid, String childSnapshotUuid, VmInstanceState vmState) { - return treeIsCurrent && (vmState == VmInstanceState.Running || vmState == VmInstanceState.Paused) - && getAliveChainSnapshotUuids().contains(targetSnapshotUuid) && getAliveChainSnapshotUuids().contains(childSnapshotUuid); + return treeIsCurrent + && isHypervisorOperation(vmState) + && getAliveChainSnapshotUuids().contains(targetSnapshotUuid) + && getAliveChainSnapshotUuids().contains(childSnapshotUuid); } // TODO(clone) : When both chain cloning and single-node snapshot deletion are enabled,