Open
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR adds per-call ChangesScheduling Options Infrastructure and Pipeline Propagation
Sequence Diagram(s)sequenceDiagram
participant Controller as Pipeline Controller
participant Pipeline as FilterWeigherPipeline
participant Filters as Filters
participant Weighers as Weighers
Controller->>Controller: Peek or build Options<br/>(e.g., ReadOnly, LockReservations,<br/>RecordHistory, MaxCandidates)
Controller->>Pipeline: Run(request, opts)
Pipeline->>Pipeline: opts.Validate()
Pipeline->>Filters: runFilters(request, opts)
Filters->>Filters: For each filter: filter.Run(..., opts)
Filters-->>Pipeline: filtered request
Pipeline->>Weighers: runWeighers(request, opts)
Weighers->>Weighers: For each weigher: weigher.Run(..., opts)
Weighers-->>Pipeline: weighted activations
Pipeline->>Pipeline: Trim by opts.MaxCandidates
Pipeline->>Pipeline: Conditional history upsert<br/>(if RecordHistory)
Pipeline-->>Controller: result + error
Controller->>Controller: Handle result based on<br/>ReadOnly/RecordHistory flags
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
mblos
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
umswmayj
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
internal/scheduling/lib/weigher_test.go (1)
37-42: ⚡ Quick winLet the mock callback receive
optsfor option-propagation assertionsLine [37] adds
opts, but Line [41] drops it. PassingoptsthroughRunFuncmakes this mock usable for validating the new per-call options behavior.🧪 Suggested refactor
type mockWeigher[RequestType FilterWeigherPipelineRequest] struct { InitFunc func(ctx context.Context, client client.Client, step v1alpha1.WeigherSpec) error ValidateFunc func(ctx context.Context, params v1alpha1.Parameters) error - RunFunc func(traceLog *slog.Logger, request RequestType) (*FilterWeigherPipelineStepResult, error) + RunFunc func(traceLog *slog.Logger, request RequestType, opts Options) (*FilterWeigherPipelineStepResult, error) } @@ func (m *mockWeigher[RequestType]) Run(traceLog *slog.Logger, request RequestType, opts Options) (*FilterWeigherPipelineStepResult, error) { if m.RunFunc == nil { return &FilterWeigherPipelineStepResult{}, nil } - return m.RunFunc(traceLog, request) + return m.RunFunc(traceLog, request, opts) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduling/lib/weigher_test.go` around lines 37 - 42, The mock Run method for mockWeigher currently accepts opts but doesn't pass it to the configured callback, so update mockWeigher.Run to call m.RunFunc with (traceLog, request, opts) instead of (traceLog, request) and ensure the RunFunc signature (and any test assignments) match the three-argument form; this allows test callbacks on RunFunc to assert per-call Options propagation for FilterWeigherPipelineStepResult handling.internal/scheduling/lib/filter_test.go (1)
19-19: ⚡ Quick win
RunFunccallback dropsopts— tests via this mock cannot observe options-dependent behavior.
RunFuncis typed withoutopts(line 19), so theRunwrapper silently discards the incomingOptionsbefore calling it. As filters grow opts-aware logic, any tests exercising those branches viaRunFuncwill be blind to the options value.♻️ Proposed fix
-RunFunc func(traceLog *slog.Logger, request RequestType) (*FilterWeigherPipelineStepResult, error) +RunFunc func(traceLog *slog.Logger, request RequestType, opts Options) (*FilterWeigherPipelineStepResult, error)func (m *mockFilter[RequestType]) Run(traceLog *slog.Logger, request RequestType, opts Options) (*FilterWeigherPipelineStepResult, error) { if m.RunFunc == nil { return &FilterWeigherPipelineStepResult{}, nil } - return m.RunFunc(traceLog, request) + return m.RunFunc(traceLog, request, opts) }Also applies to: 34-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduling/lib/filter_test.go` at line 19, The mock's RunFunc callback signature currently omits the Options parameter so the Run wrapper discards options; update the RunFunc type to accept an opts *Options (or Options) parameter (e.g., change RunFunc func(traceLog *slog.Logger, request RequestType, opts Options) (*FilterWeigherPipelineStepResult, error)), then update the Run method to forward its incoming opts to RunFunc and adjust any test mocks that set RunFunc to accept and assert on opts (also update other mock occurrences referenced around the Run wrapper). Ensure references to RunFunc, Run, Options, RequestType, and FilterWeigherPipelineStepResult are updated consistently.internal/scheduling/lib/filter_weigher_pipeline_test.go (1)
415-423: ⚡ Quick winUse
slices.Containsfor host membership check.Replace the manual loop with
slices.Contains(result.OrderedHosts, host)to align with repo conventions.As per coding guidelines: "Use
slices.Containsto check if an element is part of a slice".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduling/lib/filter_weigher_pipeline_test.go` around lines 415 - 423, Replace the manual membership loop that checks hosts in result.OrderedHosts with slices.Contains(result.OrderedHosts, host) to follow repo conventions; update the import block to include the standard library "slices" package and remove the now-unnecessary found/loop code (references: result.AggregatedOutWeights and result.OrderedHosts).internal/scheduling/nova/filter_weigher_pipeline_controller.go (2)
215-227: ⚡ Quick winConsider invoking
opts.Validate()before returning.Per
internal/scheduling/lib/options.go,Options.Validate()enforces mutual exclusivity (ReadOnlyvsRecordHistory/CreateInflight). Right now, no caller in this controller validates the constructedOptions, so an inconsistent combination (onceReadOnlystarts being set — see the related comment) would silently propagate topipeline.Run. CallingValidate()here (or once at the start ofprocess) would make misconfigurations fail fast, ideally with a clear status condition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduling/nova/filter_weigher_pipeline_controller.go` around lines 215 - 227, The buildOptions function currently constructs lib.Options but never calls Options.Validate(), so invalid combinations (e.g., ReadOnly vs RecordHistory/CreateInflight) can propagate; update FilterWeigherPipelineController.buildOptions to call opts.Validate() after construction and handle the returned error (e.g., propagate it to the caller or convert it into a failed pipeline status in the calling process method), ensuring you reference lib.Options and its Validate() method and adjust callers of buildOptions (or process) to surface the validation error rather than allowing an invalid opts to reach pipeline.Run.
56-83: 💤 Low valueRe-fetch on the exclusive path is good; document why the RLock path doesn't.
Re-fetching after
Lock()on Lines 70-72 is sensible because the cacheddecisionmay be stale by the time the writer wins the lock. The RLock branch intentionally doesn't re-fetch (multiple readers proceed in parallel and all observe the cached state), which is fine — but a one-liner explaining that asymmetry would save the next reader a thought-experiment. No behavior concern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduling/nova/filter_weigher_pipeline_controller.go` around lines 56 - 83, Add a short explanatory comment in Reconcile next to the RLock branch (around the call to c.processMu.RLock() / c.processMu.RUnlock()) stating that readers intentionally do not re-fetch the Decision after acquiring the read lock because multiple concurrent readers should observe the same cached state and re-fetching would be unnecessary/expensive; keep the existing re-fetch only on the exclusive Lock path (used when peekReadOnly(decision) is false) to ensure writers see a consistent, up-to-date object.internal/scheduling/lib/filter_weigher_pipeline.go (1)
305-311: ⚡ Quick winAvoid quadratic containment checks when trimming candidates.
Line 309 currently does
slices.Contains(hosts, host)inside a loop overoutWeights. Building a small set of kept hosts avoids O(n²) behavior on larger host lists.♻️ Proposed optimization
if opts.MaxCandidates > 0 && len(hosts) > opts.MaxCandidates { hosts = hosts[:opts.MaxCandidates] // Drop trimmed hosts from outWeights so AggregatedOutWeights stays consistent. + kept := make(map[string]struct{}, len(hosts)) + for _, host := range hosts { + kept[host] = struct{}{} + } for host := range outWeights { - if !slices.Contains(hosts, host) { + if _, ok := kept[host]; !ok { delete(outWeights, host) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduling/lib/filter_weigher_pipeline.go` around lines 305 - 311, When trimming hosts into opts.MaxCandidates, avoid the quadratic check by first constructing a small lookup set of the kept hosts (e.g. kept := map[string]struct{} and populate it from hosts[:opts.MaxCandidates]) and then iterate over outWeights deleting entries where the host key is not present in that set; replace slices.Contains(hosts, host) with a constant-time map lookup. Update references around hosts, outWeights and the trimming block in filter_weigher_pipeline.go accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/scheduling/lib/options.go`:
- Around line 40-45: The validation currently returns error messages that start
with uppercase letters; update the error strings returned when o.ReadOnly &&
o.RecordHistory and when o.ReadOnly && o.CreateInflight to start with lowercase
(e.g. change "ReadOnly and RecordHistory are mutually exclusive: ..." to
"readOnly and recordHistory are mutually exclusive: ..." or similar lowercase
wording) in the function that checks o.ReadOnly, o.RecordHistory and
o.CreateInflight so they satisfy the lint rule; keep the same descriptive text
but make the first character lowercase for both error.New(...) calls.
In `@internal/scheduling/machines/filter_weigher_pipeline_controller.go`:
- Line 147: process() currently calls pipeline.Run(request, lib.Options{}) which
always passes a zero-value lib.Options and thus drops flags like RecordHistory;
to fix, retrieve the pipeline config from c.PipelineConfigs using
decision.Spec.PipelineRef.Name (accessible via the embedded
BasePipelineController), construct an Options struct (similar to
buildOptions(request, pipelineConf) used in Nova) that at minimum sets
RecordHistory = pipelineConf.Spec.CreateHistory (and copies any other relevant
pipeline-level flags), and pass that Options into pipeline.Run instead of
lib.Options{} so history recording and other flags are honored.
In `@internal/scheduling/manila/filter_weigher_pipeline_controller.go`:
- Line 124: process() currently calls pipeline.Run(request, lib.Options{}) which
forces opts.RecordHistory=false and diverges from the Nova controller pattern;
fix by retrieving the matching pipelineConf from c.PipelineConfigs (using the
request/pipeline identifier present in process()) and build options the same way
buildOptions(request, pipelineConf) does so that opts.RecordHistory =
pipelineConf.Spec.CreateHistory before calling pipeline.Run; alternatively,
change process() to accept an opts lib.Options parameter propagated from callers
and pass that opts into pipeline.Run instead of lib.Options{}—update all call
sites accordingly.
In `@internal/scheduling/nova/filter_weigher_pipeline_controller.go`:
- Around line 40-42: buildOptions never sets Options.ReadOnly, so peekReadOnly
and the RLock paths are dead; change buildOptions to set opts.ReadOnly = true
when the run will not mutate state (e.g., when RecordHistory, CreateInflight and
LockReservations are all false) so peekReadOnly(decision) can return true and
the RLock branches in Reconcile and ProcessNewDecisionFromAPI become reachable;
update buildOptions (the function that constructs Options), reference
Options.ReadOnly, peekReadOnly, and processMu, and run/update any affected
tests.
- Around line 199-212: peekReadOnly reads c.PipelineConfigs without
synchronization and races with
HandlePipelineCreated/HandlePipelineUpdated/HandlePipelineDeleted; fix by
acquiring the controller's read lock around accesses to the map (use
c.processMu.RLock() / defer c.processMu.RUnlock()) in peekReadOnly (surround the
lookup of c.PipelineConfigs[decision.Spec.PipelineRef.Name] and any subsequent
use until you return), ensuring all early returns still release the lock;
alternatively, if you prefer map-level sync in BasePipelineController, ensure
mutations in HandlePipelineCreated/Updated/Deleted take the same lock, but the
simplest change is adding RLock/RUnlock in peekReadOnly around the map access
and before calling buildOptions.
In `@internal/scheduling/reservations/scheduler_client.go`:
- Around line 82-85: ScheduleReservation currently ignores the Options field
declared on the request (Options in the struct) because ScheduleReservation
builds externalSchedulerRequest without copying LockReservations/MaxCandidates
from req.Options, so caller-set LockReservations/MaxCandidates (e.g., in
reservation_controller.go) are effectively no-ops; add a prominent TODO comment
at the top of the ScheduleReservation function stating that Options are not
propagated to the externalSchedulerRequest today, list the specific fields
dropped (LockReservations, MaxCandidates), reference reservation_controller.go
and externalSchedulerRequest, and include a tracking issue or TODO ID (e.g.,
TODO: propagate Options once direct-call path exists — TRACK-XXXX) so future
changes know to either copy req.Options into externalSchedulerRequest or wire a
direct Go call.
---
Nitpick comments:
In `@internal/scheduling/lib/filter_test.go`:
- Line 19: The mock's RunFunc callback signature currently omits the Options
parameter so the Run wrapper discards options; update the RunFunc type to accept
an opts *Options (or Options) parameter (e.g., change RunFunc func(traceLog
*slog.Logger, request RequestType, opts Options)
(*FilterWeigherPipelineStepResult, error)), then update the Run method to
forward its incoming opts to RunFunc and adjust any test mocks that set RunFunc
to accept and assert on opts (also update other mock occurrences referenced
around the Run wrapper). Ensure references to RunFunc, Run, Options,
RequestType, and FilterWeigherPipelineStepResult are updated consistently.
In `@internal/scheduling/lib/filter_weigher_pipeline_test.go`:
- Around line 415-423: Replace the manual membership loop that checks hosts in
result.OrderedHosts with slices.Contains(result.OrderedHosts, host) to follow
repo conventions; update the import block to include the standard library
"slices" package and remove the now-unnecessary found/loop code (references:
result.AggregatedOutWeights and result.OrderedHosts).
In `@internal/scheduling/lib/filter_weigher_pipeline.go`:
- Around line 305-311: When trimming hosts into opts.MaxCandidates, avoid the
quadratic check by first constructing a small lookup set of the kept hosts (e.g.
kept := map[string]struct{} and populate it from hosts[:opts.MaxCandidates]) and
then iterate over outWeights deleting entries where the host key is not present
in that set; replace slices.Contains(hosts, host) with a constant-time map
lookup. Update references around hosts, outWeights and the trimming block in
filter_weigher_pipeline.go accordingly.
In `@internal/scheduling/lib/weigher_test.go`:
- Around line 37-42: The mock Run method for mockWeigher currently accepts opts
but doesn't pass it to the configured callback, so update mockWeigher.Run to
call m.RunFunc with (traceLog, request, opts) instead of (traceLog, request) and
ensure the RunFunc signature (and any test assignments) match the three-argument
form; this allows test callbacks on RunFunc to assert per-call Options
propagation for FilterWeigherPipelineStepResult handling.
In `@internal/scheduling/nova/filter_weigher_pipeline_controller.go`:
- Around line 215-227: The buildOptions function currently constructs
lib.Options but never calls Options.Validate(), so invalid combinations (e.g.,
ReadOnly vs RecordHistory/CreateInflight) can propagate; update
FilterWeigherPipelineController.buildOptions to call opts.Validate() after
construction and handle the returned error (e.g., propagate it to the caller or
convert it into a failed pipeline status in the calling process method),
ensuring you reference lib.Options and its Validate() method and adjust callers
of buildOptions (or process) to surface the validation error rather than
allowing an invalid opts to reach pipeline.Run.
- Around line 56-83: Add a short explanatory comment in Reconcile next to the
RLock branch (around the call to c.processMu.RLock() / c.processMu.RUnlock())
stating that readers intentionally do not re-fetch the Decision after acquiring
the read lock because multiple concurrent readers should observe the same cached
state and re-fetching would be unnecessary/expensive; keep the existing re-fetch
only on the exclusive Lock path (used when peekReadOnly(decision) is false) to
ensure writers see a consistent, up-to-date object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d607a9c-4902-493e-b539-38709b8ba3a9
📒 Files selected for processing (90)
internal/scheduling/cinder/filter_weigher_pipeline_controller.gointernal/scheduling/lib/filter_monitor.gointernal/scheduling/lib/filter_monitor_test.gointernal/scheduling/lib/filter_test.gointernal/scheduling/lib/filter_validation.gointernal/scheduling/lib/filter_validation_test.gointernal/scheduling/lib/filter_weigher_pipeline.gointernal/scheduling/lib/filter_weigher_pipeline_step.gointernal/scheduling/lib/filter_weigher_pipeline_step_monitor.gointernal/scheduling/lib/filter_weigher_pipeline_step_monitor_test.gointernal/scheduling/lib/filter_weigher_pipeline_test.gointernal/scheduling/lib/options.gointernal/scheduling/lib/weigher_monitor.gointernal/scheduling/lib/weigher_monitor_test.gointernal/scheduling/lib/weigher_test.gointernal/scheduling/lib/weigher_validation.gointernal/scheduling/lib/weigher_validation_test.gointernal/scheduling/machines/filter_weigher_pipeline_controller.gointernal/scheduling/machines/filter_weigher_pipeline_controller_test.gointernal/scheduling/machines/plugins/filters/filter_noop.gointernal/scheduling/machines/plugins/filters/filter_noop_test.gointernal/scheduling/manila/filter_weigher_pipeline_controller.gointernal/scheduling/manila/plugins/weighers/netapp_cpu_usage_balancing.gointernal/scheduling/manila/plugins/weighers/netapp_cpu_usage_balancing_test.gointernal/scheduling/nova/filter_weigher_pipeline_controller.gointernal/scheduling/nova/filter_weigher_pipeline_controller_test.gointernal/scheduling/nova/plugins/filters/filter_aggregate_metadata.gointernal/scheduling/nova/plugins/filters/filter_aggregate_metadata_test.gointernal/scheduling/nova/plugins/filters/filter_allowed_projects.gointernal/scheduling/nova/plugins/filters/filter_allowed_projects_test.gointernal/scheduling/nova/plugins/filters/filter_capabilities.gointernal/scheduling/nova/plugins/filters/filter_capabilities_test.gointernal/scheduling/nova/plugins/filters/filter_correct_az.gointernal/scheduling/nova/plugins/filters/filter_correct_az_test.gointernal/scheduling/nova/plugins/filters/filter_exclude_hosts.gointernal/scheduling/nova/plugins/filters/filter_exclude_hosts_test.gointernal/scheduling/nova/plugins/filters/filter_external_customer.gointernal/scheduling/nova/plugins/filters/filter_external_customer_test.gointernal/scheduling/nova/plugins/filters/filter_has_accelerators.gointernal/scheduling/nova/plugins/filters/filter_has_accelerators_test.gointernal/scheduling/nova/plugins/filters/filter_has_enough_capacity.gointernal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.gointernal/scheduling/nova/plugins/filters/filter_has_requested_traits.gointernal/scheduling/nova/plugins/filters/filter_has_requested_traits_test.gointernal/scheduling/nova/plugins/filters/filter_host_instructions.gointernal/scheduling/nova/plugins/filters/filter_host_instructions_test.gointernal/scheduling/nova/plugins/filters/filter_instance_group_affinity.gointernal/scheduling/nova/plugins/filters/filter_instance_group_affinity_test.gointernal/scheduling/nova/plugins/filters/filter_instance_group_anti_affinity.gointernal/scheduling/nova/plugins/filters/filter_instance_group_anti_affinity_test.gointernal/scheduling/nova/plugins/filters/filter_live_migratable.gointernal/scheduling/nova/plugins/filters/filter_live_migratable_test.gointernal/scheduling/nova/plugins/filters/filter_requested_destination.gointernal/scheduling/nova/plugins/filters/filter_requested_destination_test.gointernal/scheduling/nova/plugins/filters/filter_status_conditions.gointernal/scheduling/nova/plugins/filters/filter_status_conditions_test.gointernal/scheduling/nova/plugins/weighers/kvm_binpack.gointernal/scheduling/nova/plugins/weighers/kvm_binpack_test.gointernal/scheduling/nova/plugins/weighers/kvm_failover_evacuation.gointernal/scheduling/nova/plugins/weighers/kvm_failover_evacuation_test.gointernal/scheduling/nova/plugins/weighers/kvm_failover_reservation_consolidation.gointernal/scheduling/nova/plugins/weighers/kvm_failover_reservation_consolidation_test.gointernal/scheduling/nova/plugins/weighers/kvm_instance_group_soft_affinity.gointernal/scheduling/nova/plugins/weighers/kvm_instance_group_soft_affinity_test.gointernal/scheduling/nova/plugins/weighers/kvm_prefer_smaller_hosts.gointernal/scheduling/nova/plugins/weighers/kvm_prefer_smaller_hosts_test.gointernal/scheduling/nova/plugins/weighers/vmware_anti_affinity_noisy_projects.gointernal/scheduling/nova/plugins/weighers/vmware_anti_affinity_noisy_projects_test.gointernal/scheduling/nova/plugins/weighers/vmware_avoid_long_term_contended_hosts.gointernal/scheduling/nova/plugins/weighers/vmware_avoid_long_term_contended_hosts_test.gointernal/scheduling/nova/plugins/weighers/vmware_avoid_short_term_contended_hosts.gointernal/scheduling/nova/plugins/weighers/vmware_avoid_short_term_contended_hosts_test.gointernal/scheduling/nova/plugins/weighers/vmware_binpack.gointernal/scheduling/nova/plugins/weighers/vmware_binpack_test.gointernal/scheduling/pods/filter_weigher_pipeline_controller.gointernal/scheduling/pods/filter_weigher_pipeline_controller_test.gointernal/scheduling/pods/plugins/filters/filter_node_affinity.gointernal/scheduling/pods/plugins/filters/filter_node_affinity_test.gointernal/scheduling/pods/plugins/filters/filter_node_available.gointernal/scheduling/pods/plugins/filters/filter_node_available_test.gointernal/scheduling/pods/plugins/filters/filter_node_capacity.gointernal/scheduling/pods/plugins/filters/filter_node_capacity_test.gointernal/scheduling/pods/plugins/filters/filter_noop.gointernal/scheduling/pods/plugins/filters/filter_noop_test.gointernal/scheduling/pods/plugins/filters/filter_taint.gointernal/scheduling/pods/plugins/filters/filter_taint_test.gointernal/scheduling/pods/plugins/weighers/binpack.gointernal/scheduling/pods/plugins/weighers/binpack_test.gointernal/scheduling/reservations/commitments/reservation_controller.gointernal/scheduling/reservations/scheduler_client.go
Comment on lines
+40
to
+45
| if o.ReadOnly && o.RecordHistory { | ||
| return errors.New("ReadOnly and RecordHistory are mutually exclusive: read-only runs must not mutate state") | ||
| } | ||
| if o.ReadOnly && o.CreateInflight { | ||
| return errors.New("ReadOnly and CreateInflight are mutually exclusive: read-only runs must not mutate state") | ||
| } |
There was a problem hiding this comment.
Use lowercase error messages to satisfy linting
At Line [41] and Line [44], error strings start with uppercase words. This can fail lint checks in this repo.
🔧 Suggested patch
- return errors.New("ReadOnly and RecordHistory are mutually exclusive: read-only runs must not mutate state")
+ return errors.New("readonly and record history are mutually exclusive: read-only runs must not mutate state")
}
if o.ReadOnly && o.CreateInflight {
- return errors.New("ReadOnly and CreateInflight are mutually exclusive: read-only runs must not mutate state")
+ return errors.New("readonly and create inflight are mutually exclusive: read-only runs must not mutate state")
}As per coding guidelines: "Error messages should always be lowercase to conform to linting rules".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if o.ReadOnly && o.RecordHistory { | |
| return errors.New("ReadOnly and RecordHistory are mutually exclusive: read-only runs must not mutate state") | |
| } | |
| if o.ReadOnly && o.CreateInflight { | |
| return errors.New("ReadOnly and CreateInflight are mutually exclusive: read-only runs must not mutate state") | |
| } | |
| if o.ReadOnly && o.RecordHistory { | |
| return errors.New("readonly and record history are mutually exclusive: read-only runs must not mutate state") | |
| } | |
| if o.ReadOnly && o.CreateInflight { | |
| return errors.New("readonly and create inflight are mutually exclusive: read-only runs must not mutate state") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/scheduling/lib/options.go` around lines 40 - 45, The validation
currently returns error messages that start with uppercase letters; update the
error strings returned when o.ReadOnly && o.RecordHistory and when o.ReadOnly &&
o.CreateInflight to start with lowercase (e.g. change "ReadOnly and
RecordHistory are mutually exclusive: ..." to "readOnly and recordHistory are
mutually exclusive: ..." or similar lowercase wording) in the function that
checks o.ReadOnly, o.RecordHistory and o.CreateInflight so they satisfy the lint
rule; keep the same descriptive text but make the first character lowercase for
both error.New(...) calls.
| // Execute the scheduling pipeline. | ||
| request := ironcore.MachinePipelineRequest{Pools: pools.Items} | ||
| result, err := pipeline.Run(request) | ||
| result, err := pipeline.Run(request, lib.Options{}) |
There was a problem hiding this comment.
process() passes empty lib.Options{}, silently dropping RecordHistory and other pipeline-level flags.
Unlike the Nova controller — which calls buildOptions(request, pipelineConf) to set at minimum RecordHistory: pipelineConf.Spec.CreateHistory — process() here has no access to pipelineConf and always passes a zero-value Options{}. If the pipeline already consumes opts.RecordHistory, history recording will be disabled even when pipelineConf.Spec.CreateHistory is true.
c.PipelineConfigs[decision.Spec.PipelineRef.Name] is reachable inside process() (via the embedded BasePipelineController), so a buildOptions()-style helper can be added here without restructuring the call chain.
💡 Suggested approach
+func (c *FilterWeigherPipelineController) buildOptions(pipelineConf v1alpha1.Pipeline) lib.Options {
+ return lib.Options{
+ RecordHistory: pipelineConf.Spec.CreateHistory,
+ }
+}
func (c *FilterWeigherPipelineController) process(ctx context.Context, decision *v1alpha1.Decision) error {
...
pipeline, ok := c.Pipelines[decision.Spec.PipelineRef.Name]
if !ok { ... }
+ pipelineConf, ok := c.PipelineConfigs[decision.Spec.PipelineRef.Name]
+ if !ok {
+ return errors.New("pipeline config not found")
+ }
...
- result, err := pipeline.Run(request, lib.Options{})
+ result, err := pipeline.Run(request, c.buildOptions(pipelineConf))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/scheduling/machines/filter_weigher_pipeline_controller.go` at line
147, process() currently calls pipeline.Run(request, lib.Options{}) which always
passes a zero-value lib.Options and thus drops flags like RecordHistory; to fix,
retrieve the pipeline config from c.PipelineConfigs using
decision.Spec.PipelineRef.Name (accessible via the embedded
BasePipelineController), construct an Options struct (similar to
buildOptions(request, pipelineConf) used in Nova) that at minimum sets
RecordHistory = pipelineConf.Spec.CreateHistory (and copies any other relevant
pipeline-level flags), and pass that Options into pipeline.Run instead of
lib.Options{} so history recording and other flags are honored.
Contributor
Test Coverage ReportTest Coverage 📊: 69.2% |
PhilippMatthes
requested changes
May 6, 2026
Member
PhilippMatthes
left a comment
PhilippMatthes
left a comment
There was a problem hiding this comment.
I like the idea of extending the external scheduler request(s) with an options struct. However, we should go over the fields exposed by this struct once again. I am not sure about putting it under internal/scheduling/lib as we have lots of fields that will only be used by the Nova integration of cortex. So, why not put the options struct under the nova external scheduler request struct? In that way, you don't need to extend the lib interface and conform all other packages.
| // Run the filter and observe its execution. | ||
| func (fm *FilterMonitor[RequestType]) Run(traceLog *slog.Logger, request RequestType) (*FilterWeigherPipelineStepResult, error) { | ||
| return fm.monitor.RunWrapped(traceLog, request, fm.filter) | ||
| func (fm *FilterMonitor[RequestType]) Run(traceLog *slog.Logger, request RequestType, opts Options) (*FilterWeigherPipelineStepResult, error) { |
Member
There was a problem hiding this comment.
request RequestType is constrained by type FilterWeigherPipelineRequest interface which now carries the new GetOptions() method. So we should remove all additional opts Options parameters.
| // Run filters first to reduce the number of hosts. | ||
| // Any weights assigned to filtered out hosts are ignored. | ||
| filteredRequest, filterStepResults := p.runFilters(traceLog, request) | ||
| filteredRequest, filterStepResults := p.runFilters(traceLog, request, opts) |
Member
There was a problem hiding this comment.
Please revert this change. Scheduler steps can get the options from the provided request.
| } | ||
| } | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Please provide logging here so we see what's going on.
| // which are static and set when the pipeline is initialized. | ||
| // | ||
| // Consumed by steps: ReadOnly, LockReservations, AssumeEmptyHosts, IgnoredReservationTypes. | ||
| // Consumed by the controller after pipeline.Run(): RecordHistory, CreateInflight. |
Member
There was a problem hiding this comment.
These two code comment lines are likely to be obsolete once the controller or step logic changes. We should consider removing them.
| type Options struct { | ||
| // ReadOnly means the pipeline run does not modify shared scheduling state (reservations, | ||
| // history, inflight records). Concurrent read-only runs are safe under a shared read lock. | ||
| // Note: the controller may still write the Decision status after Run() regardless of this flag. |
Member
There was a problem hiding this comment.
This seems inconsistent. Shouldn't we draw the line here? Read-only requests create or modify NO resources and are purely to calculate host candidates for constraints.
| ReadOnly bool | ||
| // LockReservations prevents reservation unlocking, e.g. in the capacity filter. | ||
| // Set when finding hosts for new reservations (failover, CR) to see true available capacity. | ||
| LockReservations bool |
Member
There was a problem hiding this comment.
Should this be more generic such as Kind which is a typed enum? In this case, the capacity filter would just check for req.GetOptions().Kind == KindFailoverReservation to control which logic is executed. We could also add a kind KindCapacityScan for limes etc. -- this is nicely extensible and well-defined. In this case, the ReadOnly, AssumeEmptyHosts, and CreateInFlight flags could also be removed.
| // AssumeEmptyHosts treats all hosts as having no running VMs. | ||
| AssumeEmptyHosts bool | ||
| // IgnoredReservationTypes lists reservation types the capacity filter skips entirely. | ||
| IgnoredReservationTypes []v1alpha1.ReservationType |
Member
There was a problem hiding this comment.
Should we make this a substruct such as type ReservationOptions struct?
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Introduce call-time pipeline options as the foundation for consolidating duplicate pipelines — behavioral variations (lock reservations, record history, pessimistic blocking) become explicit flags instead of separate pipelines