You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "abh1sar (via GitHub)" <gi...@apache.org> on 2026/10/12 10:14:04 UTC
[PR] importVM Linstor support [cloudstack]
abh1sar opened a new pull request, #14388:
URL: https://github.com/apache/cloudstack/pull/14388
### Description
* `importVm importsource=shared` now accepts a Linstor pool, with diskpath being the resource name without the cs- prefix.
* The check refuses a resource of another resource group, reports a resource in use on any node as locked, and connects the disk on the checking host for the read and disconnects it again.
* The imported volume keeps the hypervisor default format like a native Linstor volume, as templates made from its snapshot backups take that format.
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ******************************************************************************* -->
<!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ******************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [x] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
1. Deployed a VM on a Linstor pool (two-node Linstor cluster)
2. Unmanaged it
3. Imported the VM, with the volume check on the node with the replica and on the node without it
4. Verified the VM starts on both nodes
5. Verified a diskless resource is created for the check on the node without the replica and removed after it
6. Verified the import is refused while the unmanaged VM is still running, on either node
8. Took volume snapshots on the imported volume and created a template and a volume from the snapshot
9. Verified the imported volume is recorded as QCOW2 like a natively created Linstor volume, so templates from its snapshots match their file format
10. No failures or errors were observed
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
#### How did you try to break this feature and the system with this change?
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] importVM Linstor support [cloudstack]
Posted by "andrijapanicsb (via GitHub)" <gi...@apache.org>.
andrijapanicsb commented on PR #14388:
URL: https://github.com/apache/cloudstack/pull/14388#issuecomment-6279027615
@abh1sar all changes from Rene have been merged, yes. And is seems to work all fine (third-party independent testing).
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] importVM Linstor support [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #14388:
URL: https://github.com/apache/cloudstack/pull/14388#issuecomment-6145064825
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19530
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] importVM Linstor support [cloudstack]
Posted by "rp- (via GitHub)" <gi...@apache.org>.
rp- commented on code in PR #14388:
URL: https://github.com/apache/cloudstack/pull/14388#discussion_r4281583464
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeCommandWrapper.java:
##########
@@ -61,35 +62,47 @@ public Answer execute(final CheckVolumeCommand command, final LibvirtComputingRe
KVMStoragePoolManager poolMgr = libvirtComputingResource.getStoragePoolMgr();
KVMStoragePool pool = poolMgr.getStoragePool(storageFilerTO.getType(), storageFilerTO.getUuid());
+ boolean connected = false;
try {
if (STORAGE_POOL_TYPES_SUPPORTED.contains(storageFilerTO.getType())) {
+ if (Boolean.TRUE.equals(pool.isPhysicalDiskInUse(srcFile))) {
+ Map<VolumeOnStorageTO.Detail, String> volumeDetails = new HashMap<>();
+ volumeDetails.put(VolumeOnStorageTO.Detail.IS_LOCKED, String.valueOf(Boolean.TRUE));
+ return new CheckVolumeAnswer(command, false, "", 0, volumeDetails);
+ }
+ pool.checkPhysicalDiskForImport(srcFile);
Review Comment:
If `checkPhysicalDiskForImport` throws, the catch at line 101 returns `new Answer(command, false, result)` with `result == null`. The server then only reports "Disk not found or is invalid", and the "belongs to resource group X, not to Y" message ends up in the agent log only. Could the wrapper return the exception message here, and could `importKvmVirtualMachineFromDisk` include `answer.getDetails()` in its error?
##########
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())) {
Review Comment:
The resource group check is good, but this should also refuse resources with a LUKS layer.
LINSTOR encrypts with a LUKS layer below DRBD, so the DRBD device the import reads is plaintext. `qemu-img` info never reports encrypted: yes, and the `IS_ENCRYPTED` checks in `checkVolume/checkIfVolumeIsEncrypted` never trigger. The imported volume then has no `passphrase_id`. `LinstorPrimaryDataStoreDriverImpl#copySnapshot`decides whether a volume is encrypted from `getPassphraseId() != null`. For such a volume it backs up the storage-layer snapshot device directly, and that device holds the LUKS ciphertext. The snapshot backup, and any template or volume created from it, is silently corrupt.
The easiest way in is unmanaging a VM with an encrypted LINSTOR root disk and importing it again. As far as I can see, unmanaging a VM doesn't block encrypted volumes. A resource created outside CloudStack in a LUKS-layered stack has the same problem.
Could you refuse resources whose layer stack contains `LayerType.LUKS`, e.g. by checking the layer data of the resource definition / resource we already fetch here? The error message should say why. Importing them as encrypted isn't possible without the passphrase, so refusing seems like the only safe option.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] importVM Linstor support [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on PR #14388:
URL: https://github.com/apache/cloudstack/pull/14388#issuecomment-6258368923
Closing this as https://github.com/apache/cloudstack/pull/14256 is already merged with the same functionality.
I'll check and address @rp- 's comment on main if they are missing and raise another PR
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] importVM Linstor support [cloudstack]
Posted by "nvazquez (via GitHub)" <gi...@apache.org>.
nvazquez commented on PR #14388:
URL: https://github.com/apache/cloudstack/pull/14388#issuecomment-6290644334
@abh1sar @rp- can you please backport the funcionality from main branch to 4.22?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] importVM Linstor support [cloudstack]
Posted by "Damans227 (via GitHub)" <gi...@apache.org>.
Damans227 commented on code in PR #14388:
URL: https://github.com/apache/cloudstack/pull/14388#discussion_r4257835547
##########
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())) {
Review Comment:
should this also refuse a template copy on the pool? those are cs- resources in the same resource group, and importvm doesnt check the template list like volume import does
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetVolumesOnStorageCommandWrapper.java:
##########
@@ -97,6 +103,10 @@ private GetVolumesOnStorageAnswer addVolumeByVolumePath(final GetVolumesOnStorag
return new GetVolumesOnStorageAnswer(command, false, String.format("The file format is %s, but expected to be %s", fileFormat, disk.getFormat()));
}
addDetailsToVolumeOnStorageTO(volumeOnStorageTO, info, storagePool, disk);
+ // qemu's lock only reveals a guest on this host for pools such as Linstor, so ask the pool
+ if (Boolean.TRUE.equals(storagePool.isPhysicalDiskInUse(volumePath))) {
Review Comment:
should the full list ask the pool too? right now a volume in use on another host shows as not locked there, only the lookup by path catches it
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] importVM Linstor support [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar closed pull request #14388: importVM Linstor support
URL: https://github.com/apache/cloudstack/pull/14388
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] importVM Linstor support [cloudstack]
Posted by "rajujith (via GitHub)" <gi...@apache.org>.
rajujith commented on PR #14388:
URL: https://github.com/apache/cloudstack/pull/14388#issuecomment-6144077242
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] importVM Linstor support [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #14388:
URL: https://github.com/apache/cloudstack/pull/14388#issuecomment-6144095212
@rajujith 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.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] importVM Linstor support [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #14388:
URL: https://github.com/apache/cloudstack/pull/14388#issuecomment-6122354837
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/14388?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 4.04%. Comparing base ([`437adc8`](https://app.codecov.io/gh/apache/cloudstack/commit/437adc81dec08b2fbb6a57461126db56d1a20059?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0cb9f07`](https://app.codecov.io/gh/apache/cloudstack/commit/0cb9f0732479820deb707176010c5f36c9c0adbe?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on 4.22.
> :exclamation: There is a different number of reports uploaded between BASE (437adc8) and HEAD (0cb9f07). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (437adc8) | HEAD (0cb9f07) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.22 #14388 +/- ##
=============================================
- Coverage 18.02% 4.04% -13.99%
=============================================
Files 5936 449 -5487
Lines 535823 38240 -497583
Branches 65612 7082 -58530
=============================================
- Hits 96572 1546 -95026
+ Misses 428252 36482 -391770
+ Partials 10999 212 -10787
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/14388/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/14388/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.04% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/14388/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/cloudstack/pull/14388?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] importVM Linstor support [cloudstack]
Posted by "andrijapanicsb (via GitHub)" <gi...@apache.org>.
andrijapanicsb commented on PR #14388:
URL: https://github.com/apache/cloudstack/pull/14388#issuecomment-6279032890
Ceph support has also been added, not sure if you plan for another PR of the same nature etc. Cheers
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] importVM Linstor support [cloudstack]
Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on code in PR #14388:
URL: https://github.com/apache/cloudstack/pull/14388#discussion_r4247563605
##########
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:
Matches the existing logging in this class (e.g. `getPhysicalDisk`), and the exception is rethrown with the cause attached.
##########
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:
The Linstor API has no server-side filter by resource group or name prefix; the plugin already lists all definitions the same way (`LinstorUtil.getRDListStartingWith`, `findResourceDefinition`, the volume stats cache). This is only used by the admin `listVolumesForImport` call.
##########
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:
A Linstor resource definition always has a name (it is its key); the existing helpers rely on that as well.
##########
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:
Follows the existing pattern in this class (`getCapacity`, `getUsed`, `getAvailable`, `isNodeOnline`); the pool is only ever created by `LinstorStorageAdaptor`.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] importVM Linstor support [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org