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.

Sign up for GitHub

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

Jump to bottom

[openshift-4.9] Bug 2016174: UPSTREAM: <carry>: server: Fix for v3.5 Ensure that cluster members stored in v2store and backend are in sync #99

Merged
merged 1 commit into from
Oct 29, 2021
Merged

[openshift-4.9] Bug 2016174: UPSTREAM: <carry>: server: Fix for v3.5 Ensure that cluster members stored in v2store and backend are in sync #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
77 changes: 65 additions & 12 deletions server/etcdserver/api/membership/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"crypto/sha1"
"encoding/binary"
"encoding/json"
"errors"
"fmt"
"path"
"sort"
Expand All @@ -32,6 +33,7 @@ import (
"go.etcd.io/etcd/pkg/v3/netutil"
"go.etcd.io/etcd/raft/v3"
"go.etcd.io/etcd/raft/v3/raftpb"
"go.etcd.io/etcd/server/v3/etcdserver/api/v2error"
"go.etcd.io/etcd/server/v3/etcdserver/api/v2store"
"go.etcd.io/etcd/server/v3/mvcc/backend"
"go.etcd.io/etcd/server/v3/mvcc/buckets"
Expand Down Expand Up @@ -254,12 +256,12 @@ func (c *RaftCluster) Recover(onSet func(*zap.Logger, *semver.Version)) {
c.Lock()
defer c.Unlock()

if c.be != nil {
c.version = clusterVersionFromBackend(c.lg, c.be)
c.members, c.removed = membersFromBackend(c.lg, c.be)
} else {
if c.v2store != nil {
c.version = clusterVersionFromStore(c.lg, c.v2store)
c.members, c.removed = membersFromStore(c.lg, c.v2store)
} else {
c.version = clusterVersionFromBackend(c.lg, c.be)
c.members, c.removed = membersFromBackend(c.lg, c.be)
}

if c.be != nil {
Expand Down Expand Up @@ -381,11 +383,37 @@ func (c *RaftCluster) ValidateConfigurationChange(cc raftpb.ConfChange) error {
func (c *RaftCluster) AddMember(m *Member, shouldApplyV3 ShouldApplyV3) {
c.Lock()
defer c.Unlock()

var v2Err, beErr error
if c.v2store != nil {
mustSaveMemberToStore(c.lg, c.v2store, m)
v2Err = unsafeSaveMemberToStore(c.lg, c.v2store, m)
if v2Err != nil {
if e, ok := v2Err.(*v2error.Error); !ok || e.ErrorCode != v2error.EcodeNodeExist {
c.lg.Panic(
"failed to save member to store",
zap.String("member-id", m.ID.String()),
zap.Error(v2Err),
)
}
}
}
if c.be != nil && shouldApplyV3 {
mustSaveMemberToBackend(c.lg, c.be, m)
beErr = unsafeSaveMemberToBackend(c.lg, c.be, m)
if beErr != nil && !errors.Is(beErr, errMemberAlreadyExist) {
c.lg.Panic(
"failed to save member to backend",
zap.String("member-id", m.ID.String()),
zap.Error(beErr),
)
}
}
// Panic if both storeV2 and backend report member already exist.
if v2Err != nil && (beErr != nil || c.be == nil) {
c.lg.Panic(
"failed to save member to store",
zap.String("member-id", m.ID.String()),
zap.Error(v2Err),
)
}

c.members[m.ID] = m
Expand All @@ -404,11 +432,36 @@ func (c *RaftCluster) AddMember(m *Member, shouldApplyV3 ShouldApplyV3) {
func (c *RaftCluster) RemoveMember(id types.ID, shouldApplyV3 ShouldApplyV3) {
c.Lock()
defer c.Unlock()
var v2Err, beErr error
if c.v2store != nil {
mustDeleteMemberFromStore(c.lg, c.v2store, id)
v2Err = unsafeDeleteMemberFromStore(c.v2store, id)
if v2Err != nil {
if e, ok := v2Err.(*v2error.Error); !ok || e.ErrorCode != v2error.EcodeKeyNotFound {
c.lg.Panic(
"failed to delete member from store",
zap.String("member-id", id.String()),
zap.Error(v2Err),
)
}
}
}
if c.be != nil && shouldApplyV3 {
mustDeleteMemberFromBackend(c.be, id)
beErr = unsafeDeleteMemberFromBackend(c.be, id)
if beErr != nil && !errors.Is(beErr, errMemberNotFound) {
c.lg.Panic(
"failed to delete member from backend",
zap.String("member-id", id.String()),
zap.Error(beErr),
)
}
}
// Panic if both storeV2 and backend report member not found.
if v2Err != nil && (beErr != nil || c.be == nil) {
c.lg.Panic(
"failed to delete member from store",
zap.String("member-id", id.String()),
zap.Error(v2Err),
)
}

m, ok := c.members[id]
Expand Down Expand Up @@ -443,7 +496,7 @@ func (c *RaftCluster) UpdateAttributes(id types.ID, attr Attributes, shouldApply
mustUpdateMemberAttrInStore(c.lg, c.v2store, m)
}
if c.be != nil && shouldApplyV3 {
mustSaveMemberToBackend(c.lg, c.be, m)
unsafeSaveMemberToBackend(c.lg, c.be, m)
}
return
}
Expand Down Expand Up @@ -476,7 +529,7 @@ func (c *RaftCluster) PromoteMember(id types.ID, shouldApplyV3 ShouldApplyV3) {
mustUpdateMemberInStore(c.lg, c.v2store, c.members[id])
}
if c.be != nil && shouldApplyV3 {
mustSaveMemberToBackend(c.lg, c.be, c.members[id])
unsafeSaveMemberToBackend(c.lg, c.be, c.members[id])
}

c.lg.Info(
Expand All @@ -495,7 +548,7 @@ func (c *RaftCluster) UpdateRaftAttributes(id types.ID, raftAttr RaftAttributes,
mustUpdateMemberInStore(c.lg, c.v2store, c.members[id])
}
if c.be != nil && shouldApplyV3 {
mustSaveMemberToBackend(c.lg, c.be, c.members[id])
unsafeSaveMemberToBackend(c.lg, c.be, c.members[id])
}

c.lg.Info(
Expand Down Expand Up @@ -870,7 +923,7 @@ func (c *RaftCluster) PushMembershipToStorage() {
if c.be != nil {
TrimMembershipFromBackend(c.lg, c.be)
for _, m := range c.members {
mustSaveMemberToBackend(c.lg, c.be, m)
unsafeSaveMemberToBackend(c.lg, c.be, m)
}
}
if c.v2store != nil {
Expand Down
196 changes: 194 additions & 2 deletions server/etcdserver/api/membership/cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,19 @@ import (
"path"
"reflect"
"testing"
"time"

"github.com/coreos/go-semver/semver"
"github.com/stretchr/testify/assert"
betesting "go.etcd.io/etcd/server/v3/mvcc/backend/testing"
"go.uber.org/zap"
"go.uber.org/zap/zaptest"

"go.etcd.io/etcd/client/pkg/v3/testutil"
"go.etcd.io/etcd/client/pkg/v3/types"
"go.etcd.io/etcd/raft/v3/raftpb"
"go.etcd.io/etcd/server/v3/etcdserver/api/v2store"
"go.etcd.io/etcd/server/v3/mock/mockstore"

"go.uber.org/zap"
)

func TestClusterMember(t *testing.T) {
Expand Down Expand Up @@ -1019,3 +1021,193 @@ func TestIsVersionChangable(t *testing.T) {
})
}
}

func TestAddMemberSyncsBackendAndStoreV2(t *testing.T) {
now := time.Now()
alice := NewMember("", nil, "alice", &now)

tcs := []struct {
name string

storeV2Nil bool
backendNil bool
storeV2Members []*Member
backendMembers []*Member

expectPanics bool
expectMembers map[types.ID]*Member
}{
{
name: "Adding new member should succeed",
},
{
name: "Adding member should succeed if it was only in storeV2",
storeV2Members: []*Member{alice},
},
{
name: "Adding member should succeed if it was only in backend",
backendMembers: []*Member{alice},
},
{
name: "Adding member should fail if it exists in both",
storeV2Members: []*Member{alice},
backendMembers: []*Member{alice},
expectPanics: true,
},
{
name: "Adding member should fail if it exists in storeV2 and backend is nil",
storeV2Members: []*Member{alice},
backendNil: true,
expectPanics: true,
},
{
name: "Adding member should succeed if it exists in backend and storageV2 is nil",
storeV2Nil: true,
backendMembers: []*Member{alice},
},
{
name: "Adding new member should succeed if backend is nil",
storeV2Members: []*Member{},
backendNil: true,
},
{
name: "Adding new member should fail if storageV2 is nil",
storeV2Nil: true,
backendMembers: []*Member{},
},
}
for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {
lg := zaptest.NewLogger(t)
be, _ := betesting.NewDefaultTmpBackend(t)
defer be.Close()
mustCreateBackendBuckets(be)
st := v2store.New()
for _, m := range tc.backendMembers {
unsafeSaveMemberToBackend(lg, be, m)
}
be.ForceCommit()
for _, m := range tc.storeV2Members {
mustSaveMemberToStore(lg, st, m)
}
cluster := NewCluster(lg)
if !tc.backendNil {
cluster.SetBackend(be)
}
if !tc.storeV2Nil {
cluster.SetStore(st)
}
if tc.expectPanics {
assert.Panics(t, func() {
cluster.AddMember(alice, ApplyBoth)
})
} else {
cluster.AddMember(alice, ApplyBoth)
}
if !tc.storeV2Nil {
storeV2Members, _ := membersFromStore(lg, st)
assert.Equal(t, map[types.ID]*Member{alice.ID: alice}, storeV2Members)
}
if !tc.backendNil {
be.ForceCommit()
beMembers, _ := mustReadMembersFromBackend(lg, be)
assert.Equal(t, map[types.ID]*Member{alice.ID: alice}, beMembers)
}
})
}
}

func TestRemoveMemberSyncsBackendAndStoreV2(t *testing.T) {
now := time.Now()
alice := NewMember("", nil, "alice", &now)

tcs := []struct {
name string

storeV2Nil bool
backendNil bool
storeV2Members []*Member
backendMembers []*Member

expectMembers []*Member
expectPanics bool
}{
{
name: "Removing new member should fail",
expectPanics: true,
},
{
name: "Removing member should succeed if it was only in storeV2",
storeV2Members: []*Member{alice},
},
{
name: "Removing member should succeed if it was only in backend",
backendMembers: []*Member{alice},
},
{
name: "Removing member should succeed if it exists in both",
storeV2Members: []*Member{alice},
backendMembers: []*Member{alice},
},
{
name: "Removing new member should fail if backend is nil",
storeV2Members: []*Member{},
backendNil: true,
expectPanics: true,
},
{
name: "Removing new member should succeed if storageV2 is nil",
storeV2Nil: true,
backendMembers: []*Member{},
},
{
name: "Removing member should succeed if it exists in v2storage and backend is nil",
storeV2Members: []*Member{alice},
backendNil: true,
},
{
name: "Removing member should succeed if it exists in backend and storageV2 is nil",
storeV2Nil: true,
backendMembers: []*Member{alice},
},
}
for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {
lg := zaptest.NewLogger(t)
be, _ := betesting.NewDefaultTmpBackend(t)
defer be.Close()
mustCreateBackendBuckets(be)
st := v2store.New()
for _, m := range tc.backendMembers {
unsafeSaveMemberToBackend(lg, be, m)
}
be.ForceCommit()
for _, m := range tc.storeV2Members {
mustSaveMemberToStore(lg, st, m)
}
cluster := NewCluster(lg)
if !tc.backendNil {
cluster.SetBackend(be)
}
if !tc.storeV2Nil {
cluster.SetStore(st)
}
if tc.expectPanics {
assert.Panics(t, func() {
cluster.RemoveMember(alice.ID, ApplyBoth)
})
} else {
cluster.RemoveMember(alice.ID, ApplyBoth)
}
if !tc.storeV2Nil {
storeV2Members, _ := membersFromStore(lg, st)
assert.Equal(t, map[types.ID]*Member{}, storeV2Members)
}
if !tc.backendNil {
be.ForceCommit()
beMembers, _ := mustReadMembersFromBackend(lg, be)
assert.Equal(t, map[types.ID]*Member{}, beMembers)
}
})
}
}