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.

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

Remove the bootstrap dependency on the Docker images we ship #16339

Merged
merged 10 commits into from
Jul 8, 2024
Merged

Remove the bootstrap dependency on the Docker images we ship #16339

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
17e9453
Remove bootstrap dependency for the lite image
frouioui Jul 4, 2024
7dcf64b
Remove the local docker image
frouioui Jul 4, 2024
1ab2da3
Remove bootstrap dependency for the vttestserver image
frouioui Jul 4, 2024
0a4826e
add dry-run docker build for all general images
frouioui Jul 8, 2024
aefb694
login only when we need to push the images
frouioui Jul 8, 2024
6c1158d
Merge all the worklow that builds into a single one
frouioui Jul 8, 2024
72d9cd5
Fix variable name in debug log
frouioui Jul 8, 2024
af52626
Apply suggestions from code review
frouioui Jul 8, 2024
c5ae3ee
Apply suggestions from code review
frouioui Jul 8, 2024
e99c1a7
Apply review suggestions
frouioui Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 97 additions & 14 deletions .github/workflows/docker_build_images.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,101 @@
name: Docker Build Images
name: Build Docker Images
on:
pull_request:
push:
branches:
- main
tags:
- 'v[2-9][0-9]*.*' # run only on tags greater or equal to v20.0.0 where this new way of building docker image was changed
- '*'

concurrency:
group: format('{0}-{1}', ${{ github.ref }}, 'Docker Build Images (v20+)')
group: format('{0}-{1}', ${{ github.ref }}, 'Docker Build Images')
cancel-in-progress: true
frouioui marked this conversation as resolved.
Show resolved Hide resolved

permissions: read-all

jobs:
skip_push:
name: Set skip_push if we are on a Pull Request
runs-on: ubuntu-20.04
if: github.repository == 'vitessio/vitess'
outputs:
skip_push: ${{ steps.skip_push.outputs.skip_push }}

Copy link
Member Author

Choose a reason for hiding this comment

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

This job will be executed before everyone else. Then build_and_push_vttestserver and build_and_push_lite are executed concurrently. Finally, once build_and_push_lite is done, build_and_push_components will begin building all the binaries concurrently.

steps:
- name: Set skip_push
id: skip_push
run: |
push='false'
if [[ "${{github.event.pull_request}}" == "" ]]; then
push='true'
fi
echo Push='${push}'
echo "skip_push=${push}" >> $GITHUB_OUTPUT

build_and_push_vttestserver:
name: Build and push vttestserver
runs-on: gh-hosted-runners-16cores-1
if: github.repository == 'vitessio/vitess' && needs.skip_push.result == 'success'
needs:
Copy link
Contributor

Choose a reason for hiding this comment

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

github.repository == 'vitessio/vitess' seems redundant here? And I think it's better to keep the criteria for when to skip within the skip_push step where we already check for this, if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed via e99c1a7

- skip_push

strategy:
fail-fast: true
matrix:
branch: [ mysql80 ]

steps:
- name: Check out code
uses: actions/checkout@v4

- name: Login to Docker Hub
if: needs.skip_push.outputs.skip_push == 'true'
uses: docker/login-action@v3
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we need to login to docker hub if we're NOT pushing?

Copy link
Member Author

@frouioui frouioui Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's a really good point, the naming of skip_push was misleading, I renamed it to push via e99c1a7

with:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}

- name: Set Dockerfile path
run: |
echo "DOCKERFILE=./docker/vttestserver/Dockerfile.${{ matrix.branch }}" >> $GITHUB_ENV

- name: Build and push on main
if: startsWith(github.ref, 'refs/tags/') == false
uses: docker/build-push-action@v5
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not explicitly check: if: github.ref == 'refs/heads/main'?

Copy link
Member Author

@frouioui frouioui Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We could check for this, but that will only build if the PR is based on main. Once we create the release-21.0 branch it will no longer work on that branch.

with:
context: .
file: ${{ env.DOCKERFILE }}
push: ${{ needs.skip_push.outputs.skip_push }}
tags: vitess/vttestserver:${{ matrix.branch }}

######
# All code below only applies to new tags
######
- name: Get the Git tag
if: startsWith(github.ref, 'refs/tags/')
run: echo "TAG_NAME=${GITHUB_REF#refs/tags/}" >> $GITHUB_ENV

- name: Set Docker tag name
if: startsWith(github.ref, 'refs/tags/')
run: |
echo "DOCKER_TAG=vitess/vttestserver:${TAG_NAME}-${{ matrix.branch }}" >> $GITHUB_ENV

- name: Build and push on tags
if: startsWith(github.ref, 'refs/tags/')
uses: docker/build-push-action@v5
with:
context: .
file: ${{ env.DOCKERFILE }}
push: true
tags: ${{ env.DOCKER_TAG }}


build_and_push_lite:
name: Build and push vitess/lite Docker images
name: Build and push vitess/lite
runs-on: gh-hosted-runners-16cores-1
frouioui marked this conversation as resolved.
Show resolved Hide resolved
if: github.repository == 'vitessio/vitess'
if: github.repository == 'vitessio/vitess' && needs.skip_push.result == 'success'
needs:
- skip_push

strategy:
fail-fast: true
Expand All @@ -28,6 +107,7 @@ jobs:
uses: actions/checkout@v4

- name: Login to Docker Hub
if: needs.skip_push.outputs.skip_push == 'true'
uses: docker/login-action@v3
with:
username: ${{ secrets.DOCKERHUB_USERNAME }}
Expand All @@ -42,12 +122,12 @@ jobs:
fi

- name: Build and push on main
if: github.ref == 'refs/heads/main'
if: startsWith(github.ref, 'refs/tags/') == false
uses: docker/build-push-action@v5
with:
context: .
file: ${{ env.DOCKERFILE }}
push: true
push: ${{ needs.skip_push.outputs.skip_push }}
tags: vitess/lite:${{ matrix.branch }}

######
Expand Down Expand Up @@ -76,10 +156,12 @@ jobs:
tags: ${{ env.DOCKER_TAG }}

build_and_push_components:
name: Build and push vitess components Docker images
needs: build_and_push_lite
name: Build and push
runs-on: gh-hosted-runners-16cores-1
if: github.repository == 'vitessio/vitess'
if: github.repository == 'vitessio/vitess' && needs.skip_push.result == 'success' && needs.build_and_push_lite.result == 'success'
needs:
- skip_push
- build_and_push_lite

strategy:
fail-fast: true
Expand All @@ -92,6 +174,7 @@ jobs:
uses: actions/checkout@v4

- name: Login to Docker Hub
if: needs.skip_push.outputs.skip_push == 'true'
uses: docker/login-action@v3
with:
username: ${{ secrets.DOCKERHUB_USERNAME }}
Expand All @@ -102,22 +185,22 @@ jobs:
echo "DOCKER_CTX=./docker/binaries/${{ matrix.component }}" >> $GITHUB_ENV

- name: Build and push on main latest tag
if: github.ref == 'refs/heads/main' && matrix.debian == 'bookworm'
if: startsWith(github.ref, 'refs/tags/') == false && matrix.debian == 'bookworm'
uses: docker/build-push-action@v5
with:
context: ${{ env.DOCKER_CTX }}
push: true
push: ${{ needs.skip_push.outputs.skip_push }}
tags: vitess/${{ matrix.component }}:latest
build-args: |
VT_BASE_VER=latest
DEBIAN_VER=${{ matrix.debian }}-slim

- name: Build and push on main debian specific tag
if: github.ref == 'refs/heads/main'
if: startsWith(github.ref, 'refs/tags/') == false
uses: docker/build-push-action@v5
with:
context: ${{ env.DOCKER_CTX }}
push: true
push: ${{ needs.skip_push.outputs.skip_push }}
tags: vitess/${{ matrix.component }}:latest-${{ matrix.debian }}
build-args: |
VT_BASE_VER=latest
Expand Down
65 changes: 0 additions & 65 deletions .github/workflows/docker_build_vttestserver.yml
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ docker_lite:
docker_mini:
${call build_docker_image,docker/mini/Dockerfile,vitess/mini}

DOCKER_VTTESTSERVER_SUFFIX = mysql57 mysql80
DOCKER_VTTESTSERVER_SUFFIX = mysql80
Copy link
Member Author

Choose a reason for hiding this comment

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

We only have mysql80, since a while.

DOCKER_VTTESTSERVER_TARGETS = $(addprefix docker_vttestserver_,$(DOCKER_VTTESTSERVER_SUFFIX))
$(DOCKER_VTTESTSERVER_TARGETS): docker_vttestserver_%:
${call build_docker_image,docker/vttestserver/Dockerfile.$*,vitess/vttestserver:$*}
Expand Down
25 changes: 11 additions & 14 deletions docker/lite/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,29 +12,26 @@
# See the License for the specific language governing permissions and
# limitations under the License.

# NOTE: We have to build the Vitess binaries from scratch instead of sharing
# a base image because Docker Hub dropped the feature we relied upon to
# ensure images contain the right binaries.

# Use a temporary layer for the build stage.
ARG bootstrap_version=34
ARG image="vitess/bootstrap:${bootstrap_version}-mysql80"

FROM "${image}" AS builder
FROM --platform=linux/amd64 golang:1.22.5-bullseye AS builder

# Allows docker builds to set the BUILD_NUMBER
ARG BUILD_NUMBER

# Re-copy sources from working tree.
COPY --chown=vitess:vitess . /vt/src/vitess.io/vitess
WORKDIR /vt/src/vitess.io/vitess

# Build and install Vitess in a temporary output directory.
# Create vitess user
RUN groupadd -r vitess && useradd -r -g vitess vitess
RUN mkdir -p /vt/vtdataroot /home/vitess
RUN chown -R vitess:vitess /vt /home/vitess
USER vitess

# Re-copy sources from working tree.
COPY --chown=vitess:vitess . /vt/src/vitess.io/vitess

RUN make install PREFIX=/vt/install

# Start over and build the final image.
FROM debian:bullseye-slim
FROM --platform=linux/amd64 debian:bullseye-slim
Copy link
Member Author

Choose a reason for hiding this comment

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

I have added these --platform=linux/amd64 directives here and in other places to ensure that the base image we pull is based on linux/amd64, that way, we can build the docker images from any machine. For instance, if we remove this directive and try to build from a Mac, the install dependencies step will fail as there are no Percona package for the Mac's architecture.


# Install dependencies
COPY docker/utils/install_dependencies.sh /vt/dist/install_dependencies.sh
Expand All @@ -45,7 +42,7 @@ RUN groupadd -r vitess && useradd -r -g vitess vitess
RUN mkdir -p /vt/vtdataroot && chown -R vitess:vitess /vt

# Set up Vitess environment (just enough to run pre-built Go binaries)
ENV VTROOT /vt/src/vitess.io/vitess
ENV VTROOT /vt
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this was wrong to begin with, leading to the error: bash: vtgate: command not found:

$ docker run -it --user=vitess vitess/lite:v20.0.0 bash
vitess@0d7c6ece9106:/$ vtgate
bash: vtgate: command not found
vitess@0d7c6ece9106:/$

With this fix, the vitess binaries are directly in the PATH and people can execute them without specifying the full path to the binary.

Copy link
Member Author

@frouioui frouioui Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

With the fix:

$ docker run -it --user="vitess" docker.io/vitess/lite-test:latest bash
vitess@6a3782014166:/$ vtgate --version 
vtgate version Version: 21.0.0-SNAPSHOT (Git revision 694a0cf8d9e51aa4b130ad1656c4009427eb0e3d branch 'remove-bootstrap-from-lite') built on Thu Jul  4 15:55:37 UTC 2024 by vitess@buildkitsandbox using go1.22.5 linux/amd64
vitess@6a3782014166:/$

ENV VTDATAROOT /vt/vtdataroot
ENV PATH $VTROOT/bin:$PATH

Expand Down
24 changes: 11 additions & 13 deletions docker/lite/Dockerfile.percona80
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,28 +12,26 @@
# See the License for the specific language governing permissions and
# limitations under the License.

# NOTE: We have to build the Vitess binaries from scratch instead of sharing
# a base image because Docker Hub dropped the feature we relied upon to
# ensure images contain the right binaries.

# Use a temporary layer for the build stage.
ARG bootstrap_version=34
ARG image="vitess/bootstrap:${bootstrap_version}-percona80"

FROM "${image}" AS builder
FROM --platform=linux/amd64 golang:1.22.5-bullseye AS builder

# Allows docker builds to set the BUILD_NUMBER
ARG BUILD_NUMBER

WORKDIR /vt/src/vitess.io/vitess

# Create vitess user
RUN groupadd -r vitess && useradd -r -g vitess vitess
RUN mkdir -p /vt/vtdataroot /home/vitess
RUN chown -R vitess:vitess /vt /home/vitess
USER vitess

# Re-copy sources from working tree.
COPY --chown=vitess:vitess . /vt/src/vitess.io/vitess

# Build and install Vitess in a temporary output directory.
USER vitess
RUN make install PREFIX=/vt/install

# Start over and build the final image.
FROM debian:bullseye-slim
FROM --platform=linux/amd64 debian:bullseye-slim

# Install dependencies
COPY docker/utils/install_dependencies.sh /vt/dist/install_dependencies.sh
Expand All @@ -44,7 +42,7 @@ RUN groupadd -r vitess && useradd -r -g vitess vitess
RUN mkdir -p /vt/vtdataroot && chown -R vitess:vitess /vt

# Set up Vitess environment (just enough to run pre-built Go binaries)
ENV VTROOT /vt/src/vitess.io/vitess
ENV VTROOT /vt
ENV VTDATAROOT /vt/vtdataroot
ENV PATH $VTROOT/bin:$PATH

Expand Down
45 changes: 0 additions & 45 deletions docker/local/Dockerfile
View file
Open in desktop

This file was deleted.

Loading
Loading