Skip to content

Commit 01058cd

Browse files
Fix LINSTOR volume import inspection and resource group validation
Use read-only controller metadata for listing and single-volume inspection without temporary resource placement or DRBD property changes. Validate the selected pool resource group in management-server ROOT/DATA import paths and agent inspection. Add regression coverage for remote replicas, unknown or busy resources, and cross-group imports.
1 parent fd16736 commit 01058cd

15 files changed

Lines changed: 737 additions & 74 deletions

File tree

‎engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreDriver.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,13 @@
3131
import com.cloud.utils.Pair;
3232

3333
public interface PrimaryDataStoreDriver extends DataStoreDriver {
34+
/** Validate backend ownership before adopting an existing volume, without modifying it. */
35+
default void validateVolumeForImport(StoragePool pool, String path) {
36+
if (pool.getPoolType() == StoragePoolType.Linstor) {
37+
throw new UnsupportedOperationException("The storage driver cannot validate LINSTOR volume imports");
38+
}
39+
}
40+
3441
enum QualityOfServiceState { MIGRATION, NO_MIGRATION }
3542

3643
String BASIC_CREATE = "basicCreate";

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeCommandWrapper.java‎

Lines changed: 11 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -56,39 +56,24 @@ public final class LibvirtCheckVolumeCommandWrapper extends CommandWrapper<Check
5656

5757
@Override
5858
public Answer execute(final CheckVolumeCommand command, final LibvirtComputingResource libvirtComputingResource) {
59-
String result = null;
6059
String srcFile = command.getSrcFile();
6160
StorageFilerTO storageFilerTO = command.getStorageFilerTO();
6261
KVMStoragePoolManager poolMgr = libvirtComputingResource.getStoragePoolMgr();
6362
KVMStoragePool pool = poolMgr.getStoragePool(storageFilerTO.getType(), storageFilerTO.getUuid());
6463

6564
try {
6665
if (STORAGE_POOL_TYPES_SUPPORTED.contains(storageFilerTO.getType())) {
67-
final KVMPhysicalDisk vol = pool.getPhysicalDisk(srcFile);
68-
if (Storage.StoragePoolType.RBD.equals(storageFilerTO.getType())
69-
|| Storage.StoragePoolType.Linstor.equals(storageFilerTO.getType())) {
70-
// RBD and Linstor volumes are raw block devices, not local qcow2 files:
71-
// inspect them through qemu-img (RBD by its rbd: URI, Linstor by its
72-
// /dev/drbd device path) rather than checkQcow2File, which would reject
73-
// a raw device.
74-
//
75-
// A Linstor volume only materialises as a local /dev/drbd device once the
76-
// resource is made available on THIS host (a diskless DRBD assignment). RBD
77-
// needs no such step — qemu-img reaches it over the network by its rbd: URI.
78-
// So for Linstor we connect the resource here before qemu-img inspects it and
79-
// release the diskless assignment afterwards; the replicated data on the
80-
// storage nodes is untouched (disconnect only drops a local diskless copy).
81-
boolean linstorConnected = false;
82-
if (Storage.StoragePoolType.Linstor.equals(storageFilerTO.getType())) {
83-
linstorConnected = poolMgr.connectPhysicalDisk(storageFilerTO.getType(), storageFilerTO.getUuid(), srcFile, null);
84-
}
85-
try {
86-
return checkRbdVolume(command, pool, vol);
87-
} finally {
88-
if (linstorConnected) {
89-
poolMgr.disconnectPhysicalDisk(storageFilerTO.getType(), storageFilerTO.getUuid(), srcFile);
90-
}
66+
if (Storage.StoragePoolType.Linstor.equals(storageFilerTO.getType())) {
67+
List<VolumeOnStorageTO> volumes = pool.getVolumesForImport(srcFile);
68+
if (volumes == null || volumes.size() != 1 || !srcFile.equals(volumes.get(0).getPath())) {
69+
return new CheckVolumeAnswer(command, false, "LINSTOR volume not found", 0, null);
9170
}
71+
VolumeOnStorageTO volume = volumes.get(0);
72+
return new CheckVolumeAnswer(command, true, "", volume.getVirtualSize(), volume.getDetails());
73+
}
74+
final KVMPhysicalDisk vol = pool.getPhysicalDisk(srcFile);
75+
if (Storage.StoragePoolType.RBD.equals(storageFilerTO.getType())) {
76+
return checkRbdVolume(command, pool, vol);
9277
}
9378
final String path = vol.getPath();
9479
try {
@@ -104,7 +89,7 @@ public Answer execute(final CheckVolumeCommand command, final LibvirtComputingRe
10489
}
10590
} catch (final Exception e) {
10691
logger.error("Error while checking the disk: {}", e.getMessage());
107-
return new Answer(command, false, result);
92+
return new Answer(command, false, e.getMessage());
10893
}
10994
}
11095

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetVolumesOnStorageCommandWrapper.java‎

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -64,25 +64,22 @@ public Answer execute(final GetVolumesOnStorageCommand command, final LibvirtCom
6464
final KVMStoragePoolManager storagePoolMgr = libvirtComputingResource.getStoragePoolMgr();
6565
final KVMStoragePool storagePool = storagePoolMgr.getStoragePool(pool.getType(), pool.getUuid(), true, true);
6666

67-
if (StringUtils.isNotBlank(volumePath)) {
68-
// A Linstor volume is a DRBD device that only appears on this host once its resource
69-
// is made available here (a diskless assignment); connect it before qemu-img inspects
70-
// the device and release the diskless assignment afterwards (the replicated data on
71-
// the storage nodes is untouched). RBD needs no such step.
72-
boolean linstorConnected = false;
73-
if (StoragePoolType.Linstor.equals(pool.getType())) {
74-
linstorConnected = storagePoolMgr.connectPhysicalDisk(pool.getType(), pool.getUuid(), volumePath, null);
75-
}
67+
if (StoragePoolType.Linstor.equals(pool.getType())) {
7668
try {
77-
return addVolumeByVolumePath(command, storagePool, volumePath);
78-
} finally {
79-
if (linstorConnected) {
80-
storagePoolMgr.disconnectPhysicalDisk(pool.getType(), pool.getUuid(), volumePath);
69+
List<VolumeOnStorageTO> volumes = storagePool.getVolumesForImport(
70+
StringUtils.isBlank(volumePath) ? null : volumePath);
71+
if (StringUtils.isNotBlank(keyword)) {
72+
volumes = volumes.stream().filter(volume -> volume.getName().contains(keyword)).collect(Collectors.toList());
8173
}
74+
return new GetVolumesOnStorageAnswer(command, volumes);
75+
} catch (RuntimeException e) {
76+
return new GetVolumesOnStorageAnswer(command, false, e.getMessage());
8277
}
83-
} else {
84-
return addAllVolumes(command, storagePool, keyword);
8578
}
79+
if (StringUtils.isNotBlank(volumePath)) {
80+
return addVolumeByVolumePath(command, storagePool, volumePath);
81+
}
82+
return addAllVolumes(command, storagePool, keyword);
8683
}
8784

8885
private GetVolumesOnStorageAnswer addVolumeByVolumePath(final GetVolumesOnStorageCommand command, final KVMStoragePool storagePool, String volumePath) {

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePool.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import com.cloud.agent.properties.AgentPropertiesFileHandler;
2525
import com.cloud.hypervisor.kvm.resource.LibvirtVMDef;
2626
import org.apache.cloudstack.utils.qemu.QemuImg.PhysicalDiskFormat;
27+
import org.apache.cloudstack.storage.volume.VolumeOnStorageTO;
2728
import org.joda.time.Duration;
2829

2930
import com.cloud.agent.api.to.HostTO;
@@ -56,6 +57,11 @@ default KVMPhysicalDisk createPhysicalDisk(String volumeUuid, PhysicalDiskFormat
5657

5758
List<KVMPhysicalDisk> listPhysicalDisks();
5859

60+
/** Read-only backend import metadata; null path requests all volumes in this pool. */
61+
default List<VolumeOnStorageTO> getVolumesForImport(String path) {
62+
throw new UnsupportedOperationException("Backend import metadata is not supported by this storage pool");
63+
}
64+
5965
String getUuid();
6066

6167
long getCapacity();

‎plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCheckVolumeCommandWrapperTest.java‎

Lines changed: 29 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -104,11 +104,7 @@ private void mockLinstorPool() {
104104
Mockito.when(storageFilerTO.getType()).thenReturn(Storage.StoragePoolType.Linstor);
105105
Mockito.when(storageFilerTO.getUuid()).thenReturn(poolUuid);
106106
Mockito.when(storagePoolMgr.getStoragePool(Storage.StoragePoolType.Linstor, poolUuid)).thenReturn(storagePool);
107-
Mockito.when(storagePool.getType()).thenReturn(Storage.StoragePoolType.Linstor);
108-
Mockito.when(storagePool.getPhysicalDisk(srcFile)).thenReturn(disk);
109-
// Linstor resolves the resource to a local /dev/drbd block device path.
110-
Mockito.when(disk.getPath()).thenReturn("/dev/drbd/by-res/" + srcFile + "/0");
111-
Mockito.when(disk.getFormat()).thenReturn(QemuImg.PhysicalDiskFormat.RAW);
107+
112108
}
113109

114110
private CheckVolumeCommand buildCommand() {
@@ -154,45 +150,45 @@ public void testRbdVolumeInspectedThroughRbdUri() throws Exception {
154150
}
155151

156152
@Test
157-
public void testLinstorVolumeReturnsSuccessInspectedViaDevicePath() throws Exception {
153+
public void testLinstorVolumeUsesControllerMetadataWithoutConnecting() throws Exception {
158154
mockLinstorPool();
159-
Mockito.when(disk.getVirtualSize()).thenReturn(virtualSize);
160-
qemuImg = Mockito.mockConstruction(QemuImg.class, (mock, context) ->
161-
Mockito.when(mock.info(Mockito.any(QemuImgFile.class), Mockito.anyBoolean())).thenReturn(qemuInfo));
155+
VolumeOnStorageTO volume = new VolumeOnStorageTO(null, srcFile, srcFile, "/dev/drbd/by-res/cs-x/0", "RAW", virtualSize, virtualSize);
156+
volume.addDetail(VolumeOnStorageTO.Detail.IS_LOCKED, "false");
157+
Mockito.when(storagePool.getVolumesForImport(srcFile)).thenReturn(java.util.Collections.singletonList(volume));
158+
qemuImg = Mockito.mockConstruction(QemuImg.class);
162159

163160
Answer answer = wrapper.execute(buildCommand(), libvirtComputingResource);
164161

165-
Assert.assertTrue(answer instanceof CheckVolumeAnswer);
166-
Assert.assertTrue("Linstor raw volume should validate (not rejected as non-qcow2)", answer.getResult());
162+
Assert.assertTrue(answer.getResult());
167163
Assert.assertEquals(virtualSize, ((CheckVolumeAnswer) answer).getSize());
168-
// A Linstor volume is a local block device: qemu-img must inspect the
169-
// /dev/drbd device path directly, never build an rbd: URI.
170-
ArgumentCaptor<QemuImgFile> fileCaptor = ArgumentCaptor.forClass(QemuImgFile.class);
171-
Mockito.verify(qemuImg.constructed().get(0), Mockito.atLeastOnce())
172-
.info(fileCaptor.capture(), Mockito.anyBoolean());
173-
String fileName = fileCaptor.getValue().getFileName();
174-
Assert.assertTrue("qemu-img should point at the /dev/drbd device path, got " + fileName,
175-
fileName.startsWith("/dev/drbd/"));
176-
Assert.assertFalse("Linstor must not build an rbd: URI", fileName.startsWith("rbd:"));
164+
Assert.assertTrue(qemuImg.constructed().isEmpty());
165+
Mockito.verify(storagePoolMgr, Mockito.never()).connectPhysicalDisk(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any());
166+
Mockito.verify(storagePoolMgr, Mockito.never()).disconnectPhysicalDisk(Mockito.any(), Mockito.any(), Mockito.any());
167+
Mockito.verify(storagePool, Mockito.never()).getPhysicalDisk(Mockito.any());
177168
}
178169

179170
@Test
180-
public void testLinstorVolumeInUseOnAnotherNodeIsReportedLocked() throws Exception {
171+
public void testLinstorVolumeInUseOnAnotherNodeIsReportedLocked() {
181172
mockLinstorPool();
182-
Mockito.when(disk.getVirtualSize()).thenReturn(virtualSize);
183-
Mockito.when(disk.getName()).thenReturn(srcFile);
184-
// qemu-img info succeeds, so the host-local file lock is clear ...
185-
qemuImg = Mockito.mockConstruction(QemuImg.class, (mock, context) ->
186-
Mockito.when(mock.info(Mockito.any(QemuImgFile.class), Mockito.anyBoolean())).thenReturn(qemuInfo));
187-
// ... but Linstor reports the DRBD resource in use on another node.
188-
Mockito.when(storagePool.getVolumeInUseNode(srcFile)).thenReturn("node-2");
173+
VolumeOnStorageTO volume = new VolumeOnStorageTO(null, srcFile, srcFile, "", "RAW", virtualSize, virtualSize);
174+
volume.addDetail(VolumeOnStorageTO.Detail.IS_LOCKED, "true");
175+
Mockito.when(storagePool.getVolumesForImport(srcFile)).thenReturn(java.util.Collections.singletonList(volume));
189176

190-
Answer answer = wrapper.execute(buildCommand(), libvirtComputingResource);
177+
CheckVolumeAnswer answer = (CheckVolumeAnswer) wrapper.execute(buildCommand(), libvirtComputingResource);
191178

192-
Assert.assertTrue(answer instanceof CheckVolumeAnswer);
193-
Map<VolumeOnStorageTO.Detail, String> details = ((CheckVolumeAnswer) answer).getVolumeDetails();
194-
Assert.assertEquals("cluster-wide in-use must mark the volume locked so adoption refuses it",
195-
"true", details.get(VolumeOnStorageTO.Detail.IS_LOCKED));
179+
Assert.assertEquals("true", answer.getVolumeDetails().get(VolumeOnStorageTO.Detail.IS_LOCKED));
180+
Mockito.verify(storagePoolMgr, Mockito.never()).disconnectPhysicalDisk(Mockito.any(), Mockito.any(), Mockito.any());
181+
}
182+
183+
@Test
184+
public void testLinstorInvalidGroupReturnsActionableFailure() {
185+
mockLinstorPool();
186+
Mockito.when(storagePool.getVolumesForImport(srcFile)).thenThrow(
187+
new com.cloud.utils.exception.CloudRuntimeException("Resource group mismatch: rg-ssd vs rg-hdd"));
188+
Answer answer = wrapper.execute(buildCommand(), libvirtComputingResource);
189+
Assert.assertFalse(answer.getResult());
190+
Assert.assertTrue(answer.getDetails().contains("rg-ssd"));
191+
Mockito.verify(storagePoolMgr, Mockito.never()).connectPhysicalDisk(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any());
196192
}
197193

198194
@Test
Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
package com.cloud.hypervisor.kvm.resource.wrapper;
18+
19+
import java.util.Arrays;
20+
import java.util.Collections;
21+
import com.cloud.agent.api.Answer;
22+
import com.cloud.agent.api.GetVolumesOnStorageAnswer;
23+
import com.cloud.agent.api.GetVolumesOnStorageCommand;
24+
import com.cloud.agent.api.to.StorageFilerTO;
25+
import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
26+
import com.cloud.hypervisor.kvm.storage.KVMStoragePool;
27+
import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager;
28+
import com.cloud.storage.Storage;
29+
import com.cloud.utils.exception.CloudRuntimeException;
30+
import org.apache.cloudstack.storage.volume.VolumeOnStorageTO;
31+
import org.apache.cloudstack.utils.qemu.QemuImg;
32+
import org.junit.Before;
33+
import org.junit.Test;
34+
import org.mockito.MockedConstruction;
35+
36+
import static org.junit.Assert.assertEquals;
37+
import static org.junit.Assert.assertFalse;
38+
import static org.junit.Assert.assertTrue;
39+
import static org.mockito.ArgumentMatchers.any;
40+
import static org.mockito.Mockito.mock;
41+
import static org.mockito.Mockito.mockConstruction;
42+
import static org.mockito.Mockito.never;
43+
import static org.mockito.Mockito.verify;
44+
import static org.mockito.Mockito.when;
45+
46+
public class LibvirtLinstorImportListingTest {
47+
private KVMStoragePool pool;
48+
private KVMStoragePoolManager manager;
49+
private LibvirtComputingResource resource;
50+
private StorageFilerTO filer;
51+
private final LibvirtGetVolumesOnStorageCommandWrapper wrapper = new LibvirtGetVolumesOnStorageCommandWrapper();
52+
53+
@Before
54+
public void setup() {
55+
pool = mock(KVMStoragePool.class);
56+
manager = mock(KVMStoragePoolManager.class);
57+
resource = mock(LibvirtComputingResource.class);
58+
filer = mock(StorageFilerTO.class);
59+
when(filer.getType()).thenReturn(Storage.StoragePoolType.Linstor);
60+
when(filer.getUuid()).thenReturn("pool");
61+
when(resource.getStoragePoolMgr()).thenReturn(manager);
62+
when(manager.getStoragePool(Storage.StoragePoolType.Linstor, "pool", true, true)).thenReturn(pool);
63+
}
64+
65+
private VolumeOnStorageTO volume(String name, boolean locked) {
66+
VolumeOnStorageTO volume = new VolumeOnStorageTO(null, name, name, "/dev/drbd/by-res/cs-" + name + "/0", "RAW", 1024, 1024);
67+
volume.addDetail(VolumeOnStorageTO.Detail.IS_LOCKED, Boolean.toString(locked));
68+
return volume;
69+
}
70+
71+
private void verifyNoStorageMutation() {
72+
verify(manager, never()).connectPhysicalDisk(any(), any(), any(), any());
73+
verify(manager, never()).disconnectPhysicalDisk(any(), any(), any());
74+
verify(pool, never()).getPhysicalDisk(any());
75+
verify(pool, never()).listPhysicalDisks();
76+
}
77+
78+
@Test
79+
public void listingUsesMetadataWithoutQemuOrConnectingAndKeepsKeyword() {
80+
when(pool.getVolumesForImport(null)).thenReturn(Arrays.asList(volume("alpha", false), volume("beta", true)));
81+
try (MockedConstruction<QemuImg> qemu = mockConstruction(QemuImg.class)) {
82+
GetVolumesOnStorageAnswer answer = (GetVolumesOnStorageAnswer) wrapper.execute(
83+
new GetVolumesOnStorageCommand(filer, null, "beta"), resource);
84+
assertTrue(answer.getResult());
85+
assertEquals(1, answer.getVolumes().size());
86+
assertEquals("beta", answer.getVolumes().get(0).getName());
87+
assertEquals("true", answer.getVolumes().get(0).getDetails().get(VolumeOnStorageTO.Detail.IS_LOCKED));
88+
assertTrue(qemu.constructed().isEmpty());
89+
}
90+
verifyNoStorageMutation();
91+
}
92+
93+
@Test
94+
public void singlePathAlsoUsesReadOnlyMetadata() {
95+
when(pool.getVolumesForImport("alpha")).thenReturn(Collections.singletonList(volume("alpha", false)));
96+
try (MockedConstruction<QemuImg> qemu = mockConstruction(QemuImg.class)) {
97+
GetVolumesOnStorageAnswer answer = (GetVolumesOnStorageAnswer) wrapper.execute(
98+
new GetVolumesOnStorageCommand(filer, "alpha", null), resource);
99+
assertTrue(answer.getResult());
100+
assertEquals("alpha", answer.getVolumes().get(0).getPath());
101+
assertTrue(qemu.constructed().isEmpty());
102+
}
103+
verifyNoStorageMutation();
104+
}
105+
106+
@Test
107+
public void wrongGroupDoesNotConnectOrDisconnect() {
108+
when(pool.getVolumesForImport("alpha")).thenThrow(new CloudRuntimeException("resource group mismatch"));
109+
Answer answer = wrapper.execute(new GetVolumesOnStorageCommand(filer, "alpha", null), resource);
110+
assertFalse(answer.getResult());
111+
assertEquals("resource group mismatch", answer.getDetails());
112+
verifyNoStorageMutation();
113+
}
114+
115+
@Test
116+
public void controllerFailureDoesNotReturnSuccessfulEmptyList() {
117+
when(pool.getVolumesForImport(null)).thenThrow(new CloudRuntimeException("controller unavailable"));
118+
Answer answer = wrapper.execute(new GetVolumesOnStorageCommand(filer, null, null), resource);
119+
assertFalse(answer.getResult());
120+
assertEquals("controller unavailable", answer.getDetails());
121+
verifyNoStorageMutation();
122+
}
123+
}

0 commit comments

Comments
 (0)