Copilot commented on code in PR #12796:
URL: https://github.com/apache/cloudstack/pull/12796#discussion_r3043214978
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -413,7 +424,9 @@ public Pair<Boolean, String> restoreBackedUpVolume(Backup
backup, Backup.VolumeI
restoreCommand.setBackupRepoType(backupRepository.getType());
restoreCommand.setBackupRepoAddress(backupRepository.getAddress());
restoreCommand.setVmName(vmNameAndState.first());
-
restoreCommand.setRestoreVolumePaths(Collections.singletonList(String.format("%s/%s",
getVolumePathPrefix(pool), volumeUUID)));
+ String restoreVolumePath = String.format("%s%s%s",
getVolumePathPrefix(pool), volumeUUID, getVolumePathSuffix(pool));
+
restoreCommand.setRestoreVolumePaths(Collections.singletonList(restoreVolumePath));
+
restoreCommand.setRestoreVolumeSizes(Collections.singletonList(volume.getSize()));
Review Comment:
`restoreVolumeSizes` is derived from `volume.getSize()`, but this restore
operation is for the backed-up volume where the authoritative size is
`matchingVolume.getSize()` / `backedUpVolumeSize` (already computed above). If
the original volume was resized after the backup was taken, using
`volume.getSize()` can create a Linstor target volume with the wrong size and
cause restore failures or data truncation.
```suggestion
restoreCommand.setRestoreVolumeSizes(Collections.singletonList(backedUpVolumeSize));
```
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -89,17 +89,42 @@ sanity_checks() {
### Operation methods ###
+get_ceph_uuid_from_path() {
+ local fullpath="$1"
+ # disk for rbd => rbd:<pool>/<uuid>:mon_host=<monitor_host>...
+ # sample:
rbd:cloudstack/53d5c355-d726-4d3e-9422-046a503a0b12:mon_host=10.0.1.2...
+ beforeUuid="${fullpath#*/}" # Remove up to first slash after rbd:
+ volUuid="${beforeUuid%%:*}" # Remove everything after colon to get the uuid
+ echo $volUuid
+}
+
+get_linstor_uuid_from_path() {
+ local fullpath="$1"
+ # disk for linstor => /dev/drbd/by-res/cs-<uuid>/0
+ # sample: /dev/drbd/by-res/cs-53d5c355-d726-4d3e-9422-046a503a0b12/0
+ beforeUuid="${fullpath#/dev/drbd/by-res/}"
+ volUuid="${beforeUuid%%/*}"
+ volUuid="${volUuid#cs-}"
+ echo $volUuid
Review Comment:
In these helper functions, `beforeUuid` and `volUuid` are not declared as
local variables, and `echo $volUuid` is unquoted. This can leak/overwrite
variables in the caller scope and can mangle values containing special
characters. Declare intermediate variables as `local` and use `echo "$volUuid"`
(or `printf '%s\n' "$volUuid"`).
```suggestion
local beforeUuid="${fullpath#*/}" # Remove up to first slash after rbd:
local volUuid="${beforeUuid%%:*}" # Remove everything after colon to get
the uuid
echo "$volUuid"
}
get_linstor_uuid_from_path() {
local fullpath="$1"
# disk for linstor => /dev/drbd/by-res/cs-<uuid>/0
# sample: /dev/drbd/by-res/cs-53d5c355-d726-4d3e-9422-046a503a0b12/0
local beforeUuid="${fullpath#/dev/drbd/by-res/}"
local volUuid="${beforeUuid%%/*}"
volUuid="${volUuid#cs-}"
echo "$volUuid"
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -56,10 +56,25 @@ public class LibvirtRestoreBackupCommandWrapper extends
CommandWrapper<RestoreBa
private static final String UMOUNT_COMMAND = "sudo umount %s";
private static final String FILE_PATH_PLACEHOLDER = "%s/%s";
private static final String ATTACH_QCOW2_DISK_COMMAND = " virsh
attach-disk %s %s %s --driver qemu --subdriver qcow2 --cache none";
+ private static final String ATTACH_RAW_DISK_COMMAND = " virsh attach-disk
%s %s %s --driver qemu --cache none";
private static final String ATTACH_RBD_DISK_XML_COMMAND = " virsh
attach-device %s /dev/stdin <<EOF%sEOF";
private static final String CURRRENT_DEVICE = "virsh domblklist --domain
%s | tail -n 3 | head -n 1 | awk '{print $1}'";
private static final String RSYNC_COMMAND = "rsync -az %s %s";
+ private String getVolumeUuidFromPath(String volumePath, PrimaryDataStoreTO
volumePool) {
+ if (Storage.StoragePoolType.Linstor.equals(volumePool.getPoolType())) {
+ Path path = Paths.get(volumePath);
+ String rscName = path.getParent().getFileName().toString();
+ if (rscName.startsWith("cs-")) {
+ rscName = rscName.substring(3);
+ }
+ return rscName;
+ } else {
+ int lastIndex = volumePath.lastIndexOf("/");
+ return volumePath.substring(lastIndex + 1);
+ }
+ }
Review Comment:
New Linstor-specific logic was added (path parsing in
`getVolumeUuidFromPath`, block-device restore in
`replaceBlockDeviceWithBackup`, and RAW attach), but the unit tests only
exercise NetworkFilesystem/RBD-independent paths. Add a test case covering a
Linstor restore path like `/dev/drbd/by-res/cs-<uuid>/0` to verify the derived
volume UUID and that the wrapper takes the expected Linstor branch.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -247,28 +262,43 @@ private boolean checkBackupPathExists(String backupPath) {
}
private boolean replaceVolumeWithBackup(KVMStoragePoolManager
storagePoolMgr, PrimaryDataStoreTO volumePool, String volumePath, String
backupPath, int timeout) {
- return replaceVolumeWithBackup(storagePoolMgr, volumePool, volumePath,
backupPath, timeout, false);
+ return replaceVolumeWithBackup(storagePoolMgr, volumePool, volumePath,
backupPath, timeout, false, null);
}
- private boolean replaceVolumeWithBackup(KVMStoragePoolManager
storagePoolMgr, PrimaryDataStoreTO volumePool, String volumePath, String
backupPath, int timeout, boolean createTargetVolume) {
- if (volumePool.getPoolType() != Storage.StoragePoolType.RBD) {
- int exitValue =
Script.runSimpleBashScriptForExitValue(String.format(RSYNC_COMMAND, backupPath,
volumePath));
- return exitValue == 0;
+ private boolean replaceVolumeWithBackup(KVMStoragePoolManager
storagePoolMgr, PrimaryDataStoreTO volumePool, String volumePath, String
backupPath, int timeout, boolean createTargetVolume, Long size) {
+ if (List.of(Storage.StoragePoolType.RBD,
Storage.StoragePoolType.Linstor).contains(volumePool.getPoolType())) {
+ return replaceBlockDeviceWithBackup(storagePoolMgr, volumePool,
volumePath, backupPath, timeout, createTargetVolume, size);
}
- return replaceRbdVolumeWithBackup(storagePoolMgr, volumePool,
volumePath, backupPath, timeout, createTargetVolume);
+ int exitValue =
Script.runSimpleBashScriptForExitValue(String.format(RSYNC_COMMAND, backupPath,
volumePath));
+ return exitValue == 0;
}
- private boolean replaceRbdVolumeWithBackup(KVMStoragePoolManager
storagePoolMgr, PrimaryDataStoreTO volumePool, String volumePath, String
backupPath, int timeout, boolean createTargetVolume) {
+ private boolean replaceBlockDeviceWithBackup(KVMStoragePoolManager
storagePoolMgr, PrimaryDataStoreTO volumePool, String volumePath, String
backupPath, int timeout, boolean createTargetVolume, Long size) {
KVMStoragePool volumeStoragePool =
storagePoolMgr.getStoragePool(volumePool.getPoolType(), volumePool.getUuid());
QemuImg qemu;
try {
qemu = new QemuImg(timeout * 1000, true, false);
- if (!createTargetVolume) {
- KVMPhysicalDisk rdbDisk =
volumeStoragePool.getPhysicalDisk(volumePath);
- logger.debug("Restoring RBD volume: {}", rdbDisk.toString());
+ String volumeUuid = getVolumeUuidFromPath(volumePath, volumePool);
+ KVMPhysicalDisk disk = null;
+ if (createTargetVolume) {
+ if
(Storage.StoragePoolType.Linstor.equals(volumePool.getPoolType())) {
+ if (size == null) {
+ throw new CloudRuntimeException("Restore volume size
is required for Linstor pool when creating target volume");
+ }
+ disk = volumeStoragePool.createPhysicalDisk(volumeUuid,
QemuImg.PhysicalDiskFormat.RAW, Storage.ProvisioningType.THIN, size, null);
+ }
+ } else {
+ if
(Storage.StoragePoolType.Linstor.equals(volumePool.getPoolType())) {
+
storagePoolMgr.connectPhysicalDisk(volumePool.getPoolType(),
volumePool.getUuid(), volumeUuid, null);
+ } else {
+ disk = volumeStoragePool.getPhysicalDisk(volumePath);
+ }
qemu.setSkipTargetVolumeCreation(true);
}
+ if (disk != null) {
+ logger.debug("Restoring volume: {}", disk.toString());
+ }
} catch (LibvirtException ex) {
throw new CloudRuntimeException("Failed to create qemu-img command
to restore RBD volume with backup", ex);
Review Comment:
The thrown error message still says "restore RBD volume" even though this
path now handles both RBD and Linstor. Update it to a storage-type-neutral
message (or include the pool type) so operators aren't misled during
troubleshooting.
```suggestion
throw new CloudRuntimeException(String.format("Failed to create
qemu-img command to restore %s volume with backup", volumePool.getPoolType()),
ex);
```
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -89,17 +89,42 @@ sanity_checks() {
### Operation methods ###
+get_ceph_uuid_from_path() {
+ local fullpath="$1"
+ # disk for rbd => rbd:<pool>/<uuid>:mon_host=<monitor_host>...
+ # sample:
rbd:cloudstack/53d5c355-d726-4d3e-9422-046a503a0b12:mon_host=10.0.1.2...
+ beforeUuid="${fullpath#*/}" # Remove up to first slash after rbd:
+ volUuid="${beforeUuid%%:*}" # Remove everything after colon to get the uuid
+ echo $volUuid
+}
+
+get_linstor_uuid_from_path() {
+ local fullpath="$1"
+ # disk for linstor => /dev/drbd/by-res/cs-<uuid>/0
+ # sample: /dev/drbd/by-res/cs-53d5c355-d726-4d3e-9422-046a503a0b12/0
+ beforeUuid="${fullpath#/dev/drbd/by-res/}"
+ volUuid="${beforeUuid%%/*}"
+ volUuid="${volUuid#cs-}"
+ echo $volUuid
Review Comment:
Same scoping/quoting issue as above: `beforeUuid`/`volUuid` should be
`local`, and the `echo` should be quoted to avoid word-splitting/globbing and
accidental variable reuse outside the function.
```suggestion
local beforeUuid
local volUuid
# disk for rbd => rbd:<pool>/<uuid>:mon_host=<monitor_host>...
# sample:
rbd:cloudstack/53d5c355-d726-4d3e-9422-046a503a0b12:mon_host=10.0.1.2...
beforeUuid="${fullpath#*/}" # Remove up to first slash after rbd:
volUuid="${beforeUuid%%:*}" # Remove everything after colon to get the uuid
echo "$volUuid"
}
get_linstor_uuid_from_path() {
local fullpath="$1"
local beforeUuid
local volUuid
# disk for linstor => /dev/drbd/by-res/cs-<uuid>/0
# sample: /dev/drbd/by-res/cs-53d5c355-d726-4d3e-9422-046a503a0b12/0
beforeUuid="${fullpath#/dev/drbd/by-res/}"
volUuid="${beforeUuid%%/*}"
volUuid="${volUuid#cs-}"
echo "$volUuid"
```
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -146,6 +171,25 @@ backup_running_vm() {
sleep 5
done
+ # Use qemu-img convert to sparsify linstor backups which get bloated due to
virsh backup-begin.
+ name="root"
+ while read -r disk fullpath; do
+ if [[ "$fullpath" != /dev/drbd/by-res/* ]]; then
+ continue
+ fi
+ volUuid=$(get_linstor_uuid_from_path "$fullpath")
+ if ! qemu-img convert -O qcow2 "$dest/$name.$volUuid.qcow2"
"$dest/$name.$volUuid.qcow2.tmp" >"$logFile" 2> >(cat >&2); then
+ echo "qemu-img convert failed for $dest/$name.$volUuid.qcow2"
+ cleanup
+ exit 1
+ fi
Review Comment:
This `qemu-img convert` redirects stdout to
`/var/log/cloudstack/agent/agent.log` using `>`, which truncates the agent log
on each run (and makes troubleshooting much harder). Use append (`>>`) or
redirect to a dedicated backup log file instead of overwriting the agent log.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
