Skip to content

[SPARK-22807] [Scheduler] Remove config that says docker and replace with container #19995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[SPARK-22807] [Scheduler] Remove config that says docker and replace with container #19995

wants to merge 2 commits into from

Conversation

foxish
Copy link
Contributor

@foxish foxish commented Dec 15, 2017 •
edited
Loading

What changes were proposed in this pull request?

Changes discussed in #19946 (comment)
docker -> container, since with CRI, we are not limited to running only docker images.

How was this patch tested?

Manual testing

@foxish foxish mentioned this pull request Dec 15, 2017
Closed
4 tasks
@foxish
Copy link
Contributor Author

foxish commented Dec 15, 2017

cc/ @vanzin @ueshin @jiangxb1987 @erikerlandson @liyinan926

@liyinan926
Copy link
Contributor

LGTM.

@erikerlandson
Copy link
Contributor

I'm picking up a few stragglers from git grep

resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/steps/BaseDriverConfigurationStep.scala
33=private[spark] class BaseDriverConfigurationStep(
49:  private val driverDockerImage = submissionSparkConf
51:    .getOrElse(throw new SparkException("Must specify the driver Docker image"))
113:      .withImage(driverDockerImage)

core/src/test/scala/org/apache/spark/deploy/SparkSubmitSuite.scala
95=class SparkSubmitSuite
399:      "--conf", "spark.kubernetes.driver.docker.image=bar",
415:    conf.get("spark.kubernetes.driver.docker.image") should be ("bar")

@foxish
Copy link
Contributor Author

foxish commented Dec 15, 2017

Done, did another pass over them. Thanks @erikerlandson

@erikerlandson
Copy link
Contributor

LGTM

@vanzin
Copy link
Contributor

vanzin commented Dec 15, 2017

add to whitelist

@vanzin
Copy link
Contributor

vanzin commented Dec 15, 2017

Please cleanup the PR description.

@vanzin
Copy link
Contributor

vanzin commented Dec 15, 2017

ok to test

@vanzin
Copy link
Contributor

vanzin commented Dec 16, 2017

LGTM pending tests.

1 similar comment
@ueshin
Copy link
Member

ueshin commented Dec 16, 2017

LGTM pending tests.

@SparkQA
Copy link

SparkQA commented Dec 16, 2017

Test build #84979 has finished for PR 19995 at commit b3e1af3.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@foxish
Copy link
Contributor Author

foxish commented Dec 16, 2017

@ueshin @vanzin SparkR failure seems unrelated to me. Any ideas?

@jiangxb1987
Copy link
Contributor

retest this please

jiangxb1987
jiangxb1987 approved these changes Dec 16, 2017
View reviewed changes
Copy link
Contributor

@jiangxb1987 jiangxb1987 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jiangxb1987
Copy link
Contributor

The R tests are pretty flaky recently, any ideas @HyukjinKwon ?

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Dec 16, 2017 •
edited
Loading

Seems it's related with e58f275 somehow and then fine back now. I am not yet entirely sure how this change relates to CRAN check. Will try to have some time to take a look soon (I will be super busy for the rest of this month though ..).

Some related discussions - https://github.com/apache/spark/pull/19721, https://github.com/apache/spark/pull/19944, https://github.com/apache/spark/pull/19957 and https://github.com/apache/spark/pull/19961 in an order.

See #20005.

@SparkQA
Copy link

SparkQA commented Dec 16, 2017

Test build #84986 has finished for PR 19995 at commit b3e1af3.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member

retest this please

@SparkQA
Copy link

SparkQA commented Dec 16, 2017

Test build #85004 has finished for PR 19995 at commit b3e1af3.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member

FYI, AppVeyor failure is fixed separately in #20003.

@SparkQA
Copy link

SparkQA commented Dec 17, 2017

Test build #4013 has finished for PR 19995 at commit b3e1af3.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@liyinan926 liyinan926 mentioned this pull request Dec 18, 2017
Merged
@HyukjinKwon
Copy link
Member

retest this please

@HyukjinKwon
Copy link
Member

@foxish rebasing and pushing it will make the AppVeyor test passed for sure.

@foxish foxish force-pushed the make-docker-container branch from b3e1af3 to ace4b19 Compare December 18, 2017 09:31
@foxish
Copy link
Contributor Author

foxish commented Dec 18, 2017

@HyukjinKwon done. Thanks!

@SparkQA
Copy link

SparkQA commented Dec 18, 2017

Test build #85053 has finished for PR 19995 at commit b3e1af3.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Dec 18, 2017

Test build #85055 has finished for PR 19995 at commit ace4b19.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@jiangxb1987
Copy link
Contributor

LGTM

@foxish
Copy link
Contributor Author

foxish commented Dec 18, 2017 •
edited
Loading

@ueshin, can this one and #20007 be merged now?

@vanzin
Copy link
Contributor

vanzin commented Dec 18, 2017

Merging to master.

@asfgit asfgit closed this in fb3636b Dec 18, 2017
@foxish foxish deleted the make-docker-container branch December 18, 2017 19:42
asfgit pushed a commit that referenced this pull request Dec 22, 2017
@foxish @rxin
[SPARK-22648][K8S] Spark on Kubernetes - Documentation
7ab165b 
What changes were proposed in this pull request?

This PR contains documentation on the usage of Kubernetes scheduler in Spark 2.3, and a shell script to make it easier to build docker images required to use the integration. The changes detailed here are covered by #19717 and #19468 which have merged already.

How was this patch tested?
The script has been in use for releases on our fork. Rest is documentation.

cc rxin mateiz (shepherd)
k8s-big-data SIG members & contributors: foxish ash211 mccheah liyinan926 erikerlandson ssuchter varunkatta kimoonkim tnachen ifilonenko
reviewers: vanzin felixcheung jiangxb1987 mridulm

TODO:
- [x] Add dockerfiles directory to built distribution. (#20007)
- [x] Change references to docker to instead say "container" (#19995)
- [x] Update configuration table.
- [x] Modify spark.kubernetes.allocation.batch.delay to take time instead of int (#20032)

Author: foxish <ramanathana@google.com>

Closes #19946 from foxish/update-k8s-docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jiangxb1987 jiangxb1987

@felixcheung felixcheung

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@foxish @liyinan926 @erikerlandson @vanzin @ueshin @SparkQA @jiangxb1987 @HyukjinKwon @felixcheung