Remove the config to disable the scheduling key optimisation (#3960)

* Remove the config to disable the scheduling key optimisation Signed-off-by: Chris Martin <[email protected]> * fix mere conflicts Signed-off-by: Chris Martin <[email protected]> --------- Signed-off-by: Chris Martin <[email protected]>
armadaproject · Sep 23, 2024 · cf9122e · cf9122e
1 parent c3d5faf
commit cf9122e
Show file tree

Hide file tree

Showing 8 changed files with 14 additions and 34 deletions.
diff --git a/config/scheduler/config.yaml b/config/scheduler/config.yaml
@@ -111,6 +111,5 @@ scheduling:
       resolution: "1Gi"
   executorTimeout: "10m"
   maxUnacknowledgedJobsPerExecutor: 2500
-  alwaysAttemptScheduling: false
   executorUpdateFrequency: "60s"
 
diff --git a/internal/scheduler/configuration/configuration.go b/internal/scheduler/configuration/configuration.go
@@ -227,8 +227,6 @@ type SchedulingConfig struct {
 	// Maximum number of jobs that can be assigned to a executor but not yet acknowledged, before
 	// the scheduler is excluded from consideration by the scheduler.
 	MaxUnacknowledgedJobsPerExecutor uint
-	// If true, do not during scheduling skip jobs with requirements known to be impossible to meet.
-	AlwaysAttemptScheduling bool
 	// The frequency at which the scheduler updates the cluster state.
 	ExecutorUpdateFrequency time.Duration
 	// Defines the order in which pools will be scheduled. Higher priority pools will be scheduled first

diff --git a/internal/scheduler/gang_scheduler.go b/internal/scheduler/gang_scheduler.go
@@ -31,19 +31,17 @@ func NewGangScheduler(
 	constraints schedulerconstraints.SchedulingConstraints,
 	floatingResourceTypes *floatingresources.FloatingResourceTypes,
 	nodeDb *nodedb.NodeDb,
+	skipUnsuccessfulSchedulingKeyCheck bool,
 ) (*GangScheduler, error) {
 	return &GangScheduler{
-		constraints:           constraints,
-		floatingResourceTypes: floatingResourceTypes,
-		schedulingContext:     sctx,
-		nodeDb:                nodeDb,
+		constraints:                        constraints,
+		floatingResourceTypes:              floatingResourceTypes,
+		schedulingContext:                  sctx,
+		nodeDb:                             nodeDb,
+		skipUnsuccessfulSchedulingKeyCheck: skipUnsuccessfulSchedulingKeyCheck,
 	}, nil
 }
 
-func (sch *GangScheduler) SkipUnsuccessfulSchedulingKeyCheck() {
-	sch.skipUnsuccessfulSchedulingKeyCheck = true
-}
-
 func (sch *GangScheduler) updateGangSchedulingContextOnSuccess(gctx *schedulercontext.GangSchedulingContext, gangAddedToSchedulingContext bool) error {
 	if !gangAddedToSchedulingContext {
 		// Nothing to do.

diff --git a/internal/scheduler/gang_scheduler_test.go b/internal/scheduler/gang_scheduler_test.go
@@ -643,7 +643,7 @@ func TestGangScheduler(t *testing.T) {
 			constraints := schedulerconstraints.NewSchedulingConstraints("pool", tc.TotalResources, tc.SchedulingConfig, nil, map[string]bool{})
 			floatingResourceTypes, err := floatingresources.NewFloatingResourceTypes(tc.SchedulingConfig.ExperimentalFloatingResources)
 			require.NoError(t, err)
-			sch, err := NewGangScheduler(sctx, constraints, floatingResourceTypes, nodeDb)
+			sch, err := NewGangScheduler(sctx, constraints, floatingResourceTypes, nodeDb, false)
 			require.NoError(t, err)
 
 			var actualScheduledIndices []int

diff --git a/internal/scheduler/preempting_queue_scheduler.go b/internal/scheduler/preempting_queue_scheduler.go
@@ -42,8 +42,6 @@ type PreemptingQueueScheduler struct {
 	jobIdsByGangId map[string]map[string]bool
 	// Maps job ids of gang jobs to the id of that gang.
 	gangIdByJobId map[string]string
-	// If true, the unsuccessfulSchedulingKeys check of gangScheduler is omitted.
-	skipUnsuccessfulSchedulingKeyCheck bool
 }
 
 func NewPreemptingQueueScheduler(
@@ -83,10 +81,6 @@ func NewPreemptingQueueScheduler(
 	}
 }
 
-func (sch *PreemptingQueueScheduler) SkipUnsuccessfulSchedulingKeyCheck() {
-	sch.skipUnsuccessfulSchedulingKeyCheck = true
-}
-
 // Schedule
 // - preempts jobs belonging to queues with total allocation above their fair share and
 // - schedules new jobs belonging to queues with total allocation less than their fair share.
@@ -145,6 +139,7 @@ func (sch *PreemptingQueueScheduler) Schedule(ctx *armadacontext.Context) (*sche
 		armadacontext.WithLogField(ctx, "stage", "re-schedule after balancing eviction"),
 		inMemoryJobRepo,
 		sch.jobRepo,
+		false,
 	)
 	if err != nil {
 		return nil, err
@@ -191,14 +186,13 @@ func (sch *PreemptingQueueScheduler) Schedule(ctx *armadacontext.Context) (*sche
 	// Re-schedule evicted jobs/schedule new jobs.
 	// Only necessary if a non-zero number of jobs were evicted.
 	if len(evictorResult.EvictedJctxsByJobId) > 0 {
-		// Since no new jobs are considered in this round, the scheduling key check brings no benefit.
-		sch.SkipUnsuccessfulSchedulingKeyCheck()
 		ctx.WithField("stage", "scheduling-algo").Info("Performing second scheduling ")
 		schedulerResult, err = sch.schedule(
 			armadacontext.WithLogField(ctx, "stage", "schedule after oversubscribed eviction"),
 			inMemoryJobRepo,
 			// Only evicted jobs should be scheduled in this round.
 			nil,
+			true, // Since no new jobs are considered in this round, the scheduling key check brings no benefit.
 		)
 		if err != nil {
 			return nil, err
@@ -501,7 +495,7 @@ func addEvictedJobsToNodeDb(_ *armadacontext.Context, sctx *schedulercontext.Sch
 	return nil
 }
 
-func (sch *PreemptingQueueScheduler) schedule(ctx *armadacontext.Context, inMemoryJobRepo *InMemoryJobRepository, jobRepo JobRepository) (*schedulerresult.SchedulerResult, error) {
+func (sch *PreemptingQueueScheduler) schedule(ctx *armadacontext.Context, inMemoryJobRepo *InMemoryJobRepository, jobRepo JobRepository, skipUnsuccessfulSchedulingKeyCheck bool) (*schedulerresult.SchedulerResult, error) {
 	jobIteratorByQueue := make(map[string]JobContextIterator)
 	for _, qctx := range sch.schedulingContext.QueueSchedulingContexts {
 		evictedIt := inMemoryJobRepo.GetJobIterator(qctx.Queue)
@@ -522,13 +516,11 @@ func (sch *PreemptingQueueScheduler) schedule(ctx *armadacontext.Context, inMemo
 		sch.floatingResourceTypes,
 		sch.nodeDb,
 		jobIteratorByQueue,
+		skipUnsuccessfulSchedulingKeyCheck,
 	)
 	if err != nil {
 		return nil, err
 	}
-	if sch.skipUnsuccessfulSchedulingKeyCheck {
-		sched.SkipUnsuccessfulSchedulingKeyCheck()
-	}
 	result, err := sched.Schedule(ctx)
 	if err != nil {
 		return nil, err

diff --git a/internal/scheduler/queue_scheduler.go b/internal/scheduler/queue_scheduler.go
@@ -33,13 +33,14 @@ func NewQueueScheduler(
 	floatingResourceTypes *floatingresources.FloatingResourceTypes,
 	nodeDb *nodedb.NodeDb,
 	jobIteratorByQueue map[string]JobContextIterator,
+	skipUnsuccessfulSchedulingKeyCheck bool,
 ) (*QueueScheduler, error) {
 	for queue := range jobIteratorByQueue {
 		if _, ok := sctx.QueueSchedulingContexts[queue]; !ok {
 			return nil, errors.Errorf("no scheduling context for queue %s", queue)
 		}
 	}
-	gangScheduler, err := NewGangScheduler(sctx, constraints, floatingResourceTypes, nodeDb)
+	gangScheduler, err := NewGangScheduler(sctx, constraints, floatingResourceTypes, nodeDb, skipUnsuccessfulSchedulingKeyCheck)
 	if err != nil {
 		return nil, err
 	}
@@ -58,10 +59,6 @@ func NewQueueScheduler(
 	}, nil
 }
 
-func (sch *QueueScheduler) SkipUnsuccessfulSchedulingKeyCheck() {
-	sch.gangScheduler.SkipUnsuccessfulSchedulingKeyCheck()
-}
-
 func (sch *QueueScheduler) Schedule(ctx *armadacontext.Context) (*schedulerresult.SchedulerResult, error) {
 	var scheduledJobs []*schedulercontext.JobSchedulingContext
 

diff --git a/internal/scheduler/queue_scheduler_test.go b/internal/scheduler/queue_scheduler_test.go
@@ -540,7 +540,7 @@ func TestQueueScheduler(t *testing.T) {
 				it := jobRepo.GetJobIterator(q.Name)
 				jobIteratorByQueue[q.Name] = it
 			}
-			sch, err := NewQueueScheduler(sctx, constraints, testfixtures.TestEmptyFloatingResources, nodeDb, jobIteratorByQueue)
+			sch, err := NewQueueScheduler(sctx, constraints, testfixtures.TestEmptyFloatingResources, nodeDb, jobIteratorByQueue, false)
 			require.NoError(t, err)
 
 			result, err := sch.Schedule(armadacontext.Background())

diff --git a/internal/scheduler/scheduling_algo.go b/internal/scheduler/scheduling_algo.go
@@ -484,10 +484,6 @@ func (l *FairSchedulingAlgo) schedulePool(
 		fsctx.jobIdsByGangId,
 		fsctx.gangIdByJobId,
 	)
-	if l.schedulingConfig.AlwaysAttemptScheduling {
-		scheduler.SkipUnsuccessfulSchedulingKeyCheck()
-	}
-
 	result, err := scheduler.Schedule(ctx)
 	if err != nil {
 		return nil, nil, err