propagate priority-class label for deploy and statefulset

[Abirdcfly]

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]

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

[netlify]

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	04d6fe9
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/678f0d56458d2800089ea0a8
😎 Deploy Preview	https://deploy-preview-3980--kubernetes-sigs-kueue.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[kannon92]

Why just deployments?

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

[mimowo]

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

[mimowo]

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

[mbobrovskyi]

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

[mimowo]

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

[mbobrovskyi]

I think we should keep it immutable on ValidateUpdate like we have for other Jobs

kueue/pkg/controller/jobframework/validation.go

Lines 126 to 129 in cf5ef9f

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

.

[mimowo]

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 ?

[Abirdcfly]

The Code has been updated to match the Jobs: modifications to workload-priority are not allowed.

I have synced with @mbobrovskyi that the intended validation is to make this field immutable as we do it for Jobs: 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]

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]

LGTM label has been added.

Git tree hash: 4657cdf917c1652fb5756d211a5f6513d39c7480

[mimowo]

/hold
For the pending comment #3980 (comment)

[mbobrovskyi]

/lgtm
Thanks!

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 9b9656280c850d8123030545cd4a2029b2511e22

[mimowo]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Abirdcfly, mbobrovskyi, 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:

• OWNERS [mimowo]

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

[mimowo]

/hold cancel

[mimowo]

/cherry-pick release-0.10 release-0.9

[k8s-infra-cherrypick-robot]

@mimowo: once the present PR merges, I will cherry-pick it on top of release-0.10 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.10 release-0.9

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.

[k8s-infra-cherrypick-robot]

@mimowo: #3980 failed to apply on top of branch "release-0.10":

Applying: fix: propagate priority-class label for deploy and statefulset
Using index info to reconstruct a base tree...
M	pkg/controller/jobframework/reconciler.go
M	pkg/controller/jobframework/validation.go
M	pkg/controller/jobs/statefulset/statefulset_webhook.go
M	pkg/controller/jobs/statefulset/statefulset_webhook_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/controller/jobs/statefulset/statefulset_webhook_test.go
Auto-merging pkg/controller/jobs/statefulset/statefulset_webhook.go
CONFLICT (content): Merge conflict in pkg/controller/jobs/statefulset/statefulset_webhook.go
Auto-merging pkg/controller/jobframework/validation.go
Auto-merging pkg/controller/jobframework/reconciler.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 fix: propagate priority-class label for deploy and statefulset

In response to this:

/cherry-pick release-0.10 release-0.9

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.

[Abirdcfly]

@mimowo: #3980 failed to apply on top of branch "release-0.10":

Applying: fix: propagate priority-class label for deploy and statefulset
Using index info to reconstruct a base tree...
M	pkg/controller/jobframework/reconciler.go
M	pkg/controller/jobframework/validation.go
M	pkg/controller/jobs/statefulset/statefulset_webhook.go
M	pkg/controller/jobs/statefulset/statefulset_webhook_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/controller/jobs/statefulset/statefulset_webhook_test.go
Auto-merging pkg/controller/jobs/statefulset/statefulset_webhook.go
CONFLICT (content): Merge conflict in pkg/controller/jobs/statefulset/statefulset_webhook.go
Auto-merging pkg/controller/jobframework/validation.go
Auto-merging pkg/controller/jobframework/reconciler.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 fix: propagate priority-class label for deploy and statefulset

It seems that these files have been modified in different branches, and I will manually submit 2 PRs to the branches release-0.10 and release-0.9 later.