Repository navigation
Conversation
Two unrelated-looking bugs share the same root defect: code that catches
an exception, then re-logs or re-throws it in a way that discards the
actual error detail.
- Script.executeInternal(): the non-zero-exit failure path opened a new
BufferedReader on a stream a draining Task had already consumed and
closed, so interpreter.processError() always threw "Stream closed"
instead of surfacing the real process output. Fixed by reusing
task.getResult() when interpreter.drain() is true, mirroring the
success path's existing logic. Two occurrences in the same file.
- DefaultModuleDefinitionSet.loadContext(): the caught exception was
passed as an ordinary format-string argument to logger.error() instead
of SLF4J's trailing-throwable argument, so the stack trace was never
logged. The re-thrown RuntimeException also didn't chain the original
cause. Fixed both: pass the exception as the throwable arg, and set it
as the new exception's cause.
Both were found while diagnosing real failures where the underlying
error was invisible in the logs and had to be recovered by manually
patching these exact lines.
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14498 +/- ##
============================================
+ Coverage 20.63% 20.77% +0.13%
+ Complexity 21695 20841 -854
============================================
Files 6489 6053 -436
Lines 590136 548184 -41952
Branches 72361 78666 +6305
============================================
- Hits 121788 113863 -7925
+ Misses 455100 420979 -34121
- Partials 13248 13342 +94
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✔️ suse15. SL-JID 19722 |
|
the el build failed but with errors that I would then expect on other builds as well. I am re-running the subjob to verify if it is a fluke FYI |
|
@blueorangutan package |
|
@fermosan a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19759 |
|
@DaanHoogland this time the build is ok for all. This is kinda strange. Surely has nothing to do with this specific PR. The changes are as minimal as they can get. |
so it was a fluke, this can be a repo sync issue or some other network related issue. Those are the most usual. Don't worry about your PR. |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-17285)
|
JoaoJandre
left a comment
JoaoJandre
left a comment
There was a problem hiding this comment.
CLGTM, did not test it
|
@blueorangutan test |
|
@RosiKyu a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
RosiKyu
left a comment
RosiKyu
left a comment
There was a problem hiding this comment.
Tested on a KVM env with the PR packages (24.0.0-shapeblue22296) on the management server and both KVM hosts, compared with a build without the fix.
DefaultModuleDefinitionSet: works. With a module whose context references a missing class, the log now shows the full stack trace and the "Caused by: java.lang.ClassNotFoundException" chain; before it was a single line.
Script.java: regression. With a draining interpreter (AllLinesParser), a command that fails now makes execute() / executeIgnoreExitValue() return null, because AllLinesParser.interpret() always returns null (the output is in getLines()). null means success to the callers, so the failure is no longer detected. Before this change the caller got a non-null value ("Stream closed" or ""), so at least the failure was seen.
Example on the KVM host, QemuImg.info() on a file that does not exist (qemu-img exits 1):
- without the PR: throws QemuImgException
- with the PR: no exception, returns {qemu-img=Could not open '/root/qa-does-not-exist.qcow2': Could not open '/root/qa-does-not-exist.qcow2': No such file or directory}
AllLinesParser is used in around 25 KVM and storage classes, so the failure path needs to keep returning a non-null value (for example the output captured by the parser, or the exit value when there is no output).
Regression otherwise fine: hosts reconnect, system VMs run, VM deploy/stop/start, volume create/attach and ROOT snapshot all work.
|
@fermosan could you please check my last comment and try addressing the identified regression? Thanks! |
|
[SF] Trillian test result (tid-17550)
|
|
@RosiKyu Interesting. In the new commit a small helper will make sure that all conditions are met and will handle execute() and executeIngoreExitValue(). |
|
@blueorangutan package |
|
@fermosan a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✔️ suse15. SL-JID 20035 |
|
@blueorangutan package |
|
@fermosan a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 20041 |
|
@blueorangutan test |
sorry, it is run in a private lab :( I'll run it for you |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-17578)
|
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 drainingOutputInterpreter(e.g.
AllLinesParser) is used and the process exits non-zero, the failure path opened asecond
BufferedReaderon the same process stream a backgroundTaskhad already consumedand closed. Calling
processError()on it always threwIOException: Stream closedinstead ofthe 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 alreadydoes this correctly; the failure path never applied the same check.
2.
DefaultModuleDefinitionSet.loadContext()— the caught exception was passed tologger.error()as an ordinary format-string argument instead of SLF4J's trailing-throwableargument, so the stack trace was never actually logged — only a one-line
toString()summary.The re-thrown
RuntimeExceptionalso didn't chain the original exception as its cause, so evena 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
*(not tied to a filed issue — found during manual diagnosis, no tracking ticket exists yet)*the real underlying error instead of a misleading placeholder.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
(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?
mvn -pl :cloud-utils install -DskipTestsandmvn -pl :cloud-framework-spring-module install -DskipTests, bothBUILD SUCCESS).DefaultModuleDefinitionSetfix verified end-to-end: reproduced a real module-load failure,confirmed the log previously showed only
nested exception is java.lang.ExceptionInInitializerErrorwith nothing further; after the fix, rebuilding andre-running shows the full cause chain down to the actual root exception.
Script.javafix has not been exercised against a live non-zero-exit drained-interpreterfailure 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?
Script.java: confirmed the non-draining-interpreter path (interpreter.drain() == false) is completely untouched by the ternary —processError(reader)still runs exactly asbefore for that case.
taskis only read whendrain()is true, which is the sameprecondition the existing success path already relies on, so no new null-pointer path is
introduced.
DefaultModuleDefinitionSet: checked for any other code relying on thisRuntimeExceptionnot having a cause set — found none. Setting the cause is purelyadditive; nothing downstream inspects or depends on
getCause()being null.