search

apache-spark-on-k8s / spark

Apache Spark enhanced with native Kubernetes scheduler back-end: NOTE this repository is being ARCHIVED as all new development for the kubernetes scheduler back-end is now on https://github.com/apache/spark/
https://spark.apache.org/
Apache License 2.0
612 stars 118 forks source link

Incorrect GiB -> MB units conversion #467

Closed ash211 closed 7 years ago

ash211 commented 7 years ago

If I set spark.driver.memory=6G then the created pod has these resources set on it:

resources:
  limits:
    memory: 6758M
  requests:
    cpu: "2"
    memory: 6144M

That might look correct at first, but a Java Xmx of 6G is in units GiB (per SO) and 6144M is in units MB (per k8s docs). The conversion doesn't match!

The generated request should be 6144Mi not 6144M

Because 6144M is only 5859.375MiB, this means that my pod's containers are under-sized and frequently OOM with this message: Killed process 29970 (java) total-vm:12593416kB, anon-rss:6582248kB, file-rss:0kB, shmem-rss:0kB

duyanghao commented 7 years ago

@ash211 spark should convert M to Mi when sending request(create pod) to k8s. I have created a PR to correct this problem.

ash211 commented 7 years ago

Hi @duyanghao I'm sorry I didn't comment here earlier -- there's already a PR up that fixes this which is linked here -- see number 470 above in this issue thread. So I think your PR is duplicate of that one :(

I'm going to merge the first and close yours -- sorry about the duplicate work.

duyanghao commented 7 years ago

@ash211 i am afraid #470 is not enough to cover my PR.

ash211 commented 7 years ago

Do you mean the change you made to executorMemoryString ? Now the ENV_EXECUTOR_MEMORY is memory+overhead rather than just memory. However this is the size given to the JVM, and the overhead is outside the JVM. So I don't think we want to add the overhead to the JVM heap otherwise there's no space between JVM heap size and the pod limit

duyanghao commented 7 years ago

@ash211

Do you mean the change you made to executorMemoryString ? Now the ENV_EXECUTOR_MEMORY is memory+overhead rather than just memory. However this is the size given to the JVM, and the overhead is outside the JVM. So I don't think we want to add the overhead to the JVM heap otherwise there's no space between JVM heap size and the pod limit

Right now, the ENV_EXECUTOR_MEMORY is memory but ENV_DRIVER_MEMORY is memory+overhead

If the ENV_EXECUTOR_MEMORY should only be memory, as you said above, the ENV_DRIVER_MEMORY should also only be memory rather than memory+overhead,is that right?

So i suggest that ENV_DRIVER_MEMORY should also be just memory instead of memory+overheads.

I have created a PR for to correct this problem.