This repository has been archived by the owner on Oct 9, 2023. It is now read-only.
Finish simplify retry behavior PR #623
Merged
Conversation
This file contains 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
Signed-off-by: Dennis Keck <26092524+fellhorn@users.noreply.github.com>
Signed-off-by: Dennis Keck <26092524+fellhorn@users.noreply.github.com>
Signed-off-by: Dennis Keck <26092524+fellhorn@users.noreply.github.com>
Signed-off-by: Dennis Keck <26092524+fellhorn@users.noreply.github.com>
fellhorn
commented
Sep 27, 2023
| maxAttempts = uint32(*nCtx.Node().GetRetryStrategy().MinAttempts) | ||
| } | ||
| isEligible = currentAttempt < maxAttempts | ||
| currentAttempt = nodeStatus.GetAttempts() + 1 |
There was a problem hiding this comment.
We removed nodeStatus.GetSystemFailures(). As you can see in
currentAttempt = (nodeStatus.GetAttempts() + 1) - nodeStatus.GetSystemFailures()
(line 813)
GetAttempts() is the sum of system failures and user failures
When creating the test table we did not notice this as we always only used a single retry for system failures. For our final tests we now did some more extensive search for one off errors.
| } | ||
| maxAttempts = uint32(config.GetConfig().NodeConfig.DefaultMaxAttempts) | ||
| if nCtx.Node().GetRetryStrategy() != nil && nCtx.Node().GetRetryStrategy().MinAttempts != nil && *nCtx.Node().GetRetryStrategy().MinAttempts != 1 { |
There was a problem hiding this comment.
*nCtx.Node().GetRetryStrategy().MinAttempts != 1 # Changed this 0 -> 1
If a user sets 0 retries, MinAttempts will be 1:
https://github.com/fellhorn/flytepropeller/blob/feature/simplify-retries/pkg/compiler/transformers/k8s/utils.go#L24-L25
Comment on lines
-2783
to
+2787
| {"IneligibleSystemRetries", false, 0, 1, 1, 1, core.ExecutionError_SYSTEM, false}, | ||
| {"IneligibleSystemRetries", false, 1, 1, 1, 1, core.ExecutionError_SYSTEM, false}, | ||
| {"IgnoreCauseEligibleUserRetries", true, 0, 0, 2, 0, core.ExecutionError_USER, true}, | ||
| {"IgnoreCauseIneligibleUserRetries", true, 1, 0, 2, 0, core.ExecutionError_USER, false}, | ||
| {"IgnoreCauseEligibleSystemRetries", true, 0, 0, 2, 0, core.ExecutionError_SYSTEM, true}, | ||
| {"IgnoreCauseIneligibleSystemRetries", true, 0, 1, 2, 0, core.ExecutionError_SYSTEM, false}, | ||
| {"IgnoreCauseIneligibleSystemRetries", true, 1, 1, 2, 0, core.ExecutionError_SYSTEM, false}, |
There was a problem hiding this comment.
Tests were incorrect as attempts must be >= systemFailures (attempts is sum of user failures and system retries)
| interruptible = false | ||
| c.metrics.InterruptedThresholdHit.Inc(ctx) | ||
| } | ||
| } else { | ||
| // Else a node is not considered interruptible if the system failures have exceeded the configured threshold | ||
| if interruptible && ((c.interruptibleFailureThreshold > 0 && int32(s.GetSystemFailures()) >= c.interruptibleFailureThreshold) || | ||
| (c.interruptibleFailureThreshold <= 0 && int32(s.GetSystemFailures()) > int32(c.maxNodeRetriesForSystemFailures)+c.interruptibleFailureThreshold)) { | ||
| if interruptible && isAboveInterruptibleFailureThreshold(s.GetSystemFailures(), c.maxNodeRetriesForSystemFailures+1, c.interruptibleFailureThreshold) { |
There was a problem hiding this comment.
+1 because we use >= and use a shared function isAboveInterruptibleFailureThreshold which takes maxAttempts as argument
fellhorn
requested review from
kumare3,
EngHabu and
hamersaw
as code owners
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
TL;DR
As discussed with @hamersaw this brings the last finishing touches and fixes some one off bugs, see comments below.
Type
Are all requirements met?