Skip to content

Improve scheduler actor - #63

Open
TheHeroBrine422 wants to merge 29 commits into
mainfrom
caleb/fix-scheduler-actor-cache
Open

Improve scheduler actor#63
TheHeroBrine422 wants to merge 29 commits into
mainfrom
caleb/fix-scheduler-actor-cache

Conversation

@TheHeroBrine422

Copy link
Copy Markdown
Contributor

intended changes for this PR

  • improve actor cache
    • currently the actor cache is setup in a way that doesn't really support the scheduler updating it when messages happen. this makes the cache very difficult to use because it can't be relied on for up to date information which means we can barely use it as a cache. a specific issue i hit with this was trying to implement affinity/placement groups. if we rely on the cache data for calculating affinity, we could break the affinity rules due to race conditions.
    • i am replacing the generic version I made with one that is just integrated into the scheduler directly, because otherwise I don't really feel comfortable doing direct modifications to the cache in other segments of the scheduler
    • on top of this, the vm cache arguably needs slightly different code because we basically need the ability to query by either actor id or vmid, and so it needed some sort of modification to support this either way.
    • todo: have messages sent to scheduler directly update the cache
  • Placement groups #28
  • improve scheduling algorithm
    • i realized that currently the scheduling algorithm doesnt consider the resource requirements of the new vm. so for example if an agent had 15/16 vcpus scheduled, and then a new VM was to be scheduled that needed another 16 vcpus, it could theoretically schedule it to that agent. which would be bad

All that is left is testing and review by another maintainer.

this was done for two reasons:
1) the vm and agent actor caches have slightly different requirements,
specifically around keys (actorid vs vmid)
2) the scheduler needs direct control of the cache so the cache can be
updated in response to messages it forwards. being possibly up to 1
second out of date in this data could cause problems. In theory the
scheduler could update the generic cache, but I don't think using a
generic cache if we need to modify it directly anyway really makes
sense.

Signed-off-by: Caleb Jones <caleb@calebgj.io>
Signed-off-by: Caleb Jones <caleb@calebgj.io>
Signed-off-by: Caleb Jones <caleb@calebgj.io>
Signed-off-by: Caleb Jones <caleb@calebgj.io>
Signed-off-by: Caleb Jones <caleb@calebgj.io>
Signed-off-by: Caleb Jones <caleb@calebgj.io>
Signed-off-by: Caleb Jones <caleb@calebgj.io>
Signed-off-by: Caleb Jones <caleb@calebgj.io>
Signed-off-by: Caleb Jones <caleb@calebgj.io>
scheduler affinity

Signed-off-by: Caleb Jones <caleb@calebgj.io>
…k/odorobo into caleb/fix-scheduler-actor-cache
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Dismissed
Signed-off-by: Caleb Jones <caleb@calebgj.io>
Signed-off-by: Caleb Jones <caleb@calebgj.io>

@github-advanced-security github-advanced-security AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clippy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
@TheHeroBrine422 TheHeroBrine422 changed the title Caleb/fix scheduler actor cache Improve scheduler actor Jul 16, 2026
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
Comment thread odorobo/src/actors/scheduler_actor.rs Fixed
@TheHeroBrine422
TheHeroBrine422 marked this pull request as ready for review July 16, 2026 20:23
@halfcyan

Copy link
Copy Markdown
Member

i will look at this when i get back home. have you run it through clippy?

halfcyan
halfcyan previously approved these changes Jul 17, 2026
Comment thread odorobo/src/actors/scheduler_actor.rs
Comment thread odorobo/src/actors/scheduler_actor.rs
Comment thread odorobo/src/actors/scheduler_actor.rs
Comment thread odorobo/src/actors/scheduler_actor.rs
Comment thread odorobo/src/actors/scheduler_actor.rs
Comment thread odorobo/src/actors/scheduler_actor.rs
Comment thread odorobo/src/actors/scheduler_actor.rs
Comment thread odorobo/src/actors/scheduler_actor.rs
async fn on_update(
// this function intentionally only checks against the cache. this has some positives and negatives:
// positive: it will never trigger any network requests so its very fast, and having to do network requests for scoring whenever we want to schedule a vm is likely a bad idea
// negative: it technically has a delayed view of the cluster, meaning that some things that happened in the future, may not exist yet. so we need to be careful about how this is done so affinity rules are not accidentally broken. mostly this means, if we do anything that could affect the outcome of an affinity rule (ex: network request to an agent), we need to update the cache, before we do the action.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

assuming anything later in the cache is referencing anything that changed as a result of this, i think it's fine. so like if you have multiple VM creations get scheduled, as long as each subsequent one references the one before it.

korewaChino
korewaChino previously approved these changes Jul 20, 2026
Comment thread odoroboctl/src/cli.rs Outdated
max_vcpus: None,
memory: ByteSize::gib(4),
image: "/var/lib/odorobo/f43.raw".to_string(),
image: "/var/lib/odorobo/f43-1.raw".to_string(),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why are we still hardcoding this btw

@halfcyan
halfcyan dismissed stale reviews from korewaChino and themself via 2ecbae0 July 31, 2026 06:31
?actor_ref,
"can no longer reach agent actor, stopping updater"
);
data_cache.remove(&actor_ref.id());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

when the updater gives up after six failed polls, it removes the actor from data_cache and returns, but its completed JoinHandle remains in keepalive_tasks. the finder uses contains_key to decide whether an updater exists, so it will see the stale handle and never restart polling when the actor becomes reachable again. that can permanently remove a temporarily disconnected agent/VM from scheduling.

probably remove the handle when the updater exits, or have the finder treat finished handles as stale (e.g. check JoinHandle::is_finished() and replace the entry). same issue applies to both agent_updater_task and vm_updater_task.

Comment on lines +265 to +273
if let Ok(data) = actor_ref.ask(&GetAgentStatus).await {
if data_cache.contains_key(&actor_ref.id()) {
data_cache.alter(&actor_ref.id(), |_, mut v| {
v.data = data;

v.extended_vm_set.extend(v.data.vms.iter());

v
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

when an agent update succeeds, this only extends the existing set. it doesn't remove VM IDs that disappeared from AgentStatus.vms. affinity scheduling can then treat a VM like it's still on an agent after shutdown, deletion, migration, etc.

Comment on lines +665 to +675
self.vm_data_cache.insert(
msg.vmid,
CachedVMActor {
actor_ref: None,
scheduled_agent_id: target_agent.id(),
data: GetVMInfoReply {
vmid: msg.vmid,
config: Some(msg.config.clone()),
},
},
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the placeholder VM that gets inserted here can leak if the actor dies before it has a successful update. basically, if the actor dies before sending GetVMInfo to vm_updater_task, there's no vm_actorid_ulid_map entry. because of that, on_link_died skips cleanup.

also see lines 626-640

let reply = target_agent.ask(&msg).await;

// remove from caches if the agent rejected the request
if reply.is_err() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this treats all act errors as a rejected create, but not all of them are (reply may be lost as an example)


vm_actorid_ulid_map.insert(actor_ref.id(), vmid); // should we be doing this on every loop? idk. but we at least need to do it on the first iteration given we don't know the mapping before that

data_cache.insert(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

vm_data_cache is keyed only by vmid, but migration can temporarily have two VM actors with the same VM ID. the later updater overwrites the earlier cache entry. if the old actor then dies, on_link_died can remove the cache entry belonging to the new actor and remove the VM from the wrong agent’s extended_vm_set

// todo: this code is really bad, and we should not have effectively two copies of ths same thing.
#[derive(Copy, Clone)]
struct VMActorCacheUpdater;
fn evaluate_table_value(value_option: Option<&String>, requirement: &AffinityRequirement) -> bool {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can you add some unit tests for the new affinity logic? in particular, tests for In, NotIn, Lt, and Gt, missing metadata keys, inverse rules, empty requirements, required versus preferred rules, and capacity rejection would make these semantics explicit and prevent regressions. the scheduler currently has no tests covering this new behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants