Skip to content

MachineScheduler cache loses track of bound machines#1492

Merged
lukasfrank merged 4 commits into
mainfrom
fix/scheduler-bugs
Jun 26, 2026
Merged

MachineScheduler cache loses track of bound machines#1492
lukasfrank merged 4 commits into
mainfrom
fix/scheduler-bugs

Conversation

@lukasfrank

@lukasfrank lukasfrank commented Jun 25, 2026

Copy link
Copy Markdown
Member

Proposed Changes

  • Fix instance 5ebaf389-19c5-46dc-a108-6a99f18607c4 is not present in the cache and thus cannot be updated error
  • Fix missing return nil in ForgetInstance after successful removal.
  • Use MaxAllocatable() in fitsPool to reject pools that are full.

Fixes #1491

@lukasfrank lukasfrank self-assigned this Jun 25, 2026
@lukasfrank
lukasfrank requested a review from a team June 25, 2026 13:09
@github-actions github-actions Bot added size/L bug Something isn't working labels Jun 25, 2026
Signed-off-by: Lukas Frank <lukas.frank@sap.com>
@lukasfrank
lukasfrank force-pushed the fix/scheduler-bugs branch from 0d5a3e2 to da210cb Compare June 25, 2026 13:10
@lukasfrank
lukasfrank marked this pull request as draft June 25, 2026 13:11
@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Machine and volume schedulers now fit pools from MaxAllocatable, add newly assigned instances to cache before the normal update path, and use write locks in cache mutation methods. Tests were updated and extended to cover cache-state tracking for both controllers.

Changes

Scheduler cache and assignment updates

Layer / File(s) Summary
Cache mutation locking
internal/controllers/compute/scheduler/cache.go, internal/controllers/storage/scheduler/cache.go
ForgetInstance and FinishBinding now use write locks, and successful forgets return nil after removing assumed instances.
Machine scheduler flow
internal/controllers/compute/machine_scheduler.go, internal/controllers/compute/machine_scheduler_test.go
MachineScheduler fits pools from MaxAllocatable, adds newly assigned machines to cache before continuing the update path, and the machine tests remove Image fields while adding cache-state coverage.
Volume scheduler flow
internal/controllers/storage/volume_scheduler.go, internal/controllers/storage/volume_scheduler_test.go
VolumeScheduler fits pools from MaxAllocatable, adds newly assigned volumes to cache before continuing the update path, and the volume test checks two assigned volumes and one unscheduled volume.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change to MachineScheduler cache tracking.
Linked Issues check ✅ Passed The changes address #1491 by keeping bound instances in the scheduler cache and fixing the related update path.
Out of Scope Changes check ✅ Passed The volume scheduler and cache updates are still within the linked issue's scope, which covers both machine and volume cache tracking.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The PR description matches the required template, includes three proposed changes, and references the linked issue.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/scheduler-bugs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lukasfrank
lukasfrank force-pushed the fix/scheduler-bugs branch from da210cb to 55b3efa Compare June 25, 2026 13:24
Signed-off-by: Lukas Frank <lukas.frank@sap.com>
@lukasfrank
lukasfrank force-pushed the fix/scheduler-bugs branch from 55b3efa to 5c29eb8 Compare June 25, 2026 13:26
@lukasfrank
lukasfrank marked this pull request as ready for review June 25, 2026 13:41
@lukasfrank lukasfrank changed the title Fix scheduler bugs MachineScheduler cache loses track of bound machines Jun 25, 2026
Comment thread internal/controllers/compute/machine_scheduler_test.go
@friegger

Copy link
Copy Markdown
Contributor

For the commit message I would like to see a small description what the impact of e.g. using MaxAllocatable() is.

Signed-off-by: Lukas Frank <lukas.frank@sap.com>
@lukasfrank
lukasfrank requested a review from friegger June 26, 2026 12:34
@lukasfrank
lukasfrank merged commit 8a90e86 into main Jun 26, 2026
11 checks passed
@lukasfrank
lukasfrank deleted the fix/scheduler-bugs branch June 26, 2026 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MachineScheduler cache loses track of bound machines

2 participants