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

Fix Controller: pending messages are stuck in the scheduled map #1324

Merged
merged 2 commits into from Oct 27, 2023

Conversation

co42
Copy link
Contributor

@co42 co42 commented Oct 25, 2023

Motivation

When using the Controller. If some messages are put in the pending queue using pop_queue_message_into_pending they are not removed from the scheduled map and will never be.

The message in pending will be reconciled later, but all matching messages that follow will not.

Solution

When adding a message to the pending queue remove it from the scheduled map (like it's done in poll_pop_queue_message).

May fix #1322

Signed-off-by: Corentin Regal <corentin@huggingface.co>
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Merging #1324 (b325981) into main (9c81f1f) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1324     +/-   ##
=======================================
+ Coverage   72.4%   72.4%   +0.1%     
=======================================
  Files         75      75             
  Lines       6343    6351      +8     
=======================================
+ Hits        4586    4595      +9     
+ Misses      1757    1756      -1     
Files Coverage Δ
kube-runtime/src/controller/runner.rs 95.2% <100.0%> (+0.2%) ⬆️
kube-runtime/src/scheduler.rs 97.2% <100.0%> (+0.1%) ⬆️

... and 1 file with indirect coverage changes

@einarmo
Copy link

einarmo commented Oct 25, 2023

Yep. This is what I also found, and it does fix #1322.

@clux clux added the changelog-fix changelog fix category for prs label Oct 26, 2023
@clux clux added this to the 0.87.0 milestone Oct 26, 2023
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

This looks sensible to me, and matches the other way we do it. Thanks a lot for investigating and extending the tests for this.

(The deny issue should be unrelated and fixed in main.)

Signed-off-by: Eirik A <sszynrae@gmail.com>
@clux clux merged commit e78d488 into kube-rs:main Oct 27, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-fix changelog fix category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Controller concurrency appears to cause reconciliations to be skipped
3 participants