*: Introduce version checking mechanism #1148

nolouch · 2018-07-13T02:32:04Z

What have you changed? (required)

Our system guarantees backward compatibility. But there is a compatibility problem after change RaftCommand. Almost every small change in RaftComand of TiKV requires a proper way to rolling upgrade.

What are the type of the changes (required)?

New feature

How has this PR been tested (required)?

unit tests
integration tests
manual tests

Does this PR affect documentation (docs/docs-cn) update? (optional)

Refer to a related PR or issue link (optional)

rleungx · 2018-07-13T02:42:29Z

server/version.go

+	VersionBatchSplit:                {Marjor: 2, Minor: 1},
+}
+
+// TargetVersion get target version by version featrue


s/featrue/feature/

rleungx · 2018-07-13T02:47:01Z

server/version.go

+
+// Version record version information.
+type Version struct {
+	Marjor        int32


s/Marjor/Major/

siddontang · 2018-07-13T11:32:09Z

server/version.go

+
+//ParseVersion parses a version from a string.
+//The string format should be "major.minor.patch-<unstable>".
+func ParseVersion(s string) (Version, error) {


em, why not using a 3rd lib?

I'd like to suggest using https://godoc.org/github.com/coreos/go-semver or https://godoc.org/github.com/blang/semver, not know their difference through.

siddontang · 2018-07-13T12:34:11Z

server/core/kv.go

@@ -108,6 +108,7 @@ func (kv *KV) DeleteRegion(region *metapb.Region) error {
 // SaveConfig stores marshalable cfg to the configPath.
 func (kv *KV) SaveConfig(cfg interface{}) error {
 	value, err := json.Marshal(cfg)
+	fmt.Println("pesist config:", string(value))


why print ?

pesist -> persist

siddontang · 2018-07-13T12:35:56Z

server/version.go

+// Version Fetures.
+const (
+	VersionBase VersionFeature = iota
+	VersionRegionMergeAndRaftLearner


em, too long...

Btw, they are two different things, why not using two version?

Yes, I would suggest changing from VersionFeature to just Feature and here splitting these into two features. The featuresDict can map them both to the same version. I would also remove VersionBase.

gregwebs · 2018-07-13T12:43:25Z

server/cluster_info.go

+}
+
+// IsSupport check if support the features.
+func (c *clusterInfo) IsSupport(f VersionFeature) bool {


IsSuport -> IsSupported

gregwebs · 2018-07-13T13:01:41Z

server/version.go

+)
+
+// VersionFeature is a unique identifier for minimum support version.
+type VersionFeature int


Using ordinal int can lead to merge conflicts. This is part of the reason why I used string in my error code PR.

gregwebs · 2018-07-13T13:08:32Z

server/version.go

+// Version Fetures.
+const (
+	VersionBase VersionFeature = iota
+	VersionRegionMergeAndRaftLearner


Yes, I would suggest changing from VersionFeature to just Feature and here splitting these into two features. The featuresDict can map them both to the same version. I would also remove VersionBase.

gregwebs · 2018-07-13T13:09:18Z

server/core/kv.go

@@ -108,6 +108,7 @@ func (kv *KV) DeleteRegion(region *metapb.Region) error {
 // SaveConfig stores marshalable cfg to the configPath.
 func (kv *KV) SaveConfig(cfg interface{}) error {
 	value, err := json.Marshal(cfg)
+	fmt.Println("pesist config:", string(value))


pesist -> persist

gregwebs · 2018-07-13T13:15:41Z

server/version.go

+//The string format should be "major.minor.patch-<unstable>".
+func ParseVersion(s string) (Version, error) {
+	if s == "" {
+		return TargetVersion(VersionBase), nil


I think this should be an error. Otherwise this may silently work when something is wrong. The caller can have different logic to get a default version if it doesn't actually have a version string.

I guess we need to make it compatible with old tikv server which does not send version when rolling update a online cluster.

gregwebs · 2018-07-13T13:36:53Z

server/grpc_service.go

@@ -218,6 +218,7 @@ func (s *Server) PutStore(ctx context.Context, request *pdpb.PutStoreRequest) (*
 	}

 	log.Infof("put store ok - %v", store)
+	cluster.cachedCluster.OnChangeClusterVersion()


If the machine crashes before here will the cluster version not get updated?

You are right. I need to handle this situation.

How about OnStoreVersionChange?

@gregwebs I reconsider it. The cluster version will not get updated if meet crashes before here. but also the corresponding TiKV will not start up. it will register in next time when restarting TiKV and get the update.

disksing · 2018-07-16T04:40:34Z

server/version.go

+	Major         int32
+	Minor         int32
+	Patch         int32
+	Unstable      int32


I think it should be PreRelease instead of Unstable.
See: https://semver.org/#spec-item-9 and https://godoc.org/github.com/coreos/go-semver/semver#Version

disksing · 2018-07-16T04:42:11Z

server/version.go

+	convMap := map[int32]string{
+		1: "alpha",
+		2: "beta",
+		3: "rc",


In fact the pre-release version can be any string contains [0-9A-Za-z-]. See https://semver.org/#spec-item-9

gregwebs · 2018-07-16T11:16:34Z

server/cluster_info.go

+		if s.IsTombstone() {
+			continue
+		}
+		if i == 0 {


I think it would be clearer to move this to after the v assignment as if i==0 || v.LessThan(minVersion).

disksing · 2018-07-16T11:18:37Z

server/cluster_info.go

+	}
+}
+
+// IsSupported check if support the feature.


IsSupported checks if the feature is supported by current cluster.

how about IsFeatureSupported?

disksing · 2018-07-16T11:19:02Z

server/cluster_info.go

+	if clusterVersion.LessThan(minSupportVersion) {
+		return false
+	}
+	return true


return !clusterVersion.LessThan(minSupportVersion)

disksing · 2018-07-16T11:21:20Z

server/grpc_service.go

@@ -218,6 +218,7 @@ func (s *Server) PutStore(ctx context.Context, request *pdpb.PutStoreRequest) (*
 	}

 	log.Infof("put store ok - %v", store)
+	cluster.cachedCluster.OnChangeClusterVersion()


How about OnStoreVersionChange?

disksing · 2018-07-16T11:23:32Z

server/cluster_info.go

+		if i == 0 {
+			minVersion = *MustParseVersion(s.GetVersion())
+			continue
+		}


The first store can be tombstone state.

I think you can let minVersion = *semver.Version and use if minVersion == nil || v.LessThen(minVersion) to compare.

nolouch · 2018-07-18T10:46:39Z

PTAL.

disksing · 2018-07-18T13:10:48Z

server/api/config.go

+	}
+	version, ok := input["cluster-version"]
+	if !ok {
+		h.rd.JSON(w, http.StatusInternalServerError, "not set cluster version")


Use 400 instead.

Message should use the exact key name "cluster-version". A helper function for this case would create consistent messages.

disksing · 2018-07-18T13:12:17Z

server/server.go

+		return errors.Trace(err)
+	}
+	s.scheduleOpt.SetClusterVersion(*version)
+	s.scheduleOpt.persist(s.kv)


Unchecked return value?

There are at least 4 other instances of not checking the return value for this in the same file. I am going to try to setup errcheck efficiently, but I may not finish that until next week. But here is a simple check of the server code: #1152

disksing · 2018-07-18T13:13:48Z

server/version.go

+	Version2_0
+	RegionMerge
+	RaftLearner
+	BatchSplit


Better to add some comments to describe the meaning of these features.

disksing · 2018-07-18T13:20:06Z

server/version.go

+	RegionMerge: {Major: 2, Minor: 0},
+	RaftLearner: {Major: 2, Minor: 0},
+	BatchSplit:  {Major: 2, Minor: 1},
+}


Will it be more readable to use string style? like BatchSplit: "v2.1.0".

disksing · 2018-07-19T03:47:44Z

server/server.go

+	err = s.scheduleOpt.persist(s.kv)
+	if err != nil {
+		return errors.Trace(err)
+	}


I think you want to update the version before persisting it.

disksing · 2018-07-19T05:44:06Z

server/server.go

+	if err != nil {
+		return errors.Trace(err)
+	}
+	s.scheduleOpt.SetClusterVersion(*version)


I think you want to update the version before persisting it.

siddontang · 2018-07-19T07:29:22Z

server/version.go

+// ParseVersion wraps semver.NewVersion and handles compatibility issues.
+func ParseVersion(v string) (*semver.Version, error) {
+	// for compatibility with old version which not support `version` mechanism.
+	baseVersion := semver.New(featuresDict[Base])


you can create the base version in the if clause.

siddontang · 2018-07-19T07:34:30Z

server/cluster_info.go

+			minVersion = v
+		}
+	}
+	if clusterVersion.LessThan(*minVersion) {


can we meet the case that PD itself's version is less than the min version?

Yes. but PD just provides currently knows features. so the cluster still running with lower version features. I think it's normal behavior because the cluster upgrade not finished. Maybe I need to warn it.

em, if PD's version is older, it should not be able to provide service for the features that it does not have.

disksing · 2018-07-20T07:41:55Z

LGTM.

nolouch · 2018-07-24T03:22:08Z

PTAL @siddontang @rleungx

…1.11.1 in /tools/pd-tso-bench (#5990) ref #897, ref #962, ref #969, ref #974, ref #975, ref #976, ref #986, ref prometheus/client_golang#987, ref #987, ref #989, ref #998, ref #1013, ref #1014, ref #1025, ref #1028, ref #1031, ref #1043, ref #1055, ref #1075, ref #1091, ref #1094, ref #1102, ref #1103, ref #1118, ref #1146, ref #1148, ref #1150 Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…1.11.1 in /tests/client (tikv#5992) ref tikv#897, ref tikv#962, ref tikv#969, ref tikv#974, ref tikv#975, ref tikv#976, ref tikv#986, ref tikv#987, ref prometheus/client_golang#987, ref tikv#989, ref tikv#998, ref tikv#1013, ref tikv#1014, ref tikv#1025, ref tikv#1028, ref tikv#1031, ref tikv#1043, ref tikv#1055, ref tikv#1075, ref tikv#1091, ref tikv#1094, ref tikv#1102, ref tikv#1103, ref tikv#1118, ref tikv#1146, ref tikv#1148, ref tikv#1150, ref tikv#4399 Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…1.11.1 in /tests/mcs (#5993) ref #897, ref #962, ref #969, ref #974, ref #975, ref #976, ref #986, ref #987, ref prometheus/client_golang#987, ref #989, ref #998, ref #1013, ref #1014, ref #1025, ref #1028, ref #1031, ref #1043, ref #1055, ref #1075, ref #1091, ref #1094, ref #1102, ref #1103, ref #1118, ref #1146, ref #1148, ref #1150, ref #4399 Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ti Chi Robot <[email protected]>

…1.11.1 in /client (#5991) ref #897, ref #962, ref #969, ref #974, ref #975, ref #976, ref #986, ref #987, ref prometheus/client_golang#987, ref #989, ref #998, ref #1013, ref #1014, ref #1025, ref #1028, ref #1031, ref #1043, ref #1055, ref #1075, ref #1091, ref #1094, ref #1102, ref #1103, ref #1118, ref #1146, ref #1148, ref #1150, ref #4399 Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ti Chi Robot <[email protected]>

nolouch added 6 commits July 6, 2018 00:29

update proto

cd02a27

add version

52bee45

update proto

595f133

on change cluster version

2fdd381

use cluster version to switch new feature

efc59f1

add tests

726ef07

rleungx reviewed Jul 13, 2018

View reviewed changes

server/version.go Outdated

// Version record version information.

type Version struct {

Marjor int32

Copy link

Member

rleungx Jul 13, 2018

Choose a reason for hiding this comment

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

s/Marjor/Major/

nolouch mentioned this pull request Jul 13, 2018

server: record version in node tikv/tikv#3319

Merged

nolouch added 4 commits July 13, 2018 17:29

address comments

3178fae

update proto

cbe6d32

merge master

9997f8d

fix response

d18d0aa

siddontang reviewed Jul 13, 2018

View reviewed changes

gregwebs reviewed Jul 13, 2018

View reviewed changes

disksing reviewed Jul 16, 2018

View reviewed changes

nolouch added 5 commits July 16, 2018 14:06

rename version

13bccce

use go-semver

69cf420

fix tests

9d81d84

clean up

9e6866e

handle onchange cluster version failed situation

f51bacf

nolouch requested review from gregwebs, siddontang and disksing July 16, 2018 10:52

gregwebs reviewed Jul 16, 2018

View reviewed changes

disksing reviewed Jul 16, 2018

View reviewed changes

address comments

365da06

nolouch added the status/tests-passed The PR has passed all tests. label Jul 18, 2018

nolouch added 2 commits July 18, 2018 15:12

Merge remote-tracking branch 'upstream/master' into version-introduce

3fa0a54

update proto

4478e41

disksing reviewed Jul 18, 2018

View reviewed changes

nolouch added 2 commits July 19, 2018 11:35

address comment

5eeca3d

fix

3f711b8

disksing reviewed Jul 19, 2018

View reviewed changes

address comment

d099292

disksing reviewed Jul 19, 2018

View reviewed changes

address comment

e40ff3d

siddontang reviewed Jul 19, 2018

View reviewed changes

nolouch added 2 commits July 19, 2018 15:51

Merge branch 'master' into version-introduce

1693736

address comment

8dccabd

Merge branch 'master' into version-introduce

c7c42ca

disksing approved these changes Jul 24, 2018

View reviewed changes

nolouch added 2 commits July 24, 2018 19:50

tiny clear

89a1d93

Merge branch 'master' into version-introduce

a01fc8f

rleungx approved these changes Jul 24, 2018

View reviewed changes

nolouch merged commit 6cf335f into tikv:master Jul 24, 2018

nolouch deleted the version-introduce branch July 24, 2018 13:01

nolouch added a commit to nolouch/pd that referenced this pull request Aug 15, 2018

*: Introduce version checking mechanism (tikv#1148)

3773abb

kennytm mentioned this pull request Sep 4, 2018

Check if the cluster satisfies the version requirements before starting pingcap/tidb-lightning#61

Merged

*: Introduce version checking mechanism #1148

*: Introduce version checking mechanism #1148

Conversation

nolouch commented Jul 13, 2018

What have you changed? (required)

What are the type of the changes (required)?

How has this PR been tested (required)?

Does this PR affect documentation (docs/docs-cn) update? (optional)

Refer to a related PR or issue link (optional)

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

nolouch Jul 17, 2018 • edited Loading

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

nolouch commented Jul 18, 2018

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

nolouch Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

disksing commented Jul 20, 2018

nolouch commented Jul 24, 2018

nolouch Jul 17, 2018 •

edited

Loading

nolouch Jul 19, 2018 •

edited

Loading