Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle soroban loadgen mode misconfig gracefully #4559

Conversation

ThomasBrady
Copy link
Contributor

Description

Resolves #4473

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

SirTyson
SirTyson previously approved these changes Nov 26, 2024
Copy link
Contributor

@SirTyson SirTyson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, super minor cleanup

@ThomasBrady ThomasBrady force-pushed the handle-soroban-loadgen-mode-misconfig-graceful branch from ad5ae5a to b3f3ac5 Compare November 26, 2024 21:44
@ThomasBrady ThomasBrady requested a review from SirTyson November 26, 2024 21:45
Copy link
Contributor

@SirTyson SirTyson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Helpful change :)

@ThomasBrady ThomasBrady force-pushed the handle-soroban-loadgen-mode-misconfig-graceful branch from 2311b44 to 74e198e Compare November 27, 2024 18:27
@SirTyson SirTyson enabled auto-merge November 27, 2024 18:28
@SirTyson SirTyson added this pull request to the merge queue Nov 27, 2024
Merged via the queue into stellar:master with commit 7458426 Nov 27, 2024
13 checks passed
@marta-lokhova
Copy link
Contributor

Unless I'm missing where we catch the exception, this change doesn't handle the failure gracefully, does it? The node still crashes. I think loadgen should gracefully fail and the node should continue operating normally (otherwise ssc will be stuck in pod restart loop without displaying any useful info)

@ThomasBrady
Copy link
Contributor Author

Unless I'm missing where we catch the exception, this change doesn't handle the failure gracefully, does it? The node still crashes. I think loadgen should gracefully fail and the node should continue operating normally (otherwise ssc will be stuck in pod restart loop without displaying any useful info)

Sorry you are right, this isn't "graceful", however I see there being two problems with this flow: the silent/misleading failure ("Aborted (core dumped)") and gracefully propagating/handling the failure. This PR addressed the first part: if someone invokes without running setup they will now get an exception with the relevant error message. An exception is not exactly graceful however. I'm not sure how we can gracefully recover from this while also notifying the user. LoadGenerator::generateLoad returns void; we could print an error and reset the load generator, but if the user is depending on this invocation to succeed, their test will fail regardless. Wouldn't it be better to raise an exception closer to the problematic action?

@marta-lokhova
Copy link
Contributor

marta-lokhova commented Dec 2, 2024

Load generator sets loadgen.run.failed on failure, which is used across our test suite and in supercluster (e.g. supercluster observes the metric and fails the mission properly)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

handle soroban loadgen modes misconfiguration gracefully
3 participants