Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sync tidb configmap in controller #1291

Merged
merged 17 commits into from
Dec 11, 2019
Merged

sync tidb configmap in controller #1291

merged 17 commits into from
Dec 11, 2019

Conversation

aylei
Copy link
Contributor

@aylei aylei commented Dec 5, 2019

Signed-off-by: Aylei [email protected]

What problem does this PR solve?

close #1192

What is changed and how does it work?

New controller logic to sync tidb configmap.

Generify the CreateOrUpdate logic and the template rendering code.

This PR introduce a new dependency controller-runtime in favor of its generic client for kubernetes.

Check List

Tests

  • E2E test

Does this PR introduce a user-facing change?:

The configuration of tidb-server can be managed in the `TidbCluster` resource now.

check the e2e case to get an overview of this PR: https://github.com/pingcap/tidb-operator/pull/1291/files#diff-92579a43c91e473b9716eeeee2068004

@onlymellb @cofyc @tennix @weekface @Yisaer PTAL

@aylei
Copy link
Contributor Author

aylei commented Dec 5, 2019

/run-e2e-test

3 similar comments
@aylei
Copy link
Contributor Author

aylei commented Dec 5, 2019

/run-e2e-test

@aylei
Copy link
Contributor Author

aylei commented Dec 5, 2019

/run-e2e-test

@aylei
Copy link
Contributor Author

aylei commented Dec 6, 2019

/run-e2e-test

@aylei aylei marked this pull request as ready for review December 6, 2019 10:48
@aylei
Copy link
Contributor Author

aylei commented Dec 6, 2019

/run-e2e-test

cmC.Labels = cmD.Labels
for k, v := range cmD.Annotations {
cmC.Annotations[k] = v
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why we must keep old annotations?

Copy link
Contributor Author

@aylei aylei Dec 6, 2019

Choose a reason for hiding this comment

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

It is not a must. I preserve the un-overridden annotations deliberately, which allows user and third-party components could safely add their annotations (as long as the annotation key is not taken by tidb-operator) to resources owned by TidbCluster.

ns := tc.GetNamespace()
tcName := tc.GetName()
instanceName := tc.GetLabels()[label.InstanceLabelKey]
tidbConfigMap := controller.MemberConfigMapName(tc, v1alpha1.TiDBMemberType)
if cm != nil {
tidbConfigMap = cm.Name
}
Copy link
Contributor

Choose a reason for hiding this comment

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

in which case, cm will be nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when .spec.tidb.config is nil

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

unified grep command for mac and linux (pingcap#1285)

Refactor apply

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

pdapi: only read secret for PD client cert (on TLS enabled clusters) … (pingcap#1290)

* pdapi: only read secret for PD client cert (on TLS enabled clusters) when needed

* pdapi: adjust log message

* pdapi: fix error in logging

Remove DinD related scripts and document (pingcap#1283)

* Remove DinD related scripts

* Remove local-dind-tutorial doc

* drop dind part from CONTRIBUTING guide.

Fix configmap ownerreference

Signed-off-by: Aylei <[email protected]>
@aylei
Copy link
Contributor Author

aylei commented Dec 7, 2019

/run-e2e-test

@aylei
Copy link
Contributor Author

aylei commented Dec 7, 2019

I'm not quite satisfied with the generalization made in this PR and find that controller-runtime has better implementation for common controller operations, refactoring to use controller-runtime now.

Marked as DNM, sorry for the reviewers @cofyc @onlymellb

@aylei aylei changed the title sync tidb configmap in controller [DNM] sync tidb configmap in controller Dec 7, 2019
@aylei
Copy link
Contributor Author

aylei commented Dec 8, 2019

/run-e2e-test

@aylei aylei changed the title [DNM] sync tidb configmap in controller sync tidb configmap in controller Dec 8, 2019

// GenericControlInterface manages generic object that managed by an arbitrary controller
type GenericControlInterface interface {
CreateOrUpdate(controller, obj runtime.Object, mergeFn MergeFn) (runtime.Object, error)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apply is ambiguous with the server-side apply of kubernetes, rename to CreateOrUpdate


// 5. check if the copy is actually mutated
if !apiequality.Semantic.DeepEqual(existing, mutated) {
err := c.client.Update(context.TODO(), mutated)
Copy link
Contributor Author

@aylei aylei Dec 8, 2019

Choose a reason for hiding this comment

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

Optionally, we could retry on conflict like other controls do, I choose no retry here because CreateOrUpdate will never meet conflicts if there is no concurrent modification, and if there are concurrent modifications, it is better to return the error to caller instead of aggressive retrying.

@@ -114,6 +116,11 @@ func main() {
if err != nil {
glog.Fatalf("failed to get advanced-statefulset Clientset: %v", err)
}
// TODO: optimize the read of genericCli with the shared cache
genericCli, err := client.New(cfg, client.Options{Scheme: scheme.Scheme})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the client of controller-runtime without using the informer of controller-runtime has a drawback that all read operations will go to kube-apiserver, it is acceptable now because the only read operation is to get the latest version when perform a CreateOrUpdate.

// override. Note that aggressive override usually causes unnecessary updates because the object will be mutated
// after POST/PUT to api-server (e.g. Defaulting), an annotation based technique could be used to avoid such
// updating: set a last-applied-config annotation and diff the annotation instead of the real spec.
type MergeFn func(existing, desired runtime.Object) error
Copy link
Contributor Author

@aylei aylei Dec 8, 2019

Choose a reason for hiding this comment

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

server-side apply maybe a better solution in the future.

ref: https://kubernetes.io/docs/reference/using-api/api-concepts/#server-side-apply

Signed-off-by: Aylei <[email protected]>
@aylei
Copy link
Contributor Author

aylei commented Dec 8, 2019

/run-e2e-test

@aylei aylei requested review from cofyc and onlymellb December 8, 2019 13:08
@aylei
Copy link
Contributor Author

aylei commented Dec 9, 2019

/run-e2e-test

1 similar comment
@aylei
Copy link
Contributor Author

aylei commented Dec 9, 2019

/run-e2e-test

@aylei aylei requested review from cofyc and weekface December 9, 2019 16:05
@aylei
Copy link
Contributor Author

aylei commented Dec 9, 2019

@weekface @cofyc I've addressed the comments, PTAL again

@aylei
Copy link
Contributor Author

aylei commented Dec 10, 2019

/run-e2e-test

@aylei
Copy link
Contributor Author

aylei commented Dec 10, 2019

/run-e2e-test

@aylei
Copy link
Contributor Author

aylei commented Dec 10, 2019

/run-e2e-test

cofyc
cofyc previously approved these changes Dec 10, 2019
Copy link
Contributor

@cofyc cofyc left a comment

Choose a reason for hiding this comment

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

LGTM

@aylei
Copy link
Contributor Author

aylei commented Dec 10, 2019

@weekface @tennix Could you please take a look again?

@aylei
Copy link
Contributor Author

aylei commented Dec 10, 2019

/run-e2e-test

Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

Rest LGTM

UpdateConfigMap(*v1alpha1.TidbCluster, *corev1.ConfigMap) (*corev1.ConfigMap, error)
DeleteConfigMap(*v1alpha1.TidbCluster, *corev1.ConfigMap) error
// CreateConfigMap create the given ConfigMap
CreateConfigMap(runtime.Object, *corev1.ConfigMap) (*corev1.ConfigMap, error)
Copy link
Member

Choose a reason for hiding this comment

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

Should add a comment for what the runtime.Object is.

if config.Security == nil {
config.Security = &v1alpha1.Security{}
}
config.Security.ClusterSSLCA = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt"
Copy link
Member

Choose a reason for hiding this comment

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

It's better to define these 3 paths as constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

@aylei
Copy link
Contributor Author

aylei commented Dec 10, 2019

/run-e2e-test

@aylei aylei requested review from tennix and cofyc December 10, 2019 16:57
Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

@cofyc cofyc merged commit f13b93b into pingcap:master Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/PTAL PR needs to be reviewed type/refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sync TiDB Configuration and start-script in control-loop
5 participants