[YUNIKORN-3193] Quota preemption to be turned off by default (#1061)

Quota preemption is a new functionality. Quota preemption is turned off
by default at the partition level via a config. To use quota preemption
the 'QuotaPreemptionEnabled' flag in the partition preemption config
must be set top 'true'.
----
partitions:
  - name: default
    preemption:
      quotapreemptionenabled: true
----
When turned on the delay for quota preemption is set via the queue
property: `preemption.quota.delay`, The property uses the same syntax as
the preemption delay: a Golang duration in string form.
The default delay is set to '0s' and no preemption will be triggered.

Quota preemption is only supported on managed queues not on dynamic
queues.

Closes: #1061

Signed-off-by: Wilfred Spiegelenburg <wilfreds@apache.org>
(cherry picked from commit 9883272)

Author: wilfred-s

pkg/common/configs/config.go

@@ -32,14 +32,14 @@ import (
 	"github.com/apache/yunikorn-core/pkg/log"
 )
 
-// The configuration can contain multiple partitions. Each partition contains the queue definition for a logical
+// SchedulerConfig can contain multiple partitions. Each partition contains the queue definition for a logical
 // set of scheduler resources.
 type SchedulerConfig struct {
 	Partitions []PartitionConfig
 	Checksum   string `yaml:",omitempty" json:",omitempty"`
 }
 
-// The partition object for each partition:
+// PartitionConfig for each partition:
 // - the name of the partition
 // - a list of sub or child queues
 // - a list of placement rule definition objects
@@ -60,12 +60,13 @@ type UserGroupResolver struct {
 	Type string `yaml:"type,omitempty" json:"type,omitempty"`
 }
 
-// The partition preemption configuration
+// PartitionPreemptionConfig defines global flags for both preemption types
 type PartitionPreemptionConfig struct {
-	Enabled *bool `yaml:",omitempty" json:",omitempty"`
+	Enabled                *bool `yaml:",omitempty" json:",omitempty"`
+	QuotaPreemptionEnabled *bool `yaml:",omitempty" json:",omitempty"`
 }
 
-// The queue object for each queue:
+// QueueConfig object for each queue:
 // - the name of the queue
 // - a resources object to specify resource limits on the queue
 // - the maximum number of applications that can run in the queue
@@ -84,20 +85,16 @@ type QueueConfig struct {
 	ChildTemplate   ChildTemplate     `yaml:",omitempty" json:",omitempty"`
 	Queues          []QueueConfig     `yaml:",omitempty" json:",omitempty"`
 	Limits          []Limit           `yaml:",omitempty" json:",omitempty"`
-	Preemption      Preemption        `yaml:",omitempty" json:",omitempty"`
-}
-
-type Preemption struct {
-	Delay uint64 `yaml:",omitempty" json:",omitempty"`
 }
 
+// ChildTemplate set on a parent queue with settings to be applied to the child created via a placement rule.
 type ChildTemplate struct {
 	MaxApplications uint64            `yaml:",omitempty" json:",omitempty"`
 	Properties      map[string]string `yaml:",omitempty" json:",omitempty"`
 	Resources       Resources         `yaml:",omitempty" json:",omitempty"`
 }
 
-// The resource limits to set on the queue. The definition allows for an unlimited number of types to be used.
+// The Resources limit to apply on the queue. The definition allows for an unlimited number of types to be used.
 // The mapping to "known" resources is not handled here.
 // - guaranteed resources
 // - max resources
@@ -106,12 +103,12 @@ type Resources struct {
 	Max        map[string]string `yaml:",omitempty" json:",omitempty"`
 }
 
-// The queue placement rule definition
+// The PlacementRule definition:
 // - the name of the rule
 // - create flag: can the rule create a queue
 // - user and group filter to be applied on the callers
 // - rule link to allow setting a rule to generate the parent
-// - value a generic value interpreted depending on the rule type (i.e queue name for the "fixed" rule
+// - value a generic value interpreted depending on the rule type (i.e. queue name for the "fixed" rule
 // or the application label name for the "tag" rule)
 type PlacementRule struct {
 	Name   string
@@ -121,7 +118,7 @@ type PlacementRule struct {
 	Value  string         `yaml:",omitempty" json:",omitempty"`
 }
 
-// The user and group filter for a rule.
+// Filter for users and groups for a PlacementRule.
 // - type of filter (allow or deny filter, empty means allow)
 // - list of users to filter (maybe empty)
 // - list of groups to filter (maybe empty)
@@ -132,13 +129,13 @@ type Filter struct {
 	Groups []string `yaml:",omitempty" json:",omitempty"`
 }
 
-// A list of limit objects to define limits for a partition or queue
+// Limits is a list of Limit objects to define user and group limits for a partition or queue.
 type Limits struct {
 	Limit []Limit
 }
 
-// The limit object to specify user and or group limits at different levels in the partition or queues
-// Different limits for the same user or group may be defined at different levels in the hierarchy
+// A Limit object to specify user and or group limits at different levels in the partition or queues.
+// Different limits for the same user or group may be defined at different levels in the hierarchy:
 // - limit description (optional)
 // - list of users (maybe empty)
 // - list of groups (maybe empty)
@@ -152,8 +149,10 @@ type Limit struct {
 	MaxApplications uint64            `yaml:",omitempty" json:",omitempty"`
 }
 
-// Global Node Sorting Policy section
-// - type: different type of policies supported (binpacking, fair etc)
+// NodeSortingPolicy to be applied globally.
+// - type: different type of policies supported (binpacking, fair etc.)
+// - resource weight: factor to be applied to comparisons of different resource types when sorting nodes. Types not
+// mentioned have a weight of 1.0.
 type NodeSortingPolicy struct {
 	Type            string
 	ResourceWeights map[string]float64 `yaml:",omitempty" json:",omitempty"`

pkg/common/configs/config_test.go

@@ -626,6 +626,7 @@ partitions:
       - name: root
     preemption:
       enabled: true
+      quotapreemptionenabled: true
   - name: "partition-0"
     queues:
       - name: root
@@ -637,22 +638,26 @@ partitions:
       - name: root
     preemption:
       enabled: false
+      quotapreemptionenabled: false
   - name: "partition-0"
     queues:
       - name: root
 `
 	// validate the config and check after the update
 	conf, err := CreateConfig(data)
 	assert.NilError(t, err, "should expect no error")
-	assert.Assert(t, conf.Partitions[0].Preemption.Enabled == nil, "default should be nil")
+	assert.Assert(t, conf.Partitions[0].Preemption.Enabled == nil, "default preemption should be nil")
+	assert.Assert(t, conf.Partitions[0].Preemption.QuotaPreemptionEnabled == nil, "default quota preemption should be nil")
 
 	conf, err = CreateConfig(dataEnabled)
 	assert.NilError(t, err, "should expect no error")
 	assert.Assert(t, *conf.Partitions[0].Preemption.Enabled, "preemption should be enabled")
+	assert.Assert(t, *conf.Partitions[0].Preemption.QuotaPreemptionEnabled, "quota preemption should be enabled")
 
 	conf, err = CreateConfig(dataDisabled)
 	assert.NilError(t, err, "should expect no error")
 	assert.Assert(t, !*conf.Partitions[0].Preemption.Enabled, "preemption should be disabled")
+	assert.Assert(t, !*conf.Partitions[0].Preemption.QuotaPreemptionEnabled, "quota preemption should be disabled")
 }
 
 func TestParseRule(t *testing.T) {

pkg/common/configs/configvalidator.go

@@ -41,6 +41,7 @@ const (
 	DotReplace       = "_dot_"
 	DefaultPartition = "default"
 
+	// constants defining the names for properties
 	ApplicationSortPolicy                    = "application.sort.policy"
 	ApplicationSortPriority                  = "application.sort.priority"
 	ApplicationUnschedulableAsksBackoff      = "application.unschedasks.backoff"
@@ -49,6 +50,7 @@ const (
 	PriorityOffset                           = "priority.offset"
 	PreemptionPolicy                         = "preemption.policy"
 	PreemptionDelay                          = "preemption.delay"
+	QuotaPreemptionDelay                     = "preemption.quota.delay"
 
 	// app sort priority values
 	ApplicationSortPriorityEnabled  = "enabled"
@@ -61,35 +63,39 @@ const (
 	errLastQueueLeaf
 )
 
-type placementPathCheckResult int
-
-// Priority
+// MinPriority and MaxPriority used in offset calculations. K8s uses an int32 value for priorities, internal calculations
+// use int64.
 var MinPriority int32 = math.MinInt32
 var MaxPriority int32 = math.MaxInt32
 
+// DefaultQuotaPreemptionDelay is 0, no quota preemption by default
+var DefaultQuotaPreemptionDelay time.Duration = 0
+
+// DefaultPreemptionDelay is 30 seconds, guaranteed resources must be set to trigger preemption
 var DefaultPreemptionDelay = 30 * time.Second
 var DefaultAskBackOffDelay = 30 * time.Second
 
-// A queue can be a username with the dot replaced. Most systems allow a 32 character user name.
+// QueueNameRegExp to validate the name of a queue.
+// A queue can be a username with the dot replaced. Most systems allow a 32 character username.
 // The queue name must thus allow for at least that length with the replacement of dots.
 var QueueNameRegExp = regexp.MustCompile(`^[a-zA-Z0-9_:#/@-]{1,64}$`)
 
-// User and group name check: systems allow different things POSIX is the base but we need to be lenient and allow more.
-// A username must start with a letter(uppercase or lowercase),
-// followed by any number of letter(uppercase or lowercase), digits, ':', '#', '/', '_', '.', '@', '-', and may end with '$'.
+// UserRegExp to validate a username. Systems allow different things POSIX is the base, but we need to be lenient and allow more.
+// A username must start with a letter(uppercase or lowercase), followed by any number of letter(uppercase or lowercase),
+// digits, ':', '#', '/', '_', '.', '@', '-', and may end with '$'.
+// This supports OIDC compliant names.
 var UserRegExp = regexp.MustCompile(`^[_a-zA-Z][a-zA-Z0-9:#/_.@-]*[$]?$`)
 
-// Groups should have a slightly more restrictive regexp (no # / @ or $ at the end)
+// GroupRegExp is similar to UserRegExp, groups have a slightly more restrictive regexp (no # / @ or $ at the end)
 var GroupRegExp = regexp.MustCompile(`^[_a-zA-Z][a-zA-Z0-9:_.-]*$`)
 
-// all characters that make a name different from a regexp
+// SpecialRegExp matches all characters that make a name different from a regexp
 var SpecialRegExp = regexp.MustCompile(`[\^$*+?()\[{}|]`)
 
-// The rule maps to a go identifier check that regexp only
+// RuleNameRegExp as rule names must map to a go identifier check that regexp only
 var RuleNameRegExp = regexp.MustCompile(`^[_a-zA-Z][a-zA-Z0-9_]*$`)
 
-// Minimum Quota change preemption delay
-var minQuotaChangePreemptionDelay uint64 = 60
+type placementPathCheckResult int
 
 type placementStaticPath struct {
 	path           string
@@ -668,9 +674,6 @@ func checkQueues(queue *QueueConfig, level int) error {
 			return fmt.Errorf("duplicate child name found with name '%s', level %d", child.Name, level)
 		}
 		queueMap[strings.ToLower(child.Name)] = true
-		if queue.Preemption.Delay != 0 && queue.Preemption.Delay <= minQuotaChangePreemptionDelay {
-			return fmt.Errorf("invalid preemption delay %d, must be between %d and %d", queue.Preemption.Delay, minQuotaChangePreemptionDelay, uint64(math.MaxUint64))
-		}
 	}
 
 	// recurse into the depth if this level passed

pkg/common/configs/configvalidator_test.go

@@ -2327,34 +2327,6 @@ func TestCheckQueues(t *testing.T) { //nolint:funlen
 				assert.Equal(t, 2, len(q.Queues), "Expected two queues")
 			},
 		},
-		{
-			name: "Invalid Preemption delay for leaf queue",
-			queue: &QueueConfig{
-				Name: "root",
-				Queues: []QueueConfig{
-					{Name: "leaf",
-						Preemption: Preemption{
-							Delay: 10,
-						},
-					},
-				},
-				Preemption: Preemption{
-					Delay: 10,
-				},
-			},
-			level:            1,
-			expectedErrorMsg: "invalid preemption delay 10, must be between 60 and 18446744073709551615",
-		},
-		{
-			name: "Setting Preemption delay on root queue would be ignored",
-			queue: &QueueConfig{
-				Name: "root",
-				Preemption: Preemption{
-					Delay: 10,
-				},
-			},
-			level: 0,
-		},
 	}
 
 	for _, tc := range testCases {

pkg/scheduler/objects/preemption_utilities_test.go

@@ -158,7 +158,7 @@ func resetQueue(queue *Queue) {
 	queue.maxResource = nil
 	queue.allocatedResource = nil
 	queue.guaranteedResource = nil
-	queue.isQuotaChangePreemptionRunning = false
+	queue.isQuotaPreemptionRunning = false
 	queue.preemptingResource = nil
 }