Build protos via docker #6548

ajm188 · 2020-08-10T03:13:30Z

This PR creates a dockerfile and build script for generating the go code from the .proto files.

I was playing around with the protos locally when I discovered that make proto did not work with the way I had cloned vitess (I think) (errors like /bin/bash: line 0: cd: /../../../src: No such file or directory).

After poking at it for a bit, I ended up deciding (but let me know if you think differently!) that going with a dockerized solution would probably be simpler to maintain. The main benefits I see are that we pin the version of protoc (we could also pin protoc-gen-go and goimports if we wanted) and that it doesn't rely on VTTOP or VTROOT in any way; you just need a vitess checkout and docker installed.

Testing:

❯ make proto
Mon Aug 10 13:41:59 EDT 2020: Compiling proto definitions
Sending build context to Docker daemon  210.3MB
Step 1/8 : FROM golang:1.14-buster
 ---> baaca3151cdb
Step 2/8 : RUN apt-get update &&     DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends         build-essential         ca-certificates         curl         unzip         wget
 ---> Using cache
 ---> b5fc7362d68c
Step 3/8 : ARG PROTOC_GEN_GO_VERSION=v1.3.1
 ---> Using cache
 ---> 4c3b0629bd2f
Step 4/8 : RUN mkdir proto-builder &&     cd proto-builder &&     go mod init proto-builder &&     GO111MODULE=on go get         golang.org/x/tools/cmd/goimports         github.com/golang/protobuf/protoc-gen-go@${PROTOC_GEN_GO_VERSION}
 ---> Using cache
 ---> 1eba290a1f52
Step 5/8 : ARG PROTOC_VERSION=3.12.4
 ---> Using cache
 ---> 21ade038f4b4
Step 6/8 : RUN curl -LO https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOC_VERSION}/protoc-${PROTOC_VERSION}-linux-x86_64.zip &&     unzip protoc-${PROTOC_VERSION}-linux-x86_64.zip -d /protoc &&     mv /protoc/bin/protoc /usr/local/bin/ &&     rm -rf protoc-${PROTOC_VERSION}-linux-x86_64.zip
 ---> Using cache
 ---> c30391a299f6
Step 7/8 : ADD proto proto
 ---> Using cache
 ---> 1a616f2d15ad
Step 8/8 : CMD [ "./proto/build.sh" ]
 ---> Using cache
 ---> 0fe79cf99c36
Successfully built 0fe79cf99c36
Successfully tagged proto-builder:latest
Building automation.proto ...
Building automationservice.proto ...
Building binlogdata.proto ...
Building binlogservice.proto ...
Building logutil.proto ...
Building mysqlctl.proto ...
Building query.proto ...
Building queryservice.proto ...
Building replicationdata.proto ...
Building tableacl.proto ...
Building tabletmanagerdata.proto ...
Building tabletmanagerservice.proto ...
Building throttlerdata.proto ...
Building throttlerservice.proto ...
Building topodata.proto ...
Building vschema.proto ...
Building vtctldata.proto ...
Building vtctlservice.proto ...
Building vtgate.proto ...
Building vtgateservice.proto ...
Building vtrpc.proto ...
Building vttest.proto ...
Building vttime.proto ...
Building vtworkerdata.proto ...
Building vtworkerservice.proto ...
Building workflow.proto ...

I'm not sure what version of protoc was used to generate these last time, because I definitely see changes, vtgate.pb.go, for example. I would guess pinning the protoc version lower could let us merge this change without impacting the protos (and then later we can bump the version and update the protos :D)

I pinned proto-gen-go to 1.3.2 (found here), and now the only diff is the following in go/vt/proto/binlogdata/binlogdata.pb.go:

diff --git a/go/vt/proto/binlogdata/binlogdata.pb.go b/go/vt/proto/binlogdata/binlogdata.pb.go
index 5df50ab9d..75a2108d4 100644
--- a/go/vt/proto/binlogdata/binlogdata.pb.go
+++ b/go/vt/proto/binlogdata/binlogdata.pb.go
@@ -1383,9 +1383,6 @@ func (m *VEvent) GetJournal() *Journal {
 
 func (m *VEvent) GetDml() string {
        if m != nil {
-               if m.Statement != "" {
-                       return m.Statement
-               }
                return m.Dml
        }
        return ""

With this change, you can build build protos from the root of a vitess checkout, without needing to have it checked out to "vitess.io/vitess" somewhere on your filesystem and being two directories up. Signed-off-by: Andrew Mason <amason@slack-corp.com>

Signed-off-by: Andrew Mason <amason@slack-corp.com>

dkhenry · 2020-08-12T01:45:17Z

Makefile

@@ -149,20 +149,14 @@ PROTO_SRC_NAMES = $(basename $(notdir $(PROTO_SRCS)))
 PROTO_GO_OUTS = $(foreach name, $(PROTO_SRC_NAMES), go/vt/proto/$(name)/$(name).pb.go)

 # This rule rebuilds all the go files from the proto definitions for gRPC.
-proto: $(PROTO_GO_OUTS)
-
+proto: $(PROTO_SRCS)


I don't think this should take over the proto target. At a minimum we should keep the old target to build the proto's on the local system if the developer would like to

Are you able to build those protos? The main reason I started making this change is because I could never get make proto to work for me.

I'm happy to leave the original make target alone, I'm just curious.

dkhenry · 2020-08-12T01:53:05Z

Makefile

- goimports -w $(VTROOT)/go/vt/proto/$${name}/$${name}.pb.go; \
- done
+ docker build -t proto-builder -f docker/proto/Dockerfile .
+ docker run --rm --mount type=bind,src=$(shell pwd)/go/vt/proto/,dst=/go/vt/proto/ proto-builder


This is likey why you have to sudo below. Instead of bind mounting just use -v to mount the directory and run the container as the current user.

this mount seems to have the unfortunate effect of making chmod-ing directories to 777

Yeah, I'm going to push up a commit switching to the -v style

dkhenry · 2020-08-12T01:54:23Z

docker/proto/Dockerfile

+RUN curl -LO https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOC_VERSION}/protoc-${PROTOC_VERSION}-linux-x86_64.zip && \
+ unzip protoc-${PROTOC_VERSION}-linux-x86_64.zip -d /protoc && \
+ mv /protoc/bin/protoc /usr/local/bin/ && \
+ rm -rf protoc-${PROTOC_VERSION}-linux-x86_64.zip


I am not sure what the end use of this container would be, but I would support having it be built once and pushed to the vitess repo on docker hub. That would prevent the fetching of these dependencies every time we rebuild proto's

Pushing this up somewhere sounds like a great idea! Is that something that I can do, or does that require a vitess maintainer (I see references to pushing in the Release Instructions, so I don't want to push carelessly).

shlomi-noach · 2020-08-13T15:56:22Z

Definitely works for me, and more reliable and consistent than my protoc execution.

sougou · 2020-08-20T04:57:05Z

Can you check if #6580 solves the problem?

shlomi-noach · 2020-08-20T12:17:40Z

on my machine, #6580 does not work as expected:

shlomi@sn-carbon:~/dev/github/vitessio/vitess$ make proto
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
--go_out: protoc-gen-go: plugins are not supported; use 'protoc --go-grpc_out=...' to generate gRPC
...
(infinite)

Signed-off-by: Andrew Mason <amason@slack-corp.com>

ajm188 · 2020-08-21T02:27:35Z

I believe I've fixed all the permissions issues with the resulting. generated protos, and also moved the docker-based proto generation to a separate make target, which I believe were the two biggest comments on this change.

The main thing I think is outstanding is this comment about pushing the builder image up to a registry.

deepthi · 2020-10-06T17:57:04Z

@ajm188 for some reason GH won't let me re-run the failed tests. can you try?

ajm188 · 2020-10-06T18:20:48Z

Hmmm, it looks like I'm missing the "Re-run all jobs" button, too. Did something change with permissions?

deepthi · 2020-10-06T18:41:38Z

Hmmm, it looks like I'm missing the "Re-run all jobs" button, too. Did something change with permissions?

Nope. I can do that on other PRs just not on this one. I'm going to try close followed by re-open. If that doesn't work maybe you could close this PR and open another one from the same branch.

ajm188 · 2020-10-06T18:59:44Z

I see the button again after you did that.

deepthi · 2020-10-06T19:24:18Z

a few things still to be clarified/resolved:

How would this fit into the dev process for someone who is relying on a local build?
We have to validate that the Makefile changes don't break make proto for environments where it currently works
Decide on a name/tag for the new docker image (vitess/proto?) and how it gets updated when the source files change

ajm188 · 2020-10-21T03:56:15Z

How would this fit into the dev process for someone who is relying on a local build?

Right now, anyone that has a local environment setup that can build the protos should be unaffected. For people that are having issues building protos (@shlomi-noach's issue, for example, is fixed by downgrading his install of protoc-gen-go to v1.23.0) can, assuming they have docker working locally, just switch to make docker-proto rather than change whatever is off in their environment (i.e. if Shlomi needs the newer version of protoc-gen-go for some other project).

(I think eventually we should build everything in docker because it feels simpler to me for newcomers with nothing set up, but that doesn't have to be the case with this change).

We have to validate that the Makefile changes don't break make proto for environments where it currently works

On my branch, I did the following:

❯ find go/vt/proto -iname '*.pb.go' -exec rm -rf {} \;
❯ make proto
❯ git d | cat
diff --git a/go/vt/proto/binlogdata/binlogdata.pb.go b/go/vt/proto/binlogdata/binlogdata.pb.go
index 5df50ab9d..75a2108d4 100644
--- a/go/vt/proto/binlogdata/binlogdata.pb.go
+++ b/go/vt/proto/binlogdata/binlogdata.pb.go
@@ -1383,9 +1383,6 @@ func (m *VEvent) GetJournal() *Journal {
 
 func (m *VEvent) GetDml() string {
        if m != nil {
-               if m.Statement != "" {
-                       return m.Statement
-               }
                return m.Dml
        }
        return ""

That extra diff seems to be added manually in 250caba, so the non-docker version seems to be working fine.

Decide on a name/tag for the new docker image (vitess/proto?) and how it gets updated when the source files change

vitess/proto sounds good to me! As for how it gets updated, I am thinking anytime someone opens a PR that touches something in proto/, they, or the person merging should build+push the image (maybe I should add something in ReleaseInstructions.md for this? Or maybe a separate doc). Once we have the docker images building and pushed (I may need some help, or someone to do it for me to push to vitess/*) we can update the Makefile to use that instead of always building locally.

shlomi-noach · 2020-11-15T11:49:53Z

I'm still unable to make proto and am happy to merge this PR (which adds a make docker-proto). Are there any objections?

shlomi-noach · 2020-11-15T11:50:55Z

I've cleared @dkhenry 's request for change since that change was applied.

change applied

shlomi-noach · 2020-11-15T13:51:00Z

Actually, I'm having trouble with the docker build, too. I've just opened #7041 which seems to work well (for me).

ajm188 · 2020-11-16T00:12:58Z

What issue did you run into?

shlomi-noach · 2020-11-16T07:05:16Z

What issue did you run into?

Sorry, I should have been more detailed. And... now that I came back to this, I can't reproduce and everything seems fine. What I got yesterday are successful builds but strange outputs; that is *.pb.go did not match well my *.proto changes. Perhaps I had some git/merge confusion yesterday.

shlomi-noach · 2021-03-07T15:00:28Z

All, make proto is still broken. I'm using this PR as a place to revive the discussion. Which way do we want to go?

ajm188 · 2021-03-07T16:29:56Z

@shlomi-noach how is make proto broken for you? I've been running source ./dev.env && make proto since #7519 landed and have had no issues.

ajm188 · 2021-03-07T16:31:37Z

There was an issue I was having with the new generator not being findable and I asked @vmg in vitess slack here

shlomi-noach · 2021-03-07T17:14:23Z

Now resolved... thanks to @deepthi.

I had this going on:

$ make proto
Sun 07 Mar 2021 17:26:52 IST: Installing minimal dependencies
skipping protoc install. remove /home/shlomi/dev/github/planetscale/vitess-dopple/dist/vt-protoc-3.6.1 to force re-install.
/usr/bin/etcd
etcd already installed
/home/shlomi/dev/github/planetscale/vitess-dopple/bin/k3s

bootstrap finished - run 'make build' to compile
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
protoc-gen-gofast: program not found or is not executable
--gofast_out: protoc-gen-gofast: Plugin failed with status code 1.
...

and it seems I need to manually run go install github.com/gogo/protobuf/protoc-gen-gofast; it was supposed to run as dependency for make proto, but after running this manually everything seems fine.

ajm188 added 5 commits August 9, 2020 19:31

Add dockerfile for generating go code from protos

8171bd9

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Replace make target with new docker implementation

dfc231e

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Minor path anchor changes

c230a61

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Fix pwd to work in make, use newer mount syntax

cf78b7f

Signed-off-by: Andrew Mason <amason@slack-corp.com>

ajm188 requested review from derekperkins, dkhenry and sougou as code owners August 10, 2020 03:13

Pin version of protoc-gen-go

d57b93c

Signed-off-by: Andrew Mason <amason@slack-corp.com>

ajm188 force-pushed the am_proto_docker branch from 82d1324 to d57b93c Compare August 10, 2020 17:44

Fix permissions post-docker-run

304e6e0

Signed-off-by: Andrew Mason <amason@slack-corp.com>

dkhenry previously requested changes Aug 12, 2020

View reviewed changes

ajm188 added 2 commits August 20, 2020 22:20

Checkout latest Makefile

432381d

Signed-off-by: Andrew Mason <amason@slack-corp.com>

Move to separate make target, fix directory permissions

e3d4636

Signed-off-by: Andrew Mason <amason@slack-corp.com>

deepthi closed this Oct 6, 2020

deepthi reopened this Oct 6, 2020

shlomi-noach mentioned this pull request Nov 15, 2020

fix make proto #7041

Closed

ajm188 closed this Jun 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build protos via docker #6548

Build protos via docker #6548

ajm188 commented Aug 10, 2020 •

edited

Loading

dkhenry Aug 12, 2020

ajm188 Aug 20, 2020 •

edited

Loading

dkhenry Aug 12, 2020

shlomi-noach Aug 13, 2020

ajm188 Aug 20, 2020

dkhenry Aug 12, 2020

ajm188 Aug 20, 2020 •

edited

Loading

shlomi-noach commented Aug 13, 2020

sougou commented Aug 20, 2020

shlomi-noach commented Aug 20, 2020 •

edited

Loading

ajm188 commented Aug 21, 2020

deepthi commented Oct 6, 2020

ajm188 commented Oct 6, 2020

deepthi commented Oct 6, 2020

ajm188 commented Oct 6, 2020

deepthi commented Oct 6, 2020

ajm188 commented Oct 21, 2020

shlomi-noach commented Nov 15, 2020

shlomi-noach commented Nov 15, 2020

shlomi-noach commented Nov 15, 2020

ajm188 commented Nov 16, 2020

shlomi-noach commented Nov 16, 2020

shlomi-noach commented Mar 7, 2021

ajm188 commented Mar 7, 2021

ajm188 commented Mar 7, 2021

shlomi-noach commented Mar 7, 2021

Build protos via docker #6548

Build protos via docker #6548

Conversation

ajm188 commented Aug 10, 2020 • edited Loading

dkhenry Aug 12, 2020

Choose a reason for hiding this comment

ajm188 Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

dkhenry Aug 12, 2020

Choose a reason for hiding this comment

shlomi-noach Aug 13, 2020

Choose a reason for hiding this comment

ajm188 Aug 20, 2020

Choose a reason for hiding this comment

dkhenry Aug 12, 2020

Choose a reason for hiding this comment

ajm188 Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

shlomi-noach commented Aug 13, 2020

sougou commented Aug 20, 2020

shlomi-noach commented Aug 20, 2020 • edited Loading

ajm188 commented Aug 21, 2020

deepthi commented Oct 6, 2020

ajm188 commented Oct 6, 2020

deepthi commented Oct 6, 2020

ajm188 commented Oct 6, 2020

deepthi commented Oct 6, 2020

ajm188 commented Oct 21, 2020

shlomi-noach commented Nov 15, 2020

shlomi-noach commented Nov 15, 2020

shlomi-noach commented Nov 15, 2020

ajm188 commented Nov 16, 2020

shlomi-noach commented Nov 16, 2020

shlomi-noach commented Mar 7, 2021

ajm188 commented Mar 7, 2021

ajm188 commented Mar 7, 2021

shlomi-noach commented Mar 7, 2021

ajm188 commented Aug 10, 2020 •

edited

Loading

ajm188 Aug 20, 2020 •

edited

Loading

ajm188 Aug 20, 2020 •

edited

Loading

shlomi-noach commented Aug 20, 2020 •

edited

Loading