Skip to content

Commit ca5ff34

Browse files
shwstpprdhslove
shwstppr
authored and
dhslove
committed
storage,plugins: delegate allow zone-wide volume migration check and access grant check to storage drivers (apache#8762)
* storage,plugins: delegate allow zone-wide volume migration check and access grant to storage drivers Following checks have been delegated to storage drivers, - For volumes on zone-wide storage, whether they need storage migration when VM is migrated - Whther volume required grant access Apply fixes in resolving PrimaryDataStore * add tests Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com> * unused import Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com> * Update engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java --------- Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
1 parent a5f17a8 commit ca5ff34

9 files changed

Lines changed: 181 additions & 36 deletions

File tree

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,4 +142,19 @@ default boolean requiresAccessForMigration(DataObject dataObject) {
142142
boolean isStorageSupportHA(StoragePoolType type);
143143

144144
void detachVolumeFromAllStorageNodes(Volume volume);
145+
/**
146+
* Data store driver needs its grantAccess() method called for volumes in order for them to be used with a host.
147+
* @return true if we should call grantAccess() to use a volume
148+
*/
149+
default boolean volumesRequireGrantAccessWhenUsed() {
150+
return false;
151+
}
152+
153+
/**
154+
* Zone-wide data store supports using a volume across clusters without the need for data motion
155+
* @return true if we don't need to data motion volumes across clusters for zone-wide use
156+
*/
157+
default boolean zoneWideVolumesAvailableWithoutClusterMotion() {
158+
return false;
159+
}
145160
}

‎engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java‎

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1859,6 +1859,18 @@ private VolumeVO setPassphraseForVolumeEncryption(VolumeVO volume) {
18591859
return _volsDao.persist(volume);
18601860
}
18611861

1862+
protected void grantVolumeAccessToHostIfNeeded(PrimaryDataStore volumeStore, long volumeId, Host host, String volToString) {
1863+
PrimaryDataStoreDriver driver = (PrimaryDataStoreDriver)volumeStore.getDriver();
1864+
if (!driver.volumesRequireGrantAccessWhenUsed()) {
1865+
return;
1866+
}
1867+
try {
1868+
volService.grantAccess(volFactory.getVolume(volumeId), host, volumeStore);
1869+
} catch (Exception e) {
1870+
throw new StorageAccessException(String.format("Unable to grant access to volume [%s] on host [%s].", volToString, host));
1871+
}
1872+
}
1873+
18621874
@Override
18631875
public void prepare(VirtualMachineProfile vm, DeployDestination dest) throws StorageUnavailableException, InsufficientStorageCapacityException, ConcurrentOperationException, StorageAccessException {
18641876
if (dest == null) {
@@ -1876,18 +1888,18 @@ public void prepare(VirtualMachineProfile vm, DeployDestination dest) throws Sto
18761888

18771889
List<VolumeTask> tasks = getTasks(vols, dest.getStorageForDisks(), vm);
18781890
Volume vol = null;
1879-
StoragePool pool;
1891+
PrimaryDataStore store;
18801892
for (VolumeTask task : tasks) {
18811893
if (task.type == VolumeTaskType.NOP) {
18821894
vol = task.volume;
18831895

18841896
String volToString = getReflectOnlySelectedFields(vol);
18851897

1886-
pool = (StoragePool)dataStoreMgr.getDataStore(task.pool.getId(), DataStoreRole.Primary);
1898+
store = (PrimaryDataStore)dataStoreMgr.getDataStore(task.pool.getId(), DataStoreRole.Primary);
18871899

18881900
// For zone-wide managed storage, it is possible that the VM can be started in another
18891901
// cluster. In that case, make sure that the volume is in the right access group.
1890-
if (pool.isManaged()) {
1902+
if (store.isManaged()) {
18911903
Host lastHost = _hostDao.findById(vm.getVirtualMachine().getLastHostId());
18921904
Host host = _hostDao.findById(vm.getVirtualMachine().getHostId());
18931905

@@ -1896,37 +1908,27 @@ public void prepare(VirtualMachineProfile vm, DeployDestination dest) throws Sto
18961908

18971909
if (lastClusterId != clusterId) {
18981910
if (lastHost != null) {
1899-
storageMgr.removeStoragePoolFromCluster(lastHost.getId(), vol.get_iScsiName(), pool);
1900-
1901-
DataStore storagePool = dataStoreMgr.getDataStore(pool.getId(), DataStoreRole.Primary);
1902-
1903-
volService.revokeAccess(volFactory.getVolume(vol.getId()), lastHost, storagePool);
1911+
storageMgr.removeStoragePoolFromCluster(lastHost.getId(), vol.get_iScsiName(), store);
1912+
volService.revokeAccess(volFactory.getVolume(vol.getId()), lastHost, store);
19041913
}
19051914

19061915
try {
1907-
volService.grantAccess(volFactory.getVolume(vol.getId()), host, (DataStore)pool);
1916+
volService.grantAccess(volFactory.getVolume(vol.getId()), host, store);
19081917
} catch (Exception e) {
19091918
throw new StorageAccessException(String.format("Unable to grant access to volume [%s] on host [%s].", volToString, host));
19101919
}
19111920
} else {
1912-
// This might impact other managed storages, grant access for PowerFlex and Iscsi/Solidfire storage pool only
1913-
if (pool.getPoolType() == Storage.StoragePoolType.PowerFlex || pool.getPoolType() == Storage.StoragePoolType.Iscsi) {
1914-
try {
1915-
volService.grantAccess(volFactory.getVolume(vol.getId()), host, (DataStore)pool);
1916-
} catch (Exception e) {
1917-
throw new StorageAccessException(String.format("Unable to grant access to volume [%s] on host [%s].", volToString, host));
1918-
}
1919-
}
1921+
grantVolumeAccessToHostIfNeeded(store, vol.getId(), host, volToString);
19201922
}
19211923
} else {
19221924
handleCheckAndRepairVolume(vol, vm.getVirtualMachine().getHostId());
19231925
}
19241926
} else if (task.type == VolumeTaskType.MIGRATE) {
1925-
pool = (StoragePool)dataStoreMgr.getDataStore(task.pool.getId(), DataStoreRole.Primary);
1926-
vol = migrateVolume(task.volume, pool);
1927+
store = (PrimaryDataStore) dataStoreMgr.getDataStore(task.pool.getId(), DataStoreRole.Primary);
1928+
vol = migrateVolume(task.volume, store);
19271929
} else if (task.type == VolumeTaskType.RECREATE) {
19281930
Pair<VolumeVO, DataStore> result = recreateVolume(task.volume, vm, dest);
1929-
pool = (StoragePool)dataStoreMgr.getDataStore(result.second().getId(), DataStoreRole.Primary);
1931+
store = (PrimaryDataStore) dataStoreMgr.getDataStore(result.second().getId(), DataStoreRole.Primary);
19301932
vol = result.first();
19311933
}
19321934

‎engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,13 @@
1818

1919
import java.util.ArrayList;
2020

21+
import org.apache.cloudstack.engine.subsystem.api.storage.DataObject;
22+
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
23+
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore;
24+
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStoreDriver;
25+
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
26+
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
27+
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeService;
2128
import org.apache.commons.lang3.ObjectUtils;
2229
import org.junit.Assert;
2330
import org.junit.Before;
@@ -31,14 +38,22 @@
3138
import org.mockito.stubbing.Answer;
3239

3340
import com.cloud.configuration.Resource;
41+
import com.cloud.exception.StorageAccessException;
42+
import com.cloud.host.Host;
43+
import com.cloud.host.HostVO;
3444
import com.cloud.storage.VolumeVO;
3545
import com.cloud.user.ResourceLimitService;
46+
import com.cloud.utils.exception.CloudRuntimeException;
3647

3748
@RunWith(MockitoJUnitRunner.class)
3849
public class VolumeOrchestratorTest {
3950

4051
@Mock
4152
protected ResourceLimitService resourceLimitMgr;
53+
@Mock
54+
protected VolumeService volumeService;
55+
@Mock
56+
protected VolumeDataFactory volumeDataFactory;
4257

4358
@Spy
4459
@InjectMocks
@@ -100,4 +115,44 @@ public void testCheckAndUpdateVolumeAccountResourceCountMoreSize() {
100115
public void testCheckAndUpdateVolumeAccountResourceCountLessSize() {
101116
runCheckAndUpdateVolumeAccountResourceCountTest(20L, 10L);
102117
}
118+
119+
@Test
120+
public void testGrantVolumeAccessToHostIfNeededDriverNoNeed() {
121+
PrimaryDataStore store = Mockito.mock(PrimaryDataStore.class);
122+
PrimaryDataStoreDriver driver = Mockito.mock(PrimaryDataStoreDriver.class);
123+
Mockito.when(driver.volumesRequireGrantAccessWhenUsed()).thenReturn(false);
124+
Mockito.when(store.getDriver()).thenReturn(driver);
125+
volumeOrchestrator.grantVolumeAccessToHostIfNeeded(store, 1L,
126+
Mockito.mock(HostVO.class), "");
127+
Mockito.verify(volumeService, Mockito.never())
128+
.grantAccess(Mockito.any(DataObject.class), Mockito.any(Host.class), Mockito.any(DataStore.class));
129+
}
130+
131+
@Test
132+
public void testGrantVolumeAccessToHostIfNeededDriverNeeds() {
133+
PrimaryDataStore store = Mockito.mock(PrimaryDataStore.class);
134+
PrimaryDataStoreDriver driver = Mockito.mock(PrimaryDataStoreDriver.class);
135+
Mockito.when(driver.volumesRequireGrantAccessWhenUsed()).thenReturn(true);
136+
Mockito.when(store.getDriver()).thenReturn(driver);
137+
Mockito.when(volumeDataFactory.getVolume(Mockito.anyLong())).thenReturn(Mockito.mock(VolumeInfo.class));
138+
Mockito.doReturn(true).when(volumeService)
139+
.grantAccess(Mockito.any(DataObject.class), Mockito.any(Host.class), Mockito.any(DataStore.class));
140+
volumeOrchestrator.grantVolumeAccessToHostIfNeeded(store, 1L,
141+
Mockito.mock(HostVO.class), "");
142+
Mockito.verify(volumeService, Mockito.times(1))
143+
.grantAccess(Mockito.any(DataObject.class), Mockito.any(Host.class), Mockito.any(DataStore.class));
144+
}
145+
146+
@Test(expected = StorageAccessException.class)
147+
public void testGrantVolumeAccessToHostIfNeededDriverNeedsButException() {
148+
PrimaryDataStore store = Mockito.mock(PrimaryDataStore.class);
149+
PrimaryDataStoreDriver driver = Mockito.mock(PrimaryDataStoreDriver.class);
150+
Mockito.when(driver.volumesRequireGrantAccessWhenUsed()).thenReturn(true);
151+
Mockito.when(store.getDriver()).thenReturn(driver);
152+
Mockito.when(volumeDataFactory.getVolume(Mockito.anyLong())).thenReturn(Mockito.mock(VolumeInfo.class));
153+
Mockito.doThrow(CloudRuntimeException.class).when(volumeService)
154+
.grantAccess(Mockito.any(DataObject.class), Mockito.any(Host.class), Mockito.any(DataStore.class));
155+
volumeOrchestrator.grantVolumeAccessToHostIfNeeded(store, 1L,
156+
Mockito.mock(HostVO.class), "");
157+
}
103158
}

‎plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/driver/DateraPrimaryDataStoreDriver.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1891,4 +1891,9 @@ public boolean isStorageSupportHA(StoragePoolType type) {
18911891
@Override
18921892
public void detachVolumeFromAllStorageNodes(Volume volume) {
18931893
}
1894+
1895+
@Override
1896+
public boolean volumesRequireGrantAccessWhenUsed() {
1897+
return true;
1898+
}
18941899
}

‎plugins/storage/volume/nexenta/src/main/java/org/apache/cloudstack/storage/datastore/driver/NexentaPrimaryDataStoreDriver.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,4 +269,9 @@ public boolean isStorageSupportHA(StoragePoolType type) {
269269
@Override
270270
public void detachVolumeFromAllStorageNodes(Volume volume) {
271271
}
272+
273+
@Override
274+
public boolean volumesRequireGrantAccessWhenUsed() {
275+
return true;
276+
}
272277
}

‎plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/driver/ScaleIOPrimaryDataStoreDriver.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1431,4 +1431,14 @@ public boolean isStorageSupportHA(StoragePoolType type) {
14311431
@Override
14321432
public void detachVolumeFromAllStorageNodes(Volume volume) {
14331433
}
1434+
1435+
@Override
1436+
public boolean volumesRequireGrantAccessWhenUsed() {
1437+
return true;
1438+
}
1439+
1440+
@Override
1441+
public boolean zoneWideVolumesAvailableWithoutClusterMotion() {
1442+
return true;
1443+
}
14341444
}

‎plugins/storage/volume/solidfire/src/main/java/org/apache/cloudstack/storage/datastore/driver/SolidFirePrimaryDataStoreDriver.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1673,4 +1673,9 @@ public boolean isStorageSupportHA(StoragePoolType type) {
16731673
@Override
16741674
public void detachVolumeFromAllStorageNodes(Volume volume) {
16751675
}
1676+
1677+
@Override
1678+
public boolean volumesRequireGrantAccessWhenUsed() {
1679+
return true;
1680+
}
16761681
}

‎server/src/main/java/com/cloud/server/ManagementServerImpl.java‎

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -609,6 +609,9 @@
609609
import org.apache.cloudstack.config.ConfigurationGroup;
610610
import org.apache.cloudstack.context.CallContext;
611611
import org.apache.cloudstack.engine.orchestration.service.VolumeOrchestrationService;
612+
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
613+
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore;
614+
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStoreDriver;
612615
import org.apache.cloudstack.engine.subsystem.api.storage.StoragePoolAllocator;
613616
import org.apache.cloudstack.framework.config.ConfigDepot;
614617
import org.apache.cloudstack.framework.config.ConfigKey;
@@ -975,6 +978,8 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe
975978
@Inject
976979
private PrimaryDataStoreDao _primaryDataStoreDao;
977980
@Inject
981+
private DataStoreManager dataStoreManager;
982+
@Inject
978983
private VolumeDataStoreDao _volumeStoreDao;
979984
@Inject
980985
private TemplateDataStoreDao _vmTemplateStoreDao;
@@ -1470,6 +1475,20 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
14701475
return listHostsForMigrationOfVM(vm, startIndex, pageSize, keyword, Collections.emptyList());
14711476
}
14721477

1478+
protected boolean zoneWideVolumeRequiresStorageMotion(PrimaryDataStore volumeDataStore,
1479+
final Host sourceHost, final Host destinationHost) {
1480+
if (volumeDataStore.isManaged() && sourceHost.getClusterId() != destinationHost.getClusterId()) {
1481+
PrimaryDataStoreDriver driver = (PrimaryDataStoreDriver)volumeDataStore.getDriver();
1482+
// Depends on the storage driver. For some storages simply
1483+
// changing volume access to host should work: grant access on destination
1484+
// host and revoke access on source host. For others, we still have to perform a storage migration
1485+
// because we need to create a new target volume and copy the contents of the
1486+
// source volume into it before deleting the source volume.
1487+
return !driver.zoneWideVolumesAvailableWithoutClusterMotion();
1488+
}
1489+
return false;
1490+
}
1491+
14731492
@Override
14741493
public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boolean>> listHostsForMigrationOfVM(final VirtualMachine vm, final Long startIndex, final Long pageSize,
14751494
final String keyword, List<VirtualMachine> vmList) {
@@ -1538,8 +1557,8 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
15381557
filteredHosts = new ArrayList<>(allHosts);
15391558

15401559
for (final VolumeVO volume : volumes) {
1541-
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
1542-
Long volClusterId = storagePool.getClusterId();
1560+
PrimaryDataStore primaryDataStore = (PrimaryDataStore)dataStoreManager.getPrimaryDataStore(volume.getPoolId());
1561+
Long volClusterId = primaryDataStore.getClusterId();
15431562

15441563
for (Iterator<HostVO> iterator = filteredHosts.iterator(); iterator.hasNext();) {
15451564
final Host host = iterator.next();
@@ -1549,8 +1568,8 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
15491568
}
15501569

15511570
if (volClusterId != null) {
1552-
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
1553-
if (storagePool.isManaged()) {
1571+
if (primaryDataStore.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
1572+
if (primaryDataStore.isManaged()) {
15541573
// At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
15551574
// is at the zone level and the source and target storage pool is the same.
15561575
// If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
@@ -1568,18 +1587,8 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
15681587
}
15691588
}
15701589
} else {
1571-
if (storagePool.isManaged()) {
1572-
if (srcHost.getClusterId() != host.getClusterId()) {
1573-
if (storagePool.getPoolType() == Storage.StoragePoolType.PowerFlex) {
1574-
// No need of new volume creation for zone wide PowerFlex/ScaleIO pool
1575-
// Simply, changing volume access to host should work: grant access on dest host and revoke access on source host
1576-
continue;
1577-
}
1578-
// If the volume's storage pool is managed and at the zone level, then we still have to perform a storage migration
1579-
// because we need to create a new target volume and copy the contents of the source volume into it before deleting
1580-
// the source volume.
1581-
requiresStorageMotion.put(host, true);
1582-
}
1590+
if (zoneWideVolumeRequiresStorageMotion(primaryDataStore, srcHost, host)) {
1591+
requiresStorageMotion.put(host, true);
15831592
}
15841593
}
15851594
}

‎server/src/test/java/com/cloud/server/ManagementServerImplTest.java‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,8 @@
5454
import org.apache.cloudstack.api.command.user.userdata.ListUserDataCmd;
5555
import org.apache.cloudstack.api.command.user.userdata.RegisterUserDataCmd;
5656
import org.apache.cloudstack.context.CallContext;
57+
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore;
58+
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStoreDriver;
5759
import org.apache.cloudstack.framework.config.ConfigKey;
5860
import org.apache.cloudstack.userdata.UserDataManager;
5961
import org.junit.After;
@@ -645,4 +647,41 @@ public void testFilterUefiHostsForMigrationUefiVMNoHostsAvailableFiltered() {
645647
Assert.assertNotNull(result.second());
646648
Assert.assertEquals(0, result.second().size());
647649
}
650+
651+
@Test
652+
public void testZoneWideVolumeRequiresStorageMotionNonManaged() {
653+
PrimaryDataStore dataStore = Mockito.mock(PrimaryDataStore.class);
654+
Mockito.when(dataStore.isManaged()).thenReturn(false);
655+
Assert.assertFalse(spy.zoneWideVolumeRequiresStorageMotion(dataStore,
656+
Mockito.mock(Host.class), Mockito.mock(Host.class)));
657+
}
658+
659+
@Test
660+
public void testZoneWideVolumeRequiresStorageMotionSameClusterHost() {
661+
PrimaryDataStore dataStore = Mockito.mock(PrimaryDataStore.class);
662+
Mockito.when(dataStore.isManaged()).thenReturn(true);
663+
Host host1 = Mockito.mock(Host.class);
664+
Mockito.when(host1.getClusterId()).thenReturn(1L);
665+
Host host2 = Mockito.mock(Host.class);
666+
Mockito.when(host2.getClusterId()).thenReturn(1L);
667+
Assert.assertFalse(spy.zoneWideVolumeRequiresStorageMotion(dataStore, host1, host2));
668+
}
669+
670+
@Test
671+
public void testZoneWideVolumeRequiresStorageMotionDriverDependent() {
672+
PrimaryDataStore dataStore = Mockito.mock(PrimaryDataStore.class);
673+
Mockito.when(dataStore.isManaged()).thenReturn(true);
674+
PrimaryDataStoreDriver driver = Mockito.mock(PrimaryDataStoreDriver.class);
675+
Mockito.when(dataStore.getDriver()).thenReturn(driver);
676+
Host host1 = Mockito.mock(Host.class);
677+
Mockito.when(host1.getClusterId()).thenReturn(1L);
678+
Host host2 = Mockito.mock(Host.class);
679+
Mockito.when(host2.getClusterId()).thenReturn(2L);
680+
681+
Mockito.when(driver.zoneWideVolumesAvailableWithoutClusterMotion()).thenReturn(true);
682+
Assert.assertFalse(spy.zoneWideVolumeRequiresStorageMotion(dataStore, host1, host2));
683+
684+
Mockito.when(driver.zoneWideVolumesAvailableWithoutClusterMotion()).thenReturn(false);
685+
Assert.assertTrue(spy.zoneWideVolumeRequiresStorageMotion(dataStore, host1, host2));
686+
}
648687
}

0 commit comments

Comments
 (0)