Copilot commented on code in PR #12885:
URL: https://github.com/apache/cloudstack/pull/12885#discussion_r2985654179
##########
server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java:
##########
@@ -754,11 +754,17 @@ public UserVm revertToSnapshot(Long vmSnapshotId) throws
InsufficientCapacityExc
"Instance Snapshot reverting failed because the Instance
is not in Running or Stopped state.");
}
- if (userVm.getState() == VirtualMachine.State.Running &&
vmSnapshotVo.getType() == VMSnapshot.Type.Disk || userVm.getState() ==
VirtualMachine.State.Stopped
- && vmSnapshotVo.getType() == VMSnapshot.Type.DiskAndMemory) {
+ if (userVm.getState() == VirtualMachine.State.Running &&
vmSnapshotVo.getType() == VMSnapshot.Type.Disk) {
throw new InvalidParameterValueException(
- "Reverting to the Instance Snapshot is not allowed for
running Instances as this would result in a Instance state change. For running
Instances only Snapshots with memory can be reverted. In order to revert to a
Snapshot without memory you need to first stop the Instance."
- + " Snapshot");
+ "Reverting to the Instance Snapshot is not allowed for
running Instances as this would result in a Instance state change." +
+ "For running Instances only Snapshots with memory
can be reverted." +
+ "In order to revert to a Snapshot without memory
you need to first stop the Instance.");
Review Comment:
The concatenated error message is missing spaces between sentences (e.g.,
"change.For running…"), and it also uses incorrect grammar ("a Instance" -> "an
Instance"). This reduces readability for API users; please add proper
spacing/punctuation (or use a formatted string) so the message renders as
intended.
```suggestion
"Reverting to the Instance Snapshot is not allowed for
running Instances as this would result in an Instance state change." +
" For running Instances only Snapshots with
memory can be reverted." +
" In order to revert to a Snapshot without
memory you need to first stop the Instance.");
```
##########
server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java:
##########
@@ -811,20 +817,36 @@ else if (jobResult instanceof Throwable)
}
/**
- * If snapshot was taken with a different service offering than actual
used in vm, should change it back to it.
- * We also call <code>changeUserVmServiceOffering</code> in case the
service offering is dynamic in order to
- * perform resource limit validation, as the amount of CPUs or memory may
have been changed.
- * @param vmSnapshotVo vm snapshot
+ * Check if service offering change is needed for user vm when reverting
to vm snapshot.
+ * Service offering change is needed when snapshot was taken with a
different service offering than actual used in vm.
+ * Service offering change is also needed when service offering is dynamic
and the amount of cpu, memory or cpu speed
+ * has been changed since snapshot was taken.
+ * @param userVm
+ * @param vmSnapshotVo
+ * @return true if service offering change is needed; false otherwise
*/
- protected void updateUserVmServiceOffering(UserVm userVm, VMSnapshotVO
vmSnapshotVo) {
+ private boolean userVmServiceOfferingNeedsChange(UserVm userVm,
VMSnapshotVO vmSnapshotVo) {
if (vmSnapshotVo.getServiceOfferingId() !=
userVm.getServiceOfferingId()) {
- changeUserVmServiceOffering(userVm, vmSnapshotVo);
- return;
+ return true;
}
- ServiceOfferingVO serviceOffering =
_serviceOfferingDao.findById(userVm.getServiceOfferingId());
- if (serviceOffering.isDynamic()) {
- changeUserVmServiceOffering(userVm, vmSnapshotVo);
+
+ ServiceOfferingVO currentServiceOffering =
_serviceOfferingDao.findByIdIncludingRemoved(userVm.getId(),
userVm.getServiceOfferingId());
+ if (currentServiceOffering.isDynamic()) {
+ Map<String, String> vmDetails = getVmMapDetails(vmSnapshotVo);
+ ServiceOfferingVO newServiceOffering =
_serviceOfferingDao.getComputeOffering(currentServiceOffering, vmDetails);
+
+ int newCpu = newServiceOffering.getCpu();
+ int newMemory = newServiceOffering.getRamSize();
+ int newSpeed = newServiceOffering.getSpeed();
+ int currentCpu = currentServiceOffering.getCpu();
+ int currentMemory = currentServiceOffering.getRamSize();
+ int currentSpeed = currentServiceOffering.getSpeed();
+
+ if (newCpu != currentCpu || newMemory != currentMemory || newSpeed
!= currentSpeed) {
+ return true;
Review Comment:
`ServiceOfferingVO#getCpu()`, `getRamSize()`, and `getSpeed()` return boxed
`Integer`s. In a dynamic offering, `getComputeOffering(...)` can legitimately
leave some of these values null when the corresponding custom parameter is
absent, and unboxing them into `int` here will throw a NullPointerException
during revert. Please avoid primitive unboxing and compare using
`Objects.equals(...)` (or explicit null-safe handling) for cpu/ram/speed.
##########
server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java:
##########
@@ -754,11 +754,17 @@ public UserVm revertToSnapshot(Long vmSnapshotId) throws
InsufficientCapacityExc
"Instance Snapshot reverting failed because the Instance
is not in Running or Stopped state.");
}
- if (userVm.getState() == VirtualMachine.State.Running &&
vmSnapshotVo.getType() == VMSnapshot.Type.Disk || userVm.getState() ==
VirtualMachine.State.Stopped
- && vmSnapshotVo.getType() == VMSnapshot.Type.DiskAndMemory) {
+ if (userVm.getState() == VirtualMachine.State.Running &&
vmSnapshotVo.getType() == VMSnapshot.Type.Disk) {
throw new InvalidParameterValueException(
- "Reverting to the Instance Snapshot is not allowed for
running Instances as this would result in a Instance state change. For running
Instances only Snapshots with memory can be reverted. In order to revert to a
Snapshot without memory you need to first stop the Instance."
- + " Snapshot");
+ "Reverting to the Instance Snapshot is not allowed for
running Instances as this would result in a Instance state change." +
+ "For running Instances only Snapshots with memory
can be reverted." +
+ "In order to revert to a Snapshot without memory
you need to first stop the Instance.");
+ }
+
+ if ( userVm.getState() == VirtualMachine.State.Stopped &&
vmSnapshotVo.getType() == VMSnapshot.Type.DiskAndMemory) {
+ throw new InvalidParameterValueException(
+ "Reverting to the Instance Snapshot is not allowed for
stopped Instances if the Snapshot contains memory." +
Review Comment:
The stopped-instance revert error message is also missing a space between
sentences ("memory.In order…"). Please ensure there is whitespace/punctuation
between concatenated literals so the message is human-readable.
```suggestion
"Reverting to the Instance Snapshot is not allowed for
running Instances as this would result in a Instance state change. " +
"For running Instances only Snapshots with
memory can be reverted. " +
"In order to revert to a Snapshot without memory
you need to first stop the Instance.");
}
if ( userVm.getState() == VirtualMachine.State.Stopped &&
vmSnapshotVo.getType() == VMSnapshot.Type.DiskAndMemory) {
throw new InvalidParameterValueException(
"Reverting to the Instance Snapshot is not allowed for
stopped Instances if the Snapshot contains memory. " +
```
##########
server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java:
##########
@@ -811,20 +817,36 @@ else if (jobResult instanceof Throwable)
}
/**
- * If snapshot was taken with a different service offering than actual
used in vm, should change it back to it.
- * We also call <code>changeUserVmServiceOffering</code> in case the
service offering is dynamic in order to
- * perform resource limit validation, as the amount of CPUs or memory may
have been changed.
- * @param vmSnapshotVo vm snapshot
+ * Check if service offering change is needed for user vm when reverting
to vm snapshot.
+ * Service offering change is needed when snapshot was taken with a
different service offering than actual used in vm.
+ * Service offering change is also needed when service offering is dynamic
and the amount of cpu, memory or cpu speed
+ * has been changed since snapshot was taken.
+ * @param userVm
+ * @param vmSnapshotVo
+ * @return true if service offering change is needed; false otherwise
*/
Review Comment:
This change removes/renames the previously `protected`
`updateUserVmServiceOffering(...)` method and makes the new helper `private`.
There is an existing unit test
(`server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java`) that
directly calls `updateUserVmServiceOffering`, so the test suite will no longer
compile. Either update the tests to the new behavior (preferred), or keep a
`protected` wrapper method for backward compatibility in tests/possible
subclasses.
```suggestion
*/
protected boolean updateUserVmServiceOffering(UserVm userVm,
VMSnapshotVO vmSnapshotVo) {
return userVmServiceOfferingNeedsChange(userVm, vmSnapshotVo);
}
```
--
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]
