DaanHoogland commented on code in PR #7760:
URL: https://github.com/apache/cloudstack/pull/7760#discussion_r1274535393
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -500,22 +500,29 @@ public void allocate(final String vmInstanceName, final
VirtualMachineTemplate t
allocateRootVolume(persistedVm, template, rootDiskOfferingInfo,
owner, rootDiskSizeFinal);
- if (dataDiskOfferings != null) {
- for (final DiskOfferingInfo dataDiskOfferingInfo :
dataDiskOfferings) {
- volumeMgr.allocateRawVolume(Type.DATADISK, "DATA-" +
persistedVm.getId(), dataDiskOfferingInfo.getDiskOffering(),
dataDiskOfferingInfo.getSize(),
- dataDiskOfferingInfo.getMinIops(),
dataDiskOfferingInfo.getMaxIops(), persistedVm, template, owner, null);
+ // Create new Volume context and inject event resource type, id
and details to generate VOLUME.CREATE event for the ROOT disk.
Review Comment:
this comment indicates this could be an extracted method (and javadoc
instead)
##########
api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java:
##########
@@ -765,28 +768,33 @@ public ApiCommandResourceType getApiResourceType() {
public void execute() {
UserVm result;
- try {
- CallContext.current().setEventDetails("Vm Id: " + getEntityUuid());
- result = _userVmService.startVirtualMachine(this);
- } catch (ResourceUnavailableException ex) {
- s_logger.warn("Exception: ", ex);
- throw new
ServerApiException(ApiErrorCode.RESOURCE_UNAVAILABLE_ERROR, ex.getMessage());
- } catch (ResourceAllocationException ex) {
- s_logger.warn("Exception: ", ex);
- throw new
ServerApiException(ApiErrorCode.RESOURCE_ALLOCATION_ERROR, ex.getMessage());
- } catch (ConcurrentOperationException ex) {
- s_logger.warn("Exception: ", ex);
- throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR,
ex.getMessage());
- } catch (InsufficientCapacityException ex) {
- StringBuilder message = new StringBuilder(ex.getMessage());
- if (ex instanceof InsufficientServerCapacityException) {
- if
(((InsufficientServerCapacityException)ex).isAffinityApplied()) {
- message.append(", Please check the affinity groups
provided, there may not be sufficient capacity to follow them");
+ CallContext.current().setEventDetails("Vm Id: " + getEntityUuid());
+ if (getStartVm()) {
+ try {
+ result = _userVmService.startVirtualMachine(this);
+ } catch (ResourceUnavailableException ex) {
+ s_logger.warn("Exception: ", ex);
+ throw new
ServerApiException(ApiErrorCode.RESOURCE_UNAVAILABLE_ERROR, ex.getMessage());
+ } catch (ResourceAllocationException ex) {
+ s_logger.warn("Exception: ", ex);
+ throw new
ServerApiException(ApiErrorCode.RESOURCE_ALLOCATION_ERROR, ex.getMessage());
+ } catch (ConcurrentOperationException ex) {
+ s_logger.warn("Exception: ", ex);
+ throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR,
ex.getMessage());
+ } catch (InsufficientCapacityException ex) {
+ StringBuilder message = new StringBuilder(ex.getMessage());
+ if (ex instanceof InsufficientServerCapacityException) {
+ if
(((InsufficientServerCapacityException)ex).isAffinityApplied()) {
+ message.append(", Please check the affinity groups
provided, there may not be sufficient capacity to follow them");
+ }
}
+ s_logger.info(ex);
+ s_logger.info(message.toString(), ex);
Review Comment:
a bit out of scope for this PR but these two `info`'s seem a bit redundant:
```suggestion
s_logger.info(String.format("%s: %s", message.toString(),
ex.getLocalizedMessage()));
s_logger.debug(message.toString(), ex);
```
##########
server/src/main/java/com/cloud/api/query/QueryManagerImpl.java:
##########
@@ -721,6 +721,10 @@ private Pair<List<EventJoinVO>, Integer>
searchForEventsInternal(ListEventsCmd c
ListProjectResourcesCriteria listProjectResourcesCriteria =
domainIdRecursiveListProject.third();
Filter searchFilter = new Filter(EventJoinVO.class, "createDate",
false, cmd.getStartIndex(), cmd.getPageSizeVal());
+ // additional order by since createdDate does not have milliseconds
+ // and two events, created within one second can be incorrectly
ordered (for example VM.CREATE Completed before Scheduled)
Review Comment:
yes, a known issue in usage. good fix. I would like this added to javadoc
somewhere. (maybe a separate issue, not in this PR).
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -8018,15 +8031,29 @@ private void detachVolumesFromVm(List<VolumeVO>
volumes) {
}
}
- private void deleteVolumesFromVm(List<VolumeVO> volumes, boolean expunge) {
+ private void deleteVolumesFromVm(UserVmVO vm, List<VolumeVO> volumes,
boolean expunge) {
for (VolumeVO volume : volumes) {
+ destroyVolumeInContext(vm, expunge, volume);
+ }
+ }
+ private void destroyVolumeInContext(UserVmVO vm, boolean expunge, VolumeVO
volume) {
+ // Create new context and inject correct event resource type, id and
details,
+ // otherwise VOLUME.DESTROY event will be associated with
VirtualMachine and contain VM id and other information.
+ CallContext volumeContext =
CallContext.register(CallContext.current(), ApiCommandResourceType.Volume);
+ volumeContext.setEventDetails("Volume Type: " + volume.getVolumeType()
+ " Volume Id: " + this._uuidMgr.getUuid(Volume.class, volume.getId()) + " Vm
Id: " + vm.getUuid());
+ volumeContext.setEventResourceType(ApiCommandResourceType.Volume);
+ volumeContext.setEventResourceId(volume.getId());
+ try {
Volume result = _volumeService.destroyVolume(volume.getId(),
CallContext.current().getCallingAccount(), expunge, false);
if (result == null) {
- s_logger.error("DestroyVM remove volume - failed to delete
volume " + volume.getInstanceId() + " from instance " + volume.getId());
+ s_logger.error("DestroyVM remove volume - failed to delete
volume " + volume.getId() + " from instance " + volume.getInstanceId());
Review Comment:
```suggestion
s_logger.error(String.format("DestroyVM remove volume -
failed to delete volume %s from instance %s", volume.getId(),
volume.getInstanceId()));
```
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -1277,6 +1277,21 @@ public void
doInTransactionWithoutResult(TransactionStatus status) {
}
}
+ private void destroyVolumeInContext(Volume volume) {
+ // Create new context and inject correct event resource type, id and
details,
+ // otherwise VOLUME.DESTROY event will be associated with
VirtualMachine and contain VM id and other information.
Review Comment:
could these be javadoc?
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -8018,15 +8031,29 @@ private void detachVolumesFromVm(List<VolumeVO>
volumes) {
}
}
- private void deleteVolumesFromVm(List<VolumeVO> volumes, boolean expunge) {
+ private void deleteVolumesFromVm(UserVmVO vm, List<VolumeVO> volumes,
boolean expunge) {
for (VolumeVO volume : volumes) {
+ destroyVolumeInContext(vm, expunge, volume);
+ }
+ }
+ private void destroyVolumeInContext(UserVmVO vm, boolean expunge, VolumeVO
volume) {
+ // Create new context and inject correct event resource type, id and
details,
+ // otherwise VOLUME.DESTROY event will be associated with
VirtualMachine and contain VM id and other information.
Review Comment:
this comment can be javadoc
--
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]
