Skip to content

Commit 8377415

Browse files
committed
StorPool: clean up volumes left behind by failed NAS backup/restore
Restore (KVM agent): - When restoring a backed-up volume to a new StorPool volume fails, either while copying the backup data or while attaching the volume to the running VM, the created volume was leaked. The management server persists the restored volume only on success, and StorPool assigns the volume's identity itself, so nothing else could find and delete it. - Add StorageAdaptor#cleanupCreatedPhysicalDisk (default: no-op) and call it from LibvirtRestoreBackupCommandWrapper on both failure paths. The StorPool implementation detaches the volume from this host and deletes it. The restore still fails; a cleanup failure is only logged. - Fix the delete when the attach in StorPoolStorageAdaptor #createPhysicalDisk fails: the volume was deleted by its bare globalId instead of "~<globalId>". Backup (nasbackup.sh) - Delete the clone with onAttached=detachForce, which removes it even if the attach succeeded but the device symlink never appeared. This replaces the separate detach-with-retries step.
1 parent 93238b0 commit 8377415

5 files changed

Lines changed: 119 additions & 26 deletions

File tree

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

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,12 +193,14 @@ private String restoreVolume(KVMStoragePoolManager storagePoolMgr, String backup
193193
volumeUuid = getVolumeUuidFromPath(volumePath, volumePool);
194194
verifyBackupFile(bkpPath, volumeUuid);
195195
Pair<Boolean, String> restoreResult = replaceVolumeWithBackup(storagePoolMgr, volumePool, volumePath, bkpPath, timeout, true, size);
196+
volumePath = restoreResult.second();
196197
if (!restoreResult.first()) {
198+
cleanupCreatedVolume(storagePoolMgr, volumePool, volumePath);
197199
throw new CloudRuntimeException(String.format("Unable to restore contents from the backup volume [%s].", volumeUuid));
198200
}
199-
volumePath = restoreResult.second();
200201
if (VirtualMachine.State.Running.equals(vmNameAndState.second())) {
201202
if (!attachVolumeToVm(storagePoolMgr, vmNameAndState.first(), volumePool, volumePath)) {
203+
cleanupCreatedVolume(storagePoolMgr, volumePool, volumePath);
202204
throw new CloudRuntimeException(String.format("Failed to attach volume to VM: %s", vmNameAndState.first()));
203205
}
204206
}
@@ -209,6 +211,22 @@ private String restoreVolume(KVMStoragePoolManager storagePoolMgr, String backup
209211
}
210212
}
211213

214+
/**
215+
* Best-effort release of a volume created for a restore that then failed. A failed restore
216+
* returns no volume path to the management server, so for storages that assign the created
217+
* volume's identity themselves (StorPool) nobody else could find and delete it.
218+
*/
219+
private void cleanupCreatedVolume(KVMStoragePoolManager storagePoolMgr, PrimaryDataStoreTO volumePool, String volumePath) {
220+
try {
221+
if (!storagePoolMgr.cleanupCreatedPhysicalDisk(volumePool.getPoolType(), volumePool.getUuid(), volumePath)) {
222+
logger.warn("Volume {} created for the failed restore was not cleaned up by the {} storage adaptor; it may need to be deleted manually",
223+
volumePath, volumePool.getPoolType());
224+
}
225+
} catch (Exception e) {
226+
logger.warn("Failed to clean up volume {} created for the failed restore; it may need to be deleted manually", volumePath, e);
227+
}
228+
}
229+
212230

213231
private String mountBackupDirectory(String backupRepoAddress, String backupRepoType, String mountOptions, Integer mountTimeout) {
214232
String randomChars = RandomStringUtils.random(5, true, false);

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -443,6 +443,13 @@ public boolean disconnectPhysicalDisk(StoragePoolType type, String poolUuid, Str
443443
return adaptor.disconnectPhysicalDisk(volPath, pool);
444444
}
445445

446+
public boolean cleanupCreatedPhysicalDisk(StoragePoolType type, String poolUuid, String volPath) {
447+
StorageAdaptor adaptor = getStorageAdaptor(type);
448+
KVMStoragePool pool = adaptor.getStoragePool(poolUuid);
449+
450+
return adaptor.cleanupCreatedPhysicalDisk(volPath, pool);
451+
}
452+
446453
public synchronized boolean deleteStoragePool(StoragePoolType type, String uuid) {
447454
StorageAdaptor adaptor = getStorageAdaptor(type);
448455
if (type == StoragePoolType.NetworkFilesystem) {

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/StorageAdaptor.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,4 +152,18 @@ default Pair<Boolean, String> unprepareStorageClient(String uuid, Map<String, St
152152
default void createTemplate(String templatePath, String templateUuid, int timeout, KVMStoragePool pool) {
153153
// no-op
154154
}
155+
156+
/**
157+
* Releases a disk this host created via {@link #createPhysicalDisk} that could not be put into
158+
* use (e.g. populating it from a backup failed), for storages where the management server
159+
* can't clean it up because the storage assigned the disk's identity itself: disconnects it
160+
* from this host and deletes it from the storage.
161+
*
162+
* @param volumePath the path of the disk, as returned in {@link KVMPhysicalDisk#getPath()}
163+
* @param pool storage pool the disk is part of
164+
* @return true if the disk was deleted; false if it wasn't or the adaptor doesn't support it
165+
*/
166+
default boolean cleanupCreatedPhysicalDisk(String volumePath, KVMStoragePool pool) {
167+
return false;
168+
}
155169
}

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

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,26 @@ public boolean deletePhysicalDisk(String volumeUuid, KVMStoragePool pool, Storag
327327
return true; // apparently ignored
328328
}
329329

330+
@Override
331+
public boolean cleanupCreatedPhysicalDisk(String volumePath, KVMStoragePool pool) {
332+
SP_LOG("StorPoolStorageAdaptor.cleanupCreatedPhysicalDisk: path=%s, pool=%s", volumePath, pool);
333+
final String name = getVolumeNameFromPath(volumePath, true);
334+
if (name == null) {
335+
LOGGER.warn(String.format("Unable to clean up created disk '%s': not a StorPool volume path", volumePath));
336+
return false;
337+
}
338+
339+
try {
340+
disconnectPhysicalDisk(volumePath, pool);
341+
volumeDelete(name);
342+
} catch (CloudRuntimeException e) {
343+
SP_LOG("Failed to delete created StorPool volume %s: %s", name, e.getMessage());
344+
LOGGER.warn(String.format("Failed to delete created StorPool volume %s, it must be deleted manually: %s", name, e.getMessage()));
345+
return false;
346+
}
347+
return true;
348+
}
349+
330350
@Override
331351
public List<KVMPhysicalDisk> listPhysicalDisks(String storagePoolUuid, KVMStoragePool pool) {
332352
SP_LOG("StorPoolStorageAdaptor.listPhysicalDisks: uuid=%s, pool=%s", storagePoolUuid, pool);
@@ -488,7 +508,7 @@ public KVMPhysicalDisk createPhysicalDisk(String name, KVMStoragePool pool, Phys
488508
try {
489509
attachOrDetachVolume("attach", "volume", volumePath);
490510
} catch (CloudRuntimeException e) {
491-
volumeDelete(globalId);
511+
volumeDelete("~" + globalId);
492512
throw e;
493513
}
494514

‎scripts/vm/hypervisor/kvm/nasbackup.sh‎

Lines changed: 58 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -168,23 +168,61 @@ sp_wait_for_device_symlink() {
168168
return 1
169169
}
170170

171-
# detach is retried (a live volume's detach can transiently fail while still in use), attach is
172-
# not. A successful attach also waits for the device symlink.
173-
sp_attach_detach_volume() {
174-
local cmd="$1" name="$2"
175-
local tries=10 i
176-
for ((i = 0; i < tries; i++)); do
177-
if [[ "$cmd" == "attach" ]]; then
178-
storpool -M -B attach volume "$name" here onRemoteAttached export >>"$logFile" 2>&1 && { sp_wait_for_device_symlink "$name"; return $?; }
179-
break
180-
else
181-
storpool -M -B detach volume "$name" here >>"$logFile" 2>&1 && return 0
182-
sleep 1
183-
fi
184-
done
171+
# A successful attach also waits for the device symlink.
172+
sp_attach_volume() {
173+
local name="$1"
174+
storpool -M -B attach volume "$name" here onRemoteAttached export >>"$logFile" 2>&1 && { sp_wait_for_device_symlink "$name"; return $?; }
185175
return 1
186176
}
187177

178+
sp_api_post() {
179+
local query="$1" json="$2"
180+
local key value host="" port="81" token=""
181+
while IFS='=' read -r key value; do
182+
case "$key" in
183+
SP_API_HTTP_HOST) host="$value" ;;
184+
SP_API_HTTP_PORT) port="$value" ;;
185+
SP_AUTH_TOKEN) token="$value" ;;
186+
esac
187+
done < <(storpool_confget 2>>"$logFile")
188+
if [[ -z "$host" || -z "$token" ]]; then
189+
log -ne "StorPool: SP_API_HTTP_HOST or SP_AUTH_TOKEN missing from storpool_confget, cannot call $query"
190+
return 1
191+
fi
192+
193+
local resp
194+
if ! resp=$(curl -sS --connect-timeout 10 -X POST \
195+
-H @<(printf 'Authorization: Storpool v1:%s\n' "$token") \
196+
-H "Content-Type: application/json" -d "$json" \
197+
"http://$host:$port/ctrl/1.0/$query" 2>>"$logFile"); then
198+
log -ne "StorPool: API call $query failed, see above for curl output"
199+
return 1
200+
fi
201+
python3 -c '
202+
import sys, json
203+
try:
204+
resp = json.load(sys.stdin)
205+
except Exception:
206+
sys.exit(1)
207+
if "error" in resp or "data" not in resp:
208+
sys.exit(1)
209+
print(json.dumps(resp["data"]))
210+
' <<< "$resp" || {
211+
log -ne "StorPool: API call $query returned an error: $resp"
212+
return 1
213+
}
214+
}
215+
216+
# Deletes $1, force-detaching it first wherever it is still attached (StorPool refuses to delete
217+
# an attached volume). Returns 1, after logging, if the delete failed.
218+
sp_delete_volume() {
219+
local volume_name="$1"
220+
if ! sp_api_post "MultiCluster/VolumeDelete/$volume_name" '{"onAttached":"detachForce"}' >/dev/null; then
221+
log -ne "StorPool: failed to delete volume $volume_name, it must be deleted manually"
222+
return 1
223+
fi
224+
}
225+
188226
# Clones $1 (a live StorPool volume path) into a new, point-in-time StorPool volume via the
189227
# VolumeCreate API's baseOn option, attaches it, and echoes the clone's device path. Only called
190228
# once this script's own (fresh, right-here) VM liveness check has already routed to the cold
@@ -200,8 +238,8 @@ sp_create_backup_source_disk() {
200238
log -ne "StorPool: creating backup source volume based on $base_on (from $volume_path)"
201239

202240
local resp
203-
if ! resp=$(storpool_req -P -M --json "{\"baseOn\":\"$base_on\",\"tags\":{\"cs\":\"backup\"}}" VolumeCreate 2>>"$logFile"); then
204-
log -ne "StorPool: VolumeCreate baseOn=$base_on failed, see above for storpool_req output"
241+
if ! resp=$(sp_api_post "MultiCluster/VolumeCreate" "{\"baseOn\":\"$base_on\",\"tags\":{\"cs\":\"backup\"}}"); then
242+
log -ne "StorPool: VolumeCreate baseOn=$base_on failed, see above"
205243
echo "Could not create a backup source volume based on $base_on" >&2
206244
return 1
207245
fi
@@ -226,9 +264,9 @@ print(gid)
226264
local clone_path="/dev/storpool-byid/$global_id"
227265
local clone_name="~$global_id"
228266
log -ne "StorPool: created backup source volume $clone_name ($clone_path), attaching"
229-
if ! sp_attach_detach_volume "attach" "$clone_name"; then
267+
if ! sp_attach_volume "$clone_name"; then
230268
log -ne "StorPool: failed to attach backup source volume $clone_name, deleting it"
231-
storpool_req -P -M VolumeDelete "$clone_name" >>"$logFile" 2>&1 || true
269+
sp_delete_volume "$clone_name" || true
232270
echo "Could not attach backup source volume $clone_path cloned from $base_on" >&2
233271
return 1
234272
fi
@@ -241,16 +279,12 @@ print(gid)
241279
echo "$clone_path"
242280
}
243281

244-
# Detach + delete a volume previously returned by sp_create_backup_source_disk. Best-effort:
282+
# Deletes a volume previously returned by sp_create_backup_source_disk. Best-effort:
245283
# logs on failure rather than aborting the backup, matching the wrapper's old cleanup semantics.
246284
sp_delete_backup_source_disk() {
247285
local clone_name="$1"
248286
log -ne "StorPool: releasing backup source volume $clone_name"
249-
sp_attach_detach_volume "detach" "$clone_name" || log -ne "Failed to detach StorPool backup source volume $clone_name"
250-
if ! storpool_req -P -M VolumeDelete "$clone_name" >>"$logFile" 2>&1; then
251-
log -ne "Failed to delete StorPool backup source volume $clone_name"
252-
return 1
253-
fi
287+
sp_delete_volume "$clone_name"
254288
}
255289

256290
# Safety net for any clone not already released by its own per-disk cleanup (e.g. the script

0 commit comments

Comments
 (0)