Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

fix default suspension of Deployment/StatefulSet with Kueue-managed owners #3803

Merged
merged 8 commits into from
Dec 11, 2024
Merged

fix default suspension of Deployment/StatefulSet with Kueue-managed owners #3803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
15d5c15
fix deployment and statefulset setup so ownership works
dgrove-oss Dec 10, 2024
f5b356f
enable deployment/statefulset to be recognized as child jobs
dgrove-oss Dec 10, 2024
8080292
fix typo in rbacs
dgrove-oss Dec 10, 2024
0837f47
adjust podwebhook warning for forced suspension
dgrove-oss Dec 10, 2024
b6c7cb2
redesign special pod/deployment/statefulset ownership logic
dgrove-oss Dec 11, 2024
9ab2592
revert trivial change
dgrove-oss Dec 11, 2024
a733dfd
linter fix
dgrove-oss Dec 11, 2024
47a8aaf
nit: grammar
dgrove-oss Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions charts/kueue/templates/rbac/role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ rules:
- apiGroups:
- apps
resources:
- replicasets
- statefulsets
verbs:
- get
Expand Down
138 changes: 69 additions & 69 deletions charts/kueue/templates/webhook/webhook.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,40 @@ metadata:
{{- end }}
namespace: '{{ .Release.Namespace }}'
webhooks:
- admissionReviewVersions:
- v1
clientConfig:
service:
name: '{{ include "kueue.fullname" . }}-webhook-service'
namespace: '{{ .Release.Namespace }}'
path: /mutate--v1-pod
{{- if has "pod" $integrationsConfig.frameworks }}
failurePolicy: Fail
{{- else }}
failurePolicy: Ignore
{{- end }}
name: mpod.kb.io
namespaceSelector:
{{- if and (hasKey $integrationsConfig "podOptions") (hasKey ($integrationsConfig.podOptions) "namespaceSelector") }}
{{- toYaml $integrationsConfig.podOptions.namespaceSelector | nindent 6 -}}
{{- else }}
matchExpressions:
- key: kubernetes.io/metadata.name
operator: NotIn
values:
- kube-system
- '{{ .Release.Namespace }}'
{{- end }}
rules:
- apiGroups:
- ""
apiVersions:
- v1
operations:
- CREATE
resources:
- pods
sideEffects: None
- admissionReviewVersions:
- v1
clientConfig:
Expand Down Expand Up @@ -194,40 +228,6 @@ webhooks:
resources:
- mpijobs
sideEffects: None
- admissionReviewVersions:
- v1
clientConfig:
service:
name: '{{ include "kueue.fullname" . }}-webhook-service'
namespace: '{{ .Release.Namespace }}'
path: /mutate--v1-pod
{{- if has "pod" $integrationsConfig.frameworks }}
failurePolicy: Fail
{{- else }}
failurePolicy: Ignore
{{- end }}
name: mpod.kb.io
namespaceSelector:
{{- if and (hasKey $integrationsConfig "podOptions") (hasKey ($integrationsConfig.podOptions) "namespaceSelector") }}
{{- toYaml $integrationsConfig.podOptions.namespaceSelector | nindent 6 -}}
{{- else }}
matchExpressions:
- key: kubernetes.io/metadata.name
operator: NotIn
values:
- kube-system
- '{{ .Release.Namespace }}'
{{- end }}
rules:
- apiGroups:
- ""
apiVersions:
- v1
operations:
- CREATE
resources:
- pods
sideEffects: None
- admissionReviewVersions:
- v1
clientConfig:
Expand Down Expand Up @@ -356,6 +356,41 @@ metadata:
{{- end }}
namespace: '{{ .Release.Namespace }}'
webhooks:
- admissionReviewVersions:
- v1
clientConfig:
service:
name: '{{ include "kueue.fullname" . }}-webhook-service'
namespace: '{{ .Release.Namespace }}'
path: /validate--v1-pod
{{- if has "pod" $integrationsConfig.frameworks }}
failurePolicy: Fail
{{- else }}
failurePolicy: Ignore
{{- end }}
name: vpod.kb.io
namespaceSelector:
{{- if and (hasKey $integrationsConfig "podOptions") (hasKey ($integrationsConfig.podOptions) "namespaceSelector") }}
{{- toYaml $integrationsConfig.podOptions.namespaceSelector | nindent 6 -}}
{{- else }}
matchExpressions:
- key: kubernetes.io/metadata.name
operator: NotIn
values:
- kube-system
- '{{ .Release.Namespace }}'
{{- end }}
rules:
- apiGroups:
- ""
apiVersions:
- v1
operations:
- CREATE
- UPDATE
resources:
- pods
sideEffects: None
- admissionReviewVersions:
- v1
clientConfig:
Expand Down Expand Up @@ -547,41 +582,6 @@ webhooks:
resources:
- mpijobs
sideEffects: None
- admissionReviewVersions:
- v1
clientConfig:
service:
name: '{{ include "kueue.fullname" . }}-webhook-service'
namespace: '{{ .Release.Namespace }}'
path: /validate--v1-pod
{{- if has "pod" $integrationsConfig.frameworks }}
failurePolicy: Fail
{{- else }}
failurePolicy: Ignore
{{- end }}
name: vpod.kb.io
namespaceSelector:
{{- if and (hasKey $integrationsConfig "podOptions") (hasKey ($integrationsConfig.podOptions) "namespaceSelector") }}
{{- toYaml $integrationsConfig.podOptions.namespaceSelector | nindent 6 -}}
{{- else }}
matchExpressions:
- key: kubernetes.io/metadata.name
operator: NotIn
values:
- kube-system
- '{{ .Release.Namespace }}'
{{- end }}
rules:
- apiGroups:
- ""
apiVersions:
- v1
operations:
- CREATE
- UPDATE
resources:
- pods
sideEffects: None
- admissionReviewVersions:
- v1
clientConfig:
Expand Down
1 change: 1 addition & 0 deletions config/components/rbac/role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ rules:
- apiGroups:
- apps
resources:
- replicasets
- statefulsets
verbs:
- get
Expand Down
78 changes: 39 additions & 39 deletions config/components/webhook/manifests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,25 @@ kind: MutatingWebhookConfiguration
metadata:
name: mutating-webhook-configuration
webhooks:
- admissionReviewVersions:
- v1
clientConfig:
service:
name: webhook-service
namespace: system
path: /mutate--v1-pod
failurePolicy: Fail
name: mpod.kb.io
rules:
- apiGroups:
- ""
apiVersions:
- v1
operations:
- CREATE
resources:
- pods
sideEffects: None
- admissionReviewVersions:
- v1
clientConfig:
Expand Down Expand Up @@ -176,25 +195,6 @@ webhooks:
resources:
- mpijobs
sideEffects: None
- admissionReviewVersions:
- v1
clientConfig:
service:
name: webhook-service
namespace: system
path: /mutate--v1-pod
failurePolicy: Fail
name: mpod.kb.io
rules:
- apiGroups:
- ""
apiVersions:
- v1
operations:
- CREATE
resources:
- pods
sideEffects: None
- admissionReviewVersions:
- v1
clientConfig:
Expand Down Expand Up @@ -316,6 +316,26 @@ kind: ValidatingWebhookConfiguration
metadata:
name: validating-webhook-configuration
webhooks:
- admissionReviewVersions:
- v1
clientConfig:
service:
name: webhook-service
namespace: system
path: /validate--v1-pod
failurePolicy: Fail
name: vpod.kb.io
rules:
- apiGroups:
- ""
apiVersions:
- v1
operations:
- CREATE
- UPDATE
resources:
- pods
sideEffects: None
- admissionReviewVersions:
- v1
clientConfig:
Expand Down Expand Up @@ -496,26 +516,6 @@ webhooks:
resources:
- mpijobs
sideEffects: None
- admissionReviewVersions:
- v1
clientConfig:
service:
name: webhook-service
namespace: system
path: /validate--v1-pod
failurePolicy: Fail
name: vpod.kb.io
rules:
- apiGroups:
- ""
apiVersions:
- v1
operations:
- CREATE
- UPDATE
resources:
- pods
sideEffects: None
- admissionReviewVersions:
- v1
clientConfig:
Expand Down
2 changes: 2 additions & 0 deletions pkg/controller/jobframework/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ func ApplyDefaultForSuspend(ctx context.Context, job GenericJob, k8sClient clien
return nil
}

// +kubebuilder:rbac:groups="apps",resources=replicasets,verbs=get;list;watch

// WorkloadShouldBeSuspended determines whether jobObj should be default suspended on creation
func WorkloadShouldBeSuspended(ctx context.Context, jobObj client.Object, k8sClient client.Client,
manageJobsWithoutQueueName bool, managedJobsNamespaceSelector labels.Selector) (bool, error) {
Expand Down
4 changes: 4 additions & 0 deletions pkg/controller/jobframework/integrationmanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,10 @@ func GetIntegration(name string) (IntegrationCallbacks, bool) {
return manager.get(name)
}

func IsIntegrationEnabled(name string) bool {
return manager.enabledIntegrations.Has(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this requires a read lock, because integrations can be added during runtime. We may also be useful to return the GVK (renaming to e.g. GetEnabledIntegrationGVK) to avoid hard-coding (see another comment).

}

// GetIntegrationByGVK looks-up the framework identified by GroupVersionKind in the currently
// registered list of frameworks returning its callbacks and true if found.
func GetIntegrationByGVK(gvk schema.GroupVersionKind) (IntegrationCallbacks, bool) {
Expand Down
31 changes: 21 additions & 10 deletions pkg/controller/jobs/deployment/deployment_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (

appsv1 "k8s.io/api/apps/v1"
apivalidation "k8s.io/apimachinery/pkg/api/validation"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
ctrl "sigs.k8s.io/controller-runtime"
Expand All @@ -30,19 +31,24 @@ import (
"sigs.k8s.io/kueue/pkg/controller/constants"
"sigs.k8s.io/kueue/pkg/controller/jobframework"
"sigs.k8s.io/kueue/pkg/controller/jobframework/webhook"
"sigs.k8s.io/kueue/pkg/controller/jobs/pod"
"sigs.k8s.io/kueue/pkg/queue"
)

type Webhook struct {
client client.Client
queues *queue.Manager
client client.Client
manageJobsWithoutQueueName bool
managedJobsNamespaceSelector labels.Selector
queues *queue.Manager
}

func SetupWebhook(mgr ctrl.Manager, opts ...jobframework.Option) error {
options := jobframework.ProcessOptions(opts...)
wh := &Webhook{
client: mgr.GetClient(),
queues: options.Queues,
client: mgr.GetClient(),
manageJobsWithoutQueueName: options.ManageJobsWithoutQueueName,
managedJobsNamespaceSelector: options.ManagedJobsNamespaceSelector,
queues: options.Queues,
}
obj := &appsv1.Deployment{}
return webhook.WebhookManagedBy(mgr).
Expand All @@ -63,14 +69,19 @@ func (wh *Webhook) Default(ctx context.Context, obj runtime.Object) error {
log.V(5).Info("Propagating queue-name")

jobframework.ApplyDefaultLocalQueue(deployment.Object(), wh.queues.DefaultLocalQueueExist)

// Because Deployment is built using a NoOpReconciler handling of jobs without queue names is delegating to the Pod webhook.
queueName := jobframework.QueueNameForObject(deployment.Object())
if queueName != "" {
suspend, err := jobframework.WorkloadShouldBeSuspended(ctx, deployment.Object(), wh.client, wh.manageJobsWithoutQueueName, wh.managedJobsNamespaceSelector)
if err != nil {
return err
}
if suspend {
if deployment.Spec.Template.Labels == nil {
deployment.Spec.Template.Labels = make(map[string]string, 1)
deployment.Spec.Template.Labels = make(map[string]string, 2)
}
deployment.Spec.Template.Labels[pod.SuspendedByParentLabelKey] = FrameworkName
queueName := jobframework.QueueNameForObject(deployment.Object())
if queueName != "" {
deployment.Spec.Template.Labels[constants.QueueLabel] = queueName
}
deployment.Spec.Template.Labels[constants.QueueLabel] = queueName
}

return nil
Expand Down
Loading