Explicitly cover only specific plan status for cleanup plans #1358
Conversation
This ensure that cleanup plans aren't (re-)started after a fatal error. Signed-off-by: Jan Schlicht <jan@d2iq.com>
| return plan, nil | ||
| case kudov1beta1.ExecutionComplete: |
There was a problem hiding this comment.
What happens in the case of ExecutionFatalError?
We should probably check for planStatus.isTerminal, i guess?
There was a problem hiding this comment.
I just looked at the surrounding code. I think the if i.isDeleting() { block should always return, at the moment it falls through to i.GetPlanInProgress() != nil { etc. This might lead to the correct result, but it's not really clear from the code what actually should happen
There was a problem hiding this comment.
I think the
if i.isDeleting() {block should always return, at the moment it falls through toi.GetPlanInProgress() != nil {
the i.GetPlanInProgress is outside of the i.isDeleting block. The problem here is/was that if !planStatus.Status.IsRunning() on the left side also covers the ExecutionFatalError case, where the plan is returned here and then reseted/restarted in the caller (we obviously don't want to infinitely restart plans in FATAL_ERROR status).
I'm reworking this part of logic as part of my PR so I would go for a bigger refactoring here.
There was a problem hiding this comment.
Yes, the left side is certainly not correct. I'm just noted that the new code does not clearly states what happens in case of a ExecutionFatalError. If we add an additional
case kudov1beta1.ExecutionFatalError:
// Don't restart in case of a fatal error
return nil, nil
it would feel a lot more clear to me.
There was a problem hiding this comment.
This is implicitly covered by the subsequent i.GetPlanInProgress() check and thus redundant but I don't mind.
There was a problem hiding this comment.
I'd like to see it changed, but it's not really a requirement for me, so approving for now.
| return nil, nil | ||
| } | ||
| switch planStatus.Status { | ||
| case kudov1beta1.ExecutionNeverRun: |
There was a problem hiding this comment.
I assume this would be ok, as the the cleanup plan is only ran on the deletion of the instance CR?
| return plan, nil | ||
| case kudov1beta1.ExecutionComplete: |
There was a problem hiding this comment.
I think the
if i.isDeleting() {block should always return, at the moment it falls through toi.GetPlanInProgress() != nil {
the i.GetPlanInProgress is outside of the i.isDeleting block. The problem here is/was that if !planStatus.Status.IsRunning() on the left side also covers the ExecutionFatalError case, where the plan is returned here and then reseted/restarted in the caller (we obviously don't want to infinitely restart plans in FATAL_ERROR status).
I'm reworking this part of logic as part of my PR so I would go for a bigger refactoring here.
This ensure that cleanup plans aren't (re-)started after a fatal error. Signed-off-by: Jan Schlicht <jan@d2iq.com> Signed-off-by: Thomas Runyon <runyontr@gmail.com>
What this PR does / why we need it:
This ensure that cleanup plans aren't (re-)started after a fatal error.
Fixes #1357