[
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: [email protected]
For additional commands, e-mail: [email protected]
