[SPARK-39522][INFRA]Uses Docker image cache over a custom image in pyspark job #37005

Yikun · 2022-06-27T13:08:48Z

What changes were proposed in this pull request?

Change pyspark container from original static image to just-in-time build image from cache.

See also: https://docs.google.com/document/d/1_uiId-U1DODYyYZejAZeyz2OAjxcnA-xfwjynDF6vd0

This patch has 2 changes:

Add a infra-image job to build ci image from cache images
Use the ci image as pyspark job container image, only master branch enable the new workflow now.

Why are the changes needed?

Help to speed up docker infra image build in each PR.

Does this PR introduce any user-facing change?

No

How was this patch tested?

CI passed

.github/workflows/build_and_test.yml

dev/infra/Dockerfile

… for container based job ### What changes were proposed in this pull request? This patch add GITHUB_WORKSPACE to git trust safe.directory for container based job. There are 3 container based job in Spark Infra: - sparkr - lint - pyspark ### Why are the changes needed? When upgrade `git >= 2.35.2` (such as latest dev docker image in my case), fix a [CVE-2022-24765](https://github.blog/2022-04-12-git-security-vulnerability-announced/#cve-2022-24765) , and it has been backported [latest ubuntu git](actions/checkout#760 (comment)) The `GITHUB_WORKSPACE` is belong to action user (in host), but the container user is `root` (in container) (root is required in our spark job), so cause the error like: ``` fatal: unsafe repository ('/__w/spark/spark' is owned by someone else) To add an exception for this directory, call: git config --global --add safe.directory /__w/spark/spark fatal: unsafe repository ('/__w/spark/spark' is owned by someone else) To add an exception for this directory, call: git config --global --add safe.directory /__w/spark/spark ``` The solution is from `action/checkout` actions/checkout#760 , just add `GITHUB_WORKSPACE` to git trust safe.directory to make container user can checkout successfully. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? CI passed. CI passed with latest ubuntu hosted runner, here is a simple e2e test for pyspark job #37005 Closes #37079 from Yikun/SPARK-39610. Authored-by: Yikun Jiang <yikunkero@gmail.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

.github/workflows/build_and_test.yml

Yikun · 2022-07-06T14:56:51Z

recompute is not blocked by the recovery *** FAILED *** (1 minute, 6 seconds)

Failure due to unrelated error, I will do a retrigger after #37103 merged.

Yikun · 2022-07-07T01:25:29Z

Retriggered: https://github.com/Yikun/spark/runs/7225401037?check_suite_focus=true , infra image works well, let's see pyspark job. then will ready for review.

Yikun · 2022-07-07T12:01:36Z

.github/workflows/build_and_test.yml

+        -X DELETE
+        -H "Accept: application/vnd.github+json"
+        -H "Authorization: token ${{ secrets.GHCR_DEL }}"
+        https://api.github.com/user/packages/container/${{ needs.infra-image.outputs.image_name }}


I think it's:

a little bit flaky

also bring some configure complex for developer, we have to add PAT secrets rather than GITHUB_TOKEN

also it's not supported well by actions/delete-package-versions

also github didn't provide delete package by tag API.

there are some 3rd party action supported but apache infra will say no.

maybe undeletion image tag can also help reproduce locally

So, I think apache-spark-ci-image:${{ inputs.branch }}-${{ github.run_id }} (diff tag for each pr) without any cleanup better than apache-spark-ci-image-${{ inputs.branch }}-${{ github.run_id }} (diff packages for each pr) by auto deletion.

If developers want to delete package for some reason, they can also easy delete complete image in github page.

@HyukjinKwon So, I decided to remove infra-image-post and back to use apache-spark-ci-image:tag without deletion.

Thanks for explanation.

Yikun · 2022-07-07T15:07:14Z

cc @HyukjinKwon @dongjoon-hyun

Ready for review, Thanks!

Yikun · 2022-07-07T17:08:14Z

.github/workflows/build_and_test.yml

+        uses: docker/login-action@v2
+        with:
+          registry: ghcr.io
+          username: ${{ github.actor }}


TODO: need a test for schedule job before ready to go

Case 1: The developer push a commit: ==> OK (CI passed!)

Login: github.actor: yikun

CI Image URL: yikun/apache-spark-ci-image:master-xxx

Case 2: Merge a commit in apache/spark master: ==> should OK
(committer need has right to push apache package) According to link it's inherited from repo right.

Login: ${{ github.actor }}: such as HyukjinKwon (who merge the commit)

CI Image URL: apache/apache-spark-ci-image:master-xxx

local test: login , image

Case 3: Scheduler job (master): ==> should OK
(The latest user who create the schedule trigger need has right to push apache package) same rights wtih case2

Login: ${{ github.actor }}: such as HyukjinKwon (The latest user who create the schedule trigger will be the actor for the schedule workflow)

CI Image URL: apache/apache-spark-ci-image:master-xxx

local test: login, image

Case 4: Scheduler job (branch-xxx) ==> OK

Use branches image cache directly (dongjoon/apache-spark-github-action-image:20220207, future is spark/apache-spark-github-action-image-cache:branches-*)

Case 5: The developer push a commit in branches: ==> OK
Case 6: The apache/spark merge a commit in branches: ==> OK

use branches workflow, no change by this pr

When cut branches, will same as case 1 & case 2

Yikun · 2022-07-08T07:12:47Z

Does all spark committers has right to push apache ghcr package (case 2, case 3)? According to cache image, I can confirm @HyukjinKwon has rights, but not sure any different with other commiters.

[1] https://docs.github.com/en/packages/learn-github-packages/configuring-a-packages-access-control-and-visibility#inheriting-access-for-a-container-image-from-a-repository

HyukjinKwon · 2022-07-08T07:45:50Z

Yes, all other committers have the same permission with me.

HyukjinKwon · 2022-07-08T07:48:22Z

Let's get this in and see how it gose.

HyukjinKwon · 2022-07-08T07:48:36Z

Merged to master.

Yikun · 2022-07-08T07:56:53Z

Let's get this in and see how it gose.

Sure, I will monitor in recent days. and update results here:

Case 1: The developer push a commit: ci passed already
Case2: Merge a commit in apache/spark master, work as expected: https://github.com/apache/spark/runs/7247609069?check_suite_focus=true#step:8:204
Case 3: Scheduler job (master), work as expected: https://github.com/apache/spark/runs/7252245523?check_suite_focus=true
Case 4: Scheduler job (branch-xxx) https://github.com/apache/spark/runs/7262144614?check_suite_focus=true#step:2:18

.github/workflows/build_and_test.yml

HyukjinKwon · 2022-07-08T08:12:17Z

.github/workflows/build_and_test.yml

+          fetch-depth: 0
+          repository: apache/spark
+          ref: ${{ inputs.branch }}
+      - name: Sync the current branch with the latest in Apache Spark


I think we don't need this step.

If a PR changes the dockerfile and merged, this will help to update the dockerfile in follow PR...so, I think it's necessary?

oh okay you're correct!

.github/workflows/build_and_test.yml

HyukjinKwon · 2022-07-08T08:13:18Z

.github/workflows/build_and_test.yml

@@ -251,13 +251,73 @@ jobs:
        name: unit-tests-log-${{ matrix.modules }}-${{ matrix.comment }}-${{ matrix.java }}-${{ matrix.hadoop }}-${{ matrix.hive }}
        path: "**/target/unit-tests.log"

-  pyspark:
+  infra-image:


BTW, let's add a name here to make it look pretty (e.g., Base image build)

HyukjinKwon · 2022-07-08T08:13:42Z

Feel free to make a followup or a separate PR with a separate JIRA 👍

### What changes were proposed in this pull request? Add name for infra-image and cleanup nits and fix the inline format ### Why are the changes needed? Address comments in #37005 (comment) ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? CI passed Closes #37131 from Yikun/SPARK-39718. Authored-by: Yikun Jiang <yikunkero@gmail.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

github-actions bot added BUILD INFRA labels Jun 27, 2022

Yikun commented Jun 27, 2022

View reviewed changes

.github/workflows/build_and_test.yml Outdated Show resolved Hide resolved

Yikun commented Jun 27, 2022

View reviewed changes

dev/infra/Dockerfile Outdated Show resolved Hide resolved

Yikun force-pushed the SPARK-39522-pyspark branch from d5187ce to 7c84582 Compare June 27, 2022 14:12

Yikun mentioned this pull request Jun 27, 2022

[POC][SPARK-39522][INFRA] Uses Docker image cache over a custom image #36980

Closed

Yikun mentioned this pull request Jul 5, 2022

[SPARK-39610][INFRA] Add GITHUB_WORKSPACE to git trust safe.directory for container based job #37079

Closed

Yikun force-pushed the SPARK-39522-pyspark branch 3 times, most recently from 509e126 to d63d979 Compare July 6, 2022 01:37

Yikun commented Jul 6, 2022

View reviewed changes

.github/workflows/build_and_test.yml Outdated Show resolved Hide resolved

Yikun force-pushed the SPARK-39522-pyspark branch 7 times, most recently from 96db6b3 to 764923e Compare July 6, 2022 09:54

Yikun added 3 commits July 7, 2022 12:16

Uses Docker image cache over a custom image in pyspark job

eb55feb

Only enable build from cache in master

725778a

Move out put to infra-image job

3e6bc89

Yikun force-pushed the SPARK-39522-pyspark branch from 764923e to 3e6bc89 Compare July 7, 2022 06:52

Yikun commented Jul 7, 2022

View reviewed changes

Remove infra-post-job

c15c85f

Yikun marked this pull request as ready for review July 7, 2022 15:07

Yikun commented Jul 7, 2022

View reviewed changes

HyukjinKwon approved these changes Jul 8, 2022

View reviewed changes

Yikun marked this pull request as draft July 8, 2022 05:01

Yikun marked this pull request as ready for review July 8, 2022 06:53

HyukjinKwon closed this in 5925e1d Jul 8, 2022

HyukjinKwon reviewed Jul 8, 2022

View reviewed changes

.github/workflows/build_and_test.yml Show resolved Hide resolved

HyukjinKwon reviewed Jul 8, 2022

View reviewed changes

.github/workflows/build_and_test.yml Show resolved Hide resolved

HyukjinKwon reviewed Jul 8, 2022

View reviewed changes

.github/workflows/build_and_test.yml Show resolved Hide resolved

HyukjinKwon reviewed Jul 8, 2022

View reviewed changes

Yikun mentioned this pull request Jul 8, 2022

[SPARK-39718][INFRA] Add name for infra-image and cleanup nits #37131

Closed

Yikun mentioned this pull request Jul 11, 2022

[SPARK-39735][INFRA] Enable base image build in lint job #37149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-39522][INFRA]Uses Docker image cache over a custom image in pyspark job #37005

[SPARK-39522][INFRA]Uses Docker image cache over a custom image in pyspark job #37005

Yikun commented Jun 27, 2022 •

edited

Yikun commented Jul 6, 2022 •

edited

Yikun commented Jul 7, 2022 •

edited

Yikun Jul 7, 2022 •

edited

HyukjinKwon Jul 8, 2022

Yikun commented Jul 7, 2022

Yikun Jul 7, 2022

Yikun Jul 8, 2022 •

edited

Yikun commented Jul 8, 2022 •

edited

HyukjinKwon commented Jul 8, 2022

HyukjinKwon commented Jul 8, 2022

HyukjinKwon commented Jul 8, 2022

Yikun commented Jul 8, 2022 •

edited

HyukjinKwon Jul 8, 2022

Yikun Jul 8, 2022 •

edited

HyukjinKwon Jul 8, 2022

HyukjinKwon Jul 8, 2022

HyukjinKwon commented Jul 8, 2022

[SPARK-39522][INFRA]Uses Docker image cache over a custom image in pyspark job #37005

[SPARK-39522][INFRA]Uses Docker image cache over a custom image in pyspark job #37005

Conversation

Yikun commented Jun 27, 2022 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Yikun commented Jul 6, 2022 • edited

Yikun commented Jul 7, 2022 • edited

Yikun Jul 7, 2022 • edited

Choose a reason for hiding this comment

HyukjinKwon Jul 8, 2022

Choose a reason for hiding this comment

Yikun commented Jul 7, 2022

Yikun Jul 7, 2022

Choose a reason for hiding this comment

Yikun Jul 8, 2022 • edited

Choose a reason for hiding this comment

Yikun commented Jul 8, 2022 • edited

HyukjinKwon commented Jul 8, 2022

HyukjinKwon commented Jul 8, 2022

HyukjinKwon commented Jul 8, 2022

Yikun commented Jul 8, 2022 • edited

HyukjinKwon Jul 8, 2022

Choose a reason for hiding this comment

Yikun Jul 8, 2022 • edited

Choose a reason for hiding this comment

HyukjinKwon Jul 8, 2022

Choose a reason for hiding this comment

HyukjinKwon Jul 8, 2022

Choose a reason for hiding this comment

HyukjinKwon commented Jul 8, 2022

Yikun commented Jun 27, 2022 •

edited

Yikun commented Jul 6, 2022 •

edited

Yikun commented Jul 7, 2022 •

edited

Yikun Jul 7, 2022 •

edited

Yikun Jul 8, 2022 •

edited

Yikun commented Jul 8, 2022 •

edited

Yikun commented Jul 8, 2022 •

edited

Yikun Jul 8, 2022 •

edited