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.

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

propagate priority-class label for deploy and statefulset #3980

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Abirdcfly
Copy link
Member

@Abirdcfly Abirdcfly commented Jan 16, 2025

What type of PR is this?

/kind bug

What this PR does / why we need it:

now, kueue auto propagete queue-name for deploy and statefulset, this pr will also add priority-class.

apiVersion: apps/v1
kind: Deployment
metadata:
  name: demo
  labels:
    app: demo
    kueue.x-k8s.io/queue-name: queue-1 # 1. this label will auto update to spec.template.metadata.labels
    kueue.x-k8s.io/priority-class: p10 # 2. but this label can't
spec:
  template:
    metadata:
      labels:
        app: demo
        kueue.x-k8s.io/priority-class: p10 # 3. so we need add this label manually
    ...

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Propagate the top-level setting of the `kueue.x-k8s.io/priority-class` label to the PodTemplate for
Deployments and StatefulSets. This way the Workload Priority class is no longer ignored by the workloads.

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Jan 16, 2025
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 16, 2025
Copy link

netlify bot commented Jan 16, 2025

Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 3b88a05
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/678e114edc3fea0008cec187

@Abirdcfly Abirdcfly marked this pull request as ready for review January 16, 2025 08:09
@k8s-ci-robot 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 Jan 16, 2025
@k8s-ci-robot k8s-ci-robot requested a review from tenzen-y January 16, 2025 08:09
@kannon92
Copy link
Contributor

Why just deployments?

I see that we aren’t passing these down for the other frameworks. Should we?

@mimowo
Copy link
Contributor

mimowo commented Jan 20, 2025

Thank you for the PR.

I think we need to do it for Deployments because the actual workloads are created per Pods.

Similarly we need to propagate it for LWS so that the workloads per groups are prioritized correctly cc @mbobrovskyi .

Except for Deployments and LWS the workloads are created actually at the CRD level and so I don't see a need to propagate the workload priorities down to pods for other Cards. Let me know if I'm missing a scenario.

Actually I think this is more of a bugfix than a new feature because the API already existed we just didn't support it.

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 20, 2025
@mimowo
Copy link
Contributor

mimowo commented Jan 20, 2025

@Abirdcfly can you add some test for it? I think unit or E2e would be appropriate

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 20, 2025
@Abirdcfly Abirdcfly changed the title propagate priority-class label for deploy propagate priority-class label for deploy and statefulset Jan 20, 2025
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 20, 2025
@Abirdcfly Abirdcfly changed the title propagate priority-class label for deploy and statefulset propagate priority-class label for deploy, statefulset and lws Jan 20, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add validation for this field on ValidateUpdate()?

Copy link
Contributor

Choose a reason for hiding this comment

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

What type of validation do you mean? Also, would it be part of the PR or you are asking about a follow up?

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 we should keep it immutable on ValidateUpdate like we have for other Jobs

func validateUpdateForWorkloadPriorityClassName(oldJob, newJob GenericJob) field.ErrorList {
allErrs := apivalidation.ValidateImmutableField(workloadPriorityClassName(newJob), workloadPriorityClassName(oldJob), workloadPriorityClassNamePath)
return allErrs
}
.

Copy link
Contributor

@mimowo mimowo Jan 20, 2025

Choose a reason for hiding this comment

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

I have synced with @mbobrovskyi that the intended validation is to make this field immutable as we do it for Jobs: https://github.com/kubernetes-sigs/kueue/blob/main/pkg/controller/jobframework/validation.go#L126-L129.
I think this makes sense for consistency. I think we can relax later if there are such requests.

My only concern is that strengthening the validation could break some users, but since the workload-priority is currently ignored anyway so I think it is safe to strengthen the validation, and align with Jobs, as part of the PR.

Any opinions @Abirdcfly @dgrove-oss @tenzen-y ?

@mimowo
Copy link
Contributor

mimowo commented Jan 20, 2025

Actually, LWS I suggest to move to another PR, so that we can easily cherry-pick only the Deployments and STS if needed - I'm not yet sure this is something we want to qualify for cherry-picking, but it would be good to keep the door open.

@k8s-ci-robot k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 20, 2025
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 20, 2025
@Abirdcfly Abirdcfly changed the title propagate priority-class label for deploy, statefulset and lws propagate priority-class label for deploy and statefulset Jan 20, 2025
@mimowo
Copy link
Contributor

mimowo commented Jan 20, 2025

/release-note-edit

Propagate the top-level setting of the `kueue.x-k8s.io/priority-class` label to the PodTemplate for
Deployments and StatefulSets. This way the Workload Priority class is no longer ignored by the workloads.

@k8s-ci-robot 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-none Denotes a PR that doesn't merit a release note. labels Jan 20, 2025
@mimowo
Copy link
Contributor

mimowo commented Jan 20, 2025

/remove-kind feature
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot removed the kind/feature Categorizes issue or PR as related to a new feature. label Jan 20, 2025
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 4657cdf917c1652fb5756d211a5f6513d39c7480

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Abirdcfly, mimowo

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2025
@mimowo
Copy link
Contributor

mimowo commented Jan 20, 2025

/hold
For the pending comment #3980 (comment)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants