From f85f3b7270ec77111bbd8e1c8215c7ec3f35383d Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 14 Jun 2021 14:41:29 -0400 Subject: [PATCH 1/2] Fix a data race The nodeMap contains pointers, so we need to hold the lock until all the field values have been read. Otherwise there is a data race because another goroutine could update the struct referenced by the pointer. --- memberlist.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/memberlist.go b/memberlist.go index 7ee040091..fb8566629 100644 --- a/memberlist.go +++ b/memberlist.go @@ -501,7 +501,6 @@ func (m *Memberlist) UpdateNode(timeout time.Duration) error { // Get the existing node m.nodeLock.RLock() state := m.nodeMap[m.config.Name] - m.nodeLock.RUnlock() // Format a new alive message a := alive{ @@ -512,6 +511,7 @@ func (m *Memberlist) UpdateNode(timeout time.Duration) error { Meta: meta, Vsn: m.config.BuildVsnArray(), } + m.nodeLock.RUnlock() notifyCh := make(chan struct{}) m.aliveNode(&a, notifyCh, true) From 2f390a2390179717b7f5c35c642f9992720aba00 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 14 Jun 2021 15:23:09 -0400 Subject: [PATCH 2/2] Support setting node meta without a delegate This change is being made to fix a data race in Serf and Consul. By accepting the tags to UpdateNodeMeta, we can use a lock around the local tags. Data races aside, fewer circular dependencies seems like a good direction. --- config.go | 4 ++++ delegate.go | 3 +++ memberlist.go | 35 +++++++++++++++++++++-------------- memberlist_test.go | 7 +------ 4 files changed, 29 insertions(+), 20 deletions(-) diff --git a/config.go b/config.go index 31099e75f..4f27cfa8f 100644 --- a/config.go +++ b/config.go @@ -16,6 +16,10 @@ type Config struct { // The name of this node. This must be unique in the cluster. Name string + // Meta data associated with the local node. The slice must have a length + // less than MetaMaxSize. + Meta []byte + // Transport is a hook for providing custom code to communicate with // other nodes. If this is left nil, then memberlist will by default // make a NetTransport using BindAddr and BindPort from this structure. diff --git a/delegate.go b/delegate.go index 551548892..ee2a02fe1 100644 --- a/delegate.go +++ b/delegate.go @@ -7,6 +7,9 @@ type Delegate interface { // NodeMeta is used to retrieve meta-data about the current node // when broadcasting an alive message. It's length is limited to // the given byte size. This metadata is available in the Node structure. + // + // Deprecated: set Config.Meta for initial node meta, and pass any updates to + // Memberlist.UpdateNodeMeta. NodeMeta(limit int) []byte // NotifyMsg is called when a user-data message is received. diff --git a/memberlist.go b/memberlist.go index fb8566629..501c0077a 100644 --- a/memberlist.go +++ b/memberlist.go @@ -187,6 +187,13 @@ func newMemberlist(conf *Config) (*Memberlist, error) { nodeAwareTransport = &shimNodeAwareTransport{transport} } + if len(conf.Meta) == 0 && conf.Delegate != nil { + conf.Meta = conf.Delegate.NodeMeta(MetaMaxSize) + } + if len(conf.Meta) > MetaMaxSize { + return nil, fmt.Errorf("meta data (%d) is longer than the limit (%d)", len(conf.Meta), MetaMaxSize) + } + m := &Memberlist{ config: conf, shutdownCh: make(chan struct{}), @@ -430,21 +437,12 @@ func (m *Memberlist) setAlive() error { m.logger.Printf("[WARN] memberlist: Binding to public address without encryption!") } - // Set any metadata from the delegate. - var meta []byte - if m.config.Delegate != nil { - meta = m.config.Delegate.NodeMeta(MetaMaxSize) - if len(meta) > MetaMaxSize { - panic("Node meta data provided is longer than the limit") - } - } - a := alive{ Incarnation: m.nextIncarnation(), Node: m.config.Name, Addr: addr, Port: uint16(port), - Meta: meta, + Meta: m.config.Meta, Vsn: m.config.BuildVsnArray(), } m.aliveNode(&a, nil, true) @@ -488,14 +486,23 @@ func (m *Memberlist) LocalNode() *Node { // meta data. This will block until the update message is successfully // broadcasted to a member of the cluster, if any exist or until a specified // timeout is reached. +// Deprecated: use UpdateNodeMeta func (m *Memberlist) UpdateNode(timeout time.Duration) error { - // Get the node meta data var meta []byte if m.config.Delegate != nil { meta = m.config.Delegate.NodeMeta(MetaMaxSize) - if len(meta) > MetaMaxSize { - panic("Node meta data provided is longer than the limit") - } + } + return m.UpdateNodeMeta(timeout, meta) +} + +// UpdateNodeMeta is used to trigger re-advertising the local node. This is +// primarily used with a Delegate to support dynamic updates to the local +// meta data. This will block until the update message is successfully +// broadcasted to a member of the cluster, if any exist or until a specified +// timeout is reached. +func (m *Memberlist) UpdateNodeMeta(timeout time.Duration, meta []byte) error { + if len(meta) > MetaMaxSize { + panic("Node meta data provided is longer than the limit") } // Get the existing node diff --git a/memberlist_test.go b/memberlist_test.go index 70b5d780c..df2563509 100644 --- a/memberlist_test.go +++ b/memberlist_test.go @@ -1114,8 +1114,6 @@ func TestMemberlist_delegateMeta_Update(t *testing.T) { c2 := testConfig(t) c2.BindPort = bindPort - mock2 := &MockDelegate{meta: []byte("lb")} - c2.Delegate = mock2 m2, err := Create(c2) require.NoError(t, err) @@ -1126,13 +1124,10 @@ func TestMemberlist_delegateMeta_Update(t *testing.T) { yield() - // Update the meta data roles mock1.setMeta([]byte("api")) - mock2.setMeta([]byte("db")) - err = m1.UpdateNode(0) require.NoError(t, err) - err = m2.UpdateNode(0) + err = m2.UpdateNodeMeta(0, []byte("db")) require.NoError(t, err) yield()