(refactor) Pick First LB state machine - #2784
Conversation
… ready during resolver update.
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
Leaving initial comments, haven't reviewed the entire PR.
| PickFirstState::FirstPass(s) => !s.addresses.is_empty(), | ||
| PickFirstState::SteadyState(s) => !s.addresses.is_empty(), | ||
| PickFirstState::Ready(s) => !s.addresses.is_empty(), |
There was a problem hiding this comment.
I believe in all these cases, addresses must be present and the value can directly be true.
There was a problem hiding this comment.
Correct me if I'm wrong, but if a newly built LB (in PickFirstState::Idle), with an empty address list, gets a resolver update that errors we could get here without having addresses.
I also changed the handling if we are idle, have no addresses, or are already in transient failure to enter SteadyState, to avoid being in the Idle state during transient failure.
| } | ||
|
|
||
| fn work(self, ctx: &mut PickFirstContext<'_>) -> PickFirstState { | ||
| self.exit_idle(ctx) |
There was a problem hiding this comment.
There was a TODO about checking if it's safe to assume every call to work should trigger exit_idle. Has this been confirmed?
Also, all the TODOs seem to be removed, have they been addressed?
There was a problem hiding this comment.
I think all of these have been addressed now:
- Line 125 (TODO: should steady_state be a "mode" selector enum...):
• This was the primary motivation for this PR, replacing mutable fields with the PickFirstState enum (Idle, FirstPass, SteadyState, Ready). - Line 217 (TODO: prevent redundant IDLE updates?):
• This is now being handled by state node typestates; IdleState::enter() publishes picker state only on entry and update. - Line 258 (TODO: avoid this update if we are in TF (i.e. sticky TF)?):
• Sticky TRANSIENT_FAILURE is now isolated insideSteadyState, which maintains the FailingPicker without emitting CONNECTING updates until a subchannel reaches READY. So I think this is handled. - Line 266 (TODO: set the last connection error?):
• SteadyState::enter explicitly accepts last_error: String and passes the most recent error to set_failing_picker. - Line 461 (TODO: make error mandatory.):
• ctx.set_failing_picker(&error) takes &str instead of Option. - Line 592 (TODO: is it safe to assume any call to work() while idle means we should connect?):
• I think this should be safe because IdlePicker is the sole scheduler in IDLE. This is mapped in IdleState::work via self.exit_idle(ctx). But if you can think of another scenario let me know!
| Err(e) => { | ||
| self.state = PickFirstState::Idle(IdleState { | ||
| addresses: Vec::new(), | ||
| }); |
There was a problem hiding this comment.
Pick-first should probably enter one of the Transient Failure (TF) states here.
When a valid resolver update arrives while the balancer is in TF, it should start using the new address list immediately. However, if the balancer is in IDLE, it should wait until exit_idle() is called. We should add tests to verify this.
There was a problem hiding this comment.
I've had this switch to SteadyState with empty values, which should handle this properly, I think, but please double check. There is also a new test around this.
| ctx.controller.request_resolution(); | ||
| return PickFirstState::Idle(self); |
There was a problem hiding this comment.
The balancer must transition the channel out of IDLE here. The IDLE picker only triggers exit_idle() once. If the PF balancer remained in IDLE on receiving resolver updates, it will remain permanently stuck in the IDLE state.
There was a problem hiding this comment.
I've had it enter a fresh first pass regardless here. I think that covers the cases necessary.
|
I think I've addressed everything so far. Let me know if you have any further feedback! |
2 participants
Motivation
The initial implementation of the PickFirst LB was a little tangled in how it implemented the Happy Eyeballs requirements. This meant it was handling some phases with special state and others without. Overall it made it hard to reason about.
Further, there was abehavior gaps between the original implementation and the spec: the original discards unselected addresses upon entering READY, causing subsequent reconnections from IDLE to only attempt the single failed address instead of sweeping all resolved backends.
Solution
This implementation is fairly different, using a state machine to hold state for each of the four states of Happy Eyeballs (Idle, FirstPass, SteadyState and Ready), and being clear about state transitions between them. The construction ensures the retention of appropriate information (i.e. addresses), and makes things easier to reason about.
As a bonus, this pass also eliminates a bunch of allocations that were unneeded. Gemini estimates about a 70% smaller memory footprint. As a double plus bonus, it now passes linting checks.