Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 67 additions & 53 deletions pkg/workload/workload.go
Original file line number Diff line number Diff line change
Expand Up @@ -594,28 +594,9 @@ func SetConditionAndUpdate(ctx context.Context,
Reason: reason,
Message: api.TruncateConditionMessage(message),
}

var (
wlCopy *kueue.Workload
err error
)

if features.Enabled(features.WorkloadRequestUseMergePatch) {
wlCopy = wl.DeepCopy()
err = clientutil.PatchStatus(ctx, c, wlCopy, func() (bool, error) {
return apimeta.SetStatusCondition(&wlCopy.Status.Conditions, condition), nil
})
} else {
wlCopy = BaseSSAWorkload(wl, true)
wlCopy.Status.Conditions = []metav1.Condition{condition}
err = c.Status().Patch(ctx, wlCopy, client.Apply, client.FieldOwner(managerPrefix+"-"+condition.Type))
}
if err != nil {
return err
}

wlCopy.DeepCopyInto(wl)
return nil
return PatchStatus(ctx, c, wl, client.FieldOwner(managerPrefix+"-"+condition.Type), func(wl *kueue.Workload) (bool, error) {
return apimeta.SetStatusCondition(&wl.Status.Conditions, condition), nil
})
}

// UnsetQuotaReservationWithCondition sets the QuotaReserved condition to false, clears
Expand Down Expand Up @@ -965,12 +946,14 @@ func PrepareWorkloadPatch(w *kueue.Workload, strict bool, clk clock.Clock) *kueu
return wlCopy
}

// PatchAdmissionStatusOption defines a functional option for customizing PatchAdmissionStatusOptions.
type UpdateFunc func(*kueue.Workload) (bool, error)

// PatchStatusOption defines a functional option for customizing PatchStatusOptions.
// It follows the functional options pattern, allowing callers to configure
// patch behavior at call sites without directly manipulating PatchAdmissionStatusOptions.
type PatchAdmissionStatusOption func(*PatchAdmissionStatusOptions)
// patch behavior at call sites without directly manipulating PatchStatusOptions.
type PatchStatusOption func(*PatchStatusOptions)

// PatchAdmissionStatusOptions contains configuration parameters that control how patches
// PatchStatusOptions contains configuration parameters that control how patches
// are generated and applied.
//
// Fields:
Expand All @@ -983,28 +966,28 @@ type PatchAdmissionStatusOption func(*PatchAdmissionStatusOptions)
// patch. Defaults to true. Setting StrictPatch=false preserves the current
// ResourceVersion.
//
// Typically, PatchAdmissionStatusOptions are constructed via DefaultPatchAdmissionStatusOptions and
// modified using PatchAdmissionStatusOption functions (e.g., WithLoose).
type PatchAdmissionStatusOptions struct {
// Typically, PatchStatusOptions are constructed via DefaultPatchStatusOptions and
// modified using PatchStatusOption functions (e.g., WithLoose).
type PatchStatusOptions struct {
StrictPatch bool
StrictApply bool
RetryOnConflictForPatch bool
}

// DefaultPatchAdmissionStatusOptions returns a new PatchAdmissionStatusOptions instance configured with
// DefaultPatchStatusOptions returns a new PatchStatusOptions instance configured with
// default settings.
//
// By default, StrictPatch and StrictApply is set to true, meaning ResourceVersion is cleared
// from the original object so it will always be included in the generated
// patch. This ensures stricter version handling during patch application.
func DefaultPatchAdmissionStatusOptions() *PatchAdmissionStatusOptions {
return &PatchAdmissionStatusOptions{
func DefaultPatchStatusOptions() *PatchStatusOptions {
return &PatchStatusOptions{
StrictPatch: true, // default is strict
StrictApply: true, // default is strict
}
}

// WithLooseOnApply returns a PatchAdmissionStatusOption that resets the StrictApply field on PatchAdmissionStatusOptions.
// WithLooseOnApply returns a PatchStatusOption that resets the StrictApply field on PatchStatusOptions.
//
// When using Patch Apply, setting StrictApply to false enforces looser
// version handling only for Patch Apply.
Expand All @@ -1016,52 +999,83 @@ func DefaultPatchAdmissionStatusOptions() *PatchAdmissionStatusOptions {
// return updateFn(obj), nil
// }, WithLooseOnApply()) // disables strict mode for Patch Apply

func WithLooseOnApply() PatchAdmissionStatusOption {
return func(o *PatchAdmissionStatusOptions) {
func WithLooseOnApply() PatchStatusOption {
return func(o *PatchStatusOptions) {
o.StrictApply = false
}
}

// WithRetryOnConflictForPatch configures PatchAdmissionStatusOptions to enable retry logic on conflicts.
// WithRetryOnConflictForPatch configures PatchStatusOptions to enable retry logic on conflicts.
// Note: This only works with merge patches.
func WithRetryOnConflictForPatch() PatchAdmissionStatusOption {
return func(o *PatchAdmissionStatusOptions) {
func WithRetryOnConflictForPatch() PatchStatusOption {
return func(o *PatchStatusOptions) {
o.RetryOnConflictForPatch = true
}
}

// PatchAdmissionStatus updates the admission status of a workload.
// If the WorkloadRequestUseMergePatch feature is enabled, it uses a Merge Patch with update function.
// Otherwise, it runs the update function and, if updated, applies the SSA Patch status.
func PatchAdmissionStatus(ctx context.Context, c client.Client, w *kueue.Workload, clk clock.Clock, update func(*kueue.Workload) (bool, error), options ...PatchAdmissionStatusOption) error {
opts := DefaultPatchAdmissionStatusOptions()
func patchStatusOptions(options []PatchStatusOption) *PatchStatusOptions {
opts := DefaultPatchStatusOptions()
for _, opt := range options {
opt(opts)
}
var err error
wlCopy := w.DeepCopy()
return opts
}

// patchStatus updates the status of a workload.
// If the WorkloadRequestUseMergePatch feature is enabled, it uses a Merge Patch with update function.
// Otherwise, it runs the update function and, if updated, applies the SSA Patch status.
func patchStatus(ctx context.Context, c client.Client, wl *kueue.Workload, owner client.FieldOwner, update UpdateFunc, opts *PatchStatusOptions) error {
wlCopy := wl.DeepCopy()
if features.Enabled(features.WorkloadRequestUseMergePatch) {
var patchOptions []clientutil.PatchOption
patchOptions := make([]clientutil.PatchOption, 0, 2)
if !opts.StrictPatch {
patchOptions = append(patchOptions, clientutil.WithLoose())
}
if opts.RetryOnConflictForPatch {
patchOptions = append(patchOptions, clientutil.WithRetryOnConflict())
}
err = clientutil.PatchStatus(ctx, c, wlCopy, func() (bool, error) {
err := clientutil.PatchStatus(ctx, c, wlCopy, func() (bool, error) {
return update(wlCopy)
}, patchOptions...)
if err != nil {
return err
}
} else {
if updated, err := update(wlCopy); err != nil || !updated {
return err
}
wlCopy = PrepareWorkloadPatch(wlCopy, opts.StrictApply, clk)
err = c.Status().Patch(ctx, wlCopy, client.Apply, client.FieldOwner(constants.AdmissionName), client.ForceOwnership)
}
if err == nil {
wlCopy.DeepCopyInto(w)
err := c.Status().Patch(ctx, wlCopy, client.Apply, owner, client.ForceOwnership)
if err != nil {
return err
}
}
return err
wlCopy.DeepCopyInto(wl)
return nil
}

func PatchStatus(ctx context.Context, c client.Client, wl *kueue.Workload, owner client.FieldOwner, update UpdateFunc, options ...PatchStatusOption) error {
opts := patchStatusOptions(options)
return patchStatus(ctx, c, wl, owner, func(wl *kueue.Workload) (bool, error) {
if !features.Enabled(features.WorkloadRequestUseMergePatch) {
wlPatch := BaseSSAWorkload(wl, opts.StrictApply)
wlPatch.DeepCopyInto(wl)
}
return update(wl)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@mbobrovskyi mbobrovskyi Dec 5, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately no.

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.

Copy link
Contributor Author

@mbobrovskyi mbobrovskyi Dec 5, 2025

Choose a reason for hiding this comment

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

So after patch creation we will loose all changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}, opts)
}

func PatchAdmissionStatus(ctx context.Context, c client.Client, wl *kueue.Workload, clk clock.Clock, update UpdateFunc, options ...PatchStatusOption) error {
opts := patchStatusOptions(options)
return patchStatus(ctx, c, wl, constants.AdmissionName, func(wl *kueue.Workload) (bool, error) {
if updated, err := update(wl); err != nil || !updated {
return updated, err
}
if !features.Enabled(features.WorkloadRequestUseMergePatch) {
Comment on lines +1070 to +1073
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

wlPatch := PrepareWorkloadPatch(wl, opts.StrictApply, clk)
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 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

wlPatch.DeepCopyInto(wl)
}
return true, nil
}, opts)
}

type Ordering struct {
Expand Down
Loading