From f9ac7a58a7a3e61439b72e343b40ed443a180e42 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Wed, 16 Aug 2023 11:05:59 +0200 Subject: [PATCH 01/26] visibility for cluster queue --- apis/config/v1beta1/configuration_types.go | 42 +++++++++++ apis/config/v1beta1/defaults.go | 27 ++++--- apis/config/v1beta1/defaults_test.go | 42 ++++++++++- apis/config/v1beta1/zz_generated.deepcopy.go | 70 +++++++++++++++++++ apis/kueue/v1beta1/clusterqueue_types.go | 27 +++++++ apis/kueue/v1beta1/zz_generated.deepcopy.go | 41 +++++++++++ .../crd/kueue.x-k8s.io_clusterqueues.yaml | 35 ++++++++++ charts/kueue/values.yaml | 5 ++ cmd/kueue/main_test.go | 3 + .../bases/kueue.x-k8s.io_clusterqueues.yaml | 35 ++++++++++ .../manager/controller_manager_config.yaml | 5 ++ pkg/config/config_test.go | 67 +++++++++++++++++- 12 files changed, 385 insertions(+), 14 deletions(-) diff --git a/apis/config/v1beta1/configuration_types.go b/apis/config/v1beta1/configuration_types.go index 65b2cccba9..bfbf06872a 100644 --- a/apis/config/v1beta1/configuration_types.go +++ b/apis/config/v1beta1/configuration_types.go @@ -62,6 +62,10 @@ type Configuration struct { // Integrations provide configuration options for AI/ML/Batch frameworks // integrations (including K8S job). Integrations *Integrations `json:"integrations,omitempty"` + + // QueueVisibility is configuration to expose the information about the top + // pending workloads. + QueueVisibility *QueueVisibility `json:"queueVisibility,omitempty"` } type ControllerManager struct { @@ -226,3 +230,41 @@ type Integrations struct { // - "kubeflow.org/tfjob" Frameworks []string `json:"frameworks,omitempty"` } + +type QueueVisibility struct { + // LocalQueues is configuration to expose the information + // about the top pending workloads in the local queue. + LocalQueues *LocalQueueVisibility `json:"localQueues,omitempty"` + + // ClusterQueues is configuration to expose the information + // about the top pending workloads in the cluster queue. + ClusterQueues *ClusterQueueVisibility `json:"clusterQueues,omitempty"` + + // UpdateInterval specifies the time interval for updates to the structure + // of the top pending workloads in the queues. + // Defaults to 5s. + // +optional + UpdateInterval *metav1.Duration `json:"updateInterval,omitempty"` +} + +type LocalQueueVisibility struct { + // MaxCount indicates the maximal number of pending workloads exposed in the + // local queue status. When the value is set to 0, then LocalQueue visibility + // updates are disabled. + // The maximal value is 4000. + // Defaults to 10. + MaxCount int32 `json:"maxCount,omitempty"` + + // MaxPosition indicates the maximal position of the workload in the cluster + // queue returned in the head. + MaxPosition *int32 `json:"maxPosition,omitempty"` +} + +type ClusterQueueVisibility struct { + // MaxCount indicates the maximal number of pending workloads exposed in the + // cluster queue status. When the value is set to 0, then LocalQueue + // visibility updates are disabled. + // The maximal value is 4000. + // Defaults to 10. + MaxCount int32 `json:"maxCount,omitempty"` +} diff --git a/apis/config/v1beta1/defaults.go b/apis/config/v1beta1/defaults.go index 02336f2dcd..f5a2e785c6 100644 --- a/apis/config/v1beta1/defaults.go +++ b/apis/config/v1beta1/defaults.go @@ -29,16 +29,17 @@ import ( ) const ( - DefaultNamespace = "kueue-system" - DefaultWebhookServiceName = "kueue-webhook-service" - DefaultWebhookSecretName = "kueue-webhook-server-cert" - DefaultWebhookPort = 9443 - DefaultHealthProbeBindAddress = ":8081" - DefaultMetricsBindAddress = ":8080" - DefaultLeaderElectionID = "c1f6bfd2.kueue.x-k8s.io" - DefaultClientConnectionQPS float32 = 20.0 - DefaultClientConnectionBurst int32 = 30 - defaultPodsReadyTimeout = 5 * time.Minute + DefaultNamespace = "kueue-system" + DefaultWebhookServiceName = "kueue-webhook-service" + DefaultWebhookSecretName = "kueue-webhook-server-cert" + DefaultWebhookPort = 9443 + DefaultHealthProbeBindAddress = ":8081" + DefaultMetricsBindAddress = ":8080" + DefaultLeaderElectionID = "c1f6bfd2.kueue.x-k8s.io" + DefaultClientConnectionQPS float32 = 20.0 + DefaultClientConnectionBurst int32 = 30 + defaultPodsReadyTimeout = 5 * time.Minute + DefaultQueueVisibilityUpdateInterval = 5 * time.Second ) func addDefaultingFuncs(scheme *runtime.Scheme) error { @@ -116,4 +117,10 @@ func SetDefaults_Configuration(cfg *Configuration) { if cfg.Integrations.Frameworks == nil { cfg.Integrations.Frameworks = []string{job.FrameworkName} } + if cfg.QueueVisibility == nil { + cfg.QueueVisibility = &QueueVisibility{} + } + if cfg.QueueVisibility.UpdateInterval == nil { + cfg.QueueVisibility.UpdateInterval = &metav1.Duration{Duration: DefaultQueueVisibilityUpdateInterval} + } } diff --git a/apis/config/v1beta1/defaults_test.go b/apis/config/v1beta1/defaults_test.go index 0c79f73dce..c75ba0427f 100644 --- a/apis/config/v1beta1/defaults_test.go +++ b/apis/config/v1beta1/defaults_test.go @@ -55,6 +55,9 @@ func TestSetDefaults_Configuration(t *testing.T) { defaultIntegrations := &Integrations{ Frameworks: []string{job.FrameworkName}, } + defaultQueueVisibility := &QueueVisibility{ + UpdateInterval: &metav1.Duration{Duration: DefaultQueueVisibilityUpdateInterval}, + } podsReadyTimeoutTimeout := metav1.Duration{Duration: defaultPodsReadyTimeout} podsReadyTimeoutOverwrite := metav1.Duration{Duration: time.Minute} @@ -76,6 +79,7 @@ func TestSetDefaults_Configuration(t *testing.T) { }, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, }, "defaulting ControllerManager": { @@ -111,6 +115,7 @@ func TestSetDefaults_Configuration(t *testing.T) { }, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, }, "should not default ControllerManager": { @@ -133,7 +138,8 @@ func TestSetDefaults_Configuration(t *testing.T) { InternalCertManagement: &InternalCertManagement{ Enable: ptr.To(false), }, - Integrations: defaultIntegrations, + Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, want: &Configuration{ Namespace: ptr.To(DefaultNamespace), @@ -157,6 +163,7 @@ func TestSetDefaults_Configuration(t *testing.T) { }, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, }, "should not set LeaderElectionID": { @@ -191,6 +198,7 @@ func TestSetDefaults_Configuration(t *testing.T) { }, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, }, "defaulting InternalCertManagement": { @@ -207,6 +215,7 @@ func TestSetDefaults_Configuration(t *testing.T) { }, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, }, "should not default InternalCertManagement": { @@ -224,6 +233,7 @@ func TestSetDefaults_Configuration(t *testing.T) { }, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, }, "should not default values in custom ClientConnection": { @@ -247,7 +257,8 @@ func TestSetDefaults_Configuration(t *testing.T) { QPS: ptr.To[float32](123.0), Burst: ptr.To[int32](456), }, - Integrations: defaultIntegrations, + Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, }, "should default empty custom ClientConnection": { @@ -266,6 +277,7 @@ func TestSetDefaults_Configuration(t *testing.T) { }, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, }, "defaulting waitForPodsReady.timeout": { @@ -290,6 +302,7 @@ func TestSetDefaults_Configuration(t *testing.T) { }, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, }, "set waitForPodsReady.blockAdmission to false when enable is false": { @@ -314,6 +327,7 @@ func TestSetDefaults_Configuration(t *testing.T) { }, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, }, "respecting provided waitForPodsReady.timeout": { @@ -339,6 +353,7 @@ func TestSetDefaults_Configuration(t *testing.T) { }, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, }, "integrations": { @@ -360,6 +375,29 @@ func TestSetDefaults_Configuration(t *testing.T) { Integrations: &Integrations{ Frameworks: []string{"a", "b"}, }, + QueueVisibility: defaultQueueVisibility, + }, + }, + "queue visibility": { + original: &Configuration{ + InternalCertManagement: &InternalCertManagement{ + Enable: ptr.To(false), + }, + QueueVisibility: &QueueVisibility{ + UpdateInterval: &metav1.Duration{Duration: 10 * time.Second}, + }, + }, + want: &Configuration{ + Namespace: ptr.To(DefaultNamespace), + ControllerManager: defaultCtrlManagerConfigurationSpec, + InternalCertManagement: &InternalCertManagement{ + Enable: ptr.To(false), + }, + ClientConnection: defaultClientConnection, + Integrations: defaultIntegrations, + QueueVisibility: &QueueVisibility{ + UpdateInterval: &metav1.Duration{Duration: 10 * time.Second}, + }, }, }, } diff --git a/apis/config/v1beta1/zz_generated.deepcopy.go b/apis/config/v1beta1/zz_generated.deepcopy.go index 12bc37a5a6..b94a986d5a 100644 --- a/apis/config/v1beta1/zz_generated.deepcopy.go +++ b/apis/config/v1beta1/zz_generated.deepcopy.go @@ -53,6 +53,21 @@ func (in *ClientConnection) DeepCopy() *ClientConnection { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ClusterQueueVisibility) DeepCopyInto(out *ClusterQueueVisibility) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClusterQueueVisibility. +func (in *ClusterQueueVisibility) DeepCopy() *ClusterQueueVisibility { + if in == nil { + return nil + } + out := new(ClusterQueueVisibility) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Configuration) DeepCopyInto(out *Configuration) { *out = *in @@ -83,6 +98,11 @@ func (in *Configuration) DeepCopyInto(out *Configuration) { *out = new(Integrations) (*in).DeepCopyInto(*out) } + if in.QueueVisibility != nil { + in, out := &in.QueueVisibility, &out.QueueVisibility + *out = new(QueueVisibility) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Configuration. @@ -258,6 +278,56 @@ func (in *InternalCertManagement) DeepCopy() *InternalCertManagement { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *LocalQueueVisibility) DeepCopyInto(out *LocalQueueVisibility) { + *out = *in + if in.MaxPosition != nil { + in, out := &in.MaxPosition, &out.MaxPosition + *out = new(int32) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LocalQueueVisibility. +func (in *LocalQueueVisibility) DeepCopy() *LocalQueueVisibility { + if in == nil { + return nil + } + out := new(LocalQueueVisibility) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *QueueVisibility) DeepCopyInto(out *QueueVisibility) { + *out = *in + if in.LocalQueues != nil { + in, out := &in.LocalQueues, &out.LocalQueues + *out = new(LocalQueueVisibility) + (*in).DeepCopyInto(*out) + } + if in.ClusterQueues != nil { + in, out := &in.ClusterQueues, &out.ClusterQueues + *out = new(ClusterQueueVisibility) + **out = **in + } + if in.UpdateInterval != nil { + in, out := &in.UpdateInterval, &out.UpdateInterval + *out = new(v1.Duration) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new QueueVisibility. +func (in *QueueVisibility) DeepCopy() *QueueVisibility { + if in == nil { + return nil + } + out := new(QueueVisibility) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *WaitForPodsReady) DeepCopyInto(out *WaitForPodsReady) { *out = *in diff --git a/apis/kueue/v1beta1/clusterqueue_types.go b/apis/kueue/v1beta1/clusterqueue_types.go index 22bdaa73a5..c0b9333be9 100644 --- a/apis/kueue/v1beta1/clusterqueue_types.go +++ b/apis/kueue/v1beta1/clusterqueue_types.go @@ -180,6 +180,28 @@ type ResourceQuota struct { // ResourceFlavorReference is the name of the ResourceFlavor. type ResourceFlavorReference string +// ClusterQueuePendingWorkload contains the information identifying a pending workload +// in the cluster queue. +type ClusterQueuePendingWorkload struct { + // Name indicates the name of the pending workload. + Name string `json:"name"` + + // Namespace indicates the name of the pending workload. + Namespace string `json:"namespace"` +} + +type ClusterQueuePendingWorkloadsStatus struct { + // Head contains the list of top pending workloads. + // +listType=map + // +listMapKey=name + // +listMapKey=namespace + // +optional + Head []ClusterQueuePendingWorkload `json:"clusterQueuePendingWorkload"` + + // LastChangeTime indicates the time of the last change of the structure. + LastChangeTime metav1.Time `json:"lastChangeTime"` +} + // ClusterQueueStatus defines the observed state of ClusterQueue type ClusterQueueStatus struct { // flavorsUsage are the used quotas, by flavor, currently in use by the @@ -208,6 +230,11 @@ type ClusterQueueStatus struct { // +patchStrategy=merge // +patchMergeKey=type Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` + + // PendingWorkloadsStatus contains the information exposed about the current + // status of the pending workloads in the cluster queue. + // +optional + PendingWorkloadsStatus *ClusterQueuePendingWorkloadsStatus `json:"pendingWorkloadsStatus"` } type FlavorUsage struct { diff --git a/apis/kueue/v1beta1/zz_generated.deepcopy.go b/apis/kueue/v1beta1/zz_generated.deepcopy.go index a77fdee540..ce250d0d02 100644 --- a/apis/kueue/v1beta1/zz_generated.deepcopy.go +++ b/apis/kueue/v1beta1/zz_generated.deepcopy.go @@ -222,6 +222,42 @@ func (in *ClusterQueueList) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ClusterQueuePendingWorkload) DeepCopyInto(out *ClusterQueuePendingWorkload) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClusterQueuePendingWorkload. +func (in *ClusterQueuePendingWorkload) DeepCopy() *ClusterQueuePendingWorkload { + if in == nil { + return nil + } + out := new(ClusterQueuePendingWorkload) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ClusterQueuePendingWorkloadsStatus) DeepCopyInto(out *ClusterQueuePendingWorkloadsStatus) { + *out = *in + if in.Head != nil { + in, out := &in.Head, &out.Head + *out = make([]ClusterQueuePendingWorkload, len(*in)) + copy(*out, *in) + } + in.LastChangeTime.DeepCopyInto(&out.LastChangeTime) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClusterQueuePendingWorkloadsStatus. +func (in *ClusterQueuePendingWorkloadsStatus) DeepCopy() *ClusterQueuePendingWorkloadsStatus { + if in == nil { + return nil + } + out := new(ClusterQueuePendingWorkloadsStatus) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ClusterQueuePreemption) DeepCopyInto(out *ClusterQueuePreemption) { *out = *in @@ -291,6 +327,11 @@ func (in *ClusterQueueStatus) DeepCopyInto(out *ClusterQueueStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.PendingWorkloadsStatus != nil { + in, out := &in.PendingWorkloadsStatus, &out.PendingWorkloadsStatus + *out = new(ClusterQueuePendingWorkloadsStatus) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClusterQueueStatus. diff --git a/charts/kueue/templates/crd/kueue.x-k8s.io_clusterqueues.yaml b/charts/kueue/templates/crd/kueue.x-k8s.io_clusterqueues.yaml index e5e3ee447d..ca94572e1e 100644 --- a/charts/kueue/templates/crd/kueue.x-k8s.io_clusterqueues.yaml +++ b/charts/kueue/templates/crd/kueue.x-k8s.io_clusterqueues.yaml @@ -440,6 +440,41 @@ spec: waiting to be admitted to this clusterQueue. format: int32 type: integer + pendingWorkloadsStatus: + description: PendingWorkloadsStatus contains the information exposed + about the current status of the pending workloads in the cluster + queue. + properties: + clusterQueuePendingWorkload: + description: Head contains the list of top pending workloads. + items: + description: ClusterQueuePendingWorkload contains the information + identifying a pending workload in the cluster queue. + properties: + name: + description: Name indicates the name of the pending workload. + type: string + namespace: + description: Namespace indicates the name of the pending + workload. + type: string + required: + - name + - namespace + type: object + type: array + x-kubernetes-list-map-keys: + - name + - namespace + x-kubernetes-list-type: map + lastChangeTime: + description: LastChangeTime indicates the time of the last change + of the structure. + format: date-time + type: string + required: + - lastChangeTime + type: object type: object type: object served: true diff --git a/charts/kueue/values.yaml b/charts/kueue/values.yaml index fa88220708..94221b592c 100644 --- a/charts/kueue/values.yaml +++ b/charts/kueue/values.yaml @@ -82,6 +82,11 @@ managerConfig: # enable: false # webhookServiceName: "" # webhookSecretName: "" + queueVisibility: + localQueues: + maxCount: 10 + clusterQueues: + maxCount: 10 integrations: frameworks: - "batch/job" diff --git a/cmd/kueue/main_test.go b/cmd/kueue/main_test.go index dc75621fd4..cd513ef95d 100644 --- a/cmd/kueue/main_test.go +++ b/cmd/kueue/main_test.go @@ -99,6 +99,9 @@ integrations: // therefore the batch/framework should be registered Frameworks: []string{job.FrameworkName}, }, + QueueVisibility: &config.QueueVisibility{ + UpdateInterval: &metav1.Duration{Duration: config.DefaultQueueVisibilityUpdateInterval}, + }, }, }, { diff --git a/config/components/crd/bases/kueue.x-k8s.io_clusterqueues.yaml b/config/components/crd/bases/kueue.x-k8s.io_clusterqueues.yaml index dbca92eedd..67a5b7079c 100644 --- a/config/components/crd/bases/kueue.x-k8s.io_clusterqueues.yaml +++ b/config/components/crd/bases/kueue.x-k8s.io_clusterqueues.yaml @@ -427,6 +427,41 @@ spec: waiting to be admitted to this clusterQueue. format: int32 type: integer + pendingWorkloadsStatus: + description: PendingWorkloadsStatus contains the information exposed + about the current status of the pending workloads in the cluster + queue. + properties: + clusterQueuePendingWorkload: + description: Head contains the list of top pending workloads. + items: + description: ClusterQueuePendingWorkload contains the information + identifying a pending workload in the cluster queue. + properties: + name: + description: Name indicates the name of the pending workload. + type: string + namespace: + description: Namespace indicates the name of the pending + workload. + type: string + required: + - name + - namespace + type: object + type: array + x-kubernetes-list-map-keys: + - name + - namespace + x-kubernetes-list-type: map + lastChangeTime: + description: LastChangeTime indicates the time of the last change + of the structure. + format: date-time + type: string + required: + - lastChangeTime + type: object type: object type: object served: true diff --git a/config/components/manager/controller_manager_config.yaml b/config/components/manager/controller_manager_config.yaml index 40a5872c04..7397035d3b 100644 --- a/config/components/manager/controller_manager_config.yaml +++ b/config/components/manager/controller_manager_config.yaml @@ -28,6 +28,11 @@ clientConnection: # enable: false # webhookServiceName: "" # webhookSecretName: "" +queueVisibility: + localQueues: + maxCount: 10 + clusterQueues: + maxCount: 10 integrations: frameworks: - "batch/job" diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index f9daeb57f8..1f143c58e6 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -222,6 +222,19 @@ integrations: `), os.FileMode(0600)); err != nil { t.Fatal(err) } + queueVisibilityConfig := filepath.Join(tmpDir, "queueVisibility.yaml") + if err := os.WriteFile(queueVisibilityConfig, []byte(` +apiVersion: config.kueue.x-k8s.io/v1beta1 +kind: Configuration +queueVisibility: + updateInterval: 10s + localQueues: + maxCount: 10 + clusterQueues: + maxCount: 10 +`), os.FileMode(0600)); err != nil { + t.Fatal(err) + } defaultControlOptions := ctrl.Options{ HealthProbeBindAddress: configapi.DefaultHealthProbeBindAddress, @@ -262,6 +275,10 @@ integrations: Frameworks: []string{job.FrameworkName}, } + defaultQueueVisibility := &configapi.QueueVisibility{ + UpdateInterval: &metav1.Duration{Duration: configapi.DefaultQueueVisibilityUpdateInterval}, + } + testcases := []struct { name string configFile string @@ -277,6 +294,7 @@ integrations: InternalCertManagement: enableDefaultInternalCertManagement, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, wantOptions: ctrl.Options{ HealthProbeBindAddress: configapi.DefaultHealthProbeBindAddress, @@ -312,6 +330,7 @@ integrations: InternalCertManagement: enableDefaultInternalCertManagement, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, wantOptions: defaultControlOptions, }, @@ -328,6 +347,7 @@ integrations: InternalCertManagement: enableDefaultInternalCertManagement, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, wantOptions: ctrl.Options{ HealthProbeBindAddress: ":38081", @@ -358,6 +378,7 @@ integrations: }, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, wantOptions: defaultControlOptions, }, @@ -376,6 +397,7 @@ integrations: }, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, wantOptions: defaultControlOptions, }, @@ -392,6 +414,7 @@ integrations: InternalCertManagement: enableDefaultInternalCertManagement, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, wantOptions: ctrl.Options{ HealthProbeBindAddress: configapi.DefaultHealthProbeBindAddress, @@ -423,6 +446,7 @@ integrations: }, ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, wantOptions: ctrl.Options{ HealthProbeBindAddress: configapi.DefaultHealthProbeBindAddress, @@ -449,7 +473,8 @@ integrations: QPS: ptr.To[float32](50), Burst: ptr.To[int32](100), }, - Integrations: defaultIntegrations, + Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, wantOptions: defaultControlOptions, }, @@ -468,7 +493,8 @@ integrations: QPS: ptr.To[float32](50), Burst: ptr.To[int32](100), }, - Integrations: defaultIntegrations, + Integrations: defaultIntegrations, + QueueVisibility: defaultQueueVisibility, }, wantOptions: ctrl.Options{ HealthProbeBindAddress: configapi.DefaultHealthProbeBindAddress, @@ -515,6 +541,40 @@ integrations: // therefore the batch/framework should be registered Frameworks: []string{job.FrameworkName}, }, + QueueVisibility: defaultQueueVisibility, + }, + wantOptions: ctrl.Options{ + HealthProbeBindAddress: configapi.DefaultHealthProbeBindAddress, + MetricsBindAddress: configapi.DefaultMetricsBindAddress, + WebhookServer: &webhook.DefaultServer{ + Options: webhook.Options{ + Port: configapi.DefaultWebhookPort, + }, + }, + }, + }, + { + name: "queue visibility config", + configFile: queueVisibilityConfig, + wantConfiguration: configapi.Configuration{ + TypeMeta: metav1.TypeMeta{ + APIVersion: configapi.GroupVersion.String(), + Kind: "Configuration", + }, + Namespace: ptr.To(configapi.DefaultNamespace), + ManageJobsWithoutQueueName: false, + InternalCertManagement: enableDefaultInternalCertManagement, + ClientConnection: defaultClientConnection, + Integrations: defaultIntegrations, + QueueVisibility: &configapi.QueueVisibility{ + UpdateInterval: &metav1.Duration{Duration: 10 * time.Second}, + LocalQueues: &configapi.LocalQueueVisibility{ + MaxCount: 10, + }, + ClusterQueues: &configapi.ClusterQueueVisibility{ + MaxCount: 10, + }, + }, }, wantOptions: ctrl.Options{ HealthProbeBindAddress: configapi.DefaultHealthProbeBindAddress, @@ -610,6 +670,9 @@ func TestEncode(t *testing.T) { "integrations": map[string]any{ "frameworks": []any{"batch/job"}, }, + "queueVisibility": map[string]any{ + "updateInterval": string("5s"), + }, }, }, } From 507384807ab6b52f0f74440d9a9f2d22ea24d765 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Mon, 21 Aug 2023 21:44:56 +0200 Subject: [PATCH 02/26] UpdateInterval to UpdateIntervalSeconds, remove LocalQueueVisibility structs --- apis/config/v1beta1/configuration_types.go | 23 ++------------ apis/config/v1beta1/defaults.go | 26 ++++++++-------- apis/config/v1beta1/defaults_test.go | 6 ++-- apis/config/v1beta1/zz_generated.deepcopy.go | 30 ------------------- apis/kueue/v1beta1/clusterqueue_types.go | 4 +-- .../crd/kueue.x-k8s.io_clusterqueues.yaml | 5 +--- charts/kueue/values.yaml | 2 -- cmd/kueue/main_test.go | 2 +- .../bases/kueue.x-k8s.io_clusterqueues.yaml | 5 +--- .../manager/controller_manager_config.yaml | 2 -- pkg/config/config_test.go | 13 +++----- 11 files changed, 27 insertions(+), 91 deletions(-) diff --git a/apis/config/v1beta1/configuration_types.go b/apis/config/v1beta1/configuration_types.go index bfbf06872a..d637f01fcb 100644 --- a/apis/config/v1beta1/configuration_types.go +++ b/apis/config/v1beta1/configuration_types.go @@ -232,32 +232,15 @@ type Integrations struct { } type QueueVisibility struct { - // LocalQueues is configuration to expose the information - // about the top pending workloads in the local queue. - LocalQueues *LocalQueueVisibility `json:"localQueues,omitempty"` - // ClusterQueues is configuration to expose the information // about the top pending workloads in the cluster queue. ClusterQueues *ClusterQueueVisibility `json:"clusterQueues,omitempty"` - // UpdateInterval specifies the time interval for updates to the structure + // UpdateIntervalSeconds specifies the time interval for updates to the structure // of the top pending workloads in the queues. + // The minimum value is 1. // Defaults to 5s. - // +optional - UpdateInterval *metav1.Duration `json:"updateInterval,omitempty"` -} - -type LocalQueueVisibility struct { - // MaxCount indicates the maximal number of pending workloads exposed in the - // local queue status. When the value is set to 0, then LocalQueue visibility - // updates are disabled. - // The maximal value is 4000. - // Defaults to 10. - MaxCount int32 `json:"maxCount,omitempty"` - - // MaxPosition indicates the maximal position of the workload in the cluster - // queue returned in the head. - MaxPosition *int32 `json:"maxPosition,omitempty"` + UpdateIntervalSeconds int32 `json:"updateIntervalSeconds,omitempty"` } type ClusterQueueVisibility struct { diff --git a/apis/config/v1beta1/defaults.go b/apis/config/v1beta1/defaults.go index f5a2e785c6..4c2077b1a0 100644 --- a/apis/config/v1beta1/defaults.go +++ b/apis/config/v1beta1/defaults.go @@ -29,17 +29,17 @@ import ( ) const ( - DefaultNamespace = "kueue-system" - DefaultWebhookServiceName = "kueue-webhook-service" - DefaultWebhookSecretName = "kueue-webhook-server-cert" - DefaultWebhookPort = 9443 - DefaultHealthProbeBindAddress = ":8081" - DefaultMetricsBindAddress = ":8080" - DefaultLeaderElectionID = "c1f6bfd2.kueue.x-k8s.io" - DefaultClientConnectionQPS float32 = 20.0 - DefaultClientConnectionBurst int32 = 30 - defaultPodsReadyTimeout = 5 * time.Minute - DefaultQueueVisibilityUpdateInterval = 5 * time.Second + DefaultNamespace = "kueue-system" + DefaultWebhookServiceName = "kueue-webhook-service" + DefaultWebhookSecretName = "kueue-webhook-server-cert" + DefaultWebhookPort = 9443 + DefaultHealthProbeBindAddress = ":8081" + DefaultMetricsBindAddress = ":8080" + DefaultLeaderElectionID = "c1f6bfd2.kueue.x-k8s.io" + DefaultClientConnectionQPS float32 = 20.0 + DefaultClientConnectionBurst int32 = 30 + defaultPodsReadyTimeout = 5 * time.Minute + DefaultQueueVisibilityUpdateIntervalSeconds = 5 ) func addDefaultingFuncs(scheme *runtime.Scheme) error { @@ -120,7 +120,7 @@ func SetDefaults_Configuration(cfg *Configuration) { if cfg.QueueVisibility == nil { cfg.QueueVisibility = &QueueVisibility{} } - if cfg.QueueVisibility.UpdateInterval == nil { - cfg.QueueVisibility.UpdateInterval = &metav1.Duration{Duration: DefaultQueueVisibilityUpdateInterval} + if cfg.QueueVisibility.UpdateIntervalSeconds == 0 { + cfg.QueueVisibility.UpdateIntervalSeconds = DefaultQueueVisibilityUpdateIntervalSeconds } } diff --git a/apis/config/v1beta1/defaults_test.go b/apis/config/v1beta1/defaults_test.go index c75ba0427f..a008cd86c2 100644 --- a/apis/config/v1beta1/defaults_test.go +++ b/apis/config/v1beta1/defaults_test.go @@ -56,7 +56,7 @@ func TestSetDefaults_Configuration(t *testing.T) { Frameworks: []string{job.FrameworkName}, } defaultQueueVisibility := &QueueVisibility{ - UpdateInterval: &metav1.Duration{Duration: DefaultQueueVisibilityUpdateInterval}, + UpdateIntervalSeconds: DefaultQueueVisibilityUpdateIntervalSeconds, } podsReadyTimeoutTimeout := metav1.Duration{Duration: defaultPodsReadyTimeout} podsReadyTimeoutOverwrite := metav1.Duration{Duration: time.Minute} @@ -384,7 +384,7 @@ func TestSetDefaults_Configuration(t *testing.T) { Enable: ptr.To(false), }, QueueVisibility: &QueueVisibility{ - UpdateInterval: &metav1.Duration{Duration: 10 * time.Second}, + UpdateIntervalSeconds: 10, }, }, want: &Configuration{ @@ -396,7 +396,7 @@ func TestSetDefaults_Configuration(t *testing.T) { ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, QueueVisibility: &QueueVisibility{ - UpdateInterval: &metav1.Duration{Duration: 10 * time.Second}, + UpdateIntervalSeconds: 10, }, }, }, diff --git a/apis/config/v1beta1/zz_generated.deepcopy.go b/apis/config/v1beta1/zz_generated.deepcopy.go index b94a986d5a..62b9bcde31 100644 --- a/apis/config/v1beta1/zz_generated.deepcopy.go +++ b/apis/config/v1beta1/zz_generated.deepcopy.go @@ -278,44 +278,14 @@ func (in *InternalCertManagement) DeepCopy() *InternalCertManagement { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *LocalQueueVisibility) DeepCopyInto(out *LocalQueueVisibility) { - *out = *in - if in.MaxPosition != nil { - in, out := &in.MaxPosition, &out.MaxPosition - *out = new(int32) - **out = **in - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LocalQueueVisibility. -func (in *LocalQueueVisibility) DeepCopy() *LocalQueueVisibility { - if in == nil { - return nil - } - out := new(LocalQueueVisibility) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *QueueVisibility) DeepCopyInto(out *QueueVisibility) { *out = *in - if in.LocalQueues != nil { - in, out := &in.LocalQueues, &out.LocalQueues - *out = new(LocalQueueVisibility) - (*in).DeepCopyInto(*out) - } if in.ClusterQueues != nil { in, out := &in.ClusterQueues, &out.ClusterQueues *out = new(ClusterQueueVisibility) **out = **in } - if in.UpdateInterval != nil { - in, out := &in.UpdateInterval, &out.UpdateInterval - *out = new(v1.Duration) - **out = **in - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new QueueVisibility. diff --git a/apis/kueue/v1beta1/clusterqueue_types.go b/apis/kueue/v1beta1/clusterqueue_types.go index c0b9333be9..204aeb7929 100644 --- a/apis/kueue/v1beta1/clusterqueue_types.go +++ b/apis/kueue/v1beta1/clusterqueue_types.go @@ -192,9 +192,7 @@ type ClusterQueuePendingWorkload struct { type ClusterQueuePendingWorkloadsStatus struct { // Head contains the list of top pending workloads. - // +listType=map - // +listMapKey=name - // +listMapKey=namespace + // +listType=atomic // +optional Head []ClusterQueuePendingWorkload `json:"clusterQueuePendingWorkload"` diff --git a/charts/kueue/templates/crd/kueue.x-k8s.io_clusterqueues.yaml b/charts/kueue/templates/crd/kueue.x-k8s.io_clusterqueues.yaml index ca94572e1e..3fc4ce14ac 100644 --- a/charts/kueue/templates/crd/kueue.x-k8s.io_clusterqueues.yaml +++ b/charts/kueue/templates/crd/kueue.x-k8s.io_clusterqueues.yaml @@ -463,10 +463,7 @@ spec: - namespace type: object type: array - x-kubernetes-list-map-keys: - - name - - namespace - x-kubernetes-list-type: map + x-kubernetes-list-type: atomic lastChangeTime: description: LastChangeTime indicates the time of the last change of the structure. diff --git a/charts/kueue/values.yaml b/charts/kueue/values.yaml index 94221b592c..06c0345f27 100644 --- a/charts/kueue/values.yaml +++ b/charts/kueue/values.yaml @@ -83,8 +83,6 @@ managerConfig: # webhookServiceName: "" # webhookSecretName: "" queueVisibility: - localQueues: - maxCount: 10 clusterQueues: maxCount: 10 integrations: diff --git a/cmd/kueue/main_test.go b/cmd/kueue/main_test.go index cd513ef95d..3c1e60c100 100644 --- a/cmd/kueue/main_test.go +++ b/cmd/kueue/main_test.go @@ -100,7 +100,7 @@ integrations: Frameworks: []string{job.FrameworkName}, }, QueueVisibility: &config.QueueVisibility{ - UpdateInterval: &metav1.Duration{Duration: config.DefaultQueueVisibilityUpdateInterval}, + UpdateIntervalSeconds: config.DefaultQueueVisibilityUpdateIntervalSeconds, }, }, }, diff --git a/config/components/crd/bases/kueue.x-k8s.io_clusterqueues.yaml b/config/components/crd/bases/kueue.x-k8s.io_clusterqueues.yaml index 67a5b7079c..b8713d5364 100644 --- a/config/components/crd/bases/kueue.x-k8s.io_clusterqueues.yaml +++ b/config/components/crd/bases/kueue.x-k8s.io_clusterqueues.yaml @@ -450,10 +450,7 @@ spec: - namespace type: object type: array - x-kubernetes-list-map-keys: - - name - - namespace - x-kubernetes-list-type: map + x-kubernetes-list-type: atomic lastChangeTime: description: LastChangeTime indicates the time of the last change of the structure. diff --git a/config/components/manager/controller_manager_config.yaml b/config/components/manager/controller_manager_config.yaml index 7397035d3b..3acb7dd8ff 100644 --- a/config/components/manager/controller_manager_config.yaml +++ b/config/components/manager/controller_manager_config.yaml @@ -29,8 +29,6 @@ clientConnection: # webhookServiceName: "" # webhookSecretName: "" queueVisibility: - localQueues: - maxCount: 10 clusterQueues: maxCount: 10 integrations: diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 1f143c58e6..5426700989 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -227,9 +227,7 @@ integrations: apiVersion: config.kueue.x-k8s.io/v1beta1 kind: Configuration queueVisibility: - updateInterval: 10s - localQueues: - maxCount: 10 + updateIntervalSeconds: 10 clusterQueues: maxCount: 10 `), os.FileMode(0600)); err != nil { @@ -276,7 +274,7 @@ queueVisibility: } defaultQueueVisibility := &configapi.QueueVisibility{ - UpdateInterval: &metav1.Duration{Duration: configapi.DefaultQueueVisibilityUpdateInterval}, + UpdateIntervalSeconds: configapi.DefaultQueueVisibilityUpdateIntervalSeconds, } testcases := []struct { @@ -567,10 +565,7 @@ queueVisibility: ClientConnection: defaultClientConnection, Integrations: defaultIntegrations, QueueVisibility: &configapi.QueueVisibility{ - UpdateInterval: &metav1.Duration{Duration: 10 * time.Second}, - LocalQueues: &configapi.LocalQueueVisibility{ - MaxCount: 10, - }, + UpdateIntervalSeconds: 10, ClusterQueues: &configapi.ClusterQueueVisibility{ MaxCount: 10, }, @@ -671,7 +666,7 @@ func TestEncode(t *testing.T) { "frameworks": []any{"batch/job"}, }, "queueVisibility": map[string]any{ - "updateInterval": string("5s"), + "updateIntervalSeconds": int64(configapi.DefaultQueueVisibilityUpdateIntervalSeconds), }, }, }, From 3ca5bb7f47fa104c556e47a1099d562a9821951b Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Thu, 24 Aug 2023 13:44:07 +0200 Subject: [PATCH 03/26] taking snapshot of cluster queue --- cmd/kueue/main.go | 20 ++++++- pkg/config/config.go | 1 + pkg/config/config_test.go | 42 ++++++++++++++ pkg/config/validation.go | 31 ++++++++++ .../core/clusterqueue_controller.go | 1 + .../core/clusterqueue_controller_test.go | 2 +- pkg/queue/cluster_queue_impl.go | 18 ++++++ pkg/queue/cluster_queue_interface.go | 1 + pkg/queue/manager.go | 58 ++++++++++++++++++- pkg/queue/manager_test.go | 24 ++++---- pkg/scheduler/scheduler_test.go | 4 +- .../integration/controller/core/suite_test.go | 2 +- .../controller/jobs/job/suite_test.go | 2 +- .../controller/jobs/jobset/suite_test.go | 2 +- .../controller/jobs/mpijob/suite_test.go | 2 +- .../controller/jobs/pytorchjob/suite_test.go | 2 +- .../controller/jobs/rayjob/suite_test.go | 2 +- .../controller/jobs/tfjob/suite_test.go | 2 +- .../scheduler/podsready/suite_test.go | 2 +- test/integration/scheduler/suite_test.go | 2 +- test/integration/webhook/suite_test.go | 2 +- 21 files changed, 194 insertions(+), 28 deletions(-) create mode 100644 pkg/config/validation.go diff --git a/cmd/kueue/main.go b/cmd/kueue/main.go index 5ef0d3ad9e..04f70f79de 100644 --- a/cmd/kueue/main.go +++ b/cmd/kueue/main.go @@ -25,6 +25,7 @@ import ( // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) // to ensure that exec-entrypoint and run can make use of them. + _ "k8s.io/client-go/plugin/pkg/client/auth" zaplog "go.uber.org/zap" @@ -146,7 +147,7 @@ func main() { } cCache := cache.New(mgr.GetClient(), cache.WithPodsReadyTracking(blockForPodsReady(&cfg))) - queues := queue.NewManager(mgr.GetClient(), cCache) + queues := setupManager(mgr, cCache, &cfg) ctx := ctrl.SetupSignalHandler() if err := setupIndexes(ctx, mgr, &cfg); err != nil { @@ -178,6 +179,15 @@ func main() { } } +func setupManager(mgr ctrl.Manager, cCache *cache.Cache, cfg *configapi.Configuration) *queue.Manager { + queues := queue.NewManager(mgr.GetClient(), cCache, cfg) + if err := mgr.Add(queues); err != nil { + setupLog.Error(err, "Unable to add queue manager to manager") + os.Exit(1) + } + return queues +} + func setupIndexes(ctx context.Context, mgr ctrl.Manager, cfg *configapi.Configuration) error { err := indexer.Setup(ctx, mgr.GetFieldIndexer()) if err != nil { @@ -195,7 +205,13 @@ func setupIndexes(ctx context.Context, mgr ctrl.Manager, cfg *configapi.Configur return err } -func setupControllers(mgr ctrl.Manager, cCache *cache.Cache, queues *queue.Manager, certsReady chan struct{}, cfg *configapi.Configuration, serverVersionFetcher *kubeversion.ServerVersionFetcher) { +func setupControllers( + mgr ctrl.Manager, + cCache *cache.Cache, + queues *queue.Manager, + certsReady chan struct{}, + cfg *configapi.Configuration, + serverVersionFetcher *kubeversion.ServerVersionFetcher) { // The controllers won't work until the webhooks are operating, and the webhook won't work until the // certs are all in place. setupLog.Info("Waiting for certificate generation to complete") diff --git a/pkg/config/config.go b/pkg/config/config.go index c8d9f7b51e..85eff7a764 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -163,5 +163,6 @@ func Load(scheme *runtime.Scheme, configFile string) (ctrl.Options, configapi.Co } } addTo(&options, &cfg) + err = validate(&cfg) return options, cfg, err } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 5426700989..fc903fbf0d 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -688,3 +688,45 @@ func TestEncode(t *testing.T) { }) } } + +func TestValidate(t *testing.T) { + testcases := []struct { + name string + cfg *configapi.Configuration + wantErr error + }{ + + { + name: "empty", + cfg: &configapi.Configuration{}, + wantErr: nil, + }, + { + name: "invalid queue visibility UpdateIntervalSeconds", + cfg: &configapi.Configuration{ + QueueVisibility: &configapi.QueueVisibility{ + UpdateIntervalSeconds: 0, + }, + }, + wantErr: errInvalidUpdateIntervalSeconds, + }, + { + name: "invalid queue visibility cluster queue max count", + cfg: &configapi.Configuration{ + QueueVisibility: &configapi.QueueVisibility{ + ClusterQueues: &configapi.ClusterQueueVisibility{ + MaxCount: 4001, + }, + }, + }, + wantErr: errInvalidMaxValue, + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + if diff := cmp.Diff(tc.wantErr, validate(tc.cfg), cmpopts.EquateErrors()); diff != "" { + t.Errorf("Unexpected returned error (-want,+got):\n%s", diff) + } + }) + } +} diff --git a/pkg/config/validation.go b/pkg/config/validation.go new file mode 100644 index 0000000000..cec880f62e --- /dev/null +++ b/pkg/config/validation.go @@ -0,0 +1,31 @@ +package config + +import ( + "fmt" + + configapi "sigs.k8s.io/kueue/apis/config/v1beta1" +) + +var ( + errInvalidMaxValue = fmt.Errorf("maximum value for QueueVisibility.ClusterQueues.MaxCount must be %d", queueVisibilityClusterQueuesMaxValue) + errInvalidUpdateIntervalSeconds = fmt.Errorf("minimum value for QueueVisibility.UpdateIntervalSeconds must be %d", queueVisibilityClusterQueuesUpdateIntervalSeconds) +) + +const ( + queueVisibilityClusterQueuesMaxValue = 4000 + queueVisibilityClusterQueuesUpdateIntervalSeconds = 1 +) + +func validate(cfg *configapi.Configuration) error { + if cfg.QueueVisibility != nil { + if cfg.QueueVisibility.ClusterQueues != nil { + if cfg.QueueVisibility.ClusterQueues.MaxCount > queueVisibilityClusterQueuesMaxValue { + return errInvalidMaxValue + } + } + if cfg.QueueVisibility.UpdateIntervalSeconds < queueVisibilityClusterQueuesUpdateIntervalSeconds { + return errInvalidUpdateIntervalSeconds + } + } + return nil +} diff --git a/pkg/controller/core/clusterqueue_controller.go b/pkg/controller/core/clusterqueue_controller.go index 1af0fbe9b2..974d92f22f 100644 --- a/pkg/controller/core/clusterqueue_controller.go +++ b/pkg/controller/core/clusterqueue_controller.go @@ -486,6 +486,7 @@ func (r *ClusterQueueReconciler) updateCqStatusIfChanged( cq.Status.FlavorsUsage = usage cq.Status.AdmittedWorkloads = int32(workloads) cq.Status.PendingWorkloads = int32(pendingWorkloads) + cq.Status.PendingWorkloadsStatus = r.qManager.GetWorkloadsStatus() meta.SetStatusCondition(&cq.Status.Conditions, metav1.Condition{ Type: kueue.ClusterQueueActive, Status: conditionStatus, diff --git a/pkg/controller/core/clusterqueue_controller_test.go b/pkg/controller/core/clusterqueue_controller_test.go index 4a28c43cc2..a446595ec0 100644 --- a/pkg/controller/core/clusterqueue_controller_test.go +++ b/pkg/controller/core/clusterqueue_controller_test.go @@ -174,7 +174,7 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { cl := utiltesting.NewClientBuilder().WithLists(defaultWls).WithObjects(lq, cq).WithStatusSubresource(lq, cq). Build() cqCache := cache.New(cl) - qManager := queue.NewManager(cl, cqCache) + qManager := queue.NewManager(cl, cqCache, nil) if err := cqCache.AddClusterQueue(ctx, cq); err != nil { t.Fatalf("Inserting clusterQueue in cache: %v", err) } diff --git a/pkg/queue/cluster_queue_impl.go b/pkg/queue/cluster_queue_impl.go index 0769dc13f6..b3a2714fee 100644 --- a/pkg/queue/cluster_queue_impl.go +++ b/pkg/queue/cluster_queue_impl.go @@ -210,6 +210,24 @@ func (c *clusterQueueBase) Dump() (sets.Set[string], bool) { return elements, true } +func (c *clusterQueueBase) Snapshot(maxCount int32) ([]*kueue.Workload, bool) { + if c.heap.Len() == 0 || maxCount == 0 { + return nil, false + } + elements := make([]*kueue.Workload, c.heap.Len()) + for i, e := range c.heap.List() { + if maxCount >= int32(i) { + return elements, true + } + info := e.(*workload.Info) + elements = append(elements, info.Obj) + } + for _, info := range c.inadmissibleWorkloads { + elements = append(elements, info.Obj) + } + return elements, true +} + func (c *clusterQueueBase) DumpInadmissible() (sets.Set[string], bool) { if len(c.inadmissibleWorkloads) == 0 { return nil, false diff --git a/pkg/queue/cluster_queue_interface.go b/pkg/queue/cluster_queue_interface.go index ab611f987e..badc25cfab 100644 --- a/pkg/queue/cluster_queue_interface.go +++ b/pkg/queue/cluster_queue_interface.go @@ -92,6 +92,7 @@ type ClusterQueue interface { // Otherwise returns true. Dump() (sets.Set[string], bool) DumpInadmissible() (sets.Set[string], bool) + Snapshot(maxCount int32) ([]*kueue.Workload, bool) // Info returns workload.Info for the workload key. // Users of this method should not modify the returned object. Info(string) *workload.Info diff --git a/pkg/queue/manager.go b/pkg/queue/manager.go index 5c9e4e8b33..8679f532c2 100644 --- a/pkg/queue/manager.go +++ b/pkg/queue/manager.go @@ -21,12 +21,15 @@ import ( "errors" "fmt" "sync" + "time" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + configapi "sigs.k8s.io/kueue/apis/config/v1beta1" kueue "sigs.k8s.io/kueue/apis/kueue/v1beta1" utilindexer "sigs.k8s.io/kueue/pkg/controller/core/indexer" "sigs.k8s.io/kueue/pkg/metrics" @@ -48,14 +51,18 @@ type Manager struct { clusterQueues map[string]ClusterQueue localQueues map[string]*LocalQueue + workloadsStatus *kueue.ClusterQueuePendingWorkloadsStatus + cfg *configapi.Configuration + // Key is cohort's name. Value is a set of associated ClusterQueue names. cohorts map[string]sets.Set[string] } -func NewManager(client client.Client, checker StatusChecker) *Manager { +func NewManager(client client.Client, checker StatusChecker, cfg *configapi.Configuration) *Manager { m := &Manager{ client: client, statusChecker: checker, + cfg: cfg, localQueues: make(map[string]*LocalQueue), clusterQueues: make(map[string]ClusterQueue), cohorts: make(map[string]sets.Set[string]), @@ -469,6 +476,22 @@ func (m *Manager) Dump() map[string]sets.Set[string] { return dump } +// Snapshot is a copy of the queues elements and inadmissible workloads list. +func (m *Manager) Snapshot() []*kueue.Workload { + m.Lock() + defer m.Unlock() + if len(m.clusterQueues) == 0 { + return nil + } + snapshot := make([]*kueue.Workload, 0) + for _, cq := range m.clusterQueues { + if elements, ok := cq.Snapshot(m.cfg.QueueVisibility.ClusterQueues.MaxCount); ok { + snapshot = append(snapshot, elements...) + } + } + return snapshot +} + // DumpInadmissible is a dump of the inadmissible workloads list. // Only use for testing purposes. func (m *Manager) DumpInadmissible() map[string]sets.Set[string] { @@ -547,3 +570,36 @@ func (m *Manager) reportPendingWorkloads(cqName string, cq ClusterQueue) { } metrics.ReportPendingWorkloads(cqName, active, inadmissible) } + +func (m *Manager) Start(ctx context.Context) error { + log := ctrl.LoggerFrom(ctx).WithName("clusterQueueSnapshot") + ctx = ctrl.LoggerInto(ctx, log) + + ticker := time.NewTicker( + time.Duration(m.cfg.QueueVisibility.UpdateIntervalSeconds) * time.Second, + ) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + log.V(2).Info("Context cancelled; stop doing snapshot of cluster queue") + return nil + case t := <-ticker.C: + m.workloadsStatus = &kueue.ClusterQueuePendingWorkloadsStatus{ + Head: make([]kueue.ClusterQueuePendingWorkload, 0), + LastChangeTime: metav1.Time{Time: t}, + } + for _, wl := range m.Snapshot() { + m.workloadsStatus.Head = append(m.workloadsStatus.Head, kueue.ClusterQueuePendingWorkload{ + Name: wl.Name, + Namespace: wl.Namespace, + }) + } + } + } +} + +func (m *Manager) GetWorkloadsStatus() *kueue.ClusterQueuePendingWorkloadsStatus { + return m.workloadsStatus +} diff --git a/pkg/queue/manager_test.go b/pkg/queue/manager_test.go index 2bbcc573a8..484247d3ab 100644 --- a/pkg/queue/manager_test.go +++ b/pkg/queue/manager_test.go @@ -49,7 +49,7 @@ func TestAddLocalQueueOrphans(t *testing.T) { ReserveQuota(utiltesting.MakeAdmission("cq").Obj()).Obj(), utiltesting.MakeWorkload("a", "moon").Queue("foo").Obj(), ) - manager := NewManager(kClient, nil) + manager := NewManager(kClient, nil, nil) q := utiltesting.MakeLocalQueue("foo", "earth").Obj() if err := manager.AddLocalQueue(context.Background(), q); err != nil { t.Fatalf("Failed adding queue: %v", err) @@ -83,7 +83,7 @@ func TestAddClusterQueueOrphans(t *testing.T) { queues[1], ) ctx := context.Background() - manager := NewManager(kClient, nil) + manager := NewManager(kClient, nil, nil) cq := utiltesting.MakeClusterQueue("cq").Obj() if err := manager.AddClusterQueue(ctx, cq); err != nil { t.Fatalf("Failed adding cluster queue %s: %v", cq.Name, err) @@ -139,7 +139,7 @@ func TestUpdateClusterQueue(t *testing.T) { cl := utiltesting.NewFakeClient( &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: defaultNamespace}}, ) - manager := NewManager(cl, nil) + manager := NewManager(cl, nil, nil) for _, cq := range clusterQueues { if err := manager.AddClusterQueue(ctx, cq); err != nil { t.Fatalf("Failed adding clusterQueue %s: %v", cq.Name, err) @@ -204,7 +204,7 @@ func TestUpdateLocalQueue(t *testing.T) { t.Fatalf("Failed adding kueue scheme: %s", err) } ctx := context.Background() - manager := NewManager(utiltesting.NewFakeClient(), nil) + manager := NewManager(utiltesting.NewFakeClient(), nil, nil) for _, cq := range clusterQueues { if err := manager.AddClusterQueue(ctx, cq); err != nil { t.Fatalf("Failed adding clusterQueue %s: %v", cq.Name, err) @@ -248,7 +248,7 @@ func TestDeleteLocalQueue(t *testing.T) { ctx := context.Background() cl := utiltesting.NewFakeClient(wl) - manager := NewManager(cl, nil) + manager := NewManager(cl, nil, nil) if err := manager.AddClusterQueue(ctx, cq); err != nil { t.Fatalf("Could not create ClusterQueue: %v", err) @@ -272,7 +272,7 @@ func TestDeleteLocalQueue(t *testing.T) { } func TestAddWorkload(t *testing.T) { - manager := NewManager(utiltesting.NewFakeClient(), nil) + manager := NewManager(utiltesting.NewFakeClient(), nil, nil) cq := utiltesting.MakeClusterQueue("cq").Obj() if err := manager.AddClusterQueue(context.Background(), cq); err != nil { t.Fatalf("Failed adding clusterQueue %s: %v", cq.Name, err) @@ -390,7 +390,7 @@ func TestStatus(t *testing.T) { }, } - manager := NewManager(utiltesting.NewFakeClient(), nil) + manager := NewManager(utiltesting.NewFakeClient(), nil, nil) for _, q := range queues { if err := manager.AddLocalQueue(ctx, &q); err != nil { t.Errorf("Failed adding queue: %s", err) @@ -500,7 +500,7 @@ func TestRequeueWorkloadStrictFIFO(t *testing.T) { for _, tc := range cases { t.Run(tc.workload.Name, func(t *testing.T) { cl := utiltesting.NewFakeClient() - manager := NewManager(cl, nil) + manager := NewManager(cl, nil, nil) ctx := context.Background() if err := manager.AddClusterQueue(ctx, cq); err != nil { t.Fatalf("Failed adding cluster queue %s: %v", cq.Name, err) @@ -657,7 +657,7 @@ func TestUpdateWorkload(t *testing.T) { } for name, tc := range cases { t.Run(name, func(t *testing.T) { - manager := NewManager(utiltesting.NewFakeClient(), nil) + manager := NewManager(utiltesting.NewFakeClient(), nil, nil) ctx := context.Background() for _, cq := range tc.clusterQueues { if err := manager.AddClusterQueue(ctx, cq); err != nil { @@ -773,7 +773,7 @@ func TestHeads(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), headsTimeout) defer cancel() fakeC := &fakeStatusChecker{} - manager := NewManager(utiltesting.NewFakeClient(), fakeC) + manager := NewManager(utiltesting.NewFakeClient(), fakeC, nil) for _, cq := range clusterQueues { if err := manager.AddClusterQueue(ctx, cq); err != nil { t.Fatalf("Failed adding clusterQueue %s to manager: %v", cq.Name, err) @@ -1006,7 +1006,7 @@ func TestHeadsAsync(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), headsTimeout) defer cancel() client := utiltesting.NewFakeClient(tc.initialObjs...) - manager := NewManager(client, nil) + manager := NewManager(client, nil, nil) go manager.CleanUpOnContext(ctx) tc.op(ctx, manager) heads := manager.Heads(ctx) @@ -1019,7 +1019,7 @@ func TestHeadsAsync(t *testing.T) { // TestHeadsCancelled ensures that the Heads call returns when the context is closed. func TestHeadsCancelled(t *testing.T) { - manager := NewManager(utiltesting.NewFakeClient(), nil) + manager := NewManager(utiltesting.NewFakeClient(), nil, nil) ctx, cancel := context.WithCancel(context.Background()) go func() { cancel() diff --git a/pkg/scheduler/scheduler_test.go b/pkg/scheduler/scheduler_test.go index 9737eb8006..d55ea54ba2 100644 --- a/pkg/scheduler/scheduler_test.go +++ b/pkg/scheduler/scheduler_test.go @@ -1041,7 +1041,7 @@ func TestSchedule(t *testing.T) { recorder := broadcaster.NewRecorder(scheme, corev1.EventSource{Component: constants.AdmissionName}) cqCache := cache.New(cl) - qManager := queue.NewManager(cl, cqCache) + qManager := queue.NewManager(cl, cqCache, nil) // Workloads are loaded into queues or clusterQueues as we add them. for _, q := range allQueues { if err := qManager.AddLocalQueue(ctx, &q); err != nil { @@ -1339,7 +1339,7 @@ func TestRequeueAndUpdate(t *testing.T) { broadcaster := record.NewBroadcaster() recorder := broadcaster.NewRecorder(scheme, corev1.EventSource{Component: constants.AdmissionName}) cqCache := cache.New(cl) - qManager := queue.NewManager(cl, cqCache) + qManager := queue.NewManager(cl, cqCache, nil) scheduler := New(qManager, cqCache, cl, recorder) if err := qManager.AddLocalQueue(ctx, q1); err != nil { t.Fatalf("Inserting queue %s/%s in manager: %v", q1.Namespace, q1.Name, err) diff --git a/test/integration/controller/core/suite_test.go b/test/integration/controller/core/suite_test.go index e3d504d679..487a78614b 100644 --- a/test/integration/controller/core/suite_test.go +++ b/test/integration/controller/core/suite_test.go @@ -73,7 +73,7 @@ func managerSetup(mgr manager.Manager, ctx context.Context) { gomega.Expect(err).ToNot(gomega.HaveOccurred(), "webhook", failedWebhook) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache) + queues := queue.NewManager(mgr.GetClient(), cCache, nil) controllersCfg := &config.Configuration{} controllersCfg.Metrics.EnableClusterQueueResources = true diff --git a/test/integration/controller/jobs/job/suite_test.go b/test/integration/controller/jobs/job/suite_test.go index b10d02a72b..bd33d0282f 100644 --- a/test/integration/controller/jobs/job/suite_test.go +++ b/test/integration/controller/jobs/job/suite_test.go @@ -80,7 +80,7 @@ func managerAndSchedulerSetup(opts ...jobframework.Option) framework.ManagerSetu gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache) + queues := queue.NewManager(mgr.GetClient(), cCache, nil) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/controller/jobs/jobset/suite_test.go b/test/integration/controller/jobs/jobset/suite_test.go index fde5c72188..5ab8f17f8a 100644 --- a/test/integration/controller/jobs/jobset/suite_test.go +++ b/test/integration/controller/jobs/jobset/suite_test.go @@ -79,7 +79,7 @@ func managerAndSchedulerSetup(opts ...jobframework.Option) framework.ManagerSetu gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache) + queues := queue.NewManager(mgr.GetClient(), cCache, nil) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/controller/jobs/mpijob/suite_test.go b/test/integration/controller/jobs/mpijob/suite_test.go index 0bdd34e8cb..8e698b21d7 100644 --- a/test/integration/controller/jobs/mpijob/suite_test.go +++ b/test/integration/controller/jobs/mpijob/suite_test.go @@ -93,7 +93,7 @@ func managerAndSchedulerSetup(opts ...jobframework.Option) framework.ManagerSetu gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache) + queues := queue.NewManager(mgr.GetClient(), cCache, nil) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/controller/jobs/pytorchjob/suite_test.go b/test/integration/controller/jobs/pytorchjob/suite_test.go index e900233490..be0ff2f56a 100644 --- a/test/integration/controller/jobs/pytorchjob/suite_test.go +++ b/test/integration/controller/jobs/pytorchjob/suite_test.go @@ -78,7 +78,7 @@ func managerAndSchedulerSetup(opts ...jobframework.Option) framework.ManagerSetu gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache) + queues := queue.NewManager(mgr.GetClient(), cCache, nil) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/controller/jobs/rayjob/suite_test.go b/test/integration/controller/jobs/rayjob/suite_test.go index 73a185db11..36606c563f 100644 --- a/test/integration/controller/jobs/rayjob/suite_test.go +++ b/test/integration/controller/jobs/rayjob/suite_test.go @@ -79,7 +79,7 @@ func managerAndSchedulerSetup(opts ...jobframework.Option) framework.ManagerSetu gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache) + queues := queue.NewManager(mgr.GetClient(), cCache, nil) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/controller/jobs/tfjob/suite_test.go b/test/integration/controller/jobs/tfjob/suite_test.go index e1f3d99075..ba0e29007d 100644 --- a/test/integration/controller/jobs/tfjob/suite_test.go +++ b/test/integration/controller/jobs/tfjob/suite_test.go @@ -78,7 +78,7 @@ func managerAndSchedulerSetup(opts ...jobframework.Option) framework.ManagerSetu gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache) + queues := queue.NewManager(mgr.GetClient(), cCache, nil) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/scheduler/podsready/suite_test.go b/test/integration/scheduler/podsready/suite_test.go index 546654149c..03d700d157 100644 --- a/test/integration/scheduler/podsready/suite_test.go +++ b/test/integration/scheduler/podsready/suite_test.go @@ -69,7 +69,7 @@ func managerAndSchedulerSetupWithTimeoutAdmission(mgr manager.Manager, ctx conte gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient(), cache.WithPodsReadyTracking(cfg.WaitForPodsReady.Enable && cfg.WaitForPodsReady.BlockAdmission != nil && *cfg.WaitForPodsReady.BlockAdmission)) - queues := queue.NewManager(mgr.GetClient(), cCache) + queues := queue.NewManager(mgr.GetClient(), cCache, nil) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &cfg) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/scheduler/suite_test.go b/test/integration/scheduler/suite_test.go index 29fbba3211..1761966099 100644 --- a/test/integration/scheduler/suite_test.go +++ b/test/integration/scheduler/suite_test.go @@ -73,7 +73,7 @@ func managerAndSchedulerSetup(mgr manager.Manager, ctx context.Context) { gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache) + queues := queue.NewManager(mgr.GetClient(), cCache, nil) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/webhook/suite_test.go b/test/integration/webhook/suite_test.go index 3c3b26d437..0b965d0c49 100644 --- a/test/integration/webhook/suite_test.go +++ b/test/integration/webhook/suite_test.go @@ -65,7 +65,7 @@ var _ = ginkgo.BeforeSuite(func() { gomega.Expect(err).ToNot(gomega.HaveOccurred(), "webhook", failedWebhook) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache) + queues := queue.NewManager(mgr.GetClient(), cCache, nil) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) }) From 0c137f637968f5b93e8246d7a306897518abbda4 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Fri, 25 Aug 2023 13:52:45 +0200 Subject: [PATCH 04/26] fix some review issues --- apis/config/v1beta1/defaults.go | 2 +- cmd/kueue/main.go | 19 +++++--- pkg/config/config.go | 2 +- pkg/config/config_test.go | 15 ++++-- pkg/config/validation.go | 18 ++++---- .../core/clusterqueue_controller_test.go | 2 +- pkg/queue/cluster_queue_interface.go | 3 ++ pkg/queue/manager.go | 46 ++++++++++++++++--- pkg/queue/manager_test.go | 24 +++++----- pkg/scheduler/scheduler_test.go | 4 +- .../integration/controller/core/suite_test.go | 2 +- .../controller/jobs/job/suite_test.go | 2 +- .../controller/jobs/jobset/suite_test.go | 2 +- .../controller/jobs/mpijob/suite_test.go | 2 +- .../controller/jobs/pytorchjob/suite_test.go | 2 +- .../controller/jobs/rayjob/suite_test.go | 2 +- .../controller/jobs/tfjob/suite_test.go | 2 +- .../scheduler/podsready/suite_test.go | 2 +- test/integration/scheduler/suite_test.go | 2 +- test/integration/webhook/suite_test.go | 2 +- 20 files changed, 102 insertions(+), 53 deletions(-) diff --git a/apis/config/v1beta1/defaults.go b/apis/config/v1beta1/defaults.go index 4c2077b1a0..1d46400d4f 100644 --- a/apis/config/v1beta1/defaults.go +++ b/apis/config/v1beta1/defaults.go @@ -39,7 +39,7 @@ const ( DefaultClientConnectionQPS float32 = 20.0 DefaultClientConnectionBurst int32 = 30 defaultPodsReadyTimeout = 5 * time.Minute - DefaultQueueVisibilityUpdateIntervalSeconds = 5 + DefaultQueueVisibilityUpdateIntervalSeconds int32 = 5 ) func addDefaultingFuncs(scheme *runtime.Scheme) error { diff --git a/cmd/kueue/main.go b/cmd/kueue/main.go index 04f70f79de..e521999180 100644 --- a/cmd/kueue/main.go +++ b/cmd/kueue/main.go @@ -147,7 +147,7 @@ func main() { } cCache := cache.New(mgr.GetClient(), cache.WithPodsReadyTracking(blockForPodsReady(&cfg))) - queues := setupManager(mgr, cCache, &cfg) + queues := setupQueueManager(mgr, cCache, &cfg) ctx := ctrl.SetupSignalHandler() if err := setupIndexes(ctx, mgr, &cfg); err != nil { @@ -179,11 +179,18 @@ func main() { } } -func setupManager(mgr ctrl.Manager, cCache *cache.Cache, cfg *configapi.Configuration) *queue.Manager { - queues := queue.NewManager(mgr.GetClient(), cCache, cfg) - if err := mgr.Add(queues); err != nil { - setupLog.Error(err, "Unable to add queue manager to manager") - os.Exit(1) +func setupQueueManager(mgr ctrl.Manager, cCache *cache.Cache, cfg *configapi.Configuration) *queue.Manager { + queues := queue.NewManager( + mgr.GetClient(), + cCache, + queue.WithQueueVisibilityUpdateInterval(cfg.QueueVisibility.UpdateIntervalSeconds), + queue.WithQueueVisibilityClusterQueuesMaxCount(cfg.QueueVisibility.ClusterQueues.MaxCount), + ) + if cfg.QueueVisibility.ClusterQueues.MaxCount != 0 { + if err := mgr.Add(queues); err != nil { + setupLog.Error(err, "Unable to add queue manager to manager") + os.Exit(1) + } } return queues } diff --git a/pkg/config/config.go b/pkg/config/config.go index 85eff7a764..1211ac67e1 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -162,7 +162,7 @@ func Load(scheme *runtime.Scheme, configFile string) (ctrl.Options, configapi.Co return options, cfg, err } } + err = validate(&cfg).ToAggregate() addTo(&options, &cfg) - err = validate(&cfg) return options, cfg, err } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index fc903fbf0d..4470fca021 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -18,6 +18,7 @@ package config import ( "errors" + "fmt" "io/fs" "os" "path/filepath" @@ -28,6 +29,7 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/util/yaml" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" @@ -693,7 +695,7 @@ func TestValidate(t *testing.T) { testcases := []struct { name string cfg *configapi.Configuration - wantErr error + wantErr field.ErrorList }{ { @@ -708,7 +710,9 @@ func TestValidate(t *testing.T) { UpdateIntervalSeconds: 0, }, }, - wantErr: errInvalidUpdateIntervalSeconds, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("queueVisibility").Child("updateIntervalSeconds"), 0, fmt.Sprintf("must be more or equal %d", queueVisibilityClusterQueuesUpdateIntervalSeconds)), + }, }, { name: "invalid queue visibility cluster queue max count", @@ -717,14 +721,17 @@ func TestValidate(t *testing.T) { ClusterQueues: &configapi.ClusterQueueVisibility{ MaxCount: 4001, }, + UpdateIntervalSeconds: 1, }, }, - wantErr: errInvalidMaxValue, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("queueVisibility").Child("clusterQueues").Child("maxCount"), 4001, fmt.Sprintf("must be less than %d", queueVisibilityClusterQueuesMaxValue)), + }, }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - if diff := cmp.Diff(tc.wantErr, validate(tc.cfg), cmpopts.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.wantErr, validate(tc.cfg), cmpopts.IgnoreFields(field.Error{}, "BadValue")); diff != "" { t.Errorf("Unexpected returned error (-want,+got):\n%s", diff) } }) diff --git a/pkg/config/validation.go b/pkg/config/validation.go index cec880f62e..f713acfc5d 100644 --- a/pkg/config/validation.go +++ b/pkg/config/validation.go @@ -3,12 +3,9 @@ package config import ( "fmt" - configapi "sigs.k8s.io/kueue/apis/config/v1beta1" -) + "k8s.io/apimachinery/pkg/util/validation/field" -var ( - errInvalidMaxValue = fmt.Errorf("maximum value for QueueVisibility.ClusterQueues.MaxCount must be %d", queueVisibilityClusterQueuesMaxValue) - errInvalidUpdateIntervalSeconds = fmt.Errorf("minimum value for QueueVisibility.UpdateIntervalSeconds must be %d", queueVisibilityClusterQueuesUpdateIntervalSeconds) + configapi "sigs.k8s.io/kueue/apis/config/v1beta1" ) const ( @@ -16,16 +13,19 @@ const ( queueVisibilityClusterQueuesUpdateIntervalSeconds = 1 ) -func validate(cfg *configapi.Configuration) error { +func validate(cfg *configapi.Configuration) field.ErrorList { + var allErrs field.ErrorList if cfg.QueueVisibility != nil { + queueVisibilityPath := field.NewPath("queueVisibility") if cfg.QueueVisibility.ClusterQueues != nil { + clusterQueues := queueVisibilityPath.Child("clusterQueues") if cfg.QueueVisibility.ClusterQueues.MaxCount > queueVisibilityClusterQueuesMaxValue { - return errInvalidMaxValue + allErrs = append(allErrs, field.Invalid(clusterQueues.Child("maxCount"), int(cfg.QueueVisibility.ClusterQueues.MaxCount), fmt.Sprintf("must be less than %d", queueVisibilityClusterQueuesMaxValue))) } } if cfg.QueueVisibility.UpdateIntervalSeconds < queueVisibilityClusterQueuesUpdateIntervalSeconds { - return errInvalidUpdateIntervalSeconds + allErrs = append(allErrs, field.Invalid(queueVisibilityPath.Child("updateIntervalSeconds"), cfg.QueueVisibility.UpdateIntervalSeconds, fmt.Sprintf("must be more or equal %d", queueVisibilityClusterQueuesUpdateIntervalSeconds))) } } - return nil + return allErrs } diff --git a/pkg/controller/core/clusterqueue_controller_test.go b/pkg/controller/core/clusterqueue_controller_test.go index a446595ec0..4a28c43cc2 100644 --- a/pkg/controller/core/clusterqueue_controller_test.go +++ b/pkg/controller/core/clusterqueue_controller_test.go @@ -174,7 +174,7 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { cl := utiltesting.NewClientBuilder().WithLists(defaultWls).WithObjects(lq, cq).WithStatusSubresource(lq, cq). Build() cqCache := cache.New(cl) - qManager := queue.NewManager(cl, cqCache, nil) + qManager := queue.NewManager(cl, cqCache) if err := cqCache.AddClusterQueue(ctx, cq); err != nil { t.Fatalf("Inserting clusterQueue in cache: %v", err) } diff --git a/pkg/queue/cluster_queue_interface.go b/pkg/queue/cluster_queue_interface.go index badc25cfab..34f21baf47 100644 --- a/pkg/queue/cluster_queue_interface.go +++ b/pkg/queue/cluster_queue_interface.go @@ -92,6 +92,9 @@ type ClusterQueue interface { // Otherwise returns true. Dump() (sets.Set[string], bool) DumpInadmissible() (sets.Set[string], bool) + // Snapshot returns a copy of the current workloads in the heap of + // this ClusterQueue. It returns false if the queue is empty. + // Otherwise returns true. Snapshot(maxCount int32) ([]*kueue.Workload, bool) // Info returns workload.Info for the workload key. // Users of this method should not modify the returned object. diff --git a/pkg/queue/manager.go b/pkg/queue/manager.go index 8679f532c2..ff2ef77b36 100644 --- a/pkg/queue/manager.go +++ b/pkg/queue/manager.go @@ -29,7 +29,6 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - configapi "sigs.k8s.io/kueue/apis/config/v1beta1" kueue "sigs.k8s.io/kueue/apis/kueue/v1beta1" utilindexer "sigs.k8s.io/kueue/pkg/controller/core/indexer" "sigs.k8s.io/kueue/pkg/metrics" @@ -52,17 +51,48 @@ type Manager struct { localQueues map[string]*LocalQueue workloadsStatus *kueue.ClusterQueuePendingWorkloadsStatus - cfg *configapi.Configuration + + queueVisibilityUpdateInterval int32 + queueVisibilityClusterQueuesMaxCount int32 // Key is cohort's name. Value is a set of associated ClusterQueue names. cohorts map[string]sets.Set[string] } -func NewManager(client client.Client, checker StatusChecker, cfg *configapi.Configuration) *Manager { +type Options struct { + QueueVisibilityUpdateInterval int32 + QueueVisibilityClusterQueuesMaxCount int32 +} + +// Option configures the reconciler. +type Option func(*Options) + +// WithQueueVisibilityUpdateInterval indicates if the controller should reconcile +// jobs that don't set the queue name annotation. +func WithQueueVisibilityUpdateInterval(interval int32) Option { + return func(o *Options) { + o.QueueVisibilityUpdateInterval = interval + } +} + +// WithQueueVisibilityClusterQueuesMaxCount indicates if the controller should reconcile +// jobs that don't set the queue name annotation. +func WithQueueVisibilityClusterQueuesMaxCount(value int32) Option { + return func(o *Options) { + o.QueueVisibilityClusterQueuesMaxCount = value + } +} + +var DefaultOptions = Options{} + +func NewManager(client client.Client, checker StatusChecker, opts ...Option) *Manager { + options := DefaultOptions + for _, opt := range opts { + opt(&options) + } m := &Manager{ client: client, statusChecker: checker, - cfg: cfg, localQueues: make(map[string]*LocalQueue), clusterQueues: make(map[string]ClusterQueue), cohorts: make(map[string]sets.Set[string]), @@ -483,9 +513,9 @@ func (m *Manager) Snapshot() []*kueue.Workload { if len(m.clusterQueues) == 0 { return nil } - snapshot := make([]*kueue.Workload, 0) + snapshot := make([]*kueue.Workload, 0, len(m.clusterQueues)) for _, cq := range m.clusterQueues { - if elements, ok := cq.Snapshot(m.cfg.QueueVisibility.ClusterQueues.MaxCount); ok { + if elements, ok := cq.Snapshot(m.queueVisibilityClusterQueuesMaxCount); ok { snapshot = append(snapshot, elements...) } } @@ -576,7 +606,7 @@ func (m *Manager) Start(ctx context.Context) error { ctx = ctrl.LoggerInto(ctx, log) ticker := time.NewTicker( - time.Duration(m.cfg.QueueVisibility.UpdateIntervalSeconds) * time.Second, + time.Duration(m.queueVisibilityUpdateInterval) * time.Second, ) defer ticker.Stop() @@ -601,5 +631,7 @@ func (m *Manager) Start(ctx context.Context) error { } func (m *Manager) GetWorkloadsStatus() *kueue.ClusterQueuePendingWorkloadsStatus { + m.RLock() + defer m.RUnlock() return m.workloadsStatus } diff --git a/pkg/queue/manager_test.go b/pkg/queue/manager_test.go index 484247d3ab..2bbcc573a8 100644 --- a/pkg/queue/manager_test.go +++ b/pkg/queue/manager_test.go @@ -49,7 +49,7 @@ func TestAddLocalQueueOrphans(t *testing.T) { ReserveQuota(utiltesting.MakeAdmission("cq").Obj()).Obj(), utiltesting.MakeWorkload("a", "moon").Queue("foo").Obj(), ) - manager := NewManager(kClient, nil, nil) + manager := NewManager(kClient, nil) q := utiltesting.MakeLocalQueue("foo", "earth").Obj() if err := manager.AddLocalQueue(context.Background(), q); err != nil { t.Fatalf("Failed adding queue: %v", err) @@ -83,7 +83,7 @@ func TestAddClusterQueueOrphans(t *testing.T) { queues[1], ) ctx := context.Background() - manager := NewManager(kClient, nil, nil) + manager := NewManager(kClient, nil) cq := utiltesting.MakeClusterQueue("cq").Obj() if err := manager.AddClusterQueue(ctx, cq); err != nil { t.Fatalf("Failed adding cluster queue %s: %v", cq.Name, err) @@ -139,7 +139,7 @@ func TestUpdateClusterQueue(t *testing.T) { cl := utiltesting.NewFakeClient( &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: defaultNamespace}}, ) - manager := NewManager(cl, nil, nil) + manager := NewManager(cl, nil) for _, cq := range clusterQueues { if err := manager.AddClusterQueue(ctx, cq); err != nil { t.Fatalf("Failed adding clusterQueue %s: %v", cq.Name, err) @@ -204,7 +204,7 @@ func TestUpdateLocalQueue(t *testing.T) { t.Fatalf("Failed adding kueue scheme: %s", err) } ctx := context.Background() - manager := NewManager(utiltesting.NewFakeClient(), nil, nil) + manager := NewManager(utiltesting.NewFakeClient(), nil) for _, cq := range clusterQueues { if err := manager.AddClusterQueue(ctx, cq); err != nil { t.Fatalf("Failed adding clusterQueue %s: %v", cq.Name, err) @@ -248,7 +248,7 @@ func TestDeleteLocalQueue(t *testing.T) { ctx := context.Background() cl := utiltesting.NewFakeClient(wl) - manager := NewManager(cl, nil, nil) + manager := NewManager(cl, nil) if err := manager.AddClusterQueue(ctx, cq); err != nil { t.Fatalf("Could not create ClusterQueue: %v", err) @@ -272,7 +272,7 @@ func TestDeleteLocalQueue(t *testing.T) { } func TestAddWorkload(t *testing.T) { - manager := NewManager(utiltesting.NewFakeClient(), nil, nil) + manager := NewManager(utiltesting.NewFakeClient(), nil) cq := utiltesting.MakeClusterQueue("cq").Obj() if err := manager.AddClusterQueue(context.Background(), cq); err != nil { t.Fatalf("Failed adding clusterQueue %s: %v", cq.Name, err) @@ -390,7 +390,7 @@ func TestStatus(t *testing.T) { }, } - manager := NewManager(utiltesting.NewFakeClient(), nil, nil) + manager := NewManager(utiltesting.NewFakeClient(), nil) for _, q := range queues { if err := manager.AddLocalQueue(ctx, &q); err != nil { t.Errorf("Failed adding queue: %s", err) @@ -500,7 +500,7 @@ func TestRequeueWorkloadStrictFIFO(t *testing.T) { for _, tc := range cases { t.Run(tc.workload.Name, func(t *testing.T) { cl := utiltesting.NewFakeClient() - manager := NewManager(cl, nil, nil) + manager := NewManager(cl, nil) ctx := context.Background() if err := manager.AddClusterQueue(ctx, cq); err != nil { t.Fatalf("Failed adding cluster queue %s: %v", cq.Name, err) @@ -657,7 +657,7 @@ func TestUpdateWorkload(t *testing.T) { } for name, tc := range cases { t.Run(name, func(t *testing.T) { - manager := NewManager(utiltesting.NewFakeClient(), nil, nil) + manager := NewManager(utiltesting.NewFakeClient(), nil) ctx := context.Background() for _, cq := range tc.clusterQueues { if err := manager.AddClusterQueue(ctx, cq); err != nil { @@ -773,7 +773,7 @@ func TestHeads(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), headsTimeout) defer cancel() fakeC := &fakeStatusChecker{} - manager := NewManager(utiltesting.NewFakeClient(), fakeC, nil) + manager := NewManager(utiltesting.NewFakeClient(), fakeC) for _, cq := range clusterQueues { if err := manager.AddClusterQueue(ctx, cq); err != nil { t.Fatalf("Failed adding clusterQueue %s to manager: %v", cq.Name, err) @@ -1006,7 +1006,7 @@ func TestHeadsAsync(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), headsTimeout) defer cancel() client := utiltesting.NewFakeClient(tc.initialObjs...) - manager := NewManager(client, nil, nil) + manager := NewManager(client, nil) go manager.CleanUpOnContext(ctx) tc.op(ctx, manager) heads := manager.Heads(ctx) @@ -1019,7 +1019,7 @@ func TestHeadsAsync(t *testing.T) { // TestHeadsCancelled ensures that the Heads call returns when the context is closed. func TestHeadsCancelled(t *testing.T) { - manager := NewManager(utiltesting.NewFakeClient(), nil, nil) + manager := NewManager(utiltesting.NewFakeClient(), nil) ctx, cancel := context.WithCancel(context.Background()) go func() { cancel() diff --git a/pkg/scheduler/scheduler_test.go b/pkg/scheduler/scheduler_test.go index d55ea54ba2..9737eb8006 100644 --- a/pkg/scheduler/scheduler_test.go +++ b/pkg/scheduler/scheduler_test.go @@ -1041,7 +1041,7 @@ func TestSchedule(t *testing.T) { recorder := broadcaster.NewRecorder(scheme, corev1.EventSource{Component: constants.AdmissionName}) cqCache := cache.New(cl) - qManager := queue.NewManager(cl, cqCache, nil) + qManager := queue.NewManager(cl, cqCache) // Workloads are loaded into queues or clusterQueues as we add them. for _, q := range allQueues { if err := qManager.AddLocalQueue(ctx, &q); err != nil { @@ -1339,7 +1339,7 @@ func TestRequeueAndUpdate(t *testing.T) { broadcaster := record.NewBroadcaster() recorder := broadcaster.NewRecorder(scheme, corev1.EventSource{Component: constants.AdmissionName}) cqCache := cache.New(cl) - qManager := queue.NewManager(cl, cqCache, nil) + qManager := queue.NewManager(cl, cqCache) scheduler := New(qManager, cqCache, cl, recorder) if err := qManager.AddLocalQueue(ctx, q1); err != nil { t.Fatalf("Inserting queue %s/%s in manager: %v", q1.Namespace, q1.Name, err) diff --git a/test/integration/controller/core/suite_test.go b/test/integration/controller/core/suite_test.go index 487a78614b..e3d504d679 100644 --- a/test/integration/controller/core/suite_test.go +++ b/test/integration/controller/core/suite_test.go @@ -73,7 +73,7 @@ func managerSetup(mgr manager.Manager, ctx context.Context) { gomega.Expect(err).ToNot(gomega.HaveOccurred(), "webhook", failedWebhook) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache, nil) + queues := queue.NewManager(mgr.GetClient(), cCache) controllersCfg := &config.Configuration{} controllersCfg.Metrics.EnableClusterQueueResources = true diff --git a/test/integration/controller/jobs/job/suite_test.go b/test/integration/controller/jobs/job/suite_test.go index bd33d0282f..b10d02a72b 100644 --- a/test/integration/controller/jobs/job/suite_test.go +++ b/test/integration/controller/jobs/job/suite_test.go @@ -80,7 +80,7 @@ func managerAndSchedulerSetup(opts ...jobframework.Option) framework.ManagerSetu gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache, nil) + queues := queue.NewManager(mgr.GetClient(), cCache) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/controller/jobs/jobset/suite_test.go b/test/integration/controller/jobs/jobset/suite_test.go index 5ab8f17f8a..fde5c72188 100644 --- a/test/integration/controller/jobs/jobset/suite_test.go +++ b/test/integration/controller/jobs/jobset/suite_test.go @@ -79,7 +79,7 @@ func managerAndSchedulerSetup(opts ...jobframework.Option) framework.ManagerSetu gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache, nil) + queues := queue.NewManager(mgr.GetClient(), cCache) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/controller/jobs/mpijob/suite_test.go b/test/integration/controller/jobs/mpijob/suite_test.go index 8e698b21d7..0bdd34e8cb 100644 --- a/test/integration/controller/jobs/mpijob/suite_test.go +++ b/test/integration/controller/jobs/mpijob/suite_test.go @@ -93,7 +93,7 @@ func managerAndSchedulerSetup(opts ...jobframework.Option) framework.ManagerSetu gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache, nil) + queues := queue.NewManager(mgr.GetClient(), cCache) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/controller/jobs/pytorchjob/suite_test.go b/test/integration/controller/jobs/pytorchjob/suite_test.go index be0ff2f56a..e900233490 100644 --- a/test/integration/controller/jobs/pytorchjob/suite_test.go +++ b/test/integration/controller/jobs/pytorchjob/suite_test.go @@ -78,7 +78,7 @@ func managerAndSchedulerSetup(opts ...jobframework.Option) framework.ManagerSetu gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache, nil) + queues := queue.NewManager(mgr.GetClient(), cCache) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/controller/jobs/rayjob/suite_test.go b/test/integration/controller/jobs/rayjob/suite_test.go index 36606c563f..73a185db11 100644 --- a/test/integration/controller/jobs/rayjob/suite_test.go +++ b/test/integration/controller/jobs/rayjob/suite_test.go @@ -79,7 +79,7 @@ func managerAndSchedulerSetup(opts ...jobframework.Option) framework.ManagerSetu gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache, nil) + queues := queue.NewManager(mgr.GetClient(), cCache) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/controller/jobs/tfjob/suite_test.go b/test/integration/controller/jobs/tfjob/suite_test.go index ba0e29007d..e1f3d99075 100644 --- a/test/integration/controller/jobs/tfjob/suite_test.go +++ b/test/integration/controller/jobs/tfjob/suite_test.go @@ -78,7 +78,7 @@ func managerAndSchedulerSetup(opts ...jobframework.Option) framework.ManagerSetu gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache, nil) + queues := queue.NewManager(mgr.GetClient(), cCache) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/scheduler/podsready/suite_test.go b/test/integration/scheduler/podsready/suite_test.go index 03d700d157..546654149c 100644 --- a/test/integration/scheduler/podsready/suite_test.go +++ b/test/integration/scheduler/podsready/suite_test.go @@ -69,7 +69,7 @@ func managerAndSchedulerSetupWithTimeoutAdmission(mgr manager.Manager, ctx conte gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient(), cache.WithPodsReadyTracking(cfg.WaitForPodsReady.Enable && cfg.WaitForPodsReady.BlockAdmission != nil && *cfg.WaitForPodsReady.BlockAdmission)) - queues := queue.NewManager(mgr.GetClient(), cCache, nil) + queues := queue.NewManager(mgr.GetClient(), cCache) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &cfg) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/scheduler/suite_test.go b/test/integration/scheduler/suite_test.go index 1761966099..29fbba3211 100644 --- a/test/integration/scheduler/suite_test.go +++ b/test/integration/scheduler/suite_test.go @@ -73,7 +73,7 @@ func managerAndSchedulerSetup(mgr manager.Manager, ctx context.Context) { gomega.Expect(err).NotTo(gomega.HaveOccurred()) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache, nil) + queues := queue.NewManager(mgr.GetClient(), cCache) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/webhook/suite_test.go b/test/integration/webhook/suite_test.go index 0b965d0c49..3c3b26d437 100644 --- a/test/integration/webhook/suite_test.go +++ b/test/integration/webhook/suite_test.go @@ -65,7 +65,7 @@ var _ = ginkgo.BeforeSuite(func() { gomega.Expect(err).ToNot(gomega.HaveOccurred(), "webhook", failedWebhook) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache, nil) + queues := queue.NewManager(mgr.GetClient(), cCache) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, &config.Configuration{}) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) }) From 372c7d323458af1df830a404f2b7b992d319a6ae Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Fri, 25 Aug 2023 18:31:03 +0200 Subject: [PATCH 05/26] resolve review --- pkg/config/config.go | 4 +- pkg/config/config_test.go | 2 +- pkg/config/validation.go | 4 +- pkg/queue/cluster_queue_impl.go | 24 ++++----- pkg/queue/manager.go | 87 ++++++++++++++++++++------------- pkg/util/heap/heap.go | 4 +- 6 files changed, 74 insertions(+), 51 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index 1211ac67e1..ef920014f3 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -162,7 +162,9 @@ func Load(scheme *runtime.Scheme, configFile string) (ctrl.Options, configapi.Co return options, cfg, err } } - err = validate(&cfg).ToAggregate() + if err = validate(&cfg).ToAggregate(); err != nil { + return options, cfg, err + } addTo(&options, &cfg) return options, cfg, err } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 4470fca021..899e40b54e 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -711,7 +711,7 @@ func TestValidate(t *testing.T) { }, }, wantErr: field.ErrorList{ - field.Invalid(field.NewPath("queueVisibility").Child("updateIntervalSeconds"), 0, fmt.Sprintf("must be more or equal %d", queueVisibilityClusterQueuesUpdateIntervalSeconds)), + field.Invalid(field.NewPath("queueVisibility").Child("updateIntervalSeconds"), 0, fmt.Sprintf("greater than or equal to %d", queueVisibilityClusterQueuesUpdateIntervalSeconds)), }, }, { diff --git a/pkg/config/validation.go b/pkg/config/validation.go index f713acfc5d..1204cceb9b 100644 --- a/pkg/config/validation.go +++ b/pkg/config/validation.go @@ -20,11 +20,11 @@ func validate(cfg *configapi.Configuration) field.ErrorList { if cfg.QueueVisibility.ClusterQueues != nil { clusterQueues := queueVisibilityPath.Child("clusterQueues") if cfg.QueueVisibility.ClusterQueues.MaxCount > queueVisibilityClusterQueuesMaxValue { - allErrs = append(allErrs, field.Invalid(clusterQueues.Child("maxCount"), int(cfg.QueueVisibility.ClusterQueues.MaxCount), fmt.Sprintf("must be less than %d", queueVisibilityClusterQueuesMaxValue))) + allErrs = append(allErrs, field.Invalid(clusterQueues.Child("maxCount"), cfg.QueueVisibility.ClusterQueues.MaxCount, fmt.Sprintf("must be less than %d", queueVisibilityClusterQueuesMaxValue))) } } if cfg.QueueVisibility.UpdateIntervalSeconds < queueVisibilityClusterQueuesUpdateIntervalSeconds { - allErrs = append(allErrs, field.Invalid(queueVisibilityPath.Child("updateIntervalSeconds"), cfg.QueueVisibility.UpdateIntervalSeconds, fmt.Sprintf("must be more or equal %d", queueVisibilityClusterQueuesUpdateIntervalSeconds))) + allErrs = append(allErrs, field.Invalid(queueVisibilityPath.Child("updateIntervalSeconds"), cfg.QueueVisibility.UpdateIntervalSeconds, fmt.Sprintf("greater than or equal to %d", queueVisibilityClusterQueuesUpdateIntervalSeconds))) } } return allErrs diff --git a/pkg/queue/cluster_queue_impl.go b/pkg/queue/cluster_queue_impl.go index b3a2714fee..f656b1cf3a 100644 --- a/pkg/queue/cluster_queue_impl.go +++ b/pkg/queue/cluster_queue_impl.go @@ -210,8 +210,19 @@ func (c *clusterQueueBase) Dump() (sets.Set[string], bool) { return elements, true } +func (c *clusterQueueBase) DumpInadmissible() (sets.Set[string], bool) { + if len(c.inadmissibleWorkloads) == 0 { + return nil, false + } + elements := make(sets.Set[string], len(c.inadmissibleWorkloads)) + for _, info := range c.inadmissibleWorkloads { + elements.Insert(workload.Key(info.Obj)) + } + return elements, true +} + func (c *clusterQueueBase) Snapshot(maxCount int32) ([]*kueue.Workload, bool) { - if c.heap.Len() == 0 || maxCount == 0 { + if c.heap.Len() == 0 { return nil, false } elements := make([]*kueue.Workload, c.heap.Len()) @@ -228,17 +239,6 @@ func (c *clusterQueueBase) Snapshot(maxCount int32) ([]*kueue.Workload, bool) { return elements, true } -func (c *clusterQueueBase) DumpInadmissible() (sets.Set[string], bool) { - if len(c.inadmissibleWorkloads) == 0 { - return nil, false - } - elements := make(sets.Set[string], len(c.inadmissibleWorkloads)) - for _, info := range c.inadmissibleWorkloads { - elements.Insert(workload.Key(info.Obj)) - } - return elements, true -} - func (c *clusterQueueBase) Info(key string) *workload.Info { info := c.heap.GetByKey(key) if info == nil { diff --git a/pkg/queue/manager.go b/pkg/queue/manager.go index ff2ef77b36..01a53cf848 100644 --- a/pkg/queue/manager.go +++ b/pkg/queue/manager.go @@ -50,6 +50,7 @@ type Manager struct { clusterQueues map[string]ClusterQueue localQueues map[string]*LocalQueue + wm sync.RWMutex workloadsStatus *kueue.ClusterQueuePendingWorkloadsStatus queueVisibilityUpdateInterval int32 @@ -91,11 +92,14 @@ func NewManager(client client.Client, checker StatusChecker, opts ...Option) *Ma opt(&options) } m := &Manager{ - client: client, - statusChecker: checker, - localQueues: make(map[string]*LocalQueue), - clusterQueues: make(map[string]ClusterQueue), - cohorts: make(map[string]sets.Set[string]), + client: client, + statusChecker: checker, + localQueues: make(map[string]*LocalQueue), + clusterQueues: make(map[string]ClusterQueue), + cohorts: make(map[string]sets.Set[string]), + wm: sync.RWMutex{}, + queueVisibilityUpdateInterval: options.QueueVisibilityUpdateInterval, + queueVisibilityClusterQueuesMaxCount: options.QueueVisibilityClusterQueuesMaxCount, } m.cond.L = &m.RWMutex return m @@ -486,7 +490,7 @@ func (m *Manager) Heads(ctx context.Context) []workload.Info { } } -// Dump is a dump of the queues and it's elements (unordered). +// Dump is a dump of the queues and it's elements (ordered). // Only use for testing purposes. func (m *Manager) Dump() map[string]sets.Set[string] { m.Lock() @@ -506,22 +510,6 @@ func (m *Manager) Dump() map[string]sets.Set[string] { return dump } -// Snapshot is a copy of the queues elements and inadmissible workloads list. -func (m *Manager) Snapshot() []*kueue.Workload { - m.Lock() - defer m.Unlock() - if len(m.clusterQueues) == 0 { - return nil - } - snapshot := make([]*kueue.Workload, 0, len(m.clusterQueues)) - for _, cq := range m.clusterQueues { - if elements, ok := cq.Snapshot(m.queueVisibilityClusterQueuesMaxCount); ok { - snapshot = append(snapshot, elements...) - } - } - return snapshot -} - // DumpInadmissible is a dump of the inadmissible workloads list. // Only use for testing purposes. func (m *Manager) DumpInadmissible() map[string]sets.Set[string] { @@ -542,6 +530,22 @@ func (m *Manager) DumpInadmissible() map[string]sets.Set[string] { return dump } +// Snapshot is a copy of the queues elements and inadmissible workloads list. +func (m *Manager) Snapshot() []*kueue.Workload { + m.Lock() + defer m.Unlock() + if len(m.clusterQueues) == 0 { + return nil + } + snapshot := make([]*kueue.Workload, 0, len(m.clusterQueues)) + for _, cq := range m.clusterQueues { + if elements, ok := cq.Snapshot(m.queueVisibilityClusterQueuesMaxCount); ok { + snapshot = append(snapshot, elements...) + } + } + return snapshot +} + func (m *Manager) heads() []workload.Info { var workloads []workload.Info for cqName, cq := range m.clusterQueues { @@ -616,22 +620,37 @@ func (m *Manager) Start(ctx context.Context) error { log.V(2).Info("Context cancelled; stop doing snapshot of cluster queue") return nil case t := <-ticker.C: - m.workloadsStatus = &kueue.ClusterQueuePendingWorkloadsStatus{ - Head: make([]kueue.ClusterQueuePendingWorkload, 0), - LastChangeTime: metav1.Time{Time: t}, - } - for _, wl := range m.Snapshot() { - m.workloadsStatus.Head = append(m.workloadsStatus.Head, kueue.ClusterQueuePendingWorkload{ - Name: wl.Name, - Namespace: wl.Namespace, - }) - } + m.wm.Lock() + m.workloadsStatus = m.takeSnapshot(t) + m.wm.Unlock() } } } +func (m *Manager) takeSnapshot(t time.Time) *kueue.ClusterQueuePendingWorkloadsStatus { + snapshot := m.Snapshot() + snapshotLen := len(snapshot) + if snapshotLen == 0 { + return nil + } + workloads := make([]kueue.ClusterQueuePendingWorkload, 0, snapshotLen) + for _, wl := range snapshot { + if wl == nil { + continue + } + workloads = append(workloads, kueue.ClusterQueuePendingWorkload{ + Name: wl.Name, + Namespace: wl.Namespace, + }) + } + return &kueue.ClusterQueuePendingWorkloadsStatus{ + Head: workloads, + LastChangeTime: metav1.Time{Time: t}, + } +} + func (m *Manager) GetWorkloadsStatus() *kueue.ClusterQueuePendingWorkloadsStatus { - m.RLock() - defer m.RUnlock() + m.wm.RLock() + defer m.wm.RUnlock() return m.workloadsStatus } diff --git a/pkg/util/heap/heap.go b/pkg/util/heap/heap.go index 2cdd7012ca..ac21f87755 100644 --- a/pkg/util/heap/heap.go +++ b/pkg/util/heap/heap.go @@ -20,6 +20,7 @@ package heap import ( "container/heap" + "sort" ) // lessFunc is a function that receives two items and returns true if the first @@ -169,8 +170,9 @@ func (h *Heap) Len() int { return h.data.Len() } -// List returns a list of all the items. +// List returns a sorted list of all the items. func (h *Heap) List() []interface{} { + sort.Sort(&h.data) list := make([]interface{}, 0, h.Len()) for _, item := range h.data.items { list = append(list, item.obj) From c6cf615e0787c9418873869480e07442486214cb Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Mon, 28 Aug 2023 14:50:37 +0200 Subject: [PATCH 06/26] added pending workload status in tests --- pkg/queue/cluster_queue_impl.go | 7 ++----- pkg/queue/manager.go | 4 ++-- .../core/clusterqueue_controller_test.go | 14 ++++++++++++-- test/integration/controller/core/suite_test.go | 9 ++++++++- 4 files changed, 24 insertions(+), 10 deletions(-) diff --git a/pkg/queue/cluster_queue_impl.go b/pkg/queue/cluster_queue_impl.go index f656b1cf3a..3682252ef0 100644 --- a/pkg/queue/cluster_queue_impl.go +++ b/pkg/queue/cluster_queue_impl.go @@ -225,17 +225,14 @@ func (c *clusterQueueBase) Snapshot(maxCount int32) ([]*kueue.Workload, bool) { if c.heap.Len() == 0 { return nil, false } - elements := make([]*kueue.Workload, c.heap.Len()) + elements := make([]*kueue.Workload, 0, c.heap.Len()) for i, e := range c.heap.List() { - if maxCount >= int32(i) { + if int32(i) > maxCount { return elements, true } info := e.(*workload.Info) elements = append(elements, info.Obj) } - for _, info := range c.inadmissibleWorkloads { - elements = append(elements, info.Obj) - } return elements, true } diff --git a/pkg/queue/manager.go b/pkg/queue/manager.go index 01a53cf848..bcc6483dfc 100644 --- a/pkg/queue/manager.go +++ b/pkg/queue/manager.go @@ -620,14 +620,14 @@ func (m *Manager) Start(ctx context.Context) error { log.V(2).Info("Context cancelled; stop doing snapshot of cluster queue") return nil case t := <-ticker.C: - m.wm.Lock() m.workloadsStatus = m.takeSnapshot(t) - m.wm.Unlock() } } } func (m *Manager) takeSnapshot(t time.Time) *kueue.ClusterQueuePendingWorkloadsStatus { + m.wm.Lock() + defer m.wm.Unlock() snapshot := m.Snapshot() snapshotLen := len(snapshot) if snapshotLen == 0 { diff --git a/test/integration/controller/core/clusterqueue_controller_test.go b/test/integration/controller/core/clusterqueue_controller_test.go index 0952b413bb..c63b7e683d 100644 --- a/test/integration/controller/core/clusterqueue_controller_test.go +++ b/test/integration/controller/core/clusterqueue_controller_test.go @@ -47,6 +47,8 @@ const ( ) var ignoreConditionTimestamps = cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime") +var ignoreLastChangeTime = cmpopts.IgnoreFields(kueue.ClusterQueuePendingWorkloadsStatus{}, "LastChangeTime") +var ignorePendingWorkloadsStatus = cmpopts.IgnoreFields(kueue.ClusterQueueStatus{}, "PendingWorkloadsStatus") var _ = ginkgo.Describe("ClusterQueue controller", func() { var ( @@ -183,7 +185,7 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { Message: "Can't admit new workloads; some flavors are not found", }, }, - }, ignoreConditionTimestamps)) + }, ignoreConditionTimestamps, ignorePendingWorkloadsStatus)) // Workloads are inadmissible because ResourceFlavors don't exist here yet. util.ExpectPendingWorkloadsMetric(clusterQueue, 0, 5) util.ExpectAdmittedActiveWorkloadsMetric(clusterQueue, 0) @@ -269,7 +271,15 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { Message: "Can admit new workloads", }, }, - }, ignoreConditionTimestamps)) + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{ + Head: []kueue.ClusterQueuePendingWorkload{ + { + Name: "six", + Namespace: ns.Name, + }, + }, + }, + }, ignoreConditionTimestamps, ignoreLastChangeTime)) util.ExpectPendingWorkloadsMetric(clusterQueue, 1, 0) util.ExpectAdmittedActiveWorkloadsMetric(clusterQueue, 4) diff --git a/test/integration/controller/core/suite_test.go b/test/integration/controller/core/suite_test.go index e3d504d679..18c53e2722 100644 --- a/test/integration/controller/core/suite_test.go +++ b/test/integration/controller/core/suite_test.go @@ -73,7 +73,14 @@ func managerSetup(mgr manager.Manager, ctx context.Context) { gomega.Expect(err).ToNot(gomega.HaveOccurred(), "webhook", failedWebhook) cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager(mgr.GetClient(), cCache) + queues := queue.NewManager( + mgr.GetClient(), + cCache, + queue.WithQueueVisibilityUpdateInterval(1), + queue.WithQueueVisibilityClusterQueuesMaxCount(10), + ) + err = mgr.Add(queues) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) controllersCfg := &config.Configuration{} controllersCfg.Metrics.EnableClusterQueueResources = true From 413e074ef668d3336d5ed35ceee56784254043a6 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Tue, 29 Aug 2023 12:22:54 +0200 Subject: [PATCH 07/26] generate client-go --- apis/kueue/v1beta1/clusterqueue_types.go | 40 ++++++------- .../v1beta1/clusterqueuependingworkload.go | 47 ++++++++++++++++ .../clusterqueuependingworkloadsstatus.go | 56 +++++++++++++++++++ .../kueue/v1beta1/clusterqueuestatus.go | 17 ++++-- client-go/applyconfiguration/utils.go | 4 ++ 5 files changed, 140 insertions(+), 24 deletions(-) create mode 100644 client-go/applyconfiguration/kueue/v1beta1/clusterqueuependingworkload.go create mode 100644 client-go/applyconfiguration/kueue/v1beta1/clusterqueuependingworkloadsstatus.go diff --git a/apis/kueue/v1beta1/clusterqueue_types.go b/apis/kueue/v1beta1/clusterqueue_types.go index 204aeb7929..20cacab21d 100644 --- a/apis/kueue/v1beta1/clusterqueue_types.go +++ b/apis/kueue/v1beta1/clusterqueue_types.go @@ -180,26 +180,6 @@ type ResourceQuota struct { // ResourceFlavorReference is the name of the ResourceFlavor. type ResourceFlavorReference string -// ClusterQueuePendingWorkload contains the information identifying a pending workload -// in the cluster queue. -type ClusterQueuePendingWorkload struct { - // Name indicates the name of the pending workload. - Name string `json:"name"` - - // Namespace indicates the name of the pending workload. - Namespace string `json:"namespace"` -} - -type ClusterQueuePendingWorkloadsStatus struct { - // Head contains the list of top pending workloads. - // +listType=atomic - // +optional - Head []ClusterQueuePendingWorkload `json:"clusterQueuePendingWorkload"` - - // LastChangeTime indicates the time of the last change of the structure. - LastChangeTime metav1.Time `json:"lastChangeTime"` -} - // ClusterQueueStatus defines the observed state of ClusterQueue type ClusterQueueStatus struct { // flavorsUsage are the used quotas, by flavor, currently in use by the @@ -235,6 +215,26 @@ type ClusterQueueStatus struct { PendingWorkloadsStatus *ClusterQueuePendingWorkloadsStatus `json:"pendingWorkloadsStatus"` } +type ClusterQueuePendingWorkloadsStatus struct { + // Head contains the list of top pending workloads. + // +listType=atomic + // +optional + Head []ClusterQueuePendingWorkload `json:"clusterQueuePendingWorkload"` + + // LastChangeTime indicates the time of the last change of the structure. + LastChangeTime metav1.Time `json:"lastChangeTime"` +} + +// ClusterQueuePendingWorkload contains the information identifying a pending workload +// in the cluster queue. +type ClusterQueuePendingWorkload struct { + // Name indicates the name of the pending workload. + Name string `json:"name"` + + // Namespace indicates the name of the pending workload. + Namespace string `json:"namespace"` +} + type FlavorUsage struct { // name of the flavor. Name ResourceFlavorReference `json:"name"` diff --git a/client-go/applyconfiguration/kueue/v1beta1/clusterqueuependingworkload.go b/client-go/applyconfiguration/kueue/v1beta1/clusterqueuependingworkload.go new file mode 100644 index 0000000000..c875452d74 --- /dev/null +++ b/client-go/applyconfiguration/kueue/v1beta1/clusterqueuependingworkload.go @@ -0,0 +1,47 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +// Code generated by applyconfiguration-gen. DO NOT EDIT. + +package v1beta1 + +// ClusterQueuePendingWorkloadApplyConfiguration represents an declarative configuration of the ClusterQueuePendingWorkload type for use +// with apply. +type ClusterQueuePendingWorkloadApplyConfiguration struct { + Name *string `json:"name,omitempty"` + Namespace *string `json:"namespace,omitempty"` +} + +// ClusterQueuePendingWorkloadApplyConfiguration constructs an declarative configuration of the ClusterQueuePendingWorkload type for use with +// apply. +func ClusterQueuePendingWorkload() *ClusterQueuePendingWorkloadApplyConfiguration { + return &ClusterQueuePendingWorkloadApplyConfiguration{} +} + +// WithName sets the Name field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the Name field is set to the value of the last call. +func (b *ClusterQueuePendingWorkloadApplyConfiguration) WithName(value string) *ClusterQueuePendingWorkloadApplyConfiguration { + b.Name = &value + return b +} + +// WithNamespace sets the Namespace field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the Namespace field is set to the value of the last call. +func (b *ClusterQueuePendingWorkloadApplyConfiguration) WithNamespace(value string) *ClusterQueuePendingWorkloadApplyConfiguration { + b.Namespace = &value + return b +} diff --git a/client-go/applyconfiguration/kueue/v1beta1/clusterqueuependingworkloadsstatus.go b/client-go/applyconfiguration/kueue/v1beta1/clusterqueuependingworkloadsstatus.go new file mode 100644 index 0000000000..be6ee38a79 --- /dev/null +++ b/client-go/applyconfiguration/kueue/v1beta1/clusterqueuependingworkloadsstatus.go @@ -0,0 +1,56 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +// Code generated by applyconfiguration-gen. DO NOT EDIT. + +package v1beta1 + +import ( + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// ClusterQueuePendingWorkloadsStatusApplyConfiguration represents an declarative configuration of the ClusterQueuePendingWorkloadsStatus type for use +// with apply. +type ClusterQueuePendingWorkloadsStatusApplyConfiguration struct { + Head []ClusterQueuePendingWorkloadApplyConfiguration `json:"clusterQueuePendingWorkload,omitempty"` + LastChangeTime *v1.Time `json:"lastChangeTime,omitempty"` +} + +// ClusterQueuePendingWorkloadsStatusApplyConfiguration constructs an declarative configuration of the ClusterQueuePendingWorkloadsStatus type for use with +// apply. +func ClusterQueuePendingWorkloadsStatus() *ClusterQueuePendingWorkloadsStatusApplyConfiguration { + return &ClusterQueuePendingWorkloadsStatusApplyConfiguration{} +} + +// WithHead adds the given value to the Head field in the declarative configuration +// and returns the receiver, so that objects can be build by chaining "With" function invocations. +// If called multiple times, values provided by each call will be appended to the Head field. +func (b *ClusterQueuePendingWorkloadsStatusApplyConfiguration) WithHead(values ...*ClusterQueuePendingWorkloadApplyConfiguration) *ClusterQueuePendingWorkloadsStatusApplyConfiguration { + for i := range values { + if values[i] == nil { + panic("nil value passed to WithHead") + } + b.Head = append(b.Head, *values[i]) + } + return b +} + +// WithLastChangeTime sets the LastChangeTime field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the LastChangeTime field is set to the value of the last call. +func (b *ClusterQueuePendingWorkloadsStatusApplyConfiguration) WithLastChangeTime(value v1.Time) *ClusterQueuePendingWorkloadsStatusApplyConfiguration { + b.LastChangeTime = &value + return b +} diff --git a/client-go/applyconfiguration/kueue/v1beta1/clusterqueuestatus.go b/client-go/applyconfiguration/kueue/v1beta1/clusterqueuestatus.go index 921faec474..3992496da6 100644 --- a/client-go/applyconfiguration/kueue/v1beta1/clusterqueuestatus.go +++ b/client-go/applyconfiguration/kueue/v1beta1/clusterqueuestatus.go @@ -24,10 +24,11 @@ import ( // ClusterQueueStatusApplyConfiguration represents an declarative configuration of the ClusterQueueStatus type for use // with apply. type ClusterQueueStatusApplyConfiguration struct { - FlavorsUsage []FlavorUsageApplyConfiguration `json:"flavorsUsage,omitempty"` - PendingWorkloads *int32 `json:"pendingWorkloads,omitempty"` - AdmittedWorkloads *int32 `json:"admittedWorkloads,omitempty"` - Conditions []v1.Condition `json:"conditions,omitempty"` + FlavorsUsage []FlavorUsageApplyConfiguration `json:"flavorsUsage,omitempty"` + PendingWorkloads *int32 `json:"pendingWorkloads,omitempty"` + AdmittedWorkloads *int32 `json:"admittedWorkloads,omitempty"` + Conditions []v1.Condition `json:"conditions,omitempty"` + PendingWorkloadsStatus *ClusterQueuePendingWorkloadsStatusApplyConfiguration `json:"pendingWorkloadsStatus,omitempty"` } // ClusterQueueStatusApplyConfiguration constructs an declarative configuration of the ClusterQueueStatus type for use with @@ -74,3 +75,11 @@ func (b *ClusterQueueStatusApplyConfiguration) WithConditions(values ...v1.Condi } return b } + +// WithPendingWorkloadsStatus sets the PendingWorkloadsStatus field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the PendingWorkloadsStatus field is set to the value of the last call. +func (b *ClusterQueueStatusApplyConfiguration) WithPendingWorkloadsStatus(value *ClusterQueuePendingWorkloadsStatusApplyConfiguration) *ClusterQueueStatusApplyConfiguration { + b.PendingWorkloadsStatus = value + return b +} diff --git a/client-go/applyconfiguration/utils.go b/client-go/applyconfiguration/utils.go index cc1c92e631..ab629db4ed 100644 --- a/client-go/applyconfiguration/utils.go +++ b/client-go/applyconfiguration/utils.go @@ -32,6 +32,10 @@ func ForKind(kind schema.GroupVersionKind) interface{} { return &kueuev1beta1.AdmissionApplyConfiguration{} case v1beta1.SchemeGroupVersion.WithKind("ClusterQueue"): return &kueuev1beta1.ClusterQueueApplyConfiguration{} + case v1beta1.SchemeGroupVersion.WithKind("ClusterQueuePendingWorkload"): + return &kueuev1beta1.ClusterQueuePendingWorkloadApplyConfiguration{} + case v1beta1.SchemeGroupVersion.WithKind("ClusterQueuePendingWorkloadsStatus"): + return &kueuev1beta1.ClusterQueuePendingWorkloadsStatusApplyConfiguration{} case v1beta1.SchemeGroupVersion.WithKind("ClusterQueuePreemption"): return &kueuev1beta1.ClusterQueuePreemptionApplyConfiguration{} case v1beta1.SchemeGroupVersion.WithKind("ClusterQueueSpec"): From c3a15d85561dca36e4bbf15dbed735747c7047c3 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Wed, 30 Aug 2023 15:49:51 +0200 Subject: [PATCH 08/26] multiple workers for taking snapshots --- .../core/clusterqueue_controller.go | 17 ++- pkg/queue/manager.go | 129 +++++++++++------- 2 files changed, 94 insertions(+), 52 deletions(-) diff --git a/pkg/controller/core/clusterqueue_controller.go b/pkg/controller/core/clusterqueue_controller.go index 974d92f22f..a20bb6353c 100644 --- a/pkg/controller/core/clusterqueue_controller.go +++ b/pkg/controller/core/clusterqueue_controller.go @@ -18,6 +18,7 @@ package core import ( "context" + "time" "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" @@ -486,7 +487,7 @@ func (r *ClusterQueueReconciler) updateCqStatusIfChanged( cq.Status.FlavorsUsage = usage cq.Status.AdmittedWorkloads = int32(workloads) cq.Status.PendingWorkloads = int32(pendingWorkloads) - cq.Status.PendingWorkloadsStatus = r.qManager.GetWorkloadsStatus() + cq.Status.PendingWorkloadsStatus = r.getWorkloadsStatus() meta.SetStatusCondition(&cq.Status.Conditions, metav1.Condition{ Type: kueue.ClusterQueueActive, Status: conditionStatus, @@ -498,3 +499,17 @@ func (r *ClusterQueueReconciler) updateCqStatusIfChanged( } return nil } + +func (r *ClusterQueueReconciler) getWorkloadsStatus() *kueue.ClusterQueuePendingWorkloadsStatus { + pendingWorkloads := make([]kueue.ClusterQueuePendingWorkload, 0) + for _, workloads := range r.qManager.GetSnapshots() { + pendingWorkloads = append(pendingWorkloads, workloads...) + } + if len(pendingWorkloads) == 0 { + return nil + } + return &kueue.ClusterQueuePendingWorkloadsStatus{ + Head: pendingWorkloads, + LastChangeTime: metav1.Time{Time: time.Now()}, + } +} diff --git a/pkg/queue/manager.go b/pkg/queue/manager.go index bcc6483dfc..3fefa60fd7 100644 --- a/pkg/queue/manager.go +++ b/pkg/queue/manager.go @@ -24,8 +24,9 @@ import ( "time" apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -35,6 +36,8 @@ import ( "sigs.k8s.io/kueue/pkg/workload" ) +const snapshotWorkers = 5 + var ( errQueueDoesNotExist = errors.New("queue doesn't exist") errClusterQueueDoesNotExist = errors.New("clusterQueue doesn't exist") @@ -50,8 +53,9 @@ type Manager struct { clusterQueues map[string]ClusterQueue localQueues map[string]*LocalQueue - wm sync.RWMutex - workloadsStatus *kueue.ClusterQueuePendingWorkloadsStatus + wm sync.RWMutex + queue workqueue.Interface + snapshots map[string][]kueue.ClusterQueuePendingWorkload queueVisibilityUpdateInterval int32 queueVisibilityClusterQueuesMaxCount int32 @@ -98,6 +102,8 @@ func NewManager(client client.Client, checker StatusChecker, opts ...Option) *Ma clusterQueues: make(map[string]ClusterQueue), cohorts: make(map[string]sets.Set[string]), wm: sync.RWMutex{}, + queue: workqueue.New(), + snapshots: make(map[string][]kueue.ClusterQueuePendingWorkload, 0), queueVisibilityUpdateInterval: options.QueueVisibilityUpdateInterval, queueVisibilityClusterQueuesMaxCount: options.QueueVisibilityClusterQueuesMaxCount, } @@ -530,22 +536,6 @@ func (m *Manager) DumpInadmissible() map[string]sets.Set[string] { return dump } -// Snapshot is a copy of the queues elements and inadmissible workloads list. -func (m *Manager) Snapshot() []*kueue.Workload { - m.Lock() - defer m.Unlock() - if len(m.clusterQueues) == 0 { - return nil - } - snapshot := make([]*kueue.Workload, 0, len(m.clusterQueues)) - for _, cq := range m.clusterQueues { - if elements, ok := cq.Snapshot(m.queueVisibilityClusterQueuesMaxCount); ok { - snapshot = append(snapshot, elements...) - } - } - return snapshot -} - func (m *Manager) heads() []workload.Info { var workloads []workload.Info for cqName, cq := range m.clusterQueues { @@ -606,51 +596,88 @@ func (m *Manager) reportPendingWorkloads(cqName string, cq ClusterQueue) { } func (m *Manager) Start(ctx context.Context) error { - log := ctrl.LoggerFrom(ctx).WithName("clusterQueueSnapshot") - ctx = ctrl.LoggerInto(ctx, log) + defer m.queue.ShutDown() - ticker := time.NewTicker( - time.Duration(m.queueVisibilityUpdateInterval) * time.Second, - ) - defer ticker.Stop() + queueVisibilityUpdateInterval := time.Duration(m.queueVisibilityUpdateInterval) * time.Second - for { - select { - case <-ctx.Done(): - log.V(2).Info("Context cancelled; stop doing snapshot of cluster queue") - return nil - case t := <-ticker.C: - m.workloadsStatus = m.takeSnapshot(t) - } + for i := 0; i < snapshotWorkers; i++ { + go wait.UntilWithContext(ctx, m.takeSnapshot, queueVisibilityUpdateInterval) + } + + go wait.UntilWithContext(ctx, m.enqueueSnapshotDelayed, queueVisibilityUpdateInterval) + + <-ctx.Done() + + return nil +} + +func (m *Manager) enqueueSnapshotDelayed(ctx context.Context) { + m.RLock() + defer m.RUnlock() + for cq := range m.clusterQueues { + m.queue.Add(cq) + } +} + +func (m *Manager) takeSnapshot(ctx context.Context) { + for m.processNextSnapshot(ctx) { } } -func (m *Manager) takeSnapshot(t time.Time) *kueue.ClusterQueuePendingWorkloadsStatus { +func (m *Manager) processNextSnapshot(ctx context.Context) bool { m.wm.Lock() defer m.wm.Unlock() - snapshot := m.Snapshot() - snapshotLen := len(snapshot) - if snapshotLen == 0 { - return nil + + log := ctrl.LoggerFrom(ctx).WithName("processNextSnapshot") + + key, quit := m.queue.Get() + if quit { + return false } - workloads := make([]kueue.ClusterQueuePendingWorkload, 0, snapshotLen) - for _, wl := range snapshot { - if wl == nil { - continue + + startTime := time.Now() + defer func() { + log.V(4).Info("Finished snapshot job", "key", key, "elapsed", time.Since(startTime)) + }() + + cq := m.extractClusterQueue(key) + if cq == nil { + return false + } + + defer m.queue.Done(key) + + workloads := make([]kueue.ClusterQueuePendingWorkload, 0) + if elements, ok := cq.Snapshot(m.queueVisibilityClusterQueuesMaxCount); ok { + for _, wl := range elements { + workloads = append(workloads, kueue.ClusterQueuePendingWorkload{ + Name: wl.Name, + Namespace: wl.Namespace, + }) } - workloads = append(workloads, kueue.ClusterQueuePendingWorkload{ - Name: wl.Name, - Namespace: wl.Namespace, - }) } - return &kueue.ClusterQueuePendingWorkloadsStatus{ - Head: workloads, - LastChangeTime: metav1.Time{Time: t}, + m.snapshots[key.(string)] = workloads + + return true +} + +func (m *Manager) extractClusterQueue(key interface{}) ClusterQueue { + m.RLock() + defer m.RUnlock() + + if len(m.clusterQueues) == 0 { + return nil + } + + if cq := m.clusterQueues[key.(string)]; cq != nil { + return cq } + + return nil } -func (m *Manager) GetWorkloadsStatus() *kueue.ClusterQueuePendingWorkloadsStatus { +func (m *Manager) GetSnapshots() map[string][]kueue.ClusterQueuePendingWorkload { m.wm.RLock() defer m.wm.RUnlock() - return m.workloadsStatus + return m.snapshots } From a694252481c39851941227ff1c4a1ecab8d06eb3 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Thu, 31 Aug 2023 12:37:39 +0200 Subject: [PATCH 09/26] first complete pass review --- apis/config/v1beta1/defaults.go | 6 ++ apis/config/v1beta1/defaults_test.go | 9 +++ charts/kueue/values.yaml | 3 - cmd/kueue/main.go | 9 +-- cmd/kueue/main_test.go | 3 + .../manager/controller_manager_config.yaml | 3 - pkg/config/config_test.go | 57 ++---------------- pkg/config/validation_test.go | 59 +++++++++++++++++++ .../core/clusterqueue_controller.go | 1 + pkg/queue/manager.go | 23 ++++++-- 10 files changed, 104 insertions(+), 69 deletions(-) create mode 100644 pkg/config/validation_test.go diff --git a/apis/config/v1beta1/defaults.go b/apis/config/v1beta1/defaults.go index 1d46400d4f..b1cf6a0b15 100644 --- a/apis/config/v1beta1/defaults.go +++ b/apis/config/v1beta1/defaults.go @@ -40,6 +40,7 @@ const ( DefaultClientConnectionBurst int32 = 30 defaultPodsReadyTimeout = 5 * time.Minute DefaultQueueVisibilityUpdateIntervalSeconds int32 = 5 + DefaultClusterQueuesMaxCount int32 = 10 ) func addDefaultingFuncs(scheme *runtime.Scheme) error { @@ -123,4 +124,9 @@ func SetDefaults_Configuration(cfg *Configuration) { if cfg.QueueVisibility.UpdateIntervalSeconds == 0 { cfg.QueueVisibility.UpdateIntervalSeconds = DefaultQueueVisibilityUpdateIntervalSeconds } + if cfg.QueueVisibility.ClusterQueues == nil { + cfg.QueueVisibility.ClusterQueues = &ClusterQueueVisibility{ + MaxCount: DefaultClusterQueuesMaxCount, + } + } } diff --git a/apis/config/v1beta1/defaults_test.go b/apis/config/v1beta1/defaults_test.go index a008cd86c2..b2ef277ea0 100644 --- a/apis/config/v1beta1/defaults_test.go +++ b/apis/config/v1beta1/defaults_test.go @@ -57,6 +57,9 @@ func TestSetDefaults_Configuration(t *testing.T) { } defaultQueueVisibility := &QueueVisibility{ UpdateIntervalSeconds: DefaultQueueVisibilityUpdateIntervalSeconds, + ClusterQueues: &ClusterQueueVisibility{ + MaxCount: 10, + }, } podsReadyTimeoutTimeout := metav1.Duration{Duration: defaultPodsReadyTimeout} podsReadyTimeoutOverwrite := metav1.Duration{Duration: time.Minute} @@ -385,6 +388,9 @@ func TestSetDefaults_Configuration(t *testing.T) { }, QueueVisibility: &QueueVisibility{ UpdateIntervalSeconds: 10, + ClusterQueues: &ClusterQueueVisibility{ + MaxCount: 0, + }, }, }, want: &Configuration{ @@ -397,6 +403,9 @@ func TestSetDefaults_Configuration(t *testing.T) { Integrations: defaultIntegrations, QueueVisibility: &QueueVisibility{ UpdateIntervalSeconds: 10, + ClusterQueues: &ClusterQueueVisibility{ + MaxCount: 0, + }, }, }, }, diff --git a/charts/kueue/values.yaml b/charts/kueue/values.yaml index 06c0345f27..fa88220708 100644 --- a/charts/kueue/values.yaml +++ b/charts/kueue/values.yaml @@ -82,9 +82,6 @@ managerConfig: # enable: false # webhookServiceName: "" # webhookSecretName: "" - queueVisibility: - clusterQueues: - maxCount: 10 integrations: frameworks: - "batch/job" diff --git a/cmd/kueue/main.go b/cmd/kueue/main.go index e521999180..6fd782950d 100644 --- a/cmd/kueue/main.go +++ b/cmd/kueue/main.go @@ -25,7 +25,6 @@ import ( // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) // to ensure that exec-entrypoint and run can make use of them. - _ "k8s.io/client-go/plugin/pkg/client/auth" zaplog "go.uber.org/zap" @@ -212,13 +211,7 @@ func setupIndexes(ctx context.Context, mgr ctrl.Manager, cfg *configapi.Configur return err } -func setupControllers( - mgr ctrl.Manager, - cCache *cache.Cache, - queues *queue.Manager, - certsReady chan struct{}, - cfg *configapi.Configuration, - serverVersionFetcher *kubeversion.ServerVersionFetcher) { +func setupControllers(mgr ctrl.Manager, cCache *cache.Cache, queues *queue.Manager, certsReady chan struct{}, cfg *configapi.Configuration, serverVersionFetcher *kubeversion.ServerVersionFetcher) { // The controllers won't work until the webhooks are operating, and the webhook won't work until the // certs are all in place. setupLog.Info("Waiting for certificate generation to complete") diff --git a/cmd/kueue/main_test.go b/cmd/kueue/main_test.go index 3c1e60c100..afddd3d816 100644 --- a/cmd/kueue/main_test.go +++ b/cmd/kueue/main_test.go @@ -101,6 +101,9 @@ integrations: }, QueueVisibility: &config.QueueVisibility{ UpdateIntervalSeconds: config.DefaultQueueVisibilityUpdateIntervalSeconds, + ClusterQueues: &config.ClusterQueueVisibility{ + MaxCount: config.DefaultClusterQueuesMaxCount, + }, }, }, }, diff --git a/config/components/manager/controller_manager_config.yaml b/config/components/manager/controller_manager_config.yaml index 3acb7dd8ff..40a5872c04 100644 --- a/config/components/manager/controller_manager_config.yaml +++ b/config/components/manager/controller_manager_config.yaml @@ -28,9 +28,6 @@ clientConnection: # enable: false # webhookServiceName: "" # webhookSecretName: "" -queueVisibility: - clusterQueues: - maxCount: 10 integrations: frameworks: - "batch/job" diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 899e40b54e..45141b9c8b 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -18,7 +18,6 @@ package config import ( "errors" - "fmt" "io/fs" "os" "path/filepath" @@ -29,7 +28,6 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/util/yaml" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" @@ -231,7 +229,7 @@ kind: Configuration queueVisibility: updateIntervalSeconds: 10 clusterQueues: - maxCount: 10 + maxCount: 0 `), os.FileMode(0600)); err != nil { t.Fatal(err) } @@ -277,6 +275,9 @@ queueVisibility: defaultQueueVisibility := &configapi.QueueVisibility{ UpdateIntervalSeconds: configapi.DefaultQueueVisibilityUpdateIntervalSeconds, + ClusterQueues: &configapi.ClusterQueueVisibility{ + MaxCount: 10, + }, } testcases := []struct { @@ -569,7 +570,7 @@ queueVisibility: QueueVisibility: &configapi.QueueVisibility{ UpdateIntervalSeconds: 10, ClusterQueues: &configapi.ClusterQueueVisibility{ - MaxCount: 10, + MaxCount: 0, }, }, }, @@ -669,6 +670,7 @@ func TestEncode(t *testing.T) { }, "queueVisibility": map[string]any{ "updateIntervalSeconds": int64(configapi.DefaultQueueVisibilityUpdateIntervalSeconds), + "clusterQueues": map[string]any{"maxCount": int64(10)}, }, }, }, @@ -690,50 +692,3 @@ func TestEncode(t *testing.T) { }) } } - -func TestValidate(t *testing.T) { - testcases := []struct { - name string - cfg *configapi.Configuration - wantErr field.ErrorList - }{ - - { - name: "empty", - cfg: &configapi.Configuration{}, - wantErr: nil, - }, - { - name: "invalid queue visibility UpdateIntervalSeconds", - cfg: &configapi.Configuration{ - QueueVisibility: &configapi.QueueVisibility{ - UpdateIntervalSeconds: 0, - }, - }, - wantErr: field.ErrorList{ - field.Invalid(field.NewPath("queueVisibility").Child("updateIntervalSeconds"), 0, fmt.Sprintf("greater than or equal to %d", queueVisibilityClusterQueuesUpdateIntervalSeconds)), - }, - }, - { - name: "invalid queue visibility cluster queue max count", - cfg: &configapi.Configuration{ - QueueVisibility: &configapi.QueueVisibility{ - ClusterQueues: &configapi.ClusterQueueVisibility{ - MaxCount: 4001, - }, - UpdateIntervalSeconds: 1, - }, - }, - wantErr: field.ErrorList{ - field.Invalid(field.NewPath("queueVisibility").Child("clusterQueues").Child("maxCount"), 4001, fmt.Sprintf("must be less than %d", queueVisibilityClusterQueuesMaxValue)), - }, - }, - } - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - if diff := cmp.Diff(tc.wantErr, validate(tc.cfg), cmpopts.IgnoreFields(field.Error{}, "BadValue")); diff != "" { - t.Errorf("Unexpected returned error (-want,+got):\n%s", diff) - } - }) - } -} diff --git a/pkg/config/validation_test.go b/pkg/config/validation_test.go new file mode 100644 index 0000000000..afd6eb2227 --- /dev/null +++ b/pkg/config/validation_test.go @@ -0,0 +1,59 @@ +package config + +import ( + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "k8s.io/apimachinery/pkg/util/validation/field" + + configapi "sigs.k8s.io/kueue/apis/config/v1beta1" +) + +func TestValidate(t *testing.T) { + testcases := []struct { + name string + cfg *configapi.Configuration + wantErr field.ErrorList + }{ + + { + name: "empty", + cfg: &configapi.Configuration{}, + wantErr: nil, + }, + { + name: "invalid queue visibility UpdateIntervalSeconds", + cfg: &configapi.Configuration{ + QueueVisibility: &configapi.QueueVisibility{ + UpdateIntervalSeconds: 0, + }, + }, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("queueVisibility").Child("updateIntervalSeconds"), 0, fmt.Sprintf("greater than or equal to %d", queueVisibilityClusterQueuesUpdateIntervalSeconds)), + }, + }, + { + name: "invalid queue visibility cluster queue max count", + cfg: &configapi.Configuration{ + QueueVisibility: &configapi.QueueVisibility{ + ClusterQueues: &configapi.ClusterQueueVisibility{ + MaxCount: 4001, + }, + UpdateIntervalSeconds: 1, + }, + }, + wantErr: field.ErrorList{ + field.Invalid(field.NewPath("queueVisibility").Child("clusterQueues").Child("maxCount"), 4001, fmt.Sprintf("must be less than %d", queueVisibilityClusterQueuesMaxValue)), + }, + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + if diff := cmp.Diff(tc.wantErr, validate(tc.cfg), cmpopts.IgnoreFields(field.Error{}, "BadValue")); diff != "" { + t.Errorf("Unexpected returned error (-want,+got):\n%s", diff) + } + }) + } +} diff --git a/pkg/controller/core/clusterqueue_controller.go b/pkg/controller/core/clusterqueue_controller.go index a20bb6353c..54707ab940 100644 --- a/pkg/controller/core/clusterqueue_controller.go +++ b/pkg/controller/core/clusterqueue_controller.go @@ -219,6 +219,7 @@ func (r *ClusterQueueReconciler) Delete(e event.DeleteEvent) bool { r.log.V(2).Info("ClusterQueue delete event", "clusterQueue", klog.KObj(cq)) r.cache.DeleteClusterQueue(cq) r.qManager.DeleteClusterQueue(cq) + r.qManager.DeleteSnapshot(cq) metrics.ClearClusterQueueResourceMetrics(cq.Name) r.log.V(2).Info("Cleared resource metrics for deleted ClusterQueue.", "clusterQueue", klog.KObj(cq)) diff --git a/pkg/queue/manager.go b/pkg/queue/manager.go index 3fefa60fd7..21e7355e28 100644 --- a/pkg/queue/manager.go +++ b/pkg/queue/manager.go @@ -625,9 +625,6 @@ func (m *Manager) takeSnapshot(ctx context.Context) { } func (m *Manager) processNextSnapshot(ctx context.Context) bool { - m.wm.Lock() - defer m.wm.Unlock() - log := ctrl.LoggerFrom(ctx).WithName("processNextSnapshot") key, quit := m.queue.Get() @@ -656,7 +653,7 @@ func (m *Manager) processNextSnapshot(ctx context.Context) bool { }) } } - m.snapshots[key.(string)] = workloads + m.SetSnapshot(key.(string), workloads) return true } @@ -676,8 +673,26 @@ func (m *Manager) extractClusterQueue(key interface{}) ClusterQueue { return nil } +func (m *Manager) SetSnapshot(cqName string, workloads []kueue.ClusterQueuePendingWorkload) { + m.wm.Lock() + defer m.wm.Unlock() + m.snapshots[cqName] = workloads +} + func (m *Manager) GetSnapshots() map[string][]kueue.ClusterQueuePendingWorkload { m.wm.RLock() defer m.wm.RUnlock() return m.snapshots } + +func (m *Manager) DeleteSnapshot(cq *kueue.ClusterQueue) { + m.Lock() + defer m.Unlock() + cqImpl := m.clusterQueues[cq.Name] + if cqImpl == nil { + return + } + m.wm.Lock() + defer m.wm.Unlock() + delete(m.snapshots, cq.Name) +} From 82dc8f9dc51158363bdf8b2ba37a67f77ffcf1fe Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Thu, 31 Aug 2023 19:06:46 +0200 Subject: [PATCH 10/26] second complete pass review --- cmd/kueue/main.go | 1 + .../core/clusterqueue_controller.go | 23 +++++++++-------- .../core/clusterqueue_controller_test.go | 15 +++++++++-- pkg/queue/manager.go | 4 +-- .../core/clusterqueue_controller_test.go | 5 +++- .../scheduler/podsready/scheduler_test.go | 7 ++++-- .../scheduler/workload_controller_test.go | 25 +++++++++++++------ 7 files changed, 54 insertions(+), 26 deletions(-) diff --git a/cmd/kueue/main.go b/cmd/kueue/main.go index 6fd782950d..c5fcd19ba2 100644 --- a/cmd/kueue/main.go +++ b/cmd/kueue/main.go @@ -185,6 +185,7 @@ func setupQueueManager(mgr ctrl.Manager, cCache *cache.Cache, cfg *configapi.Con queue.WithQueueVisibilityUpdateInterval(cfg.QueueVisibility.UpdateIntervalSeconds), queue.WithQueueVisibilityClusterQueuesMaxCount(cfg.QueueVisibility.ClusterQueues.MaxCount), ) + // Taking snapshot of cluster queue is enabled when maxcount non-zero if cfg.QueueVisibility.ClusterQueues.MaxCount != 0 { if err := mgr.Add(queues); err != nil { setupLog.Error(err, "Unable to add queue manager to manager") diff --git a/pkg/controller/core/clusterqueue_controller.go b/pkg/controller/core/clusterqueue_controller.go index 54707ab940..98e1b5cec9 100644 --- a/pkg/controller/core/clusterqueue_controller.go +++ b/pkg/controller/core/clusterqueue_controller.go @@ -488,7 +488,7 @@ func (r *ClusterQueueReconciler) updateCqStatusIfChanged( cq.Status.FlavorsUsage = usage cq.Status.AdmittedWorkloads = int32(workloads) cq.Status.PendingWorkloads = int32(pendingWorkloads) - cq.Status.PendingWorkloadsStatus = r.getWorkloadsStatus() + cq.Status.PendingWorkloadsStatus = r.getWorkloadsStatus(cq) meta.SetStatusCondition(&cq.Status.Conditions, metav1.Condition{ Type: kueue.ClusterQueueActive, Status: conditionStatus, @@ -501,16 +501,17 @@ func (r *ClusterQueueReconciler) updateCqStatusIfChanged( return nil } -func (r *ClusterQueueReconciler) getWorkloadsStatus() *kueue.ClusterQueuePendingWorkloadsStatus { - pendingWorkloads := make([]kueue.ClusterQueuePendingWorkload, 0) - for _, workloads := range r.qManager.GetSnapshots() { - pendingWorkloads = append(pendingWorkloads, workloads...) - } - if len(pendingWorkloads) == 0 { - return nil +func (r *ClusterQueueReconciler) getWorkloadsStatus(cq *kueue.ClusterQueue) *kueue.ClusterQueuePendingWorkloadsStatus { + pendingWorkloadsStatus := cq.Status.DeepCopy().PendingWorkloadsStatus + if pendingWorkloadsStatus == nil { + return &kueue.ClusterQueuePendingWorkloadsStatus{ + LastChangeTime: metav1.Time{Time: time.Now()}, + } } - return &kueue.ClusterQueuePendingWorkloadsStatus{ - Head: pendingWorkloads, - LastChangeTime: metav1.Time{Time: time.Now()}, + pendingWorkloads := r.qManager.GetSnapshot(cq.Name) + if !equality.Semantic.DeepEqual(pendingWorkloadsStatus.Head, pendingWorkloads) { + pendingWorkloadsStatus.Head = pendingWorkloads + pendingWorkloadsStatus.LastChangeTime = metav1.Time{Time: time.Now()} } + return pendingWorkloadsStatus } diff --git a/pkg/controller/core/clusterqueue_controller_test.go b/pkg/controller/core/clusterqueue_controller_test.go index 4a28c43cc2..51c606f46a 100644 --- a/pkg/controller/core/clusterqueue_controller_test.go +++ b/pkg/controller/core/clusterqueue_controller_test.go @@ -65,6 +65,7 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "FlavorNotFound", Message: "Can't admit new workloads; some flavors are not found", }}, + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, }, "same condition status": { @@ -76,6 +77,7 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "Ready", Message: "Can admit new workloads", }}, + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, newConditionStatus: metav1.ConditionTrue, newReason: "Ready", @@ -88,6 +90,7 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "Ready", Message: "Can admit new workloads", }}, + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, }, "same condition status with different reason and message": { @@ -99,6 +102,7 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "FlavorNotFound", Message: "Can't admit new workloads; Can't admit new workloads; some flavors are not found", }}, + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, newConditionStatus: metav1.ConditionFalse, newReason: "Terminating", @@ -111,6 +115,7 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "Terminating", Message: "Can't admit new workloads; clusterQueue is terminating", }}, + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, }, "different condition status": { @@ -122,6 +127,7 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "FlavorNotFound", Message: "Can't admit new workloads; some flavors are not found", }}, + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, newConditionStatus: metav1.ConditionTrue, newReason: "Ready", @@ -134,6 +140,7 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "Ready", Message: "Can admit new workloads", }}, + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, }, "different pendingWorkloads with same condition status": { @@ -158,6 +165,7 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "Ready", Message: "Can admit new workloads", }}, + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, }, } @@ -200,9 +208,12 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { if err != nil { t.Errorf("Updating ClusterQueueStatus: %v", err) } - if diff := cmp.Diff(tc.wantCqStatus, cq.Status, + configCmpOpts := []cmp.Option{ cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime"), - cmpopts.EquateEmpty()); len(diff) != 0 { + cmpopts.IgnoreFields(kueue.ClusterQueuePendingWorkloadsStatus{}, "LastChangeTime"), + cmpopts.EquateEmpty(), + } + if diff := cmp.Diff(tc.wantCqStatus, cq.Status, configCmpOpts...); len(diff) != 0 { t.Errorf("unexpected ClusterQueueStatus (-want,+got):\n%s", diff) } }) diff --git a/pkg/queue/manager.go b/pkg/queue/manager.go index 21e7355e28..b6b1cef048 100644 --- a/pkg/queue/manager.go +++ b/pkg/queue/manager.go @@ -679,10 +679,10 @@ func (m *Manager) SetSnapshot(cqName string, workloads []kueue.ClusterQueuePendi m.snapshots[cqName] = workloads } -func (m *Manager) GetSnapshots() map[string][]kueue.ClusterQueuePendingWorkload { +func (m *Manager) GetSnapshot(cqName string) []kueue.ClusterQueuePendingWorkload { m.wm.RLock() defer m.wm.RUnlock() - return m.snapshots + return m.snapshots[cqName] } func (m *Manager) DeleteSnapshot(cq *kueue.ClusterQueue) { diff --git a/test/integration/controller/core/clusterqueue_controller_test.go b/test/integration/controller/core/clusterqueue_controller_test.go index c63b7e683d..2e3be7a6ac 100644 --- a/test/integration/controller/core/clusterqueue_controller_test.go +++ b/test/integration/controller/core/clusterqueue_controller_test.go @@ -306,7 +306,10 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { Message: "Can admit new workloads", }, }, - }, ignoreConditionTimestamps)) + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{ + Head: []kueue.ClusterQueuePendingWorkload{}, + }, + }, ignoreConditionTimestamps, ignoreLastChangeTime)) util.ExpectPendingWorkloadsMetric(clusterQueue, 0, 0) util.ExpectAdmittedActiveWorkloadsMetric(clusterQueue, 0) }) diff --git a/test/integration/scheduler/podsready/scheduler_test.go b/test/integration/scheduler/podsready/scheduler_test.go index dc6ae1eb4b..5bb12efc6b 100644 --- a/test/integration/scheduler/podsready/scheduler_test.go +++ b/test/integration/scheduler/podsready/scheduler_test.go @@ -39,6 +39,7 @@ import ( ) var ignoreCQConditions = cmpopts.IgnoreFields(kueue.ClusterQueueStatus{}, "Conditions") +var ignoreLastChangeTime = cmpopts.IgnoreFields(kueue.ClusterQueuePendingWorkloadsStatus{}, "LastChangeTime") // +kubebuilder:docs-gen:collapse=Imports @@ -265,7 +266,8 @@ var _ = ginkgo.Describe("SchedulerWithWaitForPodsReady", func() { Total: resource.MustParse("2"), }}, }}, - }, ignoreCQConditions)) + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, + }, ignoreCQConditions, ignoreLastChangeTime)) ginkgo.By("wait for the timeout to be exceeded") time.Sleep(podsReadyTimeout) @@ -294,7 +296,8 @@ var _ = ginkgo.Describe("SchedulerWithWaitForPodsReady", func() { Total: resource.MustParse("0"), }}, }}, - }, ignoreCQConditions)) + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, + }, ignoreCQConditions, ignoreLastChangeTime)) ginkgo.By("verify the active workload metric is decreased for the cluster queue") util.ExpectAdmittedActiveWorkloadsMetric(prodClusterQ, 0) diff --git a/test/integration/scheduler/workload_controller_test.go b/test/integration/scheduler/workload_controller_test.go index 4872b86f7c..e7781d2b63 100644 --- a/test/integration/scheduler/workload_controller_test.go +++ b/test/integration/scheduler/workload_controller_test.go @@ -36,6 +36,7 @@ import ( // +kubebuilder:docs-gen:collapse=Imports var ignoreCqCondition = cmpopts.IgnoreFields(kueue.ClusterQueueStatus{}, "Conditions") +var ignoreLastChangeTime = cmpopts.IgnoreFields(kueue.ClusterQueuePendingWorkloadsStatus{}, "LastChangeTime") var _ = ginkgo.Describe("Workload controller with scheduler", func() { var ( @@ -121,7 +122,8 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { Total: resource.MustParse("2"), }}, }}, - }, ignoreCqCondition)) + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, + }, ignoreCqCondition, ignoreLastChangeTime)) }) }) }) @@ -176,7 +178,8 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { Total: resource.MustParse("1"), }}, }}, - }, ignoreCqCondition)) + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, + }, ignoreCqCondition, ignoreLastChangeTime)) }) }) }) @@ -232,7 +235,8 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { }, }, }}, - }, ignoreCqCondition)) + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, + }, ignoreCqCondition, ignoreLastChangeTime)) }) ginkgo.By("Check podSets spec", func() { @@ -274,7 +278,8 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { }, }, }}, - }, ignoreCqCondition)) + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, + }, ignoreCqCondition, ignoreLastChangeTime)) }) ginkgo.By("Check podSets spec", func() { @@ -333,7 +338,8 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { Total: resource.MustParse("1"), }}, }}, - }, ignoreCqCondition)) + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, + }, ignoreCqCondition, ignoreLastChangeTime)) }) ginkgo.By("Check podSets spec", func() { @@ -436,7 +442,8 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { Total: resource.MustParse("5"), }}, }}, - }, ignoreCqCondition)) + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, + }, ignoreCqCondition, ignoreLastChangeTime)) }) }) }) @@ -526,7 +533,8 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { Total: resource.MustParse("5"), }}, }}, - }, ignoreCqCondition)) + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, + }, ignoreCqCondition, ignoreLastChangeTime)) }) }) }) @@ -560,7 +568,8 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { Total: resource.MustParse("0"), }}, }}, - }, ignoreCqCondition)) + PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, + }, ignoreCqCondition, ignoreLastChangeTime)) }) gomega.Expect(util.DeleteNamespace(ctx, k8sClient, ns)).To(gomega.Succeed()) util.ExpectClusterQueueToBeDeleted(ctx, k8sClient, clusterQueue, true) From 8368b761907354dcfb9412f515d0702f679d6e78 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Fri, 1 Sep 2023 17:13:53 +0200 Subject: [PATCH 11/26] review fixes --- .../core/clusterqueue_controller.go | 92 ++++++++++++++----- pkg/controller/core/core.go | 9 +- pkg/queue/cluster_queue_impl.go | 26 ++++-- pkg/queue/cluster_queue_interface.go | 2 +- pkg/queue/manager.go | 68 ++++++-------- pkg/util/heap/heap.go | 4 +- .../integration/controller/core/suite_test.go | 3 + 7 files changed, 128 insertions(+), 76 deletions(-) diff --git a/pkg/controller/core/clusterqueue_controller.go b/pkg/controller/core/clusterqueue_controller.go index 98e1b5cec9..146e071ad9 100644 --- a/pkg/controller/core/clusterqueue_controller.go +++ b/pkg/controller/core/clusterqueue_controller.go @@ -52,32 +52,70 @@ type ClusterQueueUpdateWatcher interface { // ClusterQueueReconciler reconciles a ClusterQueue object type ClusterQueueReconciler struct { - client client.Client - log logr.Logger - qManager *queue.Manager - cache *cache.Cache - wlUpdateCh chan event.GenericEvent - rfUpdateCh chan event.GenericEvent - watchers []ClusterQueueUpdateWatcher - reportResourceMetrics bool + client client.Client + log logr.Logger + qManager *queue.Manager + cache *cache.Cache + wlUpdateCh chan event.GenericEvent + rfUpdateCh chan event.GenericEvent + watchers []ClusterQueueUpdateWatcher + reportResourceMetrics bool + queueVisibilityUpdateInterval time.Duration } +type ClusterQueueReconcilerOptions struct { + Watchers []ClusterQueueUpdateWatcher + ReportResourceMetrics bool + QueueVisibilityUpdateInterval time.Duration +} + +// Option configures the reconciler. +type ClusterQueueReconcilerOption func(*ClusterQueueReconcilerOptions) + +func WithWatchers(watchers ...ClusterQueueUpdateWatcher) ClusterQueueReconcilerOption { + return func(o *ClusterQueueReconcilerOptions) { + o.Watchers = watchers + } +} + +// WithReportResourceMetrics indicates if the controller should reconcile +// jobs that don't set the queue name annotation. +func WithReportResourceMetrics(report bool) ClusterQueueReconcilerOption { + return func(o *ClusterQueueReconcilerOptions) { + o.ReportResourceMetrics = report + } +} + +// WithQueueVisibilityMaxCount indicates if the controller should reconcile +// jobs that don't set the queue name annotation. +func WithQueueVisibilityUpdateInterval(interval int32) ClusterQueueReconcilerOption { + return func(o *ClusterQueueReconcilerOptions) { + o.QueueVisibilityUpdateInterval = time.Duration(interval) * time.Second + } +} + +var DefaultOptions = ClusterQueueReconcilerOptions{} + func NewClusterQueueReconciler( client client.Client, qMgr *queue.Manager, cache *cache.Cache, - resourceMetrics bool, - watchers ...ClusterQueueUpdateWatcher, + opts ...ClusterQueueReconcilerOption, ) *ClusterQueueReconciler { + options := DefaultOptions + for _, opt := range opts { + opt(&options) + } return &ClusterQueueReconciler{ - client: client, - log: ctrl.Log.WithName("cluster-queue-reconciler"), - qManager: qMgr, - cache: cache, - wlUpdateCh: make(chan event.GenericEvent, updateChBuffer), - rfUpdateCh: make(chan event.GenericEvent, updateChBuffer), - watchers: watchers, - reportResourceMetrics: resourceMetrics, + client: client, + log: ctrl.Log.WithName("cluster-queue-reconciler"), + qManager: qMgr, + cache: cache, + wlUpdateCh: make(chan event.GenericEvent, updateChBuffer), + rfUpdateCh: make(chan event.GenericEvent, updateChBuffer), + watchers: options.Watchers, + reportResourceMetrics: options.ReportResourceMetrics, + queueVisibilityUpdateInterval: options.QueueVisibilityUpdateInterval, } } @@ -502,16 +540,20 @@ func (r *ClusterQueueReconciler) updateCqStatusIfChanged( } func (r *ClusterQueueReconciler) getWorkloadsStatus(cq *kueue.ClusterQueue) *kueue.ClusterQueuePendingWorkloadsStatus { - pendingWorkloadsStatus := cq.Status.DeepCopy().PendingWorkloadsStatus - if pendingWorkloadsStatus == nil { + if cq.Status.PendingWorkloadsStatus == nil { return &kueue.ClusterQueuePendingWorkloadsStatus{ + Head: r.qManager.GetSnapshot(cq.Name), LastChangeTime: metav1.Time{Time: time.Now()}, } } - pendingWorkloads := r.qManager.GetSnapshot(cq.Name) - if !equality.Semantic.DeepEqual(pendingWorkloadsStatus.Head, pendingWorkloads) { - pendingWorkloadsStatus.Head = pendingWorkloads - pendingWorkloadsStatus.LastChangeTime = metav1.Time{Time: time.Now()} + if time.Since(cq.Status.PendingWorkloadsStatus.LastChangeTime.Time) >= r.queueVisibilityUpdateInterval { + pendingWorkloads := r.qManager.GetSnapshot(cq.Name) + if !equality.Semantic.DeepEqual(cq.Status.PendingWorkloadsStatus.Head, pendingWorkloads) { + return &kueue.ClusterQueuePendingWorkloadsStatus{ + Head: pendingWorkloads, + LastChangeTime: metav1.Time{Time: time.Now()}, + } + } } - return pendingWorkloadsStatus + return cq.Status.PendingWorkloadsStatus } diff --git a/pkg/controller/core/core.go b/pkg/controller/core/core.go index ae96536811..690a919a20 100644 --- a/pkg/controller/core/core.go +++ b/pkg/controller/core/core.go @@ -40,7 +40,14 @@ func SetupControllers(mgr ctrl.Manager, qManager *queue.Manager, cc *cache.Cache return "LocalQueue", err } - cqRec := NewClusterQueueReconciler(mgr.GetClient(), qManager, cc, cfg.Metrics.EnableClusterQueueResources, rfRec) + cqRec := NewClusterQueueReconciler( + mgr.GetClient(), + qManager, + cc, + WithQueueVisibilityUpdateInterval(cfg.QueueVisibility.UpdateIntervalSeconds), + WithReportResourceMetrics(cfg.Metrics.EnableClusterQueueResources), + WithWatchers(rfRec), + ) rfRec.AddUpdateWatcher(cqRec) if err := cqRec.SetupWithManager(mgr); err != nil { return "ClusterQueue", err diff --git a/pkg/queue/cluster_queue_impl.go b/pkg/queue/cluster_queue_impl.go index 3682252ef0..4c1a37326c 100644 --- a/pkg/queue/cluster_queue_impl.go +++ b/pkg/queue/cluster_queue_impl.go @@ -18,6 +18,8 @@ package queue import ( "context" + "sort" + "sync" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" @@ -39,6 +41,7 @@ type clusterQueueBase struct { heap heap.Heap cohort string namespaceSelector labels.Selector + rw sync.RWMutex // inadmissibleWorkloads are workloads that have been tried at least once and couldn't be admitted. inadmissibleWorkloads map[string]*workload.Info @@ -58,6 +61,7 @@ func newClusterQueueImpl(keyFunc func(obj interface{}) string, lessFunc func(a, heap: heap.New(keyFunc, lessFunc), inadmissibleWorkloads: make(map[string]*workload.Info), queueInadmissibleCycle: -1, + rw: sync.RWMutex{}, } } @@ -221,18 +225,24 @@ func (c *clusterQueueBase) DumpInadmissible() (sets.Set[string], bool) { return elements, true } -func (c *clusterQueueBase) Snapshot(maxCount int32) ([]*kueue.Workload, bool) { - if c.heap.Len() == 0 { +func (c *clusterQueueBase) Snapshot() ([]*workload.Info, bool) { + c.rw.RLock() + defer c.rw.RUnlock() + totalLen := c.heap.Len() + len(c.inadmissibleWorkloads) + if totalLen == 0 { return nil, false } - elements := make([]*kueue.Workload, 0, c.heap.Len()) - for i, e := range c.heap.List() { - if int32(i) > maxCount { - return elements, true - } + elements := make([]*workload.Info, 0, totalLen) + for _, e := range c.heap.List() { info := e.(*workload.Info) - elements = append(elements, info.Obj) + elements = append(elements, info) + } + for _, e := range c.inadmissibleWorkloads { + elements = append(elements, e) } + sort.Slice(elements, func(i, j int) bool { + return queueOrdering(elements[i], elements[j]) + }) return elements, true } diff --git a/pkg/queue/cluster_queue_interface.go b/pkg/queue/cluster_queue_interface.go index 34f21baf47..f9ab1a95d7 100644 --- a/pkg/queue/cluster_queue_interface.go +++ b/pkg/queue/cluster_queue_interface.go @@ -95,7 +95,7 @@ type ClusterQueue interface { // Snapshot returns a copy of the current workloads in the heap of // this ClusterQueue. It returns false if the queue is empty. // Otherwise returns true. - Snapshot(maxCount int32) ([]*kueue.Workload, bool) + Snapshot() ([]*workload.Info, bool) // Info returns workload.Info for the workload key. // Users of this method should not modify the returned object. Info(string) *workload.Info diff --git a/pkg/queue/manager.go b/pkg/queue/manager.go index b6b1cef048..ba58cecf2e 100644 --- a/pkg/queue/manager.go +++ b/pkg/queue/manager.go @@ -53,11 +53,11 @@ type Manager struct { clusterQueues map[string]ClusterQueue localQueues map[string]*LocalQueue - wm sync.RWMutex - queue workqueue.Interface - snapshots map[string][]kueue.ClusterQueuePendingWorkload + snapshotsMutex sync.RWMutex + snapshotsQueue workqueue.Interface + snapshots map[string][]kueue.ClusterQueuePendingWorkload - queueVisibilityUpdateInterval int32 + queueVisibilityUpdateInterval time.Duration queueVisibilityClusterQueuesMaxCount int32 // Key is cohort's name. Value is a set of associated ClusterQueue names. @@ -65,7 +65,7 @@ type Manager struct { } type Options struct { - QueueVisibilityUpdateInterval int32 + QueueVisibilityUpdateInterval time.Duration QueueVisibilityClusterQueuesMaxCount int32 } @@ -76,7 +76,7 @@ type Option func(*Options) // jobs that don't set the queue name annotation. func WithQueueVisibilityUpdateInterval(interval int32) Option { return func(o *Options) { - o.QueueVisibilityUpdateInterval = interval + o.QueueVisibilityUpdateInterval = time.Duration(interval) * time.Second } } @@ -101,8 +101,8 @@ func NewManager(client client.Client, checker StatusChecker, opts ...Option) *Ma localQueues: make(map[string]*LocalQueue), clusterQueues: make(map[string]ClusterQueue), cohorts: make(map[string]sets.Set[string]), - wm: sync.RWMutex{}, - queue: workqueue.New(), + snapshotsMutex: sync.RWMutex{}, + snapshotsQueue: workqueue.New(), snapshots: make(map[string][]kueue.ClusterQueuePendingWorkload, 0), queueVisibilityUpdateInterval: options.QueueVisibilityUpdateInterval, queueVisibilityClusterQueuesMaxCount: options.QueueVisibilityClusterQueuesMaxCount, @@ -496,7 +496,7 @@ func (m *Manager) Heads(ctx context.Context) []workload.Info { } } -// Dump is a dump of the queues and it's elements (ordered). +// Dump is a dump of the queues and it's elements (unordered). // Only use for testing purposes. func (m *Manager) Dump() map[string]sets.Set[string] { m.Lock() @@ -596,26 +596,24 @@ func (m *Manager) reportPendingWorkloads(cqName string, cq ClusterQueue) { } func (m *Manager) Start(ctx context.Context) error { - defer m.queue.ShutDown() - - queueVisibilityUpdateInterval := time.Duration(m.queueVisibilityUpdateInterval) * time.Second + defer m.snapshotsQueue.ShutDown() for i := 0; i < snapshotWorkers; i++ { - go wait.UntilWithContext(ctx, m.takeSnapshot, queueVisibilityUpdateInterval) + go wait.UntilWithContext(ctx, m.takeSnapshot, m.queueVisibilityUpdateInterval) } - go wait.UntilWithContext(ctx, m.enqueueSnapshotDelayed, queueVisibilityUpdateInterval) + go wait.UntilWithContext(ctx, m.enqueueTakeSnapshot, m.queueVisibilityUpdateInterval) <-ctx.Done() return nil } -func (m *Manager) enqueueSnapshotDelayed(ctx context.Context) { +func (m *Manager) enqueueTakeSnapshot(ctx context.Context) { m.RLock() defer m.RUnlock() for cq := range m.clusterQueues { - m.queue.Add(cq) + m.snapshotsQueue.Add(cq) } } @@ -627,14 +625,14 @@ func (m *Manager) takeSnapshot(ctx context.Context) { func (m *Manager) processNextSnapshot(ctx context.Context) bool { log := ctrl.LoggerFrom(ctx).WithName("processNextSnapshot") - key, quit := m.queue.Get() + key, quit := m.snapshotsQueue.Get() if quit { return false } startTime := time.Now() defer func() { - log.V(4).Info("Finished snapshot job", "key", key, "elapsed", time.Since(startTime)) + log.V(2).Info("Finished snapshot job", "key", key, "elapsed", time.Since(startTime)) }() cq := m.extractClusterQueue(key) @@ -642,14 +640,17 @@ func (m *Manager) processNextSnapshot(ctx context.Context) bool { return false } - defer m.queue.Done(key) + defer m.snapshotsQueue.Done(key) workloads := make([]kueue.ClusterQueuePendingWorkload, 0) - if elements, ok := cq.Snapshot(m.queueVisibilityClusterQueuesMaxCount); ok { - for _, wl := range elements { + if elements, ok := cq.Snapshot(); ok { + for index, info := range elements { + if int32(index) > m.queueVisibilityClusterQueuesMaxCount || info == nil { + continue + } workloads = append(workloads, kueue.ClusterQueuePendingWorkload{ - Name: wl.Name, - Namespace: wl.Namespace, + Name: info.Obj.Name, + Namespace: info.Obj.Namespace, }) } } @@ -661,38 +662,29 @@ func (m *Manager) processNextSnapshot(ctx context.Context) bool { func (m *Manager) extractClusterQueue(key interface{}) ClusterQueue { m.RLock() defer m.RUnlock() - if len(m.clusterQueues) == 0 { return nil } - if cq := m.clusterQueues[key.(string)]; cq != nil { return cq } - return nil } func (m *Manager) SetSnapshot(cqName string, workloads []kueue.ClusterQueuePendingWorkload) { - m.wm.Lock() - defer m.wm.Unlock() + m.snapshotsMutex.Lock() + defer m.snapshotsMutex.Unlock() m.snapshots[cqName] = workloads } func (m *Manager) GetSnapshot(cqName string) []kueue.ClusterQueuePendingWorkload { - m.wm.RLock() - defer m.wm.RUnlock() + m.snapshotsMutex.RLock() + defer m.snapshotsMutex.RUnlock() return m.snapshots[cqName] } func (m *Manager) DeleteSnapshot(cq *kueue.ClusterQueue) { - m.Lock() - defer m.Unlock() - cqImpl := m.clusterQueues[cq.Name] - if cqImpl == nil { - return - } - m.wm.Lock() - defer m.wm.Unlock() + m.snapshotsMutex.Lock() + defer m.snapshotsMutex.Unlock() delete(m.snapshots, cq.Name) } diff --git a/pkg/util/heap/heap.go b/pkg/util/heap/heap.go index ac21f87755..2cdd7012ca 100644 --- a/pkg/util/heap/heap.go +++ b/pkg/util/heap/heap.go @@ -20,7 +20,6 @@ package heap import ( "container/heap" - "sort" ) // lessFunc is a function that receives two items and returns true if the first @@ -170,9 +169,8 @@ func (h *Heap) Len() int { return h.data.Len() } -// List returns a sorted list of all the items. +// List returns a list of all the items. func (h *Heap) List() []interface{} { - sort.Sort(&h.data) list := make([]interface{}, 0, h.Len()) for _, item := range h.data.items { list = append(list, item.obj) diff --git a/test/integration/controller/core/suite_test.go b/test/integration/controller/core/suite_test.go index 18c53e2722..3f3581264a 100644 --- a/test/integration/controller/core/suite_test.go +++ b/test/integration/controller/core/suite_test.go @@ -84,6 +84,9 @@ func managerSetup(mgr manager.Manager, ctx context.Context) { controllersCfg := &config.Configuration{} controllersCfg.Metrics.EnableClusterQueueResources = true + controllersCfg.QueueVisibility = &config.QueueVisibility{ + UpdateIntervalSeconds: 2, + } failedCtrl, err := core.SetupControllers(mgr, queues, cCache, controllersCfg) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) From 17698ca20f8d851fa1b4f8addec293469416a523 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Mon, 4 Sep 2023 15:03:35 +0200 Subject: [PATCH 12/26] added integration tests --- pkg/queue/cluster_queue_impl.go | 5 - pkg/queue/manager.go | 20 +-- .../core/clusterqueue_controller_test.go | 154 +++++++++++++++++- .../integration/controller/core/suite_test.go | 19 ++- 4 files changed, 165 insertions(+), 33 deletions(-) diff --git a/pkg/queue/cluster_queue_impl.go b/pkg/queue/cluster_queue_impl.go index 4c1a37326c..fc81355ccb 100644 --- a/pkg/queue/cluster_queue_impl.go +++ b/pkg/queue/cluster_queue_impl.go @@ -19,7 +19,6 @@ package queue import ( "context" "sort" - "sync" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" @@ -41,7 +40,6 @@ type clusterQueueBase struct { heap heap.Heap cohort string namespaceSelector labels.Selector - rw sync.RWMutex // inadmissibleWorkloads are workloads that have been tried at least once and couldn't be admitted. inadmissibleWorkloads map[string]*workload.Info @@ -61,7 +59,6 @@ func newClusterQueueImpl(keyFunc func(obj interface{}) string, lessFunc func(a, heap: heap.New(keyFunc, lessFunc), inadmissibleWorkloads: make(map[string]*workload.Info), queueInadmissibleCycle: -1, - rw: sync.RWMutex{}, } } @@ -226,8 +223,6 @@ func (c *clusterQueueBase) DumpInadmissible() (sets.Set[string], bool) { } func (c *clusterQueueBase) Snapshot() ([]*workload.Info, bool) { - c.rw.RLock() - defer c.rw.RUnlock() totalLen := c.heap.Len() + len(c.inadmissibleWorkloads) if totalLen == 0 { return nil, false diff --git a/pkg/queue/manager.go b/pkg/queue/manager.go index ba58cecf2e..92b1cf679f 100644 --- a/pkg/queue/manager.go +++ b/pkg/queue/manager.go @@ -635,17 +635,15 @@ func (m *Manager) processNextSnapshot(ctx context.Context) bool { log.V(2).Info("Finished snapshot job", "key", key, "elapsed", time.Since(startTime)) }() - cq := m.extractClusterQueue(key) - if cq == nil { - return false - } - defer m.snapshotsQueue.Done(key) workloads := make([]kueue.ClusterQueuePendingWorkload, 0) - if elements, ok := cq.Snapshot(); ok { + if elements, ok := m.getSnapshotFromClusterQueue(key); ok { for index, info := range elements { - if int32(index) > m.queueVisibilityClusterQueuesMaxCount || info == nil { + if int32(index) >= m.queueVisibilityClusterQueuesMaxCount { + break + } + if info == nil { continue } workloads = append(workloads, kueue.ClusterQueuePendingWorkload{ @@ -659,16 +657,16 @@ func (m *Manager) processNextSnapshot(ctx context.Context) bool { return true } -func (m *Manager) extractClusterQueue(key interface{}) ClusterQueue { +func (m *Manager) getSnapshotFromClusterQueue(key interface{}) ([]*workload.Info, bool) { m.RLock() defer m.RUnlock() if len(m.clusterQueues) == 0 { - return nil + return nil, false } if cq := m.clusterQueues[key.(string)]; cq != nil { - return cq + return cq.Snapshot() } - return nil + return nil, false } func (m *Manager) SetSnapshot(cqName string, workloads []kueue.ClusterQueuePendingWorkload) { diff --git a/test/integration/controller/core/clusterqueue_controller_test.go b/test/integration/controller/core/clusterqueue_controller_test.go index 2e3be7a6ac..ad939cb0d4 100644 --- a/test/integration/controller/core/clusterqueue_controller_test.go +++ b/test/integration/controller/core/clusterqueue_controller_test.go @@ -17,6 +17,8 @@ limitations under the License. package core import ( + "time" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" @@ -271,15 +273,7 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { Message: "Can admit new workloads", }, }, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{ - Head: []kueue.ClusterQueuePendingWorkload{ - { - Name: "six", - Namespace: ns.Name, - }, - }, - }, - }, ignoreConditionTimestamps, ignoreLastChangeTime)) + }, ignoreConditionTimestamps, ignorePendingWorkloadsStatus)) util.ExpectPendingWorkloadsMetric(clusterQueue, 1, 0) util.ExpectAdmittedActiveWorkloadsMetric(clusterQueue, 4) @@ -616,4 +610,146 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { }, util.Timeout, util.Interval).Should(testing.BeNotFoundError()) }) }) + + ginkgo.When("Reconciling clusterQueue pending workload status", func() { + var ( + clusterQueue *kueue.ClusterQueue + localQueue *kueue.LocalQueue + onDemandFlavor *kueue.ResourceFlavor + ) + + ginkgo.BeforeEach(func() { + onDemandFlavor = testing.MakeResourceFlavor(flavorOnDemand).Obj() + gomega.Expect(k8sClient.Create(ctx, onDemandFlavor)).To(gomega.Succeed()) + clusterQueue = testing.MakeClusterQueue("cluster-queue"). + ResourceGroup( + *testing.MakeFlavorQuotas(flavorOnDemand). + Resource(corev1.ResourceCPU, "5", "5").Obj(), + ). + Obj() + gomega.Expect(k8sClient.Create(ctx, clusterQueue)).To(gomega.Succeed()) + localQueue = testing.MakeLocalQueue("queue", ns.Name).ClusterQueue(clusterQueue.Name).Obj() + gomega.Expect(k8sClient.Create(ctx, localQueue)).To(gomega.Succeed()) + }) + + ginkgo.AfterEach(func() { + util.ExpectClusterQueueToBeDeleted(ctx, k8sClient, clusterQueue, true) + util.ExpectResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true) + }) + + ginkgo.It("Should update of the pending workloads when a new workload is scheduled", func() { + workloads := []*kueue.Workload{ + testing.MakeWorkload("one", ns.Name).Queue(localQueue.Name). + Request(corev1.ResourceCPU, "2").Request(resourceGPU, "2").Obj(), + testing.MakeWorkload("two", ns.Name).Queue(localQueue.Name). + Request(corev1.ResourceCPU, "3").Request(resourceGPU, "3").Obj(), + } + + ginkgo.By("Creating workloads") + for _, w := range workloads { + gomega.Expect(k8sClient.Create(ctx, w)).To(gomega.Succeed()) + } + gomega.Eventually(func() *kueue.ClusterQueuePendingWorkloadsStatus { + var updatedCq kueue.ClusterQueue + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(clusterQueue), &updatedCq)).To(gomega.Succeed()) + return updatedCq.Status.PendingWorkloadsStatus + }, util.Timeout, util.Interval).Should(gomega.BeComparableTo(&kueue.ClusterQueuePendingWorkloadsStatus{ + Head: []kueue.ClusterQueuePendingWorkload{ + { + Name: "one", + Namespace: ns.Name, + }, + { + Name: "two", + Namespace: ns.Name, + }, + }, + }, + ignoreLastChangeTime, + cmpopts.SortSlices(func(i, j kueue.ClusterQueuePendingWorkload) bool { + return i.Name < j.Name + }))) + + ginkgo.By("Creating a new workload") + newWorkloads := []*kueue.Workload{ + testing.MakeWorkload("three", ns.Name).Queue(localQueue.Name). + Request(corev1.ResourceCPU, "2").Request(resourceGPU, "2").Obj(), + testing.MakeWorkload("four", ns.Name).Queue(localQueue.Name). + Request(corev1.ResourceCPU, "3").Request(resourceGPU, "3").Obj(), + } + for _, w := range newWorkloads { + gomega.Expect(k8sClient.Create(ctx, w)).To(gomega.Succeed()) + time.Sleep(time.Second) + } + + ginkgo.By("Cut off the head of pending workloads when the number of pending workloads exceeds MaxCount") + gomega.Eventually(func() *kueue.ClusterQueuePendingWorkloadsStatus { + var updatedCq kueue.ClusterQueue + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(clusterQueue), &updatedCq)).To(gomega.Succeed()) + return updatedCq.Status.PendingWorkloadsStatus + }, util.Timeout, util.Interval).Should(gomega.BeComparableTo(&kueue.ClusterQueuePendingWorkloadsStatus{ + Head: []kueue.ClusterQueuePendingWorkload{ + { + Name: "one", + Namespace: ns.Name, + }, + { + Name: "two", + Namespace: ns.Name, + }, + { + Name: "three", + Namespace: ns.Name, + }, + }, + }, ignoreLastChangeTime, cmpopts.SortSlices(func(i, j kueue.ClusterQueuePendingWorkload) bool { + return i.Name < j.Name + }))) + + ginkgo.By("Admitting workloads") + admissions := []*kueue.Admission{ + testing.MakeAdmission(clusterQueue.Name). + Assignment(corev1.ResourceCPU, flavorOnDemand, "2").Obj(), + testing.MakeAdmission(clusterQueue.Name). + Assignment(corev1.ResourceCPU, flavorOnDemand, "3").Obj(), + testing.MakeAdmission(clusterQueue.Name). + Assignment(corev1.ResourceCPU, flavorOnDemand, "1").Obj(), + nil, + } + for i, w := range workloads { + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(w), &newWL)).To(gomega.Succeed()) + if admissions[i] != nil { + return util.SetAdmission(ctx, k8sClient, &newWL, admissions[i]) + } + return k8sClient.Status().Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + } + + gomega.Eventually(func() *kueue.ClusterQueuePendingWorkloadsStatus { + var updatedCQ kueue.ClusterQueue + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(clusterQueue), &updatedCQ)).To(gomega.Succeed()) + return updatedCQ.Status.PendingWorkloadsStatus + }, util.Timeout, util.Interval).Should(gomega.BeComparableTo(&kueue.ClusterQueuePendingWorkloadsStatus{ + Head: []kueue.ClusterQueuePendingWorkload{ + { + Name: "three", + Namespace: ns.Name, + }, + { + Name: "four", + Namespace: ns.Name, + }, + }, + }, ignoreLastChangeTime, cmpopts.SortSlices(func(i, j kueue.ClusterQueuePendingWorkload) bool { + return i.Name < j.Name + }))) + + ginkgo.By("Finishing workload", func() { + util.FinishWorkloads(ctx, k8sClient, workloads...) + util.FinishWorkloads(ctx, k8sClient, newWorkloads...) + }) + }) + }) }) diff --git a/test/integration/controller/core/suite_test.go b/test/integration/controller/core/suite_test.go index 3f3581264a..44e1027d30 100644 --- a/test/integration/controller/core/suite_test.go +++ b/test/integration/controller/core/suite_test.go @@ -72,22 +72,25 @@ func managerSetup(mgr manager.Manager, ctx context.Context) { failedWebhook, err := webhooks.Setup(mgr) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "webhook", failedWebhook) + controllersCfg := &config.Configuration{} + controllersCfg.Metrics.EnableClusterQueueResources = true + controllersCfg.QueueVisibility = &config.QueueVisibility{ + UpdateIntervalSeconds: 1, + ClusterQueues: &config.ClusterQueueVisibility{ + MaxCount: 3, + }, + } + cCache := cache.New(mgr.GetClient()) queues := queue.NewManager( mgr.GetClient(), cCache, - queue.WithQueueVisibilityUpdateInterval(1), - queue.WithQueueVisibilityClusterQueuesMaxCount(10), + queue.WithQueueVisibilityUpdateInterval(controllersCfg.QueueVisibility.UpdateIntervalSeconds), + queue.WithQueueVisibilityClusterQueuesMaxCount(controllersCfg.QueueVisibility.ClusterQueues.MaxCount), ) err = mgr.Add(queues) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - controllersCfg := &config.Configuration{} - controllersCfg.Metrics.EnableClusterQueueResources = true - controllersCfg.QueueVisibility = &config.QueueVisibility{ - UpdateIntervalSeconds: 2, - } - failedCtrl, err := core.SetupControllers(mgr, queues, cCache, controllersCfg) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) } From 10c371fc3d5d2c87e69ebdb66e5051dc8d7b30ec Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Mon, 4 Sep 2023 15:34:53 +0200 Subject: [PATCH 13/26] additional check --- pkg/controller/core/core.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/controller/core/core.go b/pkg/controller/core/core.go index 690a919a20..271d4c28b8 100644 --- a/pkg/controller/core/core.go +++ b/pkg/controller/core/core.go @@ -44,7 +44,7 @@ func SetupControllers(mgr ctrl.Manager, qManager *queue.Manager, cc *cache.Cache mgr.GetClient(), qManager, cc, - WithQueueVisibilityUpdateInterval(cfg.QueueVisibility.UpdateIntervalSeconds), + WithQueueVisibilityUpdateInterval(updateIntervalSeconds(cfg)), WithReportResourceMetrics(cfg.Metrics.EnableClusterQueueResources), WithWatchers(rfRec), ) @@ -64,3 +64,10 @@ func podsReadyTimeout(cfg *config.Configuration) *time.Duration { } return nil } + +func updateIntervalSeconds(cfg *config.Configuration) int32 { + if cfg.QueueVisibility != nil { + return cfg.QueueVisibility.UpdateIntervalSeconds + } + return 0 +} From c1f52bba46a95521086a2e64baf96b303afda297 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Mon, 4 Sep 2023 15:55:54 +0200 Subject: [PATCH 14/26] resolve rebase --- .../integration/controller/core/clusterqueue_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/controller/core/clusterqueue_controller_test.go b/test/integration/controller/core/clusterqueue_controller_test.go index ad939cb0d4..8ecb0e741d 100644 --- a/test/integration/controller/core/clusterqueue_controller_test.go +++ b/test/integration/controller/core/clusterqueue_controller_test.go @@ -721,7 +721,7 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { var newWL kueue.Workload gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(w), &newWL)).To(gomega.Succeed()) if admissions[i] != nil { - return util.SetAdmission(ctx, k8sClient, &newWL, admissions[i]) + return util.SetQuotaReservation(ctx, k8sClient, &newWL, admissions[i]) } return k8sClient.Status().Update(ctx, &newWL) }, util.Timeout, util.Interval).Should(gomega.Succeed()) From 5d6cee166d9a135d2485ba5c8501cbb67185a6dc Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Tue, 5 Sep 2023 09:33:31 +0200 Subject: [PATCH 15/26] fix types --- apis/config/v1beta1/configuration_types.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apis/config/v1beta1/configuration_types.go b/apis/config/v1beta1/configuration_types.go index d637f01fcb..2081cbf98c 100644 --- a/apis/config/v1beta1/configuration_types.go +++ b/apis/config/v1beta1/configuration_types.go @@ -239,13 +239,13 @@ type QueueVisibility struct { // UpdateIntervalSeconds specifies the time interval for updates to the structure // of the top pending workloads in the queues. // The minimum value is 1. - // Defaults to 5s. + // Defaults to 5. UpdateIntervalSeconds int32 `json:"updateIntervalSeconds,omitempty"` } type ClusterQueueVisibility struct { // MaxCount indicates the maximal number of pending workloads exposed in the - // cluster queue status. When the value is set to 0, then LocalQueue + // cluster queue status. When the value is set to 0, then ClusterQueue // visibility updates are disabled. // The maximal value is 4000. // Defaults to 10. From 0d7a46772fe5bb26b0eeeddbc1b81c7e5721f110 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Tue, 5 Sep 2023 12:41:14 +0200 Subject: [PATCH 16/26] another round of reviews --- pkg/queue/cluster_queue_impl.go | 3 --- .../core/clusterqueue_controller_test.go | 19 +++++++++++++++---- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/pkg/queue/cluster_queue_impl.go b/pkg/queue/cluster_queue_impl.go index fc81355ccb..74095fb4bc 100644 --- a/pkg/queue/cluster_queue_impl.go +++ b/pkg/queue/cluster_queue_impl.go @@ -224,9 +224,6 @@ func (c *clusterQueueBase) DumpInadmissible() (sets.Set[string], bool) { func (c *clusterQueueBase) Snapshot() ([]*workload.Info, bool) { totalLen := c.heap.Len() + len(c.inadmissibleWorkloads) - if totalLen == 0 { - return nil, false - } elements := make([]*workload.Info, 0, totalLen) for _, e := range c.heap.List() { info := e.(*workload.Info) diff --git a/test/integration/controller/core/clusterqueue_controller_test.go b/test/integration/controller/core/clusterqueue_controller_test.go index 8ecb0e741d..071ef34dd6 100644 --- a/test/integration/controller/core/clusterqueue_controller_test.go +++ b/test/integration/controller/core/clusterqueue_controller_test.go @@ -300,10 +300,7 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { Message: "Can admit new workloads", }, }, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{ - Head: []kueue.ClusterQueuePendingWorkload{}, - }, - }, ignoreConditionTimestamps, ignoreLastChangeTime)) + }, ignoreConditionTimestamps, ignorePendingWorkloadsStatus)) util.ExpectPendingWorkloadsMetric(clusterQueue, 0, 0) util.ExpectAdmittedActiveWorkloadsMetric(clusterQueue, 0) }) @@ -645,6 +642,12 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { Request(corev1.ResourceCPU, "3").Request(resourceGPU, "3").Obj(), } + gomega.Eventually(func() *kueue.ClusterQueuePendingWorkloadsStatus { + var updatedCq kueue.ClusterQueue + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(clusterQueue), &updatedCq)).To(gomega.Succeed()) + return updatedCq.Status.PendingWorkloadsStatus + }, util.Timeout, util.Interval).Should(gomega.BeComparableTo(&kueue.ClusterQueuePendingWorkloadsStatus{}, ignoreLastChangeTime)) + ginkgo.By("Creating workloads") for _, w := range workloads { gomega.Expect(k8sClient.Create(ctx, w)).To(gomega.Succeed()) @@ -750,6 +753,14 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { util.FinishWorkloads(ctx, k8sClient, workloads...) util.FinishWorkloads(ctx, k8sClient, newWorkloads...) }) + + gomega.Eventually(func() *kueue.ClusterQueuePendingWorkloadsStatus { + var updatedCq kueue.ClusterQueue + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(clusterQueue), &updatedCq)).To(gomega.Succeed()) + return updatedCq.Status.PendingWorkloadsStatus + }, util.Timeout, util.Interval).Should(gomega.BeComparableTo(&kueue.ClusterQueuePendingWorkloadsStatus{ + Head: []kueue.ClusterQueuePendingWorkload{}, + }, ignoreLastChangeTime)) }) }) }) From 99ae6b3b5d53e096c20c870d25fe2ddac53e1a12 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Tue, 5 Sep 2023 14:01:06 +0200 Subject: [PATCH 17/26] fix priority of workloads --- .../core/clusterqueue_controller_test.go | 42 +++++-------------- 1 file changed, 10 insertions(+), 32 deletions(-) diff --git a/test/integration/controller/core/clusterqueue_controller_test.go b/test/integration/controller/core/clusterqueue_controller_test.go index 071ef34dd6..a152c338c9 100644 --- a/test/integration/controller/core/clusterqueue_controller_test.go +++ b/test/integration/controller/core/clusterqueue_controller_test.go @@ -17,8 +17,6 @@ limitations under the License. package core import ( - "time" - "github.com/google/go-cmp/cmp/cmpopts" "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" @@ -635,10 +633,11 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { }) ginkgo.It("Should update of the pending workloads when a new workload is scheduled", func() { + const lowPrio, midPrio, highPrio = 0, 10, 100 workloads := []*kueue.Workload{ - testing.MakeWorkload("one", ns.Name).Queue(localQueue.Name). + testing.MakeWorkload("one", ns.Name).Queue(localQueue.Name).Priority(highPrio). Request(corev1.ResourceCPU, "2").Request(resourceGPU, "2").Obj(), - testing.MakeWorkload("two", ns.Name).Queue(localQueue.Name). + testing.MakeWorkload("two", ns.Name).Queue(localQueue.Name).Priority(midPrio). Request(corev1.ResourceCPU, "3").Request(resourceGPU, "3").Obj(), } @@ -667,22 +666,17 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { Namespace: ns.Name, }, }, - }, - ignoreLastChangeTime, - cmpopts.SortSlices(func(i, j kueue.ClusterQueuePendingWorkload) bool { - return i.Name < j.Name - }))) + }, ignoreLastChangeTime)) ginkgo.By("Creating a new workload") newWorkloads := []*kueue.Workload{ - testing.MakeWorkload("three", ns.Name).Queue(localQueue.Name). + testing.MakeWorkload("three", ns.Name).Queue(localQueue.Name).Priority(midPrio). Request(corev1.ResourceCPU, "2").Request(resourceGPU, "2").Obj(), - testing.MakeWorkload("four", ns.Name).Queue(localQueue.Name). + testing.MakeWorkload("four", ns.Name).Queue(localQueue.Name).Priority(lowPrio). Request(corev1.ResourceCPU, "3").Request(resourceGPU, "3").Obj(), } for _, w := range newWorkloads { gomega.Expect(k8sClient.Create(ctx, w)).To(gomega.Succeed()) - time.Sleep(time.Second) } ginkgo.By("Cut off the head of pending workloads when the number of pending workloads exceeds MaxCount") @@ -705,28 +699,14 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { Namespace: ns.Name, }, }, - }, ignoreLastChangeTime, cmpopts.SortSlices(func(i, j kueue.ClusterQueuePendingWorkload) bool { - return i.Name < j.Name - }))) + }, ignoreLastChangeTime)) ginkgo.By("Admitting workloads") - admissions := []*kueue.Admission{ - testing.MakeAdmission(clusterQueue.Name). - Assignment(corev1.ResourceCPU, flavorOnDemand, "2").Obj(), - testing.MakeAdmission(clusterQueue.Name). - Assignment(corev1.ResourceCPU, flavorOnDemand, "3").Obj(), - testing.MakeAdmission(clusterQueue.Name). - Assignment(corev1.ResourceCPU, flavorOnDemand, "1").Obj(), - nil, - } - for i, w := range workloads { + for _, w := range workloads { gomega.Eventually(func() error { var newWL kueue.Workload gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(w), &newWL)).To(gomega.Succeed()) - if admissions[i] != nil { - return util.SetQuotaReservation(ctx, k8sClient, &newWL, admissions[i]) - } - return k8sClient.Status().Update(ctx, &newWL) + return util.SetQuotaReservation(ctx, k8sClient, &newWL, testing.MakeAdmission(clusterQueue.Name).Obj()) }, util.Timeout, util.Interval).Should(gomega.Succeed()) } @@ -745,9 +725,7 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { Namespace: ns.Name, }, }, - }, ignoreLastChangeTime, cmpopts.SortSlices(func(i, j kueue.ClusterQueuePendingWorkload) bool { - return i.Name < j.Name - }))) + }, ignoreLastChangeTime)) ginkgo.By("Finishing workload", func() { util.FinishWorkloads(ctx, k8sClient, workloads...) From 9a2ec37154f41abff8b62735ea3baecc3194c4fa Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Tue, 5 Sep 2023 16:05:26 +0200 Subject: [PATCH 18/26] move snapshot logic to clusterqueue controller --- cmd/kueue/main.go | 19 +-- .../core/clusterqueue_controller.go | 129 ++++++++++++++--- pkg/controller/core/core.go | 17 ++- pkg/queue/manager.go | 130 ++---------------- .../core/clusterqueue_controller_test.go | 14 +- .../integration/controller/core/suite_test.go | 9 +- .../scheduler/podsready/scheduler_test.go | 8 +- .../scheduler/workload_controller_test.go | 26 ++-- 8 files changed, 154 insertions(+), 198 deletions(-) diff --git a/cmd/kueue/main.go b/cmd/kueue/main.go index c5fcd19ba2..5ef0d3ad9e 100644 --- a/cmd/kueue/main.go +++ b/cmd/kueue/main.go @@ -146,7 +146,7 @@ func main() { } cCache := cache.New(mgr.GetClient(), cache.WithPodsReadyTracking(blockForPodsReady(&cfg))) - queues := setupQueueManager(mgr, cCache, &cfg) + queues := queue.NewManager(mgr.GetClient(), cCache) ctx := ctrl.SetupSignalHandler() if err := setupIndexes(ctx, mgr, &cfg); err != nil { @@ -178,23 +178,6 @@ func main() { } } -func setupQueueManager(mgr ctrl.Manager, cCache *cache.Cache, cfg *configapi.Configuration) *queue.Manager { - queues := queue.NewManager( - mgr.GetClient(), - cCache, - queue.WithQueueVisibilityUpdateInterval(cfg.QueueVisibility.UpdateIntervalSeconds), - queue.WithQueueVisibilityClusterQueuesMaxCount(cfg.QueueVisibility.ClusterQueues.MaxCount), - ) - // Taking snapshot of cluster queue is enabled when maxcount non-zero - if cfg.QueueVisibility.ClusterQueues.MaxCount != 0 { - if err := mgr.Add(queues); err != nil { - setupLog.Error(err, "Unable to add queue manager to manager") - os.Exit(1) - } - } - return queues -} - func setupIndexes(ctx context.Context, mgr ctrl.Manager, cfg *configapi.Configuration) error { err := indexer.Setup(ctx, mgr.GetFieldIndexer()) if err != nil { diff --git a/pkg/controller/core/clusterqueue_controller.go b/pkg/controller/core/clusterqueue_controller.go index 146e071ad9..1e961c254d 100644 --- a/pkg/controller/core/clusterqueue_controller.go +++ b/pkg/controller/core/clusterqueue_controller.go @@ -27,6 +27,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/util/workqueue" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" @@ -46,27 +47,32 @@ import ( "sigs.k8s.io/kueue/pkg/workload" ) +const snapshotWorkers = 5 + type ClusterQueueUpdateWatcher interface { NotifyClusterQueueUpdate(*kueue.ClusterQueue, *kueue.ClusterQueue) } // ClusterQueueReconciler reconciles a ClusterQueue object type ClusterQueueReconciler struct { - client client.Client - log logr.Logger - qManager *queue.Manager - cache *cache.Cache - wlUpdateCh chan event.GenericEvent - rfUpdateCh chan event.GenericEvent - watchers []ClusterQueueUpdateWatcher - reportResourceMetrics bool - queueVisibilityUpdateInterval time.Duration + client client.Client + log logr.Logger + qManager *queue.Manager + cache *cache.Cache + snapshotsQueue workqueue.Interface + wlUpdateCh chan event.GenericEvent + rfUpdateCh chan event.GenericEvent + watchers []ClusterQueueUpdateWatcher + reportResourceMetrics bool + queueVisibilityUpdateInterval time.Duration + queueVisibilityClusterQueuesMaxCount int32 } type ClusterQueueReconcilerOptions struct { - Watchers []ClusterQueueUpdateWatcher - ReportResourceMetrics bool - QueueVisibilityUpdateInterval time.Duration + Watchers []ClusterQueueUpdateWatcher + ReportResourceMetrics bool + QueueVisibilityUpdateInterval time.Duration + QueueVisibilityClusterQueuesMaxCount int32 } // Option configures the reconciler. @@ -94,6 +100,14 @@ func WithQueueVisibilityUpdateInterval(interval int32) ClusterQueueReconcilerOpt } } +// WithQueueVisibilityClusterQueuesMaxCount indicates if the controller should reconcile +// jobs that don't set the queue name annotation. +func WithQueueVisibilityClusterQueuesMaxCount(value int32) ClusterQueueReconcilerOption { + return func(o *ClusterQueueReconcilerOptions) { + o.QueueVisibilityClusterQueuesMaxCount = value + } +} + var DefaultOptions = ClusterQueueReconcilerOptions{} func NewClusterQueueReconciler( @@ -107,15 +121,17 @@ func NewClusterQueueReconciler( opt(&options) } return &ClusterQueueReconciler{ - client: client, - log: ctrl.Log.WithName("cluster-queue-reconciler"), - qManager: qMgr, - cache: cache, - wlUpdateCh: make(chan event.GenericEvent, updateChBuffer), - rfUpdateCh: make(chan event.GenericEvent, updateChBuffer), - watchers: options.Watchers, - reportResourceMetrics: options.ReportResourceMetrics, - queueVisibilityUpdateInterval: options.QueueVisibilityUpdateInterval, + client: client, + log: ctrl.Log.WithName("cluster-queue-reconciler"), + qManager: qMgr, + cache: cache, + snapshotsQueue: workqueue.New(), + wlUpdateCh: make(chan event.GenericEvent, updateChBuffer), + rfUpdateCh: make(chan event.GenericEvent, updateChBuffer), + watchers: options.Watchers, + reportResourceMetrics: options.ReportResourceMetrics, + queueVisibilityUpdateInterval: options.QueueVisibilityUpdateInterval, + queueVisibilityClusterQueuesMaxCount: options.QueueVisibilityClusterQueuesMaxCount, } } @@ -557,3 +573,74 @@ func (r *ClusterQueueReconciler) getWorkloadsStatus(cq *kueue.ClusterQueue) *kue } return cq.Status.PendingWorkloadsStatus } + +func (r *ClusterQueueReconciler) Start(ctx context.Context) error { + defer r.snapshotsQueue.ShutDown() + + for i := 0; i < snapshotWorkers; i++ { + go wait.UntilWithContext(ctx, r.takeSnapshot, r.queueVisibilityUpdateInterval) + } + + go wait.UntilWithContext(ctx, r.enqueueTakeSnapshot, r.queueVisibilityUpdateInterval) + + <-ctx.Done() + + return nil +} + +func (r *ClusterQueueReconciler) enqueueTakeSnapshot(ctx context.Context) { + for cq := range r.qManager.GetClusterQueues() { + r.snapshotsQueue.Add(cq) + } +} + +func (r *ClusterQueueReconciler) takeSnapshot(ctx context.Context) { + for r.processNextSnapshot(ctx) { + } +} + +func (r *ClusterQueueReconciler) processNextSnapshot(ctx context.Context) bool { + log := ctrl.LoggerFrom(ctx).WithName("processNextSnapshot") + + key, quit := r.snapshotsQueue.Get() + if quit { + return false + } + + startTime := time.Now() + defer func() { + log.V(2).Info("Finished snapshot job", "key", key, "elapsed", time.Since(startTime)) + }() + + defer r.snapshotsQueue.Done(key) + + workloads := make([]kueue.ClusterQueuePendingWorkload, 0) + if elements, ok := r.getSnapshotFromClusterQueue(key); ok { + for index, info := range elements { + if int32(index) >= r.queueVisibilityClusterQueuesMaxCount { + break + } + if info == nil { + continue + } + workloads = append(workloads, kueue.ClusterQueuePendingWorkload{ + Name: info.Obj.Name, + Namespace: info.Obj.Namespace, + }) + } + } + r.qManager.SetSnapshot(key.(string), workloads) + + return true +} + +func (r *ClusterQueueReconciler) getSnapshotFromClusterQueue(key interface{}) ([]*workload.Info, bool) { + clusterQueues := r.qManager.GetClusterQueues() + if len(clusterQueues) == 0 { + return nil, false + } + if cq := clusterQueues[key.(string)]; cq != nil { + return cq.Snapshot() + } + return nil, false +} diff --git a/pkg/controller/core/core.go b/pkg/controller/core/core.go index 271d4c28b8..3e8c93cb97 100644 --- a/pkg/controller/core/core.go +++ b/pkg/controller/core/core.go @@ -44,10 +44,16 @@ func SetupControllers(mgr ctrl.Manager, qManager *queue.Manager, cc *cache.Cache mgr.GetClient(), qManager, cc, - WithQueueVisibilityUpdateInterval(updateIntervalSeconds(cfg)), + WithQueueVisibilityUpdateInterval(queueVisibilityUpdateInterval(cfg)), + WithQueueVisibilityClusterQueuesMaxCount(queueVisibilityClusterQueuesMaxCount(cfg)), WithReportResourceMetrics(cfg.Metrics.EnableClusterQueueResources), WithWatchers(rfRec), ) + if queueVisibilityClusterQueuesMaxCount(cfg) != 0 { + if err := mgr.Add(cqRec); err != nil { + return "Unable to add ClusterQueue to manager", err + } + } rfRec.AddUpdateWatcher(cqRec) if err := cqRec.SetupWithManager(mgr); err != nil { return "ClusterQueue", err @@ -65,9 +71,16 @@ func podsReadyTimeout(cfg *config.Configuration) *time.Duration { return nil } -func updateIntervalSeconds(cfg *config.Configuration) int32 { +func queueVisibilityUpdateInterval(cfg *config.Configuration) int32 { if cfg.QueueVisibility != nil { return cfg.QueueVisibility.UpdateIntervalSeconds } return 0 } + +func queueVisibilityClusterQueuesMaxCount(cfg *config.Configuration) int32 { + if cfg.QueueVisibility != nil && cfg.QueueVisibility.ClusterQueues != nil { + return cfg.QueueVisibility.ClusterQueues.MaxCount + } + return 0 +} diff --git a/pkg/queue/manager.go b/pkg/queue/manager.go index 92b1cf679f..018d8812ae 100644 --- a/pkg/queue/manager.go +++ b/pkg/queue/manager.go @@ -21,12 +21,9 @@ import ( "errors" "fmt" "sync" - "time" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/util/sets" - "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -36,8 +33,6 @@ import ( "sigs.k8s.io/kueue/pkg/workload" ) -const snapshotWorkers = 5 - var ( errQueueDoesNotExist = errors.New("queue doesn't exist") errClusterQueueDoesNotExist = errors.New("clusterQueue doesn't exist") @@ -54,58 +49,21 @@ type Manager struct { localQueues map[string]*LocalQueue snapshotsMutex sync.RWMutex - snapshotsQueue workqueue.Interface snapshots map[string][]kueue.ClusterQueuePendingWorkload - queueVisibilityUpdateInterval time.Duration - queueVisibilityClusterQueuesMaxCount int32 - // Key is cohort's name. Value is a set of associated ClusterQueue names. cohorts map[string]sets.Set[string] } -type Options struct { - QueueVisibilityUpdateInterval time.Duration - QueueVisibilityClusterQueuesMaxCount int32 -} - -// Option configures the reconciler. -type Option func(*Options) - -// WithQueueVisibilityUpdateInterval indicates if the controller should reconcile -// jobs that don't set the queue name annotation. -func WithQueueVisibilityUpdateInterval(interval int32) Option { - return func(o *Options) { - o.QueueVisibilityUpdateInterval = time.Duration(interval) * time.Second - } -} - -// WithQueueVisibilityClusterQueuesMaxCount indicates if the controller should reconcile -// jobs that don't set the queue name annotation. -func WithQueueVisibilityClusterQueuesMaxCount(value int32) Option { - return func(o *Options) { - o.QueueVisibilityClusterQueuesMaxCount = value - } -} - -var DefaultOptions = Options{} - -func NewManager(client client.Client, checker StatusChecker, opts ...Option) *Manager { - options := DefaultOptions - for _, opt := range opts { - opt(&options) - } +func NewManager(client client.Client, checker StatusChecker) *Manager { m := &Manager{ - client: client, - statusChecker: checker, - localQueues: make(map[string]*LocalQueue), - clusterQueues: make(map[string]ClusterQueue), - cohorts: make(map[string]sets.Set[string]), - snapshotsMutex: sync.RWMutex{}, - snapshotsQueue: workqueue.New(), - snapshots: make(map[string][]kueue.ClusterQueuePendingWorkload, 0), - queueVisibilityUpdateInterval: options.QueueVisibilityUpdateInterval, - queueVisibilityClusterQueuesMaxCount: options.QueueVisibilityClusterQueuesMaxCount, + client: client, + statusChecker: checker, + localQueues: make(map[string]*LocalQueue), + clusterQueues: make(map[string]ClusterQueue), + cohorts: make(map[string]sets.Set[string]), + snapshotsMutex: sync.RWMutex{}, + snapshots: make(map[string][]kueue.ClusterQueuePendingWorkload, 0), } m.cond.L = &m.RWMutex return m @@ -595,78 +553,10 @@ func (m *Manager) reportPendingWorkloads(cqName string, cq ClusterQueue) { metrics.ReportPendingWorkloads(cqName, active, inadmissible) } -func (m *Manager) Start(ctx context.Context) error { - defer m.snapshotsQueue.ShutDown() - - for i := 0; i < snapshotWorkers; i++ { - go wait.UntilWithContext(ctx, m.takeSnapshot, m.queueVisibilityUpdateInterval) - } - - go wait.UntilWithContext(ctx, m.enqueueTakeSnapshot, m.queueVisibilityUpdateInterval) - - <-ctx.Done() - - return nil -} - -func (m *Manager) enqueueTakeSnapshot(ctx context.Context) { +func (m *Manager) GetClusterQueues() map[string]ClusterQueue { m.RLock() defer m.RUnlock() - for cq := range m.clusterQueues { - m.snapshotsQueue.Add(cq) - } -} - -func (m *Manager) takeSnapshot(ctx context.Context) { - for m.processNextSnapshot(ctx) { - } -} - -func (m *Manager) processNextSnapshot(ctx context.Context) bool { - log := ctrl.LoggerFrom(ctx).WithName("processNextSnapshot") - - key, quit := m.snapshotsQueue.Get() - if quit { - return false - } - - startTime := time.Now() - defer func() { - log.V(2).Info("Finished snapshot job", "key", key, "elapsed", time.Since(startTime)) - }() - - defer m.snapshotsQueue.Done(key) - - workloads := make([]kueue.ClusterQueuePendingWorkload, 0) - if elements, ok := m.getSnapshotFromClusterQueue(key); ok { - for index, info := range elements { - if int32(index) >= m.queueVisibilityClusterQueuesMaxCount { - break - } - if info == nil { - continue - } - workloads = append(workloads, kueue.ClusterQueuePendingWorkload{ - Name: info.Obj.Name, - Namespace: info.Obj.Namespace, - }) - } - } - m.SetSnapshot(key.(string), workloads) - - return true -} - -func (m *Manager) getSnapshotFromClusterQueue(key interface{}) ([]*workload.Info, bool) { - m.RLock() - defer m.RUnlock() - if len(m.clusterQueues) == 0 { - return nil, false - } - if cq := m.clusterQueues[key.(string)]; cq != nil { - return cq.Snapshot() - } - return nil, false + return m.clusterQueues } func (m *Manager) SetSnapshot(cqName string, workloads []kueue.ClusterQueuePendingWorkload) { diff --git a/test/integration/controller/core/clusterqueue_controller_test.go b/test/integration/controller/core/clusterqueue_controller_test.go index a152c338c9..c93e9105ee 100644 --- a/test/integration/controller/core/clusterqueue_controller_test.go +++ b/test/integration/controller/core/clusterqueue_controller_test.go @@ -634,7 +634,7 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { ginkgo.It("Should update of the pending workloads when a new workload is scheduled", func() { const lowPrio, midPrio, highPrio = 0, 10, 100 - workloads := []*kueue.Workload{ + workloadsFirstBatch := []*kueue.Workload{ testing.MakeWorkload("one", ns.Name).Queue(localQueue.Name).Priority(highPrio). Request(corev1.ResourceCPU, "2").Request(resourceGPU, "2").Obj(), testing.MakeWorkload("two", ns.Name).Queue(localQueue.Name).Priority(midPrio). @@ -648,7 +648,7 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { }, util.Timeout, util.Interval).Should(gomega.BeComparableTo(&kueue.ClusterQueuePendingWorkloadsStatus{}, ignoreLastChangeTime)) ginkgo.By("Creating workloads") - for _, w := range workloads { + for _, w := range workloadsFirstBatch { gomega.Expect(k8sClient.Create(ctx, w)).To(gomega.Succeed()) } gomega.Eventually(func() *kueue.ClusterQueuePendingWorkloadsStatus { @@ -669,13 +669,13 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { }, ignoreLastChangeTime)) ginkgo.By("Creating a new workload") - newWorkloads := []*kueue.Workload{ + workloadsSecondBatch := []*kueue.Workload{ testing.MakeWorkload("three", ns.Name).Queue(localQueue.Name).Priority(midPrio). Request(corev1.ResourceCPU, "2").Request(resourceGPU, "2").Obj(), testing.MakeWorkload("four", ns.Name).Queue(localQueue.Name).Priority(lowPrio). Request(corev1.ResourceCPU, "3").Request(resourceGPU, "3").Obj(), } - for _, w := range newWorkloads { + for _, w := range workloadsSecondBatch { gomega.Expect(k8sClient.Create(ctx, w)).To(gomega.Succeed()) } @@ -702,7 +702,7 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { }, ignoreLastChangeTime)) ginkgo.By("Admitting workloads") - for _, w := range workloads { + for _, w := range workloadsFirstBatch { gomega.Eventually(func() error { var newWL kueue.Workload gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(w), &newWL)).To(gomega.Succeed()) @@ -728,8 +728,8 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { }, ignoreLastChangeTime)) ginkgo.By("Finishing workload", func() { - util.FinishWorkloads(ctx, k8sClient, workloads...) - util.FinishWorkloads(ctx, k8sClient, newWorkloads...) + util.FinishWorkloads(ctx, k8sClient, workloadsFirstBatch...) + util.FinishWorkloads(ctx, k8sClient, workloadsSecondBatch...) }) gomega.Eventually(func() *kueue.ClusterQueuePendingWorkloadsStatus { diff --git a/test/integration/controller/core/suite_test.go b/test/integration/controller/core/suite_test.go index 44e1027d30..3316e226da 100644 --- a/test/integration/controller/core/suite_test.go +++ b/test/integration/controller/core/suite_test.go @@ -82,14 +82,7 @@ func managerSetup(mgr manager.Manager, ctx context.Context) { } cCache := cache.New(mgr.GetClient()) - queues := queue.NewManager( - mgr.GetClient(), - cCache, - queue.WithQueueVisibilityUpdateInterval(controllersCfg.QueueVisibility.UpdateIntervalSeconds), - queue.WithQueueVisibilityClusterQueuesMaxCount(controllersCfg.QueueVisibility.ClusterQueues.MaxCount), - ) - err = mgr.Add(queues) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + queues := queue.NewManager(mgr.GetClient(), cCache) failedCtrl, err := core.SetupControllers(mgr, queues, cCache, controllersCfg) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl) diff --git a/test/integration/scheduler/podsready/scheduler_test.go b/test/integration/scheduler/podsready/scheduler_test.go index 5bb12efc6b..d091186648 100644 --- a/test/integration/scheduler/podsready/scheduler_test.go +++ b/test/integration/scheduler/podsready/scheduler_test.go @@ -39,7 +39,7 @@ import ( ) var ignoreCQConditions = cmpopts.IgnoreFields(kueue.ClusterQueueStatus{}, "Conditions") -var ignoreLastChangeTime = cmpopts.IgnoreFields(kueue.ClusterQueuePendingWorkloadsStatus{}, "LastChangeTime") +var ignorePendingWorkloadsStatus = cmpopts.IgnoreFields(kueue.ClusterQueueStatus{}, "PendingWorkloadsStatus") // +kubebuilder:docs-gen:collapse=Imports @@ -266,8 +266,7 @@ var _ = ginkgo.Describe("SchedulerWithWaitForPodsReady", func() { Total: resource.MustParse("2"), }}, }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, - }, ignoreCQConditions, ignoreLastChangeTime)) + }, ignoreCQConditions, ignorePendingWorkloadsStatus)) ginkgo.By("wait for the timeout to be exceeded") time.Sleep(podsReadyTimeout) @@ -296,8 +295,7 @@ var _ = ginkgo.Describe("SchedulerWithWaitForPodsReady", func() { Total: resource.MustParse("0"), }}, }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, - }, ignoreCQConditions, ignoreLastChangeTime)) + }, ignoreCQConditions, ignorePendingWorkloadsStatus)) ginkgo.By("verify the active workload metric is decreased for the cluster queue") util.ExpectAdmittedActiveWorkloadsMetric(prodClusterQ, 0) diff --git a/test/integration/scheduler/workload_controller_test.go b/test/integration/scheduler/workload_controller_test.go index e7781d2b63..2cd6e818cd 100644 --- a/test/integration/scheduler/workload_controller_test.go +++ b/test/integration/scheduler/workload_controller_test.go @@ -36,7 +36,7 @@ import ( // +kubebuilder:docs-gen:collapse=Imports var ignoreCqCondition = cmpopts.IgnoreFields(kueue.ClusterQueueStatus{}, "Conditions") -var ignoreLastChangeTime = cmpopts.IgnoreFields(kueue.ClusterQueuePendingWorkloadsStatus{}, "LastChangeTime") +var ignorePendingWorkloadsStatus = cmpopts.IgnoreFields(kueue.ClusterQueueStatus{}, "PendingWorkloadsStatus") var _ = ginkgo.Describe("Workload controller with scheduler", func() { var ( @@ -122,8 +122,7 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { Total: resource.MustParse("2"), }}, }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, - }, ignoreCqCondition, ignoreLastChangeTime)) + }, ignoreCqCondition, ignorePendingWorkloadsStatus)) }) }) }) @@ -178,8 +177,7 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { Total: resource.MustParse("1"), }}, }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, - }, ignoreCqCondition, ignoreLastChangeTime)) + }, ignoreCqCondition, ignorePendingWorkloadsStatus)) }) }) }) @@ -235,8 +233,7 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { }, }, }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, - }, ignoreCqCondition, ignoreLastChangeTime)) + }, ignoreCqCondition, ignorePendingWorkloadsStatus)) }) ginkgo.By("Check podSets spec", func() { @@ -278,8 +275,7 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { }, }, }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, - }, ignoreCqCondition, ignoreLastChangeTime)) + }, ignoreCqCondition, ignorePendingWorkloadsStatus)) }) ginkgo.By("Check podSets spec", func() { @@ -338,8 +334,7 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { Total: resource.MustParse("1"), }}, }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, - }, ignoreCqCondition, ignoreLastChangeTime)) + }, ignoreCqCondition, ignorePendingWorkloadsStatus)) }) ginkgo.By("Check podSets spec", func() { @@ -442,8 +437,7 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { Total: resource.MustParse("5"), }}, }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, - }, ignoreCqCondition, ignoreLastChangeTime)) + }, ignoreCqCondition, ignorePendingWorkloadsStatus)) }) }) }) @@ -533,8 +527,7 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { Total: resource.MustParse("5"), }}, }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, - }, ignoreCqCondition, ignoreLastChangeTime)) + }, ignoreCqCondition, ignorePendingWorkloadsStatus)) }) }) }) @@ -568,8 +561,7 @@ var _ = ginkgo.Describe("Workload controller with scheduler", func() { Total: resource.MustParse("0"), }}, }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, - }, ignoreCqCondition, ignoreLastChangeTime)) + }, ignoreCqCondition, ignorePendingWorkloadsStatus)) }) gomega.Expect(util.DeleteNamespace(ctx, k8sClient, ns)).To(gomega.Succeed()) util.ExpectClusterQueueToBeDeleted(ctx, k8sClient, clusterQueue, true) From d3ac608ccfd90273979d71bee874c7002e4cd35f Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Tue, 5 Sep 2023 16:31:31 +0200 Subject: [PATCH 19/26] added rw lock for cluster queue base --- .../core/clusterqueue_controller.go | 10 +++---- pkg/controller/core/core.go | 1 + pkg/queue/cluster_queue_impl.go | 26 +++++++++++++++++++ 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/pkg/controller/core/clusterqueue_controller.go b/pkg/controller/core/clusterqueue_controller.go index 1e961c254d..4a1106150d 100644 --- a/pkg/controller/core/clusterqueue_controller.go +++ b/pkg/controller/core/clusterqueue_controller.go @@ -84,24 +84,22 @@ func WithWatchers(watchers ...ClusterQueueUpdateWatcher) ClusterQueueReconcilerO } } -// WithReportResourceMetrics indicates if the controller should reconcile -// jobs that don't set the queue name annotation. func WithReportResourceMetrics(report bool) ClusterQueueReconcilerOption { return func(o *ClusterQueueReconcilerOptions) { o.ReportResourceMetrics = report } } -// WithQueueVisibilityMaxCount indicates if the controller should reconcile -// jobs that don't set the queue name annotation. +// WithQueueVisibilityUpdateInterval specifies the time interval for updates to the structure +// of the top pending workloads in the queues. func WithQueueVisibilityUpdateInterval(interval int32) ClusterQueueReconcilerOption { return func(o *ClusterQueueReconcilerOptions) { o.QueueVisibilityUpdateInterval = time.Duration(interval) * time.Second } } -// WithQueueVisibilityClusterQueuesMaxCount indicates if the controller should reconcile -// jobs that don't set the queue name annotation. +// WithQueueVisibilityClusterQueuesMaxCount indicates the maximal number of pending workloads exposed in the +// cluster queue status func WithQueueVisibilityClusterQueuesMaxCount(value int32) ClusterQueueReconcilerOption { return func(o *ClusterQueueReconcilerOptions) { o.QueueVisibilityClusterQueuesMaxCount = value diff --git a/pkg/controller/core/core.go b/pkg/controller/core/core.go index 3e8c93cb97..c7e78d35d4 100644 --- a/pkg/controller/core/core.go +++ b/pkg/controller/core/core.go @@ -49,6 +49,7 @@ func SetupControllers(mgr ctrl.Manager, qManager *queue.Manager, cc *cache.Cache WithReportResourceMetrics(cfg.Metrics.EnableClusterQueueResources), WithWatchers(rfRec), ) + // Taking snapshot of cluster queue is enabled when maxcount non-zero if queueVisibilityClusterQueuesMaxCount(cfg) != 0 { if err := mgr.Add(cqRec); err != nil { return "Unable to add ClusterQueue to manager", err diff --git a/pkg/queue/cluster_queue_impl.go b/pkg/queue/cluster_queue_impl.go index 74095fb4bc..ab0f261c9a 100644 --- a/pkg/queue/cluster_queue_impl.go +++ b/pkg/queue/cluster_queue_impl.go @@ -19,6 +19,7 @@ package queue import ( "context" "sort" + "sync" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" @@ -52,6 +53,8 @@ type clusterQueueBase struct { // queueInadmissibleCycle stores the popId at the time when // QueueInadmissibleWorkloads is called. queueInadmissibleCycle int64 + + rwm sync.RWMutex } func newClusterQueueImpl(keyFunc func(obj interface{}) string, lessFunc func(a, b interface{}) bool) *clusterQueueBase { @@ -59,6 +62,7 @@ func newClusterQueueImpl(keyFunc func(obj interface{}) string, lessFunc func(a, heap: heap.New(keyFunc, lessFunc), inadmissibleWorkloads: make(map[string]*workload.Info), queueInadmissibleCycle: -1, + rwm: sync.RWMutex{}, } } @@ -77,6 +81,8 @@ func (c *clusterQueueBase) Cohort() string { } func (c *clusterQueueBase) AddFromLocalQueue(q *LocalQueue) bool { + c.rwm.Lock() + defer c.rwm.Unlock() added := false for _, info := range q.items { if c.heap.PushIfNotPresent(info) { @@ -87,6 +93,8 @@ func (c *clusterQueueBase) AddFromLocalQueue(q *LocalQueue) bool { } func (c *clusterQueueBase) PushOrUpdate(wInfo *workload.Info) { + c.rwm.Lock() + defer c.rwm.Unlock() key := workload.Key(wInfo.Obj) oldInfo := c.inadmissibleWorkloads[key] if oldInfo != nil { @@ -112,6 +120,8 @@ func (c *clusterQueueBase) Delete(w *kueue.Workload) { } func (c *clusterQueueBase) DeleteFromLocalQueue(q *LocalQueue) { + c.rwm.Lock() + defer c.rwm.Unlock() for _, w := range q.items { key := workload.Key(w.Obj) if wl := c.inadmissibleWorkloads[key]; wl != nil { @@ -129,6 +139,8 @@ func (c *clusterQueueBase) DeleteFromLocalQueue(q *LocalQueue) { // the workload will be pushed back to heap directly. Otherwise, the workload // will be put into the inadmissibleWorkloads. func (c *clusterQueueBase) requeueIfNotPresent(wInfo *workload.Info, immediate bool) bool { + c.rwm.Lock() + defer c.rwm.Unlock() key := workload.Key(wInfo.Obj) if immediate || c.queueInadmissibleCycle >= c.popCycle { // If the workload was inadmissible, move it back into the queue. @@ -156,6 +168,8 @@ func (c *clusterQueueBase) requeueIfNotPresent(wInfo *workload.Info, immediate b // QueueInadmissibleWorkloads moves all workloads from inadmissibleWorkloads to heap. // If at least one workload is moved, returns true. Otherwise returns false. func (c *clusterQueueBase) QueueInadmissibleWorkloads(ctx context.Context, client client.Client) bool { + c.rwm.Lock() + defer c.rwm.Unlock() c.queueInadmissibleCycle = c.popCycle if len(c.inadmissibleWorkloads) == 0 { return false @@ -178,6 +192,8 @@ func (c *clusterQueueBase) QueueInadmissibleWorkloads(ctx context.Context, clien } func (c *clusterQueueBase) Pending() int { + c.rwm.RLock() + defer c.rwm.RUnlock() return c.PendingActive() + c.PendingInadmissible() } @@ -190,6 +206,8 @@ func (c *clusterQueueBase) PendingInadmissible() int { } func (c *clusterQueueBase) Pop() *workload.Info { + c.rwm.Lock() + defer c.rwm.Unlock() c.popCycle++ if c.heap.Len() == 0 { return nil @@ -200,6 +218,8 @@ func (c *clusterQueueBase) Pop() *workload.Info { } func (c *clusterQueueBase) Dump() (sets.Set[string], bool) { + c.rwm.RLock() + defer c.rwm.RUnlock() if c.heap.Len() == 0 { return nil, false } @@ -212,6 +232,8 @@ func (c *clusterQueueBase) Dump() (sets.Set[string], bool) { } func (c *clusterQueueBase) DumpInadmissible() (sets.Set[string], bool) { + c.rwm.RLock() + defer c.rwm.RUnlock() if len(c.inadmissibleWorkloads) == 0 { return nil, false } @@ -223,6 +245,8 @@ func (c *clusterQueueBase) DumpInadmissible() (sets.Set[string], bool) { } func (c *clusterQueueBase) Snapshot() ([]*workload.Info, bool) { + c.rwm.RLock() + defer c.rwm.RUnlock() totalLen := c.heap.Len() + len(c.inadmissibleWorkloads) elements := make([]*workload.Info, 0, totalLen) for _, e := range c.heap.List() { @@ -239,6 +263,8 @@ func (c *clusterQueueBase) Snapshot() ([]*workload.Info, bool) { } func (c *clusterQueueBase) Info(key string) *workload.Info { + c.rwm.RLock() + defer c.rwm.RUnlock() info := c.heap.GetByKey(key) if info == nil { return nil From 79a46b94b7d7852f4c3e90309e0673ea4f99c898 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Tue, 5 Sep 2023 17:29:09 +0200 Subject: [PATCH 20/26] use UpdateSnapshot function intead of SetSnapshot --- .../core/clusterqueue_controller.go | 49 +++---------------- pkg/queue/cluster_queue_impl.go | 4 +- pkg/queue/cluster_queue_interface.go | 2 +- pkg/queue/manager.go | 33 +++++++++++-- 4 files changed, 41 insertions(+), 47 deletions(-) diff --git a/pkg/controller/core/clusterqueue_controller.go b/pkg/controller/core/clusterqueue_controller.go index 4a1106150d..060f888822 100644 --- a/pkg/controller/core/clusterqueue_controller.go +++ b/pkg/controller/core/clusterqueue_controller.go @@ -554,21 +554,16 @@ func (r *ClusterQueueReconciler) updateCqStatusIfChanged( } func (r *ClusterQueueReconciler) getWorkloadsStatus(cq *kueue.ClusterQueue) *kueue.ClusterQueuePendingWorkloadsStatus { - if cq.Status.PendingWorkloadsStatus == nil { + pendingWorkloads := r.qManager.GetSnapshot(cq.Name) + shouldUpdatePendingWorkloadStatus := cq.Status.PendingWorkloadsStatus == nil || + time.Since(cq.Status.PendingWorkloadsStatus.LastChangeTime.Time) >= r.queueVisibilityUpdateInterval && + !equality.Semantic.DeepEqual(cq.Status.PendingWorkloadsStatus.Head, pendingWorkloads) + if shouldUpdatePendingWorkloadStatus { return &kueue.ClusterQueuePendingWorkloadsStatus{ Head: r.qManager.GetSnapshot(cq.Name), LastChangeTime: metav1.Time{Time: time.Now()}, } } - if time.Since(cq.Status.PendingWorkloadsStatus.LastChangeTime.Time) >= r.queueVisibilityUpdateInterval { - pendingWorkloads := r.qManager.GetSnapshot(cq.Name) - if !equality.Semantic.DeepEqual(cq.Status.PendingWorkloadsStatus.Head, pendingWorkloads) { - return &kueue.ClusterQueuePendingWorkloadsStatus{ - Head: pendingWorkloads, - LastChangeTime: metav1.Time{Time: time.Now()}, - } - } - } return cq.Status.PendingWorkloadsStatus } @@ -587,7 +582,7 @@ func (r *ClusterQueueReconciler) Start(ctx context.Context) error { } func (r *ClusterQueueReconciler) enqueueTakeSnapshot(ctx context.Context) { - for cq := range r.qManager.GetClusterQueues() { + for _, cq := range r.qManager.GetClusterQueueNames() { r.snapshotsQueue.Add(cq) } } @@ -607,38 +602,10 @@ func (r *ClusterQueueReconciler) processNextSnapshot(ctx context.Context) bool { startTime := time.Now() defer func() { - log.V(2).Info("Finished snapshot job", "key", key, "elapsed", time.Since(startTime)) + log.V(5).Info("Finished snapshot job", "key", key, "elapsed", time.Since(startTime)) }() defer r.snapshotsQueue.Done(key) - - workloads := make([]kueue.ClusterQueuePendingWorkload, 0) - if elements, ok := r.getSnapshotFromClusterQueue(key); ok { - for index, info := range elements { - if int32(index) >= r.queueVisibilityClusterQueuesMaxCount { - break - } - if info == nil { - continue - } - workloads = append(workloads, kueue.ClusterQueuePendingWorkload{ - Name: info.Obj.Name, - Namespace: info.Obj.Namespace, - }) - } - } - r.qManager.SetSnapshot(key.(string), workloads) - + r.qManager.UpdateSnapshot(key.(string), r.queueVisibilityClusterQueuesMaxCount) return true } - -func (r *ClusterQueueReconciler) getSnapshotFromClusterQueue(key interface{}) ([]*workload.Info, bool) { - clusterQueues := r.qManager.GetClusterQueues() - if len(clusterQueues) == 0 { - return nil, false - } - if cq := clusterQueues[key.(string)]; cq != nil { - return cq.Snapshot() - } - return nil, false -} diff --git a/pkg/queue/cluster_queue_impl.go b/pkg/queue/cluster_queue_impl.go index ab0f261c9a..1a4170a66d 100644 --- a/pkg/queue/cluster_queue_impl.go +++ b/pkg/queue/cluster_queue_impl.go @@ -244,7 +244,7 @@ func (c *clusterQueueBase) DumpInadmissible() (sets.Set[string], bool) { return elements, true } -func (c *clusterQueueBase) Snapshot() ([]*workload.Info, bool) { +func (c *clusterQueueBase) Snapshot() []*workload.Info { c.rwm.RLock() defer c.rwm.RUnlock() totalLen := c.heap.Len() + len(c.inadmissibleWorkloads) @@ -259,7 +259,7 @@ func (c *clusterQueueBase) Snapshot() ([]*workload.Info, bool) { sort.Slice(elements, func(i, j int) bool { return queueOrdering(elements[i], elements[j]) }) - return elements, true + return elements } func (c *clusterQueueBase) Info(key string) *workload.Info { diff --git a/pkg/queue/cluster_queue_interface.go b/pkg/queue/cluster_queue_interface.go index f9ab1a95d7..9d7e86e607 100644 --- a/pkg/queue/cluster_queue_interface.go +++ b/pkg/queue/cluster_queue_interface.go @@ -95,7 +95,7 @@ type ClusterQueue interface { // Snapshot returns a copy of the current workloads in the heap of // this ClusterQueue. It returns false if the queue is empty. // Otherwise returns true. - Snapshot() ([]*workload.Info, bool) + Snapshot() []*workload.Info // Info returns workload.Info for the workload key. // Users of this method should not modify the returned object. Info(string) *workload.Info diff --git a/pkg/queue/manager.go b/pkg/queue/manager.go index 018d8812ae..f1e6e37139 100644 --- a/pkg/queue/manager.go +++ b/pkg/queue/manager.go @@ -553,15 +553,42 @@ func (m *Manager) reportPendingWorkloads(cqName string, cq ClusterQueue) { metrics.ReportPendingWorkloads(cqName, active, inadmissible) } -func (m *Manager) GetClusterQueues() map[string]ClusterQueue { +func (m *Manager) GetClusterQueueNames() []string { m.RLock() defer m.RUnlock() - return m.clusterQueues + clusterQueueNames := make([]string, 0, len(m.clusterQueues)) + for k := range m.clusterQueues { + clusterQueueNames = append(clusterQueueNames, k) + } + return clusterQueueNames +} + +func (m *Manager) getClusterQueue(cqName string) ClusterQueue { + m.RLock() + defer m.RUnlock() + return m.clusterQueues[cqName] } -func (m *Manager) SetSnapshot(cqName string, workloads []kueue.ClusterQueuePendingWorkload) { +func (m *Manager) UpdateSnapshot(cqName string, maxCount int32) { m.snapshotsMutex.Lock() defer m.snapshotsMutex.Unlock() + cq := m.getClusterQueue(cqName) + if cq == nil { + return + } + workloads := make([]kueue.ClusterQueuePendingWorkload, 0) + for index, info := range cq.Snapshot() { + if int32(index) >= maxCount { + break + } + if info == nil { + continue + } + workloads = append(workloads, kueue.ClusterQueuePendingWorkload{ + Name: info.Obj.Name, + Namespace: info.Obj.Namespace, + }) + } m.snapshots[cqName] = workloads } From b5eb23383229431dbe25defb1c42b04687bb6503 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Wed, 6 Sep 2023 11:27:24 +0200 Subject: [PATCH 21/26] next round of review changes --- pkg/controller/core/clusterqueue_controller.go | 12 +++++++----- pkg/queue/cluster_queue_interface.go | 3 +-- pkg/queue/manager.go | 8 ++++++-- .../controller/core/clusterqueue_controller_test.go | 3 ++- 4 files changed, 16 insertions(+), 10 deletions(-) diff --git a/pkg/controller/core/clusterqueue_controller.go b/pkg/controller/core/clusterqueue_controller.go index 060f888822..d6f373a500 100644 --- a/pkg/controller/core/clusterqueue_controller.go +++ b/pkg/controller/core/clusterqueue_controller.go @@ -554,11 +554,7 @@ func (r *ClusterQueueReconciler) updateCqStatusIfChanged( } func (r *ClusterQueueReconciler) getWorkloadsStatus(cq *kueue.ClusterQueue) *kueue.ClusterQueuePendingWorkloadsStatus { - pendingWorkloads := r.qManager.GetSnapshot(cq.Name) - shouldUpdatePendingWorkloadStatus := cq.Status.PendingWorkloadsStatus == nil || - time.Since(cq.Status.PendingWorkloadsStatus.LastChangeTime.Time) >= r.queueVisibilityUpdateInterval && - !equality.Semantic.DeepEqual(cq.Status.PendingWorkloadsStatus.Head, pendingWorkloads) - if shouldUpdatePendingWorkloadStatus { + if r.shouldUpdatePendingWorkloadStatus(cq) { return &kueue.ClusterQueuePendingWorkloadsStatus{ Head: r.qManager.GetSnapshot(cq.Name), LastChangeTime: metav1.Time{Time: time.Now()}, @@ -567,6 +563,12 @@ func (r *ClusterQueueReconciler) getWorkloadsStatus(cq *kueue.ClusterQueue) *kue return cq.Status.PendingWorkloadsStatus } +func (r *ClusterQueueReconciler) shouldUpdatePendingWorkloadStatus(cq *kueue.ClusterQueue) bool { + return cq.Status.PendingWorkloadsStatus == nil || + time.Since(cq.Status.PendingWorkloadsStatus.LastChangeTime.Time) >= r.queueVisibilityUpdateInterval && + !equality.Semantic.DeepEqual(cq.Status.PendingWorkloadsStatus.Head, r.qManager.GetSnapshot(cq.Name)) +} + func (r *ClusterQueueReconciler) Start(ctx context.Context) error { defer r.snapshotsQueue.ShutDown() diff --git a/pkg/queue/cluster_queue_interface.go b/pkg/queue/cluster_queue_interface.go index 9d7e86e607..ed6e27ccfd 100644 --- a/pkg/queue/cluster_queue_interface.go +++ b/pkg/queue/cluster_queue_interface.go @@ -93,8 +93,7 @@ type ClusterQueue interface { Dump() (sets.Set[string], bool) DumpInadmissible() (sets.Set[string], bool) // Snapshot returns a copy of the current workloads in the heap of - // this ClusterQueue. It returns false if the queue is empty. - // Otherwise returns true. + // this ClusterQueue. Snapshot() []*workload.Info // Info returns workload.Info for the workload key. // Users of this method should not modify the returned object. diff --git a/pkg/queue/manager.go b/pkg/queue/manager.go index f1e6e37139..ac73185b14 100644 --- a/pkg/queue/manager.go +++ b/pkg/queue/manager.go @@ -570,8 +570,6 @@ func (m *Manager) getClusterQueue(cqName string) ClusterQueue { } func (m *Manager) UpdateSnapshot(cqName string, maxCount int32) { - m.snapshotsMutex.Lock() - defer m.snapshotsMutex.Unlock() cq := m.getClusterQueue(cqName) if cq == nil { return @@ -589,6 +587,12 @@ func (m *Manager) UpdateSnapshot(cqName string, maxCount int32) { Namespace: info.Obj.Namespace, }) } + m.setSnapshot(cqName, workloads) +} + +func (m *Manager) setSnapshot(cqName string, workloads []kueue.ClusterQueuePendingWorkload) { + m.snapshotsMutex.Lock() + defer m.snapshotsMutex.Unlock() m.snapshots[cqName] = workloads } diff --git a/test/integration/controller/core/clusterqueue_controller_test.go b/test/integration/controller/core/clusterqueue_controller_test.go index c93e9105ee..0800b6ea8e 100644 --- a/test/integration/controller/core/clusterqueue_controller_test.go +++ b/test/integration/controller/core/clusterqueue_controller_test.go @@ -641,6 +641,7 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { Request(corev1.ResourceCPU, "3").Request(resourceGPU, "3").Obj(), } + ginkgo.By("Verify pending workload status before adding workloads") gomega.Eventually(func() *kueue.ClusterQueuePendingWorkloadsStatus { var updatedCq kueue.ClusterQueue gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(clusterQueue), &updatedCq)).To(gomega.Succeed()) @@ -679,7 +680,7 @@ var _ = ginkgo.Describe("ClusterQueue controller", func() { gomega.Expect(k8sClient.Create(ctx, w)).To(gomega.Succeed()) } - ginkgo.By("Cut off the head of pending workloads when the number of pending workloads exceeds MaxCount") + ginkgo.By("Verify the head of pending workloads when the number of pending workloads exceeds MaxCount") gomega.Eventually(func() *kueue.ClusterQueuePendingWorkloadsStatus { var updatedCq kueue.ClusterQueue gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(clusterQueue), &updatedCq)).To(gomega.Succeed()) From 90b18bdee0de5861442bc95320133618fbf3ced0 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Wed, 6 Sep 2023 14:14:58 +0200 Subject: [PATCH 22/26] feature is disabled in start function --- pkg/controller/core/clusterqueue_controller.go | 9 +++++++-- pkg/controller/core/core.go | 7 ++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/pkg/controller/core/clusterqueue_controller.go b/pkg/controller/core/clusterqueue_controller.go index d6f373a500..59cbc08fa3 100644 --- a/pkg/controller/core/clusterqueue_controller.go +++ b/pkg/controller/core/clusterqueue_controller.go @@ -565,11 +565,16 @@ func (r *ClusterQueueReconciler) getWorkloadsStatus(cq *kueue.ClusterQueue) *kue func (r *ClusterQueueReconciler) shouldUpdatePendingWorkloadStatus(cq *kueue.ClusterQueue) bool { return cq.Status.PendingWorkloadsStatus == nil || - time.Since(cq.Status.PendingWorkloadsStatus.LastChangeTime.Time) >= r.queueVisibilityUpdateInterval && - !equality.Semantic.DeepEqual(cq.Status.PendingWorkloadsStatus.Head, r.qManager.GetSnapshot(cq.Name)) + (time.Since(cq.Status.PendingWorkloadsStatus.LastChangeTime.Time) >= r.queueVisibilityUpdateInterval && + !equality.Semantic.DeepEqual(cq.Status.PendingWorkloadsStatus.Head, r.qManager.GetSnapshot(cq.Name))) } func (r *ClusterQueueReconciler) Start(ctx context.Context) error { + // Taking snapshot of cluster queue is enabled when maxcount non-zero + if r.queueVisibilityClusterQueuesMaxCount == 0 { + return nil + } + defer r.snapshotsQueue.ShutDown() for i := 0; i < snapshotWorkers; i++ { diff --git a/pkg/controller/core/core.go b/pkg/controller/core/core.go index c7e78d35d4..f2c175b699 100644 --- a/pkg/controller/core/core.go +++ b/pkg/controller/core/core.go @@ -49,11 +49,8 @@ func SetupControllers(mgr ctrl.Manager, qManager *queue.Manager, cc *cache.Cache WithReportResourceMetrics(cfg.Metrics.EnableClusterQueueResources), WithWatchers(rfRec), ) - // Taking snapshot of cluster queue is enabled when maxcount non-zero - if queueVisibilityClusterQueuesMaxCount(cfg) != 0 { - if err := mgr.Add(cqRec); err != nil { - return "Unable to add ClusterQueue to manager", err - } + if err := mgr.Add(cqRec); err != nil { + return "Unable to add ClusterQueue to manager", err } rfRec.AddUpdateWatcher(cqRec) if err := cqRec.SetupWithManager(mgr); err != nil { From aa4bad081104140452db096fb7dadf7f9d76899e Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Thu, 7 Sep 2023 15:10:13 +0200 Subject: [PATCH 23/26] added unit tests --- .../core/clusterqueue_controller.go | 13 ++- .../core/clusterqueue_controller_test.go | 93 +++++++++++++++++-- pkg/controller/core/core.go | 4 +- 3 files changed, 97 insertions(+), 13 deletions(-) diff --git a/pkg/controller/core/clusterqueue_controller.go b/pkg/controller/core/clusterqueue_controller.go index 59cbc08fa3..b90f83975a 100644 --- a/pkg/controller/core/clusterqueue_controller.go +++ b/pkg/controller/core/clusterqueue_controller.go @@ -92,9 +92,9 @@ func WithReportResourceMetrics(report bool) ClusterQueueReconcilerOption { // WithQueueVisibilityUpdateInterval specifies the time interval for updates to the structure // of the top pending workloads in the queues. -func WithQueueVisibilityUpdateInterval(interval int32) ClusterQueueReconcilerOption { +func WithQueueVisibilityUpdateInterval(interval time.Duration) ClusterQueueReconcilerOption { return func(o *ClusterQueueReconcilerOptions) { - o.QueueVisibilityUpdateInterval = time.Duration(interval) * time.Second + o.QueueVisibilityUpdateInterval = interval } } @@ -553,7 +553,14 @@ func (r *ClusterQueueReconciler) updateCqStatusIfChanged( return nil } +func (r *ClusterQueueReconciler) isVisibilityEnabled() bool { + return r.queueVisibilityClusterQueuesMaxCount > 0 +} + func (r *ClusterQueueReconciler) getWorkloadsStatus(cq *kueue.ClusterQueue) *kueue.ClusterQueuePendingWorkloadsStatus { + if !r.isVisibilityEnabled() { + return nil + } if r.shouldUpdatePendingWorkloadStatus(cq) { return &kueue.ClusterQueuePendingWorkloadsStatus{ Head: r.qManager.GetSnapshot(cq.Name), @@ -571,7 +578,7 @@ func (r *ClusterQueueReconciler) shouldUpdatePendingWorkloadStatus(cq *kueue.Clu func (r *ClusterQueueReconciler) Start(ctx context.Context) error { // Taking snapshot of cluster queue is enabled when maxcount non-zero - if r.queueVisibilityClusterQueuesMaxCount == 0 { + if !r.isVisibilityEnabled() { return nil } diff --git a/pkg/controller/core/clusterqueue_controller_test.go b/pkg/controller/core/clusterqueue_controller_test.go index 51c606f46a..3ef482f685 100644 --- a/pkg/controller/core/clusterqueue_controller_test.go +++ b/pkg/controller/core/clusterqueue_controller_test.go @@ -17,7 +17,9 @@ limitations under the License. package core import ( + "context" "testing" + "time" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -34,6 +36,10 @@ import ( testingmetrics "sigs.k8s.io/kueue/pkg/util/testing/metrics" ) +const ( + snapshotTimeout = time.Second +) + func TestUpdateCqStatusIfChanged(t *testing.T) { cqName := "test-cq" lqName := "test-lq" @@ -65,7 +71,6 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "FlavorNotFound", Message: "Can't admit new workloads; some flavors are not found", }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, }, "same condition status": { @@ -77,7 +82,6 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "Ready", Message: "Can admit new workloads", }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, newConditionStatus: metav1.ConditionTrue, newReason: "Ready", @@ -90,7 +94,6 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "Ready", Message: "Can admit new workloads", }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, }, "same condition status with different reason and message": { @@ -102,7 +105,6 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "FlavorNotFound", Message: "Can't admit new workloads; Can't admit new workloads; some flavors are not found", }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, newConditionStatus: metav1.ConditionFalse, newReason: "Terminating", @@ -115,7 +117,6 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "Terminating", Message: "Can't admit new workloads; clusterQueue is terminating", }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, }, "different condition status": { @@ -127,7 +128,6 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "FlavorNotFound", Message: "Can't admit new workloads; some flavors are not found", }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, newConditionStatus: metav1.ConditionTrue, newReason: "Ready", @@ -140,7 +140,6 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "Ready", Message: "Can admit new workloads", }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, }, "different pendingWorkloads with same condition status": { @@ -165,7 +164,6 @@ func TestUpdateCqStatusIfChanged(t *testing.T) { Reason: "Ready", Message: "Can admit new workloads", }}, - PendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{}, }, }, } @@ -489,3 +487,82 @@ func TestRecordResourceMetrics(t *testing.T) { }) } } + +func TestStartSnapshot(t *testing.T) { + cqName := "test-cq" + lqName := "test-lq" + const lowPrio, highPrio = 0, 100 + defaultWls := &kueue.WorkloadList{ + Items: []kueue.Workload{ + *utiltesting.MakeWorkload("one", "").Queue(lqName).Priority(highPrio).Obj(), + *utiltesting.MakeWorkload("two", "").Queue(lqName).Priority(lowPrio).Obj(), + }, + } + testCases := map[string]struct { + queueVisibilityUpdateInterval time.Duration + queueVisibilityClusterQueuesMaxCount int32 + wantPendingWorkloadsStatus *kueue.ClusterQueuePendingWorkloadsStatus + }{ + "taking snapshot of cluster queue is disabled": {}, + "taking snapshot of cluster queue is enabled": { + queueVisibilityClusterQueuesMaxCount: 2, + queueVisibilityUpdateInterval: 10 * time.Millisecond, + wantPendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{ + Head: []kueue.ClusterQueuePendingWorkload{ + {Name: "one"}, {Name: "two"}, + }, + }, + }, + "verify the head of pending workloads when the number of pending workloads exceeds MaxCount": { + queueVisibilityClusterQueuesMaxCount: 1, + queueVisibilityUpdateInterval: 10 * time.Millisecond, + wantPendingWorkloadsStatus: &kueue.ClusterQueuePendingWorkloadsStatus{ + Head: []kueue.ClusterQueuePendingWorkload{ + {Name: "one"}, + }, + }, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + cq := utiltesting.MakeClusterQueue(cqName). + QueueingStrategy(kueue.StrictFIFO).Obj() + lq := utiltesting.MakeLocalQueue(lqName, ""). + ClusterQueue(cqName).Obj() + ctx := context.Background() + + cl := utiltesting.NewClientBuilder().WithLists(defaultWls).WithObjects(lq, cq).WithStatusSubresource(lq, cq). + Build() + cCache := cache.New(cl) + qManager := queue.NewManager(cl, cCache) + if err := qManager.AddClusterQueue(ctx, cq); err != nil { + t.Fatalf("Inserting clusterQueue in manager: %v", err) + } + if err := qManager.AddLocalQueue(ctx, lq); err != nil { + t.Fatalf("Inserting localQueue in manager: %v", err) + } + + r := NewClusterQueueReconciler( + cl, + qManager, + cCache, + WithQueueVisibilityUpdateInterval(tc.queueVisibilityUpdateInterval), + WithQueueVisibilityClusterQueuesMaxCount(tc.queueVisibilityClusterQueuesMaxCount), + ) + + if err := startSnapshotWithTimeout(ctx, r); err != nil { + t.Errorf("error startSnapshotWithTimeout: %v", err) + } + + if diff := cmp.Diff(tc.wantPendingWorkloadsStatus, r.getWorkloadsStatus(cq), cmpopts.IgnoreFields(kueue.ClusterQueuePendingWorkloadsStatus{}, "LastChangeTime")); len(diff) != 0 { + t.Errorf("unexpected ClusterQueueStatus (-want,+got):\n%s", diff) + } + }) + } +} + +func startSnapshotWithTimeout(ctx context.Context, r *ClusterQueueReconciler) error { + ctx, cancel := context.WithTimeout(ctx, snapshotTimeout) + defer cancel() + return r.Start(ctx) +} diff --git a/pkg/controller/core/core.go b/pkg/controller/core/core.go index f2c175b699..138d5c76bb 100644 --- a/pkg/controller/core/core.go +++ b/pkg/controller/core/core.go @@ -69,9 +69,9 @@ func podsReadyTimeout(cfg *config.Configuration) *time.Duration { return nil } -func queueVisibilityUpdateInterval(cfg *config.Configuration) int32 { +func queueVisibilityUpdateInterval(cfg *config.Configuration) time.Duration { if cfg.QueueVisibility != nil { - return cfg.QueueVisibility.UpdateIntervalSeconds + return time.Duration(cfg.QueueVisibility.UpdateIntervalSeconds) * time.Second } return 0 } From 9e80a72b116bbded09adce1f1164e1577c2002ca Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Thu, 7 Sep 2023 16:33:50 +0200 Subject: [PATCH 24/26] call cancel in the end of test --- .../core/clusterqueue_controller.go | 2 +- .../core/clusterqueue_controller_test.go | 22 +++++++++++-------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/pkg/controller/core/clusterqueue_controller.go b/pkg/controller/core/clusterqueue_controller.go index b90f83975a..5250534b06 100644 --- a/pkg/controller/core/clusterqueue_controller.go +++ b/pkg/controller/core/clusterqueue_controller.go @@ -553,6 +553,7 @@ func (r *ClusterQueueReconciler) updateCqStatusIfChanged( return nil } +// Taking snapshot of cluster queue is enabled when maxcount non-zero func (r *ClusterQueueReconciler) isVisibilityEnabled() bool { return r.queueVisibilityClusterQueuesMaxCount > 0 } @@ -577,7 +578,6 @@ func (r *ClusterQueueReconciler) shouldUpdatePendingWorkloadStatus(cq *kueue.Clu } func (r *ClusterQueueReconciler) Start(ctx context.Context) error { - // Taking snapshot of cluster queue is enabled when maxcount non-zero if !r.isVisibilityEnabled() { return nil } diff --git a/pkg/controller/core/clusterqueue_controller_test.go b/pkg/controller/core/clusterqueue_controller_test.go index 3ef482f685..33b8d54e15 100644 --- a/pkg/controller/core/clusterqueue_controller_test.go +++ b/pkg/controller/core/clusterqueue_controller_test.go @@ -18,6 +18,7 @@ package core import ( "context" + "sync" "testing" "time" @@ -550,9 +551,18 @@ func TestStartSnapshot(t *testing.T) { WithQueueVisibilityClusterQueuesMaxCount(tc.queueVisibilityClusterQueuesMaxCount), ) - if err := startSnapshotWithTimeout(ctx, r); err != nil { - t.Errorf("error startSnapshotWithTimeout: %v", err) - } + ctx, cancel := context.WithTimeout(ctx, snapshotTimeout) + defer cancel() + + wg := &sync.WaitGroup{} + wg.Add(1) + go func() { + defer wg.Done() + if err := r.Start(ctx); err != nil { + t.Errorf("error startSnapshotWithTimeout: %v", err) + } + }() + wg.Wait() if diff := cmp.Diff(tc.wantPendingWorkloadsStatus, r.getWorkloadsStatus(cq), cmpopts.IgnoreFields(kueue.ClusterQueuePendingWorkloadsStatus{}, "LastChangeTime")); len(diff) != 0 { t.Errorf("unexpected ClusterQueueStatus (-want,+got):\n%s", diff) @@ -560,9 +570,3 @@ func TestStartSnapshot(t *testing.T) { }) } } - -func startSnapshotWithTimeout(ctx context.Context, r *ClusterQueueReconciler) error { - ctx, cancel := context.WithTimeout(ctx, snapshotTimeout) - defer cancel() - return r.Start(ctx) -} From 7ac8d214cf50a42cde8afff2357b5b0d366ae3f8 Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Thu, 7 Sep 2023 17:52:57 +0200 Subject: [PATCH 25/26] added wait.PollUntilContextTimeout --- .../core/clusterqueue_controller.go | 17 ++++++++------ .../core/clusterqueue_controller_test.go | 23 +++++++------------ 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/pkg/controller/core/clusterqueue_controller.go b/pkg/controller/core/clusterqueue_controller.go index 5250534b06..c8af28e9aa 100644 --- a/pkg/controller/core/clusterqueue_controller.go +++ b/pkg/controller/core/clusterqueue_controller.go @@ -562,21 +562,24 @@ func (r *ClusterQueueReconciler) getWorkloadsStatus(cq *kueue.ClusterQueue) *kue if !r.isVisibilityEnabled() { return nil } - if r.shouldUpdatePendingWorkloadStatus(cq) { + if cq.Status.PendingWorkloadsStatus == nil { return &kueue.ClusterQueuePendingWorkloadsStatus{ Head: r.qManager.GetSnapshot(cq.Name), LastChangeTime: metav1.Time{Time: time.Now()}, } } + if time.Since(cq.Status.PendingWorkloadsStatus.LastChangeTime.Time) >= r.queueVisibilityUpdateInterval { + pendingWorkloads := r.qManager.GetSnapshot(cq.Name) + if !equality.Semantic.DeepEqual(cq.Status.PendingWorkloadsStatus.Head, pendingWorkloads) { + return &kueue.ClusterQueuePendingWorkloadsStatus{ + Head: pendingWorkloads, + LastChangeTime: metav1.Time{Time: time.Now()}, + } + } + } return cq.Status.PendingWorkloadsStatus } -func (r *ClusterQueueReconciler) shouldUpdatePendingWorkloadStatus(cq *kueue.ClusterQueue) bool { - return cq.Status.PendingWorkloadsStatus == nil || - (time.Since(cq.Status.PendingWorkloadsStatus.LastChangeTime.Time) >= r.queueVisibilityUpdateInterval && - !equality.Semantic.DeepEqual(cq.Status.PendingWorkloadsStatus.Head, r.qManager.GetSnapshot(cq.Name))) -} - func (r *ClusterQueueReconciler) Start(ctx context.Context) error { if !r.isVisibilityEnabled() { return nil diff --git a/pkg/controller/core/clusterqueue_controller_test.go b/pkg/controller/core/clusterqueue_controller_test.go index 33b8d54e15..fe06d4c494 100644 --- a/pkg/controller/core/clusterqueue_controller_test.go +++ b/pkg/controller/core/clusterqueue_controller_test.go @@ -18,7 +18,6 @@ package core import ( "context" - "sync" "testing" "time" @@ -27,6 +26,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/utils/ptr" kueue "sigs.k8s.io/kueue/apis/kueue/v1beta1" @@ -37,10 +37,6 @@ import ( testingmetrics "sigs.k8s.io/kueue/pkg/util/testing/metrics" ) -const ( - snapshotTimeout = time.Second -) - func TestUpdateCqStatusIfChanged(t *testing.T) { cqName := "test-cq" lqName := "test-lq" @@ -551,21 +547,18 @@ func TestStartSnapshot(t *testing.T) { WithQueueVisibilityClusterQueuesMaxCount(tc.queueVisibilityClusterQueuesMaxCount), ) - ctx, cancel := context.WithTimeout(ctx, snapshotTimeout) - defer cancel() - - wg := &sync.WaitGroup{} - wg.Add(1) go func() { - defer wg.Done() if err := r.Start(ctx); err != nil { - t.Errorf("error startSnapshotWithTimeout: %v", err) + t.Errorf("error start snapshot: %v", err) } }() - wg.Wait() - if diff := cmp.Diff(tc.wantPendingWorkloadsStatus, r.getWorkloadsStatus(cq), cmpopts.IgnoreFields(kueue.ClusterQueuePendingWorkloadsStatus{}, "LastChangeTime")); len(diff) != 0 { - t.Errorf("unexpected ClusterQueueStatus (-want,+got):\n%s", diff) + diff := "" + if err := wait.PollUntilContextTimeout(ctx, time.Second, 10*time.Second, false, func(ctx context.Context) (done bool, err error) { + diff = cmp.Diff(tc.wantPendingWorkloadsStatus, r.getWorkloadsStatus(cq), cmpopts.IgnoreFields(kueue.ClusterQueuePendingWorkloadsStatus{}, "LastChangeTime")) + return diff == "", nil + }); err != nil { + t.Fatalf("Failed to get snapshot computed, last diff=%s", diff) } }) } From 3b1f7c19d16b67a267d30ba8945ae1a3bb2b501e Mon Sep 17 00:00:00 2001 From: Anton Stuchinskii Date: Thu, 7 Sep 2023 18:24:49 +0200 Subject: [PATCH 26/26] update naming --- pkg/controller/core/clusterqueue_controller_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/controller/core/clusterqueue_controller_test.go b/pkg/controller/core/clusterqueue_controller_test.go index fe06d4c494..72d19c1dca 100644 --- a/pkg/controller/core/clusterqueue_controller_test.go +++ b/pkg/controller/core/clusterqueue_controller_test.go @@ -485,7 +485,7 @@ func TestRecordResourceMetrics(t *testing.T) { } } -func TestStartSnapshot(t *testing.T) { +func TestClusterQueuePendingWorkloadsStatus(t *testing.T) { cqName := "test-cq" lqName := "test-lq" const lowPrio, highPrio = 0, 100 @@ -549,7 +549,7 @@ func TestStartSnapshot(t *testing.T) { go func() { if err := r.Start(ctx); err != nil { - t.Errorf("error start snapshot: %v", err) + t.Errorf("error starting the cluster queue reconciler: %v", err) } }() @@ -558,7 +558,7 @@ func TestStartSnapshot(t *testing.T) { diff = cmp.Diff(tc.wantPendingWorkloadsStatus, r.getWorkloadsStatus(cq), cmpopts.IgnoreFields(kueue.ClusterQueuePendingWorkloadsStatus{}, "LastChangeTime")) return diff == "", nil }); err != nil { - t.Fatalf("Failed to get snapshot computed, last diff=%s", diff) + t.Fatalf("Failed to get the expected pending workloads status, last diff=%s", diff) } }) }