Copilot commented on code in PR #14388:
URL: https://github.com/apache/cloudstack/pull/14388#discussion_r4247238850
##########
plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java:
##########
@@ -572,7 +617,30 @@ public KVMPhysicalDisk createDiskFromTemplate(
@Override
public List<KVMPhysicalDisk> listPhysicalDisks(String storagePoolUuid,
KVMStoragePool pool)
{
- throw new UnsupportedOperationException("Listing disks is not
supported for this configuration.");
+ // lists the resources of the pool's resource group by their volume
path, for volume import
+ final String rscGrpName = ((LinstorStoragePool)
pool).getResourceGroup();
+ final DevelopersApi api = getLinstorAPI(pool);
+ try {
+ List<KVMPhysicalDisk> disks = new ArrayList<>();
+ for (ResourceDefinition rd : api.resourceDefinitionList(null,
true, null, null, null)) {
Review Comment:
`resourceDefinitionList(null, true, ...)` fetches all resource definitions
and then filters client-side, which can be expensive on larger Linstor
clusters. Prefer server-side filtering if supported by the API (e.g., querying
only by resource group and/or by `cs-` prefix), or reduce result size by using
more selective parameters to avoid iterating over the full controller inventory.
##########
plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStoragePool.java:
##########
@@ -92,6 +92,18 @@ public boolean disconnectPhysicalDisk(String volumeUuid)
return _storageAdaptor.disconnectPhysicalDisk(volumeUuid, this);
}
+ @Override
+ public void checkPhysicalDiskForImport(String volumeUuid)
+ {
+
((LinstorStorageAdaptor)_storageAdaptor).checkPhysicalDiskForImport(volumeUuid,
this);
+ }
+
+ @Override
+ public Boolean isPhysicalDiskInUse(String volumeUuid)
+ {
+ return
((LinstorStorageAdaptor)_storageAdaptor).isPhysicalDiskInUse(volumeUuid, this);
+ }
Review Comment:
These downcasts to `LinstorStorageAdaptor` tightly couple
`LinstorStoragePool` to the concrete adaptor type and introduce a runtime
`ClassCastException` risk if wiring changes. Prefer storing `_storageAdaptor`
as `LinstorStorageAdaptor` in this pool (typed field) or pushing these methods
into the common adaptor interface so calls can be made polymorphically without
casts.
##########
plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java:
##########
@@ -357,6 +357,51 @@ public boolean connectPhysicalDisk(
return true;
}
+ /**
+ * Checks that a resource which is not managed by CloudStack yet can be
imported into this pool. It must
+ * belong to the resource group of the pool, otherwise a resource owned by
a volume of another pool on the
+ * same controller could be imported a second time.
+ */
+ public void checkPhysicalDiskForImport(String name, LinstorStoragePool
pool) {
+ final DevelopersApi api = getLinstorAPI(pool);
+ final String rscName = getLinstorRscName(name);
+ try {
+ List<ResourceDefinition> rscDfns = api.resourceDefinitionList(
+ Collections.singletonList(rscName), null, null, null,
null);
+ if (rscDfns == null || rscDfns.isEmpty()) {
+ throw new CloudRuntimeException(String.format("Linstor:
resource %s not found", rscName));
+ }
+ final String rscGrpName = rscDfns.get(0).getResourceGroupName();
+ if (rscGrpName == null ||
!rscGrpName.equalsIgnoreCase(pool.getResourceGroup())) {
+ throw new CloudRuntimeException(String.format(
+ "Linstor: resource %s belongs to resource group %s,
not to %s of this storage pool",
+ rscName, rscGrpName, pool.getResourceGroup()));
+ }
+ } catch (ApiException apiEx) {
+ logger.error(apiEx);
+ throw new CloudRuntimeException(apiEx.getBestMessage(), apiEx);
+ }
Review Comment:
`logger.error(apiEx);` typically logs only `toString()` and may drop the
stack trace depending on the logger implementation. Log with an explicit
message and pass the exception as the second argument (e.g.,
`logger.error("...", apiEx)`) so stack traces are preserved for
troubleshooting; apply similarly to the other new `catch (ApiException ...)`
blocks in this class.
##########
plugins/storage/volume/linstor/src/main/java/com/cloud/hypervisor/kvm/storage/LinstorStorageAdaptor.java:
##########
@@ -572,7 +617,30 @@ public KVMPhysicalDisk createDiskFromTemplate(
@Override
public List<KVMPhysicalDisk> listPhysicalDisks(String storagePoolUuid,
KVMStoragePool pool)
{
- throw new UnsupportedOperationException("Listing disks is not
supported for this configuration.");
+ // lists the resources of the pool's resource group by their volume
path, for volume import
+ final String rscGrpName = ((LinstorStoragePool)
pool).getResourceGroup();
+ final DevelopersApi api = getLinstorAPI(pool);
+ try {
+ List<KVMPhysicalDisk> disks = new ArrayList<>();
+ for (ResourceDefinition rd : api.resourceDefinitionList(null,
true, null, null, null)) {
+ if
(!rd.getName().toLowerCase().startsWith(LinstorUtil.RSC_PREFIX) ||
rd.getResourceGroupName() == null
+ ||
!rd.getResourceGroupName().equalsIgnoreCase(rscGrpName)
+ || rd.getVolumeDefinitions() == null ||
rd.getVolumeDefinitions().isEmpty()) {
+ continue;
+ }
+ final KVMPhysicalDisk disk = new
KVMPhysicalDisk(LinstorUtil.formatDrbdByResDevicePath(rd.getName()),
+
rd.getName().substring(LinstorUtil.RSC_PREFIX.length()), pool);
Review Comment:
`rd.getName().toLowerCase()` can throw `NullPointerException` if
`rd.getName()` is null. Guard `rd.getName()` before calling `toLowerCase()`
(e.g., `String name = rd.getName(); if (name == null) continue; ...`). This
keeps `listPhysicalDisks` resilient to unexpected/partial API responses.
--
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]
