-
Notifications
You must be signed in to change notification settings - Fork 6
feat: adding pipeline options #799
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
36b7d94
ad66112
4a7bc9e
d298e75
54639d1
c2d0fc2
4de31f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import ( | |
|
|
||
| type FilterWeigherPipeline[RequestType FilterWeigherPipelineRequest] interface { | ||
| // Run the scheduling pipeline with the given request. | ||
| // Call-time options are read from request.GetOptions(). | ||
| Run(request RequestType) (v1alpha1.DecisionResult, error) | ||
| } | ||
|
|
||
|
|
@@ -263,6 +264,10 @@ func (s *filterWeigherPipeline[RequestType]) sortHostsByWeights(weights map[stri | |
|
|
||
| // Evaluate the pipeline and return a list of hosts in order of preference. | ||
| func (p *filterWeigherPipeline[RequestType]) Run(request RequestType) (v1alpha1.DecisionResult, error) { | ||
| opts := request.GetOptions() | ||
| if err := opts.Validate(); err != nil { | ||
| return v1alpha1.DecisionResult{}, err | ||
| } | ||
| slogArgs := request.GetTraceLogArgs() | ||
| slogArgsAny := make([]any, 0, len(slogArgs)) | ||
| for _, arg := range slogArgs { | ||
|
|
@@ -297,6 +302,17 @@ func (p *filterWeigherPipeline[RequestType]) Run(request RequestType) (v1alpha1. | |
| hosts := p.sortHostsByWeights(outWeights) | ||
| traceLog.Info("scheduler: sorted hosts", "hosts", hosts) | ||
|
|
||
| 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. | ||
| for host := range outWeights { | ||
| if !slices.Contains(hosts, host) { | ||
| delete(outWeights, host) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please provide logging here so we see what's going on. |
||
| // Collect some metrics about the pipeline execution. | ||
| go p.monitor.observePipelineResult(request, hosts) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,49 @@ | ||||||||||||||||||||||||||
| // Copyright SAP SE | ||||||||||||||||||||||||||
| // SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| package lib | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||
| "errors" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| "github.com/cobaltcore-dev/cortex/api/v1alpha1" | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Options configure the behavior of a single pipeline run at call time. | ||||||||||||||||||||||||||
| // These are distinct from per-step YAML options (FilterWeigherPipelineStepOpts), | ||||||||||||||||||||||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be more generic such as |
||||||||||||||||||||||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we make this a substruct such as |
||||||||||||||||||||||||||
| // MaxCandidates limits the number of hosts returned after weighing. 0 means no limit. | ||||||||||||||||||||||||||
| MaxCandidates int | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // RecordHistory records the placement decision in placement history. | ||||||||||||||||||||||||||
| // Replaces pipeline.Spec.CreateHistory once pipelines consolidate. | ||||||||||||||||||||||||||
| RecordHistory bool | ||||||||||||||||||||||||||
| // CreateInflight creates pessimistic blocking reservations for all returned candidates. | ||||||||||||||||||||||||||
| CreateInflight bool | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Validate checks for mutually exclusive or inconsistent option combinations. | ||||||||||||||||||||||||||
| func (o Options) Validate() error { | ||||||||||||||||||||||||||
| if o.ReadOnly && o.RecordHistory { | ||||||||||||||||||||||||||
| return errors.New("ReadOnly and RecordHistory are mutually exclusive: read-only runs must not write scheduling history") | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| if o.ReadOnly && o.CreateInflight { | ||||||||||||||||||||||||||
| return errors.New("ReadOnly and CreateInflight are mutually exclusive: read-only runs must not create inflight reservations") | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+42
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| // Copyright SAP SE | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package lib | ||
|
|
||
| import "testing" | ||
|
|
||
| func TestOptions_Validate(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| opts Options | ||
| wantErr bool | ||
| }{ | ||
| {"zero value is valid", Options{}, false}, | ||
| {"write run with history", Options{RecordHistory: true}, false}, | ||
| {"write run with inflight", Options{CreateInflight: true}, false}, | ||
| {"read-only run, no side effects", Options{ReadOnly: true}, false}, | ||
| {"ReadOnly + RecordHistory is invalid", Options{ReadOnly: true, RecordHistory: true}, true}, | ||
| {"ReadOnly + CreateInflight is invalid", Options{ReadOnly: true, CreateInflight: true}, true}, | ||
| {"ReadOnly + both invalid", Options{ReadOnly: true, RecordHistory: true, CreateInflight: true}, true}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := tt.opts.Validate() | ||
| if tt.wantErr && err == nil { | ||
| t.Error("expected error, got nil") | ||
| } | ||
| if !tt.wantErr && err != nil { | ||
| t.Errorf("expected no error, got %v", err) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: cobaltcore-dev/cortex
Length of output: 50378
🏁 Script executed:
Repository: cobaltcore-dev/cortex
Length of output: 4457
🏁 Script executed:
Repository: cobaltcore-dev/cortex
Length of output: 2785
🏁 Script executed:
Repository: cobaltcore-dev/cortex
Length of output: 11670
🏁 Script executed:
Repository: cobaltcore-dev/cortex
Length of output: 5767
🏁 Script executed:
Repository: 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), onlyRecordHistoryis conditionally set from pipeline config. OtherOptionsfields (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
Optionsshould be reset or validated to prevent caller-supplied flags likeAssumeEmptyHosts: trueorMaxCandidates: 1from bypassing capacity checks or placement logic.🤖 Prompt for AI Agents