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

Fix Dockerfiles for vtexplain and vtctlclient #7418

Merged

Conversation

deepthi
Copy link
Member

@deepthi deepthi commented Jan 31, 2021

Description

Add VT_BASE_VER to vtexplain so that we can build release versions of the docker image.
Use lite as base for vtctlclient because k8s image is no longer being updated. has not been updated since 8.0.

Related Issue(s)

#7399
#7401

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build/CI
  • VTAdmin

@deepthi
Copy link
Member Author

deepthi commented Jan 31, 2021

We also have to ensure that the rules on dockerhub are correct.
When I built the images locally, I passed VT_BASE_VER=v9.0.0 to build them correctly.
How can we set this up on docker? Right now the build config looks like this.

vtctlclient-docker

FROM vitess/base AS base
ARG VT_BASE_VER=latest

FROM vitess/base:${VT_BASE_VER} AS base
Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason, using lite as base doesn't work.
If one of you (@dkhenry / @derekperkins) knows how to fix that, let me know.
I'd like to avoid using vitess/base if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

lite doesn't contain vtexplain in it, AFAIK. Before I discovered the vitess/vtexplain image, I had to set up my vtctld component with vitess/base and kubectl exec into it to run vtexplain, because that binary wasn't available in /vt/bin of vitess/lite.

Copy link
Member Author

@deepthi deepthi Feb 1, 2021

Choose a reason for hiding this comment

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

that explains it (no pun intended) 😄

@@ -25,7 +25,7 @@ RUN apt-get update && \
apt-get clean && \
rm -rf /var/lib/apt/lists/*

COPY --from=k8s /vt/bin/vtctlclient /usr/bin/
COPY --from=lite /vt/bin/vtctlclient /usr/bin/
Copy link
Contributor

Choose a reason for hiding this comment

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

Want to point out (but it might not be worth doing anything about; and changing it would be a breaking change) that vtctlclient is put into /usr/bin, but vtexplain is put into /vt/bin. It's inconsistent that they don't both go into one or the other, or both go into both.

vitess ((HEAD detached at origin/release-9.0)) $ git grep "COPY.*/usr/bin"
docker/k8s/orchestrator/Dockerfile:33:COPY --from=k8s /vt/bin/vtctlclient /usr/bin/
docker/k8s/vtctlclient/Dockerfile:28:COPY --from=k8s /vt/bin/vtctlclient /usr/bin/
docker/orchestrator/Dockerfile:31:COPY vtctlclient /usr/bin/vtctlclient
docker/orchestrator/Dockerfile:32:COPY orchestrator /usr/bin/orchestrator

I guess it makes some sense that orchestrator (which I guess was for the old Helm chart) and vtctlclient don't go into /vt/bin, since the former isn't a Vitess component, and the latter is a client component, not part of the Vitess service. But by that same heuristic, vtexplain also wouldn't go into /vt/bin. 🤷‍♀️ Doesn't matter that much, figured I'd point it out.

vitess ((HEAD detached at origin/release-9.0)) $ git grep "COPY.*/vt/bin"
docker/k8s/Dockerfile:44:COPY --from=base /vt/bin/mysqlctld /vt/bin/
docker/k8s/Dockerfile:45:COPY --from=base /vt/bin/vtctld /vt/bin/
docker/k8s/Dockerfile:46:COPY --from=base /vt/bin/vtctl /vt/bin/
docker/k8s/Dockerfile:47:COPY --from=base /vt/bin/vtctlclient /vt/bin/
docker/k8s/Dockerfile:48:COPY --from=base /vt/bin/vtgate /vt/bin/
docker/k8s/Dockerfile:49:COPY --from=base /vt/bin/vttablet /vt/bin/
docker/k8s/Dockerfile:50:COPY --from=base /vt/bin/vtworker /vt/bin/
docker/k8s/Dockerfile:51:COPY --from=base /vt/bin/vtbackup /vt/bin/
docker/k8s/mysqlctld/Dockerfile:37:COPY --from=k8s /vt/bin/mysqlctld /vt/bin/
docker/k8s/orchestrator/Dockerfile:33:COPY --from=k8s /vt/bin/vtctlclient /usr/bin/
docker/k8s/vtbackup/Dockerfile:29:COPY --from=k8s /vt/bin/vtbackup /vt/bin/
docker/k8s/vtctl/Dockerfile:31:COPY --from=k8s /vt/bin/vtctl /vt/bin/
docker/k8s/vtctlclient/Dockerfile:28:COPY --from=k8s /vt/bin/vtctlclient /usr/bin/
docker/k8s/vtctld/Dockerfile:29:COPY --from=k8s /vt/bin/vtctld /vt/bin/
docker/k8s/vtexplain/Dockerfile:26:COPY --from=base /vt/bin/vtexplain /vt/bin/
docker/k8s/vtgate/Dockerfile:31:COPY --from=k8s /vt/bin/vtgate /vt/bin/
docker/k8s/vttablet/Dockerfile:37:COPY --from=k8s /vt/bin/vttablet /vt/bin/
docker/k8s/vttablet/Dockerfile:38:COPY --from=k8s /vt/bin/vtctlclient /vt/bin/
docker/k8s/vtworker/Dockerfile:31:COPY --from=k8s /vt/bin/vtworker /vt/bin/
docker/mini/Dockerfile:46:COPY --from=base /vt/bin/vtctl /vt/bin/
docker/mini/Dockerfile:47:COPY --from=base /vt/bin/mysqlctl /vt/bin/

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right that it would be a breaking change :(
I have anyway included it in #7433 so that we can discuss whether or not to do anything.

@derekperkins
Copy link
Member

@deepthi I still try to keep those images up, including k8s, though we should definitely come up with a more holistic strategy. All I do is run https://github.com/vitessio/vitess/blob/master/helm/release.sh on a new release (currently running on my machine since I forgot to run it with 9.0)

@derekperkins
Copy link
Member

I put more explanation on this issue #7399

@deepthi
Copy link
Member Author

deepthi commented Feb 2, 2021

@derekperkins I'd like this merge this PR as-is. Can you approve?
I have opened #7433 to track the docker related issues so that we can decide whether and how to fix them holistically.

@deepthi
Copy link
Member Author

deepthi commented Feb 2, 2021

@derekperkins I'd like this merge this PR as-is. Can you approve?

Actually it makes sense to revert the change to vtctlclient/Dockerfile to use k8s now that we have 9.0 k8s images on dockerhub. So I have done that.

… branch instead of always using latest

Signed-off-by: deepthi <deepthi@planetscale.com>
@derekperkins derekperkins merged commit b63e92d into vitessio:release-9.0 Feb 3, 2021
@jmoldow
Copy link
Contributor

jmoldow commented Feb 3, 2021

@deepthi should this also go into HEAD? I just noticed this was to release-9.0.

@deepthi
Copy link
Member Author

deepthi commented Feb 3, 2021

Yes, it should. I’ll create a cherry-pick PR.

@shlomi-noach shlomi-noach deleted the ds-90-k8s-dockerfiles branch February 3, 2021 07:21
@askdba askdba added this to the v10.0 milestone Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants