Skip to content
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.

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

[SPARK-24547][K8S] Allow for building spark on k8s docker images without cache and don't forget to push spark-py container. #21555

Closed
wants to merge 1 commit into from

Conversation

rayburgemeestre
Copy link

@rayburgemeestre rayburgemeestre commented Jun 13, 2018

What changes were proposed in this pull request?

https://issues.apache.org/jira/browse/SPARK-24547

TL;DR from JIRA issue:

  • First time I generated images for 2.4.0 Docker was using it's cache, so actually when running jobs, old jars where still in the Docker image. This produces errors like this in the executors:

java.io.InvalidClassException: org.apache.spark.storage.BlockManagerId; local class incompatible: stream classdesc serialVersionUID = 6155820641931972169, local class serialVersionUID = -3720498261147521051

  • The second problem was that the spark container is pushed, but the spark-py container wasn't yet. This was just forgotten in the initial PR.

  • A third problem I also ran into because I had an older docker was [K8S] Fix issue in 'docker-image-tool.sh' #21551 so I have not included a fix for that in this ticket.

How was this patch tested?

I've tested it on my own Spark on k8s deployment.

cache, and include spark-py when pushing to registry.
@felixcheung
Copy link
Member

can you edit the title to add [k8s] and fix the text which is truncated?

@felixcheung
Copy link
Member

ok to test

@SparkQA
Copy link

SparkQA commented Jun 16, 2018

Test build #91966 has finished for PR 21555 at commit c17179b.

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

@rayburgemeestre rayburgemeestre changed the title [SPARK-24547] Allow for building spark on k8s docker images without [SPARK-24547][K8S] Allow for building spark on k8s docker images without cache Jun 16, 2018
@rayburgemeestre rayburgemeestre changed the title [SPARK-24547][K8S] Allow for building spark on k8s docker images without cache [SPARK-24547][K8S] Allow for building spark on k8s docker images without cache and don't forget to push spark-py container. Jun 16, 2018
Copy link
Contributor

@ifilonenko ifilonenko left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for this. @mccheah @foxish @erikerlandson for review before merge

@ifilonenko
Copy link
Contributor

retest this please

@SparkQA
Copy link

SparkQA commented Jun 19, 2018

@SparkQA
Copy link

SparkQA commented Jun 19, 2018

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/299/

@ifilonenko
Copy link
Contributor

retest this please

@SparkQA
Copy link

SparkQA commented Jun 19, 2018

@SparkQA
Copy link

SparkQA commented Jun 19, 2018

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/307/

@SparkQA
Copy link

SparkQA commented Jun 19, 2018

Test build #92054 has finished for PR 21555 at commit c17179b.

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

@SparkQA
Copy link

SparkQA commented Jun 19, 2018

Test build #92063 has finished for PR 21555 at commit c17179b.

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

@ifilonenko
Copy link
Contributor

retest this please

@SparkQA
Copy link

SparkQA commented Jun 19, 2018

@SparkQA
Copy link

SparkQA commented Jun 19, 2018

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/330/

@SparkQA
Copy link

SparkQA commented Jun 20, 2018

Test build #92103 has finished for PR 21555 at commit c17179b.

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

@ifilonenko
Copy link
Contributor

Good to merge with tests success. @mccheah and @foxish please merge

@foxish
Copy link
Contributor

foxish commented Jun 20, 2018

Lgtm. Will merge. Thanks

@mccheah
Copy link
Contributor

mccheah commented Jun 20, 2018

This change makes it such that using the tool forces building and pushing both Python and non-Python, but, what if the user wants to only build one to save time? I can imagine that being the case in something like a dev-CI workflow. Ideally the tool would allow one to be selective in managing either image or both.

@ifilonenko
Copy link
Contributor

@mccheah Good note. As this is a blocker for other PRs. It is probably best to refactor the docker-image-tool.sh in a separate PR for that is the not the focus of this PR.

@foxish
Copy link
Contributor

foxish commented Jun 20, 2018

@mccheah, that is a good point but I agree with @ifilonenko that we can do it in a subsequent PR. I'm thinking we merge this as-is and I can try to get a follow-up PR here for dockerfile refactor.

@erikerlandson
Copy link
Contributor

LGMT, I am OK to merge.

Most of the automated image build tooling I've seen is custom, but I agree w/ Matt that being able to selectively build is worth supporting, via a followup PR. It might make it easier to write custom logic using this as an abstraction.

@mccheah
Copy link
Contributor

mccheah commented Jun 21, 2018

Yup feel free to merge and follow up separately.

@foxish
Copy link
Contributor

foxish commented Jun 21, 2018

Will follow-up. Thanks all for the comments! Merging to master.

@asfgit asfgit closed this in 15747cf Jun 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants