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/11/07 16:57:44 UTC
[GitHub] [cloudstack] stephankruggg opened a new pull request, #6874: Add logs to script and command execution
stephankruggg opened a new pull request, #6874:
URL: https://github.com/apache/cloudstack/pull/6874
### Description
This PR adds and improves existing logs when executing scripts and commands, since current information is not sufficient to enable 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
### How Has This Been Tested?
I applied the changes in a local lab and checked if the updated logs were successfully logged.
--
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 #6874: Add logs to script and command execution
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1305998356
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=6874)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6874&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6874&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6874&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6874&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6874&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6874&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6874&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6874&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=6874&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6874&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6874&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=6874&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6874&metric=new_coverage&view=list) [52.5% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6874&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6874&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=6874&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 #6874: Add logs to script and command execution
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1307405752
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. LL-JID 222
--
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] Add logs to script and command execution [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr closed pull request #6874: Add logs to script and command execution
URL: https://github.com/apache/cloudstack/pull/6874
--
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 #6874: Add logs to script and command execution
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1729403841
> As mentioned in comment [#6947 (comment)](https://github.com/apache/cloudstack/pull/6947#issuecomment-1611851403), I will be helping out @stephankruggg with some of his opened PRs.
>
> @DaanHoogland given that the patch for [upgrading Log4j ](https://github.com/apache/cloudstack/pull/7131) is already ready, is guarding these logs still necessary? Could we move on with this PR?
@GaOrtiga , let me put it like this; I will not involve myself in these discussions anymore, nor have I changed my opinion. I will stand in the way of it either.
--
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] GaOrtiga commented on pull request #6874: Add logs to script and command execution
Posted by "GaOrtiga (via GitHub)" <gi...@apache.org>.
GaOrtiga commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1729374773
As mentioned in comment https://github.com/apache/cloudstack/pull/6947#issuecomment-1611851403, I will be helping out @stephankruggg with some of his opened PRs.
@DaanHoogland given that the patch for [upgrading Log4j ](https://github.com/apache/cloudstack/pull/7131) is already ready, is guarding these logs still necessary? Could we move on 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
[GitHub] [cloudstack] blueorangutan commented on pull request #6874: Add logs to script and command execution
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1307344174
@DaanHoogland a 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
[GitHub] [cloudstack] DaanHoogland commented on pull request #6874: Add logs to script and command execution
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1307343454
@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] Add logs to script and command execution [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1744600116
@shwstppr 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] Add logs to script and command execution [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1744599679
@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
[GitHub] [cloudstack] stephankruggg commented on pull request #6874: Add logs to script and command execution
Posted by GitBox <gi...@apache.org>.
stephankruggg commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1313607460
> > @DaanHoogland, the isEnabled check is detrimental in terms of code legibility. In my opinion, this check's barely noticeable performance gain is not sufficient to counter this negative impact, so it should not be used in this scenario.
>
> I don´t agree. we could change the log library to add variable parameters to the trace/debug/info messages to make the log line look cleaner. I´m very extrmely for legibility but not at cost of neat following of best practices.
I understand; however, it is not consensus that the isEnabled check should be used, and the scenarios here do not require so much processing in order to make it necessary to validate the log level before creating the string, so I rather not add them.
--
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[bot] commented on pull request #6874: Add logs to script and command execution
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1305968288
# [Codecov](https://codecov.io/gh/apache/cloudstack/pull/6874?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6874](https://codecov.io/gh/apache/cloudstack/pull/6874?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (809be7b) into [main](https://codecov.io/gh/apache/cloudstack/commit/1ee58ec3477c9f277b1151803c4e8df502d381a2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1ee58ec) will **decrease** coverage by `0.00%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #6874 +/- ##
============================================
- Coverage 10.85% 10.84% -0.01%
+ Complexity 7106 7105 -1
============================================
Files 2485 2485
Lines 245417 245417
Branches 38326 38326
============================================
- Hits 26631 26622 -9
- Misses 215516 215526 +10
+ Partials 3270 3269 -1
```
| [Impacted Files](https://codecov.io/gh/apache/cloudstack/pull/6874?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...rg/apache/cloudstack/quota/QuotaStatementImpl.java](https://codecov.io/gh/apache/cloudstack/pull/6874/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZnJhbWV3b3JrL3F1b3RhL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jbG91ZHN0YWNrL3F1b3RhL1F1b3RhU3RhdGVtZW50SW1wbC5qYXZh) | `36.28% <0.00%> (-3.99%)` | :arrow_down: |
: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] stephankruggg commented on pull request #6874: Add logs to script and command execution
Posted by GitBox <gi...@apache.org>.
stephankruggg commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1307806417
@DaanHoogland, as discussed [here](https://github.com/apache/cloudstack/pull/4707#discussion_r578495829), the isEnabled check is detrimental in terms of code legibility. In my opinion, this check's barely noticeable performance gain is not sufficient to counter this negative impact, so it should not be used in this scenario.
--
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 #6874: Add logs to script and command execution
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1312694783
> @DaanHoogland, the isEnabled check is detrimental in terms of code legibility. In my opinion, this check's barely noticeable performance gain is not sufficient to counter this negative impact, so it should not be used in this scenario.
I don´t agree. we could change the log library to add variable parameters to the trace/debug/info messages to make the log line look cleaner. I´m very extrmely for legibility but not at cost of neat following of best practices.
--
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 #6874: Add logs to script and command execution
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on code in PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#discussion_r1016720443
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -205,19 +205,20 @@ static String stackTraceAsString(Throwable throwable) {
public String execute(OutputInterpreter interpreter) {
String[] command = _command.toArray(new String[_command.size()]);
- if (_logger.isDebugEnabled()) {
- _logger.debug("Executing: " + buildCommandLine(command).split(KeyStoreUtils.KS_FILENAME)[0]);
- }
+ String commandLine = buildCommandLine(command);
+ _logger.debug(String.format("Executing command [%s].", commandLine.split(KeyStoreUtils.KS_FILENAME)[0]));
try {
+ _logger.trace(String.format("Creating process for command [%s].", commandLine));
ProcessBuilder pb = new ProcessBuilder(command);
pb.redirectErrorStream(true);
if (_workDir != null)
pb.directory(new File(_workDir));
+ _logger.trace(String.format("Starting process for command [%s].", commandLine));
Review Comment:
```suggestion
if (_logger.isTraceEnabled()) {
_logger.trace(String.format("Starting process for command [%s].", commandLine));
}
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -226,40 +227,48 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
+ _logger.trace(String.format("Scheduling the execution of command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
future = s_executors.schedule(this, _timeout, TimeUnit.MILLISECONDS);
}
+ long processPid = _process.pid();
Task task = null;
if (interpreter != null && interpreter.drain()) {
+ _logger.trace(String.format("Executing interpreting task of process [%s] for command [%s].", processPid, commandLine));
task = new Task(interpreter, ir);
s_executors.execute(task);
}
while (true) {
- _logger.debug("Executing while with timeout : " + _timeout);
+ _logger.trace(String.format("Attempting process [%s] execution for command [%s] with timeout [%s].", processPid, commandLine, _timeout));
Review Comment:
```suggestion
if (_logger.isTraceEnabled()) {
_logger.trace(String.format("Attempting process [%s] execution for command [%s] with timeout [%s].", processPid, commandLine, _timeout));
}
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -205,19 +205,20 @@ static String stackTraceAsString(Throwable throwable) {
public String execute(OutputInterpreter interpreter) {
String[] command = _command.toArray(new String[_command.size()]);
- if (_logger.isDebugEnabled()) {
- _logger.debug("Executing: " + buildCommandLine(command).split(KeyStoreUtils.KS_FILENAME)[0]);
- }
+ String commandLine = buildCommandLine(command);
+ _logger.debug(String.format("Executing command [%s].", commandLine.split(KeyStoreUtils.KS_FILENAME)[0]));
Review Comment:
```suggestion
String commandLine = buildCommandLine(command);
if (_logger.isDebugEnabled()) {
_logger.debug(String.format("Executing command [%s].", commandLine.split(KeyStoreUtils.KS_FILENAME)[0]));
}
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -226,40 +227,48 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
+ _logger.trace(String.format("Scheduling the execution of command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
future = s_executors.schedule(this, _timeout, TimeUnit.MILLISECONDS);
}
+ long processPid = _process.pid();
Task task = null;
if (interpreter != null && interpreter.drain()) {
+ _logger.trace(String.format("Executing interpreting task of process [%s] for command [%s].", processPid, commandLine));
Review Comment:
```suggestion
if (_logger.isTraceEnabled()) {
_logger.trace(String.format("Executing interpreting task of process [%s] for command [%s].", processPid, commandLine));
}
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -269,21 +278,21 @@ public String execute(OutputInterpreter interpreter) {
Thread.interrupted();
}
- //timeout without completing the process
TimedOutLogger log = new TimedOutLogger(_process);
Task timedoutTask = new Task(log, ir);
+ _logger.trace(String.format("Running timed out task of process [%s] for command [%s].", processPid, commandLine));
timedoutTask.run();
if (!_passwordCommand) {
- _logger.warn("Timed out: " + buildCommandLine(command) + ". Output is: " + timedoutTask.getResult());
+ _logger.warn(String.format("Process [%s] for command [%s] timed out. Output is [%s].", processPid, commandLine, timedoutTask.getResult()));
} else {
- _logger.warn("Timed out: " + buildCommandLine(command));
+ _logger.warn(String.format("Process [%s] for command [%s] timed out.", processPid, commandLine));
}
return ERR_TIMEOUT;
}
- _logger.debug("Exit value is " + _process.exitValue());
+ _logger.debug(String.format("Exit value of process [%s] for command [%s] is [%s].", processPid, commandLine, _process.exitValue()));
Review Comment:
```suggestion
if (_logger.isDebugEnabled()) {
_logger.debug(String.format("Exit value of process [%s] for command [%s] is [%s].", processPid, commandLine, _process.exitValue()));
}
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -226,40 +227,48 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
+ _logger.trace(String.format("Scheduling the execution of command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
future = s_executors.schedule(this, _timeout, TimeUnit.MILLISECONDS);
}
+ long processPid = _process.pid();
Task task = null;
if (interpreter != null && interpreter.drain()) {
+ _logger.trace(String.format("Executing interpreting task of process [%s] for command [%s].", processPid, commandLine));
task = new Task(interpreter, ir);
s_executors.execute(task);
}
while (true) {
- _logger.debug("Executing while with timeout : " + _timeout);
+ _logger.trace(String.format("Attempting process [%s] execution for command [%s] with timeout [%s].", processPid, commandLine, _timeout));
try {
- //process execution completed within timeout period
if (_process.waitFor(_timeout, TimeUnit.MILLISECONDS)) {
- //process completed successfully
+ _logger.trace(String.format("Process [%s] execution for command [%s] completed within timeout period [%s].", processPid, commandLine,
+ _timeout));
if (_process.exitValue() == 0) {
- _logger.debug("Execution is successful.");
+ _logger.debug(String.format("Successfully executed process [%s] for command [%s].", processPid, commandLine));
Review Comment:
```suggestion
if (_logger.isDebugEnabled()) {
_logger.debug(String.format("Successfully executed process [%s] for command [%s].", processPid, commandLine));
}
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -226,40 +227,48 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
+ _logger.trace(String.format("Scheduling the execution of command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
future = s_executors.schedule(this, _timeout, TimeUnit.MILLISECONDS);
}
+ long processPid = _process.pid();
Task task = null;
if (interpreter != null && interpreter.drain()) {
+ _logger.trace(String.format("Executing interpreting task of process [%s] for command [%s].", processPid, commandLine));
task = new Task(interpreter, ir);
s_executors.execute(task);
}
while (true) {
- _logger.debug("Executing while with timeout : " + _timeout);
+ _logger.trace(String.format("Attempting process [%s] execution for command [%s] with timeout [%s].", processPid, commandLine, _timeout));
try {
- //process execution completed within timeout period
if (_process.waitFor(_timeout, TimeUnit.MILLISECONDS)) {
- //process completed successfully
+ _logger.trace(String.format("Process [%s] execution for command [%s] completed within timeout period [%s].", processPid, commandLine,
+ _timeout));
if (_process.exitValue() == 0) {
- _logger.debug("Execution is successful.");
+ _logger.debug(String.format("Successfully executed process [%s] for command [%s].", processPid, commandLine));
if (interpreter != null) {
- return interpreter.drain() ? task.getResult() : interpreter.interpret(ir);
+ if (interpreter.drain()) {
+ _logger.trace(String.format("Returning task result of process [%s] for command [%s].", processPid, commandLine));
+ return task.getResult();
+ }
+ _logger.trace(String.format("Returning interpretation of process [%s] for command [%s].", processPid, commandLine));
+ return interpreter.interpret(ir);
} else {
// null return exitValue apparently
+ _logger.trace(String.format("Process [%s] for command [%s] exited with value [%s].", processPid, commandLine,
+ _process.exitValue()));
return String.valueOf(_process.exitValue());
}
- } else { //process failed
+ } else {
+ _logger.warn(String.format("Execution of process [%s] for command [%s] failed.", processPid, commandLine));
break;
}
- } //timeout
+ }
} catch (InterruptedException e) {
if (!_isTimeOut) {
- /*
- * This is not timeout, we are interrupted by others,
- * continue
- */
- _logger.debug("We are interrupted but it's not a timeout, just continue");
+ _logger.debug(String.format("Exception [%s] occurred; however, it was not a timeout. Therefore, proceeding with the execution of process [%s] for command "
+ + "[%s].", e.getMessage(), processPid, commandLine), e);
Review Comment:
```suggestion
if (_logger.isDebugEnabled()) {
_logger.debug(String.format("Exception [%s] occurred; however, it was not a timeout. Therefore, proceeding with the execution of process [%s] for command "
+ "[%s].", e.getMessage(), processPid, commandLine), e);
}
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -269,21 +278,21 @@ public String execute(OutputInterpreter interpreter) {
Thread.interrupted();
}
- //timeout without completing the process
TimedOutLogger log = new TimedOutLogger(_process);
Task timedoutTask = new Task(log, ir);
+ _logger.trace(String.format("Running timed out task of process [%s] for command [%s].", processPid, commandLine));
Review Comment:
```suggestion
if (_logger.isTraceEnabled()) {
_logger.trace(String.format("Running timed out task of process [%s] for command [%s].", processPid, commandLine));
}
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -205,19 +205,20 @@ static String stackTraceAsString(Throwable throwable) {
public String execute(OutputInterpreter interpreter) {
String[] command = _command.toArray(new String[_command.size()]);
- if (_logger.isDebugEnabled()) {
- _logger.debug("Executing: " + buildCommandLine(command).split(KeyStoreUtils.KS_FILENAME)[0]);
- }
+ String commandLine = buildCommandLine(command);
+ _logger.debug(String.format("Executing command [%s].", commandLine.split(KeyStoreUtils.KS_FILENAME)[0]));
try {
+ _logger.trace(String.format("Creating process for command [%s].", commandLine));
Review Comment:
```suggestion
if (_logger.isTraceEnabled()) {
_logger.trace(String.format("Creating process for command [%s].", commandLine));
}
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -226,40 +227,48 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
+ _logger.trace(String.format("Scheduling the execution of command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
future = s_executors.schedule(this, _timeout, TimeUnit.MILLISECONDS);
}
+ long processPid = _process.pid();
Task task = null;
if (interpreter != null && interpreter.drain()) {
+ _logger.trace(String.format("Executing interpreting task of process [%s] for command [%s].", processPid, commandLine));
task = new Task(interpreter, ir);
s_executors.execute(task);
}
while (true) {
- _logger.debug("Executing while with timeout : " + _timeout);
+ _logger.trace(String.format("Attempting process [%s] execution for command [%s] with timeout [%s].", processPid, commandLine, _timeout));
try {
- //process execution completed within timeout period
if (_process.waitFor(_timeout, TimeUnit.MILLISECONDS)) {
- //process completed successfully
+ _logger.trace(String.format("Process [%s] execution for command [%s] completed within timeout period [%s].", processPid, commandLine,
+ _timeout));
if (_process.exitValue() == 0) {
- _logger.debug("Execution is successful.");
+ _logger.debug(String.format("Successfully executed process [%s] for command [%s].", processPid, commandLine));
if (interpreter != null) {
- return interpreter.drain() ? task.getResult() : interpreter.interpret(ir);
+ if (interpreter.drain()) {
+ _logger.trace(String.format("Returning task result of process [%s] for command [%s].", processPid, commandLine));
+ return task.getResult();
+ }
+ _logger.trace(String.format("Returning interpretation of process [%s] for command [%s].", processPid, commandLine));
Review Comment:
```suggestion
if (_logger.isTraceEnabled()) {
_logger.trace(String.format("Returning interpretation of process [%s] for command [%s].", processPid, commandLine));
}
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -226,40 +227,48 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
+ _logger.trace(String.format("Scheduling the execution of command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
Review Comment:
```suggestion
if (_logger.isTraceEnabled() ) {
_logger.trace(String.format("Scheduling the execution of command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
}
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -226,40 +227,48 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
+ _logger.trace(String.format("Scheduling the execution of command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
future = s_executors.schedule(this, _timeout, TimeUnit.MILLISECONDS);
}
+ long processPid = _process.pid();
Task task = null;
if (interpreter != null && interpreter.drain()) {
+ _logger.trace(String.format("Executing interpreting task of process [%s] for command [%s].", processPid, commandLine));
task = new Task(interpreter, ir);
s_executors.execute(task);
}
while (true) {
- _logger.debug("Executing while with timeout : " + _timeout);
+ _logger.trace(String.format("Attempting process [%s] execution for command [%s] with timeout [%s].", processPid, commandLine, _timeout));
try {
- //process execution completed within timeout period
if (_process.waitFor(_timeout, TimeUnit.MILLISECONDS)) {
- //process completed successfully
+ _logger.trace(String.format("Process [%s] execution for command [%s] completed within timeout period [%s].", processPid, commandLine,
+ _timeout));
if (_process.exitValue() == 0) {
- _logger.debug("Execution is successful.");
+ _logger.debug(String.format("Successfully executed process [%s] for command [%s].", processPid, commandLine));
if (interpreter != null) {
- return interpreter.drain() ? task.getResult() : interpreter.interpret(ir);
+ if (interpreter.drain()) {
+ _logger.trace(String.format("Returning task result of process [%s] for command [%s].", processPid, commandLine));
Review Comment:
```suggestion
if (_logger.isTraceEnabled()) {
_logger.trace(String.format("Returning task result of process [%s] for command [%s].", processPid, commandLine));
}
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -226,40 +227,48 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
+ _logger.trace(String.format("Scheduling the execution of command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
future = s_executors.schedule(this, _timeout, TimeUnit.MILLISECONDS);
}
+ long processPid = _process.pid();
Task task = null;
if (interpreter != null && interpreter.drain()) {
+ _logger.trace(String.format("Executing interpreting task of process [%s] for command [%s].", processPid, commandLine));
task = new Task(interpreter, ir);
s_executors.execute(task);
}
while (true) {
- _logger.debug("Executing while with timeout : " + _timeout);
+ _logger.trace(String.format("Attempting process [%s] execution for command [%s] with timeout [%s].", processPid, commandLine, _timeout));
try {
- //process execution completed within timeout period
if (_process.waitFor(_timeout, TimeUnit.MILLISECONDS)) {
- //process completed successfully
+ _logger.trace(String.format("Process [%s] execution for command [%s] completed within timeout period [%s].", processPid, commandLine,
+ _timeout));
Review Comment:
```suggestion
if (_logger.isTraceEnabled()) {
_logger.trace(String.format("Process [%s] execution for command [%s] completed within timeout period [%s].", processPid, commandLine,
_timeout));
}
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -226,40 +227,48 @@ public String execute(OutputInterpreter interpreter) {
_thread = Thread.currentThread();
ScheduledFuture<String> future = null;
if (_timeout > 0) {
+ _logger.trace(String.format("Scheduling the execution of command [%s] with a timeout of [%s] milliseconds.", commandLine, _timeout));
future = s_executors.schedule(this, _timeout, TimeUnit.MILLISECONDS);
}
+ long processPid = _process.pid();
Task task = null;
if (interpreter != null && interpreter.drain()) {
+ _logger.trace(String.format("Executing interpreting task of process [%s] for command [%s].", processPid, commandLine));
task = new Task(interpreter, ir);
s_executors.execute(task);
}
while (true) {
- _logger.debug("Executing while with timeout : " + _timeout);
+ _logger.trace(String.format("Attempting process [%s] execution for command [%s] with timeout [%s].", processPid, commandLine, _timeout));
try {
- //process execution completed within timeout period
if (_process.waitFor(_timeout, TimeUnit.MILLISECONDS)) {
- //process completed successfully
+ _logger.trace(String.format("Process [%s] execution for command [%s] completed within timeout period [%s].", processPid, commandLine,
+ _timeout));
if (_process.exitValue() == 0) {
- _logger.debug("Execution is successful.");
+ _logger.debug(String.format("Successfully executed process [%s] for command [%s].", processPid, commandLine));
if (interpreter != null) {
- return interpreter.drain() ? task.getResult() : interpreter.interpret(ir);
+ if (interpreter.drain()) {
+ _logger.trace(String.format("Returning task result of process [%s] for command [%s].", processPid, commandLine));
+ return task.getResult();
+ }
+ _logger.trace(String.format("Returning interpretation of process [%s] for command [%s].", processPid, commandLine));
+ return interpreter.interpret(ir);
} else {
// null return exitValue apparently
+ _logger.trace(String.format("Process [%s] for command [%s] exited with value [%s].", processPid, commandLine,
+ _process.exitValue()));
Review Comment:
```suggestion
if (_logger.isTraceEnabled()) {
_logger.trace(String.format("Process [%s] for command [%s] exited with value [%s].", processPid, commandLine,
_process.exitValue()));
}
```
--
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] Add logs to script and command execution [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1744735045
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 7205
--
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] Add logs to script and command execution [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1744851027
@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] Add logs to script and command execution [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1744848286
@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] Add logs to script and command execution [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr closed pull request #6874: Add logs to script and command execution
URL: https://github.com/apache/cloudstack/pull/6874
--
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] Add logs to script and command execution [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1746024651
<b>[SF] Trillian test result (tid-7825)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 48594 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6874-t7825-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` | 665.97 | test_kubernetes_clusters.py
test_05_vmschedule_test_e2e | `Failure` | 270.97 | test_vm_schedule.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] Add logs to script and command execution [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874#issuecomment-1787289413
I am not +1, more like -0, but too many do not agree with my stance on the logging issue, so I'll merge this anyway.
--
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] Add logs to script and command execution [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland merged PR #6874:
URL: https://github.com/apache/cloudstack/pull/6874
--
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