*: Split etcd into 3 modules: server, client (c), integration #12204

ptabor · 2020-08-06T22:14:04Z

*: Split etcd into 3 modules:

go.etcd.io/etcd/v3 - server
go.etcd.io/etcd/c/v3 - client code
go.etcd.io/etcd/integration/v3 - integration tests

This was discussed an community meeting on 2020/07/30. The goal is to
minimize number of dependencies that our network customers needs to
carry and clearly define packets that needs to stay backward compatible.

Main files moves into ./c
Clients:
./{client,clientv3}
API protos:
./etcdserver/api/membership/membershippb
./etcdserver/api/v3rpc/rpctypes
./auth/authpb
./mvcc/mvccpb
./etcdserver/etcdserverpb
Integration tests:
./client{v3}/{integration,concurrency} -> ./integration/client{v3}/{integration,concurrency}

Tested using:

  PASSES="unit integration functional e2e" time ./test -v

  ./build

  (go build ./... && go test ./...)
  (cd c; go build ./... && go test ./...)
  (cd integration; go build ./... && go test ./...)

  ./scripts/genproto.sh

  make

  go mod tidy
  go mod vendor

Futher steps to consider:

migrate etcdctl into (c) - client code. This requires spliting some admin funtionalities that operate directly on etcd server files.
move tests/e2e and functional into integration dir

Please see/comment the attached doc

ptabor · 2020-08-06T22:17:03Z

\cc @gyuho
\cc @jpbetz

This is the PR that is separating the client code from the server code - discussed on the last community meeting. Please review.

ptabor · 2020-08-07T18:55:04Z

Travis is flaky on --- FAIL: TestGetTokenWithoutAuth (39.16s) - that is addressed in #12200

\cc @wojtek-t FYI

ptabor · 2020-08-09T09:15:45Z

More about motivation and considerations in modularization of etcd doc

gyuho · 2020-08-11T18:54:30Z

@ptabor Can we have separate commits? It's hard to review :)

ptabor · 2020-08-18T18:29:19Z

@gyuho I will split when I'm back from vacations in a week.
Majority of changes here are file moves and import changes.

Interesting changes. go to:

go.mod files (./go.mod ./c/go.mod. ./integration/go.mod)
./test file

I would appreciate if could take a look at: ./test file and the overall directory layout structure in the meantime.

/ - will contain server code /c - will contain client code /integration - will contain integration tests code

The commit is moving files needed for the client code. The files are having only imports changed.

The files got moved and the imports got changed.

This logic is solelly used by etcdctl and depends on implementation details (files layout) of server so connot be kept in the client module.

pkg/logutil is in general dependency of client code, but zap_raft has dependencies on server code and is used solelly within the server.

ptabor · 2020-08-24T23:45:47Z

@gyuho I splitted the change as requested. PTAL

ptabor · 2020-09-01T08:47:15Z

@gyuho @jpbetz @jingyih @wenjiaswe - please review

jpbetz

I've done a first pass review.

Re: Naming the client module c. For the casual observer, or anyone reading go.mod files, it won't be obvious what this is. If there is good rationale on why we can't just call it client? If this was already discussed somewhere can we link to the discussion in description? Else can summarize alternatives and naming decision somewhere for posterity? The community will ask why we chose the name we did since this is a high visibility change that impacts users for the long term.

jpbetz · 2020-09-01T19:50:47Z

c/.gitignore

@@ -0,0 +1,2 @@
+# git mod vendor will regenerate the directory
+vendor/


We should be consistent with how we vendor across the etcd project.

Historically we've checked in the /vendor directory so that the project can be built after a git clone without requiring a go mod fetch. Kubernetes does the same thing. My rough understanding is that it offers some advantages in build time (no go mod pulls). If we do decide to change how we vendor, we should probably do it separate from this PR.

Also, If we're going to have multiple vendor directories, this is going to get more complicated. Can we have one vendor directory that all the modules share? Also, we might need tooling like kubernetes has to manage multiple go.mod files sanely (update-vendor.sh, verify-vendor.sh, pin-dependency.sh)

I moved the cleanup of vendor directory to a separate PR: #12279
I performed there multiple tests and:

neither "go mod fetch" nor "go mod vendor" is needed. "go build ..." or "./build" works of fresh clients (no $GOPATH)

Performance is highly improved. Even if we wipe $GOPATH/pkg, the "./build" takes <10s without vendor and
~34s with vendor. I think that go mod must download pre-built or pre-analyzed artifacts to have such great improvement.

Multiple modules within etcd or in different repositories would share the same pre-loaded modules.

Please let me know in the PR whether I'm missed any benefits / scenarios to keep the ./vendor dir.

This is very promising. By removing vendor we are adding a build dependency on proxy.golang.org (and requiring a network connection to pull from it). This seems acceptable to me given that the proxy is quite mature at this point. I've added a comment about this on #12279 and nudged a couple maintainers. We can continue the conversation over there.

c/etcdserver/etcdserverpb/raft_internal.pb.go

jpbetz · 2020-09-01T20:06:33Z

scripts/genproto.sh

-	echo "could not find protoc 3.7.1, is it installed + in PATH?"
-	exit 255
-fi
+#if [[ $(protoc --version | cut -f2 -d' ') != "3.7.1" ]]; then


Re-enable or add comment explaining why we don't need this any more?

Re-enabled. Good catch.

Would be happy to add a comment why we need this.
Lookled over changes 3.7..3.12, and there seems nothing go related: https://github.com/protocolbuffers/protobuf/releases?before=v3.7.1.

I assume its to guarantee better repeatability for patch releases.

jpbetz · 2020-09-01T20:09:25Z

test

@@ -50,8 +50,8 @@ USERPKG=${PKG:-}
 COVER=${COVER:-"-cover"}

 # Hack: gofmt ./ will recursively check the .git directory. So use *.go for gofmt.
-IGNORE_PKGS="(vendor/|etcdserverpb|rafttest|gopath.proto|v3lockpb|v3electionpb)"
-INTEGRATION_PKGS="(integration|tests/e2e|contrib|functional)"
+IGNORE_PKGS="(^|/)(vendor|etcdserverpb|rafttest|gopath.proto|v3lockpb|v3electionpb|c)(/|$)"


I don't think really want to exclude the entire c module from gofmt.

test

jpbetz · 2020-09-01T20:13:07Z

test

-						  "${REPO_PATH}/contrib/raftexample")
+			INTEGTESTPKG=("${REPO_PATH}/contrib/raftexample")
+			go test -mod=mod -timeout "${USERTIMEOUT}" -v -cpu "${TEST_CPUS}" "${RUN_ARG}"  "$@" "${INTEGTESTPKG[@]}"
+            		(


nit: indentation of this line is odd, doesn't match 195

etcdserver/api/v3lock/v3lockpb/v3lock.pb.go

jpbetz · 2020-09-01T20:21:54Z

vendor/modules.txt

@@ -27,7 +27,7 @@ github.com/dustin/go-humanize
 github.com/etcd-io/gofail/runtime
 # github.com/fatih/color v1.7.0
 ## explicit
-# github.com/gogo/protobuf v1.2.1
+# github.com/gogo/protobuf v1.2.1 => github.com/gogo/protobuf v1.2.1


Seems unnecessary.

jpbetz · 2020-09-01T20:31:02Z

vendor/modules.txt

@@ -228,3 +256,6 @@ gopkg.in/yaml.v2
 # sigs.k8s.io/yaml v1.1.0
 ## explicit
 sigs.k8s.io/yaml
+# github.com/gogo/protobuf => github.com/gogo/protobuf v1.2.1
+# go.etcd.io/etcd/c/v3 => ./c
+# go.etcd.io/etcd/integration/v3 => ./SHOULD_NOT_BE_DEPENDENCY


Interesting. How does this line get added to modules.txt? Is it added manually or do we have some way to make sure it gets added automatically?

It's a consequence of: 053201d#diff-37aff102a57d3d7b797f152915a6dc16R54 in go.mod

replace ( go.etcd.io/etcd/integration/v3 => ./SHOULD_NOT_BE_DEPENDENCY )

A little bit a hack, but very explicit when the error happens.

ptabor · 2020-09-10T12:25:22Z

Re: Naming the client module "c".

I wrote ideas and motivation for different naming options in this section of the doc:
https://docs.google.com/document/d/19UvKD7by_fEkzLMRi-QNSKEed4kYAW286S3DsNRPDOM/edit#heading=h.rj04ue628kji

Happy to change them - agree that current choice might be sub-optimal.
My intuitions goes toward: go.etcd.io/etcd/client/v3, go.etcd.io/etcd/server/v3, go.etcd.io/etcd/tests/v3

@gyuho @jpbetz @jingyih @cfc4n et al. - please comment (here or in the doc)

jpbetz · 2020-09-10T21:25:22Z

go.etcd.io/etcd/client/v3, go.etcd.io/etcd/server/v3 and go.etcd.io/etcd/tests/v3 look great to me

gyuho · 2020-09-10T21:51:37Z

go.etcd.io/etcd/client/v3, go.etcd.io/etcd/server/v3 and go.etcd.io/etcd/tests/v3 look great to me

+1

Thanks for cleaning this up @ptabor

gyuho · 2020-09-10T21:51:45Z

/cc @philips

philips · 2020-09-12T01:00:33Z

The motivation doc from @ptabor looks great. The overall outcome seems good.

However, I don’t have time to review. But, it looks like @jpbetz is on the case.

Thanks for working on this!

ptabor · 2020-09-12T09:16:08Z

Thank you. I will let you know when its ready for another pass.

I'm creating smaller PRs like:

vendor: Get rid of ./vendor #12279 : getting rid of ./vendor
*: 'go test --short ./...' runs all UNIT tests in <25s #12286 : making go test --short ./... run only unit tests
[not yet ready] - cleanup of ./test file itself

That are preparing this refactoring, so having them merged is a dependency.

sagikazarmark · 2020-09-22T11:24:34Z

@ptabor what's the status of this PR? Can I help?

ptabor · 2020-09-22T11:29:43Z

@ptabor sagikazarmark

#12322 (so also
#12319) are the last 2 PRs, I want to get in before rebaseing this PR,
as they will:

make the changes to ./test smaller
give me confidence I'm not breaking anything as part of this process.

sagikazarmark · 2020-09-22T12:21:48Z

Cool, thanks for the update!

stale · 2020-12-27T19:23:45Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

ptabor · 2021-01-07T12:32:32Z

I'm considering this effort done.
Getting the release of 3.5.0-alpha.0 (#12498) would be a cherry on top of this.

ptabor force-pushed the modularization branch 12 times, most recently from afd2e89 to bbcfb34 Compare August 7, 2020 18:53

ptabor force-pushed the modularization branch 3 times, most recently from f30a5f4 to 9e2fcaf Compare August 7, 2020 22:57

sagikazarmark mentioned this pull request Aug 20, 2020

Upgrade crypt and etcd spf13/viper#961

Closed

ptabor added 9 commits August 25, 2020 00:31

*: Define etcd modules (c/, integration/)

053201d

/ - will contain server code /c - will contain client code /integration - will contain integration tests code

c: Move files that establish the client module

59fa4dc

The commit is moving files needed for the client code. The files are having only imports changed.

integration: Move files that establish "integration" tests module.

5d7252e

The files got moved and the imports got changed.

c: Move and regenerate proto go files.

96c7db9

/: Update scripts for modularization: Makefile, genproto & test

4a6b170

Documentation: Regenerate proto-based documentation.

b0cb040

etcdctl: Move work on snapshot's file code to etcdctl.

a413b3b

This logic is solelly used by etcdctl and depends on implementation details (files layout) of server so connot be kept in the client module.

bill-of-materials: Refresh BOM using ./scripts/updatebom.sh

903b81d

logutil/zap_raft: Migrate zap-raft to a separate package.

5bbd173

pkg/logutil is in general dependency of client code, but zap_raft has dependencies on server code and is used solelly within the server.

ptabor added 4 commits August 25, 2020 01:31

c/clientv3: Extract from integration test a mocked unit logic.

1e421fc

etcdserver: Regenerate internal protos after location change.

d9e24f3

*: Update imports after spliting code to the client module.

ae6e968

vendor: Regenerate vendor directory after modularization

7853404

ptabor force-pushed the modularization branch from 9e2fcaf to 7853404 Compare August 24, 2020 23:39

jpbetz suggested changes Sep 1, 2020

View reviewed changes

ptabor mentioned this pull request Sep 7, 2020

testing/e2e,grpcproxy: Fix: go test --tags "cluster_proxy" -v ./tests/e2e/... #12273

Merged

ptabor marked this pull request as draft September 28, 2020 18:33

sagikazarmark mentioned this pull request Oct 1, 2020

jwt-go library vulnerability spf13/viper#997

Closed

This was referenced Oct 22, 2020

Modularization: Make ./etcd server specific module #12414

Closed

Modularization: Make ./etcd server a module #12417

Merged

stale bot added the stale label Dec 27, 2020

ptabor closed this Jan 7, 2021

zhsj mentioned this pull request Feb 26, 2021

Seperate Volcano to three major repos volcano-sh/volcano#1270

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Split etcd into 3 modules: server, client (c), integration #12204

*: Split etcd into 3 modules: server, client (c), integration #12204

ptabor commented Aug 6, 2020 •

edited

Loading

ptabor commented Aug 6, 2020

ptabor commented Aug 7, 2020

ptabor commented Aug 9, 2020

gyuho commented Aug 11, 2020

ptabor commented Aug 18, 2020

ptabor commented Aug 24, 2020

ptabor commented Sep 1, 2020

jpbetz left a comment •

edited

Loading

jpbetz Sep 1, 2020

ptabor Sep 9, 2020

jpbetz Sep 9, 2020

jpbetz Sep 1, 2020

ptabor Sep 2, 2020

jpbetz Sep 1, 2020

ptabor Sep 10, 2020

jpbetz Sep 1, 2020

ptabor Sep 10, 2020

jpbetz Sep 1, 2020

ptabor Sep 10, 2020

jpbetz Sep 1, 2020

ptabor Sep 10, 2020

ptabor commented Sep 10, 2020 •

edited

Loading

jpbetz commented Sep 10, 2020

gyuho commented Sep 10, 2020

gyuho commented Sep 10, 2020

philips commented Sep 12, 2020

ptabor commented Sep 12, 2020 •

edited

Loading

sagikazarmark commented Sep 22, 2020

ptabor commented Sep 22, 2020

sagikazarmark commented Sep 22, 2020

stale bot commented Dec 27, 2020

ptabor commented Jan 7, 2021

		@@ -0,0 +1,2 @@
		# git mod vendor will regenerate the directory
		vendor/

*: Split etcd into 3 modules: server, client (c), integration #12204

*: Split etcd into 3 modules: server, client (c), integration #12204

Conversation

ptabor commented Aug 6, 2020 • edited Loading

ptabor commented Aug 6, 2020

ptabor commented Aug 7, 2020

ptabor commented Aug 9, 2020

gyuho commented Aug 11, 2020

ptabor commented Aug 18, 2020

ptabor commented Aug 24, 2020

ptabor commented Sep 1, 2020

jpbetz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptabor commented Sep 10, 2020 • edited Loading

jpbetz commented Sep 10, 2020

gyuho commented Sep 10, 2020

gyuho commented Sep 10, 2020

philips commented Sep 12, 2020

ptabor commented Sep 12, 2020 • edited Loading

sagikazarmark commented Sep 22, 2020

ptabor commented Sep 22, 2020

sagikazarmark commented Sep 22, 2020

stale bot commented Dec 27, 2020

ptabor commented Jan 7, 2021

ptabor commented Aug 6, 2020 •

edited

Loading

jpbetz left a comment •

edited

Loading

ptabor commented Sep 10, 2020 •

edited

Loading

ptabor commented Sep 12, 2020 •

edited

Loading