Skip to content
This repository was archived by the owner on May 9, 2024. It is now read-only.

Fix docker cache #369

Merged
merged 1 commit into from
Apr 11, 2023
Merged

Fix docker cache #369

merged 1 commit into from
Apr 11, 2023

Conversation

leshikus
Copy link
Contributor

@leshikus leshikus commented Apr 6, 2023

Subj

@leshikus leshikus marked this pull request as draft April 6, 2023 20:44
@leshikus leshikus force-pushed the lesh/fix-docker branch 8 times, most recently from d8fbfa1 to e6e3d0b Compare April 7, 2023 20:11
@leshikus leshikus requested a review from Devjiu April 7, 2023 20:41
@leshikus leshikus marked this pull request as ready for review April 7, 2023 20:43
@leshikus
Copy link
Contributor Author

leshikus commented Apr 7, 2023

Image build time decreased from 42m to 3m

@alexbaden
Copy link
Contributor

What Docker image repository are we using?

@leshikus
Copy link
Contributor Author

leshikus commented Apr 11, 2023

@alexbaden I use Docker Hub (= docker.io); it should give us 200 pulls for 6 hours for free

Copy link
Contributor

@Devjiu Devjiu left a comment

Choose a reason for hiding this comment

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

👍 Great PR.
Currently it uses https://hub.docker.com/u/dataved as public image and pushed new image on combined Dockerfile hash changed. As a result dockerhub is our public cache.

Docker pull step takes about ~3 minutes, that is better than docker build even using cached layers.

Please, provide description to this pr, because there are a lot of fundamental changes in Docker build workflow (files moved from omnisci folder and now used public image).

Also currently Docker image update happens automatically (can happen on each pr) I would prefer a separate job to handle changes to the image, but this is not critical, we will test this suggested approach and update later.

There are also some minor motes, that can be done in further prs.


- name: Configure and build the project
run: |
docker exec -u ghrunner build.${{ inputs.name }} dpkg -l
docker exec -u ghrunner build.${{ inputs.name }} sh /_work/omniscidb/scripts/conda/build.sh ${{ inputs.options }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think maven cache also can used from cache.
docker cp ~/.m2/. verify:/home/ghrunner/.m2 or with -v ${PWD}/.m2:/home/ghrunner/.m2.

rm -rf /tmp/.buildx-cache
mv /tmp/.buildx-cache-new /tmp/.buildx-cache
push: true
tags: ${{ env.DOCKER_NAME }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor note. It's possible to reduce amount of pulls from docker.io with caching docker local storage with github cache. As it reduces build time ~ 2 mins we decided to keep current (more simple and direct) approach. Also github cache is limited with 10 GB and current image size is ~5.6 Gb, conda env/ maven should also be cached so caching for docker storage will be checked later.

@@ -11,6 +11,9 @@ on:
options:
type: string
default: -DENABLE_CUDA=off
secrets:
DOCKERHUB_PASSWORD:
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that secrets are global vars for all github jobs, but after discussion with Alexei I understood that it's special github behavior https://github.blog/changelog/2022-05-03-github-actions-simplify-using-secrets-with-reusable-workflows/ . Maybe there are some WA, because secrets added as https://docs.github.com/en/actions/learn-github-actions/variables#creating-configuration-variables-for-an-environment .

This is only to mark and summarize our discussion.

@leshikus leshikus merged commit 0031613 into main Apr 11, 2023
@leshikus leshikus deleted the lesh/fix-docker branch April 11, 2023 14:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants