linstor: delete backup scratch left behind when the agent starts

The agent deletes the LINSTOR scratch resource of a pull-mode backup once
the backup has ended, also when the backup fails or the agent is stopped
normally. If the agent dies or the host goes down during the backup, the
resource stays behind and keeps its storage allocated.

When the agent registers the first pool of a LINSTOR controller, delete
every cs-bkscratch-* resource definition that has a resource on this node:
a scratch always has one on the node whose agent created it, diskless if its
data lives elsewhere. Pools are registered again on every reconnect to the
management server, while backups may still be running, so this happens only
once per controller and agent process, before any backup can use the pool.
Failures are only logged and never keep the pool from being registered.

After a hard crash the VM's pull-mode backup job can outlive the agent and
still hold the scratch devices, so LINSTOR refuses the delete. The warning
then says to end the job with virsh domjobabort and delete the resource
definition manually.

Author: rp-

plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java

@@ -24,7 +24,9 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.UUID;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.stream.Collectors;
 
 import javax.annotation.Nonnull;
@@ -81,6 +83,8 @@ public class LinstorStorageAdaptor implements StorageAdaptor {
     static final String BACKUP_SCRATCH_PREFIX = "cs-bkscratch-";
     static final String BACKUP_SCRATCH_VM_PROP = "Aux/cs-backup-vm";
     private static final long DEVICE_WAIT_MS = 10000;
+    // Controllers this agent has already removed its leftover backup scratch from, see deleteLeftoverBackupScratch().
+    private static final Set<String> BackupScratchSweptControllers = ConcurrentHashMap.newKeySet();
     private final String localNodeName;
 
     DevelopersApi getLinstorAPI(KVMStoragePool pool) {
@@ -198,6 +202,11 @@ public KVMStoragePool createStoragePool(String name, String host, int port, Stri
                 : Boolean.parseBoolean(LinstorConfigurationManager.InsecureSsl.defaultValue());
         LinstorStoragePool storagePool = new LinstorStoragePool(name, host, port, userInfo, type, this, insecureSsl);
 
+        // Pools are registered again whenever the agent reconnects, but only the first registration of a
+        // controller precedes every backup that could be using its scratch space.
+        if (BackupScratchSweptControllers.add(host)) {
+            deleteLeftoverBackupScratch(storagePool);
+        }
         MapStorageUuidToStoragePool.put(name, storagePool);
 
         return storagePool;
@@ -265,8 +274,9 @@ void waitForDevice(String device) {
      * node attaches diskless.
      *
      * The agent deletes the scratch once the backup has ended. If it never gets there, e.g. because the agent
-     * crashed, the resource definition has to be deleted manually; its {@value #BACKUP_SCRATCH_VM_PROP}
-     * property names the VM.
+     * crashed or the host went down, it deletes the leftover the next time it starts, see
+     * {@link #deleteLeftoverBackupScratch(KVMStoragePool)}; the {@value #BACKUP_SCRATCH_VM_PROP} property
+     * names the VM.
      */
     public Pair<String, List<String>> createBackupScratch(KVMStoragePool pool, String vmName, List<Long> sizes) {
         final LinstorStoragePool lpool = (LinstorStoragePool) pool;
@@ -371,6 +381,47 @@ private List<String> getBackupScratchDevices(DevelopersApi api, String rscName)
         return devices;
     }
 
+    /**
+     * Deletes the backup scratch resources an earlier run of this agent left on this node, e.g. because it
+     * crashed or was restarted during a backup. Called when the first pool of a controller is registered,
+     * before any backup can use it, so none of them can belong to a running backup. A scratch always has a
+     * resource on the node whose agent created it, diskless if its data lives on another node. Failures are
+     * logged only: a leftover must not keep the pool from being registered.
+     */
+    void deleteLeftoverBackupScratch(KVMStoragePool pool) {
+        final DevelopersApi api = getLinstorAPI(pool);
+        try {
+            Map<String, ResourceDefinition> scratchRds = LinstorUtil.getRDListStartingWith(api, BACKUP_SCRATCH_PREFIX)
+                    .stream().collect(Collectors.toMap(ResourceDefinition::getName, rd -> rd));
+            if (scratchRds.isEmpty()) {
+                return;
+            }
+            List<String> leftovers = api.viewResources(Collections.singletonList(localNodeName),
+                    new ArrayList<>(scratchRds.keySet()), Collections.emptyList(), null, null, null)
+                    .stream().map(Resource::getName).distinct().collect(Collectors.toList());
+            for (String rscName : leftovers) {
+                Properties props = scratchRds.get(rscName).getProps();
+                String vmName = props != null ? props.get(BACKUP_SCRATCH_VM_PROP) : null;
+                logger.warn("Linstor: deleting backup scratch {} of VM {} left behind on {}", rscName, vmName,
+                        localNodeName);
+                try {
+                    deleteBackupScratch(pool, rscName);
+                } catch (CloudRuntimeException e) {
+                    // Typically after a crash during a backup: the VM's pull-mode backup job outlived the agent
+                    // and still holds the scratch devices.
+                    logger.warn("Linstor: unable to delete leftover backup scratch {}: {}. If VM {} still has a backup "
+                            + "job, end it with 'virsh domjobabort {}', then delete the resource definition manually.",
+                            rscName, e.getMessage(), vmName, vmName);
+                }
+            }
+        } catch (ApiException apiEx) {
+            logger.warn("Linstor: unable to look for leftover backup scratch on {}: {}", localNodeName,
+                    apiEx.getBestMessage());
+        } catch (CloudRuntimeException e) {
+            logger.warn("Linstor: unable to look for leftover backup scratch on {}: {}", localNodeName, e.getMessage());
+        }
+    }
+
     public void deleteBackupScratch(KVMStoragePool pool, String rscName) {
         logger.info("Linstor: deleting backup scratch {}", rscName);
         try {

plugins/storage/volume/linstor/src/test/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptorTest.java

@@ -53,6 +53,7 @@
 import com.linbit.linstor.api.model.AutoPlaceRequest;
 import com.linbit.linstor.api.model.DrbdResource;
 import com.linbit.linstor.api.model.LayerType;
+import com.linbit.linstor.api.model.Properties;
 import com.linbit.linstor.api.model.ResourceDefinition;
 import com.linbit.linstor.api.model.ResourceDefinitionCreate;
 import com.linbit.linstor.api.model.ResourceLayer;
@@ -279,4 +280,65 @@ public void connectPhysicalDiskFailsWithoutLocalVolume() throws ApiException {
     public void waitForDeviceReturnsForAnExistingDevice() {
         new LinstorStorageAdaptor().waitForDevice("/dev/null");
     }
+
+    private static ResourceDefinition scratchRd(String name, String vmName) {
+        ResourceDefinition rd = new ResourceDefinition();
+        rd.setName(name);
+        Properties props = new Properties();
+        props.put(LinstorStorageAdaptor.BACKUP_SCRATCH_VM_PROP, vmName);
+        rd.setProps(props);
+        return rd;
+    }
+
+    private static ResourceWithVolumes resourceOf(String name) {
+        ResourceWithVolumes rsc = new ResourceWithVolumes();
+        rsc.setName(name);
+        return rsc;
+    }
+
+    // The controllers this agent has swept are remembered per process, so every test uses its own one.
+    private KVMStoragePool registerPool(String controller) {
+        return adaptor.createStoragePool("pool-" + controller, controller, 3370, "cloudstack", null,
+                Storage.StoragePoolType.Linstor, null, true);
+    }
+
+    @Test
+    @SuppressWarnings("unchecked")
+    public void createStoragePoolDeletesLeftoverScratchOfThisNode() throws ApiException {
+        ResourceDefinition volume = new ResourceDefinition();
+        volume.setName("cs-vol-1");
+        when(api.resourceDefinitionList(null, false, null, null, null)).thenReturn(Arrays.asList(
+                scratchRd("cs-bkscratch-here", "i-2-3-VM"), scratchRd("cs-bkscratch-elsewhere", "i-2-4-VM"), volume));
+        ArgumentCaptor<List<String>> rscNames = ArgumentCaptor.forClass(List.class);
+        when(api.viewResources(eq(Collections.singletonList(localNode)), rscNames.capture(), any(), any(), any(), any()))
+                .thenReturn(Collections.singletonList(resourceOf("cs-bkscratch-here")));
+        when(api.resourceDefinitionDelete("cs-bkscratch-here")).thenReturn(success());
+
+        Assert.assertNotNull(registerPool("http://sweep-1:3370"));
+
+        Assert.assertEquals(2, rscNames.getValue().size());
+        Assert.assertFalse(rscNames.getValue().contains("cs-vol-1"));
+        verify(api).resourceDefinitionDelete("cs-bkscratch-here");
+        verify(api, never()).resourceDefinitionDelete("cs-bkscratch-elsewhere");
+    }
+
+    @Test
+    public void createStoragePoolSweepsEachControllerOnlyOnce() throws ApiException {
+        when(api.resourceDefinitionList(null, false, null, null, null)).thenReturn(Collections.emptyList());
+
+        registerPool("http://sweep-2:3370");
+        registerPool("http://sweep-2:3370");
+
+        verify(api, times(1)).resourceDefinitionList(null, false, null, null, null);
+    }
+
+    @Test
+    public void createStoragePoolRegistersPoolWhenControllerIsUnreachable() throws ApiException {
+        when(api.resourceDefinitionList(null, false, null, null, null)).thenThrow(new ApiException("connection refused"));
+
+        KVMStoragePool registered = registerPool("http://sweep-3:3370");
+
+        Assert.assertSame(registered, adaptor.getStoragePool("pool-http://sweep-3:3370"));
+        verify(api, never()).resourceDefinitionDelete(anyString());
+    }
 }