Open
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (5)
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive per-call scheduling options mechanism throughout the codebase. A new ChangesPer-Call Scheduling Options and History Management Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 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.
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.
Collaborator
Author
There was a problem hiding this comment.
There are use cases, we can discuss live
| 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.
Collaborator
Author
There was a problem hiding this comment.
Not sure I get this comment, probably better to discuss live
| // 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?
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/scheduling/reservations/scheduler_client.go (1)
93-120:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
optsbefore sending the request.Invalid option combinations currently pay the marshal/HTTP round-trip and then come back as a generic scheduler failure. Failing fast here preserves the concrete validation error and avoids needless I/O.
Proposed fix
func (c *SchedulerClient) ScheduleReservation(ctx context.Context, req ScheduleReservationRequest, opts lib.Options) (*ScheduleReservationResponse, error) { logger := loggerFromContext(ctx) + if err := opts.Validate(); err != nil { + return nil, fmt.Errorf("invalid schedule options: %w", err) + } // Build weights map (all zero for reservations) weights := make(map[string]float64, len(req.EligibleHosts))🤖 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/reservations/scheduler_client.go` around lines 93 - 120, In ScheduleReservation, validate the incoming opts before constructing and sending externalSchedulerRequest to fail fast on bad option combinations: call the existing options validation function (e.g., lib.ValidateOptions or a new method on lib.Options like opts.Validate()) right after logger := loggerFromContext(ctx) and before building weights/ignoreHosts or making the HTTP call; if validation returns an error, return nil and that error immediately so the concrete validation error is preserved instead of performing the marshal/HTTP round-trip.
🧹 Nitpick comments (2)
internal/scheduling/nova/filter_weigher_pipeline_controller.go (2)
177-184: 💤 Low valueNote: callers cannot explicitly opt out of
RecordHistorywhenpipelineConf.Spec.CreateHistoryis true.Because
RecordHistoryis abool(zero-valuefalse), theif !request.Options.RecordHistorycheck cannot distinguish "caller set it tofalse" from "caller did not set it". An explicit caller-falseis silently overridden when the pipeline config enables history. If per-call opt-out is intended, switch to*bool(or an enum/tri-state). If the design is that pipeline config always wins, ignore.🤖 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 177 - 184, The code overrides caller intent because request.Options.RecordHistory is a bool and false could mean "unset" or "explicitly disabled"; change RecordHistory to a *bool (or other tri-state) on the Options type, update the fill logic in the controller to only apply pipelineConf.Spec.CreateHistory when request.Options.RecordHistory == nil (e.g. set ptr to &pipelineConf.Spec.CreateHistory), and update all uses (checks like if request.Options.RecordHistory { ... } and the upsertHistory call) to dereference the pointer safely (treat nil as resolved default). This preserves explicit caller false while keeping the pipeline default behavior.
200-211: 💤 Low valueOptional: avoid double-unmarshalling
NovaRaw.
peekReadOnlyparsesdecision.Spec.NovaRaw.Rawto readOptions.ReadOnly, thenprocess(line 149) unmarshals the same payload again. For non-trivial requests this is wasted work on every reconcile. Consider parsing once (e.g., return the decodedrequestfrompeekReadOnly, or stash it on the controller call frame) and threading it intoprocess. Low priority — keep as-is if the JSON is expected to stay small.🤖 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 200 - 211, peekReadOnly currently unmarshals decision.Spec.NovaRaw.Raw and then process re-unmarshals the same payload; change peekReadOnly to return the decoded api.ExternalSchedulerRequest (or a pointer) instead of just bool, or store the decoded request on the controller call frame and pass it into FilterWeigherPipelineController.process so process can reuse the same decoded value; update the signatures/usages of peekReadOnly and process (and any callers) to accept/propagate the decoded request (e.g., return (*api.ExternalSchedulerRequest, bool) or (api.ExternalSchedulerRequest, error) and use that result in process) to eliminate the duplicate json.Unmarshal.
🤖 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 `@api/external/nova/messages.go`:
- Around line 41-47: The Options field is currently annotated as "Nova does not
set these" but incoming Options are passed through unchanged; update the
contract or enforce server-side defaults: either clarify the comment on
ExternalSchedulerRequest.Options/ExternalSchedulerRequest.GetOptions to state
this endpoint is trust-only (Nova only), or—preferable—reset/validate
caller-supplied options in the request handling code (e.g., in the
FilterWeigherPipelineController pipeline code where RecordHistory is set) by
replacing or sanitizing Options before use (clear or set safe defaults for
AssumeEmptyHosts, IgnoredReservationTypes, MaxCandidates, etc., or whitelist
only allowed fields), and ensure the pipeline uses the sanitizedOptions instance
everywhere downstream.
---
Outside diff comments:
In `@internal/scheduling/reservations/scheduler_client.go`:
- Around line 93-120: In ScheduleReservation, validate the incoming opts before
constructing and sending externalSchedulerRequest to fail fast on bad option
combinations: call the existing options validation function (e.g.,
lib.ValidateOptions or a new method on lib.Options like opts.Validate()) right
after logger := loggerFromContext(ctx) and before building weights/ignoreHosts
or making the HTTP call; if validation returns an error, return nil and that
error immediately so the concrete validation error is preserved instead of
performing the marshal/HTTP round-trip.
---
Nitpick comments:
In `@internal/scheduling/nova/filter_weigher_pipeline_controller.go`:
- Around line 177-184: The code overrides caller intent because
request.Options.RecordHistory is a bool and false could mean "unset" or
"explicitly disabled"; change RecordHistory to a *bool (or other tri-state) on
the Options type, update the fill logic in the controller to only apply
pipelineConf.Spec.CreateHistory when request.Options.RecordHistory == nil (e.g.
set ptr to &pipelineConf.Spec.CreateHistory), and update all uses (checks like
if request.Options.RecordHistory { ... } and the upsertHistory call) to
dereference the pointer safely (treat nil as resolved default). This preserves
explicit caller false while keeping the pipeline default behavior.
- Around line 200-211: peekReadOnly currently unmarshals
decision.Spec.NovaRaw.Raw and then process re-unmarshals the same payload;
change peekReadOnly to return the decoded api.ExternalSchedulerRequest (or a
pointer) instead of just bool, or store the decoded request on the controller
call frame and pass it into FilterWeigherPipelineController.process so process
can reuse the same decoded value; update the signatures/usages of peekReadOnly
and process (and any callers) to accept/propagate the decoded request (e.g.,
return (*api.ExternalSchedulerRequest, bool) or (api.ExternalSchedulerRequest,
error) and use that result in process) to eliminate the duplicate
json.Unmarshal.
🪄 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: 1768f737-74bf-42e3-888e-6024900bab8a
📒 Files selected for processing (19)
api/external/cinder/messages.goapi/external/ironcore/messages.goapi/external/manila/messages.goapi/external/nova/messages.goapi/external/pods/messages.gointernal/scheduling/lib/filter_weigher_pipeline.gointernal/scheduling/lib/filter_weigher_pipeline_request.gointernal/scheduling/lib/filter_weigher_pipeline_request_test.gointernal/scheduling/lib/filter_weigher_pipeline_step.gointernal/scheduling/lib/filter_weigher_pipeline_test.gointernal/scheduling/lib/options.gointernal/scheduling/lib/options_test.gointernal/scheduling/nova/filter_weigher_pipeline_controller.gointernal/scheduling/nova/filter_weigher_pipeline_controller_test.gointernal/scheduling/nova/plugins/filters/filter_has_enough_capacity.gointernal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.gointernal/scheduling/reservations/commitments/reservation_controller.gointernal/scheduling/reservations/failover/reservation_scheduling.gointernal/scheduling/reservations/scheduler_client.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go
Comment on lines
+41
to
+47
| // Options configure the pipeline behavior for this scheduling call. | ||
| // Set by the caller (CR controller, failover controller, Nova). | ||
| // Nova does not set these; Cortex fills in config-derived defaults server-side. | ||
| Options lib.Options `json:"options,omitempty"` | ||
| } | ||
|
|
||
| func (r ExternalSchedulerRequest) GetOptions() lib.Options { return r.Options } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where Nova's ExternalSchedulerRequest is deserialized and where Options is overwritten on the server side.
rg -nP -C5 'ExternalSchedulerRequest' --type=go
rg -nP -C5 'request\.Options\s*=' --type=go
rg -nP -C5 'buildOptions' --type=goRepository: cobaltcore-dev/cortex
Length of output: 50378
🏁 Script executed:
#!/bin/bash
# Find HTTP handlers that accept ExternalSchedulerRequest (POST endpoints from Nova)
rg -nP 'func.*ExternalSchedulerRequest' --type=go | grep -v test | head -20
# Search for code that might override or ignore incoming Options
rg -nP '\.Options\s*=' --type=go | grep -v test | head -20
# Look for "buildOptions" or similar factory functions
rg -nP '(buildOptions|NewOptions|defaultOptions)' --type=go -i | grep -v test | head -20Repository: cobaltcore-dev/cortex
Length of output: 4457
🏁 Script executed:
#!/bin/bash
# Find the HTTP handler that receives requests FROM Nova (not sends to Nova)
# Look for POST handlers or API routes
rg -nP '(HandleFunc|POST|POST)' internal/scheduling --type=go | grep -i 'schedule\|request' | head -20
# Look at filter_weigher_pipeline_controller to see how it processes incoming requests
fd 'filter_weigher_pipeline_controller' --type=go -x cat -n {} | head -100
# Search for where the ExternalSchedulerRequest JSON is decoded
rg -nP 'Decode.*ExternalSchedulerRequest|json\.Unmarshal.*ExternalSchedulerRequest' --type=go -B3 -A3 | head -50Repository: cobaltcore-dev/cortex
Length of output: 2785
🏁 Script executed:
#!/bin/bash
# Read the external_scheduler_api.go file to see how incoming requests are handled
cat -n internal/scheduling/nova/external_scheduler_api.goRepository: cobaltcore-dev/cortex
Length of output: 11670
🏁 Script executed:
#!/bin/bash
# Search for where request.Options or r.Options is used in the pipeline
rg -nP 'request\.Options|\.GetOptions\(\)' --type=go -C3 | head -60
# Look for any code that builds or resets Options after the request is received
rg -nP 'buildOptions|setOptions|resetOptions' --type=go -i -C2Repository: cobaltcore-dev/cortex
Length of output: 5767
🏁 Script executed:
#!/bin/bash
# Check the Options struct definition to see all fields
fd -e go 'Options' api/external/nova --type=f -x cat -n {} | head -100
# Also check what the full Options struct looks like
rg -nP 'type Options struct' --type=go -A20Repository: cobaltcore-dev/cortex
Length of output: 1990
Clarify Options handling contract between Nova and Cortex, or reset untrusted caller-supplied Options.
The field comment states that "Nova does not set these; Cortex fills in config-derived defaults server-side," but the code only partially enforces this: in filter_weigher_pipeline_controller.go (lines 177–180), only RecordHistory is conditionally set from pipeline config. Other Options fields (AssumeEmptyHosts, IgnoredReservationTypes, MaxCandidates, etc.) are passed through directly from the incoming request without validation or reset.
If this API is restricted to trusted Nova calls only, the comment should clarify that. If external/untrusted callers can reach this endpoint, the incoming Options should be reset or validated to prevent caller-supplied flags like AssumeEmptyHosts: true or MaxCandidates: 1 from bypassing capacity checks or placement logic.
🤖 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 `@api/external/nova/messages.go` around lines 41 - 47, The Options field is
currently annotated as "Nova does not set these" but incoming Options are passed
through unchanged; update the contract or enforce server-side defaults: either
clarify the comment on
ExternalSchedulerRequest.Options/ExternalSchedulerRequest.GetOptions to state
this endpoint is trust-only (Nova only), or—preferable—reset/validate
caller-supplied options in the request handling code (e.g., in the
FilterWeigherPipelineController pipeline code where RecordHistory is set) by
replacing or sanitizing Options before use (clear or set safe defaults for
AssumeEmptyHosts, IgnoredReservationTypes, MaxCandidates, etc., or whitelist
only allowed fields), and ensure the pipeline uses the sanitizedOptions instance
everywhere downstream.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/scheduling/lib/options.go (1)
39-44:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLowercase the error messages.
The error strings on Lines 40 and 43 start with uppercase identifiers (
ReadOnly and ...), which violates the repository's lint rule that error messages must be lowercase.As per coding guidelines: "Error messages should always be lowercase to conform to linting rules".
🤖 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 39 - 44, The returned error messages in the validation logic for the options struct use capitalized text; change the strings in the checks that reference o.ReadOnly && o.RecordHistory and o.ReadOnly && o.CreateInflight so they start with lowercase (e.g., "readOnly and recordHistory..." -> "readOnly and recordHistory..." or better "read-only and record-history are mutually exclusive: read-only runs must not write scheduling history" and similarly for inflight), updating the error return values in the function containing those checks so they conform to the repository lint rule requiring lowercase error messages.
🧹 Nitpick comments (2)
internal/scheduling/lib/filter_weigher_pipeline.go (1)
305-318: 💤 Low valueOptional: simplify the prune step with
maps.DeleteFunc.The kept-set + manual deletion loop can be expressed more concisely (and idiomatically) with
maps.DeleteFunc, which avoids the intermediate set allocation.♻️ Proposed refactor
if opts.MaxCandidates > 0 && len(hosts) > opts.MaxCandidates { traceLog.Info("scheduler: trimming candidate list", "maxCandidates", opts.MaxCandidates, "before", len(hosts)) hosts = hosts[:opts.MaxCandidates] - // Drop trimmed hosts from outWeights so AggregatedOutWeights stays consistent. - kept := make(map[string]struct{}, len(hosts)) - for _, h := range hosts { - kept[h] = struct{}{} - } - for host := range outWeights { - if _, ok := kept[host]; !ok { - delete(outWeights, host) - } - } + // Drop trimmed hosts from outWeights so AggregatedOutWeights stays consistent. + kept := make(map[string]struct{}, len(hosts)) + for _, h := range hosts { + kept[h] = struct{}{} + } + maps.DeleteFunc(outWeights, func(host string, _ float64) bool { + _, ok := kept[host] + return !ok + }) }🤖 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 - 318, The current prune logic uses an intermediate kept set and manual loop to remove trimmed hosts from outWeights; replace it by using maps.DeleteFunc to remove entries whose keys are not in the truncated hosts slice (the slice produced when opts.MaxCandidates > 0 && len(hosts) > opts.MaxCandidates), avoiding the kept map allocation and manual delete loop and keeping AggregatedOutWeights consistent with hosts; call maps.DeleteFunc(outWeights, func(k string) bool { return !containsInTruncatedHosts(k) }) or equivalent using the truncated hosts slice to check membership.internal/scheduling/nova/filter_weigher_pipeline_controller.go (1)
202-213: 💤 Low value
peekReadOnlyre-parsesNovaRawon every reconcile.
peekReadOnlyunmarshalsNovaRawjust to readOptions.ReadOnly(lines 208–213), and thenprocess()unmarshals the same payload again at line 151. Both methods are called sequentially on the samedecisionobject inReconcile()(line 63 then 79) andProcessNewDecisionFromAPI()(line 89 then 97). For decisions with large host/weight payloads, this doubles JSON parsing work. Consider parsing once and passing the request object toprocess(), or use a lightweight shape that only decodes theReadOnlyfield.🤖 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 202 - 213, peekReadOnly currently unmarshals Decision.Spec.NovaRaw.Raw again, causing duplicate JSON parsing because process() also unmarshals the same payload; change the flow so the payload is parsed once and reused: either modify callers (Reconcile and ProcessNewDecisionFromAPI) to unmarshal into api.ExternalSchedulerRequest once and pass that parsed request into process() (and update process() signature) and into peekReadOnly (or replace peekReadOnly with a lightweight helper that accepts the parsed request), or implement a tiny struct that only decodes Options.ReadOnly (e.g., type readOnlyOnly struct{ Options struct{ ReadOnly bool } }) and use that in peekReadOnly to avoid full reparse; ensure references to Decision.Spec.NovaRaw, api.ExternalSchedulerRequest, peekReadOnly, process(), Reconcile, and ProcessNewDecisionFromAPI are updated 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/nova/filter_weigher_pipeline_controller.go`:
- Around line 179-186: The code defaults request.Options.RecordHistory from
pipelineConf.Spec.CreateHistory even when the caller set Options.ReadOnly,
causing a conflict in opts.Validate(); change the logic in
filter_weigher_pipeline_controller.go so RecordHistory is only defaulted when
the caller did not explicitly request ReadOnly (i.e., skip applying
pipelineConf.Spec.CreateHistory if request.Options.ReadOnly is true) or, better,
treat RecordHistory as an explicitly-set value (e.g., add an explicit
"IsRecordHistorySet" check) and only fill it when unset; update the branch
around request.Options.RecordHistory / pipelineConf.Spec.CreateHistory and
ensure pipeline.Run(request) no longer receives conflicting ReadOnly and
RecordHistory flags (see symbols: request.Options.RecordHistory,
request.Options.ReadOnly, pipelineConf.Spec.CreateHistory, pipeline.Run and
opts.Validate).
---
Duplicate comments:
In `@internal/scheduling/lib/options.go`:
- Around line 39-44: The returned error messages in the validation logic for the
options struct use capitalized text; change the strings in the checks that
reference o.ReadOnly && o.RecordHistory and o.ReadOnly && o.CreateInflight so
they start with lowercase (e.g., "readOnly and recordHistory..." -> "readOnly
and recordHistory..." or better "read-only and record-history are mutually
exclusive: read-only runs must not write scheduling history" and similarly for
inflight), updating the error return values in the function containing those
checks so they conform to the repository lint rule requiring lowercase error
messages.
---
Nitpick comments:
In `@internal/scheduling/lib/filter_weigher_pipeline.go`:
- Around line 305-318: The current prune logic uses an intermediate kept set and
manual loop to remove trimmed hosts from outWeights; replace it by using
maps.DeleteFunc to remove entries whose keys are not in the truncated hosts
slice (the slice produced when opts.MaxCandidates > 0 && len(hosts) >
opts.MaxCandidates), avoiding the kept map allocation and manual delete loop and
keeping AggregatedOutWeights consistent with hosts; call
maps.DeleteFunc(outWeights, func(k string) bool { return
!containsInTruncatedHosts(k) }) or equivalent using the truncated hosts slice to
check membership.
In `@internal/scheduling/nova/filter_weigher_pipeline_controller.go`:
- Around line 202-213: peekReadOnly currently unmarshals
Decision.Spec.NovaRaw.Raw again, causing duplicate JSON parsing because
process() also unmarshals the same payload; change the flow so the payload is
parsed once and reused: either modify callers (Reconcile and
ProcessNewDecisionFromAPI) to unmarshal into api.ExternalSchedulerRequest once
and pass that parsed request into process() (and update process() signature) and
into peekReadOnly (or replace peekReadOnly with a lightweight helper that
accepts the parsed request), or implement a tiny struct that only decodes
Options.ReadOnly (e.g., type readOnlyOnly struct{ Options struct{ ReadOnly bool
} }) and use that in peekReadOnly to avoid full reparse; ensure references to
Decision.Spec.NovaRaw, api.ExternalSchedulerRequest, peekReadOnly, process(),
Reconcile, and ProcessNewDecisionFromAPI are updated accordingly.
🪄 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: 9f52024e-914b-4aa5-8dcc-6cb6d769b7ea
📒 Files selected for processing (4)
internal/scheduling/lib/filter_weigher_pipeline.gointernal/scheduling/lib/filter_weigher_pipeline_test.gointernal/scheduling/lib/options.gointernal/scheduling/nova/filter_weigher_pipeline_controller.go
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/scheduling/machines/filter_weigher_pipeline_controller_test.go (1)
337-337:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMisleading test case name: says "without history creation" but now expects history to be created.
Since
request.Optionsis always zero-valued for the machines controller (SkipHistorydefaults tofalse), history is unconditionally recorded on every successful run. The name "without history creation" no longer reflects the test's actual assertion (expectHistoryCreated: true).✏️ Suggested rename
- name: "successful machine processing without history creation", + name: "successful machine processing always records history",🤖 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_test.go` at line 337, Rename the misleading test case in filter_weigher_pipeline_controller_test.go where the test entry currently has name "successful machine processing without history creation" to reflect that history is created (e.g., "successful machine processing with history creation"); update the test case name in the test table/array that contains expectHistoryCreated: true so it matches the assertion and avoid confusion with request.Options.SkipHistory (which is zero-valued/false for the machines controller).
🤖 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 `@api/scheduling/options_test.go`:
- Around line 14-21: Remove the duplicate test rows and add a case that actually
exercises the CreateInflight validation: update the table of test cases for
Options so only one zero-value entry remains (remove the duplicated Options{}),
remove the duplicated Options{ReadOnly: true, CreateInflight: true} row, and add
a new test case using Options{ReadOnly: true, CreateInflight: true, SkipHistory:
true} so Validate() passes the first ReadOnly && !SkipHistory guard and
exercises the ReadOnly && CreateInflight rule in Options.Validate().
In `@api/scheduling/options.go`:
- Line 43: Change the error string to start with a lowercase rune to satisfy
ST1005: replace the current errors.New(...) message that begins "ReadOnly and
CreateInflight..." with a message that begins lowercase (e.g., "readOnly and
CreateInflight are mutually exclusive: read-only runs must not create inflight
reservations" or use hyphenated field names like "read-only and create-inflight
are mutually exclusive...") in the same return statement so the error text in
options.go uses a lowercase first letter.
In `@internal/scheduling/cinder/filter_weigher_pipeline_controller_test.go`:
- Line 378: The test currently only asserts zero History when !tt.expectError,
so add an explicit assertion of history count == 0 whenever
tt.expectHistoryCreated is false (regardless of tt.expectError) after calling
ProcessNewDecisionFromAPI(); i.e. move or duplicate the expectHistoryCreated
check out of the !tt.expectError branch so the test fails if
ProcessNewDecisionFromAPI() creates History even when it returns an error. Apply
the same change for the other case group around the lines that correspond to the
second nil-raw check (the block covering the additional assertions referenced in
the comment).
In `@internal/scheduling/manila/filter_weigher_pipeline_controller_test.go`:
- Around line 374-376: The test cases with expectHistoryCreated=false (including
the nil-raw case and the block around lines 428-444) only assert history absence
when !tt.expectError, so a History could be created before an error and the test
would still pass; update the test harness in
internal/scheduling/manila/filter_weigher_pipeline_controller_test.go to always
verify the History creation state against tt.expectHistoryCreated regardless of
tt.expectError by adding/asserting the lookup of the History object (or the
absence) after calling ProcessNewDecisionFromAPI() and comparing it to
tt.expectHistoryCreated; reference the test table entries that set
expectHistoryCreated and the call to ProcessNewDecisionFromAPI to locate where
to add the unconditional assertion.
In `@internal/scheduling/nova/filter_weigher_pipeline_controller.go`:
- Around line 179-182: The code calls c.upsertHistory after pipeline.Run guarded
only by request.Options.SkipHistory but not by request.Options.ReadOnly, so a
read-only request can still mutate History; change the guard so history is only
written when both SkipHistory is false AND ReadOnly is false (i.e. if
!request.Options.SkipHistory && !request.Options.ReadOnly), and ensure the call
to c.upsertHistory happens on the write-lock path (the branch that acquired the
exclusive Lock, not the RLock) so history writes are performed only under the
write lock; update the logic around pipeline.Run / lock handling to move or
conditionalize the c.upsertHistory call accordingly.
In `@internal/scheduling/pods/filter_weigher_pipeline_controller.go`:
- Around line 150-156: PodPipelineRequest is built with zero-value Options so
SkipHistory is never set; before calling pipeline.Run(request) populate
request.Options.SkipHistory using the existing pipeline spec fallback: set
request.Options.SkipHistory = !decision.PipelineSpec.CreateHistory (with a
nil-safe check so if decision or decision.PipelineSpec is nil you default to
CreateHistory=true, i.e. SkipHistory=false). This preserves the previous
PipelineSpec.CreateHistory behavior for pod scheduling and prevents
unconditional history writes while we add explicit options to pod requests.
---
Outside diff comments:
In `@internal/scheduling/machines/filter_weigher_pipeline_controller_test.go`:
- Line 337: Rename the misleading test case in
filter_weigher_pipeline_controller_test.go where the test entry currently has
name "successful machine processing without history creation" to reflect that
history is created (e.g., "successful machine processing with history
creation"); update the test case name in the test table/array that contains
expectHistoryCreated: true so it matches the assertion and avoid confusion with
request.Options.SkipHistory (which is zero-valued/false for the machines
controller).
🪄 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: 54399345-1f94-4fc7-a4d2-6249bbd215a6
📒 Files selected for processing (28)
api/external/cinder/messages.goapi/external/ironcore/messages.goapi/external/manila/messages.goapi/external/nova/messages.goapi/external/pods/messages.goapi/scheduling/options.goapi/scheduling/options_test.goapi/v1alpha1/pipeline_types.gohelm/library/cortex/files/crds/cortex.cloud_pipelines.yamlinternal/scheduling/cinder/filter_weigher_pipeline_controller.gointernal/scheduling/cinder/filter_weigher_pipeline_controller_test.gointernal/scheduling/lib/filter_weigher_pipeline_request.gointernal/scheduling/lib/filter_weigher_pipeline_request_test.gointernal/scheduling/lib/filter_weigher_pipeline_test.gointernal/scheduling/machines/filter_weigher_pipeline_controller.gointernal/scheduling/machines/filter_weigher_pipeline_controller_test.gointernal/scheduling/manila/filter_weigher_pipeline_controller.gointernal/scheduling/manila/filter_weigher_pipeline_controller_test.gointernal/scheduling/nova/filter_weigher_pipeline_controller.gointernal/scheduling/nova/filter_weigher_pipeline_controller_test.gointernal/scheduling/nova/integration_test.gointernal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.gointernal/scheduling/pods/filter_weigher_pipeline_controller.gointernal/scheduling/pods/filter_weigher_pipeline_controller_test.gointernal/scheduling/reservations/commitments/reservation_controller.gointernal/scheduling/reservations/failover/integration_test.gointernal/scheduling/reservations/failover/reservation_scheduling.gointernal/scheduling/reservations/scheduler_client.go
💤 Files with no reviewable changes (2)
- api/v1alpha1/pipeline_types.go
- helm/library/cortex/files/crds/cortex.cloud_pipelines.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- api/external/ironcore/messages.go
- internal/scheduling/lib/filter_weigher_pipeline_request_test.go
- api/external/cinder/messages.go
- api/external/pods/messages.go
- internal/scheduling/lib/filter_weigher_pipeline_request.go
- api/external/manila/messages.go
- api/external/nova/messages.go
Comment on lines
+14
to
+21
| {"zero value is valid", Options{}, false}, | ||
| {"write run, history recorded by default", Options{}, false}, | ||
| {"write run with inflight", Options{CreateInflight: true}, false}, | ||
| {"read-only run, skipping history", Options{ReadOnly: true, SkipHistory: true}, false}, | ||
| {"ReadOnly without SkipHistory is invalid", Options{ReadOnly: true}, true}, | ||
| {"ReadOnly + CreateInflight is invalid", Options{ReadOnly: true, CreateInflight: true}, true}, | ||
| {"ReadOnly + both invalid", Options{ReadOnly: true, CreateInflight: true}, true}, | ||
| } |
There was a problem hiding this comment.
Duplicate test cases and the CreateInflight validation rule is never exercised.
Two separate problems:
-
Duplicate inputs: lines 14–15 are both
Options{}, and lines 19–20 are bothOptions{ReadOnly: true, CreateInflight: true}— neither pair adds coverage. -
Unexercised rule: in both lines 19 and 20,
SkipHistorydefaults tofalse, soValidate()returns early on the first check (ReadOnly && !SkipHistory). The second rule —ReadOnly && CreateInflight— is never actually reached. To test it, a case withSkipHistory: trueis needed so the first guard passes.
✏️ Suggested fix
- {"zero value is valid", Options{}, false},
- {"write run, history recorded by default", Options{}, false},
- {"write run with inflight", Options{CreateInflight: true}, false},
- {"read-only run, skipping history", Options{ReadOnly: true, SkipHistory: true}, false},
- {"ReadOnly without SkipHistory is invalid", Options{ReadOnly: true}, true},
- {"ReadOnly + CreateInflight is invalid", Options{ReadOnly: true, CreateInflight: true}, true},
- {"ReadOnly + both invalid", Options{ReadOnly: true, CreateInflight: true}, true},
+ {"zero value is valid", Options{}, false},
+ {"write run with inflight", Options{CreateInflight: true}, false},
+ {"read-only run, skipping history", Options{ReadOnly: true, SkipHistory: true}, false},
+ {"ReadOnly without SkipHistory is invalid", Options{ReadOnly: true}, true},
+ // Tests the second rule: CreateInflight is incompatible with ReadOnly even when SkipHistory is set.
+ {"ReadOnly + CreateInflight is invalid", Options{ReadOnly: true, SkipHistory: true, CreateInflight: true}, true},📝 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
| {"zero value is valid", Options{}, false}, | |
| {"write run, history recorded by default", Options{}, false}, | |
| {"write run with inflight", Options{CreateInflight: true}, false}, | |
| {"read-only run, skipping history", Options{ReadOnly: true, SkipHistory: true}, false}, | |
| {"ReadOnly without SkipHistory is invalid", Options{ReadOnly: true}, true}, | |
| {"ReadOnly + CreateInflight is invalid", Options{ReadOnly: true, CreateInflight: true}, true}, | |
| {"ReadOnly + both invalid", Options{ReadOnly: true, CreateInflight: true}, true}, | |
| } | |
| {"zero value is valid", Options{}, false}, | |
| {"write run with inflight", Options{CreateInflight: true}, false}, | |
| {"read-only run, skipping history", Options{ReadOnly: true, SkipHistory: true}, false}, | |
| {"ReadOnly without SkipHistory is invalid", Options{ReadOnly: true}, true}, | |
| // Tests the second rule: CreateInflight is incompatible with ReadOnly even when SkipHistory is set. | |
| {"ReadOnly + CreateInflight is invalid", Options{ReadOnly: true, SkipHistory: true, CreateInflight: true}, true}, |
🤖 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 `@api/scheduling/options_test.go` around lines 14 - 21, Remove the duplicate
test rows and add a case that actually exercises the CreateInflight validation:
update the table of test cases for Options so only one zero-value entry remains
(remove the duplicated Options{}), remove the duplicated Options{ReadOnly: true,
CreateInflight: true} row, and add a new test case using Options{ReadOnly: true,
CreateInflight: true, SkipHistory: true} so Validate() passes the first ReadOnly
&& !SkipHistory guard and exercises the ReadOnly && CreateInflight rule in
Options.Validate().
| return errors.New("read-only runs must not write scheduling history: set SkipHistory=true") | ||
| } | ||
| if o.ReadOnly && o.CreateInflight { | ||
| return errors.New("ReadOnly and CreateInflight are mutually exclusive: read-only runs must not create inflight reservations") |
There was a problem hiding this comment.
Error message starts with a capitalized field name, violating the lowercase error message guideline.
"ReadOnly and CreateInflight are mutually exclusive..." starts with the capitalized field name ReadOnly, which will be flagged by ST1005/errcheck linters. As per coding guidelines, error messages must always be lowercase.
✏️ Proposed fix
- return errors.New("ReadOnly and CreateInflight are mutually exclusive: read-only runs must not create inflight reservations")
+ return errors.New("readOnly and createInflight are mutually exclusive: read-only runs must not create inflight reservations")🤖 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 `@api/scheduling/options.go` at line 43, Change the error string to start with
a lowercase rune to satisfy ST1005: replace the current errors.New(...) message
that begins "ReadOnly and CreateInflight..." with a message that begins
lowercase (e.g., "readOnly and CreateInflight are mutually exclusive: read-only
runs must not create inflight reservations" or use hyphenated field names like
"read-only and create-inflight are mutually exclusive...") in the same return
statement so the error text in options.go uses a lowercase first letter.
| createHistory: true, | ||
| expectError: true, | ||
| expectHistoryCreated: true, | ||
| expectHistoryCreated: false, |
There was a problem hiding this comment.
Assert the no-history expectation on error paths.
expectHistoryCreated: false for the nil-raw case is not currently enforced, because the test only checks for zero History objects in the !tt.expectError branch. If ProcessNewDecisionFromAPI() accidentally creates history before returning the error, this case will still pass.
Also applies to: 434-450
🤖 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/cinder/filter_weigher_pipeline_controller_test.go` at
line 378, The test currently only asserts zero History when !tt.expectError, so
add an explicit assertion of history count == 0 whenever tt.expectHistoryCreated
is false (regardless of tt.expectError) after calling
ProcessNewDecisionFromAPI(); i.e. move or duplicate the expectHistoryCreated
check out of the !tt.expectError branch so the test fails if
ProcessNewDecisionFromAPI() creates History even when it returns an error. Apply
the same change for the other case group around the lines that correspond to the
second nil-raw check (the block covering the additional assertions referenced in
the comment).
Comment on lines
374
to
+376
| createHistory: true, | ||
| expectError: true, | ||
| expectHistoryCreated: true, | ||
| expectHistoryCreated: false, |
There was a problem hiding this comment.
The nil-raw case still doesn't verify history absence.
With expectHistoryCreated: false on an error case, this test will still pass if ProcessNewDecisionFromAPI() creates a History object before returning the error, because the harness only checks absence when !tt.expectError.
Also applies to: 428-444
🤖 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/manila/filter_weigher_pipeline_controller_test.go` around
lines 374 - 376, The test cases with expectHistoryCreated=false (including the
nil-raw case and the block around lines 428-444) only assert history absence
when !tt.expectError, so a History could be created before an error and the test
would still pass; update the test harness in
internal/scheduling/manila/filter_weigher_pipeline_controller_test.go to always
verify the History creation state against tt.expectHistoryCreated regardless of
tt.expectError by adding/asserting the lookup of the History object (or the
absence) after calling ProcessNewDecisionFromAPI() and comparing it to
tt.expectHistoryCreated; reference the test table entries that set
expectHistoryCreated and the call to ProcessNewDecisionFromAPI to locate where
to add the unconditional assertion.
Comment on lines
179
to
+182
| result, err := pipeline.Run(request) | ||
| if !request.Options.SkipHistory { | ||
| c.upsertHistory(ctx, decision, err) | ||
| } |
There was a problem hiding this comment.
Read-only requests can still write history under RLock().
The lock choice now depends on request.Options.ReadOnly, but history writes are only guarded by SkipHistory. A read-only caller that omits SkipHistory: true will still mutate History CRs while running on the shared-lock path.
Suggested guard
result, err := pipeline.Run(request)
- if !request.Options.SkipHistory {
+ if !request.Options.ReadOnly && !request.Options.SkipHistory {
c.upsertHistory(ctx, decision, err)
}📝 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
| result, err := pipeline.Run(request) | |
| if !request.Options.SkipHistory { | |
| c.upsertHistory(ctx, decision, err) | |
| } | |
| result, err := pipeline.Run(request) | |
| if !request.Options.ReadOnly && !request.Options.SkipHistory { | |
| c.upsertHistory(ctx, decision, err) | |
| } |
🤖 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
179 - 182, The code calls c.upsertHistory after pipeline.Run guarded only by
request.Options.SkipHistory but not by request.Options.ReadOnly, so a read-only
request can still mutate History; change the guard so history is only written
when both SkipHistory is false AND ReadOnly is false (i.e. if
!request.Options.SkipHistory && !request.Options.ReadOnly), and ensure the call
to c.upsertHistory happens on the write-lock path (the branch that acquired the
exclusive Lock, not the RLock) so history writes are performed only under the
write lock; update the logic around pipeline.Run / lock handling to move or
conditionalize the c.upsertHistory call accordingly.
Comment on lines
150
to
+156
| request := pods.PodPipelineRequest{Nodes: nodes.Items, Pod: *pod} | ||
| result, err := pipeline.Run(request) | ||
| if !request.Options.SkipHistory { | ||
| if upsertErr := c.HistoryManager.CreateOrUpdateHistory(ctx, decision, nil, err); upsertErr != nil { | ||
| log.Error(upsertErr, "failed to create/update history") | ||
| } | ||
| } |
There was a problem hiding this comment.
SkipHistory is never populated for pod scheduling.
PodPipelineRequest is constructed with zero-value options here, so this branch always creates/updates history. That drops the previous PipelineSpec.CreateHistory behavior for pods and turns history writes into an unconditional side effect.
Suggested minimal fallback until pod requests carry explicit options
request := pods.PodPipelineRequest{Nodes: nodes.Items, Pod: *pod}
+ if pipelineConf, ok := c.PipelineConfigs[decision.Spec.PipelineRef.Name]; ok {
+ request.Options.SkipHistory = !pipelineConf.Spec.CreateHistory
+ }
result, err := pipeline.Run(request)
if !request.Options.SkipHistory {
if upsertErr := c.HistoryManager.CreateOrUpdateHistory(ctx, decision, nil, err); upsertErr != nil {
log.Error(upsertErr, "failed to create/update history")
}📝 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
| request := pods.PodPipelineRequest{Nodes: nodes.Items, Pod: *pod} | |
| result, err := pipeline.Run(request) | |
| if !request.Options.SkipHistory { | |
| if upsertErr := c.HistoryManager.CreateOrUpdateHistory(ctx, decision, nil, err); upsertErr != nil { | |
| log.Error(upsertErr, "failed to create/update history") | |
| } | |
| } | |
| request := pods.PodPipelineRequest{Nodes: nodes.Items, Pod: *pod} | |
| if pipelineConf, ok := c.PipelineConfigs[decision.Spec.PipelineRef.Name]; ok { | |
| request.Options.SkipHistory = !pipelineConf.Spec.CreateHistory | |
| } | |
| result, err := pipeline.Run(request) | |
| if !request.Options.SkipHistory { | |
| if upsertErr := c.HistoryManager.CreateOrUpdateHistory(ctx, decision, nil, err); upsertErr != nil { | |
| log.Error(upsertErr, "failed to create/update history") | |
| } | |
| } |
🤖 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/pods/filter_weigher_pipeline_controller.go` around lines
150 - 156, PodPipelineRequest is built with zero-value Options so SkipHistory is
never set; before calling pipeline.Run(request) populate
request.Options.SkipHistory using the existing pipeline spec fallback: set
request.Options.SkipHistory = !decision.PipelineSpec.CreateHistory (with a
nil-safe check so if decision or decision.PipelineSpec is nil you default to
CreateHistory=true, i.e. SkipHistory=false). This preserves the previous
PipelineSpec.CreateHistory behavior for pod scheduling and prevents
unconditional history writes while we add explicit options to pod requests.
Contributor
Test Coverage ReportTest Coverage 📊: 69.1% |
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