Skip to content

Commit ba1b15a

Browse files
JoaoJandreJoaoJandre
committed
address reviesws
1 parent 9ff4040 commit ba1b15a

9 files changed

Lines changed: 59 additions & 53 deletions

File tree

‎api/src/main/java/com/cloud/vm/VmDetailConstants.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,4 +145,5 @@ public interface VmDetailConstants {
145145
String VALIDATION_COMMAND_TIMEOUT = "backupValidationCommandTimeout";
146146
String VALIDATION_SCREENSHOT_WAIT = "backupValidationScreenshotWait";
147147
String VALIDATION_BOOT_TIMEOUT = "backupValidationBootTimeout";
148+
String LAST_KNOWN_STATE = "last_known_state";
148149
}

‎api/src/main/java/org/apache/cloudstack/api/ApiConstants.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1444,7 +1444,6 @@ public class ApiConstants {
14441444

14451445
public static final String SCHEDULED = "scheduled";
14461446
public static final String SCHEDULED_DATE = "scheduleddate";
1447-
public static final String LAST_KNOWN_STATE = "last_known_state";
14481447

14491448
/**
14501449
* This enum specifies IO Drivers, each option controls specific policies on I/O.

‎api/src/main/java/org/apache/cloudstack/backup/BackupManager.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer
141141
BackupOffering importBackupOffering(final ImportBackupOfferingCmd cmd);
142142

143143
/**
144-
* Add a new Backup and Recovery policy to CloudStack. Currently only supported for KBOSS.
144+
* Create a new Backup and Recovery policy to CloudStack. Currently only supported for KBOSS.
145145
* @param cmd import backup offering cmd
146146
*/
147147
BackupOffering createBackupOffering(final CreateBackupOfferingCmd cmd);

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,7 @@ public class KbossBackupProvider extends AdapterBase implements InternalBackupPr
284284
private final List<VirtualMachine.State> allowedVmStates = Arrays.asList(VirtualMachine.State.Running, VirtualMachine.State.Stopped);
285285
@Override
286286
public String getDescription() {
287-
return "Native Incremental KVM Backup Plugin";
287+
return "KVM Backup on Secondary Storage";
288288
}
289289

290290
@Override
@@ -301,7 +301,7 @@ public boolean isValidProviderOffering(Long zoneId, String uuid) {
301301
public boolean assignVMToBackupOffering(VirtualMachine vm, BackupOffering backupOffering) {
302302
logger.debug("Assigning VM [{}] to KBOSS backup offering with name:[{}], uuid: [{}].", vm.getUuid(), backupOffering.getName(), backupOffering.getUuid());
303303
if (!Hypervisor.HypervisorType.KVM.equals(vm.getHypervisorType())) {
304-
logger.error("KVM Native Incremental Backup provider is only supported for KVM.");
304+
logger.error("KVM Backup on Secondary Storage provider is only supported for KVM.");
305305
return false;
306306
}
307307

@@ -328,7 +328,7 @@ public boolean removeVMFromBackupOffering(VirtualMachine vm) {
328328
}
329329
UserVmVO vmVO = userVmDao.findById(vm.getId());
330330
logger.error("Failed to merge deltas for VM [{}] during backup offering removal process. Changing its state to [{}].", vm, VirtualMachine.State.BackupError);
331-
vmInstanceDetailsDao.addDetail(vm.getId(), ApiConstants.LAST_KNOWN_STATE, vmVO.getState().name(), false);
331+
vmInstanceDetailsDao.addDetail(vm.getId(), VmDetailConstants.LAST_KNOWN_STATE, vmVO.getState().name(), false);
332332
vmVO.setState(VirtualMachine.State.BackupError);
333333
userVmDao.update(vmVO.getId(), vmVO);
334334

@@ -1249,7 +1249,7 @@ protected Boolean getValidationEndChainOnFail(BackupVO backupVO) {
12491249
}
12501250

12511251
protected boolean normalizeBackupErrorAndFinishChain(UserVmVO userVmVO) {
1252-
VMInstanceDetailVO detail = vmInstanceDetailsDao.findDetail(userVmVO.getId(), ApiConstants.LAST_KNOWN_STATE);
1252+
VMInstanceDetailVO detail = vmInstanceDetailsDao.findDetail(userVmVO.getId(), VmDetailConstants.LAST_KNOWN_STATE);
12531253
boolean runningVM = detail == null || VirtualMachine.State.valueOf(detail.getValue()) == VirtualMachine.State.Running;
12541254

12551255
BackupVO backupVO = backupDao.findLatestByStatusAndVmId(Backup.Status.Error, userVmVO.getId());
@@ -1307,7 +1307,7 @@ protected boolean processCleanupBackupErrorAnswer(UserVmVO userVmVO, Answer answ
13071307
runningVM = cleanAnswer.isVmRunning();
13081308
userVmVO.setState(runningVM ? VirtualMachine.State.Running : VirtualMachine.State.Stopped);
13091309
userVmDao.update(userVmVO.getId(), userVmVO);
1310-
vmInstanceDetailsDao.removeDetail(userVmVO.getId(), ApiConstants.LAST_KNOWN_STATE);
1310+
vmInstanceDetailsDao.removeDetail(userVmVO.getId(), VmDetailConstants.LAST_KNOWN_STATE);
13111311
return chainAlreadyEnded;
13121312
}
13131313

@@ -2109,7 +2109,7 @@ protected void processBackupFailure(Answer answer, VirtualMachine vm, long hostI
21092109
} else {
21102110
logger.info("Backup [{}] of VM [{}] ended in error. We are not sure if the VM is consistent; thus, we will set it as BackupError.", backupVO.getUuid(), vm.getUuid());
21112111
transitVmStateWithoutThrow(vm, VirtualMachine.Event.OperationFailedToError, hostId);
2112-
vmInstanceDetailsDao.addDetail(vm.getId(), ApiConstants.LAST_KNOWN_STATE, runningVm ? VirtualMachine.State.Running.name() : VirtualMachine.State.Stopped.name(), false);
2112+
vmInstanceDetailsDao.addDetail(vm.getId(), VmDetailConstants.LAST_KNOWN_STATE, runningVm ? VirtualMachine.State.Running.name() : VirtualMachine.State.Stopped.name(), false);
21132113
backupVO.setStatus(Backup.Status.Error);
21142114
}
21152115

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7363,25 +7363,25 @@ protected Boolean checkBlockPullProgress(Domain vm, String diskLabel, String vmN
73637363
try {
73647364
Thread.sleep(1000);
73657365
} catch (InterruptedException ex) {
7366-
logger.trace(String.format("Thread that was tracking the block pull progress %s was interrupted. Ignoring.", partialLog), ex);
7366+
logger.trace("Thread that was tracking the block pull progress {} was interrupted. Ignoring.", partialLog, ex);
73677367
continue;
73687368
}
73697369

73707370
try {
73717371
result = vm.getBlockJobInfo(diskLabel, 0);
73727372
} catch (LibvirtException ex) {
7373-
logger.warn(String.format("Exception while getting block job info %s: [%s].", partialLog, ex.getMessage()), ex);
7373+
logger.warn("Exception while getting block job info {}: [{}].", partialLog, ex.getMessage(), ex);
73747374
return false;
73757375
}
73767376

73777377
if (result == null || result.type == 0 && result.end == 0 && result.cur == 0) {
7378-
logger.debug(String.format("Block pull job %s has finished.", partialLog));
7378+
logger.debug("Block pull job {} has finished.", partialLog);
73797379
return true;
73807380
}
73817381

73827382
long currentCommittedBytes = result.cur;
73837383
if (currentCommittedBytes > lastCommittedBytes) {
7384-
logger.debug(String.format("The block pull %s is at [%s] of [%s].", partialLog, currentCommittedBytes, result.end));
7384+
logger.debug("The block pull {} is at [{}] of [{}].", partialLog, currentCommittedBytes, result.end);
73857385
}
73867386
lastCommittedBytes = currentCommittedBytes;
73877387
endBytes = result.end;

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ private Pair<List<VolumeObjectTO>, Boolean> cleanupRunningVm(CleanupKbossBackupE
7171
dm = serverResource.getDomain(serverResource.getLibvirtUtilitiesHelper().getConnection(), command.getVmName());
7272
return new Pair<>(mergeDeltasForRunningVmIfNeeded(command, serverResource, dm), true);
7373
} catch (LibvirtException e) {
74-
if (e.getError().getCode() == Error.ErrorNumber.VIR_ERR_NO_DOMAIN && IsVmReallyStopped(command, serverResource)) {
74+
if (e.getError().getCode() == Error.ErrorNumber.VIR_ERR_NO_DOMAIN && isVmReallyStopped(command, serverResource)) {
7575
return new Pair<>(mergeDeltasForStoppedVmIfNeeded(command, serverResource), false);
7676
}
7777
logger.error("Error while trying to get VM [{}]. Aborting the process.", command.getVmName(), e);
@@ -185,7 +185,7 @@ private boolean mergeDeltaIfNeeded(LibvirtComputingResource serverResource, Kbos
185185
}
186186
}
187187

188-
private boolean IsVmReallyStopped(CleanupKbossBackupErrorCommand command, LibvirtComputingResource serverResource) {
188+
private boolean isVmReallyStopped(CleanupKbossBackupErrorCommand command, LibvirtComputingResource serverResource) {
189189
VolumeObjectTO volume = command.getKbossTOs().stream()
190190
.filter(kbossTO -> kbossTO.getVolumeObjectTO().getDeviceId() == 0)
191191
.map(KbossTO::getVolumeObjectTO).findFirst().orElseThrow();

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

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ public Answer execute(ValidateKbossVmCommand command, LibvirtComputingResource s
6262
secondaryStorage = storagePoolMgr.getStoragePoolByURI(command.getBackupDeltaTO().getDataStore().getUrl());
6363
logger.info("Validating VM [{}].", vm.getName());
6464
boolean bootValidated = waitForBoot(command, vm);
65-
String screenshotPath = takeScreenshot(command, vm, secondaryStorage, serverResource);
65+
String screenshotPath = takeScreenshot(command, vm, secondaryStorage);
6666
String scriptResult = runScript(command, vm);
6767
return new ValidateKbossVmAnswer(command, bootValidated, screenshotPath, scriptResult);
6868
} catch (LibvirtException e) {
@@ -107,7 +107,7 @@ private boolean waitForBoot(ValidateKbossVmCommand cmd, Domain vm) throws Libvir
107107
return false;
108108
}
109109

110-
private String takeScreenshot(ValidateKbossVmCommand command, Domain vm, KVMStoragePool secondaryStorage, LibvirtComputingResource resource) throws LibvirtException {
110+
private String takeScreenshot(ValidateKbossVmCommand command, Domain vm, KVMStoragePool secondaryStorage) throws LibvirtException {
111111
if (!command.isTakeScreenshot()) {
112112
return null;
113113
}
@@ -124,31 +124,35 @@ private String takeScreenshot(ValidateKbossVmCommand command, Domain vm, KVMStor
124124
throw new CloudRuntimeException(String.format("Got an unexpected error while trying to execute the screenshot validation step for VM [%s].", vmName));
125125
}
126126
try {
127-
File inputFile = new File(ssPath);
128-
String pngPath = ssPath + ".png";
129-
File outputFile = new File(pngPath);
130-
BufferedImage image = ImageIO.read(inputFile);
131-
132-
String warnMessage = String.format("Unable to convert screenshot at [%s] to PNG. Leaving it as is.", ssPath);
133-
if (image == null) {
134-
logger.warn(warnMessage);
135-
return ssPath.substring(ssPath.indexOf("backups"));
136-
}
137-
boolean result = ImageIO.write(image, "png", outputFile);
138-
139-
if (result) {
140-
logger.debug("Successfully converted image at [{}] to PNG at [{}].", ssPath, pngPath);
141-
Files.deleteIfExists(Path.of(ssPath));
142-
return pngPath.substring(ssPath.indexOf("backups"));
143-
} else {
144-
logger.warn(warnMessage);
145-
return ssPath.substring(ssPath.indexOf("backups"));
146-
}
127+
return tryToConvertFileToPng(ssPath);
147128
} catch (IOException ex) {
148129
throw new CloudRuntimeException(ex);
149130
}
150131
}
151132

133+
private String tryToConvertFileToPng(String ssPath) throws IOException {
134+
File inputFile = new File(ssPath);
135+
String pngPath = ssPath + ".png";
136+
File outputFile = new File(pngPath);
137+
BufferedImage image = ImageIO.read(inputFile);
138+
139+
String warnMessage = String.format("Unable to convert screenshot at [%s] to PNG. Leaving it as is.", ssPath);
140+
if (image == null) {
141+
logger.warn(warnMessage);
142+
return ssPath.substring(ssPath.indexOf("backups"));
143+
}
144+
boolean result = ImageIO.write(image, "png", outputFile);
145+
146+
if (result) {
147+
logger.debug("Successfully converted image at [{}] to PNG at [{}].", ssPath, pngPath);
148+
Files.deleteIfExists(Path.of(ssPath));
149+
return pngPath.substring(ssPath.indexOf("backups"));
150+
} else {
151+
logger.warn(warnMessage);
152+
return ssPath.substring(ssPath.indexOf("backups"));
153+
}
154+
}
155+
152156
private String runScript(ValidateKbossVmCommand command, Domain vm) throws LibvirtException {
153157
if (!command.isExecuteScript()) {
154158
return null;
@@ -177,6 +181,12 @@ private String runScript(ValidateKbossVmCommand command, Domain vm) throws Libvi
177181
JsonObject ret = root.getAsJsonObject("return");
178182
String pid = ret.get("pid").getAsString();
179183

184+
return waitForCommandResult(command, vm, pid, script, arguments);
185+
}
186+
187+
private String waitForCommandResult(ValidateKbossVmCommand command, Domain vm, String pid, String script, String arguments) throws LibvirtException {
188+
JsonObject root;
189+
JsonObject ret;
180190
String expectedResult = command.getExpectedResult();
181191
Integer timeout = command.getScriptTimeout();
182192
while (timeout > 0) {

‎server/src/main/java/org/apache/cloudstack/backup/InternalBackupServiceJobController.java‎

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -219,24 +219,22 @@ protected List<Pair<HostVO, Long>> filterHostsWithTooManyJobs(HashMap<HostVO, Lo
219219
for (HostVO host : hostToNumberOfExecutingJobs.keySet()) {
220220
Long numberOfJobs = hostToNumberOfExecutingJobs.get(host);
221221
hostDao.loadDetails(host);
222-
Integer maxConcurrentCompressionsPerHost = getMaxConcurrentCompressionsPerHost(jobsPerHostConfiguration, host);
223-
if (maxConcurrentCompressionsPerHost > 0 && numberOfJobs >= maxConcurrentCompressionsPerHost) {
224-
logger.debug("Host [{}] is already executing the maximum number of concurrent compression jobs set in [{}]. Current number of jobs being executed is " +
225-
"[{}], the value for the configuration is [{}].", host, jobsPerHostConfiguration.toString(), numberOfJobs, maxConcurrentCompressionsPerHost);
222+
Integer maxConcurrentJobsPerHost = getMaxConcurrentJobsPerHost(jobsPerHostConfiguration, host);
223+
if (maxConcurrentJobsPerHost > 0 && numberOfJobs >= maxConcurrentJobsPerHost) {
224+
logger.debug("Host [{}] is already executing the maximum number of concurrent {} jobs set in [{}]. Current number of jobs being executed is " +
225+
"[{}], the value for the configuration is [{}].", host, controllerType, jobsPerHostConfiguration.toString(), numberOfJobs, maxConcurrentJobsPerHost);
226226
continue;
227227
}
228228
hostAndNumberOfJobsPairList.add(new Pair<>(host, numberOfJobs));
229229
}
230230
return hostAndNumberOfJobsPairList;
231231
}
232232

233-
protected Integer getMaxConcurrentCompressionsPerHost(ConfigKey<Integer> jobsPerHostConfiguration, HostVO host) {
233+
protected Integer getMaxConcurrentJobsPerHost(ConfigKey<Integer> jobsPerHostConfiguration, HostVO host) {
234234
if (host.getDetail(jobsPerHostConfiguration.key()) != null) {
235235
return Integer.valueOf(host.getDetail(jobsPerHostConfiguration.key()));
236-
} else {
237-
return jobsPerHostConfiguration.valueIn(host.getClusterId());
238236
}
239-
237+
return jobsPerHostConfiguration.valueIn(host.getClusterId());
240238
}
241239

242240
/**
@@ -268,7 +266,7 @@ protected void submitQueuedJobsForExecution(List<InternalBackupServiceJobVO> job
268266

269267
submitQueuedJob(job, zoneId, logId);
270268

271-
Integer maxJobsPerHost = getMaxConcurrentCompressionsPerHost(maxJobPerHostConfig, hostAndNumberOfJobs.first());
269+
Integer maxJobsPerHost = getMaxConcurrentJobsPerHost(maxJobPerHostConfig, hostAndNumberOfJobs.first());
272270
if (hostAndNumberOfJobs.second() < maxJobsPerHost || maxJobsPerHost < 0) {
273271
hostAndNumberOfJobsPairList.add(hostAndNumberOfJobs);
274272
hostAndNumberOfJobsPairList.sort(Comparator.comparing(Pair::second));
@@ -282,8 +280,7 @@ protected Boolean isFrameworkEnabledForZone(DataCenterVO zone) {
282280
return BackupManager.BackupFrameworkEnabled.valueIn(zone.getId());
283281
}
284282

285-
protected void submitQueuedJob(InternalBackupServiceJobVO job, long zoneId, String logId) {
286-
}
283+
protected abstract void submitQueuedJob(InternalBackupServiceJobVO job, long zoneId, String logId);
287284

288285
/**
289286
* Implementing classes should override this if they want jobs to be tried more than once.
@@ -306,6 +303,5 @@ protected List<InternalBackupServiceJobVO> getLostJobs(ClusterVO clusterVO, Cale
306303
return List.of();
307304
}
308305

309-
protected void searchAndDispatchJobs() {
310-
}
306+
protected abstract void searchAndDispatchJobs();
311307
}

‎server/src/test/java/org/apache/cloudstack/backup/BackupCompressionServiceJobControllerTest.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -344,7 +344,7 @@ public void thinJobsToStartListTestNoCurrentExecutingJobsAndNewJobsLowerThanMax(
344344

345345
@Test
346346
public void filterHostsWithTooManyJobsTestUnlimitedCompressionPerHost() {
347-
doReturn(0).when(backupCompressionServiceJobControllerSpy).getMaxConcurrentCompressionsPerHost(maxConcurrentJobsConfigKey, hostVO);
347+
doReturn(0).when(backupCompressionServiceJobControllerSpy).getMaxConcurrentJobsPerHost(maxConcurrentJobsConfigKey, hostVO);
348348

349349
HashMap<HostVO, Long> hostToNumberOfExecutingJobs = new HashMap<>();
350350
hostToNumberOfExecutingJobs.put(hostVO, 10L);
@@ -356,7 +356,7 @@ public void filterHostsWithTooManyJobsTestUnlimitedCompressionPerHost() {
356356

357357
@Test
358358
public void filterHostsWithTooManyJobsTestLimitedCompressionPerHost() {
359-
doReturn(4).when(backupCompressionServiceJobControllerSpy).getMaxConcurrentCompressionsPerHost(any(), any());
359+
doReturn(4).when(backupCompressionServiceJobControllerSpy).getMaxConcurrentJobsPerHost(any(), any());
360360

361361

362362
HashMap<HostVO, Long> hostToNumberOfExecutingJobs = new HashMap<>();
@@ -403,7 +403,7 @@ public void submitQueuedJobsForExecutionTestSchedulesJobAndRequeuesHostWhenBelow
403403
doReturn(jobId).when(internalBackupServiceJobVoMock).getId();
404404
doReturn(backupId).when(internalBackupServiceJobVoMock).getBackupId();
405405
doReturn(hostId).when(hostVO).getId();
406-
doReturn(5).when(backupCompressionServiceJobControllerSpy).getMaxConcurrentCompressionsPerHost(maxConcurrentJobsConfigKey, hostVO);
406+
doReturn(5).when(backupCompressionServiceJobControllerSpy).getMaxConcurrentJobsPerHost(maxConcurrentJobsConfigKey, hostVO);
407407
doNothing().when(backupCompressionServiceJobControllerSpy).submitQueuedJob(any(), eq(datacenterId), any());
408408

409409
List<Pair<HostVO, Long>> hostAndNumberOfJobsPairList = new java.util.ArrayList<>();
@@ -429,7 +429,7 @@ public void submitQueuedJobsForExecutionTestDoesNotRequeueHostWhenLimitReached()
429429
doReturn(jobId).when(internalBackupServiceJobVoMock).getId();
430430
doReturn(backupId).when(internalBackupServiceJobVoMock).getBackupId();
431431
doReturn(hostId).when(hostVO).getId();
432-
doReturn(1).when(backupCompressionServiceJobControllerSpy).getMaxConcurrentCompressionsPerHost(maxConcurrentJobsConfigKey, hostVO);
432+
doReturn(1).when(backupCompressionServiceJobControllerSpy).getMaxConcurrentJobsPerHost(maxConcurrentJobsConfigKey, hostVO);
433433
doNothing().when(backupCompressionServiceJobControllerSpy).submitQueuedJob(any(), eq(datacenterId), any());
434434

435435
List<Pair<HostVO, Long>> hostAndNumberOfJobsPairList = new java.util.ArrayList<>();

0 commit comments

Comments
 (0)