-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Scheduler can recieve its policy configuration from a ConfigMap #43892
Changes from all commits
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 |
---|---|---|
|
@@ -63,7 +63,10 @@ func (s *SchedulerServer) AddFlags(fs *pflag.FlagSet) { | |
fs.Int32Var(&s.Port, "port", s.Port, "The port that the scheduler's http service runs on") | ||
fs.StringVar(&s.Address, "address", s.Address, "The IP address to serve on (set to 0.0.0.0 for all interfaces)") | ||
fs.StringVar(&s.AlgorithmProvider, "algorithm-provider", s.AlgorithmProvider, "The scheduling algorithm provider to use, one of: "+factory.ListAlgorithmProviders()) | ||
fs.StringVar(&s.PolicyConfigFile, "policy-config-file", s.PolicyConfigFile, "File with scheduler policy configuration") | ||
fs.StringVar(&s.PolicyConfigFile, "policy-config-file", s.PolicyConfigFile, "File with scheduler policy configuration. This file is used if policy ConfigMap is not provided or --use-legacy-policy-config==true") | ||
fs.StringVar(&s.PolicyConfigMapName, "policy-configmap", s.PolicyConfigMapName, "Name of the ConfigMap object that contains scheduler's policy configuration. It must exist in the system namespace before scheduler initialization if --use-legacy-policy-config==false") | ||
fs.StringVar(&s.PolicyConfigMapNamespace, "policy-configmap-namespace", s.PolicyConfigMapNamespace, "The namespace where policy ConfigMap is located. The system namespace will be used if this is not provided or is empty.") | ||
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. The default value should be "kube-system" 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. Wait, we do not need that if we have that set in the defaults.go @k82cn 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. When the 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. En, it's set in Anyway, not a block comments. |
||
fs.BoolVar(&s.UseLegacyPolicyConfig, "use-legacy-policy-config", false, "When set to true, scheduler will ignore policy ConfigMap and uses policy config file") | ||
fs.BoolVar(&s.EnableProfiling, "profiling", true, "Enable profiling via web interface host:port/debug/pprof/") | ||
fs.BoolVar(&s.EnableContentionProfiling, "contention-profiling", false, "Enable lock contention profiling, if profiling is enabled") | ||
fs.StringVar(&s.Master, "master", s.Master, "The address of the Kubernetes API server (overrides any value in kubeconfig)") | ||
|
@@ -80,6 +83,5 @@ func (s *SchedulerServer) AddFlags(fs *pflag.FlagSet) { | |
fs.StringVar(&s.FailureDomains, "failure-domains", api.DefaultFailureDomains, "Indicate the \"all topologies\" set for an empty topologyKey when it's used for PreferredDuringScheduling pod anti-affinity.") | ||
fs.MarkDeprecated("failure-domains", "Doesn't have any effect. Will be removed in future version.") | ||
leaderelection.BindFlags(&s.LeaderElection, fs) | ||
|
||
utilfeature.DefaultFeatureGate.AddFlag(fs) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,4 +41,10 @@ type SchedulerExtender interface { | |
// onto machines. | ||
type ScheduleAlgorithm interface { | ||
Schedule(*v1.Pod, NodeLister) (selectedMachine string, err error) | ||
// Predicates() returns a pointer to a map of predicate functions. This is | ||
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. d/a pointer to/ |
||
// exposed for testing. | ||
Predicates() map[string]FitPredicate | ||
// Prioritizers returns a slice of priority config. This is exposed for | ||
// testing. | ||
Prioritizers() []PriorityConfig | ||
} |
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.
seems brittle.
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.
Why do you think it is brittle? When one imports an existing policy from a config file to a ConfigMap, the
Data
section of the map will get only one element. If there are more than one elements in the map, we won't know which one to use.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.
If a person makes a honest mistake it doesn't make a best effort to interpret and log a warning. I won't block on this, but it's worth noting.
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.
The fact that we don't make a best effort is intentional. IMO it is better to fail to initialize the scheduler and let the user know that their config has errors than to initialize with an unintended config and surprise the user with the behavior. The latter would be harder to debug. You know how many warnings are thrown in each module and how effective they are in helping debug large systems.
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.
I kind of wrote my second comment in haste, I completely agree with failing fast.
I think my initial aversion here is that this almost seems like a logic portion of a obj.deserialization f(n), which should have it's own checks in place.
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.
the scheduler should either look for a fixed key or be able to be told which key to use
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.
@liggitt When an existing policy file is imported by
kubectl
, the key will be the name of the file and the content of the file will go in the value. I wanted to make it easy for customers to import their existing configs and use it. If more keys are needed in the config, they can be added to the content which is the "value" of the first element.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.
accepting a single arbitrary key is confusing... kubectl can set the key when importing an existing config file (
--from-file=config.json=/path/to/my-scheduler-config.yaml
), and we should direct the user to do that.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.
for example, if I have a working scheduler pointing at a config map, I would not expect adding a second key to break the scheduler
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.
I see. I didn't know that kubectl can set the key. I will send a PR to remove the restriction on having only one element in the map and will add the logic to look for a specific key.