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

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

Merged
merged 10 commits into from
Jul 8, 2024

Conversation

frouioui
Copy link
Member

@frouioui frouioui commented Jul 4, 2024

Description

This PR removes the dependency that the vitess/lite and vitess/vttestsever Docker images had on the vitess/bootstrap image. We do not need to build these two images on top of bootstrap as we end-up installing all the required dependencies in the final stage of the build anyway.

The docker/local folder is also removed as a follow up of #16335.

With this PR the whole dependency chain of our images is simplified and any future work on the docker images will be easier. This is also the first step towards simplifying and potentially removing the bootstrap image, the only use right now is for our CI test (in ./test.go and ./docker/test).

I reworked our two github action workflows that enabled us to build and push docker images (vttestserver, lite, and all the binaries). I merged the two workflows into a single one, which will make it easier to know if all images we ship are building correctly. Moreover, for every Pull Requests we will now try to build the Docker Images without pushing them, that way we know if we break a docker build. We often realized this too late in the past, while we were doing a release. Here is what the new workflow looks like:

image

Note

The new workflow we are adding to CI spawns about 33 jobs that will show up on the workflows list of every Pull Request. Despite having a lot of new jobs, the time needed to complete all workflows should not be impacted by much, we need about 5-8 minutes to complete all 33 jobs of the Build Docker Images workflow.

This workflow should be marked as required once the PR is merged.

Finally, some changes are needed on the vitess-releaser to use the new workflow to ensure GHA has built all the Docker images during the release. (https://github.com/vitessio/vitess-releaser/blob/main/go/releaser/release/docker.go)

This is marked as a draft, we will merge it when the following PRs are merged:

Copy link
Contributor

vitess-bot bot commented Jul 4, 2024

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
  • vtctl command output order should be stable and awk-able.

@vitess-bot vitess-bot bot added NeedsBackportReason If backport labels have been applied to a PR, a justification is required NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Jul 4, 2024
@github-actions github-actions bot added this to the v21.0.0 milestone Jul 4, 2024
@frouioui frouioui added Backport to: release-18.0 Backport to: release-19.0 Needs to be back ported to release-19.0 Backport to: release-20.0 Needs to be backport to release-20.0 Type: Bug Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Docker and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Jul 4, 2024
Copy link

codecov bot commented Jul 4, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 68.71%. Comparing base (0db3577) to head (c5ae3ee).

Files Patch % Lines
go/tools/go-upgrade/go-upgrade.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #16339   +/-   ##
=======================================
  Coverage   68.71%   68.71%           
=======================================
  Files        1547     1547           
  Lines      198287   198287           
=======================================
+ Hits       136243   136249    +6     
+ Misses      62044    62038    -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@frouioui frouioui removed Backport to: release-18.0 Backport to: release-19.0 Needs to be back ported to release-19.0 Backport to: release-20.0 Needs to be backport to release-20.0 labels Jul 4, 2024
@frouioui frouioui marked this pull request as draft July 4, 2024 17:09
@frouioui frouioui removed the Type: Bug label Jul 4, 2024
@frouioui frouioui force-pushed the remove-bootstrap-from-lite branch from ecc0b28 to 1ab2da3 Compare July 8, 2024 16:33
@frouioui frouioui marked this pull request as ready for review July 8, 2024 17:17
@frouioui frouioui requested a review from rohit-nayak-ps as a code owner July 8, 2024 17:17
@@ -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.

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.

@@ -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

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:/$

Comment on lines -438 to -440
"./docker/lite",
"./docker/local",
"./docker/vttestserver",
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 no longer update the bootstrap version in these path.

Comment on lines +396 to +398
"./docker/lite/Dockerfile",
"./docker/lite/Dockerfile.percona80",
"./docker/vttestserver/Dockerfile.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 now have to bump the golang version in more places since we base more of our images on the official go docker image.

Comment on lines 17 to 22
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.

@frouioui frouioui changed the title Remove bootstrap dependency for the lite and vttestserver Docker image Simplify the Docker images we ship by removing their dependency on bootstrap Jul 8, 2024
@frouioui frouioui changed the title Simplify the Docker images we ship by removing their dependency on bootstrap Remove the bootstrap dependency on the Docker images we ship Jul 8, 2024
Signed-off-by: Florent Poinsard <[email protected]>
@frouioui frouioui requested a review from deepthi July 8, 2024 18:29
.github/workflows/docker_build_images.yml Outdated Show resolved Hide resolved
.github/workflows/docker_build_images.yml Outdated Show resolved Hide resolved
.github/workflows/docker_build_images.yml Outdated Show resolved Hide resolved
.github/workflows/docker_build_images.yml Outdated Show resolved Hide resolved
Co-authored-by: Deepthi Sigireddi <[email protected]>
Signed-off-by: Florent Poinsard <[email protected]>
.github/workflows/docker_build_images.yml Outdated Show resolved Hide resolved
.github/workflows/docker_build_images.yml Outdated Show resolved Hide resolved
Signed-off-by: Florent Poinsard <[email protected]>
Copy link
Contributor

@mattlord mattlord left a comment

Choose a reason for hiding this comment

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

I had a few questions, but perhaps I'm missing or misunderstanding somethings?

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'
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

uses: actions/checkout@v4

- name: Login to Docker Hub
if: needs.skip_push.outputs.skip_push == 'true'
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

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

echo "DOCKERFILE=./docker/vttestserver/Dockerfile.${{ matrix.branch }}" >> $GITHUB_ENV

- name: Build and push on main
if: startsWith(github.ref, 'refs/tags/') == false
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

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.

Signed-off-by: Florent Poinsard <[email protected]>
@frouioui frouioui merged commit 6d95d7c into vitessio:main Jul 8, 2024
124 checks passed
@frouioui frouioui deleted the remove-bootstrap-from-lite branch July 8, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Docker Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants