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 service in control-loop #1242

Merged
merged 11 commits into from
Nov 29, 2019
112 changes: 110 additions & 2 deletions pkg/manager/member/tidb_member_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,11 @@ func (tmm *tidbMemberManager) Sync(tc *v1alpha1.TidbCluster) error {
}

// Sync Tidb StatefulSet
return tmm.syncTiDBStatefulSetForTidbCluster(tc)
if err := tmm.syncTiDBStatefulSetForTidbCluster(tc); err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit worried that if something goes wrong with sts synchronization, then the service will not be synchronized too.

Copy link
Contributor Author

@aylei aylei Nov 27, 2019

Choose a reason for hiding this comment

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

Yes, such dangers are everywhere in today's tidb-controller-manager because of synchronized reconciling, this should be a systematic improvement that I'd like to collect efforts in a dedicated ticket

Copy link
Contributor Author

Choose a reason for hiding this comment

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

follow up: #1245

}

return tmm.syncTiDBService(tc)
}

func (tmm *tidbMemberManager) syncTiDBHeadlessServiceForTidbCluster(tc *v1alpha1.TidbCluster) error {
Expand Down Expand Up @@ -122,7 +126,7 @@ func (tmm *tidbMemberManager) syncTiDBHeadlessServiceForTidbCluster(tc *v1alpha1
if !equal {
svc := *oldSvc
svc.Spec = newSvc.Spec
err = SetServiceLastAppliedConfigAnnotation(newSvc)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix an old typo

err = SetServiceLastAppliedConfigAnnotation(&svc)
if err != nil {
return err
}
Expand Down Expand Up @@ -297,6 +301,110 @@ func (tmm *tidbMemberManager) syncTiDBClientCerts(tc *v1alpha1.TidbCluster) erro
return tmm.certControl.Create(controller.GetOwnerRef(tc), certOpts)
}

func (tmm *tidbMemberManager) syncTiDBService(tc *v1alpha1.TidbCluster) error {
aylei marked this conversation as resolved.
Show resolved Hide resolved

newSvc := getNewTiDBServiceOrNil(tc)
// TODO: delete tidb service if user remove the service spec deliberately
if newSvc == nil {
return nil
}

ns := newSvc.Namespace

oldSvcTmp, err := tmm.svcLister.Services(ns).Get(newSvc.Name)
if errors.IsNotFound(err) {
err = SetServiceLastAppliedConfigAnnotation(newSvc)
if err != nil {
return err
}
return tmm.svcControl.CreateService(tc, newSvc)
}
if err != nil {
return err
}
oldSvc := oldSvcTmp.DeepCopy()

// Adopt orphaned service by simply overriding
if metav1.GetControllerOf(oldSvc) == 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.

Adopting orphaned objects is a systematic problem that should be considered at project level in a separate issue, this PR adopts the TiDB service only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

err = SetServiceLastAppliedConfigAnnotation(newSvc)
if err != nil {
return err
}
_, err = tmm.svcControl.UpdateService(tc, newSvc)
return err
}

equal, err := serviceEqual(newSvc, oldSvc)
if err != nil {
return err
}
annoEqual := isSubMapOf(newSvc.Annotations, oldSvc.Annotations)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Annotation directly added by user will be retained.

if !equal || !annoEqual {
svc := *oldSvc
svc.Spec = newSvc.Spec
aylei marked this conversation as resolved.
Show resolved Hide resolved
err = SetServiceLastAppliedConfigAnnotation(&svc)
if err != nil {
return err
}
// apply change of annotations
for k, v := range newSvc.Annotations {
svc.Annotations[k] = v
}
_, err = tmm.svcControl.UpdateService(tc, &svc)
return err
}

return nil
}

func getNewTiDBServiceOrNil(tc *v1alpha1.TidbCluster) *corev1.Service {

svcSpec := tc.Spec.TiDB.Service
if svcSpec == nil {
return nil
}

ns := tc.Namespace
tcName := tc.Name
instanceName := tc.GetLabels()[label.InstanceLabelKey]
tidbLabels := label.New().Instance(instanceName).TiDB().Labels()
svcName := controller.TiDBMemberName(tcName)

ports := []corev1.ServicePort{
{
Name: "mysql-client",
Port: 4000,
TargetPort: intstr.FromInt(4000),
Protocol: corev1.ProtocolTCP,
},
}
if svcSpec.ExposeStatus {
ports = append(ports, corev1.ServicePort{
Name: "status",
Port: 10080,
TargetPort: intstr.FromInt(10080),
Protocol: corev1.ProtocolTCP,
})
}

return &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: svcName,
Namespace: ns,
Labels: tidbLabels,
Annotations: svcSpec.Annotations,
OwnerReferences: []metav1.OwnerReference{controller.GetOwnerRef(tc)},
},
Spec: corev1.ServiceSpec{
Type: svcSpec.Type,
Ports: ports,
ExternalTrafficPolicy: svcSpec.ExternalTrafficPolicy,
Copy link
Member

Choose a reason for hiding this comment

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

For api server, this should have validation and defaulting.

LoadBalancerIP: svcSpec.LoadBalancerIP,
Selector: tidbLabels,
},
}
}

func getNewTiDBHeadlessServiceForTidbCluster(tc *v1alpha1.TidbCluster) *corev1.Service {
ns := tc.Namespace
tcName := tc.Name
Expand Down
Loading