dongjoon-hyun commented on PR #58187: URL: https://github.com/apache/spark/pull/58187#issuecomment-5884220699
To summarize my review comments (in order of importance): **1. Regression for successful submissions** - [The owner reference refresh now requires `delete` permission](https://github.com/apache/spark/pull/58187#discussion_r4129910475): Since the driver conf ConfigMap is created before the pod without an owner reference, the refresh at L242 is an UPDATE. Under `OwnerReferencesPermissionEnforcement` (enabled by default on OpenShift), it requires `delete` permission on ConfigMaps/Secrets. So, submitters without it (e.g. the default RBAC of kubeflow/spark-operator v2.5.2) fail on every submission and leak a ConfigMap per attempt. **2. Design** - [Trade-off of the ownerless window](https://github.com/apache/spark/pull/58187#discussion_r4129910563): This trades a transient, self-healing `FailedMount` for a new window where the credential Secrets and the conf ConfigMap have no owner reference. `spec.schedulingGates` would avoid the window, the shutdown hook, and the RBAC issue above. - [Scope of the shutdown hook](https://github.com/apache/spark/pull/58187#discussion_r4129910571): The hook is about four times larger than the fix but covers only a part of the window. Accepting the window (with the trade-off documented) or a separate JIRA issue might be better. The PR description also needs to be updated. **3. Failure handling (if the shutdown hook is kept)** - [A cleanup failure masks the original error](https://github.com/apache/spark/pull/58187#discussion_r4129910488) (L224, L234, L246) - [`finally` removes the hook even when the cleanup didn't happen](https://github.com/apache/spark/pull/58187#discussion_r4129910495) - [Signals during the in-flight apply/create are not handled](https://github.com/apache/spark/pull/58187#discussion_r4129910500) - [No deadline in the hook, so Ctrl-C can hang up to 30s](https://github.com/apache/spark/pull/58187#discussion_r4129910507) - [The list deletion stops at the first failure](https://github.com/apache/spark/pull/58187#discussion_r4129910518) - [The per-JVM driver ConfigMap name can delete another app's ConfigMap](https://github.com/apache/spark/pull/58187#discussion_r4129910528) **4. Impact on `apache/spark-kubernetes-operator`** - [Ownerless pre-resources can leak on abrupt operator stops](https://github.com/apache/spark/pull/58187#discussion_r4129910536) - [Re-applying the immutable delegation token Secret can fail with 422](https://github.com/apache/spark/pull/58187#discussion_r4129910544) (rare) **5. Tests and cleanups** - [Tests don't verify the hook wiring, the ordering, or the failure-path deletion](https://github.com/apache/spark/pull/58187#discussion_r4129910551) - [`KubernetesDriverBuilderSuite` now depends on `HADOOP_CONF_DIR`](https://github.com/apache/spark/pull/58187#discussion_r4129910558) - [Duplicated client creation and test-only seams](https://github.com/apache/spark/pull/58187#discussion_r4129910578) - [Too verbose and partially inaccurate comments](https://github.com/apache/spark/pull/58187#discussion_r4129910586) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
