fix: add a warning message on startup if the server name is default #14613

nic-chen · 2022-10-23T16:26:00Z

codecov-commenter · 2022-10-23T17:03:48Z

Codecov Report

Merging #14613 (191fb30) into main (e5790d2) will decrease coverage by 0.32%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #14613      +/-   ##
==========================================
- Coverage   75.76%   75.44%   -0.33%     
==========================================
  Files         457      457              
  Lines       37267    37269       +2     
==========================================
- Hits        28235    28117     -118     
- Misses       7284     7385     +101     
- Partials     1748     1767      +19

Flag	Coverage Δ
all	`75.44% <100.00%> (-0.33%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/embed/config.go	`74.35% <100.00%> (+0.13%)`	⬆️
server/proxy/grpcproxy/register.go	`69.76% <0.00%> (-9.31%)`	⬇️
server/storage/mvcc/watchable_store.go	`84.42% <0.00%> (-9.06%)`	⬇️
client/pkg/v3/testutil/leak.go	`62.83% <0.00%> (-7.08%)`	⬇️
client/v3/namespace/watch.go	`87.87% <0.00%> (-6.07%)`	⬇️
server/lease/lease.go	`94.87% <0.00%> (-5.13%)`	⬇️
raft/rafttest/node.go	`95.00% <0.00%> (-5.00%)`	⬇️
client/v3/concurrency/session.go	`88.63% <0.00%> (-4.55%)`	⬇️
server/storage/mvcc/kvstore_compaction.go	`95.65% <0.00%> (-4.35%)`	⬇️
client/v3/leasing/cache.go	`87.77% <0.00%> (-3.89%)`	⬇️
... and 23 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ahrtr · 2022-10-24T20:23:01Z

server/etcdserver/api/membership/cluster.go

+		for idx := range urls {
+			m := NewMember(name, urls[idx:idx+1], token, nil)
+			if _, ok := c.members[m.ID]; ok {
+				return nil, fmt.Errorf("member exists with identical ID %v", m)


You might be heading in the wrong direction. Usually each member has different peerURL(s), they must have different memberID. FYI.

etcd/server/etcdserver/api/membership/member.go

Lines 63 to 76 in f215cd8

func computeMemberId(peerURLs types.URLs, clusterName string, now *time.Time) types.ID {

peerURLstrs := peerURLs.StringSlice()

sort.Strings(peerURLstrs)

joinedPeerUrls := strings.Join(peerURLstrs, "")

b := []byte(joinedPeerUrls)

b = append(b, []byte(clusterName)...)

if now != nil {

b = append(b, []byte(fmt.Sprintf("%d", now.Unix()))...)

}

hash := sha1.Sum(b)

return types.ID(binary.BigEndian.Uint64(hash[:8]))

}

For simplicity, we can just add a warning message on startup if --name isn't provided a value.

Thank you very much for the reminder.

updated. Please take a look again.
Instead of actually checking if --name is empty, I'm checking if it's default.

Signed-off-by: nic-chen <chenjunxu6@gmail.com>

ahrtr · 2022-10-26T22:53:19Z

server/etcdmain/etcd.go

@@ -90,6 +90,10 @@ func startEtcdOrProxyV2(args []string) {
 		lg.Info("failed to detect default host", zap.Error(dhErr))
 	}

+	if cfg.ec.Name == embed.DefaultName {
+		lg.Warn("the server is using default name, which might cause error that failed the startup.")
+	}


I suggest to add the check into (*Config) Validate(), so that we have all configuration checker in one place.

Thanks!
I tried to find such a place, but failed.
So I put it here according to the content in the log 'data-dir' was empty; using default...

server/etcdmain/etcd.go

Signed-off-by: nic-chen <chenjunxu6@gmail.com>

server/embed/config.go

ahrtr

LGTM

Thank you @nic-chen

ahrtr · 2022-10-27T21:16:12Z

server/embed/config.go

+	// because each member can have multiple client or peer URLs.
+	// Please refer to https://github.com/etcd-io/etcd/issues/13757
+	if cfg.Name == DefaultName {
+		cfg.logger.Warn("the server is using default name, which might cause error that failed the startup.")


I was planning to add a comment for this, but somehow forgot to do it:(

Please change it to something like below,

return fmt.Errorf("it isn't recommended to use %q name, please set a value for --name. Note that etcd might run into issue when multiple members have the same default name", DefaultName)

Please ignore my previous comment. We can't return an error, because etcd may fail to get started. Let's just update the message,

cfg.logger.Warn("it isn't recommended to use %q name, please set a value for --name. Note that etcd might run into issue when multiple members have the same default name", DefaultName)

OK, I will update it later today or tomorrow

updated. #14642
please take a look at your convenience. thanks.

ahrtr reviewed Oct 24, 2022

View reviewed changes

fix: add a warning message on startup if the server name is default

d4fbfb8

Signed-off-by: nic-chen <chenjunxu6@gmail.com>

nic-chen force-pushed the fix/duplicate-cluster-name branch from 86e4a06 to d4fbfb8 Compare October 26, 2022 15:31

nic-chen marked this pull request as ready for review October 26, 2022 15:35

nic-chen changed the title ~~fix: the issue about duplicate cluster names caused member count is unequal error when initializing a cluster~~ fix: add a warning message on startup if the server name is default Oct 26, 2022

ahrtr reviewed Oct 26, 2022

View reviewed changes

server/etcdmain/etcd.go Outdated Show resolved Hide resolved

fix: apply review suggestion

191fb30

Signed-off-by: nic-chen <chenjunxu6@gmail.com>

ahrtr reviewed Oct 27, 2022

View reviewed changes

server/embed/config.go Show resolved Hide resolved

ahrtr approved these changes Oct 27, 2022

View reviewed changes

ahrtr added the stage/merge-when-tests-green label Oct 27, 2022

ahrtr merged commit 1e12426 into etcd-io:main Oct 27, 2022

ahrtr reviewed Oct 27, 2022

View reviewed changes

nic-chen deleted the fix/duplicate-cluster-name branch October 28, 2022 01:05

nic-chen mentioned this pull request Oct 28, 2022

chore: update the warn message on startup when the server name is default #14642

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add a warning message on startup if the server name is default #14613

fix: add a warning message on startup if the server name is default #14613

nic-chen commented Oct 23, 2022 •

edited

Loading

codecov-commenter commented Oct 23, 2022 •

edited

Loading

ahrtr Oct 24, 2022

nic-chen Oct 25, 2022

nic-chen Oct 26, 2022

ahrtr Oct 26, 2022

nic-chen Oct 27, 2022

nic-chen Oct 27, 2022

ahrtr left a comment

ahrtr Oct 27, 2022

ahrtr Oct 28, 2022

nic-chen Oct 28, 2022

nic-chen Oct 28, 2022

nic-chen Oct 28, 2022

	func computeMemberId(peerURLs types.URLs, clusterName string, now *time.Time) types.ID {
	peerURLstrs := peerURLs.StringSlice()
	sort.Strings(peerURLstrs)
	joinedPeerUrls := strings.Join(peerURLstrs, "")
	b := []byte(joinedPeerUrls)

	b = append(b, []byte(clusterName)...)
	if now != nil {
	b = append(b, []byte(fmt.Sprintf("%d", now.Unix()))...)
	}

	hash := sha1.Sum(b)
	return types.ID(binary.BigEndian.Uint64(hash[:8]))
	}

fix: add a warning message on startup if the server name is default #14613

fix: add a warning message on startup if the server name is default #14613

Conversation

nic-chen commented Oct 23, 2022 • edited Loading

codecov-commenter commented Oct 23, 2022 • edited Loading

Codecov Report

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

ahrtr left a 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

Choose a reason for hiding this comment

nic-chen commented Oct 23, 2022 •

edited

Loading

codecov-commenter commented Oct 23, 2022 •

edited

Loading