From 9dc22532e5916c3346d62de5e00bb6c5063c8ab3 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Tue, 26 Jan 2016 16:43:42 -0800 Subject: [PATCH] Respond to comments --- nomad/structs/node_class.go | 13 ++----- nomad/structs/structs.go | 3 -- scheduler/context.go | 76 +++++++++++++++++++++++-------------- scheduler/feasible.go | 18 ++++++--- scheduler/feasible_test.go | 14 +++---- scheduler/stack.go | 10 ++++- 6 files changed, 79 insertions(+), 55 deletions(-) diff --git a/nomad/structs/node_class.go b/nomad/structs/node_class.go index eb1f1a703..e0d7d7b74 100644 --- a/nomad/structs/node_class.go +++ b/nomad/structs/node_class.go @@ -84,15 +84,10 @@ func constraintTargetEscapes(target string) bool { switch { case strings.HasPrefix(target, "$node.unique."): return true - - case strings.HasPrefix(target, "$attr."): - attr := strings.TrimPrefix(target, "$attr.") - return strings.HasPrefix(attr, NodeUniqueNamespace) - - case strings.HasPrefix(target, "$meta."): - meta := strings.TrimPrefix(target, "$meta.") - return strings.HasPrefix(meta, NodeUniqueNamespace) - + case strings.HasPrefix(target, "$attr.unique."): + return true + case strings.HasPrefix(target, "$meta.unique."): + return true default: return false } diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index b58e60168..ed82b7918 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -496,9 +496,6 @@ type Node struct { // client. This is opaque to Nomad. Meta map[string]string - // UniqueAttributes are attributes that uniquely identify a node. - UniqueAttributes map[string]struct{} - // NodeClass is an opaque identifier used to group nodes // together for the purpose of determining scheduling pressure. NodeClass string diff --git a/scheduler/context.go b/scheduler/context.go index d70727b8d..c6e80b8a6 100644 --- a/scheduler/context.go +++ b/scheduler/context.go @@ -138,41 +138,51 @@ func (e *EvalContext) Eligibility() *EvalEligibility { return e.eligibility } -type ComputedClassEligibility byte +type ComputedClassFeasibility byte const ( - // The EvalComputedClass enums denote the eligibility of the computed class - // for the evaluation. - EvalComputedClassUnknown ComputedClassEligibility = iota + // EvalComputedClassUnknown is the initial state until the eligibility has + // been explicitely marked to eligible/ineligible or escaped. + EvalComputedClassUnknown ComputedClassFeasibility = iota + + // EvalComputedClassIneligible is used to mark the computed class as + // ineligible for the evaluation. EvalComputedClassIneligible + + // EvalComputedClassIneligible is used to mark the computed class as + // eligible for the evaluation. EvalComputedClassEligible + + // EvalComputedClassEscaped signals that computed class can not determine + // eligibility because a constraint exists that is not captured by computed + // node classes. EvalComputedClassEscaped ) // EvalEligibility tracks eligibility of nodes by computed node class over the // course of an evaluation. type EvalEligibility struct { - // Job tracks the eligibility at the job level per computed node class. - Job map[uint64]ComputedClassEligibility + // job tracks the eligibility at the job level per computed node class. + job map[uint64]ComputedClassFeasibility - // JobEscapedConstraints tracks escaped constraints at the job level. - JobEscapedConstraints []*structs.Constraint + // jobEscapedConstraints tracks escaped constraints at the job level. + jobEscapedConstraints []*structs.Constraint - // TaskGroups tracks the eligibility at the task group level per computed + // taskGroups tracks the eligibility at the task group level per computed // node class. - TaskGroups map[string]map[uint64]ComputedClassEligibility + taskGroups map[string]map[uint64]ComputedClassFeasibility - // TgEscapedConstraints is a map of task groups to a set of constraints that + // tgEscapedConstraints is a map of task groups to a set of constraints that // have escaped. - TgEscapedConstraints map[string][]*structs.Constraint + tgEscapedConstraints map[string][]*structs.Constraint } // NewEvalEligibility returns an eligibility tracker for the context of an evaluation. func NewEvalEligibility() *EvalEligibility { return &EvalEligibility{ - Job: make(map[uint64]ComputedClassEligibility), - TaskGroups: make(map[string]map[uint64]ComputedClassEligibility), - TgEscapedConstraints: make(map[string][]*structs.Constraint), + job: make(map[uint64]ComputedClassFeasibility), + taskGroups: make(map[string]map[uint64]ComputedClassFeasibility), + tgEscapedConstraints: make(map[string][]*structs.Constraint), } } @@ -180,7 +190,7 @@ func NewEvalEligibility() *EvalEligibility { // at the job and task group level. func (e *EvalEligibility) SetJob(job *structs.Job) { // Determine the escaped constraints for the job. - e.JobEscapedConstraints = structs.EscapedConstraints(job.Constraints) + e.jobEscapedConstraints = structs.EscapedConstraints(job.Constraints) // Determine the escaped constraints per task group. for _, tg := range job.TaskGroups { @@ -189,17 +199,20 @@ func (e *EvalEligibility) SetJob(job *structs.Job) { constraints = append(constraints, task.Constraints...) } - e.TgEscapedConstraints[tg.Name] = structs.EscapedConstraints(constraints) + e.tgEscapedConstraints[tg.Name] = structs.EscapedConstraints(constraints) } } // JobStatus returns the eligibility status of the job. -func (e *EvalEligibility) JobStatus(class uint64) ComputedClassEligibility { - if len(e.JobEscapedConstraints) != 0 { +func (e *EvalEligibility) JobStatus(class uint64) ComputedClassFeasibility { + // COMPAT: Computed node class was introduced in 0.3. Clients running < 0.3 + // will not have a computed class. The safest value to return is the escaped + // case, since it disables any optimization. + if len(e.jobEscapedConstraints) != 0 || class == 0 { return EvalComputedClassEscaped } - if status, ok := e.Job[class]; ok { + if status, ok := e.job[class]; ok { return status } return EvalComputedClassUnknown @@ -209,21 +222,28 @@ func (e *EvalEligibility) JobStatus(class uint64) ComputedClassEligibility { // node class. func (e *EvalEligibility) SetJobEligibility(eligible bool, class uint64) { if eligible { - e.Job[class] = EvalComputedClassEligible + e.job[class] = EvalComputedClassEligible } else { - e.Job[class] = EvalComputedClassIneligible + e.job[class] = EvalComputedClassIneligible } } // TaskGroupStatus returns the eligibility status of the task group. -func (e *EvalEligibility) TaskGroupStatus(tg string, class uint64) ComputedClassEligibility { - if escaped, ok := e.TgEscapedConstraints[tg]; ok { +func (e *EvalEligibility) TaskGroupStatus(tg string, class uint64) ComputedClassFeasibility { + // COMPAT: Computed node class was introduced in 0.3. Clients running < 0.3 + // will not have a computed class. The safest value to return is the escaped + // case, since it disables any optimization. + if class == 0 { + return EvalComputedClassEscaped + } + + if escaped, ok := e.tgEscapedConstraints[tg]; ok { if len(escaped) != 0 { return EvalComputedClassEscaped } } - if classes, ok := e.TaskGroups[tg]; ok { + if classes, ok := e.taskGroups[tg]; ok { if status, ok := classes[class]; ok { return status } @@ -234,16 +254,16 @@ func (e *EvalEligibility) TaskGroupStatus(tg string, class uint64) ComputedClass // SetTaskGroupEligibility sets the eligibility status of the task group for the // computed node class. func (e *EvalEligibility) SetTaskGroupEligibility(eligible bool, tg string, class uint64) { - var eligibility ComputedClassEligibility + var eligibility ComputedClassFeasibility if eligible { eligibility = EvalComputedClassEligible } else { eligibility = EvalComputedClassIneligible } - if classes, ok := e.TaskGroups[tg]; ok { + if classes, ok := e.taskGroups[tg]; ok { classes[class] = eligibility } else { - e.TaskGroups[tg] = map[uint64]ComputedClassEligibility{class: eligibility} + e.taskGroups[tg] = map[uint64]ComputedClassFeasibility{class: eligibility} } } diff --git a/scheduler/feasible.go b/scheduler/feasible.go index 3d8eda1df..a9cd0b7fe 100644 --- a/scheduler/feasible.go +++ b/scheduler/feasible.go @@ -497,7 +497,7 @@ OUTER: } // Check if the job has been marked as eligible or ineligible. - jobEscaped := false + jobEscaped, jobUnknown := false, false switch evalElig.JobStatus(option.ComputedClass) { case EvalComputedClassIneligible: // Fast path the ineligible case @@ -505,6 +505,8 @@ OUTER: continue case EvalComputedClassEscaped: jobEscaped = true + case EvalComputedClassUnknown: + jobUnknown = true } // Run the job feasibility checks. @@ -520,13 +522,14 @@ OUTER: } } - // Set the job eligibility if the constraints weren't escaped. - if !jobEscaped { + // Set the job eligibility if the constraints weren't escaped and it + // hasn't been set before. + if !jobEscaped && jobUnknown { evalElig.SetJobEligibility(true, option.ComputedClass) } // Check if the task group has been marked as eligible or ineligible. - tgEscaped := false + tgEscaped, tgUnknown := false, false switch evalElig.TaskGroupStatus(w.tg, option.ComputedClass) { case EvalComputedClassIneligible: // Fast path the ineligible case @@ -537,6 +540,8 @@ OUTER: return option case EvalComputedClassEscaped: tgEscaped = true + case EvalComputedClassUnknown: + tgUnknown = true } // Run the task group feasibility checks. @@ -552,8 +557,9 @@ OUTER: } } - // Set the task group eligibility if the constraints weren't escaped. - if !tgEscaped { + // Set the task group eligibility if the constraints weren't escaped and + // it hasn't been set before. + if !tgEscaped && tgUnknown { evalElig.SetTaskGroupEligibility(true, w.tg, option.ComputedClass) } diff --git a/scheduler/feasible_test.go b/scheduler/feasible_test.go index d72e2f5d0..f50f30c68 100644 --- a/scheduler/feasible_test.go +++ b/scheduler/feasible_test.go @@ -661,7 +661,7 @@ func TestFeasibilityWrapper_JobEscapes(t *testing.T) { // Set the job to escaped cc := nodes[0].ComputedClass - ctx.Eligibility().Job[cc] = EvalComputedClassEscaped + ctx.Eligibility().job[cc] = EvalComputedClassEscaped // Run the wrapper. out := collectFeasible(wrapper) @@ -687,7 +687,7 @@ func TestFeasibilityWrapper_JobAndTg_Eligible(t *testing.T) { // Set the job to escaped cc := nodes[0].ComputedClass - ctx.Eligibility().Job[cc] = EvalComputedClassEligible + ctx.Eligibility().job[cc] = EvalComputedClassEligible ctx.Eligibility().SetTaskGroupEligibility(true, "foo", cc) wrapper.SetTaskGroup("foo") @@ -709,7 +709,7 @@ func TestFeasibilityWrapper_JobEligible_TgIneligible(t *testing.T) { // Set the job to escaped cc := nodes[0].ComputedClass - ctx.Eligibility().Job[cc] = EvalComputedClassEligible + ctx.Eligibility().job[cc] = EvalComputedClassEligible ctx.Eligibility().SetTaskGroupEligibility(false, "foo", cc) wrapper.SetTaskGroup("foo") @@ -731,9 +731,9 @@ func TestFeasibilityWrapper_JobEligible_TgEscaped(t *testing.T) { // Set the job to escaped cc := nodes[0].ComputedClass - ctx.Eligibility().Job[cc] = EvalComputedClassEligible - ctx.Eligibility().TaskGroups["foo"] = - map[uint64]ComputedClassEligibility{cc: EvalComputedClassEscaped} + ctx.Eligibility().job[cc] = EvalComputedClassEligible + ctx.Eligibility().taskGroups["foo"] = + map[uint64]ComputedClassFeasibility{cc: EvalComputedClassEscaped} wrapper.SetTaskGroup("foo") // Run the wrapper. @@ -743,7 +743,7 @@ func TestFeasibilityWrapper_JobEligible_TgEscaped(t *testing.T) { t.Fatalf("bad: %#v %v", out, tgMock.calls()) } - if e, ok := ctx.Eligibility().TaskGroups["foo"][cc]; !ok || e != EvalComputedClassEscaped { + if e, ok := ctx.Eligibility().taskGroups["foo"][cc]; !ok || e != EvalComputedClassEscaped { t.Fatalf("bad: %v %v", e, ok) } } diff --git a/scheduler/stack.go b/scheduler/stack.go index 90fdfa81a..ccbf14e28 100644 --- a/scheduler/stack.go +++ b/scheduler/stack.go @@ -73,7 +73,10 @@ func NewGenericStack(batch bool, ctx Context) *GenericStack { // Filter on task group constraints second s.taskGroupConstraint = NewConstraintChecker(ctx, nil) - // Create the feasibility wrapper. + // Create the feasibility wrapper which wraps all feasibility checks in + // which feasibility checking can be skipped if the computed node class has + // previously been marked as eligible or ineligible. Generally this will be + // checks that only needs to examine the single node to determine feasibility. jobs := []FeasibilityChecker{s.jobConstraint} tgs := []FeasibilityChecker{s.taskGroupDrivers, s.taskGroupConstraint} s.wrappedChecks = NewFeasibilityWrapper(ctx, s.source, jobs, tgs) @@ -197,7 +200,10 @@ func NewSystemStack(ctx Context) *SystemStack { // Filter on task group constraints second s.taskGroupConstraint = NewConstraintChecker(ctx, nil) - // Create the feasibility wrapper. + // Create the feasibility wrapper which wraps all feasibility checks in + // which feasibility checking can be skipped if the computed node class has + // previously been marked as eligible or ineligible. Generally this will be + // checks that only needs to examine the single node to determine feasibility. jobs := []FeasibilityChecker{s.jobConstraint} tgs := []FeasibilityChecker{s.taskGroupDrivers, s.taskGroupConstraint} s.wrappedChecks = NewFeasibilityWrapper(ctx, s.source, jobs, tgs)