fermosan opened a new pull request, #14498:
URL: https://github.com/apache/cloudstack/pull/14498
### Description
Fixes two unrelated-looking bugs that share the same root defect: code
that catches an
exception, then re-logs or re-throws it in a way that discards the real
error detail, leaving
only a generic/misleading message in the logs.
**1. `Script.executeInternal()` (two occurrences)** — when a draining
`OutputInterpreter`
(e.g. `AllLinesParser`) is used and the process exits non-zero, the
failure path opened a
*second* `BufferedReader` on the same process stream a background `Task`
had already consumed
and closed. Calling `processError()` on it always threw `IOException:
Stream closed` instead of
the real process output — the actual stdout/stderr content was silently
discarded even though
it had already been captured in `task.getResult()`. The success path a few
lines above already
does this correctly; the failure path never applied the same check.
**2. `DefaultModuleDefinitionSet.loadContext()`** — the caught exception
was passed to
`logger.error()` as an ordinary format-string argument instead of SLF4J's
trailing-throwable
argument, so the stack trace was never actually logged — only a one-line
`toString()` summary.
The re-thrown `RuntimeException` also didn't chain the original exception
as its cause, so even
a caller further up the stack couldn't recover it via `getCause()`.
Functional behavior change: none for the success/non-error paths. On
failure, both now surface
the real underlying error instead of a misleading placeholder.
<!-- Fixes: # --> *(not tied to a filed issue — found during manual
diagnosis, no tracking ticket exists yet)*
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [x] Minor
- [ ] Trivial
*(Neither bug causes incorrect behavior or data loss by itself — both are
observability bugs
that hide the real cause of an unrelated failure. Marked Minor on that
basis)*
### Screenshots (if appropriate):
N/A — logging/exception-handling fix, no UI or visible behavior change.
### How Has This Been Tested?
- Both changes compile cleanly (`mvn -pl :cloud-utils install -DskipTests`
and
`mvn -pl :cloud-framework-spring-module install -DskipTests`, both
`BUILD SUCCESS`).
- `DefaultModuleDefinitionSet` fix verified end-to-end: reproduced a real
module-load failure,
confirmed the log previously showed only `nested exception is
java.lang.ExceptionInInitializerError` with nothing further; after the
fix, rebuilding and
re-running shows the full cause chain down to the actual root exception.
- `Script.java` fix has not been exercised against a live non-zero-exit
drained-interpreter
failure in this round of testing — the only real-world repro we had was
a one-off incident
(malformed SSH key rejected by OpenSSH) that isn't easily reproduced on
demand. The fix is a
direct structural mirror of the already-correct success-path logic a few
lines above, in the
same method.
#### How did you try to break this feature and the system with this change?
- For `Script.java`: confirmed the non-draining-interpreter path
(`interpreter.drain() ==
false`) is completely untouched by the ternary — `processError(reader)`
still runs exactly as
before for that case. `task` is only read when `drain()` is true, which
is the same
precondition the existing success path already relies on, so no new
null-pointer path is
introduced.
- For `DefaultModuleDefinitionSet`: checked for any other code relying on
this
`RuntimeException` *not* having a cause set — found none. Setting the
cause is purely
additive; nothing downstream inspects or depends on `getCause()` being
null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
