weizhouapache commented on code in PR #10140:
URL: https://github.com/apache/cloudstack/pull/10140#discussion_r2036970174
##########
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java:
##########
@@ -834,10 +794,11 @@ private Backup.RestorePoint
getRestorePointFromBlock(String[] parts) {
type = split[1].trim();
}
}
- return new Backup.RestorePoint(id, created, type);
+ Backup.Metric metric = metricsMap.get(id);
Review Comment:
is it possible metric is null ?
##########
plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java:
##########
@@ -35,12 +37,14 @@
import com.cloud.vm.VirtualMachine;
public class DummyBackupProvider extends AdapterBase implements BackupProvider
{
-
+ private static final Logger LOG =
LogManager.getLogger(DummyBackupProvider.class);
Review Comment:
`logger` can be used in this class
##########
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/VeeamBackupProvider.java:
##########
@@ -213,7 +214,7 @@ public boolean removeVMFromBackupOffering(final
VirtualMachine vm) {
@Override
public boolean willDeleteBackupsOnOfferingRemoval() {
- return true;
+ return false;
Review Comment:
why this is changed ?
##########
plugins/backup/dummy/src/main/java/org/apache/cloudstack/backup/DummyBackupProvider.java:
##########
@@ -118,7 +110,7 @@ public boolean removeVMFromBackupOffering(VirtualMachine
vm) {
@Override
public boolean willDeleteBackupsOnOfferingRemoval() {
- return true;
+ return false;
Review Comment:
can you explain a bit ?
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -8987,6 +9024,215 @@ public boolean unmanageUserVM(Long vmId) {
return true;
}
+ @Override
+ public UserVm allocateVMFromBackup(CreateVMFromBackupCmd cmd) throws
InsufficientCapacityException, ResourceAllocationException,
ResourceUnavailableException {
Review Comment:
since the process of CreateVMFromBackupCmd and DeployVMCmd are different, I
would suggest
- CreateVMFromBackupCmd is not extended from DeployVMCmd, some unsupported
parameters can be removed (for example userdata)
- do not change DeployVMCmd so there will be less impact. Changing the
properties of existing parameter will lead to some other changes, for example
in cloudstack-go project
##########
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java:
##########
@@ -659,9 +656,7 @@ public boolean setJobSchedule(final String jobName) {
public boolean deleteJobAndBackup(final String jobName) {
Pair<Boolean, String> result = executePowerShellCommands(Arrays.asList(
String.format("$job = Get-VBRJob -Name '%s'", jobName),
- "if ($job) { Remove-VBRJob -Job $job -Confirm:$false }",
- String.format("$backup = Get-VBRBackup -Name '%s'", jobName),
- "if ($backup) { Remove-VBRBackup -Backup $backup -FromDisk
-Confirm:$false }"
+ "if ($job) { Remove-VBRJob -Job $job -Confirm:$false }"
Review Comment:
so, only job is deleted, backups are not ?
##########
plugins/backup/veeam/src/main/java/org/apache/cloudstack/backup/veeam/VeeamClient.java:
##########
@@ -855,55 +816,61 @@ public List<Backup.RestorePoint>
listRestorePointsLegacy(String backupName, Stri
}
logger.debug(String.format("Found restore points from [backupName:
%s, vmInternalName: %s] which is: [%s].", backupName, vmInternalName, block));
final String[] parts = block.split("\r\n");
- restorePoints.add(getRestorePointFromBlock(parts));
+ restorePoints.add(getRestorePointFromBlock(parts, metricsMap));
}
return restorePoints;
}
- public List<Backup.RestorePoint> listRestorePoints(String backupName,
String vmInternalName) {
+ public List<Backup.RestorePoint> listRestorePoints(String backupName,
String vmwareDcName, String vmInternalName, Map<String, Backup.Metric>
metricsMap) {
if (isLegacyServer()) {
- return listRestorePointsLegacy(backupName, vmInternalName);
+ return listRestorePointsLegacy(backupName, vmInternalName,
metricsMap);
} else {
- return listVmRestorePointsViaVeeamAPI(vmInternalName);
+ return listVmRestorePointsViaVeeamAPI(vmwareDcName,
vmInternalName, metricsMap);
}
}
- public List<Backup.RestorePoint> listVmRestorePointsViaVeeamAPI(String
vmInternalName) {
+ public List<Backup.RestorePoint> listVmRestorePointsViaVeeamAPI(String
vmwareDcName, String vmInternalName, Map<String, Backup.Metric> metricsMap) {
logger.debug(String.format("Trying to list VM restore points via Veeam
B&R API for VM %s: ", vmInternalName));
try {
final HttpResponse response =
get(String.format("/vmRestorePoints?format=Entity"));
checkResponseOK(response);
- return
processHttpResponseForVmRestorePoints(response.getEntity().getContent(),
vmInternalName);
+ return
processHttpResponseForVmRestorePoints(response.getEntity().getContent(),
vmwareDcName, vmInternalName, metricsMap);
} catch (final IOException e) {
logger.error("Failed to list VM restore points via Veeam B&R API
due to:", e);
checkResponseTimeOut(e);
}
return new ArrayList<>();
}
- public List<Backup.RestorePoint>
processHttpResponseForVmRestorePoints(InputStream content, String
vmInternalName) {
+ public List<Backup.RestorePoint>
processHttpResponseForVmRestorePoints(InputStream content, String vmwareDcName,
String vmInternalName, Map<String, Backup.Metric> metricsMap) {
List<Backup.RestorePoint> vmRestorePointList = new ArrayList<>();
try {
final ObjectMapper objectMapper = new XmlMapper();
final VmRestorePoints vmRestorePoints =
objectMapper.readValue(content, VmRestorePoints.class);
+ final String hierarchyId = findDCHierarchy(vmwareDcName);
+ final String hierarchyUuid =
hierarchyId.substring(hierarchyId.lastIndexOf(":") + 1);
if (vmRestorePoints == null) {
throw new CloudRuntimeException("Could not get VM restore
points via Veeam B&R API");
}
for (final VmRestorePoint vmRestorePoint :
vmRestorePoints.getVmRestorePoints()) {
logger.debug(String.format("Processing VM restore point
Name=%s, VmDisplayName=%s for vm name=%s",
vmRestorePoint.getName(),
vmRestorePoint.getVmDisplayName(), vmInternalName));
- if (!vmInternalName.equals(vmRestorePoint.getVmDisplayName()))
{
+ if (!vmInternalName.equals(vmRestorePoint.getVmDisplayName())
|| !vmRestorePoint.getHierarchyObjRef().contains(hierarchyUuid)) {
continue;
}
boolean isReady = true;
+ String backupFileId = null;
List<Link> links = vmRestorePoint.getLink();
for (Link link : links) {
if (Arrays.asList(BACKUP_FILE_REFERENCE,
RESTORE_POINT_REFERENCE).contains(link.getType()) &&
!link.getRel().equals("Up")) {
logger.info(String.format("The VM restore point is not
ready. Reference: %s, state: %s", link.getType(), link.getRel()));
isReady = false;
break;
}
+ if (link.getType() != null &&
link.getType().equals(BACKUP_FILE_REFERENCE)) {
+ backupFileId =
link.getHref().substring(link.getHref().lastIndexOf('/') + 1);
Review Comment:
there are several lines similar to (but different separator)
```
backupFileId = xxx.substring(xxx.lastIndexOf('/') + 1);
```
maybe create a method like
```
parseXXX(String value, String serapator)
```
--
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]
