Skip to content
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.

Sign up for GitHub

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

Jump to bottom

scheduler: allow configuring default preemption for system scheduler #6935

Merged
merged 2 commits into from
Jan 28, 2020
Merged

scheduler: allow configuring default preemption for system scheduler #6935

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
744c9a4
scheduler: allow configuring default preemption for system scheduler
Jan 12, 2020
31025d6
Support customizing full scheduler config
Jan 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions command/agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,13 @@ func convertServerConfig(agentConfig *Config) (*nomad.Config, error) {
return nil, fmt.Errorf("server_service_name must be set when auto_advertise is enabled")
}

// handle system scheduler preemption default
if agentConfig.Server.SystemSchedulerPreemptionEnabledDefault != nil {
conf.SystemSchedulerPreemptionEnabledDefault = *agentConfig.Server.SystemSchedulerPreemptionEnabledDefault
} else {
conf.SystemSchedulerPreemptionEnabledDefault = true
}

// Add the Consul and Vault configs
conf.ConsulConfig = agentConfig.Consul
conf.VaultConfig = agentConfig.Vault
Expand Down
29 changes: 29 additions & 0 deletions command/agent/agent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package agent

import (
"fmt"
"io/ioutil"
"os"
"path/filepath"
Expand Down Expand Up @@ -274,6 +275,34 @@ func TestAgent_ServerConfig(t *testing.T) {
}
}

func TestAgent_ServerConfig_SchedulerFlags(t *testing.T) {
cases := []struct {
input *bool
expected bool
}{
{nil, true},
{helper.BoolToPtr(false), false},
{helper.BoolToPtr(true), true},
}

for _, c := range cases {
t.Run(fmt.Sprintf("case: %v", c.input), func(t *testing.T) {
conf := DefaultConfig()
conf.Server.SystemSchedulerPreemptionEnabledDefault = c.input

a := &Agent{config: conf}
conf.AdvertiseAddrs.Serf = "127.0.0.1:4000"
conf.AdvertiseAddrs.RPC = "127.0.0.1:4001"
conf.AdvertiseAddrs.HTTP = "10.10.11.1:4005"
conf.ACL.Enabled = true
require.NoError(t, conf.normalizeAddrs())

out, err := a.serverConfig()
require.NoError(t, err)
require.Equal(t, c.expected, out.SystemSchedulerPreemptionEnabledDefault)
})
}
}
func TestAgent_ClientConfig(t *testing.T) {
t.Parallel()
conf := DefaultConfig()
Expand Down
3 changes: 3 additions & 0 deletions command/agent/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,9 @@ type ServerConfig struct {
// ServerJoin contains information that is used to attempt to join servers
ServerJoin *ServerJoin `hcl:"server_join"`

// SystemSchedulerPreemptionEnabledDefault is used to determin whether to enable system preemption by default in a new cluster
Copy link
Contributor

@preetapan preetapan Jan 13, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/determin/determine here and everywhere else below

SystemSchedulerPreemptionEnabledDefault *bool `hcl:"system_scheduler_preemption_enabled_default"`
Copy link
Member

@schmichael schmichael Jan 14, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we added a Defaults struct for this field so we can reuse it for all API configured settings? I think we should provide file-configurable-defaults for all scheduler configuration parameters to better support Packer & TF users who don't want to have to run some commands to tweak their cluster post deployment.

Then hcl files baked into golden images would look something like:

server {
  enabled = true

 # ... a bunch of other stuff ...

  defaults {
    system_scheduler_preemption = false
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, just noticed your comment above saying almost the exact same thing @notnoop. 😅

Another naming idea based off of yours:

server {
  enabled = true

 # ... a bunch of other stuff ...

  scheduler_defaults {
    system_job_preemption = false
  }
}

We can drop _enabled as other bool fields don't have it. Otherwise I think some sort of qualifier is necessary for "system" as shortening it to system_preemption may not be immediately obvious to users that its referring to system jobs since the word "system" is quite generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree: a generic defaults make sense. Though, I would prefer to use the same json format or key names that the API accepts. Given that the API is the canonical way for configuration, I opted to use the same API terms as-is. Making them hcl-friendly risks hcl format being behind/divergent from the api format and complicates documentation/implementation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, we should probably follow the jobspec's lead here:

  1. The style should vary by format (underscores for HCL, Caps for JSON)
  2. Otherwise the names should match


// ExtraKeysHCL is used by hcl to surface unexpected keys
ExtraKeysHCL []string `hcl:",unusedKeys" json:"-"`
}
Expand Down
8 changes: 6 additions & 2 deletions nomad/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,9 @@ type Config struct {
// dead servers.
AutopilotInterval time.Duration

// SystemSchedulerPreemptionEnabledDefault is used to determin whether to enable system preemption by default in a new cluster
SystemSchedulerPreemptionEnabledDefault bool

// PluginLoader is used to load plugins.
PluginLoader loader.PluginCatalog

Expand Down Expand Up @@ -377,8 +380,9 @@ func DefaultConfig() *Config {
MaxTrailingLogs: 250,
ServerStabilizationTime: 10 * time.Second,
},
ServerHealthInterval: 2 * time.Second,
AutopilotInterval: 10 * time.Second,
ServerHealthInterval: 2 * time.Second,
AutopilotInterval: 10 * time.Second,
SystemSchedulerPreemptionEnabledDefault: true,
}

// Enable all known schedulers by default
Expand Down
16 changes: 9 additions & 7 deletions nomad/leader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,14 @@ var minAutopilotVersion = version.Must(version.NewVersion("0.8.0"))
var minSchedulerConfigVersion = version.Must(version.NewVersion("0.9.0"))

// Default configuration for scheduler with preemption enabled for system jobs
var defaultSchedulerConfig = &structs.SchedulerConfiguration{
PreemptionConfig: structs.PreemptionConfig{
SystemSchedulerEnabled: true,
BatchSchedulerEnabled: false,
ServiceSchedulerEnabled: false,
},
func (s *Server) defaultSchedulerConfig() structs.SchedulerConfiguration {
return structs.SchedulerConfiguration{
PreemptionConfig: structs.PreemptionConfig{
SystemSchedulerEnabled: s.config.SystemSchedulerPreemptionEnabledDefault,
BatchSchedulerEnabled: false,
ServiceSchedulerEnabled: false,
},
}
}

// monitorLeadership is used to monitor if we acquire or lose our role
Expand Down Expand Up @@ -1319,7 +1321,7 @@ func (s *Server) getOrCreateSchedulerConfig() *structs.SchedulerConfiguration {
return nil
}

req := structs.SchedulerSetConfigRequest{Config: *defaultSchedulerConfig}
req := structs.SchedulerSetConfigRequest{Config: s.defaultSchedulerConfig()}
if _, _, err = s.raftApply(structs.SchedulerConfigRequestType, req); err != nil {
s.logger.Named("core").Error("failed to initialize config", "error", err)
return nil
Expand Down