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

upgrade to etcd/client/v3 #12

Open
wants to merge 7 commits into
base: flatcar-master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
296 changes: 195 additions & 101 deletions lock/etcd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,132 +16,226 @@ package lock

import (
"errors"
"reflect"
"testing"

"go.etcd.io/etcd/client"
pb "go.etcd.io/etcd/api/v3/mvccpb"
client "go.etcd.io/etcd/client/v3"

"golang.org/x/net/context"
Comment on lines +21 to 24
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pb "go.etcd.io/etcd/api/v3/mvccpb"
client "go.etcd.io/etcd/client/v3"
"golang.org/x/net/context"
pb "go.etcd.io/etcd/api/v3/mvccpb"
client "go.etcd.io/etcd/client/v3"
"golang.org/x/net/context"

)

const testGroup = ""

// testEtcdClient is the struct used to mock the `etcd`
// client
type testEtcdClient struct {
err error
resp *client.Response
err error
getResp *client.GetResponse
txn testTxn
Copy link
Member

Choose a reason for hiding this comment

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

Should this be client.Txn type?

}

func (t *testEtcdClient) Get(ctx context.Context, key string, opts *client.GetOptions) (*client.Response, error) {
return t.resp, t.err
// testTxn implements the client.Txn interface
type testTxn struct {
err error
txnSuccess bool
}

func (t *testEtcdClient) Set(ctx context.Context, key, value string, opts *client.SetOptions) (*client.Response, error) {
return t.resp, t.err
func (t testTxn) If(cs ...client.Cmp) client.Txn {
return t
}

func (t *testEtcdClient) Create(ctx context.Context, key, value string) (*client.Response, error) {
return t.resp, t.err
func (t testTxn) Then(ops ...client.Op) client.Txn {
return t
}

func TestEtcdLockClientInit(t *testing.T) {
for i, tt := range []struct {
ee error
want bool
group string
keypath string
}{
{nil, false, "", SemaphorePrefix},
{client.Error{Code: client.ErrorCodeNodeExist}, false, "", SemaphorePrefix},
{client.Error{Code: client.ErrorCodeKeyNotFound}, true, "", SemaphorePrefix},
{errors.New("some random error"), true, "", SemaphorePrefix},
{client.Error{Code: client.ErrorCodeKeyNotFound}, true, "database", "coreos.com/updateengine/rebootlock/groups/database/semaphore"},
{nil, false, "prod/database", "coreos.com/updateengine/rebootlock/groups/prod%2Fdatabase/semaphore"},
} {
elc, got := NewEtcdLockClient(&testEtcdClient{err: tt.ee}, tt.group)
if (got != nil) != tt.want {
t.Errorf("case %d: unexpected error state initializing Client: got %v", i, got)
continue
}

if got != nil {
continue
}

if elc.keypath != tt.keypath {
t.Errorf("case %d: unexpected etcd key path: got %v want %v", i, elc.keypath, tt.keypath)
}
}
func (t testTxn) Else(ops ...client.Op) client.Txn {
return t
}

func (t testTxn) Commit() (*client.TxnResponse, error) {
return &client.TxnResponse{Succeeded: t.txnSuccess}, t.err
}

func (t *testEtcdClient) Get(ctx context.Context, key string, opts ...client.OpOption) (*client.GetResponse, error) {
return t.getResp, t.err
}

func makeResponse(idx int, val string) *client.Response {
return &client.Response{
Node: &client.Node{
Value: val,
ModifiedIndex: uint64(idx),
func (t *testEtcdClient) Txn(ctx context.Context) client.Txn {
return t.txn
}

func TestEtcdLockClientInit(t *testing.T) {
t.Run("Success", func(t *testing.T) {
elc, err := NewEtcdLockClient(&testEtcdClient{
err: nil,
Copy link
Member

Choose a reason for hiding this comment

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

I guess we can make use of zero value here? Or you want nil error to be explicit?

getResp: &client.GetResponse{},
},
}
testGroup,
)
if err != nil {
t.Fatalf("error should be nil, got: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

Similar to errors, it's good to explain what went wrong when you fail the test. For example:

Suggested change
t.Fatalf("error should be nil, got: %v", err)
t.Fatalf("Failed creating new etcd lock client: %v", err)

}

if elc == nil {
t.Fatalf("etcd lock client should not be nil")
}
})
t.Run("Error", func(t *testing.T) {
_, err := NewEtcdLockClient(&testEtcdClient{
txn: testTxn{err: errors.New("connection refused")},
Copy link
Member

Choose a reason for hiding this comment

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

If you set the error here, you should be able to make use of errors.Is() perhaps?

getResp: &client.GetResponse{Count: 0},
},
testGroup,
)
if err == nil {
t.Fatal("error should not be nil")
}

if err.Error() != "unable to init etcd lock client: unable to commit initial transaction: connection refused" {
t.Fatalf("error should be 'unable to init etcd lock client: unable to commit initial transaction: connection refused', got: %v", err)
Comment on lines +94 to +95
Copy link
Member

Choose a reason for hiding this comment

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

The string error could be put in variable at least, to avoid duplication, if we stay on string comparison for errors.

}
})
}

func TestEtcdLockClientGet(t *testing.T) {
for i, tt := range []struct {
ee error
er *client.Response
ws *Semaphore
we bool
}{
// errors returned from etcd
{errors.New("some error"), nil, nil, true},
{client.Error{Code: client.ErrorCodeKeyNotFound}, nil, nil, true},
// bad JSON should cause errors
{nil, makeResponse(0, "asdf"), nil, true},
{nil, makeResponse(0, `{"semaphore:`), nil, true},
// successful calls
{nil, makeResponse(10, `{"semaphore": 1}`), &Semaphore{Index: 10, Semaphore: 1}, false},
{nil, makeResponse(1024, `{"semaphore": 1, "max": 2, "holders": ["foo", "bar"]}`), &Semaphore{Index: 1024, Semaphore: 1, Max: 2, Holders: []string{"foo", "bar"}}, false},
// index should be set from etcd, not json!
{nil, makeResponse(1234, `{"semaphore": 89, "index": 4567}`), &Semaphore{Index: 1234, Semaphore: 89}, false},
} {
elc := &EtcdLockClient{
keyapi: &testEtcdClient{
err: tt.ee,
resp: tt.er,
t.Run("Success", func(t *testing.T) {
elc, err := NewEtcdLockClient(&testEtcdClient{
err: nil,
getResp: &client.GetResponse{
Count: 1,
Kvs: []*pb.KeyValue{
&pb.KeyValue{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
&pb.KeyValue{
{

Key: []byte(SemaphorePrefix),
// index should be set from etcd, not json (backported from legacy test)
Value: []byte(`{"index": 12, "semaphore": 1, "max": 2, "holders": ["foo", "bar"]}`),
Version: 1234,
},
},
},
},
testGroup,
)
if err != nil {
t.Fatalf("error should be nil, got: %v", err)
}

res, err := elc.Get()
if err != nil {
t.Fatalf("error should be nil, got: %v", err)
}

if res.Index != uint64(1234) {
t.Fatalf("index should be 1234, got: %d", res.Index)
}
})
t.Run("SuccessNotFound", func(t *testing.T) {
elc, err := NewEtcdLockClient(&testEtcdClient{
err: nil,
getResp: &client.GetResponse{Count: 0},
},
testGroup,
)
if err != nil {
t.Fatalf("error should be nil, got: %v", err)
}

_, err = elc.Get()
if err == nil {
t.Fatal("error should not be nil")
}

if !errors.Is(err, ErrNotFound) {
t.Fatalf("error should be ErrNotFound, got: %v", err)
}
})
t.Run("ErrorWithMalformedJSON", func(t *testing.T) {
elc, err := NewEtcdLockClient(&testEtcdClient{
err: nil,
getResp: &client.GetResponse{
Count: 1,
Kvs: []*pb.KeyValue{
&pb.KeyValue{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
&pb.KeyValue{
{

Key: []byte(SemaphorePrefix),
// notice the missing `,` in the array
Value: []byte(`{"semaphore": 1, "max": 2, "holders": ["foo" "bar"]}`),
},
},
},
},
testGroup,
)
if err != nil {
t.Fatalf("error should be nil, got: %v", err)
}
gs, ge := elc.Get()
if tt.we {
if ge == nil {
t.Fatalf("case %d: expected error but got nil!", i)
}
} else {
if ge != nil {
t.Fatalf("case %d: unexpected error: %v", i, ge)
}

_, err = elc.Get()
if err == nil {
t.Fatal("error should not be nil")
}
if !reflect.DeepEqual(gs, tt.ws) {
t.Fatalf("case %d: bad semaphore: got %#v, want %#v", i, gs, tt.ws)

if err.Error() != "invalid character '\"' after array element" {
t.Fatalf("error should mention invalid character, got: %v", err)
}
}
})
}

func TestEtcdLockClientSet(t *testing.T) {
for i, tt := range []struct {
sem *Semaphore
ee error // error returned from etcd
want bool // do we expect Set to return an error
}{
// nil semaphore cannot be set
{nil, nil, true},
// empty semaphore is OK
{&Semaphore{}, nil, false},
{&Semaphore{Index: uint64(1234)}, nil, false},
// all errors returned from etcd should propagate
{&Semaphore{}, client.Error{Code: client.ErrorCodeNodeExist}, true},
{&Semaphore{}, client.Error{Code: client.ErrorCodeKeyNotFound}, true},
{&Semaphore{}, errors.New("some random error"), true},
} {
elc := &EtcdLockClient{
keyapi: &testEtcdClient{err: tt.ee},
}
got := elc.Set(tt.sem)
if (got != nil) != tt.want {
t.Errorf("case %d: unexpected error state calling Set: got %v", i, got)
}
}
t.Run("Success", func(t *testing.T) {
elc, err := NewEtcdLockClient(&testEtcdClient{
err: nil,
getResp: &client.GetResponse{Count: 0},
txn: testTxn{txnSuccess: true},
},
testGroup,
)
if err != nil {
t.Fatalf("error should be nil, got: %v", err)
}

err = elc.Set(&Semaphore{})
if err != nil {
t.Fatalf("error should be nil, got: %v", err)
}

})
t.Run("ErrorNilSemaphore", func(t *testing.T) {
elc, err := NewEtcdLockClient(&testEtcdClient{
err: nil,
getResp: &client.GetResponse{Count: 0},
},
testGroup,
)
if err != nil {
t.Fatalf("error should be nil, got: %v", err)
}

err = elc.Set(nil)
if err == nil {
t.Fatal("error should not be nil")
}

if err.Error() != "cannot set nil semaphore" {
t.Fatalf("error should 'cannot set nil semaphore', got: %v", err)
}
})
t.Run("ErrorTransaction", func(t *testing.T) {
elc, err := NewEtcdLockClient(&testEtcdClient{
err: nil,
getResp: &client.GetResponse{Count: 0},
},
testGroup,
)
if err != nil {
t.Fatalf("error should be nil, got: %v", err)
}

err = elc.Set(&Semaphore{})
if err == nil {
t.Fatal("error should not be nil")
}

if err.Error() != "failed to set the semaphore - it got updated in the meantime" {
t.Fatalf("error should be 'failed to set the semaphore - it got updated in the meantime', got: %v", err)
}
})
}