Migrate cluster attributes to use v3 backend #11427

YoyinZyc · 2019-12-06T01:06:19Z

Related #11380
Things did in this pr:

add two raft internal types ClusterVersionSet ClusterMemberAttrSet
use v3 server internal raft type to update cluster version.
add publishV3 to publish member attributes through v3 backend. Keeping using v2 publish to avoid incompatible during upgrade.
recover clusterVersion from backend instead of v2Store

Added ClusterVersionSetRequest for setting cluster version via v3 apply. Added ClusterMemberAttrSetRequest for setting clsuter member attributes via v3 apply.

codecov-io · 2019-12-06T01:25:44Z

Codecov Report

Merging #11427 into master will decrease coverage by 0.02%.
The diff coverage is 26.25%.

@@            Coverage Diff             @@
##           master   #11427      +/-   ##
==========================================
- Coverage   64.63%    64.6%   -0.03%     
==========================================
  Files         403      403              
  Lines       37998    38070      +72     
==========================================
+ Hits        24560    24596      +36     
- Misses      11802    11843      +41     
+ Partials     1636     1631       -5

Impacted Files	Coverage Δ
etcdserver/apply_v2.go	`86.15% <ø> (-3.4%)`	⬇️
etcdserver/apply.go	`77.63% <41.17%> (-0.99%)`	⬇️
etcdserver/api/membership/cluster.go	`72.77% <57.89%> (+0.35%)`	⬆️
etcdserver/server.go	`66.68% <6.81%> (-1.86%)`	⬇️
pkg/transport/timeout_conn.go	`80% <0%> (-20%)`	⬇️
proxy/grpcproxy/register.go	`69.44% <0%> (-11.12%)`	⬇️
etcdserver/api/v3rpc/lease.go	`69.31% <0%> (-5.69%)`	⬇️
pkg/logutil/zap_grpc.go	`47.61% <0%> (-4.77%)`	⬇️
proxy/grpcproxy/watcher.go	`89.79% <0%> (-4.09%)`	⬇️
proxy/grpcproxy/watch.go	`89.94% <0%> (-2.96%)`	⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f8764b...b657b78. Read the comment docs.

jingyih · 2019-12-06T01:20:06Z

tests/e2e/ctl_v3_migrate_test.go

-	if resp.Kvs[0].CreateRevision != 7 {
-		t.Fatalf("resp.Kvs[0].CreateRevision expected 7, got %d", resp.Kvs[0].CreateRevision)
+
+	if resp.Kvs[0].CreateRevision <= rev {


Is CreateRevision == rev + 1 ?

jingyih · 2019-12-06T01:20:49Z

tests/e2e/ctl_v3_migrate_test.go

@@ -85,11 +83,22 @@ func TestCtlV3Migrate(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	rev := resp.Header.Revision


rename to revAfterMigrate for better readability?

jingyih · 2019-12-06T01:25:06Z

etcdserver/server.go

+// information is the JSON representation of this server's member struct, updated
+// with the static clientURLs of the server.
+// The function keeps attempting to register until it succeeds,
+// or its server is stopped.


Add a TODO to replace publish() in etcd 3.6?

jingyih · 2019-12-06T01:42:16Z

To clarify, the intention is to add new internal raft type ClusterVersionSet and ClusterMemberAttrSet in v3.5 (so that v3.5 servers understand these types), but they will only be exercised in next minor version release v3.6.

jingyih · 2019-12-06T01:57:58Z

@gyuho Could you help take a look?

The main motivation of this PR is that currently cluster version is recovered from v2 store during server restart. Because v2 store is in memory, cluster version information after server restart could be very stale. During cluster downgrade, when server starts and checks the compatibility of its own server version against cluster version, the check could fail if the cluster version info is very stale. This could be very confusing to user. As an example: #11362 (comment)

So the plan is to use v3 request and v3 backend for all cluster information updates (except members).

gyuho · 2019-12-08T04:36:17Z

clientv3/integration/kv_test.go

@@ -893,7 +893,7 @@ func TestKVLargeRequests(t *testing.T) {
 		expectError error
 	}{
 		{
-			maxRequestBytesServer:  1,
+			maxRequestBytesServer:  256,


Why do we change this?

Basically, if we keep the previous test data, the test cluster will fail to start. It is because the cluster will make several v3 raft requests like cluster version set during start. Old code will use v2 for starting server which will bypass the check.

etcd/etcdserver/v3_server.go

Lines 633 to 635 in 1f8764b

if len(data) > int(s.Cfg.MaxRequestBytes) {

return nil, ErrRequestTooLarge

}

Changing a larger maxRequestBytesServer will not affect the test behavior as long as maxRequestBytesServer is smaller then valueSize.

gyuho · 2019-12-08T04:41:34Z

etcdserver/server.go

+	for {
+		select {
+		case <-s.stopping:
+			if lg := s.getLogger(); lg != nil {


Can we update getLogger to return zap.NewNop(), so we don't need all these manual nil checks?

Done. Will fix other getLogger in a separate pr.

gyuho · 2019-12-08T04:42:14Z

etcdserver/server.go

+			return
+
+		default:
+			if lg := s.getLogger(); lg != nil {


Same as above. Can we store variable once at top (before for loop), and use the same logger?

gyuho · 2019-12-08T04:43:41Z

etcdserver/etcdserverpb/raft_internal.proto

@@ -58,6 +59,9 @@ message InternalRaftRequest {
  AuthRoleGetRequest auth_role_get = 1202;
  AuthRoleGrantPermissionRequest auth_role_grant_permission = 1203;
  AuthRoleRevokePermissionRequest auth_role_revoke_permission = 1204;
+
+  membershippb.ClusterVersionSetRequest cluster_version_set = 1300;
+  membershippb.ClusterMemberAttrSetRequest cluster_member_attr_set = 1301;


these are unknown fields for older etcd? What happens to the older etcd if it receives this request?

Ok, now I see. These will be ignored in old etcd, and it instead uses v2 API.

Looks like these two fields made v3.5 to v3.4 downgrade fail.

Can we make it an experimental API in v3.4 and add these two fields to make downgrade from v3.5 to v3.4 possible?

panic: not implemented goroutine 214 [running]: go.etcd.io/etcd/etcdserver.(*applierV3backend).Apply(0xc000238468, 0xc0002b2460, 0x0) /home/chaochn/workplace/EKS-etcd/src/EKS-etcd/etcdserver/apply.go:173 +0xf85 go.etcd.io/etcd/etcdserver.(*authApplierV3).Apply(0xc000242000, 0xc0002b2460, 0x0) /home/chaochn/workplace/EKS-etcd/src/EKS-etcd/etcdserver/apply_auth.go:60 +0xd2 go.etcd.io/etcd/etcdserver.(*EtcdServer).applyEntryNormal(0xc000290600, 0xc0002d14d8) /home/chaochn/workplace/EKS-etcd/src/EKS-etcd/etcdserver/server.go:2230 +0x1fb go.etcd.io/etcd/etcdserver.(*EtcdServer).apply(0xc000290600, 0xc000302a80, 0xa, 0xc, 0xc0001c8000, 0x0, 0xc000046000, 0xc00033f640) /home/chaochn/workplace/EKS-etcd/src/EKS-etcd/etcdserver/server.go:2144 +0x579 go.etcd.io/etcd/etcdserver.(*EtcdServer).applyEntries(0xc000290600, 0xc0001c8000, 0xc00002c000) /home/chaochn/workplace/EKS-etcd/src/EKS-etcd/etcdserver/server.go:1396 +0xe5 go.etcd.io/etcd/etcdserver.(*EtcdServer).applyAll(0xc000290600, 0xc0001c8000, 0xc00002c000) /home/chaochn/workplace/EKS-etcd/src/EKS-etcd/etcdserver/server.go:1120 +0x88 go.etcd.io/etcd/etcdserver.(*EtcdServer).run.func8(0x11b6e30, 0xc0005da040) /home/chaochn/workplace/EKS-etcd/src/EKS-etcd/etcdserver/server.go:1065 +0x3c go.etcd.io/etcd/pkg/schedule.(*fifo).run(0xc0003c0180) /home/chaochn/workplace/EKS-etcd/src/EKS-etcd/pkg/schedule/schedule.go:157 +0xf3 created by go.etcd.io/etcd/pkg/schedule.NewFIFOScheduler /home/chaochn/workplace/EKS-etcd/src/EKS-etcd/pkg/schedule/schedule.go:70 +0x13b

Do you know which one failed:

ClusterVersionSetRequest or ClusterMemberAttrSetRequest

ClusterMemberAttrSetRequest is only called by publishV3, that is not yet called in v3.5. so I assume not this one.

ClusterVersionSetRequest seems to be called from:

etcd/server/etcdserver/server.go

Line 2431 in f82b5cb

func (s *EtcdServer) monitorVersions() {

@hexfusion @wenjiaswe @ychen11 shell we postpone enabling monitorVersions() till etcd-3.6 ?

Yea, I confirmed in v3.4 we haven't yet implemented ClusterVersionSet API just now. And the raftRequest MustUnmarshal just slipped this through, which is surprising to me.

case r.ClusterVersionSet != nil: a.s.applyV3Internal.ClusterVersionSet(r.ClusterVersionSet) default: panic("not implemented")

so do we want to make a equivalent monitorVersionsV3() in etcd-3.5? @ptabor

shell we postpone enabling monitorVersions() till etcd-3.6 ?

Yes, this breaks upgrade case, which is not acceptable for 3.5 + 3.5 + 3.4.

Thanks for the catch @chaochn47.

@chaochn47 Can you help making this optional?

FTR: #12987

gyuho · 2019-12-08T04:44:25Z

tests/e2e/cluster_test.go

@@ -290,6 +290,7 @@ func (cfg *etcdProcessClusterConfig) etcdServerProcessConfigs() []*etcdServerPro
 	for i := range etcdCfgs {
 		etcdCfgs[i].initialCluster = strings.Join(initialCluster, ",")
 		etcdCfgs[i].args = append(etcdCfgs[i].args, initialClusterArgs...)
+		etcdCfgs[i].args = append(etcdCfgs[i].args, "--logger=zap")


@jingyih Should we make zap default in a separate PR, so we don't make this unrelated change?

gyuho · 2019-12-08T04:44:39Z

tests/e2e/ctl_v2_test.go

@@ -289,6 +289,7 @@ func testCtlV2Backup(t *testing.T, snapCount int, v3 bool) {

 	if v3 {
 		// v3 must lock the db to backup, so stop process
+		time.Sleep(time.Second)


need comment on why

Removed in current pr since we keep using v2 publish(). It seems like the cluster needs more time publish using publishV3 which makes TestCtlV2BackupV3 a flaky test. Adding a sleep is to address this.

gyuho · 2019-12-08T04:45:15Z

tests/e2e/etcd_config_test.go

@@ -148,6 +150,7 @@ func TestEtcdPeerCNAuth(t *testing.T) {
 			"--listen-peer-urls", fmt.Sprintf("https://127.0.0.1:%d,https://127.0.0.1:%d", etcdProcessBasePort+i, etcdProcessBasePort+len(peers)+i),
 			"--initial-advertise-peer-urls", fmt.Sprintf("https://127.0.0.1:%d", etcdProcessBasePort+i),
 			"--initial-cluster", ic,
+			"--logger=zap",


same as above, let's make zap default in a separate PR

gyuho · 2019-12-08T04:46:14Z

etcdserver/api/membership/cluster.go

+		if lg != nil {
+			lg.Panic(
+				"unexpected number of keys when getting cluster version from backend",
+				zap.Int("number fo keys", len(keys)),


can we make zap field as json tag as in Go? since it's structured logging, it should be logged as something like "number-of-keys" or "key"?

gyuho · 2019-12-08T04:57:30Z

integration/v3_grpc_test.go

@@ -1898,7 +1898,7 @@ func TestV3LargeRequests(t *testing.T) {
 		expectError     error
 	}{
 		// don't set to 0. use 0 as the default.
-		{1, 1024, rpctypes.ErrGRPCRequestTooLarge},
+		{256, 1024, rpctypes.ErrGRPCRequestTooLarge},


can we explain why we need this change?

explained above.
#11427 (comment)

gyuho · 2019-12-08T05:00:36Z

So the plan is to use v3 request and v3 backend for all cluster information updates

Sounds good. Overall approach looks good.

gyuho · 2019-12-08T05:04:13Z

etcdserver/api/membership/cluster.go

+	if c.be != nil {
+		c.version = clusterVersionFromBackend(c.lg, c.be)
+	} else {
+		c.version = clusterVersionFromStore(c.lg, c.v2store)


Do we need fallback on v2 version data if v3 version is nil (upgraded etcd process)?

c.version = clusterVersionFromStore(c.lg, c.v2store) if c.be != nil { v3Ver := clusterVersionFromBackend(c.lg, c.be) if v3Ver != nil { c.version = v3Ver } }

Not sure about it. When will v3 version data is nil while v2 is not? If v3 backend is available, the v3 version will not be nil except starting a new server? Setting version will store new version info into both v2 store and v3 backend.

etcd/etcdserver/api/membership/cluster.go

Lines 571 to 576 in 1f8764b

if c.v2store != nil {

mustSaveClusterVersionToStore(c.v2store, ver)

}

if c.be != nil {

mustSaveClusterVersionToBackend(c.be, ver)

}

Ok, that answers my question. Thanks.

gyuho · 2019-12-09T20:32:53Z

etcdserver/server.go

+			ClientUrls: s.attributes.ClientURLs,
+		},
+	}
+	lg := zap.NewNop()


I meant, we still use the zap logger, but a separate function getLogger should handle the previous if-statements. Can we use the logger from EtcdServer in this PR, and iterate more to clean up getLogger method in the separate PR?

Got you. Will update getLogger() in another PR.

gyuho

lgtm, great job!

Please update CHANGELOG accordingly.

jingyih · 2019-12-09T21:42:17Z

Thanks for taking the time to review! @gyuho
Good work! @YoyinZyc

CHANGELOG-3.5: update for #11427

jingyih and others added 4 commits December 5, 2019 16:25

etcdserver: add v3 request type for cluster attr

dcd622b

Added ClusterVersionSetRequest for setting cluster version via v3 apply. Added ClusterMemberAttrSetRequest for setting clsuter member attributes via v3 apply.

etcdserver: use V3 to update cluster version

0c3401f

etcdserver: use v3 to publish member attr

5cd2502

etcdserver: recover cluster version from backend

ed5a01a

jingyih reviewed Dec 6, 2019

View reviewed changes

gyuho reviewed Dec 8, 2019

View reviewed changes

YoyinZyc force-pushed the migration-cluster-attr branch from 5b86d3d to b657b78 Compare December 9, 2019 19:09

gyuho reviewed Dec 9, 2019

View reviewed changes

etcdserver: remove v2 version set; e2e: fix tests.

7784ca8

YoyinZyc force-pushed the migration-cluster-attr branch from b657b78 to 7784ca8 Compare December 9, 2019 21:09

gyuho approved these changes Dec 9, 2019

View reviewed changes

gyuho merged commit 9f81002 into etcd-io:master Dec 9, 2019

YoyinZyc added a commit to YoyinZyc/etcd that referenced this pull request Dec 9, 2019

CHANGELOG-3.5: update for etcd-io#11427

283acbd

YoyinZyc added a commit to YoyinZyc/etcd that referenced this pull request Dec 11, 2019

CHANGELOG-3.5: update for etcd-io#11427

b5b1923

YoyinZyc added a commit to YoyinZyc/etcd that referenced this pull request Dec 11, 2019

CHANGELOG-3.5: update for etcd-io#11427

69841bd

jingyih added a commit that referenced this pull request Dec 11, 2019

Merge pull request #11432 from YoyinZyc/changelog-11427

b7d9f45

CHANGELOG-3.5: update for #11427

jingyih mentioned this pull request Dec 11, 2019

Migrate cluster attributes to use v3 backend #11380

Closed

4 tasks

ptabor mentioned this pull request May 17, 2021

3.5 release next steps: Code freeze Monday 5/17 #12970

Closed

4 tasks

gyuho mentioned this pull request May 17, 2021

3.4 to 3.5 upgrade may panic #12987

Closed

ahrtr mentioned this pull request Jul 27, 2023

Downgrade support from 3.5 to 3.4 #15878

Open

	if len(data) > int(s.Cfg.MaxRequestBytes) {
	return nil, ErrRequestTooLarge
	}

	if c.v2store != nil {
	mustSaveClusterVersionToStore(c.v2store, ver)
	}
	if c.be != nil {
	mustSaveClusterVersionToBackend(c.be, ver)
	}

Migrate cluster attributes to use v3 backend #11427

Migrate cluster attributes to use v3 backend #11427

Conversation

YoyinZyc commented Dec 6, 2019

codecov-io commented Dec 6, 2019 • edited

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

jingyih commented Dec 6, 2019

jingyih commented Dec 6, 2019 • edited

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Dec 8, 2019 • edited

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

gyuho left a comment

Choose a reason for hiding this comment

jingyih commented Dec 9, 2019

codecov-io commented Dec 6, 2019 •

edited

jingyih commented Dec 6, 2019 •

edited

gyuho commented Dec 8, 2019 •

edited