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

v2 emulation over v3 #8407

Merged
merged 11 commits into from
Aug 31, 2017
Merged

v2 emulation over v3 #8407

merged 11 commits into from
Aug 31, 2017

Conversation

heyitsanthony
Copy link
Contributor

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
Copy link
Contributor

gyuho commented Aug 16, 2017

c.f. #6925

Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's document mkPathDepth when this works.

embed/etcd.go Outdated
@@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need \n?

@heyitsanthony
Copy link
Contributor Author

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

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below two lines are redundant?

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

import (
"github.com/coreos/etcd/store"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do takes a v2 request ...

store/metrics.go Outdated
@@ -86,7 +86,10 @@ const (
)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where else do we register readCounter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/cc @xiang90

etcdmain/help.go Outdated
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

xiang90 commented Aug 31, 2017

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

other than that looks good to me.

@heyitsanthony heyitsanthony merged commit 1b19a5c into etcd-io:master Aug 31, 2017
@raoofm raoofm mentioned this pull request Dec 21, 2017
5 tasks
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.

3 participants