Use a uniquely identifying selector based on recommended labels for Kueue deployment #1695

astefanutti · 2024-02-06T16:06:34Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR proposes to change the default label selector currently used by the Kueue deployment from:

control-plane: controller-manager

to the following selector, that uses Kubernetes recommended labels to uniquely identify Kueue Pods:

app.kubernetes.io/name: kueue
app.kubernetes.io/component: controller

Which issue(s) this PR fixes:

Fixes #1685

Special notes for your reviewer:

The script that updates the Helm charts has to be adapted to apply parameterised labels selectors.

Does this PR introduce a user-facing change?

Use recommended labels and a uniquely identifying selector for Kueue deployment resources.

ACTION REQUIRED: You need to recreate the Kueue deployment if you had it previously installed,
 as the label selector field is immutable.

netlify · 2024-02-06T16:06:39Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`0915e3b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/65e6d12828f25d00084a6b53

k8s-ci-robot · 2024-02-06T16:06:43Z

Hi @astefanutti. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

alculquicondor · 2024-02-09T20:01:15Z

/ok-to-test

mimowo

Generally lgtm, some asks for explanations as I'm not that familiar with the helm scripts yet

charts/kueue/templates/_helpers.tpl

charts/kueue/templates/prometheus/monitor.yaml

charts/kueue/templates/rbac/batch_admin_role.yaml

config/components/prometheus/monitor.yaml

hack/update-helm.sh

astefanutti · 2024-02-15T15:46:55Z

/test pull-kueue-test-e2e-main-1-27

astefanutti · 2024-02-27T08:37:30Z

/test pull-kueue-test-integration-main

tenzen-y

Overall lgtm
I left comments for some nits.
/approve

Makefile

config/components/visibility/service.yaml

astefanutti · 2024-03-04T17:44:12Z

/retest

astefanutti · 2024-03-04T18:13:19Z

/test pull-kueue-verify-main

alculquicondor · 2024-03-04T18:15:20Z

It looks like something went rogue in the bot.

alculquicondor · 2024-03-04T18:24:24Z

PTAL kubernetes/test-infra#32152

Co-authored-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

tenzen-y

Thank you!
/lgtm
/approve

k8s-ci-robot · 2024-03-05T08:29:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astefanutti, tenzen-y

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:

~~OWNERS~~ [tenzen-y]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2024-03-05T08:29:15Z

LGTM label has been added.

Git tree hash: da7d427c1e61473d95ef960c610b8dfd6c313c96

…ueue deployment (kubernetes-sigs#1695) * Use recommended labels and a uniquely identifying selector * update-helm script applies k8s recommended labels * Use kueue.labels as label selector in Helm ServiceMonitor chart * Add app.kubernetes.io/instance label back to Helm kueue.selectorLabels * Update config/components/visibility/service.yaml Co-authored-by: Yuki Iwai <yuki.iwai.tz@gmail.com> * Update Makefile Co-authored-by: Yuki Iwai <yuki.iwai.tz@gmail.com> --------- Co-authored-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

alculquicondor · 2024-05-08T17:55:12Z

I'm worried that this change can cause significant friction to upgrade.

Should we consider a revert? Is anyone familiar with how other projects have achieved this change with minimal disruption?

tenzen-y · 2024-05-08T18:00:12Z

I'm worried that this change can cause significant friction to upgrade.

Should we consider a revert? Is anyone familiar with how other projects have achieved this change with minimal disruption?

Your concern is right. Actually, the apply command should be failed since updating the label is not allowed.
In my OSS experience, I usually follow the deprecation steps like:

Support both (old and new) labels for 1 or 2 releases and notice breaking changes to users.
After 1 or 2 releases pass away, old labels are removed.

@alculquicondor I'm curious about whether we can do a similar approach.
WDYT?

alculquicondor · 2024-05-08T18:56:25Z

Overall, that sounds good, but: at some point, we will have to update the Deployment object. And that will always be a breaking change.
Unless, in addition to your plan, we create a new Deployment and change the existing one to have 0 replicas?

tenzen-y · 2024-05-09T06:30:06Z

Overall, that sounds good, but: at some point, we will have to update the Deployment object. And that will always be a breaking change.

You're right.

Unless, in addition to your plan, we create a new Deployment and change the existing one to have 0 replicas?

That makes sense.

alculquicondor · 2024-05-09T18:18:01Z

So for now, let's revert the selector change and add the old and new labels?

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Feb 6, 2024

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 6, 2024

k8s-ci-robot requested review from mimowo and trasc February 6, 2024 16:06

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 6, 2024

astefanutti force-pushed the pr-12 branch from 8823722 to 5d4a90d Compare February 13, 2024 14:49

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2024

astefanutti marked this pull request as ready for review February 13, 2024 18:18

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 13, 2024

k8s-ci-robot requested a review from denkensk February 13, 2024 18:18

mimowo reviewed Feb 14, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2024

astefanutti force-pushed the pr-12 branch from cc5dc1b to d48acee Compare February 27, 2024 08:15

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2024

tenzen-y reviewed Mar 2, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

config/components/visibility/service.yaml Show resolved Hide resolved

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 2, 2024

astefanutti force-pushed the pr-12 branch from 10cc605 to aea884c Compare March 4, 2024 17:30

astefanutti mentioned this pull request Mar 4, 2024

Improve Helm charts sync from Kustomize manifests #1798

Open

3 tasks

astefanutti and others added 6 commits March 5, 2024 09:00

Use recommended labels and a uniquely identifying selector

39cdf80

update-helm script applies k8s recommended labels

ed71816

Use kueue.labels as label selector in Helm ServiceMonitor chart

71e8d46

Add app.kubernetes.io/instance label back to Helm kueue.selectorLabels

35be093

Update config/components/visibility/service.yaml

53e07fa

Co-authored-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

Update Makefile

0915e3b

Co-authored-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

astefanutti force-pushed the pr-12 branch from aea884c to 0915e3b Compare March 5, 2024 08:00

tenzen-y reviewed Mar 5, 2024

View reviewed changes

k8s-ci-robot assigned tenzen-y Mar 5, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2024

k8s-ci-robot merged commit bbcb47b into kubernetes-sigs:main Mar 5, 2024
14 checks passed

k8s-ci-robot added this to the v0.7 milestone Mar 5, 2024

astefanutti deleted the pr-12 branch March 5, 2024 09:31

mimowo mentioned this pull request Mar 13, 2024

[e2e] Fix visibility tests. #1831

Merged

k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a uniquely identifying selector based on recommended labels for Kueue deployment #1695

Use a uniquely identifying selector based on recommended labels for Kueue deployment #1695

astefanutti commented Feb 6, 2024 •

edited by alculquicondor

netlify bot commented Feb 6, 2024 •

edited

k8s-ci-robot commented Feb 6, 2024

alculquicondor commented Feb 9, 2024

mimowo left a comment

astefanutti commented Feb 15, 2024

astefanutti commented Feb 27, 2024

tenzen-y left a comment

astefanutti commented Mar 4, 2024

astefanutti commented Mar 4, 2024

alculquicondor commented Mar 4, 2024

alculquicondor commented Mar 4, 2024

tenzen-y left a comment

k8s-ci-robot commented Mar 5, 2024

k8s-ci-robot commented Mar 5, 2024

alculquicondor commented May 8, 2024

tenzen-y commented May 8, 2024

alculquicondor commented May 8, 2024

tenzen-y commented May 9, 2024

alculquicondor commented May 9, 2024

Use a uniquely identifying selector based on recommended labels for Kueue deployment #1695

Use a uniquely identifying selector based on recommended labels for Kueue deployment #1695

Conversation

astefanutti commented Feb 6, 2024 • edited by alculquicondor

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

netlify bot commented Feb 6, 2024 • edited

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

k8s-ci-robot commented Feb 6, 2024

alculquicondor commented Feb 9, 2024

mimowo left a comment

Choose a reason for hiding this comment

astefanutti commented Feb 15, 2024

astefanutti commented Feb 27, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

astefanutti commented Mar 4, 2024

astefanutti commented Mar 4, 2024

alculquicondor commented Mar 4, 2024

alculquicondor commented Mar 4, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 5, 2024

k8s-ci-robot commented Mar 5, 2024

alculquicondor commented May 8, 2024

tenzen-y commented May 8, 2024

alculquicondor commented May 8, 2024

tenzen-y commented May 9, 2024

alculquicondor commented May 9, 2024

astefanutti commented Feb 6, 2024 •

edited by alculquicondor

netlify bot commented Feb 6, 2024 •

edited