Repository navigation
Conversation
|
Fixing #39184 is worthwhile — tasks hanging indefinitely because no sidecar is injected to read Two practical notes before I review the substance. This branch is 1047 commits behind More importantly, it overlaps with #68786 from you: both edit My suggestion is to land #68786 first and rebase this one on top. It is the smaller and independently correct change, and this PR's Once the ordering is settled and this is rebased, I will review the SparkKubernetesOperator side properly. Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting |
potiuk
left a comment
There was a problem hiding this comment.
Thanks for adding XCom push to SparkKubernetesOperator; it's a useful feature. One blocking issue inline, plus a few smaller ones:
- Rebase needed. The
add_xcom_sidecarchange (pod_cp.spec.volumes = pod_cp.spec.volumes or []) is already onmainvia #72522, which also deep-copies the default volume and mount. Please drop that hunk when rebasing rather than taking this side, or the shared-default-object bug comes back. - Volume serialization:
PodDefaults.VOLUME.to_dict()produces Python-style keys (empty_dir) plus about 30 null fields, not the API form (emptyDir). The CRD drops the unknown key and the volume ends up with no source; it only works because Kubernetes defaults such a volume to an empty directory.ApiClient().sanitize_for_serialization(...), which the PR already uses for the mount, gives the right form. - Sidecar resources: without resources configured on the connection, the Spark sidecar has no requests, while the regular pod operator's sidecar defaults to cpu 1m / memory 10Mi. In a namespace with a ResourceQuota on requests and no defaults, the driver pod is rejected.
- Security context: #69613 (merged after this PR was written) added the
xcom_sidecar_container_security_contextconnection setting. The Spark path only passes image and resources, so after rebasing it ignores the security context, and the sidecar is rejected in namespaces enforcing the "restricted" pod security level.
Drafted-by: Claude Code (Opus 5.5); reviewed by @potiuk before posting
| if self.do_xcom_push: | ||
| self.log.debug("Adding xcom sidecar to driver pod spec in task %s", self.task_id) | ||
| driver_template = template_body["spark"]["spec"] | ||
| template_body["spark"]["spec"] = add_sidecar_to_spark_operator_pod_spec( |
There was a problem hiding this comment.
This doesn't survive templates that use the documented top-level kubernetes section, as the spark_job_template in the docs does. CustomObjectLauncher.get_body() then sets spec["volumes"] and the driver's volumeMounts from that section (custom_object_launcher.py:244–255). That replaces the XCom volume and driver mount injected here, but keeps the sidecar, whose mount then points at a volume that no longer exists, so Kubernetes rejects the driver pod. The tests only use templates without a kubernetes section. Could you inject after get_body() has merged the kubernetes section, or merge into its lists, and add a test with a kubernetes section?
Fixes: #39184 where SparkKubernetesOperator tasks hang indefinitely because no sidecar is injected to read /airflow/xcom/return.json.