feat: auto handle pod template with configmap#141
feat: auto handle pod template with configmap#141parisni wants to merge 11 commits intohussein-awala:mainfrom
Conversation
hussein-awala
left a comment
hussein-awala
left a comment
There was a problem hiding this comment.
Thanks @parisni for the PR, glad to see you contributing to the project.
I added some comments, I'll merge the PR once they're addressed, and sorry for the delay in the first review.
spark_on_k8s/cli/options.py
Outdated
| ("--executor-template", "executor_template"), | ||
| type=str, | ||
| default=Configuration.SPARK_ON_K8S_EXECUTOR_TEMPLATE, | ||
| show_default=True, |
There was a problem hiding this comment.
Showing the default could be a bit annoying if it's too big, but let's show it and check if we have any negative feedback
spark_on_k8s/client.py
Outdated
| # Set Spark configuration to use the mounted template | ||
| basic_conf["spark.kubernetes.executor.podTemplateFile"] = "/opt/spark/executor-template/executor-template.yaml" |
There was a problem hiding this comment.
It would be better to override executor_pod_template_path and replace the elif above with an if, and also change the if configuration of this block to fail if both executor_pod_template_path and executor_template are provided
There was a problem hiding this comment.
i did take most of what you ask, however i kept both variables independent (no override) to avoid unexpected side effect in the futur
spark_on_k8s/client.py
Outdated
| # If this is the executor template ConfigMap, only add it to executor volume mounts | ||
| # (driver mount was already added earlier when processing executor_template) | ||
| if executor_template_configmap and configmap.metadata.name == f"{app_id}-executor-template": | ||
| executor_volume_mounts.append( | ||
| k8s.V1VolumeMount( | ||
| name=configmap.metadata.name, | ||
| mount_path=configmap_mount_path, | ||
| ) | ||
| ) | ||
| else: | ||
| # Mount to driver for other ConfigMaps (like driver_ephemeral_configmaps_volumes) | ||
| driver_volume_mounts.append( | ||
| k8s.V1VolumeMount( | ||
| name=configmap.metadata.name, | ||
| mount_path=configmap_mount_path, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
the executor does not need to know what its pod template is, only the driver needs it to create the executor pods, you can revert the chanes in this block
spark_on_k8s/client.py
Outdated
| executor_labels: dict[str, str] | ArgNotSet = NOTSET, | ||
| driver_tolerations: list[k8s.V1Toleration] | ArgNotSet = NOTSET, | ||
| executor_pod_template_path: str | ArgNotSet = NOTSET, | ||
| executor_template: str | ArgNotSet = NOTSET, |
There was a problem hiding this comment.
I believe executor_pod_template is a better name, as we use executor_pod_template_path for the alternative option
spark_on_k8s/client.py
Outdated
| if (executor_template.startswith(('.', '/', '~')) or | ||
| executor_template.endswith(('.yaml', '.yml')) or | ||
| os.path.exists(executor_template)): |
There was a problem hiding this comment.
if os.path.exists(executor_template) is false, then with open(os.path.expanduser(executor_template), 'r') as f: would fail, so the logic needs to be udpated
There was a problem hiding this comment.
done: improve the logic, and add few tests
|
thanks for the review, i tried to address your feedback |
2 participants
This aims at simplifying the executor template management by letting the user to just pass the yaml content for the executor template and let the lib manage a configmap and manage it's lifecycle
The pr :
executor_pod_template_pathLet me know