GutoVeronezi commented on code in PR #6367:
URL: https://github.com/apache/cloudstack/pull/6367#discussion_r870214822
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java:
##########
@@ -341,20 +343,25 @@ public KVMPhysicalDisk getPhysicalDisk(StoragePoolType
type, String poolUuid, St
public KVMStoragePool createStoragePool(String name, String host, int
port, String path, String userInfo, StoragePoolType type) {
// primary storage registers itself through here
- return createStoragePool(name, host, port, path, userInfo, type, true);
+ return createStoragePool(name, host, port, path, userInfo, type, null,
true);
+ }
+
+ public KVMStoragePool createStoragePool(String name, String host, int
port, String path, String userInfo, StoragePoolType type, Map<String, String>
details) {
+ // primary storage registers itself through here
Review Comment:
We can turn this comment into a Javadoc.
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/client/ScaleIOGatewayClientImpl.java:
##########
@@ -1013,6 +1013,22 @@ public Sdc getSdc(String sdcId) {
return get("/instances/Sdc::" + sdcId, Sdc.class);
}
+ @Override
+ public String getSdcIdByGuid(String sdcGuid) {
+ Preconditions.checkArgument(StringUtils.isNotEmpty(sdcGuid), "SDC Guid
cannot be null");
+
+ List<Sdc> sdcs = listSdcs();
+ if(sdcs != null) {
+ for (Sdc sdc : sdcs) {
+ if (sdcGuid.equalsIgnoreCase(sdc.getSdcGuid())) {
+ return sdc.getId();
+ }
+ }
+ }
Review Comment:
Suggestion: we can invert the first `if` and add a return to reduce
indentation:
```suggestion
if (sdcs == null) {
return null;
}
for (Sdc sdc : sdcs) {
if (sdcGuid.equalsIgnoreCase(sdc.getSdcGuid())) {
return sdc.getId();
}
}
```
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/util/ScaleIOUtil.java:
##########
@@ -97,6 +107,52 @@ public static final String getSystemIdForVolume(String
volumeId) {
return result;
}
+ public static String getSdcGuid() {
+ String queryGuidCmd = ScaleIOUtil.SDC_HOME_PATH + "/bin/" +
ScaleIOUtil.QUERY_GUID_CMD;
+ String result = Script.runSimpleBashScript(queryGuidCmd);
+ if (result == null) {
+ LOGGER.warn("Failed to get SDC guid");
+ return null;
+ }
+
+ if (result.isEmpty()) {
+ LOGGER.warn("No SDC guid retrieved");
+ return null;
+ }
+
+ String guidRegEx =
"^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$";
+ if (!result.matches(guidRegEx)) {
Review Comment:
We can use `UuidUtils#validateUUID` here, as it does the same validation.
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/provider/ScaleIOHostListener.java:
##########
@@ -78,27 +116,38 @@ public boolean hostConnect(long hostId, long poolId) {
StoragePoolHostVO storagePoolHost =
_storagePoolHostDao.findByPoolHost(poolId, hostId);
if (storagePoolHost == null) {
- storagePoolHost = new StoragePoolHostVO(poolId, hostId, "");
+ storagePoolHost = new StoragePoolHostVO(poolId, hostId, sdcId);
_storagePoolHostDao.persist(storagePoolHost);
+ } else {
+ storagePoolHost.setLocalPath(sdcId);
+ _storagePoolHostDao.update(storagePoolHost.getId(),
storagePoolHost);
}
- StoragePool storagePool =
(StoragePool)_dataStoreMgr.getDataStore(poolId, DataStoreRole.Primary);
- ModifyStoragePoolCommand cmd = new ModifyStoragePoolCommand(true,
storagePool);
- sendModifyStoragePoolCommand(cmd, storagePool, hostId);
+ s_logger.info("Connection established between storage pool: " +
storagePool + " and host: " + hostId);
return true;
}
- private boolean isHostSdcConnected(String hostIpAddress, long poolId) {
+ private String getHostSdcId(String sdcGuid, long poolId) {
+ try {
+ ScaleIOGatewayClient client =
ScaleIOGatewayClientConnectionPool.getInstance().getClient(poolId,
_storagePoolDetailsDao);
+ return client.getSdcIdByGuid(sdcGuid);
+ } catch (NoSuchAlgorithmException | KeyManagementException |
URISyntaxException e) {
+ s_logger.error("Failed to get host SDC id", e);
Review Comment:
We can add more context to the log.
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/ScaleIOPrimaryDataStoreLifeCycle.java:
##########
@@ -352,6 +325,22 @@ public boolean attachZone(DataStore dataStore, ZoneScope
scope, Hypervisor.Hyper
return true;
}
+ private void checkConnectedSdcs(Long dataStoreId) {
+ boolean haveConnectedSdcs = false;
+ try {
+ ScaleIOGatewayClient client =
ScaleIOGatewayClientConnectionPool.getInstance().getClient(dataStoreId,
storagePoolDetailsDao);
+ haveConnectedSdcs = client.haveConnectedSdcs();
+ } catch (NoSuchAlgorithmException | KeyManagementException |
URISyntaxException e) {
+ LOGGER.error("Failed to create storage pool", e);
Review Comment:
We can add more context to the log, like the `dataStoreId`.
##########
core/src/main/java/com/cloud/agent/api/ModifyStoragePoolCommand.java:
##########
@@ -32,13 +33,21 @@ public class ModifyStoragePoolCommand extends Command {
private StorageFilerTO pool;
private String localPath;
private String storagePath;
+ private Map<String, String> details;
public ModifyStoragePoolCommand(boolean add, StoragePool pool, String
localPath) {
this.add = add;
this.pool = new StorageFilerTO(pool);
this.localPath = localPath;
}
+ public ModifyStoragePoolCommand(boolean add, StoragePool pool, String
localPath, Map<String, String> details) {
+ this.add = add;
+ this.pool = new StorageFilerTO(pool);
+ this.localPath = localPath;
Review Comment:
```suggestion
this(add, pool, localPath);
```
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/client/ScaleIOGatewayClientImpl.java:
##########
@@ -1035,28 +1051,39 @@ public Sdc getConnectedSdcByIp(String ipAddress) {
}
@Override
- public List<String> listConnectedSdcIps() {
- List<String> sdcIps = new ArrayList<>();
+ public boolean haveConnectedSdcs() {
List<Sdc> sdcs = listSdcs();
if(sdcs != null) {
for (Sdc sdc : sdcs) {
if
(MDM_CONNECTED_STATE.equalsIgnoreCase(sdc.getMdmConnectionState())) {
- sdcIps.add(sdc.getSdcIp());
+ return true;
}
}
}
- return sdcIps;
+ return false;
+ }
+
+ @Override
+ public boolean isSdcConnected(String sdcId) {
+ Preconditions.checkArgument(StringUtils.isNotEmpty(sdcId), "SDC Id
cannot be null");
+
+ Sdc sdc = getSdc(sdcId);
+ if (sdc != null &&
MDM_CONNECTED_STATE.equalsIgnoreCase(sdc.getMdmConnectionState())) {
+ return true;
+ }
+
+ return false;
Review Comment:
```suggestion
return sdc != null &&
MDM_CONNECTED_STATE.equalsIgnoreCase(sdc.getMdmConnectionState());
```
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/ScaleIOPrimaryDataStoreLifeCycle.java:
##########
@@ -352,6 +325,22 @@ public boolean attachZone(DataStore dataStore, ZoneScope
scope, Hypervisor.Hyper
return true;
}
+ private void checkConnectedSdcs(Long dataStoreId) {
+ boolean haveConnectedSdcs = false;
+ try {
+ ScaleIOGatewayClient client =
ScaleIOGatewayClientConnectionPool.getInstance().getClient(dataStoreId,
storagePoolDetailsDao);
+ haveConnectedSdcs = client.haveConnectedSdcs();
+ } catch (NoSuchAlgorithmException | KeyManagementException |
URISyntaxException e) {
+ LOGGER.error("Failed to create storage pool", e);
+ throw new CloudRuntimeException("Failed to establish connection
with PowerFlex Gateway to create storage pool");
+ }
+
+ if (!haveConnectedSdcs) {
+ LOGGER.debug("No connected SDCs found for the PowerFlex storage
pool");
Review Comment:
We can add more context to the log, like the `dataStoreId`.
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/driver/ScaleIOPrimaryDataStoreDriver.java:
##########
@@ -191,41 +194,59 @@ public void revokeAccess(DataObject dataObject, Host
host, DataStore dataStore)
final VolumeVO volume = volumeDao.findById(dataObject.getId());
LOGGER.debug("Revoking access for PowerFlex volume: " +
volume.getPath());
- final ScaleIOGatewayClient client =
getScaleIOClient(dataStore.getId());
- final Sdc sdc =
client.getConnectedSdcByIp(host.getPrivateIpAddress());
- if (sdc == null) {
+ final String sdcId = getConnectedSdc(dataStore.getId(),
host.getId());
+ if (StringUtils.isBlank(sdcId)) {
throw new CloudRuntimeException("Unable to revoke access
for volume: " + dataObject.getId() + ", no Sdc connected with host ip: " +
host.getPrivateIpAddress());
}
-
client.unmapVolumeFromSdc(ScaleIOUtil.getVolumePath(volume.getPath()),
sdc.getId());
+ final ScaleIOGatewayClient client =
getScaleIOClient(dataStore.getId());
+
client.unmapVolumeFromSdc(ScaleIOUtil.getVolumePath(volume.getPath()), sdcId);
} else if (DataObjectType.TEMPLATE.equals(dataObject.getType())) {
final VMTemplateStoragePoolVO templatePoolRef =
vmTemplatePoolDao.findByPoolTemplate(dataStore.getId(), dataObject.getId(),
null);
LOGGER.debug("Revoking access for PowerFlex template volume: "
+ templatePoolRef.getInstallPath());
- final ScaleIOGatewayClient client =
getScaleIOClient(dataStore.getId());
- final Sdc sdc =
client.getConnectedSdcByIp(host.getPrivateIpAddress());
- if (sdc == null) {
+ final String sdcId = getConnectedSdc(dataStore.getId(),
host.getId());
+ if (StringUtils.isBlank(sdcId)) {
throw new CloudRuntimeException("Unable to revoke access
for template: " + dataObject.getId() + ", no Sdc connected with host ip: " +
host.getPrivateIpAddress());
}
-
client.unmapVolumeFromSdc(ScaleIOUtil.getVolumePath(templatePoolRef.getInstallPath()),
sdc.getId());
+ final ScaleIOGatewayClient client =
getScaleIOClient(dataStore.getId());
+
client.unmapVolumeFromSdc(ScaleIOUtil.getVolumePath(templatePoolRef.getInstallPath()),
sdcId);
} else if (DataObjectType.SNAPSHOT.equals(dataObject.getType())) {
SnapshotInfo snapshot = (SnapshotInfo) dataObject;
LOGGER.debug("Revoking access for PowerFlex volume snapshot: "
+ snapshot.getPath());
- final ScaleIOGatewayClient client =
getScaleIOClient(dataStore.getId());
- final Sdc sdc =
client.getConnectedSdcByIp(host.getPrivateIpAddress());
- if (sdc == null) {
+ final String sdcId = getConnectedSdc(dataStore.getId(),
host.getId());
+ if (StringUtils.isBlank(sdcId)) {
throw new CloudRuntimeException("Unable to revoke access
for snapshot: " + dataObject.getId() + ", no Sdc connected with host ip: " +
host.getPrivateIpAddress());
}
-
client.unmapVolumeFromSdc(ScaleIOUtil.getVolumePath(snapshot.getPath()),
sdc.getId());
+ final ScaleIOGatewayClient client =
getScaleIOClient(dataStore.getId());
+
client.unmapVolumeFromSdc(ScaleIOUtil.getVolumePath(snapshot.getPath()), sdcId);
}
} catch (Exception e) {
LOGGER.warn("Failed to revoke access due to: " + e.getMessage(),
e);
}
}
+ private String getConnectedSdc(long poolId, long hostId) {
+ try {
+ StoragePoolHostVO poolHostVO =
storagePoolHostDao.findByPoolHost(poolId, hostId);
+ if (poolHostVO == null) {
+ return null;
+ }
+
+ final ScaleIOGatewayClient client = getScaleIOClient(poolId);
+ if (client.isSdcConnected(poolHostVO.getLocalPath())) {
+ return poolHostVO.getLocalPath();
+ }
+ } catch (Exception e) {
Review Comment:
Is necessary a catch Pokémon here?
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/provider/ScaleIOHostListener.java:
##########
@@ -69,7 +73,41 @@ public boolean hostConnect(long hostId, long poolId) {
return false;
}
- if (!isHostSdcConnected(host.getPrivateIpAddress(), poolId)) {
+ StoragePool storagePool =
(StoragePool)_dataStoreMgr.getDataStore(poolId, DataStoreRole.Primary);
+
+ String systemId = _storagePoolDetailsDao.findDetail(poolId,
ScaleIOGatewayClient.STORAGE_POOL_SYSTEM_ID).getValue();
+ if (systemId == null) {
+ throw new CloudRuntimeException("Failed to get the system id for
PowerFlex storage pool " + storagePool.getName());
+ }
+ Map<String,String> details = new HashMap<>();
+ details.put(ScaleIOGatewayClient.STORAGE_POOL_SYSTEM_ID, systemId);
+
+ ModifyStoragePoolCommand cmd = new ModifyStoragePoolCommand(true,
storagePool, storagePool.getPath(), details);
+ ModifyStoragePoolAnswer answer = sendModifyStoragePoolCommand(cmd,
storagePool, hostId);
+ Map<String,String> poolDetails = answer.getPoolInfo().getDetails();
+ if (MapUtils.isEmpty(poolDetails)) {
+ s_logger.warn("SDC details not found on the host: " + hostId);
+ String msg = "SDC details not found on the host: " + hostId + ",
(re)install SDC and restart agent";
Review Comment:
```suggestion
String msg = "SDC details not found on the host: " + hostId + ",
(re)install SDC and restart agent";
s_logger.warn(msg);
```
##########
plugins/storage/volume/scaleio/src/main/java/org/apache/cloudstack/storage/datastore/provider/ScaleIOHostListener.java:
##########
@@ -69,7 +73,41 @@ public boolean hostConnect(long hostId, long poolId) {
return false;
}
- if (!isHostSdcConnected(host.getPrivateIpAddress(), poolId)) {
+ StoragePool storagePool =
(StoragePool)_dataStoreMgr.getDataStore(poolId, DataStoreRole.Primary);
+
+ String systemId = _storagePoolDetailsDao.findDetail(poolId,
ScaleIOGatewayClient.STORAGE_POOL_SYSTEM_ID).getValue();
+ if (systemId == null) {
+ throw new CloudRuntimeException("Failed to get the system id for
PowerFlex storage pool " + storagePool.getName());
+ }
+ Map<String,String> details = new HashMap<>();
+ details.put(ScaleIOGatewayClient.STORAGE_POOL_SYSTEM_ID, systemId);
+
+ ModifyStoragePoolCommand cmd = new ModifyStoragePoolCommand(true,
storagePool, storagePool.getPath(), details);
+ ModifyStoragePoolAnswer answer = sendModifyStoragePoolCommand(cmd,
storagePool, hostId);
+ Map<String,String> poolDetails = answer.getPoolInfo().getDetails();
+ if (MapUtils.isEmpty(poolDetails)) {
+ s_logger.warn("SDC details not found on the host: " + hostId);
+ String msg = "SDC details not found on the host: " + hostId + ",
(re)install SDC and restart agent";
+ _alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_HOST,
host.getDataCenterId(), host.getPodId(), "SDC not found on host: " +
host.getUuid(), msg);
+ return false;
+ }
+
+ String sdcId = null;
+ if (poolDetails.containsKey(ScaleIOGatewayClient.SDC_ID)) {
+ sdcId = poolDetails.get(ScaleIOGatewayClient.SDC_ID);
+ } else if (poolDetails.containsKey(ScaleIOGatewayClient.SDC_GUID)) {
+ String sdcGuid = poolDetails.get(ScaleIOGatewayClient.SDC_GUID);
+ sdcId = getHostSdcId(sdcGuid, poolId);
+ }
+
+ if (StringUtils.isBlank(sdcId)) {
+ s_logger.warn("Couldn't retrieve SDC details from the host: " +
hostId);
+ String msg = "Couldn't retrieve SDC details from the host: " +
hostId + ", (re)install SDC and restart agent";
Review Comment:
```suggestion
String msg = "Couldn't retrieve SDC details from the host: " +
hostId + ", (re)install SDC and restart agent";
s_logger.warn(msg);
```
--
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]
