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.

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

Make number of scheduler workers reloadable #11593

Merged
merged 32 commits into from
Jan 6, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
1908187
Working POC
angrycub Nov 20, 2021
0071e55
Unexport setupNewWorkers; improve comments
angrycub Nov 23, 2021
763671a
Added some VSCode codetours
angrycub Nov 23, 2021
339316a
Update shutdown to use context
angrycub Nov 30, 2021
1a985b3
Apply suggestions from code review
angrycub Dec 1, 2021
22f93b7
Implement GET for SchedulerWorker API + tests
angrycub Dec 1, 2021
16f9dd4
Merge branch 'f-reload-num-schedulers' of github.com:hashicorp/nomad …
angrycub Dec 1, 2021
48428e7
Wired API, refactors, more testing
angrycub Dec 3, 2021
1258128
Merge branch 'main' into f-reload-num-schedulers
angrycub Dec 6, 2021
1845577
Fix linter complaints
angrycub Dec 6, 2021
9c4e5c4
Updating worker to cache EnabledScheduler list
angrycub Dec 6, 2021
0d8b7ec
Refactor `unsafe...` func names to `...Locked`
angrycub Dec 8, 2021
f5bb227
Passing enabled schedulers list to worker
angrycub Dec 10, 2021
292518b
Add note about scheduler death
angrycub Dec 10, 2021
1337f04
Worker API refactor
angrycub Dec 10, 2021
bd345e0
Made handler methods public for OpenAPI, remove unused test bool
angrycub Dec 10, 2021
31687cd
Implement SchedulerWorker status part 1
angrycub Dec 10, 2021
3739987
Fix broken Pause logic; split WorkloadWaiting status
angrycub Dec 11, 2021
7fe5949
Added scheduler info api
angrycub Dec 11, 2021
60d53fa
Added worker info api to api package
angrycub Dec 11, 2021
3d755aa
bugfixes
angrycub Dec 11, 2021
4ee6b8c
Adding stringer to build deps
angrycub Dec 13, 2021
71dab36
Changing route to /v1/agent/schedulers
angrycub Dec 20, 2021
1dc9f96
Adding docs for scheduler worker api
angrycub Dec 21, 2021
0417332
Adding API test for bad worker info
angrycub Dec 22, 2021
420a158
Add changelog message
angrycub Dec 23, 2021
fd016de
typo in changelog 🤦
angrycub Dec 23, 2021
167c6a3
Incorporate API code review feedback
angrycub Jan 3, 2022
f4f610b
Incorporate api-docs feedback
angrycub Jan 4, 2022
689fa77
Updates to worker/leader code from code review
angrycub Jan 4, 2022
982c397
Fix test response type
angrycub Jan 5, 2022
7581957
Set both statuses in markStopped so they are atomic
angrycub Jan 6, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions .tours/scheduler-worker---hot-reload.tour
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
{
"$schema": "https://aka.ms/codetour-schema",
"title": "Scheduler Worker - Hot Reload",
"steps": [
{
"file": "nomad/server.go",
"description": "## Server.Reload()\n\nServer configuration reloads start here.",
"line": 782,
"selection": {
"start": {
"line": 780,
"character": 4
},
"end": {
"line": 780,
"character": 10
}
}
},
{
"file": "nomad/server.go",
"description": "## Did NumSchedulers change?\nIf the number of schedulers has changed between the running configuration and the new one we need to adopt that change in realtime.",
"line": 812
},
{
"file": "nomad/server.go",
"description": "## Server.setupNewWorkers()\n\nsetupNewWorkers performs three tasks:\n\n- makes a copy of the existing worker pointers\n\n- creates a fresh array and loads a new set of workers into them\n\n- iterates through the \"old\" workers and shuts them down in individual\n goroutines for maximum parallelism",
"line": 1482,
"selection": {
"start": {
"line": 1480,
"character": 4
},
"end": {
"line": 1480,
"character": 12
}
}
},
{
"file": "nomad/server.go",
"description": "Once all of the work in setupNewWorkers is complete, we stop the old ones.",
"line": 1485
},
{
"file": "nomad/server.go",
"description": "The `stopOldWorkers` function iterates through the array of workers and calls their `Shutdown` method\nas a goroutine to prevent blocking.",
"line": 1505
},
{
"file": "nomad/worker.go",
"description": "The `Shutdown` method sets `w.stop` to true signaling that we intend for the `Worker` to stop the next time we consult it. We also manually unpause the `Worker` by setting w.paused to false and sending a `Broadcast()` via the cond.",
"line": 110
}
],
"ref": "f-reload-num-schedulers"
}
66 changes: 66 additions & 0 deletions .tours/scheduler-worker---pause.tour
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
{
"$schema": "https://aka.ms/codetour-schema",
"title": "Scheduler Worker - Pause",
"steps": [
{
"file": "nomad/leader.go",
"description": "## Server.establishLeadership()\n\nUpon becoming a leader, the server pauses a subset of the workers to allow for the additional burden of the leader's goroutines. The `handlePausableWorkers` function takes a boolean that states whether or not the current node is a leader or not. Because we are in `establishLeadership` we use `true` rather than calling `s.IsLeader()`",
"line": 233,
"selection": {
"start": {
"line": 233,
"character": 4
},
"end": {
"line": 233,
"character": 12
}
}
},
{
"file": "nomad/leader.go",
"description": "## Server.handlePausableWorkers()\n\nhandlePausableWorkers ranges over a slice of Workers and manipulates their paused state by calling their `SetPause` method.",
"line": 443,
"selection": {
"start": {
"line": 443,
"character": 18
},
"end": {
"line": 443,
"character": 26
}
}
},
{
"file": "nomad/leader.go",
"description": "## Server.pausableWorkers()\n\nThe pausableWorkers function provides a consistent slice of workers that the server can pause and unpause. Since the Worker array is never mutated, the same slice is returned by pausableWorkers on every invocation.\nThis comment is interesting/potentially confusing\n\n```golang\n // Disabling 3/4 of the workers frees CPU for raft and the\n\t// plan applier which uses 1/2 the cores.\n``` \n\nHowever, the key point is that it will return a slice containg 3/4th of the workers.",
"line": 1100,
"selection": {
"start": {
"line": 1104,
"character": 1
},
"end": {
"line": 1105,
"character": 43
}
}
},
{
"file": "nomad/worker.go",
"description": "## Worker.SetPause()\n\nThe `SetPause` function is used to signal an intention to pause the worker. Because the worker's work is happening in the `run()` goroutine, pauses happen asynchronously.",
"line": 91
},
{
"file": "nomad/worker.go",
"description": "## Worker.dequeueEvaluation()\n\nCalls checkPaused, which will be the function we wait in if the scheduler is set to be paused. \n\n> **NOTE:** This is called here rather than in run() because this function loops in case of an error fetching a evaluation.",
"line": 206
},
{
"file": "nomad/worker.go",
"description": "## Worker.checkPaused()\n\nWhen `w.paused` is `true`, we call the `Wait()` function on the condition. Execution of this goroutine will stop here until it receives a `Broadcast()` or a `Signal()`. At this point, the `Worker` is paused.",
"line": 104
}
]
}
51 changes: 51 additions & 0 deletions .tours/scheduler-worker---unpause.tour
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
{
"$schema": "https://aka.ms/codetour-schema",
"title": "Scheduler Worker - Unpause",
"steps": [
{
"file": "nomad/leader.go",
"description": "## revokeLeadership()\n\nAs a server transistions from leader to non-leader, the pausableWorkers are resumed since the other leader goroutines are stopped providing extra capacity.",
"line": 1040,
"selection": {
"start": {
"line": 1038,
"character": 10
},
"end": {
"line": 1038,
"character": 20
}
}
},
{
"file": "nomad/leader.go",
"description": "## handlePausableWorkers()\n\nThe handlePausableWorkers method is called with `false`. We fetch the pausableWorkers and call their SetPause method with `false`.\n",
"line": 443,
"selection": {
"start": {
"line": 443,
"character": 18
},
"end": {
"line": 443,
"character": 27
}
}
},
{
"file": "nomad/worker.go",
"description": "## Worker.SetPause()\n\nDuring unpause, p is false. We update w.paused in the mutex, and then call Broadcast on the cond. This wakes the goroutine sitting in the Wait() inside of `checkPaused()`",
"line": 91
},
{
"file": "nomad/worker.go",
"description": "## Worker.checkPaused()\n\nOnce the goroutine receives the `Broadcast()` message from `SetPause()`, execution continues here. Now that `w.paused == false`, we exit the loop and return to the caller (the `dequeueEvaluation()` function).",
"line": 104
},
{
"file": "nomad/worker.go",
"description": "## Worker.dequeueEvaluation\n\nWe return back into dequeueEvaluation after the call to checkPaused. At this point the worker will either stop (if that signal boolean has been set) or continue looping after returning to run().",
"line": 207
}
]
}
36 changes: 36 additions & 0 deletions .tours/scheduler-worker.tour
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
{
"$schema": "https://aka.ms/codetour-schema",
"title": "Scheduler Worker - Start",
"steps": [
{
"file": "nomad/server.go",
"description": "## Server.NewServer()\n\nScheduler workers are started as the agent starts the `server` go routines.",
"line": 402
},
{
"file": "nomad/server.go",
"description": "## Server.setupWorkers()\n\nThe `setupWorkers()` function validates that there are enabled Schedulers by type and count. It then creates s.config.NumSchedulers by calling `NewWorker()`\n\nThe `_core` scheduler _**must**_ be enabled. **TODO: why?**\n",
"line": 1443,
"selection": {
"start": {
"line": 1442,
"character": 4
},
"end": {
"line": 1442,
"character": 12
}
}
},
{
"file": "nomad/worker.go",
"description": "## Worker.NewWorker\n\nNewWorker creates the Worker and starts `run()` in a goroutine.",
"line": 78
},
{
"file": "nomad/worker.go",
"description": "## Worker.run()\n\nThe `run()` function runs in a loop until it's paused, it's stopped, or the server indicates that it is shutting down. All of the work the `Worker` performs should be\nimplemented in or called from here.\n",
"line": 152
}
]
}
43 changes: 43 additions & 0 deletions api/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -483,3 +483,46 @@ type HostDataResponse struct {
AgentID string
HostData *HostData `json:",omitempty"`
}

// GetSchedulerWorkerConfig returns the targeted agent's worker pool configuration
func (a *Agent) GetSchedulerWorkerConfig() (*SchedulerWorkerPoolArgs, error) {
var resp AgentSchedulerWorkerConfigResponse
_, err := a.client.query("/v1/agent/workers", &resp, nil)
if err != nil {
return nil, err
}

return &SchedulerWorkerPoolArgs{NumSchedulers: resp.NumSchedulers, EnabledSchedulers: resp.EnabledSchedulers}, nil
}

// SetSchedulerWorkerConfig attempts to update the targeted agent's worker pool configuration
func (a *Agent) SetSchedulerWorkerConfig(args SchedulerWorkerPoolArgs) (*SchedulerWorkerPoolArgs, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we need WriteOptions here (and QueryOptions for GetSchedulerConfig above) to support ACLs and any HTTP params we might want in the future. And we always seem to want it eventually so that way we don't have to make a SetSchedulerWorkerConfigWithOptions later on.

We can probably get away without having a QueryMeta returned here because everything in QueryMeta is used for comes out of raft? None of the other agent APIs in this file have it.

req := AgentSchedulerWorkerConfigRequest{
NumSchedulers: args.NumSchedulers,
EnabledSchedulers: args.EnabledSchedulers,
}

var resp AgentSchedulerWorkerConfigResponse
_, err := a.client.write("/v1/agent/workers", &req, &resp, nil)
if err != nil {
return nil, err
}

return &SchedulerWorkerPoolArgs{NumSchedulers: resp.NumSchedulers, EnabledSchedulers: resp.EnabledSchedulers}, nil
}

type SchedulerWorkerPoolArgs struct {
NumSchedulers int
EnabledSchedulers []string
}

// AgentSchedulerWorkerConfig
angrycub marked this conversation as resolved.
Show resolved Hide resolved
type AgentSchedulerWorkerConfigRequest struct {
NumSchedulers int `json:"num_schedulers"`
EnabledSchedulers []string `json:"enabled_schedulers"`
}

type AgentSchedulerWorkerConfigResponse struct {
NumSchedulers int `json:"num_schedulers"`
EnabledSchedulers []string `json:"enabled_schedulers"`
}
15 changes: 15 additions & 0 deletions api/agent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -456,3 +456,18 @@ func TestAgentProfile(t *testing.T) {
require.Nil(t, resp)
}
}

func TestAgent_SchedulerWorkerConfig(t *testing.T) {
t.Parallel()

c, s := makeClient(t, nil, nil)
defer s.Stop()
a := c.Agent()

config, err := a.GetSchedulerWorkerConfig()
require.Nil(t, err)
newConfig := SchedulerWorkerPoolArgs{NumSchedulers: 0, EnabledSchedulers: []string{"_core", "system"}}
resp, err := a.SetSchedulerWorkerConfig(newConfig)
require.NoError(t, err)
assert.NotEqual(t, config, resp)
}
88 changes: 87 additions & 1 deletion command/agent/agent_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
cstructs "github.com/hashicorp/nomad/client/structs"
"github.com/hashicorp/nomad/command/agent/host"
"github.com/hashicorp/nomad/command/agent/pprof"
"github.com/hashicorp/nomad/nomad"
"github.com/hashicorp/nomad/nomad/structs"
"github.com/hashicorp/serf/serf"
"github.com/mitchellh/copystructure"
Expand Down Expand Up @@ -362,7 +363,7 @@ func (s *HTTPServer) agentPprof(reqType pprof.ReqType, resp http.ResponseWriter,

// Parse query param int values
// Errors are dropped here and default to their zero values.
// This is to mimick the functionality that net/pprof implements.
// This is to mimic the functionality that net/pprof implements.
seconds, _ := strconv.Atoi(req.URL.Query().Get("seconds"))
debug, _ := strconv.Atoi(req.URL.Query().Get("debug"))
gc, _ := strconv.Atoi(req.URL.Query().Get("gc"))
Expand Down Expand Up @@ -740,3 +741,88 @@ func (s *HTTPServer) AgentHostRequest(resp http.ResponseWriter, req *http.Reques

return reply, rpcErr
}

// AgentSchedulerWorkerConfigRequest is used to query the count (and state eventually)
// of the scheduler workers running in a Nomad server agent.
// This endpoint can also be used to update the count of running workers for a
// given agent.
func (s *HTTPServer) AgentSchedulerWorkerConfigRequest(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
if s.agent.Server() == nil {
return nil, CodedError(http.StatusBadRequest, "server only endpoint")
}
switch req.Method {
case "PUT", "POST":
return s.updateScheduleWorkersConfig(resp, req)
case "GET":
return s.getScheduleWorkersConfig(resp, req)
default:
return nil, CodedError(http.StatusMethodNotAllowed, ErrInvalidMethod)
}
}

func (s *HTTPServer) getScheduleWorkersConfig(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
srv := s.agent.Server()
if srv == nil {
return nil, CodedError(http.StatusBadRequest, "server only endpoint")
}

var secret string
s.parseToken(req, &secret)

// Check agent read permissions
if aclObj, err := s.agent.Server().ResolveToken(secret); err != nil {
return nil, CodedError(http.StatusInternalServerError, err.Error())
} else if aclObj != nil && !aclObj.AllowAgentRead() {
return nil, CodedError(http.StatusForbidden, structs.ErrPermissionDenied.Error())
}

config := srv.GetSchedulerWorkerConfig()
response := &agentSchedulerWorkerConfig{
NumSchedulers: config.NumSchedulers,
EnabledSchedulers: config.EnabledSchedulers,
}
return response, nil
}

func (s *HTTPServer) updateScheduleWorkersConfig(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
srv := s.agent.Server()
if srv == nil {
return nil, CodedError(400, "server only endpoint")
}

var secret string
s.parseToken(req, &secret)

// Check agent write permissions
if aclObj, err := srv.ResolveToken(secret); err != nil {
return nil, CodedError(http.StatusInternalServerError, err.Error())
} else if aclObj != nil && !aclObj.AllowAgentWrite() {
return nil, CodedError(http.StatusForbidden, structs.ErrPermissionDenied.Error())
}

var args agentSchedulerWorkerConfig

if err := decodeBody(req, &args); err != nil {
return nil, CodedError(http.StatusBadRequest, err.Error())
}
newArgs := nomad.SchedulerWorkerPoolArgs{
NumSchedulers: args.NumSchedulers,
EnabledSchedulers: args.EnabledSchedulers,
}
if newArgs.IsInvalid() {
return nil, CodedError(http.StatusBadRequest, "invalid arguments")
}
reply := srv.SetSchedulerWorkerConfig(newArgs)

response := &agentSchedulerWorkerConfig{
NumSchedulers: reply.NumSchedulers,
EnabledSchedulers: reply.EnabledSchedulers,
}

return response, nil
}

type agentSchedulerWorkerConfig struct {
NumSchedulers int `json:"num_schedulers"`
EnabledSchedulers []string `json:"enabled_schedulers"`
}
Loading