Register executors using pod IPs instead of pod host names #215
Conversation
There was a problem hiding this comment.
This looks great @kimoonkim ! I think IPs will be more reliable here anyway.
I think this fixes a broader issue we have where shuffle with >1 pod was previously broken due to us incorrectly relying on DNS resolution of pod hostnames, which isn't guaranteed by kube-dns.
@mccheah can you look specifically at the blockmanager change? I'm not sure how important that is but using the same default port on both driver and executor seems sane.
| @@ -182,6 +183,12 @@ private[spark] class KubernetesClusterSchedulerBackend( | |||
| .withName(env._1) | |||
| .withValue(env._2) | |||
| .build()) | |||
| requiredEnv = requiredEnv ++ Seq(new EnvVarBuilder() | |||
There was a problem hiding this comment.
can you keep requiredEnv a val by directly concatenating that value with the new item you're appending? Better to keep the final reference if it's that small of a change
There was a problem hiding this comment.
Thanks for the suggestion. Done in the new diff.
|
+1 after @ash211 comment. We should cherry-pick this to the branch-2.1-kubernetes-release branch as well and tag that. |
|
There is also the downward API for exposing PodIP to the container. |
|
Oh never mind. I just noticed that it's the downward API being used here. LGTM. |
|
Thanks for the reviews. Addressed @ash211 's comment. Maybe ready after Jenkins bless. |
|
rerun unit tests please |
|
Merging now. @kimoonkim can you cherrypick this change onto the release branch? |
* Register executors using pod IPs * Fix block manager port typo * Fix import * Keep requiredEnv to be a val * Clean up indentation
* Register executors using pod IPs * Fix block manager port typo * Fix import * Keep requiredEnv to be a val * Clean up indentation
…ark-on-k8s#215) * Register executors using pod IPs * Fix block manager port typo * Fix import * Keep requiredEnv to be a val * Clean up indentation
What changes were proposed in this pull request?
cc @foxish @ash211
Fixes #214 by registering executors with pod IPs instead of pod host names that kube-dns does not support. Also fixes a typo in block manager port key.
How was this patch tested?
Ran integration test successfully.
Tested with my k8s cluster.