fix: requeue transient policy snapshot lookup errors - #796
Open
Jack Walker (jackwalkerlabs) wants to merge 1 commit into
Open
fix: requeue transient policy snapshot lookup errors#796Jack Walker (jackwalkerlabs) wants to merge 1 commit into
Jack Walker (jackwalkerlabs) wants to merge 1 commit into
Conversation
Signed-off-by: Jack Walker <jackwalkerlabs@gmail.com>
Member
|
Hi Jack! Thanks so much for the fix 🙏 I think it looks great. |
Chen Yu (michaelawyu)
left a comment
Chen Yu (michaelawyu)
left a comment
Member
There was a problem hiding this comment.
Some minor comments. Thanks again for submitting the fix.
| @@ -0,0 +1,146 @@ | |||
| /* | |||
Member
There was a problem hiding this comment.
Just a minor issue with the UTs: if possible, could this be merged into the current scheduler_test.go file?
| "github.com/kubefleet-dev/kubefleet/pkg/scheduler/queue" | ||
| ) | ||
|
|
||
| type trackingPlacementSchedulingQueue struct { |
Member
There was a problem hiding this comment.
Hi Jack! It might be easier to stub the rate limiter and use the simple queue instead. But that's not a blocker in any way.
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
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.
Description of your changes
The scheduler treated every failure to look up the latest policy snapshot as watcher-recoverable. That is correct when no latest snapshot exists (and for the multiple-active-snapshot invariant), but a transient API-server or cache
Listfailure has no guaranteed follow-up event, soForgetcould drop a quiet placement indefinitely.This change branches on the two dedicated non-requeue sentinels. Those cases still call
Forget, while all other lookup failures callAddRateLimitedand retry with backoff.Fixes #681
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
main:go test ./pkg/scheduler -run TestScheduleOncePolicySnapshotLookupErrors -count=1failed because API-server and unexpected cache errors were forgotten instead of rate-limited.KUBEBUILDER_ASSETS="$(make --silent kubebuilder-assets-path)" go test ./pkg/scheduler/... -count=1CGO_ENABLED=1 KUBEBUILDER_ASSETS="$(make --silent kubebuilder-assets-path)" go test ./pkg/scheduler -race -count=1make reviewablegit diff --checkSpecial notes for your reviewer
There are no API or schema changes. The new scheduler-level test uses an intercepted fake client and a tracking queue so it can verify the exact
ForgetversusAddRateLimiteddecision without waiting on real backoff timers.AI assistance disclosure: OpenAI Codex was used to help inspect the repository, implement the change, and run validation. I reviewed the complete diff and verified the red/green behavior locally.