fix: use sparkImage.pullPolicy in all containers - #764
Conversation
a7f8906 to
fd3967e
Compare
adwk67
moved this from Development: Waiting for Review
to Development: In Review
in Stackable Engineering
e7a1601 to
fb03739
Compare
| <2> Apache Spark version bundled in your custom image. | ||
|
|
||
| `sparkImage.pullPolicy` governs every container that the operator selects an image for: the submit, driver and executor containers, the `job`, `requirements` and `tls` init containers, and the Spark Connect server and its executors. | ||
| This includes the user-supplied `spec.image`, which the `job` init container runs. |
There was a problem hiding this comment.
Is the history server excluded? If so, maybe make that clear here. Also Connect Server takes spec.image and not sparkImage (maybe that is implied but we could make it clearer).
|
|
||
| #[test] | ||
| fn image_pull_policy_is_set_on_every_container_spark_does_not_rebuild() { | ||
| let validated = validated_cluster_with_s3_tls(); |
There was a problem hiding this comment.
Nit: if we passed PULL_POLICY_NEVER to this function it would make it a bit clearer as to what the test is expecting.
| Some(spark_image), | ||
| ), | ||
| ( | ||
| "spark.kubernetes.container.image.pullPolicy".to_string(), |
There was a problem hiding this comment.
The executor properties are merged last, so a user override for this property will be silently overridden (the other properties added in this function are "internal"). This should go before user overrides are applied e.g. in server_properties?
| .iter() | ||
| .flatten() | ||
| .chain(pod_spec.containers.iter()) | ||
| .filter(|container| container.name != SparkConnectContainer::Spark.to_string()) |
There was a problem hiding this comment.
Can we also assert the expected value for the excluded container, so that it checks/guards that the template container does not carry its own pull policy? (and so we can't change that silently in the future)
| } | ||
|
|
||
| #[cfg(test)] | ||
| pub(crate) fn tls_connection() -> Self { |
There was a problem hiding this comment.
This seems to duplicate the ConnectionSpec part of connection_fixture: maybe consolidate them (e.g. move connection_fixture to here and have tls_connection call it)?
| <2> Apache Spark version bundled in your custom image. | ||
|
|
||
| `sparkImage.pullPolicy` governs every container that the operator selects an image for: the submit, driver and executor containers, the `job`, `requirements` and `tls` init containers, and the Spark Connect server and its executors. | ||
| This includes the user-supplied `spec.image`, which the `job` init container runs. |
There was a problem hiding this comment.
Maybe we should mention the corner case where sparkImage.pullPolicy is set to Never (e.g. the user knows it is pre-loaded) - in which case spec.image also has to be pre-loaded (as it uses the same pull policy). It is implied but might be good to make that clear.
Co-authored-by: Andrew Kenworthy <1712947+adwk67@users.noreply.github.com>
2 participants
Description
Solves #762
sparkImage.pullPolicyis no longer ignored by the driver and executor containers, the Spark Connect executors and the truststore andjobinit containers, and it now covers the user-suppliedspec.imagetoo.This has a breaking component, because this now leads to the default pull policy of
Alwaysfor driver and executor.Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker