vishesh92 commented on code in PR #8628:
URL: https://github.com/apache/cloudstack/pull/8628#discussion_r1492268094
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/provider/ScaleIOHostListener.java:
##########
@@ -70,12 +71,33 @@ public boolean hostAdded(long hostId) {
public boolean hostConnect(long hostId, long poolId) {
HostVO host = _hostDao.findById(hostId);
if (host == null) {
- logger.error("Failed to add host by HostListener as host was not
found with id : " + hostId);
+ logger.error("Failed to connect host by ScaleIOHostListener as
host was not found with id : " + hostId);
return false;
}
StoragePool storagePool =
(StoragePool)_dataStoreMgr.getDataStore(poolId, DataStoreRole.Primary);
+ Pair<String, String> sdcDetails = getConnectedSdcIdOfHost(host,
storagePool, true);
+ String sdcId = sdcDetails.first();
+ if (sdcId == null) {
+ return false;
+ }
+ StoragePoolHostVO storagePoolHost =
_storagePoolHostDao.findByPoolHost(poolId, hostId);
+ if (storagePoolHost == null) {
+ storagePoolHost = new StoragePoolHostVO(poolId, hostId, sdcId);
+ _storagePoolHostDao.persist(storagePoolHost);
+ } else {
+ storagePoolHost.setLocalPath(sdcId);
+ _storagePoolHostDao.update(storagePoolHost.getId(),
storagePoolHost);
+ }
+
+ logger.info("Connection established between PowerFlex storage pool: "
+ storagePool + " and host: " + hostId);
+ return true;
+ }
+
+ private Pair<String, String> getConnectedSdcIdOfHost(HostVO host,
StoragePool storagePool, boolean alert) {
Review Comment:
We don't seem to be using the message being returned as part of the Pair. We
can just return the SdcId.
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/provider/ScaleIOHostListener.java:
##########
@@ -70,12 +71,33 @@ public boolean hostAdded(long hostId) {
public boolean hostConnect(long hostId, long poolId) {
HostVO host = _hostDao.findById(hostId);
if (host == null) {
- logger.error("Failed to add host by HostListener as host was not
found with id : " + hostId);
+ logger.error("Failed to connect host by ScaleIOHostListener as
host was not found with id : " + hostId);
return false;
}
StoragePool storagePool =
(StoragePool)_dataStoreMgr.getDataStore(poolId, DataStoreRole.Primary);
+ Pair<String, String> sdcDetails = getConnectedSdcIdOfHost(host,
storagePool, true);
+ String sdcId = sdcDetails.first();
+ if (sdcId == null) {
+ return false;
+ }
+ StoragePoolHostVO storagePoolHost =
_storagePoolHostDao.findByPoolHost(poolId, hostId);
+ if (storagePoolHost == null) {
+ storagePoolHost = new StoragePoolHostVO(poolId, hostId, sdcId);
+ _storagePoolHostDao.persist(storagePoolHost);
+ } else {
+ storagePoolHost.setLocalPath(sdcId);
+ _storagePoolHostDao.update(storagePoolHost.getId(),
storagePoolHost);
+ }
+
+ logger.info("Connection established between PowerFlex storage pool: "
+ storagePool + " and host: " + hostId);
+ return true;
+ }
+
+ private Pair<String, String> getConnectedSdcIdOfHost(HostVO host,
StoragePool storagePool, boolean alert) {
Review Comment:
alert is always true as well.
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ScaleIOStoragePoolTest.java:
##########
@@ -87,49 +104,27 @@ public void testAttributes() {
@Test
public void testSdcIdAttribute() {
- final String uuid = "345fc603-2d7e-47d2-b719-a0110b3732e6";
- final String systemId = "218ce1797566a00f";
- final String sdcId = "301b852c00000003";
- final StoragePoolType type = StoragePoolType.PowerFlex;
- Map<String,String> details = new HashMap<String, String>();
- details.put(ScaleIOGatewayClient.STORAGE_POOL_SYSTEM_ID, systemId);
-
- try (MockedStatic<Script> ignored = Mockito.mockStatic(Script.class)) {
- when(Script.runSimpleBashScript(
- "/opt/emc/scaleio/sdc/bin/drv_cfg --query_mdms|grep
218ce1797566a00f|awk '{print $5}'")).thenReturn(
- sdcId);
-
- ScaleIOStoragePool pool1 = new ScaleIOStoragePool(uuid,
"192.168.1.19", 443, "a519be2f00000000", type,
- details, adapter);
- assertEquals(systemId,
pool1.getDetails().get(ScaleIOGatewayClient.STORAGE_POOL_SYSTEM_ID));
- assertEquals(sdcId,
pool1.getDetails().get(ScaleIOGatewayClient.SDC_ID));
- }
+ when(Script.runSimpleBashScript("/opt/emc/scaleio/sdc/bin/drv_cfg
--query_mdms|grep " + systemId + "|awk '{print $5}'")).thenReturn(sdcId);
+ ScaleIOStoragePool pool1 = new ScaleIOStoragePool(uuid,
"192.168.1.19", 443, "a519be2f00000000", type, details, adapter);
+ assertEquals(systemId,
pool1.getDetails().get(ScaleIOGatewayClient.STORAGE_POOL_SYSTEM_ID));
+ assertEquals(sdcId,
pool1.getDetails().get(ScaleIOGatewayClient.SDC_ID));
}
@Test
public void testSdcGuidAttribute() {
- final String uuid = "345fc603-2d7e-47d2-b719-a0110b3732e6";
- final String systemId = "218ce1797566a00f";
final String sdcGuid = "B0E3BFB8-C20B-43BF-93C8-13339E85AA50";
- final StoragePoolType type = StoragePoolType.PowerFlex;
- Map<String,String> details = new HashMap<String, String>();
- details.put(ScaleIOGatewayClient.STORAGE_POOL_SYSTEM_ID, systemId);
-
- try (MockedStatic<Script> ignored = Mockito.mockStatic(Script.class)) {
- when(Script.runSimpleBashScript(
- "/opt/emc/scaleio/sdc/bin/drv_cfg --query_mdms|grep
218ce1797566a00f|awk '{print $5}'")).thenReturn(
- null);
- when(Script.runSimpleBashScript("/opt/emc/scaleio/sdc/bin/drv_cfg
--query_guid")).thenReturn(sdcGuid);
- ScaleIOStoragePool pool1 = new ScaleIOStoragePool(uuid,
"192.168.1.19", 443, "a519be2f00000000", type,
- details, adapter);
- assertEquals(systemId,
pool1.getDetails().get(ScaleIOGatewayClient.STORAGE_POOL_SYSTEM_ID));
- assertEquals(sdcGuid,
pool1.getDetails().get(ScaleIOGatewayClient.SDC_GUID));
- }
+ when(Script.runSimpleBashScript("/opt/emc/scaleio/sdc/bin/drv_cfg
--query_mdms|grep" + systemId + "|awk '{print $5}'")).thenReturn(null);
Review Comment:
```suggestion
when(Script.runSimpleBashScript("/opt/emc/scaleio/sdc/bin/drv_cfg
--query_mdms|grep " + systemId + " |awk '{print $5}'")).thenReturn(null);
```
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ScaleIOStoragePoolTest.java:
##########
@@ -140,55 +135,173 @@ public void testDefaults() {
assertTrue(pool.isExternalSnapshot());
}
- public void testGetPhysicalDiskWithWildcardFileFilter() throws Exception {
+ @Test
+ public void testGetPhysicalDiskWithWildcardFileFilterNoFiles() {
+ when(Script.runSimpleBashScript("/opt/emc/scaleio/sdc/bin/drv_cfg
--query_mdms|grep " + systemId + "|awk '{print $5}'")).thenReturn(sdcId);
+ ScaleIOStoragePool pool = new ScaleIOStoragePool(uuid, "192.168.1.19",
443, "a519be2f00000000", type, details, adapter);
final String volumePath = "6c3362b500000001:vol-139-3d2c-12f0";
- final String systemId = "218ce1797566a00f";
-
- // TODO: Mock file in dir
try (MockedConstruction<File> ignored =
Mockito.mockConstruction(File.class, (mock, context) -> {
+
Mockito.when(mock.listFiles(any(FileFilter.class))).thenReturn(null);
+ })) {
+ KVMPhysicalDisk disk = adapter.getPhysicalDisk(volumePath, pool);
+ assertNull(disk);
+ }
+ }
+
+ @Test
+ public void testGetPhysicalDiskWithWildcardFileFilterFilesExists() {
+ when(Script.runSimpleBashScript("/opt/emc/scaleio/sdc/bin/drv_cfg
--query_mdms|grep " + systemId + "|awk '{print $5}'")).thenReturn(sdcId);
+ ScaleIOStoragePool pool = new ScaleIOStoragePool(uuid, "192.168.1.19",
443, "a519be2f00000000", type, details, adapter);
+ final String volumePath = "6c3362b500000001:vol-139-3d2c-12f0";
+ OutputInterpreter.OneLineParser spyParser = spy(new
OutputInterpreter.OneLineParser());
+
+ try (MockedConstruction<QemuImg> ignored =
Mockito.mockConstruction(QemuImg.class, (mockQemuImg, contextQemuImg) -> {
+ Map<String, String> details = new HashMap<>();
+
Mockito.doReturn(details).when(mockQemuImg).info(any(QemuImgFile.class));
+ }); MockedConstruction<File> ignored1 =
Mockito.mockConstruction(File.class, (mockFile, contextFile) -> {
File[] files = new File[1];
String volumeId = ScaleIOUtil.getVolumePath(volumePath);
- String diskFilePath =
- ScaleIOUtil.DISK_PATH + File.separator +
ScaleIOUtil.DISK_NAME_PREFIX + systemId + "-" + volumeId;
+ String diskFilePath = ScaleIOUtil.DISK_PATH + File.separator +
ScaleIOUtil.DISK_NAME_PREFIX + systemId + "-" + volumeId;
files[0] = new File(diskFilePath);
-
Mockito.when(mock.listFiles(any(FileFilter.class))).thenReturn(files);
+
Mockito.when(mockFile.listFiles(any(FileFilter.class))).thenReturn(files);
+ }); MockedConstruction<WildcardFileFilter> ignored2 =
Mockito.mockConstruction(WildcardFileFilter.class); MockedConstruction<Script>
ignored3 = Mockito.mockConstruction(Script.class, (mockScript, contextScript)
-> {
+
Mockito.doReturn(null).when(mockScript).execute(any(OutputInterpreter.class));
+ }); MockedConstruction<OutputInterpreter.OneLineParser> ignored4 =
Mockito.mockConstruction(OutputInterpreter.OneLineParser.class,
withSettings().spiedInstance(spyParser), (mockParser, contextParser) -> {
+ Mockito.doReturn("8192").when(mockParser).getLine();
})) {
KVMPhysicalDisk disk = adapter.getPhysicalDisk(volumePath, pool);
- assertNull(disk);
+ assertNotNull(disk);
+ assertEquals("/dev/disk/by-id/emc-vol-" + systemId +
"-6c3362b500000001", disk.getPath());
+ assertEquals(PhysicalDiskFormat.RAW, disk.getFormat());
+ assertEquals(8192, disk.getSize());
+ assertEquals(8192, disk.getVirtualSize());
}
}
+ @Test
+ public void
testGetPhysicalDiskWithWildcardFileFilterFilesExistsWithDetails() {
+ when(Script.runSimpleBashScript("/opt/emc/scaleio/sdc/bin/drv_cfg
--query_mdms|grep " + systemId + "|awk '{print $5}'")).thenReturn(sdcId);
+ ScaleIOStoragePool pool = new ScaleIOStoragePool(uuid, "192.168.1.19",
443, "a519be2f00000000", type, details, adapter);
+ final String volumePath = "6c3362b500000001:vol-139-3d2c-12f0";
+ OutputInterpreter.OneLineParser spyParser = spy(new
OutputInterpreter.OneLineParser());
+
+ try (MockedConstruction<QemuImg> ignored =
Mockito.mockConstruction(QemuImg.class, (mockQemuImg, contextQemuImg) -> {
+ Map<String, String> details = new HashMap<>();
+ details.put(QemuImg.VIRTUAL_SIZE, "16384");
+ details.put(QemuImg.FILE_FORMAT,
PhysicalDiskFormat.QCOW2.toString());
+
Mockito.doReturn(details).when(mockQemuImg).info(any(QemuImgFile.class));
+ }); MockedConstruction<File> ignored1 =
Mockito.mockConstruction(File.class, (mockFile, contextFile) -> {
+ File[] files = new File[1];
+ String volumeId = ScaleIOUtil.getVolumePath(volumePath);
+ String diskFilePath = ScaleIOUtil.DISK_PATH + File.separator +
ScaleIOUtil.DISK_NAME_PREFIX + systemId + "-" + volumeId;
+ files[0] = new File(diskFilePath);
+
Mockito.when(mockFile.listFiles(any(FileFilter.class))).thenReturn(files);
+ }); MockedConstruction<WildcardFileFilter> ignored2 =
Mockito.mockConstruction(WildcardFileFilter.class); MockedConstruction<Script>
ignored3 = Mockito.mockConstruction(Script.class, (mockScript, contextScript)
-> {
+
Mockito.doReturn(null).when(mockScript).execute(any(OutputInterpreter.class));
+ }); MockedConstruction<OutputInterpreter.OneLineParser> ignored4 =
Mockito.mockConstruction(OutputInterpreter.OneLineParser.class,
withSettings().spiedInstance(spyParser), (mockParser, contextParser) -> {
+ Mockito.doReturn("16384").when(mockParser).getLine();
+ })) {
+ KVMPhysicalDisk disk = adapter.getPhysicalDisk(volumePath, pool);
+ assertNotNull(disk);
+ assertEquals("/dev/disk/by-id/emc-vol-" + systemId +
"-6c3362b500000001", disk.getPath());
+ assertEquals(PhysicalDiskFormat.QCOW2, disk.getFormat());
+ assertEquals(16384, disk.getSize());
+ assertEquals(16384, disk.getVirtualSize());
+ }
+ }
@Test
- public void testGetPhysicalDiskWithSystemId() throws Exception {
+ public void testGetPhysicalDiskWithSystemIdNoFile() {
+ when(Script.runSimpleBashScript("/opt/emc/scaleio/sdc/bin/drv_cfg
--query_mdms|grep " + systemId + "|awk '{print $5}'")).thenReturn(sdcId);
+ ScaleIOStoragePool pool = new ScaleIOStoragePool(uuid, "192.168.1.19",
443, "a519be2f00000000", type, details, adapter);
final String volumePath = "6c3362b500000001:vol-139-3d2c-12f0";
final String volumeId = ScaleIOUtil.getVolumePath(volumePath);
- final String systemId = "218ce1797566a00f";
- try (MockedStatic<ScaleIOUtil> ignored =
Mockito.mockStatic(ScaleIOUtil.class, Mockito.CALLS_REAL_METHODS)) {
+ try (MockedStatic<ScaleIOUtil> ignored =
Mockito.mockStatic(ScaleIOUtil.class, Mockito.CALLS_REAL_METHODS);
MockedConstruction<File> ignored1 = Mockito.mockConstruction(File.class, (mock,
context) -> {
+ Mockito.when(mock.exists()).thenReturn(false);
+ })) {
when(ScaleIOUtil.getSystemIdForVolume(volumeId)).thenReturn(systemId);
- // TODO: Mock file exists
-
KVMPhysicalDisk disk = adapter.getPhysicalDisk(volumePath, pool);
assertNull(disk);
}
}
+ @Test
+ public void testGetPhysicalDiskWithSystemIdFileExists() {
+ when(Script.runSimpleBashScript("/opt/emc/scaleio/sdc/bin/drv_cfg
--query_mdms|grep " + systemId + "|awk '{print $5}'")).thenReturn(sdcId);
+ ScaleIOStoragePool pool = new ScaleIOStoragePool(uuid, "192.168.1.19",
443, "a519be2f00000000", type, details, adapter);
+ final String volumePath = "6c3362b500000001:vol-139-3d2c-12f0";
+ final String volumeId = "6c3362b500000001";
+ OutputInterpreter.OneLineParser spyParser = spy(new
OutputInterpreter.OneLineParser());
+
+ try (MockedConstruction<QemuImg> ignored =
Mockito.mockConstruction(QemuImg.class, (mockQemuImg, contextQemuImg) -> {
+ Map<String, String> details = new HashMap<>();
+
Mockito.doReturn(details).when(mockQemuImg).info(any(QemuImgFile.class));
+ }); MockedConstruction<File> ignored1 =
Mockito.mockConstruction(File.class, (mockFile, contextFile) -> {
+ Mockito.when(mockFile.exists()).thenReturn(true);
+ }); MockedConstruction<Script> ignored2 =
Mockito.mockConstruction(Script.class, (mockScript, contextScript) -> {
+
Mockito.doReturn(null).when(mockScript).execute(any(OutputInterpreter.class));
+ }); MockedConstruction<OutputInterpreter.OneLineParser> ignored3 =
Mockito.mockConstruction(OutputInterpreter.OneLineParser.class,
withSettings().spiedInstance(spyParser), (mockParser, contextParser) -> {
+ Mockito.doReturn("8192").when(mockParser).getLine();
+ }); MockedStatic<ScaleIOUtil> ignored5 =
Mockito.mockStatic(ScaleIOUtil.class, Mockito.CALLS_REAL_METHODS)) {
+
when(ScaleIOUtil.getSystemIdForVolume(volumeId)).thenReturn(systemId);
+
+ KVMPhysicalDisk disk = adapter.getPhysicalDisk(volumePath, pool);
+ assertNotNull(disk);
+ assertEquals("/dev/disk/by-id/emc-vol-" + systemId +
"-6c3362b500000001", disk.getPath());
+ assertEquals(PhysicalDiskFormat.RAW, disk.getFormat());
+ assertEquals(8192, disk.getSize());
+ assertEquals(8192, disk.getVirtualSize());
+ }
+ }
+
+ @Test
+ public void testGetPhysicalDiskWithSystemIdFileExistsWithDetails() {
+ when(Script.runSimpleBashScript("/opt/emc/scaleio/sdc/bin/drv_cfg
--query_mdms|grep " + systemId + "|awk '{print $5}'")).thenReturn(sdcId);
+ ScaleIOStoragePool pool = new ScaleIOStoragePool(uuid, "192.168.1.19",
443, "a519be2f00000000", type, details, adapter);
+ final String volumePath = "6c3362b500000001:vol-139-3d2c-12f0";
+ final String volumeId = "6c3362b500000001";
+ OutputInterpreter.OneLineParser spyParser = spy(new
OutputInterpreter.OneLineParser());
+
+ try (MockedConstruction<QemuImg> ignored =
Mockito.mockConstruction(QemuImg.class, (mockQemuImg, contextQemuImg) -> {
+ Map<String, String> details = new HashMap<>();
+ details.put(QemuImg.VIRTUAL_SIZE, "16384");
+ details.put(QemuImg.FILE_FORMAT,
PhysicalDiskFormat.QCOW2.toString());
+
Mockito.doReturn(details).when(mockQemuImg).info(any(QemuImgFile.class));
+ }); MockedConstruction<File> ignored1 =
Mockito.mockConstruction(File.class, (mockFile, contextFile) -> {
+ Mockito.when(mockFile.exists()).thenReturn(true);
+ }); MockedConstruction<Script> ignored2 =
Mockito.mockConstruction(Script.class, (mockScript, contextScript) -> {
+
Mockito.doReturn(null).when(mockScript).execute(any(OutputInterpreter.class));
+ }); MockedConstruction<OutputInterpreter.OneLineParser> ignored3 =
Mockito.mockConstruction(OutputInterpreter.OneLineParser.class,
withSettings().spiedInstance(spyParser), (mockParser, contextParser) -> {
+ Mockito.doReturn("16384").when(mockParser).getLine();
+ }); MockedStatic<ScaleIOUtil> ignored5 =
Mockito.mockStatic(ScaleIOUtil.class, Mockito.CALLS_REAL_METHODS)) {
+
when(ScaleIOUtil.getSystemIdForVolume(volumeId)).thenReturn(systemId);
+
+ KVMPhysicalDisk disk = adapter.getPhysicalDisk(volumePath, pool);
+ assertNotNull(disk);
+ assertEquals("/dev/disk/by-id/emc-vol-" + systemId +
"-6c3362b500000001", disk.getPath());
+ assertEquals(PhysicalDiskFormat.QCOW2, disk.getFormat());
+ assertEquals(16384, disk.getSize());
+ assertEquals(16384, disk.getVirtualSize());
+ }
+ }
+
@Test
public void testConnectPhysicalDisk() {
+ when(Script.runSimpleBashScript("/opt/emc/scaleio/sdc/bin/drv_cfg
--query_mdms|grep " + systemId + "|awk '{print $5}'")).thenReturn(sdcId);
Review Comment:
This string is getting duplicated a lot. Can we replace this with something
like this?
```java
DRV_CFG_SCRIPT = "/opt/emc/scaleio/sdc/bin/drv_cfg --query_mdms|grep %s |awk
'{print $5}'"
...
when(Script.runSimpleBashScript(String.format(DRV_CFG_SCRIPT,
systemId)).thenReturn(sdcId);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
