Skip to content
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.

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

Queue Limits By Priority #1697

Merged
merged 14 commits into from
Oct 28, 2022
Merged

Queue Limits By Priority #1697

merged 14 commits into from
Oct 28, 2022

Conversation

d80tb7
Copy link
Collaborator

@d80tb7 d80tb7 commented Oct 25, 2022

Implements queue limits by priority. This enables lower (preemptible) pirotiy classes to be given higher queue limits in order to incentivize preemption.

Preemption configuration is now of the form:

PreemptionConfig:
PriorityClasses:
  armada-default:
    priority: 30000
    maximalResourceFractionPerQueue:
      cpu: 20
      memory: 20  
  armada-preemptible:
    priority: 20000
    maximalResourceFractionPerQueue:
      cpu: 90
      memory: 90  

As part of this PR I removed the preemption logic from the old scheduler.

@d80tb7 d80tb7 changed the title [WIP] Queue Limits By Priority Queue Limits By Priority Oct 28, 2022
@@ -248,7 +252,7 @@ type JetstreamConfig struct {

type QueueManagementConfig struct {
AutoCreateQueues bool
DefaultPriorityFactor queue.PriorityFactor
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this because it was causing a circular reference. Essentially other armada module imported by the configuration package is problematic because configuration is imported everywhere. In this case all we were using this for was a type alias so it shouldn't really matter.

@@ -6,8 +6,6 @@ import (
"math/rand"
"time"

v1 "k8s.io/api/core/v1"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the old scheduler where I have removed the preemption logic

@@ -108,13 +108,12 @@ func matchAnyNodeTypeAllocation(
job *api.Job,
nodeAllocations []*nodeTypeAllocation,
alreadyConsumed nodeTypeUsedResources,
supportedPriorityClasses map[string]int32,
) (nodeTypeUsedResources, bool, error) {
newlyConsumed := nodeTypeUsedResources{}

for _, podSpec := range job.GetAllPodSpecs() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the old scheduler where I have removed preemption;

// TODO Is this all validation that needs to be done?
func validateArmadaConfig(config *configuration.ArmadaConfig) error {
// TODO: Is this all validation that needs to be done?
func validateCancelJobsBatchSizeConfig(config *configuration.ArmadaConfig) error {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've renamed this function because it claimed to validate all the armada config but all it actually did was check that the cancelJobsBatchSize was greater than 0! Clealry we need to have a better strategy for validating our configuration but that's a job for another day.

@d80tb7 d80tb7 enabled auto-merge (squash) October 28, 2022 12:04
@d80tb7 d80tb7 merged commit 0b2ca8e into master Oct 28, 2022
@owenthomas17 owenthomas17 deleted the f/chrisma/limit-by-priority branch August 25, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants