Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12779 +/- ##
============================================
+ Coverage 17.60% 17.68% +0.08%
- Complexity 15659 15780 +121
============================================
Files 5917 5917
Lines 531394 535073 +3679
Branches 64970 66522 +1552
============================================
+ Hits 93575 94653 +1078
- Misses 427269 429785 +2516
- Partials 10550 10635 +85
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR cleans up static analysis warnings in UserVmManagerImpl and related files, including the service interface and a template interface. The changes address issues like raw types, unnecessary casts, unused variables, inefficient null checks, and missing throw keywords.
Changes:
- Removed unused imports, fields (
statsCollector,vmScheduleManager), unused constants (MAX_HTTP_GET_LENGTH, etc.), and dead code (unused constructors/methods inVmAndCountDetails,getName(),getRandomPrivateTemplateName()) - Simplified conditionals: replaced
== null ? false : valuewith directBoolean.parseBoolean, replaced!list.stream().anyMatch(...)withlist.stream().noneMatch(...), replacedsize() > 0with!isEmpty(), etc. - Fixed multiple bugs: missing
throwforInvalidParameterValueExceptionat validation, NPE in error messages using null objects' methods, removedcallerparameter from methods where it was unused, improved method signatures to remove unchecked exceptions
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java |
Major cleanup: remove unused imports/fields/methods, fix bugs (missing throw, null NPE in error messages), simplify conditionals/collections usage |
server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java |
Update test method calls to match new method signatures (removed caller parameters, replaced 1l with 1L) |
api/src/main/java/com/cloud/vm/UserVmService.java |
Remove StorageUnavailableException from method signatures, remove redundant public modifier from interface method |
api/src/main/java/com/cloud/template/VirtualMachineTemplate.java |
Add generic type parameter to getDetails() return type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| private void closeCheckeReservation(List<CheckedReservation> checkedReservations) { |
There was a problem hiding this comment.
The method closeCheckeReservation has a typo in its name — it should be closeCheckedReservation. This is an inconsistency that will persist in the codebase and may cause confusion when searching for this method later.
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17071 |
| } catch (CloudException e) { | ||
| throw new CloudRuntimeException("Unable to contact the agent to stop the Instance " + vm, e); | ||
| } | ||
| return status; | ||
| } | ||
|
|
||
| private UserVm rebootVirtualMachine(long userId, long vmId, boolean enterSetup, boolean forced) throws InsufficientCapacityException, ResourceUnavailableException { | ||
| private UserVm rebootVirtualMachine(long vmId, boolean enterSetup, boolean forced) throws InsufficientCapacityException, ResourceUnavailableException { |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15595)
|
|
@sureshanaparti can you have a look as well, please? |
| VMInstanceVO vmInstance = _vmDao.findById(vmId); | ||
|
|
||
| if (password == null || password.equals("")) { | ||
| if (password == null || password.isEmpty()) { |
There was a problem hiding this comment.
| if (password == null || password.isEmpty()) { | |
| if (StringUtils.isBlank(password)) { |
| } else { | ||
| _dailyOrHourly = false; | ||
| } | ||
| } else _dailyOrHourly = _usageAggregationRange == HOURLY_TIME; |
There was a problem hiding this comment.
if (_usageAggregationRange == DAILY_TIME || _usageAggregationRange == HOURLY_TIME) {
_dailyOrHourly = true;
}
| @@ -3449,7 +3411,7 @@ public UserVm rebootVirtualMachine(RebootVMCmd cmd) throws InsufficientCapacityE | |||
| throw new InvalidParameterValueException("Booting into a hardware setup menu is not implemented on " + vmInstance.getHypervisorType()); | |||
| } | |||
|
|
|||
| UserVm userVm = rebootVirtualMachine(CallContext.current().getCallingUserId(), vmId, enterSetup == null ? false : cmd.getBootIntoSetup(), cmd.isForced()); | |||
| UserVm userVm = rebootVirtualMachine(vmId, enterSetup != null && cmd.getBootIntoSetup(), cmd.isForced()); | |||
There was a problem hiding this comment.
boolean enterSetup = Boolean.TRUE.equals(cmd.getBootIntoSetup());
if (enterSetup && !HypervisorType.VMware.equals(vmInstance.getHypervisorType())) {
throw new InvalidParameterValueException("Booting into a hardware setup menu is not implemented on " + vmInstance.getHypervisorType());
}
UserVm userVm = rebootVirtualMachine(vmId, enterSetup, cmd.isForced());
| @@ -3769,7 +3726,7 @@ public void removeInstanceFromInstanceGroup(long vmId) { | |||
| } | |||
|
|
|||
| private boolean validPassword(String password) { | |||
| if (password == null || password.length() == 0) { | |||
| if (password == null || password.isEmpty()) { | |||
There was a problem hiding this comment.
| if (password == null || password.isEmpty()) { | |
| if (StringUtils.isBlank(password)) { |
| logger.error("error during resource reservation and allocation", e); | ||
| throw new CloudRuntimeException(e); | ||
| } | ||
| closeCheckeReservation(checkedReservations); |
There was a problem hiding this comment.
| closeCheckeReservation(checkedReservations); | |
| closeCheckedReservations(checkedReservations); |
| @@ -5186,7 +5118,7 @@ public void doInTransactionWithoutResult(TransactionStatus status) { | |||
| sc_nic.addAnd("macAddress", SearchCriteria.Op.EQ, vmNetworkStat.getMacAddress()); | |||
| NicVO nic = _nicDao.search(sc_nic, null).get(0); | |||
| List<VlanVO> vlan = _vlanDao.listVlansByNetworkId(nic.getNetworkId()); | |||
| if (vlan == null || vlan.size() == 0 || vlan.get(0).getVlanType() != VlanType.DirectAttached) | |||
| if (vlan == null || vlan.isEmpty() || vlan.get(0).getVlanType() != VlanType.DirectAttached) | |||
There was a problem hiding this comment.
| if (vlan == null || vlan.isEmpty() || vlan.get(0).getVlanType() != VlanType.DirectAttached) | |
| if (CollectionUtils.isEmpty(vlan) || vlan.get(0).getVlanType() != VlanType.DirectAttached) |
| @@ -5560,9 +5486,9 @@ public boolean finalizeStart(VirtualMachineProfile profile, long hostId, Command | |||
| } | |||
|
|
|||
| Answer answer = cmds.getAnswer("restoreVMSnapshot"); | |||
| if (answer != null && answer instanceof RestoreVMSnapshotAnswer) { | |||
| if (answer instanceof RestoreVMSnapshotAnswer) { | |||
| RestoreVMSnapshotAnswer restoreVMSnapshotAnswer = (RestoreVMSnapshotAnswer) answer; | |||
There was a problem hiding this comment.
can't answer be null here?
| @@ -5598,7 +5524,7 @@ public void finalizeExpunge(VirtualMachine vm) { | |||
| } | |||
|
|
|||
| private void checkForceStopVmPermission(Account callingAccount) { | |||
| if (!AllowUserForceStopVm.valueIn(callingAccount.getId())) { | |||
| if (callingAccount == null || !AllowUserForceStopVm.valueIn(callingAccount.getId())) { | |||
There was a problem hiding this comment.
we can check the config's global value if callingAccount is null
| @@ -6066,7 +5988,7 @@ public void doInTransactionWithoutResult(TransactionStatus status) { | |||
| SearchCriteria<VolumeVO> sc_volume = _volsDao.createSearchCriteria(); | |||
| sc_volume.addAnd("path", SearchCriteria.Op.LIKE, vmDiskStat.getPath() + "%"); | |||
| List<VolumeVO> volumes = _volsDao.search(sc_volume, null); | |||
| if ((volumes == null) || (volumes.size() == 0)) { | |||
| if ((volumes == null) || (volumes.isEmpty())) { | |||
There was a problem hiding this comment.
| if ((volumes == null) || (volumes.isEmpty())) { | |
| if (CollectionUtils.isEmpty(volumes)) { |
| @@ -9321,7 +9222,7 @@ public void finalizeUnmanage(VirtualMachine vm) { | |||
|
|
|||
| private void encryptAndStorePassword(UserVmVO vm, String password) { | |||
| String sshPublicKeys = vm.getDetail(VmDetailConstants.SSH_PUBLIC_KEY); | |||
| if (sshPublicKeys != null && !sshPublicKeys.equals("") && password != null && !password.equals("saved_password")) { | |||
| if (sshPublicKeys != null && !sshPublicKeys.isEmpty() && password != null && !password.equals("saved_password")) { | |||
There was a problem hiding this comment.
| if (sshPublicKeys != null && !sshPublicKeys.isEmpty() && password != null && !password.equals("saved_password")) { | |
| if (!StringUtils.isEmpty(sshPublicKeys) && password != null && !password.equals("saved_password")) { |
Description
This PR cleans static analysis warnings from UserVmManagerImpl and some in dependencies.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?