Skip to content

Commit 41b616b

Browse files
abh1sarabh1sar
committed
Address review comments: null/empty checks and restore answer guard
1 parent 1e58104 commit 41b616b

2 files changed

Lines changed: 33 additions & 24 deletions

File tree

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

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@
6363
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
6464
import org.apache.cloudstack.storage.to.PrimaryDataStoreTO;
6565
import org.apache.commons.collections.CollectionUtils;
66+
import org.apache.commons.lang3.StringUtils;
6667
import org.apache.logging.log4j.Logger;
6768
import org.apache.logging.log4j.LogManager;
6869

@@ -393,7 +394,7 @@ private String readVmActiveCheckpoint(long vmId) {
393394
return null;
394395
}
395396
String v = d.getValue();
396-
return (v == null || v.isEmpty()) ? null : v;
397+
return StringUtils.isBlank(v) ? null : v;
397398
}
398399

399400
/**
@@ -403,7 +404,7 @@ private String readVmActiveCheckpoint(long vmId) {
403404
*/
404405
private Backup findLatestBackedUpBackup(long vmId) {
405406
List<Backup> history = backupDao.listByVmId(null, vmId);
406-
if (history == null || history.isEmpty()) {
407+
if (CollectionUtils.isEmpty(history)) {
407408
return null;
408409
}
409410
return history.stream()
@@ -435,17 +436,21 @@ private List<String> composeParentBackupPaths(Backup parent, long vmId) {
435436
// backupPath is stored as externalId by createBackupObject — e.g.
436437
// "i-2-1234-VM/2026.04.27.13.45.00".
437438
String dir = parent.getExternalId();
438-
if (dir == null || dir.isEmpty()) {
439+
if (StringUtils.isBlank(dir)) {
440+
LOG.debug("Parent backup {} has no backup path recorded; cannot compose parent backup paths", parent.getUuid());
439441
return null;
440442
}
441443

442444
List<Backup.VolumeInfo> parentVols = parent.getBackedUpVolumes();
443-
if (parentVols == null || parentVols.isEmpty()) {
445+
if (CollectionUtils.isEmpty(parentVols)) {
446+
LOG.debug("Parent backup {} has no backed up volumes recorded; cannot compose parent backup paths", parent.getUuid());
444447
return null;
445448
}
446449

447450
List<VolumeVO> currentVols = volumeDao.findByInstance(vmId);
448-
if (currentVols == null || currentVols.size() != parentVols.size()) {
451+
if (CollectionUtils.isEmpty(currentVols) || currentVols.size() != parentVols.size()) {
452+
LOG.debug("VM id={} currently has {} volume(s) but parent backup {} recorded {}; cannot compose parent backup paths",
453+
vmId, currentVols == null ? 0 : currentVols.size(), parent.getUuid(), parentVols.size());
449454
return null;
450455
}
451456

@@ -721,9 +726,12 @@ private Pair<Boolean, String> restoreVMBackup(VirtualMachine vm, Backup backup)
721726
} catch (OperationTimedoutException e) {
722727
throw new CloudRuntimeException("Operation to restore backup timed out, please try again");
723728
}
724-
// After a restore the QEMU dirty-bitmap chain is gone — clear active_checkpoint_id so
729+
if (answer == null) {
730+
throw new CloudRuntimeException(String.format("No answer received from the backend control plane while restoring VM %s from backup %s", vm.getInstanceName(), backup.getUuid()));
731+
}
732+
// After a restore the QEMU dirty-bitmap chain is gone, so clear active_checkpoint_id so
725733
// the next backup is taken as a fresh full and starts a new chain. See decideChain.
726-
if (answer != null && answer.getResult()) {
734+
if (answer.getResult()) {
727735
clearVmActiveCheckpoint(vm.getId());
728736
}
729737
return new Pair<>(answer.getResult(), answer.getDetails());

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

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919

2020
package com.cloud.hypervisor.kvm.resource.wrapper;
2121

22-
import com.amazonaws.util.CollectionUtils;
2322
import com.cloud.agent.api.Answer;
2423
import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
2524
import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk;
@@ -33,6 +32,8 @@
3332
import org.apache.cloudstack.backup.BackupAnswer;
3433
import org.apache.cloudstack.backup.TakeBackupCommand;
3534
import org.apache.cloudstack.storage.to.PrimaryDataStoreTO;
35+
import org.apache.commons.collections.CollectionUtils;
36+
import org.apache.commons.lang3.StringUtils;
3637

3738
import java.util.ArrayList;
3839
import java.util.Arrays;
@@ -92,7 +93,7 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
9293

9394
if (result.first() != 0) {
9495
logger.debug("Failed to take VM backup: " + result.second());
95-
BackupAnswer answer = new BackupAnswer(command, false, result.second().trim());
96+
BackupAnswer answer = new BackupAnswer(command, false, StringUtils.trimToEmpty(result.second()));
9697
if (EXIT_CLEANUP_FAILED.equals(result.first())) {
9798
logger.debug("Backup cleanup failed");
9899
answer.setNeedsCleanup(true);
@@ -104,7 +105,7 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
104105
// parent checkpoint can't be re-registered) and signals it with INCREMENTAL_FALLBACK
105106
// on stdout. Detect it, then strip the marker line before parsing the backup size.
106107
String rawStdout = result.second();
107-
boolean incrementalFallback = rawStdout.contains(INCREMENTAL_FALLBACK_MARKER);
108+
boolean incrementalFallback = StringUtils.contains(rawStdout, INCREMENTAL_FALLBACK_MARKER);
108109
String stdout = stripMarkerLines(rawStdout).trim();
109110
long backupSize = parseBackupSize(stdout, diskPaths);
110111

@@ -119,7 +120,7 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
119120

120121
/** Remove nasbackup.sh's stdout signalling marker lines so they don't pollute size parsing. */
121122
private String stripMarkerLines(String stdout) {
122-
if (stdout == null || stdout.isEmpty()) {
123+
if (StringUtils.isBlank(stdout)) {
123124
return "";
124125
}
125126
StringBuilder sb = new StringBuilder();
@@ -151,21 +152,21 @@ private Pair<Integer, String> runBackupScript(LibvirtComputingResource libvirtCo
151152
"-m", Objects.nonNull(mountOptions) ? mountOptions : "",
152153
"-p", backupPath,
153154
"-q", command.getQuiesce() != null && command.getQuiesce() ? "true" : "false",
154-
"-d", diskPaths.isEmpty() ? "" : String.join(",", diskPaths)
155+
"-d", CollectionUtils.isEmpty(diskPaths) ? "" : String.join(",", diskPaths)
155156
));
156-
if (mode != null && !mode.isEmpty()) {
157+
if (StringUtils.isNotBlank(mode)) {
157158
argv.add("-M");
158159
argv.add(mode);
159160
}
160-
if (bitmapNew != null && !bitmapNew.isEmpty()) {
161+
if (StringUtils.isNotBlank(bitmapNew)) {
161162
argv.add("--bitmap-new");
162163
argv.add(bitmapNew);
163164
}
164-
if (bitmapParent != null && !bitmapParent.isEmpty()) {
165+
if (StringUtils.isNotBlank(bitmapParent)) {
165166
argv.add("--bitmap-parent");
166167
argv.add(bitmapParent);
167168
}
168-
if (parentPaths != null && !parentPaths.isEmpty()) {
169+
if (CollectionUtils.isNotEmpty(parentPaths)) {
169170
argv.add("--parent-paths");
170171
argv.add(String.join(",", parentPaths));
171172
}
@@ -181,29 +182,29 @@ private Pair<Integer, String> runBackupScript(LibvirtComputingResource libvirtCo
181182
*/
182183
private String validateBackupArgs(TakeBackupCommand command) {
183184
String mode = command.getMode();
184-
if (mode == null || mode.isEmpty()) {
185-
return null; // legacy full-only — no extra args expected
185+
if (StringUtils.isBlank(mode)) {
186+
return null; // legacy full-only, no extra args expected
186187
}
187188
if (MODE_INCREMENTAL.equals(mode)) {
188-
if (command.getBitmapNew() == null || command.getBitmapNew().isEmpty()) {
189+
if (StringUtils.isBlank(command.getBitmapNew())) {
189190
return "incremental mode requires bitmapNew";
190191
}
191-
if (command.getBitmapParent() == null || command.getBitmapParent().isEmpty()) {
192+
if (StringUtils.isBlank(command.getBitmapParent())) {
192193
return "incremental mode requires bitmapParent";
193194
}
194-
if (command.getParentPaths() == null || command.getParentPaths().isEmpty()) {
195+
if (CollectionUtils.isEmpty(command.getParentPaths())) {
195196
return "incremental mode requires parentPaths";
196197
}
197198
return null;
198199
}
199200
if (MODE_FULL.equals(mode)) {
200-
if (command.getBitmapNew() == null || command.getBitmapNew().isEmpty()) {
201+
if (StringUtils.isBlank(command.getBitmapNew())) {
201202
return "full mode requires bitmapNew (the bitmap to create for the next incremental)";
202203
}
203204
return null;
204205
}
205206
if (MODE_LEGACY_FULL.equals(mode)) {
206-
return null; // feature-off full backup — no bitmap or chain args expected
207+
return null; // feature-off full backup, no bitmap or chain args expected
207208
}
208209
return "Unknown backup mode: " + mode;
209210
}
@@ -215,7 +216,7 @@ private String validateBackupArgs(TakeBackupCommand command) {
215216
*/
216217
private long parseBackupSize(String stdout, List<String> diskPaths) {
217218
long backupSize = 0L;
218-
if (CollectionUtils.isNullOrEmpty(diskPaths)) {
219+
if (CollectionUtils.isEmpty(diskPaths)) {
219220
List<String> outputLines = Arrays.asList(stdout.split("\n"));
220221
if (!outputLines.isEmpty()) {
221222
backupSize = Long.parseLong(outputLines.get(outputLines.size() - 1).trim());

0 commit comments

Comments
 (0)