addressed reviews

Author: slavkap

plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java

@@ -384,17 +384,15 @@ protected boolean allVolumesOnCheckpointCapableStorage(VirtualMachine vm) {
     }
 
     /**
-     * True when any of the VM's volumes sits on a StorPool pool. Used to decide whether volume
-     * pool/path info must be sent to the agent even though the VM currently looks Running — see
-     * the caller in {@link #takeBackup}.
+     * True when any of the given volumes sits on a pool of the given type.
      */
-    private boolean hasStorPoolVolume(List<VolumeVO> volumes) {
+    private boolean hasVolumeOnPoolType(List<VolumeVO> volumes, Storage.StoragePoolType poolType) {
         if (volumes == null) {
             return false;
         }
         for (VolumeVO volume : volumes) {
             StoragePoolVO pool = primaryDataStoreDao.findById(volume.getPoolId());
-            if (pool != null && Storage.StoragePoolType.StorPool.equals(pool.getPoolType())) {
+            if (pool != null && poolType.equals(pool.getPoolType())) {
                 return true;
             }
         }
@@ -613,7 +611,7 @@ public Pair<Boolean, Backup> takeBackup(final VirtualMachine vm, Boolean quiesce
         // — without it there is nothing to derive that from.
         boolean vmStopped = VirtualMachine.State.Stopped.equals(vm.getState());
         List<VolumeVO> vmVolumes = volumeDao.findByInstance(vm.getId());
-        boolean hasStorPoolVolume = hasStorPoolVolume(vmVolumes);
+        boolean hasStorPoolVolume = hasVolumeOnPoolType(vmVolumes, Storage.StoragePoolType.StorPool);
         if (vmStopped || hasStorPoolVolume) {
             if (!vmStopped) {
                 logger.debug("VM {} looks {} but has a StorPool volume — sending volume pool/path info to the agent anyway, "

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java

@@ -422,7 +422,7 @@ private boolean attachVolumeToVm(KVMStoragePoolManager storagePoolMgr, String vm
         virshCmd.add(deviceToAttachDiskTo);
         virshCmd.add("--driver");
         virshCmd.add("qemu");
-        if (!Storage.StoragePoolType.Linstor.equals(volumePool.getPoolType())) {
+        if (!List.of(Storage.StoragePoolType.Linstor, Storage.StoragePoolType.StorPool).contains(volumePool.getPoolType())) {
             virshCmd.add("--subdriver");
             virshCmd.add("qcow2");
         }

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java

@@ -695,6 +695,17 @@ public void testAttachVolumeOmitsQcow2SubdriverForLinstor() throws Exception {
         Assert.assertFalse(args.contains("--subdriver"));
     }
 
+    @Test
+    public void testAttachVolumeOmitsQcow2SubdriverForStorPool() throws Exception {
+        String[] cmd = captureAttachCommand(Storage.StoragePoolType.StorPool);
+        List<String> args = Arrays.asList(cmd);
+
+        // StorPoolStorageAdaptor#createPhysicalDisk always provisions RAW block devices,
+        // declaring qcow2 makes libvirt try to parse the raw data as a qcow2 image.
+        Assert.assertTrue(args.contains("attach-disk"));
+        Assert.assertFalse(args.contains("--subdriver"));
+    }
+
     @Test
     public void testAttachVolumePassesRbdXmlThroughAFile() throws Exception {
         String[] cmd = captureAttachCommand(Storage.StoragePoolType.RBD);

plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java

@@ -485,9 +485,11 @@ public KVMPhysicalDisk createPhysicalDisk(String name, KVMStoragePool pool, Phys
         }
 
         String volumePath = StorPoolUtil.devPath(globalId);
-        if (!attachOrDetachVolume("attach", "volume", volumePath)) {
+        try {
+            attachOrDetachVolume("attach", "volume", volumePath);
+        } catch (CloudRuntimeException e) {
             volumeDelete(globalId);
-            throw new CloudRuntimeException(String.format("Could not attach newly created StorPool volume %s", volumePath));
+            throw e;
         }
 
         KVMPhysicalDisk disk = new KVMPhysicalDisk(volumePath, name, pool);

scripts/vm/hypervisor/kvm/nasbackup.sh

@@ -233,7 +233,10 @@ print(gid)
     return 1
   fi
 
-  SP_CLEANUP_VOLUMES+=("$clone_name")
+  # Not registered in SP_CLEANUP_VOLUMES here: the caller always invokes this function via
+  # command substitution ($(...)), which runs it in a subshell — any append to the array here
+  # would be a mutation of that subshell's own copy and vanish once it exits. The caller must
+  # register $clone_name itself right after reading it back from stdout below.
   log -ne "StorPool: backup source volume $clone_name attached at $clone_path, reading from it for this backup"
   echo "$clone_path"
 }
@@ -244,7 +247,10 @@ sp_delete_backup_source_disk() {
   local clone_name="$1"
   log -ne "StorPool: releasing backup source volume $clone_name"
   sp_attach_detach_volume "detach" "$clone_name" || log -ne "Failed to detach StorPool backup source volume $clone_name"
-  storpool_req -P -M VolumeDelete "$clone_name" >>"$logFile" 2>&1 || log -ne "Failed to delete StorPool backup source volume $clone_name"
+  if ! storpool_req -P -M VolumeDelete "$clone_name" >>"$logFile" 2>&1; then
+    log -ne "Failed to delete StorPool backup source volume $clone_name"
+    return 1
+  fi
 }
 
 # Safety net for any clone not already released by its own per-disk cleanup (e.g. the script
@@ -580,6 +586,9 @@ backup_stopped_vm() {
         exit 1
       fi
       sp_clone_name=$(sp_volume_name_from_path "$read_disk")
+      # Register the clone here, in the parent shell, right after the command substitution
+      # above returns — sp_create_backup_source_disk cannot do this itself (see its comment).
+      SP_CLEANUP_VOLUMES+=("$sp_clone_name")
     else
       volUuid="${disk##*/}"
     fi
@@ -593,13 +602,16 @@ backup_stopped_vm() {
     log -ne "Wrote $output from $read_disk"
 
     if [[ -n "$sp_clone_name" ]]; then
-      sp_delete_backup_source_disk "$sp_clone_name"
-      local -a remaining_clones=()
-      local tracked_clone
-      for tracked_clone in "${SP_CLEANUP_VOLUMES[@]}"; do
-        [[ "$tracked_clone" == "$sp_clone_name" ]] || remaining_clones+=("$tracked_clone")
-      done
-      SP_CLEANUP_VOLUMES=("${remaining_clones[@]}")
+      # Only drop the clone from SP_CLEANUP_VOLUMES once it's actually deleted — otherwise the
+      # EXIT trap's safety net would never see (and retry) a clone left behind by a failed delete.
+      if sp_delete_backup_source_disk "$sp_clone_name"; then
+        local -a remaining_clones=()
+        local tracked_clone
+        for tracked_clone in "${SP_CLEANUP_VOLUMES[@]}"; do
+          [[ "$tracked_clone" == "$sp_clone_name" ]] || remaining_clones+=("$tracked_clone")
+        done
+        SP_CLEANUP_VOLUMES=("${remaining_clones[@]}")
+      fi
     fi
 
     # Pre-seed a persistent bitmap on the source disk so the NEXT backup (taken

test/integration/plugins/storpool/TestNasBackupStorPool.py

@@ -556,6 +556,68 @@ def test_05_backup_stopped_vm(self):
 
         self.backup_offering.removeOffering(self.apiclient, self.vm.id)
 
+    @attr(tags=["advanced", "backup", "storpool"], required_hardware="true")
+    def test_06_restore_volume_attach_to_running_vm(self):
+        """Restore a volume from backup and hot-attach it to an already-Running VM.
+
+        Regression test for LibvirtRestoreBackupCommandWrapper.attachVolumeToVm only
+        excluding LINSTOR from the qcow2 --subdriver flag: a restored StorPool volume
+        is a RAW block device (like LINSTOR), so attaching it with --subdriver qcow2
+        makes virsh try to open the raw device as a qcow2 image. test_03 never caught
+        this because it attaches to a target VM that is still Stopped at attach time.
+        """
+        self.backup_offering.assignOffering(self.apiclient, self.vm.id)
+
+        marker = "storpool_nas_running_attach_%s.txt" % int(time.time())
+        self._write_guest_marker(self.vm, marker)
+        time.sleep(5)
+        backup = self._create_backup(self.vm.id)
+        self.cleanup_backups.append(backup)
+
+        volumes = Volume.list(self.apiclient, virtualmachineid=self.vm.id, listall=True)
+        data_disk = next((v for v in volumes if v.type == "DATADISK"), None)
+        self.assertIsNotNone(data_disk, "Source VM should have a data disk")
+
+        # No startvm=False here: the target VM must already be Running when
+        # restoreVolumeFromBackupAndAttachToVM is called, to exercise the
+        # live virsh attach-disk path instead of an offline volume write.
+        target_vm = VirtualMachine.create(
+            self.apiclient,
+            {"name": "SP-NasRunAttach-%s" % uuid.uuid4()},
+            zoneid=self.zone.id,
+            templateid=self.template.id,
+            accountid=self.account.name,
+            domainid=self.account.domainid,
+            serviceofferingid=self.service_offering.id,
+            hypervisor=self.hypervisor,
+            networkids=self.network_ids,
+            rootdisksize=self.template_size_gb,
+            mode=self.services["mode"])
+        self.cleanup.append(target_vm)
+
+        listed = VirtualMachine.list(self.apiclient, id=target_vm.id, listall=True) or []
+        self.assertTrue(listed and listed[0].state == "Running",
+                        "Target VM should be Running before the volume is hot-attached")
+
+        Backup.restoreVolumeFromBackupAndAttachToVM(
+            self.apiclient,
+            backupid=backup.id,
+            volumeid=data_disk.id,
+            virtualmachineid=target_vm.id)
+
+        target_volumes = self._verify_vm_volumes_on_storpool(target_vm)
+        self.assertEqual(2, len(target_volumes),
+                         "Target VM should have ROOT + hot-attached restored DATADISK volumes")
+        self._assert_guest_reachable(
+            target_vm, msg="VM should stay reachable after a volume is hot-attached from backup")
+
+        ssh = target_vm.get_ssh_client(reconnect=True)
+        result = ssh.execute("lsblk -d -n -o NAME | grep -c .")
+        self.assertTrue(result and int(result[0].strip()) >= 2,
+                        "Guest should see the hot-attached disk after restore")
+
+        self.backup_offering.removeOffering(self.apiclient, self.vm.id)
+
 
 class TestNasBackupMixedStorage(TestNasBackupStorPool):
     """Repeats the NAS backup smoke tests (test_01 .. test_05, inherited