Exclude kube-system from webhooks targeting common resources#11192
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
|
Hi @dkaluza. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
| {{- if (hasKey $managerConfig "managedJobsNamespaceSelector") }} | ||
| namespaceSelector: | ||
| {{- toYaml $managerConfig.managedJobsNamespaceSelector | nindent 6 }} | ||
| {{- else }} |
There was a problem hiding this comment.
I do not like the fact that we have to modify 200+ lines to make such small change, but it looks like we want to be really explicit about the changes here, so didn't want to change this.
| - webhooks.[name=mstatefulset.kb.io].namespaceSelector | ||
| - webhooks.[name=mtfjob.kb.io].namespaceSelector | ||
| - webhooks.[name=mtrainjob.kb.io].namespaceSelector | ||
| - webhooks.[name=mxgboostjob.kb.io].namespaceSelector |
There was a problem hiding this comment.
This unfortunately still requires registration of new webhook here when there is a new integration.
This is not the case if we just do * but then we will also target Kueue crds... This can be solved by splitting those in some way, but I'm not convinced this is worth it right now.
There was a problem hiding this comment.
Cool, do I understand that this change only then impacts all workloads other than Pod, Deployment, StatefulSet? Because Pod, Deployment, StatefulSet already had the exclusions in the deleted code here:
- patch: |-
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
name: mutating-webhook-configuration
webhooks:
- name: mpod.kb.io
namespaceSelector:
matchExpressions:
- key: kubernetes.io/metadata.name
operator: NotIn
values:
- kube-system
- kueue-system
- name: mdeployment.kb.io
namespaceSelector:
matchExpressions:
- key: kubernetes.io/metadata.name
operator: NotIn
values:
- kube-system
- kueue-system
- name: mstatefulset.kb.io
namespaceSelector:
matchExpressions:
- key: kubernetes.io/metadata.name
operator: NotIn
values:
- kube-system
- kueue-system
….yaml Co-authored-by: Olek Zabłocki <olekz@google.com>
|
/test pull-kueue-test-e2e-multikueue-main |
| namespaceSelector: | ||
| {{- if (hasKey $managerConfig "managedJobsNamespaceSelector") -}} | ||
| {{- toYaml $managerConfig.managedJobsNamespaceSelector | nindent 6 -}} | ||
| {{- if (hasKey $managerConfig "managedJobsNamespaceSelector") }} |
There was a problem hiding this comment.
this par looks duplicated for all crds:
namespaceSelector:
{{- if (hasKey $managerConfig "managedJobsNamespaceSelector") -}}
{{- toYaml $managerConfig.managedJobsNamespaceSelector | nindent 6 -}}
{{- else }}
matchExpressions:
- key: kubernetes.io/metadata.name
operator: NotIn
values:
- kube-system
- '{{ .Release.Namespace }}'
{{- end }}
i'm wondering if there would be a way to introduce some kind of placeholder for that in processing-plain.yaml, wdyt?
This is not a blocker, but it would be good to research this topic on this occasion so that we can open follow up issue to maybe introduce that capability if not existing currently.
There was a problem hiding this comment.
I believe it should be generally doable with just a better yq selector(condition here), no need for named template yet, but generally I agree named templates also look like a viable way to achieve deduplication in similar cases.
I got some strange behavior of our yaml processor with new condition(not pushed yet), I will investigate on Monday.
There was a problem hiding this comment.
Done, needed a small hotfix to make this feasible, added a test for that.
| - webhooks.[name=vleaderworkerset.kb.io].namespaceSelector | ||
| - webhooks.[name=vmpijob.kb.io].namespaceSelector | ||
| - webhooks.[name=vpaddlejob.kb.io].namespaceSelector | ||
| - webhooks.[name=vpod.kb.io].namespaceSelector |
There was a problem hiding this comment.
FYI this target path is exactly the source path.
An AI bot told me it'll still work, even with create: true (just silently ignoring this target).
So maybe this is desired, for the completeness of the target list.
Or maybe it could be considered confusing/misleading.
I'm leaving the choice to you; I just wanted to make sure you're aware of this.
There was a problem hiding this comment.
Thanks for pointing this out.
Generally I consider this beneficial, as it provides mentioned completeness. In this way the number of webhooks in both selects is the same. Also they can be easier matched against expected number, you do not have to always remember this special case.
An AI bot told me it'll still work, even with create: true (just silently ignoring this target).
According to kustomize docs create: true just means to create the target path if it does not exists instead of throwing an error.
Generally this is a replacement of the same content with the same content, so I wouldn't expect any problems here.
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 56e78ee8647edbf06877089eb5a2a3c0437e523c |
|
/release-note-edit |
| path: /mutate-workload-codeflare-dev-v1beta2-appwrapper | ||
| failurePolicy: Fail | ||
| name: mappwrapper.kb.io | ||
| {{- if (hasKey $managerConfig "managedJobsNamespaceSelector") }} |
There was a problem hiding this comment.
qq: does this change:
{{- if (hasKey $managerConfig "managedJobsNamespaceSelector") }}
namespaceSelector:
{{- toYaml $managerConfig.managedJobsNamespaceSelector | nindent 6 }}
{{- end }}
to
namespaceSelector:
{{- if (hasKey $managerConfig "managedJobsNamespaceSelector") }}
{{- toYaml $managerConfig.managedJobsNamespaceSelector | nindent 6 }}
{{- else }}
matchExpressions:
- key: kubernetes.io/metadata.name
operator: NotIn
values:
- kube-system
- '{{ .Release.Namespace }}'
{{- end }}
impact the default configuration by Helm? IIUC it does because by default we would keep managedJobsNamespaceSelector as nil, and so by default Helm would cover kueue-system and kube-system, but I want to double check.
There was a problem hiding this comment.
Right now default helm deployment has namespace excludes only for the mentioned:
- statefulsets
- pods
- deployments
After this PR the namespaces will be excluded for all of the workload source types by default . Which makes those consistent across types and aligned with the release manifests.
There was a problem hiding this comment.
Cool, let's capture that in the release note
…r in processing plan
| buffer.WriteString(line + "\n") | ||
|
|
||
| if slices.Contains(keyLines, i+offset) { | ||
| if slices.Contains(keyLines, i) { |
There was a problem hiding this comment.
As keyLines are from the original yaml, we shouldn't be skipping lines if we add a new snippet.
|
/release-note-edit |
|
/lgtm Any opinion @tenzen-y ? |
|
LGTM label has been added. DetailsGit tree hash: e3a7c6d93ff7bd83f4bd9fcb5369d24c3c3adb80 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dkaluza, mimowo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I think we can skip the cherrypick as of now because no user requested the change as a bugfix for now. We will reconsider when this is needed to CP another bugfix, or if a user requests. |
What type of PR is this?
/kind feature
/area integrations
What this PR does / why we need it:
Adds
kube-systemandkueue-system(or other release namespace in case of helm installation) namespace excludes to webhooks targeting common resource types.(Custom resources that are not added by Kueue)
This makes sure Kueue deployment does not suspend crucial system components, e.g. jobs run by
kubeadm.Which issue(s) this PR fixes:
Fixes #11006
Special notes for your reviewer:
I haven't modified webhook selectors for Kueue's own resources as those shouldn't be run by other crucial system components. But if you think it is worth to also exclude those, just let me know.
Prepared with Antigravity, but carefully reviewed to not miss any resource.
Does this PR introduce a user-facing change?