You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "hsato03 (via GitHub)" <gi...@apache.org> on 2023/10/03 19:28:30 UTC
[PR] Snapshot removal and storage cleanup logs [cloudstack]
hsato03 opened a new pull request, #8031:
URL: https://github.com/apache/cloudstack/pull/8031
### Description
The snapshot removal and storage cleanup logs have few information about these processes, which makes troubleshooting difficult.
This PR intends to add new log messages and rewrite the old ones to make troubleshooting easier.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [X] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
I decreased the value of the `storage.cleanup.interval` configuration and deleted a snapshot. Then I checked the management server logs.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "vishesh92 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1351615410
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java:
##########
@@ -418,15 +418,13 @@ public boolean deleteSnapshot(SnapshotInfo snapInfo) {
if (result.isFailed()) {
throw new CloudRuntimeException(result.getResult());
}
+ s_logger.debug(String.format("Successfully deleted snapshot [%s] with ID [%s].", snapInfo.getName(), snapInfo.getUuid()));
return true;
- } catch (InterruptedException e) {
- s_logger.debug("delete snapshot is failed: " + e.toString());
- } catch (ExecutionException e) {
- s_logger.debug("delete snapshot is failed: " + e.toString());
+ } catch (InterruptedException | ExecutionException e) {
+ s_logger.error(String.format("Failed to delete snapshot [%s] due to: [%s].", snapInfo.getUuid(), e.getMessage()), e);
Review Comment:
```suggestion
s_logger.error(String.format("Failed to delete snapshot [%s] due to: [%s].", snapInfo.getUuid(), e.getMessage()));
s_logger.error(String.format("Failed to delete snapshot [%s].", snapInfo.getUuid()), e);
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1351625216
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1397,17 +1412,17 @@ public void cleanupStorage(boolean recurring) {
// expunge volume from secondary if volume is on image store
VolumeInfo volOnSecondary = volFactory.getVolume(volume.getId(), DataStoreRole.Image);
if (volOnSecondary != null) {
- s_logger.info("Expunging volume " + volume.getUuid() + " uploaded using HTTP POST from secondary data store");
+ s_logger.info(String.format("Expunging volume [%s] uploaded using HTTP POST from secondary data store.", volume.getUuid()));
AsyncCallFuture<VolumeApiResult> future = volService.expungeVolumeAsync(volOnSecondary);
VolumeApiResult result = future.get();
if (!result.isSuccess()) {
- s_logger.warn("Failed to expunge volume " + volume.getUuid() + " from the image store " + dataStore.getName() + " due to: " + result.getResult());
+ s_logger.warn(String.format("Failed to expunge volume [%s] from the image store [%s] due to: [%s].", volume.getUuid(), dataStore.getName(), result.getResult()));
}
}
}
}
} catch (Throwable th) {
- s_logger.warn("Unable to destroy uploaded volume " + volume.getUuid() + ". Error details: " + th.getMessage());
+ s_logger.error(String.format("Unable to destroy uploaded volume [%s] due to: [%s].", volume.getUuid(), th.getMessage()), th);
Review Comment:
```suggestion
s_logger.error(String.format("Unable to destroy uploaded volume [%s] due to: [%s].", volume.getUuid(), th.getMessage()));
s_logger.error(String.format("Unable to destroy uploaded volume [%s].", volume.getUuid()), th);
```
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1397,17 +1412,17 @@ public void cleanupStorage(boolean recurring) {
// expunge volume from secondary if volume is on image store
VolumeInfo volOnSecondary = volFactory.getVolume(volume.getId(), DataStoreRole.Image);
if (volOnSecondary != null) {
- s_logger.info("Expunging volume " + volume.getUuid() + " uploaded using HTTP POST from secondary data store");
+ s_logger.info(String.format("Expunging volume [%s] uploaded using HTTP POST from secondary data store.", volume.getUuid()));
AsyncCallFuture<VolumeApiResult> future = volService.expungeVolumeAsync(volOnSecondary);
VolumeApiResult result = future.get();
if (!result.isSuccess()) {
- s_logger.warn("Failed to expunge volume " + volume.getUuid() + " from the image store " + dataStore.getName() + " due to: " + result.getResult());
+ s_logger.warn(String.format("Failed to expunge volume [%s] from the image store [%s] due to: [%s].", volume.getUuid(), dataStore.getName(), result.getResult()));
}
}
}
}
} catch (Throwable th) {
- s_logger.warn("Unable to destroy uploaded volume " + volume.getUuid() + ". Error details: " + th.getMessage());
+ s_logger.error(String.format("Unable to destroy uploaded volume [%s] due to: [%s].", volume.getUuid(), th.getMessage()), th);
Review Comment:
```suggestion
s_logger.error(String.format("Unable to destroy uploaded volume [%s] due to: [%s].", volume.getUuid(), th.getMessage()));
s_logger.debug(String.format("Unable to destroy uploaded volume [%s].", volume.getUuid()), th);
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1351627754
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1456,7 +1471,7 @@ public void cleanupStorage(boolean recurring) {
}
}
} catch (Throwable th) {
- s_logger.warn("Unable to destroy uploaded template " + template.getUuid() + ". Error details: " + th.getMessage());
+ s_logger.error(String.format("Unable to destroy uploaded template [%s] due to: [%s].", template.getUuid(), th.getMessage()), th);
Review Comment:
```suggestion
s_logger.error(String.format("Unable to destroy uploaded template [%s] due to: [%s].", template.getUuid(), th.getMessage()));
s_logger.debug(String.format("Unable to destroy uploaded template [%s].", template.getUuid()), th);
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "hsato03 (via GitHub)" <gi...@apache.org>.
hsato03 commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1353116594
##########
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:
As mentioned in https://github.com/apache/cloudstack/pull/8031/files#r1353106386, snapshot info can 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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1351623632
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1369,7 +1384,7 @@ public void cleanupStorage(boolean recurring) {
}
_snapshotDao.expunge(snapshotVO.getId());
} catch (Exception e) {
- s_logger.warn("Unable to destroy snapshot " + snapshotVO.getUuid(), e);
+ s_logger.error(String.format("Unable to destroy snapshot [%s] due to: [%s].", snapshotVO.getUuid(), e.getMessage()), e);
Review Comment:
```suggestion
s_logger.error(String.format("Unable to destroy snapshot [%s] due to: [%s].", snapshotVO.getUuid(), e.getMessage()));
s_logger.debug(String.format("Unable to destroy snapshot [%s].", snapshotVO.getUuid()), e);
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "hsato03 (via GitHub)" <gi...@apache.org>.
hsato03 commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1355773436
##########
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:
@DaanHoogland I made the changes to check if debug is enabled. By the way, the error log can no longer use the snapshot uuid anymore.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "hsato03 (via GitHub)" <gi...@apache.org>.
hsato03 commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1353106386
##########
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:
You are right. But, snapshotInfo is getting only snapshots from Image data store role. If a snapshot is in destroying state in a Primary data store role and the same snapshot in the Image data store role has already been cleaned up, a NPE will be thrown.
Snapshots from ID 18 & 19 were cleaned up in Image data store but the snapshot still remains in destroying state in Primary data store.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1354349206
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1352,10 +1370,10 @@ public void cleanupStorage(boolean recurring) {
volService.ensureVolumeIsExpungeReady(vol.getId());
volService.expungeVolumeAsync(volumeInfo);
} else {
- s_logger.debug("Volume " + vol.getUuid() + " is already destroyed");
+ s_logger.debug(String.format("Volume [%s] is already destroyed.", vol.getUuid()));
}
} catch (Exception e) {
- s_logger.warn("Unable to destroy volume " + vol.getUuid(), e);
+ s_logger.error(String.format("Unable to destroy volume [%s] due to: [%s].", vol.getUuid(), e.getMessage()), e);
Review Comment:
```suggestion
s_logger.error(String.format("Unable to destroy volume [%s] due to: [%s].", vol.getUuid(), e.getMessage()));
s_logger.debug(String.format("Unable to destroy volume [%s].", vol.getUuid()), e);
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#issuecomment-1759419333
@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#issuecomment-1759646108
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland merged PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "GutoVeronezi (via GitHub)" <gi...@apache.org>.
GutoVeronezi commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1350790682
##########
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:
Actually, it is not a consensus.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "hsato03 (via GitHub)" <gi...@apache.org>.
hsato03 commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1353106386
##########
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:
You are right. But, snapshotInfo is getting only snapshots from Image data store role. If a snapshot is in destroying state in a Primary data store role and the same snapshot in the Image data store role has already been cleaned up, snapshotInfo will be null and a NPE will be thrown.
Snapshots from ID 18 & 19 were cleaned up in Image data store but the snapshot still remains in destroying state in Primary data store.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#issuecomment-1746427509
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 7222
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#issuecomment-1759409935
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#issuecomment-1759648335
@DaanHoogland a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#issuecomment-1760679565
<b>[SF] Trillian test result (tid-7943)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 44936 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8031-t7943-kvm-centos7.zip
Smoke tests completed. 111 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_08_upgrade_kubernetes_ha_cluster | `Failure` | 628.45 | test_kubernetes_clusters.py
test_03_deploy_vm_wrong_checksum | `Error` | 39.60 | test_templates.py
test_09_list_templates_download_details | `Failure` | 0.05 | test_templates.py
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1351619558
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1343,7 +1358,7 @@ public void cleanupStorage(boolean recurring) {
// system, but not necessary.
handleManagedStorage(vol);
} catch (Exception e) {
- s_logger.warn("Unable to destroy host-side clustered file system " + vol.getUuid(), e);
+ s_logger.error(String.format("Unable to destroy host-side clustered file system [%s] due to: [%s].", vol.getUuid(), e.getMessage()), e);
Review Comment:
```suggestion
s_logger.error(String.format("Unable to destroy host-side clustered file system [%s] due to: [%s].", vol.getUuid(), e.getMessage()));
s_logger.error(String.format("Unable to destroy host-side clustered file system [%s].", vol.getUuid()), e);
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "hsato03 (via GitHub)" <gi...@apache.org>.
hsato03 commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1353106386
##########
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:
You are right. But, `snapshotInfo` is getting only snapshots from Image data store role. If a snapshot is in destroying state in a Primary data store role and the same snapshot in the Image data store role has already been cleaned up, `snapshotInfo` will be null and a NPE will be thrown.
Snapshots from ID 18 & 19 were cleaned up in Image data store but the snapshot still remains in destroying state in Primary data store.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "vishesh92 (via GitHub)" <gi...@apache.org>.
vishesh92 commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1353146802
##########
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:
As per my understanding, the below code takes care of that. We don't need to check for `snapshot` and `snapshotInfo` separately. Just a check on snapshotInfo should be fine and the code above (https://github.com/apache/cloudstack/pull/8031/files#diff-cf68e132ad7711cfa9250cda5819c09f51cba9933aa595cd82ad5a02b4edbe64R1319-R1325) can be removed.
https://github.com/apache/cloudstack/blob/b4c7705d4bc8d29f20a44c48081b80d06ab2a789/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java#L91-L111
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1351616477
##########
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);
Review Comment:
```suggestion
s_logger.error(String.format("Failed to clean up primary storage pool [%s] due to: [%s].", pool, e.getMessage()));
s_logger.debug(String.format("Failed to clean up primary storage pool [%s].", pool), e);
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1354362555
##########
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:
@hsato03 , if I understand correctly you do a DB query for the sole purpose of logging. That seems a stretch. An operator can do this out of bound. If at all one would want this it should be turn-offable like behind an `if (LOGGER.isDebugEnabled()) {}` guard.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "hsato03 (via GitHub)" <gi...@apache.org>.
hsato03 commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1353206492
##########
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:
The code you sugested to remove is being used to get the snapshot uuid and to log it in places that are out of `snapshotInfo` scope and in cases where `snapshotInfo` is 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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1351616477
##########
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);
Review Comment:
```suggestion
s_logger.error(String.format("Failed to clean up primary storage pool [%s] due to: [%s].", pool, e.getMessage()));
s_logger.error(String.format("Failed to clean up primary storage pool [%s].", pool), e);
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1351614400
##########
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:
```suggestion
s_logger.error("An exception occurred while processing an event in delete snapshot callback.");
s_logger.debug("Exception while processing an event in delete snapshot callback.", e);
```
stacktraces should not go in error level logging
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1351619558
##########
server/src/main/java/com/cloud/storage/StorageManagerImpl.java:
##########
@@ -1343,7 +1358,7 @@ public void cleanupStorage(boolean recurring) {
// system, but not necessary.
handleManagedStorage(vol);
} catch (Exception e) {
- s_logger.warn("Unable to destroy host-side clustered file system " + vol.getUuid(), e);
+ s_logger.error(String.format("Unable to destroy host-side clustered file system [%s] due to: [%s].", vol.getUuid(), e.getMessage()), e);
Review Comment:
```suggestion
s_logger.error(String.format("Unable to destroy host-side clustered file system [%s] due to: [%s].", vol.getUuid(), e.getMessage()));
s_logger.debug(String.format("Unable to destroy host-side clustered file system [%s].", vol.getUuid()), e);
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#issuecomment-1746333037
@harikrishna-patnala a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "harikrishna-patnala (via GitHub)" <gi...@apache.org>.
harikrishna-patnala commented on PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#issuecomment-1746331168
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#issuecomment-1746346755
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/8031?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#8031](https://app.codecov.io/gh/apache/cloudstack/pull/8031?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (f5e5dbf) into [main](https://app.codecov.io/gh/apache/cloudstack/commit/2e9b3d8e084b5f6ddf4690afe266e0a2b3f751bb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (2e9b3d8) will **decrease** coverage by `23.72%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #8031 +/- ##
============================================
- Coverage 28.57% 4.86% -23.72%
============================================
Files 5100 342 -4758
Lines 358565 25689 -332876
Branches 52316 4418 -47898
============================================
- Hits 102464 1250 -101214
+ Misses 241968 24305 -217663
+ Partials 14133 134 -13999
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/8031/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [simulator-marvin-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8031/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/8031/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.86% <ø> (ø)` | |
| [unit-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8031/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
[see 4758 files with indirect coverage changes](https://app.codecov.io/gh/apache/cloudstack/pull/8031/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1351618298
##########
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) {
+ s_logger.debug(String.format("Snapshot [%s] in destroying state found in image data store [%s]; therefore, it will be destroyed.", snapshotUuid, snapshotInfo.getDataStore().getUuid()));
+ _snapshotService.deleteSnapshot(snapshotInfo);
+ } else {
+ s_logger.debug(String.format("Did not find snapshot [%s] in destroying state in any image data store.", snapshotUuid));
+ }
} catch (Exception e) {
- s_logger.debug("Failed to delete snapshot: " + ssSnapshotVO.getId() + " from storage");
+ s_logger.error(String.format("Failed to delete snapshot [%s] from storage due to: [%s].", snapshotUuid, e.getMessage()), e);
Review Comment:
```suggestion
s_logger.error(String.format("Failed to delete snapshot [%s] from storage due to: [%s].", snapshotUuid, e.getMessage()));
s_logger.debug(String.format("Failed to delete snapshot [%s] from storage.", snapshotUuid), e);
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1351615410
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java:
##########
@@ -418,15 +418,13 @@ public boolean deleteSnapshot(SnapshotInfo snapInfo) {
if (result.isFailed()) {
throw new CloudRuntimeException(result.getResult());
}
+ s_logger.debug(String.format("Successfully deleted snapshot [%s] with ID [%s].", snapInfo.getName(), snapInfo.getUuid()));
return true;
- } catch (InterruptedException e) {
- s_logger.debug("delete snapshot is failed: " + e.toString());
- } catch (ExecutionException e) {
- s_logger.debug("delete snapshot is failed: " + e.toString());
+ } catch (InterruptedException | ExecutionException e) {
+ s_logger.error(String.format("Failed to delete snapshot [%s] due to: [%s].", snapInfo.getUuid(), e.getMessage()), e);
Review Comment:
```suggestion
s_logger.error(String.format("Failed to delete snapshot [%s] due to: [%s].", snapInfo.getUuid(), e.getMessage()));
s_logger.debug(String.format("Failed to delete snapshot [%s].", snapInfo.getUuid()), e);
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "hsato03 (via GitHub)" <gi...@apache.org>.
hsato03 commented on code in PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#discussion_r1355773436
##########
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:
@DaanHoogland I made the changes to check if debug is enabled. By the way, the error log can no longer use the snapshot uuid.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Snapshot removal and storage cleanup logs [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8031:
URL: https://github.com/apache/cloudstack/pull/8031#issuecomment-1761176301
@GutoVeronezi @vishesh92 Are you allright with this PR?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org