Skip to content

Commit 76bdf24

Browse files
sandeeplocharlasandeeplocharla
committed
Fixed few more review comments
1 parent 8df4bc9 commit 76bdf24

2 files changed

Lines changed: 29 additions & 39 deletions

File tree

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java‎

Lines changed: 28 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -71,33 +71,33 @@ public class OntapHostListener implements HypervisorHostListener {
7171

7272
@Override
7373
public boolean hostConnect(long hostId, long poolId) {
74-
logger.info("Connect to host " + hostId + " from pool " + poolId);
74+
logger.info("hostConnect: Connecting host {} to pool {}", hostId, poolId);
7575
Host host = _hostDao.findById(hostId);
7676
if (host == null) {
77-
logger.error("host was not found with id : {}", hostId);
77+
logger.error("hostConnect: Host was not found with id: {}", hostId);
7878
return false;
7979
}
8080
if (!host.getHypervisorType().equals(Hypervisor.HypervisorType.KVM)) {
81-
logger.error("ONTAP plugin does not support {} type host currently ", host.getHypervisorType());
81+
logger.error("hostConnect: ONTAP plugin does not support {} type host currently", host.getHypervisorType());
8282
return false;
8383
}
8484

8585
StoragePool pool = _storagePoolDao.findById(poolId);
8686
if (pool == null) {
87-
logger.error("Failed to connect host - storage pool not found with id: {}", poolId);
87+
logger.error("hostConnect: Failed to connect host - storage pool not found with id: {}", poolId);
8888
return false;
8989
}
90-
logger.info("Connecting host {} to ONTAP storage pool {}", host.getName(), pool.getName());
90+
logger.info("hostConnect: Connecting host {} to ONTAP storage pool {}", host.getName(), pool.getName());
9191
try {
9292
// Load storage pool details from database to pass mount options and other config to agent
9393
Map<String, String> detailsMap = _storagePoolDetailsDao.listDetailsKeyPairs(poolId);
9494
if (detailsMap == null || detailsMap.isEmpty()) {
95-
logger.error("Failed to load storage pool details for pool id: {}", poolId);
95+
logger.error("hostConnect: Failed to load storage pool details for pool id: {}", poolId);
9696
return false;
9797
}
9898

9999
if (detailsMap.get(OntapStorageConstants.PROTOCOL) == null) {
100-
logger.error("Storage pool details missing required protocol type for pool id: {}", poolId);
100+
logger.error("hostConnect: Storage pool details missing required protocol type for pool id: {}", poolId);
101101
return false;
102102
}
103103

@@ -139,19 +139,19 @@ public boolean hostConnect(long hostId, long poolId) {
139139
}
140140

141141
String localPath = poolInfo.getLocalPath();
142-
logger.info("Storage pool {} successfully mounted at: {}", pool.getName(), localPath);
142+
logger.info("hostConnect: Storage pool {} successfully mounted at: {}", pool.getName(), localPath);
143143

144144
// Update or create the storage_pool_host_ref entry with the correct local_path
145145
StoragePoolHostVO storagePoolHost = storagePoolHostDao.findByPoolHost(poolId, hostId);
146146

147147
if (storagePoolHost == null) {
148148
storagePoolHost = new StoragePoolHostVO(poolId, hostId, localPath);
149149
storagePoolHostDao.persist(storagePoolHost);
150-
logger.info("Created storage_pool_host_ref entry for pool {} and host {}", pool.getName(), host.getName());
150+
logger.info("hostConnect: Created storage_pool_host_ref entry for pool {} and host {}", pool.getName(), host.getName());
151151
} else {
152152
storagePoolHost.setLocalPath(localPath);
153153
storagePoolHostDao.update(storagePoolHost.getId(), storagePoolHost);
154-
logger.info("Updated storage_pool_host_ref entry with local_path: {}", localPath);
154+
logger.info("hostConnect: Updated storage_pool_host_ref entry with local_path: {}", localPath);
155155
}
156156

157157
// Update pool capacity/usage information
@@ -160,11 +160,11 @@ public boolean hostConnect(long hostId, long poolId) {
160160
poolVO.setCapacityBytes(poolInfo.getCapacityBytes());
161161
poolVO.setUsedBytes(poolInfo.getCapacityBytes() - poolInfo.getAvailableBytes());
162162
_storagePoolDao.update(poolVO.getId(), poolVO);
163-
logger.info("Updated storage pool capacity: {} GB, used: {} GB", poolInfo.getCapacityBytes() / (1024 * 1024 * 1024), (poolInfo.getCapacityBytes() - poolInfo.getAvailableBytes()) / (1024 * 1024 * 1024));
163+
logger.info("hostConnect: Updated storage pool capacity: {} GB, used: {} GB", poolInfo.getCapacityBytes() / (1024 * 1024 * 1024), (poolInfo.getCapacityBytes() - poolInfo.getAvailableBytes()) / (1024 * 1024 * 1024));
164164
}
165165

166166
} catch (Exception e) {
167-
logger.error("Exception while connecting host {} to storage pool {}", host.getName(), pool.getName(), e);
167+
logger.error("hostConnect: Exception while connecting host {} to storage pool {}", host.getName(), pool.getName(), e);
168168
// CRITICAL: Don't throw exception - it crashes the agent and causes restart loops
169169
// Return false to indicate failure without crashing
170170
return false;
@@ -191,7 +191,7 @@ private void updateNfsExportPolicyForConnectedHostIfNeeded(long poolId, long hos
191191

192192
StorageStrategy strategy = OntapStorageUtils.getStrategyByStoragePoolDetails(detailsMap);
193193
strategy.updateAccessGroup(accessGroup);
194-
logger.info("Updated NFS export policy rules for host {} on storage pool {}", host.getName(), poolId);
194+
logger.info("hostConnect: updateNfsExportPolicyForConnectedHostIfNeeded: Updated NFS export policy rules for host {} on storage pool {}", host.getName(), poolId);
195195
}
196196

197197
private boolean isNfs3EnabledOnHost(Host host) {
@@ -201,7 +201,7 @@ private boolean isNfs3EnabledOnHost(Host host) {
201201

202202
String storageIp = host.getStorageIpAddress() != null ? host.getStorageIpAddress().trim() : "";
203203
if (storageIp.isEmpty() && StringUtils.isBlank(host.getPrivateIpAddress())) {
204-
logger.warn("Host {} is not eligible for NFS3 protocol: both storage IP and private IP are empty",
204+
logger.warn("isNfs3EnabledOnHost: Host {} is not eligible for NFS3 protocol: both storage IP and private IP are empty",
205205
host.getId());
206206
return false;
207207
}
@@ -211,35 +211,35 @@ private boolean isNfs3EnabledOnHost(Host host) {
211211

212212
@Override
213213
public boolean hostDisconnected(long hostId, long poolId) {
214-
logger.info("Disconnect from host " + hostId + " from pool " + poolId);
214+
logger.info("hostDisconnected: Disconnecting host {} from pool {}", hostId, poolId);
215215
// Note: This is not currently being called for NetApp ONTAP storage plugin.
216216
return false;
217217
}
218218

219219
@Override
220220
public boolean hostAboutToBeRemoved(long hostId) {
221-
logger.info("Host {} is about to be removed", hostId);
221+
logger.info("hostAboutToBeRemoved: Host {} is about to be removed", hostId);
222222

223223
Host host = _hostDao.findById(hostId);
224224
if (host == null) {
225-
logger.warn("Host not found with id: {}, considering it as no-op", hostId);
225+
logger.warn("hostAboutToBeRemoved: Host not found with id: {}, considering it as no-op", hostId);
226226
return true;
227227
}
228228

229229
List<StoragePoolHostVO> poolHostRefs = storagePoolHostDao.listByHostId(hostId);
230230
if (poolHostRefs == null || poolHostRefs.isEmpty()) {
231-
logger.debug("No storage pool associations found for host {}", hostId);
231+
logger.debug("hostAboutToBeRemoved: No storage pool associations found for host {}", hostId);
232232
return true;
233233
}
234234

235235
for (StoragePoolHostVO ref : poolHostRefs) {
236236
StoragePoolVO pool = _storagePoolDao.findById(ref.getPoolId());
237237
if (pool != null) {
238-
removeHostFromOntapPoolIfNeeded(hostId, pool, host);
238+
removeHostFromOntapPoolIfNeeded(pool, host);
239239
}
240240
}
241241

242-
logger.info("Cleaned up ONTAP export policies for host {} about to be removed", hostId);
242+
logger.info("hostAboutToBeRemoved: Cleaned up ONTAP export policies for host {} about to be removed", hostId);
243243
return true;
244244
}
245245

@@ -248,23 +248,23 @@ public boolean hostRemoved(long hostId, long clusterId) {
248248
return false;
249249
}
250250

251-
private void removeHostFromOntapPoolIfNeeded(long hostId, StoragePoolVO pool, Host host) {
251+
private void removeHostFromOntapPoolIfNeeded(StoragePoolVO pool, Host host) {
252252
try {
253253
Map<String, String> detailsMap = _storagePoolDetailsDao.listDetailsKeyPairs(pool.getId());
254254
if (detailsMap == null || detailsMap.isEmpty()) {
255-
logger.debug("No pool details found for pool id: {}", pool.getId());
255+
logger.debug("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: No pool details found for pool id: {}", pool.getId());
256256
return;
257257
}
258258

259-
// Skip non-NFS3 pools
259+
// Skip non-NFS3 pools; Currently, for iSCSI type, iGroup rules are being handled as part of revokeAccess in OntapPrimaryDataStoreDriver, so no need to handle here.
260260
if (!ProtocolType.NFS3.name().equalsIgnoreCase(detailsMap.get(OntapStorageConstants.PROTOCOL))) {
261261
return;
262262
}
263263

264-
logger.info("Removing export policy rule for host {} from storage pool {}", host.getName(), pool.getName());
264+
logger.info("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: Removing export policy rule for host {} from storage pool {}", host.getName(), pool.getName());
265265
if (!isNfs3EnabledOnHost(host)) {
266-
logger.warn("Skipping NFS export policy removal for host {} on pool {} as host is not NFS-enabled",
267-
hostId, pool.getId());
266+
logger.warn("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: Skipping NFS export policy removal for host {} on pool {} as host is not NFS-enabled",
267+
host.getId(), pool.getId());
268268
return;
269269
}
270270
AccessGroup accessGroup = new AccessGroup();
@@ -274,9 +274,9 @@ private void removeHostFromOntapPoolIfNeeded(long hostId, StoragePoolVO pool, Ho
274274

275275
StorageStrategy strategy = OntapStorageUtils.getStrategyByStoragePoolDetails(detailsMap);
276276
strategy.updateAccessGroup(accessGroup);
277-
logger.info("Removed NFS export policy rules for removed host {} from storage pool {}", host.getName(), pool.getName());
277+
logger.info("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: Removed NFS export policy rules for removed host {} from storage pool {}", host.getName(), pool.getName());
278278
} catch (Exception e) {
279-
logger.warn("Failed to remove NFS export policy rule for host {} from pool {}: {}", hostId, pool.getName(), e.getMessage());
279+
logger.warn("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: Failed to remove NFS export policy rule for host {} from pool {}: {}", host.getId(), pool.getName(), e.getMessage());
280280
// Continue processing other pools even if one fails
281281
}
282282
}

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -228,19 +228,9 @@ public AccessGroup updateAccessGroup(AccessGroup accessGroup) {
228228
}
229229

230230
List<ExportRule> rules = existingPolicy.getRules();
231-
// An existing export policy should always have at least one rule. A null or empty rules list
232-
// indicates the policy was corrupted or modified externally on ONTAP and is in an unexpected
233-
// state. updateAccessGroup only modifies an existing policy — it does not create rules from scratch.
234231
if (rules == null || rules.isEmpty()) {
235-
throw new CloudRuntimeException("Export policy " + existingPolicy.getName() + " has no rules. " +
236-
"Cannot update an export policy with no existing rules.");
237-
}
238-
239-
// This plugin creates a single NFS export rule per policy. More than one rule means the
240-
// policy was changed outside the plugin and we no longer know which rule should be mutated.
241-
if (rules.size() != 1) {
242232
throw new CloudRuntimeException("Export policy " + existingPolicy.getName() +
243-
" is expected to have exactly one rule but found: " + rules.size());
233+
" has no rules — unexpected state, the plugin always creates a rule at pool registration");
244234
}
245235

246236
ExportRule targetRule = rules.get(0);

0 commit comments

Comments
 (0)