v2 emulation over v3 #8407

heyitsanthony · 2017-08-16T16:54:28Z

Still WIP; rough around the edges and I would like to get discovery.etcd.io working. Interested in general comments.

This patch set:

changes v2http, etcdhttp to accept etcdserver.Server* interfaces instead of *EtcdServer
splits out store tests into whitebox/blackbox tests
implements store.Store using clientv3.Client
adds --experimental-enable-v2v3

Passes most blackbox Store tests. The watch tests fail expecting the 'Action' to be preserved, which would be expensive to emulate. However, v2http doesn't need the exact action, so the returned action can be relaxed to make it easier to emulate.

Currently untested with integration/e2e tests. etcdctl sort of works. Performance with bench.sh is worse on spinning disk than ordinary v2, possibly due to boltdb sync latency.

gyuho · 2017-08-16T17:01:36Z

c.f. #6925

gyuho

Let's document mkPathDepth when this works.

gyuho · 2017-08-16T22:37:48Z

embed/etcd.go

@@ -389,7 +391,13 @@ func (e *Etcd) serve() (err error) {
 	// Start a client server goroutine for each listen address
 	var h http.Handler
 	if e.Config().EnableV2 {
-		h = v2http.NewClientHandler(e.Server, e.Server.Cfg.ReqTimeout())
+		if len(e.Config().ExperimentalEnableV2V3) > 0 {
+			plog.Infof("starting experimental v2 over v3 (TODO: support prefix for real via namespacing)\n")


No need \n?

heyitsanthony · 2017-08-25T07:26:20Z

Works with discovery:

etcd --name discovery \
        --listen-client-urls http://127.0.0.1:12379 \
        --advertise-client-urls http://127.0.0.1:12379 \
        --listen-peer-urls http://127.0.0.1:12380 \
        --initial-advertise-peer-urls http://127.0.0.1:12380 \
        --initial-cluster-token etcd-cluster-1 \
        --initial-cluster 'discovery=http://127.0.0.1:12380' \
        --experimental-enable-v2v3 '/v2/' \
        --initial-cluster-state new &
sleep 1s
discovery.etcd.io -e http://127.0.0.1:12379 -h http://localhost:8087 &
sleep 1s

token=$(curl "http://localhost:8087/new?size=3")

etcd    --name infra1 \
        --discovery "$token" \
        --listen-client-urls http://127.0.0.1:2379 \
        --advertise-client-urls http://127.0.0.1:2379 \
        --listen-peer-urls http://127.0.0.1:2380 \
        --initial-advertise-peer-urls http://127.0.0.1:2380 \
        --initial-cluster-token etcd-cluster-1 \
        --initial-cluster-state new &
etcd    --name infra2 \
        --discovery "$token" \
        --listen-client-urls http://127.0.0.1:22379 \
        --advertise-client-urls http://127.0.0.1:22379 \
        --listen-peer-urls http://127.0.0.1:22380 \
        --initial-advertise-peer-urls http://127.0.0.1:22380 \
        --initial-cluster-token etcd-cluster-1 \
        --initial-cluster-state new &
etcd    --name infra3 \
        --discovery "$token" \
        --listen-client-urls http://127.0.0.1:32379 \
        --advertise-client-urls http://127.0.0.1:32379 \
        --listen-peer-urls http://127.0.0.1:32380 \
        --initial-advertise-peer-urls http://127.0.0.1:32380 \
        --initial-cluster-token etcd-cluster-1 \
        --initial-cluster-state new &

sleep 2s

etcdctl set /abc def

gyuho · 2017-08-25T21:22:32Z

etcdserver/server.go

+// begin serving requests. It must be called before Do or Process.
+// Start must be non-blocking; any long-running server functionality
+// should be implemented in goroutines.
+// Start performs any initialization of the Server necessary for it to


Below two lines are redundant?

gyuho · 2017-08-25T21:25:21Z

etcdserver/v2_server.go

@@ -15,41 +15,86 @@
 package etcdserver

 import (
+	"github.com/coreos/etcd/store"


nits: move this around pb "github.com...?

xiang90 · 2017-08-28T22:48:24Z

etcdserver/server.go

-	// ID returns the ID of the Server.
+type ServerV2 interface {
+	Server
+	// Do takes a request and attempts to fulfill it, returning a Response.


Do takes a v2 request ...

gyuho · 2017-08-30T22:33:04Z

store/metrics.go

@@ -86,7 +86,10 @@ const (
 )

 func init() {
-	prometheus.MustRegister(readCounter)
+	if prometheus.Register(readCounter) != nil {
+		// Tests will try to double register, so ignore.


Where else do we register readCounter?

It's from having tests using both store_test and store packages; it will init once for store and once for store_test. I'll clarify the comment

There were also problems from v2v3 using the vendored store path and the tests using the unvendored path.

gyuho

Looks good to me. Since it's experimental, seems safe to merge. Are those failures related to this PR?

/cc @xiang90

xiang90 · 2017-08-31T04:33:47Z

etcdmain/help.go

@@ -183,5 +183,7 @@ auth flags:
 experimental flags:
 	--experimental-corrupt-check-time '0s'
 	        duration of time between cluster corruption check passes.
+	--experimental-enable-v2v3 ''
+		store v2 keys under the given v3 prefix instead of v2 backend.


serve v2 requests through v3 backend?

we not only store v2 keys under v3, but also get keys from v3. this sounds that users can still get v2 keys from the previous v2 store.

xiang90 · 2017-08-31T04:34:49Z

@heyitsanthony can you create an umbrella issue to track what has not been done?

other than that looks good to me.

…aces

Makes interfaces more reusable.

Changes main store tests to use a timeout select instead of expecting events to be immediately posted before returning.

heyitsanthony added the WIP label Aug 16, 2017

gyuho added the kind/release-note label Aug 16, 2017

gyuho reviewed Aug 16, 2017

View reviewed changes

heyitsanthony force-pushed the v2v3 branch 2 times, most recently from c686ad3 to 8de76c1 Compare August 25, 2017 07:24

heyitsanthony removed the WIP label Aug 25, 2017

heyitsanthony force-pushed the v2v3 branch 2 times, most recently from 84b4e41 to 662636d Compare August 25, 2017 21:20

gyuho reviewed Aug 25, 2017

View reviewed changes

heyitsanthony force-pushed the v2v3 branch 2 times, most recently from 1856020 to 2d6210a Compare August 28, 2017 19:35

xiang90 reviewed Aug 28, 2017

View reviewed changes

heyitsanthony force-pushed the v2v3 branch from 2d6210a to 1698b81 Compare August 29, 2017 16:28

gyuho reviewed Aug 30, 2017

View reviewed changes

gyuho approved these changes Aug 30, 2017

View reviewed changes

xiang90 reviewed Aug 31, 2017

View reviewed changes

heyitsanthony force-pushed the v2v3 branch 2 times, most recently from 1db5cdb to cd7df67 Compare August 31, 2017 18:45

Anthony Romano added 8 commits August 31, 2017 11:47

etcdhttp, v2http, etcdserver: use etcdserver.{Server,ServerV2} interf…

1d3afd4

…aces

etcdserver: refactor v2 request processing

758c3c0

Makes interfaces more reusable.

etcdctl3: update to use RequestV2 instead of Request

525fbba

v2v3: ServerV2 backed by clientv3

8091be6

store: separate tests that need Store from those needing *store

cab7572

store: test v2v3 store

75eb05a

Changes main store tests to use a timeout select instead of expecting events to be immediately posted before returning.

embed: support experimental v2v3 proxy option

5d66929

etcdmain: add command line flag to etcdmain

a0adee5

Anthony Romano added 2 commits August 31, 2017 11:47

e2e: test v2v3 emulation

e9cf07f

hack/benchmark: update bench.sh to match procfile

d4b8193

heyitsanthony force-pushed the v2v3 branch from cd7df67 to 32bfd9e Compare August 31, 2017 19:23

test: add v2v3 store tests to integration and cov passes

32bfd9e

heyitsanthony merged commit 1b19a5c into etcd-io:master Aug 31, 2017

raoofm mentioned this pull request Dec 21, 2017

v2v3: stabilize v2 emulation #8473

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v2 emulation over v3 #8407

v2 emulation over v3 #8407

heyitsanthony commented Aug 16, 2017

gyuho commented Aug 16, 2017

gyuho left a comment

gyuho Aug 16, 2017

heyitsanthony commented Aug 25, 2017

gyuho Aug 25, 2017

gyuho Aug 25, 2017

xiang90 Aug 28, 2017

gyuho Aug 30, 2017

heyitsanthony Aug 30, 2017

heyitsanthony Aug 30, 2017

gyuho left a comment

xiang90 Aug 31, 2017

xiang90 commented Aug 31, 2017

v2 emulation over v3 #8407

v2 emulation over v3 #8407

Conversation

heyitsanthony commented Aug 16, 2017

gyuho commented Aug 16, 2017

gyuho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyitsanthony commented Aug 25, 2017

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

gyuho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang90 commented Aug 31, 2017