Introduce workload.PatchStatus function to commonize workload status updates #8063

mbobrovskyi · 2025-12-03T18:33:20Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Use PatchStatus().

Which issue(s) this PR fixes:

Part of #7035

Special notes for your reviewer:

As we discussed here #8031 (comment) we should comminize patch logic.

Does this PR introduce a user-facing change?

NONE

netlify · 2025-12-03T18:33:27Z

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	`dbaa8e1`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-kueue/deploys/6932b9e09a207800080113f4
😎 Deploy Preview	https://deploy-preview-8063--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 project configuration.

mbobrovskyi · 2025-12-03T18:34:16Z

/cc @mszadkow @mimowo

pkg/workload/workload.go

mbobrovskyi · 2025-12-04T05:19:13Z

pkg/workload/workload.go

+		if updated, err := update(wl); err != nil || !updated {
+			return updated, err
+		}
+		if !features.Enabled(features.WorkloadRequestUseMergePatch) {


In PatchAdmissionStatus, we should first apply the desired changes and then execute PrepareWorkloadPatch. But in PatchStatus, we should start with BaseSSAWorkload patch and apply the changes afterward.

mimowo · 2025-12-04T06:45:11Z

/retitle Introduce workload.PatchStatus function to commonize workload status updates

pkg/workload/workload.go

k8s-ci-robot · 2025-12-04T12:36:14Z

LGTM label has been added.

Git tree hash: 3870fd8964c77a0fced0c3e42252b3783c45979c

mbobrovskyi · 2025-12-04T13:55:59Z

https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_kueue/8063/pull-kueue-test-integration-baseline-main/1996576852675661824

Looks like this is flaky issue. Created fix #8073.

mbobrovskyi · 2025-12-04T14:01:05Z

@IrvingMg @mszadkow @mykysha thank you for review 🙏

mbobrovskyi · 2025-12-04T14:01:12Z

cc: @mimowo

mimowo · 2025-12-04T15:03:01Z

https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_kueue/8063/pull-kueue-test-e2e-customconfigs-main/1996583380052873216
do you think this might be related? I think we have this as a known flake, but worth to check

mbobrovskyi · 2025-12-05T03:14:49Z

https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_kueue/8063/pull-kueue-test-e2e-customconfigs-main/1996583380052873216 do you think this might be related? I think we have this as a known flake, but worth to check

I think this is a flake #5106. But after introducing 4573156, we’re seeing this flake more often.

mbobrovskyi · 2025-12-05T07:17:34Z

/retest

Due to #8073 merged.

…updates.

This reverts commit 0511918.

mimowo · 2025-12-05T17:46:24Z

pkg/workload/workload.go

+			return updated, err
+		}
+		if !features.Enabled(features.WorkloadRequestUseMergePatch) {
+			wlPatch := PrepareWorkloadPatch(wl, opts.StrictApply, clk)


I think we need to rename this function, or even remove and just use

wlPatch := BaseSSAWorkload(wl, opts.StrictApply) PrepareAdmissionPatch(w, wlCopy, clk) return wlCopy

where PrepareAdmissionPatch encapsulates the functions:

admissionStatusPatch admissionChecksStatusPatch

It is rather a follow up

We have other places that use this function. After fully migrating to this approach, I think we can remove it entirely.

mimowo · 2025-12-05T17:49:17Z

pkg/workload/workload.go

+			wlPatch := BaseSSAWorkload(wl, opts.StrictApply)
+			wlPatch.DeepCopyInto(wl)
+		}
+		return update(wl)


Why is the update here at the bottom, and for PatchAdmissionStatus it is at the top?

I think it is actually slightly better to place it at the top, then it can make adjustments based on the original workload.

Also, I'm wondering if we could then commonize calling update and place it inside patchStatus, wdyt?

It will not work. In some places we use other fields to construct the status (for example .spec.localQueue). If we move it before the update, we will get a lot of errors because the fields will be empty.

Right but my point is to move calling the update before BaseSSAWorkload in both cases, would this not work either?

Unfortunately no.

kueue/pkg/workload/workload.go

Lines 735 to 757 in 98c941f

func BaseSSAWorkload(w *kueue.Workload, strict bool) *kueue.Workload {

wlCopy := &kueue.Workload{

ObjectMeta: metav1.ObjectMeta{

UID: w.UID,

Name: w.Name,

Namespace: w.Namespace,

Generation: w.Generation, // Produce a conflict if there was a change in the spec.

Annotations: maps.Clone(w.Annotations),

Labels: maps.Clone(w.Labels),

},

TypeMeta: w.TypeMeta,

}

if wlCopy.APIVersion == "" {

wlCopy.APIVersion = kueue.GroupVersion.String()

}

if wlCopy.Kind == "" {

wlCopy.Kind = "Workload"

}

if strict {

wlCopy.ResourceVersion = w.ResourceVersion

}

return wlCopy

}

Base BaseSSAWorkload() creates almost empty patch.

So after patch creation we will loose all changes.

Do you mean that calling wlPatch.DeepCopyInto(wl) would zero all the workload fields in wl other than those in the wlPatch?

Yes, exactly – we will replace all fields with zero values after running wlPatch.DeepCopyInto(wl).

Gotcha. I think to make it more generic we may actually make the update function to accept two workloads (origWorkload, patchWorkload). Then, if all fields are needed, then it would use origWorkload, but it would record changes to patchWorkload. However, this may not be very easy because many functions we have don't make this distinction.

I'm ok for now, but this is something we may need to still rethink , but I still don't know the ideal state for now, so let's keep it evolving and see where we are.

mimowo

LGTM overall, thank you for commonizing. It will make it easier to maintain over time. I have one comment to potentially commonize the code even more https://github.com/kubernetes-sigs/kueue/pull/8063/files#r2593474604
It could also be a follow up.

mimowo

/lgtm
/approve
Thank you 👍

k8s-ci-robot · 2025-12-05T18:40:12Z

LGTM label has been added.

Git tree hash: b8778772b523d5eb46ffc7b84fbf00b48da3a5d5

k8s-ci-robot · 2025-12-05T18:40:14Z

[APPROVALNOTIFIER] This PR is APPROVED

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

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Dec 3, 2025

k8s-ci-robot requested review from kannon92 and tenzen-y December 3, 2025 18:33

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 3, 2025

mbobrovskyi mentioned this pull request Dec 3, 2025

Use WorkloadRequestUseMergePatch in UpdateReclaimablePods(). #8031

Merged

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 3, 2025

k8s-ci-robot requested review from mimowo and mszadkow December 3, 2025 18:34

mimowo reviewed Dec 3, 2025

View reviewed changes

pkg/workload/workload.go Outdated Show resolved Hide resolved

pkg/workload/workload.go Show resolved Hide resolved

mbobrovskyi commented Dec 4, 2025

View reviewed changes

mbobrovskyi force-pushed the cleanup/use-patch-status-2 branch from 7736341 to 06f8280 Compare December 4, 2025 06:28

k8s-ci-robot changed the title ~~Use PatchStatus().~~ Introduce workload.PatchStatus function to commonize workload status updates Dec 4, 2025

mszadkow reviewed Dec 4, 2025

View reviewed changes

pkg/workload/workload.go Outdated Show resolved Hide resolved

mbobrovskyi force-pushed the cleanup/use-patch-status-2 branch 2 times, most recently from 8e62a7d to fa678b5 Compare December 4, 2025 09:28

mbobrovskyi requested review from mimowo and mszadkow December 4, 2025 09:29

mbobrovskyi force-pushed the cleanup/use-patch-status-2 branch 3 times, most recently from e231766 to 3473192 Compare December 4, 2025 10:01

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 4, 2025

IrvingMg reviewed Dec 4, 2025

View reviewed changes

pkg/workload/workload.go Outdated Show resolved Hide resolved

pkg/workload/workload.go Outdated Show resolved Hide resolved

mbobrovskyi force-pushed the cleanup/use-patch-status-2 branch 2 times, most recently from 8dbde58 to 75978bf Compare December 4, 2025 11:49

mbobrovskyi requested a review from IrvingMg December 4, 2025 11:53

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2025

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2025

k8s-ci-robot requested a review from mykysha December 4, 2025 13:05

mbobrovskyi force-pushed the cleanup/use-patch-status-2 branch from 5c10848 to e354e53 Compare December 4, 2025 13:43

mykysha mentioned this pull request Dec 4, 2025

Fix DRA race conditions. #8073

Merged

mbobrovskyi force-pushed the cleanup/use-patch-status-2 branch from e9e5752 to e354e53 Compare December 4, 2025 14:12

mbobrovskyi added 4 commits December 5, 2025 16:23

Introduce workload.PatchStatus function to commonize workload status …

59737bb

…updates.

Split patchStatus to applyPatchStatus and patchStatus.

0511918

Optimize unit tests

71bcef8

Revert "Split patchStatus to applyPatchStatus and patchStatus."

dbaa8e1

This reverts commit 0511918.

mbobrovskyi force-pushed the cleanup/use-patch-status-2 branch from e354e53 to dbaa8e1 Compare December 5, 2025 10:54

mimowo reviewed Dec 5, 2025

View reviewed changes

k8s-ci-robot assigned mimowo Dec 5, 2025

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2025

k8s-ci-robot merged commit e834426 into kubernetes-sigs:main Dec 5, 2025
28 checks passed

k8s-ci-robot added this to the v0.16 milestone Dec 5, 2025

mbobrovskyi deleted the cleanup/use-patch-status-2 branch December 6, 2025 02:50

	func BaseSSAWorkload(w kueue.Workload, strict bool) kueue.Workload {
	wlCopy := &kueue.Workload{
	ObjectMeta: metav1.ObjectMeta{
	UID: w.UID,
	Name: w.Name,
	Namespace: w.Namespace,
	Generation: w.Generation, // Produce a conflict if there was a change in the spec.
	Annotations: maps.Clone(w.Annotations),
	Labels: maps.Clone(w.Labels),
	},
	TypeMeta: w.TypeMeta,
	}
	if wlCopy.APIVersion == "" {
	wlCopy.APIVersion = kueue.GroupVersion.String()
	}
	if wlCopy.Kind == "" {
	wlCopy.Kind = "Workload"
	}
	if strict {
	wlCopy.ResourceVersion = w.ResourceVersion
	}
	return wlCopy
	}

Introduce workload.PatchStatus function to commonize workload status updates #8063

Introduce workload.PatchStatus function to commonize workload status updates #8063

Conversation

mbobrovskyi commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

netlify bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Uh oh!

mbobrovskyi commented Dec 3, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimowo commented Dec 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

k8s-ci-robot commented Dec 4, 2025

Uh oh!

mbobrovskyi commented Dec 4, 2025

Uh oh!

mbobrovskyi commented Dec 4, 2025

Uh oh!

mbobrovskyi commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mimowo commented Dec 4, 2025

Uh oh!

mbobrovskyi commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbobrovskyi commented Dec 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbobrovskyi Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbobrovskyi Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimowo left a comment

Choose a reason for hiding this comment

Uh oh!

mimowo left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Dec 5, 2025

Uh oh!

k8s-ci-robot commented Dec 5, 2025

Uh oh!

Uh oh!

Reviewers

mbobrovskyi commented Dec 3, 2025 •

edited

Loading

netlify bot commented Dec 3, 2025 •

edited

Loading

mbobrovskyi commented Dec 4, 2025 •

edited

Loading

mbobrovskyi commented Dec 5, 2025 •

edited

Loading

mbobrovskyi Dec 5, 2025 •

edited

Loading

mbobrovskyi Dec 5, 2025 •

edited

Loading