-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add vtadmin binary to release and Docker images #9405
Conversation
Signed-off-by: Adam Jedro <adamjedro@gmail.com>
This looks good to me, and I appreciate you taking the time to do this! 💖 That said, I'm definitely not the person to sign off on go releases -- @ajm188 @deepthi @rohit-nayak-ps would one of you mind signing off on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all docker image changes we like to see a before and after size to make sure it is not getting too bloated. vtadmin will be a core component so we eventually need to get it in regardless of size, but it will be nice to see how much bigger the image gets.
@@ -50,6 +50,7 @@ COPY --from=base /vt/bin/vtgate /vt/bin/ | |||
COPY --from=base /vt/bin/vttablet /vt/bin/ | |||
COPY --from=base /vt/bin/vtworker /vt/bin/ | |||
COPY --from=base /vt/bin/vtbackup /vt/bin/ | |||
COPY --from=base /vt/bin/vtadmin /vt/bin/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you using the k8s image? The standard image is vitess/lite
. You are already adding vtadmin
to the make install
target, that gets it into all the lite
images. This change should not be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I publish all the individual component images from the k8s
image, so IMO this should stay. There is probably an additional step required to copy out the files needed for the web. This step COPY --from=base $VTROOT/web /vt/web/
is what copies them for the existing UI, so I'd expect to see something similar for vtadmin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once this gets merged and we come up with an appropriate Dockerfile, I'll add a new directory and vtadmin image repo on Docker Hub that approximates the existing vtctld. https://github.com/vitessio/vitess/blob/main/docker/k8s/vtctld/Dockerfile
Side note: there's an open issue for consolidating images, but for now we still need to support people using lite
and the component images. #7433
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derekperkins
I have added /web/vtadmin
to the lite image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw there is a difference between vtadmin and orchestrator web files: orchestrator stores js/css files in final form in the repo, however, for vtadmin
we copy the whole frontend project that needs to be built by npm run build
- only then we get the final js/css files.
This is not a problem since in my project I use vitess/lite
image as a base builder
and then copy binaries/web files to final images (vtadmin-web, vtorc, vtgate, vtadmin-server).
Example of how I do it for vtorc:
ARG BASE_REGISTRY=gcr.io
ARG BASE_IMAGE=distroless/base
ARG BASE_TAG=nonroot
FROM vitess/lite:v12.0.1 AS builder
FROM ${BASE_REGISTRY}/${BASE_IMAGE}:${BASE_TAG}
COPY --from=builder vt/bin/vtorc /vtorc
COPY --from=builder vt/web/orchestrator /web/orchestrator
USER 65532
ENTRYPOINT ["/vtorc"]
HEALTHCHECK NONE
but IMO we should include the final build (js/css etc) somehow.
I see three potential ways of fixing it:
- we copy raw
web/vtadmin
todocker/lite
and it's up to the user ofdocker/lite
to build the frontend. That adds 10M to thedocker/lite
size. This is what is done in that PR for now - https://github.com/vitessio/vitess/pull/9405/files#diff-bdcbcdaa92c26cad86d024c8d1061d14c36e20f67c5c8e577c17e3bbb196c607R54 - we run
npm run build
as a part ofdocker/lite
build and copybuild/
dir into finaldocker/lite
. That adds additional 13M to the size ofdocker/lite
(just checked) - we start building separate
vtadmin-web
image:- use
node
Docker image as a builder - copy all vtadmin web files
- run
npm install
- copy over generated js/css files to the final image (and serve them by
nginx
or another webserver)
- use
cc @doeg ^
I will be happy to send a PR if we decide on the final form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakon89 wow, thanks so much for the thorough notes! I really really appreciate it.
Between the three, I don't have a strong opinion. I'm happy to go with whatever is most ergonomic for Vitess operators! 🤔 The third option is the closest to what we do at Slack, though that doesn't mean it's the best for everyone. @jakon89 @derekperkins do either you have a preference?
P.S. There is definitely work to be done to make the front-end build smaller; the TypeScript generated from the protobufs is particularly huge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we follow the same pattern as we do for Golang binaries, we should do #2. I'm fine with that. Not sure if you want to do that as a separate PR, or include it in this one. If this PR is complete, it LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deepthi let's merge that, I will implement building vtadmin-ui in new PR. Thanks
Hey @deepthi The binary alone is 52M on linux:
and web files take 10M:
Docker image before:
and after:
|
Signed-off-by: Adam Jedro <adamjedro@gmail.com>
Not sure why all the CI actions got canceled. I re-ran a unit test and a cluster test before merging. |
@@ -51,6 +51,7 @@ ENV PATH $VTROOT/bin:$PATH | |||
# Copy artifacts from builder layer. | |||
COPY --from=builder --chown=vitess:vitess /vt/install /vt | |||
COPY --from=builder --chown=vitess:vitess /vt/src/vitess.io/vitess/web/orchestrator /vt/web/orchestrator | |||
COPY --from=builder --chown=vitess:vitess /vt/src/vitess.io/vitess/web/vtadmin /vt/web/vtadmin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this part was not included in mysql8?
@deepthi we are building separate images |
Signed-off-by: Adam Jedro adamjedro@gmail.com
Description
This adds
vtadmin
binary (server) to the release package and Docker images. I would like to runvtadmin
for qa/prod clusters but unless binary is in the official package I am not allowed to do it.Related Issue(s)
Checklist
Deployment Notes