Skip to content

Commit ffd5972

Browse files
shwstpprshwstppr
authored
storage,plugins: delegate allow zone-wide volume migration check and access grant check to storage drivers (#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 9acba90 commit ffd5972

File tree

9 files changed

+184
-39
lines changed

9 files changed

+184
-39
lines changed

‎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
@@ -1856,6 +1856,18 @@ private VolumeVO setPassphraseForVolumeEncryption(VolumeVO volume) {
18561856
return _volsDao.persist(volume);
18571857
}
18581858

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

18741886
List<VolumeTask> tasks = getTasks(vols, dest.getStorageForDisks(), vm);
18751887
Volume vol = null;
1876-
StoragePool pool;
1888+
PrimaryDataStore store;
18771889
for (VolumeTask task : tasks) {
18781890
if (task.type == VolumeTaskType.NOP) {
18791891
vol = task.volume;
18801892

18811893
String volToString = getReflectOnlySelectedFields(vol);
18821894

1883-
pool = (StoragePool)dataStoreMgr.getDataStore(task.pool.getId(), DataStoreRole.Primary);
1895+
store = (PrimaryDataStore)dataStoreMgr.getDataStore(task.pool.getId(), DataStoreRole.Primary);
18841896

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

@@ -1893,37 +1905,27 @@ public void prepare(VirtualMachineProfile vm, DeployDestination dest) throws Sto
18931905

18941906
if (lastClusterId != clusterId) {
18951907
if (lastHost != null) {
1896-
storageMgr.removeStoragePoolFromCluster(lastHost.getId(), vol.get_iScsiName(), pool);
1897-
1898-
DataStore storagePool = dataStoreMgr.getDataStore(pool.getId(), DataStoreRole.Primary);
1899-
1900-
volService.revokeAccess(volFactory.getVolume(vol.getId()), lastHost, storagePool);
1908+
storageMgr.removeStoragePoolFromCluster(lastHost.getId(), vol.get_iScsiName(), store);
1909+
volService.revokeAccess(volFactory.getVolume(vol.getId()), lastHost, store);
19011910
}
19021911

19031912
try {
1904-
volService.grantAccess(volFactory.getVolume(vol.getId()), host, (DataStore)pool);
1913+
volService.grantAccess(volFactory.getVolume(vol.getId()), host, store);
19051914
} catch (Exception e) {
19061915
throw new StorageAccessException(String.format("Unable to grant access to volume [%s] on host [%s].", volToString, host));
19071916
}
19081917
} else {
1909-
// This might impact other managed storages, grant access for PowerFlex and Iscsi/Solidfire storage pool only
1910-
if (pool.getPoolType() == Storage.StoragePoolType.PowerFlex || pool.getPoolType() == Storage.StoragePoolType.Iscsi) {
1911-
try {
1912-
volService.grantAccess(volFactory.getVolume(vol.getId()), host, (DataStore)pool);
1913-
} catch (Exception e) {
1914-
throw new StorageAccessException(String.format("Unable to grant access to volume [%s] on host [%s].", volToString, host));
1915-
}
1916-
}
1918+
grantVolumeAccessToHostIfNeeded(store, vol.getId(), host, volToString);
19171919
}
19181920
} else {
19191921
handleCheckAndRepairVolume(vol, vm.getVirtualMachine().getHostId());
19201922
}
19211923
} else if (task.type == VolumeTaskType.MIGRATE) {
1922-
pool = (StoragePool)dataStoreMgr.getDataStore(task.pool.getId(), DataStoreRole.Primary);
1923-
vol = migrateVolume(task.volume, pool);
1924+
store = (PrimaryDataStore) dataStoreMgr.getDataStore(task.pool.getId(), DataStoreRole.Primary);
1925+
vol = migrateVolume(task.volume, store);
19241926
} else if (task.type == VolumeTaskType.RECREATE) {
19251927
Pair<VolumeVO, DataStore> result = recreateVolume(task.volume, vm, dest);
1926-
pool = (StoragePool)dataStoreMgr.getDataStore(result.second().getId(), DataStoreRole.Primary);
1928+
store = (PrimaryDataStore) dataStoreMgr.getDataStore(result.second().getId(), DataStoreRole.Primary);
19271929
vol = result.first();
19281930
}
19291931

‎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
@@ -1890,4 +1890,9 @@ public boolean isStorageSupportHA(StoragePoolType type) {
18901890
@Override
18911891
public void detachVolumeFromAllStorageNodes(Volume volume) {
18921892
}
1893+
1894+
@Override
1895+
public boolean volumesRequireGrantAccessWhenUsed() {
1896+
return true;
1897+
}
18931898
}

‎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
@@ -268,4 +268,9 @@ public boolean isStorageSupportHA(StoragePoolType type) {
268268
@Override
269269
public void detachVolumeFromAllStorageNodes(Volume volume) {
270270
}
271+
272+
@Override
273+
public boolean volumesRequireGrantAccessWhenUsed() {
274+
return true;
275+
}
271276
}

‎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
@@ -1430,4 +1430,14 @@ public boolean isStorageSupportHA(StoragePoolType type) {
14301430
@Override
14311431
public void detachVolumeFromAllStorageNodes(Volume volume) {
14321432
}
1433+
1434+
@Override
1435+
public boolean volumesRequireGrantAccessWhenUsed() {
1436+
return true;
1437+
}
1438+
1439+
@Override
1440+
public boolean zoneWideVolumesAvailableWithoutClusterMotion() {
1441+
return true;
1442+
}
14331443
}

‎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
@@ -1672,4 +1672,9 @@ public boolean isStorageSupportHA(StoragePoolType type) {
16721672
@Override
16731673
public void detachVolumeFromAllStorageNodes(Volume volume) {
16741674
}
1675+
1676+
@Override
1677+
public boolean volumesRequireGrantAccessWhenUsed() {
1678+
return true;
1679+
}
16751680
}

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

Lines changed: 28 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,6 @@
4343
import javax.inject.Inject;
4444
import javax.naming.ConfigurationException;
4545

46-
import com.cloud.network.dao.PublicIpQuarantineDao;
47-
import com.cloud.hypervisor.HypervisorGuru;
4846
import org.apache.cloudstack.acl.ControlledEntity;
4947
import org.apache.cloudstack.acl.SecurityChecker;
5048
import org.apache.cloudstack.affinity.AffinityGroupProcessor;
@@ -225,8 +223,8 @@
225223
import org.apache.cloudstack.api.command.admin.storage.ListStoragePoolsCmd;
226224
import org.apache.cloudstack.api.command.admin.storage.ListStorageProvidersCmd;
227225
import org.apache.cloudstack.api.command.admin.storage.ListStorageTagsCmd;
228-
import org.apache.cloudstack.api.command.admin.storage.MigrateSecondaryStorageDataCmd;
229226
import org.apache.cloudstack.api.command.admin.storage.MigrateResourcesToAnotherSecondaryStorageCmd;
227+
import org.apache.cloudstack.api.command.admin.storage.MigrateSecondaryStorageDataCmd;
230228
import org.apache.cloudstack.api.command.admin.storage.PreparePrimaryStorageForMaintenanceCmd;
231229
import org.apache.cloudstack.api.command.admin.storage.SyncStoragePoolCmd;
232230
import org.apache.cloudstack.api.command.admin.storage.UpdateCloudToUseObjectStoreCmd;
@@ -606,6 +604,9 @@
606604
import org.apache.cloudstack.config.ConfigurationGroup;
607605
import org.apache.cloudstack.context.CallContext;
608606
import org.apache.cloudstack.engine.orchestration.service.VolumeOrchestrationService;
607+
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
608+
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore;
609+
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStoreDriver;
609610
import org.apache.cloudstack.engine.subsystem.api.storage.StoragePoolAllocator;
610611
import org.apache.cloudstack.framework.config.ConfigDepot;
611612
import org.apache.cloudstack.framework.config.ConfigKey;
@@ -717,6 +718,7 @@
717718
import com.cloud.hypervisor.Hypervisor.HypervisorType;
718719
import com.cloud.hypervisor.HypervisorCapabilities;
719720
import com.cloud.hypervisor.HypervisorCapabilitiesVO;
721+
import com.cloud.hypervisor.HypervisorGuru;
720722
import com.cloud.hypervisor.dao.HypervisorCapabilitiesDao;
721723
import com.cloud.hypervisor.kvm.dpdk.DpdkHelper;
722724
import com.cloud.info.ConsoleProxyInfo;
@@ -736,6 +738,7 @@
736738
import com.cloud.network.dao.NetworkDomainDao;
737739
import com.cloud.network.dao.NetworkDomainVO;
738740
import com.cloud.network.dao.NetworkVO;
741+
import com.cloud.network.dao.PublicIpQuarantineDao;
739742
import com.cloud.network.vpc.dao.VpcDao;
740743
import com.cloud.org.Cluster;
741744
import com.cloud.org.Grouping.AllocationState;
@@ -967,6 +970,8 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe
967970
@Inject
968971
private PrimaryDataStoreDao _primaryDataStoreDao;
969972
@Inject
973+
private DataStoreManager dataStoreManager;
974+
@Inject
970975
private VolumeDataStoreDao _volumeStoreDao;
971976
@Inject
972977
private TemplateDataStoreDao _vmTemplateStoreDao;
@@ -1414,6 +1419,20 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
14141419
return listHostsForMigrationOfVM(vm, startIndex, pageSize, keyword, Collections.emptyList());
14151420
}
14161421

1422+
protected boolean zoneWideVolumeRequiresStorageMotion(PrimaryDataStore volumeDataStore,
1423+
final Host sourceHost, final Host destinationHost) {
1424+
if (volumeDataStore.isManaged() && sourceHost.getClusterId() != destinationHost.getClusterId()) {
1425+
PrimaryDataStoreDriver driver = (PrimaryDataStoreDriver)volumeDataStore.getDriver();
1426+
// Depends on the storage driver. For some storages simply
1427+
// changing volume access to host should work: grant access on destination
1428+
// host and revoke access on source host. For others, we still have to perform a storage migration
1429+
// because we need to create a new target volume and copy the contents of the
1430+
// source volume into it before deleting the source volume.
1431+
return !driver.zoneWideVolumesAvailableWithoutClusterMotion();
1432+
}
1433+
return false;
1434+
}
1435+
14171436
@Override
14181437
public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Host, Boolean>> listHostsForMigrationOfVM(final VirtualMachine vm, final Long startIndex, final Long pageSize,
14191438
final String keyword, List<VirtualMachine> vmList) {
@@ -1482,8 +1501,8 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
14821501
filteredHosts = new ArrayList<>(allHosts);
14831502

14841503
for (final VolumeVO volume : volumes) {
1485-
StoragePool storagePool = _poolDao.findById(volume.getPoolId());
1486-
Long volClusterId = storagePool.getClusterId();
1504+
PrimaryDataStore primaryDataStore = (PrimaryDataStore)dataStoreManager.getPrimaryDataStore(volume.getPoolId());
1505+
Long volClusterId = primaryDataStore.getClusterId();
14871506

14881507
for (Iterator<HostVO> iterator = filteredHosts.iterator(); iterator.hasNext();) {
14891508
final Host host = iterator.next();
@@ -1493,8 +1512,8 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
14931512
}
14941513

14951514
if (volClusterId != null) {
1496-
if (storagePool.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
1497-
if (storagePool.isManaged()) {
1515+
if (primaryDataStore.isLocal() || !host.getClusterId().equals(volClusterId) || usesLocal) {
1516+
if (primaryDataStore.isManaged()) {
14981517
// At the time being, we do not support storage migration of a volume from managed storage unless the managed storage
14991518
// is at the zone level and the source and target storage pool is the same.
15001519
// If the source and target storage pool is the same and it is managed, then we still have to perform a storage migration
@@ -1512,18 +1531,8 @@ public Ternary<Pair<List<? extends Host>, Integer>, List<? extends Host>, Map<Ho
15121531
}
15131532
}
15141533
} else {
1515-
if (storagePool.isManaged()) {
1516-
if (srcHost.getClusterId() != host.getClusterId()) {
1517-
if (storagePool.getPoolType() == Storage.StoragePoolType.PowerFlex) {
1518-
// No need of new volume creation for zone wide PowerFlex/ScaleIO pool
1519-
// Simply, changing volume access to host should work: grant access on dest host and revoke access on source host
1520-
continue;
1521-
}
1522-
// If the volume's storage pool is managed and at the zone level, then we still have to perform a storage migration
1523-
// because we need to create a new target volume and copy the contents of the source volume into it before deleting
1524-
// the source volume.
1525-
requiresStorageMotion.put(host, true);
1526-
}
1534+
if (zoneWideVolumeRequiresStorageMotion(primaryDataStore, srcHost, host)) {
1535+
requiresStorageMotion.put(host, true);
15271536
}
15281537
}
15291538
}

0 commit comments

Comments
 (0)