-
Notifications
You must be signed in to change notification settings - Fork 450
services: add label app to services to be able to use a labelSelector #7371
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.
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
services: add label app to services to be able to use a labelSelector #7371
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
|
Hi @rphillips. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions 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 |
e86f8d7 to
bf879bb
Compare
|
@rphillips have you had a chance to test end-to-end manually if this works well on upgrade? Ideally we test it both for manifests-based version and Helm |
bf879bb to
8a9457e
Compare
I agree being able to select services of a specific application like Kueue is important, but I we already have the app.kubernetes.io/name label for this purpose. I tested locally installing Kueue both from manifest and Helm and got results. after Helm installation: ❯ k get svc -nkueue-system --show-labels
NAME TYPE CLUSTER-IP EXTERNAL-IP PORT(S) AGE LABELS
kueue-controller-manager-metrics-service ClusterIP 10.96.227.177 <none> 8443/TCP 35m app.kubernetes.io/component=metrics,app.kubernetes.io/instance=kueue,app.kubernetes.io/managed-by=Helm,app.kubernetes.io/name=kueue,app.kubernetes.io/version=v0.14.2,control-plane=controller-manager,helm.sh/chart=kueue-0.14.2
kueue-visibility-server ClusterIP 10.96.7.227 <none> 443/TCP 35m app.kubernetes.io/instance=kueue,app.kubernetes.io/managed-by=Helm,app.kubernetes.io/name=kueue,app.kubernetes.io/version=v0.14.2,control-plane=controller-manager,helm.sh/chart=kueue-0.14.2
kueue-webhook-service ClusterIP 10.96.114.17 <none> 443/TCP 35m app.kubernetes.io/instance=kueue,app.kubernetes.io/managed-by=Helm,app.kubernetes.io/name=kueue,app.kubernetes.io/version=v0.14.2,control-plane=controller-manager,helm.sh/chart=kueue-0.14.2after manifest installation: ❯ k get svc -nkueue-system --show-labels
NAME TYPE CLUSTER-IP EXTERNAL-IP PORT(S) AGE LABELS
kueue-controller-manager-metrics-service ClusterIP 10.96.128.161 <none> 8443/TCP 44s app.kubernetes.io/component=controller,app.kubernetes.io/managed-by=kustomize,app.kubernetes.io/name=kueue,control-plane=controller-manager
kueue-visibility-server ClusterIP 10.96.204.159 <none> 443/TCP 44s app.kubernetes.io/component=controller,app.kubernetes.io/name=kueue,control-plane=controller-manager
kueue-webhook-service ClusterIP 10.96.212.149 <none> 443/TCP 44s app.kubernetes.io/component=controller,app.kubernetes.io/name=kueue,control-plane=controller-managerI think what could be improved clearly is the use of the Also, instead of the generic "app" label I prefer to make sure the standard Let me know if I'm missing something, and let me hold until this is clarfied. |
8a9457e to
a5c43d2
Compare
|
No problem. I updated the helm chart to result in: And the kustomize charts to: Are there any other changes you would like to see? |
|
The output for Helm looks great. I just need another test if the upgrade works well, unfortunately we don't have automated upgrade tests and this change has a potential impact. The output for installation from manifests contains |
a5c43d2 to
9743ab4
Compare
|
Yep. Sorry. Copy and paste error on the kustomize output (fixed). I also updated the gist with the latest test scripts I used. Also fixed pull-kueue-verify-main. |
|
@rphillips I think as we should update the release note as the PR changed. Proposal: /release-note-edit wdyt? |
|
Works for me! Looks like the PR got updated. |
|
Thank you 👍 |
|
@mimowo: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions 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. |
|
LGTM label has been added. Git tree hash: 35a11dd42e454ea9f895d2a39ff2b553b032b7aa
|
| labels: | ||
| - pairs: | ||
| app.kubernetes.io/name: kueue | ||
| app.kubernetes.io/component: controller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this drop the app.kubernetes.io/component label from the kueue-controller-manager deployment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to fix this slightly different. This specific spot adds app.kubernetes.io/component=controller to all the other components. I'll update the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed the fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
|
/hold |
9743ab4 to
e8542a4
Compare
|
I don't see app.kubernetes.io/component set with 0.14.2 on controller-manager with Helm, but I'll fix that. With this PR and helm: With this PR and kustomize: |
config/default/kustomization.yaml
Outdated
| name: controller-manager-metrics-service | ||
| namespace: system | ||
| labels: | ||
| app.kubernetes.io/component: controller-manager-metrics-service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| app.kubernetes.io/component: controller-manager-metrics-service | |
| app.kubernetes.io/component: metrics-service |
I want to propose simpler name here. We don't need to rename the service name in this PR, just the label. The name of the labels does not need to match the service name exactly anyway as the service names have the "kueue-" prefix anyway. wdyt? cc @tenzen-y
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should have a prefix since users typically deploy the external manager including job controller implementing jobframework and external dispatchers to the same namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by prefix do you mean controller-manager- or kueue-?
In any case I don't think this is needed, because we have another standard app.kubernetes.io/name label which sets the context. So users who write label selectors can use both app.kubernetes.io/name: kueue and app.kubernetes.io/component: metrics-service.
I think this is the intention of the app.kubernetes.io/component is not to repeat the app name, see examples here: https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels
# This is an excerpt
apiVersion: apps/v1
kind: StatefulSet
metadata:
labels:
app.kubernetes.io/name: mysql
app.kubernetes.io/instance: mysql-abcxyz
app.kubernetes.io/version: "5.7.21"
app.kubernetes.io/component: database
app.kubernetes.io/part-of: wordpress
app.kubernetes.io/managed-by: HelmThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense.
In that case, the service for keueu external-controller-manager could have app.kubernetes.io/name: kueue-external-manager and app.kubernetes.io/instance: metrics-service label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is kueue external-controller-manager ? TBH I think the app name is simply "kueue".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kueue Controller Manager is hosted in this repository.
Kueue External Controller Manager is served in users's respotisoty which has custom JobFramework controller and external dispatchers.
In this repository, we use just app.kubernetes.io/instance: metrics-service and app.kubernetes.io/name: kueue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thank you for clarifying 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll commit this and squash the commits. Thank you for the feedback.
|
Updated. With this PR and helm: With this PR and kustomize: |
e8542a4 to
d12fd59
Compare
|
/lgtm |
|
@mimowo: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions 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. |
|
LGTM label has been added. Git tree hash: 2d7ac314faa9078832209e45c068b7704cb0f8ce
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mimowo, rphillips The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
|
@mimowo: new pull request created: #7450 In response to this:
Instructions 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. |
|
@mimowo: new pull request created: #7451 In response to this:
Instructions 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. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR is trivial though important. There is not a current way to individually select a service by a labelSelector. For instance,
control-plane: controller-managermatches numerous components.It becomes more important when wiring up Prometheus to a specific endpoint. Not all the services export a /metrics endpoint thus causing Prometheus to fail on requests to that endpoint, so matching on everything with
control-plane: controller-managerdoes not work.The PR adds a unique label - the same name as the service - so a labelSelector can be used to select a particular service.
I would like to request this patch be backported to 0.14.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?