Render TiProxy labels into configmap (#6937) (#6943)

Author: ti-chi-bot

api/core/v1alpha1/tiproxy_types.go

@@ -193,9 +193,8 @@ type TiProxyServer struct {
 	Ports TiProxyPorts `json:"ports,omitempty"`
 
 	// Labels defines the server labels of the TiProxy server.
-	// Operator will set these `labels` by API.
+	// Operator will render these `labels` into the TiProxy config file.
 	// If a label in this field is conflict with the config file, this field takes precedence.
-	// NOTE: If a label is removed, operator will not delete it automatically.
 	// NOTE: these label keys are managed by TiDB Operator, it will be set automatically and you can not modify them:
 	//  - host
 	//  - region

manifests/crd/core.pingcap.com_tiproxies.yaml

@@ -8518,9 +8518,8 @@ spec:
                       type: string
                     description: |-
                       Labels defines the server labels of the TiProxy server.
-                      Operator will set these `labels` by API.
+                      Operator will render these `labels` into the TiProxy config file.
                       If a label in this field is conflict with the config file, this field takes precedence.
-                      NOTE: If a label is removed, operator will not delete it automatically.
                       NOTE: these label keys are managed by TiDB Operator, it will be set automatically and you can not modify them:
                        - host
                        - region

manifests/crd/core.pingcap.com_tiproxygroups.yaml

@@ -8806,9 +8806,8 @@ spec:
                               type: string
                             description: |-
                               Labels defines the server labels of the TiProxy server.
-                              Operator will set these `labels` by API.
+                              Operator will render these `labels` into the TiProxy config file.
                               If a label in this field is conflict with the config file, this field takes precedence.
-                              NOTE: If a label is removed, operator will not delete it automatically.
                               NOTE: these label keys are managed by TiDB Operator, it will be set automatically and you can not modify them:
                                - host
                                - region

pkg/configs/tiproxy/config.go

@@ -23,6 +23,7 @@ import (
 	"github.com/pingcap/tidb-operator/api/v2/core/v1alpha1"
 	coreutil "github.com/pingcap/tidb-operator/v2/pkg/apiutil/core/v1alpha1"
 	"github.com/pingcap/tidb-operator/v2/pkg/runtime/scope"
+	maputil "github.com/pingcap/tidb-operator/v2/pkg/utils/map"
 	stringutil "github.com/pingcap/tidb-operator/v2/pkg/utils/string"
 )
 
@@ -74,6 +75,9 @@ func (c *Config) Overlay(cluster *v1alpha1.Cluster, tiproxy *v1alpha1.TiProxy) e
 	c.Proxy.AdvertiseAddress = getAdvertiseAddress(cluster, tiproxy)
 	c.Proxy.PDAddress = stringutil.RemoveHTTPPrefix(cluster.Status.PD)
 	c.API.Address = coreutil.ListenAddress(coreutil.TiProxyAPIPort(tiproxy))
+	if len(tiproxy.Spec.Server.Labels) > 0 {
+		c.Labels = maputil.Merge(c.Labels, tiproxy.Spec.Server.Labels)
+	}
 
 	if coreutil.IsTLSClusterEnabled(cluster) {
 		c.Security.ClusterTLS.CA = path.Join(v1alpha1.DirPathClusterTLSTiProxy, corev1.ServiceAccountRootCAKey)

pkg/configs/tiproxy/config_test.go

@@ -284,3 +284,46 @@ func TestOverlay(t *testing.T) {
 		})
 	}
 }
+
+func TestOverlayLabels(t *testing.T) {
+	cluster := &v1alpha1.Cluster{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "ns1",
+			Name:      "db",
+		},
+		Status: v1alpha1.ClusterStatus{
+			PD: "http://db-pd.ns1:2379",
+		},
+	}
+	tiproxy := &v1alpha1.TiProxy{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "ns1",
+			Name:      "db-foo",
+		},
+		Spec: v1alpha1.TiProxySpec{
+			Subdomain: "db-tiproxy-peer",
+			TiProxyTemplateSpec: v1alpha1.TiProxyTemplateSpec{
+				Server: v1alpha1.TiProxyServer{
+					Labels: map[string]string{
+						"zone": "from-server-labels",
+						"env":  "prod",
+					},
+				},
+			},
+		},
+	}
+
+	got := &Config{
+		Labels: map[string]string{
+			"rack": "rack-1",
+			"zone": "from-config",
+		},
+	}
+	err := got.Overlay(cluster, tiproxy)
+	require.NoError(t, err)
+	assert.Equal(t, map[string]string{
+		"rack": "rack-1",
+		"zone": "from-server-labels",
+		"env":  "prod",
+	}, got.Labels)
+}

pkg/controllers/tiproxy/builder.go

@@ -15,8 +15,6 @@
 package tiproxy
 
 import (
-	"context"
-
 	"github.com/pingcap/tidb-operator/v2/pkg/controllers/common"
 	"github.com/pingcap/tidb-operator/v2/pkg/controllers/tiproxy/tasks"
 	"github.com/pingcap/tidb-operator/v2/pkg/runtime/scope"
@@ -73,10 +71,6 @@ func (r *Reconciler) NewRunner(state *tasks.ReconcileContext, reporter task.Task
 		tasks.TaskConfigMap(state, r.Client),
 		common.TaskPVC[scope.TiProxy](state, r.Client, r.VolumeModifierFactory, tasks.PVCNewer()),
 		tasks.TaskPod(state, r.Client),
-		common.TaskServerLabels[scope.TiProxy](state, r.Client, r.PDClientManager, func(ctx context.Context, labels map[string]string) error {
-			// TODO(liubo02): compare before setting
-			return state.TiProxyClient.SetLabels(ctx, labels)
-		}),
 		common.TaskInstanceConditionSynced[scope.TiProxy](state),
 		common.TaskInstanceConditionReady[scope.TiProxy](state),
 		common.TaskInstanceConditionRunning[scope.TiProxy](state),

pkg/controllers/tiproxy/tasks/cm_test.go

@@ -27,19 +27,22 @@ import (
 
 	"github.com/pingcap/tidb-operator/api/v2/core/v1alpha1"
 	"github.com/pingcap/tidb-operator/v2/pkg/client"
+	tiproxycfg "github.com/pingcap/tidb-operator/v2/pkg/configs/tiproxy"
 	"github.com/pingcap/tidb-operator/v2/pkg/utils/fake"
 	"github.com/pingcap/tidb-operator/v2/pkg/utils/task/v3"
+	"github.com/pingcap/tidb-operator/v2/pkg/utils/toml"
 )
 
 const fakePDAddr = "any string, useless in test"
 
 func TestTaskConfigMap(t *testing.T) {
 	cases := []struct {
-		desc          string
-		state         *ReconcileContext
-		objs          []client.Object
-		unexpectedErr bool
-		invalidConfig bool
+		desc           string
+		state          *ReconcileContext
+		objs           []client.Object
+		unexpectedErr  bool
+		invalidConfig  bool
+		expectedLabels map[string]string
 
 		expectedStatus task.Status
 	}{
@@ -88,6 +91,29 @@ func TestTaskConfigMap(t *testing.T) {
 			},
 			expectedStatus: task.SFail,
 		},
+		{
+			desc: "server labels are rendered into config",
+			state: &ReconcileContext{
+				State: &state{
+					tiproxy: fake.FakeObj("aaa-xxx", func(obj *v1alpha1.TiProxy) *v1alpha1.TiProxy {
+						obj.Spec.Server.Labels = map[string]string{
+							"env":  "prod",
+							"rack": "rack-1",
+						}
+						return obj
+					}),
+					cluster: fake.FakeObj("cluster", func(obj *v1alpha1.Cluster) *v1alpha1.Cluster {
+						obj.Status.PD = fakePDAddr
+						return obj
+					}),
+				},
+			},
+			expectedLabels: map[string]string{
+				"env":  "prod",
+				"rack": "rack-1",
+			},
+			expectedStatus: task.SComplete,
+		},
 		{
 			desc: "has config map",
 			state: &ReconcileContext{
@@ -166,6 +192,12 @@ func TestTaskConfigMap(t *testing.T) {
 				}
 				require.NoError(tt, fc.Get(ctx, client.ObjectKeyFromObject(&cm), &cm), c.desc)
 				assert.NotEmpty(tt, cm.Data, c.desc)
+				if c.expectedLabels != nil {
+					cfg := tiproxycfg.Config{}
+					decoder, _ := toml.Codec[tiproxycfg.Config]()
+					require.NoError(tt, decoder.Decode([]byte(cm.Data[v1alpha1.FileNameConfig]), &cfg), c.desc)
+					assert.Equal(tt, c.expectedLabels, cfg.Labels, c.desc)
+				}
 			}
 		})
 	}

pkg/reloadable/tiproxy.go

@@ -99,9 +99,6 @@ func convertTiProxyTemplate(tmpl *v1alpha1.TiProxyTemplate) *v1alpha1.TiProxyTem
 	newTmpl.Labels = convertLabels(newTmpl.Labels)
 	newTmpl.Annotations = convertAnnotations(newTmpl.Annotations)
 
-	// server labels can be updated dynamically
-	newTmpl.Spec.Server.Labels = nil
-
 	newTmpl.Spec.Volumes = convertVolumes(newTmpl.Spec.Volumes)
 	newTmpl.Spec.Overlay = convertOverlay(newTmpl.Spec.Overlay)
 
@@ -111,14 +108,17 @@ func convertTiProxyTemplate(tmpl *v1alpha1.TiProxyTemplate) *v1alpha1.TiProxyTem
 func equalTiProxyTemplate(c, p *v1alpha1.TiProxyTemplate) bool {
 	p = convertTiProxyTemplate(p)
 	c = convertTiProxyTemplate(c)
+	serverLabelsChanged := !apiequality.Semantic.DeepEqual(p.Spec.Server.Labels, c.Spec.Server.Labels)
 	// not equal only when current strategy is Restart and config is changed
-	if c.Spec.UpdateStrategy.Config == v1alpha1.ConfigUpdateStrategyRestart && p.Spec.Config != c.Spec.Config {
+	if c.Spec.UpdateStrategy.Config == v1alpha1.ConfigUpdateStrategyRestart && (p.Spec.Config != c.Spec.Config || serverLabelsChanged) {
 		return false
 	}
 
 	// ignore these fields
 	p.Spec.Config = ""
 	c.Spec.Config = ""
+	p.Spec.Server.Labels = nil
+	c.Spec.Server.Labels = nil
 	p.Spec.UpdateStrategy.Config = ""
 	c.Spec.UpdateStrategy.Config = ""
 

pkg/reloadable/tiproxy_test.go

@@ -18,6 +18,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/pingcap/tidb-operator/api/v2/core/v1alpha1"
@@ -62,3 +63,48 @@ func TestCheckTiProxy(t *testing.T) {
 		})
 	}
 }
+
+func TestCheckTiProxyPodWhenServerLabelsChange(t *testing.T) {
+	tests := []struct {
+		name      string
+		strategy  v1alpha1.ConfigUpdateStrategy
+		wantCheck bool
+	}{
+		{
+			name:      "restart strategy",
+			strategy:  v1alpha1.ConfigUpdateStrategyRestart,
+			wantCheck: false,
+		},
+		{
+			name:      "hot reload strategy",
+			strategy:  v1alpha1.ConfigUpdateStrategyHotReload,
+			wantCheck: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			previous := tiproxyWithServerLabels(tt.strategy, map[string]string{"zone": "z1"})
+			current := tiproxyWithServerLabels(tt.strategy, map[string]string{"zone": "z2"})
+			pod := &corev1.Pod{}
+			MustEncodeLastTiProxyTemplate(previous, pod)
+
+			assert.Equal(t, tt.wantCheck, CheckTiProxyPod(current, pod))
+		})
+	}
+}
+
+func tiproxyWithServerLabels(strategy v1alpha1.ConfigUpdateStrategy, labels map[string]string) *v1alpha1.TiProxy {
+	return &v1alpha1.TiProxy{
+		Spec: v1alpha1.TiProxySpec{
+			TiProxyTemplateSpec: v1alpha1.TiProxyTemplateSpec{
+				Server: v1alpha1.TiProxyServer{
+					Labels: labels,
+				},
+				UpdateStrategy: v1alpha1.UpdateStrategy{
+					Config: strategy,
+				},
+			},
+		},
+	}
+}