Preemption for system jobs #4794

preetapan · 2018-10-17T02:35:23Z

This PR brings in prior work from PR #4710, rebased to master and modifying all the internal comparison logic to use the new ComparableResources struct.

preetapan · 2018-10-17T16:10:39Z

scheduler/preemption.go

+
+}
+
+// computeCurrentPreemptions counts the number of other allocations being preempted that match the job and task group of


todo this could be calculated ahead of time per jobid in the set of existing allocations on the node

dadgar · 2018-10-17T19:37:56Z

scheduler/preemption.go

+	// after accounting for reserved resources and all allocations
+
+	// Subtract the reserved resources of the node
+	if node.ComparableReservedResources() != nil {


if c := node.ComparableReservedResources(); c != nil

Still use the suggestion

dadgar · 2018-10-17T22:52:02Z

command/agent/operator_endpoint_test.go

@@ -257,3 +258,102 @@ func TestOperator_ServerHealth_Unhealthy(t *testing.T) {
 		})
 	})
 }
+
+func TestOperator_SchedulerGetConfiguration(t *testing.T) {


Still needs ACL tests

nomad/fsm.go

dadgar · 2018-10-17T22:53:47Z

nomad/fsm.go

+	if err := structs.Decode(buf, &req); err != nil {
+		panic(fmt.Errorf("failed to decode request: %v", err))
+	}
+	defer metrics.MeasureSince([]string{"nomad", "fsm", "scheduler-config"}, time.Now())


Can you check what other metrics do when they are two words. I feel like it may be underscore

dadgar · 2018-10-17T22:55:22Z

nomad/fsm_test.go

+	buf, err = structs.Encode(structs.SchedulerConfigRequestType, req)
+	require.Nil(err)
+
+	resp = fsm.Apply(makeLog(buf))


Shouldn't you check the resp as you are returning an actual value

dadgar · 2018-10-17T22:57:10Z

nomad/leader.go

+		return config
+	}
+
+	config = &structs.SchedulerConfiguration{PreemptionConfig: structs.PreemptionConfig{SystemSchedulerEnabled: true}}


Can this be a variable at the top of the file with a comment on it

dadgar · 2018-10-17T23:00:33Z

nomad/operator_endpoint.go

+	}
+
+	*reply = *config
+


Set indexes on the responses

nomad/state/schema.go

dadgar · 2018-10-17T23:03:06Z

nomad/state/state_store.go

@@ -3956,3 +3991,81 @@ func (r *StateRestore) addEphemeralDiskToTaskGroups(job *structs.Job) {
 		}
 	}
 }
+


Move the code above the restore functions

dadgar · 2018-10-17T23:04:59Z

nomad/fsm.go

+	defer metrics.MeasureSince([]string{"nomad", "fsm", "scheduler-config"}, time.Now())
+
+	if req.CAS {
+		act, err := n.state.SchedulerCASConfig(index, req.Config.ModifyIndex, &req.Config)


act is a weird variable name? applied?

nomad/structs/operator.go

dadgar · 2018-10-17T23:09:00Z

nomad/fsm.go

@@ -1833,6 +1835,23 @@ func (s *nomadSnapshot) persistACLTokens(sink raft.SnapshotSink,
 	return nil
 }

+func (n *nomadFSM) applySchedulerConfigUpdate(buf []byte, index uint64) interface{} {


Move to be above here:

nomad/nomad/fsm.go

Line 998 in 4db7e87

dadgar · 2018-10-17T23:09:32Z

nomad/fsm.go

@@ -1833,6 +1835,23 @@ func (s *nomadSnapshot) persistACLTokens(sink raft.SnapshotSink,
 	return nil
 }

+func (n *nomadFSM) applySchedulerConfigUpdate(buf []byte, index uint64) interface{} {


Where is the snapshot and restore code?

nomad/fsm_test.go

dadgar · 2018-10-17T23:12:30Z

nomad/structs/structs_test.go

@@ -1869,6 +1869,58 @@ func TestResource_Add_Network(t *testing.T) {
 	}
 }

+func TestResource_Subtract(t *testing.T) {


Convert to comparable resource test

dadgar · 2018-10-17T23:12:51Z

scheduler/generic_sched.go

@@ -130,7 +130,8 @@ func (s *GenericScheduler) Process(eval *structs.Evaluation) error {
 		structs.EvalTriggerRollingUpdate, structs.EvalTriggerQueuedAllocs,
 		structs.EvalTriggerPeriodicJob, structs.EvalTriggerMaxPlans,
 		structs.EvalTriggerDeploymentWatcher, structs.EvalTriggerRetryFailedAlloc,
-		structs.EvalTriggerFailedFollowUp:
+		structs.EvalTriggerFailedFollowUp,


Join into one line

dadgar · 2018-10-17T23:15:24Z

nomad/plan_apply.go

+	// Also gather jobids to create follow up evals
+	preemptedJobIDs := make(map[structs.NamespacedID]struct{})
+	for _, alloc := range req.NodePreemptions {
+		if alloc.CreateTime == 0 {


Create time doesn't make sense since we are preempting an alloc that exists

nomad/plan_apply.go

dadgar · 2018-10-18T00:16:28Z

scheduler/rank.go

+					offer, err = netIdx.AssignNetwork(ask)
+					if offer == nil {
+						iter.ctx.Metrics().ExhaustedNode(option.Node,
+							fmt.Sprintf("unexecpted error, unable to create offer after preempting:%v", err))


Hm seems odd to expose this in the metrics. I would drop that an instead emit an error level log (iter.ctx.Logger().Error(...))

dadgar · 2018-10-18T00:27:13Z

scheduler/rank.go

@@ -195,6 +199,16 @@ OUTER:
 				DiskMB: int64(iter.taskGroup.EphemeralDisk.SizeMB),
 			},
 		}
+


I think this method has to be refactored. It will get even more complex once devices are considered. I had a hard to following the code and I know it quite well

scheduler/rank.go

dadgar · 2018-10-18T00:28:10Z

scheduler/rank.go

+			// any allocs can be preempted
+
+			// Remove the last element containing the current placement from proposed allocs
+			current := proposed[:len(proposed)-1]


Not a fan of this. Capture it outside as a variable

scheduler/system_sched.go

This commit implements an allocation selection algorithm for finding allocations to preempt. It currently special cases network resource asks from others (cpu/memory/disk/iops).

…eemption

…ler type

…on fields

dadgar · 2018-10-31T17:13:49Z

command/agent/operator_endpoint.go

+
+		return resp, nil
+
+	case "PUT":


We accept POST and PUT

dadgar · 2018-10-31T17:14:35Z

command/agent/operator_endpoint.go

+	// Switch on the method
+	switch req.Method {
+	case "GET":
+		var args structs.GenericRequest


Generally split the methods up so that it only handles one type of HTTP verb: https://github.com/hashicorp/nomad/blob/master/command/agent/job_endpoint.go#L15

dadgar · 2018-10-31T17:16:41Z

api/operator.go

+	SchedulerConfig SchedulerConfiguration
+
+	// CreateIndex/ModifyIndex store the create/modify indexes of this configuration.
+	CreateIndex uint64


Should be WriteMeta and QueryMeta: https://github.com/hashicorp/nomad/blob/master/api/jobs.go#L1039-L1046

dadgar · 2018-10-31T17:18:15Z

command/agent/operator_endpoint.go

+		return reply, nil
+
+	default:
+		return nil, CodedError(404, ErrInvalidMethod)


dadgar · 2018-10-31T17:19:41Z

command/agent/operator_endpoint.go

+		if !args.CAS {
+			return true, nil
+		}
+		return reply, nil


Use SetIndex: https://github.com/hashicorp/nomad/blob/master/command/agent/job_endpoint.go#L396

dadgar · 2018-10-31T17:26:45Z

nomad/operator_endpoint.go

+		CreateIndex:     config.CreateIndex,
+		ModifyIndex:     config.ModifyIndex,
+	}
+	*reply = *resp


Just use the reply, there is no need for the resp variable?

dadgar · 2018-10-31T17:28:31Z

nomad/state/state_store.go

+
+// SchedulerCASConfig is used to try updating the scheduler configuration with a
+// given Raft index. If the CAS index specified is not equal to the last observed index
+// for the config, then the call is a noop,


Last comma should be a period

dadgar · 2018-10-31T17:28:49Z

nomad/state/state_store.go

+	return nil
+}
+
+// SchedulerCASConfig is used to try updating the scheduler configuration with a


is used to update the scheduler configuration...

dadgar · 2018-10-31T17:30:46Z

nomad/state/state_store.go

+	// index arg, then we shouldn't update anything and can safely
+	// return early here.
+	e, ok := existing.(*structs.SchedulerConfiguration)
+	if !ok || e.ModifyIndex != cidx {


I may be wrong here butt can't e == nil and ok == true, in which case you can panic dereferencing the modify index.

dadgar · 2018-10-31T17:31:42Z

nomad/structs/operator.go

+	SchedulerConfig SchedulerConfiguration
+
+	// CreateIndex/ModifyIndex store the create/modify indexes of this configuration.
+	CreateIndex uint64


dadgar

Main comment is that we need to reduce the number of times we call ComparableResources on allocations. It should only ever be called once per alloc

dadgar · 2018-10-31T17:34:19Z

nomad/plan_apply.go

@@ -181,6 +187,36 @@ func (p *planner) applyPlan(plan *structs.Plan, result *structs.PlanResult, snap
 		alloc.ModifyTime = now
 	}

+	// Set create and modify time for preempted allocs if any


remove create and

dadgar · 2018-10-31T17:36:47Z

nomad/plan_apply.go

@@ -318,6 +356,25 @@ func evaluatePlanPlacements(pool *EvaluatePool, snap *state.StateSnapshot, plan
 		if nodeAlloc := plan.NodeAllocation[nodeID]; len(nodeAlloc) > 0 {
 			result.NodeAllocation[nodeID] = nodeAlloc
 		}
+
+		if nodePreemptions := plan.NodePreemptions[nodeID]; nodePreemptions != nil {
+			var filteredNodePreemptions []*structs.Allocation


Looks a bit cleaner if you put the slice that is being used right above the for loop. So 1. Comment 2. slice declaration 3. for loop

nomad/plan_apply.go

dadgar · 2018-10-31T17:46:09Z

nomad/structs/structs.go

+// by reading AllocatedResources which are populated starting in 0.9 and
+// falling back to pre 0.9 fields (Resources/TaskResources) if set
+func (a *Allocation) CompatibleNetworkResources() []*NetworkResource {
+	var ret []*NetworkResource


Delete this now?

nomad/structs/structs.go

dadgar · 2018-10-31T20:30:20Z

scheduler/preemption.go

+		// First try to satisfy needed reserved ports
+		if len(reservedPortsNeeded) > 0 {
+			for _, alloc := range currentAllocs {
+				for _, tr := range alloc.TaskResources {


Shouldn't you be using comparable here. This doesn't use the new AllocatedResources stanza and will break once TaskResources is deprecated

dadgar · 2018-10-31T20:31:39Z

scheduler/preemption.go

+}
+
+func (n *NetworkPreemptionResource) Distance() float64 {
+	networkCoord := math.MaxFloat64


Call networkResourceDistance() to be DRY

dadgar · 2018-10-31T20:31:52Z

scheduler/preemption.go

+		networkCoord = float64(resourceNeeded.MBits-resourceUsed.MBits) / float64(resourceNeeded.MBits)
+	}
+
+	originDist := math.Sqrt(


dadgar · 2018-10-31T20:33:34Z

scheduler/preemption.go

+	// Do another pass to eliminate allocations that are a superset of other allocations
+	// in the preemption set
+	for _, alloc := range bestAllocs {
+		if preemptedResources == nil {


Same comment, you are just accumulating node free resources + preempted resources so just use one object

scheduler/preemption.go

…m alloc.Resources

also removed now unused method

Also added test case

dadgar · 2018-11-02T20:57:15Z

scheduler/preemption.go

+		if tg != nil && tg.Migrate != nil {
+			maxParallel = tg.Migrate.MaxParallel
+		}
+		p.allocDetails[alloc.ID] = &allocInfo{maxParallel, alloc.ComparableResources()}


I think vet will complain at this. You should use the field name when constructing the struct

dadgar · 2018-11-02T20:59:07Z

scheduler/preemption.go

 		if p.jobPriority-alloc.Job.Priority < 10 {
+
+			// If this allocation uses a needed reserved port
+			// preemption is impossible so we return early


Comment a bit stale

dadgar · 2018-11-02T21:00:26Z

scheduler/preemption.go

 	return filteredBestAllocs
 }

+func usedReservedPorts(net *structs.NetworkResource, portMap map[int]interface{}) bool {


dadgar · 2018-11-02T21:04:11Z

scheduler/preemption.go

+			availableResources.Add(closestResources)
+
+			// This step needs the original resources asked for as the second arg, can't use the running total
+			allRequirementsMet, _ = availableResources.Superset(resourceAsk.Comparable())


compute resourceAsk.Comparable() once at the top of this method?

dadgar · 2018-11-02T21:05:19Z

scheduler/preemption.go

+	reservedPortsNeeded := networkResourceAsk.ReservedPorts
+
+	// Build map of reserved ports needed for fast access
+	reservedPorts := make(map[int]interface{})


Use struct{} instead of interface{}. It is a smaller object and more canonical for set implementation.

dadgar · 2018-11-02T21:06:04Z

scheduler/preemption.go

+		reservedPorts[port.Value] = struct{}{}
+	}
+
+	filteredReservedPorts := make(map[string]map[int]struct{})


Comment on what this map is for

dadgar · 2018-11-02T21:06:34Z

scheduler/preemption.go

+		// Reset allocsToPreempt since we don't want to preempt across devices for the same task
+		allocsToPreempt = nil
+
+		usedPortToAlloc := make(map[int]*structs.Allocation)


dadgar · 2018-11-02T21:17:35Z

LGTM after last comments are addressed

github-actions · 2023-02-25T02:18:31Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

preetapan requested a review from dadgar October 17, 2018 16:08

preetapan commented Oct 17, 2018

View reviewed changes

dadgar reviewed Oct 17, 2018

View reviewed changes

dadgar requested changes Oct 17, 2018

View reviewed changes

dadgar reviewed Oct 17, 2018

View reviewed changes

dadgar requested changes Oct 17, 2018

View reviewed changes

dadgar requested changes Oct 18, 2018

View reviewed changes

Preetha Appan added 23 commits October 30, 2018 11:06

structs and API changes to plan and alloc structs needed for preemption

5f27e00

REview feedback

8004160

Add number of evictions to DesiredUpdates struct to use in CLI/API

bf7192c

structs and API changes to plan and alloc structs needed for preemption

fc266f7

Implement preemption for system jobs.

715d869

This commit implements an allocation selection algorithm for finding allocations to preempt. It currently special cases network resource asks from others (cpu/memory/disk/iops).

Fix comment

b5cbd73

Fix logic bug, unit test for plan apply method in state store

13e314c

Fix linting and better comments

fd6bff2

Fix linting

51c5bae

Support for new scheduler config API, first use case is to disable pr…

784b96c

…eemption

Use scheduler config from state store to enable/disable preemption

2143fa2

Make preemption config a struct to allow for enabling based on schedu…

6966e3c

…ler type

Modify preemption code to use new style of resource structs

24b3934

fix end to end scheduler test to use new resource structs correctly

9f35923

comments

a960cce

Preempted allocations should be removed from proposed allocations

655689a

Restore/Snapshot plus unit tests for scheduler configuration

c4e0e66

Add fsm layer tests

c3b8e4f

make default config a variable

12af2ae

style fixes

c4a04eb

More review comments

191b862

More style and readablity fixes from review

21432d6

more minor cleanup

4cc21fb

Preetha Appan added 4 commits October 30, 2018 11:06

refactor preemption code to use method recievers and setters for comm…

35635ba

…on fields

Introduce a response object for scheduler configuration

8800585

Introduce interface with multiple implementations for resource distance

17344a7

Fix return type in tests after refactor

f2b0277

preetapan force-pushed the f-preemption-systemjobs branch from 21a9b8b to f2b0277 Compare October 30, 2018 16:11

preetapan mentioned this pull request Oct 30, 2018

show preemptions in nomad plan CLI #4823

Merged

dadgar requested changes Oct 31, 2018

View reviewed changes

Preetha Appan added 10 commits November 1, 2018 12:01

review comments

22d156f

Cleaner way to exit early, and fixed a couple more places reading fro…

993b6a2

…m alloc.Resources

More review comments

3ad7b3f

Plumb alloc resource cache in a few more places.

06ad182

also removed now unused method

more minor review feedback

35d31f8

unit test plan apply with preemptions

b3738a0

Handle static port preemption when there are multiple devices

0015095

Also added test case

Fix static port preemption to be device aware

8235919

dereference safely

1380acb

Fix test setup

c49a3e2

dadgar requested changes Nov 2, 2018

View reviewed changes

dadgar approved these changes Nov 2, 2018

View reviewed changes

Address more minor code review feedback

97cf4e1

preetapan merged commit 6aa0c7f into master Nov 2, 2018

preetapan deleted the f-preemption-systemjobs branch November 2, 2018 23:13

preetapan mentioned this pull request Nov 12, 2018

improvements to scheduler config API #4865

Merged

github-actions bot locked as resolved and limited conversation to collaborators Feb 25, 2023


		}

		// computeCurrentPreemptions counts the number of other allocations being preempted that match the job and task group of

Preemption for system jobs #4794

Preemption for system jobs #4794

Conversation

preetapan commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadgar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadgar commented Nov 2, 2018

github-actions bot commented Feb 25, 2023