vishesh92 commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1350738026
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java:
##########
@@ -364,7 +364,7 @@ protected Void
deleteSnapshotCallback(AsyncCallbackDispatcher<SnapshotServiceImp
SnapshotResult res = null;
try {
if (result.isFailed()) {
- s_logger.debug("delete snapshot failed" + result.getResult());
+ s_logger.debug(String.format("Failed to delete snapshot [%s]
due to: [%s].", snapshot.getUuid(), result.getResult()));
Review Comment:
While logging uuid/id, prefix it with `[id=`. This is how we are logging at
other places as well.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1289,37 +1289,52 @@ public void cleanupStorage(boolean recurring) {
try {
List<VMTemplateStoragePoolVO>
unusedTemplatesInPool = _tmpltMgr.getUnusedTemplatesInPool(pool);
- s_logger.debug("Storage pool garbage collector
found " + unusedTemplatesInPool.size() + " templates to clean up in storage
pool: " + pool.getName());
+ s_logger.debug(String.format("Storage pool
garbage collector found [%s] templates to be cleaned up in storage pool [%s].",
unusedTemplatesInPool.size(), pool.getName()));
for (VMTemplateStoragePoolVO templatePoolVO :
unusedTemplatesInPool) {
if (templatePoolVO.getDownloadState() !=
VMTemplateStorageResourceAssoc.Status.DOWNLOADED) {
- s_logger.debug("Storage pool garbage
collector is skipping template with ID: " + templatePoolVO.getTemplateId() + "
on pool " + templatePoolVO.getPoolId()
- + " because it is not completely
downloaded.");
+ s_logger.debug(String.format("Storage
pool garbage collector is skipping template [%s] clean up on pool [%s] " +
+ "because it is not completely
downloaded.", templatePoolVO.getTemplateId(), templatePoolVO.getPoolId()));
continue;
}
if (!templatePoolVO.getMarkedForGC()) {
templatePoolVO.setMarkedForGC(true);
_vmTemplatePoolDao.update(templatePoolVO.getId(), templatePoolVO);
- s_logger.debug("Storage pool garbage
collector has marked template with ID: " + templatePoolVO.getTemplateId() + "
on pool " + templatePoolVO.getPoolId()
- + " for garbage collection.");
+ s_logger.debug(String.format("Storage
pool garbage collector has marked template [%s] on pool [%s] " +
+ "for garbage collection.",
templatePoolVO.getTemplateId(), templatePoolVO.getPoolId()));
continue;
}
_tmpltMgr.evictTemplateFromStoragePool(templatePoolVO);
}
} catch (Exception e) {
- s_logger.warn("Problem cleaning up primary
storage pool " + pool, e);
+ s_logger.error(String.format("Failed to clean
up primary storage pool [%s] due to: [%s].", pool, e.getMessage()), e);
}
}
}
//destroy snapshots in destroying state in
snapshot_store_ref
List<SnapshotDataStoreVO> ssSnapshots =
_snapshotStoreDao.listByState(ObjectInDataStoreStateMachine.State.Destroying);
for (SnapshotDataStoreVO ssSnapshotVO : ssSnapshots) {
+ SnapshotVO snapshot =
_snapshotDao.findById(ssSnapshotVO.getSnapshotId());
Review Comment:
Use `getSnapshotVO` on snapshot info object to get the snapshot & it's uuid.
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java:
##########
@@ -373,7 +373,7 @@ protected Void
deleteSnapshotCallback(AsyncCallbackDispatcher<SnapshotServiceImp
res = new SnapshotResult(context.snapshot, null);
}
} catch (Exception e) {
- s_logger.debug("Failed to in deleteSnapshotCallback", e);
+ s_logger.error("An exception occurred while processing an event in
delete snapshot callback.", e);
Review Comment:
Log snapshot uuid here as well
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1289,37 +1289,52 @@ public void cleanupStorage(boolean recurring) {
try {
List<VMTemplateStoragePoolVO>
unusedTemplatesInPool = _tmpltMgr.getUnusedTemplatesInPool(pool);
- s_logger.debug("Storage pool garbage collector
found " + unusedTemplatesInPool.size() + " templates to clean up in storage
pool: " + pool.getName());
+ s_logger.debug(String.format("Storage pool
garbage collector found [%s] templates to be cleaned up in storage pool [%s].",
unusedTemplatesInPool.size(), pool.getName()));
for (VMTemplateStoragePoolVO templatePoolVO :
unusedTemplatesInPool) {
if (templatePoolVO.getDownloadState() !=
VMTemplateStorageResourceAssoc.Status.DOWNLOADED) {
- s_logger.debug("Storage pool garbage
collector is skipping template with ID: " + templatePoolVO.getTemplateId() + "
on pool " + templatePoolVO.getPoolId()
- + " because it is not completely
downloaded.");
+ s_logger.debug(String.format("Storage
pool garbage collector is skipping template [%s] clean up on pool [%s] " +
+ "because it is not completely
downloaded.", templatePoolVO.getTemplateId(), templatePoolVO.getPoolId()));
continue;
}
if (!templatePoolVO.getMarkedForGC()) {
templatePoolVO.setMarkedForGC(true);
_vmTemplatePoolDao.update(templatePoolVO.getId(), templatePoolVO);
- s_logger.debug("Storage pool garbage
collector has marked template with ID: " + templatePoolVO.getTemplateId() + "
on pool " + templatePoolVO.getPoolId()
- + " for garbage collection.");
+ s_logger.debug(String.format("Storage
pool garbage collector has marked template [%s] on pool [%s] " +
+ "for garbage collection.",
templatePoolVO.getTemplateId(), templatePoolVO.getPoolId()));
continue;
}
_tmpltMgr.evictTemplateFromStoragePool(templatePoolVO);
}
} catch (Exception e) {
- s_logger.warn("Problem cleaning up primary
storage pool " + pool, e);
+ s_logger.error(String.format("Failed to clean
up primary storage pool [%s] due to: [%s].", pool, e.getMessage()), e);
}
}
}
//destroy snapshots in destroying state in
snapshot_store_ref
List<SnapshotDataStoreVO> ssSnapshots =
_snapshotStoreDao.listByState(ObjectInDataStoreStateMachine.State.Destroying);
for (SnapshotDataStoreVO ssSnapshotVO : ssSnapshots) {
+ SnapshotVO snapshot =
_snapshotDao.findById(ssSnapshotVO.getSnapshotId());
+ if (snapshot == null) {
Review Comment:
`snapshot` will only null when the snapshot has been removed but the entry
is still present in `SnapshotDataStoreVO`.
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1289,37 +1289,52 @@ public void cleanupStorage(boolean recurring) {
try {
List<VMTemplateStoragePoolVO>
unusedTemplatesInPool = _tmpltMgr.getUnusedTemplatesInPool(pool);
- s_logger.debug("Storage pool garbage collector
found " + unusedTemplatesInPool.size() + " templates to clean up in storage
pool: " + pool.getName());
+ s_logger.debug(String.format("Storage pool
garbage collector found [%s] templates to be cleaned up in storage pool [%s].",
unusedTemplatesInPool.size(), pool.getName()));
for (VMTemplateStoragePoolVO templatePoolVO :
unusedTemplatesInPool) {
if (templatePoolVO.getDownloadState() !=
VMTemplateStorageResourceAssoc.Status.DOWNLOADED) {
- s_logger.debug("Storage pool garbage
collector is skipping template with ID: " + templatePoolVO.getTemplateId() + "
on pool " + templatePoolVO.getPoolId()
- + " because it is not completely
downloaded.");
+ s_logger.debug(String.format("Storage
pool garbage collector is skipping template [%s] clean up on pool [%s] " +
+ "because it is not completely
downloaded.", templatePoolVO.getTemplateId(), templatePoolVO.getPoolId()));
continue;
}
if (!templatePoolVO.getMarkedForGC()) {
templatePoolVO.setMarkedForGC(true);
_vmTemplatePoolDao.update(templatePoolVO.getId(), templatePoolVO);
- s_logger.debug("Storage pool garbage
collector has marked template with ID: " + templatePoolVO.getTemplateId() + "
on pool " + templatePoolVO.getPoolId()
- + " for garbage collection.");
+ s_logger.debug(String.format("Storage
pool garbage collector has marked template [%s] on pool [%s] " +
+ "for garbage collection.",
templatePoolVO.getTemplateId(), templatePoolVO.getPoolId()));
continue;
}
_tmpltMgr.evictTemplateFromStoragePool(templatePoolVO);
}
} catch (Exception e) {
- s_logger.warn("Problem cleaning up primary
storage pool " + pool, e);
+ s_logger.error(String.format("Failed to clean
up primary storage pool [%s] due to: [%s].", pool, e.getMessage()), e);
}
}
}
//destroy snapshots in destroying state in
snapshot_store_ref
List<SnapshotDataStoreVO> ssSnapshots =
_snapshotStoreDao.listByState(ObjectInDataStoreStateMachine.State.Destroying);
for (SnapshotDataStoreVO ssSnapshotVO : ssSnapshots) {
+ SnapshotVO snapshot =
_snapshotDao.findById(ssSnapshotVO.getSnapshotId());
+ if (snapshot == null) {
+ s_logger.warn(String.format("Did not find snapshot
[%s] in destroying state; therefore, it cannot be destroyed.",
ssSnapshotVO.getSnapshotId()));
+ continue;
+ }
+
+ String snapshotUuid = snapshot.getUuid();
try {
-
_snapshotService.deleteSnapshot(snapshotFactory.getSnapshot(ssSnapshotVO.getSnapshotId(),
DataStoreRole.Image));
+ s_logger.debug(String.format("Verifying if
snapshot [%s] is in destroying state in any image data store.", snapshotUuid));
+ SnapshotInfo snapshotInfo =
snapshotFactory.getSnapshot(snapshot.getId(), DataStoreRole.Image);
+
+ if (snapshotInfo != null) {
Review Comment:
If snapshot doesn't exist, snapshotInfo will also be null.
--
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]
