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?

Add recommended Kubernetes labels to uniquely identify Pods and other resources installed with Kueue.
The Deployment selector remains unchanged to allow for a seamless upgrade.

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

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?

tenzen-y · 2024-05-10T15:19:28Z

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

SGTM Just confirmation. After reverting this PR, we will create a new PR with manifests using new labels, right?

alculquicondor · 2024-05-10T15:26:17Z

Well, this PR has a few more changes.

Is it easier to just manually undo the changes to the Deployment selector?

Is this something you can take @tenzen-y ?

alculquicondor · 2024-05-23T16:48:58Z

The partial revert is not trivial. I think we should do a full revert #2246 (comment)

tenzen-y · 2024-05-24T04:01:26Z

The partial revert is not trivial. I think we should do a full revert #2246 (comment)

I'm ok with completely reverting.

* Restore control-plane label in kustomize templates * Restore control-plane label in helm template

alculquicondor · 2024-05-28T18:33:36Z

Because we merged #1695

/release-note-edit

Add recommended Kubernetes labels to uniquely identify Pods and other resources installed with Kueue.
The Deployment selector remains unchanged to allow for a seamless upgrade.

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 2 commits March 5, 2024 09:00

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

alculquicondor mentioned this pull request May 17, 2024

Partial revert of #1695: Use a uniquely identifying selector based #2218

Closed

IrvingMg mentioned this pull request May 21, 2024

Partial revert of #1695: Restore control-plane selector #2246

Merged

k8s-ci-robot pushed a commit that referenced this pull request May 28, 2024

Partial revert of #1695: Restore control-plane selector (#2246)

e999515

* Restore control-plane label in kustomize templates * Restore control-plane label in helm template

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels May 28, 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 k8s-ci-robot

Loading

netlify bot commented Feb 6, 2024 •

edited

Loading

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

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

tenzen-y commented May 10, 2024

alculquicondor commented May 10, 2024

alculquicondor commented May 23, 2024 •

edited

Loading

tenzen-y commented May 24, 2024

alculquicondor commented May 28, 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 k8s-ci-robot Loading

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 Loading

✅ 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

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

tenzen-y commented May 10, 2024

alculquicondor commented May 10, 2024

alculquicondor commented May 23, 2024 • edited Loading

tenzen-y commented May 24, 2024

alculquicondor commented May 28, 2024

astefanutti commented Feb 6, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented Feb 6, 2024 •

edited

Loading

alculquicondor commented May 23, 2024 •

edited

Loading