Very minor refactor

Author: João Jandre

api/src/main/java/org/apache/cloudstack/api/command/user/backup/DownloadValidationScreenshotCmd.java

@@ -19,6 +19,7 @@
 import com.cloud.event.EventTypes;
 import com.cloud.exception.ConcurrentOperationException;
 import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.InvalidParameterValueException;
 import com.cloud.exception.NetworkRuleConflictException;
 import com.cloud.exception.ResourceAllocationException;
 import com.cloud.exception.ResourceUnavailableException;
@@ -72,12 +73,15 @@ public String getEventType() {
 
     @Override
     public String getEventDescription() {
-        return "Downloading validation screenshot of backup " + getBackupId();
+        Backup backup = _entityMgr.findById(Backup.class, getBackupId());
+        if (backup == null) {
+            throw new InvalidParameterValueException(String.format("Unable to find backup with ID [%s]", getBackupId()));
+        }
+        return "Downloading validation screenshot of backup " + backup.getUuid();
     }
 
     @Override
-    public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException, ResourceAllocationException,
-            NetworkRuleConflictException {
+    public void execute() {
         ExtractResponse response = internalBackupService.downloadScreenshot(getBackupId());
         response.setResponseName(getCommandName());
         response.setObjectName(getCommandName());

api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkCmd.java

@@ -430,6 +430,7 @@ public CreateNetworkCmd(long networkOfferingId, String name, String displayText,
         this.subdomainAccess = subdomainAccess;
         this.displayNetwork = displayNetwork;
     }
+
     @Override
     public String getCommandName() {
         return s_name;

plugins/backup/kboss/src/main/java/org/apache/cloudstack/backup/KbossBackupProvider.java

@@ -1236,7 +1236,7 @@ protected void endBackupChainIfConfigured(BackupVO backupVO) {
 
         // Get updated record
         InternalBackupJoinVO backupJoinVO = internalBackupJoinDao.findById(backupVO.getId());
-        if (backupJoinVO.getCurrent() || (!backupChildren.isEmpty() && backupChildren.get(backupChildren.size() -1).getCurrent())) {
+        if (backupJoinVO.getCurrent() || (!backupChildren.isEmpty() && backupChildren.get(backupChildren.size() - 1).getCurrent())) {
             logger.info("As [{}] is true, we are ending the backup chain for VM [{}]. The next backup will be a full backup.",
                     BackupValidationServiceController.backupValidationEndChainOnFail.toString());
             endBackupChain(userVmDao.findById(backupVO.getVmId()));
@@ -2206,32 +2206,32 @@ protected boolean processValidationAnswer(Answer answer, BackupVO backupVO, User
             setBackupAsInvalidAndSendAlert(backupVO, msg);
             return false;
         }
-        if (answer instanceof ValidateKbossVmAnswer) {
-            ValidateKbossVmAnswer validateKbossVmAnswer = (ValidateKbossVmAnswer)answer;
-            boolean result = true;
-            String msg = String.format("Backup [%s] was validated using dummy VM [%s]. The backup was deemed invalid due to: ", backupVO.getUuid(), validationVm.getName());
-            if (validateKbossVmCommand.isWaitForBoot() && !validateKbossVmAnswer.isBootValidated()) {
-                result = false;
-                msg += "\n - The VM did not boot within the expected time.";
-            }
-            if (validateKbossVmCommand.isExecuteScript() && validateKbossVmAnswer.getScriptResult() != null) {
-                result = false;
-                msg += "\n - The script did not output the expected output. Captured output: " + validateKbossVmAnswer.getScriptResult();
-            }
-            if (validateKbossVmCommand.isTakeScreenshot() && validateKbossVmAnswer.getScreenshotPath() == null) {
-                result = false;
-                msg += "\n - We were unable to take a screenshot of the VM.";
-            } else if (validateKbossVmCommand.isTakeScreenshot()) {
-                logger.debug("Saving validation screenshot path [{}] to the backup details of backup [{}].", validateKbossVmAnswer.getScreenshotPath(), backupVO.getUuid());
-                backupDetailDao.addDetail(backupVO.getId(), SCREENSHOT_PATH, validateKbossVmAnswer.getScreenshotPath(), false);
-            }
-            if (!result) {
-                setBackupAsInvalidAndSendAlert(backupVO, msg);
-            }
-
-            return result;
+        if (!(answer instanceof ValidateKbossVmAnswer)) {
+            return false;
         }
-        return false;
+        ValidateKbossVmAnswer validateKbossVmAnswer = (ValidateKbossVmAnswer)answer;
+        boolean result = true;
+        String msg = String.format("Backup [%s] was validated using dummy VM [%s]. The backup was deemed invalid due to: ", backupVO.getUuid(), validationVm.getName());
+        if (validateKbossVmCommand.isWaitForBoot() && !validateKbossVmAnswer.isBootValidated()) {
+            result = false;
+            msg += "\n - The VM did not boot within the expected time.";
+        }
+        if (validateKbossVmCommand.isExecuteScript() && validateKbossVmAnswer.getScriptResult() != null) {
+            result = false;
+            msg += "\n - The script did not output the expected output. Captured output: " + validateKbossVmAnswer.getScriptResult();
+        }
+        if (validateKbossVmCommand.isTakeScreenshot() && validateKbossVmAnswer.getScreenshotPath() == null) {
+            result = false;
+            msg += "\n - We were unable to take a screenshot of the VM.";
+        } else if (validateKbossVmCommand.isTakeScreenshot()) {
+            logger.debug("Saving validation screenshot path [{}] to the backup details of backup [{}].", validateKbossVmAnswer.getScreenshotPath(), backupVO.getUuid());
+            backupDetailDao.addDetail(backupVO.getId(), SCREENSHOT_PATH, validateKbossVmAnswer.getScreenshotPath(), false);
+        }
+        if (!result) {
+            setBackupAsInvalidAndSendAlert(backupVO, msg);
+        }
+
+        return result;
     }
 
     protected void handleBackupExceptionInRestore(VirtualMachine vm, BackupException jobResult) {
@@ -2566,15 +2566,17 @@ protected void configureValidationSteps(ValidateKbossVmCommand cmd, BackupVO bac
                             BackupValidationServiceController.backupValidationBootDefaultTimeout.valueIn(backup.getAccountId()));
                     break;
                 case execute_command:
-                    configureValidationScript(cmd, backup.getVmId(), backup.getAccountId());
+                    configureValidationScript(cmd, backup);
                     break;
             }
         }
     }
 
-    protected void configureValidationScript(ValidateKbossVmCommand cmd, long vmId, long accountId) {
+    protected void configureValidationScript(ValidateKbossVmCommand cmd, BackupVO backupVO) {
+        long vmId = backupVO.getVmId();
         VMInstanceDetailVO script = vmInstanceDetailsDao.findDetail(vmId, VmDetailConstants.VALIDATION_COMMAND);
         if (script == null) {
+            logger.warn("Execute command step was configured but no script given. Ignoring this step for backup [{}].", backupVO.getUuid());
             return;
         }
         cmd.setExecuteScript(true);
@@ -2585,7 +2587,7 @@ protected void configureValidationScript(ValidateKbossVmCommand cmd, long vmId,
         cmd.setExpectedResult(scriptExpectedResult != null ? scriptExpectedResult.getValue() : "0");
         VMInstanceDetailVO scriptTimeout = vmInstanceDetailsDao.findDetail(vmId, VmDetailConstants.VALIDATION_COMMAND_TIMEOUT);
         cmd.setScriptTimeout(scriptTimeout != null ? Integer.valueOf(scriptTimeout.getValue()) :
-                BackupValidationServiceController.backupValidationScriptDefaultTimeout.valueIn(accountId));
+                BackupValidationServiceController.backupValidationScriptDefaultTimeout.valueIn(backupVO.getId()));
     }
 
     protected void createBasicBackupDetails(Long imageStoreId, Long parentId, BackupVO backupVO) {

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

@@ -33,6 +33,11 @@ public Answer execute(CleanupKbossValidationCommand command, LibvirtComputingRes
         return new Answer(command);
     }
 
+    /**
+     * The objective of this command is to remove the secondary storage references after the validation VM was stopped.
+     * Since the `getStoragePoolByURI` and `deleteStoragePool` have a reference counter, where the first method increases the count and the second one decreases the count,
+     * we must call the deleteStoragePool twice so that the command is count negative.
+     * */
     private void cleanupSecondaryStorages(CleanupKbossValidationCommand command, KVMStoragePoolManager storagePoolMgr) {
         logger.info("Cleaning up secondary storage references after backup validation process using VM [{}].", command.getVmName());
         for (String secondaryUrl : command.getSecondaryStorages()) {

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

@@ -155,6 +155,7 @@ private String runScript(ValidateKbossVmCommand command, Domain vm) throws Libvi
         }
         String script = command.getScriptToExecute();
         if (script == null) {
+            logger.warn("This command is malformed, we should execute an script for VM [{}], but no script was configured. Please review the original VM configurations.", vm.getName());
             return null;
         }
         String arguments = command.getScriptArguments();

server/src/main/java/com/cloud/vm/UserVmManager.java

@@ -113,7 +113,7 @@ public interface UserVmManager extends UserVmService {
 
     ConfigKey<Boolean> EnforceResourceLimitOnValidationVm = new ConfigKey<Boolean>(
             "Advanced", Boolean.class, "enforce.resource.limit.on.backup.validation.vm", "false", "If set to true, validation VMs will be accounted in the resource limit of the " +
-            "account/domain.",true, ConfigKey.Scope.Account);
+            "account/domain.", true, ConfigKey.Scope.Account);
 
     static final int MAX_USER_DATA_LENGTH_BYTES = 2048;
 

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

@@ -10185,7 +10185,6 @@ public UserVm allocateVMForValidation(long backupId, HypervisorType hypervisor)
         return vm;
     }
 
-
     private Network getValidationNetwork(long zoneId) {
         NetworkVO networkVo = _networkDao.findByZoneIdAndAccountIdAndGuestType(zoneId, Account.ACCOUNT_ID_SYSTEM, GuestType.Shared);
         AccountVO accountVO = _accountDao.findById(Account.ACCOUNT_ID_SYSTEM);

server/src/main/java/org/apache/cloudstack/backup/BackupValidationServiceController.java

@@ -141,14 +141,11 @@ protected void searchAndDispatchJobs() {
         }
     }
 
-
-
     @Override
     protected void submitQueuedJob(InternalBackupServiceJobVO job, long zoneId, String logId) {
         executor.submit(() -> startBackupValidation(job, zoneId, logId));
     }
 
-
     @Override
     protected List<InternalBackupServiceJobVO> getLostJobs(ClusterVO clusterVO, Calendar date, List<HostVO> hostVOS) {
         date.add(Calendar.SECOND, (int)Math.round(InternalBackupProvider.backupValidationTimeout.valueIn(clusterVO.getId()) * -RESCHEDULE_TO_TIMEOUT_RATIO));

services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java

@@ -2185,18 +2185,19 @@ protected Answer deleteBackup(DeleteCommand cmd) {
     }
 
     protected BackupDeleteAnswer deleteScreenshot(DeleteCommand cmd, String screenshotRelativePath, String parent) {
-        if (screenshotRelativePath != null) {
-            if (screenshotRelativePath.startsWith(File.separator)) {
-                screenshotRelativePath = screenshotRelativePath.substring(1);
-            }
-            String fullScreenshotPath = parent + screenshotRelativePath;
-            logger.debug("Deleting screenshot at [{}].", fullScreenshotPath);
-            String screenshotDeleteResult = deleteLocalFile(fullScreenshotPath);
-            if (screenshotDeleteResult != null) {
-                String details = String.format("Failed to delete backup validation screenshot [%s] with result [%s]. ", fullScreenshotPath, screenshotDeleteResult);
-                logger.warn(details);
-                return new BackupDeleteAnswer(cmd, false, details);
-            }
+        if (screenshotRelativePath == null) {
+            return null;
+        }
+        if (screenshotRelativePath.startsWith(File.separator)) {
+            screenshotRelativePath = screenshotRelativePath.substring(1);
+        }
+        String fullScreenshotPath = parent + screenshotRelativePath;
+        logger.debug("Deleting screenshot at [{}].", fullScreenshotPath);
+        String screenshotDeleteResult = deleteLocalFile(fullScreenshotPath);
+        if (screenshotDeleteResult != null) {
+            String details = String.format("Failed to delete backup validation screenshot [%s] with result [%s]. ", fullScreenshotPath, screenshotDeleteResult);
+            logger.warn(details);
+            return new BackupDeleteAnswer(cmd, false, details);
         }
         return null;
     }