Extend tidbcluster v1alpha1 to support configuration and pump #1193

aylei · 2019-11-19T17:54:20Z

What problem does this PR solve?

This PR extends the TidbCluster v1alpha1 type to support the management of component configuration, service and pump.

Support cluster-level setting for components.

There is no logic change introduced.

Next step: controllers, as stated in #1121

Does this PR introduce a user-facing change?:

Support cluster-level setting for components

Signed-off-by: Aylei <[email protected]>

aylei · 2019-11-19T18:00:01Z

pkg/apis/pingcap/v1alpha1/types.go

+
+	// +k8s:openapi-gen=false
+	// TODO: add schema
+	Config map[string]json.JsonObject `json:"config,omitempty"`


Configuration schema and validation requires kernel-side support, I leave these as future works and the first edition of controller-side configuration management will blindly mashal the config object to toml.

aylei · 2019-11-20T01:16:23Z

/run-e2e-in-kind

Signed-off-by: Aylei <[email protected]>

weekface · 2019-11-20T02:25:52Z

Is it compatible with 1.0.x? How do these 1.0.x users upgrade?

DanielZhangQD · 2019-11-20T02:41:11Z

pkg/apis/pingcap/v1alpha1/tidbcluster.go

+		}
+		image = fmt.Sprintf("%s:%s", baseImage, version)
+	}
+	return image


image requires a tag and baseImage cannot have a tag? Why define two images in componentSpec?

version is used as tag, effectively

For backward compatibility, we keep the image field.

aylei · 2019-11-20T04:09:31Z

Is it compatible with 1.0.x? How do these 1.0.x users upgrade?

Yes, no actions required as stated in the design doc. If use specify new specs added in this PR, for example, .spec.tidb.service, the control-loop will try to adopt the orphaned objects created by helm first.

aylei · 2019-11-20T04:12:40Z

pkg/apis/pingcap/v1alpha1/types.go

-	TiDB          TiDBSpec `json:"tidb,omitempty"`
-	TiKV          TiKVSpec `json:"tikv,omitempty"`
-	// +k8s:openapi-gen=false
-	TiKVPromGateway TiKVPromGatewaySpec `json:"tikvPromGateway,omitempty"`


This field is not used anymore, removing this is backward compatible as long as the validation code do not reject this field

aylei · 2019-11-20T04:15:27Z

pkg/apis/pingcap/v1alpha1/tidbcluster.go

+	if baseImage != "" {
+		version := a.ComponentSpec.Version
+		if version == "" {
+			version = a.ClusterSpec.Version


validation constraints will guarantee either .spec.version or .spec..version is not-empty if baseImage is present

pkg/apis/pingcap/v1alpha1/tidbcluster.go

pkg/apis/pingcap/v1alpha1/types.go

tennix · 2019-11-20T04:02:16Z

pkg/apis/pingcap/v1alpha1/types.go

-	Timezone        string                               `json:"timezone,omitempty"`
+
+	// Whether enable PVC reclaim for orphan PVC left by statefulset scale-in
+	EnablePVReclaim bool `json:"enablePVReclaim,omitempty"`


Not related to this PR, this variable name is confusing. Maybe better to rename it as AutoReclaimPV.

I suggest PVCReclaimPolicy with enum value Deferred and Immediate if we are going to do the renaming

pkg/apis/pingcap/v1alpha1/types.go

pkg/util/util.go

Signed-off-by: Aylei <[email protected]>

aylei · 2019-11-21T03:14:08Z

/run-e2e-in-kind

aylei · 2019-11-21T03:19:28Z

pkg/apis/pingcap/v1alpha1/types.go

@@ -90,19 +91,64 @@ type TidbClusterList struct {
 // +k8s:openapi-gen=true
 // TidbClusterSpec describes the attributes that a user creates on a tidb cluster
 type TidbClusterSpec struct {


Modification of .status will be introduced along with the controller logic

Signed-off-by: Aylei <[email protected]>

…e-1182

aylei · 2019-11-21T03:57:27Z

/run-e2e-in-kind

aylei · 2019-11-21T04:02:40Z

/run-e2e-in-kind

aylei · 2019-11-23T08:00:54Z

/run-e2e-in-kind

aylei · 2019-11-23T08:12:14Z

@tennix
After I remove the .spec.tidb.storageClassNamefield, I find that this field has been existing from the very beginning and there is a unit test for it.

We can remove this field and adjust the UT because it is not used anywhere, but I suggest we leaving it as is because TiDB MAY use persistent volume according to the RFC pingcap/tidb#13481, which introduce a built-in logging aggregation mechanism that requires local persistent (for a short time) log storage.

Signed-off-by: Aylei <[email protected]>

…e-1182

aylei · 2019-11-23T09:01:08Z

/run-e2e-in-kind

aylei · 2019-11-23T15:10:54Z

@tennix @weekface PTAL again

DanielZhangQD

LGTM

tennix

LGTM

tennix · 2019-11-25T07:08:25Z

@tennix
After I remove the .spec.tidb.storageClassNamefield, I find that this field has been existing from the very beginning and there is a unit test for it.

We can remove this field and adjust the UT because it is not used anywhere, but I suggest we leaving it as is because TiDB MAY use persistent volume according to the RFC pingcap/tidb#13481, which introduce a built-in logging aggregation mechanism that requires local persistent (for a short time) log storage.

OK, sounds good to me.

weekface · 2019-11-25T07:41:17Z

the conflicts should be resolved

Signed-off-by: Aylei <[email protected]>

aylei · 2019-11-25T08:59:56Z

@weekface conflicts addressed, PTAL

@tennix @DanielZhangQD PTAL again

&
/run-e2e-in-kind

onlymellb · 2019-11-25T10:22:21Z

pkg/apis/pingcap/v1alpha1/tidbcluster.go

+	pp := a.ComponentSpec.ImagePullPolicy
+	if pp == nil {
+		pp = &a.ClusterSpec.ImagePullPolicy
+	}


ClusterSpec.ImagePullPolicy must not be empty?

Yes, there will be a defaulting through mutating-webhook and validation to ensure clusterSpec.ImagePullPolicy and component.ImagePullPolicy is not both empty.

And it's not an issue if clusterSpec.ImagePullPolicy is empty, we just write empty pull policy in PodSpec and use the default of kubernetes

tennix

LGTM

aylei · 2019-11-25T13:30:22Z

/run-e2e-in-kind

DanielZhangQD

LGTM

aylei · 2019-11-26T09:22:31Z

/run-e2e-in-kind

aylei added 2 commits November 20, 2019 01:13

Extend tidbcluster v1alpha1 to support configuration and pump

5f5f1fc

Signed-off-by: Aylei <[email protected]>

Re-generate code

974e289

Signed-off-by: Aylei <[email protected]>

aylei requested review from weekface, cofyc, tennix, onlymellb and DanielZhangQD November 19, 2019 17:54

Add missing fields

8b03f61

Signed-off-by: Aylei <[email protected]>

aylei commented Nov 20, 2019

View reviewed changes

Refine service spec

9a3e851

Signed-off-by: Aylei <[email protected]>

DanielZhangQD reviewed Nov 20, 2019

View reviewed changes

aylei commented Nov 20, 2019

View reviewed changes

aylei added 2 commits November 20, 2019 12:15

Merge branch 'master' into issue-1182

a293880

Merge branch 'master' into issue-1182

356b8cd

tennix reviewed Nov 20, 2019

View reviewed changes

tennix assigned tennix and weekface Nov 20, 2019

aylei added 3 commits November 20, 2019 19:59

Address review comments

387a7c9

Signed-off-by: Aylei <[email protected]>

Merge branch 'master' into issue-1182

1d2036d

Merge branch 'master' into issue-1182

03bb83e

aylei requested a review from LinuxGit November 21, 2019 03:15

aylei commented Nov 21, 2019

View reviewed changes

aylei added 2 commits November 21, 2019 11:21

Fix unit test accordingly

4a89156

Signed-off-by: Aylei <[email protected]>

Merge branch 'issue-1182' of github.com:aylei/tidb-operator into issu…

5fc8cef

…e-1182

aylei added 2 commits November 23, 2019 16:32

Fix UT

6682235

Signed-off-by: Aylei <[email protected]>

Merge branch 'issue-1182' of github.com:aylei/tidb-operator into issu…

d109f36

…e-1182

aylei dismissed stale reviews from weekface and tennix via d109f36 November 23, 2019 08:39

aylei requested review from DanielZhangQD, tennix and weekface November 23, 2019 15:10

DanielZhangQD reviewed Nov 25, 2019

View reviewed changes

tennix previously approved these changes Nov 25, 2019

View reviewed changes

Merge upstream master

9d3df34

Signed-off-by: Aylei <[email protected]>

aylei dismissed tennix’s stale review via 9d3df34 November 25, 2019 08:59

aylei requested review from tennix and DanielZhangQD November 25, 2019 09:00

onlymellb reviewed Nov 25, 2019

View reviewed changes

tennix approved these changes Nov 25, 2019

View reviewed changes

Merge branch 'master' into issue-1182

72ea45c

aylei requested a review from onlymellb November 26, 2019 05:42

DanielZhangQD reviewed Nov 26, 2019

View reviewed changes

weekface approved these changes Nov 26, 2019

View reviewed changes

Merge branch 'master' into issue-1182

32fc5c0

aylei merged commit f67ed33 into pingcap:master Nov 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend tidbcluster v1alpha1 to support configuration and pump #1193

Extend tidbcluster v1alpha1 to support configuration and pump #1193

aylei commented Nov 19, 2019

aylei Nov 19, 2019 •

edited

Loading

aylei commented Nov 20, 2019

weekface commented Nov 20, 2019

DanielZhangQD Nov 20, 2019

aylei Nov 20, 2019

aylei commented Nov 20, 2019

aylei Nov 20, 2019

aylei Nov 20, 2019

tennix Nov 20, 2019

aylei Nov 20, 2019

aylei commented Nov 21, 2019

aylei Nov 21, 2019

aylei commented Nov 21, 2019

aylei commented Nov 21, 2019

aylei commented Nov 23, 2019

aylei commented Nov 23, 2019 •

edited

Loading

aylei commented Nov 23, 2019

aylei commented Nov 23, 2019

DanielZhangQD left a comment

tennix left a comment

tennix commented Nov 25, 2019

weekface commented Nov 25, 2019

aylei commented Nov 25, 2019 •

edited

Loading

onlymellb Nov 25, 2019

aylei Nov 25, 2019

aylei Nov 25, 2019 •

edited

Loading

tennix left a comment

aylei commented Nov 25, 2019

DanielZhangQD left a comment

aylei commented Nov 26, 2019

Extend tidbcluster v1alpha1 to support configuration and pump #1193

Extend tidbcluster v1alpha1 to support configuration and pump #1193

Conversation

aylei commented Nov 19, 2019

What problem does this PR solve?

Does this PR introduce a user-facing change?:

aylei Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

aylei commented Nov 20, 2019

weekface commented Nov 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aylei commented Nov 20, 2019

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

aylei commented Nov 21, 2019

Choose a reason for hiding this comment

aylei commented Nov 21, 2019

aylei commented Nov 21, 2019

aylei commented Nov 23, 2019

aylei commented Nov 23, 2019 • edited Loading

aylei commented Nov 23, 2019

aylei commented Nov 23, 2019

DanielZhangQD left a comment

Choose a reason for hiding this comment

tennix left a comment

Choose a reason for hiding this comment

tennix commented Nov 25, 2019

weekface commented Nov 25, 2019

aylei commented Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aylei Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

tennix left a comment

Choose a reason for hiding this comment

aylei commented Nov 25, 2019

DanielZhangQD left a comment

Choose a reason for hiding this comment

aylei commented Nov 26, 2019

aylei Nov 19, 2019 •

edited

Loading

aylei commented Nov 23, 2019 •

edited

Loading

aylei commented Nov 25, 2019 •

edited

Loading

aylei Nov 25, 2019 •

edited

Loading