Skip to content

Conversation

@vakarisbk
Copy link
Contributor

@vakarisbk vakarisbk commented Oct 13, 2023

What changes were proposed in this pull request?

  1. Create Java17 base images alongside Java11 images starting from spark 3.5.0
  2. Change ubuntu version to 22.04 for scala2.12-java17-*

Why are the changes needed?

Spark supports multiple Java versions, but the images are currently built only with Java 11.

Does this PR introduce any user-facing change?

New images would be available in the repositories.

How was this patch tested?

@vakarisbk vakarisbk changed the title [WIP] Add support for java 17 and explicit Python versions from 3.5.0 [WIP] Add support for java 17 and explicit Python versions from spark 3.5.0 onwards Oct 13, 2023
@vakarisbk vakarisbk marked this pull request as ready for review October 13, 2023 14:17
@vakarisbk vakarisbk marked this pull request as draft October 13, 2023 14:18
@vakarisbk vakarisbk changed the title [WIP] Add support for java 17 and explicit Python versions from spark 3.5.0 onwards Add support for java 17 from spark 3.5.0 onwards Oct 15, 2023
@vakarisbk vakarisbk changed the title Add support for java 17 from spark 3.5.0 onwards Add support for java 17 from spark 3.5.0 Oct 15, 2023
@vakarisbk vakarisbk marked this pull request as ready for review October 15, 2023 11:05
@vakarisbk
Copy link
Contributor Author

It seems to me that the builds are failing due to insufficient storage on the runners.

[info]   java.lang.AssertionError: assertion failed: Failed to execute -- bash -c MINIKUBE_IN_STYLE=true minikube status  --
[info] minikube
[info] type: Control Plane
[info] host: InsufficientStorage
[info] kubelet: Running
[info] apiserver: Running
[info] kubeconfig: Configured
[info] docker-env: in-use

Maybe it's possible to try switching to larger runners?

@Yikun
Copy link
Member

Yikun commented Oct 17, 2023

I never hit this before, but if it is storage limit, you could try to remove some tmp file to save space, such as:

  1. https://github.com/apache/spark-docker/blob/master/.github/workflows/main.yml#L249
sudo install minikube-linux-amd64 /usr/local/bin/minikube
rm minikube-linux-amd64

It's about to save 80MB

(later upadte) I also noticed this change also apply on main repo: https://github.com/apache/spark/blob/master/.github/workflows/build_and_test.yml#L1045

  1. (if step 1 is ok, we don't need this step)

remove fetch-depth: 0, seems also save some space?

@vakarisbk
Copy link
Contributor Author

vakarisbk commented Oct 17, 2023

I never hit this before, but if it is storage limit, you could try to remove some tmp file to save space, such as:

  1. https://github.com/apache/spark-docker/blob/master/.github/workflows/main.yml#L249
sudo install minikube-linux-amd64 /usr/local/bin/minikube
rm minikube-linux-amd64

It's about to save 80MB

(later upadte) I also noticed this change also apply on main repo: https://github.com/apache/spark/blob/master/.github/workflows/build_and_test.yml#L1045

  1. (if step 1 is ok, we don't need this step)

remove fetch-depth: 0, seems also save some space?

added rm minikube-linux-amd64 and removed accidental changes in 3.3.0. Now only 3.5.0 will be built.

removing fetch-deph: 0 would help with space, but the default is fetch-deph: 1 which only fetches a single commit form the main/master branch. That would make the 3.3.0 build fail as it needs to cherry-pick commits from history.
I've tested the removal of fetch-deph:0 with 3.3.0 on my repo: https://github.com/vakarisbk/spark-docker/actions/runs/6545230049/job/17773271828

@vakarisbk
Copy link
Contributor Author

  1. made fetch-depth: 0 applicable only for 3.3.0 (saves about 400MB for other builds)
  2. borrowed CI runner cleanup scripts from the main spark repo free_disk_space_container free_disk_space and added an action step to execute them.
    These scripts made the CI runner filesystem go from this:
Filesystem      Size  Used Avail Use% Mounted on
/dev/root        84G   66G   18G  80% /
tmpfs           3.4G  172K  3.4G   1% /dev/shm
tmpfs           1.4G  1.2M  1.4G   1% /run
tmpfs           5.0M     0  5.0M   0% /run/lock
/dev/sdb15      105M  6.1M   99M   6% /boot/efi
/dev/sda1        14G  4.1G  9.0G  31% /mnt
tmpfs           693M   12K  693M   1% /run/user/1001

to this:

Filesystem      Size  Used Avail Use% Mounted on
/dev/root        84G   29G   55G  35% /
tmpfs           3.4G  172K  3.4G   1% /dev/shm
tmpfs           1.4G  1.2M  1.4G   1% /run
tmpfs           5.0M     0  5.0M   0% /run/lock
/dev/sdb15      105M  6.1M   99M   6% /boot/efi
/dev/sda1        14G  4.1G  9.0G  31% /mnt
tmpfs           693M   12K  693M   1% /run/user/1001

@Yikun could you trigger the builds one more time?

@vakarisbk
Copy link
Contributor Author

All tests have passed except the 3.3.2 build.

The 3.3.2 build fails due to an issue with the GPG key on keys.openpgp.org (key on keyserver.ubuntu.com works fine)

- gpg --keyserver hkps://keys.openpgp.org --recv-key "C56349D886F2B01F8CAE794C653C2301FEA493EE" 

gpg: key 653C2301FEA493EE: no user ID
gpg: Total number processed: 1 

- gpg --batch --verify spark.tgz.asc spark.tgz

gpg: Signature made Pn Vas 10 22:40:58 2023 EET
gpg:                using RSA key C56349D886F2B01F8CAE794C653C2301FEA493EE
gpg:                issuer "[email protected]"
gpg: Can't check signature: No public key

But that's probably not relevant to this PR?

Apart from that, the PR is ready for review.

@Yikun
Copy link
Member

Yikun commented Oct 21, 2023

@viirya Hi, Would you mind taking a look on 3.3.2 release key issue. It might needs your help to upload the public key to opengpg, see [1] as ref.

[1] #55 (comment)

@viirya
Copy link
Member

viirya commented Oct 21, 2023

@Yikun Just uploaded to openpgp.

Copy link
Member

@Yikun Yikun left a comment

Choose a reason for hiding this comment

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

LGTM except some tiny nits, Please also make sure:

  1. All dockerfiles and entrypoint.sh should be generated by add-dockerfiles.sh
  2. It would be better if you can publish these images in your local repo to test (by appending a local change line in your local branch .github/workflows/publish.yml L50), It's just a test but shouldn't be changed in this PR.

Thanks for your efforts, I believe this PR will be merged soon.

@vakarisbk
Copy link
Contributor Author

vakarisbk commented Oct 22, 2023

Please also make sure:

  1. All dockerfiles and entrypoint.sh should be generated by add-dockerfiles.sh

All 3.5.0 dockerfiles and entrypoints were generated using the add-dockerfiles.sh
To validate, I ran this diff:

mv 3.5.0 3.5.0_copy; \
./add-dockerfiles.sh 3.5.0; \
diff -r 3.5.0 3.5.0_copy;
  1. It would be better if you can publish these images in your local repo to test (by appending a local change line in your local branch .github/workflows/publish.yml L50), It's just a test but shouldn't be changed in this PR.

I've published the images in my forked repository.
Publish job logs can be found here.

Let me know if anything else is needed

Copy link
Member

@Yikun Yikun left a comment

Choose a reason for hiding this comment

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

LGTM, only one minor nit for me.

I also use tools/manifest.py manifest to check the versions.json, looks good! Thanks for your efforts!

$ tools/manifest.py manifest
Maintainers: Apache Spark Developers <[email protected]> (@ApacheSpark)
GitRepo: https://github.com/apache/spark-docker.git

Tags: 3.5.0-scala2.12-java17-python3-ubuntu, 3.5.0-java17-python3, 3.5.0-java17, python3-java17
Architectures: amd64, arm64v8
GitCommit: 277235945e8b9774cb927dd0f4bffdac38908352
Directory: ./3.5.0/scala2.12-java17-python3-ubuntu

Tags: 3.5.0-scala2.12-java17-r-ubuntu, 3.5.0-java17-r
Architectures: amd64, arm64v8
GitCommit: 277235945e8b9774cb927dd0f4bffdac38908352
Directory: ./3.5.0/scala2.12-java17-r-ubuntu

Tags: 3.5.0-scala2.12-java17-ubuntu, 3.5.0-java17-scala
Architectures: amd64, arm64v8
GitCommit: 277235945e8b9774cb927dd0f4bffdac38908352
Directory: ./3.5.0/scala2.12-java17-ubuntu

Tags: 3.5.0-scala2.12-java17-python3-r-ubuntu
Architectures: amd64, arm64v8
GitCommit: 277235945e8b9774cb927dd0f4bffdac38908352
Directory: ./3.5.0/scala2.12-java17-python3-r-ubuntu

Tags: 3.5.0-scala2.12-java11-python3-ubuntu, 3.5.0-python3, 3.5.0, python3, latest
Architectures: amd64, arm64v8
GitCommit: 277235945e8b9774cb927dd0f4bffdac38908352
Directory: ./3.5.0/scala2.12-java11-python3-ubuntu

Tags: 3.5.0-scala2.12-java11-r-ubuntu, 3.5.0-r, r
Architectures: amd64, arm64v8
GitCommit: 277235945e8b9774cb927dd0f4bffdac38908352
Directory: ./3.5.0/scala2.12-java11-r-ubuntu

Tags: 3.5.0-scala2.12-java11-ubuntu, 3.5.0-scala, scala
Architectures: amd64, arm64v8
GitCommit: 277235945e8b9774cb927dd0f4bffdac38908352
Directory: ./3.5.0/scala2.12-java11-ubuntu

Tags: 3.5.0-scala2.12-java11-python3-r-ubuntu
Architectures: amd64, arm64v8
GitCommit: 277235945e8b9774cb927dd0f4bffdac38908352
Directory: ./3.5.0/scala2.12-java11-python3-r-ubuntu

scala2.12-java17-python3-r-ubuntu
scala2.12-java17-python3-ubuntu
scala2.12-java17-r-ubuntu
scala2.12-java17-ubuntu
Copy link
Member

Choose a reason for hiding this comment

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

Because we only add after 3.5 version, so we should skip 3.3 / 3.4 version. So seems we need some thing like below:

if ! echo $VERSION | grep -Eq "^3.3|^3.4"; then
   TAGS+="
   scala2.12-java17-python3-r-ubuntu
   scala2.12-java17-python3-ubuntu
   scala2.12-java17-r-ubuntu
   scala2.12-java17-ubuntu
   "
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@Yikun
Copy link
Member

Yikun commented Oct 24, 2023

cc @HyukjinKwon @zhengruifeng Would you mind also taking a look?

@vakarisbk
Copy link
Contributor Author

@Yikun maybe we can have this PR merged even without @HyukjinKwon and @zhengruifeng approval? This will not impact the existing images

@RoeiA
Copy link

RoeiA commented Nov 8, 2023

Waiting for this one for a while now, thank you @vakarisbk for this PR, would love to see it merged!

@Yikun
Copy link
Member

Yikun commented Nov 9, 2023

Thanks for your efforts @vakarisbk , I'm going to merge this PR later today or tomorrow.

@Yikun Yikun closed this in 6f68fe0 Nov 10, 2023
@Yikun Yikun changed the title Add support for java 17 from spark 3.5.0 [SPARK-43305] Add support for java 17 from spark 3.5.0 Nov 10, 2023
@Yikun
Copy link
Member

Yikun commented Nov 10, 2023

@vakarisbk Merged to master.

Thanks all!

@Yikun
Copy link
Member

Yikun commented Nov 10, 2023

@Yikun
Copy link
Member

Yikun commented Nov 10, 2023

  1. Image published on GHCR @vakarisbk, would you mind doing a post validation:

https://github.com/apache/spark-docker/pkgs/container/spark-docker%2Fspark

  1. Then you could feel free to open a PR on official image like to add java17 images:
    Update spark version to 3.5.0 docker-library/official-images#15363

The content can be generated by tools/manifest.py manifest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants