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

vendor: Get rid of ./vendor #12279

Merged
merged 6 commits into from
Sep 17, 2020
Merged

vendor: Get rid of ./vendor #12279

merged 6 commits into from
Sep 17, 2020

Conversation

ptabor
Copy link
Contributor

@ptabor ptabor commented Sep 9, 2020

  • Updated scripts and documentation to not recommend vendoring.
  • Implemented best practices for tools installation.

Performed multiple tests to confirm its not breaking any workflows and
has no negative performance impact:

  1. PASSES="fmt unit integration e2e functional" ./test
  2. ./scripts/updatebom.sh
  3. ./scripts/updatedep.sh
  4. ./scripts/genproto.sh - works - ca be simplified - in follow up PR
  5. Installation without explicit GOPATH:
% unset GOPATH
% [sudo] rm -rf ~/go
% git clone https://github.com/etcd-io/etcd.git
% time ./build
go: downloading google.golang.org/grpc v1.26.0
go: downloading github.com/jonboulle/clockwork v0.1.0
go: downloading github.com/prometheus/client_golang v1.0.0
go: downloading github.com/soheilhy/cmux v0.1.4
go: downloading github.com/gogo/protobuf v1.2.1
go: downloading sigs.k8s.io/yaml v1.1.0
go: downloading golang.org/x/time v0.0.0-20180412165947-fbb02b2291d2
go: downloading github.com/golang/groupcache v0.0.0-20160516000752-02826c3e7903
go: downloading go.etcd.io/bbolt v1.3.5
go: downloading go.uber.org/zap v1.15.0
go: downloading golang.org/x/crypto v0.0.0-20191002192127-34f69633bfdc
go: downloading github.com/golang/protobuf v1.3.2
go: downloading github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8
go: downloading github.com/beorn7/perks v1.0.0
go: downloading github.com/dustin/go-humanize v0.0.0-20171111073723-bb3d318650d4
go: downloading github.com/coreos/go-systemd/v22 v22.0.0
go: downloading gopkg.in/yaml.v2 v2.2.2
go: downloading github.com/coreos/go-semver v0.2.0
go: downloading github.com/sirupsen/logrus v1.4.2
go: downloading golang.org/x/net v0.0.0-20190813141303-74dc4d7220e7
go: downloading github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0
go: downloading github.com/google/uuid v1.0.0
go: downloading github.com/modern-go/reflect2 v1.0.1
go: downloading github.com/prometheus/common v0.4.1
go: downloading github.com/spf13/cobra v0.0.3
go: downloading github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2
go: downloading github.com/gorilla/websocket v0.0.0-20170926233335-4201258b820c
go: downloading github.com/spf13/pflag v1.0.1
go: downloading github.com/json-iterator/go v1.1.7
go: downloading github.com/dgrijalva/jwt-go v3.2.0+incompatible
go: downloading github.com/google/btree v1.0.0
go: downloading go.uber.org/atomic v1.6.0
go: downloading github.com/prometheus/procfs v0.0.2
go: downloading go.uber.org/multierr v1.5.0
go: downloading github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd
go: downloading golang.org/x/sys v0.0.0-20200202164722-d101bd2416d5
go: downloading github.com/grpc-ecosystem/grpc-gateway v1.9.5
go: downloading github.com/grpc-ecosystem/go-grpc-middleware v1.0.1-0.20190118093823-f849b5445de4
go: downloading github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4
go: downloading google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55
go: downloading github.com/matttproud/golang_protobuf_extensions v1.0.1
go: downloading golang.org/x/text v0.3.3
go: downloading github.com/olekukonko/tablewriter v0.0.0-20170122224234-a0225b3f23b5
go: downloading github.com/bgentry/speakeasy v0.1.0
go: downloading gopkg.in/cheggaaa/pb.v1 v1.0.25
go: downloading github.com/urfave/cli v1.20.0
go: downloading github.com/mattn/go-runewidth v0.0.2
./build  8.22s user 2.31s system 117% cpu 8.961 total
  1. Rebuild without changes:
% time ./build
./build  1.43s user 0.83s system 168% cpu 1.336 total
  1. Instantation of vendor directory (assuming ./build loaded them to
    $GOPATH/pkg):
time go mod vendor
go: downloading github.com/inconshreveable/mousetrap v1.0.0
go: downloading github.com/cockroachdb/datadriven v0.0.0-20190809214429-80d97fb3cbaa
go: downloading github.com/creack/pty v1.1.11
go: downloading github.com/etcd-io/gofail v0.0.0-20190801230047-ad7f989257ca
go: downloading github.com/konsorten/go-windows-terminal-sequences v1.0.1
go mod vendor  0.51s user 0.44s system 110% cpu 0.861 total
  1. Fresh instantation of vendor:
% rm -rf vendor
% [sudo] rm -rf ~/go

% time go mod vendor
go: downloading github.com/coreos/go-systemd/v22 v22.0.0
go: downloading github.com/spf13/cobra v0.0.3
go: downloading github.com/prometheus/client_golang v1.0.0
go: downloading golang.org/x/net v0.0.0-20190813141303-74dc4d7220e7
go: downloading github.com/dustin/go-humanize v0.0.0-20171111073723-bb3d318650d4
go: downloading github.com/gogo/protobuf v1.2.1
go: downloading sigs.k8s.io/yaml v1.1.0
go: downloading google.golang.org/grpc v1.26.0
go: downloading github.com/urfave/cli v1.20.0
go: downloading go.uber.org/zap v1.15.0
go: downloading github.com/spf13/pflag v1.0.1
go: downloading github.com/soheilhy/cmux v0.1.4
go: downloading github.com/json-iterator/go v1.1.7
go: downloading github.com/coreos/go-semver v0.2.0
go: downloading github.com/prometheus/common v0.4.1
go: downloading github.com/prometheus/procfs v0.0.2
go: downloading go.uber.org/atomic v1.6.0
go: downloading github.com/olekukonko/tablewriter v0.0.0-20170122224234-a0225b3f23b5
go: downloading github.com/golang/protobuf v1.3.2
go: downloading github.com/cockroachdb/datadriven v0.0.0-20190809214429-80d97fb3cbaa
go: downloading github.com/grpc-ecosystem/go-grpc-middleware v1.0.1-0.20190118093823-f849b5445de4
go: downloading github.com/modern-go/reflect2 v1.0.1
go: downloading github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd
go: downloading go.uber.org/multierr v1.5.0
go: downloading github.com/creack/pty v1.1.11
go: downloading github.com/mattn/go-runewidth v0.0.2
go: downloading github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0
go: downloading golang.org/x/crypto v0.0.0-20191002192127-34f69633bfdc
go: downloading golang.org/x/sys v0.0.0-20200202164722-d101bd2416d5
go: downloading github.com/jonboulle/clockwork v0.1.0
go: downloading gopkg.in/yaml.v2 v2.2.2
go: downloading github.com/etcd-io/gofail v0.0.0-20190801230047-ad7f989257ca
go: downloading github.com/grpc-ecosystem/grpc-gateway v1.9.5
go: downloading github.com/google/btree v1.0.0
go: downloading google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55
go: downloading github.com/beorn7/perks v1.0.0
go: downloading github.com/dgrijalva/jwt-go v3.2.0+incompatible
go: downloading github.com/google/uuid v1.0.0
go: downloading golang.org/x/time v0.0.0-20180412165947-fbb02b2291d2
go: downloading github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8
go: downloading go.etcd.io/bbolt v1.3.5
go: downloading golang.org/x/text v0.3.3
go: downloading gopkg.in/cheggaaa/pb.v1 v1.0.25
go: downloading github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2
go: downloading github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4
go: downloading github.com/inconshreveable/mousetrap v1.0.0
go: downloading github.com/gorilla/websocket v0.0.0-20170926233335-4201258b820c
go: downloading github.com/matttproud/golang_protobuf_extensions v1.0.1
go: downloading github.com/bgentry/speakeasy v0.1.0
go: downloading github.com/golang/groupcache v0.0.0-20160516000752-02826c3e7903
go: downloading github.com/sirupsen/logrus v1.4.2
go: downloading github.com/konsorten/go-windows-terminal-sequences v1.0.1
go mod vendor  3.62s user 1.30s system 127% cpu 3.854 total
  1. Size of the repository - before: 39M, after: 18M

Before:

% time git clone https://github.com/etcd-io/etcd.git
Cloning into 'etcd'...
remote: Enumerating objects: 97872, done.
remote: Total 97872 (delta 0), reused 0 (delta 0), pack-reused 97872
Receiving objects: 100% (97872/97872), 58.97 MiB | 20.53 MiB/s, done.
Resolving deltas: 100% (63091/63091), done.
git clone https://github.com/etcd-io/etcd.git  4.66s user 1.02s system 93% cpu 6.068 total

% du -h --exclude .git -d 1
944K    ./clientv3
108K    ./etcdmain
5.4M    ./Documentation
384K    ./security
384K    ./mvcc
28K     ./.github
8.0K    ./version
144K    ./contrib
240K    ./proxy
2.5M    ./etcdserver
112K    ./embed
536K    ./integration
332K    ./tools
116K    ./lease
108K    ./logos
896K    ./tests
960K    ./raft
216K    ./client
52K     ./scripts
100K    ./hack
464K    ./etcdctl
3.0M    ./pkg
620K    ./functional
136K    ./wal
152K    ./auth
21M     ./vendor
39M

After:

% time git clone https://github.com/ptabor/etcd.git -b 20200908-no-vendor
Cloning into 'etcd'...
remote: Enumerating objects: 38, done.
remote: Counting objects: 100% (38/38), done.
remote: Compressing objects: 100% (37/37), done.
remote: Total 98489 (delta 10), reused 8 (delta 1), pack-reused 98451
Receiving objects: 100% (98489/98489), 59.23 MiB | 21.26 MiB/s, done.
Resolving deltas: 100% (63572/63572), done.
git clone https://github.com/ptabor/etcd.git -b 20200908-no-vendor  5.56s user 1.05s system 105% cpu 6.260 total

% du -h --exclude .git -d 1
944K    ./clientv3
108K    ./etcdmain
5.4M    ./Documentation
384K    ./security
384K    ./mvcc
28K     ./.github
8.0K    ./version
144K    ./contrib
240K    ./proxy
2.5M    ./etcdserver
112K    ./embed
536K    ./integration
332K    ./tools
116K    ./lease
108K    ./logos
896K    ./tests
960K    ./raft
216K    ./client
56K     ./scripts
100K    ./hack
464K    ./etcdctl
3.0M    ./pkg
620K    ./functional
136K    ./wal
152K    ./auth
19M     .

@jpbetz
Copy link
Contributor

jpbetz commented Sep 9, 2020

@ptabor These numbers are pretty good. Did we check how TravisCI and Semaphore build times are impacted? That's worth checking.

@gyuho @xiang90 What do you think? The key question here is if we are willing to dropping /vendor and add a dependency on build dependency on proxy.golang.org (and the required network connection to pull from it)?

@ptabor ptabor force-pushed the 20200908-no-vendor branch 3 times, most recently from 765e957 to 47c7d01 Compare September 9, 2020 18:09
@ptabor
Copy link
Contributor Author

ptabor commented Sep 9, 2020

@jpbetz:

I don't see nice chart in Travis showing test duration.

The (without ./vendor) last run (https://travis-ci.com/github/etcd-io/etcd/builds/183602154) took:

 Ran for 23 min 12 sec
 Total time 1 hr 50 min 7 sec

1:47h - 1:50h (total) is aligned with other Travis runs in the Master branch:
e.g. https://travis-ci.com/github/etcd-io/etcd/builds/183582829

@ptabor
Copy link
Contributor Author

ptabor commented Sep 9, 2020

@gyuho @xiang90 What do you think? The key question here is if we are willing to dropping /vendor and add a dependency on build dependency on proxy.golang.org (and the required network connection to pull from it)?

Note: The proxy.golang.org dependency is only for the first fetch. Later the modules are cached locally within: ${GOPATH}/pkg/mod directory.
For Travis that (I assume) is stateless, it might mean fetching them on each CI cycle.

@jpbetz
Copy link
Contributor

jpbetz commented Sep 10, 2020

For Travis that (I assume) is stateless, it might mean fetching them on each CI cycle.

Yeah, this I why I'm so interested on performance impact on Travis and semaphore builds.

Also, we need to be sure we are not making builds less repeatable. CI shouldn't be able to fail because a change to some upstream dependency got pulled without any change to the checked in go mod files.

@sagikazarmark
Copy link

Hey folks!

I think this is a great change! Go with modules becoming the new standard seems to be turning away from vendoring.

A couple thoughts:

For Travis that (I assume) is stateless, it might mean fetching them on each CI cycle.

Yep. Based on the build times, it might make sense to cache ${GOPATH}/pkg/mod

CI shouldn't be able to fail because a change to some upstream dependency got pulled without any change to the checked in go mod files.

CI should and actually will fail. If someone moved a tag on an upstream repository for instance, it will fail the sum check that Go does based on the go.sum file. This ensures that upstream dependencies are not tampered with, otherwise a dependency maintainer could just transparently inject different code into your application.

@jpbetz
Copy link
Contributor

jpbetz commented Sep 10, 2020

CI shouldn't be able to fail because a change to some upstream dependency got pulled without any change to the checked in go mod files.

CI should and actually will fail. If someone moved a tag on an upstream repository for instance, it will fail the sum check that Go does based on the go.sum file. This ensures that upstream dependencies are not tampered with, otherwise a dependency maintainer could just transparently inject different code into your application.

Tampering is one problem, which, as you mention, is prevented by the go.sum files (and the proxy, which we use by default). And maybe I'm mis-remembering how go modules works, but isn't there a second problem where mod dependencies are not required to be pinned to a specific git hash or tag, and so can pull the latest of a branch?

@ptabor
Copy link
Contributor Author

ptabor commented Sep 10, 2020 via email

@gyuho
Copy link
Contributor

gyuho commented Sep 10, 2020

Would this work without proxy?

go env -w GOPROXY=direct

@sagikazarmark
Copy link

sagikazarmark commented Sep 11, 2020

And maybe I'm mis-remembering how go modules works, but isn't there a second problem where mod dependencies are not required to be pinned to a specific git hash or tag, and so can pull the latest of a branch?

go build and other commands are actually allowed to update versions, if go.mod is not up-to-date. For example, you forget to commit go.mod. CI would still run, because it can update go.mod, to pull-in the required dependencies. In that case, if a new library requires a higher version of a pre-existing dependency, Go would update that dependency without asking.

To prevent that, you can use the -mod=readonly flag, or even better, set it in the GOFLAGS env var, so you don't forget to apply it everywhere: GOFLAGS=-mod=readonly. It will make the CI fail, complaining about not being able to update go.mod. Then you can run go mod tidy or a similar command to fix the problem locally.

Would this work without proxy?

The proxy actually does two things:

  • Speeds up downloads (considerably)
  • If a tag or a package has been removed, it can still serve it from cache (not sure for how long though)

So the proxy is actually optional.

There is also the sumdb which stores the calculated sums in go.sum. When you add a new dependency (and sumdb is enabled), it will compare the calculated sum with the one in the sumdb and will complain if those don't match. This is to make sure, that when you add a version of a dependency, it's actually the same that others have downloaded previously.

But this is also only required "locally" when you change dependencies, optional in CI I guess.

@ptabor
Copy link
Contributor Author

ptabor commented Sep 11, 2020

@gyuho GOPROXY=direct works. Is slower [50s vs 9s] (as I assume all modules are getting built from scratch)

%unset GOPATH; sudo rm -rf ~/go/pkg
%go env -w GOPROXY=direct
% time ./build -v -x
go: downloading github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0
go: downloading github.com/spf13/cobra v0.0.3
go: downloading google.golang.org/grpc v1.26.0
go: downloading sigs.k8s.io/yaml v1.1.0
go: downloading github.com/json-iterator/go v1.1.7
go: downloading github.com/prometheus/client_golang v1.0.0
go: downloading go.etcd.io/bbolt v1.3.5
go: downloading golang.org/x/crypto v0.0.0-20191002192127-34f69633bfdc
go: downloading github.com/modern-go/reflect2 v1.0.1
go: downloading github.com/golang/protobuf v1.3.2
go: downloading github.com/spf13/pflag v1.0.1
go: downloading go.uber.org/zap v1.15.0
go: downloading github.com/google/uuid v1.0.0
go: downloading github.com/dustin/go-humanize v0.0.0-20171111073723-bb3d318650d4
go: downloading github.com/coreos/go-semver v0.2.0
go: downloading golang.org/x/net v0.0.0-20190813141303-74dc4d7220e7
go: downloading github.com/gogo/protobuf v1.2.1
go: downloading golang.org/x/time v0.0.0-20180412165947-fbb02b2291d2
go: downloading go.uber.org/atomic v1.6.0
go: downloading github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8
go: downloading github.com/sirupsen/logrus v1.4.2
go: downloading github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd
go: downloading github.com/soheilhy/cmux v0.1.4
go: downloading google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55
go: downloading github.com/gorilla/websocket v0.0.0-20170926233335-4201258b820c
go: downloading github.com/grpc-ecosystem/grpc-gateway v1.9.5
go: downloading github.com/beorn7/perks v1.0.0
go: downloading github.com/prometheus/common v0.4.1
go: downloading gopkg.in/yaml.v2 v2.2.2
go: downloading golang.org/x/sys v0.0.0-20200202164722-d101bd2416d5
go: downloading github.com/coreos/go-systemd/v22 v22.0.0
go: downloading github.com/prometheus/procfs v0.0.2
go: downloading github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4
go: downloading github.com/google/btree v1.0.0
go: downloading go.uber.org/multierr v1.5.0
go: downloading github.com/grpc-ecosystem/go-grpc-middleware v1.0.1-0.20190118093823-f849b5445de4
go: downloading github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2
go: downloading github.com/matttproud/golang_protobuf_extensions v1.0.1
go: downloading golang.org/x/text v0.3.3
go: downloading github.com/jonboulle/clockwork v0.1.0
go: downloading github.com/dgrijalva/jwt-go v3.2.0+incompatible
go: downloading github.com/golang/groupcache v0.0.0-20160516000752-02826c3e7903
go: downloading github.com/urfave/cli v1.20.0
go: downloading github.com/bgentry/speakeasy v0.1.0
go: downloading github.com/olekukonko/tablewriter v0.0.0-20170122224234-a0225b3f23b5
go: downloading gopkg.in/cheggaaa/pb.v1 v1.0.25
go: downloading github.com/mattn/go-runewidth v0.0.2
./build -v -x  45.54s user 15.27s system 120% cpu 50.295 total

% time ./build -v                   
./build -v  1.14s user 0.35s system 224% cpu 0.664 total

% sudo rm -rf ~/go/pkg; rm -rf ./bin/*
% go env -w GOPROXY=proxy.golang.org 
% time ./build -v -x
...
./build -v -x  8.06s user 2.04s system 110% cpu 9.160 total

@sagikazarmark
Copy link

as I assume all modules are getting built from scratch

The proxy doesn't cache prebuilt artifacts. The reason it speeds up considerably is caching go.mod files, so you don't have to download the source of every module in the graph if it's not actually a dependency.

@jpbetz
Copy link
Contributor

jpbetz commented Sep 11, 2020

And maybe I'm mis-remembering how go modules works, but isn't there a second problem where mod dependencies are not required to be pinned to a specific git hash or tag, and so can pull the latest of a branch?

go build and other commands are actually allowed to update versions, if go.mod is not up-to-date. For example, you forget to commit go.mod. CI would still run, because it can update go.mod, to pull-in the required dependencies. In that case, if a new library requires a higher version of a pre-existing dependency, Go would update that dependency without asking.

To prevent that, you can use the -mod=readonly flag, or even better, set it in the GOFLAGS env var, so you don't forget to apply it everywhere: GOFLAGS=-mod=readonly. It will make the CI fail, complaining about not being able to update go.mod. Then you can run go mod tidy or a similar command to fix the problem locally.

Huge thanks @sagikazarmark. We should absolutely have a pre submit check that ensures that go.mod is up-to-date and notifies PR authors if they need to update go.mod. @ptabor I don't have a strong opinion about how we do the check, but maybe it would be simplest if we add a new test pass and run it from travisCI?

@ptabor
Copy link
Contributor Author

ptabor commented Sep 11, 2020

@jpbetz @sagikazarmark - presubmit checks implemented. PTAL.

9ba87f9 - We set export GOFLAGS=-mod=readonly in the ./test script, so it never modifies the files (analogous behavior to PASSES="gofmt" check)

c55bef6 - Implements PASSES="mod_tidy" ./test and adds it as part of PASSES="fmt" ./test so CI checks whether 'go mod tidy' is an no-op.

@jpbetz
Copy link
Contributor

jpbetz commented Sep 11, 2020

Joe Betz Márk Sági-Kazár - presubmit checks implemented. PTAL.

Excellent. Thank you for doing this @ptabor!

@jpbetz
Copy link
Contributor

jpbetz commented Sep 11, 2020

Approach LGTM. I haven't done a review of all the changes (excluding the removal of the vendor directory of course). So it would be good if someone would do that. @jingyih do you have the bandwidth?

@jingyih
Copy link
Contributor

jingyih commented Sep 15, 2020

I did a search for -mod=mod and found the following. Is there a reason why we don't need to update these?

.travis.yml:          /bin/bash -c "GO_BUILD_FLAGS='-v -mod=mod' ./build && GOARCH=amd64 PASSES='functional' ./test"
.travis.yml:            && GO_BUILD_FLAGS='-v -mod=mod' GOOS=darwin GOARCH=amd64 ./build \
.travis.yml:            && GO_BUILD_FLAGS='-v -mod=mod' GOOS=windows GOARCH=amd64 ./build \
.travis.yml:            && GO_BUILD_FLAGS='-v -mod=mod' GOARCH=arm ./build \
.travis.yml:            && GO_BUILD_FLAGS='-v -mod=mod' GOARCH=arm64 ./build \
.travis.yml:            && GO_BUILD_FLAGS='-v -mod=mod' GOARCH=ppc64le ./build \
.travis.yml:            && GO_BUILD_FLAGS='-v -mod=mod' GOARCH=s390x ./build"
functional/build:CGO_ENABLED=0 go build -v -mod=mod -installsuffix cgo -ldflags "-s" -o ./bin/etcd-agent ./functional/cmd/etcd-agent
functional/build:CGO_ENABLED=0 go build -v -mod=mod -installsuffix cgo -ldflags "-s" -o ./bin/etcd-proxy ./functional/cmd/etcd-proxy
functional/build:CGO_ENABLED=0 go build -v -mod=mod -installsuffix cgo -ldflags "-s" -o ./bin/etcd-runner ./functional/cmd/etcd-runner
functional/build:CGO_ENABLED=0 go build -v -mod=mod -installsuffix cgo -ldflags "-s" -o ./bin/etcd-tester ./functional/cmd/etcd-tester

@ptabor
Copy link
Contributor Author

ptabor commented Sep 15, 2020

@jingyih

I did a search for -mod=mod and found the following. Is there a reason why we don't need to update these?

...

I updated them. Thank you for finding that. For travis I use --mod=readonly, for just the scripts I use the go-lang default (so mod).
./test script is already using --mod=readonly so no reason to set it explicitly twice.

PTAL

.gitignore Outdated Show resolved Hide resolved
Updated scripts and documentation to not recommend vendoring.
Implemented best practices for tools installation.

Performed multiple tests to confirm its not breaking any workflows and
has no negative performance impact. Rather see 3x speedup.

1. PASSES="fmt unit integration e2e functional" ./test
2. ./scripts/updatebom.sh
3. ./scripts/updatedep.sh
4. ./scripts/genproto.sh - works - ca be simplified - in follow up PR
5. Installation without explicit GOPATH:

```
% unset GOPATH
% [sudo] rm -rf ~/go
% git clone https://github.com/etcd-io/etcd.git
% time ./build
go: downloading google.golang.org/grpc v1.26.0
go: downloading github.com/jonboulle/clockwork v0.1.0
go: downloading github.com/prometheus/client_golang v1.0.0
go: downloading github.com/soheilhy/cmux v0.1.4
go: downloading github.com/gogo/protobuf v1.2.1
go: downloading sigs.k8s.io/yaml v1.1.0
go: downloading golang.org/x/time v0.0.0-20180412165947-fbb02b2291d2
go: downloading github.com/golang/groupcache v0.0.0-20160516000752-02826c3e7903
go: downloading go.etcd.io/bbolt v1.3.5
go: downloading go.uber.org/zap v1.15.0
go: downloading golang.org/x/crypto v0.0.0-20191002192127-34f69633bfdc
go: downloading github.com/golang/protobuf v1.3.2
go: downloading github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8
go: downloading github.com/beorn7/perks v1.0.0
go: downloading github.com/dustin/go-humanize v0.0.0-20171111073723-bb3d318650d4
go: downloading github.com/coreos/go-systemd/v22 v22.0.0
go: downloading gopkg.in/yaml.v2 v2.2.2
go: downloading github.com/coreos/go-semver v0.2.0
go: downloading github.com/sirupsen/logrus v1.4.2
go: downloading golang.org/x/net v0.0.0-20190813141303-74dc4d7220e7
go: downloading github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0
go: downloading github.com/google/uuid v1.0.0
go: downloading github.com/modern-go/reflect2 v1.0.1
go: downloading github.com/prometheus/common v0.4.1
go: downloading github.com/spf13/cobra v0.0.3
go: downloading github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2
go: downloading github.com/gorilla/websocket v0.0.0-20170926233335-4201258b820c
go: downloading github.com/spf13/pflag v1.0.1
go: downloading github.com/json-iterator/go v1.1.7
go: downloading github.com/dgrijalva/jwt-go v3.2.0+incompatible
go: downloading github.com/google/btree v1.0.0
go: downloading go.uber.org/atomic v1.6.0
go: downloading github.com/prometheus/procfs v0.0.2
go: downloading go.uber.org/multierr v1.5.0
go: downloading github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd
go: downloading golang.org/x/sys v0.0.0-20200202164722-d101bd2416d5
go: downloading github.com/grpc-ecosystem/grpc-gateway v1.9.5
go: downloading github.com/grpc-ecosystem/go-grpc-middleware v1.0.1-0.20190118093823-f849b5445de4
go: downloading github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4
go: downloading google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55
go: downloading github.com/matttproud/golang_protobuf_extensions v1.0.1
go: downloading golang.org/x/text v0.3.3
go: downloading github.com/olekukonko/tablewriter v0.0.0-20170122224234-a0225b3f23b5
go: downloading github.com/bgentry/speakeasy v0.1.0
go: downloading gopkg.in/cheggaaa/pb.v1 v1.0.25
go: downloading github.com/urfave/cli v1.20.0
go: downloading github.com/mattn/go-runewidth v0.0.2
./build  8.22s user 2.31s system 117% cpu 8.961 total
```

Before:
```
% git clone https://github.com/etcd-io/etcd.git && cd etcd && time ./build
Cloning into 'etcd'...
remote: Enumerating objects: 97872, done.
remote: Total 97872 (delta 0), reused 0 (delta 0), pack-reused 97872
Receiving objects: 100% (97872/97872), 58.97 MiB | 19.85 MiB/s, done.
Resolving deltas: 100% (63091/63091), done.

./build  34.97s user 4.15s system 236% cpu 16.555 total
```

6. Rebuild without changes:

```
% time ./build
./build  1.43s user 0.83s system 168% cpu 1.336 total
```

7. Instantation of vendor directory (assuming ./build loaded them to
$GOPATH/pkg):

```
time go mod vendor
go: downloading github.com/inconshreveable/mousetrap v1.0.0
go: downloading github.com/cockroachdb/datadriven v0.0.0-20190809214429-80d97fb3cbaa
go: downloading github.com/creack/pty v1.1.11
go: downloading github.com/etcd-io/gofail v0.0.0-20190801230047-ad7f989257ca
go: downloading github.com/konsorten/go-windows-terminal-sequences v1.0.1
go mod vendor  0.51s user 0.44s system 110% cpu 0.861 total
```

8. Fresh instantation of vendor:

```
% rm -rf vendor
% [sudo] rm -rf ~/go

% time go mod vendor
go: downloading github.com/coreos/go-systemd/v22 v22.0.0
go: downloading github.com/spf13/cobra v0.0.3
go: downloading github.com/prometheus/client_golang v1.0.0
go: downloading golang.org/x/net v0.0.0-20190813141303-74dc4d7220e7
go: downloading github.com/dustin/go-humanize v0.0.0-20171111073723-bb3d318650d4
go: downloading github.com/gogo/protobuf v1.2.1
go: downloading sigs.k8s.io/yaml v1.1.0
go: downloading google.golang.org/grpc v1.26.0
go: downloading github.com/urfave/cli v1.20.0
go: downloading go.uber.org/zap v1.15.0
go: downloading github.com/spf13/pflag v1.0.1
go: downloading github.com/soheilhy/cmux v0.1.4
go: downloading github.com/json-iterator/go v1.1.7
go: downloading github.com/coreos/go-semver v0.2.0
go: downloading github.com/prometheus/common v0.4.1
go: downloading github.com/prometheus/procfs v0.0.2
go: downloading go.uber.org/atomic v1.6.0
go: downloading github.com/olekukonko/tablewriter v0.0.0-20170122224234-a0225b3f23b5
go: downloading github.com/golang/protobuf v1.3.2
go: downloading github.com/cockroachdb/datadriven v0.0.0-20190809214429-80d97fb3cbaa
go: downloading github.com/grpc-ecosystem/go-grpc-middleware v1.0.1-0.20190118093823-f849b5445de4
go: downloading github.com/modern-go/reflect2 v1.0.1
go: downloading github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd
go: downloading go.uber.org/multierr v1.5.0
go: downloading github.com/creack/pty v1.1.11
go: downloading github.com/mattn/go-runewidth v0.0.2
go: downloading github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0
go: downloading golang.org/x/crypto v0.0.0-20191002192127-34f69633bfdc
go: downloading golang.org/x/sys v0.0.0-20200202164722-d101bd2416d5
go: downloading github.com/jonboulle/clockwork v0.1.0
go: downloading gopkg.in/yaml.v2 v2.2.2
go: downloading github.com/etcd-io/gofail v0.0.0-20190801230047-ad7f989257ca
go: downloading github.com/grpc-ecosystem/grpc-gateway v1.9.5
go: downloading github.com/google/btree v1.0.0
go: downloading google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55
go: downloading github.com/beorn7/perks v1.0.0
go: downloading github.com/dgrijalva/jwt-go v3.2.0+incompatible
go: downloading github.com/google/uuid v1.0.0
go: downloading golang.org/x/time v0.0.0-20180412165947-fbb02b2291d2
go: downloading github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8
go: downloading go.etcd.io/bbolt v1.3.5
go: downloading golang.org/x/text v0.3.3
go: downloading gopkg.in/cheggaaa/pb.v1 v1.0.25
go: downloading github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2
go: downloading github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4
go: downloading github.com/inconshreveable/mousetrap v1.0.0
go: downloading github.com/gorilla/websocket v0.0.0-20170926233335-4201258b820c
go: downloading github.com/matttproud/golang_protobuf_extensions v1.0.1
go: downloading github.com/bgentry/speakeasy v0.1.0
go: downloading github.com/golang/groupcache v0.0.0-20160516000752-02826c3e7903
go: downloading github.com/sirupsen/logrus v1.4.2
go: downloading github.com/konsorten/go-windows-terminal-sequences v1.0.1
go mod vendor  3.62s user 1.30s system 127% cpu 3.854 total
```

9. Size of the repository - before: 39M, after: 18M

Before:

```
% time git clone https://github.com/etcd-io/etcd.git
Cloning into 'etcd'...
remote: Enumerating objects: 97872, done.
remote: Total 97872 (delta 0), reused 0 (delta 0), pack-reused 97872
Receiving objects: 100% (97872/97872), 58.97 MiB | 20.53 MiB/s, done.
Resolving deltas: 100% (63091/63091), done.
git clone https://github.com/etcd-io/etcd.git  4.66s user 1.02s system 93% cpu 6.068 total

% du -h --exclude .git -d 1
944K	./clientv3
108K	./etcdmain
5.4M	./Documentation
384K	./security
384K	./mvcc
28K	./.github
8.0K	./version
144K	./contrib
240K	./proxy
2.5M	./etcdserver
112K	./embed
536K	./integration
332K	./tools
116K	./lease
108K	./logos
896K	./tests
960K	./raft
216K	./client
52K	./scripts
100K	./hack
464K	./etcdctl
3.0M	./pkg
620K	./functional
136K	./wal
152K	./auth
21M	./vendor
39M
```

After:
```
% time git clone https://github.com/ptabor/etcd.git -b 20200908-no-vendor
Cloning into 'etcd'...
remote: Enumerating objects: 38, done.
remote: Counting objects: 100% (38/38), done.
remote: Compressing objects: 100% (37/37), done.
remote: Total 98489 (delta 10), reused 8 (delta 1), pack-reused 98451
Receiving objects: 100% (98489/98489), 59.23 MiB | 21.26 MiB/s, done.
Resolving deltas: 100% (63572/63572), done.
git clone https://github.com/ptabor/etcd.git -b 20200908-no-vendor  5.56s user 1.05s system 105% cpu 6.260 total

% du -h --exclude .git -d 1
944K	./clientv3
108K	./etcdmain
5.4M	./Documentation
384K	./security
384K	./mvcc
28K	./.github
8.0K	./version
144K	./contrib
240K	./proxy
2.5M	./etcdserver
112K	./embed
536K	./integration
332K	./tools
116K	./lease
108K	./logos
896K	./tests
960K	./raft
216K	./client
56K	./scripts
100K	./hack
464K	./etcdctl
3.0M	./pkg
620K	./functional
136K	./wal
152K	./auth
19M	.
```
Tested and received as expected:
```
...
go: updates to go.mod needed, disabled by -mod=readonly
```
Added check that ensures that go.mod & go.sum files are up-to-date.
The check verifies whether 'go mod tidy' does not generate any mutations
in these files.
The check can be run on its own:
  PASSES="mod_tidy" ./test
Or as part of "fmt" pass:
  PASSES="fmt" ./test

Examplar outputs:

```
% PASSES="fmt" ./test

Running with TEST_CPUS: 1,2,4
Starting 'fmt' pass at Fri 11 Sep 2020 11:07:54 PM CEST
'shellcheck' started at Fri 11 Sep 2020 11:07:54 PM CEST
'shellcheck' completed at Fri 11 Sep 2020 11:07:54 PM CEST
'markdown_you' started at Fri 11 Sep 2020 11:07:54 PM CEST
'markdown_you' completed at Fri 11 Sep 2020 11:07:54 PM CEST
'goword' started at Fri 11 Sep 2020 11:07:54 PM CEST
'goword' completed at Fri 11 Sep 2020 11:07:54 PM CEST
'gofmt' started at Fri 11 Sep 2020 11:07:54 PM CEST
'gofmt' completed at Fri 11 Sep 2020 11:07:55 PM CEST
'govet' started at Fri 11 Sep 2020 11:07:55 PM CEST
'govet' completed at Fri 11 Sep 2020 11:07:57 PM CEST
'revive' started at Fri 11 Sep 2020 11:07:57 PM CEST
Skipping revive...
'revive' completed at Fri 11 Sep 2020 11:07:57 PM CEST
'license_header' started at Fri 11 Sep 2020 11:07:57 PM CEST
'license_header' completed at Fri 11 Sep 2020 11:07:58 PM CEST
'receiver_name' started at Fri 11 Sep 2020 11:07:58 PM CEST
'receiver_name' completed at Fri 11 Sep 2020 11:07:58 PM CEST
'commit_title' started at Fri 11 Sep 2020 11:07:58 PM CEST
'commit_title' completed at Fri 11 Sep 2020 11:07:58 PM CEST
'mod_tidy' started at Fri 11 Sep 2020 11:07:58 PM CEST
*** /tmp/fileiALKRA_go.mod	2020-09-11 23:07:58.838010716 +0200
--- ./go.mod	2020-09-11 23:07:58.974010922 +0200
***************
*** 29,39 ****
  	github.com/mattn/go-runewidth v0.0.2 // indirect
  	github.com/modern-go/reflect2 v1.0.1
  	github.com/olekukonko/tablewriter v0.0.0-20170122224234-a0225b3f23b5
  	github.com/prometheus/client_golang v1.0.0
  	github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4
- 	github.com/prometheus/common v0.4.1
  	github.com/sirupsen/logrus v1.4.2 // indirect
  	github.com/soheilhy/cmux v0.1.4
  	github.com/spf13/cobra v0.0.3
  	github.com/spf13/pflag v1.0.1
  	github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8
--- 29,38 ----
./go.mod is not in sync with 'go mod tidy'
```

```
% PASSES="mod_tidy" ./test

Running with TEST_CPUS: 1,2,4
Starting 'mod_tidy' pass at Fri 11 Sep 2020 11:09:21 PM CEST
*** /tmp/file9gy4so_go.mod	2020-09-11 23:09:21.166133290 +0200
--- ./go.mod	2020-09-11 23:09:21.286133466 +0200
***************
*** 29,39 ****
  	github.com/mattn/go-runewidth v0.0.2 // indirect
  	github.com/modern-go/reflect2 v1.0.1
  	github.com/olekukonko/tablewriter v0.0.0-20170122224234-a0225b3f23b5
  	github.com/prometheus/client_golang v1.0.0
  	github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4
- 	github.com/prometheus/common v0.4.1
  	github.com/sirupsen/logrus v1.4.2 // indirect
  	github.com/soheilhy/cmux v0.1.4
  	github.com/spf13/cobra v0.0.3
  	github.com/spf13/pflag v1.0.1
  	github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8
--- 29,38 ----
./go.mod is not in sync with 'go mod tidy'
```
@jingyih
Copy link
Contributor

jingyih commented Sep 16, 2020

@ptabor One more question, since we are getting rid of vendor folder, do we want ./scripts/updatedep.sh to copy the dependencies back to vendor folder?

@jingyih
Copy link
Contributor

jingyih commented Sep 16, 2020

@liggitt We are going to get rid of vendor folder in etcd repo. I do not think this affects other repos that depend on etcd (such as kubernetes). Cc'ing you in case I was wrong.

@ptabor
Copy link
Contributor Author

ptabor commented Sep 16, 2020

@jingyih kubernetes depends on go.mod relationship:
https://github.com/kubernetes/kubernetes/blob/fcba5fe4e4eb1b1a2e2b14d2a68037fecd75e3ab/go.mod#L100
and tar.balls.

After modularization and as part of switch of K8S to 3.5 release (when it will happen) I will take care of proper dependency declarations between K8S and etcd.

As we get rid of ./vendor there is no need to update the dependencies.

If anyone needs up-to-date vendor directory locally, getting it is as
simple as:

```go mod vendor```
@ptabor
Copy link
Contributor Author

ptabor commented Sep 16, 2020

@ptabor One more question, since we are getting rid of vendor folder, do we want ./scripts/updatedep.sh to copy the dependencies back to vendor folder?

I removed this script. I kept it just-in-case somebody depended on it locally...
But go mod vendor is actually shorter than: ./scripts/updatedep.sh

@jingyih
Copy link
Contributor

jingyih commented Sep 16, 2020

lgtm
Thanks!

@jpbetz
Copy link
Contributor

jpbetz commented Sep 16, 2020

How are we ensuring we have repeatable builds in CI? I was expecting -mod=readonly to be set somewhere, like in /build.

@sagikazarmark
Copy link

How are we ensuring we have repeatable builds in CI? I was expecting -mod=readonly to be set somewhere, like in /build.

I think it's set here: https://github.com/etcd-io/etcd/pull/12279/files#diff-354f30a63fb0907d4ad57269548329e3R110

@jpbetz
Copy link
Contributor

jpbetz commented Sep 16, 2020

How are we ensuring we have repeatable builds in CI? I was expecting -mod=readonly to be set somewhere, like in /build.

I think it's set here: https://github.com/etcd-io/etcd/pull/12279/files#diff-354f30a63fb0907d4ad57269548329e3R110

Ah, I missed that.

Should we also make readonly the default for ./build? I wouldn't expect developers to want the dependencies to change unless they explicitly ask for it.

@ptabor
Copy link
Contributor Author

ptabor commented Sep 16, 2020

How are we ensuring we have repeatable builds in CI? I was expecting -mod=readonly to be set somewhere, like in /build.

I think it's set here: https://github.com/etcd-io/etcd/pull/12279/files#diff-354f30a63fb0907d4ad57269548329e3R110

Ah, I missed that.

Should we also make readonly the default for ./build? I wouldn't expect developers to want the dependencies to change unless they explicitly ask for it.

I assumed we want to follow golang default behavior. In my opinion its more natural if go build ./... and ./build behaves in a similar way, and for some reason go developers picked convenience&automatization on the development path.
What's the risk if ./build will add some dependency during development ?

Developer will spot it during "git commit" at least and fix if its unintended. As long as current dependency is 'sufficient' go build does not bumps the versions up.

Happy to change if you prefer otherwise.

@jpbetz
Copy link
Contributor

jpbetz commented Sep 16, 2020

I assumed we want to follow golang default behavior. In my opinion its more natural if go build ./... and ./build behaves in a similar way, and for some reason go developers picked convenience&automatization on the development path.

I'd be more inclined to agree if I though the default behavior was a good experience for contributors.

What's the risk if ./build will add some dependency during development ?

Scenario I'm not happy with:

  • developer clones etcd to implement a contribution
  • changes the code they care about
  • builds it to see if it works, picking up various dependency bumps

Some possible outcomes:

  • Developer has to go to the trouble of excluding the dependency changes they didn't intend to pick up (this is more annoying if they're mixed with changes the developer did intend). This adds friction to submitting their PR.
  • Developer isn't paying enough attention and just includes everything that change in their PR, bumping dependencies that have nothing to do with their change. Maintainers have to deal with in review.

Maybe I'm overreacting, but it seems like it would be better to have developers explicitly bump dependencies only when they actually want to like is done in Kubernetes.

Feel free to disagree with me, I just want to bring this up and make sure we've thought it through carefully.

@sagikazarmark
Copy link

sagikazarmark commented Sep 16, 2020

Normally, if someone changes code, dependencies shouldn't change. If they do, that's either a bug in Go (happened quite a few times), or a problem caused by Go version differences or a local Go cache issue. It actually happened a lot in the early days of Go modules, but these days, it works pretty well. So if dependencies change unintentionally, that's not a normal Go behavior or the previous go.mod change was screwed up (in which case developers should still be able to work, until it's fixed upstream).

If Go wants to modify the go.mod file for some reason and -mod=readonly is passed to the go tool, it will simply fail. In terms of user experience this is probably bad, because users should be able to build/debug what's happening and they will simply get rid of that option.

Another scenario when someone submits a PR with an unintended go.mod change is when the developer doesn't give a sh*t, but I feel like it can happen either way, so a default in the build script won't stop them.

So I would say -mod=readonly is probably counterproductive as a default. My experience (when it comes to Go, especially modules) is that users will submit what they think is right, and they will hack until it works for them. Ultimately, it's up to the maintainers to block those PRs until fixed properly.

My two cents.

@jpbetz
Copy link
Contributor

jpbetz commented Sep 17, 2020

If we're confident go build is only going to pick up relevant changes, I'm satisfied. Maybe I'm just jaded by the early go mod days where it seemed to randomly pull in changes for no obvious reason.

LGTM

@jingyih jingyih merged commit 2fee6bd into etcd-io:master Sep 17, 2020
@jingyih
Copy link
Contributor

jingyih commented Sep 17, 2020

@ptabor Could you help add an entry for this change in CHANGELOG-3.5?

ptabor added a commit to ptabor/etcd that referenced this pull request Sep 17, 2020
ptabor added a commit to ptabor/etcd that referenced this pull request Sep 19, 2020
ptabor added a commit to ptabor/etcd that referenced this pull request Sep 22, 2020
ptabor added a commit to ptabor/etcd that referenced this pull request Sep 22, 2020
ptabor added a commit to ptabor/etcd that referenced this pull request Sep 25, 2020
ptabor added a commit to ptabor/etcd that referenced this pull request Sep 25, 2020
xiang90 pushed a commit that referenced this pull request Sep 25, 2020
@ptabor ptabor deleted the 20200908-no-vendor branch October 22, 2020 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants