fix(scheduler): Scheduler wait on server connect #5930
Merged
Conversation
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
added 27 commits
September 13, 2024 10:22
|
The idea of the test with a pipeline is to make sure that we dont have any dropped messages.
on with this change we dont have any dropped messages |
sakoush
commented
Sep 24, 2024
lc525
reviewed
Sep 24, 2024
lc525
reviewed
Sep 24, 2024
lc525
reviewed
Sep 24, 2024
lc525
approved these changes
Sep 24, 2024
jtayl222
pushed a commit
to jtayl222/seldon-core
that referenced
this pull request
Jul 20, 2025
* Start envoy xDS server last * Orginise starter cmd for scheduler * Add synchroniser interface and simple timerbased impl * Separate out logic when agent connects * Integrate simple synchroniser in code * Add first sync option to ServerNotify * Adjust controller to set isFirstSync * Allow servernotify initial sync to start the sync process in scheduler * Rename start to signal in synchroniser * fFx test * Add test for ServerNotify * Change interface to allow for number of signals to wait for * Changes from SeldonIO#5886 (to include server events) * Add testing for servers (and other events) in hub * Add test for AddServerReplica * Add server sync impl * Add test for server sync * Add more testing coverage * Add logging * Wireup server changes in starter cmd * Add extra logging * Skip logging if not required * Start timer from the begining. * Tidy up logic and add more tests * Wire up simple sync for the non k8s case * Use waut group instead of a channel for sync * Tidy up log messages * Lint fixes * Set default timeout for scheduler readiness in docker-compose setup * Add explicit envar for scheduler ready timeout (compose) * Fix lint * Fix test * Add architecture design at the start of the file for server sync * Add new line * Tidy up name in test * Add parametrisation for helm for SCHEDULER_READY_TIMEOUT_SECONDS * Add log message for variable * Add note why xDS starts last * Add extra wait for routes to be established. * Tidy up event hub code * Tidy up event handling code
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What this PR does / why we need it:
This change introduces a
Synchroniseron the scheduler side to coordinate the startup process and especially to attempt to wait for expected model servers to connect before proceeding with any control plane operations. This is important in the case that the scheduler restarts so that we do no interrupt ongoing dataplane operations that should be valid to be served.There are 2 implementations for this
Synchroniser:SimpleSynchroniser: a timeout based sync for non k8s deploymentsServerBasedSynchroniser: this is for k8s deployments that tries to match expected server states (frometcdk8s controller) with actual agents connecting to the scheduler. More description provided here.Other changes in this PR:
ServerNotifyevent from the controller to indicate the list of expected servers collected from k8s etcd.Synchronise. Thanks to @lc525 for this change.This PR builds on top of #5893.
Which issue(s) this PR fixes:
Fixes #
INFRA-747 (internal)