You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2022/08/10 19:51:31 UTC
[GitHub] [cloudstack] paulazomig opened a new pull request, #6408: Improve log messages on VolumeOrchestrator class
paulazomig opened a new pull request, #6408:
URL: https://github.com/apache/cloudstack/pull/6408
### Description
This PR aims to improve several log entries in the VolumeOrchestrator class by adding more clear and precise information to facilitate future troubleshooting.
### 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)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [x] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### How Has This Been Tested?
The changes were tested by checking the log entries during volume manipulation (creating, deleting, migrating volumes between different storages, creating volumes from snapshots, etc.).
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#issuecomment-1152665342
UI build: :heavy_check_mark:
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6408 (SL-JID-1714)
--
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
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r890852596
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -1831,7 +1917,7 @@ public void cleanupStorageJobs() {
cleanupVolumeDuringSnapshotFailure(work.getVolumeId(), work.getSnapshotId());
}
} catch (Exception e) {
- s_logger.debug("clean up job failure, will continue", e);
+ s_logger.error(String.format("Clean up job failed due to [%s]. Will continue with other clean up jobs.", e.getMessage()), e);
Review Comment:
please do not log an exception on non fatal errors. if needed add a debug statement that logs the stacktrace but keep info and up clean.
--
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#issuecomment-1134770435
> I like the premise, but some of the implementation details worry me.
btw, I reviewed the sonarcloud bugs reported. I think we can ignore these for now as these are not really new ones.
As for the code smells, please have a look. Some of those can certainly be addressed;
> SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6408)
>
> [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG) [6 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL) [10 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL)
>
> [](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_coverage&view=list) [](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_duplicated_lines_density&view=list) [0.0% Duplica
tion](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r890856967
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -1153,13 +1192,13 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
AsyncCallFuture<VolumeApiResult> future = null;
for (VolumeVO expunge : toBeExpunged) {
+ String expungeToString = getReflectOnlySelectedFields(expunge);
+
future = volService.expungeVolumeAsync(volFactory.getVolume(expunge.getId()));
try {
future.get();
- } catch (InterruptedException e) {
- s_logger.debug("failed expunge volume" + expunge.getId(), e);
- } catch (ExecutionException e) {
- s_logger.debug("failed expunge volume" + expunge.getId(), e);
+ } catch (InterruptedException | ExecutionException e) {
+ s_logger.error(String.format("Failed to expunge volume [%s] due to [%s].", expungeToString, e.getMessage()), e);
Review Comment:
please don´t log error with exception. If need be add a debug that logs the stacktrace
--
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
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r890860072
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -546,15 +553,18 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume, Snapshot snapshot, Use
// create volume on primary from snapshot
AsyncCallFuture<VolumeApiResult> future = volService.createVolumeFromSnapshot(vol, store, snapInfo);
+ String snapshotToString = getReflectOnlySelectedFields(snapInfo.getSnapshotVO());
+
try {
VolumeApiResult result = future.get();
if (result.isFailed()) {
- s_logger.debug("Failed to create volume from snapshot:" + result.getResult());
- throw new CloudRuntimeException("Failed to create volume from snapshot:" + result.getResult());
+ String logMsg = String.format("Failed to create volume from snapshot [%s] due to [%s].", snapshotToString, result.getResult());
+ s_logger.error(logMsg);
+ throw new CloudRuntimeException(logMsg);
}
return result.getVolume();
} catch (InterruptedException | ExecutionException e) {
- String message = String.format("Failed to create volume from snapshot [%s] due to [%s].", snapInfo.getTO(), e.getMessage());
+ String message = String.format("Failed to create volume from snapshot [%s] due to [%s].", snapshotToString, e.getMessage());
s_logger.error(message, e);
Review Comment:
not your doing, but please don´t log error with exception. If need be add a debug that logs the stacktrace
--
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
[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#issuecomment-1146311371
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6408)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG) [6 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL) [7 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r890849832
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/PrimaryDataStoreImpl.java:
##########
@@ -91,6 +91,9 @@ public class PrimaryDataStoreImpl implements PrimaryDataStore {
private VolumeDao volumeDao;
private Map<String, String> _details;
+ private String uuid;
Review Comment:
ok, acceptable ;)
--
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
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r884105817
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -638,46 +655,57 @@ public VolumeInfo createVolume(VolumeInfo volume, VirtualMachine vm, VirtualMach
pool = findStoragePool(dskCh, dc, pod, clusterId, vm.getHostId(), vm, avoidPools);
if (pool == null) {
- s_logger.warn("Unable to find suitable primary storage when creating volume " + volume.getName());
- throw new CloudRuntimeException("Unable to find suitable primary storage when creating volume " + volume.getName());
+ String msg = String.format("Unable to find suitable primary storage when creating volume [%s].", volumeToString);
+ s_logger.error(msg);
+ throw new CloudRuntimeException(msg);
}
+ String poolToString = getReflectOnlySelectedFields(pool);
+
if (s_logger.isDebugEnabled()) {
- s_logger.debug("Trying to create " + volume + " on " + pool);
+ s_logger.debug(String.format("Trying to create volume [%s] on storage pool [%s].",
+ volumeToString, poolToString));
}
DataStore store = dataStoreMgr.getDataStore(pool.getId(), DataStoreRole.Primary);
for (int i = 0; i < 2; i++) {
// retry one more time in case of template reload is required for Vmware case
AsyncCallFuture<VolumeApiResult> future = null;
- boolean isNotCreatedFromTemplate = volume.getTemplateId() == null ? true : false;
+ boolean isNotCreatedFromTemplate = volumeInfo.getTemplateId() == null ? true : false;
Review Comment:
there are some others, like reducing cyclic complexity and extracting a string constant that would be easy wins. Not a -1, but easy wins they would be.
--
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
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r890856649
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -1235,12 +1280,10 @@ public Volume migrateVolume(Volume volume, StoragePool destPool) throws StorageU
}
}
return result.getVolume();
- } catch (InterruptedException e) {
- s_logger.debug("migrate volume failed", e);
- throw new CloudRuntimeException(e.getMessage());
- } catch (ExecutionException e) {
- s_logger.debug("migrate volume failed", e);
- throw new CloudRuntimeException(e.getMessage());
+ } catch (InterruptedException | ExecutionException e) {
+ String msg = String.format("Volume [%s] migration failed due to [%s].", volumeToString, e.getMessage());
+ s_logger.error(msg, e);
Review Comment:
please don´t log error with exception. If need be add a debug that logs the stacktrace
--
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
[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#issuecomment-1152699825
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6408)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG) [6 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL) [8 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [cloudstack] DaanHoogland merged pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland merged PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408
--
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
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r890858929
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -638,46 +655,57 @@ public VolumeInfo createVolume(VolumeInfo volume, VirtualMachine vm, VirtualMach
pool = findStoragePool(dskCh, dc, pod, clusterId, vm.getHostId(), vm, avoidPools);
if (pool == null) {
- s_logger.warn("Unable to find suitable primary storage when creating volume " + volume.getName());
- throw new CloudRuntimeException("Unable to find suitable primary storage when creating volume " + volume.getName());
+ String msg = String.format("Unable to find suitable primary storage when creating volume [%s].", volumeToString);
+ s_logger.error(msg);
+ throw new CloudRuntimeException(msg);
}
+ String poolToString = getReflectOnlySelectedFields(pool);
+
if (s_logger.isDebugEnabled()) {
- s_logger.debug("Trying to create " + volume + " on " + pool);
+ s_logger.debug(String.format("Trying to create volume [%s] on storage pool [%s].",
+ volumeToString, poolToString));
}
DataStore store = dataStoreMgr.getDataStore(pool.getId(), DataStoreRole.Primary);
for (int i = 0; i < 2; i++) {
// retry one more time in case of template reload is required for Vmware case
AsyncCallFuture<VolumeApiResult> future = null;
- boolean isNotCreatedFromTemplate = volume.getTemplateId() == null ? true : false;
+ boolean isNotCreatedFromTemplate = (volumeInfo.getTemplateId() == null);
if (isNotCreatedFromTemplate) {
- future = volService.createVolumeAsync(volume, store);
+ future = volService.createVolumeAsync(volumeInfo, store);
} else {
TemplateInfo templ = tmplFactory.getTemplate(template.getId(), DataStoreRole.Image);
- future = volService.createVolumeFromTemplateAsync(volume, store.getId(), templ);
+ future = volService.createVolumeFromTemplateAsync(volumeInfo, store.getId(), templ);
}
try {
VolumeApiResult result = future.get();
if (result.isFailed()) {
if (result.getResult().contains(REQUEST_TEMPLATE_RELOAD) && (i == 0)) {
- s_logger.debug("Retry template re-deploy for vmware");
+ s_logger.debug(String.format("Retrying to deploy template [%s] for VMware, attempt 2/2. ", templateToString));
continue;
} else {
- s_logger.debug("create volume failed: " + result.getResult());
- throw new CloudRuntimeException("create volume failed:" + result.getResult());
+ String msg = String.format("Failed to create volume [%s] due to [%s].", volumeToString, result.getResult());
+ s_logger.error(msg);
+ throw new CloudRuntimeException(msg);
}
}
return result.getVolume();
- } catch (InterruptedException e) {
- s_logger.error("create volume failed", e);
- throw new CloudRuntimeException("create volume failed", e);
- } catch (ExecutionException e) {
- s_logger.error("create volume failed", e);
- throw new CloudRuntimeException("create volume failed", e);
+ } catch (InterruptedException | ExecutionException e) {
+ String msg = String.format("Failed to create volume [%s] due to [%s].", volumeToString, e.getMessage());
+ s_logger.error(msg, e);
Review Comment:
please don´t log error with exception. If need be add a debug that logs the stacktrace
--
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
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r890856029
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -1250,18 +1293,18 @@ public Volume liveMigrateVolume(Volume volume, StoragePool destPool) {
VolumeInfo vol = volFactory.getVolume(volume.getId());
DataStore dataStoreTarget = dataStoreMgr.getDataStore(destPool.getId(), DataStoreRole.Primary);
AsyncCallFuture<VolumeApiResult> future = volService.migrateVolume(vol, dataStoreTarget);
+
+ String volToString = getReflectOnlySelectedFields(vol.getVolume());
+
try {
VolumeApiResult result = future.get();
if (result.isFailed()) {
- s_logger.debug("migrate volume failed:" + result.getResult());
+ s_logger.error(String.format("Volume [%s] migration failed due to [%s].", volToString, result.getResult()));
return null;
}
return result.getVolume();
- } catch (InterruptedException e) {
- s_logger.debug("migrate volume failed", e);
- return null;
- } catch (ExecutionException e) {
- s_logger.debug("migrate volume failed", e);
+ } catch (InterruptedException | ExecutionException e) {
+ s_logger.error(String.format("Volume [%s] migration failed due to [%s].", volToString, e.getMessage()), e);
Review Comment:
please don´t log error with exception. If need be add a debug that logs the stacktrace
--
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
[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#issuecomment-1139670919
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6408)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG) [6 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL) [9 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#issuecomment-1146272956
@acs-robot a Jenkins job has been kicked to build UI QA env. 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
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r884106455
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/PrimaryDataStoreImpl.java:
##########
@@ -91,6 +91,9 @@ public class PrimaryDataStoreImpl implements PrimaryDataStore {
private VolumeDao volumeDao;
private Map<String, String> _details;
+ private String uuid;
Review Comment:
The extra fields are not exposed and the `psdv` is available to retrieve the `uuid` and `name`, so why would you store the fields extra? the old implementation of `getUuid()` and `getName()` seems to suffice.
--
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
[GitHub] [cloudstack] codecov-commenter commented on pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#issuecomment-1211253998
# [Codecov](https://codecov.io/gh/apache/cloudstack/pull/6408?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> :exclamation: No coverage uploaded for pull request base (`main@96594ae`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#section-missing-base-commit).
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #6408 +/- ##
=======================================
Coverage ? 5.87%
Complexity ? 3933
=======================================
Files ? 2454
Lines ? 242614
Branches ? 37965
=======================================
Hits ? 14246
Misses ? 226791
Partials ? 1577
```
: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=The+Apache+Software+Foundation)
--
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
[GitHub] [cloudstack] acs-robot commented on pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
acs-robot commented on PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#issuecomment-1152660345
Found UI changes, kicking a new UI QA build
@blueorangutan ui
--
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
[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#issuecomment-1134760495
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6408)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG) [6 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6408&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL) [10 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6408&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6408&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [cloudstack] paulazomig commented on pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
paulazomig commented on PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#issuecomment-1139825160
@DaanHoogland About the code smells, most of them were not related to the changes made in this PR, but I addressed the ones that were and could be improved.
--
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
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r884105282
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -638,46 +655,57 @@ public VolumeInfo createVolume(VolumeInfo volume, VirtualMachine vm, VirtualMach
pool = findStoragePool(dskCh, dc, pod, clusterId, vm.getHostId(), vm, avoidPools);
if (pool == null) {
- s_logger.warn("Unable to find suitable primary storage when creating volume " + volume.getName());
- throw new CloudRuntimeException("Unable to find suitable primary storage when creating volume " + volume.getName());
+ String msg = String.format("Unable to find suitable primary storage when creating volume [%s].", volumeToString);
+ s_logger.error(msg);
+ throw new CloudRuntimeException(msg);
}
+ String poolToString = getReflectOnlySelectedFields(pool);
+
if (s_logger.isDebugEnabled()) {
- s_logger.debug("Trying to create " + volume + " on " + pool);
+ s_logger.debug(String.format("Trying to create volume [%s] on storage pool [%s].",
+ volumeToString, poolToString));
}
DataStore store = dataStoreMgr.getDataStore(pool.getId(), DataStoreRole.Primary);
for (int i = 0; i < 2; i++) {
// retry one more time in case of template reload is required for Vmware case
AsyncCallFuture<VolumeApiResult> future = null;
- boolean isNotCreatedFromTemplate = volume.getTemplateId() == null ? true : false;
+ boolean isNotCreatedFromTemplate = volumeInfo.getTemplateId() == null ? true : false;
Review Comment:
not due to your code @paulazomig , but an easy improvement.
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#issuecomment-1146277402
UI build: :heavy_check_mark:
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6408 (SL-JID-1673)
--
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
[GitHub] [cloudstack] acs-robot commented on pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
acs-robot commented on PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#issuecomment-1146272389
Found UI changes, kicking a new UI QA build
@blueorangutan ui
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#issuecomment-1152660932
@acs-robot a Jenkins job has been kicked to build UI QA env. 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
[GitHub] [cloudstack] paulazomig commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
paulazomig commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r883623598
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/PrimaryDataStoreImpl.java:
##########
@@ -91,6 +91,9 @@ public class PrimaryDataStoreImpl implements PrimaryDataStore {
private VolumeDao volumeDao;
private Map<String, String> _details;
+ private String uuid;
Review Comment:
@DaanHoogland the changes here were made in order to have the proper information to identify the objects in the log messages
--
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
[GitHub] [cloudstack] paulazomig commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
paulazomig commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r883623967
##########
engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/VolumeOrchestrationService.java:
##########
@@ -112,7 +112,7 @@ DiskProfile allocateRawVolume(Type type, String name, DiskOffering offering, Lon
void release(VirtualMachineProfile profile);
- void release(long vmId, long hostId);
+ void release(VirtualMachine vm, Host host);
Review Comment:
@DaanHoogland removed the changes
--
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
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r890859485
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -563,68 +573,75 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume, Snapshot snapshot, Use
}
- protected DiskProfile createDiskCharacteristics(VolumeInfo volume, VirtualMachineTemplate template, DataCenter dc, DiskOffering diskOffering) {
- if (volume.getVolumeType() == Type.ROOT && Storage.ImageFormat.ISO != template.getFormat()) {
+ protected DiskProfile createDiskCharacteristics(VolumeInfo volumeInfo, VirtualMachineTemplate template, DataCenter dc, DiskOffering diskOffering) {
+ if (volumeInfo.getVolumeType() == Type.ROOT && Storage.ImageFormat.ISO != template.getFormat()) {
+ String templateToString = getReflectOnlySelectedFields(template);
+ String zoneToString = getReflectOnlySelectedFields(dc);
+
TemplateDataStoreVO ss = _vmTemplateStoreDao.findByTemplateZoneDownloadStatus(template.getId(), dc.getId(), VMTemplateStorageResourceAssoc.Status.DOWNLOADED);
if (ss == null) {
- throw new CloudRuntimeException("Template " + template.getName() + " has not been completely downloaded to zone " + dc.getId());
+ throw new CloudRuntimeException(String.format("Template [%s] has not been completely downloaded to the zone [%s].",
+ templateToString, zoneToString));
}
- return new DiskProfile(volume.getId(), volume.getVolumeType(), volume.getName(), diskOffering.getId(), ss.getSize(), diskOffering.getTagsArray(), diskOffering.isUseLocalStorage(),
+ return new DiskProfile(volumeInfo.getId(), volumeInfo.getVolumeType(), volumeInfo.getName(), diskOffering.getId(), ss.getSize(), diskOffering.getTagsArray(), diskOffering.isUseLocalStorage(),
diskOffering.isRecreatable(), Storage.ImageFormat.ISO != template.getFormat() ? template.getId() : null);
} else {
- return new DiskProfile(volume.getId(), volume.getVolumeType(), volume.getName(), diskOffering.getId(), diskOffering.getDiskSize(), diskOffering.getTagsArray(),
+ return new DiskProfile(volumeInfo.getId(), volumeInfo.getVolumeType(), volumeInfo.getName(), diskOffering.getId(), diskOffering.getDiskSize(), diskOffering.getTagsArray(),
diskOffering.isUseLocalStorage(), diskOffering.isRecreatable(), null);
}
}
@DB
- public VolumeInfo copyVolumeFromSecToPrimary(VolumeInfo volume, VirtualMachine vm, VirtualMachineTemplate template, DataCenter dc, Pod pod, Long clusterId, ServiceOffering offering,
+ public VolumeInfo copyVolumeFromSecToPrimary(VolumeInfo volumeInfo, VirtualMachine vm, VirtualMachineTemplate template, DataCenter dc, Pod pod, Long clusterId, ServiceOffering offering,
DiskOffering diskOffering, List<StoragePool> avoids, long size, HypervisorType hyperType) throws NoTransitionException {
+ String volumeToString = getReflectOnlySelectedFields(volumeInfo.getVolume());
final HashSet<StoragePool> avoidPools = new HashSet<StoragePool>(avoids);
- DiskProfile dskCh = createDiskCharacteristics(volume, template, dc, diskOffering);
+ DiskProfile dskCh = createDiskCharacteristics(volumeInfo, template, dc, diskOffering);
dskCh.setHyperType(vm.getHypervisorType());
storageMgr.setDiskProfileThrottling(dskCh, null, diskOffering);
// Find a suitable storage to create volume on
StoragePool destPool = findStoragePool(dskCh, dc, pod, clusterId, null, vm, avoidPools);
if (destPool == null) {
- throw new CloudRuntimeException("Failed to find a suitable storage pool to create Volume in the pod/cluster of the provided VM "+ vm.getUuid());
+ throw new CloudRuntimeException(String.format("Failed to find a suitable storage pool in the pod/cluster of the provided VM [%s] to create the volume in.", vm));
}
DataStore destStore = dataStoreMgr.getDataStore(destPool.getId(), DataStoreRole.Primary);
- AsyncCallFuture<VolumeApiResult> future = volService.copyVolume(volume, destStore);
+ AsyncCallFuture<VolumeApiResult> future = volService.copyVolume(volumeInfo, destStore);
try {
VolumeApiResult result = future.get();
if (result.isFailed()) {
- s_logger.debug("copy volume failed: " + result.getResult());
- throw new CloudRuntimeException("copy volume failed: " + result.getResult());
+ String msg = String.format("Copy of the volume [%s] failed due to [%s].", volumeToString, result.getResult());
+ s_logger.error(msg);
+ throw new CloudRuntimeException(msg);
}
return result.getVolume();
- } catch (InterruptedException e) {
- s_logger.debug("Failed to copy volume: " + volume.getId(), e);
- throw new CloudRuntimeException("Failed to copy volume", e);
- } catch (ExecutionException e) {
- s_logger.debug("Failed to copy volume: " + volume.getId(), e);
- throw new CloudRuntimeException("Failed to copy volume", e);
+ } catch (InterruptedException | ExecutionException e) {
+ String msg = String.format("Failed to copy the volume [%s] due to [%s].", volumeToString, e.getMessage());
+ s_logger.error(msg, e);
Review Comment:
please don´t log error with exception. If need be add a debug that logs the stacktrace
--
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
[GitHub] [cloudstack] GutoVeronezi closed pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
GutoVeronezi closed pull request #6408: Improve log messages on VolumeOrchestrator class
URL: https://github.com/apache/cloudstack/pull/6408
--
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
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r879532158
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -546,15 +554,18 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume, Snapshot snapshot, Use
// create volume on primary from snapshot
AsyncCallFuture<VolumeApiResult> future = volService.createVolumeFromSnapshot(vol, store, snapInfo);
+ String snapshotToString = getReflectOnlySelectedFields(snapInfo.getSnapshotVO());
+
try {
VolumeApiResult result = future.get();
if (result.isFailed()) {
- s_logger.debug("Failed to create volume from snapshot:" + result.getResult());
- throw new CloudRuntimeException("Failed to create volume from snapshot:" + result.getResult());
+ String logMsg = String.format("Failed to create volume from snapshot [%s] due to [%s].", snapshotToString, result.getResult());
+ s_logger.error(logMsg);
Review Comment:
:+1:
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/PrimaryDataStoreImpl.java:
##########
@@ -91,6 +91,9 @@ public class PrimaryDataStoreImpl implements PrimaryDataStore {
private VolumeDao volumeDao;
private Map<String, String> _details;
+ private String uuid;
Review Comment:
why the changes in this file? they seem to only add maintenance risk.
##########
engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/VolumeOrchestrationService.java:
##########
@@ -171,5 +171,5 @@ DiskProfile importVolume(Type type, String name, DiskOffering offering, Long siz
/**
* Unmanage VM volumes
*/
- void unmanageVolumes(long vmId);
+ void unmanageVolumes(VirtualMachine vm);
Review Comment:
same here. Why should we pass around the object?
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -484,13 +489,14 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume, Snapshot snapshot, Use
}
if (pool == null) {
+ String logMsg = String.format("Could not find a storage pool in the pod/cluster of the provided VM [%s] to create the volume [%s] in.", vm, volumeToString);
+
//pool could not be found in the VM's pod/cluster.
if (s_logger.isDebugEnabled()) {
- s_logger.debug("Could not find any storage pool to create Volume in the pod/cluster of the provided VM " + vm.getUuid());
+ s_logger.error(logMsg);
Review Comment:
remove the if (isDebugEnabled()). doesn´t make sense anymore.
##########
engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/VolumeOrchestrationService.java:
##########
@@ -112,7 +112,7 @@ DiskProfile allocateRawVolume(Type type, String name, DiskOffering offering, Lon
void release(VirtualMachineProfile profile);
- void release(long vmId, long hostId);
+ void release(VirtualMachine vm, Host host);
Review Comment:
I am not to sure about passing objects from service to service here. Is this change needed?
--
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
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r884105245
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -638,46 +655,57 @@ public VolumeInfo createVolume(VolumeInfo volume, VirtualMachine vm, VirtualMach
pool = findStoragePool(dskCh, dc, pod, clusterId, vm.getHostId(), vm, avoidPools);
if (pool == null) {
- s_logger.warn("Unable to find suitable primary storage when creating volume " + volume.getName());
- throw new CloudRuntimeException("Unable to find suitable primary storage when creating volume " + volume.getName());
+ String msg = String.format("Unable to find suitable primary storage when creating volume [%s].", volumeToString);
+ s_logger.error(msg);
+ throw new CloudRuntimeException(msg);
}
+ String poolToString = getReflectOnlySelectedFields(pool);
+
if (s_logger.isDebugEnabled()) {
- s_logger.debug("Trying to create " + volume + " on " + pool);
+ s_logger.debug(String.format("Trying to create volume [%s] on storage pool [%s].",
+ volumeToString, poolToString));
}
DataStore store = dataStoreMgr.getDataStore(pool.getId(), DataStoreRole.Primary);
for (int i = 0; i < 2; i++) {
// retry one more time in case of template reload is required for Vmware case
AsyncCallFuture<VolumeApiResult> future = null;
- boolean isNotCreatedFromTemplate = volume.getTemplateId() == null ? true : false;
+ boolean isNotCreatedFromTemplate = volumeInfo.getTemplateId() == null ? true : false;
Review Comment:
```suggestion
boolean isNotCreatedFromTemplate = (volumeInfo.getTemplateId() == 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
[GitHub] [cloudstack] paulazomig commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
paulazomig commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r889246860
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/PrimaryDataStoreImpl.java:
##########
@@ -91,6 +91,9 @@ public class PrimaryDataStoreImpl implements PrimaryDataStore {
private VolumeDao volumeDao;
private Map<String, String> _details;
+ private String uuid;
Review Comment:
@DaanHoogland, sorry, my explanation was not clear in the first comment.
Those fields were added this way because we're using `ReflectionToStringBuilderUtils` to build the string to print in the log. As `ReflectionToStringBuilderUtils` uses the object attributes to build the string, this object is declared as an interface `StoragePool` (in runtime this object is an instance of `PrimaryDataStoreImpl`) and we cannot retrieve the `StoragePoolVO` contained in it, we declared these attributes (that already are there) to be reflected later.
--
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
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #6408: Improve log messages on VolumeOrchestrator class
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r890857697
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -1061,16 +1096,16 @@ public VolumeVO doInTransaction(TransactionStatus status) {
try {
stateTransitTo(existingVolume, Volume.Event.DestroyRequested);
} catch (NoTransitionException e) {
- s_logger.debug("Unable to destroy existing volume: " + e.toString());
+ s_logger.error(String.format("Unable to destroy existing volume [%s] due to [%s].", volumeToString, e.getMessage()));
}
// In case of VMware VM will continue to use the old root disk until expunged, so force expunge old root disk
if (vm.getHypervisorType() == HypervisorType.VMware) {
- s_logger.info("Expunging volume " + existingVolume.getId() + " from primary data store");
+ s_logger.info(String.format("Trying to expunge volume [%s] from primary data storage.", volumeToString));
AsyncCallFuture<VolumeApiResult> future = volService.expungeVolumeAsync(volFactory.getVolume(existingVolume.getId()));
try {
future.get();
} catch (Exception e) {
- s_logger.debug("Failed to expunge volume:" + existingVolume.getId(), e);
+ s_logger.error(String.format("Failed to expunge volume [%s] from primary data storage due to [%s].", volumeToString, e.getMessage()), e);
Review Comment:
please don´t log error with exception. If need be add a debug that logs the stacktrace
--
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