add controller for tidb initializer #1403

DanielZhangQD · 2019-12-24T12:34:47Z

What problem does this PR solve?

fix #1262

What is changed and how does it work?

Add controller for TidbInitializer CR

Check List

Tests

Manual test (add detailed scripts or steps below)

Code changes

Has Go code change
Has documents change

Side effects

Related changes

Need to update the documentation

Does this PR introduce a user-facing change?:

Support initializing TiDB Cluster with TidbInitializer Custom Resource.

DanielZhangQD · 2019-12-24T12:38:26Z

Manual test passed, UT and e2e test will be done in #1404.

pkg/apis/pingcap/v1alpha1/tidbcluster.go

aylei · 2019-12-24T12:40:01Z

pkg/apis/pingcap/v1alpha1/tidbinitializer_types.go

@@ -71,13 +71,17 @@ type TidbInitializerSpec struct {

 	// InitSqlConfigMapName reference a configmap that provide init-sql, take high precedence than initSql if set
 	// +optional
-	InitSqlConfigMap *corev1.LocalObjectReference `json:"initSqlConfigMap,omitempty"`


It would be more extensible to use a struct, for example, we could support reference configmap cross namespaces by adding namespace to the object, that is, change LocalObjectReference to ObjectReference

A Pod cannot mount a configmap from other ns, from the definition of ConfigMapVolumeSource, only LocalObjectReference is included.

pkg/manager/member/tidb_init_manager.go

pkg/controller/controller_utils.go

pkg/manager/member/tidb_init_manager.go

pkg/label/label.go

pkg/manager/member/tidb_init_manager.go

pkg/controller/configmap_control.go

pkg/manager/member/tidb_init_manager.go

cofyc · 2019-12-25T03:18:14Z

do we have a basic TiDBInitializer e2e case right now? if not, I prefer adding one with this feature PR instead of another PR. This help reviewers to review PR.

now, we can create tidb cluster without helm, it's very easy to start a tidb cluster. A basic e2e case can be:

create a TiDBInitializer object
create a TiDBCluster object
wait and verify tidb cluster is ready in a certain time

This is an example e2e case which create and verify tidb cluster without helm: https://github.com/pingcap/tidb-operator/pull/1396/files#diff-92579a43c91e473b9716eeeee2068004R653

DanielZhangQD · 2019-12-25T13:10:25Z

@onlymellb @aylei comments addressed, PTAL.
@cofyc I will add e2e soon.

aylei

Rest LGTM

aylei · 2019-12-25T14:48:37Z

pkg/manager/member/tidb_init_manager.go

+	name := controller.TiDBInitializerMemberName(ti.Spec.Clusters.Name)
+	ns := ti.Namespace
+	cm := &corev1.ConfigMap{}
+
+	exist, err := tm.typedControl.Exist(client.ObjectKey{
+		Namespace: ns,
+		Name:      name,
+	}, cm)
+	if err != nil {
+		return err
+	}
+	if !exist {
+		err = tm.syncTiDBInitConfigMap(ti)
+		if err != nil {
+			return err
+		}
+	}


Instead of check-and-create, it is recommended to create the object directly and expect no err or an AlreadyExisted error if the only purpose of the check is to determine whether we should create the object, which is atomic compared to two separate API calls.

Also, this could make the logic simple and the name calculation of TiDB init configmap don't have to be maintained in two places

For a one-time deal, I agree that we can just create the object, but here is the logic in sync, it will be done at least periodically, it's not reasonable to create the object each time.

Check before creation can reduce the API call to kube-apiserver, and I have moved the check block to separate sync functions.

aylei · 2019-12-25T14:49:19Z

pkg/manager/member/tidb_init_manager.go

+	job, err := tm.jobLister.Jobs(ns).Get(name)
+	if err == nil {
+		// job has been created
+		return tm.updateStatus(ti.DeepCopy(), job)
+	}
+
+	if !errors.IsNotFound(err) {
+		return fmt.Errorf("TiDBInitializer %s/%s get job %s failed, err: %v", ns, ti.Name, name, err)
+	}
+
+	return tm.syncTiDBInitJob(ti)


Check before creation can reduce the API call to kube-apiserver, and I have moved the check block to separate sync functions.

pkg/manager/member/tidb_init_manager.go

aylei · 2019-12-25T15:00:44Z

pkg/manager/member/tidb_init_manager.go

+	var update bool
+	if !apiequality.Semantic.DeepEqual(ti.Status.JobStatus, job.Status) {
+		job.Status.DeepCopyInto(&ti.Status.JobStatus)
+		update = true
+	}
+	if ti.Status.Phase != phase {
+		ti.Status.Phase = phase
+		update = true
+	}
+	if update {
+		return tm.retryUpdateStatus(ti)
+	}
+	return nil


This could be simplified using GuaranteedUpdate, we don't have to check the equality in client-side actually, if the update do not mutate the object, the resourceVersion of object won't be changed, that is, the object will not be affected at all.

Check before creation can reduce the API call to kube-apiserver, and I have updated to use GuaranteedUpdate.

DanielZhangQD · 2019-12-26T03:45:03Z

@aylei comments addressed, PTAL again.

onlymellb

LGTM

aylei

LGTM

aylei · 2019-12-26T03:56:29Z

/merge

sre-bot · 2019-12-26T03:56:38Z

/run-all-tests

sre-bot · 2019-12-26T04:10:12Z

/run-all-tests

* zh: doc for how to evict tikv pod * zh: fix script syntax * zh: add notes for deprecated commands * zh: add doc to maintain PD instance * Update maintain-a-kubernetes-node.md * fix broken link * Update maintain-a-kubernetes-node.md * Update maintain-a-kubernetes-node.md * Update maintain-a-kubernetes-node.md * Apply suggestions from code review Co-authored-by: Grace Cai <qqzczy@126.com> * fix comments * Apply suggestions from code review Co-authored-by: Grace Cai <qqzczy@126.com> * Apply suggestions from code review * Apply suggestions from code review Co-authored-by: Grace Cai <qqzczy@126.com> * fix lint * fix lint Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com> Co-authored-by: Grace Cai <qqzczy@126.com>

DanielZhangQD requested review from cofyc, LinuxGit, onlymellb, Yisaer and aylei December 24, 2019 12:34

DanielZhangQD mentioned this pull request Dec 24, 2019

UT & e2e test for TidbInitializer controller #1404

Closed

aylei reviewed Dec 24, 2019

View reviewed changes

onlymellb reviewed Dec 24, 2019

View reviewed changes

pkg/label/label.go Outdated Show resolved Hide resolved

onlymellb reviewed Dec 24, 2019

View reviewed changes

pkg/manager/member/tidb_init_manager.go Show resolved Hide resolved

onlymellb reviewed Dec 24, 2019

View reviewed changes

pkg/manager/member/tidb_init_manager.go Outdated Show resolved Hide resolved

onlymellb reviewed Dec 24, 2019

View reviewed changes

pkg/controller/configmap_control.go Outdated Show resolved Hide resolved

onlymellb reviewed Dec 24, 2019

View reviewed changes

pkg/manager/member/tidb_init_manager.go Outdated Show resolved Hide resolved

onlymellb reviewed Dec 24, 2019

View reviewed changes

pkg/manager/member/tidb_init_manager.go Outdated Show resolved Hide resolved

DanielZhangQD added 2 commits December 25, 2019 21:11

add controller for tidb initializer

460c6d8

address comments

91dc9c9

aylei reviewed Dec 25, 2019

View reviewed changes

address comments

61ddb9f

DanielZhangQD force-pushed the init branch from d0e4b55 to 61ddb9f Compare December 26, 2019 03:41

Merge branch 'master' into init

4a76183

onlymellb approved these changes Dec 26, 2019

View reviewed changes

aylei approved these changes Dec 26, 2019

View reviewed changes

sre-bot added the status/can-merge label Dec 26, 2019

Merge branch 'master' into init

b66a48c

sre-bot merged commit eb67f58 into pingcap:master Dec 26, 2019

DanielZhangQD deleted the init branch December 26, 2019 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add controller for tidb initializer #1403

add controller for tidb initializer #1403

DanielZhangQD commented Dec 24, 2019

DanielZhangQD commented Dec 24, 2019

aylei Dec 24, 2019

DanielZhangQD Dec 25, 2019

cofyc commented Dec 25, 2019

DanielZhangQD commented Dec 25, 2019

aylei left a comment

aylei Dec 25, 2019

DanielZhangQD Dec 26, 2019

DanielZhangQD Dec 26, 2019

aylei Dec 25, 2019

DanielZhangQD Dec 26, 2019

aylei Dec 25, 2019

DanielZhangQD Dec 26, 2019

DanielZhangQD commented Dec 26, 2019

onlymellb left a comment

aylei left a comment

aylei commented Dec 26, 2019

sre-bot commented Dec 26, 2019

sre-bot commented Dec 26, 2019

add controller for tidb initializer #1403

add controller for tidb initializer #1403

Conversation

DanielZhangQD commented Dec 24, 2019

What problem does this PR solve?

What is changed and how does it work?

Check List

Does this PR introduce a user-facing change?:

DanielZhangQD commented Dec 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc commented Dec 25, 2019

DanielZhangQD commented Dec 25, 2019

aylei 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielZhangQD commented Dec 26, 2019

onlymellb left a comment

Choose a reason for hiding this comment

aylei left a comment

Choose a reason for hiding this comment

aylei commented Dec 26, 2019

sre-bot commented Dec 26, 2019

sre-bot commented Dec 26, 2019