-
Notifications
You must be signed in to change notification settings - Fork 136
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
cmd/run: detect simulation errors and exit with appropriate rc #53
base: master
Are you sure you want to change the base?
Conversation
printGoodbye(failedSimNames != nil) | ||
if failedSimNames != nil { | ||
msg := fmt.Sprintf( | ||
"The following simulations experienced errors: %v", | ||
strings.Join(failedSimNames, ", ")) | ||
return errors.New(msg) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
printGoodbye
is a function which is called only once, so the only reason for its existence is to make the code cleaner. Splitting half of the goodbye message into printGoodbye
and leaving another half inlined in the function doesn't make sense. We should either pass the failed simulations into the printGoodbye or remove printGoodbye and inline everything.
@@ -412,6 +433,7 @@ func run(sims []*Simulation, bind simulator.BindAddr, size int) error { | |||
// TODO: some module can return custom messages (e.g. hijack) | |||
// and "ERROR" prefix shouldn't be printed then | |||
printMsg(sim, fmt.Sprintf("ERROR: %s: %s", host, err.Error())) | |||
failedSims[sim.Name()] = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this won't be misleading in some scenarios. What if we run the simulation against 10 hosts and one of them fails? Should we report the whole module as failed?
I don't have an answer here, but we need to define what it means for the module to fail. Maybe simple boolean flag is not enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also most probably we should extract this for loop body into a separate function/structure so we don't repeat the errors handling (printMsg+failedSims). But we before we do so we should address what we really want to report at the end and how to handle partial failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK... I was a little concerned that a single host failure was a bit too coarse. I'll mark this as a draft PR until we discuss further.
Thanks for the review.
Something along these lines: