Skip to content

Commit

Permalink
Review Remarks
Browse files Browse the repository at this point in the history
  • Loading branch information
@trasc
trasc committed Jan 31, 2024
1 parent a0dbd5a commit bf79060
Show file tree
Hide file tree
Showing 23 changed files with 205 additions and 201 deletions.
5 changes: 0 additions & 5 deletions apis/config/v1beta1/configuration_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,6 @@ type Configuration struct {
// QueueVisibility is configuration to expose the information about the top
// pending workloads.
QueueVisibility *QueueVisibility `json:"queueVisibility,omitempty"`

// SingleInstanceAdmissionCheckControllers contains a list of AdmissionCheck controller names for which
// at most one AdmissionCheck managed by it can be configured in a ClusterQueue.
// A ClusterQueue that violates this rule is marked as Inactive.
SingleInstanceAdmissionCheckControllers []string `json:"singleInstanceAdmissionCheckControllers,omitempty"`
}

type ControllerManager struct {
Expand Down
6 changes: 0 additions & 6 deletions apis/config/v1beta1/defaults.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,6 @@ func SetDefaults_Configuration(cfg *Configuration) {
}
}

if len(cfg.SingleInstanceAdmissionCheckControllers) == 0 {
cfg.SingleInstanceAdmissionCheckControllers = []string{
"kueue.x-k8s.io/multikueue",
}
}

if cfg.Integrations.PodOptions == nil {
cfg.Integrations.PodOptions = &PodIntegrationOptions{}
}
Expand Down
100 changes: 33 additions & 67 deletions apis/config/v1beta1/defaults_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
},
}

defaultSingleInstanceAdmissionCheckControllers := []string{"kueue.x-k8s.io/multikueue"}

podsReadyTimeoutTimeout := metav1.Duration{Duration: defaultPodsReadyTimeout}
podsReadyTimeoutOverwrite := metav1.Duration{Duration: time.Minute}

Expand All @@ -117,10 +115,9 @@ func TestSetDefaults_Configuration(t *testing.T) {
InternalCertManagement: &InternalCertManagement{
Enable: ptr.To(false),
},
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
SingleInstanceAdmissionCheckControllers: defaultSingleInstanceAdmissionCheckControllers,
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
},
},
"defaulting ControllerManager": {
Expand Down Expand Up @@ -158,10 +155,9 @@ func TestSetDefaults_Configuration(t *testing.T) {
InternalCertManagement: &InternalCertManagement{
Enable: ptr.To(false),
},
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
SingleInstanceAdmissionCheckControllers: defaultSingleInstanceAdmissionCheckControllers,
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
},
},
"should not default ControllerManager": {
Expand Down Expand Up @@ -215,10 +211,9 @@ func TestSetDefaults_Configuration(t *testing.T) {
InternalCertManagement: &InternalCertManagement{
Enable: ptr.To(false),
},
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
SingleInstanceAdmissionCheckControllers: defaultSingleInstanceAdmissionCheckControllers,
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
},
},
"should not set LeaderElectionID": {
Expand Down Expand Up @@ -256,10 +251,9 @@ func TestSetDefaults_Configuration(t *testing.T) {
InternalCertManagement: &InternalCertManagement{
Enable: ptr.To(false),
},
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
SingleInstanceAdmissionCheckControllers: defaultSingleInstanceAdmissionCheckControllers,
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
},
},
"defaulting InternalCertManagement": {
Expand All @@ -274,10 +268,9 @@ func TestSetDefaults_Configuration(t *testing.T) {
WebhookServiceName: ptr.To(DefaultWebhookServiceName),
WebhookSecretName: ptr.To(DefaultWebhookSecretName),
},
ClientConnection: defaultClientConnection,
Integrations: overwriteNamespaceIntegrations,
QueueVisibility: defaultQueueVisibility,
SingleInstanceAdmissionCheckControllers: defaultSingleInstanceAdmissionCheckControllers,
ClientConnection: defaultClientConnection,
Integrations: overwriteNamespaceIntegrations,
QueueVisibility: defaultQueueVisibility,
},
},
"should not default InternalCertManagement": {
Expand All @@ -293,10 +286,9 @@ func TestSetDefaults_Configuration(t *testing.T) {
InternalCertManagement: &InternalCertManagement{
Enable: ptr.To(false),
},
ClientConnection: defaultClientConnection,
Integrations: overwriteNamespaceIntegrations,
QueueVisibility: defaultQueueVisibility,
SingleInstanceAdmissionCheckControllers: defaultSingleInstanceAdmissionCheckControllers,
ClientConnection: defaultClientConnection,
Integrations: overwriteNamespaceIntegrations,
QueueVisibility: defaultQueueVisibility,
},
},
"should not default values in custom ClientConnection": {
Expand All @@ -320,9 +312,8 @@ func TestSetDefaults_Configuration(t *testing.T) {
QPS: ptr.To[float32](123.0),
Burst: ptr.To[int32](456),
},
Integrations: overwriteNamespaceIntegrations,
QueueVisibility: defaultQueueVisibility,
SingleInstanceAdmissionCheckControllers: defaultSingleInstanceAdmissionCheckControllers,
Integrations: overwriteNamespaceIntegrations,
QueueVisibility: defaultQueueVisibility,
},
},
"should default empty custom ClientConnection": {
Expand All @@ -339,10 +330,9 @@ func TestSetDefaults_Configuration(t *testing.T) {
InternalCertManagement: &InternalCertManagement{
Enable: ptr.To(false),
},
ClientConnection: defaultClientConnection,
Integrations: overwriteNamespaceIntegrations,
QueueVisibility: defaultQueueVisibility,
SingleInstanceAdmissionCheckControllers: defaultSingleInstanceAdmissionCheckControllers,
ClientConnection: defaultClientConnection,
Integrations: overwriteNamespaceIntegrations,
QueueVisibility: defaultQueueVisibility,
},
},
"defaulting waitForPodsReady values": {
Expand All @@ -366,10 +356,9 @@ func TestSetDefaults_Configuration(t *testing.T) {
InternalCertManagement: &InternalCertManagement{
Enable: ptr.To(false),
},
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
SingleInstanceAdmissionCheckControllers: defaultSingleInstanceAdmissionCheckControllers,
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
},
},
"set waitForPodsReady.blockAdmission to false when enable is false": {
Expand All @@ -393,10 +382,9 @@ func TestSetDefaults_Configuration(t *testing.T) {
InternalCertManagement: &InternalCertManagement{
Enable: ptr.To(false),
},
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
SingleInstanceAdmissionCheckControllers: defaultSingleInstanceAdmissionCheckControllers,
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
},
},
"respecting provided waitForPodsReady values": {
Expand All @@ -422,10 +410,9 @@ func TestSetDefaults_Configuration(t *testing.T) {
InternalCertManagement: &InternalCertManagement{
Enable: ptr.To(false),
},
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
SingleInstanceAdmissionCheckControllers: defaultSingleInstanceAdmissionCheckControllers,
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
},
},
"integrations": {
Expand All @@ -448,8 +435,7 @@ func TestSetDefaults_Configuration(t *testing.T) {
Frameworks: []string{"a", "b"},
PodOptions: defaultIntegrations.PodOptions,
},
QueueVisibility: defaultQueueVisibility,
SingleInstanceAdmissionCheckControllers: defaultSingleInstanceAdmissionCheckControllers,
QueueVisibility: defaultQueueVisibility,
},
},
"queue visibility": {
Expand Down Expand Up @@ -478,26 +464,6 @@ func TestSetDefaults_Configuration(t *testing.T) {
MaxCount: 0,
},
},
SingleInstanceAdmissionCheckControllers: defaultSingleInstanceAdmissionCheckControllers,
},
},
"single instance AdmissionCheck controllers": {
original: &Configuration{
InternalCertManagement: &InternalCertManagement{
Enable: ptr.To(false),
},
SingleInstanceAdmissionCheckControllers: []string{"controller1", "controller2"},
},
want: &Configuration{
Namespace: ptr.To(DefaultNamespace),
ControllerManager: defaultCtrlManagerConfigurationSpec,
InternalCertManagement: &InternalCertManagement{
Enable: ptr.To(false),
},
ClientConnection: defaultClientConnection,
Integrations: defaultIntegrations,
QueueVisibility: defaultQueueVisibility,
SingleInstanceAdmissionCheckControllers: []string{"controller1", "controller2"},
},
},
}
Expand Down
5 changes: 0 additions & 5 deletions apis/config/v1beta1/zz_generated.deepcopy.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions apis/kueue/v1beta1/admissioncheck_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,13 @@ type AdmissionCheckStatus struct {
// +patchStrategy=merge
// +patchMergeKey=type
Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`

// singleInstannceInClusterQueue indicates if the AdmissionCheck should be the only one managed by
// the same Controller in a ClusterQueue.
// Having multiple AdmissionChecks managed by the same Controller of which at least one has this
// set to true will cause the ClusterQueue to be marked as Inactive.
// +optional
SingleInstannceInClusterQueue *bool `json:"singleInstannceInClusterQueue,omitempty"`
}

const (
Expand Down
5 changes: 5 additions & 0 deletions apis/kueue/v1beta1/zz_generated.deepcopy.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions charts/kueue/templates/crd/kueue.x-k8s.io_admissionchecks.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,13 @@ spec:
x-kubernetes-list-map-keys:
- type
x-kubernetes-list-type: map
singleInstannceInClusterQueue:
description: singleInstannceInClusterQueue indicates if the AdmissionCheck
should be the only one managed by the same Controller in a ClusterQueue.
Having multiple AdmissionChecks managed by the same Controller of
which at least one has this set to true will cause the ClusterQueue
to be marked as Inactive.
type: boolean
type: object
type: object
served: true
Expand Down
11 changes: 10 additions & 1 deletion client-go/applyconfiguration/kueue/v1beta1/admissioncheckstatus.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion cmd/kueue/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ func main() {
close(certsReady)
}

cCache := cache.New(mgr.GetClient(), cache.WithPodsReadyTracking(blockForPodsReady(&cfg)), cache.WithSingleInstanceAdmissionCheckControllers(cfg.SingleInstanceAdmissionCheckControllers))
cCache := cache.New(mgr.GetClient(), cache.WithPodsReadyTracking(blockForPodsReady(&cfg)))
queues := queue.NewManager(mgr.GetClient(), cCache, queue.WithPodsReadyRequeuingTimestamp(podsReadyRequeuingTimestamp(&cfg)))

ctx := ctrl.SetupSignalHandler()
Expand Down
1 change: 0 additions & 1 deletion cmd/kueue/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ integrations:
MaxCount: config.DefaultClusterQueuesMaxCount,
},
},
SingleInstanceAdmissionCheckControllers: []string{"kueue.x-k8s.io/multikueue"},
},
},
{
Expand Down
7 changes: 7 additions & 0 deletions config/components/crd/bases/kueue.x-k8s.io_admissionchecks.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,13 @@ spec:
x-kubernetes-list-map-keys:
- type
x-kubernetes-list-type: map
singleInstannceInClusterQueue:
description: singleInstannceInClusterQueue indicates if the AdmissionCheck
should be the only one managed by the same Controller in a ClusterQueue.
Having multiple AdmissionChecks managed by the same Controller of
which at least one has this set to true will cause the ClusterQueue
to be marked as Inactive.
type: boolean
type: object
type: object
served: true
Expand Down
2 changes: 0 additions & 2 deletions config/components/manager/controller_manager_config.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,5 +48,3 @@ integrations:
# - key: kubernetes.io/metadata.name
# operator: NotIn
# values: [ kube-system, kueue-system ]
#singleInstanceAdmissionCheckControllers:
# - kueue.x-k8s.io/multikueue
5 changes: 3 additions & 2 deletions pkg/cache/admissioncheck.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package cache

type AdmissionCheck struct {
Active bool
Controller string
Active bool
Controller string
SingleInstanceInClusterQueue bool
}
Loading

0 comments on commit bf79060

Please sign in to comment.