Make it easy to describe the preempting workload

[avrittrohwer]

What would you like to be added:

Make it easy to kubectl describe the workload that evicted another workload.

Why is this needed:

Kueue Workloads have events like kueue-admission Preempted to accommodate a workload (UID: <UID>) due to fair sharing within the cohort

As far as I can tell kubectl --selector field only supports filtering on values in the object .metadata.labels. The Workload UID is in the .metadata.UID field which can not be filtered on by kubectl --selector. For clusters with few workloads grepping on kubectl get workloads -A would work but does not scale for clusters with thousands of workloads.

Completion requirements:

Make it easy to kubectl describe the Workload that preempted another workload. Simple solution: add the Workload UID to the Workload labels.

This enhancement requires the following artifacts:

• Design doc
• API change
• [ x] Docs update -> add docs on how to get details of preempting workload.

The artifacts should be linked in subsequent comments.

[kannon92]

Have you looked at kueuectrl? maybe the UX would be better using the CLI?

[mimowo]

I don't remember if kueuectl would allow it, probably not. However, in any case as an external binary the only way to find the preempting workload by UID would be to list all workloads first, which is not scalable indeed.

I like the idea of adding the UID as a label. cc @tenzen-y WDYT?

[tenzen-y]

I don't remember if kueuectl would allow it, probably not. However, in any case as an external binary the only way to find the preempting workload by UID would be to list all workloads first, which is not scalable indeed.

I like the idea of adding the UID as a label. cc @tenzen-y WDYT?

Thank you for creating this issue. IMO, instead of label with UID, I would propose the CRD field-selector: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/4358-custom-resource-field-selectors

Have you checked if we can implement a custom field selector for Workload by the feature?

[mimowo]

Have you checked if we can implement a custom field selector for Workload by the feature?

I have not checked this myself. It might be worth trying as an alternative, but not sure this works with metadata.ownerReferences.uid. Actually, I see we are indexing the field already in Kueue, but this is in-memory only:

kueue/pkg/controller/core/indexer/indexer.go

Line 40 in b9aa1c3

OwnerReferenceUID = "metadata.ownerReferences.uid"

[tenzen-y]

Have you checked if we can implement a custom field selector for Workload by the feature?

I have not checked this myself. It might be worth trying as an alternative, but not sure this works with metadata.ownerReferences.uid. Actually, I see we are indexing the field already in Kueue, but this is in-memory only:

kueue/pkg/controller/core/indexer/indexer.go

Line 40 in b9aa1c3

OwnerReferenceUID = "metadata.ownerReferences.uid"

I see. In that case, can we try confirming if the custom CRD field is selected? This can be confirmed easily, IMO.
After we confirm the CRD field selector is not runnable, we can add an owner to the label.

[mimowo]

sgtm, we can start by checking if the https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/4358-custom-resource-field-selectors#proposal can be used to filter the workloads first

[avrittrohwer]

Looks like controllergen has support for a +kubebuilder:selectablefield marker: kubernetes-sigs/controller-tools#1039

However the Workload UID comes from k8s.io/apimachinery/pkg/apis/meta/v1.ObjectMeta (https://github.com/kubernetes-sigs/kueue/blob/main/apis/kueue/v1beta1/workload_types.go#L616) so I'm not sure how we could add that marker

[mimowo]

Actually, I think in this use-case we need to index metadata.uid which is a scalar, so it's worth checking if +kubebuilder:selectablefield can be used.

Also, I synced with @deads2k and the KEP only supports scalar fields, so it would not work for metadata.ownerReferences which is a list (but IIUC it is not necessary in our case).

[avrittrohwer]

Looked into the selectableFileds some more. JSON paths into metadata are explicitly now allowed, from https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#selectablefield-v1-apiextensions-k8s-io:

Must not point to metdata fields.

Which aligns with the proposal KEP validation rules section: https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/4358-custom-resource-field-selectors/README.md#validation-rules

[tenzen-y]

https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/4358-custom-resource-field-selectors/README.md#validation-rules

I'm currently a slightly confusing since the KEP uses metadata fields as examples: https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/4358-custom-resource-field-selectors/README.md#openapi-discovery

And they mention may not in selectableFields may not refer to metadata fields.
Have you seen any validation errors if you add uid fieldSelector to any objects?

[avrittrohwer]

@tenzen-y yes I tested out in main...avrittrohwer:kueue:log.

Validation error when applying the CRD to a 1.31 kind cluster:

The CustomResourceDefinition "workloads.kueue.x-k8s.io" is invalid: spec.selectableFields[0].jsonPath: Invalid value: ".metadata.UID": is an invalid path: does not refer to a valid field
make: *** [Makefile:235: install] Error 1

I tested with // +kubebuilder:selectablefield:JSONPath=`.spec.queueName` which worked so I think the issue is with the metadata json path

[tenzen-y]

@tenzen-y yes I tested out in main...avrittrohwer:kueue:log.

Validation error when applying the CRD to a 1.31 kind cluster:

The CustomResourceDefinition "workloads.kueue.x-k8s.io" is invalid: spec.selectableFields[0].jsonPath: Invalid value: ".metadata.UID": is an invalid path: does not refer to a valid field
make: *** [Makefile:235: install] Error 1

I tested with // +kubebuilder:selectablefield:JSONPath=.spec.queueName which worked so I think the issue is with the metadata json path

Thank you for confirming that! As I can check impl, it seems to be correct...
https://github.com/kubernetes/kubernetes/blob/15a186a888dc2e908681c876e321468b8d32a37b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go#L830-L832

Lets's add annotation if @mimowo does not have any objections.

[mimowo]

Lets's add annotation if @mimowo does not have any objections.

You mean the "UID" label? Sure we can do it, and mark it as immutable since UIDs are immutable.