Remove useAdjustedFairShareProtection flag (#3806)

Signed-off-by: Chris Martin <[email protected]> Co-authored-by: Chris Martin <[email protected]>
armadaproject · Jul 22, 2024 · f8013fc · f8013fc
1 parent 62302d9
commit f8013fc
Show file tree

Hide file tree

Showing 7 changed files with 16 additions and 31 deletions.
diff --git a/config/scheduler/config.yaml b/config/scheduler/config.yaml
@@ -87,7 +87,6 @@ scheduling:
   disableScheduling: false
   enableAssertions: false
   protectedFractionOfFairShare: 1.0
-  useAdjustedFairShareProtection: true
   nodeIdLabel: "kubernetes.io/hostname"
   priorityClasses:
     armada-default:

diff --git a/internal/scheduler/configuration/configuration.go b/internal/scheduler/configuration/configuration.go
@@ -167,8 +167,6 @@ type SchedulingConfig struct {
 	EnableAssertions bool
 	// Only queues allocated more than this fraction of their fair share are considered for preemption.
 	ProtectedFractionOfFairShare float64 `validate:"gte=0"`
-	// Use Max(AdjustedFairShare, FairShare) for fair share protection. If false then FairShare will be used.
-	UseAdjustedFairShareProtection bool
 	// Armada adds a node selector term to every scheduled pod using this label with the node name as value.
 	// This to force kube-scheduler to schedule pods on the node chosen by Armada.
 	// For example, if NodeIdLabel is "kubernetes.io/hostname" and armada schedules a pod on node "myNode",

diff --git a/internal/scheduler/preempting_queue_scheduler.go b/internal/scheduler/preempting_queue_scheduler.go
@@ -28,13 +28,12 @@ import (
 // PreemptingQueueScheduler is a scheduler that makes a unified decisions on which jobs to preempt and schedule.
 // Uses QueueScheduler as a building block.
 type PreemptingQueueScheduler struct {
-	schedulingContext              *schedulercontext.SchedulingContext
-	constraints                    schedulerconstraints.SchedulingConstraints
-	floatingResourceTypes          *floatingresources.FloatingResourceTypes
-	protectedFractionOfFairShare   float64
-	useAdjustedFairShareProtection bool
-	jobRepo                        JobRepository
-	nodeDb                         *nodedb.NodeDb
+	schedulingContext            *schedulercontext.SchedulingContext
+	constraints                  schedulerconstraints.SchedulingConstraints
+	floatingResourceTypes        *floatingresources.FloatingResourceTypes
+	protectedFractionOfFairShare float64
+	jobRepo                      JobRepository
+	nodeDb                       *nodedb.NodeDb
 	// Maps job ids to the id of the node the job is associated with.
 	// For scheduled or running jobs, that is the node the job is assigned to.
 	// For preempted jobs, that is the node the job was preempted from.
@@ -54,7 +53,6 @@ func NewPreemptingQueueScheduler(
 	constraints schedulerconstraints.SchedulingConstraints,
 	floatingResourceTypes *floatingresources.FloatingResourceTypes,
 	protectedFractionOfFairShare float64,
-	useAdjustedFairShareProtection bool,
 	jobRepo JobRepository,
 	nodeDb *nodedb.NodeDb,
 	initialNodeIdByJobId map[string]string,
@@ -75,16 +73,15 @@ func NewPreemptingQueueScheduler(
 		initialJobIdsByGangId[gangId] = maps.Clone(jobIds)
 	}
 	return &PreemptingQueueScheduler{
-		schedulingContext:              sctx,
-		constraints:                    constraints,
-		floatingResourceTypes:          floatingResourceTypes,
-		protectedFractionOfFairShare:   protectedFractionOfFairShare,
-		useAdjustedFairShareProtection: useAdjustedFairShareProtection,
-		jobRepo:                        jobRepo,
-		nodeDb:                         nodeDb,
-		nodeIdByJobId:                  maps.Clone(initialNodeIdByJobId),
-		jobIdsByGangId:                 initialJobIdsByGangId,
-		gangIdByJobId:                  maps.Clone(initialGangIdByJobId),
+		schedulingContext:            sctx,
+		constraints:                  constraints,
+		floatingResourceTypes:        floatingResourceTypes,
+		protectedFractionOfFairShare: protectedFractionOfFairShare,
+		jobRepo:                      jobRepo,
+		nodeDb:                       nodeDb,
+		nodeIdByJobId:                maps.Clone(initialNodeIdByJobId),
+		jobIdsByGangId:               initialJobIdsByGangId,
+		gangIdByJobId:                maps.Clone(initialGangIdByJobId),
 	}
 }
 
@@ -136,10 +133,7 @@ func (sch *PreemptingQueueScheduler) Schedule(ctx *armadacontext.Context) (*Sche
 				}
 				if qctx, ok := sch.schedulingContext.QueueSchedulingContexts[job.Queue()]; ok {
 					actualShare := sch.schedulingContext.FairnessCostProvider.UnweightedCostFromQueue(qctx) / totalCost
-					fairShare := qctx.FairShare
-					if sch.useAdjustedFairShareProtection {
-						fairShare = math.Max(qctx.AdjustedFairShare, fairShare)
-					}
+					fairShare := math.Max(qctx.AdjustedFairShare, qctx.FairShare)
 					fractionOfFairShare := actualShare / fairShare
 					if fractionOfFairShare <= sch.protectedFractionOfFairShare {
 						return false

diff --git a/internal/scheduler/preempting_queue_scheduler_test.go b/internal/scheduler/preempting_queue_scheduler_test.go
@@ -1869,7 +1869,6 @@ func TestPreemptingQueueScheduler(t *testing.T) {
 					constraints,
 					testfixtures.TestEmptyFloatingResources,
 					tc.SchedulingConfig.ProtectedFractionOfFairShare,
-					tc.SchedulingConfig.UseAdjustedFairShareProtection,
 					NewSchedulerJobRepositoryAdapter(jobDbTxn),
 					nodeDb,
 					nodeIdByJobId,
@@ -2224,7 +2223,6 @@ func BenchmarkPreemptingQueueScheduler(b *testing.B) {
 				constraints,
 				testfixtures.TestEmptyFloatingResources,
 				tc.SchedulingConfig.ProtectedFractionOfFairShare,
-				tc.SchedulingConfig.UseAdjustedFairShareProtection,
 				NewSchedulerJobRepositoryAdapter(jobDbTxn),
 				nodeDb,
 				nil,
@@ -2286,7 +2284,6 @@ func BenchmarkPreemptingQueueScheduler(b *testing.B) {
 					constraints,
 					testfixtures.TestEmptyFloatingResources,
 					tc.SchedulingConfig.ProtectedFractionOfFairShare,
-					tc.SchedulingConfig.UseAdjustedFairShareProtection,
 					NewSchedulerJobRepositoryAdapter(jobDbTxn),
 					nodeDb,
 					nil,

diff --git a/internal/scheduler/scheduling_algo.go b/internal/scheduler/scheduling_algo.go
@@ -498,7 +498,6 @@ func (l *FairSchedulingAlgo) schedulePool(
 		constraints,
 		l.floatingResourceTypes,
 		l.schedulingConfig.ProtectedFractionOfFairShare,
-		l.schedulingConfig.UseAdjustedFairShareProtection,
 		NewSchedulerJobRepositoryAdapter(fsctx.txn),
 		nodeDb,
 		fsctx.nodeIdByJobId,

diff --git a/internal/scheduler/simulator/simulator.go b/internal/scheduler/simulator/simulator.go
@@ -520,7 +520,6 @@ func (s *Simulator) handleScheduleEvent(ctx *armadacontext.Context) error {
 				constraints,
 				nloatingResourceTypes,
 				s.schedulingConfig.ProtectedFractionOfFairShare,
-				s.schedulingConfig.UseAdjustedFairShareProtection,
 				scheduler.NewSchedulerJobRepositoryAdapter(txn),
 				nodeDb,
 				// TODO: Necessary to support partial eviction.

diff --git a/internal/scheduler/testfixtures/testfixtures.go b/internal/scheduler/testfixtures/testfixtures.go
@@ -192,7 +192,6 @@ func TestSchedulingConfig() schedulerconfiguration.SchedulingConfig {
 		ExecutorTimeout:                             15 * time.Minute,
 		MaxUnacknowledgedJobsPerExecutor:            math.MaxInt,
 		SupportedResourceTypes:                      GetTestSupportedResourceTypes(),
-		UseAdjustedFairShareProtection:              true,
 	}
 }