From 5a2449d236647a5c12e73d18e8678741a527e406 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Wed, 19 Apr 2017 10:54:03 -0700 Subject: [PATCH] Respond to review comments --- nomad/state/state_store.go | 11 +++++------ nomad/structs/structs.go | 5 +++++ scheduler/generic_sched.go | 16 +++++----------- scheduler/system_sched.go | 16 +++++----------- scheduler/util.go | 2 +- 5 files changed, 21 insertions(+), 29 deletions(-) diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go index 314613f36..983c20a33 100644 --- a/nomad/state/state_store.go +++ b/nomad/state/state_store.go @@ -4,7 +4,6 @@ import ( "fmt" "io" "log" - "sort" "github.com/hashicorp/go-memdb" "github.com/hashicorp/nomad/nomad/structs" @@ -469,7 +468,7 @@ func (s *StateStore) deleteJobVersions(index uint64, job *structs.Job, txn *memd } if _, err = txn.DeleteAll("job_versions", "id", job.ID, job.Version); err != nil { - return fmt.Errorf("deleing job versions failed: %v", err) + return fmt.Errorf("deleting job versions failed: %v", err) } } @@ -595,10 +594,10 @@ func (s *StateStore) jobVersionByID(txn *memdb.Txn, ws *memdb.WatchSet, id strin all = append(all, j) } - // Sort with highest versions first - sort.Slice(all, func(i, j int) bool { - return all[i].Version >= all[j].Version - }) + // Reverse so that highest versions first + for i, j := 0, len(all)-1; i < j; i, j = i+1, j-1 { + all[i], all[j] = all[j], all[i] + } return all, nil } diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 4a86f85db..f04a525f0 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -1388,6 +1388,11 @@ func (j *Job) CombinedTaskMeta(groupName, taskName string) map[string]string { return meta } +// Stopped returns if a job is stopped. +func (j *Job) Stopped() bool { + return j == nil || j.Stop +} + // Stub is used to return a summary of the job func (j *Job) Stub(summary *JobSummary) *JobListStub { return &JobListStub{ diff --git a/scheduler/generic_sched.go b/scheduler/generic_sched.go index d2df0344e..43f38f15e 100644 --- a/scheduler/generic_sched.go +++ b/scheduler/generic_sched.go @@ -179,12 +179,6 @@ func (s *GenericScheduler) createBlockedEval(planFailure bool) error { return s.planner.CreateEval(s.blocked) } -// isStoppedJob returns if the scheduling is for a stopped job and the scheduler -// should stop all its allocations. -func (s *GenericScheduler) isStoppedJob() bool { - return s.job == nil || s.job.Stop -} - // process is wrapped in retryMax to iteratively run the handler until we have no // further work or we've made the maximum number of attempts. func (s *GenericScheduler) process() (bool, error) { @@ -197,7 +191,7 @@ func (s *GenericScheduler) process() (bool, error) { s.eval.JobID, err) } numTaskGroups := 0 - if !s.isStoppedJob() { + if !s.job.Stopped() { numTaskGroups = len(s.job.TaskGroups) } s.queuedAllocs = make(map[string]int, numTaskGroups) @@ -213,7 +207,7 @@ func (s *GenericScheduler) process() (bool, error) { // Construct the placement stack s.stack = NewGenericStack(s.batch, s.ctx) - if !s.isStoppedJob() { + if !s.job.Stopped() { s.stack.SetJob(s.job) } @@ -357,7 +351,7 @@ func (s *GenericScheduler) filterCompleteAllocs(allocs []*structs.Allocation) ([ func (s *GenericScheduler) computeJobAllocs() error { // Materialize all the task groups, job could be missing if deregistered var groups map[string]*structs.TaskGroup - if !s.isStoppedJob() { + if !s.job.Stopped() { groups = materializeTaskGroups(s.job) } @@ -404,7 +398,7 @@ func (s *GenericScheduler) computeJobAllocs() error { // Check if a rolling upgrade strategy is being used limit := len(diff.update) + len(diff.migrate) + len(diff.lost) - if !s.isStoppedJob() && s.job.Update.Rolling() { + if !s.job.Stopped() && s.job.Update.Rolling() { limit = s.job.Update.MaxParallel } @@ -420,7 +414,7 @@ func (s *GenericScheduler) computeJobAllocs() error { // Nothing remaining to do if placement is not required if len(diff.place) == 0 { - if !s.isStoppedJob() { + if !s.job.Stopped() { for _, tg := range s.job.TaskGroups { s.queuedAllocs[tg.Name] = 0 } diff --git a/scheduler/system_sched.go b/scheduler/system_sched.go index b95e5b5fe..a90d27dd2 100644 --- a/scheduler/system_sched.go +++ b/scheduler/system_sched.go @@ -83,12 +83,6 @@ func (s *SystemScheduler) Process(eval *structs.Evaluation) error { s.queuedAllocs) } -// isStoppedJob returns if the scheduling is for a stopped job and the scheduler -// should stop all its allocations. -func (s *SystemScheduler) isStoppedJob() bool { - return s.job == nil || s.job.Stop -} - // process is wrapped in retryMax to iteratively run the handler until we have no // further work or we've made the maximum number of attempts. func (s *SystemScheduler) process() (bool, error) { @@ -101,13 +95,13 @@ func (s *SystemScheduler) process() (bool, error) { s.eval.JobID, err) } numTaskGroups := 0 - if !s.isStoppedJob() { + if !s.job.Stopped() { numTaskGroups = len(s.job.TaskGroups) } s.queuedAllocs = make(map[string]int, numTaskGroups) // Get the ready nodes in the required datacenters - if !s.isStoppedJob() { + if !s.job.Stopped() { s.nodes, s.nodesByDC, err = readyNodesInDCs(s.state, s.job.Datacenters) if err != nil { return false, fmt.Errorf("failed to get ready nodes: %v", err) @@ -125,7 +119,7 @@ func (s *SystemScheduler) process() (bool, error) { // Construct the placement stack s.stack = NewSystemStack(s.ctx) - if !s.isStoppedJob() { + if !s.job.Stopped() { s.stack.SetJob(s.job) } @@ -234,7 +228,7 @@ func (s *SystemScheduler) computeJobAllocs() error { // Check if a rolling upgrade strategy is being used limit := len(diff.update) - if !s.isStoppedJob() && s.job.Update.Rolling() { + if !s.job.Stopped() && s.job.Update.Rolling() { limit = s.job.Update.MaxParallel } @@ -243,7 +237,7 @@ func (s *SystemScheduler) computeJobAllocs() error { // Nothing remaining to do if placement is not required if len(diff.place) == 0 { - if !s.isStoppedJob() { + if !s.job.Stopped() { for _, tg := range s.job.TaskGroups { s.queuedAllocs[tg.Name] = 0 } diff --git a/scheduler/util.go b/scheduler/util.go index 01f10c8dc..c84283b1c 100644 --- a/scheduler/util.go +++ b/scheduler/util.go @@ -21,7 +21,7 @@ type allocTuple struct { // a job requires. This is used to do the count expansion. func materializeTaskGroups(job *structs.Job) map[string]*structs.TaskGroup { out := make(map[string]*structs.TaskGroup) - if job == nil || job.Stop { + if job.Stopped() { return out }