Skip to content

Commit 0f34dee

Browse files
committed
feat(supervisor): spread shared supervisors evenly across gateway replicas (closes #5434)
Signed-off-by: divesh <dgude@nvidia.com>
1 parent 9cf85f9 commit 0f34dee

12 files changed

Lines changed: 454 additions & 31 deletions

File tree

‎.config/nextest.toml‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ test-group = "kubernetes-ha"
3636
# they kill members or gateways, or scale the pool. Never run them at once.
3737
# Test names, not binary(), for the reason above.
3838
[[profile.e2e-kubernetes.overrides]]
39-
filter = "test(/^(losing_a_supervisor_pod_moves_its_sandboxes_without_restarting_them|scaling_a_supervisor_pool_up_and_down_keeps_its_sandboxes_running|pool_sessions_follow_their_supervisors_gateway)$/)"
39+
filter = "test(/^(losing_a_supervisor_pod_moves_its_sandboxes_without_restarting_them|scaling_a_supervisor_pool_up_and_down_keeps_its_sandboxes_running|pool_sessions_follow_their_supervisors_gateway|supervisors_spread_evenly_across_gateways)$/)"
4040
test-group = "kubernetes-shared-supervisor-pool"
4141

4242
# Scaling the pool runs two bounded Helm upgrades (up to 6 minutes each), waits
@@ -51,6 +51,12 @@ slow-timeout = { period = "60s", terminate-after = 20 }
5151
filter = "test(/^pool_sessions_follow_their_supervisors_gateway$/)"
5252
slow-timeout = { period = "60s", terminate-after = 20 }
5353

54+
# One bounded rollout per supervisor and one for the gateway, each followed by
55+
# a settle round.
56+
[[profile.e2e-kubernetes.overrides]]
57+
filter = "test(/^supervisors_spread_evenly_across_gateways$/)"
58+
slow-timeout = { period = "60s", terminate-after = 20 }
59+
5460
# Relative to the profile store dir (`e2e/rust/target/nextest/e2e-kubernetes/`).
5561
[profile.e2e-kubernetes.junit]
5662
path = "../../../../../results/e2e-kubernetes.xml"

‎crates/openshell-server/src/gateway_metrics.rs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ use tonic::{Code, Status};
2323

2424
// Gauges
2525
pub const SUPERVISOR_SESSIONS: &str = "openshell_server_supervisor_sessions";
26+
pub const SUPERVISOR_HOSTS: &str = "openshell_server_supervisor_hosts";
27+
pub const SUPERVISOR_HOST_INSTANCES_MAX: &str = "openshell_server_supervisor_host_instances_max";
2628
pub const RELAY_PENDING: &str = "openshell_server_relay_pending";
2729
pub const RELAY_PENDING_CAPACITY: &str = "openshell_server_relay_pending_capacity";
2830
pub const MUTATION_LOCK_CONNECTIONS_IN_USE: &str =
@@ -262,6 +264,16 @@ pub fn describe_and_initialize(relay: RelayCapacity, mutation_lock_connections:
262264
Unit::Count,
263265
"Supervisor control sessions registered on this gateway replica."
264266
);
267+
describe_gauge!(
268+
SUPERVISOR_HOSTS,
269+
Unit::Count,
270+
"Shared supervisors whose assignment streams this gateway replica serves."
271+
);
272+
describe_gauge!(
273+
SUPERVISOR_HOST_INSTANCES_MAX,
274+
Unit::Count,
275+
"Most sandbox instances placed on one shared supervisor this gateway replica serves."
276+
);
265277
describe_gauge!(
266278
RELAY_PENDING,
267279
Unit::Count,
@@ -335,6 +347,8 @@ pub fn describe_and_initialize(relay: RelayCapacity, mutation_lock_connections:
335347

336348
// `increment(0)` registers a series without overwriting a value recorded earlier.
337349
gauge!(SUPERVISOR_SESSIONS).increment(0.0);
350+
gauge!(SUPERVISOR_HOSTS).increment(0.0);
351+
gauge!(SUPERVISOR_HOST_INSTANCES_MAX).increment(0.0);
338352
gauge!(RELAY_PENDING).increment(0.0);
339353
gauge!(RELAY_PENDING_CAPACITY).set(count_as_f64(relay.per_replica));
340354
for kind in RelayKind::ALL {
@@ -433,6 +447,11 @@ impl Drop for GaugeSlot {
433447
}
434448
}
435449

450+
pub fn record_supervisor_hosts(hosts: usize, most_instances: usize) {
451+
gauge!(SUPERVISOR_HOSTS).set(count_as_f64(hosts));
452+
gauge!(SUPERVISOR_HOST_INSTANCES_MAX).set(count_as_f64(most_instances));
453+
}
454+
436455
pub fn record_relay_rejected(reason: RelayRejection) {
437456
counter!(RELAY_REJECTED_TOTAL, LABEL_REASON => reason.label()).increment(1);
438457
}
@@ -642,6 +661,8 @@ mod tests {
642661
("openshell_server_mutation_lock_connections_capacity", 4),
643662
("openshell_server_mutation_lock_connections_in_use", 0),
644663
("openshell_server_supervisor_sessions", 0),
664+
("openshell_server_supervisor_hosts", 0),
665+
("openshell_server_supervisor_host_instances_max", 0),
645666
("openshell_server_relay_pending", 0),
646667
(
647668
"openshell_server_relay_rejected_total{reason=\"replica_capacity\"}",

‎crates/openshell-server/src/gateway_ring.rs‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,23 @@ impl GatewayRing {
8484
Some(self.points[index].1.as_str())
8585
}
8686

87+
/// The first replica clockwise from `key` whose `load` is below `cap`,
88+
/// or the owner of `key` when every replica is full.
89+
pub fn first_below(&self, key: &str, cap: usize, load: impl Fn(&str) -> usize) -> Option<&str> {
90+
let owner = self.owner_for(key)?;
91+
let start = self
92+
.points
93+
.partition_point(|(candidate, _)| *candidate < hash_point(key));
94+
let mut seen = std::collections::HashSet::new();
95+
self.points[start..]
96+
.iter()
97+
.chain(&self.points[..start])
98+
.map(|(_, id)| id.as_str())
99+
.filter(|id| seen.insert(*id))
100+
.find(|id| load(id) < cap)
101+
.or(Some(owner))
102+
}
103+
87104
/// The same ring with `replica_id` taken out.
88105
pub fn without(&self, replica_id: &str) -> Self {
89106
Self {
@@ -250,4 +267,44 @@ mod tests {
250267
"adding one replica moved {moved} of {total} sandboxes"
251268
);
252269
}
270+
271+
#[test]
272+
fn first_below_skips_full_replicas_and_falls_back_to_the_owner() {
273+
let ring = GatewayRing::new(["gw-0", "gw-1"]);
274+
let owner = ring.owner_for("sup-0").unwrap();
275+
let other = if owner == "gw-0" { "gw-1" } else { "gw-0" };
276+
277+
assert_eq!(ring.first_below("sup-0", 1, |_| 0), Some(owner));
278+
assert_eq!(
279+
ring.first_below("sup-0", 1, |id| usize::from(id == owner)),
280+
Some(other)
281+
);
282+
assert_eq!(ring.first_below("sup-0", 1, |_| 1), Some(owner));
283+
assert_eq!(GatewayRing::default().first_below("sup-0", 1, |_| 0), None);
284+
}
285+
286+
/// Supervisors arriving one by one under a cap of their even share fill
287+
/// every replica to within that cap.
288+
#[test]
289+
fn first_below_spreads_arrivals_evenly() {
290+
for (replicas, supervisors) in [(2, 2), (3, 7), (4, 40)] {
291+
let ids: Vec<String> = (0..replicas).map(|i| format!("gw-{i}")).collect();
292+
let ring = GatewayRing::new(&ids);
293+
let cap = usize::div_ceil(supervisors, replicas);
294+
let mut load: HashMap<String, usize> = HashMap::new();
295+
for i in 0..supervisors {
296+
let home = ring
297+
.first_below(&format!("sup-{i}"), cap, |id| {
298+
load.get(id).copied().unwrap_or(0)
299+
})
300+
.unwrap()
301+
.to_string();
302+
*load.entry(home).or_default() += 1;
303+
}
304+
assert!(
305+
load.values().all(|count| *count <= cap) && load.len() == replicas,
306+
"{supervisors} supervisors on {replicas} replicas: {load:?}"
307+
);
308+
}
309+
}
253310
}

‎crates/openshell-server/src/grpc/supervisor_host.rs‎

Lines changed: 116 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,21 @@
88
//! the handler sees a [`Principal::Supervisor`] and hands the stream to the
99
//! controller the in-process compute driver installed.
1010
11+
use std::collections::HashMap;
1112
use std::pin::Pin;
1213
use std::sync::Arc;
14+
use std::sync::atomic::Ordering;
1315

16+
use futures::StreamExt as _;
1417
use openshell_core::proto::supervisor_host::v1::supervisor_host_server::SupervisorHost;
1518
use openshell_core::proto::supervisor_host::v1::{HostGatewayMessage, HostMessage};
19+
use openshell_supervisor_control::MemberView;
20+
use openshell_supervisor_control::controller::redirect;
1621
use tonic::{Request, Response, Status, Streaming};
1722

1823
use crate::ServerState;
1924
use crate::auth::principal::Principal;
25+
use crate::gateway_ring::GatewayRing;
2026

2127
/// gRPC path prefix of the supervisor host service.
2228
pub const SUPERVISOR_HOST_PATH_PREFIX: &str = "/openshell.supervisor.host.v1.SupervisorHost/";
@@ -65,10 +71,116 @@ impl SupervisorHost for SupervisorHostService {
6571
if *shutdown.borrow() {
6672
return Err(Status::unavailable("gateway is shutting down"));
6773
}
68-
let outbound = controller.attach(identity, request.into_inner()).await?;
69-
let outbound = futures::StreamExt::take_until(outbound, async move {
70-
let _ = shutdown.wait_for(|shutdown| *shutdown).await;
71-
});
74+
let state = self.state.clone();
75+
let (member_id, outbound) = controller
76+
.attach(identity, request.into_inner(), |member_id| {
77+
host_home(
78+
&state,
79+
&controller.pool_view().unwrap_or_default().members,
80+
member_id,
81+
)
82+
})
83+
.await?;
84+
let handoff = futures::stream::once(async move {
85+
let stopping = *state.supervisor_sessions.subscribe_shutdown().borrow();
86+
let members = controller.pool_view().unwrap_or_default().members;
87+
stopping
88+
.then(|| host_home(&state, &members, &member_id))
89+
.flatten()
90+
.map(|peer_endpoint| Ok(redirect(peer_endpoint)))
91+
})
92+
.filter_map(std::future::ready);
93+
let outbound = outbound
94+
.take_until(async move {
95+
let _ = shutdown.wait_for(|shutdown| *shutdown).await;
96+
})
97+
.chain(handoff);
7298
Ok(Response::new(Box::pin(outbound)))
7399
}
74100
}
101+
102+
/// Peer endpoint of the replica that should serve the shared supervisor
103+
/// `member_id` when that is not this replica. A stopping replica leaves
104+
/// itself out.
105+
fn host_home(state: &ServerState, members: &[MemberView], member_id: &str) -> Option<String> {
106+
if state.store.is_single_replica() {
107+
return None;
108+
}
109+
let ring = state.gateway_ring.read().ok()?;
110+
let ring = if state.gateway_shutting_down.load(Ordering::Acquire) {
111+
ring.without(&state.replica_id)
112+
} else {
113+
ring.clone()
114+
};
115+
let home = spread_home(&ring, members, member_id)?;
116+
if home == state.replica_id {
117+
return None;
118+
}
119+
state
120+
.gateway_peers
121+
.read()
122+
.ok()?
123+
.get(home)
124+
.filter(|endpoint| !endpoint.is_empty())
125+
.cloned()
126+
}
127+
128+
/// The first replica clockwise from `member_id` on the gateway ring that
129+
/// serves fewer than an even share of the pool's other supervisors.
130+
fn spread_home<'a>(
131+
ring: &'a GatewayRing,
132+
members: &[MemberView],
133+
member_id: &str,
134+
) -> Option<&'a str> {
135+
let mut load = HashMap::<&str, usize>::new();
136+
let mut supervisors = 1_usize;
137+
for member in members
138+
.iter()
139+
.filter(|member| member.member_id != member_id && !member.leaving)
140+
{
141+
supervisors += 1;
142+
*load.entry(member.gateway_replica.as_str()).or_default() += 1;
143+
}
144+
let cap = supervisors.div_ceil(ring.replica_count().max(1));
145+
ring.first_below(member_id, cap, |replica| {
146+
load.get(replica).copied().unwrap_or(0)
147+
})
148+
}
149+
150+
#[cfg(test)]
151+
mod tests {
152+
use super::*;
153+
154+
fn member(member_id: &str, gateway_replica: &str) -> MemberView {
155+
MemberView {
156+
member_id: member_id.to_string(),
157+
process_id: String::new(),
158+
runtime_address: String::new(),
159+
leaving: false,
160+
gateway_replica: gateway_replica.to_string(),
161+
}
162+
}
163+
164+
#[test]
165+
fn a_supervisor_goes_to_a_replica_below_its_even_share() {
166+
let ring = GatewayRing::new(["gw-a", "gw-b"]);
167+
let owner = ring.owner_for("sup-2").unwrap();
168+
let other = if owner == "gw-a" { "gw-b" } else { "gw-a" };
169+
170+
assert_eq!(
171+
spread_home(&ring, &[member("sup-1", owner)], "sup-2"),
172+
Some(other)
173+
);
174+
assert_eq!(
175+
spread_home(&ring, &[member("sup-1", other)], "sup-2"),
176+
Some(owner)
177+
);
178+
// Its own record and leaving members do not count against a replica.
179+
let mut leaving = member("sup-3", owner);
180+
leaving.leaving = true;
181+
assert_eq!(
182+
spread_home(&ring, &[member("sup-2", owner), leaving], "sup-2"),
183+
Some(owner)
184+
);
185+
}
186+
}

‎crates/openshell-server/src/supervisor_pool.rs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,7 @@ pub struct LiveMember {
182182
pub process_id: String,
183183
pub runtime_address: String,
184184
pub leaving: bool,
185+
pub gateway_replica: String,
185186
}
186187

187188
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
@@ -1168,6 +1169,7 @@ impl SupervisorPool {
11681169
process_id: payload.process_id,
11691170
runtime_address: payload.runtime_address,
11701171
leaving: payload.leaving,
1172+
gateway_replica: payload.gateway_replica,
11711173
});
11721174
}
11731175
members.sort_by(|left, right| left.member_id.cmp(&right.member_id));
@@ -2671,6 +2673,7 @@ impl SupervisorPool {
26712673
process_id: payload.process_id,
26722674
runtime_address: payload.runtime_address,
26732675
leaving: payload.leaving,
2676+
gateway_replica: payload.gateway_replica,
26742677
});
26752678
Ok(takeover_cause(owner, member.as_ref()))
26762679
}
@@ -2825,6 +2828,7 @@ impl SupervisorPool {
28252828
process_id: member.process_id.clone(),
28262829
runtime_address: member.runtime_address.clone(),
28272830
leaving: member.leaving,
2831+
gateway_replica: member.gateway_replica.clone(),
28282832
})
28292833
.collect(),
28302834
owners: BTreeMap::new(),
@@ -2868,6 +2872,19 @@ impl SupervisorPool {
28682872
);
28692873
}
28702874
lock(&self.minted).retain(|term, _| current_terms.contains(term));
2875+
let mut placed: BTreeMap<&str, usize> = served
2876+
.iter()
2877+
.map(|(member, _)| (member.as_str(), 0))
2878+
.collect();
2879+
for owner in view.owners.values() {
2880+
if let Some(count) = placed.get_mut(owner.member_id.as_str()) {
2881+
*count += 1;
2882+
}
2883+
}
2884+
crate::gateway_metrics::record_supervisor_hosts(
2885+
placed.len(),
2886+
placed.values().copied().max().unwrap_or(0),
2887+
);
28712888
Some(view)
28722889
}
28732890

@@ -5519,6 +5536,7 @@ mod tests {
55195536
process_id: process_id.to_string(),
55205537
runtime_address: "10.0.0.1:5500".to_string(),
55215538
leaving,
5539+
gateway_replica: String::new(),
55225540
};
55235541
assert_eq!(takeover_cause(&owner, None), Some("expired"));
55245542
assert_eq!(
@@ -5543,6 +5561,7 @@ mod tests {
55435561
process_id: format!("p-{id}"),
55445562
runtime_address: "10.0.0.1:5500".to_string(),
55455563
leaving,
5564+
gateway_replica: String::new(),
55465565
};
55475566
let members = vec![member("m1", false), member("m2", false), member("m3", true)];
55485567
let ring = ring_of(&members);

‎crates/openshell-supervisor-control/README.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,10 @@ re-implementing the control contracts.
4848
`pool_timing::GATEWAY_SILENCE_TIMEOUT` of silence, and drops a member that
4949
stops reading as soon. `HostAccepted` carries this replica's peer endpoint
5050
(`set_peer_endpoint`); the host dials its instances' sandbox sessions
51-
there, so each session runs on the replica serving its supervisor.
51+
there, so each session runs on the replica serving its supervisor. A host
52+
whose hello does not set `stay` is sent a `HostRedirect` instead when the
53+
gateway places its member on another replica; the gateway picks the first
54+
replica on its ring below an even share of supervisors.
5255
- `pool_timing`: the timing contract between pool hosts and the gateway: the
5356
15 s membership TTL, the 2 s record refresh, 1 s heartbeats, host silence
5457
detection and bounded reconnect attempts, and `reconnect_budget`, the worst

0 commit comments

Comments
 (0)