Skip to content

Cleanup unused, conditions always true, etc #4361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jparraga-stackav wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Cleanup unused, conditions always true, etc #4361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fee018f
Cleanup unused, conditions always true, etc
jparraga-stackav May 29, 2025
d71c5e2
Merge branch 'master' into misc-cleanup
d80tb7 Jun 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/scheduler/internaltypes/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const (
// This helps avoid scheduling new jobs onto nodes that make it impossible to re-schedule evicted jobs.
EvictedPriority int32 = -1
// MinPriority is the smallest possible priority class priority within the NodeDb.
MinPriority int32 = EvictedPriority
MinPriority = EvictedPriority
)

// Node is a scheduler-internal representation of one Kubernetes node.
Expand Down
1 change: 0 additions & 1 deletion internal/scheduler/internaltypes/node_factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ func (f *NodeFactory) CreateNodeAndType(
labels map[string]string,
totalResources ResourceList,
allocatableResources ResourceList,
unallocatableResources map[int32]ResourceList,
allocatableByPriority map[int32]ResourceList,
) *Node {
return CreateNodeAndType(
Expand Down
8 changes: 4 additions & 4 deletions internal/scheduler/internaltypes/resource_list_map_util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ func TestRlMapSumValues(t *testing.T) {
factory := testFactory()

assert.Equal(t, testResourceList(factory, "3", "3Ki"), RlMapSumValues(testMapAllPositive(factory)))
assert.True(t, RlMapSumValues(testMapEmpty(factory)).IsEmpty())
assert.True(t, RlMapSumValues(testMapEmpty()).IsEmpty())
}

func TestRlMapAllZero(t *testing.T) {
Expand All @@ -26,7 +26,7 @@ func TestRlMapAllZero(t *testing.T) {
assert.True(t, RlMapAllZero(testMapAllZero(factory)))
assert.False(t, RlMapAllZero(testMapOneZero(factory)))
assert.False(t, RlMapAllZero(testMapOneNegative(factory)))
assert.True(t, RlMapAllZero(testMapEmpty(factory)))
assert.True(t, RlMapAllZero(testMapEmpty()))
}

func TestRlMapHasNegativeValues(t *testing.T) {
Expand All @@ -36,7 +36,7 @@ func TestRlMapHasNegativeValues(t *testing.T) {
assert.False(t, RlMapHasNegativeValues(testMapAllZero(factory)))
assert.False(t, RlMapHasNegativeValues(testMapOneZero(factory)))
assert.True(t, RlMapHasNegativeValues(testMapOneNegative(factory)))
assert.False(t, RlMapHasNegativeValues(testMapEmpty(factory)))
assert.False(t, RlMapHasNegativeValues(testMapEmpty()))
}

func TestRlMapRemoveZeros(t *testing.T) {
Expand Down Expand Up @@ -131,6 +131,6 @@ func testMapOneZero(factory *ResourceListFactory) map[string]ResourceList {
}
}

func testMapEmpty(factory *ResourceListFactory) map[string]ResourceList {
func testMapEmpty() map[string]ResourceList {
return map[string]ResourceList{}
}
6 changes: 3 additions & 3 deletions internal/scheduler/internaltypes/unschedulable.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ package internaltypes
import v1 "k8s.io/api/core/v1"

const (
unschedulableTaintKey string = "armadaproject.io/unschedulable"
unschedulableTaintValue string = "true"
unschedulableTaintEffect v1.TaintEffect = v1.TaintEffectNoSchedule
unschedulableTaintKey string = "armadaproject.io/unschedulable"
unschedulableTaintValue string = "true"
unschedulableTaintEffect = v1.TaintEffectNoSchedule
)

// UnschedulableTaint returns the taint automatically added to unschedulable nodes on inserting into the nodeDb.
Expand Down
2 changes: 1 addition & 1 deletion internal/scheduler/jobdb/comparison.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func SchedulingOrderCompare(job, other *Job) int {
// If both jobs are active, order by time since the job was scheduled.
// This ensures jobs that have been running for longer are rescheduled first,
// which reduces wasted compute time when preempting.
if jobIsActive && otherIsActive {
if jobIsActive {
if job.activeRunTimestamp < other.activeRunTimestamp {
return -1
} else if job.activeRunTimestamp > other.activeRunTimestamp {
Expand Down
14 changes: 0 additions & 14 deletions internal/scheduler/jobdb/test_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,6 @@ func getTestFloatingResourceTypes() []schedulerconfiguration.FloatingResourceCon
}
}

//func WithJobDbJobPodRequirements(job *Job, reqs *schedulerobjects.PodRequirements) *Job {
// return JobWithJobSchedulingInfo(job, &schedulerobjects.JobSchedulingInfo{
// PriorityClassName: job.JobSchedulingInfo().PriorityClassName,
// SubmitTime: job.JobSchedulingInfo().SubmitTime,
// ObjectRequirements: []*schedulerobjects.ObjectRequirements{
// {
// Requirements: &schedulerobjects.ObjectRequirements_PodRequirements{
// PodRequirements: reqs,
// },
// },
// },
// })
//}

func JobWithJobSchedulingInfo(job *Job, jobSchedulingInfo *internaltypes.JobSchedulingInfo) *Job {
j, err := job.WithJobSchedulingInfo(jobSchedulingInfo)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion internal/scheduler/kubernetesobjects/taint/taint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func FindMatchingUntoleratedTaint(taints []v1.Taint, tolerations ...[]v1.Tolerat
for _, taint := range taints {
taintTolerated := false
for _, ts := range tolerations {
taintTolerated = taintTolerated || corev1.TolerationsTolerateTaint(ts, &taint)
taintTolerated = corev1.TolerationsTolerateTaint(ts, &taint)
if taintTolerated {
break
}
Expand Down
4 changes: 2 additions & 2 deletions internal/scheduler/nodedb/nodedb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ func NewNodeDb(
nodeDbPriorities = append(nodeDbPriorities, types.AllowedPriorities(priorityClasses)...)

indexedResourceNames := slices.Map(indexedResources, func(v configuration.ResourceType) string { return v.Name })
schema, indexNameByPriority, keyIndexByPriority := nodeDbSchema(nodeDbPriorities, indexedResourceNames)
schema, indexNameByPriority, keyIndexByPriority := nodeDbSchema(nodeDbPriorities)
db, err := memdb.NewMemDB(schema)
if err != nil {
return nil, errors.WithStack(err)
Expand Down Expand Up @@ -1076,7 +1076,7 @@ func (nodeDb *NodeDb) AddEvictedJobSchedulingContextWithTxn(txn *memdb.Txn, inde
return nil
}

func nodeDbSchema(priorities []int32, resources []string) (*memdb.DBSchema, map[int32]string, map[int32]int) {
func nodeDbSchema(priorities []int32) (*memdb.DBSchema, map[int32]string, map[int32]int) {
nodesTable, indexNameByPriority, keyIndexByPriority := nodesTableSchema(priorities)
evictionsTable := evictionsTableSchema()
return &memdb.DBSchema{
Expand Down
4 changes: 2 additions & 2 deletions internal/scheduler/nodedb/nodedb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
)

func TestNodeDbSchema(t *testing.T) {
schema, _, _ := nodeDbSchema(testfixtures.TestPriorities, testfixtures.TestResourceNames)
schema, _, _ := nodeDbSchema(testfixtures.TestPriorities)
assert.NoError(t, schema.Validate())
}

Expand Down Expand Up @@ -606,7 +606,7 @@ func TestAwayNodeScheduling(t *testing.T) {
job := testfixtures.TestJob(
testfixtures.TestQueue, jobId,
"armada-preemptible-away",
testfixtures.Test1Cpu4GiPodReqs(testfixtures.TestQueue, jobId, 30000),
testfixtures.Test1Cpu4GiPodReqs(),
)
if tc.shouldSubmitGang {
job = testfixtures.WithGangAnnotationsJobs([]*jobdb.Job{job.DeepCopy(), job.DeepCopy()})[0]
Expand Down
2 changes: 1 addition & 1 deletion internal/scheduler/nodedb/nodeidindex.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func (nii *nodeIdIndex) FromObject(obj interface{}) (bool, []byte, error) {
node, ok := obj.(*internaltypes.Node)
if !ok {
return false, nil,
fmt.Errorf("Expected type *Node but got %v", reflect.TypeOf(obj))
fmt.Errorf("expected type *Node but got %v", reflect.TypeOf(obj))
}
val := node.GetId()
if val == "" {
Expand Down
98 changes: 41 additions & 57 deletions internal/scheduler/nodedb/nodematching_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,21 +267,17 @@ func TestNodeSchedulingRequirementsMet(t *testing.T) {
expectSuccess: false,
},
"sufficient cpu": {
node: makeTestNodeResources(
t,
map[int32]internaltypes.ResourceList{
0: rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("1"),
},
),
},
rlFactory.FromJobResourceListIgnoreUnknown(
node: makeTestNodeResources(map[int32]internaltypes.ResourceList{
0: rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("1"),
},
),
),
}, rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("1"),
},
)),
req: &internaltypes.PodRequirements{
ResourceRequirements: v1.ResourceRequirements{
Requests: v1.ResourceList{
Expand All @@ -293,20 +289,16 @@ func TestNodeSchedulingRequirementsMet(t *testing.T) {
expectSuccess: true,
},
"insufficient cpu": {
node: makeTestNodeResources(
t,
map[int32]internaltypes.ResourceList{
0: rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("0"),
},
),
},
rlFactory.FromJobResourceListIgnoreUnknown(map[string]resource.Quantity{
"cpu": resource.MustParse("0"),
},
node: makeTestNodeResources(map[int32]internaltypes.ResourceList{
0: rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("0"),
},
),
),
}, rlFactory.FromJobResourceListIgnoreUnknown(map[string]resource.Quantity{
"cpu": resource.MustParse("0"),
},
)),
req: &internaltypes.PodRequirements{
ResourceRequirements: v1.ResourceRequirements{
Requests: v1.ResourceList{
Expand All @@ -318,26 +310,22 @@ func TestNodeSchedulingRequirementsMet(t *testing.T) {
expectSuccess: false,
},
"sufficient cpu at priority": {
node: makeTestNodeResources(
t,
map[int32]internaltypes.ResourceList{
0: rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("0"),
},
),
1: rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("1"),
},
),
},
rlFactory.FromJobResourceListIgnoreUnknown(
node: makeTestNodeResources(map[int32]internaltypes.ResourceList{
0: rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("0"),
},
),
1: rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("1"),
},
),
),
}, rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("1"),
},
)),
req: &internaltypes.PodRequirements{
ResourceRequirements: v1.ResourceRequirements{
Requests: v1.ResourceList{
Expand All @@ -349,26 +337,22 @@ func TestNodeSchedulingRequirementsMet(t *testing.T) {
expectSuccess: true,
},
"insufficient cpu at priority": {
node: makeTestNodeResources(
t,
map[int32]internaltypes.ResourceList{
0: rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("0"),
},
),
1: rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("1"),
},
),
},
rlFactory.FromJobResourceListIgnoreUnknown(
node: makeTestNodeResources(map[int32]internaltypes.ResourceList{
0: rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("0"),
},
),
1: rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("1"),
},
),
),
}, rlFactory.FromJobResourceListIgnoreUnknown(
map[string]resource.Quantity{
"cpu": resource.MustParse("1"),
},
)),
req: &internaltypes.PodRequirements{
ResourceRequirements: v1.ResourceRequirements{
Requests: v1.ResourceList{
Expand Down Expand Up @@ -679,7 +663,7 @@ func makeTestNodeTaintsLabels(taints []v1.Taint, labels map[string]string) *inte
)
}

func makeTestNodeResources(t *testing.T, allocatableByPriority map[int32]internaltypes.ResourceList, totalResources internaltypes.ResourceList) *internaltypes.Node {
func makeTestNodeResources(allocatableByPriority map[int32]internaltypes.ResourceList, totalResources internaltypes.ResourceList) *internaltypes.Node {
return internaltypes.CreateNode(
"id",
internaltypes.NewNodeType([]v1.Taint{},
Expand Down
3 changes: 0 additions & 3 deletions internal/scheduler/scheduling/constraints/constraints.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@ const (
// Indicates that the limit on resources scheduled per round has been exceeded.
MaximumResourcesScheduledUnschedulableReason = "maximum resources scheduled"

// Indicates that a queue has been assigned more than its allowed amount of resources.
MaximumResourcesPerQueueExceededUnschedulableReason = "maximum total resources for this queue exceeded"

// Indicates that the scheduling rate limit has been exceeded.
GlobalRateLimitExceededUnschedulableReason = "global scheduling rate limit exceeded"
QueueRateLimitExceededUnschedulableReason = "queue scheduling rate limit exceeded"
Expand Down
13 changes: 0 additions & 13 deletions internal/scheduler/scheduling/fairness/fairness_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,6 @@ import (

const poolName = "pool"

type MinimalQueue struct {
allocation internaltypes.ResourceList
weight float64
}

func (q MinimalQueue) GetAllocation() internaltypes.ResourceList {
return q.allocation
}

func (q MinimalQueue) GetWeight() float64 {
return q.weight
}

func TestNewDominantResourceFairness(t *testing.T) {
rlFactory := makeTestResourceListFactory()
_, err := NewDominantResourceFairness(
Expand Down
8 changes: 4 additions & 4 deletions internal/scheduler/scheduling/gang_scheduler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,10 +456,10 @@ func TestGangScheduler(t *testing.T) {
jobId = util.ULID()
gangs = append(gangs, []*jobdb.Job{
testfixtures.
TestJob("A", jobId, "armada-preemptible-away", testfixtures.Test1Cpu4GiPodReqs("A", jobId, 30000)),
TestJob("A", jobId, "armada-preemptible-away", testfixtures.Test1Cpu4GiPodReqs()),
})
jobId = util.ULID()
gangs = append(gangs, []*jobdb.Job{testfixtures.TestJob("A", jobId, "armada-preemptible-away-both", testfixtures.Test1Cpu4GiPodReqs("A", jobId, 30000))})
gangs = append(gangs, []*jobdb.Job{testfixtures.TestJob("A", jobId, "armada-preemptible-away-both", testfixtures.Test1Cpu4GiPodReqs())})
return
}(),
ExpectedScheduledIndices: []int{1},
Expand Down Expand Up @@ -502,9 +502,9 @@ func TestGangScheduler(t *testing.T) {
),
Gangs: func() (gangs [][]*jobdb.Job) {
jobId := util.ULID()
gangs = append(gangs, []*jobdb.Job{testfixtures.TestJob("A", jobId, "armada-preemptible-away", testfixtures.Test32Cpu256GiWithLargeJobTolerationPodReqs("A", jobId, 30000))})
gangs = append(gangs, []*jobdb.Job{testfixtures.TestJob("A", jobId, "armada-preemptible-away", testfixtures.Test32Cpu256GiWithLargeJobTolerationPodReqs())})
jobId = util.ULID()
gangs = append(gangs, []*jobdb.Job{testfixtures.TestJob("A", jobId, "armada-preemptible-away", testfixtures.Test32Cpu256GiWithLargeJobTolerationPodReqs("A", jobId, 30000))})
gangs = append(gangs, []*jobdb.Job{testfixtures.TestJob("A", jobId, "armada-preemptible-away", testfixtures.Test32Cpu256GiWithLargeJobTolerationPodReqs())})
return
}(),
ExpectedScheduledIndices: []int{0},
Expand Down
Loading
Loading