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

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?

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?

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:

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.

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

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

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).

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

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?

@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 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.

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.

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.

Yeah, I meant UID label.

@mimowo @tenzen-y I took an initial pass at this in #4339. I haven't contributed to kubebuilder controllers before so let me know if this is the right general approach

Any thoughts on how we want to handle MultiKueue remote workloads? Should the UID label just get dropped when we make a remote copy?

I see, why the question: because the UID from the worker cluster (present in the condition message of the preempted workload) would not be found anyway on the management cluster.

I think ideally:

1. we set the new UID label on the management workload unconditionally
2. when creating workloads on the worker clusters we inprint the information about the UID from the management cluster (for example in the MainUID label)
3. when preemption on the worker cluster happens we put into the message the value of the MainWorkloadUID label which can be used for searching the workload in (1.). Maybe the message could be kueue-admission Preempted to accommodate a workload (UID: <Main UID>, Worker UID: <Worker UID>) due to fair sharing within the cohort. Here Worker UID would be logged only for workloads with "MainUID".

WDYT?

Thinking about it more. To be fair I would just do now 1. unconditionally, and defer the MK 2. and 3. as a future improvement.

It is not that much more effort to add a kueue.x-k8s.io/multikueue-main-workload-uid label when copying over the workload to the remote cluster. How about we take that step in this change and leave the preemption logging change as a future improvement?

Actually, in the name of keeping integration test changes more scoped in each PR, lets defer all the 'main-workload-uid' labeling on the remote objects to a follow-up change

The naive approach of adding an update call in Workload Reconcile loop is negatively impacting the scheduability perf tests: #4339 (comment). It seems like increasing the Kueue qps and burst parameters could mitigate this.

Taking a step back however, is there a good reason in the kueue-admission Preempted to accommodate a workload (UID: <UID>) due to fair sharing within the cohort condition we only include the UID? We have access to the full Workload when we construct the condition: https://github.com/kubernetes-sigs/kueue/blob/main/pkg/scheduler/preemption/preemption.go#L220. Why don't we just additionally log the preempting workload's namespace and name? Then users could very easily kubectl get the preempting workload without using label selector

Why don't we just additionally log the preempting workload's namespace and name?

Logging full name and namespace is ok. We actually have a PR which does it in 0.11: 7dcff89. I think we could justify cherry-picking it to 0.10.

However, when creating the message for the condition we only include UID to avoid leaking of potentially namespace-sensitive information which could be present just in the workload name. The analogous approach is taken for preemption in the core k8s (for events and condition): https://github.com/kubernetes/kubernetes/blob/0e2a2afc4c7687e71f2dddbb7edfd03f082c6876/pkg/scheduler/framework/preemption/preemption.go#L198

Ah okay. Any ideas on an existing controller Workload update or patch call that we could use to also update the labels? I've run the scheduling perf tests a bunch and adding another update call is adding too much overhead to the workload controller loop

Maybe I could update the defaulting workload webhook to also run on updates and do the label defaulting there? It is not possible on creates because the workload object does not have UID yet

adding another update call is adding too much overhead to the workload controller loop

How much of a hit it is? Maybe we could consider just swallowing the pill and bump the thresholds.

Maybe I could update the defaulting workload webhook to also run on updates and do the label defaulting there?

This actually sounds like it could add even more overhead (but maybe it would not be visible in the perf tests, cannot recall if they install webhooks).

@mimowo @avrittrohwer We should not record preemptor name and namespace in events and logging since those are PII.
So, I think we should revert 7dcff89 and keep UID in logging in this enhancement.

You can find a similar discussion: #1942 (comment)

It is not possible on creates because the workload object does not have UID yet

I know, I was hoping this will change with MAP, as then it all happens within API server, but it seems it will not be visible in the CEL rules (though I haven't seen explicit mention in the KEP).

So, I think we should revert 7dcff89 and keep UID in logging in this enhancement.

I think no reason to revert, this PR does exactly that - only adds to logging, but does not modify the message used on the condition. Also, we have integration tests showing the message format:

So, I think we should revert 7dcff89 and keep UID in logging in this enhancement.

I think no reason to revert, this PR does exactly that - only adds to logging, but does not modify the message used on the condition. Also, we have integration tests showing the message format:

kueue/test/integration/singlecluster/scheduler/preemption_test.go

Line 300 in 099a239

Message: fmt.Sprintf("Previously: Preempted to accommodate a workload (UID: %s) due to %s", alphaMidWl.UID, preemption.HumanReadablePreemptionReasons[kueue.InClusterQueueReason]),

Ah, I see. It seems to focus only on logging. In that case, it should be fine. There are no leaks between tenants.

Instead of kueue.x-k8s.io/multikueue-main-workload-uid label, why we can not add management or workload cluster UID as a field?
I meant wondering if we can add dedicated fields something like managemendUID to recorded management cluster workload UID.
IIUC, adding UID labeling has the motivation to easy list workloads, right? In that case, we can add it to dedicated fields.

Taking a step back - another option is that workloads already contain owner Job UID: kueue.x-k8s.io/job-uid. So, maybe in the messages for preemption we also record Job UID. Then, a user could use it for fast fetch. This would be no new requests, and no performance hit.

The only problem is that with Pod Groups. However, maybe this is a corner case. Also, we could for a pod group record UID of the "first" pod. This would already allow users to identify the preempting group/

wdyt @tenzen-y @avrittrohwer about the Job UID approach for the fast search?

wdyt @tenzen-y @avrittrohwer about the Job UID approach for the fast search?

I don't think that would help, because we have the same problem of 'how do I get a job by its UID' which is not supported by kubernetes: kubernetes/kubernetes#20572

We will search workloads by JobUID, as it is already recorded on the workload object:

Lavereging the fact at the moment of wokload creation we already have JobUID, but not workloadUID.
So we only need to let user know the JobUID (along with the Workload UID in preemption.go).

Right, but once the user has JobUID, how do they kubectl get it by UID?

Oh they could do kubectl get workloads --selector=kueue.x-k8s.io/job-uid=<UID>. I was thinking how would they do kubectl get <jobAPI>

Adding jobUID to the condition and event message is a much easier change. Lets do that

Taking a step back - another option is that workloads already contain owner Job UID: kueue.x-k8s.io/job-uid. So, maybe in the messages for preemption we also record Job UID. Then, a user could use it for fast fetch. This would be no new requests, and no performance hit.

The only problem is that with Pod Groups. However, maybe this is a corner case. Also, we could for a pod group record UID of the "first" pod. This would already allow users to identify the preempting group/

That sounds reasonable.