Copilot commented on code in PR #12451:
URL: https://github.com/apache/cloudstack/pull/12451#discussion_r2697916067
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1564,8 +1564,10 @@ protected String
getStoragePoolNonDestroyedVolumesLog(long storagePoolId) {
sb.append("[");
for (VolumeVO vol : nonDestroyedVols) {
- volInstance = _vmInstanceDao.findById(vol.getInstanceId());
- logMessageInfo.add(String.format("Volume [%s] (attached to VM
[%s])", vol.getUuid(), volInstance.getUuid()));
+ if (vol.getInstanceId() != null) {
+ volInstance = _vmInstanceDao.findById(vol.getInstanceId());
+ logMessageInfo.add(String.format("Volume [%s] (attached to VM
[%s])", vol.getUuid(), volInstance.getUuid()));
Review Comment:
The findById call could potentially return null if the VM instance has been
deleted from the database but the volume still references it. This would cause
the same NPE on line 1569. Add a null check for volInstance: `if (volInstance
!= null) { logMessageInfo.add(...); }`
```suggestion
if (volInstance != null) {
logMessageInfo.add(String.format("Volume [%s] (attached
to VM [%s])", vol.getUuid(), volInstance.getUuid()));
}
```
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1564,8 +1564,10 @@ protected String
getStoragePoolNonDestroyedVolumesLog(long storagePoolId) {
sb.append("[");
for (VolumeVO vol : nonDestroyedVols) {
- volInstance = _vmInstanceDao.findById(vol.getInstanceId());
- logMessageInfo.add(String.format("Volume [%s] (attached to VM
[%s])", vol.getUuid(), volInstance.getUuid()));
+ if (vol.getInstanceId() != null) {
+ volInstance = _vmInstanceDao.findById(vol.getInstanceId());
+ logMessageInfo.add(String.format("Volume [%s] (attached to VM
[%s])", vol.getUuid(), volInstance.getUuid()));
+ }
Review Comment:
The existing test
`getStoragePoolNonDestroyedVolumesLogTestNonDestroyedVolumesReturnLog` only
covers volumes with attached VMs. Add test cases for: (1) volumes with null
instanceId (detached volumes), and (2) volumes where instanceId exists but the
VM instance is not found in the database.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1564,8 +1564,10 @@ protected String
getStoragePoolNonDestroyedVolumesLog(long storagePoolId) {
sb.append("[");
for (VolumeVO vol : nonDestroyedVols) {
- volInstance = _vmInstanceDao.findById(vol.getInstanceId());
- logMessageInfo.add(String.format("Volume [%s] (attached to VM
[%s])", vol.getUuid(), volInstance.getUuid()));
+ if (vol.getInstanceId() != null) {
+ volInstance = _vmInstanceDao.findById(vol.getInstanceId());
+ logMessageInfo.add(String.format("Volume [%s] (attached to VM
[%s])", vol.getUuid(), volInstance.getUuid()));
Review Comment:
Detached volumes (where instanceId is null) are silently skipped from the
log message, making it incomplete and potentially misleading. The log should
still include information about detached volumes. Consider adding an else
branch to log detached volumes separately, for example:
`logMessageInfo.add(String.format(\"Volume [%s] (detached)\", vol.getUuid()));`
```suggestion
logMessageInfo.add(String.format("Volume [%s] (attached to
VM [%s])", vol.getUuid(), volInstance.getUuid()));
} else {
logMessageInfo.add(String.format("Volume [%s] (detached)",
vol.getUuid()));
```
--
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]
