[jira] [Updated] (SPARK-59578) Clarify the Kueue `integrations.externalFrameworks` recommendation in `docs/operations.md`

["Peter Toth (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/SPARK-59578?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Peter Toth updated SPARK-59578:
-------------------------------
    Description: 
docs/operations.md tells users to register SparkApplication.v1.spark.apache.org and SparkCluster.v1.spark.apache.org in Kueue's integrations.externalFrameworks whenever operatorRbac.kueue.enabled is set. That registration has no effect for this operator, so the instruction asks users to configure something that does nothing.

Traced against Kueue v0.19.4:

- integrations.externalFrameworks only populates a lookup table. registerExternal (pkg/controller/jobframework/integrationmanager.go:149-169) stores a PartialObjectMetadata for the kind in m.externalIntegrations, and setupControllers (pkg/controller/jobframework/setup.go:66-70) calls RegisterExternalJobType for external frameworks and nothing else - no controller and no webhook is built for them.
- The only owner-related consumer of that table is getJobTypeForOwner (integrationmanager.go:230-244).
- Its caller IsOwnerManagedByKueueForObject (integrationmanager.go:358-363) starts with metav1.GetControllerOf(obj), which returns the owner reference whose controller field is true.
- The operator does not set a controller owner reference on the pods it creates. ModelUtils.buildOwnerReferenceTo (spark-operator-api) sets blockOwnerDeletion only. The single place the operator sets controller: true is KueueWorkloadFactory, on the Workload itself.

So GetControllerOf returns nil for a driver or executor pod and the external-framework registry is never consulted. The operator also does not need Kueue to manage Workloads on its behalf: it builds them with KueueWorkloadFactory and deletes them in AppCleanUpStep.

Proposed change: replace the registration instruction in the Kueue entry of docs/operations.md with a sentence saying it is not needed, and why. No CI or chart change.

Correction to the original description of this issue: it argued that the registration still mattered because defaultLocalQueueApplies (pkg/controller/jobframework/defaults.go:80-88) has no feature gate, so in a namespace holding a LocalQueue named "default" Kueue would stamp queue-name: default onto the operator's driver pod. That gate is indeed ungated, but the branch is unreachable here for the same reason as above: defaultLocalQueueApplies returns !IsOwnerManagedByKueueForObject(jobObj), which is false only when GetControllerOf finds a controller reference. With no controller reference on the pod it evaluates the same way whether or not the frameworks are registered, so registering them does not prevent that defaulting.

A separate question, not in scope here: whether the operator should set controller: true on the owner references of the pods and StatefulSets it creates. That is what would let Kueue recognise them as owned, and it has garbage-collection and adoption implications beyond this documentation fix.

Found while reviewing https://github.com/apache/spark-kubernetes-operator/pull/827


  was:
docs/operations.md:40-45 lists Kueue's integrations.externalFrameworks registration of SparkApplication.v1.spark.apache.org and SparkCluster.v1.spark.apache.org as something "required" whenever operatorRbac.kueue.enabled is set. That overstates it, and CI does not do it: the kueue jobs install the stock manifests.yaml, which registers neither.

Analysis by Dongjoon Hyun on https://github.com/apache/spark-kubernetes-operator/pull/829#discussion_r4027226533, checked against Kueue v0.19.4:

- integrations.externalFrameworks is only read on the job framework paths - pkg/controller/jobframework/reconciler.go:919, pkg/controller/jobframework/defaults.go:88 and :107, and pkg/controller/jobs/pod/pod_webhook.go:269, where it only builds a warning string.
- The Workload controller, the scheduler and the Workload webhook never look at the owner kind. The only owner check in pkg/controller/core/workload_controller.go is isOrphanedWorkload, which fires only on empty ownerReferences. The operator always sets a controller reference, so a Workload it creates is admitted without the registration.

So the registration is not a hard prerequisite of operatorRbac.kueue.enabled. It does matter in one concrete case, and that is what the doc should say instead:

- defaultLocalQueueApplies (pkg/controller/jobframework/defaults.go:80-88) has no feature gate, unlike ApplyDefaultWorkloadPriorityClass immediately below it. It returns true when the namespace has a LocalQueue named "default", the object carries no queue-name, and the owner is not a kind Kueue manages.
- So in a namespace with a default LocalQueue, an unregistered SparkApplication means Kueue adds queue-name: default to the driver pod the operator creates, queueing that pod on its own.

{code}
func defaultLocalQueueApplies(jobObj client.Object, defaultQueueExist func(string) bool) bool {
	if !defaultQueueExist(jobObj.GetNamespace()) {
		return false
	}
	if QueueNameForObject(jobObj) != "" {
		return false
	}
	// Do not default the queue-name for a job whose owner is already managed by Kueue
	return !IsOwnerManagedByKueueForObject(jobObj)
}
{code}

Proposed change: reword docs/operations.md:40-45 to present the registration as recommended rather than required, and to name the default-LocalQueue pod-queueing case as the reason. No CI change - PR #829 leaves the stock install in place deliberately.



> Clarify the Kueue `integrations.externalFrameworks` recommendation in `docs/operations.md`
> ------------------------------------------------------------------------------------------
>
>                 Key: SPARK-59578
>                 URL: https://issues.apache.org/jira/browse/SPARK-59578
>             Project: Spark
>          Issue Type: Sub-task
>          Components: Kubernetes
>    Affects Versions: kubernetes-operator-1.1.0
>            Reporter: Peter Toth
>            Priority: Major
>
> docs/operations.md tells users to register SparkApplication.v1.spark.apache.org and SparkCluster.v1.spark.apache.org in Kueue's integrations.externalFrameworks whenever operatorRbac.kueue.enabled is set. That registration has no effect for this operator, so the instruction asks users to configure something that does nothing.
> Traced against Kueue v0.19.4:
> - integrations.externalFrameworks only populates a lookup table. registerExternal (pkg/controller/jobframework/integrationmanager.go:149-169) stores a PartialObjectMetadata for the kind in m.externalIntegrations, and setupControllers (pkg/controller/jobframework/setup.go:66-70) calls RegisterExternalJobType for external frameworks and nothing else - no controller and no webhook is built for them.
> - The only owner-related consumer of that table is getJobTypeForOwner (integrationmanager.go:230-244).
> - Its caller IsOwnerManagedByKueueForObject (integrationmanager.go:358-363) starts with metav1.GetControllerOf(obj), which returns the owner reference whose controller field is true.
> - The operator does not set a controller owner reference on the pods it creates. ModelUtils.buildOwnerReferenceTo (spark-operator-api) sets blockOwnerDeletion only. The single place the operator sets controller: true is KueueWorkloadFactory, on the Workload itself.
> So GetControllerOf returns nil for a driver or executor pod and the external-framework registry is never consulted. The operator also does not need Kueue to manage Workloads on its behalf: it builds them with KueueWorkloadFactory and deletes them in AppCleanUpStep.
> Proposed change: replace the registration instruction in the Kueue entry of docs/operations.md with a sentence saying it is not needed, and why. No CI or chart change.
> Correction to the original description of this issue: it argued that the registration still mattered because defaultLocalQueueApplies (pkg/controller/jobframework/defaults.go:80-88) has no feature gate, so in a namespace holding a LocalQueue named "default" Kueue would stamp queue-name: default onto the operator's driver pod. That gate is indeed ungated, but the branch is unreachable here for the same reason as above: defaultLocalQueueApplies returns !IsOwnerManagedByKueueForObject(jobObj), which is false only when GetControllerOf finds a controller reference. With no controller reference on the pod it evaluates the same way whether or not the frameworks are registered, so registering them does not prevent that defaulting.
> A separate question, not in scope here: whether the operator should set controller: true on the owner references of the pods and StatefulSets it creates. That is what would let Kueue recognise them as owned, and it has garbage-collection and adoption implications beyond this documentation fix.
> Found while reviewing https://github.com/apache/spark-kubernetes-operator/pull/827



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@spark.apache.org
For additional commands, e-mail: issues-help@spark.apache.org