ddl: Remove explicit exclude for "engine" notIn "tiflash" (#58637)

close #58633

pkg/ddl/placement/bundle.go

@@ -314,24 +314,16 @@ func (b *Bundle) String() string {
 
 // Tidy will post optimize Rules, trying to generate rules that suits PD.
 func (b *Bundle) Tidy() error {
+	// refer to tidb#58633
+	// Does not explicitly set exclude rule with label.key==EngineLabelKey, because the
+	// PD may wrongly add peer to the unexpected stores if that key is specified.
 	tempRules := b.Rules[:0]
 	id := 0
 	for _, rule := range b.Rules {
 		// useless Rule
 		if rule.Count <= 0 {
 			continue
 		}
-		// refer to tidb#22065.
-		// add -engine=tiflash to every rule to avoid schedules to tiflash instances.
-		// placement rules in SQL is not compatible with `set tiflash replica` yet
-		err := AddConstraint(&rule.LabelConstraints, pd.LabelConstraint{
-			Op:     pd.NotIn,
-			Key:    EngineLabelKey,
-			Values: []string{EngineLabelTiFlash},
-		})
-		if err != nil {
-			return err
-		}
 		rule.ID = strconv.Itoa(id)
 		tempRules = append(tempRules, rule)
 		id++

pkg/ddl/placement/bundle_test.go

@@ -347,9 +347,11 @@ func TestString(t *testing.T) {
 	require.NoError(t, err)
 	rules2, err := newRules(pd.Voter, 4, `["-zone=sh", "+zone=bj"]`)
 	require.NoError(t, err)
-	bundle.Rules = append(rules1, rules2...)
+	rules3, err := newRules(pd.Voter, 3, `["-engine=tiflash", "-engine=tiflash_compute"]`)
+	require.NoError(t, err)
+	bundle.Rules = append(append(rules1, rules2...), rules3...)
 
-	require.Equal(t, "{\"group_id\":\"TiDB_DDL_1\",\"group_index\":0,\"group_override\":false,\"rules\":[{\"group_id\":\"\",\"id\":\"\",\"start_key\":\"\",\"end_key\":\"\",\"role\":\"voter\",\"is_witness\":false,\"count\":3,\"label_constraints\":[{\"key\":\"zone\",\"op\":\"in\",\"values\":[\"sh\"]}]},{\"group_id\":\"\",\"id\":\"\",\"start_key\":\"\",\"end_key\":\"\",\"role\":\"voter\",\"is_witness\":false,\"count\":4,\"label_constraints\":[{\"key\":\"zone\",\"op\":\"notIn\",\"values\":[\"sh\"]},{\"key\":\"zone\",\"op\":\"in\",\"values\":[\"bj\"]}]}]}", bundle.String())
+	require.Equal(t, "{\"group_id\":\"TiDB_DDL_1\",\"group_index\":0,\"group_override\":false,\"rules\":[{\"group_id\":\"\",\"id\":\"\",\"start_key\":\"\",\"end_key\":\"\",\"role\":\"voter\",\"is_witness\":false,\"count\":3,\"label_constraints\":[{\"key\":\"zone\",\"op\":\"in\",\"values\":[\"sh\"]}]},{\"group_id\":\"\",\"id\":\"\",\"start_key\":\"\",\"end_key\":\"\",\"role\":\"voter\",\"is_witness\":false,\"count\":4,\"label_constraints\":[{\"key\":\"zone\",\"op\":\"notIn\",\"values\":[\"sh\"]},{\"key\":\"zone\",\"op\":\"in\",\"values\":[\"bj\"]}]},{\"group_id\":\"\",\"id\":\"\",\"start_key\":\"\",\"end_key\":\"\",\"role\":\"voter\",\"is_witness\":false,\"count\":3,\"label_constraints\":[{\"key\":\"engine\",\"op\":\"notIn\",\"values\":[\"tiflash\"]},{\"key\":\"engine\",\"op\":\"notIn\",\"values\":[\"tiflash_compute\"]}]}]}", bundle.String())
 
 	require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/pkg/ddl/placement/MockMarshalFailure", `return(true)`))
 	defer func() {
@@ -956,12 +958,7 @@ func TestTidy(t *testing.T) {
 	require.NoError(t, err)
 	require.Len(t, bundle.Rules, 1)
 	require.Equal(t, "0", bundle.Rules[0].ID)
-	require.Len(t, bundle.Rules[0].LabelConstraints, 3)
-	require.Equal(t, pd.LabelConstraint{
-		Op:     pd.NotIn,
-		Key:    EngineLabelKey,
-		Values: []string{EngineLabelTiFlash},
-	}, bundle.Rules[0].LabelConstraints[2])
+	require.Len(t, bundle.Rules[0].LabelConstraints, 2)
 
 	// merge
 	rules3, err := newRules(pd.Follower, 4, "")
@@ -986,13 +983,7 @@ func TestTidy(t *testing.T) {
 		require.Equal(t, "0", bundle.Rules[0].ID)
 		require.Equal(t, "1", bundle.Rules[1].ID)
 		require.Equal(t, 9, bundle.Rules[1].Count)
-		require.Equal(t, []pd.LabelConstraint{
-			{
-				Op:     pd.NotIn,
-				Key:    EngineLabelKey,
-				Values: []string{EngineLabelTiFlash},
-			},
-		}, bundle.Rules[1].LabelConstraints)
+		require.Equal(t, 0, len(bundle.Rules[1].LabelConstraints))
 		require.Equal(t, []string{"zone", "host"}, bundle.Rules[1].LocationLabels)
 	}
 	err = bundle.Tidy()
@@ -1009,13 +1000,6 @@ func TestTidy(t *testing.T) {
 	err = bundle2.Tidy()
 	require.NoError(t, err)
 	require.Equal(t, bundle, bundle2)
-
-	bundle.Rules[1].LabelConstraints = append(bundle.Rules[1].LabelConstraints, pd.LabelConstraint{
-		Op:     pd.In,
-		Key:    EngineLabelKey,
-		Values: []string{EngineLabelTiFlash},
-	})
-	require.ErrorIs(t, bundle.Tidy(), ErrConflictingConstraints)
 }
 
 func TestTidy2(t *testing.T) {
@@ -1468,12 +1452,6 @@ func TestTidy2(t *testing.T) {
 
 			for i, rule := range tt.bundle.Rules {
 				expectedRule := tt.expected.Rules[i]
-				// Tiflash is always excluded from the constraints.
-				AddConstraint(&expectedRule.LabelConstraints, pd.LabelConstraint{
-					Op:     pd.NotIn,
-					Key:    EngineLabelKey,
-					Values: []string{EngineLabelTiFlash},
-				})
 				if !reflect.DeepEqual(rule, expectedRule) {
 					t.Errorf("unexpected rule at index %d:\nactual=%#v,\nexpected=%#v\n", i, rule, expectedRule)
 				}

pkg/ddl/placement/constraint.go

@@ -54,6 +54,7 @@ func NewConstraint(label string) (pd.LabelConstraint, error) {
 		return r, fmt.Errorf("%w: %s", ErrInvalidConstraintFormat, label)
 	}
 
+	// Does not allow adding rule of tiflash.
 	if op == pd.In && key == EngineLabelKey && strings.ToLower(val) == EngineLabelTiFlash {
 		return r, fmt.Errorf("%w: %s", ErrUnsupportedConstraint, label)
 	}

pkg/ddl/placement/constraint_test.go

@@ -64,6 +64,15 @@ func TestNewConstraint(t *testing.T) {
 				Values: []string{"tiflash"},
 			},
 		},
+		{
+			name:  "not tiflash_compute",
+			input: "-engine  =  tiflash_compute  ",
+			label: pd.LabelConstraint{
+				Key:    "engine",
+				Op:     pd.NotIn,
+				Values: []string{"tiflash_compute"},
+			},
+		},
 		{
 			name:  "disallow tiflash",
 			input: "+engine=Tiflash",