Add retry execute mechanism #634
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideAdds a retry-capable pod command execution method and improves handling of execution failures by ensuring execution completion is signaled and errors are logged across failure and exit paths. Sequence diagram for executeWithRetry pod command execution with failure handlingsequenceDiagram
participant Caller
participant PodShell
participant KubernetesApi
participant StateExecListener
Caller->>PodShell: executeWithRetry(commands)
loop up to 3 attempts
PodShell->>PodShell: execute(commands)
PodShell->>KubernetesApi: startExec(podName, commands, StateExecListener)
KubernetesApi-->>StateExecListener: onOpen()
alt command_execution_success
KubernetesApi-->>StateExecListener: onExit(code, status)
StateExecListener->>StateExecListener: executionDone.set(true)
StateExecListener-->>PodShell: hasExecutionFinished() == true
PodShell-->>Caller: PodShellOutput
PodShell->>PodShell: break loop
else command_execution_failure
KubernetesApi-->>StateExecListener: onFailure(throwable, response)
StateExecListener->>StateExecListener: log.error(...)
StateExecListener->>StateExecListener: executionDone.set(true)
StateExecListener-->>PodShell: hasExecutionFinished() == true
PodShell-->>PodShell: throw WaiterException
PodShell-->>Caller: propagate WaiterException
Caller-->>PodShell: WaiterException caught by executeWithRetry
PodShell->>PodShell: log.warn(attempt, podName, message)
alt attempt < 3
PodShell->>PodShell: Thread.sleep(5000)
else last_attempt
PodShell->>Caller: throw WaiterException("All attempts to execute command on pod ... have failed.")
PodShell->>PodShell: break loop
end
end
end
Updated class diagram for PodShell with executeWithRetry and StateExecListener changesclassDiagram
class PodShell {
- String podName
- Logger log
PodShellOutput execute(String[] commands)
PodShellOutput executeWithRetry(String[] commands)
}
class PodShellOutput {
- String stdout
- String stderr
}
class StateExecListener {
- AtomicBoolean executionDone
+ StateExecListener()
+ void onOpen()
+ void onFailure(Throwable throwable, Response response)
+ void onClose(int code, String reason)
+ void onExit(int code, Status status)
+ boolean hasExecutionFinished()
}
class ExecListener {
<<interface>>
+ void onOpen()
+ void onFailure(Throwable throwable, Response response)
+ void onClose(int code, String reason)
+ void onExit(int code, Status status)
}
class WaiterException {
+ WaiterException(String message)
}
class Logger {
+ void warn(String message, Object attempt, Object retryCount, Object podName, Object errorMessage)
+ void error(String message, Object podName, Object errorMessage)
}
class AtomicBoolean {
+ AtomicBoolean(boolean initialValue)
+ boolean get()
+ void set(boolean newValue)
}
class Response
class Status
PodShell "1" *-- "1" StateExecListener : creates
PodShell "1" o-- "1" PodShellOutput : returns
StateExecListener ..|> ExecListener : implements
PodShell ..> WaiterException : throws
PodShell ..> Logger : uses
StateExecListener ..> Logger : uses
StateExecListener ..> AtomicBoolean : uses
StateExecListener ..> Response : uses
StateExecListener ..> Status : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
laDok8
force-pushed
the
fix_os_timeout
branch
from
615967e to
54f446b
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Consider extracting POD_SHELL_RETRY_COUNT and POD_SHELL_RETRY_DELAY_MS into class-level constants or configuration to avoid magic numbers and to make tuning behavior easier without code changes.
- When the retry loop exhausts all attempts, it would be helpful to include the lastException as the cause in the final WaiterException so downstream code and logs retain the original failure context.
- In onExit, you might want to log non-zero exit codes (or the Status) to distinguish expected terminations from failures, since you now rely on this hook to mark execution as finished.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting POD_SHELL_RETRY_COUNT and POD_SHELL_RETRY_DELAY_MS into class-level constants or configuration to avoid magic numbers and to make tuning behavior easier without code changes.
- When the retry loop exhausts all attempts, it would be helpful to include the lastException as the cause in the final WaiterException so downstream code and logs retain the original failure context.
- In onExit, you might want to log non-zero exit codes (or the Status) to distinguish expected terminations from failures, since you now rely on this hook to mark execution as finished.
## Individual Comments
### Comment 1
<location> `core/src/main/java/cz/xtf/core/openshift/PodShell.java:78-87` </location>
<code_context>
+ WaiterException lastException = null;
</code_context>
<issue_to_address>
**issue:** The final WaiterException loses the original cause; consider chaining the last exception.
You store `lastException` but then create a new `WaiterException` without using it, which discards useful debugging context. Prefer chaining it as the cause, e.g. `throw new WaiterException("All attempts...", lastException);`, or at least incorporating its message so callers can inspect the underlying failure.
</issue_to_address>
### Comment 2
<location> `core/src/main/java/cz/xtf/core/openshift/PodShell.java:85-94` </location>
<code_context>
+ return this.execute(commands);
+ } catch (WaiterException e) {
+ lastException = e;
+ log.warn("Attempt {}/{} failed for command execution on pod {}: {}",
+ attempt, POD_SHELL_RETRY_COUNT, podName, e.getMessage());
+ if (attempt < POD_SHELL_RETRY_COUNT) {
</code_context>
<issue_to_address>
**suggestion:** Logging only the exception message drops stack traces; consider logging the Throwable itself.
Both the retry warning and the `onFailure` log pass only `getMessage()` to the logger. Instead, pass the exception as the final argument (e.g. `log.warn("Attempt {}/{} failed for command execution on pod {}", attempt, POD_SHELL_RETRY_COUNT, podName, e);` and `log.error("Execution failed in pod '{}'", podName, throwable);`) so the full stack trace and causes are logged while keeping the format consistent.
Suggested implementation:
```java
log.warn("Attempt {}/{} failed for command execution on pod {}",
attempt, POD_SHELL_RETRY_COUNT, podName, e);
```
Elsewhere in `PodShell.java` (or related classes) where failures are logged using only `throwable.getMessage()` (e.g. `log.error("Execution failed in pod '{}': {}", podName, throwable.getMessage());`), update them similarly to pass the throwable as the last parameter: `log.error("Execution failed in pod '{}'", podName, throwable);`.
</issue_to_address>
### Comment 3
<location> `core/src/main/java/cz/xtf/core/openshift/PodShell.java:108-114` </location>
<code_context>
@Override
public void onFailure(Throwable throwable, Response response) {
- // DO NOTHING
</code_context>
<issue_to_address>
**suggestion (bug_risk):** onExit ignores exit code and status; consider handling/logging non-zero exits explicitly.
Right now we only mark execution as finished and ignore failure details. If upstream logic uses `StateExecListener` to decide success or trigger retries, consider at least logging non-zero exit codes/status and/or propagating a failure state so callers can react correctly.
Suggested implementation:
```java
@Override
public void onExit(int code, Status status) {
if (code != 0) {
if (status != null) {
log.error(
"Command in pod '{}' exited with non-zero code {}. Status: reason='{}', message='{}', status='{}'",
podName,
code,
status.getReason(),
status.getMessage(),
status.getStatus()
);
} else {
log.error("Command in pod '{}' exited with non-zero code {} and no status information.", podName, code);
}
}
executionDone.set(true);
}
```
If callers should programmatically react to failures (e.g., retry on non-zero exit codes), you may also want to:
1. Add a failure flag to `StateExecListener` (e.g., `AtomicBoolean executionFailed`) and set it when `code != 0` or `onFailure` is called.
2. Expose that failure flag via an accessor or adapt existing logic that consumes `StateExecListener` to use it when deciding success vs retry.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
85
to
87
| log.warn("Attempt {}/{} failed for command execution on pod {}: {}", | ||
| attempt, POD_SHELL_RETRY_COUNT, podName, e.getMessage()); | ||
| if (attempt < POD_SHELL_RETRY_COUNT) { | ||
| try { | ||
| Thread.sleep(POD_SHELL_RETRY_DELAY_MS); | ||
| } catch (InterruptedException ie) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new RuntimeException("Thread has been interrupted!"); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: Logging only the exception message drops stack traces; consider logging the Throwable itself.
Both the retry warning and the onFailure log pass only getMessage() to the logger. Instead, pass the exception as the final argument (e.g. log.warn("Attempt {}/{} failed for command execution on pod {}", attempt, POD_SHELL_RETRY_COUNT, podName, e); and log.error("Execution failed in pod '{}'", podName, throwable);) so the full stack trace and causes are logged while keeping the format consistent.
Suggested implementation:
log.warn("Attempt {}/{} failed for command execution on pod {}",
attempt, POD_SHELL_RETRY_COUNT, podName, e);Elsewhere in PodShell.java (or related classes) where failures are logged using only throwable.getMessage() (e.g. log.error("Execution failed in pod '{}': {}", podName, throwable.getMessage());), update them similarly to pass the throwable as the last parameter: log.error("Execution failed in pod '{}'", podName, throwable);.
Comment on lines
108
to
117
| @Override | ||
| public void onFailure(Throwable throwable, Response response) { | ||
| // DO NOTHING | ||
| log.error("Execution failed in pod '{}': {}", podName, throwable.getMessage()); | ||
| executionDone.set(true); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
suggestion (bug_risk): onExit ignores exit code and status; consider handling/logging non-zero exits explicitly.
Right now we only mark execution as finished and ignore failure details. If upstream logic uses StateExecListener to decide success or trigger retries, consider at least logging non-zero exit codes/status and/or propagating a failure state so callers can react correctly.
Suggested implementation:
@Override
public void onExit(int code, Status status) {
if (code != 0) {
if (status != null) {
log.error(
"Command in pod '{}' exited with non-zero code {}. Status: reason='{}', message='{}', status='{}'",
podName,
code,
status.getReason(),
status.getMessage(),
status.getStatus()
);
} else {
log.error("Command in pod '{}' exited with non-zero code {} and no status information.", podName, code);
}
}
executionDone.set(true);
}If callers should programmatically react to failures (e.g., retry on non-zero exit codes), you may also want to:
- Add a failure flag to
StateExecListener(e.g.,AtomicBoolean executionFailed) and set it whencode != 0oronFailureis called. - Expose that failure flag via an accessor or adapt existing logic that consumes
StateExecListenerto use it when deciding success vs retry.
laDok8
commented
Jan 12, 2026
| public void onFailure(Throwable throwable, Response response) { | ||
| // DO NOTHING | ||
| log.error("Execution failed in pod '{}': {}", podName, throwable.getMessage()); | ||
| executionDone.set(true); |
Contributor
Author
There was a problem hiding this comment.
@mnovak thinking about this we have now way of knowing if command succeeded this way
Contributor
Author
There was a problem hiding this comment.
last changes should clear that up
laDok8
force-pushed
the
fix_os_timeout
branch
2 times, most recently
from
c55a177 to
db13605
Compare
laDok8
force-pushed
the
fix_os_timeout
branch
from
db13605 to
800517b
Compare
mnovak
approved these changes
Jan 14, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
onFailure()changes should result in fast fail.executeWithRetry()simply tries execute up to 3 times. This may take up to 3 minutes, but IMO it's preferable to making code more complex with parametrization.Summary by Sourcery
Introduce a retryable pod command execution path and ensure shell execution failures are surfaced and terminate waiting.
New Features:
Enhancements: